(1) internal/registry/doc.go, internal/registry/options.go

All ENV vars start with “STALWARTSYNC” which does not match the name of the service. They would be more intuitively named “STALWART_URL”, “STALWART_USERNAME”, etc.

(2) Architectural design

What is the difference between internal/registry and internal/jmap since both expose a way to construct a jmap.Client? This is a confusing API design because it’s not clear in what scenarios the caller should go through the registry package to get a client and why the registry package owns the upstream URL and secrets. The jmap.Client also takes an endpoint, so that configuration is duplicated in multiple places but the only differences become apparent by reading the doc strings about whether it should include the /jmap path or not. In what scenario does the caller go straight to jmap.New to get a client vs. registry.Options{}.Client()?
The design of the client construction is not idiomatic Go code. It would not be normal to call registry.Options{}.Client() to get a functional client. This implies that the Options struct may be mutated and the client reconstructed every time instead of being created once through a New method or similar. Wouldn’t a more idiomatic API be registry.NewClient(o registry.Options) (*jmap.Client, error)? But still it’s not clear why the registry has it’s own client constructor function for a jmap.Client while jmap also has jmap.New with subtle differences in how the endpoints are constructed. If the intent is to create a JMAP client with all appropriate methods to interact with the API, then registry should be combined with the JMAP package to make a cleaner API surface that surfaces one jmap.Client constructor function that is provided with all required configuration (endpoint, secret files, etc.) Internally it can expose a basic jmap.new() function if necessary to hide some of the implementation details.
Generally, internal packages should not reach out to the environment to configure themselves. If some configuration value is required, it should be an argument to the constructor function and the service is configured from the outside by the caller. The caller reads ENV vars or parses a config value in a single place, then configures all required services with that information. If every client reads its own configuration in different ways, it makes the overall configuration of the service very difficult to track. If a registry.NewClient needs a URL, username, and secret to function, then it declares those as part of it’s NewClient function and the caller is responsible for getting those values from the environment or other configuration location. Internal packages should not have side effects, they should be as pure as possible. It should look like registry.NewClient(endpoint, user, secret). Some other top-level config.go should be getting these from the environment, parsing a config file, and reading the secrets from disk, then failing fast when required config values are absent. This happens in the main() or close to it in the call stack on startup, not deep in the internal packages.

2 Comments

agent dd7435c Aug 25

Task 4a: Merge jmap+registry into `internal/stalwart` — one client, pure constructors

Fixes review feedback in d73c3db + 19e419e. Executed subagent-driven with spec+quality review gates.

Decisions locked with owner

Package name internal/stalwart; constructor NewClient(endpoint, username, secret) (*Client, error) validating non-empty values + parseable endpoint URL
Env assembly (STALWART_URL, STALWART_USERNAME, STALWART_SECRET_FILE) moves out of the library — interim home is the integration-test helper _test.go; permanent home is future cmd/ config near main()
Auth privatized (nil-Authenticator footgun eliminated by construction)

Final API surface

package stalwart // internal/stalwart

type Client struct { Endpoint string; HTTP *http.Client; /* unexported auth */ }
func NewClient(endpoint, username, secret string) (*Client, error)
    // rejects empty username/secret, empty or unparseable endpoint (url.Parse, scheme+host required);
    // builds unexported Basic authenticator internally

// protocol level (generic RFC-8620; behavior unchanged except NewRequest caps)
func NewRequest(extraCapabilities ...string) *Request  // core URN ALWAYS included (19e419e #2); extras dedup'd
type Request / RawResponse / SetError / Error(+HTTPStatus)
func Send[T any](ctx, c, r) (T, error); SendAll; Decode[T]   // stays exported (account.IssuePassword needs raw path w/ custom accountId in Task 5)

// registry level (methods on Client; wire semantics identical to today)
func (c *Client) Get(ctx, objType string, ids, properties []string) ([]json.RawMessage, []string, error)
func (c *Client) Query(ctx, objType string, f Filter, position int) (ids []string, total int, err error)
func (c *Client) QueryEquals(ctx, objType, attribute, value string) ([]string, error)
func (c *Client) Set(ctx, objType string, create map[string]any, update map[string]any, destroy []string) (SetResult, error)
func (c *Client) Destroy(ctx, objType string, ids ...string) (SetResult, error)
func (c *Client) VerifySchema(ctx context.Context) error     // PinnedSchemaHash stays package-level

// types & consts moved wholesale
Filter; SetResult{...} (+CreatedID/Err/*SetErrorFailure);
CapabilityURN, CoreCapability, AccountID="a", MASKED_PASSWORD="****";
SetErrForbidden…SetErrCannotDeleteLinked (incl. camelCase-inference caveats)

File mapping

From	To
internal/jmap/{client,envelope,request,send,errors}.go	internal/stalwart/ same names (client.go gains NewClient; auth field unexported; Authenticator interface → unexported `authenticator`)
internal/jmap/*_test.go	migrate; constructor-dependent tests rewritten for NewClient signature
internal/registry/{consts,result,schema,registry}.go	internal/stalwart/{consts,result,schema,registry}.go — funcs become methods on *Client; ServerBase unexported
internal/registry/options.go + options_test.go	deleted (no replacement in library)
internal/{tenant,domain}/*/.go	import `git.kilimanjaro.io/sovrn/internal/stalwart`; symbol renames registry.X→stalwart.X, jmap.X→stalwart.X; signatures take *stalwart.Client
doc.go ×3 (stalwart/tenant/domain)	rewritten: stalwart = “Stalwart control-plane client: RFC-8620 plumbing + Registry provisioning ops”; keep rejected-libraries note; drop purity claim; record packages say “Built on internal/stalwart”

Bug-item checklist (acceptance)

[ ] d73c3db #2: exactly ONE client constructor in the tree; no Options struct anywhere; no os.Getenv under internal/*/ excluding _test.go
[ ] d73c3db #1: env names STALWART_* wherever env is read (integration-test helper only, at this stage)
[ ] d73c3db idiomatic: construction is a plain positional call; no mutatable Options
[ ] 19e419e #1: nil-authenticator impossible (unexported, always populated)
[ ] 19e419e #2: core capability unconditional; test asserts envelope using contains core even when extras passed + dedup
[ ] 19e419e #3: all four errcheck sites fixed
[ ] Full suite green: gofmt/vet/go test -race ./internal/…

Tasks

R1 implementer → commit refactor(stalwart): merge jmap+registry into single client package R2 spec reviewer (checklist vs actual tree) R3 quality reviewer (merged surface coherence, doc quality, zero-mutation test semantics preserved) R4 completion comments on both bugs; left open for owner verification.

agent df7b3cc Aug 25

Implemented — 2026-08-25 (change xzmvnpzr)

All review points addressed by merging internal/jmap + internal/registry into a single package internal/stalwart (plan: comment dd7435c).

One constructor: stalwart.NewClient(endpoint, username, secret) (*Client, error) — validates endpoint (parse/scheme/host//jmap-suffix) + non-empty username/secret. No Options struct; no Options{}.Client() pattern.
No environment side effects: FromEnv and all os.Getenv reads removed from library code (grep-verified zero outside _test.go). Config assembly belongs near main(); interim env reading (STALWART_URL/STALWART_USERNAME/STALWART_SECRET_FILE) will live in the Task 8 integration-test helper.
Naming: registry ops are now methods on the one Client (Get/Query/QueryEquals/Set/Destroy/VerifySchema); serverBase unexported; error prefixes normalized to stalwart:.
19e419e items folded in: nil-authenticator impossible (auth unexported, always populated); NewRequest always includes core capability (+dedup); all errcheck violations fixed.
Gates: gofmt/go vet/go test -race ./internal/... green; golangci-lint run ./internal/... = 0 issues.

Left open for your verification per protocol.

review: internal/registry package