Repository navigation
refactor(adapter): give the steps the client adapters repeat one owner - #244
Merged
Merged
Conversation
A fresh-context review found copies across the client adapters: the Converge preamble, a guarded multi-file write with reverse rollback, JSON encoders, receipt slices and the state-file suffix. It also found Codex forwarders that only tests call, and surface metadata that never varies. This Change gives each repeated step one owner in `internal/adapter` or `internal/platform` without changing released behavior. Test fixtures copied between adapter packages stay, because the repository keeps test readers package-local. Change adapter-simplification.
Claude Code, Claude Desktop, Codex and Hermes each began Converge with the same steps: resolve the client's Route and stop on an unselected binding, require an enabled Client Binding, resolve the executable, and check that AIGW's credential store holds the selected Account's Token. That sequence now lives once in adapter.ConvergeBinding, and each adapter keeps only its targets rule: Claude Code's bound targets, Claude Desktop's one configuration library, Codex's converged homes and Hermes' discovered home. Hermes still requires a runnable executable before it reads the secret store, through ConvergeBinding's requireRunnable; the other three still read the store before they require a non-empty executable. Every error keeps its text. Codex now derives its targets after the credential check instead of before it; that derivation only reads discovery results and paths, so the outcome is the same. ResolveExecutable had no caller outside these four preambles and becomes the unexported resolveExecutable. go build passes, go vet passes for darwin, linux and windows, and gofmt and golangci-lint report nothing for internal/adapter/... Change adapter-simplification.
…seam The client adapters each carried a copy of a helper that the adapter seam can own: - Claude Code (settings and state), Claude Desktop, Codex and Hermes each encoded their JSON documents with json.MarshalIndent and a final newline. adapter.EncodeJSON does that once. - Claude Code's encodeRaw and Claude Desktop's raw both encoded one value with json.Marshal. adapter.RawJSON replaces them. - Claude Desktop's and Hermes' receipt slices rolled back in reverse order like the registry's own. The registry's slice is exported as adapter.ProjectionReceipts and serves all three. - Five spellings of ".aigw-state.json" become adapter.StateSuffix. Every copy used the same options: no prefix, a two-space indent, the standard HTML escaping and one final newline, so the written bytes stay the same. Claude Code and Codex ignored an encoding error and Claude Desktop and Hermes returned it; they still do, and the values they encode always encode. A receipt rollback still reaches every receipt in reverse order; a joined failure keeps its text, and only its nesting differs. Claude Desktop's test that its encoder refuses an unencodable value moves to the encoder's owner, which now also pins the document bytes. go test passes for internal/adapter/... and internal/clients/..., go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin and windows) report nothing. Change adapter-simplification.
Codex kept a lattice of forwarders around its reconciliation. PlanReconciliation, ReconcileConfigs and their Transition and AuthorizedTransition variants only bound previous to runtime or set replaceRootSelections, and SyncConfig, DisableConfig, codexUserConfig, removeCodexProjection, prepareCodexReconciliation and prepareCodexRestore had no production caller on darwin, linux or windows. They go. The adapter now passes deps.AuthorizedSelectionClient == ID straight to planCodexReconciliationTransition and to ReconcileConfigs, which is the former reconcileCodexConfigsTransition under the exported name, and verification's isolated projection passes true. It stays exported because tests outside the package call it: SyncConfig had 66 such uses (51 in the codex_test files and 15 in internal/app/synchronization, internal/cli/acceptance, internal/cli/check, internal/cli/use, internal/terminal and tools/release) and DisableConfig had 12. HomeTarget names the default Codex Home target that the isolated projection and those tests build, and replaces codexHomeTargets. Every test now calls these functions with the previous runtime and selection flag that the forwarders bound. restoreModelSelection's two identical branches fold into one call, and managedBlockHashMatches is inlined as a hash comparison. Plans, writes, rollback and error text are unchanged. go test passes for internal/adapter/..., internal/clients/..., internal/app/synchronization, internal/cli/acceptance, internal/cli/check, internal/cli/use, internal/terminal and tools/release. go vet passes for darwin, linux and windows with and without the acceptance build tags, and gofmt and golangci-lint (darwin and windows) report nothing. Change adapter-simplification.
Discovery surfaces and Codex targets carried metadata that only ever held one value, or none: - surface.Authority and AuthorityAIGW: every adapter's surfaces declared "aigw", and the only reader copied it into a Codex TargetRef. - TargetRef.Authority and TargetRef.ProjectionMode: every constructor set "aigw" and full-selection, so validateDesiredCodexTarget reduces to an IsHome check. - discovery.Surface.Product: written by three adapters and never read. - discovery.Surface.ManualFallbackAllowed: never set or read. - discovery.Surface's json tags: nothing encodes a Surface or a discovery.Result, and golangci-lint's musttag finds no encoding of the untagged struct. They go, and so does surface_test.go, which tested only AuthorityAIGW. The Codex sidecar keeps its persisted projection_mode, and its validation is unchanged. A refused desired target keeps the text that released versions print, since every production target carried those values: "Codex target <id> cannot use authority aigw with projection mode full-selection"; a test now pins it. normalizeCodexTargets now names only surface_id and path as required. No production target can lack either: targetRefs and HomeTarget always set the surface, and targetRefs refuses an empty path first, so the released program cannot reach the changed text. reconcile_test.go's codexHomeTarget duplicated HomeTarget and goes. go test passes for internal/adapter/..., internal/clients/..., internal/app/synchronization, internal/cli/acceptance, internal/cli/client, internal/cli/setup and internal/cli/use. go vet passes for darwin, linux and windows with and without the acceptance build tags, and gofmt, golangci-lint (darwin and windows) and the architecture gate report nothing. Change adapter-simplification.
The implementation found that the guarded multi-file writes of Claude Code, Claude Desktop and Codex differ. Claude Code restores its settings before its state and attempts both, while Claude Desktop and Codex restore in reverse write order. Their errors are also wrapped differently. One helper would change released behavior, so each adapter keeps its own sequence. The proposal drops the shared write, the design records why, and task 1.2 records the comparison. The spec scenario no longer names the guarded write as an example. Tasks 1.1 and 1.3 to 1.5 are ticked for 5672318, eab14d7, a1eeb11 and 6bc85a3. The design names the two remaining trade-offs: `ReconcileConfigs` and `HomeTarget` are exported because tests in other packages call the function production calls, and one refusal keeps its released text. Change adapter-simplification.
…verge A fresh-context review of the shared Converge preamble found that no test pinned the `requireRunnable` choice of Hermes and Claude Desktop. Flipping either value still passed every package; only Codex's flip failed, in `TestDesiredClientConfigurationSurfacesCredentialObservationFailures`. The test now also covers both. Claude Desktop must surface a failing credential store, as Claude Code and Codex do. Hermes, which checks its executable first, must neither read the store nor activate without a runnable executable. Flipping Hermes's value through a test overlay now fails at the Hermes assertion, and flipping Claude Desktop's fails at its store assertion; the head passes. Change adapter-simplification.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The client adapters repeated code that the adapter seam can own. This lane (Change
adapter-simplification) gives each repeated step one owner ininternal/adapter. The released programs behave the same, and non-test Go code shrinks by 175 lines.adapter.ConvergeBindingholds the Converge preamble that Claude Code, Claude Desktop, Codex and Hermes each wrote: resolve the Route, require an enabled Client Binding, resolve the executable and check the credential store. Each adapter keeps only its targets rule. Hermes still requires a runnable executable before it reads the secret store (requireRunnable), and every error keeps its text.adapter.EncodeJSON,adapter.RawJSON,adapter.ProjectionReceiptsandadapter.StateSuffixreplace per-adapter copies. Every copy used the same encoder options, so the written bytes stay the same.ReconcileConfigs, which stays exported because tests in other packages call it, with aHomeTargetconstructor.projection_mode.The guarded multi-file writes of Claude Code, Claude Desktop and Codex stay with their adapters. Claude Code restores its settings before its state and attempts both, while the other two restore in reverse write order, and their errors are wrapped differently. A shared helper would need an order switch and per-adapter error formatting, so
design.mdrecords the comparison instead.Two texts can differ, and neither is reachable from the released program.
normalizeCodexTargetsnow names onlysurface_idandpathas required, and a joined rollback failure keeps its text with different nesting. A refused desired Codex target keeps the released text, which names an authority and a projection mode the code no longer models; a test pins it.Review guide
The order inside
ConvergeBindingis the one place where a mistake changes behavior.TestDesiredClientConfigurationSurfacesCredentialObservationFailurespins it per adapter. Claude Code, Codex and Claude Desktop must surface a failing credential store, and Hermes must neither read the store nor activate without a runnable executable.Validation
mise run checkpasses at eb38900 (statement coverage 95.18%, above the 95% floor), and so doesmise run dependencies:check.go vetpasses for darwin, linux and windows, with and without the acceptance build tags, and golangci-lint reports nothing on darwin and windows.requireRunnablefor Hermes and Claude Desktop. eb38900 adds that test: flipping either value through a test overlay fails it, and the head passes.