refactor(cli): one shared plan→drift walk behind status, diff, reconcile and explain (#229 PR-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

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.
 
 ---
 

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 <agent>` 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)`.

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.
 
 ---
 

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 <path> 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 <path> 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
+}