diff --git a/.agentsync/memory/AGENTS.md b/.agentsync/memory/AGENTS.md index 63d1c569..cffad683 100644 --- a/.agentsync/memory/AGENTS.md +++ b/.agentsync/memory/AGENTS.md @@ -225,14 +225,15 @@ Capture **refuses the whole write** rather than persist cleartext. It errs towar refusing; the user updates the vault or edits the canonical source directly. Two narrow **deletion-only** exceptions sit outside the funnel: `reconcile`'s `removeDroppedSource` (`internal/cli/reconcile.go`) unlinks a canonical -`mcp/.toml` when the user explicitly chooses `[w]rite-back` for a -destination-side server deletion, and `source.RemoveHooks` deletes a stale -`hooks/.toml` during import's stale-hook retirement. Both are safe -without Capture because a pure deletion carries no content to re-reference — -there is no secret material to persist — and both are guarded (the reconcile -path is keystroke-gated and `withinDir`-bounded to `~/.agentsync`; RemoveHooks -validates the native-supplied event id before touching a path). Anything that -*writes content* dest→source still MUST go through `capture.Capture`. +`mcp/.toml` when the user chooses write-back for a destination-side server +deletion — a per-item `[w]`, a confirmed bulk `[W]`, or `--auto-writeback` — +and `source.RemoveHooks` deletes a stale `hooks/.toml` during import's +stale-hook retirement. Both are safe without Capture because a pure deletion +carries no content to re-reference — there is no secret material to persist — +and both are guarded (the reconcile path is `withinDir`-bounded to +`~/.agentsync`; RemoveHooks validates the native-supplied event id before +touching a path). Anything that *writes content* dest→source still MUST go +through `capture.Capture`. **3. Resolved vs templated types.** `secrets.SubstituteCanonical` returns `secrets.Resolved` (a wrapper, NOT assignable to `source.Canonical`); it is the diff --git a/AGENTS.md b/AGENTS.md index 7e0d6be3..c5c91619 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -232,14 +232,15 @@ Capture **refuses the whole write** rather than persist cleartext. It errs towar refusing; the user updates the vault or edits the canonical source directly. Two narrow **deletion-only** exceptions sit outside the funnel: `reconcile`'s `removeDroppedSource` (`internal/cli/reconcile.go`) unlinks a canonical -`mcp/.toml` when the user explicitly chooses `[w]rite-back` for a -destination-side server deletion, and `source.RemoveHooks` deletes a stale -`hooks/.toml` during import's stale-hook retirement. Both are safe -without Capture because a pure deletion carries no content to re-reference — -there is no secret material to persist — and both are guarded (the reconcile -path is keystroke-gated and `withinDir`-bounded to `~/.agentsync`; RemoveHooks -validates the native-supplied event id before touching a path). Anything that -*writes content* dest→source still MUST go through `capture.Capture`. +`mcp/.toml` when the user chooses write-back for a destination-side server +deletion — a per-item `[w]`, a confirmed bulk `[W]`, or `--auto-writeback` — +and `source.RemoveHooks` deletes a stale `hooks/.toml` during import's +stale-hook retirement. Both are safe without Capture because a pure deletion +carries no content to re-reference — there is no secret material to persist — +and both are guarded (the reconcile path is `withinDir`-bounded to +`~/.agentsync`; RemoveHooks validates the native-supplied event id before +touching a path). Anything that *writes content* dest→source still MUST go +through `capture.Capture`. **3. Resolved vs templated types.** `secrets.SubstituteCanonical` returns `secrets.Resolved` (a wrapper, NOT assignable to `source.Canonical`); it is the diff --git a/CHANGELOG.md b/CHANGELOG.md index d89f6471..8dfd59b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -289,6 +289,29 @@ source layout, CLI surface, and state schema are stabilizing but may still chang `Adapter` interface and is deferred to [#250](https://github.com/spxrogers/agentsync/issues/250). +- **Internal: `reconcile`'s interactive pass is a session type** + ([#232](https://github.com/spxrogers/agentsync/issues/232)). `reconcileRun` + was 375 lines with five `goto done`s, 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` whose methods are the two prompts, the `--auto-*` + dispatch, the action switch and the run's tail. Every `goto` is a plain + return, and the tail runs from exactly one call site — deliberately not a + `defer`, since it re-applies the queued `[o]verride` ops and saves state. The + bulk-action state machine is a typed enum instead of a `byte` with `ch | 0x20` + case folding, which also writes down the two keystrokes the prompt + deliberately does **not** fold (`I` is not a bulk ignore; `D` is not a + diff). No user-visible behaviour changes: 39 scripted-stdin scenarios — + every prompt, bulk confirmation, EOF, `[q]uit` (including a quit with a + queued override), `--auto-*` mode, project-scope override, exit code, masked + secret value and resulting source/state tree — are byte-identical to the + pre-change binary (`main` at `309fde0`). Two smaller things went with it: + `printItemDiff` was a pure alias of `renderItemValues`, and inverting a hook + pointer's native event spelling now uses the registry the caller already + holds instead of rebuilding all 31 adapters (~10 µs) on every resolution. + The docs no longer call `removeDroppedSource`'s gate "keystroke-gated" + (`--auto-writeback` never was one): SECURITY.md, architecture §5 and the + project memory name the three write-back routes that reach it. + - **`.state/targets.json` is now `schema_version: 2`.** The upgrade is automatic and requires nothing: every command reads the old keys, and the first command that WRITES state (`apply`, `import`, `reconcile`, `migrate`, `agent disable diff --git a/CLAUDE.md b/CLAUDE.md index 6a55e3e7..d98af0d3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -232,14 +232,15 @@ Capture **refuses the whole write** rather than persist cleartext. It errs towar refusing; the user updates the vault or edits the canonical source directly. Two narrow **deletion-only** exceptions sit outside the funnel: `reconcile`'s `removeDroppedSource` (`internal/cli/reconcile.go`) unlinks a canonical -`mcp/.toml` when the user explicitly chooses `[w]rite-back` for a -destination-side server deletion, and `source.RemoveHooks` deletes a stale -`hooks/.toml` during import's stale-hook retirement. Both are safe -without Capture because a pure deletion carries no content to re-reference — -there is no secret material to persist — and both are guarded (the reconcile -path is keystroke-gated and `withinDir`-bounded to `~/.agentsync`; RemoveHooks -validates the native-supplied event id before touching a path). Anything that -*writes content* dest→source still MUST go through `capture.Capture`. +`mcp/.toml` when the user chooses write-back for a destination-side server +deletion — a per-item `[w]`, a confirmed bulk `[W]`, or `--auto-writeback` — +and `source.RemoveHooks` deletes a stale `hooks/.toml` during import's +stale-hook retirement. Both are safe without Capture because a pure deletion +carries no content to re-reference — there is no secret material to persist — +and both are guarded (the reconcile path is `withinDir`-bounded to +`~/.agentsync`; RemoveHooks validates the native-supplied event id before +touching a path). Anything that *writes content* dest→source still MUST go +through `capture.Capture`. **3. Resolved vs templated types.** `secrets.SubstituteCanonical` returns `secrets.Resolved` (a wrapper, NOT assignable to `source.Canonical`); it is the diff --git a/SECURITY.md b/SECURITY.md index 80144f0e..d53f77e0 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -29,10 +29,11 @@ can resolve secrets into native config files. Areas of particular interest: cannot verify the source's `${secret:…}` refs (the leak check would be blind), so a locked vault never silently degrades into persisting a credential. The only write-backs outside this path are two guarded pure *deletions* of - canonical files (reconcile's dest-dropped MCP-server removal, keystroke-gated - and path-bounded to `~/.agentsync`; import's stale-hook retirement) — a - deletion carries no secret content to persist, and anything that writes - content back still goes through `capture.Capture`. + canonical files (reconcile's dest-dropped MCP-server removal, which runs only + for a chosen write-back — per-item `[w]`, confirmed bulk `[W]`, or + `--auto-writeback` — and is path-bounded to `~/.agentsync`; import's + stale-hook retirement) — a deletion carries no secret content to persist, + and anything that writes content back still goes through `capture.Capture`. The alternative `backend = "env"` stores nothing: `${secret:…}` resolves from the process environment at apply time, so there is no vault, no identity file and no decryption — the credential's protection is whatever protects the diff --git a/docs/architecture.md b/docs/architecture.md index 2490ffbc..63bad97b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -716,7 +716,8 @@ to `int64`/`float64` before writing. No other code path writes destination data back into the source. (Two guarded code paths *delete* canonical files without going through Capture: `reconcile`'s `removeDroppedSource` unlinks `mcp/.toml` when the user writes back a -destination-side server deletion — keystroke-gated, `withinDir`-bounded to +destination-side server deletion — gated on that chosen write-back (a per-item +`[w]`, a confirmed bulk `[W]`, or `--auto-writeback`), `withinDir`-bounded to `~/.agentsync` — and import's stale-hook retirement calls `source.RemoveHooks` on `hooks/.toml`. A pure deletion carries no content to re-reference, so the funnel's secret guarantees are not in play; anything that writes *content* diff --git a/docs/components.md b/docs/components.md index c02fe4bf..4e793110 100644 --- a/docs/components.md +++ b/docs/components.md @@ -54,7 +54,13 @@ is the only package that depends on nearly all the others. behind `status`, `diff`, `reconcile` and `explain` (`planwalk.go`); its `planItem` is deliberately unexported field-for-field so it can never become a `--json` surface, because a plan built from `secrets.SubstituteCanonical` - carries resolved cleartext in `op.Content`; `destReadPath` / `readDestText` + carries resolved cleartext in `op.Content`; `reconcileSession` — one + `agentsync reconcile` pass (printer, scripted input, registry, loaded state, + redaction map) plus its run-scoped bookkeeping, so the two prompts, the + `--auto-*` dispatch, the action switch and the run's tail (`finish`) are + separately testable methods rather than five `goto`s over seven loose locals + (#232); `finish` has exactly one call site and is never deferred, because it + writes and it returns the run's error; `destReadPath` / `readDestText` (`destread.go`) — the whole-file destination readers that carry the symlink policy (`AGENTSYNC_ALLOW_SYMLINK_DEST`), mirroring `iox.AtomicWrite`'s. - **Commands:** `init`, `agent {add,remove,list,enable,disable}`, `apply`, diff --git a/internal/cli/apply_state_order_internal_test.go b/internal/cli/apply_state_order_internal_test.go index 69c14f5c..57e3a1f9 100644 --- a/internal/cli/apply_state_order_internal_test.go +++ b/internal/cli/apply_state_order_internal_test.go @@ -1,9 +1,6 @@ package cli import ( - "os" - "path/filepath" - "runtime" "strings" "testing" ) @@ -39,20 +36,11 @@ import ( // If this ever gains a genuine observable (a component the re-apply does not // re-record), replace half A with a test of that. func TestApplyPipelineLoadsStateAfterSourceReload(t *testing.T) { - _, thisFile, _, ok := runtime.Caller(0) - if !ok { - t.Fatal("runtime.Caller failed") - } - dir := filepath.Dir(thisFile) - // The two halves are independent contracts; separate subtests so a Fatal // in one never hides a failure in the other. t.Run("pipeline loads state after the source reload", func(t *testing.T) { - applySrc, err := os.ReadFile(filepath.Join(dir, "apply.go")) - if err != nil { - t.Fatal(err) - } - body := funcBody(string(applySrc), "func runApplyPipeline(") + applySrc := readFileForGuard(t, repoRootFromCaller(t), "internal/cli/apply.go") + body := funcBody(applySrc, "func runApplyPipeline(") if body == "" { t.Fatal("runApplyPipeline not found in apply.go — update this guard") } @@ -86,7 +74,7 @@ func TestApplyPipelineLoadsStateAfterSourceReload(t *testing.T) { // The signature must not accept state either: a caller-supplied // *state.Targets is read before this function runs, which has the same // defect and is how the bug originally shipped. - sig := funcSignature(string(applySrc), "func runApplyPipeline(") + sig := funcSignature(applySrc, "func runApplyPipeline(") if strings.Contains(sig, "*state.Targets") { t.Errorf("runApplyPipeline takes a *state.Targets again: %s\n"+ "A caller reads it before the source reload that can rewrite it. Load it inside, after the reload.", sig) @@ -97,11 +85,7 @@ func TestApplyPipelineLoadsStateAfterSourceReload(t *testing.T) { // The plugin re-apply must BE the pipeline, not a copy of it. The // positive check and the four negatives are Errorf, not Fatal, so a // hand-rolled re-apply reports every way it diverged in one run. - pollSrc, err := os.ReadFile(filepath.Join(dir, "plugin_poll.go")) - if err != nil { - t.Fatal(err) - } - reapply := funcBody(string(pollSrc), "func reapplyAfterPluginChange(") + reapply := funcBody(readFileForGuard(t, repoRootFromCaller(t), "internal/cli/plugin_poll.go"), "func reapplyAfterPluginChange(") if reapply == "" { t.Fatal("reapplyAfterPluginChange not found in plugin_poll.go — update this guard") } diff --git a/internal/cli/explain.go b/internal/cli/explain.go index 7ac5e37c..ee7aec7f 100644 --- a/internal/cli/explain.go +++ b/internal/cli/explain.go @@ -252,6 +252,7 @@ func explainRun(cmd *cobra.Command, rawPath, ptr string, jsonOut bool) error { target: target, pointer: ptr, plan: plan, + reg: reg, agents: reg.Names(), canonical: c, state: s, diff --git a/internal/cli/explain_model.go b/internal/cli/explain_model.go index 6466d17e..656c4cee 100644 --- a/internal/cli/explain_model.go +++ b/internal/cli/explain_model.go @@ -18,10 +18,13 @@ import ( // explainInputs bundles everything buildExplainModel needs, so the builder stays // a pure-ish function the tests can drive without a cobra command. type explainInputs struct { - fs afero.Fs - target string - pointer string - plan render.RenderPlan + fs afero.Fs + target string + pointer string + plan render.RenderPlan + // reg is the registry the plan was rendered with; it resolves a renaming + // adapter's native hook-event spelling back to the canonical one. + reg *adapter.Registry agents []string // registry order, for stable owner ordering canonical source.Canonical state *state.Targets @@ -173,7 +176,7 @@ func fileItem(in explainInputs, it planItem, skips []adapter.Skip, func keyItem(in explainInputs, it planItem, skips []adapter.Skip, origins map[string]explainPluginOrigin, secretRefs map[secrets.RefLocation][]string, hookEvents []string, ) explainItem { - kind, name := componentFromPointer(it.agent, it.ptr, hookEvents) + kind, name := componentFromPointer(in.reg, it.agent, it.ptr, hookEvents) item := explainItem{ Pointer: it.ptr, @@ -245,7 +248,7 @@ func sourceOf(in explainInputs, srcID, kind string) *explainSource { // pointer shape (the mcp/lsp/hooks container families), falling back to the // op-level SourceID when the pointer names no single source. func pointerSource(in explainInputs, agent, ptr, opSourceID string, hookEvents []string) *explainSource { - abs := pointerSourceFile(in.srcHome, agent, ptr, hookEvents) + abs := pointerSourceFile(in.reg, in.srcHome, agent, ptr, hookEvents) if abs == "" { if opSourceID == "" { return nil @@ -297,7 +300,7 @@ func componentFromSourceID(srcID string) (kind, name string) { // componentFromPointer maps a NATIVE key-merge pointer to (kind, name), routing // hooks through the same canonical-event inversion reconcile's write-back uses. -func componentFromPointer(agent, ptr string, hookEvents []string) (kind, name string) { +func componentFromPointer(reg *adapter.Registry, agent, ptr string, hookEvents []string) (kind, name string) { parts := strings.SplitN(strings.TrimPrefix(ptr, "/"), "/", 3) if len(parts) < 2 || parts[1] == "" { return "", "" @@ -308,7 +311,7 @@ func componentFromPointer(agent, ptr string, hookEvents []string) (kind, name st case "lspServers", "lsp": return "lsp", parts[1] case "hooks": - if event, ok := canonicalHookEvent(agent, parts[1], hookEvents); ok { + if event, ok := canonicalHookEvent(reg, agent, parts[1], hookEvents); ok { return "hook", event } return "hook", "" diff --git a/internal/cli/planwalk_characterization_test.go b/internal/cli/planwalk_characterization_test.go index 350ce925..cd04a4e6 100644 --- a/internal/cli/planwalk_characterization_test.go +++ b/internal/cli/planwalk_characterization_test.go @@ -1090,6 +1090,7 @@ func TestPlanWalkCharacterization(t *testing.T) { target: tc.target(userHome), pointer: tc.pointer, plan: plan, + reg: reg, agents: reg.Names(), state: s, userHome: userHome, diff --git a/internal/cli/planwalk_internal_test.go b/internal/cli/planwalk_internal_test.go index fc668087..e0033a3f 100644 --- a/internal/cli/planwalk_internal_test.go +++ b/internal/cli/planwalk_internal_test.go @@ -623,6 +623,7 @@ func TestPathFilterFlagsSurviveAZeroItemOp(t *testing.T) { fs: afero.NewMemMapFs(), target: d, plan: plan, + reg: registryFactory(), agents: names, state: state.New(), userHome: userHome, diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index 2c306ae4..79bb4a46 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -61,7 +61,8 @@ type reconcileItem struct { // native config. attemptWriteBack turns it into a guarded deletion of the // canonical mcp/.toml: a pure deletion carries no secret to re-reference, so // os.Remove (the same primitive `mcp remove` uses) is the approved funnel, and it -// runs only when the user explicitly chose [w]rite-back for that item. +// runs only for a chosen write-back on that item — a per-item [w], a confirmed +// bulk [W], or --auto-writeback. var errDestDroppedServer = errors.New("destination dropped server") func newReconcileCmd() *cobra.Command { @@ -98,12 +99,246 @@ first time. Rule of thumb: import adopts, reconcile resolves.`, return cmd } +// reconcileAction is the resolved outcome for one non-orphan item: what the +// interactive prompt, a confirmed bulk choice, or an --auto-* mode decided to +// do with it. +// +// Unlike adapter.Action, whose zero value is the valid common case (a write), +// the zero value here is deliberately NOT an action — the adapter.SkipKind +// convention, whose zero is likewise invalid. actionNone is the "nothing +// decided yet" state both the unset bulk choice and "no --auto-* mode claimed +// this item" need, so an unresolved item can never be mistaken for a resolved +// one. +// It replaces the byte the loop used to carry ('w'/'o'/'s'/'i'/'q', with a +// `ch | 0x20` case fold open-coded at two sites); parseItemKey is now the one +// place a keystroke becomes an action. +type reconcileAction int + +const ( + // actionNone is the zero value: no action chosen. It never reaches + // applyAction. + actionNone reconcileAction = iota + // actionWriteBack persists the destination value into the canonical source. + actionWriteBack + // actionOverride queues a re-apply of this item's op over the destination. + actionOverride + // actionSkip leaves the item alone. + actionSkip + // actionIgnore appends the item's label to ignore.toml. + actionIgnore + // actionQuit ends the pass; finish still runs. + actionQuit +) + +// key is the lowercase hotkey that chooses the action. The bulk-confirm prompt +// prints it ("apply 'w' to all N remaining items?"), so its spelling is +// user-visible; 0 for actionNone, which is never printed. +func (a reconcileAction) key() byte { + switch a { + case actionWriteBack: + return 'w' + case actionOverride: + return 'o' + case actionSkip: + return 's' + case actionIgnore: + return 'i' + case actionQuit: + return 'q' + } + return 0 +} + +// String is the Stringer form, kept for %v in test failure messages the way +// adapter.Action's is; nothing in production prints an action's name (the +// prompt prints key()). +func (a reconcileAction) String() string { + switch a { + case actionNone: + return "none" + case actionWriteBack: + return "write-back" + case actionOverride: + return "override" + case actionSkip: + return "skip" + case actionIgnore: + return "ignore" + case actionQuit: + return "quit" + default: + return fmt.Sprintf("reconcileAction(%d)", int(a)) + } +} + +// parseItemKey maps one keystroke at the per-item prompt to what it means: +// the action it chooses, whether it is the CAPITAL "apply to all remaining" +// spelling, and whether it is [d]iff (which chooses no action and re-prompts). +// ok is false for every other byte, which the prompt ignores and re-reads. +// +// The folding is deliberately NOT uniform, and that asymmetry is exactly what +// the old `case 'w', 'W', 'o', 'O', 's', 'S', 'i', 'q', 'Q'` switch encoded by +// omission: there is no bulk [i]gnore and no capital [D]iff, so 'I' and 'D' are +// unknown keys. Case-folding every byte would silently add two accepted +// keystrokes — one of them a bulk-ignore that has no confirmation path. +func parseItemKey(ch byte) (act reconcileAction, bulk, diff, ok bool) { + switch ch { + case 'w': + return actionWriteBack, false, false, true + case 'W': + return actionWriteBack, true, false, true + case 'o': + return actionOverride, false, false, true + case 'O': + return actionOverride, true, false, true + case 's': + return actionSkip, false, false, true + case 'S': + return actionSkip, true, false, true + case 'i': + return actionIgnore, false, false, true + case 'q', 'Q': + return actionQuit, false, false, true + case 'd': + return actionNone, false, true, true + } + return actionNone, false, false, false +} + +// reconcileAuto is the --auto-* mode for the run. At most one field is ever +// set: newReconcileSession rejects more than one as its first step, before it +// loads anything, so no session it builds can carry two modes. (A struct +// literal bypasses that check; the session tests build one and set at most one +// mode.) +type reconcileAuto struct { + writeBack bool // --auto-writeback + override bool // --auto-override + safe bool // --auto-safe +} + +// active reports whether any auto mode is set, i.e. the run is not interactive. +func (a reconcileAuto) active() bool { return a.writeBack || a.override || a.safe } + +// overrideOp is one item the user chose to [o]verride. finish re-applies ONLY +// these ops, never the full plan — pressing [o] on one drifted item must not +// silently re-apply every other item in the plan as a side effect. +type overrideOp struct { + agentName string + op adapter.FileOp +} + +// reconcileSession is one `agentsync reconcile` pass: the wiring it was built +// with (printer, input, registry, loaded state, scope, redaction map) and the +// run-scoped bookkeeping the walk accumulates (the queued overrides, the bulk +// choice, the three counters, the per-source write ledger). +// +// It exists because that bookkeeping used to travel as seven loose locals inside +// a 375-line reconcileRun whose only exits were five `goto done`s and two +// labeled loops (issue #232). With the state on a receiver each phase — the two +// prompts, the --auto-* dispatch, the action switch and the finish block — is a +// method a test can drive directly, and every `goto done` is a plain return out +// of walk with exactly one finish call site. +// +// It changes no dest→source write: [w]rite-back still runs through +// writeBackItem → capture.Capture, and the one 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. +type reconcileSession struct { + // --- wiring, fixed for the run --- + cmd *cobra.Command + p *ui.Printer + // w is p.Out: the transcript every prompt, echo and result line writes to. + w io.Writer + br *bufio.Reader + // reg is the adapter registry the plan was rendered with; finish looks up + // each override's adapter in it, and it resolves a hook pointer's native + // event spelling on write-back, so no per-item registry rebuild is needed. + reg *adapter.Registry + home string // ~/.agentsync (canonical source root) + userHome string + statePath string + scope adapter.Scope + projectRoot string + st *state.Targets + auto reconcileAuto + // hookEvents is the canonical hook-event vocabulary of the loaded model, + // computed once: it is the candidate set every hook pointer is inverted + // against, and the model cannot change mid-run. + hookEvents []string + // redact/canMask drive the prompt's masked value display: redact maps each + // resolved secret value back to its ${secret:…} placeholder, and canMask is + // false when some reference could not be resolved now — in which case the + // display falls back to SHA prefixes rather than risk printing a credential. + redact map[string]string + canMask bool + + // --- run-scoped bookkeeping --- + // bulk is the confirmed W/O/S choice applied to every remaining item; + // actionNone until the user confirms one. + bulk reconcileAction + // stateDirty tracks orphan removals so finish persists the pruned state. + stateDirty bool + // autoSkipped counts items an --auto-* mode left unresolved, so the run + // ends with a summary instead of silently doing nothing. + autoSkipped int + // writeBackFailed counts [w]rite-back attempts that errored. A failed + // write-back did NOT persist the user's dest edit, so the run must exit + // non-zero rather than report success (a scripted `reconcile --auto-writeback + // && deploy` must not proceed, and the next apply would clobber the edit). + writeBackFailed int + // writtenSources records, per canonical source file written this run, the + // bytes that landed — so a SECOND write-back to the same file (a server/skill + // that fanned out to multiple agents, each drifted differently) is detected + // instead of silently last-writer-wins clobbering the first. + writtenSources map[string][]byte + // overrideOps is the queue finish re-applies; dedupOverride keeps us from + // re-applying the same path twice when the user picks [o] for two pointers + // inside the same merge file. + overrideOps []overrideOp + dedupOverride map[string]bool +} + func reconcileRun(cmd *cobra.Command, in io.Reader, autoWB, autoOR, autoSafe bool, agentsCSV string) error { + s, items, err := newReconcileSession(cmd, in, reconcileAuto{writeBack: autoWB, override: autoOR, safe: autoSafe}, agentsCSV) + if err != nil { + return err + } + // No actionable items? + if !anyRequiresAction(items) { + fmt.Fprintln(s.w, "nothing to reconcile") + return nil + } + // walk returns no error: every way out of the loop — the last item, an EOF + // at either prompt, an EOF mid bulk-confirm, and [q]uit at either prompt — + // is a plain return that lands here, so finish has exactly ONE call site and + // still runs on every path that used to `goto done`. + // + // finish is deliberately NOT deferred. It WRITES (the override re-apply and + // the state save) and it returns the run's error: a defer would run those + // writes while a panic unwound, and would have to clobber or swallow the + // error of any future early return added above it. + s.walk(items) + return s.finish() +} + +// newReconcileSession loads everything one reconcile pass works from and +// returns the session plus the classified items (drift items first, then +// orphans). +// +// The load order is preserved from the inline setup this replaced — source +// before the printer, state before the registry, plan before the item +// collection — because it is observable: which step fails decides which error +// the user sees, and the printer does not exist yet when the source load fails. +func newReconcileSession(cmd *cobra.Command, in io.Reader, auto reconcileAuto, agentsCSV string) (*reconcileSession, []reconcileItem, error) { // The three auto modes are mutually exclusive — writeback (dest→source) // and override (source→dest) are exact opposites, and silently accepting - // both (writeback won) was a data-loss footgun. - if n := b2i(autoWB) + b2i(autoOR) + b2i(autoSafe); n > 1 { - return fmt.Errorf("--auto-writeback, --auto-override, and --auto-safe are mutually exclusive; pass at most one") + // both (writeback won) was a data-loss footgun. The check is the + // constructor's, not the caller's, so the invariant reconcileAuto's doc + // states is enforced where a session is built: resolveAuto's switch would + // otherwise let writeBack win again for a hand-built session. + if n := b2i(auto.writeBack) + b2i(auto.override) + b2i(auto.safe); n > 1 { + return nil, nil, fmt.Errorf("--auto-writeback, --auto-override, and --auto-safe are mutually exclusive; pass at most one") } home := paths.AgentsyncHome(paths.OSEnv{}) userHome := paths.HomeDir(paths.OSEnv{}) @@ -111,12 +346,12 @@ func reconcileRun(cmd *cobra.Command, in io.Reader, autoWB, autoOR, autoSafe boo // plugin-managed components instead of reporting them as untracked. c, sc, projectRoot, err := loadProjectedForScope(cmd, afero.NewOsFs(), home, false) if err != nil { - return err + return nil, nil, err } p, err := newPrinter(cmd) if err != nil { - return err + return nil, nil, err } // Redaction map for the prompt/[d]iff value display. The destination content @@ -132,9 +367,9 @@ func reconcileRun(cmd *cobra.Command, in io.Reader, autoWB, autoOR, autoSafe boo canMask := len(secrets.UnresolvedSecretRefs(&c, secBackend, envBackend)) == 0 statePath := filepath.Join(home, ".state", "targets.json") - s, err := state.Load(statePath) + st, err := state.Load(statePath) if err != nil { - return err + return nil, nil, err } reg := registryFactory() var agents []string @@ -149,15 +384,15 @@ func reconcileRun(cmd *cobra.Command, in io.Reader, autoWB, autoOR, autoSafe boo if len(agents) > 0 { sel, aerr := selectAgents(cmd, agents, enabled, agentsCSV) if aerr != nil { - return aerr + return nil, nil, aerr } agents = sel } // reconcile hashes the rendered TEMPLATED source for drift; wrap as a // render-only Resolved without substituting (no backend needed). - plan, err := render.Plan(secrets.ForRender(c), reg, agents, sc, projectRoot, s, userHome) + plan, err := render.Plan(secrets.ForRender(c), reg, agents, sc, projectRoot, st, userHome) if err != nil { - return err + return nil, nil, err } // Collect all items in order, then append orphaned whole-file dests @@ -170,56 +405,47 @@ func reconcileRun(cmd *cobra.Command, in io.Reader, autoWB, autoOR, autoSafe boo if sc == adapter.ScopeProject && c.Project != nil { ownerSrc = *c.Project } - items, orphans := collectReconcileItems(plan, reg, s, sc, projectRoot, userHome, pluginProvidedSourceIDs(ownerSrc)) + items, orphans := collectReconcileItems(plan, reg, st, sc, projectRoot, userHome, pluginProvidedSourceIDs(ownerSrc)) items = append(items, orphans...) - w := p.Out - // stateDirty tracks orphan removals so we persist the pruned state at the end. - stateDirty := false + return &reconcileSession{ + cmd: cmd, + p: p, + w: p.Out, + br: bufio.NewReader(in), + reg: reg, + home: home, + userHome: userHome, + statePath: statePath, + scope: sc, + projectRoot: projectRoot, + st: st, + auto: auto, + hookEvents: canonicalHookEvents(c), + redact: redact, + canMask: canMask, + writtenSources: map[string][]byte{}, + dedupOverride: map[string]bool{}, + }, items, nil +} - // No actionable items? - needsPrompt := 0 +// anyRequiresAction reports whether the pass has anything to ask about: an item +// whose class requires action, or an orphan (which has its own remove/keep +// prompt). When it is false the run prints "nothing to reconcile" and stops +// before the session's walk — nothing is queued, so there is nothing to finish. +func anyRequiresAction(items []reconcileItem) bool { for _, it := range items { if requiresAction(it.cls) || it.orphan { - needsPrompt++ + return true } } - if needsPrompt == 0 { - fmt.Fprintln(w, "nothing to reconcile") - return nil - } - - // Track ops the user explicitly chose to override (re-apply source on - // top of dest). We re-apply ONLY these ops at the end, never the full - // plan — pressing [o] on one drifted item must not silently re-apply - // every other item in the plan as a side effect. - type overrideOp struct { - agentName string - op adapter.FileOp - } - var overrideOps []overrideOp - // dedupOverride keeps us from re-applying the same path twice when the - // user picks [o] for two pointers inside the same merge file. - dedupOverride := map[string]bool{} - - // bulkAction is set when user presses W/O/S to apply to all remaining items. - bulkAction := byte(0) - // autoSkipped counts items an --auto-* mode left unresolved, so the run - // ends with a summary instead of silently doing nothing. - autoSkipped := 0 - // writeBackFailed counts [w]rite-back attempts that errored. A failed - // write-back did NOT persist the user's dest edit, so the run must exit - // non-zero rather than report success (a scripted `reconcile --auto-writeback - // && deploy` must not proceed, and the next apply would clobber the edit). - writeBackFailed := 0 - // writtenSources records, per canonical source file written this run, the - // bytes that landed — so a SECOND write-back to the same file (a server/skill - // that fanned out to multiple agents, each drifted differently) is detected - // instead of silently last-writer-wins clobbering the first. - writtenSources := map[string][]byte{} - - br := bufio.NewReader(in) + return false +} +// walk runs the pass over items, resolving each one through the bulk choice, +// the --auto-* dispatch, or the interactive prompt. It returns as soon as the +// user quits or the input ends; the caller then calls finish exactly once. +func (s *reconcileSession) walk(items []reconcileItem) { for idx := range items { it := items[idx] if !requiresAction(it.cls) && !it.orphan { @@ -229,247 +455,301 @@ func reconcileRun(cmd *cobra.Command, in io.Reader, autoWB, autoOR, autoSafe boo // Orphans get a dedicated delete/keep prompt — deletion is never done // in an auto mode (too destructive to do non-interactively). if it.orphan { - if autoWB || autoOR || autoSafe { - fmt.Fprintf(w, "orphan left in place (run `agentsync reconcile` interactively to remove): %s\n", ui.Sanitize(it.op.Path)) - autoSkipped++ + if s.auto.active() { + fmt.Fprintf(s.w, "orphan left in place (run `agentsync reconcile` interactively to remove): %s\n", ui.Sanitize(it.op.Path)) + s.autoSkipped++ continue } - fmt.Fprintf(w, "\n%s (orphan — source no longer produces this file)\n", ui.Sanitize(it.op.Path)) - // [k]eep is honest only for the kinds `apply` does NOT auto-reclaim. - // Skills, subagents, and commands are reclaimed on the next apply, so - // "keep" there means "keep until then" — say so rather than imply it - // is permanent. - // - // This asks the KIND question (OrphanIsReclaimable), not the - // readability one (OrphanDeleteWillProceed): a reclaimable orphan that - // happens to be unreadable right now is retried on every subsequent - // apply, so "until the next apply reclaims it" is still the truth for - // it. Using the readability predicate here would print the permanent - // wording for exactly the file apply keeps coming back to. - if render.OrphanIsReclaimable(it.op.SourceID) { - fmt.Fprintf(w, " [r]emove (backs up first) [k]eep (until the next apply reclaims it) [q]uit\n > ") - } else { - fmt.Fprintf(w, " [r]emove (backs up first) [k]eep [q]uit\n > ") - } - orphanPrompt: - for { - ch, readErr := readChar(br) - if readErr != nil { - goto done // EOF → finish (persist any pruned state) - } - switch ch { - case 'r', 'R': - fmt.Fprintf(w, "%c\n", ch) - bk, berr := render.BackupFile(home, it.op.Path) - if berr != nil { - p.Fdiagf(w, ui.LevelError, "backup failed, NOT removing: %s", ui.Sanitize(berr.Error())) - break orphanPrompt - } - if bk != "" { - fmt.Fprintf(w, " backup: %s\n", ui.Sanitize(bk)) - } - if rmErr := os.Remove(it.op.Path); rmErr != nil && !os.IsNotExist(rmErr) { //nolint:forbidigo // the one NATIVE-destination delete outside DestWriter: interactive orphan removal, safe only because render.BackupFile succeeded just above - p.Fdiagf(w, ui.LevelError, "remove failed: %s", ui.Sanitize(rmErr.Error())) - break orphanPrompt - } - pruneStateFilesForPath(s, userHome, it.op.Path) - stateDirty = true - fmt.Fprintf(w, " removed: %s\n", ui.Sanitize(it.op.Path)) - break orphanPrompt - case 'k', 'K': - fmt.Fprintf(w, "%c\n", ch) - fmt.Fprintf(w, " kept: %s\n", ui.Sanitize(it.op.Path)) - break orphanPrompt - case 'q', 'Q': - fmt.Fprintln(w, "quit") - goto done - default: - // ignore unknown key, re-read - } + if s.promptOrphan(it) { + return } continue } - // Apply bulk action if set. - action := bulkAction - if action == 0 { - switch { - case autoWB: - // ForeignCollision is a never-applied pre-existing native - // file. Writing it back would overwrite the curated source - // with foreign content — the worst data-loss path. Refuse to - // do that non-interactively; leave it for an explicit choice. - switch { - case it.cls == drift.ForeignCollision: - fmt.Fprintf(w, "skipped (foreign-collision, would overwrite source): %s — resolve interactively\n", itemLabelDisp(it)) - autoSkipped++ - action = 's' - case it.pluginOwner != "": - // A plugin-provided component cannot be written back at all — - // it has no canonical file of its own. That refusal is - // STRUCTURAL and permanent, not a transient failure, so - // letting it count as a write-back FAILURE would make - // `reconcile --auto-writeback` exit non-zero on every run - // forever, breaking any `reconcile && deploy` until the user - // disables the plugin. Skip it like a foreign collision — the - // same "cannot be resolved non-interactively" shape. - fmt.Fprintf(w, "skipped (provided by plugin %q, no canonical file to write into): %s — use [o]verride, or change it upstream\n", - ui.Sanitize(it.pluginOwner), itemLabelDisp(it)) - autoSkipped++ - action = 's' - default: - action = 'w' - } - case autoOR: - action = 'o' - case autoSafe: - // auto-safe resolves nothing: everything that reaches this loop - // needs a human. - fmt.Fprintf(w, "skipped (needs manual review): %s (%s)\n", itemLabelDisp(it), it.cls) - autoSkipped++ - action = 's' + // A confirmed bulk choice wins; otherwise an --auto-* mode may claim the + // item; otherwise ask. + action := s.bulk + if action == actionNone { + action = s.resolveAuto(it) + } + if action == actionNone { + var stop bool + action, stop = s.promptItem(it, items[idx:]) + if stop { + return } } + if s.applyAction(it, action) { + return + } + } +} - if action == 0 { - // Interactive prompt. - label := itemLabelDisp(it) - fmt.Fprintf(w, "\n%s (%s)\n", label, it.cls) - renderItemValues(w, p, it, redact, canMask) - fmt.Fprintf(w, " [w]rite-back [o]verride [s]kip [i]gnore [d]iff [q]uit\n > ") - - prompt: - for { - ch, readErr := readChar(br) - if readErr != nil { - // EOF → finish gracefully, but reach `done:` so any queued - // [o]verride ops are applied and pruned/dirty state is flushed - // (a bare `return nil` here dropped both — issue #171). - goto done - } - switch ch { - case 'w', 'W', 'o', 'O', 's', 'S', 'i', 'q', 'Q': - if ch == 'W' || ch == 'O' || ch == 'S' { - // Capital letter = "apply this choice to all - // remaining items." Confirm before locking it - // in — a stray shift-W on a hooks item used to - // silently no-op data away across the whole - // queue. Show the count and require an - // explicit y/N. Default is N. - // - // The count is the TRUE blast radius of this bulk action: - // the items from HERE forward it will actually act on — - // remaining actionable, non-orphan items (orphans have their - // own r/k prompt and are never swept by a bulk choice). The - // prior count walked the WHOLE queue including items already - // handled, overstating the reach. - remaining := 0 - for j := idx; j < len(items); j++ { - if requiresAction(items[j].cls) && !items[j].orphan { - remaining++ - } - } - lower := ch | 0x20 - fmt.Fprintf(w, "%c\n", ch) - fmt.Fprintf(w, " apply '%c' to all %d remaining items? [y/N] ", lower, remaining) - confirm, readErr := readChar(br) - if readErr != nil { - goto done // EOF mid-confirm → flush queued overrides + state (issue #171) - } - fmt.Fprintf(w, "%c\n", confirm) - if confirm != 'y' && confirm != 'Y' { - fmt.Fprintln(w, " cancelled; choose a per-item action") - continue - } - bulkAction = lower - action = lower - break prompt - } - action = ch | 0x20 - fmt.Fprintf(w, "%c\n", ch) - break prompt - case 'd': - printItemDiff(w, p, it, redact, canMask) - fmt.Fprintf(w, " [w]rite-back [o]verride [s]kip [i]gnore [d]iff [q]uit\n > ") - default: - // ignore unknown key - } +// promptOrphan runs the remove/keep prompt for one orphaned destination and +// reports whether the whole run must stop — the user pressed [q]uit, or the +// input ended. Both land on the same finish the walk's other exits do, so a +// removal already made this run is still persisted (issue #171). +func (s *reconcileSession) promptOrphan(it reconcileItem) bool { + w := s.w + fmt.Fprintf(w, "\n%s (orphan — source no longer produces this file)\n", ui.Sanitize(it.op.Path)) + // [k]eep is honest only for the kinds `apply` does NOT auto-reclaim. + // Skills, subagents, and commands are reclaimed on the next apply, so + // "keep" there means "keep until then" — say so rather than imply it + // is permanent. + // + // This asks the KIND question (OrphanIsReclaimable), not the + // readability one (OrphanDeleteWillProceed): a reclaimable orphan that + // happens to be unreadable right now is retried on every subsequent + // apply, so "until the next apply reclaims it" is still the truth for + // it. Using the readability predicate here would print the permanent + // wording for exactly the file apply keeps coming back to. + if render.OrphanIsReclaimable(it.op.SourceID) { + fmt.Fprintf(w, " [r]emove (backs up first) [k]eep (until the next apply reclaims it) [q]uit\n > ") + } else { + fmt.Fprintf(w, " [r]emove (backs up first) [k]eep [q]uit\n > ") + } + for { + ch, readErr := readChar(s.br) + if readErr != nil { + return true // EOF → finish (persist any pruned state) + } + switch ch { + case 'r', 'R': + fmt.Fprintf(w, "%c\n", ch) + bk, berr := render.BackupFile(s.home, it.op.Path) + if berr != nil { + s.p.Fdiagf(w, ui.LevelError, "backup failed, NOT removing: %s", ui.Sanitize(berr.Error())) + return false } + if bk != "" { + fmt.Fprintf(w, " backup: %s\n", ui.Sanitize(bk)) + } + if rmErr := os.Remove(it.op.Path); rmErr != nil && !os.IsNotExist(rmErr) { //nolint:forbidigo // the one NATIVE-destination delete outside DestWriter: interactive orphan removal, safe only because render.BackupFile succeeded just above + s.p.Fdiagf(w, ui.LevelError, "remove failed: %s", ui.Sanitize(rmErr.Error())) + return false + } + pruneStateFilesForPath(s.st, s.userHome, it.op.Path) + s.stateDirty = true + fmt.Fprintf(w, " removed: %s\n", ui.Sanitize(it.op.Path)) + return false + case 'k', 'K': + fmt.Fprintf(w, "%c\n", ch) + fmt.Fprintf(w, " kept: %s\n", ui.Sanitize(it.op.Path)) + return false + case 'q', 'Q': + fmt.Fprintln(w, "quit") + return true + default: + // ignore unknown key, re-read + } + } +} + +// resolveAuto is the --auto-* dispatch for one non-orphan item: the action the +// mode chose, or actionNone when no mode is set and the item needs the +// interactive prompt. The two refusals print their own line and count the skip, +// because a refusal IS the mode's answer for that item. +func (s *reconcileSession) resolveAuto(it reconcileItem) reconcileAction { + switch { + case s.auto.writeBack: + switch { + case it.cls == drift.ForeignCollision: + // ForeignCollision is a never-applied pre-existing native + // file. Writing it back would overwrite the curated source + // with foreign content — the worst data-loss path. Refuse to + // do that non-interactively; leave it for an explicit choice. + fmt.Fprintf(s.w, "skipped (foreign-collision, would overwrite source): %s — resolve interactively\n", itemLabelDisp(it)) + s.autoSkipped++ + return actionSkip + case it.pluginOwner != "": + // A plugin-provided component cannot be written back at all — + // it has no canonical file of its own. That refusal is + // STRUCTURAL and permanent, not a transient failure, so + // letting it count as a write-back FAILURE would make + // `reconcile --auto-writeback` exit non-zero on every run + // forever, breaking any `reconcile && deploy` until the user + // disables the plugin. Skip it like a foreign collision — the + // same "cannot be resolved non-interactively" shape. + fmt.Fprintf(s.w, "skipped (provided by plugin %q, no canonical file to write into): %s — use [o]verride, or change it upstream\n", + ui.Sanitize(it.pluginOwner), itemLabelDisp(it)) + s.autoSkipped++ + return actionSkip + default: + return actionWriteBack } + case s.auto.override: + return actionOverride + case s.auto.safe: + // auto-safe resolves nothing: everything that reaches this loop + // needs a human. + fmt.Fprintf(s.w, "skipped (needs manual review): %s (%s)\n", itemLabelDisp(it), it.cls) + s.autoSkipped++ + return actionSkip + } + return actionNone +} - switch action { - case 'w': - // write-back: persist destination value into the canonical source. - if attemptWriteBack(cmd, p, w, home, it, canonicalHookEvents(c), writtenSources) { - writeBackFailed++ +// itemMenu is the per-item menu line and the "> " prompt that follows it. +// promptItem prints it before the first read and again after [d]iff; one +// spelling keeps the two sites in step. +const itemMenu = " [w]rite-back [o]verride [s]kip [i]gnore [d]iff [q]uit\n > " + +// promptItem runs the per-item prompt and returns the action the user chose. +// stop is true when the run must end without acting on this item: the input +// ended at the prompt, or ended mid bulk-confirm — both reach finish so queued +// [o]verride ops are applied and pruned state is flushed (issue #171). +// +// rest is the queue from this item forward; the bulk-confirm count is measured +// over it. A confirmed bulk choice is recorded on the session, so every later +// item skips this prompt entirely. +func (s *reconcileSession) promptItem(it reconcileItem, rest []reconcileItem) (reconcileAction, bool) { + w := s.w + label := itemLabelDisp(it) + fmt.Fprintf(w, "\n%s (%s)\n", label, it.cls) + renderItemValues(w, s.p, it, s.redact, s.canMask) + fmt.Fprint(w, itemMenu) + + for { + ch, readErr := readChar(s.br) + if readErr != nil { + return actionNone, true + } + act, bulk, diff, ok := parseItemKey(ch) + switch { + case !ok: + // ignore unknown key + case diff: + renderItemValues(w, s.p, it, s.redact, s.canMask) + fmt.Fprint(w, itemMenu) + case bulk: + // Capital letter = "apply this choice to all + // remaining items." Confirm before locking it + // in — a stray shift-W on a hooks item used to + // silently no-op data away across the whole + // queue. Show the count and require an + // explicit y/N. Default is N. + remaining := bulkTargets(rest) + fmt.Fprintf(w, "%c\n", ch) + fmt.Fprintf(w, " apply '%c' to all %d remaining items? [y/N] ", act.key(), remaining) + confirm, readErr := readChar(s.br) + if readErr != nil { + return actionNone, true } - case 'o': - // override: queue a re-apply of this item's op. - dedupKey := it.agentName + "\x00" + it.op.Path - if !dedupOverride[dedupKey] { - dedupOverride[dedupKey] = true - overrideOps = append(overrideOps, overrideOp{it.agentName, it.op}) + fmt.Fprintf(w, "%c\n", confirm) + if confirm != 'y' && confirm != 'Y' { + fmt.Fprintln(w, " cancelled; choose a per-item action") + continue } - case 's': - // skip: do nothing. - case 'i': - // ignore: append to ignore.toml (best-effort). - _ = appendIgnore(home, itemLabel(it)) - fmt.Fprintf(w, " ignored: %s\n", itemLabelDisp(it)) - case 'q': - fmt.Fprintln(w, "quit") - goto done + s.bulk = act + return act, false + default: + fmt.Fprintf(w, "%c\n", ch) + return act, false + } + } +} + +// bulkTargets is the TRUE blast radius of a bulk action chosen at the head of +// rest: the items from HERE forward it will actually act on — remaining +// actionable, non-orphan items, including the current one. Orphans have their +// own r/k prompt and are never swept by a bulk choice. An earlier count walked +// the WHOLE queue including items already handled, overstating the reach. +func bulkTargets(rest []reconcileItem) int { + n := 0 + for _, it := range rest { + if requiresAction(it.cls) && !it.orphan { + n++ } } + return n +} -done: +// applyAction performs the resolved action for one item and reports whether the +// run must stop ([q]uit). It is the single place an action's meaning lives, so +// the bulk, --auto-* and interactive paths cannot drift apart on what one does. +func (s *reconcileSession) applyAction(it reconcileItem, action reconcileAction) bool { + switch action { + case actionWriteBack: + // write-back: persist destination value into the canonical source. + if s.attemptWriteBack(it) { + s.writeBackFailed++ + } + case actionOverride: + // override: queue a re-apply of this item's op. + dedupKey := it.agentName + "\x00" + it.op.Path + if !s.dedupOverride[dedupKey] { + s.dedupOverride[dedupKey] = true + s.overrideOps = append(s.overrideOps, overrideOp{it.agentName, it.op}) + } + case actionSkip: + // skip: do nothing. + case actionIgnore: + // ignore: append to ignore.toml (best-effort). + _ = appendIgnore(s.home, itemLabel(it)) + fmt.Fprintf(s.w, " ignored: %s\n", itemLabelDisp(it)) + case actionQuit: + fmt.Fprintln(s.w, "quit") + return true + } + return false +} + +// finish is the tail of every reconcile pass — the `done:` block the five +// `goto done`s used to jump to. It re-applies the queued [o]verride ops, +// persists state when an orphan removal pruned it, prints the unresolved +// summary and decides the exit code. +// +// It has exactly one caller (reconcileRun, after walk returns) and is never +// deferred: it writes and it returns the run's error. +func (s *reconcileSession) finish() error { + w := s.w // Execute override re-applies — ONLY for the ops the user opted into, // grouped by adapter so each adapter sees its own ops. The previous // implementation re-ran Apply for the entire plan, which silently // re-applied every other agent's ops as a side effect. - if len(overrideOps) > 0 { + if len(s.overrideOps) > 0 { byAgent := map[string][]adapter.FileOp{} - for _, oo := range overrideOps { + for _, oo := range s.overrideOps { byAgent[oo.agentName] = append(byAgent[oo.agentName], oo.op) } for name, ops := range byAgent { - a := reg.Lookup(name) + a := s.reg.Lookup(name) if a == nil { return fmt.Errorf("reconcile override: adapter %q not registered", name) } - rw := render.NewWriter(s, home, userHome, sc, projectRoot, name) + rw := render.NewWriter(s.st, s.home, s.userHome, s.scope, s.projectRoot, name) if err := a.Apply(ops, rw); err != nil { return fmt.Errorf("reconcile override apply %s: %w", name, err) } for _, r := range rw.Reports() { fmt.Fprintf(w, " backup: %s\n", r.String()) } - if err := render.RecordOpsState(s, userHome, name, sc, projectRoot, ops); err != nil { + if err := render.RecordOpsState(s.st, s.userHome, name, s.scope, s.projectRoot, ops); err != nil { return err } } - if err := state.Save(statePath, s); err != nil { + if err := state.Save(s.statePath, s.st); err != nil { return err } - stateDirty = false // override save already persisted the pruned state - fmt.Fprintf(w, "override: applied %d item(s)\n", len(overrideOps)) + s.stateDirty = false // override save already persisted the pruned state + fmt.Fprintf(w, "override: applied %d item(s)\n", len(s.overrideOps)) } // Persist state if orphan removals pruned ownership and the override block // above didn't already save. - if stateDirty { - if err := state.Save(statePath, s); err != nil { + if s.stateDirty { + if err := state.Save(s.statePath, s.st); err != nil { return err } } - if autoSkipped > 0 { - fmt.Fprintf(w, "%d item(s) left unresolved; run `agentsync reconcile` interactively to handle them\n", autoSkipped) + if s.autoSkipped > 0 { + fmt.Fprintf(w, "%d item(s) left unresolved; run `agentsync reconcile` interactively to handle them\n", s.autoSkipped) } // A write-back that errored did NOT persist the edit; surface it as a // non-zero exit so callers (and scripts) don't treat the sync as complete. - if writeBackFailed > 0 { - return fmt.Errorf("reconcile: %d item(s) failed to write back", writeBackFailed) + if s.writeBackFailed > 0 { + return fmt.Errorf("reconcile: %d item(s) failed to write back", s.writeBackFailed) } return nil } @@ -766,10 +1046,6 @@ func renderItemValues(w io.Writer, p *ui.Printer, it reconcileItem, redact map[s } } -func printItemDiff(w io.Writer, p *ui.Printer, it reconcileItem, redact map[string]string, canMask bool) { - renderItemValues(w, p, it, redact, canMask) -} - // readChar reads a single non-whitespace character from r. func readChar(r *bufio.Reader) (byte, error) { for { @@ -783,9 +1059,6 @@ func readChar(r *bufio.Reader) (byte, error) { } } -// writeBackItem persists the current destination value for item it back into -// the canonical source (~/.agentsync/). Only MCP-server items are fully -// supported in v1; other item types fall back to a raw file copy. // attemptWriteBack writes one item back and guards against silent // last-writer-wins when a single reconcile run writes the SAME canonical source // file from more than one agent. A server/skill that fans out to claude AND @@ -796,36 +1069,38 @@ func readChar(r *bufio.Reader) (byte, error) { // file and this write changes it, revert to the first write and report a // conflict (counted as a failure → non-zero exit) for the user to resolve. // Returns true on failure/conflict. -func attemptWriteBack(cmd *cobra.Command, p *ui.Printer, w io.Writer, home string, it reconcileItem, hookEvents []string, writtenSources map[string][]byte) bool { - srcFile := itemSourceFile(home, it, hookEvents) +func (s *reconcileSession) attemptWriteBack(it reconcileItem) bool { + w := s.w + srcFile := s.itemSourceFile(it) var prior []byte priorWritten := false if srcFile != "" { - prior, priorWritten = writtenSources[srcFile] + prior, priorWritten = s.writtenSources[srcFile] } - werr := writeBackItem(cmd, home, it) + werr := writeBackItem(s.cmd, s.home, it) if errors.Is(werr, errDestDroppedServer) { // Tombstone: the user deleted this MCP server from the native config, and - // chose [w]rite-back to persist that. A pure deletion carries no secret, so + // chose write-back (per-item [w], confirmed bulk [W], or --auto-writeback) + // to persist that. A pure deletion carries no secret, so // remove the canonical mcp/.toml directly (the same os.Remove primitive // `mcp remove` uses) rather than routing an empty spec through capture. - return removeDroppedSource(p, w, home, it, srcFile, prior, priorWritten, writtenSources) + return s.removeDroppedSource(it, srcFile, prior, priorWritten) } if werr != nil { - p.Fdiagf(w, ui.LevelError, "write-back: %s", ui.Sanitize(werr.Error())) + s.p.Fdiagf(w, ui.LevelError, "write-back: %s", ui.Sanitize(werr.Error())) return true } if srcFile != "" { if after, rerr := os.ReadFile(srcFile); rerr == nil { if priorWritten && string(prior) != string(after) { revertSource(srcFile, prior) // undo this write; keep the first - rel, _ := filepath.Rel(home, srcFile) + rel, _ := filepath.Rel(s.home, srcFile) fmt.Fprintf(w, " conflict: %s — another agent drifted the same source (%s) to a different "+ "value this run; kept the first write and skipped this one. Make the agents agree, or "+ "reconcile one at a time, then re-run.\n", itemLabelDisp(it), ui.Sanitize(rel)) return true } - writtenSources[srcFile] = after + s.writtenSources[srcFile] = after } } fmt.Fprintf(w, " write-back: %s\n", itemLabelDisp(it)) @@ -841,30 +1116,31 @@ func attemptWriteBack(cmd *cobra.Command, p *ui.Printer, w io.Writer, home strin // (nil bytes) in writtenSources so a LATER content write-back to the same file // this run is likewise flagged rather than silently resurrecting it. Returns true // on failure/conflict. -func removeDroppedSource(p *ui.Printer, w io.Writer, home string, it reconcileItem, srcFile string, prior []byte, priorWritten bool, writtenSources map[string][]byte) bool { +func (s *reconcileSession) removeDroppedSource(it reconcileItem, srcFile string, prior []byte, priorWritten bool) bool { + w := s.w if srcFile == "" { - p.Fdiagf(w, ui.LevelError, "write-back: %s — cannot locate the canonical source file to delete", itemLabelDisp(it)) + s.p.Fdiagf(w, ui.LevelError, "write-back: %s — cannot locate the canonical source file to delete", itemLabelDisp(it)) return true } // Defense-in-depth: srcFile derives from a native-config-supplied server id; // never let a traversal segment escape ~/.agentsync into an arbitrary unlink. - if !withinDir(home, srcFile) { - p.Fdiagf(w, ui.LevelError, "write-back: %s — refusing to delete outside the source tree", itemLabelDisp(it)) + if !withinDir(s.home, srcFile) { + s.p.Fdiagf(w, ui.LevelError, "write-back: %s — refusing to delete outside the source tree", itemLabelDisp(it)) return true } if priorWritten && len(prior) > 0 { - rel, _ := filepath.Rel(home, srcFile) - p.Fdiagf(w, ui.LevelError, "conflict: %s — another agent wrote this source (%s) this run, so it is still in use; "+ + rel, _ := filepath.Rel(s.home, srcFile) + s.p.Fdiagf(w, ui.LevelError, "conflict: %s — another agent wrote this source (%s) this run, so it is still in use; "+ "not deleting it. Make the agents agree (remove it from every native config), then re-run.", itemLabelDisp(it), ui.Sanitize(rel)) return true } if rmErr := os.Remove(srcFile); rmErr != nil && !os.IsNotExist(rmErr) { //nolint:forbidigo // removes a canonical source file under ~/.agentsync (withinDir-guarded above), not a native destination - p.Fdiagf(w, ui.LevelError, "write-back: remove %s: %s", itemLabelDisp(it), ui.Sanitize(rmErr.Error())) + s.p.Fdiagf(w, ui.LevelError, "write-back: remove %s: %s", itemLabelDisp(it), ui.Sanitize(rmErr.Error())) return true } - writtenSources[srcFile] = nil // deletion sentinel for a later same-file write - rel, _ := filepath.Rel(home, srcFile) + s.writtenSources[srcFile] = nil // deletion sentinel for a later same-file write + rel, _ := filepath.Rel(s.home, srcFile) fmt.Fprintf(w, " write-back: removed source %s (destination dropped %s)\n", ui.Sanitize(rel), itemLabelDisp(it)) return false } @@ -884,14 +1160,14 @@ func revertSource(srcFile string, prior []byte) { // targets, so two agents writing the same component can be detected. Both the // claude (/mcpServers/) and opencode (/mcp/) pointers map to the SAME // mcp/.toml. Returns "" for items with no single source-of-record. -func itemSourceFile(home string, it reconcileItem, hookEvents []string) string { +func (s *reconcileSession) itemSourceFile(it reconcileItem) string { if it.ptr == "" { if it.op.SourceID == "" || strings.HasSuffix(it.op.SourceID, "(multiple)") { return "" } - return filepath.Join(home, it.op.SourceID) + return filepath.Join(s.home, it.op.SourceID) } - return pointerSourceFile(home, it.agentName, it.ptr, hookEvents) + return pointerSourceFile(s.reg, s.home, it.agentName, it.ptr, s.hookEvents) } // pointerSourceFile maps a NATIVE key-merge JSON pointer back to the canonical @@ -911,8 +1187,11 @@ func itemSourceFile(home string, it reconcileItem, hookEvents []string) string { // comes from one of them, so scanning that set is both sufficient and free of a // second, drift-prone enumeration of the event vocabulary. // +// reg is the caller's own registry — reconcile's session, explain's — so the +// inversion costs a map lookup instead of rebuilding all 31 adapters per call. +// // Returns "" when the pointer names no single canonical source-of-record. -func pointerSourceFile(home, agent, ptr string, canonicalEvents []string) string { +func pointerSourceFile(reg *adapter.Registry, home, agent, ptr string, canonicalEvents []string) string { parts := strings.SplitN(strings.TrimPrefix(ptr, "/"), "/", 3) if len(parts) < 2 || parts[1] == "" { return "" @@ -923,7 +1202,7 @@ func pointerSourceFile(home, agent, ptr string, canonicalEvents []string) string case "lspServers", "lsp": return filepath.Join(home, "lsp", parts[1]+".toml") case "hooks": - event, ok := canonicalHookEvent(agent, parts[1], canonicalEvents) + event, ok := canonicalHookEvent(reg, agent, parts[1], canonicalEvents) if !ok { return "" } @@ -937,8 +1216,28 @@ func pointerSourceFile(home, agent, ptr string, canonicalEvents []string) string // codex — no HookEventNamer) passes the segment through. A renaming adapter is // inverted by asking it for the native spelling of each candidate canonical // event; ok is false when nothing matches. -func canonicalHookEvent(agent, native string, canonicalEvents []string) (string, bool) { - namer, ok := registryFactory().Lookup(agent).(adapter.HookEventNamer) +// +// reg is the CALLER's registry (reconcile's session, explain's), not a fresh +// registryFactory() per call: that spelling rebuilt all 31 adapters every time +// a hook pointer was resolved (measured: ~10µs per call, building 31 +// adapters). Registry.Lookup returns a nil adapter.Adapter for an unregistered +// name, and a comma-ok type assertion on a nil interface yields (nil, false) +// rather than panicking, so an unknown agent is treated as non-renaming and +// passes the segment through — unchanged from the registryFactory() spelling, +// which had exactly the same contents. +// +// The precondition is a non-nil reg — every production caller holds one — but +// Registry.Lookup dereferences its receiver, so a nil *Registry is guarded here. +// It resolves NOTHING (ok is false) rather than passing the segment through: a +// caller that forgot its registry would otherwise get a plausible wrong answer +// (gemini's /hooks/BeforeTool as hooks/BeforeTool.toml) that no test could tell +// from a right one, whereas "no source" is visibly incomplete in explain and +// leaves reconcile's write-back no file to touch. +func canonicalHookEvent(reg *adapter.Registry, agent, native string, canonicalEvents []string) (string, bool) { + if reg == nil { + return "", false + } + namer, ok := reg.Lookup(agent).(adapter.HookEventNamer) if !ok { return native, true } @@ -972,6 +1271,9 @@ func canonicalHookEvents(c source.Canonical) []string { return out } +// writeBackItem persists the current destination value for item it back into +// the canonical source (~/.agentsync/). Only MCP-server items are fully +// supported in v1; other item types fall back to a raw file copy. func writeBackItem(cmd *cobra.Command, home string, it reconcileItem) error { // A plugin-provided component has no canonical file of its own: it is // re-derived from the plugin cache on every load. Writing the destination @@ -1035,7 +1337,8 @@ func writeBackKeyItem(cmd *cobra.Command, home string, it reconcileItem) error { specRaw, ok := mcpServers[serverID] if !ok { // Server removed from dest: the user deleted it from the native - // config and chose [w]rite-back to persist that. Signal a tombstone; + // config and chose write-back (per-item [w], confirmed bulk [W], or + // --auto-writeback) to persist that. Signal a tombstone; // attemptWriteBack deletes the canonical mcp/.toml through the // approved os.Remove funnel (a pure deletion carries no secret), with // the multi-agent fan-out guard. diff --git a/internal/cli/reconcile_hookevent_internal_test.go b/internal/cli/reconcile_hookevent_internal_test.go new file mode 100644 index 00000000..89267bfd --- /dev/null +++ b/internal/cli/reconcile_hookevent_internal_test.go @@ -0,0 +1,79 @@ +package cli + +import ( + "testing" + + "github.com/spxrogers/agentsync/internal/adapter" +) + +// TestCanonicalHookEvent pins the inversion contract at the moment it stopped +// building its own registry (#232). Passing the caller's registry cannot change +// the answer — it is the same registryFactory() product either way — but the +// old spelling made two facts implicit that the table below makes explicit: +// +// - a NON-renaming adapter (claude, codex: no HookEventNamer) passes the +// native segment straight through, because the comma-ok assertion fails; and +// - an UNREGISTERED agent takes exactly the same branch, because +// Registry.Lookup returns a nil adapter.Adapter and a comma-ok assertion on +// a nil interface yields (nil, false) rather than panicking. That was +// nil-safe only by Go's assertion semantics, never by an explicit guard, and +// nothing asserted it. +// - a NIL registry is guarded explicitly (Registry.Lookup dereferences its +// receiver) and resolves nothing: the precondition is a non-nil registry, +// and a caller that forgot to set one gets "no source" rather than the +// plausible wrong path a passthrough would hand it. +// +// The renaming rows are the ones that must keep working: a gemini +// `/hooks/BeforeTool` pointer has to resolve to hooks/PreToolUse.toml, or +// reconcile's write-back and `explain #` name a file that does +// not exist. +func TestCanonicalHookEvent(t *testing.T) { + reg := registryFactory() + events := []string{"PreToolUse", "PostToolUse"} + + tests := []struct { + name string + nilReg bool // pass a nil *Registry instead of reg + agent string + native string + want string + wantOK bool + }{ + {name: "claude does not rename", agent: "claude", native: "PreToolUse", want: "PreToolUse", wantOK: true}, + {name: "codex does not rename", agent: "codex", native: "PostToolUse", want: "PostToolUse", wantOK: true}, + {name: "gemini renames", agent: "gemini", native: "BeforeTool", want: "PreToolUse", wantOK: true}, + {name: "cursor renames", agent: "cursor", native: "preToolUse", want: "PreToolUse", wantOK: true}, + {name: "renaming agent, unknown native spelling", agent: "gemini", native: "NoSuchEvent"}, + {name: "unregistered agent passes through", agent: "no-such-agent", native: "PreToolUse", want: "PreToolUse", wantOK: true}, + {name: "empty agent passes through", agent: "", native: "PreToolUse", want: "PreToolUse", wantOK: true}, + {name: "nil registry resolves nothing", nilReg: true, agent: "gemini", native: "BeforeTool"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + r := reg + if tc.nilReg { + r = nil + } + got, ok := canonicalHookEvent(r, tc.agent, tc.native, events) + if got != tc.want || ok != tc.wantOK { + t.Errorf("canonicalHookEvent(%q, %q) = (%q, %v), want (%q, %v)", + tc.agent, tc.native, got, ok, tc.want, tc.wantOK) + } + }) + } +} + +// TestCanonicalHookEvent_UnregisteredLookupIsNil is the half of the above the +// table cannot show: WHY an unregistered agent is safe. If Lookup ever returned +// a non-nil zero adapter, the comma-ok assertion could start succeeding and the +// passthrough row above would silently change meaning. +func TestCanonicalHookEvent_UnregisteredLookupIsNil(t *testing.T) { + a := registryFactory().Lookup("no-such-agent") + if a != nil { + t.Fatalf("Registry.Lookup of an unregistered name = %T, want a nil adapter.Adapter: "+ + "canonicalHookEvent's passthrough depends on the comma-ok assertion failing", a) + } + if _, ok := a.(adapter.HookEventNamer); ok { + t.Fatal("the comma-ok assertion on Lookup's nil result reported ok=true") + } +} diff --git a/internal/cli/reconcile_session_internal_test.go b/internal/cli/reconcile_session_internal_test.go new file mode 100644 index 00000000..65bec40f --- /dev/null +++ b/internal/cli/reconcile_session_internal_test.go @@ -0,0 +1,519 @@ +package cli + +import ( + "bufio" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" + + "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/drift" + "github.com/spxrogers/agentsync/internal/ui" +) + +// The tests below drive reconcileSession's methods directly. That is the point +// of the type (#232): before it existed, the prompt loops, the bulk-confirm +// state machine and the run's exit paths were reachable only by running the +// whole 375-line reconcileRun against a real ~/.agentsync tree, a real plan and +// a real destination — so the paths that decide when a pass STOPS (an EOF at +// either prompt, an EOF mid bulk-confirm, [q]uit with items still queued) were +// pinned by nothing. Measured on the pre-#232 tree: mutating any of those five +// exits failed zero tests. +// +// Every item carries hasText=false, so the prompt renders the SHA-prefix +// fallback and no destination is read. The one test that writes, +// TestApplyAction_IgnoreAppendsToIgnoreFile, points the session's home at a +// temp dir first; everything else here is a no-op, a print or an in-memory +// queue. + +// newTestSession builds a session wired to a scripted stdin and an in-memory +// transcript. Only the fields the prompt/walk path reads are set; home, st and +// reg are deliberately left zero, and that bounds what a test may drive. Two +// arms PANIC on this session rather than misbehave — the orphan [r]emove arm +// (pruneStateFilesForPath on a nil st) and finish with a queued override +// (Registry.Lookup on a nil reg) — so the orphan tests press only [k], [q] or +// EOF, nothing here calls finish (TestReconcile_FinishRunsExactlyOnce covers it +// end to end), and [w]rite-back, which needs a real ~/.agentsync, a plan and a +// destination, stays in reconcile_test.go. A test that needs home sets it. +// +// The reader is NOT a fake: it is the same *bufio.Reader production wraps stdin +// in, over a strings.Reader, so readChar sees production's exact EOF behaviour. +func newTestSession(t *testing.T, stdin string) (*reconcileSession, *strings.Builder) { + t.Helper() + var out strings.Builder + p := ui.New(&out, &out, ui.ColorNever) + return &reconcileSession{ + p: p, + w: p.Out, + br: bufio.NewReader(strings.NewReader(stdin)), + writtenSources: map[string][]byte{}, + dedupOverride: map[string]bool{}, + }, &out +} + +// driftItem is a minimal actionable, non-orphan item: drift class, a distinct +// path, and no text (so the prompt shows the hash fallback and reads nothing). +func driftItem(path string) reconcileItem { + return reconcileItem{ + agentName: "claude", + op: adapter.FileOp{Path: path}, + cls: drift.Drift, + hsrc: "aaaa", hdest: "bbbb", + } +} + +// TestParseItemKey pins the per-item hotkey table, including the two case +// foldings the prompt deliberately does NOT do. +// +// The old loop matched `case 'w','W','o','O','s','S','i','q','Q'` and only then +// folded with `ch | 0x20`, so 'I' and 'D' fell to the ignore-and-re-read +// default by OMISSION. Folding every byte uniformly would silently add two +// accepted keystrokes — one of them a bulk [I]gnore that has no confirmation +// step at all, which is exactly the "stray capital wipes the queue" failure the +// bulk confirmation (#155) exists to prevent. The asymmetry is behaviour; this +// table is where it is written down. +func TestParseItemKey(t *testing.T) { + tests := []struct { + name string + ch byte + wantAct reconcileAction + wantBulk bool + wantDiff bool + wantOK bool + }{ + {name: "w write-back", ch: 'w', wantAct: actionWriteBack, wantOK: true}, + {name: "W bulk write-back", ch: 'W', wantAct: actionWriteBack, wantBulk: true, wantOK: true}, + {name: "o override", ch: 'o', wantAct: actionOverride, wantOK: true}, + {name: "O bulk override", ch: 'O', wantAct: actionOverride, wantBulk: true, wantOK: true}, + {name: "s skip", ch: 's', wantAct: actionSkip, wantOK: true}, + {name: "S bulk skip", ch: 'S', wantAct: actionSkip, wantBulk: true, wantOK: true}, + {name: "i ignore", ch: 'i', wantAct: actionIgnore, wantOK: true}, + {name: "q quit", ch: 'q', wantAct: actionQuit, wantOK: true}, + {name: "Q quit folds", ch: 'Q', wantAct: actionQuit, wantOK: true}, + {name: "d diff", ch: 'd', wantAct: actionNone, wantDiff: true, wantOK: true}, + // The two deliberate non-foldings, and the reason this table exists. + {name: "I is NOT a bulk ignore", ch: 'I'}, + {name: "D is NOT a diff", ch: 'D'}, + {name: "unknown letter", ch: 'x'}, + {name: "digit", ch: '7'}, + {name: "NUL", ch: 0}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + act, bulk, diff, ok := parseItemKey(tc.ch) + if act != tc.wantAct || bulk != tc.wantBulk || diff != tc.wantDiff || ok != tc.wantOK { + t.Errorf("parseItemKey(%q) = (%v, bulk=%v, diff=%v, ok=%v), want (%v, bulk=%v, diff=%v, ok=%v)", + tc.ch, act, bulk, diff, ok, tc.wantAct, tc.wantBulk, tc.wantDiff, tc.wantOK) + } + }) + } +} + +// TestBulkTargets pins the bulk-confirm count as the TRUE blast radius (#155): +// the items from the current one forward that a bulk choice would act on. Items +// already answered are behind the slice; a non-actionable item is not swept; and +// an orphan is never swept, because it has its own r/k prompt. +// +// A count of len(rest) passes the pre-existing end-to-end coverage (three +// drifted servers, all actionable, none orphaned — measured), so the mixed rows +// below are the ones that make the assertion mean anything. +func TestBulkTargets(t *testing.T) { + orphan := driftItem("/o") + orphan.orphan = true + clean := driftItem("/c") + clean.cls = drift.Clean + + tests := []struct { + name string + rest []reconcileItem + want int + }{ + {name: "empty queue", rest: nil, want: 0}, + {name: "all actionable", rest: []reconcileItem{driftItem("/a"), driftItem("/b")}, want: 2}, + {name: "orphans are never swept", rest: []reconcileItem{driftItem("/a"), orphan}, want: 1}, + {name: "non-actionable classes do not count", rest: []reconcileItem{driftItem("/a"), clean}, want: 1}, + {name: "only orphans", rest: []reconcileItem{orphan}, want: 0}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := bulkTargets(tc.rest); got != tc.want { + t.Errorf("bulkTargets = %d, want %d", got, tc.want) + } + }) + } +} + +// TestPromptItem_EOFStopsThePass pins the first of the three EOF exits: the +// input ends while the per-item prompt is waiting. The pass must STOP (so the +// caller reaches finish and flushes queued overrides + pruned state — #171) +// rather than treat the item as answered. +func TestPromptItem_EOFStopsThePass(t *testing.T) { + s, out := newTestSession(t, "") + it := driftItem("/dest/a") + act, stop := s.promptItem(it, []reconcileItem{it}) + if !stop { + t.Errorf("promptItem at EOF: stop = false, want true — the pass must end, not fall through to the next item") + } + if act != actionNone { + t.Errorf("promptItem at EOF: action = %v, want %v — an unanswered item must not resolve to anything", act, actionNone) + } + if !strings.Contains(out.String(), "[w]rite-back") { + t.Errorf("the prompt itself should still have been printed; got:\n%s", out.String()) + } +} + +// TestPromptItem_EOFMidBulkConfirmStopsThePass pins the exit the issue calls out +// as unreachable from a test before the session existed: the input ends AFTER a +// capital W/O/S has printed the confirmation prompt but BEFORE the y/N answer. +// +// Two things are asserted, and the second is the subtle one: the transcript must +// end at "[y/N] " with no echoed character, because the echo is written only +// after the read succeeds. Treating an EOF as a declined confirmation would +// print a stray NUL and keep going. +func TestPromptItem_EOFMidBulkConfirmStopsThePass(t *testing.T) { + s, out := newTestSession(t, "W") + it := driftItem("/dest/a") + act, stop := s.promptItem(it, []reconcileItem{it, driftItem("/dest/b")}) + if !stop { + t.Errorf("promptItem at EOF mid-confirm: stop = false, want true") + } + if act != actionNone { + t.Errorf("promptItem at EOF mid-confirm: action = %v, want %v", act, actionNone) + } + if s.bulk != actionNone { + t.Errorf("an unconfirmed bulk choice must not be recorded; s.bulk = %v", s.bulk) + } + got := out.String() + if !strings.Contains(got, "apply 'w' to all 2 remaining items? [y/N] ") { + t.Errorf("the confirmation prompt should name the action and the blast radius; got:\n%s", got) + } + if !strings.HasSuffix(got, "[y/N] ") { + t.Errorf("the transcript must end at the unanswered confirmation — nothing is echoed for a read that failed; got:\n%q", got) + } +} + +// TestPromptOrphan_EOFStopsThePass pins the third EOF exit, on the orphan +// remove/keep prompt. Same contract as the item prompt: stop the pass so finish +// still persists a removal made earlier in the same run. +func TestPromptOrphan_EOFStopsThePass(t *testing.T) { + s, out := newTestSession(t, "") + it := driftItem("/dest/skills/demo/SKILL.md") + it.orphan = true + if stop := s.promptOrphan(it); !stop { + t.Error("promptOrphan at EOF: stop = false, want true") + } + if s.stateDirty { + t.Error("an unanswered orphan prompt must not mark state dirty") + } + if !strings.Contains(out.String(), "[r]emove") { + t.Errorf("the orphan prompt should still have been printed; got:\n%s", out.String()) + } +} + +// TestPromptOrphan_QuitStopsThePass is the orphan prompt's [q]uit exit, the +// second of the two quits. It prints "quit" (the EOF exit does not) and stops. +func TestPromptOrphan_QuitStopsThePass(t *testing.T) { + s, out := newTestSession(t, "q") + it := driftItem("/dest/skills/demo/SKILL.md") + it.orphan = true + if stop := s.promptOrphan(it); !stop { + t.Error("promptOrphan on [q]: stop = false, want true") + } + if !strings.Contains(out.String(), "quit") { + t.Errorf("[q] at the orphan prompt should print quit; got:\n%s", out.String()) + } +} + +// TestWalk_QuitLeavesRemainingItemsUnprompted pins that [q]uit ENDS the pass. +// The observable is the transcript: the third item is never printed at all. +// +// The pre-existing end-to-end quit tests each have a single drifted item, so +// "quit" appearing in the output is true whether or not the walk stops — +// measured: making applyAction's quit arm return false failed zero tests. +func TestWalk_QuitLeavesRemainingItemsUnprompted(t *testing.T) { + s, out := newTestSession(t, "sq") + s.walk([]reconcileItem{driftItem("/dest/a"), driftItem("/dest/b"), driftItem("/dest/c")}) + got := out.String() + if n := strings.Count(got, "[w]rite-back"); n != 2 { + t.Errorf("after [s] then [q] the walk must stop: %d item prompts, want 2\n%s", n, got) + } + if strings.Contains(got, "/dest/c") { + t.Errorf("the item after [q]uit must never be prompted; got:\n%s", got) + } +} + +// TestWalk_ConfirmedBulkSkipsLaterPrompts pins the other half of the bulk state +// machine: once confirmed, the choice is recorded ON THE SESSION and every later +// item is resolved without a prompt. +// +// [S]kip is used deliberately: applying it touches nothing, so the assertion is +// purely about the prompt count. Dropping the `s.bulk = act` assignment — the +// bug this pins — leaves the confirmation working for the current item only and +// re-prompts the rest; measured against the pre-existing suite, it failed zero +// tests. +func TestWalk_ConfirmedBulkSkipsLaterPrompts(t *testing.T) { + s, out := newTestSession(t, "Sy") + s.walk([]reconcileItem{driftItem("/dest/a"), driftItem("/dest/b"), driftItem("/dest/c")}) + got := out.String() + if n := strings.Count(got, "[w]rite-back"); n != 1 { + t.Errorf("a confirmed bulk choice must prompt exactly once: %d prompts\n%s", n, got) + } + if s.bulk != actionSkip { + t.Errorf("s.bulk = %v, want %v — the confirmed choice must persist for the rest of the queue", s.bulk, actionSkip) + } + if !strings.Contains(got, "apply 's' to all 3 remaining items?") { + t.Errorf("the confirmation should name the action and the full remaining count; got:\n%s", got) + } +} + +// TestApplyAction_OnlyQuitStopsThePass pins the applyAction contract each walk +// exit depends on: quit stops, and nothing else does. The override row also +// asserts what "queues" means — one overrideOp per distinct agent+path, so a +// second [o] on the same item (two pointers inside one merge file) does not +// re-apply the file twice (dedupOverride). +func TestApplyAction_OnlyQuitStopsThePass(t *testing.T) { + tests := []struct { + name string + action reconcileAction + wantStop bool + wantQueue int // overrideOps after applying the action twice to the same item + }{ + {name: "skip", action: actionSkip}, + {name: "override queues", action: actionOverride, wantQueue: 1}, + {name: "quit", action: actionQuit, wantStop: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + s, _ := newTestSession(t, "") + it := driftItem("/dest/a") + if got := s.applyAction(it, tc.action); got != tc.wantStop { + t.Errorf("applyAction(%v) = %v, want %v", tc.action, got, tc.wantStop) + } + if tc.action == actionOverride && len(s.overrideOps) != 1 { + t.Fatalf("after one [o]: %d queued override ops, want 1", len(s.overrideOps)) + } + // The same item a second time: nothing new may be queued, and + // stop must not change. + if got := s.applyAction(it, tc.action); got != tc.wantStop { + t.Errorf("second applyAction(%v) = %v, want %v", tc.action, got, tc.wantStop) + } + if len(s.overrideOps) != tc.wantQueue { + t.Errorf("after applying %v twice: %d queued override ops, want %d", tc.action, len(s.overrideOps), tc.wantQueue) + } + }) + } +} + +// TestApplyAction_OverrideDedupsByAgentAndPath pins the dedup key: [o] twice +// on the same agent's item queues one re-apply of that path, but another +// agent's item at the same path is a different re-apply and must queue too. +func TestApplyAction_OverrideDedupsByAgentAndPath(t *testing.T) { + s, _ := newTestSession(t, "") + it := driftItem("/dest/a") + s.applyAction(it, actionOverride) + s.applyAction(it, actionOverride) + if len(s.overrideOps) != 1 { + t.Fatalf("after [o] twice on one item: %d queued, want 1", len(s.overrideOps)) + } + if oo := s.overrideOps[0]; oo.agentName != "claude" || oo.op.Path != "/dest/a" { + t.Errorf("queued override = %+v, want claude's op for /dest/a", oo) + } + other := it + other.agentName = "opencode" + s.applyAction(other, actionOverride) + if len(s.overrideOps) != 2 { + t.Errorf("after a second agent's [o] on the same path: %d queued, want 2", len(s.overrideOps)) + } +} + +// wantItemMenu is deliberately a COPY of the production itemMenu, not a +// reference to it: the tests below count it to see how many times the user was +// (re)prompted, and a change to the menu's wording should fail here rather +// than be absorbed by a constant both sides share. +const wantItemMenu = " [w]rite-back [o]verride [s]kip [i]gnore [d]iff [q]uit\n > " + +// TestPromptItem_DiffReprintsValuesAndMenu pins the [d]iff arm: it re-renders +// the item's values and the menu, echoes nothing, chooses no action, and the +// next key still decides the item. Measured on the pre-#232 tree and on commit +// 1: deleting the re-render failed zero tests — the arm was pinned only by the +// scripted-stdin harness. +func TestPromptItem_DiffReprintsValuesAndMenu(t *testing.T) { + s, out := newTestSession(t, "ds") + it := driftItem("/dest/a") + act, stop := s.promptItem(it, []reconcileItem{it}) + if stop || act != actionSkip { + t.Fatalf("promptItem = (%v, stop=%v), want (%v, false): [d] must not choose, the [s] after it must", act, stop, actionSkip) + } + got := out.String() + if n := strings.Count(got, wantItemMenu); n != 2 { + t.Errorf("menu printed %d time(s), want 2 (once before [d], once after); transcript:\n%s", n, got) + } + if n := strings.Count(got, " destination: "); n != 2 { + t.Errorf("values rendered %d time(s), want 2; transcript:\n%s", n, got) + } + if !strings.HasSuffix(got, " > s\n") { + t.Errorf("[d] is not echoed and only the deciding key is; the transcript must end at the echoed s, got:\n%q", got) + } +} + +// TestPromptItem_DeclinedBulkDoesNotReprintMenu pins the cancel path of the +// bulk confirmation: [N] prints "cancelled; choose a per-item action", records +// no bulk choice, and reads the next key WITHOUT re-printing the menu — the +// user is still at the same prompt, not a new one. +func TestPromptItem_DeclinedBulkDoesNotReprintMenu(t *testing.T) { + s, out := newTestSession(t, "Wns") + it := driftItem("/dest/a") + act, stop := s.promptItem(it, []reconcileItem{it, driftItem("/dest/b")}) + if stop || act != actionSkip { + t.Fatalf("promptItem = (%v, stop=%v), want (%v, false)", act, stop, actionSkip) + } + if s.bulk != actionNone { + t.Errorf("a declined bulk choice must not be recorded; s.bulk = %v", s.bulk) + } + got := out.String() + want := "apply 'w' to all 2 remaining items? [y/N] n\n cancelled; choose a per-item action\ns\n" + if !strings.HasSuffix(got, want) { + t.Errorf("after declining, the next key is read at the same prompt with no menu re-print; transcript must end %q, got:\n%q", want, got) + } + if n := strings.Count(got, wantItemMenu); n != 1 { + t.Errorf("menu printed %d time(s), want exactly 1; transcript:\n%s", n, got) + } +} + +// TestPromptItem_UnknownKeyIsIgnored pins what the prompt does with a byte +// parseItemKey rejects: nothing — no echo, no re-prompt, just the next read. The +// byte is a capital I, so this also pins that "not a bulk ignore" means ignored +// as a keystroke, not accepted as something else. +func TestPromptItem_UnknownKeyIsIgnored(t *testing.T) { + s, out := newTestSession(t, "Is") + it := driftItem("/dest/a") + act, stop := s.promptItem(it, []reconcileItem{it}) + if stop || act != actionSkip { + t.Fatalf("promptItem = (%v, stop=%v), want (%v, false)", act, stop, actionSkip) + } + if got := out.String(); !strings.HasSuffix(got, wantItemMenu+"s\n") { + t.Errorf("an unknown key must leave the transcript untouched until a known one arrives; want it to end with the menu then the echoed s, got:\n%q", got) + } +} + +// TestWalk_BulkNeverSweepsOrphans pins that a confirmed bulk choice acts on +// the remaining actionable items only: an orphan in the queue still gets its +// own remove/keep prompt, and the confirmation's count excludes it. +func TestWalk_BulkNeverSweepsOrphans(t *testing.T) { + s, out := newTestSession(t, "Syk") + orphan := driftItem("/dest/b") + orphan.orphan = true + s.walk([]reconcileItem{driftItem("/dest/a"), orphan, driftItem("/dest/c")}) + got := out.String() + if s.bulk != actionSkip { + t.Fatalf("s.bulk = %v, want %v", s.bulk, actionSkip) + } + if !strings.Contains(got, "apply 's' to all 2 remaining items? [y/N] y\n") { + t.Errorf("the blast radius must count the two drift items and not the orphan; transcript:\n%s", got) + } + if n := strings.Count(got, wantItemMenu); n != 1 { + t.Errorf("item menu printed %d time(s), want 1 — the bulk choice answers /dest/c; transcript:\n%s", n, got) + } + if !strings.Contains(got, " kept: /dest/b\n") { + t.Errorf("the orphan must still be prompted and [k]ept; transcript:\n%s", got) + } +} + +// TestApplyAction_IgnoreAppendsToIgnoreFile pins the [i]gnore arm: the item's +// RAW label lands in ignore.toml under home and the transcript says so. +// Measured on commit 1: deleting the append and the print failed zero tests. +// home is the one piece of wiring this test sets, because appendIgnore writes +// under it; nothing else here needs a filesystem. +func TestApplyAction_IgnoreAppendsToIgnoreFile(t *testing.T) { + s, out := newTestSession(t, "") + s.home = t.TempDir() + it := driftItem("/dest/.claude.json") + it.ptr = "/mcpServers/demo" + if stop := s.applyAction(it, actionIgnore); stop { + t.Fatal("applyAction(actionIgnore) = stop, want the pass to continue") + } + data, err := os.ReadFile(filepath.Join(s.home, "ignore.toml")) + if err != nil { + t.Fatalf("ignore.toml not written: %v", err) + } + if want := "ignore = \"/dest/.claude.json#/mcpServers/demo\"\n"; string(data) != want { + t.Errorf("ignore.toml = %q, want %q", data, want) + } + if got, want := out.String(), " ignored: /dest/.claude.json#/mcpServers/demo\n"; got != want { + t.Errorf("transcript = %q, want %q", got, want) + } +} + +// TestNewReconcileSession_RejectsMultipleAutoModes pins that the constructor, +// not its caller, enforces reconcileAuto's "at most one mode" invariant, so no +// session with two modes can be built (resolveAuto's switch would otherwise +// let writeBack win silently, the data-loss shape the check exists to +// prevent). That the check runs BEFORE anything loads is +// TestNewReconcileSession_ChecksModesBeforeLoading's job: measured, a check +// moved below the loads still passes this test against the empty temp home +// set up here, which only keeps a regressed constructor away from the real +// ~/.agentsync. +func TestNewReconcileSession_RejectsMultipleAutoModes(t *testing.T) { + // A constructor that regressed to loading BEFORE checking would read the + // ambient ~/.agentsync; point both home lookups at an empty temp dir so + // that regression fails against nothing real. + root := t.TempDir() + t.Setenv("AGENTSYNC_TARGET_ROOT", root) + t.Setenv("AGENTSYNC_HOME", filepath.Join(root, ".agentsync")) + for _, auto := range []reconcileAuto{ + {writeBack: true, override: true}, + {writeBack: true, safe: true}, + {override: true, safe: true}, + {writeBack: true, override: true, safe: true}, + } { + s, items, err := newReconcileSession(&cobra.Command{}, strings.NewReader(""), auto, "") + if err == nil || !strings.Contains(err.Error(), "mutually exclusive") { + t.Errorf("newReconcileSession(%+v) error = %v, want the mutually-exclusive error", auto, err) + } + if s != nil || items != nil { + t.Errorf("newReconcileSession(%+v) returned a session or items alongside the error", auto) + } + } +} + +// TestNewReconcileSession_ChecksModesBeforeLoading is the source-text guard for +// the constructor doc's "as its first step, before it loads anything": the +// mutual-exclusion check must precede every load in newReconcileSession, so a +// bad flag combination costs no I/O and is the FIRST error the user sees even +// when the source itself is unloadable. Measured before this guard existed: +// moving the check below loadProjectedForScope left +// TestNewReconcileSession_RejectsMultipleAutoModes passing — the ordering is +// behaviour that nothing else pins. Same shape as the apply pipeline's +// TestApplyPipelineLoadsStateAfterSourceReload. +func TestNewReconcileSession_ChecksModesBeforeLoading(t *testing.T) { + body := funcBody(readFileForGuard(t, repoRootFromCaller(t), "internal/cli/reconcile.go"), "func newReconcileSession(") + if body == "" { + t.Fatal("newReconcileSession not found in reconcile.go") + } + // The capture must end at the constructor's top-level return of the + // session literal; anything else means funcBody was cut short above the + // calls this guard orders. + if !strings.HasSuffix(body, "\t}, items, nil") { + t.Fatalf("funcBody captured a truncated newReconcileSession (the body does not end at the "+ + "`}, items, nil` return); tail: %q", body[len(body)-min(80, len(body)):]) + } + check := strings.Index(body, "b2i(auto.writeBack)") + if check < 0 { + t.Fatal("newReconcileSession no longer checks the auto modes with b2i(auto.writeBack); update this guard") + } + for _, load := range []string{ + "loadProjectedForScope(", "newPrinter(", "state.Load(", "registryFactory()", "selectAgents(", "render.Plan(", + } { + at := strings.Index(body, load) + if at < 0 { + t.Errorf("newReconcileSession no longer calls %s; update this guard", load) + continue + } + if at < check { + t.Errorf("%s runs before the --auto-* mutual-exclusion check; the check must be the constructor's first step", load) + } + } +} diff --git a/internal/cli/reconcile_test.go b/internal/cli/reconcile_test.go index 6aa43459..1a58252b 100644 --- a/internal/cli/reconcile_test.go +++ b/internal/cli/reconcile_test.go @@ -1,12 +1,15 @@ package cli_test import ( + "encoding/json" "io" "os" "path/filepath" "strings" "testing" + "github.com/spxrogers/agentsync/internal/paths" + "github.com/spxrogers/agentsync/internal/state" "github.com/spxrogers/agentsync/internal/ui" ) @@ -21,9 +24,14 @@ func TestReconcile_OrphanFile(t *testing.T) { env = map[string]string{"AGENTSYNC_TARGET_ROOT": tmp} mustRun(t, env, "init") mustRun(t, env, "agent", "add", "claude") - skill := filepath.Join(tmp, ".agentsync", "skills", "demo", "SKILL.md") - _ = os.MkdirAll(filepath.Dir(skill), 0o755) - _ = os.WriteFile(skill, []byte("---\nname: demo\ndescription: d\n---\nbody\n"), 0o644) + // Two skills: demo becomes the orphan; insync stays in sync, so its + // state entry must survive the orphan's prune — a wiped state file + // would pass a "demo is gone" check for the wrong reason. + for _, name := range []string{"demo", "insync"} { + skill := filepath.Join(tmp, ".agentsync", "skills", name, "SKILL.md") + _ = os.MkdirAll(filepath.Dir(skill), 0o755) + _ = os.WriteFile(skill, []byte("---\nname: "+name+"\ndescription: d\n---\nbody\n"), 0o644) + } mustRun(t, env, "apply") dest = filepath.Join(tmp, ".claude", "skills", "demo", "SKILL.md") if _, err := os.Stat(dest); err != nil { @@ -36,6 +44,29 @@ func TestReconcile_OrphanFile(t *testing.T) { t.Run("remove backs up and deletes", func(t *testing.T) { env, dest := setup(t) + // apply recorded the dest in state.Files; [r] must prune that entry AND + // the run must persist the prune, or the next apply still believes it + // owns a file that is gone (issue #171). Measured before this assertion + // existed: dropping the prune's stateDirty flag failed zero tests. + root := env["AGENTSYNC_TARGET_ROOT"] + insync := filepath.Join(root, ".claude", "skills", "insync", "SKILL.md") + stateOwns := func(p string) bool { + t.Helper() + st, err := state.Load(filepath.Join(root, ".agentsync", ".state", "targets.json")) + if err != nil { + t.Fatalf("load state: %v", err) + } + portable := paths.HomeRelative(root, p) + for key := range st.Files { + if key.Path == portable { + return true + } + } + return false + } + if !stateOwns(dest) || !stateOwns(insync) { + t.Fatal("precondition: apply should have recorded both skills in state.Files") + } out, err := runCLIWithStdin(t, env, "r", "reconcile") if err != nil { t.Fatalf("reconcile: %v\n%s", err, out) @@ -43,6 +74,12 @@ func TestReconcile_OrphanFile(t *testing.T) { if _, err := os.Stat(dest); !os.IsNotExist(err) { t.Fatalf("orphan dest should have been removed; stat err=%v\n%s", err, out) } + if stateOwns(dest) { + t.Fatalf("state still owns the removed orphan: the prune was not persisted\n%s", out) + } + if !stateOwns(insync) { + t.Fatalf("the prune must be exact: the in-sync sibling's state entry is gone too\n%s", out) + } // A backup of the removed file must exist. backups := filepath.Join(env["AGENTSYNC_TARGET_ROOT"], ".agentsync", ".state", "backups") found := false @@ -565,3 +602,256 @@ func TestReconcile_EOFFlushesOverrides(t *testing.T) { t.Fatalf("EOF after [o]verride dropped the queued override (source value not restored):\n%s", final) } } + +// TestReconcile_FinishRunsExactlyOnce is the behavioural half of the #232 +// decision that the run's tail (finish) has exactly ONE call site and is never +// deferred. finish prints one summary line per run — "N item(s) left +// unresolved" in an auto mode, "override: applied N item(s)" after an +// [o]verride — so the observable trace of a finish that ran twice (a defer plus +// the explicit call, or a second call added later) is exactly a duplicated line. +// +// Three exits are covered: the natural end of an --auto-safe pass, an EOF after +// [o]verride, and [q]uit after [o]verride. The last one also pins that quitting +// still APPLIES the queued override: both quits reach finish exactly as the +// EOFs do (TestReconcile_EOFFlushesOverrides), but no test or scripted scenario +// ever queued an override and then quit, so a quit that dropped the queue +// failed nothing. +func TestReconcile_FinishRunsExactlyOnce(t *testing.T) { + setup := func(t *testing.T) (env map[string]string, dst string) { + t.Helper() + tmp := t.TempDir() + env = map[string]string{"AGENTSYNC_TARGET_ROOT": tmp} + mustRun(t, env, "init") + mustRun(t, env, "agent", "add", "claude") + for _, name := range []string{"aaa", "bbb"} { + mcp := filepath.Join(tmp, ".agentsync", "mcp", name+".toml") + _ = os.MkdirAll(filepath.Dir(mcp), 0o755) + _ = os.WriteFile(mcp, []byte("[server]\ntype=\"stdio\"\ncommand=\"npx\"\n"), 0o644) + } + mustRun(t, env, "apply") + // Drift BOTH servers in the destination (npx -> npm). + dst = filepath.Join(tmp, ".claude.json") + body, _ := os.ReadFile(dst) + if err := os.WriteFile(dst, []byte(strings.ReplaceAll(string(body), `"npx"`, `"npm"`)), 0o644); err != nil { + t.Fatal(err) + } + return env, dst + } + // commandOf reads mcpServers..command from the claude destination. + commandOf := func(t *testing.T, dst, id string) string { + t.Helper() + var doc struct { + MCPServers map[string]struct { + Command string `json:"command"` + } `json:"mcpServers"` + } + body, err := os.ReadFile(dst) + if err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(body, &doc); err != nil { + t.Fatalf("parse %s: %v\n%s", dst, err, body) + } + return doc.MCPServers[id].Command + } + + t.Run("auto-safe prints the unresolved summary once", func(t *testing.T) { + env, _ := setup(t) + out, err := runCLI(t, env, "reconcile", "--auto-safe") + if err != nil { + t.Fatalf("reconcile --auto-safe: %v\n%s", err, out) + } + if n := strings.Count(out, "item(s) left unresolved"); n != 1 { + t.Fatalf("the unresolved summary must be printed exactly once (finish ran %d times):\n%s", n, out) + } + if !strings.Contains(out, "2 item(s) left unresolved") { + t.Fatalf("both drifted servers should be counted; got:\n%s", out) + } + }) + + t.Run("EOF after override applies the queue once", func(t *testing.T) { + env, dst := setup(t) + out, err := runCLIWithStdin(t, env, "o", "reconcile") + if err != nil { + t.Fatalf("reconcile: %v\n%s", err, out) + } + if n := strings.Count(out, "override: applied"); n != 1 { + t.Fatalf("the override summary must be printed exactly once (finish ran %d times):\n%s", n, out) + } + if got := commandOf(t, dst, "aaa"); got != "npx" { + t.Fatalf("the queued override was not applied at EOF: aaa.command = %q, want npx", got) + } + }) + + t.Run("quit after override still applies the queue once", func(t *testing.T) { + env, dst := setup(t) + out, err := runCLIWithStdin(t, env, "oq", "reconcile") + if err != nil { + t.Fatalf("reconcile: %v\n%s", err, out) + } + if !strings.Contains(out, "quit") { + t.Fatalf("expected the [q]uit echo; got:\n%s", out) + } + if n := strings.Count(out, "override: applied 1 item(s)"); n != 1 { + t.Fatalf("[q]uit must still apply the ONE queued override, exactly once (got %d summary lines):\n%s", n, out) + } + // Only the first server is asserted: the queued op is the whole merge + // file's op (which is why dedupOverride keys by path), so the re-apply + // restores every owned key in .claude.json, not just aaa. + if got := commandOf(t, dst, "aaa"); got != "npx" { + t.Fatalf("[q]uit dropped the queued override: aaa.command = %q, want npx (restored from source)", got) + } + }) +} + +// TestReconcile_ProjectScope_OverrideRecordsProjectState pins the scope and +// project root that finish hands to render.NewWriter and RecordOpsState. Every +// other reconcile test runs at user scope, where a transposition to +// ScopeUser/"" is invisible; at project scope it makes the writer look the +// destination up under the wrong state key, treat the project's own .mcp.json +// as a never-applied foreign file, back it up and print a "backup:" line, and +// record the re-apply against the wrong scope. +func TestReconcile_ProjectScope_OverrideRecordsProjectState(t *testing.T) { + tmpHome := t.TempDir() + projectDir := t.TempDir() + env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmpHome} + mustRun(t, env, "init") + mustRun(t, env, "agent", "add", "claude") + mustRun(t, env, "init", "--scope", "project", "--project", projectDir) + declareProjectAgent(t, env, projectDir, "claude") + scaffoldProjectMCP(t, projectDir, "github", "npx") + mustRun(t, env, "apply", "--project", projectDir) + + mcpPath := filepath.Join(projectDir, ".mcp.json") + body, err := os.ReadFile(mcpPath) + if err != nil { + t.Fatalf("read project .mcp.json: %v", err) + } + drifted := strings.ReplaceAll(string(body), `"npx"`, `"npm"`) + if drifted == string(body) { + t.Fatalf("fixture did not contain the expected command to drift:\n%s", body) + } + if err := os.WriteFile(mcpPath, []byte(drifted), 0o644); err != nil { + t.Fatal(err) + } + + out, err := runCLIWithStdin(t, env, "o", "reconcile", "--project", projectDir) + if err != nil { + t.Fatalf("reconcile --project: %v\n%s", err, out) + } + if !strings.Contains(out, "override: applied 1 item(s)") { + t.Fatalf("expected the override to be applied; got:\n%s", out) + } + if strings.Contains(out, "backup:") { + t.Fatalf("the project's own destination was treated as a foreign collision (wrong scope in finish):\n%s", out) + } + if final, _ := os.ReadFile(mcpPath); !strings.Contains(string(final), `"npx"`) { + t.Fatalf("override did not restore the project source value:\n%s", final) + } + for _, backups := range []string{ + filepath.Join(tmpHome, ".agentsync", ".state", "backups"), + filepath.Join(projectDir, ".agentsync", ".state", "backups"), + } { + if _, err := os.Stat(backups); !os.IsNotExist(err) { + t.Fatalf("no backup may be taken for an owned project destination; found %s (stat err=%v)", backups, err) + } + } + // The re-apply must be recorded against the project's state, so the tree + // reads clean at project scope afterwards. + if out, err := runCLI(t, env, "status", "--project", projectDir, "--exit-code"); err != nil { + t.Fatalf("status --project after override should be clean: %v\n%s", err, out) + } +} + +// TestReconcile_DroppedServer_WriteBackRemovesSource pins the deletion-only +// exception to the capture funnel, removeDroppedSource, on each of the three +// routes the secret-handling docs name for it: a per-item [w], a confirmed +// bulk [W], and --auto-writeback. Each must unlink the canonical mcp/.toml +// of the server the destination dropped, while the drifted sibling is written +// back normally. Before this test the function had no in-repo coverage at all; +// only the out-of-tree scripted-stdin harness reached it. +func TestReconcile_DroppedServer_WriteBackRemovesSource(t *testing.T) { + setup := func(t *testing.T) (env map[string]string, srcDir string) { + t.Helper() + tmp := t.TempDir() + env = map[string]string{"AGENTSYNC_TARGET_ROOT": tmp} + mustRun(t, env, "init") + mustRun(t, env, "agent", "add", "claude") + srcDir = filepath.Join(tmp, ".agentsync", "mcp") + _ = os.MkdirAll(srcDir, 0o755) + for _, name := range []string{"dropped", "kept"} { + _ = os.WriteFile(filepath.Join(srcDir, name+".toml"), []byte("[server]\ntype=\"stdio\"\ncommand=\"npx\"\n"), 0o644) + } + mustRun(t, env, "apply") + // The destination drops one server outright and drifts the other. + dst := filepath.Join(tmp, ".claude.json") + body, err := os.ReadFile(dst) + if err != nil { + t.Fatal(err) + } + var doc map[string]any + if err := json.Unmarshal(body, &doc); err != nil { + t.Fatalf("parse %s: %v\n%s", dst, err, body) + } + servers, _ := doc["mcpServers"].(map[string]any) + if servers == nil || servers["dropped"] == nil || servers["kept"] == nil { + t.Fatalf("apply did not render both servers into %s:\n%s", dst, body) + } + delete(servers, "dropped") + servers["kept"].(map[string]any)["command"] = "npm" + out, err := json.Marshal(doc) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(dst, out, 0o644); err != nil { + t.Fatal(err) + } + return env, srcDir + } + + tests := []struct { + name string + stdin string + args []string + // want is a transcript line only this route prints; noPrompt is the + // pin for the route that prints nothing route-specific — an auto mode + // never asks, so the prompt marker must be absent. + want string + noPrompt bool + }{ + {name: "per-item [w]", stdin: "ww", want: " > w\n"}, + {name: "confirmed bulk [W]", stdin: "Wy", want: "apply 'w' to all 2 remaining items? [y/N] y\n"}, + // The [q] on stdin must never be read: a flag that regressed into the + // interactive pass would quit at the first prompt and fail the shared + // assertions below, instead of blocking on a terminal's stdin. + {name: "--auto-writeback", stdin: "q", args: []string{"--auto-writeback"}, noPrompt: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + env, srcDir := setup(t) + out, err := runCLIWithStdin(t, env, tc.stdin, append([]string{"reconcile"}, tc.args...)...) + if err != nil { + t.Fatalf("reconcile: %v\n%s", err, out) + } + if tc.want != "" && !strings.Contains(out, tc.want) { + t.Fatalf("transcript should show the %s route; want %q in:\n%s", tc.name, tc.want, out) + } + if tc.noPrompt && strings.Contains(out, " > ") { + t.Fatalf("%s must not prompt; transcript:\n%s", tc.name, out) + } + if !strings.Contains(out, "write-back: removed source mcp/dropped.toml (destination dropped ") { + t.Errorf("the dropped server's source removal must be reported; transcript:\n%s", out) + } + if _, err := os.Stat(filepath.Join(srcDir, "dropped.toml")); !os.IsNotExist(err) { + t.Errorf("mcp/dropped.toml should have been unlinked; stat err = %v\n%s", err, out) + } + kept, err := os.ReadFile(filepath.Join(srcDir, "kept.toml")) + if err != nil { + t.Fatalf("mcp/kept.toml must survive: %v", err) + } + if !strings.Contains(string(kept), "npm") { + t.Errorf("the drifted sibling should have been written back to npm; kept.toml:\n%s", kept) + } + }) + } +}