From 71a9cd0cc64a4e4ddde14206d86dc58fadbd5509 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 02:39:51 +0000 Subject: [PATCH 01/11] fix(cli): measure mode drift against the mode apply writes, and report it in explain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 ` 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 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 16 +++ internal/cli/explain_model.go | 6 +- internal/cli/explain_path_test.go | 72 +++++++++++++ internal/cli/iss162_internal_test.go | 101 +++++++++++------- internal/cli/planwalk.go | 78 +++++++------- .../cli/planwalk_characterization_test.go | 15 +-- internal/cli/planwalk_internal_test.go | 11 +- internal/cli/status.go | 17 ++- 8 files changed, 217 insertions(+), 99 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0709f9de..852bdbf8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,22 @@ source layout, CLI surface, and state schema are stabilizing but may still chang ### Fixed +- **`status`'s permission check now measures the mode the next `apply` + writes** ([#229](https://github.com/spxrogers/agentsync/issues/229)). It + compared the destination's permission bits against the mode RECORDED at the + last apply, so a file whose recorded mode was unset (state written before + modes were recorded) or whose adapter changed the mode it renders was + reported `clean` while the next apply would chmod it. It now asks `op.Mode`, + the question `diff`'s `mode` hunk already asked, so the two agree. + +- **`explain ` now reports a mode-only drift instead of `clean`** + ([#229](https://github.com/spxrogers/agentsync/issues/229)). A + content-identical chmod made `status` say `drift` and `explain` say `clean` + about the same file in the same second; both now read the one folded class. + `reconcile` still ignores mode entirely — it reports "nothing to reconcile" + for a drift the other three surfaces name — and that gap is tracked as its + own issue rather than fixed here. + - **A FIFO at a managed destination no longer hangs `status`, `diff`, `explain`, or `reconcile`'s drift walk and write-back.** (A directory there never hung — `os.ReadFile` fails it immediately with `EISDIR` — and for `status` and `diff` nothing about diff --git a/internal/cli/explain_model.go b/internal/cli/explain_model.go index 46fc3055..6466d17e 100644 --- a/internal/cli/explain_model.go +++ b/internal/cli/explain_model.go @@ -140,7 +140,9 @@ func buildExplainModel(in explainInputs) explainModel { } // fileItem builds the provenance for a whole-file destination from its walk -// item, whose happlied and cls are the recorded hash and the classification. +// item, whose happlied is the recorded hash. Drift is classWithModeDrift — the +// same folded reading status reports — so a content-identical chmod is `drift` +// here exactly when it is there, rather than `clean` beside status's `drift`. func fileItem(in explainInputs, it planItem, skips []adapter.Skip, origins map[string]explainPluginOrigin, secretRefs map[secrets.RefLocation][]string, ) explainItem { @@ -152,7 +154,7 @@ func fileItem(in explainInputs, it planItem, skips []adapter.Skip, Name: name, Source: sourceOf(in, it.op.SourceID, kind), Transforms: matchingSkips(skips, kind, name), - Drift: it.cls.String(), + Drift: it.classWithModeDrift().String(), } if o, ok := origins[componentKey(kind, name)]; ok { po := o diff --git a/internal/cli/explain_path_test.go b/internal/cli/explain_path_test.go index de107545..5ca921a6 100644 --- a/internal/cli/explain_path_test.go +++ b/internal/cli/explain_path_test.go @@ -576,3 +576,75 @@ func TestExplainPath_SharedDestGroupsPerAgent(t *testing.T) { t.Fatalf("a shared destination must group per owning agent; got %+v\n%s", m.Owners, out) } } + +// TestExplain_ReportsModeDrift pins #229 axis 14 for explain: a +// content-identical chmod of a managed file is `drift` to explain exactly when +// it is `drift` to status, because both read the one folded class +// (planItem.classWithModeDrift). Before #229 PR-C explain printed `clean` +// beside status's `drift` about the same file in the same second. status's +// verdict is asserted in the same test so the fixture cannot silently stop +// producing mode drift and leave the explain half vacuous. +func TestExplain_ReportsModeDrift(t *testing.T) { + tmp, env := explainFixture(t) + dest := filepath.Join(tmp, ".claude", "agents", "reviewer.md") + // A subagent renders at 0644 (its op.Mode); the chmod leaves the content + // byte-identical, so only the permission bits differ from what the next + // apply would chmod to. + if err := os.Chmod(dest, 0o600); err != nil { + t.Fatal(err) + } + + sOut, err := runCLI(t, env, "status", "--json") + if err != nil { + t.Fatalf("status --json: %v\n%s", err, sOut) + } + var st struct { + Agents []struct { + Agent string `json:"agent"` + Items []struct { + Path string `json:"path"` + Class string `json:"class"` + } `json:"items"` + } `json:"agents"` + } + if err := json.Unmarshal([]byte(sOut), &st); err != nil { + t.Fatalf("status --json: invalid JSON: %v\n%s", err, sOut) + } + statusClass := "" + for _, ag := range st.Agents { + if ag.Agent != "claude" { + continue + } + for _, it := range ag.Items { + if it.Path == dest { + statusClass = it.Class + } + } + } + if statusClass != "drift" { + t.Fatalf("fixture: status must call the chmod'd subagent drift, got %q\n%s", statusClass, sOut) + } + + eOut, err := runCLI(t, env, "explain", dest, "--json") + if err != nil { + t.Fatalf("explain --json: %v\n%s", err, eOut) + } + var ex struct { + Owners []struct { + Agent string `json:"agent"` + Items []struct { + Drift string `json:"drift"` + } `json:"items"` + } `json:"owners"` + } + if err := json.Unmarshal([]byte(eOut), &ex); err != nil { + t.Fatalf("explain --json: invalid JSON: %v\n%s", err, eOut) + } + if len(ex.Owners) != 1 || ex.Owners[0].Agent != "claude" || len(ex.Owners[0].Items) != 1 { + t.Fatalf("expected one claude owner with one item; got %+v", ex.Owners) + } + if got := ex.Owners[0].Items[0].Drift; got != statusClass { + t.Errorf("explain drift = %q, status class = %q: a content-identical chmod must get the "+ + "SAME verdict from both — they read one folded class", got, statusClass) + } +} diff --git a/internal/cli/iss162_internal_test.go b/internal/cli/iss162_internal_test.go index 88f426ec..40ec495a 100644 --- a/internal/cli/iss162_internal_test.go +++ b/internal/cli/iss162_internal_test.go @@ -10,15 +10,17 @@ import ( "github.com/spxrogers/agentsync/internal/state" ) -// 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. +// TestStatusDiff_ModeDriftDetection exercises the helpers that give status, +// diff and explain their mode-drift awareness (issue #162 item D): destModePerm's +// filesystem triage feeding planItem.opModeDrifted — the ONE mode question every +// surface asks since #229 PR-C — 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 intended mode is unspecified (0), +// stays a no-op — preserving the mtime-churn-avoidance intent. The four +// predicate rows are the truth table of the RECORDED-mode helper status used +// before #229 PR-C, carried over 1:1 with "recorded" reread as "intended" +// (op.Mode); the status-side oracle that distinguishes the two formulas is +// TestStatus_ModeDriftUsesOpModeNotRecordedMode below. func TestStatusDiff_ModeDriftDetection(t *testing.T) { dir := t.TempDir() p := filepath.Join(dir, "run.sh") @@ -29,17 +31,17 @@ func TestStatusDiff_ModeDriftDetection(t *testing.T) { if err := os.Chmod(p, 0o755); err != nil { // umask-proof t.Fatal(err) } - item := func(recorded uint32, path string) planItem { + item := func(intended uint32, path string) planItem { perm, regular := destModePerm(path) - return planItem{recordedMode: recorded, destPerm: perm, destRegular: regular} + return planItem{op: adapter.FileOp{Path: path, Mode: intended}, destPerm: perm, destRegular: regular} } - // recordedModeDrifted (status side), over destModePerm's triage. + // opModeDrifted, 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 item(0o755, p).opModeDrifted() { + t.Errorf("opModeDrifted: 0755 intended vs 0755 on disk must be false") } if err := os.Chmod(p, 0o644); err != nil { t.Fatal(err) @@ -47,50 +49,69 @@ func TestStatusDiff_ModeDriftDetection(t *testing.T) { if perm, reg := destModePerm(p); perm != 0o644 || !reg { t.Fatalf("destModePerm(0644 file) = (%04o, %v), want (0644, true)", perm, reg) } - if !item(0o755, p).recordedModeDrifted() { - t.Errorf("recordedModeDrifted: 0755 recorded vs 0644 on disk must be true (drift)") + if !item(0o755, p).opModeDrifted() { + t.Errorf("opModeDrifted: 0755 intended vs 0644 on disk must be true (drift)") } - if item(0, p).recordedModeDrifted() { - t.Errorf("recordedModeDrifted: recorded mode 0 (unspecified) must never be drift") + if item(0, p).opModeDrifted() { + t.Errorf("opModeDrifted: intended 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") - } - - // 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) + if item(0o755, absent).opModeDrifted() { + t.Errorf("opModeDrifted: a missing file must not be reported as mode drift") } // 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)) + src, dst, ok := modeHunk(item(0o755, p)) 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(hunkItem(0o644)); ok { + if _, _, ok := modeHunk(item(0o644, p)); ok { t.Errorf("modeHunk: no hunk expected when intended mode == on-disk mode") } - if _, _, ok := modeHunk(hunkItem(0)); ok { + if _, _, ok := modeHunk(item(0, p)); ok { t.Errorf("modeHunk: no hunk expected for an unspecified intended mode (0)") } } + +// TestStatus_ModeDriftUsesOpModeNotRecordedMode pins #229 axis 14 for status: +// the permission check measures the destination against the mode the next +// apply would WRITE (op.Mode), not the mode state RECORDED at the last apply. +// The fixture is the one configuration on which the two formulas disagree — +// recorded 0644 == disk 0644, op.Mode 0755 — so it is a single-fault flip: +// the recorded-mode formula answers `clean`, the op.Mode formula `drift`. +// (The characterization harness's T-09 does NOT discriminate here: its +// recorded mode and op.Mode BOTH differ from disk, so both formulas fire.) +func TestStatus_ModeDriftUsesOpModeNotRecordedMode(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "run.sh") + const content = "#!/bin/sh\n" + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(p, 0o644); err != nil { // umask-proof + t.Fatal(err) + } + 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"}, + }}}} + model := buildStatusModel(plan, []string{"claude"}, s, userHome, adapter.ScopeUser, "") + // Both halves, so a walk that emitted zero items cannot pass: exactly one + // item, and it is the drifted one. + if len(model.Agents) != 1 || len(model.Agents[0].Items) != 1 { + t.Fatalf("want exactly one status item, got %+v", model.Agents) + } + if model.Summary["drift"] != 1 || model.Agents[0].Items[0].Class != "drift" { + t.Errorf("status must compare against op.Mode (0755 != disk 0644), not the RECORDED mode "+ + "(0644 == disk): summary=%v item=%+v", model.Summary, model.Agents[0].Items[0]) + } +} diff --git a/internal/cli/planwalk.go b/internal/cli/planwalk.go index 274f7e6d..1384a14f 100644 --- a/internal/cli/planwalk.go +++ b/internal/cli/planwalk.go @@ -44,7 +44,8 @@ type planItem struct { orphan bool // cls is the CONTENT-only classification. It deliberately does NOT fold in - // permission drift; see recordedModeDrifted / opModeDrifted. + // permission drift; classWithModeDrift is the folded reading status and + // explain report, and opModeDrifted the predicate behind it. cls drift.Class // The triple cls was computed from. hdest is "" for absent-or-unreadable, @@ -52,14 +53,14 @@ type planItem struct { // 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 + // Whole-file mode facts, 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). The intended mode is op.Mode; the mode state + // RECORDED at the last apply is deliberately not carried, because no + // surface asks it (#229 axis 14). + 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 @@ -72,21 +73,13 @@ type planItem struct { 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. +// opModeDrifted is THE mode question, the one every surface that reports +// permission drift asks: do the destination's permission bits differ from the +// mode the next apply would WRITE (op.Mode — render.Writer.Write chmods to +// it)? An unspecified op.Mode (0) is never drift, and an absent / symlinked / +// non-regular destination is left to the content classifier. diff's modeHunk +// and classWithModeDrift below are both gated on exactly this, so they cannot +// disagree (#229 axis 14). func (i planItem) opModeDrifted() bool { if i.op.Mode == 0 || !i.destRegular { return false @@ -94,6 +87,17 @@ func (i planItem) opModeDrifted() bool { return os.FileMode(i.destPerm).Perm() != os.FileMode(i.op.Mode).Perm() } +// classWithModeDrift is the class status and explain report: the content class, +// upgraded from clean to drift when only the permission bits differ from what the +// next apply would WRITE (op.Mode — render.Writer.Write chmods to it). A merged key +// has no mode, and an orphan's synthesized op carries Mode 0, so both fall through. +func (i planItem) classWithModeDrift() drift.Class { + if i.ptr == "" && i.cls == drift.Clean && i.opModeDrifted() { + return drift.Drift + } + return i.cls +} + // 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 @@ -226,14 +230,13 @@ func walkPlanItems(w planWalk) []planItem { 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, + agent: name, + op: op, + hsrc: hashContent(op.Content), + happlied: entry.SHA256, + hdest: hashFile(op.Path), + destPerm: perm, + destRegular: reg, } it.cls = drift.Classify(it.hsrc, it.happlied, it.hdest) if w.withText { @@ -256,12 +259,11 @@ func walkPlanItems(w planWalk) []planItem { // 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, + op: adapter.FileOp{Action: "delete", Path: orphan, SourceID: entry.SourceID}, + happlied: entry.SHA256, + hdest: hashFile(orphan), + destPerm: perm, + destRegular: reg, } it.cls = drift.Classify("", it.happlied, it.hdest) out = append(out, it) diff --git a/internal/cli/planwalk_characterization_test.go b/internal/cli/planwalk_characterization_test.go index 10951dc1..56e1f95a 100644 --- a/internal/cli/planwalk_characterization_test.go +++ b/internal/cli/planwalk_characterization_test.go @@ -311,11 +311,14 @@ func planFixtures() []planFixture { 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. + // T-09: content clean, permission bits drifted. status and explain fold + // mode drift measured against op.Mode into `drift`; diff emits a "mode" + // hunk against op.Mode; reconcile ignores mode entirely. The recorded + // mode (0755) != op.Mode (0700) != on disk (0644), so D's + // `Source: "mode 0700"` still catches a walk that swapped op.Mode for + // the recorded mode. S does NOT discriminate between the two formulas + // here (both fire); TestStatus_ModeDriftUsesOpModeNotRecordedMode is + // that oracle. E is the projection #229 PR-C moved (was `clean`). { name: "whole-file/mode-drift-only", target: func(h string) string { return dest(h, "run.sh") }, @@ -350,7 +353,7 @@ func planFixtures() []planFixture { }} }, wantE: func(string) eProj { - return eProj{rows: []eRow{{"claude", "", "managed", "clean"}}, pathManaged: true} + return eProj{rows: []eRow{{"claude", "", "managed", "drift"}}, pathManaged: true} }, }, diff --git a/internal/cli/planwalk_internal_test.go b/internal/cli/planwalk_internal_test.go index 2d2b5182..435011ba 100644 --- a/internal/cli/planwalk_internal_test.go +++ b/internal/cli/planwalk_internal_test.go @@ -186,13 +186,16 @@ func TestWalkPlanItems(t *testing.T) { 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() { + // The on-disk mode facts ARE populated (0644, regular)… + if items[0].destPerm != 0o644 || !items[0].destRegular { 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. + // class is the classifier's, untouched by the chmod. Since #229 + // PR-C that protection is STRUCTURAL rather than a projection-side + // exclusion: the fold asks op.Mode, and a synthesized orphan op + // carries Mode 0, so opModeDrifted cannot fire for it no matter + // what state recorded (Mode 0o755 here) or what is on disk. 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) diff --git a/internal/cli/status.go b/internal/cli/status.go index 164de60a..6146d8c1 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -287,7 +287,8 @@ func containsStar(names []string) bool { // 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. +// content-clean whole file, measured against the mode the next apply writes +// (planItem.classWithModeDrift, the reading explain shares). 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{} @@ -314,14 +315,12 @@ func buildStatusModel(plan render.RenderPlan, names []string, s *state.Targets, 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() - } + // Content clean but the permission bits differ from what the next + // apply would chmod to (op.Mode) is still drift — the question + // diff's mode hunk and explain ask too, through classWithModeDrift, + // so the three cannot disagree. Whole-file items only: a merged key + // has no mode, and an orphan's synthesized op carries Mode 0. + cls := it.classWithModeDrift().String() ag.Items = append(ag.Items, statusItem{Path: it.op.Path, Pointer: it.ptr, Class: cls}) model.Summary[cls]++ } From 5d38add660b6269134d771a2258a6d577077bf97 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 02:49:38 +0000 Subject: [PATCH 02/11] fix(cli): mirror apply's symlink policy on the destination-read side MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 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 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 21 ++ internal/cli/destread.go | 71 ++++++- internal/cli/destread_unix_internal_test.go | 120 ++++++++++-- internal/cli/diff.go | 38 +++- internal/cli/planwalk.go | 55 ++++-- .../cli/planwalk_characterization_test.go | 35 ++-- internal/cli/planwalk_internal_test.go | 52 ++++- internal/cli/reconcile.go | 28 ++- internal/cli/status.go | 37 ++-- internal/cli/symlink_dest_test.go | 181 ++++++++++++++++++ internal/iox/atomic.go | 9 +- internal/iox/atomic_test.go | 28 +++ 12 files changed, 591 insertions(+), 84 deletions(-) create mode 100644 internal/cli/symlink_dest_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 852bdbf8..2204594c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,27 @@ source layout, CLI surface, and state schema are stabilizing but may still chang ### Fixed +- **A symlinked destination under `AGENTSYNC_ALLOW_SYMLINK_DEST=1` is no + longer reported as permanently drifted** + ([#229](https://github.com/spxrogers/agentsync/issues/229)). In the + documented chezmoi/Stow configuration, where `apply` writes THROUGH the link, + `status`, `reconcile` and `explain` hashed the link itself and answered a + sentinel that can never equal a content hash — so every run reported `drift` + no apply could clear and `status --exit-code` failed CI forever, while `diff` + read through the link and said `no diff`. The switch now governs the READ + side too, on every surface: with it set, all four resolve the link and + compare the file it points at (content, text and permission bits alike), so + a converged chezmoi setup reports `clean`; with it unset, all four refuse the + link as they always did, and `diff` prints a `symlink` hunk (`--json` + `pointer: "symlink"`, alongside the existing `mode` pseudo-pointer) naming + the switch instead of silently agreeing there is no difference. `reconcile` + shows such an item with the SHA display rather than a text diff against an + empty destination, and its `[w]`rite-back refuses to capture through a link + the classification did not read through (naming the switch, which every + command now needs — not only `apply`). Key-merged destinations (a symlinked + `~/.claude.json`) are decoded through the link either way, deliberately, as + `apply` treats them. + - **`status`'s permission check now measures the mode the next `apply` writes** ([#229](https://github.com/spxrogers/agentsync/issues/229)). It compared the destination's permission bits against the mode RECORDED at the diff --git a/internal/cli/destread.go b/internal/cli/destread.go index d8aa3992..24544205 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -5,7 +5,9 @@ import ( "fmt" "io/fs" "os" + "path/filepath" + "github.com/spxrogers/agentsync/internal/iox" "github.com/spxrogers/agentsync/internal/render" ) @@ -57,13 +59,14 @@ var errDestUnstattable = errors.New("cannot stat destination") // wrapping the real errno, NOT as a shape error. "Present and the wrong // shape" and "shape unknown" are different facts, and one of them reaches a // user. -// - Symlinks are followed here but refused outright by hashFile, so `status` -// calls a symlinked destination drifted while `diff` reads through it. The -// reads this gate replaced followed links too, so changing that is a -// behavior decision for #229 — which also owns the consequence that under -// AGENTSYNC_ALLOW_SYMLINK_DEST=1, where apply writes THROUGH the link, -// `status` reports drift no apply can clear. TestHashFileSentinels asserts -// both halves. +// - Symlinks are FOLLOWED here, deliberately: this is also the key-merge +// read (readDestFile) and import's state-seeding read, where the symlink +// policy does not apply — a key-merge destination is decoded through the +// link on every surface, as apply writes through it. The WHOLE-FILE reads +// (hashFile, destModePerm, readDestText) apply the policy first, through +// destReadPath below, and hand this function a symlink's resolved target +// only when AGENTSYNC_ALLOW_SYMLINK_DEST=1 opted into it. +// TestHashFileSentinels asserts both halves. // // Callers mostly do not surface the refusal — reconcile's write-back is the // only one that names the shape today — so it is more a diagnosis available to @@ -104,3 +107,57 @@ func readDestBytes(path string) ([]byte, error) { } return os.ReadFile(path) } + +// destReadPath applies the destination SYMLINK policy and answers the path the +// whole-file destination reads should actually look at. ok is false when the +// destination is a symlink this configuration does not look through; each caller +// answers its own "cannot read" value for that (hashFile the symlink sentinel, +// destModePerm (0, false), readDestText ""). +// +// It mirrors iox.resolveSymlinkDest, the write side, through the shared +// iox.SymlinkDestAllowed, so the read side and apply cannot disagree about +// whether a symlinked destination is supported. The mirror is a POLICY one, not +// a literal prediction: apply only errors when the CONTENT differs (its +// convergence read follows the link), so with the env unset this answers "a +// managed regular file became a link you have not opted into — that is drift" +// rather than "apply would fail here". +// +// The policy is scoped to WHOLE-FILE facts: the symlink sentinel is a +// whole-file-only policy signal. A key-merge destination is decoded through the +// link by readDestFile on every surface, as apply does — a converged symlinked +// key-merge destination is a no-op plus a chmod through the link, a differing +// one is iox.ErrSymlinkDest, exactly as for a whole-file destination, so the +// read-through already has apply-parity. Refusing it instead would make EVERY +// owned pointer of a chezmoi ~/.claude.json classify against +// permanently — there is no per-pointer sentinel the classifier could carry — +// with reconcile's [d] showing and its [w] failing on "not found in +// destination". +func destReadPath(path string) (resolved string, ok bool) { + fi, err := os.Lstat(path) + if err != nil || fi.Mode()&os.ModeSymlink == 0 { + return path, true + } + if !iox.SymlinkDestAllowed() { + return "", false + } + r, err := filepath.EvalSymlinks(path) + if err != nil { + return "", false + } + return r, true +} + +// readDestText is the whole-file destination text read behind the drift walk: +// the destination's bytes, or "" for a refused symlink, a refused shape, or any +// read error — the three collapse because no caller distinguishes them. +func readDestText(path string) string { + p, ok := destReadPath(path) + if !ok { + return "" + } + b, err := readDestBytes(p) + if err != nil { + return "" + } + return string(b) +} diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index b940dc50..04ada8b8 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -12,6 +12,7 @@ import ( "time" "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/iox" ) // TestReadDestBytesShape pins the gate every destination read now passes @@ -257,12 +258,13 @@ func TestHashFileSentinels(t *testing.T) { want: "", }, { - // The symlink arm, which runs BEFORE the shape gate and is the one - // place status and diff deliberately disagree: this refuses the - // link, while readDestBytes (and so diff) follows it. destread.go's - // doc comment asserts exactly that divergence; this is the test - // behind the claim. - name: "a symlink to a regular file is refused as a symlink, not followed", + // The symlink arm, which runs BEFORE the shape gate: with + // AGENTSYNC_ALLOW_SYMLINK_DEST unset (the suite's state) a link is + // refused, not followed — the same policy under which apply + // refuses to write through it. This is what every whole-file + // surface classifies a refused link from; diff keys its "symlink" + // hunk on the same value. + name: "a symlink to a regular file is refused as a symlink when the env is unset", setup: func(t *testing.T, tmp string) string { t.Helper() target := filepath.Join(tmp, "target") @@ -277,6 +279,29 @@ func TestHashFileSentinels(t *testing.T) { }, want: "symlink-not-regular-file", }, + { + // The opt-in half of the same policy: with the env set the link is + // resolved and the TARGET's content hashed — the file apply + // converges through the link — so a chezmoi setup can classify + // clean. t.Setenv is scoped to this subtest's t. + name: "a symlink to a regular file is resolved when the env is set", + setup: func(t *testing.T, tmp string) string { + t.Helper() + t.Setenv(iox.AllowSymlinkDestEnv, "1") + target := filepath.Join(tmp, "target") + if err := os.WriteFile(target, []byte("payload"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(tmp, "link") + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + return link + }, + // A literal, for the same reason as the regular-file row below: + // this is the digest of "payload", what state records for it. + want: "239f59ed55e737c77147cf55ad0c1b030b6d7ee748a7426952f9b852d5a935e5", + }, { name: "a FIFO is refused by shape", setup: mkfifoDest, @@ -338,9 +363,13 @@ func TestHashFileSentinels(t *testing.T) { }) } - // The divergence destread.go documents, asserted in both directions in one - // place so the claim cannot rot: hashFile refuses the link, readDestBytes - // reads through it. + // The asymmetry destread.go documents, asserted in both directions in one + // place so the claim cannot rot: hashFile (a WHOLE-FILE read) refuses the + // link with the env unset, while readDestBytes still reads through it — + // because it is also the key-merge read (readDestFile) and import's + // state-seeding read, where the symlink policy deliberately does not + // apply. The whole-file reads route through destReadPath before reaching + // it; this one does not. t.Run("readDestBytes follows the symlink hashFile refuses", func(t *testing.T) { tmp := t.TempDir() target := filepath.Join(tmp, "target") @@ -353,9 +382,76 @@ func TestHashFileSentinels(t *testing.T) { } data, err := readDestBytes(link) if err != nil || string(data) != "payload" { - t.Fatalf("readDestBytes(symlink) = (%q, %v), want the target's content: this "+ - "asymmetry with hashFile is what makes status report drift on a symlinked "+ - "destination while diff reads through it, and destread.go says so", data, err) + t.Fatalf("readDestBytes(symlink) = (%q, %v), want the target's content: it is "+ + "the key-merge and import read, which decode through a link on every "+ + "surface as apply writes through it; only the whole-file reads apply "+ + "the symlink policy, and destread.go says so", data, err) + } + if got := hashFile(link); got != "symlink-not-regular-file" { + t.Fatalf("hashFile(symlink) = %q with the env unset, want the symlink sentinel: "+ + "the whole-file read must NOT share readDestBytes' read-through", got) + } + }) +} + +// TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough pins the +// reconcile half of #229 axis 9. A whole-file destination the drift walk +// refused as a symlink reaches the prompt as drift; one keystroke later [w] +// must not read THROUGH the link and capture a file the classification never +// looked at. With the env unset the write-back refuses, names the switch and +// says it is needed for every command; with it set the write-back captures +// the linked file — so the gate follows the policy rather than always +// refusing. +func TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough(t *testing.T) { + mkLink := func(t *testing.T) string { + t.Helper() + tmp := t.TempDir() + target := filepath.Join(tmp, "target") + if err := os.WriteFile(target, []byte("payload"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(tmp, "link.md") + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + return link + } + + t.Run("env unset: refuses, naming the switch and that every command needs it", func(t *testing.T) { + t.Setenv(iox.AllowSymlinkDestEnv, "") // registers the restore + if err := os.Unsetenv(iox.AllowSymlinkDestEnv); err != nil { + t.Fatal(err) + } + home := t.TempDir() + link := mkLink(t) + err := writeBackFileItem(home, reconcileItem{op: adapter.FileOp{Path: link, SourceID: "demo"}}) + if err == nil { + t.Fatal("writeBackFileItem = nil for a refused symlink, want an error: [w] must not " + + "capture through a link the classification did not read through") + } + for _, want := range []string{link, iox.AllowSymlinkDestEnv + "=1", "for every command", "[o]verride", "[i]gnore"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error = %q, want it to contain %q", err, want) + } + } + if strings.Contains(err.Error(), "non-regular") { + t.Errorf("error = %q calls a symlink non-regular; the remedy must match the failure", err) + } + if _, serr := os.Stat(filepath.Join(home, "demo")); serr == nil { + t.Errorf("write-back wrote %s despite refusing", filepath.Join(home, "demo")) + } + }) + + t.Run("env set: captures the linked file", func(t *testing.T) { + t.Setenv(iox.AllowSymlinkDestEnv, "1") + home := t.TempDir() + link := mkLink(t) + if err := writeBackFileItem(home, reconcileItem{op: adapter.FileOp{Path: link, SourceID: "demo"}}); err != nil { + t.Fatalf("writeBackFileItem with the env set: %v", err) + } + got, err := os.ReadFile(filepath.Join(home, "demo")) + if err != nil || string(got) != "payload" { + t.Fatalf("captured source = (%q, %v), want the linked file's content", got, err) } }) } diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 349a94c8..c2895858 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -11,6 +11,7 @@ import ( "github.com/spf13/afero" "github.com/spf13/cobra" "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/iox" "github.com/spxrogers/agentsync/internal/paths" "github.com/spxrogers/agentsync/internal/render" "github.com/spxrogers/agentsync/internal/secrets" @@ -216,10 +217,10 @@ func renderDiffText(p *ui.Printer, diffs []diffmatchpatch.Diff) string { // modeHunk describes a permission-bit mismatch between the mode apply would // 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". +// is 0 (unspecified), or the file is absent/refused-symlink/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 @@ -228,6 +229,27 @@ func modeHunk(it planItem) (source, dest string, ok bool) { fmt.Sprintf("mode %04o", os.FileMode(it.destPerm).Perm()), true } +// symlinkHunkDest is the Dest of a "symlink" hunk. A CONSTANT, deliberately: +// the link TARGET is attacker-choosable and this string reaches the terminal +// unsanitized (only the hunk label goes through ui.Sanitize), so embedding it +// would reopen the #93/#171 escape-injection class. +const symlinkHunkDest = "symlink (not compared through; set " + iox.AllowSymlinkDestEnv + + "=1 to read and write through the link)" + +// symlinkHunk describes a destination that is a symlink the read side will not +// look through because AGENTSYNC_ALLOW_SYMLINK_DEST is not set — the same +// condition under which iox.AtomicWrite refuses to write through one. Without +// it diff read THROUGH the link and printed "no diff" for a destination status +// called drift and `--exit-code` failed a CI gate on. A whole-file hunk with an +// empty Dest was rejected too: it asserts the destination is EMPTY, which is +// false, and renders the entire source as one insertion. +func symlinkHunk(it planItem) (source, dest string, ok bool) { + if !it.destSymlinkRefused() { + return "", "", false + } + return "regular file", symlinkHunkDest, true +} + func marshalPretty(v any) string { if v == nil { return "" @@ -281,7 +303,13 @@ func collectDiffHunks(plan render.RenderPlan, names []string, filterPath string, hunks = append(hunks, diffHunk{Path: it.op.Path, Pointer: it.ptr, Source: srcStr, Dest: dstStr}) continue } - // File-level diff. + // File-level diff. The symlink check runs BEFORE the text comparison: + // a refused link's dstText is "", and an empty op.Content must not fall + // through to "equal" and then into the mode branch. + if src, dst, ok := symlinkHunk(it); ok { + hunks = append(hunks, diffHunk{Path: it.op.Path, Pointer: "symlink", Source: src, Dest: dst}) + continue + } if srcStr == dstStr { // Content identical: surface a mode-only drift as a "mode" hunk. if src, dst, ok := modeHunk(it); ok { diff --git a/internal/cli/planwalk.go b/internal/cli/planwalk.go index 1384a14f..dc9bb27d 100644 --- a/internal/cli/planwalk.go +++ b/internal/cli/planwalk.go @@ -49,12 +49,12 @@ type planItem struct { 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 + // and one of two opaque sentinels for a refused-symlink or wrong-shaped // destination — see hashFile, whose semantics this reproduces exactly. hsrc, happlied, hdest string // Whole-file mode facts, from destModePerm: destRegular is false for an - // absent, symlinked or non-regular destination, which is what keeps + // absent, refused-symlink or non-regular destination, which is what keeps // `chmod 000` distinguishable from "absent" (destPerm 0, regular true vs // destPerm 0, regular false). The intended mode is op.Mode; the mode state // RECORDED at the last apply is deliberately not carried, because no @@ -65,21 +65,21 @@ type planItem struct { // 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. + // the raw op content and readDestText's guarded destination read ("" on a + // refused symlink, a refused shape, or any read error), which applies the + // same symlink policy as the hash half above, so the two sides cannot + // disagree about whether a link is looked through (#229 axis 9). Text is + // RAW: callers mask (secrets.MaskResolved) at their own existing call sites. srcText, dstText string } // opModeDrifted is THE mode question, the one every surface that reports // permission drift asks: do the destination's permission bits differ from the // mode the next apply would WRITE (op.Mode — render.Writer.Write chmods to -// it)? An unspecified op.Mode (0) is never drift, and an absent / symlinked / -// non-regular destination is left to the content classifier. diff's modeHunk -// and classWithModeDrift below are both gated on exactly this, so they cannot -// disagree (#229 axis 14). +// it)? An unspecified op.Mode (0) is never drift, and an absent / +// refused-symlink / non-regular destination is left to the content +// classifier. diff's modeHunk and classWithModeDrift below are both gated on +// exactly this, so they cannot disagree (#229 axis 14). func (i planItem) opModeDrifted() bool { if i.op.Mode == 0 || !i.destRegular { return false @@ -98,11 +98,25 @@ func (i planItem) classWithModeDrift() drift.Class { return i.cls } +// destSymlinkRefused reports whether a whole-file destination is a symlink the +// read side did not look through (destReadPath, AGENTSYNC_ALLOW_SYMLINK_DEST +// unset). It is a DERIVATION from hdest, not a field: diff keys its symlink +// hunk on the very hash status, reconcile and explain classified from, so the +// four surfaces cannot disagree about it (#229 axis 9). +func (i planItem) destSymlinkRefused() bool { return i.ptr == "" && i.hdest == symlinkSentinel } + // 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). +// regular is false — and perm 0 — for an absent, refused-symlink +// (destReadPath: a link this configuration does not look through) or +// non-regular destination, so a caller can tell `chmod 000` (0, true) from +// "not there" (0, false). With AGENTSYNC_ALLOW_SYMLINK_DEST=1 a link is +// resolved and the perm is the TARGET's — the bits apply's mode fix chmods +// through the link. func destModePerm(path string) (perm uint32, regular bool) { + path, ok := destReadPath(path) + if !ok { + return 0, false + } fi, err := os.Lstat(path) if err != nil || !fi.Mode().IsRegular() { return 0, false @@ -174,9 +188,11 @@ type planWalk struct { // 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). +// Every destination read goes through hashFile / readDestFile / readDestText, +// each of which reads through the readDestBytes gate, so a FIFO, device, +// socket or directory at a destination can never block a read-only command +// (internal/cli/destread.go, #240). The whole-file reads also share +// destReadPath, the symlink policy (#229 axis 9). func walkPlanItems(w planWalk) []planItem { readDest := w.readDestConfig if readDest == nil { @@ -240,10 +256,7 @@ func walkPlanItems(w planWalk) []planItem { } 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) - } + it.srcText, it.dstText = string(op.Content), readDestText(op.Path) } out = append(out, it) } diff --git a/internal/cli/planwalk_characterization_test.go b/internal/cli/planwalk_characterization_test.go index 56e1f95a..57193c41 100644 --- a/internal/cli/planwalk_characterization_test.go +++ b/internal/cli/planwalk_characterization_test.go @@ -357,11 +357,14 @@ func planFixtures() []planFixture { }, }, - // 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. + // T-10: the destination is a symlink to an identical file, with + // AGENTSYNC_ALLOW_SYMLINK_DEST unset (the suite's state). Every surface + // refuses the link (#229 axis 9): the hash side answers the symlink + // sentinel → drift; diff emits a "symlink" hunk naming the switch + // instead of reading through and printing nothing; reconcile carries + // no destination text (hasText false → the SHA display). 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") }, @@ -383,12 +386,17 @@ func planFixtures() []planFixture { summary: map[string]int{"drift": 1}, } }, - wantD: func(string) dProj { return dProj{filterMatched: true} }, + wantD: func(h string) dProj { + return dProj{hunks: []diffHunk{{ + Path: dest(h, "link.md"), Pointer: "symlink", Source: "regular file", + Dest: "symlink (not compared through; set AGENTSYNC_ALLOW_SYMLINK_DEST=1 to read and write through the link)", + }}, 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", + hasText: false, srcText: "SOURCE", dstText: "", }} }, wantE: func(string) eProj { @@ -396,12 +404,12 @@ func planFixtures() []planFixture { }, 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. + // The refusal, 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) + if r[0].dstText != "" || r[0].hasText || len(d.hunks) != 1 || d.hunks[0].Pointer != "symlink" { + t.Errorf("text side must refuse the link too — no dest text, and one symlink hunk: r=%+v d=%+v", r[0], d) } }, }, @@ -1011,10 +1019,11 @@ func projectS(m statusModel) sProj { } 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. + // A "mode" or "symlink" hunk is a whole-file row wearing a label, not a + // pointer; keep both out of the run sort so their placement stays asserted + // in order. hunks = normalizeRuns(hunks, func(h diffHunk) (string, string, string) { - if h.Pointer == "mode" { + if h.Pointer == "mode" || h.Pointer == "symlink" { return "", h.Path, "" } return "", h.Path, h.Pointer diff --git a/internal/cli/planwalk_internal_test.go b/internal/cli/planwalk_internal_test.go index 435011ba..925af961 100644 --- a/internal/cli/planwalk_internal_test.go +++ b/internal/cli/planwalk_internal_test.go @@ -12,6 +12,7 @@ import ( "github.com/spf13/afero" "github.com/spxrogers/agentsync/internal/adapter" "github.com/spxrogers/agentsync/internal/drift" + "github.com/spxrogers/agentsync/internal/iox" "github.com/spxrogers/agentsync/internal/render" "github.com/spxrogers/agentsync/internal/state" ) @@ -323,7 +324,7 @@ func TestWalkPlanItems(t *testing.T) { }, }, { - name: "symlink-hash-text-split", + name: "symlink-refused-when-env-unset", run: func(t *testing.T, h string) { real, link := dest(h, "real.md"), dest(h, "link.md") mustWrite(t, real, "SOURCE") @@ -338,16 +339,51 @@ func TestWalkPlanItems(t *testing.T) { 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) + // #229 axis 9: with AGENTSYNC_ALLOW_SYMLINK_DEST unset the hash + // side answers hashFile's symlink sentinel (→ drift) AND the + // text side refuses too (→ ""): the two agree, where they once + // split (the text side used to read through the link). + if it.hdest != "symlink-not-regular-file" || it.cls != drift.Drift || !it.destSymlinkRefused() { + t.Errorf("hash side: hdest=%q cls=%v refused=%v", it.hdest, it.cls, it.destSymlinkRefused()) + } + if it.srcText != "SOURCE" || it.dstText != "" { + t.Errorf("text side: src=%q dst=%q, want the text read to refuse the link too", it.srcText, it.dstText) + } + if it.destRegular { + t.Errorf("a refused symlink is not a regular file for the mode predicates") + } + }, + }, + { + name: "symlink-resolved-when-env-set", + run: func(t *testing.T, h string) { + t.Setenv(iox.AllowSymlinkDestEnv, "1") + real, link := dest(h, "real.md"), dest(h, "link.md") + mustWrite(t, real, "SOURCE") + if err := os.Chmod(real, 0o644); err != nil { + t.Fatal(err) + } + 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] + // The opt-in half: every whole-file fact resolves the link to + // the file apply writes through, so the item classifies clean. + if it.hdest != hf("SOURCE") || it.cls != drift.Clean || it.destSymlinkRefused() { + t.Errorf("hash side: hdest=%q cls=%v refused=%v", it.hdest, it.cls, it.destSymlinkRefused()) } 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") + if !it.destRegular || it.destPerm != 0o644 { + t.Errorf("mode facts must be the TARGET's: regular=%v perm=%04o", it.destRegular, it.destPerm) } }, }, @@ -486,7 +522,7 @@ func TestDestModePerm(t *testing.T) { {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: "symlink-refused-when-env-unset", path: link, wantPerm: 0, wantRegular: false}, {name: "directory", path: dir, wantPerm: 0, wantRegular: false}, } { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index ee8c30c3..e941f90a 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -42,8 +42,10 @@ type reconcileItem struct { orphan bool // owned-in-state whole-file dest no agent renders anymore // srcText/dstText carry the actual (masked-on-display) source and destination // content so the prompt/[d]iff can show a real value diff instead of only SHA - // prefixes. hasText is false for items with no meaningful textual content - // (orphans), which fall back to the hash display. + // prefixes. hasText is false for items with no meaningful textual content — + // orphans, and a whole-file destination that is a symlink agentsync is not + // reading through (planItem.destSymlinkRefused) — which fall back to the + // hash display. srcText string dstText string hasText bool @@ -666,7 +668,11 @@ func collectReconcileItems(plan render.RenderPlan, reg *adapter.Registry, s *sta orphans = append(orphans, ri) continue } - ri.srcText, ri.dstText, ri.hasText = it.srcText, it.dstText, true + // A refused whole-file symlink has no destination text to show: fall + // back to the SHA display rather than render the entire source as an + // insertion against an empty destination — the rendering diff's + // symlink hunk exists to avoid (#229 axis 9). + ri.srcText, ri.dstText, ri.hasText = it.srcText, it.dstText, !it.destSymlinkRefused() if it.ptr != "" { ri.pluginOwner = pluginOwnerForKeyItem(it.op.SourceID, it.ptr, pluginOwners) } else { @@ -1090,7 +1096,21 @@ func writeBackKeyItem(cmd *cobra.Command, home string, it reconcileItem) error { // // Both used to return nil with a success message, hiding data loss. func writeBackFileItem(home string, it reconcileItem) error { - data, err := readDestBytes(it.op.Path) + readPath, ok := destReadPath(it.op.Path) + if !ok { + // The destination is a symlink this configuration does not look + // through — the same refusal the drift walk classified the item under, + // so [w] cannot quietly capture a file the classification never read. + // [o]verride stays offered, unlike the non-regular arm below: + // Writer.Write's convergence read follows the link and either no-ops + // (content converged) or fails cleanly with iox.ErrSymlinkDest; no + // FIFO is involved, so it cannot hang. + return fmt.Errorf("read dest %s: destination is a symlink agentsync is not reading through — "+ + "set %s=1 (for every command) to capture the linked file, replace the link with a regular "+ + "file, use [o]verride to re-apply canonical, or [i]gnore to suppress this item", + it.op.Path, iox.AllowSymlinkDestEnv) + } + data, err := readDestBytes(readPath) if err != nil { // Named next steps, like this function's other refusals: the user is // mid-prompt with a keystroke to choose, and "read dest X: not a regular diff --git a/internal/cli/status.go b/internal/cli/status.go index 6146d8c1..5388e537 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -6,7 +6,6 @@ import ( "encoding/json" "errors" "fmt" - "os" "path/filepath" "sort" "strings" @@ -939,6 +938,12 @@ func stateKeyKey(userHome, agent string, sc adapter.Scope, projectRoot, path, pt return state.NewPointerKey(userHome, agent, sc.String(), projectRoot, path, ptr) } +// symlinkSentinel is hashFile's answer for a symlink this configuration does +// not read through (destReadPath). Opaque: it exists only to never equal a +// content hash, and diff keys its symlink hunk on the same value +// (planItem.destSymlinkRefused), so the two sites must agree on it. +const symlinkSentinel = "symlink-not-regular-file" + func hashContent(b []byte) string { sum := sha256.Sum256(b) return hex.EncodeToString(sum[:]) @@ -950,20 +955,26 @@ func hashContent(b []byte) string { // the expected signal for Orphan / OrphanDrifted. A destination whose SHAPE is // wrong, or which cannot be stat'd, answers the opaque marker below instead. // -// If the path is a symlink, hashFile returns a special marker so the -// drift classifier can flag the file as drifted in a way the user can -// act on. A managed file becoming a symlink (e.g. user replaced -// .claude.json with `ln -s /dev/null`) used to silently read through -// the link and compare hashes — making the swap invisible to status. +// A SYMLINK at the path answers symlinkSentinel unless +// AGENTSYNC_ALLOW_SYMLINK_DEST=1 (destReadPath — the gate apply writes under). +// The sentinel is a whole-file-only policy signal: a managed regular file +// became a link you have not opted into. Reading through such a link and +// comparing hashes, as this once did, made the swap invisible to status. With +// the env set the link is resolved and its TARGET hashed — the file apply +// converges — so a chezmoi setup reports clean after a successful apply +// instead of a drift no apply can clear. Opting in never lets a non-regular +// target through: a link to one (`ln -s /dev/null`) resolves and then answers +// the SHAPE sentinel below; a dangling link answers symlinkSentinel with the +// env set or unset, mirroring apply's "resolve symlink" failure. func hashFile(path string) string { - info, lerr := os.Lstat(path) - if lerr == nil && info.Mode()&os.ModeSymlink != 0 { - // Return a sentinel that will never match a content hash. - // We don't include the link target to keep the sentinel stable - // (target may resolve to whatever attacker chose); just signal - // "this is a symlink now." - return "symlink-not-regular-file" + p, ok := destReadPath(path) + if !ok { + // The link target is deliberately NOT part of the sentinel: it is + // attacker-choosable, and a sentinel must stay a stable opaque token + // that never equals a content hash. + return symlinkSentinel } + path = p // A FIFO, device, or socket at a destination path would make os.ReadFile // BLOCK forever rather than fail — wedging `status`, which is advertised as // read-only, and reconcile's orphan listing. None has a content hash worth diff --git a/internal/cli/symlink_dest_test.go b/internal/cli/symlink_dest_test.go new file mode 100644 index 00000000..358de439 --- /dev/null +++ b/internal/cli/symlink_dest_test.go @@ -0,0 +1,181 @@ +package cli_test + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spxrogers/agentsync/internal/iox" +) + +// #229 axis 9: the read side of the symlink policy mirrors the write side. +// AGENTSYNC_ALLOW_SYMLINK_DEST=1 is the one switch under which `apply` writes +// THROUGH a symlinked destination; these two tests pin that the SAME switch +// decides whether `status` and `diff` read through it. +// +// Both run the CLI in-process, so t.Setenv reaches iox.AtomicWrite and the +// walk's destReadPath alike. + +// symlinkedMemoryFixture stands up a user-scope home with claude enabled and a +// canonical memory, then pre-creates the chezmoi-style link: the rendered +// CLAUDE.md destination is a symlink into a "dotfiles" directory BEFORE the +// first apply. It returns the link path and its target. +func symlinkedMemoryFixture(t *testing.T, env map[string]string, tmp string) (link, target string) { + t.Helper() + mustRun(t, env, "init") + mustRun(t, env, "agent", "add", "claude") + if err := os.WriteFile(filepath.Join(tmp, ".agentsync", "memory", "AGENTS.md"), + []byte("# Memory\n\nRendered through a link.\n"), 0o644); err != nil { + t.Fatal(err) + } + target = filepath.Join(tmp, "dotfiles", "CLAUDE.md") + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(target, []byte("stale dotfiles copy\n"), 0o644); err != nil { + t.Fatal(err) + } + link = filepath.Join(tmp, ".claude", "CLAUDE.md") + if err := os.MkdirAll(filepath.Dir(link), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + return link, target +} + +// applyThroughLink applies with the switch set and asserts the chezmoi +// contract held: the link is still a link and the TARGET holds the rendered +// memory. That is what makes the drift assertions below non-vacuous — they +// cannot pass by the link having been replaced with a regular file. +func applyThroughLink(t *testing.T, env map[string]string, link, target string) { + t.Helper() + t.Setenv(iox.AllowSymlinkDestEnv, "1") + mustRun(t, env, "apply") + lst, err := os.Lstat(link) + if err != nil { + t.Fatal(err) + } + if lst.Mode()&os.ModeSymlink == 0 { + t.Fatalf("apply replaced the symlink at %s with a regular file (mode=%v)", link, lst.Mode()) + } + got, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(got), "Rendered through a link.") { + t.Fatalf("apply did not write the rendered memory through the link; target holds:\n%s", got) + } +} + +// statusClassOf runs `status --json` and returns the class of the item at path +// (fatal if absent) plus the whole summary tally. +func statusClassOf(t *testing.T, env map[string]string, path string) (class string, summary map[string]int) { + t.Helper() + out, err := runCLI(t, env, "status", "--json") + if err != nil { + t.Fatalf("status --json: %v\n%s", err, out) + } + var got struct { + Agents []struct { + Agent string `json:"agent"` + Items []struct { + Path string `json:"path"` + Class string `json:"class"` + } `json:"items"` + } `json:"agents"` + Summary map[string]int `json:"summary"` + } + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("status --json: invalid JSON: %v\n%s", err, out) + } + for _, ag := range got.Agents { + for _, it := range ag.Items { + if it.Path == path { + return it.Class, got.Summary + } + } + } + t.Fatalf("status --json lists no item for %s:\n%s", path, out) + return "", nil +} + +// TestSymlinkedDestConvergesWhenAllowed (N-4) is the chezmoi contract end to +// end: with the switch set, `apply` writes through a pre-created link, and then +// `status` says clean and `diff` says no diff — the permanent phantom drift the +// old link-refusing hash produced under this exact, documented configuration is +// gone. +func TestSymlinkedDestConvergesWhenAllowed(t *testing.T) { + tmp := t.TempDir() + env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp} + link, target := symlinkedMemoryFixture(t, env, tmp) + applyThroughLink(t, env, link, target) + + class, summary := statusClassOf(t, env, link) + if class != "clean" || summary["drift"] != 0 { + t.Errorf("status must be clean through an allowed symlink: class=%q summary=%v", class, summary) + } + out, err := runCLI(t, env, "diff") + if err != nil { + t.Fatalf("diff: %v\n%s", err, out) + } + if out != "no diff" { + t.Errorf("diff must compare through an allowed symlink and find nothing; got:\n%s", out) + } +} + +// TestSymlinkedDestIsDriftWhenRefused (N-5) is the other half: the same +// converged link, with the switch UNSET, is drift on every surface — and `diff` +// says so with a "symlink" hunk naming the switch, instead of reading through +// the link and printing "no diff" beside a `status --exit-code` that fails. +// The content is genuinely converged (applied through the link first), so the +// assertions cannot be satisfied by ordinary content drift. +func TestSymlinkedDestIsDriftWhenRefused(t *testing.T) { + tmp := t.TempDir() + env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp} + link, target := symlinkedMemoryFixture(t, env, tmp) + applyThroughLink(t, env, link, target) + // applyThroughLink's t.Setenv registered the restore; unset for the reads. + if err := os.Unsetenv(iox.AllowSymlinkDestEnv); err != nil { + t.Fatal(err) + } + + if class, summary := statusClassOf(t, env, link); class != "drift" || summary["drift"] != 1 { + t.Errorf("status must call a refused symlink drift: class=%q summary=%v", class, summary) + } + + out, err := runCLI(t, env, "diff", "--json") + if err != nil { + t.Fatalf("diff --json: %v\n%s", err, out) + } + var got struct { + Hunks []struct { + Path string `json:"path"` + Pointer string `json:"pointer"` + Source string `json:"source"` + Dest string `json:"dest"` + } `json:"hunks"` + } + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("diff --json: invalid JSON: %v\n%s", err, out) + } + if len(got.Hunks) != 1 || got.Hunks[0].Path != link || got.Hunks[0].Pointer != "symlink" { + t.Fatalf("diff must say the destination is a symlink it is not comparing through — one "+ + "symlink hunk for %s; got %+v", link, got.Hunks) + } + h := got.Hunks[0] + if h.Source != "regular file" || !strings.Contains(h.Dest, iox.AllowSymlinkDestEnv+"=1") { + t.Errorf("symlink hunk must name the switch that reads through the link: %+v", h) + } + // Terminal safety: the link TARGET is attacker-choosable and the hunk's + // Dest reaches the terminal unsanitized, so it must never embed it. + if strings.Contains(h.Dest, target) || strings.Contains(h.Dest, "dotfiles") { + t.Errorf("symlink hunk embeds the link target; it must be a constant: %+v", h) + } + if strings.Contains(h.Dest, "/") { + t.Errorf("symlink hunk Dest contains a path separator; it must embed no path: %q", h.Dest) + } +} diff --git a/internal/iox/atomic.go b/internal/iox/atomic.go index 552fe2f7..4f157849 100644 --- a/internal/iox/atomic.go +++ b/internal/iox/atomic.go @@ -19,6 +19,13 @@ const AllowSymlinkDestEnv = "AGENTSYNC_ALLOW_SYMLINK_DEST" // caller has not set AGENTSYNC_ALLOW_SYMLINK_DEST=1. var ErrSymlinkDest = errors.New("destination is a symlink") +// SymlinkDestAllowed reports whether the user has opted in to symlinked +// destinations. It is the single reading of AllowSymlinkDestEnv: the write side +// (resolveSymlinkDest) and the diagnostic read side (internal/cli/destread.go) +// both ask it, so they cannot disagree about whether a symlinked destination is +// a supported configuration. +func SymlinkDestAllowed() bool { return os.Getenv(AllowSymlinkDestEnv) == "1" } + // AtomicWrite writes data to dest using a three-phase approach: write to a // sibling .agentsync.tmp file (always created mode 0o600 so cleartext // payloads — secrets, age TOML — never sit world-readable in the destination @@ -127,7 +134,7 @@ func resolveSymlinkDest(dest string) (string, error) { if info.Mode()&os.ModeSymlink == 0 { return dest, nil } - if os.Getenv(AllowSymlinkDestEnv) != "1" { + if !SymlinkDestAllowed() { target, _ := os.Readlink(dest) return "", fmt.Errorf("%w: %s -> %s (refusing to replace the symlink; set %s=1 to write through it)", ErrSymlinkDest, dest, target, AllowSymlinkDestEnv) diff --git a/internal/iox/atomic_test.go b/internal/iox/atomic_test.go index 2028f1bb..c4b2e18f 100644 --- a/internal/iox/atomic_test.go +++ b/internal/iox/atomic_test.go @@ -109,6 +109,34 @@ func TestAtomicWrite_RefusesSymlinkDest(t *testing.T) { } } +// TestSymlinkDestAllowed pins the one reading of AllowSymlinkDestEnv that the +// write side (resolveSymlinkDest) and the read side (internal/cli's +// destReadPath) share: only the literal "1" opts in. +func TestSymlinkDestAllowed(t *testing.T) { + for _, tc := range []struct { + name string + value string + unset bool + want bool + }{ + {name: "unset", unset: true, want: false}, + {name: "set to 1", value: "1", want: true}, + {name: "set to 0", value: "0", want: false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(iox.AllowSymlinkDestEnv, tc.value) // registers the restore + if tc.unset { + if err := os.Unsetenv(iox.AllowSymlinkDestEnv); err != nil { + t.Fatal(err) + } + } + if got := iox.SymlinkDestAllowed(); got != tc.want { + t.Errorf("SymlinkDestAllowed() = %v, want %v", got, tc.want) + } + }) + } +} + // TestAtomicWrite_AllowsSymlinkDestWithEnv proves the documented escape // hatch works for users who explicitly accept the chezmoi-link semantics. func TestAtomicWrite_AllowsSymlinkDestWithEnv(t *testing.T) { From 562cd8e4735b5a009317bf545a2d407b4124bd54 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 02:52:04 +0000 Subject: [PATCH 03/11] docs: record #229's mode and symlink policies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 4 +- README.md | 4 +- SECURITY.md | 7 +++- docs/architecture.md | 37 +++++++++++++++++-- docs/capability-matrix.md | 8 +++- docs/components.md | 10 +++-- docs/user-guide.md | 8 +++- .../src/content/docs/help/troubleshooting.mdx | 13 +++++++ .../src/content/docs/internals/security.mdx | 5 ++- .../content/docs/reference/environment.mdx | 6 ++- 10 files changed, 82 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2204594c..456b085f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -239,8 +239,8 @@ source layout, CLI surface, and state schema are stabilizing but may still chang 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. + same snapshot. The ways the four surfaces disagreed (mode-only drift, + symlinked destinations) are resolved in the same release; see Fixed above. - **`.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 diff --git a/README.md b/README.md index 7c0d1007..24c7038e 100644 --- a/README.md +++ b/README.md @@ -206,7 +206,7 @@ If you lose your age private key, you lose access to all encrypted secrets. Reco - **Hand-edits to agentsync-owned keys** in shared agent files (e.g. an MCP server entry in `~/.claude.json` that agentsync owns): the next `apply` overwrites them with NO foreign-collision backup, because agentsync considers them its own. Use `agentsync reconcile` (the drift classifier catches the edit and offers `[w]`rite-back) BEFORE the next apply if you want to keep them. - **Destination git backup is local-only**: `apply` keeps each managed destination dir (`~/.claude`, `~/.codex`, …) in its own git history so `agentsync revert` can undo a bad apply — but that history is **never pushed** (the rendered files hold `${secret:…}` references resolved to **cleartext**, so its commits may too; the `.git` dir is hardened to `0700`, which is **POSIX-only** — a Windows no-op, where NTFS ACLs are the boundary). A destination dir already under **your own** source control is detected as `foreign source control` and left un-versioned (only `agentsync-versioned` dirs are reverted; an `untracked` dir is a candidate for init). `$HOME`-level strays (Claude's `~/.claude.json`) are never versioned — agentsync never inits a repo at `$HOME`. `revert`'s "nothing is lost" guarantee covers **tracked files only**: untracked / gitignored scratch files in the dir are left untouched, never snapshotted. - **Plain-http / git:// plugin sources** are rejected by default to prevent MITM swap. Set `AGENTSYNC_ALLOW_INSECURE_URLS=1` for internal mirrors. -- **Symlinked destinations** (e.g. `~/.claude.json` is a chezmoi symlink into your dotfiles repo) are rejected by default — a rename onto the path would replace the symlink with a regular file and strand your linked source. Set `AGENTSYNC_ALLOW_SYMLINK_DEST=1` to write through the symlink instead (the underlying file is updated in place; the link survives). +- **Symlinked destinations** (e.g. `~/.claude.json` is a chezmoi symlink into your dotfiles repo) are rejected by default — a rename onto the path would replace the symlink with a regular file and strand your linked source. Set `AGENTSYNC_ALLOW_SYMLINK_DEST=1` to write through the symlink instead (the underlying file is updated in place; the link survives). The same switch governs how agentsync READS a symlinked whole-file destination (a `CLAUDE.md`, a skill, a subagent), so it is needed for every command, not only `apply`: without it `status`, `diff`, `reconcile` and `explain` report the file as drifted rather than comparing through the link (and `diff` says so, with a `symlink` hunk naming the switch); with it they resolve the link and compare the file it points at, so a chezmoi setup reports clean after a successful apply. A key-merged file such as `~/.claude.json` is read through the link either way. - **Aider** and **Firebender**: deliberately deferred — no faithful generic projection (Aider has no MCP and only an `.aider.conf.yml` `read:` pointer for memory; Firebender's config is unverified). ## Environment overrides @@ -215,7 +215,7 @@ If you lose your age private key, you lose access to all encrypted secrets. Reco | --- | --- | | `AGENTSYNC_HOME` | Override `~/.agentsync/` location (absolute path). | | `AGENTSYNC_TARGET_ROOT` | Redirect `$HOME` for testing (used by the hermetic test container). | -| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Permit writes to symlinked destination files (resolves the link first). | +| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (chezmoi-managed files). Needed by every command, not only `apply`. | | `AGENTSYNC_ALLOW_INSECURE_URLS=1` | Accept http:// and git:// plugin / marketplace sources. | | `AGENTSYNC_ALLOW_UNIMPLEMENTED=1` | Register an agent that has no implemented adapter yet (none today — every valid agent is real). | | `AGENTSYNC_ALLOW_PLUGIN_DRIFT=1` | Bypass the plugin-cache manifest-SHA check (after hand-editing). | diff --git a/SECURITY.md b/SECURITY.md index 92681d0d..6c56c3b4 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -94,8 +94,11 @@ can resolve secrets into native config files. Areas of particular interest: diagnostics surface — native marketplace ids and source types) stay plain strings. - **Destination writes**: writes are atomic and refuse to clobber symlinked - destinations by default; pre-existing foreign files are backed up before - overwrite. + destinations by default — and, since #229, every read-side surface + (`status`, `diff`, `reconcile`, `explain`) refuses to read through a + symlinked whole-file destination under the same switch, + `AGENTSYNC_ALLOW_SYMLINK_DEST`, reporting it as drift instead; pre-existing + foreign files are backed up before overwrite. ## Sensitive files diff --git a/docs/architecture.md b/docs/architecture.md index cb6fdf4f..9e694d6d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -888,9 +888,37 @@ 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. +re-partitions whole-file rows ahead of key rows, and both `status` and `explain` +fold permission drift into the class of a content-clean whole file — measured +against `op.Mode`, the mode the next apply chmods to, which is the same question +`diff`'s `mode` hunk asks (`planItem.classWithModeDrift`, one method, so the +three cannot disagree). `reconcile` still ignores mode entirely; that gap is +tracked as its own issue. `diff` masks and compares text, `reconcile` excludes +an orphan another agent still renders, `explain` groups by owner. + +A **symlinked** destination is read through only when +`AGENTSYNC_ALLOW_SYMLINK_DEST=1` — the same switch under which `iox.AtomicWrite` +writes through the link. `iox.SymlinkDestAllowed` is the one reading of the +switch, and `destReadPath` (`internal/cli/destread.go`) is the read-side gate +that the three whole-file destination facts — the content hash, the permission +bits and the text — pass through. Unset, all four surfaces answer an opaque +sentinel: the file classifies `drift`; `diff` prints a `symlink` hunk naming the +switch rather than reading through and reporting no difference; `reconcile` +shows the SHA display instead of a text diff and its `[w]`rite-back refuses to +capture through the link. Set, all four resolve the link and compare the file it +points at, so a converged chezmoi setup reports `clean`. The mirror is a policy, +not a prediction: `apply` itself only fails on a symlinked destination when the +content differs (its convergence read follows the link), so the unset answer +means "a managed regular file became a link you have not opted into — that is +drift", not "the next apply would fail". The rule covers **whole-file** +destinations only; the symlink sentinel is a whole-file-only policy signal. A +key-merged destination is decoded through the link on every surface, as `apply` +treats it (a converged symlinked key-merge destination is a no-op plus a chmod +through the link, a differing one is `ErrSymlinkDest` — exactly as for a +whole-file destination, so the read-through already has apply-parity): refusing +it would make every owned pointer of a chezmoi `~/.claude.json` classify against +an absent value permanently, because the classifier has no per-pointer sentinel +to carry, with `reconcile`'s `[d]` showing `` and its `[w]` failing. --- @@ -907,7 +935,8 @@ All present in v1.0 (`internal/iox`, `internal/render`, `internal/state`): bans `os.UserHomeDir()` in `_test.go`. 4. **First-apply backups** — the `foreign-collision` case copies the pre-existing destination into `.state/backups//` before writing. Symlinked - destinations are refused by default. + destinations are refused by default — and, on the read path, classified as + drift rather than read through (§6). 5. **Manifest-SHA pinning** — every plugin records a `tree:v1:` content hash over its *entire* cache tree (every projected component body — skills, command/subagent markdown — not just `plugin.json`, excluding `.git/`), so a diff --git a/docs/capability-matrix.md b/docs/capability-matrix.md index 286bb4aa..8adce8d3 100644 --- a/docs/capability-matrix.md +++ b/docs/capability-matrix.md @@ -585,8 +585,12 @@ in the [README](../README.md#known-limits); the highlights: - **Insecure sources** — `http://` and `git://` plugin/marketplace sources are rejected by default (MITM protection); override with `AGENTSYNC_ALLOW_INSECURE_URLS=1`. -- **Symlinked destinations** are rejected by default; override with - `AGENTSYNC_ALLOW_SYMLINK_DEST=1`. +- **Symlinked destinations** are rejected by default, for both writing and + drift comparison (`status`/`diff`/`reconcile`/`explain` report a symlinked + whole-file destination as drifted rather than reading through the link; a + key-merged file such as `~/.claude.json` is read through either way); + override with `AGENTSYNC_ALLOW_SYMLINK_DEST=1`, which every command then + needs. - **Planned / deferred**: Aider and Firebender (see [Breadth tier](#breadth-tier) § "Deliberate exclusions"). diff --git a/docs/components.md b/docs/components.md index e0a3b51a..ef78b07a 100644 --- a/docs/components.md +++ b/docs/components.md @@ -49,7 +49,9 @@ is the only package that depends on nearly all the others. `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`. + resolved cleartext in `op.Content`; `destReadPath` / `readDestText` + (`destread.go`) — the whole-file destination readers that carry the symlink + policy (`AGENTSYNC_ALLOW_SYMLINK_DEST`), mirroring `iox.AtomicWrite`'s. - **Commands:** `init`, `agent {add,remove,list,enable,disable}`, `apply`, `revert`, `status`, `diff`, `reconcile`, `import`, `doctor`, `check`, `mcp {add,remove,list,enable,disable}`, @@ -59,7 +61,7 @@ is the only package that depends on nearly all the others. `migrate subagents`, `explain `, `version`. - **Depends on:** adapter, source, state, secrets, paths, render, marketplace, - project, drift, git, ui, log. + project, drift, git, iox, ui, log. - **Files:** `root.go` + one file per command group + shared helpers (`destread.go`, `planwalk.go`). @@ -498,7 +500,9 @@ cleartext secrets the rendered files already contain). ### `internal/iox` Atomic file IO and locking. - **Key:** `AtomicWrite(dest, data, mode)`; `Lock`/`AcquireLock`/ - `AcquireLockTimeout`; `ErrSymlinkDest`; `AllowSymlinkDestEnv`. + `AcquireLockTimeout`; `ErrSymlinkDest`; `AllowSymlinkDestEnv`; + `SymlinkDestAllowed` (the one reading of it, shared with `internal/cli`'s + read-side gate). - **Files:** `atomic.go`, `lock.go`. ### `internal/jsonkeys` diff --git a/docs/user-guide.md b/docs/user-guide.md index 2e6c1ec2..feb71485 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -1029,7 +1029,11 @@ exclusive. Every `list` accepts `ls`, and every `remove` accepts `rm`. `status --json` and `diff [] --json` emit the structured report instead of the formatted one, suitable for CI gates and dashboards (`status --json` is never collapsed — it carries every tracked file; -`diff --json` masks the same resolved secrets the formatted diff does). For a +`diff --json` masks the same resolved secrets the formatted diff does; its +`pointer` field is an RFC-6901 pointer for a merged key, and one of two +pseudo-pointers for a whole-file finding that is not a text difference — +`mode` for a content-identical permission change, `symlink` for a symlinked +destination agentsync is not comparing through). For a gate that should **fail the build** on drift, add `--exit-code`: `status --exit-code` / `diff --exit-code` exit `2` when drift/hunks exist and `0` when clean (exit `2` is distinct from the generic error exit `1`, and prints no extra @@ -1058,7 +1062,7 @@ and the complete environment-variable table. The ones you'll reach for most: | Env var | Purpose | |---|---| | `AGENTSYNC_HOME` | Override the `~/.agentsync/` location. | -| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations (e.g. chezmoi-managed files). | +| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (e.g. chezmoi-managed files). Needed by every command — `status`/`diff`/`reconcile`/`explain` too — not only `apply`. | | `AGENTSYNC_ALLOW_INSECURE_URLS=1` | Accept `http://`/`git://` plugin/marketplace sources. | | `AGENTSYNC_ALLOW_OFFLINE_VERIFY=1` | Let `check` validate reference *shape* only, skipping resolution (CI without an age key). | | `AGENTSYNC_NO_UPGRADE_NOTICE=1` | Never show the one-time first-run-after-upgrade notice. | diff --git a/website/src/content/docs/help/troubleshooting.mdx b/website/src/content/docs/help/troubleshooting.mdx index 1f8d860d..f09a995d 100644 --- a/website/src/content/docs/help/troubleshooting.mdx +++ b/website/src/content/docs/help/troubleshooting.mdx @@ -89,6 +89,19 @@ the underlying file in place, keeping the link): AGENTSYNC_ALLOW_SYMLINK_DEST=1 agentsync apply ``` +The same switch governs how agentsync **reads** a symlinked whole-file +destination (a `CLAUDE.md`, a skill, a subagent), so it is needed for every +command, not only `apply`. Without it, `agentsync status` reports the file as +`drift` on every run even right after an apply, and `agentsync diff` prints a +`symlink` hunk instead of a content diff — that is the same refusal, seen from +the read side. With it, `status`, `diff`, `reconcile` and `explain` resolve the +link and compare the file it points at. (A key-merged file such as +`~/.claude.json` is read through the link either way.) + +```bash +AGENTSYNC_ALLOW_SYMLINK_DEST=1 agentsync status +``` + ### OpenCode hooks aren't auto-translated OpenCode hooks are JS/TS plugins, not declarative shell commands. agentsync does **not** auto-translate Claude hooks to OpenCode — hand-author a small JS/TS plugin diff --git a/website/src/content/docs/internals/security.mdx b/website/src/content/docs/internals/security.mdx index 4d5d092e..caa1542b 100644 --- a/website/src/content/docs/internals/security.mdx +++ b/website/src/content/docs/internals/security.mdx @@ -68,7 +68,10 @@ boundary, fetchers: - Writes are **atomic** (two-phase: stage → fsync → rename). - agentsync refuses to clobber **symlinked destinations** by default (a rename would replace the link with a regular file and strand your linked source); - override with `AGENTSYNC_ALLOW_SYMLINK_DEST=1`. + override with `AGENTSYNC_ALLOW_SYMLINK_DEST=1`. Since #229 the same switch + gates every read-side surface too: unset, `status`, `diff`, `reconcile` and + `explain` classify a symlinked whole-file destination as drift rather than + reading through it, and `reconcile` refuses to write back through it. - Pre-existing **foreign files** are backed up to `.state/backups//` before overwrite. diff --git a/website/src/content/docs/reference/environment.mdx b/website/src/content/docs/reference/environment.mdx index 0740e220..dbb7a9fd 100644 --- a/website/src/content/docs/reference/environment.mdx +++ b/website/src/content/docs/reference/environment.mdx @@ -15,7 +15,7 @@ the rest are escape hatches you'll rarely need. | Env var | Purpose | | --- | --- | | `AGENTSYNC_HOME` | Override the `~/.agentsync/` location (absolute path). | -| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations (e.g. chezmoi-managed files). | +| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (e.g. chezmoi-managed files). Needed by every command, not only `apply`. | | `AGENTSYNC_ALLOW_INSECURE_URLS=1` | Accept `http://` / `git://` plugin & marketplace sources. | | `AGENTSYNC_ALLOW_OFFLINE_VERIFY=1` | Let `check` validate reference *shape* only, skipping resolution (CI without an age key). | | `AGENTSYNC_NO_UPGRADE_NOTICE=1` | Never show the one-time [upgrade notice](/reference/upgrading/). | @@ -26,7 +26,7 @@ the rest are escape hatches you'll rarely need. | --- | --- | | `AGENTSYNC_HOME` | Override `~/.agentsync/` location (absolute path). | | `AGENTSYNC_TARGET_ROOT` | Redirect `$HOME` for testing (used by the hermetic test container). | -| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Permit writes to symlinked destination files (resolves the link first). | +| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (resolves the link first). Needed by every command — `status`/`diff`/`reconcile`/`explain` too — not only `apply`. | | `AGENTSYNC_ALLOW_INSECURE_URLS=1` | Accept `http://` and `git://` plugin / marketplace sources. | | `AGENTSYNC_ALLOW_UNIMPLEMENTED=1` | Register an agent with no implemented adapter yet (none today — every valid agent is real). | | `AGENTSYNC_ALLOW_PLUGIN_DRIFT=1` | Bypass the plugin-cache manifest-SHA check (after hand-editing). | @@ -41,4 +41,6 @@ the rest are escape hatches you'll rarely need. MITM protection on insecure URLs, strand-protection on symlinked destinations, tamper-detection on the plugin cache. Set them deliberately and scope them narrowly (e.g. a single command invocation), not globally in your shell profile. + The exception is `AGENTSYNC_ALLOW_SYMLINK_DEST`: once you rely on it, every + command needs it, because it governs reads as well as writes. From a9a628af3880ae971c2850bc9cb27a69a1a7c4a0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 02:53:57 +0000 Subject: [PATCH 04/11] docs: point the reconcile mode-drift gap at #245 The issue the mode commit deferred to now exists. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 4 ++-- docs/architecture.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 456b085f..6bb74a74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,8 +45,8 @@ source layout, CLI surface, and state schema are stabilizing but may still chang content-identical chmod made `status` say `drift` and `explain` say `clean` about the same file in the same second; both now read the one folded class. `reconcile` still ignores mode entirely — it reports "nothing to reconcile" - for a drift the other three surfaces name — and that gap is tracked as its - own issue rather than fixed here. + for a drift the other three surfaces name — and that gap is + [#245](https://github.com/spxrogers/agentsync/issues/245), not fixed here. - **A FIFO at a managed destination no longer hangs `status`, `diff`, `explain`, or `reconcile`'s drift walk and write-back.** (A directory there never hung — `os.ReadFile` diff --git a/docs/architecture.md b/docs/architecture.md index 9e694d6d..f20e3af6 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -893,7 +893,7 @@ fold permission drift into the class of a content-clean whole file — measured against `op.Mode`, the mode the next apply chmods to, which is the same question `diff`'s `mode` hunk asks (`planItem.classWithModeDrift`, one method, so the three cannot disagree). `reconcile` still ignores mode entirely; that gap is -tracked as its own issue. `diff` masks and compares text, `reconcile` excludes +#245. `diff` masks and compares text, `reconcile` excludes an orphan another agent still renders, `explain` groups by owner. A **symlinked** destination is read through only when From 68df434a00e9b2a40ec9e955d6bb03b7126ffdaf Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 07:41:49 +0000 Subject: [PATCH 05/11] =?UTF-8?q?fix(cli):=20close=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20withhold=20[o]=20on=20a=20refused=20symlink,=20spli?= =?UTF-8?q?t=20"unresolvable"=20from=20"refused",=20fold=20mode=20drift=20?= =?UTF-8?q?on=20converged=20content?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 22 +++---- SECURITY.md | 3 +- docs/architecture.md | 15 +++-- internal/cli/destread.go | 65 ++++++++++--------- internal/cli/destread_unix_internal_test.go | 59 ++++++++++++++++- internal/cli/diff.go | 18 +++-- internal/cli/planwalk.go | 13 ++-- .../cli/planwalk_characterization_test.go | 16 ++--- internal/cli/planwalk_internal_test.go | 30 +++++++++ internal/cli/reconcile.go | 53 ++++++++++----- internal/cli/status.go | 19 ++++-- internal/cli/symlink_dest_test.go | 6 +- internal/iox/atomic_test.go | 2 + 13 files changed, 224 insertions(+), 97 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bb74a74..125fff47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,18 +19,13 @@ source layout, CLI surface, and state schema are stabilizing but may still chang sentinel that can never equal a content hash — so every run reported `drift` no apply could clear and `status --exit-code` failed CI forever, while `diff` read through the link and said `no diff`. The switch now governs the READ - side too, on every surface: with it set, all four resolve the link and - compare the file it points at (content, text and permission bits alike), so - a converged chezmoi setup reports `clean`; with it unset, all four refuse the - link as they always did, and `diff` prints a `symlink` hunk (`--json` - `pointer: "symlink"`, alongside the existing `mode` pseudo-pointer) naming - the switch instead of silently agreeing there is no difference. `reconcile` - shows such an item with the SHA display rather than a text diff against an - empty destination, and its `[w]`rite-back refuses to capture through a link - the classification did not read through (naming the switch, which every - command now needs — not only `apply`). Key-merged destinations (a symlinked - `~/.claude.json`) are decoded through the link either way, deliberately, as - `apply` treats them. + side too, on every surface and for every command, not only `apply`: set, all + four resolve the link and compare the file it points at; unset, all four + refuse the link, `diff` prints a `symlink` hunk (`--json` `pointer: + "symlink"`, alongside `mode`) naming the switch, and `reconcile`'s + `[w]`rite-back refuses to capture through it (and does not offer + `[o]verride`: #248). A link that does not resolve — dangling, loop — is + reported as such rather than as "set the switch". - **`status`'s permission check now measures the mode the next `apply` writes** ([#229](https://github.com/spxrogers/agentsync/issues/229)). It @@ -38,7 +33,8 @@ source layout, CLI surface, and state schema are stabilizing but may still chang last apply, so a file whose recorded mode was unset (state written before modes were recorded) or whose adapter changed the mode it renders was reported `clean` while the next apply would chmod it. It now asks `op.Mode`, - the question `diff`'s `mode` hunk already asked, so the two agree. + the question `diff`'s `mode` hunk already asked, so the two agree — for + `converged` content as well as `clean`, which the old check skipped. - **`explain ` now reports a mode-only drift instead of `clean`** ([#229](https://github.com/spxrogers/agentsync/issues/229)). A diff --git a/SECURITY.md b/SECURITY.md index 6c56c3b4..80144f0e 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -94,7 +94,8 @@ can resolve secrets into native config files. Areas of particular interest: diagnostics surface — native marketplace ids and source types) stay plain strings. - **Destination writes**: writes are atomic and refuse to clobber symlinked - destinations by default — and, since #229, every read-side surface + destinations by default (one known gap: `Writer.Write`'s convergence chmod + still follows a link, #248) — and, since #229, every read-side surface (`status`, `diff`, `reconcile`, `explain`) refuses to read through a symlinked whole-file destination under the same switch, `AGENTSYNC_ALLOW_SYMLINK_DEST`, reporting it as drift instead; pre-existing diff --git a/docs/architecture.md b/docs/architecture.md index f20e3af6..2cdb4fb7 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -902,11 +902,16 @@ writes through the link. `iox.SymlinkDestAllowed` is the one reading of the switch, and `destReadPath` (`internal/cli/destread.go`) is the read-side gate that the three whole-file destination facts — the content hash, the permission bits and the text — pass through. Unset, all four surfaces answer an opaque -sentinel: the file classifies `drift`; `diff` prints a `symlink` hunk naming the -switch rather than reading through and reporting no difference; `reconcile` -shows the SHA display instead of a text diff and its `[w]`rite-back refuses to -capture through the link. Set, all four resolve the link and compare the file it -points at, so a converged chezmoi setup reports `clean`. The mirror is a policy, +sentinel that can never equal a content hash, so the classifier sees a changed +destination (`drift`, or `conflict`/`foreign-collision` by its usual table); +`diff` prints a `symlink` hunk naming the switch rather than reading through and +reporting no difference; `reconcile` shows the SHA display instead of a text +diff, and its `[w]`rite-back refuses to capture through the link and withholds +`[o]verride` (#248: `Writer.Write`'s mode arm chmods through a link before the +policy is consulted). Set, all four resolve the link and compare the file it +points at, so a converged chezmoi setup reports `clean`; a link that does not +resolve answers a second sentinel so the advice is "fix the link", not "set the +switch". The mirror is a policy, not a prediction: `apply` itself only fails on a symlinked destination when the content differs (its convergence read follows the link), so the unset answer means "a managed regular file became a link you have not opted into — that is diff --git a/internal/cli/destread.go b/internal/cli/destread.go index 24544205..2066c0ce 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -29,6 +29,16 @@ var errDestNotRegular = errors.New("not a regular file") // compared only for equality, deliberately treats both alike — see its comment. var errDestUnstattable = errors.New("cannot stat destination") +// pathlessStatErr strips the redundant path from a *fs.PathError, mirroring +// secrets.pathlessErr. errors.Is still matches the underlying errno. +func pathlessStatErr(err error) error { + var pe *fs.PathError + if errors.As(err, &pe) { + return pe.Err + } + return err +} + // readDestBytes reads a destination file's bytes, refusing before the open any // path whose shape cannot be read as a file. // @@ -76,16 +86,6 @@ var errDestUnstattable = errors.New("cannot stat destination") // An ABSENT path is not refused: os.ReadFile runs and its ENOENT reaches the // caller unchanged, because manufacturing a shape error for a file that is not // there would name the wrong problem. -// pathlessStatErr strips the redundant path from a *fs.PathError, mirroring -// secrets.pathlessErr. errors.Is still matches the underlying errno. -func pathlessStatErr(err error) error { - var pe *fs.PathError - if errors.As(err, &pe) { - return pe.Err - } - return err -} - func readDestBytes(path string) ([]byte, error) { // render.IsRegularOrAbsent stays the single authority on SHAPE, and it is // asked FIRST so the ordinary read costs exactly one stat. It answers false @@ -108,11 +108,21 @@ func readDestBytes(path string) ([]byte, error) { return os.ReadFile(path) } +// symlinkRefusal is why destReadPath would not look through a symlink. +type symlinkRefusal int + +const ( + symlinkNone symlinkRefusal = iota // not a symlink, or resolved through it + symlinkRefusedByEnv // switch unset: apply would not write through it either + symlinkUnresolvable // opted in, but the link does not resolve (dangling, loop): apply fails on it too +) + // destReadPath applies the destination SYMLINK policy and answers the path the -// whole-file destination reads should actually look at. ok is false when the -// destination is a symlink this configuration does not look through; each caller -// answers its own "cannot read" value for that (hashFile the symlink sentinel, -// destModePerm (0, false), readDestText ""). +// whole-file destination reads should actually look at. why is symlinkNone when +// resolved can be read; otherwise each caller answers its own "cannot read" +// value (hashFile a sentinel per reason, destModePerm (0, false), readDestText +// ""), and the two reasons are kept apart so the advice a user sees is right: +// "set the switch" is wrong advice for a link that would not resolve anyway. // // It mirrors iox.resolveSymlinkDest, the write side, through the shared // iox.SymlinkDestAllowed, so the read side and apply cannot disagree about @@ -122,37 +132,30 @@ func readDestBytes(path string) ([]byte, error) { // managed regular file became a link you have not opted into — that is drift" // rather than "apply would fail here". // -// The policy is scoped to WHOLE-FILE facts: the symlink sentinel is a -// whole-file-only policy signal. A key-merge destination is decoded through the -// link by readDestFile on every surface, as apply does — a converged symlinked -// key-merge destination is a no-op plus a chmod through the link, a differing -// one is iox.ErrSymlinkDest, exactly as for a whole-file destination, so the -// read-through already has apply-parity. Refusing it instead would make EVERY -// owned pointer of a chezmoi ~/.claude.json classify against -// permanently — there is no per-pointer sentinel the classifier could carry — -// with reconcile's [d] showing and its [w] failing on "not found in -// destination". -func destReadPath(path string) (resolved string, ok bool) { +// The policy is scoped to WHOLE-FILE facts; a key-merge destination is decoded +// through the link on every surface, as apply does (docs/architecture.md §6 +// has the reasoning). +func destReadPath(path string) (resolved string, why symlinkRefusal) { fi, err := os.Lstat(path) if err != nil || fi.Mode()&os.ModeSymlink == 0 { - return path, true + return path, symlinkNone } if !iox.SymlinkDestAllowed() { - return "", false + return "", symlinkRefusedByEnv } r, err := filepath.EvalSymlinks(path) if err != nil { - return "", false + return "", symlinkUnresolvable } - return r, true + return r, symlinkNone } // readDestText is the whole-file destination text read behind the drift walk: // the destination's bytes, or "" for a refused symlink, a refused shape, or any // read error — the three collapse because no caller distinguishes them. func readDestText(path string) string { - p, ok := destReadPath(path) - if !ok { + p, why := destReadPath(path) + if why != symlinkNone { return "" } b, err := readDestBytes(p) diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index 04ada8b8..b840816d 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -302,6 +302,22 @@ func TestHashFileSentinels(t *testing.T) { // this is the digest of "payload", what state records for it. want: "239f59ed55e737c77147cf55ad0c1b030b6d7ee748a7426952f9b852d5a935e5", }, + { + // Opted in, but the link does not resolve: a distinct sentinel, so + // diff and reconcile can say "fix the link" instead of "set the + // switch you already set". Pins destReadPath's EvalSymlinks arm. + name: "a dangling link is unresolvable when the env is set", + setup: func(t *testing.T, tmp string) string { + t.Helper() + t.Setenv(iox.AllowSymlinkDestEnv, "1") + link := filepath.Join(tmp, "link") + if err := os.Symlink(filepath.Join(tmp, "gone"), link); err != nil { + t.Fatal(err) + } + return link + }, + want: symlinkUnresolvableSentinel, + }, { name: "a FIFO is refused by shape", setup: mkfifoDest, @@ -429,11 +445,16 @@ func TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough(t *testing.T) { t.Fatal("writeBackFileItem = nil for a refused symlink, want an error: [w] must not " + "capture through a link the classification did not read through") } - for _, want := range []string{link, iox.AllowSymlinkDestEnv + "=1", "for every command", "[o]verride", "[i]gnore"} { + for _, want := range []string{link, iox.AllowSymlinkDestEnv + "=1", "for every command", "[i]gnore"} { if !strings.Contains(err.Error(), want) { t.Errorf("error = %q, want it to contain %q", err, want) } } + // [o]verride re-applies through Writer.Write, whose mode arm chmods + // the TARGET through the link (#248): never suggest it here. + if strings.Contains(err.Error(), "[o]verride") { + t.Errorf("error = %q offers [o]verride on a refused symlink", err) + } if strings.Contains(err.Error(), "non-regular") { t.Errorf("error = %q calls a symlink non-regular; the remedy must match the failure", err) } @@ -442,6 +463,42 @@ func TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough(t *testing.T) { } }) + t.Run("env unset: a link to a FIFO takes the shape arm, which withholds [o]verride", func(t *testing.T) { + t.Setenv(iox.AllowSymlinkDestEnv, "") + if err := os.Unsetenv(iox.AllowSymlinkDestEnv); err != nil { + t.Fatal(err) + } + tmp := t.TempDir() + fifo := mkfifoDest(t, tmp) + link := filepath.Join(tmp, "link.md") + if err := os.Symlink(fifo, link); err != nil { + t.Fatal(err) + } + err := writeBackFileItem(t.TempDir(), reconcileItem{op: adapter.FileOp{Path: link, SourceID: "demo"}}) + if err == nil || !errors.Is(err, errDestNotRegular) { + t.Fatalf("writeBackFileItem = %v, want the shape refusal (errDestNotRegular) for a link to a FIFO", err) + } + if strings.Contains(err.Error(), "[o]verride") || strings.Contains(err.Error(), iox.AllowSymlinkDestEnv) { + t.Errorf("error = %q: a link to a FIFO must get the shape remedy, not the symlink one", err) + } + }) + + t.Run("env set: a dangling link is unresolvable and the advice does not name the switch", func(t *testing.T) { + t.Setenv(iox.AllowSymlinkDestEnv, "1") + tmp := t.TempDir() + link := filepath.Join(tmp, "link.md") + if err := os.Symlink(filepath.Join(tmp, "gone"), link); err != nil { + t.Fatal(err) + } + err := writeBackFileItem(t.TempDir(), reconcileItem{op: adapter.FileOp{Path: link, SourceID: "demo"}}) + if err == nil || !strings.Contains(err.Error(), "cannot be resolved") { + t.Fatalf("writeBackFileItem = %v, want the unresolvable-link refusal", err) + } + if strings.Contains(err.Error(), iox.AllowSymlinkDestEnv+"=1") || strings.Contains(err.Error(), "[o]verride") { + t.Errorf("error = %q tells a user who already set the switch to set it, or offers [o]verride", err) + } + }) + t.Run("env set: captures the linked file", func(t *testing.T) { t.Setenv(iox.AllowSymlinkDestEnv, "1") home := t.TempDir() diff --git a/internal/cli/diff.go b/internal/cli/diff.go index c2895858..d7219e69 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -236,17 +236,23 @@ func modeHunk(it planItem) (source, dest string, ok bool) { const symlinkHunkDest = "symlink (not compared through; set " + iox.AllowSymlinkDestEnv + "=1 to read and write through the link)" +// symlinkUnresolvableHunkDest is its sibling for a link the user opted into +// that does not resolve (dangling, loop); apply fails on it the same way. +const symlinkUnresolvableHunkDest = "symlink (target cannot be resolved: dangling or loop; apply refuses it too)" + // symlinkHunk describes a destination that is a symlink the read side will not -// look through because AGENTSYNC_ALLOW_SYMLINK_DEST is not set — the same -// condition under which iox.AtomicWrite refuses to write through one. Without -// it diff read THROUGH the link and printed "no diff" for a destination status -// called drift and `--exit-code` failed a CI gate on. A whole-file hunk with an -// empty Dest was rejected too: it asserts the destination is EMPTY, which is -// false, and renders the entire source as one insertion. +// look through — refused by AGENTSYNC_ALLOW_SYMLINK_DEST being unset, the same +// condition under which iox.AtomicWrite refuses to write through one, or +// unresolvable. A whole-file hunk with an empty Dest was rejected: it asserts +// the destination is EMPTY, which is false, and renders the entire source as +// one insertion. func symlinkHunk(it planItem) (source, dest string, ok bool) { if !it.destSymlinkRefused() { return "", "", false } + if it.hdest == symlinkUnresolvableSentinel { + return "regular file", symlinkUnresolvableHunkDest, true + } return "regular file", symlinkHunkDest, true } diff --git a/internal/cli/planwalk.go b/internal/cli/planwalk.go index dc9bb27d..b70e015c 100644 --- a/internal/cli/planwalk.go +++ b/internal/cli/planwalk.go @@ -88,11 +88,12 @@ func (i planItem) opModeDrifted() bool { } // classWithModeDrift is the class status and explain report: the content class, -// upgraded from clean to drift when only the permission bits differ from what the +// upgraded from clean or converged (content in sync either way) to drift when +// only the permission bits differ from what the // next apply would WRITE (op.Mode — render.Writer.Write chmods to it). A merged key // has no mode, and an orphan's synthesized op carries Mode 0, so both fall through. func (i planItem) classWithModeDrift() drift.Class { - if i.ptr == "" && i.cls == drift.Clean && i.opModeDrifted() { + if i.ptr == "" && (i.cls == drift.Clean || i.cls == drift.Converged) && i.opModeDrifted() { return drift.Drift } return i.cls @@ -103,7 +104,9 @@ func (i planItem) classWithModeDrift() drift.Class { // unset). It is a DERIVATION from hdest, not a field: diff keys its symlink // hunk on the very hash status, reconcile and explain classified from, so the // four surfaces cannot disagree about it (#229 axis 9). -func (i planItem) destSymlinkRefused() bool { return i.ptr == "" && i.hdest == symlinkSentinel } +func (i planItem) destSymlinkRefused() bool { + return i.ptr == "" && (i.hdest == symlinkSentinel || i.hdest == symlinkUnresolvableSentinel) +} // destModePerm answers the permission bits of the REGULAR file at path. // regular is false — and perm 0 — for an absent, refused-symlink @@ -113,8 +116,8 @@ func (i planItem) destSymlinkRefused() bool { return i.ptr == "" && i.hdest == s // resolved and the perm is the TARGET's — the bits apply's mode fix chmods // through the link. func destModePerm(path string) (perm uint32, regular bool) { - path, ok := destReadPath(path) - if !ok { + path, why := destReadPath(path) + if why != symlinkNone { return 0, false } fi, err := os.Lstat(path) diff --git a/internal/cli/planwalk_characterization_test.go b/internal/cli/planwalk_characterization_test.go index 57193c41..05b507fb 100644 --- a/internal/cli/planwalk_characterization_test.go +++ b/internal/cli/planwalk_characterization_test.go @@ -17,14 +17,14 @@ import ( // 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. +// answered before they were unified behind one shared walk (#244). 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 that behaviour, so a refactor that needs to edit a +// golden here is not a refactor. Two goldens are the deliberate exceptions and +// encode the policy #229's PR-C CHOSE, not the old answer: T-09 +// (whole-file/mode-drift-only: explain folds mode drift) and T-10 +// (whole-file/dest-is-symlink: diff's symlink hunk, reconcile's SHA fallback). // // 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 diff --git a/internal/cli/planwalk_internal_test.go b/internal/cli/planwalk_internal_test.go index 925af961..545d89d6 100644 --- a/internal/cli/planwalk_internal_test.go +++ b/internal/cli/planwalk_internal_test.go @@ -241,6 +241,36 @@ func TestWalkPlanItems(t *testing.T) { } }, }, + { + // Converged content (dest == source, state stale) with a mode + // difference is drift too: the next apply chmods it, and diff + // already prints a mode hunk for it. Clean-only folding let status + // say "in sync" while diff --exit-code failed. + name: "converged-content-with-mode-drift-folds-to-drift", + run: func(t *testing.T, h string) { + p := dest(h, "s.sh") + mustWrite(t, p, "NEW") + if err := os.Chmod(p, 0o644); err != nil { + t.Fatal(err) + } + s := state.New() + s.Files[fileKey(h, "claude", p)] = state.FileEntry{SHA256: hf("OLD")} + op := fileOp(p, "NEW") + op.Mode = 0o755 + items := walkUser(h, planFor(map[string][]adapter.FileOp{"claude": {op}}), s, []string{"claude"}, nil) + if len(items) != 1 || items[0].cls != drift.Converged { + t.Fatalf("fixture: want one converged item, got %+v", itemKeys(items)) + } + if got := items[0].classWithModeDrift(); got != drift.Drift { + t.Errorf("classWithModeDrift on converged content with a mode difference = %v, want drift", got) + } + op.Mode = 0o644 + items = walkUser(h, planFor(map[string][]adapter.FileOp{"claude": {op}}), s, []string{"claude"}, nil) + if got := items[0].classWithModeDrift(); got != drift.Converged { + t.Errorf("classWithModeDrift with the mode in sync = %v, want converged untouched", got) + } + }, + }, { name: "action-not-write-is-skipped", run: func(t *testing.T, h string) { diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index e941f90a..495fae6d 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -670,8 +670,8 @@ func collectReconcileItems(plan render.RenderPlan, reg *adapter.Registry, s *sta } // A refused whole-file symlink has no destination text to show: fall // back to the SHA display rather than render the entire source as an - // insertion against an empty destination — the rendering diff's - // symlink hunk exists to avoid (#229 axis 9). + // insertion against an empty destination (the rendering diff's symlink + // hunk exists to avoid). ri.srcText, ri.dstText, ri.hasText = it.srcText, it.dstText, !it.destSymlinkRefused() if it.ptr != "" { ri.pluginOwner = pluginOwnerForKeyItem(it.op.SourceID, it.ptr, pluginOwners) @@ -728,10 +728,19 @@ func shortVal(hash string) string { if hash == "" { return "" } - if len(hash) > 16 { + if len(hash) > 16 && isHexDigest(hash) { return hash[:16] + "..." } - return hash + return hash // a sentinel ("symlink-not-regular-file") is shown whole +} + +func isHexDigest(s string) bool { + for _, c := range s { + if (c < '0' || c > '9') && (c < 'a' || c > 'f') { + return false + } + } + return true } // renderItemValues shows the differing source/destination CONTENT for an item as @@ -1096,19 +1105,27 @@ func writeBackKeyItem(cmd *cobra.Command, home string, it reconcileItem) error { // // Both used to return nil with a success message, hiding data loss. func writeBackFileItem(home string, it reconcileItem) error { - readPath, ok := destReadPath(it.op.Path) - if !ok { - // The destination is a symlink this configuration does not look - // through — the same refusal the drift walk classified the item under, - // so [w] cannot quietly capture a file the classification never read. - // [o]verride stays offered, unlike the non-regular arm below: - // Writer.Write's convergence read follows the link and either no-ops - // (content converged) or fails cleanly with iox.ErrSymlinkDest; no - // FIFO is involved, so it cannot hang. + readPath, why := destReadPath(it.op.Path) + if why != symlinkNone && !render.IsRegularOrAbsent(it.op.Path) { + // A link to a FIFO or device is a SHAPE problem first (IsRegularOrAbsent + // Stats, so it follows the link): take the shape arm below, which is + // the arm that must never suggest [o]verride. + why, readPath = symlinkNone, it.op.Path + } + switch why { + case symlinkRefusedByEnv: + // The drift walk classified this item without reading through the + // link, so [w] must not quietly capture through it. [o]verride is + // withheld on both symlink arms: Writer.Write's convergence read + // follows the link, and its mode arm chmods the TARGET through it + // before the symlink policy is consulted (#248). return fmt.Errorf("read dest %s: destination is a symlink agentsync is not reading through — "+ - "set %s=1 (for every command) to capture the linked file, replace the link with a regular "+ - "file, use [o]verride to re-apply canonical, or [i]gnore to suppress this item", - it.op.Path, iox.AllowSymlinkDestEnv) + "set %s=1 (for every command) to read and write through the link, replace the link with a "+ + "regular file, or [i]gnore to suppress this item", it.op.Path, iox.AllowSymlinkDestEnv) + case symlinkUnresolvable: + return fmt.Errorf("read dest %s: destination is a symlink whose target cannot be resolved "+ + "(dangling or loop; apply refuses it too) — fix or replace the link, or [i]gnore to "+ + "suppress this item", it.op.Path) } data, err := readDestBytes(readPath) if err != nil { @@ -1116,8 +1133,8 @@ func writeBackFileItem(home string, it reconcileItem) error { // mid-prompt with a keystroke to choose, and "read dest X: not a regular // file" alone does not tell them which one gets them unstuck. // - // [o]verride is deliberately NOT offered for THIS arm, unlike the peer - // refusals in this file and unlike the arm below. It re-applies + // [o]verride is deliberately NOT offered for THIS arm (nor for the + // symlink arms above), unlike the absent arm below. It re-applies // through render.Writer.Write, whose convergence read is not // shape-guarded, so on this exact item it does not fail — it HANGS // (measured: `reconcile --auto-override` rc=124). diff --git a/internal/cli/status.go b/internal/cli/status.go index 5388e537..abf0df01 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -944,6 +944,11 @@ func stateKeyKey(userHome, agent string, sc adapter.Scope, projectRoot, path, pt // (planItem.destSymlinkRefused), so the two sites must agree on it. const symlinkSentinel = "symlink-not-regular-file" +// symlinkUnresolvableSentinel is hashFile's answer for a symlink the user opted +// into reading through that does not resolve (dangling, loop). Equally opaque; +// a different token only so diff and reconcile can give the right advice. +const symlinkUnresolvableSentinel = "symlink-target-unresolvable" + func hashContent(b []byte) string { sum := sha256.Sum256(b) return hex.EncodeToString(sum[:]) @@ -964,15 +969,19 @@ func hashContent(b []byte) string { // converges — so a chezmoi setup reports clean after a successful apply // instead of a drift no apply can clear. Opting in never lets a non-regular // target through: a link to one (`ln -s /dev/null`) resolves and then answers -// the SHAPE sentinel below; a dangling link answers symlinkSentinel with the -// env set or unset, mirroring apply's "resolve symlink" failure. +// the SHAPE sentinel below; a dangling or looping link answers +// symlinkUnresolvableSentinel once opted in (unset, it is refused like any +// other link), mirroring apply's "resolve symlink" failure. func hashFile(path string) string { - p, ok := destReadPath(path) - if !ok { - // The link target is deliberately NOT part of the sentinel: it is + p, why := destReadPath(path) + switch why { + case symlinkRefusedByEnv: + // The link target is deliberately NOT part of either sentinel: it is // attacker-choosable, and a sentinel must stay a stable opaque token // that never equals a content hash. return symlinkSentinel + case symlinkUnresolvable: + return symlinkUnresolvableSentinel } path = p // A FIFO, device, or socket at a destination path would make os.ReadFile diff --git a/internal/cli/symlink_dest_test.go b/internal/cli/symlink_dest_test.go index 358de439..58114e9c 100644 --- a/internal/cli/symlink_dest_test.go +++ b/internal/cli/symlink_dest_test.go @@ -128,7 +128,8 @@ func TestSymlinkedDestConvergesWhenAllowed(t *testing.T) { } // TestSymlinkedDestIsDriftWhenRefused (N-5) is the other half: the same -// converged link, with the switch UNSET, is drift on every surface — and `diff` +// converged link, with the switch UNSET, is drift on status and diff (reconcile +// and explain are pinned at the walk level by the harness's T-10) — and `diff` // says so with a "symlink" hunk naming the switch, instead of reading through // the link and printing "no diff" beside a `status --exit-code` that fails. // The content is genuinely converged (applied through the link first), so the @@ -175,7 +176,4 @@ func TestSymlinkedDestIsDriftWhenRefused(t *testing.T) { if strings.Contains(h.Dest, target) || strings.Contains(h.Dest, "dotfiles") { t.Errorf("symlink hunk embeds the link target; it must be a constant: %+v", h) } - if strings.Contains(h.Dest, "/") { - t.Errorf("symlink hunk Dest contains a path separator; it must embed no path: %q", h.Dest) - } } diff --git a/internal/iox/atomic_test.go b/internal/iox/atomic_test.go index c4b2e18f..5ca43910 100644 --- a/internal/iox/atomic_test.go +++ b/internal/iox/atomic_test.go @@ -122,6 +122,8 @@ func TestSymlinkDestAllowed(t *testing.T) { {name: "unset", unset: true, want: false}, {name: "set to 1", value: "1", want: true}, {name: "set to 0", value: "0", want: false}, + {name: "set to true", value: "true", want: false}, + {name: "set to 1 with trailing space", value: "1 ", want: false}, } { t.Run(tc.name, func(t *testing.T) { t.Setenv(iox.AllowSymlinkDestEnv, tc.value) // registers the restore From f37820bdbe32adf8b8903ce2891655ed1536aaf9 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 07:58:30 +0000 Subject: [PATCH 06/11] =?UTF-8?q?fix(cli):=20close=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20decide=20a=20link's=20target=20shape=20in=20the=20g?= =?UTF-8?q?ate,=20pin=20the=20unresolvable=20path=20on=20every=20surface?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 9 +-- README.md | 4 +- docs/concepts.md | 5 +- docs/user-guide.md | 2 +- internal/cli/destread.go | 16 +++-- internal/cli/destread_unix_internal_test.go | 71 ++++++++++++++++++- internal/cli/diff.go | 8 +-- internal/cli/planwalk.go | 6 +- internal/cli/planwalk_internal_test.go | 46 ++++++++++++ internal/cli/reconcile.go | 14 ++-- internal/cli/status.go | 20 +++--- internal/cli/symlink_dest_test.go | 6 ++ .../content/docs/reference/environment.mdx | 8 +-- 13 files changed, 170 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 125fff47..8150da2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,13 +19,14 @@ source layout, CLI surface, and state schema are stabilizing but may still chang sentinel that can never equal a content hash — so every run reported `drift` no apply could clear and `status --exit-code` failed CI forever, while `diff` read through the link and said `no diff`. The switch now governs the READ - side too, on every surface and for every command, not only `apply`: set, all + side too — `apply`, `status`, `diff`, `reconcile` and `explain` all need it: set, all four resolve the link and compare the file it points at; unset, all four refuse the link, `diff` prints a `symlink` hunk (`--json` `pointer: "symlink"`, alongside `mode`) naming the switch, and `reconcile`'s `[w]`rite-back refuses to capture through it (and does not offer `[o]verride`: #248). A link that does not resolve — dangling, loop — is - reported as such rather than as "set the switch". + reported as such rather than as "set the switch", and a link to a FIFO or a + directory is reported as that shape, which the switch could not fix. - **`status`'s permission check now measures the mode the next `apply` writes** ([#229](https://github.com/spxrogers/agentsync/issues/229)). It @@ -33,8 +34,8 @@ source layout, CLI surface, and state schema are stabilizing but may still chang last apply, so a file whose recorded mode was unset (state written before modes were recorded) or whose adapter changed the mode it renders was reported `clean` while the next apply would chmod it. It now asks `op.Mode`, - the question `diff`'s `mode` hunk already asked, so the two agree — for - `converged` content as well as `clean`, which the old check skipped. + the question `diff`'s `mode` hunk already asked, so the two agree — on + `converged` destinations as well as `clean` ones. - **`explain ` now reports a mode-only drift instead of `clean`** ([#229](https://github.com/spxrogers/agentsync/issues/229)). A diff --git a/README.md b/README.md index 24c7038e..5778413b 100644 --- a/README.md +++ b/README.md @@ -206,7 +206,7 @@ If you lose your age private key, you lose access to all encrypted secrets. Reco - **Hand-edits to agentsync-owned keys** in shared agent files (e.g. an MCP server entry in `~/.claude.json` that agentsync owns): the next `apply` overwrites them with NO foreign-collision backup, because agentsync considers them its own. Use `agentsync reconcile` (the drift classifier catches the edit and offers `[w]`rite-back) BEFORE the next apply if you want to keep them. - **Destination git backup is local-only**: `apply` keeps each managed destination dir (`~/.claude`, `~/.codex`, …) in its own git history so `agentsync revert` can undo a bad apply — but that history is **never pushed** (the rendered files hold `${secret:…}` references resolved to **cleartext**, so its commits may too; the `.git` dir is hardened to `0700`, which is **POSIX-only** — a Windows no-op, where NTFS ACLs are the boundary). A destination dir already under **your own** source control is detected as `foreign source control` and left un-versioned (only `agentsync-versioned` dirs are reverted; an `untracked` dir is a candidate for init). `$HOME`-level strays (Claude's `~/.claude.json`) are never versioned — agentsync never inits a repo at `$HOME`. `revert`'s "nothing is lost" guarantee covers **tracked files only**: untracked / gitignored scratch files in the dir are left untouched, never snapshotted. - **Plain-http / git:// plugin sources** are rejected by default to prevent MITM swap. Set `AGENTSYNC_ALLOW_INSECURE_URLS=1` for internal mirrors. -- **Symlinked destinations** (e.g. `~/.claude.json` is a chezmoi symlink into your dotfiles repo) are rejected by default — a rename onto the path would replace the symlink with a regular file and strand your linked source. Set `AGENTSYNC_ALLOW_SYMLINK_DEST=1` to write through the symlink instead (the underlying file is updated in place; the link survives). The same switch governs how agentsync READS a symlinked whole-file destination (a `CLAUDE.md`, a skill, a subagent), so it is needed for every command, not only `apply`: without it `status`, `diff`, `reconcile` and `explain` report the file as drifted rather than comparing through the link (and `diff` says so, with a `symlink` hunk naming the switch); with it they resolve the link and compare the file it points at, so a chezmoi setup reports clean after a successful apply. A key-merged file such as `~/.claude.json` is read through the link either way. +- **Symlinked destinations** (e.g. `~/.claude.json` is a chezmoi symlink into your dotfiles repo) are rejected by default — a rename onto the path would replace the symlink with a regular file and strand your linked source. Set `AGENTSYNC_ALLOW_SYMLINK_DEST=1` to write through the symlink instead (the underlying file is updated in place; the link survives). The same switch governs how agentsync READS a symlinked whole-file destination (a `CLAUDE.md`, a skill, a subagent), so `apply` and every drift command — `status`, `diff`, `reconcile`, `explain` — need it: without it `status`, `diff`, `reconcile` and `explain` report the file as drifted rather than comparing through the link (and `diff` says so, with a `symlink` hunk naming the switch); with it they resolve the link and compare the file it points at, so a chezmoi setup reports clean after a successful apply. A key-merged file such as `~/.claude.json` is read through the link either way. - **Aider** and **Firebender**: deliberately deferred — no faithful generic projection (Aider has no MCP and only an `.aider.conf.yml` `read:` pointer for memory; Firebender's config is unverified). ## Environment overrides @@ -215,7 +215,7 @@ If you lose your age private key, you lose access to all encrypted secrets. Reco | --- | --- | | `AGENTSYNC_HOME` | Override `~/.agentsync/` location (absolute path). | | `AGENTSYNC_TARGET_ROOT` | Redirect `$HOME` for testing (used by the hermetic test container). | -| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (chezmoi-managed files). Needed by every command, not only `apply`. | +| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (chezmoi-managed files). Needed by `apply` and by `status`/`diff`/`reconcile`/`explain`. | | `AGENTSYNC_ALLOW_INSECURE_URLS=1` | Accept http:// and git:// plugin / marketplace sources. | | `AGENTSYNC_ALLOW_UNIMPLEMENTED=1` | Register an agent that has no implemented adapter yet (none today — every valid agent is real). | | `AGENTSYNC_ALLOW_PLUGIN_DRIFT=1` | Bypass the plugin-cache manifest-SHA check (after hand-editing). | diff --git a/docs/concepts.md b/docs/concepts.md index 81e50879..02063670 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -120,7 +120,10 @@ recovery beyond your own source control of the destination. **clean** — both mean `apply` has nothing left to do, and the distinction above is bookkeeping the classifier and `status --json` need, not something a human scanning the report benefits from. `status --legend` prints this table -(as a CLI reference); `status --json` always reports the real class. +(as a CLI reference); `status --json` always reports the real class. One fold +applies to both: a clean or converged whole file whose permission bits differ +from what `apply` writes is reported as **drift**, because the next apply +changes it. Granularity is **per-key** for structured files (JSON/JSONC/TOML, tracked by JSON pointer) and **per-file** for everything else. Keys agentsync never wrote diff --git a/docs/user-guide.md b/docs/user-guide.md index feb71485..2fcbafbd 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -1062,7 +1062,7 @@ and the complete environment-variable table. The ones you'll reach for most: | Env var | Purpose | |---|---| | `AGENTSYNC_HOME` | Override the `~/.agentsync/` location. | -| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (e.g. chezmoi-managed files). Needed by every command — `status`/`diff`/`reconcile`/`explain` too — not only `apply`. | +| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (e.g. chezmoi-managed files). Needed by `apply` and by `status`/`diff`/`reconcile`/`explain`. | | `AGENTSYNC_ALLOW_INSECURE_URLS=1` | Accept `http://`/`git://` plugin/marketplace sources. | | `AGENTSYNC_ALLOW_OFFLINE_VERIFY=1` | Let `check` validate reference *shape* only, skipping resolution (CI without an age key). | | `AGENTSYNC_NO_UPGRADE_NOTICE=1` | Never show the one-time first-run-after-upgrade notice. | diff --git a/internal/cli/destread.go b/internal/cli/destread.go index 2066c0ce..2d3a0329 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -114,15 +114,15 @@ type symlinkRefusal int const ( symlinkNone symlinkRefusal = iota // not a symlink, or resolved through it symlinkRefusedByEnv // switch unset: apply would not write through it either - symlinkUnresolvable // opted in, but the link does not resolve (dangling, loop): apply fails on it too + symlinkUnresolvable // opted in, but the link does not resolve (dangling, loop, unreadable): apply fails on it too ) // destReadPath applies the destination SYMLINK policy and answers the path the // whole-file destination reads should actually look at. why is symlinkNone when // resolved can be read; otherwise each caller answers its own "cannot read" -// value (hashFile a sentinel per reason, destModePerm (0, false), readDestText -// ""), and the two reasons are kept apart so the advice a user sees is right: -// "set the switch" is wrong advice for a link that would not resolve anyway. +// value. The two refusals stay apart so that, once a user has opted in, a link +// that still cannot be read is reported as broken rather than as "set the +// switch". // // It mirrors iox.resolveSymlinkDest, the write side, through the shared // iox.SymlinkDestAllowed, so the read side and apply cannot disagree about @@ -140,6 +140,14 @@ func destReadPath(path string) (resolved string, why symlinkRefusal) { if err != nil || fi.Mode()&os.ModeSymlink == 0 { return path, symlinkNone } + // A link to a PRESENT non-regular target (FIFO, directory, device) is a + // shape problem whatever the switch says — opting in would only resolve + // to the shape refusal — so leave it to readDestBytes, and every surface + // names the shape rather than the link. A loop or a dangling link fails + // this Stat and stays a symlink refusal. + if ti, serr := os.Stat(path); serr == nil && !ti.Mode().IsRegular() { + return path, symlinkNone + } if !iox.SymlinkDestAllowed() { return "", symlinkRefusedByEnv } diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index b840816d..9332b932 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -6,6 +6,7 @@ import ( "errors" "os" "path/filepath" + "strconv" "strings" "syscall" "testing" @@ -316,7 +317,42 @@ func TestHashFileSentinels(t *testing.T) { } return link }, - want: symlinkUnresolvableSentinel, + want: "symlink-target-unresolvable", + }, + { + // A loop fails EvalSymlinks like a dangling link does; it must not + // slip into the shape gate (Stat fails with ELOOP, not "present"). + name: "a symlink loop is unresolvable when the env is set", + setup: func(t *testing.T, tmp string) string { + t.Helper() + t.Setenv(iox.AllowSymlinkDestEnv, "1") + loop := filepath.Join(tmp, "loop") + if err := os.Symlink("loop", loop); err != nil { + t.Fatal(err) + } + return loop + }, + want: "symlink-target-unresolvable", + }, + { + // A link to a present non-regular target is a SHAPE problem whatever + // the switch says: every surface names the FIFO, not the link, so + // nobody is told to set a switch that would only reveal the FIFO. + name: "a symlink to a FIFO is refused by shape even when the env is unset", + setup: func(t *testing.T, tmp string) string { + t.Helper() + t.Setenv(iox.AllowSymlinkDestEnv, "") + if err := os.Unsetenv(iox.AllowSymlinkDestEnv); err != nil { + t.Fatal(err) + } + fifo := mkfifoDest(t, tmp) + link := filepath.Join(tmp, "link") + if err := os.Symlink(fifo, link); err != nil { + t.Fatal(err) + } + return link + }, + want: "not-a-regular-file", }, { name: "a FIFO is refused by shape", @@ -415,7 +451,7 @@ func TestHashFileSentinels(t *testing.T) { // refused as a symlink reaches the prompt as drift; one keystroke later [w] // must not read THROUGH the link and capture a file the classification never // looked at. With the env unset the write-back refuses, names the switch and -// says it is needed for every command; with it set the write-back captures +// says apply and every drift command need it; with it set the write-back captures // the linked file — so the gate follows the policy rather than always // refusing. func TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough(t *testing.T) { @@ -445,7 +481,7 @@ func TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough(t *testing.T) { t.Fatal("writeBackFileItem = nil for a refused symlink, want an error: [w] must not " + "capture through a link the classification did not read through") } - for _, want := range []string{link, iox.AllowSymlinkDestEnv + "=1", "for every command", "[i]gnore"} { + for _, want := range []string{link, iox.AllowSymlinkDestEnv + "=1", "for apply and every drift command", "[i]gnore"} { if !strings.Contains(err.Error(), want) { t.Errorf("error = %q, want it to contain %q", err, want) } @@ -499,6 +535,35 @@ func TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough(t *testing.T) { } }) + for _, env := range []string{"", "1"} { + t.Run("a symlink loop is refused without [o]verride, env="+strconv.Quote(env), func(t *testing.T) { + t.Setenv(iox.AllowSymlinkDestEnv, env) + if env == "" { + if err := os.Unsetenv(iox.AllowSymlinkDestEnv); err != nil { + t.Fatal(err) + } + } + tmp := t.TempDir() + loop := filepath.Join(tmp, "loop.md") + if err := os.Symlink("loop.md", loop); err != nil { + t.Fatal(err) + } + err := writeBackFileItem(t.TempDir(), reconcileItem{op: adapter.FileOp{Path: loop, SourceID: "demo"}}) + if err == nil { + t.Fatal("writeBackFileItem = nil for a symlink loop") + } + if strings.Contains(err.Error(), "[o]verride") || strings.Contains(err.Error(), "cannot stat") { + t.Errorf("error = %q: a loop must take a symlink arm (no [o]verride), not the generic one", err) + } + if env == "1" && !strings.Contains(err.Error(), "cannot be resolved") { + t.Errorf("error = %q, want the unresolvable-link refusal once opted in", err) + } + if env == "" && !strings.Contains(err.Error(), iox.AllowSymlinkDestEnv+"=1") { + t.Errorf("error = %q, want the switch named while it is unset", err) + } + }) + } + t.Run("env set: captures the linked file", func(t *testing.T) { t.Setenv(iox.AllowSymlinkDestEnv, "1") home := t.TempDir() diff --git a/internal/cli/diff.go b/internal/cli/diff.go index d7219e69..4329f4cf 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -229,16 +229,16 @@ func modeHunk(it planItem) (source, dest string, ok bool) { fmt.Sprintf("mode %04o", os.FileMode(it.destPerm).Perm()), true } -// symlinkHunkDest is the Dest of a "symlink" hunk. A CONSTANT, deliberately: +// symlinkRefusedHunkDest is the Dest of a "symlink" hunk. A CONSTANT, deliberately: // the link TARGET is attacker-choosable and this string reaches the terminal // unsanitized (only the hunk label goes through ui.Sanitize), so embedding it // would reopen the #93/#171 escape-injection class. -const symlinkHunkDest = "symlink (not compared through; set " + iox.AllowSymlinkDestEnv + +const symlinkRefusedHunkDest = "symlink (not compared through; set " + iox.AllowSymlinkDestEnv + "=1 to read and write through the link)" // symlinkUnresolvableHunkDest is its sibling for a link the user opted into // that does not resolve (dangling, loop); apply fails on it the same way. -const symlinkUnresolvableHunkDest = "symlink (target cannot be resolved: dangling or loop; apply refuses it too)" +const symlinkUnresolvableHunkDest = "symlink (target cannot be resolved: dangling, loop, or unreadable; apply refuses it too)" // symlinkHunk describes a destination that is a symlink the read side will not // look through — refused by AGENTSYNC_ALLOW_SYMLINK_DEST being unset, the same @@ -253,7 +253,7 @@ func symlinkHunk(it planItem) (source, dest string, ok bool) { if it.hdest == symlinkUnresolvableSentinel { return "regular file", symlinkUnresolvableHunkDest, true } - return "regular file", symlinkHunkDest, true + return "regular file", symlinkRefusedHunkDest, true } func marshalPretty(v any) string { diff --git a/internal/cli/planwalk.go b/internal/cli/planwalk.go index b70e015c..418698f6 100644 --- a/internal/cli/planwalk.go +++ b/internal/cli/planwalk.go @@ -100,12 +100,12 @@ func (i planItem) classWithModeDrift() drift.Class { } // destSymlinkRefused reports whether a whole-file destination is a symlink the -// read side did not look through (destReadPath, AGENTSYNC_ALLOW_SYMLINK_DEST -// unset). It is a DERIVATION from hdest, not a field: diff keys its symlink +// read side did not look through (destReadPath: the switch unset, or the link +// unresolvable once opted in). It is a DERIVATION from hdest, not a field: diff keys its symlink // hunk on the very hash status, reconcile and explain classified from, so the // four surfaces cannot disagree about it (#229 axis 9). func (i planItem) destSymlinkRefused() bool { - return i.ptr == "" && (i.hdest == symlinkSentinel || i.hdest == symlinkUnresolvableSentinel) + return i.ptr == "" && (i.hdest == symlinkRefusedSentinel || i.hdest == symlinkUnresolvableSentinel) } // destModePerm answers the permission bits of the REGULAR file at path. diff --git a/internal/cli/planwalk_internal_test.go b/internal/cli/planwalk_internal_test.go index 545d89d6..0f0bb0c2 100644 --- a/internal/cli/planwalk_internal_test.go +++ b/internal/cli/planwalk_internal_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "reflect" "sort" + "strings" "testing" "github.com/spf13/afero" @@ -271,6 +272,32 @@ func TestWalkPlanItems(t *testing.T) { } }, }, + { + // The unresolvable sentinel must reach diff as its own hunk: a user + // who already set the switch is told to fix the link, not to set it. + name: "unresolvable-link-gets-its-own-diff-hunk", + run: func(t *testing.T, h string) { + t.Setenv(iox.AllowSymlinkDestEnv, "1") + link, target := dest(h, "link.md"), filepath.Join(h, "gone") + if err := os.MkdirAll(filepath.Dir(link), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + hunks, _ := collectDiffHunks(planFor(map[string][]adapter.FileOp{"claude": {fileOp(link, "SOURCE")}}), []string{"claude"}, "", nil) + if len(hunks) != 1 || hunks[0].Pointer != "symlink" { + t.Fatalf("want one symlink hunk for an unresolvable link, got %+v", hunks) + } + d := hunks[0].Dest + if !strings.Contains(d, "cannot be resolved") || strings.Contains(d, iox.AllowSymlinkDestEnv) { + t.Errorf("Dest = %q: must say the link is broken and must not advise the switch", d) + } + if strings.Contains(d, target) || strings.Contains(d, "/") { + t.Errorf("Dest = %q embeds a path; it must be a constant", d) + } + }, + }, { name: "action-not-write-is-skipped", run: func(t *testing.T, h string) { @@ -602,3 +629,22 @@ func TestPathFilterFlagsSurviveAZeroItemOp(t *testing.T) { model.Unmanaged, model.pathManaged) } } + +// TestShortValShowsSentinelsWhole pins reconcile's prompt display: a digest is +// abbreviated, a sentinel is not — "symlink-not-regu..." told a user nothing. +func TestShortValShowsSentinelsWhole(t *testing.T) { + digest := strings.Repeat("ab", 32) + for _, tc := range []struct{ name, in, want string }{ + {name: "absent", in: "", want: ""}, + {name: "digest is abbreviated", in: digest, want: digest[:16] + "..."}, + {name: "symlink sentinel shown whole", in: symlinkRefusedSentinel, want: "symlink-not-regular-file"}, + {name: "unresolvable sentinel shown whole", in: "symlink-target-unresolvable", want: "symlink-target-unresolvable"}, + {name: "shape sentinel shown whole", in: "not-a-regular-file", want: "not-a-regular-file"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := shortVal(tc.in); got != tc.want { + t.Errorf("shortVal(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index 495fae6d..b5e71285 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -731,7 +731,7 @@ func shortVal(hash string) string { if len(hash) > 16 && isHexDigest(hash) { return hash[:16] + "..." } - return hash // a sentinel ("symlink-not-regular-file") is shown whole + return hash // a sentinel (the symlink and shape tokens) is shown whole } func isHexDigest(s string) bool { @@ -1106,12 +1106,6 @@ func writeBackKeyItem(cmd *cobra.Command, home string, it reconcileItem) error { // Both used to return nil with a success message, hiding data loss. func writeBackFileItem(home string, it reconcileItem) error { readPath, why := destReadPath(it.op.Path) - if why != symlinkNone && !render.IsRegularOrAbsent(it.op.Path) { - // A link to a FIFO or device is a SHAPE problem first (IsRegularOrAbsent - // Stats, so it follows the link): take the shape arm below, which is - // the arm that must never suggest [o]verride. - why, readPath = symlinkNone, it.op.Path - } switch why { case symlinkRefusedByEnv: // The drift walk classified this item without reading through the @@ -1120,12 +1114,12 @@ func writeBackFileItem(home string, it reconcileItem) error { // follows the link, and its mode arm chmods the TARGET through it // before the symlink policy is consulted (#248). return fmt.Errorf("read dest %s: destination is a symlink agentsync is not reading through — "+ - "set %s=1 (for every command) to read and write through the link, replace the link with a "+ + "set %s=1 (for apply and every drift command) to read and write through the link, replace the link with a "+ "regular file, or [i]gnore to suppress this item", it.op.Path, iox.AllowSymlinkDestEnv) case symlinkUnresolvable: return fmt.Errorf("read dest %s: destination is a symlink whose target cannot be resolved "+ - "(dangling or loop; apply refuses it too) — fix or replace the link, or [i]gnore to "+ - "suppress this item", it.op.Path) + "(dangling, loop, or unreadable; apply refuses it too) — fix or replace the link, or [i]gnore "+ + "to suppress this item", it.op.Path) } data, err := readDestBytes(readPath) if err != nil { diff --git a/internal/cli/status.go b/internal/cli/status.go index abf0df01..8e8359ba 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -938,11 +938,11 @@ func stateKeyKey(userHome, agent string, sc adapter.Scope, projectRoot, path, pt return state.NewPointerKey(userHome, agent, sc.String(), projectRoot, path, ptr) } -// symlinkSentinel is hashFile's answer for a symlink this configuration does +// symlinkRefusedSentinel is hashFile's answer for a symlink this configuration does // not read through (destReadPath). Opaque: it exists only to never equal a // content hash, and diff keys its symlink hunk on the same value // (planItem.destSymlinkRefused), so the two sites must agree on it. -const symlinkSentinel = "symlink-not-regular-file" +const symlinkRefusedSentinel = "symlink-not-regular-file" // symlinkUnresolvableSentinel is hashFile's answer for a symlink the user opted // into reading through that does not resolve (dangling, loop). Equally opaque; @@ -960,7 +960,7 @@ func hashContent(b []byte) string { // the expected signal for Orphan / OrphanDrifted. A destination whose SHAPE is // wrong, or which cannot be stat'd, answers the opaque marker below instead. // -// A SYMLINK at the path answers symlinkSentinel unless +// A SYMLINK at the path answers symlinkRefusedSentinel unless // AGENTSYNC_ALLOW_SYMLINK_DEST=1 (destReadPath — the gate apply writes under). // The sentinel is a whole-file-only policy signal: a managed regular file // became a link you have not opted into. Reading through such a link and @@ -969,9 +969,10 @@ func hashContent(b []byte) string { // converges — so a chezmoi setup reports clean after a successful apply // instead of a drift no apply can clear. Opting in never lets a non-regular // target through: a link to one (`ln -s /dev/null`) resolves and then answers -// the SHAPE sentinel below; a dangling or looping link answers -// symlinkUnresolvableSentinel once opted in (unset, it is refused like any -// other link), mirroring apply's "resolve symlink" failure. +// the SHAPE sentinel below, with the switch set or unset — the switch cannot +// help there; a dangling or looping link answers symlinkUnresolvableSentinel +// once opted in (unset, it is refused like any other link), mirroring apply's +// "resolve symlink" failure. func hashFile(path string) string { p, why := destReadPath(path) switch why { @@ -979,7 +980,7 @@ func hashFile(path string) string { // The link target is deliberately NOT part of either sentinel: it is // attacker-choosable, and a sentinel must stay a stable opaque token // that never equals a content hash. - return symlinkSentinel + return symlinkRefusedSentinel case symlinkUnresolvable: return symlinkUnresolvableSentinel } @@ -998,8 +999,9 @@ func hashFile(path string) string { data, err := readDestBytes(path) if err != nil { // Both refusals map to the SAME opaque token, deliberately. These - // sentinels are never shown; they exist only to never equal a content - // hash. Before this gate existed the predicate answered false for an + // sentinels exist to never equal a content hash (reconcile's prompt + // shows one whole, via shortVal, rather than as a truncated prefix). + // Before this gate existed the predicate answered false for an // unstattable destination too, so splitting them here would move a // parent-ENOTDIR dest from ForeignCollision to New — and New is // SafeForAutoApply. A plain read failure still answers "", as it did. diff --git a/internal/cli/symlink_dest_test.go b/internal/cli/symlink_dest_test.go index 58114e9c..fc5eaa72 100644 --- a/internal/cli/symlink_dest_test.go +++ b/internal/cli/symlink_dest_test.go @@ -176,4 +176,10 @@ func TestSymlinkedDestIsDriftWhenRefused(t *testing.T) { if strings.Contains(h.Dest, target) || strings.Contains(h.Dest, "dotfiles") { t.Errorf("symlink hunk embeds the link target; it must be a constant: %+v", h) } + // The generic form of the same guard: no path of any kind, so a future + // edit cannot smuggle one in. (The unresolvable variant is pinned the same + // way, in-package, by TestWalkPlanItems/unresolvable-link-gets-its-own-diff-hunk.) + if strings.Contains(h.Dest, "/") { + t.Errorf("symlink hunk Dest contains a path separator; it must embed no path: %q", h.Dest) + } } diff --git a/website/src/content/docs/reference/environment.mdx b/website/src/content/docs/reference/environment.mdx index dbb7a9fd..a17cf63f 100644 --- a/website/src/content/docs/reference/environment.mdx +++ b/website/src/content/docs/reference/environment.mdx @@ -15,7 +15,7 @@ the rest are escape hatches you'll rarely need. | Env var | Purpose | | --- | --- | | `AGENTSYNC_HOME` | Override the `~/.agentsync/` location (absolute path). | -| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (e.g. chezmoi-managed files). Needed by every command, not only `apply`. | +| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (e.g. chezmoi-managed files). Needed by `apply` and by `status`/`diff`/`reconcile`/`explain`. | | `AGENTSYNC_ALLOW_INSECURE_URLS=1` | Accept `http://` / `git://` plugin & marketplace sources. | | `AGENTSYNC_ALLOW_OFFLINE_VERIFY=1` | Let `check` validate reference *shape* only, skipping resolution (CI without an age key). | | `AGENTSYNC_NO_UPGRADE_NOTICE=1` | Never show the one-time [upgrade notice](/reference/upgrading/). | @@ -26,7 +26,7 @@ the rest are escape hatches you'll rarely need. | --- | --- | | `AGENTSYNC_HOME` | Override `~/.agentsync/` location (absolute path). | | `AGENTSYNC_TARGET_ROOT` | Redirect `$HOME` for testing (used by the hermetic test container). | -| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (resolves the link first). Needed by every command — `status`/`diff`/`reconcile`/`explain` too — not only `apply`. | +| `AGENTSYNC_ALLOW_SYMLINK_DEST=1` | Write through symlinked destinations, and compare through them when reading (resolves the link first). Needed by `apply` and by `status`/`diff`/`reconcile`/`explain`. | | `AGENTSYNC_ALLOW_INSECURE_URLS=1` | Accept `http://` and `git://` plugin / marketplace sources. | | `AGENTSYNC_ALLOW_UNIMPLEMENTED=1` | Register an agent with no implemented adapter yet (none today — every valid agent is real). | | `AGENTSYNC_ALLOW_PLUGIN_DRIFT=1` | Bypass the plugin-cache manifest-SHA check (after hand-editing). | @@ -41,6 +41,6 @@ the rest are escape hatches you'll rarely need. MITM protection on insecure URLs, strand-protection on symlinked destinations, tamper-detection on the plugin cache. Set them deliberately and scope them narrowly (e.g. a single command invocation), not globally in your shell profile. - The exception is `AGENTSYNC_ALLOW_SYMLINK_DEST`: once you rely on it, every - command needs it, because it governs reads as well as writes. + The exception is `AGENTSYNC_ALLOW_SYMLINK_DEST`: once you rely on it, `apply` + and every drift command need it, because it governs reads as well as writes. From a804423a7aa560ec05bdbec258fd084ac929e35b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 08:21:03 +0000 Subject: [PATCH 07/11] =?UTF-8?q?fix(cli):=20close=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20give=20diff=20a=20shape=20hunk,=20finish=20the=20"e?= =?UTF-8?q?very=20command"=20retraction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 12 ++-- docs/architecture.md | 4 +- docs/capability-matrix.md | 4 +- docs/user-guide.md | 3 +- internal/cli/destread.go | 36 +++++------ internal/cli/destread_unix_internal_test.go | 60 +++++++++++++------ internal/cli/diff.go | 27 +++++++-- internal/cli/planwalk.go | 6 +- internal/cli/planwalk_internal_test.go | 4 +- internal/cli/reconcile.go | 2 +- internal/cli/status.go | 13 ++-- .../src/content/docs/help/troubleshooting.mdx | 4 +- .../content/docs/reference/environment.mdx | 5 +- 13 files changed, 115 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8150da2e..c091810a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,14 +19,16 @@ source layout, CLI surface, and state schema are stabilizing but may still chang sentinel that can never equal a content hash — so every run reported `drift` no apply could clear and `status --exit-code` failed CI forever, while `diff` read through the link and said `no diff`. The switch now governs the READ - side too — `apply`, `status`, `diff`, `reconcile` and `explain` all need it: set, all - four resolve the link and compare the file it points at; unset, all four + side too — `apply`, `status`, `diff`, `reconcile` and `explain` all need it: + set, all resolve the link and compare the file it points at; unset, all refuse the link, `diff` prints a `symlink` hunk (`--json` `pointer: "symlink"`, alongside `mode`) naming the switch, and `reconcile`'s `[w]`rite-back refuses to capture through it (and does not offer - `[o]verride`: #248). A link that does not resolve — dangling, loop — is - reported as such rather than as "set the switch", and a link to a FIFO or a - directory is reported as that shape, which the switch could not fix. + `[o]verride`: #248). Once opted in, a link that does not resolve — dangling, + loop — is reported as such rather than as "set the switch". A link to a + FIFO, device or directory is a shape problem the switch cannot fix and is + refused as a bare one is; `diff` now prints a `shape` hunk for both instead + of rendering the whole source against an "empty" destination. - **`status`'s permission check now measures the mode the next `apply` writes** ([#229](https://github.com/spxrogers/agentsync/issues/229)). It diff --git a/docs/architecture.md b/docs/architecture.md index 2cdb4fb7..a3539b9e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -911,7 +911,9 @@ diff, and its `[w]`rite-back refuses to capture through the link and withholds policy is consulted). Set, all four resolve the link and compare the file it points at, so a converged chezmoi setup reports `clean`; a link that does not resolve answers a second sentinel so the advice is "fix the link", not "set the -switch". The mirror is a policy, +switch". A link to a FIFO, device or directory is a shape problem the switch +cannot fix: it is refused as a bare one is, and `diff` prints a `shape` hunk +for both. The mirror is a policy, not a prediction: `apply` itself only fails on a symlinked destination when the content differs (its convergence read follows the link), so the unset answer means "a managed regular file became a link you have not opted into — that is diff --git a/docs/capability-matrix.md b/docs/capability-matrix.md index 8adce8d3..8801b92c 100644 --- a/docs/capability-matrix.md +++ b/docs/capability-matrix.md @@ -589,8 +589,8 @@ in the [README](../README.md#known-limits); the highlights: drift comparison (`status`/`diff`/`reconcile`/`explain` report a symlinked whole-file destination as drifted rather than reading through the link; a key-merged file such as `~/.claude.json` is read through either way); - override with `AGENTSYNC_ALLOW_SYMLINK_DEST=1`, which every command then - needs. + override with `AGENTSYNC_ALLOW_SYMLINK_DEST=1`, which `apply` and the four + drift commands (`status`, `diff`, `reconcile`, `explain`) then need. - **Planned / deferred**: Aider and Firebender (see [Breadth tier](#breadth-tier) § "Deliberate exclusions"). diff --git a/docs/user-guide.md b/docs/user-guide.md index 2fcbafbd..23ac20dd 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -1033,7 +1033,8 @@ dashboards (`status --json` is never collapsed — it carries every tracked file `pointer` field is an RFC-6901 pointer for a merged key, and one of two pseudo-pointers for a whole-file finding that is not a text difference — `mode` for a content-identical permission change, `symlink` for a symlinked -destination agentsync is not comparing through). For a +destination agentsync is not comparing through, `shape` for a destination +that is not a regular file). For a gate that should **fail the build** on drift, add `--exit-code`: `status --exit-code` / `diff --exit-code` exit `2` when drift/hunks exist and `0` when clean (exit `2` is distinct from the generic error exit `1`, and prints no extra diff --git a/internal/cli/destread.go b/internal/cli/destread.go index 2d3a0329..283eabcf 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -78,22 +78,16 @@ func pathlessStatErr(err error) error { // only when AGENTSYNC_ALLOW_SYMLINK_DEST=1 opted into it. // TestHashFileSentinels asserts both halves. // -// Callers mostly do not surface the refusal — reconcile's write-back is the -// only one that names the shape today — so it is more a diagnosis available to -// them than one the user sees. Narrowing that means changing what several -// commands print; it is catalogued on #229 rather than here. +// diff's shape hunk and reconcile's write-back name the refusal to the user; +// status and explain classify with an opaque sentinel (hashFile). // // An ABSENT path is not refused: os.ReadFile runs and its ENOENT reaches the // caller unchanged, because manufacturing a shape error for a file that is not // there would name the wrong problem. func readDestBytes(path string) ([]byte, error) { - // render.IsRegularOrAbsent stays the single authority on SHAPE, and it is - // asked FIRST so the ordinary read costs exactly one stat. It answers false - // for two different situations, though — a path that is present and the - // wrong shape, and one that cannot be stat'd at all — so the refusal path - // pays a second stat to tell them apart. Collapsing them was a real defect: - // reconcile's refusal names errDestNotRegular and told someone with a - // permission problem to "remove or replace the non-regular file". + // render.IsRegularOrAbsent stays the single authority on SHAPE, asked FIRST + // so the ordinary read costs one stat; the refusal path pays a second stat + // to tell "wrong shape" from "cannot stat" (see errDestUnstattable). if !render.IsRegularOrAbsent(path) { if _, serr := os.Stat(path); serr != nil { // Pathless, for the reason errDestNotRegular carries no path: the @@ -112,17 +106,17 @@ func readDestBytes(path string) ([]byte, error) { type symlinkRefusal int const ( - symlinkNone symlinkRefusal = iota // not a symlink, or resolved through it + symlinkNone symlinkRefusal = iota // not a symlink, resolved, or a shape for readDestBytes to refuse symlinkRefusedByEnv // switch unset: apply would not write through it either - symlinkUnresolvable // opted in, but the link does not resolve (dangling, loop, unreadable): apply fails on it too + symlinkUnresolvable // opted in, but the link does not resolve: apply fails on it too ) // destReadPath applies the destination SYMLINK policy and answers the path the -// whole-file destination reads should actually look at. why is symlinkNone when -// resolved can be read; otherwise each caller answers its own "cannot read" -// value. The two refusals stay apart so that, once a user has opted in, a link -// that still cannot be read is reported as broken rather than as "set the -// switch". +// whole-file destination reads should go on to read (readDestBytes still +// applies its shape gate there). why is symlinkNone in that case; otherwise +// each caller answers its own "cannot read" value. The two refusals stay apart +// so that, once a user has opted in, a link that still cannot be read is +// reported as broken rather than as "set the switch". // // It mirrors iox.resolveSymlinkDest, the write side, through the shared // iox.SymlinkDestAllowed, so the read side and apply cannot disagree about @@ -142,9 +136,9 @@ func destReadPath(path string) (resolved string, why symlinkRefusal) { } // A link to a PRESENT non-regular target (FIFO, directory, device) is a // shape problem whatever the switch says — opting in would only resolve - // to the shape refusal — so leave it to readDestBytes, and every surface - // names the shape rather than the link. A loop or a dangling link fails - // this Stat and stays a symlink refusal. + // to the shape refusal — so hand it to readDestBytes, which refuses it by + // shape on every surface exactly as it does a bare FIFO. A loop or a + // dangling link fails this Stat and stays a symlink refusal. if ti, serr := os.Stat(path); serr == nil && !ti.Mode().IsRegular() { return path, symlinkNone } diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index 9332b932..e1260ca9 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -319,21 +319,6 @@ func TestHashFileSentinels(t *testing.T) { }, want: "symlink-target-unresolvable", }, - { - // A loop fails EvalSymlinks like a dangling link does; it must not - // slip into the shape gate (Stat fails with ELOOP, not "present"). - name: "a symlink loop is unresolvable when the env is set", - setup: func(t *testing.T, tmp string) string { - t.Helper() - t.Setenv(iox.AllowSymlinkDestEnv, "1") - loop := filepath.Join(tmp, "loop") - if err := os.Symlink("loop", loop); err != nil { - t.Fatal(err) - } - return loop - }, - want: "symlink-target-unresolvable", - }, { // A link to a present non-regular target is a SHAPE problem whatever // the switch says: every surface names the FIFO, not the link, so @@ -451,7 +436,7 @@ func TestHashFileSentinels(t *testing.T) { // refused as a symlink reaches the prompt as drift; one keystroke later [w] // must not read THROUGH the link and capture a file the classification never // looked at. With the env unset the write-back refuses, names the switch and -// says apply and every drift command need it; with it set the write-back captures +// says apply and the four drift commands need it; with it set the write-back captures // the linked file — so the gate follows the policy rather than always // refusing. func TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough(t *testing.T) { @@ -481,7 +466,7 @@ func TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough(t *testing.T) { t.Fatal("writeBackFileItem = nil for a refused symlink, want an error: [w] must not " + "capture through a link the classification did not read through") } - for _, want := range []string{link, iox.AllowSymlinkDestEnv + "=1", "for apply and every drift command", "[i]gnore"} { + for _, want := range []string{link, iox.AllowSymlinkDestEnv + "=1", "for apply, status, diff, reconcile and explain", "[i]gnore"} { if !strings.Contains(err.Error(), want) { t.Errorf("error = %q, want it to contain %q", err, want) } @@ -624,3 +609,44 @@ func TestReadDestBytesReportsAStatFailureAsItself(t *testing.T) { "wraps it with the path and a *fs.PathError would double it", err, n) } } + +// TestDiffPrintsAShapeHunkForANonRegularDestination pins that a FIFO at a +// whole-file destination — bare, or behind a symlink whatever the switch says — +// reaches diff as a "shape" hunk rather than as the whole source rendered +// against an "empty" destination. +func TestDiffPrintsAShapeHunkForANonRegularDestination(t *testing.T) { + for _, tc := range []struct { + name string + env string + link bool + }{ + {name: "bare FIFO", env: ""}, + {name: "link to a FIFO, env unset", env: "", link: true}, + {name: "link to a FIFO, env set", env: "1", link: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(iox.AllowSymlinkDestEnv, tc.env) + if tc.env == "" { + if err := os.Unsetenv(iox.AllowSymlinkDestEnv); err != nil { + t.Fatal(err) + } + } + tmp := t.TempDir() + path := mkfifoDest(t, tmp) + if tc.link { + link := filepath.Join(tmp, "link.md") + if err := os.Symlink(path, link); err != nil { + t.Fatal(err) + } + path = link + } + hunks, _ := collectDiffHunks(planFor(map[string][]adapter.FileOp{"claude": {fileOp(path, "SOURCE")}}), []string{"claude"}, "", nil) + if len(hunks) != 1 || hunks[0].Pointer != "shape" { + t.Fatalf("want one shape hunk, got %+v", hunks) + } + if h := hunks[0]; h.Source != "regular file" || !strings.Contains(h.Dest, "not a regular file") || strings.Contains(h.Dest, "/") || strings.Contains(h.Dest, "SOURCE") { + t.Errorf("shape hunk = %+v: must name the shape, embed no path, and never render the source", h) + } + }) + } +} diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 4329f4cf..0a01187f 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -229,10 +229,10 @@ func modeHunk(it planItem) (source, dest string, ok bool) { fmt.Sprintf("mode %04o", os.FileMode(it.destPerm).Perm()), true } -// symlinkRefusedHunkDest is the Dest of a "symlink" hunk. A CONSTANT, deliberately: -// the link TARGET is attacker-choosable and this string reaches the terminal -// unsanitized (only the hunk label goes through ui.Sanitize), so embedding it -// would reopen the #93/#171 escape-injection class. +// symlinkRefusedHunkDest is the Dest of a "symlink" hunk. A CONSTANT, +// deliberately: the link TARGET is attacker-choosable and this string reaches +// the terminal unsanitized (only the hunk label goes through ui.Sanitize), so +// embedding it would reopen the #93/#171 escape-injection class. const symlinkRefusedHunkDest = "symlink (not compared through; set " + iox.AllowSymlinkDestEnv + "=1 to read and write through the link)" @@ -256,6 +256,21 @@ func symlinkHunk(it planItem) (source, dest string, ok bool) { return "regular file", symlinkRefusedHunkDest, true } +// shapeHunkDest is the Dest of a "shape" hunk; a constant for the same reason +// as the symlink ones. +const shapeHunkDest = "not a regular file (FIFO, device, socket or directory); remove or replace it" + +// shapeHunk describes a whole-file destination readDestBytes refused by shape +// — a bare FIFO, or a link to one. Its text read is "" (there is no content to +// compare), so without this hunk diff rendered the whole source as an +// insertion against an "empty" destination that is not empty at all. +func shapeHunk(it planItem) (source, dest string, ok bool) { + if it.ptr != "" || it.hdest != shapeSentinel { + return "", "", false + } + return "regular file", shapeHunkDest, true +} + func marshalPretty(v any) string { if v == nil { return "" @@ -316,6 +331,10 @@ func collectDiffHunks(plan render.RenderPlan, names []string, filterPath string, hunks = append(hunks, diffHunk{Path: it.op.Path, Pointer: "symlink", Source: src, Dest: dst}) continue } + if src, dst, ok := shapeHunk(it); ok { + hunks = append(hunks, diffHunk{Path: it.op.Path, Pointer: "shape", Source: src, Dest: dst}) + continue + } if srcStr == dstStr { // Content identical: surface a mode-only drift as a "mode" hunk. if src, dst, ok := modeHunk(it); ok { diff --git a/internal/cli/planwalk.go b/internal/cli/planwalk.go index 418698f6..f69f2ab0 100644 --- a/internal/cli/planwalk.go +++ b/internal/cli/planwalk.go @@ -101,9 +101,9 @@ func (i planItem) classWithModeDrift() drift.Class { // destSymlinkRefused reports whether a whole-file destination is a symlink the // read side did not look through (destReadPath: the switch unset, or the link -// unresolvable once opted in). It is a DERIVATION from hdest, not a field: diff keys its symlink -// hunk on the very hash status, reconcile and explain classified from, so the -// four surfaces cannot disagree about it (#229 axis 9). +// unresolvable once opted in). It is a DERIVATION from hdest, not a field: +// diff keys its symlink hunk on the very hash status, reconcile and explain +// classified from, so the four surfaces cannot disagree about it (#229 axis 9). func (i planItem) destSymlinkRefused() bool { return i.ptr == "" && (i.hdest == symlinkRefusedSentinel || i.hdest == symlinkUnresolvableSentinel) } diff --git a/internal/cli/planwalk_internal_test.go b/internal/cli/planwalk_internal_test.go index 0f0bb0c2..dff3ac45 100644 --- a/internal/cli/planwalk_internal_test.go +++ b/internal/cli/planwalk_internal_test.go @@ -638,8 +638,8 @@ func TestShortValShowsSentinelsWhole(t *testing.T) { {name: "absent", in: "", want: ""}, {name: "digest is abbreviated", in: digest, want: digest[:16] + "..."}, {name: "symlink sentinel shown whole", in: symlinkRefusedSentinel, want: "symlink-not-regular-file"}, - {name: "unresolvable sentinel shown whole", in: "symlink-target-unresolvable", want: "symlink-target-unresolvable"}, - {name: "shape sentinel shown whole", in: "not-a-regular-file", want: "not-a-regular-file"}, + {name: "unresolvable sentinel shown whole", in: symlinkUnresolvableSentinel, want: "symlink-target-unresolvable"}, + {name: "shape sentinel shown whole", in: shapeSentinel, want: "not-a-regular-file"}, } { t.Run(tc.name, func(t *testing.T) { if got := shortVal(tc.in); got != tc.want { diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index b5e71285..67a573da 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -1114,7 +1114,7 @@ func writeBackFileItem(home string, it reconcileItem) error { // follows the link, and its mode arm chmods the TARGET through it // before the symlink policy is consulted (#248). return fmt.Errorf("read dest %s: destination is a symlink agentsync is not reading through — "+ - "set %s=1 (for apply and every drift command) to read and write through the link, replace the link with a "+ + "set %s=1 (for apply, status, diff, reconcile and explain) to read and write through the link, replace the link with a "+ "regular file, or [i]gnore to suppress this item", it.op.Path, iox.AllowSymlinkDestEnv) case symlinkUnresolvable: return fmt.Errorf("read dest %s: destination is a symlink whose target cannot be resolved "+ diff --git a/internal/cli/status.go b/internal/cli/status.go index 8e8359ba..16d2e8f7 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -938,12 +938,17 @@ func stateKeyKey(userHome, agent string, sc adapter.Scope, projectRoot, path, pt return state.NewPointerKey(userHome, agent, sc.String(), projectRoot, path, ptr) } -// symlinkRefusedSentinel is hashFile's answer for a symlink this configuration does -// not read through (destReadPath). Opaque: it exists only to never equal a -// content hash, and diff keys its symlink hunk on the same value +// symlinkRefusedSentinel is hashFile's answer for a symlink this configuration +// does not read through (destReadPath). Opaque: it exists only to never equal +// a content hash, and diff keys its symlink hunk on the same value // (planItem.destSymlinkRefused), so the two sites must agree on it. const symlinkRefusedSentinel = "symlink-not-regular-file" +// shapeSentinel is hashFile's answer for a destination readDestBytes refuses: +// present and not a regular file (FIFO, device, socket, directory), or +// unstattable. diff keys its shape hunk on it. +const shapeSentinel = "not-a-regular-file" + // symlinkUnresolvableSentinel is hashFile's answer for a symlink the user opted // into reading through that does not resolve (dangling, loop). Equally opaque; // a different token only so diff and reconcile can give the right advice. @@ -1006,7 +1011,7 @@ func hashFile(path string) string { // parent-ENOTDIR dest from ForeignCollision to New — and New is // SafeForAutoApply. A plain read failure still answers "", as it did. if errors.Is(err, errDestNotRegular) || errors.Is(err, errDestUnstattable) { - return "not-a-regular-file" + return shapeSentinel } return "" } diff --git a/website/src/content/docs/help/troubleshooting.mdx b/website/src/content/docs/help/troubleshooting.mdx index f09a995d..b782f804 100644 --- a/website/src/content/docs/help/troubleshooting.mdx +++ b/website/src/content/docs/help/troubleshooting.mdx @@ -90,8 +90,8 @@ AGENTSYNC_ALLOW_SYMLINK_DEST=1 agentsync apply ``` The same switch governs how agentsync **reads** a symlinked whole-file -destination (a `CLAUDE.md`, a skill, a subagent), so it is needed for every -command, not only `apply`. Without it, `agentsync status` reports the file as +destination (a `CLAUDE.md`, a skill, a subagent), so `apply`, `status`, +`diff`, `reconcile` and `explain` all need it. Without it, `agentsync status` reports the file as `drift` on every run even right after an apply, and `agentsync diff` prints a `symlink` hunk instead of a content diff — that is the same refusal, seen from the read side. With it, `status`, `diff`, `reconcile` and `explain` resolve the diff --git a/website/src/content/docs/reference/environment.mdx b/website/src/content/docs/reference/environment.mdx index a17cf63f..5bb76521 100644 --- a/website/src/content/docs/reference/environment.mdx +++ b/website/src/content/docs/reference/environment.mdx @@ -41,6 +41,7 @@ the rest are escape hatches you'll rarely need. MITM protection on insecure URLs, strand-protection on symlinked destinations, tamper-detection on the plugin cache. Set them deliberately and scope them narrowly (e.g. a single command invocation), not globally in your shell profile. - The exception is `AGENTSYNC_ALLOW_SYMLINK_DEST`: once you rely on it, `apply` - and every drift command need it, because it governs reads as well as writes. + The exception is `AGENTSYNC_ALLOW_SYMLINK_DEST`: once you rely on it, `apply`, + `status`, `diff`, `reconcile` and `explain` all need it, because it governs + reads as well as writes. From 499f419c42c85246a57db38bd37fb068e3cb7540 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 08:35:17 +0000 Subject: [PATCH 08/11] =?UTF-8?q?fix(cli):=20close=20review=20round=204=20?= =?UTF-8?q?=E2=80=94=20word=20the=20shape=20hunk=20for=20both=20facts=20it?= =?UTF-8?q?=20covers,=20pin=20the=20pseudo-hunk=20ordering?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 5 +++-- docs/architecture.md | 4 ++-- docs/user-guide.md | 2 +- internal/cli/destread.go | 8 ++++---- internal/cli/destread_unix_internal_test.go | 20 +++++++++++-------- internal/cli/diff.go | 12 ++++++----- .../cli/planwalk_characterization_test.go | 2 +- internal/cli/planwalk_internal_test.go | 4 +++- internal/cli/status.go | 17 +++++----------- 9 files changed, 38 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c091810a..d90ed3d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,8 +20,9 @@ source layout, CLI surface, and state schema are stabilizing but may still chang no apply could clear and `status --exit-code` failed CI forever, while `diff` read through the link and said `no diff`. The switch now governs the READ side too — `apply`, `status`, `diff`, `reconcile` and `explain` all need it: - set, all resolve the link and compare the file it points at; unset, all - refuse the link, `diff` prints a `symlink` hunk (`--json` `pointer: + set, all resolve the link and compare the file it points at; unset, the four + drift commands refuse the link (`apply` fails only when the content + differs), `diff` prints a `symlink` hunk (`--json` `pointer: "symlink"`, alongside `mode`) naming the switch, and `reconcile`'s `[w]`rite-back refuses to capture through it (and does not offer `[o]verride`: #248). Once opted in, a link that does not resolve — dangling, diff --git a/docs/architecture.md b/docs/architecture.md index a3539b9e..978630b8 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -891,8 +891,8 @@ list them reproducibly. Each surface keeps its own presentation on top: `status` re-partitions whole-file rows ahead of key rows, and both `status` and `explain` fold permission drift into the class of a content-clean whole file — measured against `op.Mode`, the mode the next apply chmods to, which is the same question -`diff`'s `mode` hunk asks (`planItem.classWithModeDrift`, one method, so the -three cannot disagree). `reconcile` still ignores mode entirely; that gap is +`diff`'s `mode` hunk asks (`planItem.opModeDrifted`, one predicate behind all +three, so they cannot disagree). `reconcile` still ignores mode entirely; that gap is #245. `diff` masks and compares text, `reconcile` excludes an orphan another agent still renders, `explain` groups by owner. diff --git a/docs/user-guide.md b/docs/user-guide.md index 23ac20dd..f06d7f4e 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -1030,7 +1030,7 @@ exclusive. Every `list` accepts `ls`, and every `remove` accepts `rm`. `status the structured report instead of the formatted one, suitable for CI gates and dashboards (`status --json` is never collapsed — it carries every tracked file; `diff --json` masks the same resolved secrets the formatted diff does; its -`pointer` field is an RFC-6901 pointer for a merged key, and one of two +`pointer` field is an RFC-6901 pointer for a merged key, and one of three pseudo-pointers for a whole-file finding that is not a text difference — `mode` for a content-identical permission change, `symlink` for a symlinked destination agentsync is not comparing through, `shape` for a destination diff --git a/internal/cli/destread.go b/internal/cli/destread.go index 283eabcf..b555cd47 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -23,10 +23,10 @@ var errDestNotRegular = errors.New("not a regular file") // real errno. // // It is separate from errDestNotRegular because the two are not the same claim -// and one caller shows its sentinel to a user: reconcile's write-back refusal -// says "remove or replace the non-regular file at that path", which is false -// for a permission problem. hashFile, whose sentinels are opaque tokens -// compared only for equality, deliberately treats both alike — see its comment. +// and reconcile's write-back refusal shows it: "remove or replace the +// non-regular file at that path" is false for a permission problem. hashFile +// deliberately maps both to one token (see its comment), so diff's shape hunk, +// which keys on that token, words its advice for both. var errDestUnstattable = errors.New("cannot stat destination") // pathlessStatErr strips the redundant path from a *fs.PathError, mirroring diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index e1260ca9..8a224f10 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -454,7 +454,7 @@ func TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough(t *testing.T) { return link } - t.Run("env unset: refuses, naming the switch and that every command needs it", func(t *testing.T) { + t.Run("env unset: refuses, naming the switch and the commands that need it", func(t *testing.T) { t.Setenv(iox.AllowSymlinkDestEnv, "") // registers the restore if err := os.Unsetenv(iox.AllowSymlinkDestEnv); err != nil { t.Fatal(err) @@ -616,13 +616,17 @@ func TestReadDestBytesReportsAStatFailureAsItself(t *testing.T) { // against an "empty" destination. func TestDiffPrintsAShapeHunkForANonRegularDestination(t *testing.T) { for _, tc := range []struct { - name string - env string - link bool + name string + env string + link bool + content string }{ - {name: "bare FIFO", env: ""}, - {name: "link to a FIFO, env unset", env: "", link: true}, - {name: "link to a FIFO, env set", env: "1", link: true}, + {name: "bare FIFO", env: "", content: "SOURCE"}, + {name: "link to a FIFO, env unset", env: "", link: true, content: "SOURCE"}, + {name: "link to a FIFO, env set", env: "1", link: true, content: "SOURCE"}, + // An EMPTY rendered source equals the refused read's "" — the shape + // check must run before the text compare or this prints "no diff". + {name: "bare FIFO, empty source", env: "", content: ""}, } { t.Run(tc.name, func(t *testing.T) { t.Setenv(iox.AllowSymlinkDestEnv, tc.env) @@ -640,7 +644,7 @@ func TestDiffPrintsAShapeHunkForANonRegularDestination(t *testing.T) { } path = link } - hunks, _ := collectDiffHunks(planFor(map[string][]adapter.FileOp{"claude": {fileOp(path, "SOURCE")}}), []string{"claude"}, "", nil) + hunks, _ := collectDiffHunks(planFor(map[string][]adapter.FileOp{"claude": {fileOp(path, tc.content)}}), []string{"claude"}, "", nil) if len(hunks) != 1 || hunks[0].Pointer != "shape" { t.Fatalf("want one shape hunk, got %+v", hunks) } diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 0a01187f..67850728 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -258,12 +258,14 @@ func symlinkHunk(it planItem) (source, dest string, ok bool) { // shapeHunkDest is the Dest of a "shape" hunk; a constant for the same reason // as the symlink ones. -const shapeHunkDest = "not a regular file (FIFO, device, socket or directory); remove or replace it" +const shapeHunkDest = "not a regular file (FIFO, device, socket or directory), or the path cannot be " + + "stat'd; remove or replace it, or check permissions on the path" -// shapeHunk describes a whole-file destination readDestBytes refused by shape -// — a bare FIFO, or a link to one. Its text read is "" (there is no content to -// compare), so without this hunk diff rendered the whole source as an -// insertion against an "empty" destination that is not empty at all. +// shapeHunk describes a whole-file destination readDestBytes refused — a bare +// FIFO, a link to one, or a path it cannot stat; hashFile answers one token +// for both facts, so the Dest names both. Its text read is "" (there is no +// content to compare), so without this hunk diff rendered the whole source as +// an insertion against an "empty" destination that is not empty at all. func shapeHunk(it planItem) (source, dest string, ok bool) { if it.ptr != "" || it.hdest != shapeSentinel { return "", "", false diff --git a/internal/cli/planwalk_characterization_test.go b/internal/cli/planwalk_characterization_test.go index 05b507fb..ca01a261 100644 --- a/internal/cli/planwalk_characterization_test.go +++ b/internal/cli/planwalk_characterization_test.go @@ -1023,7 +1023,7 @@ func projectD(hunks []diffHunk, matched bool) dProj { // pointer; keep both out of the run sort so their placement stays asserted // in order. hunks = normalizeRuns(hunks, func(h diffHunk) (string, string, string) { - if h.Pointer == "mode" || h.Pointer == "symlink" { + if h.Pointer == "mode" || h.Pointer == "symlink" || h.Pointer == "shape" { return "", h.Path, "" } return "", h.Path, h.Pointer diff --git a/internal/cli/planwalk_internal_test.go b/internal/cli/planwalk_internal_test.go index dff3ac45..fd4ff285 100644 --- a/internal/cli/planwalk_internal_test.go +++ b/internal/cli/planwalk_internal_test.go @@ -285,7 +285,9 @@ func TestWalkPlanItems(t *testing.T) { if err := os.Symlink(target, link); err != nil { t.Fatal(err) } - hunks, _ := collectDiffHunks(planFor(map[string][]adapter.FileOp{"claude": {fileOp(link, "SOURCE")}}), []string{"claude"}, "", nil) + // An EMPTY source equals the refused read's "": the symlink check + // must run before the text compare or this prints "no diff". + hunks, _ := collectDiffHunks(planFor(map[string][]adapter.FileOp{"claude": {fileOp(link, "")}}), []string{"claude"}, "", nil) if len(hunks) != 1 || hunks[0].Pointer != "symlink" { t.Fatalf("want one symlink hunk for an unresolvable link, got %+v", hunks) } diff --git a/internal/cli/status.go b/internal/cli/status.go index 16d2e8f7..ee3a8c81 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -990,23 +990,16 @@ func hashFile(path string) string { return symlinkUnresolvableSentinel } path = p - // A FIFO, device, or socket at a destination path would make os.ReadFile - // BLOCK forever rather than fail — wedging `status`, which is advertised as - // read-only, and reconcile's orphan listing. None has a content hash worth - // computing, so answer a sentinel that can never match one. It is a DIFFERENT - // sentinel from the symlink case above so a diagnostic never calls a FIFO a - // symlink; both are opaque to callers, which only ever compare hashes for - // equality. - // // The shape rule itself lives in readDestBytes, the one gate every // destination read in this package passes through; this function maps its - // refusal onto the sentinel above rather than re-deciding it. + // refusal onto a sentinel — distinct from the symlink ones, so a diagnostic + // never calls a FIFO a symlink — rather than re-deciding it. data, err := readDestBytes(path) if err != nil { // Both refusals map to the SAME opaque token, deliberately. These - // sentinels exist to never equal a content hash (reconcile's prompt - // shows one whole, via shortVal, rather than as a truncated prefix). - // Before this gate existed the predicate answered false for an + // sentinels exist to never equal a content hash; the two surfaces that + // show one (reconcile's prompt via shortVal, diff's shape hunk) word it + // for both facts. Before this gate existed the predicate answered false for an // unstattable destination too, so splitting them here would move a // parent-ENOTDIR dest from ForeignCollision to New — and New is // SafeForAutoApply. A plain read failure still answers "", as it did. From 91b1490fe0dbc9d11e8a0eee9efd112414dcc220 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 08:50:32 +0000 Subject: [PATCH 09/11] =?UTF-8?q?fix(cli):=20close=20review=20round=205=20?= =?UTF-8?q?=E2=80=94=20print=20pseudo-hunk=20labels=20whole,=20use=20the?= =?UTF-8?q?=20SHA=20display=20for=20a=20shape-refused=20item?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 2 +- docs/architecture.md | 8 ++--- docs/user-guide.md | 2 +- internal/cli/destread_unix_internal_test.go | 39 ++++++++++++++++++++- internal/cli/diff.go | 11 ++++++ internal/cli/planwalk.go | 4 +-- internal/cli/reconcile.go | 11 +++--- internal/cli/status.go | 6 ++-- internal/cli/symlink_dest_test.go | 9 +++++ 9 files changed, 75 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d90ed3d0..4b2a5f4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,7 @@ source layout, CLI surface, and state schema are stabilizing but may still chang drift commands refuse the link (`apply` fails only when the content differs), `diff` prints a `symlink` hunk (`--json` `pointer: "symlink"`, alongside `mode`) naming the switch, and `reconcile`'s - `[w]`rite-back refuses to capture through it (and does not offer + `[w]`rite-back refuses to capture through it (with advice that omits `[o]verride`: #248). Once opted in, a link that does not resolve — dangling, loop — is reported as such rather than as "set the switch". A link to a FIFO, device or directory is a shape problem the switch cannot fix and is diff --git a/docs/architecture.md b/docs/architecture.md index 978630b8..01c76f2f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -906,14 +906,14 @@ sentinel that can never equal a content hash, so the classifier sees a changed destination (`drift`, or `conflict`/`foreign-collision` by its usual table); `diff` prints a `symlink` hunk naming the switch rather than reading through and reporting no difference; `reconcile` shows the SHA display instead of a text -diff, and its `[w]`rite-back refuses to capture through the link and withholds -`[o]verride` (#248: `Writer.Write`'s mode arm chmods through a link before the -policy is consulted). Set, all four resolve the link and compare the file it +diff, and its `[w]`rite-back refuses to capture through the link with advice +that omits `[o]verride` (#248: `Writer.Write`'s mode arm chmods through a link +before the policy is consulted). Set, all four resolve the link and compare the file it points at, so a converged chezmoi setup reports `clean`; a link that does not resolve answers a second sentinel so the advice is "fix the link", not "set the switch". A link to a FIFO, device or directory is a shape problem the switch cannot fix: it is refused as a bare one is, and `diff` prints a `shape` hunk -for both. The mirror is a policy, +for both — as it does for a path it cannot stat, which shares the token. The mirror is a policy, not a prediction: `apply` itself only fails on a symlinked destination when the content differs (its convergence read follows the link), so the unset answer means "a managed regular file became a link you have not opted into — that is diff --git a/docs/user-guide.md b/docs/user-guide.md index f06d7f4e..0cf417f3 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -1034,7 +1034,7 @@ dashboards (`status --json` is never collapsed — it carries every tracked file pseudo-pointers for a whole-file finding that is not a text difference — `mode` for a content-identical permission change, `symlink` for a symlinked destination agentsync is not comparing through, `shape` for a destination -that is not a regular file). For a +that is not a readable regular file — wrong shape, or cannot be stat'd). For a gate that should **fail the build** on drift, add `--exit-code`: `status --exit-code` / `diff --exit-code` exit `2` when drift/hunks exist and `0` when clean (exit `2` is distinct from the generic error exit `1`, and prints no extra diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index 8a224f10..710a13e2 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -14,6 +14,7 @@ import ( "github.com/spxrogers/agentsync/internal/adapter" "github.com/spxrogers/agentsync/internal/iox" + "github.com/spxrogers/agentsync/internal/state" ) // TestReadDestBytesShape pins the gate every destination read now passes @@ -391,7 +392,7 @@ func TestHashFileSentinels(t *testing.T) { case h := <-got: if h != tc.want { t.Errorf("hashFile = %q, want %q — these sentinels are compared only for "+ - "equality, so changing one silently changes drift.Classify's verdict", + "equality — and diff now keys its shape hunk's prose on the shape one", h, tc.want) } case <-time.After(5 * time.Second): @@ -654,3 +655,39 @@ func TestDiffPrintsAShapeHunkForANonRegularDestination(t *testing.T) { }) } } + +// TestDiffPrintsAShapeHunkForAnUnstattableDestination pins the other fact the +// shape token carries: a path under a regular-file parent cannot be stat'd, +// reaches diff as the same hunk, and the hedged Dest says so. +func TestDiffPrintsAShapeHunkForAnUnstattableDestination(t *testing.T) { + tmp := t.TempDir() + blocker := filepath.Join(tmp, "notadir") + if err := os.WriteFile(blocker, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + path := filepath.Join(blocker, "dest.md") + hunks, _ := collectDiffHunks(planFor(map[string][]adapter.FileOp{"claude": {fileOp(path, "SOURCE")}}), []string{"claude"}, "", nil) + if len(hunks) != 1 || hunks[0].Pointer != "shape" { + t.Fatalf("want one shape hunk for an unstattable destination, got %+v", hunks) + } + if d := hunks[0].Dest; !strings.Contains(d, "stat") || strings.Contains(d, "/") { + t.Errorf("Dest = %q: must say the path cannot be stat'd and embed no path", d) + } +} + +// TestReconcileUsesTheSHADisplayForAShapeRefusedItem pins reconcile's prompt +// half of the same rule diff's shape hunk enforces: a FIFO at a destination +// has no text to show, so the item falls back to the SHA display instead of +// rendering the whole source as an insertion against an "empty" destination. +func TestReconcileUsesTheSHADisplayForAShapeRefusedItem(t *testing.T) { + tmp := t.TempDir() + fifo := mkfifoDest(t, tmp) + items, _ := collectReconcileItems(planFor(map[string][]adapter.FileOp{"claude": {fileOp(fifo, "SOURCE")}}), + registryFactory(), state.New(), adapter.ScopeUser, "", tmp, nil) + if len(items) != 1 || items[0].hdest != shapeSentinel { + t.Fatalf("want one shape-refused item, got %+v", items) + } + if items[0].hasText { + t.Error("hasText = true for a shape-refused item; the prompt would render the source against an empty destination") + } +} diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 67850728..6731ddef 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -166,6 +166,13 @@ func newDiffCmd() *cobra.Command { fmt.Fprintf(p.Out, "%s %s\n", p.Red("--- source"), label) fmt.Fprintf(p.Out, "%s %s\n", p.Green("+++ dest "), label) diffs := dmp.DiffMain(h.Dest, h.Source, false) + if isPseudoPointer(h.Pointer) { + // Two short labels, not two texts: a character diff + // shreds "symlink (not compared through; …)" against + // "regular file" into fragments. Semantic cleanup + // re-joins each side whole. + diffs = dmp.DiffCleanupSemantic(diffs) + } fmt.Fprintln(p.Out, renderDiffText(p, diffs)) } } @@ -256,6 +263,10 @@ func symlinkHunk(it planItem) (source, dest string, ok bool) { return "regular file", symlinkRefusedHunkDest, true } +// isPseudoPointer reports whether a hunk's Pointer is one of diff's whole-file +// labels rather than an RFC-6901 pointer. +func isPseudoPointer(p string) bool { return p == "mode" || p == "symlink" || p == "shape" } + // shapeHunkDest is the Dest of a "shape" hunk; a constant for the same reason // as the symlink ones. const shapeHunkDest = "not a regular file (FIFO, device, socket or directory), or the path cannot be " + diff --git a/internal/cli/planwalk.go b/internal/cli/planwalk.go index f69f2ab0..86732d6b 100644 --- a/internal/cli/planwalk.go +++ b/internal/cli/planwalk.go @@ -49,8 +49,8 @@ type planItem struct { cls drift.Class // The triple cls was computed from. hdest is "" for absent-or-unreadable, - // and one of two opaque sentinels for a refused-symlink or wrong-shaped - // destination — see hashFile, whose semantics this reproduces exactly. + // and one of three opaque sentinels for a refused symlink, an unresolvable + // one, or a refused shape — see hashFile, whose semantics this reproduces. hsrc, happlied, hdest string // Whole-file mode facts, from destModePerm: destRegular is false for an diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index 67a573da..c53c5329 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -668,11 +668,12 @@ func collectReconcileItems(plan render.RenderPlan, reg *adapter.Registry, s *sta orphans = append(orphans, ri) continue } - // A refused whole-file symlink has no destination text to show: fall - // back to the SHA display rather than render the entire source as an - // insertion against an empty destination (the rendering diff's symlink - // hunk exists to avoid). - ri.srcText, ri.dstText, ri.hasText = it.srcText, it.dstText, !it.destSymlinkRefused() + // A refused whole-file destination — a symlink not read through, or a + // shape readDestBytes refused — has no text to show: fall back to the + // SHA display rather than render the entire source as an insertion + // against an "empty" destination (the rendering diff's symlink and + // shape hunks exist to avoid). + ri.srcText, ri.dstText, ri.hasText = it.srcText, it.dstText, !it.destSymlinkRefused() && it.hdest != shapeSentinel if it.ptr != "" { ri.pluginOwner = pluginOwnerForKeyItem(it.op.SourceID, it.ptr, pluginOwners) } else { diff --git a/internal/cli/status.go b/internal/cli/status.go index ee3a8c81..f0a12ab4 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -997,9 +997,9 @@ func hashFile(path string) string { data, err := readDestBytes(path) if err != nil { // Both refusals map to the SAME opaque token, deliberately. These - // sentinels exist to never equal a content hash; the two surfaces that - // show one (reconcile's prompt via shortVal, diff's shape hunk) word it - // for both facts. Before this gate existed the predicate answered false for an + // sentinels exist to never equal a content hash; diff's shape hunk, + // which keys prose on this one, words it for both facts (reconcile's + // prompt shows the token itself). Before this gate existed the predicate answered false for an // unstattable destination too, so splitting them here would move a // parent-ENOTDIR dest from ForeignCollision to New — and New is // SafeForAutoApply. A plain read failure still answers "", as it did. diff --git a/internal/cli/symlink_dest_test.go b/internal/cli/symlink_dest_test.go index fc5eaa72..34f5dd43 100644 --- a/internal/cli/symlink_dest_test.go +++ b/internal/cli/symlink_dest_test.go @@ -182,4 +182,13 @@ func TestSymlinkedDestIsDriftWhenRefused(t *testing.T) { if strings.Contains(h.Dest, "/") { t.Errorf("symlink hunk Dest contains a path separator; it must embed no path: %q", h.Dest) } + // The formatted diff must print the two labels WHOLE: a character diff of + // "symlink (…)" against "regular file" shreds both into fragments. + text, err := runCLI(t, env, "diff") + if err != nil { + t.Fatalf("diff: %v\n%s", err, text) + } + if !strings.Contains(text, "symlink (not compared through") || !strings.Contains(text, "regular file") { + t.Errorf("formatted diff must print the symlink hunk's labels whole; got:\n%s", text) + } } From 64b923cdb08c47b01a6863938c951758cf792c05 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 10:44:49 +0000 Subject: [PATCH 10/11] =?UTF-8?q?fix(cli):=20close=20review=20round=206=20?= =?UTF-8?q?=E2=80=94=20emit=20label=20hunks=20whole=20instead=20of=20clean?= =?UTF-8?q?ing=20up=20a=20character=20diff?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- docs/architecture.md | 14 +++--- docs/user-guide.md | 2 +- internal/cli/destread_unix_internal_test.go | 4 +- internal/cli/diff.go | 46 ++++++++++++------- internal/cli/planwalk.go | 5 ++ .../cli/planwalk_characterization_test.go | 2 +- internal/cli/planwalk_internal_test.go | 25 ++++++++++ internal/cli/reconcile.go | 25 +++++----- internal/cli/status.go | 5 +- 9 files changed, 85 insertions(+), 43 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 01c76f2f..f0a04c3b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -908,13 +908,13 @@ destination (`drift`, or `conflict`/`foreign-collision` by its usual table); reporting no difference; `reconcile` shows the SHA display instead of a text diff, and its `[w]`rite-back refuses to capture through the link with advice that omits `[o]verride` (#248: `Writer.Write`'s mode arm chmods through a link -before the policy is consulted). Set, all four resolve the link and compare the file it -points at, so a converged chezmoi setup reports `clean`; a link that does not -resolve answers a second sentinel so the advice is "fix the link", not "set the -switch". A link to a FIFO, device or directory is a shape problem the switch -cannot fix: it is refused as a bare one is, and `diff` prints a `shape` hunk -for both — as it does for a path it cannot stat, which shares the token. The mirror is a policy, -not a prediction: `apply` itself only fails on a symlinked destination when the +before the policy is consulted). Set, all four resolve the link and compare the +file it points at, so a converged chezmoi setup reports `clean`; a link that +does not resolve answers a second sentinel so the advice is "fix the link", not +"set the switch". A link to a FIFO, device or directory is a shape problem the +switch cannot fix: it is refused as a bare one is, and `diff` prints a `shape` +hunk for both — as it does for a path it cannot stat, which shares the token. +The mirror is a policy, not a prediction: `apply` itself only fails on a symlinked destination when the content differs (its convergence read follows the link), so the unset answer means "a managed regular file became a link you have not opted into — that is drift", not "the next apply would fail". The rule covers **whole-file** diff --git a/docs/user-guide.md b/docs/user-guide.md index 0cf417f3..0428885a 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -1034,7 +1034,7 @@ dashboards (`status --json` is never collapsed — it carries every tracked file pseudo-pointers for a whole-file finding that is not a text difference — `mode` for a content-identical permission change, `symlink` for a symlinked destination agentsync is not comparing through, `shape` for a destination -that is not a readable regular file — wrong shape, or cannot be stat'd). For a +that is not a regular file, or cannot be stat'd). For a gate that should **fail the build** on drift, add `--exit-code`: `status --exit-code` / `diff --exit-code` exit `2` when drift/hunks exist and `0` when clean (exit `2` is distinct from the generic error exit `1`, and prints no extra diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index 710a13e2..8748458f 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -208,7 +208,7 @@ func TestWriteBackFileItemMessageMatchesTheFailure(t *testing.T) { if !strings.Contains(err.Error(), "[i]gnore") { t.Errorf("error = %q, want it to still name a next step", err) } - // [o]verride is withheld only for a NON-REGULAR destination, where it would + // [o]verride is left out of the advice only for a NON-REGULAR destination, where it would // hang (#241). For an absent one it is safe — Writer.Write's convergence // read gets ENOENT and falls through to the write — and it is the actual // fix, so withholding it here would deny the user the remedy that works. @@ -485,7 +485,7 @@ func TestWriteBackFileItemRefusesASymlinkItIsNotReadingThrough(t *testing.T) { } }) - t.Run("env unset: a link to a FIFO takes the shape arm, which withholds [o]verride", func(t *testing.T) { + t.Run("env unset: a link to a FIFO takes the shape arm, whose advice omits [o]verride", func(t *testing.T) { t.Setenv(iox.AllowSymlinkDestEnv, "") if err := os.Unsetenv(iox.AllowSymlinkDestEnv); err != nil { t.Fatal(err) diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 6731ddef..94bcc763 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -165,15 +165,7 @@ func newDiffCmd() *cobra.Command { label = ui.Sanitize(label) fmt.Fprintf(p.Out, "%s %s\n", p.Red("--- source"), label) fmt.Fprintf(p.Out, "%s %s\n", p.Green("+++ dest "), label) - diffs := dmp.DiffMain(h.Dest, h.Source, false) - if isPseudoPointer(h.Pointer) { - // Two short labels, not two texts: a character diff - // shreds "symlink (not compared through; …)" against - // "regular file" into fragments. Semantic cleanup - // re-joins each side whole. - diffs = dmp.DiffCleanupSemantic(diffs) - } - fmt.Fprintln(p.Out, renderDiffText(p, diffs)) + fmt.Fprintln(p.Out, renderDiffText(p, hunkDiffs(dmp, h))) } } // --exit-code turns diff into a CI gate: non-zero (stable) when any @@ -263,9 +255,31 @@ func symlinkHunk(it planItem) (source, dest string, ok bool) { return "regular file", symlinkRefusedHunkDest, true } -// isPseudoPointer reports whether a hunk's Pointer is one of diff's whole-file -// labels rather than an RFC-6901 pointer. -func isPseudoPointer(p string) bool { return p == "mode" || p == "symlink" || p == "shape" } +// The pseudo-pointers: a whole-file finding that is not a text difference +// carries one of these in diffHunk.Pointer instead of an RFC-6901 pointer. +const ( + ptrMode = "mode" + ptrSymlink = "symlink" + ptrShape = "shape" +) + +// isPseudoPointer reports whether a hunk's Pointer is one of the labels above. +func isPseudoPointer(p string) bool { return p == ptrMode || p == ptrSymlink || p == ptrShape } + +// hunkDiffs is what the formatted diff renders for one hunk. A symlink or +// shape hunk carries two UNRELATED labels ("regular file" against a sentence), +// and a character diff of those shreds both into fragments — so each side is +// emitted whole. A mode hunk's two sides share their shape ("mode 0644" / +// "mode 0755"), where the character diff reads well, and text hunks are text. +func hunkDiffs(dmp *diffmatchpatch.DiffMatchPatch, h diffHunk) []diffmatchpatch.Diff { + if h.Pointer == ptrSymlink || h.Pointer == ptrShape { + return []diffmatchpatch.Diff{ + {Type: diffmatchpatch.DiffDelete, Text: h.Dest}, + {Type: diffmatchpatch.DiffInsert, Text: h.Source}, + } + } + return dmp.DiffMain(h.Dest, h.Source, false) +} // shapeHunkDest is the Dest of a "shape" hunk; a constant for the same reason // as the symlink ones. @@ -278,7 +292,7 @@ const shapeHunkDest = "not a regular file (FIFO, device, socket or directory), o // content to compare), so without this hunk diff rendered the whole source as // an insertion against an "empty" destination that is not empty at all. func shapeHunk(it planItem) (source, dest string, ok bool) { - if it.ptr != "" || it.hdest != shapeSentinel { + if !it.destShapeRefused() { return "", "", false } return "regular file", shapeHunkDest, true @@ -341,17 +355,17 @@ func collectDiffHunks(plan render.RenderPlan, names []string, filterPath string, // a refused link's dstText is "", and an empty op.Content must not fall // through to "equal" and then into the mode branch. if src, dst, ok := symlinkHunk(it); ok { - hunks = append(hunks, diffHunk{Path: it.op.Path, Pointer: "symlink", Source: src, Dest: dst}) + hunks = append(hunks, diffHunk{Path: it.op.Path, Pointer: ptrSymlink, Source: src, Dest: dst}) continue } if src, dst, ok := shapeHunk(it); ok { - hunks = append(hunks, diffHunk{Path: it.op.Path, Pointer: "shape", Source: src, Dest: dst}) + hunks = append(hunks, diffHunk{Path: it.op.Path, Pointer: ptrShape, Source: src, Dest: dst}) continue } 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}) + hunks = append(hunks, diffHunk{Path: it.op.Path, Pointer: ptrMode, Source: src, Dest: dst}) } continue } diff --git a/internal/cli/planwalk.go b/internal/cli/planwalk.go index 86732d6b..c3ae12b0 100644 --- a/internal/cli/planwalk.go +++ b/internal/cli/planwalk.go @@ -108,6 +108,11 @@ func (i planItem) destSymlinkRefused() bool { return i.ptr == "" && (i.hdest == symlinkRefusedSentinel || i.hdest == symlinkUnresolvableSentinel) } +// destShapeRefused is its sibling for a whole-file destination readDestBytes +// refused: present and not a regular file, or unstattable (hashFile's one +// token for both). +func (i planItem) destShapeRefused() bool { return i.ptr == "" && i.hdest == shapeSentinel } + // destModePerm answers the permission bits of the REGULAR file at path. // regular is false — and perm 0 — for an absent, refused-symlink // (destReadPath: a link this configuration does not look through) or diff --git a/internal/cli/planwalk_characterization_test.go b/internal/cli/planwalk_characterization_test.go index ca01a261..072bceca 100644 --- a/internal/cli/planwalk_characterization_test.go +++ b/internal/cli/planwalk_characterization_test.go @@ -1023,7 +1023,7 @@ func projectD(hunks []diffHunk, matched bool) dProj { // pointer; keep both out of the run sort so their placement stays asserted // in order. hunks = normalizeRuns(hunks, func(h diffHunk) (string, string, string) { - if h.Pointer == "mode" || h.Pointer == "symlink" || h.Pointer == "shape" { + if isPseudoPointer(h.Pointer) { return "", h.Path, "" } return "", h.Path, h.Pointer diff --git a/internal/cli/planwalk_internal_test.go b/internal/cli/planwalk_internal_test.go index fd4ff285..cde65b41 100644 --- a/internal/cli/planwalk_internal_test.go +++ b/internal/cli/planwalk_internal_test.go @@ -10,6 +10,8 @@ import ( "strings" "testing" + "github.com/sergi/go-diff/diffmatchpatch" + "github.com/spf13/afero" "github.com/spxrogers/agentsync/internal/adapter" "github.com/spxrogers/agentsync/internal/drift" @@ -650,3 +652,26 @@ func TestShortValShowsSentinelsWhole(t *testing.T) { }) } } + +// TestLabelHunksAreNotCharacterDiffed pins the formatted-diff rule for the +// pseudo-pointers: a symlink or shape hunk's two unrelated labels are emitted +// whole (a character diff shredded them), while a mode hunk keeps the +// character diff its two like-shaped sides read well under. +func TestLabelHunksAreNotCharacterDiffed(t *testing.T) { + dmp := diffmatchpatch.New() + for _, h := range []diffHunk{ + {Pointer: ptrSymlink, Source: "regular file", Dest: symlinkRefusedHunkDest}, + {Pointer: ptrSymlink, Source: "regular file", Dest: symlinkUnresolvableHunkDest}, + {Pointer: ptrShape, Source: "regular file", Dest: shapeHunkDest}, + } { + got := hunkDiffs(dmp, h) + if len(got) != 2 || got[0].Type != diffmatchpatch.DiffDelete || got[0].Text != h.Dest || + got[1].Type != diffmatchpatch.DiffInsert || got[1].Text != h.Source { + t.Errorf("%s hunk: got %+v, want [-Dest-] then {+Source+} whole", h.Pointer, got) + } + } + mode := hunkDiffs(dmp, diffHunk{Pointer: ptrMode, Source: "mode 0755", Dest: "mode 0644"}) + if len(mode) < 3 || mode[0].Type != diffmatchpatch.DiffEqual || mode[0].Text != "mode 0" { + t.Errorf("mode hunk: got %+v, want the shared \"mode 0\" prefix kept as equal text", mode) + } +} diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index c53c5329..194f35e4 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -42,10 +42,10 @@ type reconcileItem struct { orphan bool // owned-in-state whole-file dest no agent renders anymore // srcText/dstText carry the actual (masked-on-display) source and destination // content so the prompt/[d]iff can show a real value diff instead of only SHA - // prefixes. hasText is false for items with no meaningful textual content — - // orphans, and a whole-file destination that is a symlink agentsync is not - // reading through (planItem.destSymlinkRefused) — which fall back to the - // hash display. + // prefixes. hasText is false for items with no textual content — orphans, + // and a whole-file destination the walk refused to read (a symlink not + // read through, or a shape readDestBytes refuses) — which fall back to + // the hash display. srcText string dstText string hasText bool @@ -668,12 +668,9 @@ func collectReconcileItems(plan render.RenderPlan, reg *adapter.Registry, s *sta orphans = append(orphans, ri) continue } - // A refused whole-file destination — a symlink not read through, or a - // shape readDestBytes refused — has no text to show: fall back to the - // SHA display rather than render the entire source as an insertion - // against an "empty" destination (the rendering diff's symlink and - // shape hunks exist to avoid). - ri.srcText, ri.dstText, ri.hasText = it.srcText, it.dstText, !it.destSymlinkRefused() && it.hdest != shapeSentinel + // A refused destination has no text to show: the SHA display, not the + // whole source rendered against an "empty" destination. + ri.srcText, ri.dstText, ri.hasText = it.srcText, it.dstText, !it.destSymlinkRefused() && !it.destShapeRefused() if it.ptr != "" { ri.pluginOwner = pluginOwnerForKeyItem(it.op.SourceID, it.ptr, pluginOwners) } else { @@ -1110,8 +1107,8 @@ func writeBackFileItem(home string, it reconcileItem) error { switch why { case symlinkRefusedByEnv: // The drift walk classified this item without reading through the - // link, so [w] must not quietly capture through it. [o]verride is - // withheld on both symlink arms: Writer.Write's convergence read + // link, so [w] must not quietly capture through it. Neither symlink + // arm's advice suggests [o]verride: Writer.Write's convergence read // follows the link, and its mode arm chmods the TARGET through it // before the symlink policy is consulted (#248). return fmt.Errorf("read dest %s: destination is a symlink agentsync is not reading through — "+ @@ -1128,8 +1125,8 @@ func writeBackFileItem(home string, it reconcileItem) error { // mid-prompt with a keystroke to choose, and "read dest X: not a regular // file" alone does not tell them which one gets them unstuck. // - // [o]verride is deliberately NOT offered for THIS arm (nor for the - // symlink arms above), unlike the absent arm below. It re-applies + // This arm's advice deliberately omits [o]verride (so does the symlink + // arms'), unlike the absent arm below. It re-applies // through render.Writer.Write, whose convergence read is not // shape-guarded, so on this exact item it does not fail — it HANGS // (measured: `reconcile --auto-override` rc=124). diff --git a/internal/cli/status.go b/internal/cli/status.go index f0a12ab4..a260d880 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -999,8 +999,9 @@ func hashFile(path string) string { // Both refusals map to the SAME opaque token, deliberately. These // sentinels exist to never equal a content hash; diff's shape hunk, // which keys prose on this one, words it for both facts (reconcile's - // prompt shows the token itself). Before this gate existed the predicate answered false for an - // unstattable destination too, so splitting them here would move a + // prompt shows the token itself). Before this gate existed the + // predicate answered false for an unstattable destination too, so + // splitting them here would move a // parent-ENOTDIR dest from ForeignCollision to New — and New is // SafeForAutoApply. A plain read failure still answers "", as it did. if errors.Is(err, errDestNotRegular) || errors.Is(err, errDestUnstattable) { From 8e56d028d8bca534b44b8cd4e4c3e7f454e4e8e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 14:13:47 +0000 Subject: [PATCH 11/11] =?UTF-8?q?docs(cli):=20close=20review=20round=207?= =?UTF-8?q?=20=E2=80=94=20prose=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- docs/architecture.md | 5 +++-- docs/user-guide.md | 6 +++--- internal/cli/destread_unix_internal_test.go | 9 ++++---- internal/cli/diff.go | 6 ++++-- internal/cli/planwalk_internal_test.go | 23 +++++++++++++-------- internal/cli/reconcile.go | 10 ++++----- internal/cli/status.go | 6 +++--- 7 files changed, 37 insertions(+), 28 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index f0a04c3b..21395291 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -914,8 +914,9 @@ does not resolve answers a second sentinel so the advice is "fix the link", not "set the switch". A link to a FIFO, device or directory is a shape problem the switch cannot fix: it is refused as a bare one is, and `diff` prints a `shape` hunk for both — as it does for a path it cannot stat, which shares the token. -The mirror is a policy, not a prediction: `apply` itself only fails on a symlinked destination when the -content differs (its convergence read follows the link), so the unset answer +The mirror is a policy, not a prediction: `apply` itself only fails on a +symlinked destination when the content differs (its convergence read follows +the link), so the unset answer means "a managed regular file became a link you have not opted into — that is drift", not "the next apply would fail". The rule covers **whole-file** destinations only; the symlink sentinel is a whole-file-only policy signal. A diff --git a/docs/user-guide.md b/docs/user-guide.md index 0428885a..6b4dcad8 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -1033,9 +1033,9 @@ dashboards (`status --json` is never collapsed — it carries every tracked file `pointer` field is an RFC-6901 pointer for a merged key, and one of three pseudo-pointers for a whole-file finding that is not a text difference — `mode` for a content-identical permission change, `symlink` for a symlinked -destination agentsync is not comparing through, `shape` for a destination -that is not a regular file, or cannot be stat'd). For a -gate that should **fail the build** on drift, add `--exit-code`: `status +destination agentsync is not comparing through, `shape` for a destination that +is not a regular file, or cannot be stat'd). For a gate that should **fail the +build** on drift, add `--exit-code`: `status --exit-code` / `diff --exit-code` exit `2` when drift/hunks exist and `0` when clean (exit `2` is distinct from the generic error exit `1`, and prints no extra error line). Interactive prompts (e.g. the scope menu) always go to **stderr**, diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index 8748458f..942b52ad 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -208,10 +208,11 @@ func TestWriteBackFileItemMessageMatchesTheFailure(t *testing.T) { if !strings.Contains(err.Error(), "[i]gnore") { t.Errorf("error = %q, want it to still name a next step", err) } - // [o]verride is left out of the advice only for a NON-REGULAR destination, where it would - // hang (#241). For an absent one it is safe — Writer.Write's convergence - // read gets ENOENT and falls through to the write — and it is the actual - // fix, so withholding it here would deny the user the remedy that works. + // [o]verride is left out of the advice only for a NON-REGULAR destination, + // where it would hang (#241). For an absent one it is safe — Writer.Write's + // convergence read gets ENOENT and falls through to the write — and it is + // the actual fix, so leaving it out here would deny the user the remedy + // that works. if !strings.Contains(err.Error(), "[o]verride") { t.Errorf("error = %q, want it to offer [o]verride: the destination is absent, not "+ "non-regular, so re-applying canonical is safe and is the fix", err) diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 94bcc763..07ef5086 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -269,8 +269,10 @@ func isPseudoPointer(p string) bool { return p == ptrMode || p == ptrSymlink || // hunkDiffs is what the formatted diff renders for one hunk. A symlink or // shape hunk carries two UNRELATED labels ("regular file" against a sentence), // and a character diff of those shreds both into fragments — so each side is -// emitted whole. A mode hunk's two sides share their shape ("mode 0644" / -// "mode 0755"), where the character diff reads well, and text hunks are text. +// emitted whole, Delete for the destination and Insert for the source, the +// direction DiffMain(h.Dest, h.Source) gives every other hunk. A mode hunk's +// two sides share their shape ("mode 0644" / "mode 0755"), where a character +// diff highlights the changed digits, and text hunks are text. func hunkDiffs(dmp *diffmatchpatch.DiffMatchPatch, h diffHunk) []diffmatchpatch.Diff { if h.Pointer == ptrSymlink || h.Pointer == ptrShape { return []diffmatchpatch.Diff{ diff --git a/internal/cli/planwalk_internal_test.go b/internal/cli/planwalk_internal_test.go index cde65b41..f71425ac 100644 --- a/internal/cli/planwalk_internal_test.go +++ b/internal/cli/planwalk_internal_test.go @@ -659,16 +659,21 @@ func TestShortValShowsSentinelsWhole(t *testing.T) { // character diff its two like-shaped sides read well under. func TestLabelHunksAreNotCharacterDiffed(t *testing.T) { dmp := diffmatchpatch.New() - for _, h := range []diffHunk{ - {Pointer: ptrSymlink, Source: "regular file", Dest: symlinkRefusedHunkDest}, - {Pointer: ptrSymlink, Source: "regular file", Dest: symlinkUnresolvableHunkDest}, - {Pointer: ptrShape, Source: "regular file", Dest: shapeHunkDest}, + for _, tc := range []struct { + name string + hunk diffHunk + }{ + {name: "symlink-refused", hunk: diffHunk{Pointer: ptrSymlink, Source: "regular file", Dest: symlinkRefusedHunkDest}}, + {name: "symlink-unresolvable", hunk: diffHunk{Pointer: ptrSymlink, Source: "regular file", Dest: symlinkUnresolvableHunkDest}}, + {name: "shape", hunk: diffHunk{Pointer: ptrShape, Source: "regular file", Dest: shapeHunkDest}}, } { - got := hunkDiffs(dmp, h) - if len(got) != 2 || got[0].Type != diffmatchpatch.DiffDelete || got[0].Text != h.Dest || - got[1].Type != diffmatchpatch.DiffInsert || got[1].Text != h.Source { - t.Errorf("%s hunk: got %+v, want [-Dest-] then {+Source+} whole", h.Pointer, got) - } + t.Run(tc.name, func(t *testing.T) { + got := hunkDiffs(dmp, tc.hunk) + if len(got) != 2 || got[0].Type != diffmatchpatch.DiffDelete || got[0].Text != tc.hunk.Dest || + got[1].Type != diffmatchpatch.DiffInsert || got[1].Text != tc.hunk.Source { + t.Errorf("got %+v, want [-Dest-] then {+Source+} whole", got) + } + }) } mode := hunkDiffs(dmp, diffHunk{Pointer: ptrMode, Source: "mode 0755", Dest: "mode 0644"}) if len(mode) < 3 || mode[0].Type != diffmatchpatch.DiffEqual || mode[0].Text != "mode 0" { diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index 194f35e4..4c260ee2 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -1125,9 +1125,9 @@ func writeBackFileItem(home string, it reconcileItem) error { // mid-prompt with a keystroke to choose, and "read dest X: not a regular // file" alone does not tell them which one gets them unstuck. // - // This arm's advice deliberately omits [o]verride (so does the symlink - // arms'), unlike the absent arm below. It re-applies - // through render.Writer.Write, whose convergence read is not + // This arm's advice deliberately omits [o]verride, as the symlink + // arms' does, unlike the absent arm below. It re-applies through + // render.Writer.Write, whose convergence read is not // shape-guarded, so on this exact item it does not fail — it HANGS // (measured: `reconcile --auto-override` rc=124). // An earlier version of this message recommended it, which walked the @@ -1141,8 +1141,8 @@ func writeBackFileItem(home string, it reconcileItem) error { // an ABSENT destination — the user deleted a managed file, which is // itself drift — and there [o]verride is both safe and usually the fix: // Writer.Write's convergence read gets ENOENT and falls straight - // through to the write. Withholding it is only correct for the - // non-regular case above. + // through to the write. Leaving it out of the advice is only right for + // the non-regular case above. return fmt.Errorf("read dest %s: %w — use [o]verride to restore it from canonical, "+ "or [i]gnore to suppress this item", it.op.Path, err) } diff --git a/internal/cli/status.go b/internal/cli/status.go index a260d880..e7a3906e 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -1001,9 +1001,9 @@ func hashFile(path string) string { // which keys prose on this one, words it for both facts (reconcile's // prompt shows the token itself). Before this gate existed the // predicate answered false for an unstattable destination too, so - // splitting them here would move a - // parent-ENOTDIR dest from ForeignCollision to New — and New is - // SafeForAutoApply. A plain read failure still answers "", as it did. + // splitting them here would move a parent-ENOTDIR dest from + // ForeignCollision to New — and New is SafeForAutoApply. A plain read + // failure still answers "", as it did. if errors.Is(err, errDestNotRegular) || errors.Is(err, errDestUnstattable) { return shapeSentinel }