refactor(cli): one shared plan→drift walk behind status, diff, reconcile and explain (#229 PR-B) - #244
Merged
Merged
Conversation
Move the hunk-collection loop in `diff`'s RunE verbatim into collectDiffHunks(plan, names, filterPath, redact) (hunks, filterMatched). RunE calls it with reg.Names(); the secret-redaction map is still built at the same call site and the filter-miss error, --json, "no diff" and --exit-code handling are untouched. No logic change: same per-agent `seen` map placement, same `filterMatched := filterPath == ""` initialiser set on op MATCH (before the key-merge/whole-file branch), same modeHunk(op.Path, op.Mode) call, same never-dedupe-key-merge-ops-by-path rule. diff_test.go passes unmodified. This is the first step of #229 PR-B: the four plan→drift walks (status, diff, reconcile, explain) get a characterization harness next, and that harness needs diff's walk callable without a cobra command. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Pin, fixture by fixture, what buildStatusModel (S), collectDiffHunks (D), collectItems ++ collectOrphanFileItems (R) and buildExplainModel (E) answer TODAY, so #229's shared walk can be introduced behind them with zero harness edits — a conversion that needs a golden changed is not a refactor. 26 fixtures (T-01..T-24, with orphan and empty-plan split in two) cover every whole-file drift class, orphan / orphan-drifted, mode-only drift (status folds the RECORDED mode in, diff hunks against op.Mode, reconcile and explain ignore it), a symlinked destination (hash side answers the sentinel, text side reads through — axis 9, preserved), per-agent whole-file dedupe vs never-dedupe for key-merge ops, five sorted pointers, JSONC / unparseable / missing merged destinations, RFC-6901-escaped ids, the shared-orphan asymmetry between status and reconcile, project scope, and three ordering fixtures: status's whole-file-before-key partition, reconcile's all-items-before-all- orphans composition, and an emptied "{}" key-merge section whose zero items must still count as a path match for diff and explain. Goldens are literal values from the classifier table and each surface's rules, confirmed against unmodified production code. Only pointer order inside one (agent, path) run is normalized — render.CollectPointers ranges a map — everything else is compared in order. The S projection carries the statusAgent skeleton so an agent with zero items stays visible; a fixture other than plan/empty that projects nothing on S, D and R fails. TestPathFilterFlagsSurviveAZeroItemOp pins that diff's filterMatched and explain's pathManaged are side effects of the op MATCH, not of the item count; both hold today. Verified the harness can fail: swapping status's two passes, interleaving reconcile's orphans per agent, deriving either filter flag from the item count, deduping key-merge ops by path, and dropping the whole-file per-agent dedupe each turned exactly the intended fixture red. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Add internal/cli/planwalk.go: one walk that classifies every destination a rendered plan touches — whole files, merged keys, whole-file orphans — against state and the on-disk contents, composing render.IsKeyMerge, render.CollectPointers, render.OrphanFiles, the guarded destination readers (hashFile / readDestFile / readDestBytes) and drift.Classify. It is purely additive: status, diff, reconcile and explain still run their own copies; the next four commits convert each onto this one. Contract (all pinned by TestWalkPlanItems): agents in the caller's order, ops in plan order, pointers SORTED within one key-merge op (#229 axis 13 — CollectPointers ranges a map), whole-file ops deduped by path per agent, key-merge ops NEVER deduped (one agent emits several to one file), orphans appended after their agent's ops and unfiltered across agents, matchOp called exactly once per op that survives the Action filter (side effects are the intended use — amendment A3), withText governing only srcText/dstText (amendment A5). A key-merge destination is decoded once per op, not per pointer (axis 5; TestWalkPlanItems_ReadsKeyMergeDestOncePerOp via the readDestConfig seam). The whole-file hash side keeps hashFile's unconditional symlink sentinel while the text side reads through the link (reconcile's existing split, D2) — dstText is set only when the guarded read returns no error (amendment B4). Mode facts travel as recordedMode / destPerm / destRegular from destModePerm, which answers (perm, regular) so chmod 000 is not confused with absent (amendment A7). Two methods carry the two questions: recordedModeDrifted (status's, modeDrifted's exact truth table) and opModeDrifted (diff's, modeHunk's gate). Nothing changes which one a surface asks — that is PR-C (axis 14). Every planItem field is unexported, deliberately: op.Content carries resolved cleartext for status and explain, and encoding/json ignoring unexported fields makes "never a --json surface" a property of the type. TestPlanItemIsNotASerializationSurface fails on an exported field or a json tag; no json tags, no logging, no secrets.* call in the walk. Tests: TestWalkPlanItems (16 subtests, the FIFO one in a unix-only file), the once-per-op read count, the serialization guard, and TestPathFilterFlagsSurviveAZeroItemOp moved here verbatim from the characterization harness (body unchanged). Break-verified: dropping the pointer sort, the per-agent whole-file dedupe, adding a key-merge dedupe, hashing through the symlink, reading the destination per pointer, exporting a field and adding a json tag each turned exactly the intended test red. Behavior-preserving; characterization harness unchanged (N-16 relocated only). No new package edges (go list -deps ./internal/cli still has one drift edge). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Replace collectItems' and collectOrphanFileItems' private copies of the plan→drift walk with ONE walkPlanItems call (includeOrphans, withText) in collectReconcileItems, which returns the rendered items and the orphans as two slices so reconcileRun keeps its prompt order — ALL rendered items, then ALL orphans — instead of the walk's per-agent interleaving. Reconcile's own rules stay on top of the walk, verbatim: the `rendered` cross-agent exclusion (never offer to delete a file another enabled agent still renders, computed from the plan with its `op.Action != "write"` skip), the global per-path dedupe of orphans, the two pluginOwner spellings, and hasText = !orphan. collectItems and collectOrphanFileItems remain as the two halves of that single call — the composition the characterization harness pins — with no second walk in production. Behavior changes, both deliberate and the only ones in this PR: - axis 13: merged keys are now listed in sorted pointer order, so the prompt queue no longer shuffles between runs (render.CollectPointers ranges a map); TestMergedKeyOrderIsDeterministic/reconcile pins it. - amendment B4: a whole-file dstText is now "" on ANY read error rather than keeping the partial bytes os.ReadFile returns alongside an EIO mid-read — diff's semantics, not reproducible without fault injection. Otherwise behavior-preserving; characterization harness unchanged. Break-verified: appending orphans in walk order fails only T-23 (order/two-agents-orphan-then-ops); dropping the rendered exclusion fails T-19; hasText on orphans fails T-08 and T-23; dropping the walk's pointer sort fails the new ordering test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
…modeDrifted)
buildStatusModel now projects walkPlanItems' output (includeOrphans,
withText false) instead of running its own two-pass copy of the walk.
Status's presentation stays on top, verbatim in effect: every name with
a plan result gets a statusAgent even with zero items ("(no tracked
items)"), each agent's rows are a stable partition of the walk's output
— whole-file, then merged keys, then orphans — and a content-clean
whole file whose permission bits differ from the RECORDED mode is folded
into `drift`. That upgrade applies to rendered whole-file items only; a
merged key has no mode and an orphan's class is the classifier's alone.
modeDrifted is deleted: its Lstat/symlink/IsRegular triage is
destModePerm and its formula is planItem.recordedModeDrifted, both from
the walk. iss162_internal_test.go's four modeDrifted assertions are
rewritten in that form — same four truth-table rows, now exercising
destModePerm's filesystem triage and the pure formula separately — plus
one new row pinning that status compares against the recorded mode and
not op.Mode (recorded 0644 == disk, op.Mode 0755 → clean), which is the
one-line switch #229 PR-C owns (axis 14). The modeHunk half of that test
is untouched here; commit 7 converts it.
Behavior change (axis 13, deliberate): merged keys are listed in sorted
pointer order, so `status --json` is reproducible run to run;
TestMergedKeyOrderIsDeterministic/status pins it. Otherwise
behavior-preserving; characterization harness unchanged.
TestStatusJSON_NeverEmitsResolvedSecret is added as an end-to-end
backstop: status renders from secrets.SubstituteCanonical, so every
op.Content it hashes is cleartext. It is a two-fault tripwire (withText
on AND a text field on statusItem), not a single-fault detector — the
real guard is TestPlanItemIsNotASerializationSurface.
Break-verified: a single pass in walk order fails only T-22
(order/key-merge-op-before-whole-file); swapping recordedModeDrifted for
opModeDrifted fails the new iss162 row (T-09's status golden is `drift`
under both formulas, since recorded 0755, op 0700 and disk 0644 all
differ, so it cannot discriminate); dropping the walk's pointer sort
fails the status ordering row; the two-fault mutation fails the secret
backstop. Dropping the `!it.orphan` guard alone is unobservable — an
orphan never classifies clean — while upgrading an orphan's class on
mode drift fails TestWalkPlanItems/orphan-ignores-mode.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
collectDiffHunks now projects walkPlanItems' output (withText, no
orphans) in a single pass instead of running its own copy of the walk.
The <path> filter is the walk's matchOp closure, and filterMatched is
set INSIDE it on op match — before any item exists — so a matching op
that yields no item (an emptied "{}" section) still counts as a managed
path rather than a typo (#229 amendment A3). Whether a hunk prints is
still 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 (A6).
secrets.MaskResolved, UnresolvedSecretRefs and CollectResolved stay at
their exact call sites; the walk never sees them.
diff has no "applied" side and consulted no state before, so the walk
runs against an empty state.New() at this call site (the harness pins
the four-argument signature) and its classes go unused. diff reads only
the walk's dstText, which follows symlinks exactly as its own
readDestBytes did — the hash the walk also computes is discarded (spec
risk R5, accepted: an extra Lstat plus one read per whole-file op, not
observable).
modeHunk becomes a pure formatter over the walk's mode facts —
modeHunk(it planItem), printing op.Mode and destPerm through .Perm() as
before, gated by planItem.opModeDrifted (its exact Lstat/symlink/
IsRegular/0-mode rule). iss162_internal_test.go's three modeHunk
assertions are updated to that signature — same three cases, same
"mode 0755" / "mode 0644" strings — with the on-disk side taken from
destModePerm on the real chmod'd file.
Behavior change (axis 13, deliberate): merged keys are diffed in sorted
pointer order; TestMergedKeyOrderIsDeterministic/diff pins it.
Otherwise behavior-preserving; characterization harness unchanged, and
diff_test.go passes unmodified.
Break-verified: deriving filterMatched from the hunk count fails T-24
(key-merge/emptied-section), T-17, TestPathFilterFlagsSurviveAZeroItemOp
and TestDiff_UnmanagedPathDistinctFromClean; dropping the walk's pointer
sort fails the diff ordering row; deciding "differs" by class instead
of masked text fails seven characterization fixtures.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
buildExplainModel's op loop now projects walkPlanItems' output (no
orphans, no text) grouped by agent in in.agents order, instead of
running its own copy of the walk. The target match is the walk's
matchOp closure, and pathManaged and keyMergers are recorded INSIDE it
on the op match — never derived from the yielded items — because both
must hold for an op that yields zero items (an emptied "{}" section;
#229 amendment A3). keyMergers is verbatim today's rule and unobservable
on any real plan (an owner is appended only when it has items, and
foreign rows attach only to owners); it is kept as-is rather than
re-derived (amendment B3).
fileItem and keyItem take the planItem and use its happlied for
ownership and its cls for Drift. keyItem no longer calls readDestFile
per pointer: the walk decodes a key-merge destination ONCE per op, so
every pointer of one file classifies against the same snapshot — #229
axis 5, the second of this PR's two deliberate behavior changes
(TestWalkPlanItems_ReadsKeyMergeDestOncePerOp pins it at the walk).
foreignPointers keeps its own readDestFile: it decodes the destination
for a different purpose (the un-rendered remainder) and is not part of
the walk. explain still ignores mode drift (PR-C), still sorts each
owner's items by pointer, and everything after the loop — foreign keys,
crossAgentConflicts, Unmanaged, pathManaged — is unchanged.
With this, the four copies are gone: drift.Classify is called from
internal/cli/planwalk.go only. Behavior-preserving; characterization
harness unchanged, explain_path_test.go passes unmodified.
Break-verified: deriving pathManaged from the item count fails T-24
(key-merge/emptied-section), T-17 and TestPathFilterFlagsSurviveAZeroItemOp;
moving the walk's destination read inside the pointer loop fails the
once-per-op test. Recording renderedPointers after the pointer filter
is unobservable (the foreign loop already narrows to in.pointer, which
is rendered) and the order is kept verbatim.
Two comments that named the retired status helper are reworded so no
reference to it remains under internal/.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
docs/architecture.md §6: add the paragraph describing walkPlanItems — the one iterator behind status, diff, reconcile and explain, what it composes, the per-agent whole-file dedupe vs never-deduped key-merge ops, sorted merged keys, and the presentation each surface keeps on top. Correct two sentences that were false before #229 PR-B and stayed false: `drift.SafeForAutoApply` is not what `reconcile --auto-safe` consults (it has no production caller; --auto-safe reports every item that reaches its loop as skipped and leaves orphans in place), and an unparseable structured destination does not degrade to a file-level item — readDestFile decodes it to an empty document, so each owned pointer classifies against an absent value. docs/components.md: add walkPlanItems to internal/cli's Key bullet with the unexported-planItem rationale, list the shared helpers (destread.go, planwalk.go) on its Files line, drop the false `drift` dependency from internal/render (go list -deps shows none), and note under internal/drift where the walk that feeds it lives. CHANGELOG.md [Unreleased] › Changed (existing heading, entries added at its top): merged keys are now listed in a stable sorted order by `status --json`, `diff` and `reconcile` (measured before: five distinct orderings across 200 CollectPointers calls); the four walks are one, with `explain` decoding a key-merged destination once per section rather than once per key, and the pre-existing per-surface disagreements preserved deliberately for PR-C. No CLI, flag, agent, component or schema change, so README, the user guide, the capability matrix and the website reference pages are untouched; the website's contract pages regenerate from docs/*.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
… contracts, fix three stale --auto-safe docs Round 1 of the review loop on #244 (four lenses, no BLOCKER): - collectItems / collectOrphanFileItems had no production caller after the walk landed; they existed only so the harness could compose them, and made its R projection walk the plan twice. Deleted; the harness and the order test call collectReconcileItems once and concatenate. - Two contracts the mutation sweep found unpinned: reconcile's GLOBAL orphan dedupe (two agents owning one orphan path must prompt once) gets fixture T-25, whose golden was confirmed against the unmodified production code at the harness commit; and matchOp being offered a duplicate whole-file op BEFORE the per-agent path dedupe gets a second op in the once-per-op test. - The PR corrected docs/architecture.md's claim that --auto-safe converges no-risk classes, but docs/user-guide.md, the daily-loop guide and the CLI reference still said it. requiresAction admits only drift, conflict, orphan-drifted and foreign-collision, so nothing --auto-safe sees is safe to resolve; all three now say it resolves nothing and reports what needs a human. - TestPlanItemIsNotASerializationSurface also rejects an embedded field and a json.Marshaler implementation, the two ways an all-unexported struct can still marshal. - Trimmed: an unreachable !it.orphan guard in status's mode fold (an orphan never classifies clean), a redundant ModeSymlink test in destModePerm (Lstat's mode for a link already fails IsRegular), an eight-line comment in reconcile that restated planwalk.go, the state doc line that contradicted diff's state.New(), and the CHANGELOG entry's issue narrative. The harness header now names T-09/T-10 as PR-C's expected edits. Behavior-preserving; characterization harness goldens unchanged (one added). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
…, pin two more contracts, trim duplicated comments Round 2 of the review loop on #244 (four lenses, no BLOCKER): - `reconcile --help` still described --auto-safe as "auto-resolve only converged/pending/new (no-op)" — the one copy of the claim round 1's doc sweep missed, and the one users actually read. It now says what the code does: non-interactive, resolves nothing, reports every item that needs a human. The stale comment in the auto-safe arm goes with it. - destModePerm's documented "chmod 000 is (0, true), absent is (0, false)" had no test; a mutation conflating them stayed green. TestDestModePerm pins regular / chmod 000 / absent / symlink / directory. - TestPlanItemIsNotASerializationSurface also rejects encoding.TextMarshaler: encoding/json falls through to it after json.Marshaler, and a MarshalText on planItem was measured marshalling real content past the guard. - Three test comments still named the shims round 1 deleted. - T-25's extra assertion duplicated its wantR DeepEqual; dropped. Its comment now also states the E golden: explain calls a state-owned, unrendered path unmanaged because it never consults orphans. - Comments that restated an adjacent doc comment verbatim are cut in diff.go, status.go, planwalk.go and explain_model.go; the absolute "an orphan never classifies clean" is now scoped to state agentsync writes. Behavior-preserving except the help string; harness goldens unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Round 3 of the review loop on #244: three lenses CLEAN, one minor ISSUE — the help-string fix in round 2 is user-visible, so CLAUDE.md's doc-sync table requires an [Unreleased] entry. Nothing else changed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
This was referenced Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second of three PRs for #229 (after #240, the destination-read gate).
status,diff,reconcileandexplaineach held their own copy of the plan→state→destination drift walk, and the copies disagreed. This PR extracts one walk,walkPlanItemsininternal/cli/planwalk.go, and rebuilds all four surfaces on it. The four copies are gone;drift.Classifynow has three call sites, all inplanwalk.go.Strictly behavior-preserving, with exactly two deliberate changes:
render.CollectPointersranges a Go map; measured five distinct orderings across 200 calls.status --json,diffhunks andreconcile's prompt queue are now reproducible run to run.explainalready sorted and is unchanged.explaindecodes a key-merged destination once per op, not once per pointer (axis 5). Removes an O(pointers) read amplification and a window where two keys of one file classified against different snapshots.Everything else the copies disagreed on is preserved on purpose so each policy decision gets its own review in PR-C: mode drift (
statusmeasures against the recorded mode,diffagainstop.Mode,reconcile/explainignore it), and the symlinked-destination split (hashFilerefuses, the text read follows).How preservation is proved. Commit 2 lands a characterization harness (
planwalk_characterization_test.go, 26 fixtures × four golden projections S/D/R/E) that was green against the unmodified code before any production change, and it is untouched through every conversion. The harness sorts only pointers within one(agent, path)run, so it stays blind to exactly the one thing that was nondeterministic and to nothing else; three fixtures (order/key-merge-op-before-whole-file,order/two-agents-orphan-then-ops,key-merge/emptied-section) exist specifically because a fresh plan review showed the ordering and filter-flag preservation would otherwise have had no failing case.Design points worth a reviewer's eye:
internal/cli, notinternal/renderas the original plan proposed. Post-fix(cli): stop status, diff and reconcile hanging on a non-regular destination #240 every guarded read helper is incli,internal/renderstill has seven bareos.ReadFile(op.Path)sites (apply and apply --dry-run hang forever on a non-regular destination #241), andTestEveryDestinationReadGoesThroughTheGateis deliberately scoped tocli. Moving would have meant ~230 lines of helper relocation for zero behavior change.internal/driftstays a pure leaf.planItemis unexported field-for-field.statusandexplainbuild their plan fromsecrets.SubstituteCanonical, soop.Contentcarries resolved cleartext;encoding/jsonignoring unexported fields makes "never a--jsonsurface" a property of the type, pinned by a reflection test.matchOpis a side-effect seam by design.diff'sfilterMatchedandexplain'spathManagedare set inside the closure, not derived fromlen(items): a synthesized orphan-cleanup op hasContent: "{}", yields zero items, and must still count as "managed" ordiff <path>on an emptied section turns from "no diff" into an error.modeDriftedis retired; its four truth-table rows survive asrecordedModeDriftedassertions iniss162_internal_test.go.Not quite byte-identical, recorded rather than hidden:
reconcile'sdstTexton anEIOmid-read now shows""instead of a truncated prefix (os.ReadFilereturns partial data with the error;diffalready discarded it, and the walk cannot match both).diffnow also computes and discards a content hash per whole-file op.Pre-existing, equal across all four, not fixed here: no surface reports a key-merge orphan, because synthesized
{}cleanup ops yield no pointers.difffilters byfilepath.Abswhileexplainresolves symlinks (axis 17).reconcileplans from the templated model (#239).Size, honestly: net production +201 lines (
planwalk.gois 277, roughly half doc comments that carry the ordering, security and side-effect contracts); tests +2077, of which the harness is 1074. Given #243, the doc-comment density inplanwalk.goand the twoCHANGELOGentries are fair game for trimming in review.Closes nothing on its own; #229 closes with PR-C.
Type of change
Test plan
go build ./...,AGENTSYNC_TEST_IN_CONTAINER=1 go test -race ./internal/cli/..., full suite and BDD green after every commit (justis not installable in this container; the recipes were run directly).GOTOOLCHAIN=go1.26.2 go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.12.2 run ./...→ 0 issues; gofmt/gofumpt clean.planwalk_characterization_test.go,planwalk_internal_test.go(walk unit tests incl. once-per-op read count and the no-serialization guard),planwalk_unix_internal_test.go(FIFO),planwalk_order_internal_test.go(determinism forstatus --json,diff,reconcile),TestStatusJSON_NeverEmitsResolvedSecret.Checklist
internal/secrets,internal/capture, or anysource.Write*path; the walk never sees asecrets.Resolvedor calls.Canonical(); no newforbidigoexclusion.docs/architecture.md§6 (the shared walk, plus two corrected claims:SafeForAutoApplyhas no production caller, and an unparseable structured dest classifies per pointer, not file-level),docs/components.md(internal/cli,internal/renderno longer falsely listsdrift,internal/drift),CHANGELOG.md.🤖 Generated with Claude Code
https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Generated by Claude Code