Skip to content

refactor(cli): reconcileRun becomes a reconcileSession (#232) - #253

Open
spxrogers wants to merge 6 commits into
mainfrom
claude/issue-232-reconcile-session
Open

refactor(cli): reconcileRun becomes a reconcileSession (#232)#253
spxrogers wants to merge 6 commits into
mainfrom
claude/issue-232-reconcile-session

Conversation

@spxrogers

@spxrogers spxrogers commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

Closes #232. Sixth PR in the #226 code-quality series (after #240, #244, #247, #249, #252). A behaviour-preserving restructure of reconcile's interactive pass, plus the two per-item rebuilds the issue asked about. No user-visible change, and that claim is measured rather than argued (below).

1. reconcileRun becomes a reconcileSession (commit 1, 34bbf3c, internal/cli/reconcile.go + docs). The old body was 375 lines with five goto dones, two labeled loops and seven pieces of run-scoped state travelling as loose locals. It is now a 22-line entry point over a reconcileSession struct: the wiring the run was built with (printer, buffered input, registry, loaded state, scope, redaction map, the hoisted canonical hook-event vocabulary) and the run-scoped bookkeeping (queued overrides + dedup map, the confirmed bulk choice, stateDirty, autoSkipped, writeBackFailed, the per-source write ledger). Each phase is a method a test can drive directly: promptOrphan, resolveAuto, promptItem, applyAction, finish; attemptWriteBack, removeDroppedSource and itemSourceFile become methods too. Every goto done is a plain return out of walk, which returns no error, and finish has exactly one call site. It is deliberately not deferred: it writes (the override re-apply, the state save) and returns the run's error, and a defer would run those writes while a panic unwound.

The bulk-action byte ('w'/'o'/'s'/'i'/'q' with ch | 0x20 open-coded at three sites) is a typed reconcileAction whose zero value is invalid (the adapter.SkipKind convention, not adapter.Action's). parseItemKey is now the one place a keystroke becomes an action, and it writes down the asymmetry the old case 'w', 'W', 'o', 'O', 's', 'S', 'i', 'q', 'Q' switch encoded by omission: I is not a bulk ignore and D is not a diff; folding uniformly would have silently added an unconfirmed bulk-ignore keystroke. bulkTargets is the blast-radius count as a function. The at-most-one --auto-* check is the constructor's, so the invariant reconcileAuto's doc states is enforced where a session is built.

The dest→source surface is unchanged. [w]rite-back still runs through writeBackItem → capture.Capture; the deletion-only exception removeDroppedSource keeps its gate (it runs only for a chosen write-back, interactive [w] or --auto-writeback, whose destination dropped the server) and its withinDir bound. The CLAUDE.md sentence naming it stays true as a method.

Why one file. The session stays in reconcile.go rather than a new reconcile_session.go. A split would not move any fenced site (both iox.AtomicWrite calls stay put, and the three os.Remove sites carry line-scoped //nolint), but it would falsify the prose of .golangci.yml's fence entry, which names "reconcile.go's interactive orphan removal". A behaviour-preserving refactor does not touch .golangci.yml, not even its prose.

2. Tests (commit 2, 7eea03b, extended in review round 1). Fifteen session-level unit tests (reconcile_session_internal_test.go) drive the methods on a scripted bufio.Reader: TestParseItemKey (15 rows, including the two non-folded capitals), TestBulkTargets, the four exits (TestPromptItem_EOFStopsThePass, TestPromptItem_EOFMidBulkConfirmStopsThePass whose transcript must end at [y/N] , TestPromptOrphan_EOFStopsThePass, TestPromptOrphan_QuitStopsThePass), TestWalk_QuitLeavesRemainingItemsUnprompted, TestWalk_ConfirmedBulkSkipsLaterPrompts, TestApplyAction_OnlyQuitStopsThePass (which also pins the dedup key as agent and path), and from round 1: the [d]iff arm, the declined bulk confirm (no menu re-print), an unknown key, a bulk choice that must not sweep an orphan, the [i]gnore arm, and the constructor's --auto-* check. Two end-to-end tests in reconcile_test.go: TestReconcile_FinishRunsExactlyOnce (three subtests: the auto-safe summary prints once; EOF after an [o] applies the queue once; [q] after an [o] still applies the queue once, the case the review found unpinned) and TestReconcile_ProjectScope_OverrideRecordsProjectState (a project-scope [o] records project state and takes no backup, pinning the scope/projectRoot finish now reads from the session; every other scenario and test is user-scope).

3. printItemDiff dropped (commit 3, fab41c2): it was a pure alias of renderItemValues; the [d]iff arm calls the real thing.

4. canonicalHookEvent uses the caller's registry (commit 4, 18c4f42). Inverting a hook pointer's native event spelling rebuilt all 31 adapters via registryFactory() on every resolution (~10 µs measured). It now takes the registry the caller already holds: the session's in reconcile, and explainInputs.reg threaded through explain.go / explain_model.go for explain. The comma-ok assertion already meant it could not panic on a non-renaming adapter (the issue's question); a reg == nil guard exists because Registry.Lookup dereferences its receiver, and it resolves nothing (ok false) rather than passing the native spelling through, so a caller that forgot its registry gets "no source" instead of a plausible wrong path. registryFactory() call sites in the file: 2 → 1.

Review provenance. The plan and execution spec were reviewed by a fresh reviewer who replicated all four commits and the fixture from the artifacts and found five issues before execution: quit-with-a-queued-override was unpinned (added as two scenarios and a subtest), finish's hoisted scope/projectRoot was unpinned (added as a project-scope scenario and test), the CHANGELOG compared against the wrong binary, the one-file reason was wrong (corrected above), and commit 4's registry could be nil from two test literals (guard + row). All fixed before execution.

Review loop. Round 1 (four lenses on 18c4f42; every finding accepted; closed by 97682da and a doc precision in 20bf626): the constructor now enforces the at-most-one --auto-* invariant its own doc states (it lived in the caller); the nil-registry guard resolves nothing rather than passing through; "six loose locals" was seven; removeDroppedSource's gate is a chosen write-back, not a keystroke; the session test helper names the two arms that would panic on a zero session; six session-level tests pin the arms that were covered only by the uncommitted harness. Byte-identity re-measured after the round against the base binary: 39 scenarios, diff -r empty. Declined: pinning the orphan [r] backup/remove-failure arms and finish's two error returns (each needs a failing backup dir or a failing adapter Apply; out of proportion for a refactor PR).

Type of change

  • Bug fix
  • New feature / enhancement
  • Refactor (no behavior change)
  • Docs
  • Tests / CI / tooling

Test plan

Byte-identity oracle. Two binaries, main at 309fde0 and each of commit 1, the round-0 head and the round-1 tree, were run through 39 scripted-stdin scenarios covering every prompt, bulk confirmation (confirmed and cancelled), EOF at both prompts and mid-confirm, [q]uit at both prompts (including a quit with a queued override), every --auto-* mode and the mutually-exclusive error, a project-scope override, the write-back conflict and dropped-server paths, exit codes, and a masked secret value. Per scenario the harness captures stdout, stderr, exit status and the normalized source/state tree (195 files per arm). Determinism was proved first (base-vs-base diff -r empty), then base-vs-commit-1, base-vs-head and base-vs-round-1: all empty. Run from a working directory with no .agentsync/ ancestor.

Break-verifies (literal --- FAIL sets, each mutation asserted to land exactly once):

  • On the commit-1 tree with only the pre-existing suite, 9 of 11 control-flow mutations (EOF/quit exits returning "continue", bulkTargets → len(rest), folding I/D, dropping the bulk latch, dropping the stateDirty reset, calling finish twice) failed zero tests. That is the gap commit 2 closes.
  • On the commit-2 tree each of those fails exactly its named test: BV-1 → TestPromptItem_EOFStopsThePass; BV-2 → TestPromptItem_EOFMidBulkConfirmStopsThePass; BV-3 → TestApplyAction_OnlyQuitStopsThePass + TestWalk_QuitLeavesRemainingItemsUnprompted; BV-4 → TestPromptOrphan_EOFStopsThePass; BV-5 → three TestBulkTargets rows; BV-6 → the two TestParseItemKey capital rows; BV-7 → TestWalk_ConfirmedBulkSkipsLaterPrompts; BV-11 (finish twice) → TestReconcile_FinishRunsExactlyOnce and all three subtests. Discarding the queue on [q] → exactly the quit_after_override subtest. Hard-coding user scope in finish → exactly TestReconcile_ProjectScope_OverrideRecordsProjectState.
  • BV-10 (dropping s.stateDirty = false after the override save) fails 0 tests on both trees and the fixture is identical: it is an equivalent mutation (a second byte-identical state.Save). Reported, no test added.
  • On the commit-4 tree: a fresh empty registry → the three renaming TestCanonicalHookEvent rows and TestExplainPath_RenamedHookEventResolves; a single-value type assertion → the claude_does_not_rename row with the missing method NativeHookEvent panic the issue asked about; removing the nil guard → the nil_registry row with a nil-pointer dereference inside Registry.Lookup.
  • Round 1: moving the --auto-* check back to the caller → TestNewReconcileSession_RejectsMultipleAutoModes + TestReconcile_AutoFlagsMutuallyExclusive; nil guard passing through → the nil_registry_resolves_nothing row; deleting the [d]iff re-render, re-printing the menu after a declined confirm, echoing an unknown key, letting a bulk choice sweep an orphan, dropping the appendIgnore call, keying the dedup on path only → each exactly its new test.

Accepted fixture coverage holes: no #247 refused-symlink/shape item (covered by dest_fifo_e2e_unix_test.go); no Q/Y capitals; no --auto-override with an orphan; no interactive w on a ForeignCollision.

Post-conditions on internal/cli/reconcile.go: goto 5 → 0; labels 3 → 0; reconcileRun 375 → 22 lines; registryFactory() 2 → 1; capture.Capture( 1 → 1; source.Write*( 1 → 1; iox.AtomicWrite( 2 → 2; os.Remove( 3 → 3; //nolint: 4 → 4; .Canonical() in non-test internal/cli 0 → 0; .golangci.yml absent from the diff.

Gates, each commit independently and again on the head: go build, go vet, gofmt -l empty, gofumpt + go mod tidy no rewrites, AGENTSYNC_TEST_IN_CONTAINER=1 go test ./... (29 packages ok), go test -race ./internal/cli/..., go test -tags=e2e ./test/e2e/..., go test -tags=bdd ./test/bdd/..., GOTOOLCHAIN=go1.26.2 golangci-lint@v2.12.2 run ./... → 0 issues.

  • just test-release is green (the release bar) — just is not installable in this session; every layer the recipe orchestrates (vet → build → race → e2e → bdd) was run directly as listed above, and CI's hermetic test-release job runs on the PR.
  • just lint is clean — run directly with the pinned toolchain; go.mod/go.sum untouched.

Checklist

  • Conventional commit messages with a scope (e.g. fix(secrets): …).
  • Tests added/updated for the behavior changed.
  • If this touches internal/secrets, internal/capture, or any
    source.Write* path, I've re-read the secret-handling invariants in
    CLAUDE.md / SECURITY.md and not weakened them. — Neither package is touched. reconcile.go's one capture.Capture call and one source.Write* call are unmoved (counts above); removeDroppedSource keeps its gate and withinDir bound as a method; no .Canonical() unwrap is added; the redaction map reaches both renderItemValues sites.
  • Docs updated if behavior, CLI surface, or capability coverage changed. — docs/components.md (a reconcileSession entry in the internal/cli key list), CHANGELOG.md (Internal bullet under Changed). No CLI surface or capability change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG

@claude
refactor(cli): reconcileRun becomes a reconcileSession (#232)
34bbf3c 
reconcileRun was 375 lines (internal/cli/reconcile.go:101-475) with five
`goto done`s (three EOF exits, two [q]uits), two labeled loops and six
pieces of run-scoped state travelling as loose locals. It is now a 28-line
entry point over a reconcileSession whose methods are the two prompts
(promptOrphan, promptItem), the --auto-* dispatch (resolveAuto), the action
switch (applyAction) and the run's tail (finish). Every goto is a plain
return out of walk; finish has exactly one call site and is deliberately
not deferred, because it writes (the override re-apply, the state save) and
returns the run's error.

The bulk-action byte + `ch | 0x20` folding becomes a typed reconcileAction
(zero value invalid, the adapter.SkipKind convention) and one parseItemKey
table, which writes down the two keystrokes the prompt deliberately does
NOT fold: 'I' is not a bulk ignore and 'D' is not a diff. Uniform folding
would add a bulk-ignore with no confirmation step (#155).

attemptWriteBack, removeDroppedSource and itemSourceFile become methods;
canonicalHookEvents(c) is computed once per run instead of per [w]
keystroke. No dest→source write moves: capture.Capture 1→1, source.Write*
1→1, iox.AtomicWrite 2→2, os.Remove 3→3, //nolint 4→4; removeDroppedSource
keeps its keystroke gate and withinDir bound; the redaction map is read at
the same two renderItemValues sites. .golangci.yml is untouched.

Behaviour preservation: 39 scripted-stdin scenarios (every prompt, bulk
confirmation, EOF, quit — including quit with a queued override — auto
mode, project-scope override, exit code, masked secret and the resulting
tree) are byte-identical between the 309fde0 binary and this one, after a
base-vs-base run proved the harness deterministic (195 files).

Zero test files are edited. Measured against this suite, nine control-flow
mutations of the new methods fail no test (item-prompt EOF not stopping,
EOF mid bulk-confirm not stopping, quit not stopping, orphan-prompt EOF not
stopping, bulkTargets = len(rest), uniform key folding, a confirmed bulk
choice not recorded, stateDirty not reset after the override save, finish
called twice); the next commit pins them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
@claude
test(cli): pin the reconcile session's exits, bulk actions and finish (…
7eea03b 
…#232)

Before the session type, the interactive loop was reachable only by running
the whole 375-line reconcileRun against a real tree, so the paths that decide
when a pass STOPS were pinned by nothing. Measured on the commit-1 tree with
the pre-existing suite, these mutations failed zero tests: item-prompt EOF
not stopping, EOF mid bulk-confirm not stopping, [q]uit not stopping,
orphan-prompt EOF not stopping, bulkTargets = len(rest), uniform key folding
(I = bulk ignore, D = diff), a confirmed bulk choice not recorded, stateDirty
not reset after the override save, finish called twice.

reconcile_session_internal_test.go drives the methods directly over
production's own bufio.Reader (not a fake, so readChar sees the real EOF):
parseItemKey's table including the two deliberate non-foldings, bulkTargets
over mixed queues, the three EOF exits, both quits, a walk that stops after
[q] with items still queued, a confirmed bulk [S]kip that prompts once, and
applyAction's contract (only quit stops; [o] queues one op per agent+path,
deduplicated on a second [o]).

reconcile_test.go adds two end-to-end pins. TestReconcile_FinishRunsExactlyOnce
is the behavioural half of "finish has one call site and is never deferred":
a finish that ran twice prints its summary line twice. Its third subtest,
[o] then [q], also pins that quitting still applies the queued override —
neither quit was covered by any test or scripted scenario before.
TestReconcile_ProjectScope_OverrideRecordsProjectState pins the scope and
project root finish hands to render.NewWriter/RecordOpsState: at user scope
a transposition to ScopeUser/"" is invisible; at project scope it backs the
project's own .mcp.json up as a foreign collision.

Break-verified: each mutation above fails exactly the test written for it.
The stateDirty reset is the one exception — dropping it causes a second,
byte-identical state.Save through the same atomic write, which no observable
distinguishes, so no test is written for it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
@claude
refactor(cli): drop the printItemDiff alias (#232)
fab41c2 
printItemDiff's body was exactly renderItemValues(w, p, it, redact, canMask)
— one caller (the [d]iff arm of the item prompt), no test callers, no doc
mentions. The [d] arm now calls renderItemValues directly, the same call the
prompt's initial render already makes two lines above it. The `unused`
linter is not enabled, so this is a deliberate cleanup rather than a lint
fix; no behaviour changes and the fixture stays byte-identical.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
@claude
refactor(cli): canonicalHookEvent uses the session's registry (#232)
18c4f42 
canonicalHookEvent built its own adapter registry — registryFactory(), all
31 adapters, ~10µs — every time a hook pointer's native event spelling was
inverted (reconcile's write-back through itemSourceFile → pointerSourceFile,
and explain's componentFromPointer / pointerSource). The caller already
holds the registry the plan was rendered with, so it is threaded through:
pointerSourceFile and canonicalHookEvent take a leading *adapter.Registry,
explainInputs gains a reg field that explain.go fills from its own
registry, and the session passes s.reg.

The answer cannot change: registryFactory() is a package var only tests
reassign, never mid-run, so the session's registry has exactly the contents
the per-call rebuild had. What was implicit is now written down and
pinned: a non-renaming adapter (claude, codex) and an UNREGISTERED agent
take the same passthrough, because Registry.Lookup yields a nil
adapter.Adapter and a comma-ok assertion on a nil interface is (nil, false)
rather than a panic. A nil *Registry — which Lookup would dereference — is
guarded explicitly and takes that same passthrough; the two test literals
that build an explainInputs without one now set it as well.

TestCanonicalHookEvent pins the passthrough rows, the renaming rows
(gemini BeforeTool / cursor preToolUse → PreToolUse), the unknown native
spelling, and the nil registry; TestCanonicalHookEvent_UnregisteredLookupIsNil
pins the Lookup contract the passthrough rests on. registryFactory() call
sites in reconcile.go: 2 → 1. The 39-scenario fixture is still byte-identical
to the 309fde0 binary.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
@claude
refactor(cli): close review-loop round 1 on #253 (#232)
97682da 
Round 1 (four lenses on 18c4f42) — every finding accepted after audit:

- newReconcileSession enforces reconcileAuto's "at most one mode" as its
  first statement, before anything is loaded; reconcileRun no longer checks
  it in the caller. Same error text, same timing (harness s27 identical).
- canonicalHookEvent's nil-registry guard resolves nothing (ok=false)
  instead of passing the native segment through: a caller that forgot its
  registry gets "no source", not a plausible wrong path.
- Doc corrections: seven loose locals, not six (reconcile.go, CHANGELOG,
  components.md); removeDroppedSource's gate is a chosen write-back —
  interactive [w] or --auto-writeback — not a keystroke; the session test
  helper names the two arms that would panic on the zero session; the
  entry point is 22 lines now.
- Tests: TestNewReconcileSession_RejectsMultipleAutoModes; the [d]iff arm,
  the declined bulk confirm (no menu re-print), an unknown key (capital
  I), a bulk choice that must not sweep an orphan, and the [i]gnore arm
  are pinned at the session level; the override dedup key is pinned as
  agent AND path; the nil-registry row flips to "resolves nothing".

Byte-identity re-measured: all 39 harness scenarios, base (309fde0) vs
this tree, diff -r empty over 195 files. Eight mutations each fail
exactly their target test.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
@claude
docs(cli): scope reconcileAuto's invariant to constructor-built sessi…
20bf626 
…ons (#232)

"No session with two modes can exist" overclaimed: a struct literal — which
the session tests build — bypasses newReconcileSession's check. Say exactly
what the constructor guarantees.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Restructure reconcileRun into a session type

2 participants

@spxrogers @claude