diff --git a/CHANGELOG.md b/CHANGELOG.md index f3306617..0709f9de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -179,8 +179,32 @@ source layout, CLI surface, and state schema are stabilizing but may still chang The upgrade-notice probe's answer is unchanged, by design — a stricter answer there would fire a breaking-change banner at a brand-new user. +- **`reconcile --help` no longer says `--auto-safe` "auto-resolve[s] only + converged/pending/new".** It resolves nothing: every item that reaches + `reconcile` needs a human, and the flag reports each one as left unresolved. + The user guide, the daily-loop guide and the CLI reference said the same + wrong thing and are corrected with it. + ### Changed +- **`status --json`, `diff` and `reconcile` now list a shared file's merged keys + in a stable, sorted order.** The three walked `render.CollectPointers`' output + directly, which ranges a Go map, so with five MCP servers in one + `~/.claude.json` a `status --json` payload came back in a different key order + almost every run (measured: five distinct orderings across 200 calls), diff + hunks reordered, and `reconcile`'s prompt queue shuffled between runs. Merged + keys are now walked in ascending pointer order. No item is added, dropped or + reclassified; only the order changes — but a `status --json` diffed between + runs, or a `reconcile` transcript compared against a previous one, will be + stable for the first time. `explain` already sorted and is unchanged. + +- **Internal: `status`, `diff`, `reconcile` and `explain` now share one + plan→drift walk** ([#229](https://github.com/spxrogers/agentsync/issues/229)). + `explain` now decodes a key-merged destination once per rendered section + rather than once per key, so every key in one file is classified against the + same snapshot. The ways the four surfaces still disagree (mode-only drift, + symlinked destinations) are unchanged and tracked in #229. + - **`.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/docs/architecture.md b/docs/architecture.md index dcaddce2..cb6fdf4f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -828,8 +828,12 @@ without consulting ownership; and any ONE unowned, differing pointer elsewhere in an otherwise-owned file triggers a whole-file backup that, as a side effect, also preserves your owned, drifted keys alongside it. -`drift.SafeForAutoApply(class)` is what `reconcile --auto-safe` consults — it -auto-resolves only the cases that can't lose work (`converged`, `pending`). +`reconcile --auto-safe` resolves nothing on its own: every item that reaches its +loop needs manual review (`drift`, `conflict`, `foreign-collision`, +`orphan-drifted`, or an orphan), so it reports each one as skipped and leaves +orphans in place. `drift.SafeForAutoApply` names the classes that could be +resolved without losing work (`clean`, `pending`, `new`, `converged`) but has no +production caller today. **Orphan reclamation on `apply`.** `apply` itself reclaims two kinds of orphan so a removed component doesn't linger in the destination: emptied key-merge sections @@ -871,7 +875,22 @@ from the canonical source, which previously lingered until a `reconcile`. pointer**, so agentsync can own `$.mcpServers.github` inside `~/.claude.json` without touching keys it didn't write. Those untouched keys are **foreign keys** — surfaced in `status` but never entering the classifier. If a structured file -fails to parse, the algorithm degrades to file-level on the whole file. +fails to parse, the read degrades to an empty document: every key agentsync +owns in it classifies against an absent value — per pointer, never as one +file-level item. + +Every surface that classifies — `status`, `diff`, `reconcile`, `explain` — walks +the plan through one shared iterator, `walkPlanItems` in +`internal/cli/planwalk.go`, which composes `render.IsKeyMerge`, +`render.CollectPointers`, `render.OrphanFiles`, the guarded destination readers +and `drift.Classify`. Whole-file ops are deduped by destination path per agent; +key-merge ops never are, because one agent emits several of them to one file +(Codex's `/mcp_servers` and `/hooks` both land in `config.toml`). Merged keys +are walked in sorted pointer order, so `status --json`, `diff` and `reconcile` +list them reproducibly. Each surface keeps its own presentation on top: `status` +re-partitions whole-file rows ahead of key rows and folds permission drift into +the class, `diff` masks and compares text, `reconcile` excludes an orphan +another agent still renders, `explain` groups by owner. --- diff --git a/docs/components.md b/docs/components.md index 11237e28..e0a3b51a 100644 --- a/docs/components.md +++ b/docs/components.md @@ -44,7 +44,12 @@ The binary's `main`. Injects `Version`/`Commit`/`Date` via `-ldflags` and calls Wires every cobra subcommand into the root tree and dispatches to handlers; this is the only package that depends on nearly all the others. - **Key:** `NewRoot() *cobra.Command`, `Execute() int` (returns the process exit - code and owns the terminal `✗ ERROR` line), `Version`/`Commit`/`Date`. + code and owns the terminal `✗ ERROR` line), `Version`/`Commit`/`Date`; + `walkPlanItems` — the single plan→state→destination drift walk 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`. - **Commands:** `init`, `agent {add,remove,list,enable,disable}`, `apply`, `revert`, `status`, `diff`, `reconcile`, `import`, `doctor`, `check`, `mcp {add,remove,list,enable,disable}`, @@ -55,7 +60,8 @@ is the only package that depends on nearly all the others. `version`. - **Depends on:** adapter, source, state, secrets, paths, render, marketplace, project, drift, git, ui, log. -- **Files:** `root.go` + one file per command group. +- **Files:** `root.go` + one file per command group + shared helpers + (`destread.go`, `planwalk.go`). --- @@ -403,7 +409,7 @@ symmetric with the dest→source write boundary (see architecture §7). `reconcile --auto-override` (which re-applies through `Writer.Write`), `import ` and `doctor` (through its plugin check) still block on a non-regular destination — issues #241 and #242. -- **Depends on:** adapter, secrets, source, state, paths, iox, drift. +- **Depends on:** adapter, secrets, source, state, paths, iox. - **Files:** `pipeline.go`, `writer.go`, `state_apply.go`, `report.go`. ### `internal/capture` @@ -414,7 +420,9 @@ source-only fields, writes via `source.Write*`. Used by `import` and reconcile. - **Files:** `capture.go`, `leak_fixture.go` (compile-time leak guard). ### `internal/drift` -Pure 3-way classifier — no IO. +Pure 3-way classifier — no IO. The walk that feeds it (`walkPlanItems`) lives in +`internal/cli` beside the destination readers it needs, so this package stays +IO-free. - **Key:** `Class` (`Clean`, `Pending`, `Drift`, `Converged`, `Conflict`, `New`, `ForeignCollision`, `Orphan`, `OrphanDrifted`); `Classify(hsrc, happlied, hdest)`; `SafeForAutoApply(c)`. diff --git a/docs/user-guide.md b/docs/user-guide.md index b3f9a778..2e6c1ec2 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -260,8 +260,10 @@ Inside `reconcile`, for each drifting item: - **`i`** stop tracking this path (adds it to `~/.agentsync/ignore.toml`). - **`s`/`q`** skip / quit. -Scripting it? `--auto-writeback`, `--auto-override`, or `--auto-safe` (which only -auto-resolves changes that can't lose work). +Scripting it? `--auto-writeback`, `--auto-override`, or `--auto-safe`. The last +resolves nothing — every item that reaches `reconcile` needs a human — and +instead lists each one as left unresolved, so it works as a non-interactive +"does anything need me" check. --- diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 6e0ab399..349a94c8 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -10,6 +10,7 @@ import ( "github.com/sergi/go-diff/diffmatchpatch" "github.com/spf13/afero" "github.com/spf13/cobra" + "github.com/spxrogers/agentsync/internal/adapter" "github.com/spxrogers/agentsync/internal/paths" "github.com/spxrogers/agentsync/internal/render" "github.com/spxrogers/agentsync/internal/secrets" @@ -132,70 +133,7 @@ func newDiffCmd() *cobra.Command { // formatted diff or --json. Pretty rendering and JSON share the // same masked strings, so the secret-leak guards above protect // both modes. - var hunks []diffHunk - // filterMatched tracks whether a argument matched any rendered - // op across every selected agent, so a path that matches NOTHING (a - // typo, or an unmanaged file) can be reported distinctly from a managed - // path that is genuinely in sync ("no diff"). - filterMatched := filterPath == "" - for _, name := range reg.Names() { - res, ok := plan.PerAgent[name] - if !ok { - continue - } - seen := map[string]bool{} - for _, op := range res.Ops { - if filterPath != "" && op.Path != filterPath { - continue - } - filterMatched = true - if render.IsKeyMerge(op.MergeStrategy) { - // Key-level diff: compare per pointer. NOT deduped by path — - // one agent emits several key-merge ops to one file (codex - // writes /mcp_servers AND /hooks to config.toml; claude writes - // /hooks AND /lspServers to settings.json), each owning a - // distinct section, so every op must be walked. Deduping by - // path here dropped the second section's drift (status's key - // loop and the apply pipeline never path-dedup key-merge ops). - var ours map[string]interface{} - _ = json.Unmarshal(op.Content, &ours) - final := readDestFile(op.MergeStrategy, op.Path) - for _, ptr := range render.CollectPointers(ours, "") { - srcStr := secrets.MaskResolved(marshalPretty(getPointerValue(ours, ptr)), redact) - dstStr := secrets.MaskResolved(marshalPretty(getPointerValue(final, ptr)), redact) - if srcStr == dstStr { - continue - } - hunks = append(hunks, diffHunk{Path: op.Path, Pointer: ptr, Source: srcStr, Dest: dstStr}) - } - } else { - // File-level diff. - if seen[op.Path] { - continue - } - seen[op.Path] = true - srcStr := secrets.MaskResolved(string(op.Content), redact) - dstBytes, readErr := readDestBytes(op.Path) - dstStr := "" - if readErr == nil { - dstStr = secrets.MaskResolved(string(dstBytes), redact) - } - if srcStr == dstStr { - // Content is identical, but the file MODE may have drifted - // from what apply maintains (op.Mode). A content-identical - // chmod produces no text hunk, so surface it as a small - // "mode" hunk instead of reporting "no diff" — the mode - // analog of a content drift hunk (render.Writer.Write - // re-converges it on the next apply). - if src, dst, ok := modeHunk(op.Path, op.Mode); ok { - hunks = append(hunks, diffHunk{Path: op.Path, Pointer: "mode", Source: src, Dest: dst}) - } - continue - } - hunks = append(hunks, diffHunk{Path: op.Path, Source: srcStr, Dest: dstStr}) - } - } - } + hunks, filterMatched := collectDiffHunks(plan, reg.Names(), filterPath, redact) // A that matched no rendered op is a typo or an unmanaged file // — distinct from a managed path that is in sync ("no diff"). Fail with @@ -276,25 +214,18 @@ func renderDiffText(p *ui.Printer, diffs []diffmatchpatch.Diff) string { } // modeHunk describes a permission-bit mismatch between the mode apply would -// maintain for path (wantMode, from op.Mode) and the file's current perm on -// disk. ok is false when they match, wantMode is 0 (unspecified), or the file is -// absent/symlinked/non-regular (the content path already covers those). It lets -// `diff` surface a content-identical chmod — which yields no text hunk — rather -// than silently reporting "no diff". -func modeHunk(path string, wantMode uint32) (source, dest string, ok bool) { - if wantMode == 0 { - return "", "", false - } - fi, err := os.Lstat(path) - if err != nil || fi.Mode()&os.ModeSymlink != 0 || !fi.Mode().IsRegular() { - return "", "", false - } - want := os.FileMode(wantMode).Perm() - got := fi.Mode().Perm() - if want == got { +// maintain for a whole-file item (op.Mode) and the file's current perm on +// disk, both as the walk recorded them. ok is false when they match, op.Mode +// is 0 (unspecified), or the file is absent/symlinked/non-regular (the content +// path already covers those) — planItem.opModeDrifted's gate. It lets `diff` +// surface a content-identical chmod — which yields no text hunk — rather than +// silently reporting "no diff". +func modeHunk(it planItem) (source, dest string, ok bool) { + if !it.opModeDrifted() { return "", "", false } - return fmt.Sprintf("mode %04o", want), fmt.Sprintf("mode %04o", got), true + return fmt.Sprintf("mode %04o", os.FileMode(it.op.Mode).Perm()), + fmt.Sprintf("mode %04o", os.FileMode(it.destPerm).Perm()), true } func marshalPretty(v any) string { @@ -307,3 +238,58 @@ func marshalPretty(v any) string { } return strings.TrimSpace(string(data)) } + +// collectDiffHunks runs every selected agent's rendered ops through +// walkPlanItems and collects the masked source/dest hunks that differ, in walk +// order (registry order, plan order, merged keys sorted). names is the agent +// iteration order (reg.Names()); filterPath, when non-empty, narrows the walk +// to ops whose Path equals it exactly. filterMatched reports whether that path +// matched ANY rendered op — it is set inside the walk's matchOp, on op match, +// before any item is produced, so a matching op that yields no item (an emptied +// "{}" section) still counts as managed rather than as a typo (#229 amendment +// A3). +// +// diff never consults state: it has no "applied" side, and whether a hunk +// prints is decided by MASKED-TEXT equality, never by the walk's class — a +// templated source against a cleartext destination classifies `conflict` yet +// masks to equal, and diff must print nothing there. The walk therefore runs +// against an empty state, exactly as the pre-#229 copy consulted none; the +// classes it computes are unused here. +func collectDiffHunks(plan render.RenderPlan, names []string, filterPath string, + redact map[string]string, +) (hunks []diffHunk, filterMatched bool) { + filterMatched = filterPath == "" + items := walkPlanItems(planWalk{ + plan: plan, agents: names, state: state.New(), + withText: true, + matchOp: func(_ string, op adapter.FileOp) bool { + if filterPath != "" && op.Path != filterPath { + return false + } + filterMatched = true + return true + }, + }) + for _, it := range items { + srcStr := secrets.MaskResolved(it.srcText, redact) + dstStr := secrets.MaskResolved(it.dstText, redact) + if it.ptr != "" { + // Key-level diff: one hunk per differing pointer. + if srcStr == dstStr { + continue + } + hunks = append(hunks, diffHunk{Path: it.op.Path, Pointer: it.ptr, Source: srcStr, Dest: dstStr}) + continue + } + // File-level diff. + if srcStr == dstStr { + // Content identical: surface a mode-only drift as a "mode" hunk. + if src, dst, ok := modeHunk(it); ok { + hunks = append(hunks, diffHunk{Path: it.op.Path, Pointer: "mode", Source: src, Dest: dst}) + } + continue + } + hunks = append(hunks, diffHunk{Path: it.op.Path, Source: srcStr, Dest: dstStr}) + } + return hunks, filterMatched +} diff --git a/internal/cli/explain_model.go b/internal/cli/explain_model.go index 0261578f..46fc3055 100644 --- a/internal/cli/explain_model.go +++ b/internal/cli/explain_model.go @@ -1,14 +1,12 @@ package cli import ( - "encoding/json" "path/filepath" "sort" "strings" "github.com/spf13/afero" "github.com/spxrogers/agentsync/internal/adapter" - "github.com/spxrogers/agentsync/internal/drift" "github.com/spxrogers/agentsync/internal/marketplace" "github.com/spxrogers/agentsync/internal/render" "github.com/spxrogers/agentsync/internal/secrets" @@ -66,46 +64,46 @@ func buildExplainModel(in explainInputs) explainModel { // are reported as the different problems they are. pathManaged := false + items := walkPlanItems(planWalk{ + plan: in.plan, agents: in.agents, state: in.state, userHome: in.userHome, scope: in.scope, projectRoot: in.projectRoot, + matchOp: func(agent string, op adapter.FileOp) bool { + if normalizeDestPathBestEffort(op.Path) != in.target { + return false + } + pathManaged = true + if render.IsKeyMerge(op.MergeStrategy) && !containsString(keyMergers, agent) { + keyMergers = append(keyMergers, agent) + } + return true + }, + }) + byAgent := map[string][]planItem{} + for _, it := range items { + byAgent[it.agent] = append(byAgent[it.agent], it) + } for _, name := range in.agents { res, ok := in.plan.PerAgent[name] if !ok { continue } owner := explainOwner{Agent: name} - seenFile := map[string]bool{} - for _, op := range res.Ops { - if op.Action != "" && op.Action != "write" { - continue - } - if normalizeDestPathBestEffort(op.Path) != in.target { - continue - } - pathManaged = true - if render.IsKeyMerge(op.MergeStrategy) { - if !containsString(keyMergers, name) { - keyMergers = append(keyMergers, name) - } - var ours map[string]any - _ = json.Unmarshal(op.Content, &ours) - ptrs := render.CollectPointers(ours, "") - sort.Strings(ptrs) - for _, ptr := range ptrs { - renderedPointers[ptr] = true - if in.pointer != "" && ptr != in.pointer { - continue - } - owner.Items = append(owner.Items, - keyItem(in, name, op, ptr, ours, res.Skips, origins, secretRefs, hookEvents)) + for _, it := range byAgent[name] { + if it.ptr != "" { + // Every rendered pointer counts as owned for the foreign-key + // complement, whether or not the query narrows to one. + renderedPointers[it.ptr] = true + if in.pointer != "" && it.ptr != in.pointer { + continue } + owner.Items = append(owner.Items, keyItem(in, it, res.Skips, origins, secretRefs, hookEvents)) continue } // Whole-file item. A pointer query does not apply to it. - if in.pointer != "" || seenFile[op.Path] { + if in.pointer != "" { continue } - seenFile[op.Path] = true - fileContent[name] = string(op.Content) - owner.Items = append(owner.Items, fileItem(in, name, op, res.Skips, origins, secretRefs)) + fileContent[name] = string(it.op.Content) + owner.Items = append(owner.Items, fileItem(in, it, res.Skips, origins, secretRefs)) } if len(owner.Items) > 0 { model.Owners = append(model.Owners, owner) @@ -141,21 +139,20 @@ func buildExplainModel(in explainInputs) explainModel { return model } -// fileItem builds the provenance for a whole-file destination. -func fileItem(in explainInputs, agent string, op adapter.FileOp, skips []adapter.Skip, +// fileItem builds the provenance for a whole-file destination from its walk +// item, whose happlied and cls are the recorded hash and the classification. +func fileItem(in explainInputs, it planItem, skips []adapter.Skip, origins map[string]explainPluginOrigin, secretRefs map[secrets.RefLocation][]string, ) explainItem { - kind, name := componentFromSourceID(op.SourceID) - entry := in.state.Files[stateFileKey(in.userHome, agent, in.scope, in.projectRoot, op.Path)] - cls := drift.Classify(hashContent(op.Content), entry.SHA256, hashFile(op.Path)) + kind, name := componentFromSourceID(it.op.SourceID) item := explainItem{ - Ownership: ownership(entry.SHA256), + Ownership: ownership(it.happlied), Component: kind, Name: name, - Source: sourceOf(in, op.SourceID, kind), + Source: sourceOf(in, it.op.SourceID, kind), Transforms: matchingSkips(skips, kind, name), - Drift: cls.String(), + Drift: it.cls.String(), } if o, ok := origins[componentKey(kind, name)]; ok { po := o @@ -167,35 +164,29 @@ func fileItem(in explainInputs, agent string, op adapter.FileOp, skips []adapter return item } -// keyItem builds the provenance for one merged key inside a shared destination. -func keyItem(in explainInputs, agent string, op adapter.FileOp, ptr string, ours map[string]any, - skips []adapter.Skip, origins map[string]explainPluginOrigin, +// keyItem builds the provenance for one merged key inside a shared destination +// from its walk item. The walk decoded the destination once for the whole op, +// so every pointer of one file classifies against the same snapshot (#229 +// axis 5); nothing here reads the destination again. +func keyItem(in explainInputs, it planItem, skips []adapter.Skip, origins map[string]explainPluginOrigin, secretRefs map[secrets.RefLocation][]string, hookEvents []string, ) explainItem { - kind, name := componentFromPointer(agent, ptr, hookEvents) - key := stateKeyKey(in.userHome, agent, in.scope, in.projectRoot, op.Path, ptr) - applied := in.state.Keys[key].SHA256 - dest := readDestFile(op.MergeStrategy, op.Path) - cls := drift.Classify( - hashAnyValue(getPointerValue(ours, ptr)), - applied, - hashAnyValue(getPointerValue(dest, ptr)), - ) + kind, name := componentFromPointer(it.agent, it.ptr, hookEvents) item := explainItem{ - Pointer: ptr, - Ownership: ownership(applied), + Pointer: it.ptr, + Ownership: ownership(it.happlied), Component: kind, Name: name, Transforms: matchingSkips(skips, kind, name), - Drift: cls.String(), + Drift: it.cls.String(), } // KeyEntry.SourceID inherits the OP-level SourceID, which for every MCP/hook // key-merge op is a "/* (multiple)" sentinel — the state file genuinely // does not say which mcp/.toml produced /mcpServers/github. The real // per-key resolution is the pointer-shape mapping reconcile owns, so this // derives from it rather than trusting the coarser recorded value. - item.Source = pointerSource(in, agent, ptr, op.SourceID, hookEvents) + item.Source = pointerSource(in, it.agent, it.ptr, it.op.SourceID, hookEvents) if o, ok := origins[componentKey(kind, name)]; ok { po := o item.Plugin = &po diff --git a/internal/cli/iss162_internal_test.go b/internal/cli/iss162_internal_test.go index 3e837761..88f426ec 100644 --- a/internal/cli/iss162_internal_test.go +++ b/internal/cli/iss162_internal_test.go @@ -4,51 +4,93 @@ import ( "os" "path/filepath" "testing" + + "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/render" + "github.com/spxrogers/agentsync/internal/state" ) -// TestStatusDiff_ModeDriftDetection exercises the two helpers that give status -// and diff their mode-drift awareness (issue #162 item D): modeDrifted upgrades a -// content-clean file whose recorded mode diverged from disk to `drift` in -// buildStatusModel, and modeHunk makes diff emit a "mode" hunk for a -// content-identical chmod that would otherwise read as "no diff". A file whose -// mode still matches, or whose recorded/intended mode is unspecified (0), stays a -// no-op — preserving the mtime-churn-avoidance intent. +// TestStatusDiff_ModeDriftDetection exercises the helpers that give status and +// diff their mode-drift awareness (issue #162 item D): destModePerm's filesystem +// triage feeding planItem.recordedModeDrifted, which upgrades a content-clean +// file whose RECORDED mode diverged from disk to `drift` in buildStatusModel, +// and modeHunk, which makes diff emit a "mode" hunk for a content-identical +// chmod that would otherwise read as "no diff". A file whose mode still +// matches, or whose recorded/intended mode is unspecified (0), stays a no-op — +// preserving the mtime-churn-avoidance intent. The four status rows are the +// truth table of the pre-#229 status helper this replaced. func TestStatusDiff_ModeDriftDetection(t *testing.T) { dir := t.TempDir() p := filepath.Join(dir, "run.sh") - if err := os.WriteFile(p, []byte("#!/bin/sh\n"), 0o755); err != nil { + const content = "#!/bin/sh\n" + if err := os.WriteFile(p, []byte(content), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(p, 0o755); err != nil { // umask-proof t.Fatal(err) } + item := func(recorded uint32, path string) planItem { + perm, regular := destModePerm(path) + return planItem{recordedMode: recorded, destPerm: perm, destRegular: regular} + } - // modeDrifted (status side). - if modeDrifted(0o755, p) { - t.Errorf("modeDrifted: 0755 recorded vs 0755 on disk must be false") + // recordedModeDrifted (status side), over destModePerm's triage. + if perm, reg := destModePerm(p); perm != 0o755 || !reg { + t.Fatalf("destModePerm(0755 file) = (%04o, %v), want (0755, true)", perm, reg) + } + if item(0o755, p).recordedModeDrifted() { + t.Errorf("recordedModeDrifted: 0755 recorded vs 0755 on disk must be false") } if err := os.Chmod(p, 0o644); err != nil { t.Fatal(err) } - if !modeDrifted(0o755, p) { - t.Errorf("modeDrifted: 0755 recorded vs 0644 on disk must be true (drift)") + if perm, reg := destModePerm(p); perm != 0o644 || !reg { + t.Fatalf("destModePerm(0644 file) = (%04o, %v), want (0644, true)", perm, reg) } - if modeDrifted(0, p) { - t.Errorf("modeDrifted: recorded mode 0 (unspecified) must never be drift") + if !item(0o755, p).recordedModeDrifted() { + t.Errorf("recordedModeDrifted: 0755 recorded vs 0644 on disk must be true (drift)") } - if modeDrifted(0o755, filepath.Join(dir, "absent")) { - t.Errorf("modeDrifted: a missing file must not be reported as mode drift") + if item(0, p).recordedModeDrifted() { + t.Errorf("recordedModeDrifted: recorded mode 0 (unspecified) must never be drift") + } + absent := filepath.Join(dir, "absent") + if perm, reg := destModePerm(absent); perm != 0 || reg { + t.Fatalf("destModePerm(absent) = (%04o, %v), want (0, false)", perm, reg) + } + if item(0o755, absent).recordedModeDrifted() { + t.Errorf("recordedModeDrifted: a missing file must not be reported as mode drift") } - // modeHunk (diff side): intended 0755 vs on-disk 0644 → a hunk. - src, dst, ok := modeHunk(p, 0o755) + // status asks the RECORDED mode, not the op's (#229 axis 14 is PR-C's + // switch): recorded 0644 matches the 0644 on disk, so the file is clean even + // though op.Mode says the next apply would chmod it to 0755. + userHome := t.TempDir() + s := state.New() + s.Files[stateFileKey(userHome, "claude", adapter.ScopeUser, "", p)] = state.FileEntry{SHA256: hashContent([]byte(content)), Mode: 0o644} + plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{"claude": {Ops: []adapter.FileOp{ + {Action: "write", Path: p, Content: []byte(content), Mode: 0o755, SourceID: "skills/x/run.sh"}, + }}}} + if model := buildStatusModel(plan, []string{"claude"}, s, userHome, adapter.ScopeUser, ""); model.Summary["clean"] != 1 { + t.Errorf("status must compare against the RECORDED mode (0644 == disk), not op.Mode (0755): summary=%v", model.Summary) + } + + // modeHunk (diff side): intended 0755 vs on-disk 0644 → a hunk. The walk + // records the on-disk side (destModePerm) and the op carries the intended. + hunkItem := func(wantMode uint32) planItem { + perm, regular := destModePerm(p) + return planItem{op: adapter.FileOp{Path: p, Mode: wantMode}, destPerm: perm, destRegular: regular} + } + src, dst, ok := modeHunk(hunkItem(0o755)) if !ok { t.Fatalf("modeHunk: expected a hunk for intended 0755 vs on-disk 0644") } if src != "mode 0755" || dst != "mode 0644" { t.Errorf("modeHunk src=%q dst=%q, want 'mode 0755' / 'mode 0644'", src, dst) } - if _, _, ok := modeHunk(p, 0o644); ok { + if _, _, ok := modeHunk(hunkItem(0o644)); ok { t.Errorf("modeHunk: no hunk expected when intended mode == on-disk mode") } - if _, _, ok := modeHunk(p, 0); ok { + if _, _, ok := modeHunk(hunkItem(0)); ok { t.Errorf("modeHunk: no hunk expected for an unspecified intended mode (0)") } } diff --git a/internal/cli/planwalk.go b/internal/cli/planwalk.go new file mode 100644 index 00000000..274f7e6d --- /dev/null +++ b/internal/cli/planwalk.go @@ -0,0 +1,271 @@ +package cli + +import ( + "encoding/json" + "os" + "sort" + + "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/drift" + "github.com/spxrogers/agentsync/internal/render" + "github.com/spxrogers/agentsync/internal/state" +) + +// planItem is one classified destination the walk yields: a whole destination +// file, one RFC-6901 pointer inside a key-merged file, or a whole-file orphan +// (a destination this agent still owns in state but no longer renders). +// +// SECURITY. Every field is unexported, deliberately. op.Content holds RESOLVED +// CLEARTEXT whenever the caller built its plan from secrets.SubstituteCanonical +// — `status` and `explain` both do. A planItem must therefore never be +// marshalled, logged or persisted. encoding/json ignores unexported fields, +// which makes "planItem is not a serialization surface" a property of the type +// rather than a rule a reviewer has to remember; +// TestPlanItemIsNotASerializationSurface fails if a field is ever exported or +// given a `json:` tag. Callers project a planItem into their own statusItem / +// diffHunk / reconcileItem / explainItem and mask before display +// (secrets.MaskResolved). +type planItem struct { + agent string + + // op is the plan op that produced the item. For an ORPHAN it is SYNTHESIZED + // from state — adapter.FileOp{Action: "delete", Path, SourceID} — with Mode + // left 0 because the orphan removal path never reads it. + op adapter.FileOp + + // ptr is the RFC-6901 pointer for a key item; "" for a whole-file item. + ptr string + + // orphan marks a whole-file destination owned in state that this agent no + // longer renders. The set is PER AGENT and UNFILTERED: a path another + // enabled agent still renders IS yielded (status's ownership view). + // reconcile applies its own cross-agent exclusion and path dedupe on top — + // see collectReconcileItems. + orphan bool + + // cls is the CONTENT-only classification. It deliberately does NOT fold in + // permission drift; see recordedModeDrifted / opModeDrifted. + cls drift.Class + + // The triple cls was computed from. hdest is "" for absent-or-unreadable, + // and one of two opaque sentinels for a symlinked or wrong-shaped + // destination — see hashFile, whose semantics this reproduces exactly. + hsrc, happlied, hdest string + + // Whole-file mode facts. recordedMode is state's FileEntry.Mode (0 = + // unrecorded). destPerm/destRegular come from destModePerm: destRegular is + // false for an absent, symlinked or non-regular destination, which is what + // keeps `chmod 000` distinguishable from "absent" (destPerm 0, regular true + // vs destPerm 0, regular false). + recordedMode uint32 + destPerm uint32 + destRegular bool + + // srcText/dstText are populated only when planWalk.withText, and never for + // an orphan. For a key item they are marshalPretty of the pointer's value + // on each side ("" when missing); for a whole-file item they are + // the raw op content and the guarded destination read ("" on any read + // error), which FOLLOWS symlinks — the hash half above does not. That split + // is reconcile's existing behavior and is what keeps `diff` byte-identical + // (#229 axis 9 is PR-C's call). Text is RAW: callers mask + // (secrets.MaskResolved) at their own existing call sites. + srcText, dstText string +} + +// recordedModeDrifted is status's question: does the destination's permission +// bits differ from the mode agentsync RECORDED for it? Exactly the truth table +// of the status-side helper it replaced (#229): an unrecorded mode (0) is never +// drift, and an absent / symlinked / non-regular destination is left to the +// content classifier. +func (i planItem) recordedModeDrifted() bool { + if i.recordedMode == 0 || !i.destRegular { + return false + } + return os.FileMode(i.destPerm).Perm() != os.FileMode(i.recordedMode).Perm() +} + +// opModeDrifted is diff's question: does it differ from the mode the next apply +// would WRITE (op.Mode — render.Writer.Write chmods to it)? Exactly modeHunk's +// gate. status will move to this in PR-C (#229 axis 14); do not move it here. +func (i planItem) opModeDrifted() bool { + if i.op.Mode == 0 || !i.destRegular { + return false + } + return os.FileMode(i.destPerm).Perm() != os.FileMode(i.op.Mode).Perm() +} + +// destModePerm answers the permission bits of the REGULAR file at path. +// regular is false — and perm 0 — for an absent, symlinked (Lstat: the link +// itself is not regular) or non-regular destination, so a caller can tell +// `chmod 000` (0, true) from "not there" (0, false). +func destModePerm(path string) (perm uint32, regular bool) { + fi, err := os.Lstat(path) + if err != nil || !fi.Mode().IsRegular() { + return 0, false + } + return uint32(fi.Mode().Perm()), true +} + +// planWalk is the input to walkPlanItems. +type planWalk struct { + plan render.RenderPlan + // agents is the iteration order (production callers pass reg.Names()). + agents []string + // state is REQUIRED, non-nil. status, reconcile and explain pass the + // loaded state; diff passes an empty state.New() because it never reads the + // applied side. The walk itself must not fall back to one. + state *state.Targets + // userHome is the user's $HOME, the HomeRelative base for state keys. + userHome string + scope adapter.Scope + projectRoot string + + // matchOp, when non-nil, narrows the walk to the ops it accepts. It is + // called EXACTLY ONCE per op that survives the Action filter, in walk + // order, and side effects are an intended use: `diff` sets filterMatched + // and `explain` sets pathManaged/keyMergers inside it, because both must + // record "the path matched an op" for an op that yields ZERO items (a + // synthesized orphan-cleanup op has Content "{}" → no pointers). Deriving + // those flags from len(items) is a regression — see #229 amendment A3. + // matchOp is NOT applied to orphan items: a filtered walk that also sets + // includeOrphans yields every orphan, not the matching ones. + matchOp func(agent string, op adapter.FileOp) bool + + // includeOrphans appends each agent's render.OrphanFiles items AFTER that + // agent's op items, unfiltered (see planItem.orphan). + includeOrphans bool + + // withText populates srcText/dstText. It governs THOSE FIELDS ONLY: + // op.Content still carries resolved cleartext for status and explain + // whether or not this is set, so it is a cost control, not a secrecy + // control (#229 amendment A5). The secrecy guard is the unexported-field + // rule on planItem. + withText bool + + // readDestConfig is a TEST SEAM: nil means readDestFile. It exists so a + // test can count key-merge destination reads and prove they are once-per-op + // rather than once-per-pointer (#229 axis 5). Production callers leave it + // nil. + readDestConfig func(strategy, path string) map[string]any +} + +// walkPlanItems classifies every destination a rendered plan touches, for one +// scope, against state and the current on-disk contents. It is the single walk +// behind `status`, `diff`, `reconcile` and `explain`; before #229 each of those +// held its own copy and the four disagreed. +// +// ORDER is part of the contract. For each name in w.agents that w.plan has a +// result for, in w.agents order: the agent's ops in plan order; within one +// key-merge op, pointers SORTED ascending; then, when w.includeOrphans, that +// agent's render.OrphanFiles (already path-sorted). Whole-file ops are deduped +// by path PER AGENT. Key-merge ops are NEVER deduped by path — one agent emits +// several of them to one file (codex /mcp_servers + /hooks → config.toml; +// claude /hooks + /lspServers → settings.json), each owning a distinct section, +// and deduping dropped the second section's items. +// +// ERRORS DO NOT TRAVEL. Every read, stat and parse failure becomes item state — +// an empty or sentinel hdest, an empty decoded map, an empty dstText — exactly +// as all four copies did. A malformed op.Content is swallowed the same way +// (json.Unmarshal's error is discarded, ours stays nil, the op contributes no +// items). Introducing an error return would be a behavior change with no +// oracle. +// +// Every destination read goes through hashFile / readDestFile / readDestBytes, +// so a FIFO, device, socket or directory at a destination can never block a +// read-only command (internal/cli/destread.go, #240). +func walkPlanItems(w planWalk) []planItem { + readDest := w.readDestConfig + if readDest == nil { + readDest = readDestFile + } + var out []planItem + for _, name := range w.agents { + res, ok := w.plan.PerAgent[name] + if !ok { + continue + } + seenPath := map[string]bool{} + for _, op := range res.Ops { + if op.Action != "" && op.Action != "write" { + continue + } + if w.matchOp != nil && !w.matchOp(name, op) { + continue + } + if render.IsKeyMerge(op.MergeStrategy) { + var ours map[string]any + _ = json.Unmarshal(op.Content, &ours) + // ONCE per op, not per pointer: every pointer of one op + // classifies against the same destination snapshot (#229 axis 5). + final := readDest(op.MergeStrategy, op.Path) + ptrs := render.CollectPointers(ours, "") + sort.Strings(ptrs) // CollectPointers ranges a map (#229 axis 13) + for _, ptr := range ptrs { + srcV := getPointerValue(ours, ptr) + dstV := getPointerValue(final, ptr) + it := planItem{ + agent: name, + op: op, + ptr: ptr, + hsrc: hashAnyValue(srcV), + happlied: w.state.Keys[stateKeyKey(w.userHome, name, w.scope, w.projectRoot, op.Path, ptr)].SHA256, + hdest: hashAnyValue(dstV), + } + it.cls = drift.Classify(it.hsrc, it.happlied, it.hdest) + if w.withText { + it.srcText, it.dstText = marshalPretty(srcV), marshalPretty(dstV) + } + out = append(out, it) + } + continue + } + if seenPath[op.Path] { + continue + } + seenPath[op.Path] = true + entry := w.state.Files[stateFileKey(w.userHome, name, w.scope, w.projectRoot, op.Path)] + perm, reg := destModePerm(op.Path) + it := planItem{ + agent: name, + op: op, + hsrc: hashContent(op.Content), + happlied: entry.SHA256, + hdest: hashFile(op.Path), + recordedMode: entry.Mode, + destPerm: perm, + destRegular: reg, + } + it.cls = drift.Classify(it.hsrc, it.happlied, it.hdest) + if w.withText { + it.srcText = string(op.Content) + if b, err := readDestBytes(op.Path); err == nil { + it.dstText = string(b) + } + } + out = append(out, it) + } + if !w.includeOrphans { + continue + } + for _, orphan := range render.OrphanFiles(w.state, w.userHome, name, w.scope, w.projectRoot, res.Ops) { + entry := w.state.Files[stateFileKey(w.userHome, name, w.scope, w.projectRoot, orphan)] + perm, reg := destModePerm(orphan) + it := planItem{ + agent: name, + orphan: true, + // SourceID matters: the reclaimable-KIND check behind reconcile's + // prompt wording is SourceID-keyed and silently degrades to + // "unknown kind" without it. + op: adapter.FileOp{Action: "delete", Path: orphan, SourceID: entry.SourceID}, + happlied: entry.SHA256, + hdest: hashFile(orphan), + recordedMode: entry.Mode, + destPerm: perm, + destRegular: reg, + } + it.cls = drift.Classify("", it.happlied, it.hdest) + out = append(out, it) + } + } + return out +} diff --git a/internal/cli/planwalk_characterization_test.go b/internal/cli/planwalk_characterization_test.go new file mode 100644 index 00000000..10951dc1 --- /dev/null +++ b/internal/cli/planwalk_characterization_test.go @@ -0,0 +1,1113 @@ +package cli + +import ( + "fmt" + "os" + "path/filepath" + "reflect" + "sort" + "testing" + + "github.com/spf13/afero" + "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/render" + "github.com/spxrogers/agentsync/internal/state" +) + +// This file is the characterization harness for #229: it pins, fixture by +// fixture, what the four plan→drift walks — buildStatusModel (S), +// collectDiffHunks (D), collectReconcileItems (R) and buildExplainModel (E) — +// answer TODAY, before they are unified behind one shared walk. Every golden +// below was written from the drift classifier's truth table and each surface's +// documented rules, then confirmed against the unmodified code; the harness is +// an oracle for the OLD behaviour, so a refactor that needs to edit a golden +// here is not a refactor. The two policy changes #229 defers to its PR-C +// (axes 9 and 14) are the expected exceptions: they edit T-10 +// (whole-file/dest-is-symlink) and T-09 (whole-file/mode-drift-only), nothing +// else. +// +// What is compared IN ORDER: agent order, op order across paths, status's +// whole-file-before-key partition, reconcile's all-items-before-all-orphans +// composition. The ONLY thing the harness is blind to is pointer order within +// one (agent, path) run — see normalizeRuns — because render.CollectPointers +// ranges a Go map and today's output there is genuinely nondeterministic. + +// sRow is one status item with its agent, the S projection's row currency. +type sRow struct { + agent, path, ptr, cls string +} + +// sAgentProj is one statusAgent: the agent name and its ordered rows. The +// skeleton is part of the contract — status appends an agent that has a plan +// result even when it has zero items ("(no tracked items)"). +type sAgentProj struct { + agent string + rows []sRow +} + +type sProj struct { + agents []sAgentProj + summary map[string]int +} + +type dProj struct { + hunks []diffHunk + filterMatched bool +} + +// rRow projects one reconcileItem: everything the reconcile loop reads off it +// except scope/projectRoot (fixture inputs) and pluginOwner (no plugins here). +type rRow struct { + agent, path, ptr, cls string + hsrc, happlied, hdest string + orphan, hasText bool + srcText, dstText string +} + +type eRow struct { + agent, ptr, ownership, drift string +} + +type eProj struct { + rows []eRow + unmanaged bool + pathManaged bool +} + +// normalizeRuns stable-sorts, by pointer, each maximal run of adjacent rows that +// share (agent, path) and carry a pointer. That is the ONLY thing today's code +// leaves nondeterministic (render.CollectPointers ranges a Go map), so it is the +// only thing the harness is allowed to be blind to. Everything else — agent +// order, op order across paths, the whole-file-before-key partition, orphan +// placement — is compared IN ORDER. key returns (agent, path, ptr) for a row. +func normalizeRuns[T any](rows []T, key func(T) (agent, path, ptr string)) []T { + if rows == nil { + return nil + } + out := append([]T(nil), rows...) + for i := 0; i < len(out); { + a, p, ptr := key(out[i]) + if ptr == "" { + i++ + continue + } + j := i + 1 + for j < len(out) { + a2, p2, ptr2 := key(out[j]) + if a2 != a || p2 != p || ptr2 == "" { + break + } + j++ + } + run := out[i:j] + sort.SliceStable(run, func(x, y int) bool { + _, _, px := key(run[x]) + _, _, py := key(run[y]) + return px < py + }) + i = j + } + return out +} + +// planFixture is one characterized scenario. setup builds the on-disk tree, +// the plan and the state under userHome (already symlink-resolved) and returns +// them; the four wants are the goldens. +type planFixture struct { + name string + // scope/projectRoot default to user scope; T-21 overrides. + scope adapter.Scope + projectRoot func(userHome string) string + // diffFilter, when set, is collectDiffHunks's argument. + diffFilter func(userHome string) string + // target/pointer feed buildExplainModel. + target func(userHome string) string + pointer string + setup func(t *testing.T, userHome string) (render.RenderPlan, *state.Targets) + wantS func(userHome string) sProj + wantD func(userHome string) dProj + wantR func(userHome string) []rRow + wantE func(userHome string) eProj + // extra runs fixture-specific assertions on top of the four projections. + extra func(t *testing.T, userHome string, s sProj, d dProj, r []rRow, e eProj) +} + +// hf is the whole-file hash of a literal content string, the value state +// records and hashFile answers for it. +func hf(s string) string { return hashContent([]byte(s)) } + +// mcpVal is the decoded value of one fixture MCP server: every fixture server +// is {"command": }, so its key hash and pretty form are functions of cmd. +func mcpVal(cmd string) map[string]any { return map[string]any{"command": cmd} } + +// hv is hashAnyValue over a fixture server value, what state records for a key. +func hv(cmd string) string { return hashAnyValue(mcpVal(cmd)) } + +// pretty is marshalPretty's rendering of a fixture server value — the literal +// text diff and reconcile show for it. +func pretty(cmd string) string { return "{\n \"command\": \"" + cmd + "\"\n}" } + +// mcpJSON renders a {"mcpServers": {id: {"command": id}}} document for the ids. +func mcpJSON(ids ...string) string { + out := `{"mcpServers":{` + for i, id := range ids { + if i > 0 { + out += "," + } + out += fmt.Sprintf(`%q:{"command":%q}`, id, id) + } + return out + "}}" +} + +func fileKey(userHome, agent, path string) state.Key { + return stateFileKey(userHome, agent, adapter.ScopeUser, "", path) +} + +func ptrKey(userHome, agent, path, ptr string) state.Key { + return stateKeyKey(userHome, agent, adapter.ScopeUser, "", path, ptr) +} + +func fileOp(path, content string) adapter.FileOp { + return adapter.FileOp{Action: "write", Path: path, Content: []byte(content), SourceID: "memory/AGENTS.md"} +} + +func keyOp(path, content string) adapter.FileOp { + return adapter.FileOp{ + Action: "write", Path: path, Content: []byte(content), + MergeStrategy: "merge-json-keys", SourceID: "mcp/* (multiple)", + } +} + +func planFor(agents map[string][]adapter.FileOp) render.RenderPlan { + plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{}} + for name, ops := range agents { + plan.PerAgent[name] = render.AgentResult{Ops: ops} + } + return plan +} + +func dest(userHome, name string) string { return filepath.Join(userHome, "dest", name) } + +func planFixtures() []planFixture { + // Single-agent, single whole-file fixtures share one shape: the op renders + // SOURCE to dest/file.md; disk and state vary per drift class. + wholeFile := func(name, onDisk, applied, rendered, cls, ownership string, dHunk bool) planFixture { + return planFixture{ + name: name, + target: func(h string) string { return dest(h, "file.md") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + d := dest(h, "file.md") + if onDisk != "" { + mustWrite(t, d, onDisk) + } + s := state.New() + if applied != "" { + s.Files[fileKey(h, "claude", d)] = state.FileEntry{SHA256: hf(applied), SourceID: "memory/AGENTS.md"} + } + return planFor(map[string][]adapter.FileOp{"claude": {fileOp(d, rendered)}}), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{{agent: "claude", rows: []sRow{{"claude", dest(h, "file.md"), "", cls}}}}, + summary: map[string]int{cls: 1}, + } + }, + wantD: func(h string) dProj { + if !dHunk { + return dProj{filterMatched: true} + } + return dProj{hunks: []diffHunk{{Path: dest(h, "file.md"), Source: rendered, Dest: onDisk}}, filterMatched: true} + }, + wantR: func(h string) []rRow { + happlied := "" + if applied != "" { + happlied = hf(applied) + } + hdest := "" + if onDisk != "" { + hdest = hf(onDisk) + } + return []rRow{{ + agent: "claude", path: dest(h, "file.md"), cls: cls, + hsrc: hf(rendered), happlied: happlied, hdest: hdest, + hasText: true, srcText: rendered, dstText: onDisk, + }} + }, + wantE: func(string) eProj { + return eProj{rows: []eRow{{"claude", "", ownership, cls}}, pathManaged: true} + }, + } + } + + // Single-agent, single key-merge fixtures: claude key-merges one server + // "gh" into dest/settings.json; disk and state vary per class. + keyMerge := func(name, onDisk string, seedApplied bool, cls, ownership string, dHunk bool, dstText string, foreign []eRow) planFixture { + return planFixture{ + name: name, + target: func(h string) string { return dest(h, "settings.json") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + d := dest(h, "settings.json") + if onDisk != "" { + mustWrite(t, d, onDisk) + } + s := state.New() + if seedApplied { + s.Keys[ptrKey(h, "claude", d, "/mcpServers/gh")] = state.KeyEntry{SHA256: hv("gh"), SourceID: "mcp/* (multiple)"} + } + return planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, mcpJSON("gh"))}}), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{{agent: "claude", rows: []sRow{{"claude", dest(h, "settings.json"), "/mcpServers/gh", cls}}}}, + summary: map[string]int{cls: 1}, + } + }, + wantD: func(h string) dProj { + if !dHunk { + return dProj{filterMatched: true} + } + return dProj{hunks: []diffHunk{{Path: dest(h, "settings.json"), Pointer: "/mcpServers/gh", Source: pretty("gh"), Dest: dstText}}, filterMatched: true} + }, + wantR: func(h string) []rRow { + happlied := "" + if seedApplied { + happlied = hv("gh") + } + hdest := "" + if dstText != "" { + hdest = hv("gh") + } + return []rRow{{ + agent: "claude", path: dest(h, "settings.json"), ptr: "/mcpServers/gh", cls: cls, + hsrc: hv("gh"), happlied: happlied, hdest: hdest, + hasText: true, srcText: pretty("gh"), dstText: dstText, + }} + }, + wantE: func(string) eProj { + rows := append([]eRow{{"claude", "/mcpServers/gh", ownership, cls}}, foreign...) + return eProj{rows: rows, pathManaged: true} + }, + } + } + + return []planFixture{ + // T-01..T-07: the seven non-orphan classes for a whole file, straight + // from drift.Classify's table (hsrc, happlied, hdest). + wholeFile("whole-file/clean", "SOURCE", "SOURCE", "SOURCE", "clean", "managed", false), + wholeFile("whole-file/pending", "APPLIED", "APPLIED", "SOURCE2", "pending", "managed", true), + wholeFile("whole-file/drift", "EDITED", "SOURCE", "SOURCE", "drift", "managed", true), + wholeFile("whole-file/conflict", "EDITED", "APPLIED", "SOURCE", "conflict", "managed", true), + wholeFile("whole-file/converged", "SOURCE", "APPLIED", "SOURCE", "converged", "managed", false), + // T-06: absent dest, no state → new; diff shows the whole source against "". + wholeFile("whole-file/new", "", "", "SOURCE", "new", "untracked", true), + // T-07: a pre-existing native file, never applied → foreign-collision. + wholeFile("whole-file/foreign-collision", "FOREIGN", "", "SOURCE", "foreign-collision", "untracked", true), + + // T-08: an orphan (state-owned, no op renders it) beside a kept file. + // status lists it after the agent's rendered items; reconcile lists it + // in the orphan half with NO text; diff and explain never see it. + orphanFixture("whole-file/orphan", "APPLIED", "orphan"), + orphanFixture("whole-file/orphan-drifted", "EDITED", "orphan-drifted"), + + // T-09: content clean, permission bits drifted. Three answers today: + // status folds RECORDED-mode drift into `drift`; diff emits a "mode" + // hunk against op.Mode; reconcile and explain ignore mode entirely. + // recordedMode (0755) != op.Mode (0700) != on disk (0644), so a walk + // that swapped the recorded mode for op.Mode would still be caught. + { + name: "whole-file/mode-drift-only", + target: func(h string) string { return dest(h, "run.sh") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + d := dest(h, "run.sh") + mustWrite(t, d, "SOURCE") + if err := os.Chmod(d, 0o644); err != nil { + t.Fatal(err) + } + s := state.New() + s.Files[fileKey(h, "claude", d)] = state.FileEntry{SHA256: hf("SOURCE"), Mode: 0o755, SourceID: "skills/x/run.sh"} + op := fileOp(d, "SOURCE") + op.Mode = 0o700 + op.SourceID = "skills/x/run.sh" + return planFor(map[string][]adapter.FileOp{"claude": {op}}), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{{agent: "claude", rows: []sRow{{"claude", dest(h, "run.sh"), "", "drift"}}}}, + summary: map[string]int{"drift": 1}, + } + }, + wantD: func(h string) dProj { + return dProj{hunks: []diffHunk{{Path: dest(h, "run.sh"), Pointer: "mode", Source: "mode 0700", Dest: "mode 0644"}}, filterMatched: true} + }, + wantR: func(h string) []rRow { + return []rRow{{ + agent: "claude", path: dest(h, "run.sh"), cls: "clean", + hsrc: hf("SOURCE"), happlied: hf("SOURCE"), hdest: hf("SOURCE"), + hasText: true, srcText: "SOURCE", dstText: "SOURCE", + }} + }, + wantE: func(string) eProj { + return eProj{rows: []eRow{{"claude", "", "managed", "clean"}}, pathManaged: true} + }, + }, + + // T-10: the destination is a symlink to an identical file. The HASH side + // (status/reconcile/explain) answers the symlink sentinel → drift; the + // TEXT side (diff, reconcile's dstText) reads THROUGH the link → equal. + // That split is #229 axis 9 and is preserved as-is. explain's target is + // the link's TARGET: explain resolves op paths through symlinks. + { + name: "whole-file/dest-is-symlink", + target: func(h string) string { return dest(h, "real.md") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + real := dest(h, "real.md") + mustWrite(t, real, "SOURCE") + link := dest(h, "link.md") + if err := os.Symlink(real, link); err != nil { + t.Fatal(err) + } + s := state.New() + s.Files[fileKey(h, "claude", link)] = state.FileEntry{SHA256: hf("SOURCE"), SourceID: "memory/AGENTS.md"} + return planFor(map[string][]adapter.FileOp{"claude": {fileOp(link, "SOURCE")}}), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{{agent: "claude", rows: []sRow{{"claude", dest(h, "link.md"), "", "drift"}}}}, + summary: map[string]int{"drift": 1}, + } + }, + wantD: func(string) dProj { return dProj{filterMatched: true} }, + wantR: func(h string) []rRow { + return []rRow{{ + agent: "claude", path: dest(h, "link.md"), cls: "drift", + hsrc: hf("SOURCE"), happlied: hf("SOURCE"), hdest: "symlink-not-regular-file", + hasText: true, srcText: "SOURCE", dstText: "SOURCE", + }} + }, + wantE: func(string) eProj { + return eProj{rows: []eRow{{"claude", "", "managed", "drift"}}, pathManaged: true} + }, + extra: func(t *testing.T, _ string, s sProj, d dProj, r []rRow, _ eProj) { + t.Helper() + // D2's split, asserted by name so the intent survives a golden edit. + if r[0].hdest != "symlink-not-regular-file" || r[0].cls != "drift" || s.agents[0].rows[0].cls != "drift" { + t.Errorf("hash side must answer the symlink sentinel and classify drift: %+v", r[0]) + } + if r[0].srcText != r[0].dstText || len(d.hunks) != 0 { + t.Errorf("text side must read through the link and produce no hunk: r=%+v d=%+v", r[0], d) + } + }, + }, + + // T-11: claude renders the SAME path twice (deduped per agent, once), and + // opencode renders it too (NOT deduped across agents). Disk drifted so + // every surface yields something to count. + { + name: "whole-file/two-agents-same-path", + target: func(h string) string { return dest(h, "AGENTS.md") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + d := dest(h, "AGENTS.md") + mustWrite(t, d, "EDITED") + s := state.New() + for _, a := range []string{"claude", "opencode"} { + s.Files[fileKey(h, a, d)] = state.FileEntry{SHA256: hf("SHARED"), SourceID: "memory/AGENTS.md"} + } + return planFor(map[string][]adapter.FileOp{ + "claude": {fileOp(d, "SHARED"), fileOp(d, "SHARED")}, + "opencode": {fileOp(d, "SHARED")}, + }), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{ + {agent: "claude", rows: []sRow{{"claude", dest(h, "AGENTS.md"), "", "drift"}}}, + {agent: "opencode", rows: []sRow{{"opencode", dest(h, "AGENTS.md"), "", "drift"}}}, + }, + summary: map[string]int{"drift": 2}, + } + }, + wantD: func(h string) dProj { + return dProj{hunks: []diffHunk{ + {Path: dest(h, "AGENTS.md"), Source: "SHARED", Dest: "EDITED"}, + {Path: dest(h, "AGENTS.md"), Source: "SHARED", Dest: "EDITED"}, + }, filterMatched: true} + }, + wantR: func(h string) []rRow { + row := func(a string) rRow { + return rRow{ + agent: a, path: dest(h, "AGENTS.md"), cls: "drift", + hsrc: hf("SHARED"), happlied: hf("SHARED"), hdest: hf("EDITED"), + hasText: true, srcText: "SHARED", dstText: "EDITED", + } + } + return []rRow{row("claude"), row("opencode")} + }, + wantE: func(string) eProj { + return eProj{rows: []eRow{{"claude", "", "managed", "drift"}, {"opencode", "", "managed", "drift"}}, pathManaged: true} + }, + }, + + // T-12: two key-merge ops to ONE path (claude's /mcpServers and + // /lspServers both land in settings.json). Never deduped by path: both + // sections' pointers must appear. The lsp key is drifted on disk. + { + name: "key-merge/two-ops-one-path", + target: func(h string) string { return dest(h, "settings.json") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + d := dest(h, "settings.json") + mustWrite(t, d, `{"mcpServers":{"gh":{"command":"gh"}},"lspServers":{"gopls":{"command":"gopls2"}}}`) + s := state.New() + s.Keys[ptrKey(h, "claude", d, "/mcpServers/gh")] = state.KeyEntry{SHA256: hv("gh"), SourceID: "mcp/* (multiple)"} + s.Keys[ptrKey(h, "claude", d, "/lspServers/gopls")] = state.KeyEntry{SHA256: hv("gopls"), SourceID: "lsp/* (multiple)"} + lsp := keyOp(d, `{"lspServers":{"gopls":{"command":"gopls"}}}`) + lsp.SourceID = "lsp/* (multiple)" + return planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, mcpJSON("gh")), lsp}}), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{{agent: "claude", rows: []sRow{ + {"claude", dest(h, "settings.json"), "/lspServers/gopls", "drift"}, + {"claude", dest(h, "settings.json"), "/mcpServers/gh", "clean"}, + }}}, + summary: map[string]int{"clean": 1, "drift": 1}, + } + }, + wantD: func(h string) dProj { + return dProj{hunks: []diffHunk{ + {Path: dest(h, "settings.json"), Pointer: "/lspServers/gopls", Source: pretty("gopls"), Dest: pretty("gopls2")}, + }, filterMatched: true} + }, + wantR: func(h string) []rRow { + return []rRow{ + { + agent: "claude", path: dest(h, "settings.json"), ptr: "/lspServers/gopls", cls: "drift", + hsrc: hv("gopls"), happlied: hv("gopls"), hdest: hv("gopls2"), + hasText: true, srcText: pretty("gopls"), dstText: pretty("gopls2"), + }, + { + agent: "claude", path: dest(h, "settings.json"), ptr: "/mcpServers/gh", cls: "clean", + hsrc: hv("gh"), happlied: hv("gh"), hdest: hv("gh"), + hasText: true, srcText: pretty("gh"), dstText: pretty("gh"), + }, + } + }, + wantE: func(string) eProj { + return eProj{rows: []eRow{ + {"claude", "/lspServers/gopls", "managed", "drift"}, + {"claude", "/mcpServers/gh", "managed", "clean"}, + }, pathManaged: true} + }, + extra: func(t *testing.T, h string, s sProj, _ dProj, r []rRow, _ eProj) { + t.Helper() + // Axis 11: the multiset of pointers at the shared path carries BOTH + // sections. normalizeRuns merges the two ops into one run, so this + // is asserted explicitly rather than by order. + want := map[string]int{"/lspServers/gopls": 1, "/mcpServers/gh": 1} + gotR := map[string]int{} + for _, it := range r { + if it.path == dest(h, "settings.json") { + gotR[it.ptr]++ + } + } + gotS := map[string]int{} + for _, it := range s.agents[0].rows { + gotS[it.ptr]++ + } + if !reflect.DeepEqual(gotR, want) || !reflect.DeepEqual(gotS, want) { + t.Errorf("pointer multiset at the shared path: R=%v S=%v want %v", gotR, gotS, want) + } + }, + }, + + // T-13: five servers under one op, inserted in non-sorted order; "tango" + // is absent from disk. Rows are compared in sorted-pointer order (the + // run normalization), classes per pointer. + { + name: "key-merge/many-pointers", + target: func(h string) string { return dest(h, "settings.json") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + d := dest(h, "settings.json") + mustWrite(t, d, mcpJSON("zulu", "mike", "alpha", "bravo")) + s := state.New() + for _, id := range []string{"zulu", "mike", "alpha", "tango", "bravo"} { + s.Keys[ptrKey(h, "claude", d, "/mcpServers/"+id)] = state.KeyEntry{SHA256: hv(id), SourceID: "mcp/* (multiple)"} + } + return planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, mcpJSON("zulu", "mike", "alpha", "tango", "bravo"))}}), s + }, + wantS: func(h string) sProj { + d := dest(h, "settings.json") + return sProj{ + agents: []sAgentProj{{agent: "claude", rows: []sRow{ + {"claude", d, "/mcpServers/alpha", "clean"}, + {"claude", d, "/mcpServers/bravo", "clean"}, + {"claude", d, "/mcpServers/mike", "clean"}, + {"claude", d, "/mcpServers/tango", "drift"}, + {"claude", d, "/mcpServers/zulu", "clean"}, + }}}, + summary: map[string]int{"clean": 4, "drift": 1}, + } + }, + wantD: func(h string) dProj { + return dProj{hunks: []diffHunk{ + {Path: dest(h, "settings.json"), Pointer: "/mcpServers/tango", Source: pretty("tango"), Dest: ""}, + }, filterMatched: true} + }, + wantR: func(h string) []rRow { + d := dest(h, "settings.json") + clean := func(id string) rRow { + return rRow{ + agent: "claude", path: d, ptr: "/mcpServers/" + id, cls: "clean", + hsrc: hv(id), happlied: hv(id), hdest: hv(id), + hasText: true, srcText: pretty(id), dstText: pretty(id), + } + } + return []rRow{ + clean("alpha"), clean("bravo"), clean("mike"), + { + agent: "claude", path: d, ptr: "/mcpServers/tango", cls: "drift", + hsrc: hv("tango"), happlied: hv("tango"), hdest: "", + hasText: true, srcText: pretty("tango"), dstText: "", + }, + clean("zulu"), + } + }, + wantE: func(string) eProj { + return eProj{rows: []eRow{ + {"claude", "/mcpServers/alpha", "managed", "clean"}, + {"claude", "/mcpServers/bravo", "managed", "clean"}, + {"claude", "/mcpServers/mike", "managed", "clean"}, + {"claude", "/mcpServers/tango", "managed", "drift"}, + {"claude", "/mcpServers/zulu", "managed", "clean"}, + }, pathManaged: true} + }, + }, + + // T-14: the merged destination does not exist yet → new per key. + keyMerge("key-merge/dest-missing", "", false, "new", "untracked", true, "", nil), + // T-15: a hand-commented JSONC destination with trailing commas decodes + // (hujson), so the owned key is clean; its foreign sibling is reported by + // explain as first-class. + keyMerge("key-merge/dest-is-JSONC-with-comments", + "// managed by agentsync\n{\n \"mcpServers\": {\n \"gh\": {\"command\": \"gh\"},\n },\n \"other\": {\"x\": 1},\n}\n", + true, "clean", "managed", false, pretty("gh"), []eRow{{"claude", "/other/x", "foreign", ""}}), + // T-16: an unparseable destination decodes to an EMPTY document, so the + // owned key classifies against an absent value (drift, not conflict, not + // a file-level item), and explain reports no foreign keys. + keyMerge("key-merge/dest-unparseable", "{not json", true, "drift", "managed", true, "", nil), + + // T-17: op.Content that is not JSON yields no pointers → zero items, but + // the agent still has a status entry and the path still counts as + // managed for diff's filter and explain. + { + name: "key-merge/op-content-not-json", + diffFilter: func(h string) string { return dest(h, "settings.json") }, + target: func(h string) string { return dest(h, "settings.json") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + d := dest(h, "settings.json") + mustWrite(t, d, mcpJSON("gh")) + return planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, "not json")}}), state.New() + }, + wantS: func(string) sProj { + return sProj{agents: []sAgentProj{{agent: "claude"}}, summary: map[string]int{}} + }, + wantD: func(string) dProj { return dProj{filterMatched: true} }, + wantR: func(string) []rRow { return nil }, + wantE: func(string) eProj { return eProj{unmanaged: true, pathManaged: true} }, + }, + + // T-18: a server id containing '/' and '~' is RFC-6901-escaped in the + // pointer and decoded again on lookup, so it classifies clean rather + // than as phantom drift. explain is narrowed to that pointer. + { + name: "key-merge/pointer-id-with-slash-and-tilde", + target: func(h string) string { return dest(h, "settings.json") }, + pointer: "/mcpServers/a~1b~0c", + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + d := dest(h, "settings.json") + mustWrite(t, d, mcpJSON("a/b~c")) + s := state.New() + s.Keys[ptrKey(h, "claude", d, "/mcpServers/a~1b~0c")] = state.KeyEntry{SHA256: hv("a/b~c"), SourceID: "mcp/* (multiple)"} + return planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, mcpJSON("a/b~c"))}}), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{{agent: "claude", rows: []sRow{{"claude", dest(h, "settings.json"), "/mcpServers/a~1b~0c", "clean"}}}}, + summary: map[string]int{"clean": 1}, + } + }, + wantD: func(string) dProj { return dProj{filterMatched: true} }, + wantR: func(h string) []rRow { + return []rRow{{ + agent: "claude", path: dest(h, "settings.json"), ptr: "/mcpServers/a~1b~0c", cls: "clean", + hsrc: hv("a/b~c"), happlied: hv("a/b~c"), hdest: hv("a/b~c"), + hasText: true, srcText: pretty("a/b~c"), dstText: pretty("a/b~c"), + }} + }, + wantE: func(string) eProj { + return eProj{rows: []eRow{{"claude", "/mcpServers/a~1b~0c", "managed", "clean"}}, pathManaged: true} + }, + }, + + // T-19: claude owns P in state but no longer renders it; opencode still + // does. status shows claude's ownership view (P is an orphan for claude); + // reconcile EXCLUDES the orphan because another agent renders the path. + { + name: "orphan/shared-dest-other-agent-renders", + target: func(h string) string { return dest(h, "AGENTS.md") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + p := dest(h, "AGENTS.md") + k := dest(h, "CLAUDE.md") + mustWrite(t, p, "SHARED") + mustWrite(t, k, "KEPT") + s := state.New() + s.Files[fileKey(h, "claude", p)] = state.FileEntry{SHA256: hf("SHARED"), SourceID: "memory/AGENTS.md"} + s.Files[fileKey(h, "claude", k)] = state.FileEntry{SHA256: hf("KEPT"), SourceID: "memory/AGENTS.md"} + s.Files[fileKey(h, "opencode", p)] = state.FileEntry{SHA256: hf("SHARED"), SourceID: "memory/AGENTS.md"} + return planFor(map[string][]adapter.FileOp{ + "claude": {fileOp(k, "KEPT")}, + "opencode": {fileOp(p, "SHARED")}, + }), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{ + {agent: "claude", rows: []sRow{ + {"claude", dest(h, "CLAUDE.md"), "", "clean"}, + {"claude", dest(h, "AGENTS.md"), "", "orphan"}, + }}, + {agent: "opencode", rows: []sRow{{"opencode", dest(h, "AGENTS.md"), "", "clean"}}}, + }, + summary: map[string]int{"clean": 2, "orphan": 1}, + } + }, + wantD: func(string) dProj { return dProj{filterMatched: true} }, + wantR: func(h string) []rRow { + return []rRow{ + { + agent: "claude", path: dest(h, "CLAUDE.md"), cls: "clean", + hsrc: hf("KEPT"), happlied: hf("KEPT"), hdest: hf("KEPT"), + hasText: true, srcText: "KEPT", dstText: "KEPT", + }, + { + agent: "opencode", path: dest(h, "AGENTS.md"), cls: "clean", + hsrc: hf("SHARED"), happlied: hf("SHARED"), hdest: hf("SHARED"), + hasText: true, srcText: "SHARED", dstText: "SHARED", + }, + } + }, + wantE: func(string) eProj { + return eProj{rows: []eRow{{"opencode", "", "managed", "clean"}}, pathManaged: true} + }, + }, + + // T-20: an empty plan (the one fixture allowed to project nothing at + // all), and an agent with a plan result but no ops (a status skeleton + // entry with zero items). + { + name: "plan/empty", + target: func(h string) string { return dest(h, "file.md") }, + setup: func(_ *testing.T, _ string) (render.RenderPlan, *state.Targets) { + return render.RenderPlan{PerAgent: map[string]render.AgentResult{}}, state.New() + }, + wantS: func(string) sProj { return sProj{summary: map[string]int{}} }, + wantD: func(string) dProj { return dProj{filterMatched: true} }, + wantR: func(string) []rRow { return nil }, + wantE: func(string) eProj { return eProj{unmanaged: true} }, + }, + { + name: "plan/agent-with-no-ops", + target: func(h string) string { return dest(h, "file.md") }, + setup: func(_ *testing.T, _ string) (render.RenderPlan, *state.Targets) { + return planFor(map[string][]adapter.FileOp{"claude": nil}), state.New() + }, + wantS: func(string) sProj { + return sProj{agents: []sAgentProj{{agent: "claude"}}, summary: map[string]int{}} + }, + wantD: func(string) dProj { return dProj{filterMatched: true} }, + wantR: func(string) []rRow { return nil }, + wantE: func(string) eProj { return eProj{unmanaged: true} }, + }, + + // T-21: project scope. State keys are scoped, so a user-scope entry for + // a different path is neither the project file's record nor an orphan + // of the project walk. + { + name: "scope/project", + scope: adapter.ScopeProject, + projectRoot: func(h string) string { return filepath.Join(h, "proj") }, + target: func(h string) string { return filepath.Join(h, "proj", "CLAUDE.md") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + root := filepath.Join(h, "proj") + d := filepath.Join(root, "CLAUDE.md") + mustWrite(t, d, "SOURCE") + s := state.New() + s.Files[stateFileKey(h, "claude", adapter.ScopeProject, root, d)] = state.FileEntry{SHA256: hf("SOURCE"), SourceID: "memory/AGENTS.md"} + // A USER-scope entry for another path: out of this walk's tree. + s.Files[fileKey(h, "claude", dest(h, "user-only.md"))] = state.FileEntry{SHA256: hf("USER"), SourceID: "memory/AGENTS.md"} + return planFor(map[string][]adapter.FileOp{"claude": {fileOp(d, "SOURCE")}}), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{{agent: "claude", rows: []sRow{{"claude", filepath.Join(h, "proj", "CLAUDE.md"), "", "clean"}}}}, + summary: map[string]int{"clean": 1}, + } + }, + wantD: func(string) dProj { return dProj{filterMatched: true} }, + wantR: func(h string) []rRow { + return []rRow{{ + agent: "claude", path: filepath.Join(h, "proj", "CLAUDE.md"), cls: "clean", + hsrc: hf("SOURCE"), happlied: hf("SOURCE"), hdest: hf("SOURCE"), + hasText: true, srcText: "SOURCE", dstText: "SOURCE", + }} + }, + wantE: func(string) eProj { + return eProj{rows: []eRow{{"claude", "", "managed", "clean"}}, pathManaged: true} + }, + }, + + // T-22: a key-merge op BEFORE a whole-file op in plan order, both + // drifted. status re-partitions whole-file rows ahead of key rows; diff + // and reconcile keep plan order. + { + name: "order/key-merge-op-before-whole-file", + target: func(h string) string { return dest(h, "MEM.md") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + sj := dest(h, "settings.json") + mem := dest(h, "MEM.md") + mustWrite(t, sj, `{"mcpServers":{"x":{"command":"npm"}}}`) + mustWrite(t, mem, "EDITED") + s := state.New() + s.Keys[ptrKey(h, "claude", sj, "/mcpServers/x")] = state.KeyEntry{SHA256: hv("npx"), SourceID: "mcp/* (multiple)"} + s.Files[fileKey(h, "claude", mem)] = state.FileEntry{SHA256: hf("SOURCE"), SourceID: "memory/AGENTS.md"} + return planFor(map[string][]adapter.FileOp{"claude": { + keyOp(sj, `{"mcpServers":{"x":{"command":"npx"}}}`), + fileOp(mem, "SOURCE"), + }}), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{{agent: "claude", rows: []sRow{ + {"claude", dest(h, "MEM.md"), "", "drift"}, + {"claude", dest(h, "settings.json"), "/mcpServers/x", "drift"}, + }}}, + summary: map[string]int{"drift": 2}, + } + }, + wantD: func(h string) dProj { + return dProj{hunks: []diffHunk{ + {Path: dest(h, "settings.json"), Pointer: "/mcpServers/x", Source: pretty("npx"), Dest: pretty("npm")}, + {Path: dest(h, "MEM.md"), Source: "SOURCE", Dest: "EDITED"}, + }, filterMatched: true} + }, + wantR: func(h string) []rRow { + return []rRow{ + { + agent: "claude", path: dest(h, "settings.json"), ptr: "/mcpServers/x", cls: "drift", + hsrc: hv("npx"), happlied: hv("npx"), hdest: hv("npm"), + hasText: true, srcText: pretty("npx"), dstText: pretty("npm"), + }, + { + agent: "claude", path: dest(h, "MEM.md"), cls: "drift", + hsrc: hf("SOURCE"), happlied: hf("SOURCE"), hdest: hf("EDITED"), + hasText: true, srcText: "SOURCE", dstText: "EDITED", + }, + } + }, + wantE: func(string) eProj { + return eProj{rows: []eRow{{"claude", "", "managed", "drift"}}, pathManaged: true} + }, + }, + + // T-23: claude has a plan result with no ops and a state-owned orphan P; + // opencode renders Q. reconcile lists ALL rendered items before ALL + // orphans (Q then P) even though claude sorts first; status keeps per + // agent grouping. P matches no op, so diff's filter misses and explain + // calls it unmanaged. + { + name: "order/two-agents-orphan-then-ops", + diffFilter: func(h string) string { return dest(h, "P.md") }, + target: func(h string) string { return dest(h, "P.md") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + p := dest(h, "P.md") + q := dest(h, "Q.md") + mustWrite(t, p, "APPLIED") + mustWrite(t, q, "SOURCE") + s := state.New() + s.Files[fileKey(h, "claude", p)] = state.FileEntry{SHA256: hf("APPLIED"), SourceID: "memory/AGENTS.md"} + s.Files[fileKey(h, "opencode", q)] = state.FileEntry{SHA256: hf("SOURCE"), SourceID: "memory/AGENTS.md"} + return planFor(map[string][]adapter.FileOp{ + "claude": nil, + "opencode": {fileOp(q, "SOURCE")}, + }), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{ + {agent: "claude", rows: []sRow{{"claude", dest(h, "P.md"), "", "orphan"}}}, + {agent: "opencode", rows: []sRow{{"opencode", dest(h, "Q.md"), "", "clean"}}}, + }, + summary: map[string]int{"clean": 1, "orphan": 1}, + } + }, + wantD: func(string) dProj { return dProj{filterMatched: false} }, + wantR: func(h string) []rRow { + return []rRow{ + { + agent: "opencode", path: dest(h, "Q.md"), cls: "clean", + hsrc: hf("SOURCE"), happlied: hf("SOURCE"), hdest: hf("SOURCE"), + hasText: true, srcText: "SOURCE", dstText: "SOURCE", + }, + { + agent: "claude", path: dest(h, "P.md"), cls: "orphan", + hsrc: "", happlied: hf("APPLIED"), hdest: hf("APPLIED"), + orphan: true, + }, + } + }, + wantE: func(string) eProj { return eProj{unmanaged: true, pathManaged: false} }, + }, + + // T-25: two agents own the SAME orphan path in state and neither renders + // it. status reports it under each owner (per-agent ownership view); + // reconcile prompts for the file ONCE, under the first owner in + // registry order — its orphan dedupe is global, not per agent; explain + // calls the path unmanaged — it never consults orphans. + { + name: "orphan/two-agents-own-one-orphan", + diffFilter: func(h string) string { return dest(h, "P.md") }, + target: func(h string) string { return dest(h, "P.md") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + p := dest(h, "P.md") + mustWrite(t, p, "APPLIED") + s := state.New() + s.Files[fileKey(h, "claude", p)] = state.FileEntry{SHA256: hf("APPLIED"), SourceID: "memory/AGENTS.md"} + s.Files[fileKey(h, "opencode", p)] = state.FileEntry{SHA256: hf("APPLIED"), SourceID: "memory/AGENTS.md"} + return planFor(map[string][]adapter.FileOp{"claude": nil, "opencode": nil}), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{ + {agent: "claude", rows: []sRow{{"claude", dest(h, "P.md"), "", "orphan"}}}, + {agent: "opencode", rows: []sRow{{"opencode", dest(h, "P.md"), "", "orphan"}}}, + }, + summary: map[string]int{"orphan": 2}, + } + }, + wantD: func(string) dProj { return dProj{filterMatched: false} }, + wantR: func(h string) []rRow { + return []rRow{{ + agent: "claude", path: dest(h, "P.md"), cls: "orphan", + hsrc: "", happlied: hf("APPLIED"), hdest: hf("APPLIED"), + orphan: true, + }} + }, + wantE: func(string) eProj { return eProj{unmanaged: true, pathManaged: false} }, + }, + + // T-24: a key-merge op whose Content is "{}" — the shape render.Plan + // synthesizes to clean up an emptied section — against a destination + // that still carries foreign keys there. It yields ZERO items, yet the + // path matched an op: diff's filterMatched and explain's pathManaged are + // side effects of the match, not of the item count (#229 amendment A3). + { + name: "key-merge/emptied-section", + diffFilter: func(h string) string { return dest(h, "settings.json") }, + target: func(h string) string { return dest(h, "settings.json") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + d := dest(h, "settings.json") + mustWrite(t, d, `{"mcpServers":{"foreign":{"command":"x"}}}`) + return planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, "{}")}}), state.New() + }, + wantS: func(string) sProj { + return sProj{agents: []sAgentProj{{agent: "claude"}}, summary: map[string]int{}} + }, + wantD: func(string) dProj { return dProj{filterMatched: true} }, + wantR: func(string) []rRow { return nil }, + // Zero owners: an owner is appended only when it has items, and + // foreign rows attach only to owners — so none are reported. + wantE: func(string) eProj { return eProj{unmanaged: true, pathManaged: true} }, + }, + } +} + +// orphanFixture is T-08's shape (from iss155_internal_test.go): claude renders +// kept.md (clean) and still owns orphan.md in state, which no op renders; the +// orphan's on-disk content decides orphan vs orphan-drifted. +func orphanFixture(name, onDisk, cls string) planFixture { + return planFixture{ + name: name, + target: func(h string) string { return dest(h, "orphan.md") }, + setup: func(t *testing.T, h string) (render.RenderPlan, *state.Targets) { + t.Helper() + orphan := dest(h, "orphan.md") + kept := dest(h, "kept.md") + mustWrite(t, orphan, onDisk) + mustWrite(t, kept, "KEPT") + s := state.New() + s.Files[fileKey(h, "claude", orphan)] = state.FileEntry{SHA256: hf("APPLIED"), SourceID: "memory/AGENTS.md"} + s.Files[fileKey(h, "claude", kept)] = state.FileEntry{SHA256: hf("KEPT"), SourceID: "memory/AGENTS.md"} + return planFor(map[string][]adapter.FileOp{"claude": {fileOp(kept, "KEPT")}}), s + }, + wantS: func(h string) sProj { + return sProj{ + agents: []sAgentProj{{agent: "claude", rows: []sRow{ + {"claude", dest(h, "kept.md"), "", "clean"}, + {"claude", dest(h, "orphan.md"), "", cls}, + }}}, + summary: map[string]int{"clean": 1, cls: 1}, + } + }, + wantD: func(string) dProj { return dProj{filterMatched: true} }, + wantR: func(h string) []rRow { + return []rRow{ + { + agent: "claude", path: dest(h, "kept.md"), cls: "clean", + hsrc: hf("KEPT"), happlied: hf("KEPT"), hdest: hf("KEPT"), + hasText: true, srcText: "KEPT", dstText: "KEPT", + }, + { + agent: "claude", path: dest(h, "orphan.md"), cls: cls, + hsrc: "", happlied: hf("APPLIED"), hdest: hf(onDisk), + orphan: true, + }, + } + }, + // explain never sees an orphan: no op renders the path. + wantE: func(string) eProj { return eProj{unmanaged: true, pathManaged: false} }, + } +} + +// ---- projections -------------------------------------------------------------- + +func projectS(m statusModel) sProj { + out := sProj{summary: m.Summary} + for _, ag := range m.Agents { + a := sAgentProj{agent: ag.Agent} + for _, it := range ag.Items { + a.rows = append(a.rows, sRow{ag.Agent, it.Path, it.Pointer, it.Class}) + } + a.rows = normalizeRuns(a.rows, func(r sRow) (string, string, string) { return r.agent, r.path, r.ptr }) + out.agents = append(out.agents, a) + } + return out +} + +func projectD(hunks []diffHunk, matched bool) dProj { + // A "mode" hunk is a whole-file row wearing a label, not a pointer; keep it + // out of the run sort so its placement stays asserted in order. + hunks = normalizeRuns(hunks, func(h diffHunk) (string, string, string) { + if h.Pointer == "mode" { + return "", h.Path, "" + } + return "", h.Path, h.Pointer + }) + return dProj{hunks: hunks, filterMatched: matched} +} + +func projectR(items []reconcileItem) []rRow { + var out []rRow + for _, it := range items { + out = append(out, rRow{ + agent: it.agentName, path: it.op.Path, ptr: it.ptr, cls: it.cls.String(), + hsrc: it.hsrc, happlied: it.happlied, hdest: it.hdest, + orphan: it.orphan, hasText: it.hasText, srcText: it.srcText, dstText: it.dstText, + }) + } + return normalizeRuns(out, func(r rRow) (string, string, string) { return r.agent, r.path, r.ptr }) +} + +func projectE(m explainModel) eProj { + var rows []eRow + for _, o := range m.Owners { + for _, it := range o.Items { + rows = append(rows, eRow{o.Agent, it.Pointer, it.Ownership, it.Drift}) + } + } + rows = normalizeRuns(rows, func(r eRow) (string, string, string) { return r.agent, m.Path, r.ptr }) + return eProj{rows: rows, unmanaged: m.Unmanaged, pathManaged: m.pathManaged} +} + +// TestPlanWalkCharacterization runs every fixture through the four builders +// and compares each projection to its golden. The four calls below mirror the +// production call sites verbatim: reg.Names() as the agent order everywhere, +// and reconcile's items-then-orphans composition (reconcile.go, reconcileRun). +func TestPlanWalkCharacterization(t *testing.T) { + reg := registryFactory() + for _, tc := range planFixtures() { + t.Run(tc.name, func(t *testing.T) { + // explain resolves op paths through symlinks before matching, so the + // fixture root must already be its own resolved form. + userHome, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + sc, root := adapter.ScopeUser, "" + if tc.projectRoot != nil { + sc, root = tc.scope, tc.projectRoot(userHome) + } + plan, s := tc.setup(t, userHome) + + gotS := projectS(buildStatusModel(plan, reg.Names(), s, userHome, sc, root)) + + filter := "" + if tc.diffFilter != nil { + filter = tc.diffFilter(userHome) + } + gotD := projectD(collectDiffHunks(plan, reg.Names(), filter, nil)) + + items, orphans := collectReconcileItems(plan, reg, s, sc, root, userHome, nil) + gotR := projectR(append(items, orphans...)) + + gotE := projectE(buildExplainModel(explainInputs{ + fs: afero.NewMemMapFs(), + target: tc.target(userHome), + pointer: tc.pointer, + plan: plan, + agents: reg.Names(), + state: s, + userHome: userHome, + agentsyncHome: filepath.Join(userHome, ".agentsync"), + srcHome: filepath.Join(userHome, ".agentsync"), + scope: sc, + projectRoot: root, + })) + + // Vacuity guard: a fixture that projects nothing on S, D and R + // characterizes nothing. Only the empty plan may. + if tc.name != "plan/empty" && len(gotS.agents) == 0 && len(gotD.hunks) == 0 && len(gotR) == 0 { + t.Fatalf("fixture projects nothing on S, D and R — it pins no behaviour") + } + + if want := tc.wantS(userHome); !reflect.DeepEqual(gotS, want) { + t.Errorf("S (status) projection mismatch\n got: %+v\nwant: %+v", gotS, want) + } + if want := tc.wantD(userHome); !reflect.DeepEqual(gotD, want) { + t.Errorf("D (diff) projection mismatch\n got: %+v\nwant: %+v", gotD, want) + } + if want := tc.wantR(userHome); !reflect.DeepEqual(gotR, want) { + t.Errorf("R (reconcile) projection mismatch\n got: %+v\nwant: %+v", gotR, want) + } + if want := tc.wantE(userHome); !reflect.DeepEqual(gotE, want) { + t.Errorf("E (explain) projection mismatch\n got: %+v\nwant: %+v", gotE, want) + } + if tc.extra != nil { + tc.extra(t, userHome, gotS, gotD, gotR, gotE) + } + }) + } +} diff --git a/internal/cli/planwalk_internal_test.go b/internal/cli/planwalk_internal_test.go new file mode 100644 index 00000000..2d2b5182 --- /dev/null +++ b/internal/cli/planwalk_internal_test.go @@ -0,0 +1,535 @@ +package cli + +import ( + "encoding" + "encoding/json" + "os" + "path/filepath" + "reflect" + "sort" + "testing" + + "github.com/spf13/afero" + "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/drift" + "github.com/spxrogers/agentsync/internal/render" + "github.com/spxrogers/agentsync/internal/state" +) + +// walkUser runs walkPlanItems at user scope with the fixture defaults the +// characterization harness uses (userHome as the state base, no project). +func walkUser(userHome string, plan render.RenderPlan, s *state.Targets, agents []string, opts func(*planWalk)) []planItem { + w := planWalk{plan: plan, agents: agents, state: s, userHome: userHome, scope: adapter.ScopeUser} + if opts != nil { + opts(&w) + } + return walkPlanItems(w) +} + +// itemKeys projects the walk output to "agent path#ptr" (with "!" for an +// orphan) so an ORDER assertion reads as one slice comparison. +func itemKeys(items []planItem) []string { + var out []string + for _, it := range items { + k := it.agent + " " + it.op.Path + if it.ptr != "" { + k += "#" + it.ptr + } + if it.orphan { + k += "!" + } + out = append(out, k) + } + return out +} + +// TestWalkPlanItems pins the walk's own contract (#229 N-12): agent and op +// order, sorted pointers, per-agent whole-file dedupe vs never-dedupe for +// key-merge ops, orphan placement and filtering, the matchOp side-effect +// contract, the Action filter, the withText fields and the hash/text split. +func TestWalkPlanItems(t *testing.T) { + tests := []struct { + name string + run func(t *testing.T, h string) + }{ + { + name: "agent-and-op-order", + run: func(t *testing.T, h string) { + a, b, c := dest(h, "a.md"), dest(h, "b.md"), dest(h, "c.md") + plan := planFor(map[string][]adapter.FileOp{ + "claude": {fileOp(a, "A"), fileOp(b, "B")}, + "opencode": {fileOp(c, "C")}, + "codex": {fileOp(a, "A")}, + }) + // agents order is the caller's, NOT sorted, and an agent the + // plan has no result for is skipped. + got := itemKeys(walkUser(h, plan, state.New(), []string{"opencode", "claude", "cursor"}, nil)) + want := []string{"opencode " + c, "claude " + a, "claude " + b} + if !reflect.DeepEqual(got, want) { + t.Errorf("order: got %v want %v", got, want) + } + }, + }, + { + name: "pointers-sorted", + run: func(t *testing.T, h string) { + d := dest(h, "settings.json") + ids := []string{"zulu", "mike", "alpha", "tango", "bravo"} + plan := planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, mcpJSON(ids...))}}) + // CollectPointers ranges a map, so one lucky run proves nothing; + // 50 unsorted runs agreeing has probability ~120^-49. + for i := 0; i < 50; i++ { + items := walkUser(h, plan, state.New(), []string{"claude"}, nil) + if len(items) != len(ids) { + t.Fatalf("run %d: got %d items want %d", i, len(items), len(ids)) + } + var ptrs []string + for _, it := range items { + ptrs = append(ptrs, it.ptr) + } + if !sort.StringsAreSorted(ptrs) { + t.Fatalf("run %d: pointers not sorted: %v", i, ptrs) + } + } + }, + }, + { + name: "whole-file-deduped-per-agent", + run: func(t *testing.T, h string) { + d := dest(h, "AGENTS.md") + plan := planFor(map[string][]adapter.FileOp{ + "claude": {fileOp(d, "X"), fileOp(d, "X")}, + "opencode": {fileOp(d, "X")}, + }) + got := itemKeys(walkUser(h, plan, state.New(), []string{"claude", "opencode"}, nil)) + want := []string{"claude " + d, "opencode " + d} + if !reflect.DeepEqual(got, want) { + t.Errorf("dedupe: got %v want %v", got, want) + } + }, + }, + { + name: "key-merge-never-deduped", + run: func(t *testing.T, h string) { + d := dest(h, "settings.json") + plan := planFor(map[string][]adapter.FileOp{"claude": { + keyOp(d, mcpJSON("gh")), + keyOp(d, `{"lspServers":{"gopls":{"command":"gopls"}}}`), + }}) + got := itemKeys(walkUser(h, plan, state.New(), []string{"claude"}, nil)) + want := []string{"claude " + d + "#/mcpServers/gh", "claude " + d + "#/lspServers/gopls"} + if !reflect.DeepEqual(got, want) { + t.Errorf("both sections must be walked: got %v want %v", got, want) + } + }, + }, + { + name: "orphans-follow-their-agent", + run: func(t *testing.T, h string) { + a, p, b := dest(h, "a.md"), dest(h, "p.md"), dest(h, "b.md") + mustWrite(t, p, "P") + s := state.New() + s.Files[fileKey(h, "claude", p)] = state.FileEntry{SHA256: hf("P"), SourceID: "skills/x/SKILL.md"} + plan := planFor(map[string][]adapter.FileOp{ + "claude": {fileOp(a, "A")}, + "opencode": {fileOp(b, "B")}, + }) + items := walkUser(h, plan, s, []string{"claude", "opencode"}, func(w *planWalk) { w.includeOrphans = true }) + got := itemKeys(items) + want := []string{"claude " + a, "claude " + p + "!", "opencode " + b} + if !reflect.DeepEqual(got, want) { + t.Errorf("orphan placement: got %v want %v", got, want) + } + o := items[1] + if o.op.Action != "delete" || o.op.SourceID != "skills/x/SKILL.md" || o.op.Mode != 0 || + o.cls != drift.Orphan || o.hsrc != "" || o.happlied != hf("P") || o.hdest != hf("P") { + t.Errorf("orphan item: %+v", o) + } + // Without includeOrphans the orphan is not yielded at all. + if got := itemKeys(walkUser(h, plan, s, []string{"claude", "opencode"}, nil)); len(got) != 2 { + t.Errorf("includeOrphans=false must yield ops only: %v", got) + } + }, + }, + { + name: "orphans-are-not-cross-agent-filtered", + run: func(t *testing.T, h string) { + p := dest(h, "AGENTS.md") + mustWrite(t, p, "SHARED") + s := state.New() + s.Files[fileKey(h, "claude", p)] = state.FileEntry{SHA256: hf("SHARED")} + plan := planFor(map[string][]adapter.FileOp{ + "claude": nil, + "opencode": {fileOp(p, "SHARED")}, + }) + got := itemKeys(walkUser(h, plan, s, []string{"claude", "opencode"}, func(w *planWalk) { w.includeOrphans = true })) + // opencode still renders p; the walk yields claude's orphan + // anyway — that exclusion is reconcile's, not the walk's. + want := []string{"claude " + p + "!", "opencode " + p} + if !reflect.DeepEqual(got, want) { + t.Errorf("orphan set must be per agent and unfiltered: got %v want %v", got, want) + } + }, + }, + { + name: "orphan-ignores-mode", + run: func(t *testing.T, h string) { + p := dest(h, "orphan.sh") + mustWrite(t, p, "APPLIED") + if err := os.Chmod(p, 0o644); err != nil { + t.Fatal(err) + } + s := state.New() + s.Files[fileKey(h, "claude", p)] = state.FileEntry{SHA256: hf("APPLIED"), Mode: 0o755} + plan := planFor(map[string][]adapter.FileOp{"claude": nil}) + items := walkUser(h, plan, s, []string{"claude"}, func(w *planWalk) { w.includeOrphans = true }) + if len(items) != 1 || !items[0].orphan { + t.Fatalf("want one orphan item, got %+v", items) + } + // The mode facts ARE populated (recorded 0755, disk 0644 → the + // predicate would say drift)… + if !items[0].recordedModeDrifted() { + t.Fatalf("fixture: orphan mode facts not populated: %+v", items[0]) + } + // …and the status projection must not consult them: an orphan's + // class is the classifier's, untouched by the chmod. + model := buildStatusModel(plan, []string{"claude"}, s, h, adapter.ScopeUser, "") + if got := model.Summary; got["orphan"] != 1 || got["drift"] != 0 { + t.Errorf("status must leave an orphan's class alone under a chmod: summary=%v", got) + } + }, + }, + { + name: "matchOp-filters-and-is-called-once-per-op", + run: func(t *testing.T, h string) { + a, b, p := dest(h, "a.md"), dest(h, "b.md"), dest(h, "p.md") + mustWrite(t, p, "P") + s := state.New() + s.Files[fileKey(h, "claude", p)] = state.FileEntry{SHA256: hf("P")} + del := fileOp(b, "B") + del.Action = "delete" + plan := planFor(map[string][]adapter.FileOp{ + "claude": {fileOp(a, "A"), fileOp(a, "A"), fileOp(b, "B"), del, keyOp(b, "{}")}, + "opencode": {fileOp(b, "B")}, + }) + var calls []string + items := walkUser(h, plan, s, []string{"claude", "opencode"}, func(w *planWalk) { + w.includeOrphans = true + w.matchOp = func(agent string, op adapter.FileOp) bool { + calls = append(calls, agent+" "+op.Path) + return op.Path == a + } + }) + // Once per op that survives the Action filter, in walk order, + // with the agent name; the "delete" op never reaches it, and the + // duplicate whole-file op at `a` is offered BEFORE the per-agent + // path dedupe drops it. + wantCalls := []string{"claude " + a, "claude " + a, "claude " + b, "claude " + b, "opencode " + b} + if !reflect.DeepEqual(calls, wantCalls) { + t.Errorf("matchOp calls: got %v want %v", calls, wantCalls) + } + // Only the accepted op yields; the orphan is NOT subject to + // matchOp and is still yielded. + got := itemKeys(items) + want := []string{"claude " + a, "claude " + p + "!"} + if !reflect.DeepEqual(got, want) { + t.Errorf("filtered items: got %v want %v", got, want) + } + }, + }, + { + name: "action-not-write-is-skipped", + run: func(t *testing.T, h string) { + a, b := dest(h, "a.md"), dest(h, "b.md") + del := fileOp(b, "B") + del.Action = "delete" + empty := fileOp(a, "A") + empty.Action = "" + plan := planFor(map[string][]adapter.FileOp{"claude": {del, empty}}) + got := itemKeys(walkUser(h, plan, state.New(), []string{"claude"}, nil)) + // "" and "write" are the accepted spellings; anything else is + // skipped (matches explain and render.OrphanFiles). + if want := []string{"claude " + a}; !reflect.DeepEqual(got, want) { + t.Errorf("got %v want %v", got, want) + } + }, + }, + { + name: "withText-off-leaves-text-empty", + run: func(t *testing.T, h string) { + f, k := dest(h, "f.md"), dest(h, "settings.json") + mustWrite(t, f, "F") + mustWrite(t, k, mcpJSON("gh")) + plan := planFor(map[string][]adapter.FileOp{"claude": {fileOp(f, "F"), keyOp(k, mcpJSON("gh"))}}) + for _, it := range walkUser(h, plan, state.New(), []string{"claude"}, nil) { + if it.srcText != "" || it.dstText != "" { + t.Errorf("withText=false must leave text empty: %+v", it) + } + } + }, + }, + { + name: "withText-on-whole-file-is-raw-content-and-guarded-read", + run: func(t *testing.T, h string) { + f, absent, dir := dest(h, "f.md"), dest(h, "absent.md"), dest(h, "dir.md") + mustWrite(t, f, "ON DISK") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + plan := planFor(map[string][]adapter.FileOp{"claude": { + fileOp(f, "RENDERED"), fileOp(absent, "RENDERED"), fileOp(dir, "RENDERED"), + }}) + items := walkUser(h, plan, state.New(), []string{"claude"}, func(w *planWalk) { w.withText = true }) + if len(items) != 3 { + t.Fatalf("got %d items", len(items)) + } + for _, it := range items { + if it.srcText != "RENDERED" { + t.Errorf("srcText is the raw op content: %q", it.srcText) + } + } + if items[0].dstText != "ON DISK" || items[0].hdest != hf("ON DISK") { + t.Errorf("regular dest: %+v", items[0]) + } + if items[1].dstText != "" || items[1].hdest != "" { + t.Errorf("absent dest reads as empty text and absent hash: %+v", items[1]) + } + // A wrong-shaped destination is refused before the open: the + // hash side answers the shape sentinel, the text side "". + if items[2].dstText != "" || items[2].hdest != "not-a-regular-file" { + t.Errorf("directory dest: %+v", items[2]) + } + }, + }, + { + name: "withText-on-key-is-marshalPretty-with-absent-sentinel", + run: func(t *testing.T, h string) { + d := dest(h, "settings.json") + mustWrite(t, d, mcpJSON("gh")) + plan := planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, mcpJSON("gh", "missing"))}}) + items := walkUser(h, plan, state.New(), []string{"claude"}, func(w *planWalk) { w.withText = true }) + if len(items) != 2 { + t.Fatalf("got %d items", len(items)) + } + if items[0].ptr != "/mcpServers/gh" || items[0].srcText != pretty("gh") || items[0].dstText != pretty("gh") { + t.Errorf("present key: %+v", items[0]) + } + if items[1].ptr != "/mcpServers/missing" || items[1].srcText != pretty("missing") || items[1].dstText != "" { + t.Errorf("absent key: %+v", items[1]) + } + }, + }, + { + name: "symlink-hash-text-split", + run: func(t *testing.T, h string) { + real, link := dest(h, "real.md"), dest(h, "link.md") + mustWrite(t, real, "SOURCE") + if err := os.Symlink(real, link); err != nil { + t.Fatal(err) + } + s := state.New() + s.Files[fileKey(h, "claude", link)] = state.FileEntry{SHA256: hf("SOURCE")} + plan := planFor(map[string][]adapter.FileOp{"claude": {fileOp(link, "SOURCE")}}) + items := walkUser(h, plan, s, []string{"claude"}, func(w *planWalk) { w.withText = true }) + if len(items) != 1 { + t.Fatalf("got %d items", len(items)) + } + it := items[0] + // D2: the hash side answers hashFile's symlink sentinel (→ drift); + // the text side reads THROUGH the link (→ identical text). + if it.hdest != "symlink-not-regular-file" || it.cls != drift.Drift { + t.Errorf("hash side: hdest=%q cls=%v", it.hdest, it.cls) + } + if it.srcText != "SOURCE" || it.dstText != "SOURCE" { + t.Errorf("text side: src=%q dst=%q", it.srcText, it.dstText) + } + if it.destRegular { + t.Errorf("a symlinked destination is not a regular file for the mode predicates") + } + }, + }, + { + name: "op-content-not-json-yields-no-items", + run: func(t *testing.T, h string) { + d := dest(h, "settings.json") + mustWrite(t, d, mcpJSON("gh")) + plan := planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, "not json")}}) + if got := walkUser(h, plan, state.New(), []string{"claude"}, nil); len(got) != 0 { + t.Errorf("malformed op.Content contributes no items, got %+v", got) + } + }, + }, + { + name: "nil-decoded-dest-classifies-per-key", + run: func(t *testing.T, h string) { + d := dest(h, "settings.json") + s := state.New() + s.Keys[ptrKey(h, "claude", d, "/mcpServers/gh")] = state.KeyEntry{SHA256: hv("gh")} + plan := planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, mcpJSON("gh", "new"))}}) + items := walkUser(h, plan, s, []string{"claude"}, func(w *planWalk) { + w.readDestConfig = func(string, string) map[string]any { return nil } + }) + if len(items) != 2 { + t.Fatalf("got %d items", len(items)) + } + // An empty decoded document is "absent" per key, never one + // file-level item: a recorded key is drift, an unrecorded one new. + if items[0].ptr != "/mcpServers/gh" || items[0].cls != drift.Drift || items[0].hdest != "" { + t.Errorf("recorded key: %+v", items[0]) + } + if items[1].ptr != "/mcpServers/new" || items[1].cls != drift.New { + t.Errorf("unrecorded key: %+v", items[1]) + } + }, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + h, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + tc.run(t, h) + }) + } +} + +// TestWalkPlanItems_ReadsKeyMergeDestOncePerOp pins #229 axis 5: a key-merge +// destination is decoded ONCE per op, so every pointer of one op classifies +// against the same snapshot and a file with N servers is not read N times. +// The readDestConfig seam exists for exactly this count. +func TestWalkPlanItems_ReadsKeyMergeDestOncePerOp(t *testing.T) { + h := t.TempDir() + d := dest(h, "settings.json") + mustWrite(t, d, mcpJSON("a", "b", "c", "d")) + plan := planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, mcpJSON("a", "b", "c", "d"))}}) + reads := 0 + items := walkUser(h, plan, state.New(), []string{"claude"}, func(w *planWalk) { + w.readDestConfig = func(strategy, path string) map[string]any { + reads++ + return readDestFile(strategy, path) + } + }) + if len(items) != 4 { + t.Fatalf("fixture must yield four pointers, got %d", len(items)) + } + if reads != 1 { + t.Errorf("key-merge destination read %d times for one op; want exactly 1", reads) + } +} + +// TestPlanItemIsNotASerializationSurface pins the walk's secrecy guard (#229 +// N-13): a planItem carries resolved cleartext in op.Content, and the ONLY +// thing that keeps it out of a --json payload is that encoding/json ignores +// unexported fields. Exporting a field, or tagging one `json:`, is how that +// property dies — so either fails here. (staticcheck's SA9005 independently +// rejects a json.Marshal of a struct with no exported fields, which is why this +// test asserts the shape of the type rather than marshalling one.) +func TestPlanItemIsNotASerializationSurface(t *testing.T) { + typ := reflect.TypeOf(planItem{}) + // Proof of life: the reflection must actually be walking the struct. + if typ.NumField() < 10 { + t.Fatalf("planItem has only %d fields; the guard is not looking at the type it claims to", typ.NumField()) + } + for i := 0; i < typ.NumField(); i++ { + f := typ.Field(i) + if f.IsExported() { + t.Errorf("planItem.%s is exported — a planItem must never be marshalled; keep every field unexported", f.Name) + } + if f.Anonymous { + t.Errorf("planItem embeds %s — an embedded type's exported fields are promoted into a marshalled form", f.Name) + } + if tag, ok := f.Tag.Lookup("json"); ok { + t.Errorf("planItem.%s carries a json tag %q — a planItem is not a serialization surface", f.Name, tag) + } + } + // encoding/json also honours encoding.TextMarshaler, which would marshal + // the whole item as one string — same egress, different door. + for _, iface := range []reflect.Type{ + reflect.TypeOf((*json.Marshaler)(nil)).Elem(), + reflect.TypeOf((*encoding.TextMarshaler)(nil)).Elem(), + } { + if typ.Implements(iface) || reflect.PointerTo(typ).Implements(iface) { + t.Errorf("planItem implements %s — a planItem must never be marshalled", iface) + } + } +} + +// TestDestModePerm pins the (perm, regular) contract: `chmod 000` is a regular +// file with perm 0, distinguishable from "not there" — the two the mode +// predicates must not conflate. +func TestDestModePerm(t *testing.T) { + h := t.TempDir() + reg, zero, link, dir := filepath.Join(h, "reg"), filepath.Join(h, "zero"), filepath.Join(h, "link"), filepath.Join(h, "dir") + mustWrite(t, reg, "x") + mustWrite(t, zero, "x") + if err := os.Chmod(reg, 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(zero, 0); err != nil { + t.Fatal(err) + } + if err := os.Symlink(reg, link); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(dir, 0o755); err != nil { + t.Fatal(err) + } + for _, tc := range []struct { + name, path string + wantPerm uint32 + wantRegular bool + }{ + {name: "regular", path: reg, wantPerm: 0o644, wantRegular: true}, + {name: "chmod-000-is-still-regular", path: zero, wantPerm: 0, wantRegular: true}, + {name: "absent", path: filepath.Join(h, "nope"), wantPerm: 0, wantRegular: false}, + {name: "symlink-is-the-link-not-the-target", path: link, wantPerm: 0, wantRegular: false}, + {name: "directory", path: dir, wantPerm: 0, wantRegular: false}, + } { + t.Run(tc.name, func(t *testing.T) { + perm, regular := destModePerm(tc.path) + if perm != tc.wantPerm || regular != tc.wantRegular { + t.Errorf("destModePerm = (%04o, %v); want (%04o, %v)", perm, regular, tc.wantPerm, tc.wantRegular) + } + }) + } +} + +// TestPathFilterFlagsSurviveAZeroItemOp pins #229 amendment A3: diff's +// filterMatched and explain's pathManaged are set when the path MATCHES an op, +// not when the op yields an item. The op here is the exact shape render.Plan +// synthesizes to clean up an emptied key-merge section — Content "{}" — which +// yields zero pointers. Deriving either flag from the item count would turn +// today's "no diff" into "path … is not managed by agentsync", and explain's +// "managed path, no owners" into "unmanaged path". +func TestPathFilterFlagsSurviveAZeroItemOp(t *testing.T) { + userHome, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + d := dest(userHome, ".claude.json") + mustWrite(t, d, `{"mcpServers":{"foreign":{"command":"x"}}}`) + plan := planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, "{}")}}) + names := []string{"claude"} + + hunks, matched := collectDiffHunks(plan, names, d, nil) + if len(hunks) != 0 || !matched { + t.Errorf("collectDiffHunks: got %d hunks, filterMatched=%v; want 0 hunks and filterMatched=true", len(hunks), matched) + } + + model := buildExplainModel(explainInputs{ + fs: afero.NewMemMapFs(), + target: d, + plan: plan, + agents: names, + state: state.New(), + userHome: userHome, + srcHome: filepath.Join(userHome, ".agentsync"), + scope: adapter.ScopeUser, + projectRoot: "", + }) + if !model.Unmanaged || !model.pathManaged { + t.Errorf("buildExplainModel: Unmanaged=%v pathManaged=%v; want Unmanaged=true (no owners) and pathManaged=true (the path matched an op)", + model.Unmanaged, model.pathManaged) + } +} diff --git a/internal/cli/planwalk_order_internal_test.go b/internal/cli/planwalk_order_internal_test.go new file mode 100644 index 00000000..da598faf --- /dev/null +++ b/internal/cli/planwalk_order_internal_test.go @@ -0,0 +1,110 @@ +package cli + +import ( + "sort" + "testing" + + "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/render" + "github.com/spxrogers/agentsync/internal/state" +) + +// TestMergedKeyOrderIsDeterministic pins #229 axis 13 for every surface that +// lists merged keys: five servers under one key-merge op, inserted in an order +// that is NOT sorted, listed by each builder 50 times, must come back in +// exactly one order, and that order ascending. render.CollectPointers ranges a +// Go map — measured five distinct orderings over 200 calls before the shared +// walk — so 50 identical unsorted runs would have probability ~120^-49. +func TestMergedKeyOrderIsDeterministic(t *testing.T) { + ids := []string{"zulu", "mike", "alpha", "tango", "bravo"} + tests := []struct { + name string + // pointers lists the merged-key pointers the builder reports, in the + // builder's own order, for the fixture at userHome. + pointers func(t *testing.T, userHome, path string, plan render.RenderPlan, s *state.Targets) []string + }{ + { + // N-6: status's --json payload and dashboard rows. + name: "status", + pointers: func(t *testing.T, userHome, _ string, plan render.RenderPlan, s *state.Targets) []string { + t.Helper() + var out []string + for _, it := range buildStatusModel(plan, []string{"claude"}, s, userHome, adapter.ScopeUser, "").Agents[0].Items { + out = append(out, it.Pointer) + } + return out + }, + }, + { + // N-7: diff's hunks (every key differs, so all five print). + name: "diff", + pointers: func(t *testing.T, _, _ string, plan render.RenderPlan, _ *state.Targets) []string { + t.Helper() + hunks, _ := collectDiffHunks(plan, []string{"claude"}, "", nil) + var out []string + for _, h := range hunks { + out = append(out, h.Pointer) + } + return out + }, + }, + { + // N-8: reconcile's prompt queue. + name: "reconcile", + pointers: func(t *testing.T, userHome, _ string, plan render.RenderPlan, s *state.Targets) []string { + t.Helper() + var out []string + items, _ := collectReconcileItems(plan, registryFactory(), s, adapter.ScopeUser, "", userHome, nil) + for _, it := range items { + out = append(out, it.ptr) + } + return out + }, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + userHome := t.TempDir() + d := dest(userHome, "settings.json") + // Every key differs on all three sides (source, state, disk), so + // a builder that lists only differing keys (diff) still reports + // all five. + disk := `{"mcpServers":{` + for i, id := range ids { + if i > 0 { + disk += "," + } + disk += `"` + id + `":{"command":"` + id + `-disk"}` + } + mustWrite(t, d, disk+"}}") + s := state.New() + for _, id := range ids { + s.Keys[ptrKey(userHome, "claude", d, "/mcpServers/"+id)] = state.KeyEntry{SHA256: hv(id + "-old")} + } + plan := planFor(map[string][]adapter.FileOp{"claude": {keyOp(d, mcpJSON(ids...))}}) + + distinct := map[string]bool{} + var first []string + for i := 0; i < 50; i++ { + got := tc.pointers(t, userHome, d, plan, s) + if len(got) != len(ids) { + t.Fatalf("run %d: %d pointers, want %d: %v", i, len(got), len(ids), got) + } + key := "" + for _, p := range got { + key += p + "\x00" + } + distinct[key] = true + if first == nil { + first = got + } + } + if len(distinct) != 1 { + t.Errorf("%d distinct pointer orderings over 50 runs; want exactly 1", len(distinct)) + } + if !sort.StringsAreSorted(first) { + t.Errorf("pointers are not ascending: %v", first) + } + }) + } +} diff --git a/internal/cli/planwalk_unix_internal_test.go b/internal/cli/planwalk_unix_internal_test.go new file mode 100644 index 00000000..20d53712 --- /dev/null +++ b/internal/cli/planwalk_unix_internal_test.go @@ -0,0 +1,54 @@ +//go:build unix + +package cli + +import ( + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/state" +) + +// TestWalkPlanItems_FIFODestAnswersTheShapeSentinelAndDoesNotBlock is the +// unix-only N-12 subtest: a FIFO at a whole-file destination must not wedge the +// walk (os.ReadFile on a FIFO blocks in the open rather than failing), and the +// answer must be the SHAPE sentinel on the hash side and "" on the text side — +// asserted by value, so a skipped Mkfifo cannot quietly become a permanent hole +// that a "returned in time" check would hide. +func TestWalkPlanItems_FIFODestAnswersTheShapeSentinelAndDoesNotBlock(t *testing.T) { + h := t.TempDir() + fifo := filepath.Join(h, "dest", "pipe.md") + mustWrite(t, filepath.Join(h, "dest", ".keep"), "") + if err := syscall.Mkfifo(fifo, 0o644); err != nil { + t.Skipf("mkfifo unsupported here: %v", err) + } + plan := planFor(map[string][]adapter.FileOp{"claude": {fileOp(fifo, "SOURCE")}}) + + var items []planItem + done := make(chan struct{}) + go func() { + defer close(done) + items = walkUser(h, plan, state.New(), []string{"claude"}, func(w *planWalk) { w.withText = true }) + }() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("walkPlanItems BLOCKED on a FIFO destination") + } + if len(items) != 1 { + t.Fatalf("got %d items", len(items)) + } + it := items[0] + if it.hdest != "not-a-regular-file" { + t.Errorf("hash side must answer the shape sentinel, got %q", it.hdest) + } + if it.dstText != "" || it.srcText != "SOURCE" { + t.Errorf("text side must be empty for a refused shape: src=%q dst=%q", it.srcText, it.dstText) + } + if it.destRegular { + t.Errorf("a FIFO is not a regular file for the mode predicates") + } +} diff --git a/internal/cli/plugin_namespace_test.go b/internal/cli/plugin_namespace_test.go index 9e39381c..37a76301 100644 --- a/internal/cli/plugin_namespace_test.go +++ b/internal/cli/plugin_namespace_test.go @@ -869,7 +869,8 @@ func TestImport_NamedHookEventAllPluginProvided(t *testing.T) { // TestReconcile_OrphanPromptSaysAutoReclaimKindsAreTemporary pins the prompt // wording — and exists because the branch shipped DEAD once already. // -// reconcile's orphan items are built by collectOrphanFileItems, which originally +// reconcile's orphan items are built by walkPlanItems (collectReconcileItems' +// orphan half), which originally // synthesized a FileOp with no SourceID. The wording branch asks a // SourceID-keyed question, so it silently answered "not reclaimable" for every // orphan and the conditional text could never print. Nothing caught it because diff --git a/internal/cli/plugin_owner_internal_test.go b/internal/cli/plugin_owner_internal_test.go index 45d2398f..07a7f0bb 100644 --- a/internal/cli/plugin_owner_internal_test.go +++ b/internal/cli/plugin_owner_internal_test.go @@ -39,7 +39,7 @@ func TestPluginOwnerForKeyItem(t *testing.T) { // NOTE: Continue's per-server SourceID ("mcp/.toml") is deliberately // NOT exercised here. Its op is MergeStrategy "replace", so it is a - // WHOLE-FILE item that never reaches this function — collectItems looks + // WHOLE-FILE item that never reaches this function — collectReconcileItems looks // it up by SourceID instead. Asserting it here would test a shape that // cannot occur; the real path is covered by // TestPluginProvidedSourceIDs_RegistersBothServerKeyForms and the @@ -102,7 +102,7 @@ func TestUnescapeJSONPointer(t *testing.T) { // the section-wide SourceID "mcp/* (multiple)" and only the JSON pointer names // the server — that is the "/" key. Continue instead renders ONE FILE // PER SERVER with the per-server SourceID "mcp/.toml", making it a whole-file -// item that collectItems looks up by SourceID directly. Keying only the bare form +// item that collectReconcileItems looks up by SourceID directly. Keying only the bare form // left that path unguarded: a plugin-provided server on Continue could still be // captured. func TestPluginProvidedSourceIDs_RegistersBothServerKeyForms(t *testing.T) { diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index 419d480a..ee8c30c3 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -90,7 +90,7 @@ first time. Rule of thumb: import adopts, reconcile resolves.`, } cmd.Flags().BoolVar(&autoWB, "auto-writeback", false, "auto-resolve drift by writing dest back to source") cmd.Flags().BoolVar(&autoOR, "auto-override", false, "auto-resolve drift by re-applying source to dest") - cmd.Flags().BoolVar(&autoSafe, "auto-safe", false, "auto-resolve only converged/pending/new (no-op)") + cmd.Flags().BoolVar(&autoSafe, "auto-safe", false, "non-interactive: resolve nothing, report every item that needs a human") addAgentsFlag(cmd, &agentsCSV, "reconcile pass") markScopeAware(cmd) return cmd @@ -168,8 +168,8 @@ func reconcileRun(cmd *cobra.Command, in io.Reader, autoWB, autoOR, autoSafe boo if sc == adapter.ScopeProject && c.Project != nil { ownerSrc = *c.Project } - items := collectItems(plan, reg, s, sc, projectRoot, userHome, pluginProvidedSourceIDs(ownerSrc)) - items = append(items, collectOrphanFileItems(plan, reg, s, sc, projectRoot, userHome)...) + items, orphans := collectReconcileItems(plan, reg, s, 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. @@ -321,8 +321,8 @@ func reconcileRun(cmd *cobra.Command, in io.Reader, autoWB, autoOR, autoSafe boo case autoOR: action = 'o' case autoSafe: - // auto-safe: skip non-safe items (they require prompting, but - // auto-safe only silently handles safe ones which never reach here). + // 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' @@ -603,87 +603,24 @@ func unescapeJSONPointer(tok string) string { return strings.ReplaceAll(strings.ReplaceAll(tok, "~1", "/"), "~0", "~") } -// collectItems builds the flat reconcile list from a rendered plan + state. +// collectReconcileItems builds reconcile's flat item list from a rendered plan +// + state with ONE walkPlanItems call. It answers two slices so the caller can +// keep reconcile's prompt order — ALL rendered items, then ALL orphans — rather +// than the walk's per-agent interleaving: items holds every whole-file / key +// item in walk order (agents in registry order, ops in plan order, merged keys +// sorted); orphans holds the whole-file dests agentsync still OWNS in state but +// NO enabled agent renders anymore (the source component was removed), offered +// for interactive delete/keep. +// // userHome (the user's $HOME) is the HomeRelative base for state-key lookups. // pluginOwners (pluginProvidedSourceIDs) tags each item whose component comes // from a plugin, so write-back can refuse it. -func collectItems(plan render.RenderPlan, reg *adapter.Registry, s *state.Targets, sc adapter.Scope, projectRoot, userHome string, pluginOwners map[string]string) []reconcileItem { - var items []reconcileItem - for _, name := range reg.Names() { - res, ok := plan.PerAgent[name] - if !ok { - continue - } - seen := map[string]bool{} - for _, op := range res.Ops { - if render.IsKeyMerge(op.MergeStrategy) { - // NOT deduped by path: one agent emits several key-merge ops to one - // file (codex /mcp_servers + /hooks → config.toml; claude /hooks + - // settings.json), each a distinct section, so every op - // must be walked. Deduping by path dropped the second section's - // items (matching status's key loop and the apply pipeline). - var ours map[string]interface{} - _ = json.Unmarshal(op.Content, &ours) - final := readDestFile(op.MergeStrategy, op.Path) - for _, ptr := range render.CollectPointers(ours, "") { - hsrc := hashAnyValue(getPointerValue(ours, ptr)) - happlied := s.Keys[stateKeyKey(userHome, name, sc, projectRoot, op.Path, ptr)].SHA256 - hdest := hashAnyValue(getPointerValue(final, ptr)) - cls := drift.Classify(hsrc, happlied, hdest) - items = append(items, reconcileItem{ - agentName: name, - op: op, - ptr: ptr, - cls: cls, - hsrc: hsrc, - happlied: happlied, - hdest: hdest, - scope: sc, - projectRoot: projectRoot, - srcText: marshalPretty(getPointerValue(ours, ptr)), - dstText: marshalPretty(getPointerValue(final, ptr)), - hasText: true, - pluginOwner: pluginOwnerForKeyItem(op.SourceID, ptr, pluginOwners), - }) - } - } else { - if seen[op.Path] { - continue - } - seen[op.Path] = true - hsrc := hashContent(op.Content) - happlied := s.Files[stateFileKey(userHome, name, sc, projectRoot, op.Path)].SHA256 - hdest := hashFile(op.Path) - cls := drift.Classify(hsrc, happlied, hdest) - dstBytes, _ := readDestBytes(op.Path) - items = append(items, reconcileItem{ - agentName: name, - op: op, - cls: cls, - hsrc: hsrc, - happlied: happlied, - hdest: hdest, - scope: sc, - projectRoot: projectRoot, - srcText: string(op.Content), - dstText: string(dstBytes), - hasText: true, - pluginOwner: pluginOwners[filepath.ToSlash(op.SourceID)], - }) - } - } - } - return items -} - -// collectOrphanFileItems returns reconcile items for whole-file dests that -// agentsync still OWNS in state but NO enabled agent renders anymore (the -// source component was removed). These are offered for interactive delete/keep. // -// A path that ANY enabled agent still renders is excluded — never offer to -// delete a file another agent depends on (the shared-skill case). Deduped by -// path so a file owned by two agents is prompted once. -func collectOrphanFileItems(plan render.RenderPlan, reg *adapter.Registry, s *state.Targets, sc adapter.Scope, projectRoot, userHome string) []reconcileItem { +// The walk's orphan set is per agent and unfiltered; reconcile narrows it: a +// path that ANY enabled agent still renders is excluded — never offer to +// delete a file another agent depends on (the shared-skill case) — and the +// rest is deduped by path so a file owned by two agents is prompted once. +func collectReconcileItems(plan render.RenderPlan, reg *adapter.Registry, s *state.Targets, sc adapter.Scope, projectRoot, userHome string, pluginOwners map[string]string) (items, orphans []reconcileItem) { rendered := map[string]bool{} for _, name := range reg.Names() { res, ok := plan.PerAgent[name] @@ -703,39 +640,41 @@ func collectOrphanFileItems(plan render.RenderPlan, reg *adapter.Registry, s *st } } seen := map[string]bool{} - var items []reconcileItem - for _, name := range reg.Names() { - res, ok := plan.PerAgent[name] - if !ok { - continue + walk := planWalk{ + plan: plan, agents: reg.Names(), state: s, userHome: userHome, scope: sc, projectRoot: projectRoot, + includeOrphans: true, + withText: true, + } + for _, it := range walkPlanItems(walk) { + ri := reconcileItem{ + agentName: it.agent, + op: it.op, + ptr: it.ptr, + cls: it.cls, + hsrc: it.hsrc, + happlied: it.happlied, + hdest: it.hdest, + scope: sc, + projectRoot: projectRoot, + orphan: it.orphan, } - for _, orphan := range render.OrphanFiles(s, userHome, name, sc, projectRoot, res.Ops) { - if rendered[orphan] || seen[orphan] { + if it.orphan { + if rendered[it.op.Path] || seen[it.op.Path] { continue } - seen[orphan] = true - entry := s.Files[stateFileKey(userHome, name, sc, projectRoot, orphan)] - happlied := entry.SHA256 - hdest := hashFile(orphan) - items = append(items, reconcileItem{ - agentName: name, - // SourceID matters: the reclaimable-KIND check behind this item's - // prompt wording is SourceID-keyed and silently degrades to - // "unknown kind" without it — which is exactly how that branch - // once shipped dead. Mode is deliberately NOT carried: this path - // removes via render.BackupFile + os.Remove, never Writer.Delete, - // so nothing reads it and setting it would only imply otherwise. - op: adapter.FileOp{Action: "delete", Path: orphan, SourceID: entry.SourceID}, - cls: drift.Classify("", happlied, hdest), - happlied: happlied, - hdest: hdest, - scope: sc, - projectRoot: projectRoot, - orphan: true, - }) + seen[it.op.Path] = true + orphans = append(orphans, ri) + continue + } + ri.srcText, ri.dstText, ri.hasText = it.srcText, it.dstText, true + if it.ptr != "" { + ri.pluginOwner = pluginOwnerForKeyItem(it.op.SourceID, it.ptr, pluginOwners) + } else { + ri.pluginOwner = pluginOwners[filepath.ToSlash(it.op.SourceID)] } + items = append(items, ri) } - return items + return items, orphans } // pruneStateFilesForPath removes every agent's Files state entry for a single diff --git a/internal/cli/status.go b/internal/cli/status.go index 662ce8a3..164de60a 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -281,68 +281,51 @@ func containsStar(names []string) bool { // buildStatusModel classifies every tracked file/key/orphan across agents into // the structured statusModel. It is the single source of truth both the // formatted dashboard and --json render from. +// +// The classification itself is walkPlanItems'; status adds its presentation on +// top. An agent with a plan result is listed even with zero items ("(no +// tracked items)"). Each agent's rows are partitioned whole-file → merged key +// → orphan — a stable re-ordering of the single-pass walk, which yields ops in +// plan order — and permission drift is folded into the class of a +// content-clean whole file. func buildStatusModel(plan render.RenderPlan, names []string, s *state.Targets, userHome string, sc adapter.Scope, projectRoot string) statusModel { model := statusModel{Summary: map[string]int{}} + byAgent := map[string][]planItem{} + // withText stays false: status hashes op.Content but never shows it. + for _, it := range walkPlanItems(planWalk{ + plan: plan, agents: names, state: s, userHome: userHome, scope: sc, projectRoot: projectRoot, + includeOrphans: true, + }) { + byAgent[it.agent] = append(byAgent[it.agent], it) + } for _, name := range names { - res, ok := plan.PerAgent[name] - if !ok { + if _, ok := plan.PerAgent[name]; !ok { continue } ag := statusAgent{Agent: name} - seen := map[string]bool{} - // file-level: every non-key-merge op is a whole-file item (including the - // "replace" strategy used by skills/subagents/commands/memory). - for _, op := range res.Ops { - if render.IsKeyMerge(op.MergeStrategy) { - continue // covered key-by-key below - } - if seen[op.Path] { - continue - } - seen[op.Path] = true - entry := s.Files[stateFileKey(userHome, name, sc, projectRoot, op.Path)] - hsrc := hashContent(op.Content) - happlied := entry.SHA256 - hdest := hashFile(op.Path) - cls := drift.Classify(hsrc, happlied, hdest).String() - // A file whose CONTENT is clean but whose permission bits drifted from - // what agentsync last applied is still drift — the next apply re- - // converges the mode (render.Writer.Write chmods a content-identical - // file whose mode differs). Without this, a skill script that lost its - // +x bit reports "clean" yet the next apply would change it. - if cls == drift.Clean.String() && modeDrifted(entry.Mode, op.Path) { - cls = drift.Drift.String() - } - ag.Items = append(ag.Items, statusItem{Path: op.Path, Class: cls}) - model.Summary[cls]++ - } - // key-level: walk owned pointers for each merge op. - for _, op := range res.Ops { - if !render.IsKeyMerge(op.MergeStrategy) { - continue - } - var ours map[string]any - _ = json.Unmarshal(op.Content, &ours) - final := readDestFile(op.MergeStrategy, op.Path) - for _, ptr := range render.CollectPointers(ours, "") { - hsrc := hashAnyValue(getPointerValue(ours, ptr)) - happlied := s.Keys[stateKeyKey(userHome, name, sc, projectRoot, op.Path, ptr)].SHA256 - hdest := hashAnyValue(getPointerValue(final, ptr)) - cls := drift.Classify(hsrc, happlied, hdest).String() - ag.Items = append(ag.Items, statusItem{Path: op.Path, Pointer: ptr, Class: cls}) + items := byAgent[name] + // Stable partition: whole-file → merged key → orphan. + for _, pass := range []func(planItem) bool{ + func(it planItem) bool { return it.ptr == "" && !it.orphan }, + func(it planItem) bool { return it.ptr != "" }, + func(it planItem) bool { return it.orphan }, + } { + for _, it := range items { + if !pass(it) { + continue + } + cls := it.cls.String() + // Content clean but permission bits drifted from what agentsync + // RECORDED is still drift: the next apply re-chmods it. Whole-file + // items only — a merged key has no mode. (An orphan classifies + // clean only from a hand-corrupted state entry, so no exclusion.) + if it.ptr == "" && cls == drift.Clean.String() && it.recordedModeDrifted() { + cls = drift.Drift.String() + } + ag.Items = append(ag.Items, statusItem{Path: it.op.Path, Pointer: it.ptr, Class: cls}) model.Summary[cls]++ } } - // orphans: whole-file dests this agent still owns in state but no longer - // renders (the source component was removed). Without these, status - // reports nothing for a file that lingers and the next apply/reconcile - // would act on. - for _, orphan := range render.OrphanFiles(s, userHome, name, sc, projectRoot, res.Ops) { - happlied := s.Files[stateFileKey(userHome, name, sc, projectRoot, orphan)].SHA256 - cls := drift.Classify("", happlied, hashFile(orphan)).String() - ag.Items = append(ag.Items, statusItem{Path: orphan, Class: cls}) - model.Summary[cls]++ - } model.Agents = append(model.Agents, ag) } return model @@ -1009,26 +992,6 @@ func hashFile(path string) string { return hashContent(data) } -// modeDrifted reports whether the regular file at path exists with permission -// bits that differ from the mode agentsync last recorded for it (state -// FileEntry.Mode). A recorded mode of 0 means "unspecified" (older state, or an -// op whose adapter left Mode unset — the writer defaults those to 0o644 on -// write), so it never counts as drift. A missing, symlinked, or non-regular file -// is left to the content classifier (which already flags it), so this returns -// false there. It is the permission-bit analog of the content-hash drift the -// classifier detects: a content-identical chmod is real drift the next apply -// re-converges (render.Writer.Write). -func modeDrifted(recordedMode uint32, path string) bool { - if recordedMode == 0 { - return false - } - fi, err := os.Lstat(path) - if err != nil || fi.Mode()&os.ModeSymlink != 0 || !fi.Mode().IsRegular() { - return false - } - return fi.Mode().Perm() != os.FileMode(recordedMode).Perm() -} - func hashAnyValue(v any) string { if v == nil { return "" diff --git a/internal/cli/status_test.go b/internal/cli/status_test.go index 9762e822..ed83496a 100644 --- a/internal/cli/status_test.go +++ b/internal/cli/status_test.go @@ -743,3 +743,72 @@ func TestStatus_LegendRejectsConflictingFlags(t *testing.T) { }) } } + +// TestStatusJSON_NeverEmitsResolvedSecret pins that `status --json` (and the +// dashboard) never carries a resolved secret. status builds its plan from +// secrets.SubstituteCanonical, so every whole-file op.Content and merged-key +// value it HASHES is cleartext; the payload must stay paths, pointers, classes +// and counts. +// +// This is a two-fault tripwire, not a single-fault detector: the payload can +// leak only if buildStatusModel asks the walk for text (withText: true) AND +// statusItem grows a field that carries it. The real guard is +// TestPlanItemIsNotASerializationSurface — a planItem has no exported field and +// no json tag, so it cannot be marshalled by accident; this is the end-to-end +// backstop behind it, on the same fixture TestDiff_DoesNotLeakResolvedSecrets +// uses. +func TestStatusJSON_NeverEmitsResolvedSecret(t *testing.T) { + const sentinel = "ghp_SENTINEL_DO_NOT_LEAK_THIS_TOKEN_123456789" + + tmp := t.TempDir() + env := map[string]string{ + "AGENTSYNC_TARGET_ROOT": tmp, + "GITHUB_TOKEN": sentinel, + } + if _, err := runCLI(t, env, "init"); err != nil { + t.Fatal(err) + } + if _, err := runCLI(t, env, "agent", "add", "claude"); err != nil { + t.Fatal(err) + } + mcp := filepath.Join(tmp, ".agentsync", "mcp", "github.toml") + _ = os.MkdirAll(filepath.Dir(mcp), 0o755) + body := `[server] +type = "stdio" +command = "npx" +args = ["-y", "@modelcontextprotocol/server-github"] + +[server.env] +GITHUB_TOKEN = "${env:GITHUB_TOKEN}" +` + _ = os.WriteFile(mcp, []byte(body), 0o644) + if _, err := runCLI(t, env, "apply"); err != nil { + t.Fatal(err) + } + + // Setup invariant: the destination really holds the cleartext, so a leak + // is possible and the assertion below is not vacuous. Then drift it, so + // the item is not merely "clean". + dst := filepath.Join(tmp, ".claude.json") + dstBytes, err := os.ReadFile(dst) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(dstBytes), sentinel) { + t.Fatalf("setup invariant failed — dest does not contain sentinel; got: %s", dstBytes) + } + _ = os.WriteFile(dst, []byte(strings.ReplaceAll(string(dstBytes), `"npx"`, `"npm"`)), 0o644) + + for _, args := range [][]string{{"status", "--json"}, {"status"}} { + out, err := runCLI(t, env, args...) + if err != nil { + t.Fatalf("%v: %v\n%s", args, err, out) + } + if strings.Contains(out, sentinel) { + t.Fatalf("SECURITY: %v output leaked sentinel secret %q\n%s", args, sentinel, out) + } + if !strings.Contains(out, "drift") { + t.Fatalf("%v: expected the drifted MCP key to be reported; got:\n%s", args, out) + } + } +} diff --git a/website/src/content/docs/guides/daily-loop.mdx b/website/src/content/docs/guides/daily-loop.mdx index 2fae9432..953a98ba 100644 --- a/website/src/content/docs/guides/daily-loop.mdx +++ b/website/src/content/docs/guides/daily-loop.mdx @@ -63,14 +63,16 @@ flags instead of the menu: | --- | --- | | `--auto-writeback` | Adopt every destination edit. | | `--auto-override` | Re-impose source everywhere, discarding edits. | -| `--auto-safe` | Only auto-resolve changes that **can't lose work** (`converged`, `pending`). | +| `--auto-safe` | Resolve nothing; list every item as left unresolved. | ```bash agentsync reconcile --auto-safe ``` -`--auto-safe` is the conservative default for automation: it converges the -no-risk cases and leaves anything that could discard work for a human. +`--auto-safe` is the conservative choice for automation: nothing that reaches +`reconcile` is safe to resolve unattended (`converged` and `pending` never reach +it — the next `apply` handles those), so it changes nothing and reports what +needs a human. ` | **root flag** — same set as `--scope` | Target a specific project's tree (implies `--scope project`). | | `--dry-run` | apply, import | Preview without writing. | -| `--auto-safe` | reconcile | Auto-resolve only no-risk drift classes. (The plugin-side flag of the same name became `plugin upgrade --lossless` — they meant two unrelated things.) | +| `--auto-safe` | reconcile | Resolve nothing; report every item as left unresolved — nothing that reaches `reconcile` is risk-free. (The plugin-side flag of the same name became `plugin upgrade --lossless` — they meant two unrelated things.) | | `ls` / `rm` | every `list` / `remove` subcommand | Aliases. | | `--json` | status, diff, explain, plugin explain | Emit the structured payload to stdout (advisory diagnostics still go to stderr). `plugin explain --json` emits the translation `rows`; `explain --json` emits the provenance envelope (and still emits it, with `"unmanaged": true`, on a path nothing renders). | | `--agents ` | apply, status, diff, reconcile, revert | The single "which agents" selector, with identical parsing everywhere (`*` = all enabled; an empty or unknown value is rejected identically by all five). On `revert` it is a spelling of the positional form — `--agents`, a positional agent, and `--all` stay mutually exclusive. |