fix(cli): mode drift measured against what apply writes; symlinked destinations follow apply's policy on every surface (#229 PR-C) - #247
Merged
Conversation
…t it in explain #229 axis 14. `status` folded permission drift into a content-clean whole file's class by comparing the destination's bits against the mode RECORDED at the last apply, while `diff`'s mode hunk asked the question that actually predicts the next apply: does the destination differ from `op.Mode`, the mode render.Writer.Write chmods to? The two disagreed whenever the recorded mode was unset (state written before modes were recorded) or an adapter changed the mode it renders — `status` said `clean` while the next apply would chmod the file. `explain` never asked either question, so a content-identical chmod made `status` say `drift` and `explain` say `clean` about the same file in the same second. One method now answers for both: planItem.classWithModeDrift upgrades a content-clean whole-file item to drift when opModeDrifted — the gate diff's modeHunk already used — fires. `status` and `explain`'s fileItem read it; `diff` is unchanged; `reconcile` still ignores mode entirely (tracked separately). recordedMode and recordedModeDrifted are deleted: nothing reads them, and the orphan exclusion status carried is now structural (a synthesized orphan op has Mode 0). User-visible: `status` reports `drift` for a file whose bits differ from what the next apply writes, and `explain <path>` reports a mode-only drift instead of `clean`. Tests: iss162's truth table moves to opModeDrifted 1:1; its status assertion flips and becomes TestStatus_ModeDriftUsesOpModeNotRecordedMode, the one fixture (recorded == disk, op.Mode differs) on which the two formulas disagree. TestExplain_ReportsModeDrift pins status/explain agreement end to end. The characterization harness moves exactly one golden: T-09's E projection (`clean` → `drift`). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
#229 axis 9. `apply` writes THROUGH a symlinked destination only under AGENTSYNC_ALLOW_SYMLINK_DEST=1 — the documented chezmoi/Stow setup — but the read side never consulted the switch: `status`, `reconcile` and `explain` hashed the link itself and answered a sentinel that can never equal a content hash, so in exactly that configuration every run reported `drift` no apply could clear and `status --exit-code` failed CI forever, while `diff` read through the link and printed `no diff`. One gate now owns the read-side decision: destReadPath in internal/cli/destread.go, which mirrors iox.resolveSymlinkDest through the new shared iox.SymlinkDestAllowed (the single reading of the env). The three whole-file destination facts — hashFile (→ symlinkSentinel), destModePerm (→ (0, false)) and the walk's text read, now readDestText (→ "") — pass through it, so with the switch set all four surfaces resolve the link and compare the file apply converges, and with it unset all four refuse the link together. The mirror is a policy, not a prediction: apply itself only fails when the content differs. User-visible, switch set: a converged symlinked destination reports `clean` and `diff` prints nothing. Switch unset: `diff` prints a `symlink` hunk (`--json` pointer "symlink", beside the existing "mode" pseudo-pointer) naming the switch instead of reading through; the Dest text is a constant so an attacker-chosen link target never reaches the terminal. `reconcile` shows such an item with the SHA display rather than the whole source as an insertion against an empty destination, and its [w]rite-back refuses to capture through a link the classification did not read through, naming the switch and that every command needs it ([o]verride stays offered: Writer.Write's convergence read follows the link and either no-ops or fails cleanly with ErrSymlinkDest). Scope, deliberately: the policy covers WHOLE-FILE facts only. A key-merged destination (a symlinked ~/.claude.json) is still decoded through the link by readDestFile on every surface, as apply treats it — a converged symlinked key-merge dest is a no-op plus a chmod through the link, a differing one is ErrSymlinkDest — and refusing it would make every owned pointer classify against <absent> permanently, since the classifier has no per-pointer sentinel to carry. readDestBytes and import's reads are unchanged. Tests: TestSymlinkedDestConvergesWhenAllowed and TestSymlinkedDestIsDriftWhenRefused drive the real CLI through a pre-created link with the switch set and unset; TestHashFileSentinels, TestWalkPlanItems and TestDestModePerm gain the opt-in rows; TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough pins the write-back gate both ways; TestSymlinkDestAllowed pins the env reading. The characterization harness moves exactly one fixture, T-10 (D gains the symlink hunk; R loses its destination text), plus the normalizeRuns key learning the "symlink" label beside "mode". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Doc-sync for the two policy changes in the preceding commits, so no sentence in the contract docs, the user-facing docs or the website is left describing behavior that no longer holds. Mode (#229 axis 14): architecture.md §6 now says `status` and `explain` fold permission drift measured against `op.Mode` — the mode the next apply chmods to, the same question `diff`'s `mode` hunk asks — through one method, and that `reconcile` still ignores mode (tracked as its own issue). Symlinks (#229 axis 9): every place that described AGENTSYNC_ALLOW_SYMLINK_DEST as a write-only switch (README, user-guide, capability-matrix, SECURITY.md, the website's environment, troubleshooting and security pages) now says it governs reads on every surface and is therefore needed by every command, not only `apply`; what the four surfaces answer with it unset (drift, a `symlink` hunk naming the switch, the SHA display and a refused [w]); and that the rule covers WHOLE-FILE destinations only — a key-merged file such as ~/.claude.json is read through the link either way, as apply treats it, with the real reason (the classifier has no per-pointer sentinel; a refusal would classify every owned pointer against <absent> forever). architecture.md §6 carries the full rationale and §7 item 4 the read-path consequence; components.md gains destReadPath/readDestText and iox.SymlinkDestAllowed, and lists internal/cli's existing iox dependency; the user-guide's `diff --json` prose names both pseudo-pointers (`mode`, `symlink`). CHANGELOG: the Changed bullet for #229's shared walk no longer claims the mode/symlink disagreements are "unchanged and tracked in #229" — they are resolved in the same release (see Fixed). No test changes. The website's architecture, components and capability-matrix pages regenerate from docs/*.md at build time. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
The issue the mode commit deferred to now exists. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
…plit "unresolvable" from "refused", fold mode drift on converged content Round 1 of the review loop on #247 (four lenses, no BLOCKER): - reconcile's symlink refusal arm offered [o]verride. Two lenses showed why it must not: a link to a FIFO takes that arm too, and [o] re-applies through Writer.Write's convergence read, which hangs on a FIFO (#241); and on converged content Writer.Write's mode arm chmods the TARGET through the link before the symlink policy is consulted (#248, filed). The arm now checks the shape first (a link to a FIFO takes the shape arm, as it did on main) and withholds [o] otherwise. - destReadPath folded "the switch is unset" and "the link does not resolve" into one refusal, so a user who had already set the switch was told to set it. It now answers a reason; hashFile maps the second to its own sentinel (symlink-target-unresolvable), diff prints a matching hunk, and the write-back arm says "fix the link". Pinned by a hash row and a write-back row. - classWithModeDrift only upgraded Clean, so converged content with a mode difference left status saying "in sync" while diff --exit-code failed on its mode hunk. It now upgrades Converged too, which makes the "one method, three surfaces cannot disagree" claim actually true. - TestSymlinkDestAllowed gains "true" and "1 " rows; the harness header now says T-09/T-10 encode the chosen policy rather than the old answer; shortVal no longer truncates a sentinel to an opaque prefix in reconcile's prompt. - Prose: SECURITY.md and docs/architecture.md name #248 instead of implying no write path follows a link; the architecture paragraph no longer says the sentinel classifies "drift" (the classifier's table decides); CHANGELOG entry trimmed; pathlessStatErr moved so readDestBytes owns its own doc comment. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
…gate, pin the unresolvable path on every surface Round 2 of the review loop on #247 (four lenses, no BLOCKER): - Three lenses converged on the same flaw from three angles: hashFile refused a symlink before looking at its target, so for a link to a FIFO diff said "set the switch" while reconcile's write-back said "not a regular file", and setting the switch would only have revealed the FIFO; meanwhile the round-1 shape pre-check in reconcile let a symlink LOOP fall through to the generic arm that offers [o]verride. The decision now lives in destReadPath: a link whose target is present and non-regular is a shape problem whatever the switch says, so every surface names the shape; a loop or dangling link fails that Stat and stays a symlink refusal. The reconcile pre-check is deleted. Pinned by hash rows (link→FIFO with the switch unset → shape sentinel; loop → unresolvable) and write-back rows (loop, both env states, no [o]verride). - The unresolvable path was pinned only at hashFile and the write-back arm; its diff surface and the destSymlinkRefused OR were free to regress. A collectDiffHunks test now asserts the unresolvable hunk's text, that it does not advise the switch, and that it embeds no path. The generic "no path separator" guard the round-1 commit dropped is restored. - shortVal's isHexDigest gets its table; a stale "these sentinels are never shown" comment is corrected. - Renames for honesty: symlinkSentinel → symlinkRefusedSentinel and symlinkHunkDest → symlinkRefusedHunkDest (values unchanged); the unresolvable wording says "dangling, loop, or unreadable" rather than asserting a cause EvalSymlinks does not report. - Docs: "needed by every command" over-claimed — import's state-seeding read and adapter Ingest follow links regardless — so README, the user guide, the environment reference, the CHANGELOG and the write-back message now say apply and every drift command; docs/concepts.md's converged sentence now states the mode fold. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
…every command" retraction Round 3 of the review loop on #247 (four lenses, no BLOCKER): - Round 2 routed a link to a FIFO into readDestBytes' shape refusal on every surface — and thereby, on diff, into the plain text arm, which rendered the whole source as an insertion against an "empty" destination that is not empty at all. That was the rendering the symlink hunk was introduced to avoid, and a bare FIFO had done the same on main. diff now prints a `shape` hunk keyed on the shape sentinel (promoted to a named constant), for both; pinned for a bare FIFO and a linked one in both switch states. - Two docs still said the switch is "needed by every command": docs/capability-matrix.md (mirrored to the site) and the troubleshooting page. Both now name apply and the four drift commands, and "drift command" is spelled out wherever it stood alone — the write-back message, the environment reference. - Comments corrected: destReadPath no longer claims every surface names the shape (status and explain classify with a sentinel) or that symlinkNone means "readable" (readDestBytes still gates); readDestBytes' doc no longer says reconcile's write-back is the only surface that names a shape; the not-regular-vs-unstattable rationale is stated once instead of three times. - CHANGELOG: "all four" after listing five commands; the "does not resolve" sentence scoped to "once opted in", as the code is. - The redundant loop row in TestHashFileSentinels is dropped (the dangling row pins the same branch; the loop's distinct value is in the write-back rows). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
…t covers, pin the pseudo-hunk ordering Round 4 of the review loop on #247 (four lenses, no BLOCKER): - hashFile deliberately maps "present and the wrong shape" and "cannot be stat'd" to one token (base parity), and round 3's shape hunk keyed on that token with wording for only the first fact — so a destination under a regular-file parent, or one the user cannot stat, was told to "remove or replace" a FIFO that is not there. Three lenses agreed; two preferred a distinct sentinel and one preferred hedging the constant. The hedge wins: a distinct token classifies identically and buys one string of precision on one surface at the cost of threading it through hashFile, shortVal and the parity tests. The Dest now names both facts and both remedies; the comments in hashFile and on errDestUnstattable that said the sentinels are only ever compared for equality are corrected, since diff now keys prose on one. - The documented claim that the pseudo-hunk checks run BEFORE the text compare was unpinned: moving them after it stayed green while an empty rendered source against a FIFO went from one hunk to "no diff". A fourth shape row (empty source) and the unresolvable-link test (now also empty source) pin both arms. - docs/user-guide.md said "one of two pseudo-pointers" and listed three; CHANGELOG said "all refuse the link" across a list that includes apply, which only fails when content differs; docs/architecture.md credited the shared mode question to classWithModeDrift where opModeDrifted is the predicate all three surfaces ask. The harness's projectD treats "shape" as a label like "mode"/"symlink". One subtest name still said "every command". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
…the SHA display for a shape-refused item Round 5 of the review loop on #247 (four lenses, no BLOCKER; two CLEAN): - The formatted diff ran a character diff between a pseudo-hunk's two constant labels, so the symlink hunk printed as shredded fragments ("[-symlink (not compa-]re[-d throu-]g…"); every earlier round had asserted only the --json shape. Semantic cleanup for the three pseudo pointers re-joins each side whole and leaves mode and shape byte-identical. N-5 now asserts the printed labels. - reconcile's SHA-display fallback covered a refused link but not a shape-refused item, so a FIFO still rendered as the whole source inserted against an "empty" destination in the prompt — the rendering diff's shape hunk was added to avoid. The condition now covers the shape token too; pinned. - The hedged shape wording had no test that reached diff with an unstattable path; a regular-file-parent row pins both the wording and the claim that the case reaches diff as a shape hunk. - Wording: the docs said [o]verride is "withheld" when the prompt still lists it and only the refusal's advice omits it; the user guide's `shape` gloss and the architecture paragraph now name both facts the token carries; two "one of two" counts became three; a test message no longer says the sentinels are only compared for equality. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
…eaning up a character diff Round 6 of the review loop on #247, targeted at round 5's fixes (four lenses, no BLOCKER; two CLEAN): - Round 5's semantic cleanup re-joined the symlink hunk's labels but left the shape hunk spliced ("[-not a -]regular file[- (FIFO, …)-]") — measured by two lenses — while its comment claimed both were whole. A symlink or shape hunk carries two unrelated labels with no honest character diff, so hunkDiffs now emits each side whole for those two and keeps the character diff for mode, whose two like-shaped sides read well under it. The three pseudo-pointer labels are named constants shared by the hunk constructors, the predicate and the harness's projectD; a unit test pins the rule for all three label constants and the mode exception. - planItem.destShapeRefused joins destSymlinkRefused so reconcile's hasText and diff's shapeHunk share one gate with the ptr guard. - Comments and a test name still said [o]verride is "withheld"/"NOT offered" where the docs were corrected to "advice that omits"; the user guide's "readable regular file" over-claimed (an unreadable regular file answers "", not the shape token); two over-long rewrapped lines. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Round 7 of the review loop on #247, targeted at round 6's fix: all four lenses CLEAN. The remaining NITs were prose: the hunkDiffs doc now states the Delete-is-destination orientation every hunk shares; two comments and a test comment stop saying [o]verride is "withheld" (the prompt lists it; only the refusal's advice omits it); the label-hunk test names its rows; three over-long or orphaned lines from earlier rewraps are rewrapped. 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
Third and last PR for #229. PR-B (#244) unified the four plan→drift walks and deliberately preserved the two places the copies disagreed by policy, so each could be reviewed on its own. This PR decides both. Closes #229.
Axis 14, mode drift.
statuscompared a destination's permission bits against the mode recorded at the last apply, so a state entry with no recorded mode (written before modes were recorded) or an adapter that changed the mode it renders reportedcleanwhile the nextapplywould chmod the file. It now asksop.Mode, the moderender.Writer.Writeactually chmods to, which is the questiondiff'smodehunk already asked.explainfolds the same answer into its class, so a content-identical chmod no longer makesstatussaydriftandexplainsaycleanabout the same file. One method,planItem.classWithModeDrift, serves both;recordedModeand its predicate are deleted because nothing reads them.reconcilestill ignores mode; that is #245, filed alongside this PR, because[w]rite-backof a permission bit has nowhere to land in the canonical model.Axis 9, symlinked destinations. Under the documented chezmoi/Stow setup (
AGENTSYNC_ALLOW_SYMLINK_DEST=1, whereapplywrites through the link),status,reconcileandexplainhashed the link itself and reporteddriftno apply could clear, whilediffread through it and saidno diff. The read side now mirrors the write side through one gate,destReadPath, keyed on a newiox.SymlinkDestAllowed()thatresolveSymlinkDestalso uses, so the two cannot disagree. With the switch set, all four surfaces resolve the link and compare the target, content and permission bits alike; a converged chezmoi setup reportsclean. With it unset, all four refuse the link, anddiffprints asymlinkpseudo-hunk (pointer: "symlink", next to the existingmode) naming the switch instead of reading through.reconcileshows such an item with the SHA display rather than a text diff against an empty destination, and its[w]refuses to capture through a link the classification never read through;[o]stays offered because the convergence read cannot hang.Three design points a reviewer should weigh:
~/.claude.json) are decoded through the link on every surface regardless of the switch, exactly asapplytreats them: a converged symlinked key-merge dest is a no-op plus a chmod through the link, a differing one isiox.ErrSymlinkDest, identical to the whole-file case, so the read-through already has apply-parity. Refusing it would make every owned pointer of a chezmoi~/.claude.jsonclassify against<absent>permanently, with no per-pointer sentinel the classifier could carry. The asymmetry is documented indestReadPath's comment anddocs/architecture.md§6.symlinkhunk'sDestis a constant. The link target is attacker-choosable and hunk text reaches the terminal unsanitized (only the label goes throughui.Sanitize), so the target is deliberately not embedded. A test pins that the constant carries no path.Harness discipline. PR-B's characterization harness is the oracle, and its header promised PR-C would edit exactly two goldens. This PR edits T-09 (
whole-file/mode-drift-only, the E projection) and T-10 (whole-file/dest-is-symlink, the D and R projections) and nothing else, plus one cosmetic line innormalizeRunsso thesymlinklabel is treated likemode. The full suite reported exactly those failures before the golden edits and nothing else after them.Filed with this PR: #245 (reconcile ignores mode-only drift), #246 (
apply's per-agent orphan delete can remove a shared file another agent still renders, order-dependent).Type of change
Test plan
go build ./...,AGENTSYNC_TEST_IN_CONTAINER=1 go test -raceoninternal/cliandinternal/iox, full suite, BDD and e2e 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;go mod tidya no-op.t.Setenvwith not.Parallel; the packages pass at-count=2.iss162assertion goes red);destReadPathforced to always follow (N-5, T-10, the env-unset rows red) and always refuse (N-4, the env-set rows red); the reconcilehasTextfallback dropped (T-10 R red); the write-back gate dropped (the new refusal test red); the link target embedded in the hunk constant (N-5 and T-10 D red).symlink_dest_test.go(N-4: converged under the switch reportsclean/no diff; N-5: the same setup with the switch unset reports the sentinel on every surface),TestStatusDiff_ModeDriftDetectionflipped to theop.Modesemantics (N-9), anexplainmode-drift row (N-10), env rows inTestHashFileSentinelsandTestWalkPlanItems, a write-back refusal row,iox.SymlinkDestAllowedunit test.Checklist
internal/secrets,internal/capture, or anysource.Write*path;readDestBytesandreadDestFileare unchanged; no newforbidigoexclusion.docs/architecture.md§6,docs/components.md,docs/capability-matrix.md,README.md,docs/user-guide.md(the switch now governs reads on every command, and thediff --jsonpseudo-pointers), the website environment/troubleshooting/security pages,SECURITY.md,CHANGELOG.md(three### Fixedentries and the corrected Extract a shared plan→drift-classification walk for status/diff/reconcile/explain #229### Changednote).🤖 Generated with Claude Code
https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Generated by Claude Code