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

[spxrogers]

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 two 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 per-item menu and its prompt are one itemMenu const printed at both prompt sites. The at-most-one --auto-* check is the constructor's first statement, so the invariant reconcileAuto's doc states is enforced where a session is built, before anything loads.

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, a per-item [w], a confirmed bulk [W] or --auto-writeback, whose destination dropped the server) and its withinDir bound. The secret-invariants docs used to call that gate "keystroke-gated", which --auto-writeback never was; the canonical project memory (.agentsync/memory/AGENTS.md), its renders CLAUDE.md/AGENTS.md (re-rendered with agentsync apply --scope project), SECURITY.md, docs/architecture.md §5 and every in-code comment now name the three routes (eabe6d8, 75d0e1a, 93ff469), and each route is pinned by a test (round 3).

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 rounds 1–4). Seventeen 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, and from the review rounds: 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, the override dedup key (agent and path), the constructor's --auto-* rejection (against an empty temp home), and a source-text guard that the check precedes every load (reading source through the package's readFileForGuard/funcBody guard convention). End-to-end 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), TestReconcile_ProjectScope_OverrideRecordsProjectState (a project-scope [o] records project state and takes no backup), TestReconcile_OrphanFile's remove subtest asserts the removed orphan's state entry is pruned and an in-sync sibling's survives (a wipe of state passed the whole package before), and TestReconcile_DroppedServer_WriteBackRemovesSource pins removeDroppedSource on each of its three routes (per-item [w], confirmed bulk [W], and --auto-writeback, the last pinned by the absence of any prompt with a never-read [q] on stdin): the dropped server's mcp/<id>.toml is unlinked and the drifted sibling written back. That function had no in-repo coverage before.

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 (five rounds, the full budget; every finding from every round fixed). Round 1 (four lenses on 18c4f42; closed by 97682da + 20bf626): the constructor now enforces the at-most-one --auto-* invariant its own doc states; the nil-registry guard resolves nothing rather than passing through; "six loose locals" was seven; six session-level tests pin the arms that were covered only by the uncommitted harness. Round 2 (on 20bf626; closed by eabe6d8 + 58201c5): the docs' "keystroke-gated" claim corrected at its canonical source and every copy (a confirmed bulk [W] was a third route nobody had named); the production itemMenu const with the tests keeping a deliberate copy; the orphan-remove state-prune assertion; the load-order source-text guard and a hermetic constructor test; the dedup pin in its own test. Round 3 (on 58201c5; correctness and API design CLEAN, adversarial and test rigor ship-it; closed by 4f1f492): the three-route test for removeDroppedSource, the exact-prune sibling assertion, prose tightening; the adversarial lens's pre-existing finding, that a confirmed bulk [W] writes back ForeignCollision items --auto-writeback refuses, is filed as #255. Round 4 (on 4f1f492; three lenses ship-it; closed by 75d0e1a): the guards read source through the package's existing readFileForGuard/repoRootFromCaller convention instead of a round-3 helper that had also displaced funcBody's doc; the --auto-writeback route is pinned by the absence of any prompt, with a never-read [q] on stdin so a regressed flag fails instead of blocking on a terminal; test rigor's pre-existing finding, that the per-item diff labels --- source / +++ dest but renders the sides the other way round, is filed as #256. Round 5 (on 75d0e1a; API design, correctness and test rigor CLEAN, adversarial ship-it; closed by 93ff469): three comment-only residuals (a miscount of the old case-fold sites, the last per-item-keystroke framing of the deletion gate, and a doc paragraph that had sat on the wrong function). Byte-identity re-measured after every round that touched production code: 39 scenarios, diff -r empty; round 4's correctness lens independently re-ran 11 dropped-server scenarios against the base binary, and round 5's proved the sorted string-literal sets of base and head differ only by the deduplicated menu. Declined with reason: 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); folding --auto-* into a single mode enum (it would only relocate the same check to the flag boundary; a follow-up candidate); turning the nil-registry guard into a panic (house style is a documented guard, and every production call site is verified non-nil). One reviewer's closing reflection, recorded rather than acted on: the load-order source-text guard is the one test they would cut as mild over-pinning; two other lenses had validated it as complementary to the behavioural test.

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, the round-1 tree and the round-2 tree (rounds 3–5 changed only comments in production), 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 each arm against base: 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.
• Round 2: dropping the orphan prune's stateDirty = true, or the pruneStateFilesForPath call → exactly TestReconcile_OrphanFile/remove_backs_up_and_deletes; moving the --auto-* check below loadProjectedForScope → exactly TestNewReconcileSession_ChecksModesBeforeLoading; a column-0 brace inside the constructor → the guard's truncation check; changing the production menu wording → the four count/suffix tests, each printing the transcript; dedup on path only → exactly TestApplyAction_OverrideDedupsByAgentAndPath.
• Round 3: skipping the errDestDroppedServer branch → all three TestReconcile_DroppedServer_WriteBackRemovesSource subtests; --auto-writeback resolving to skip → exactly its --auto-writeback subtest; not latching the confirmed bulk choice → exactly its confirmed_bulk_[W] subtest (+ TestWalk_ConfirmedBulkSkipsLaterPrompts); replacing the orphan prune with a wipe of state.Files → exactly the orphan remove subtest, on the sibling assertion.
• Round 4: --auto-writeback falling through to the interactive prompt → exactly its subtest (must not prompt); the ordering check moved below the loads → exactly the guard, through the readFileForGuard read path.

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, and each of its three routes is now pinned by a test; no .Canonical() unwrap is added; the redaction map reaches both renderItemValues sites. The invariants' own wording was made accurate (the gate is a chosen write-back, not a keystroke), not weakened.
• 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, incl. the gate-wording note), .agentsync/memory/AGENTS.md + rendered CLAUDE.md/AGENTS.md, SECURITY.md, docs/architecture.md §5. No CLI surface or capability change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG