diff --git a/CHANGELOG.md b/CHANGELOG.md index 0709f9de..4b2a5f4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,43 @@ 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 — `apply`, `status`, `diff`, `reconcile` and `explain` all need it: + 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 (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 + 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 + 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 — 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 + 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 + [#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` fails it immediately with `EISDIR` — and for `status` and `diff` nothing about @@ -202,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..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). +- **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` | 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 `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/SECURITY.md b/SECURITY.md index 92681d0d..80144f0e 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -94,8 +94,12 @@ 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 (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 + foreign files are backed up before overwrite. ## Sensitive files diff --git a/docs/architecture.md b/docs/architecture.md index cb6fdf4f..21395291 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -888,9 +888,45 @@ 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.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. + +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 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 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 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 +943,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..8801b92c 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 `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/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/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 2e6c1ec2..6b4dcad8 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -1029,8 +1029,13 @@ 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 -gate that should **fail the build** on drift, add `--exit-code`: `status +`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 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 --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**, @@ -1058,7 +1063,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 `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 d8aa3992..b555cd47 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" ) @@ -21,12 +23,22 @@ 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 +// 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. // @@ -57,40 +69,25 @@ 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 -// 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. -// 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 - // 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 @@ -104,3 +101,68 @@ 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, 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: apply fails on it too +) + +// destReadPath applies the destination SYMLINK policy and answers the path the +// 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 +// 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; 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, 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 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 + } + if !iox.SymlinkDestAllowed() { + return "", symlinkRefusedByEnv + } + r, err := filepath.EvalSymlinks(path) + if err != nil { + return "", symlinkUnresolvable + } + 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, why := destReadPath(path) + if why != symlinkNone { + 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..942b52ad 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -6,12 +6,15 @@ import ( "errors" "os" "path/filepath" + "strconv" "strings" "syscall" "testing" "time" "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 @@ -205,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 withheld 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) @@ -257,12 +261,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 +282,65 @@ 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", + }, + { + // 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: "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", setup: mkfifoDest, @@ -329,7 +393,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): @@ -338,9 +402,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 +421,146 @@ 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 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) { + 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 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) + } + 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 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) + } + } + // [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) + } + 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 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) + } + 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) + } + }) + + 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() + 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) } }) } @@ -406,3 +611,84 @@ 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 + content string + }{ + {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) + 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, tc.content)}}), []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) + } + }) + } +} + +// 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 349a94c8..07ef5086 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" @@ -164,8 +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) - 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 @@ -216,10 +216,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 +228,78 @@ 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. +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, 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 +// 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", symlinkRefusedHunkDest, true +} + +// 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, 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{ + {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. +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 — 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.destShapeRefused() { + return "", "", false + } + return "regular file", shapeHunkDest, true +} + func marshalPretty(v any) string { if v == nil { return "" @@ -281,11 +353,21 @@ 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: ptrSymlink, Source: src, Dest: dst}) + continue + } + if src, dst, ok := shapeHunk(it); ok { + 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/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..c3ae12b0 100644 --- a/internal/cli/planwalk.go +++ b/internal/cli/planwalk.go @@ -44,49 +44,42 @@ 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, - // and one of two opaque sentinels for a symlinked 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. 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, 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 + // 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 // 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 } -// 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 / +// 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 @@ -94,11 +87,44 @@ 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 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.cls == drift.Converged) && i.opModeDrifted() { + return drift.Drift + } + return i.cls +} + +// 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). +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, 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, why := destReadPath(path) + if why != symlinkNone { + return 0, false + } fi, err := os.Lstat(path) if err != nil || !fi.Mode().IsRegular() { return 0, false @@ -170,9 +196,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 { @@ -226,21 +254,17 @@ 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 { - 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) } @@ -256,12 +280,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..072bceca 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 @@ -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,15 +353,18 @@ 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} }, }, - // 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") }, @@ -380,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 { @@ -393,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) } }, }, @@ -1008,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 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 2d2b5182..f71425ac 100644 --- a/internal/cli/planwalk_internal_test.go +++ b/internal/cli/planwalk_internal_test.go @@ -7,11 +7,15 @@ import ( "path/filepath" "reflect" "sort" + "strings" "testing" + "github.com/sergi/go-diff/diffmatchpatch" + "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" ) @@ -186,13 +190,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) @@ -237,6 +244,64 @@ 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) + } + }, + }, + { + // 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) + } + // 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) + } + 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) { @@ -320,7 +385,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") @@ -335,16 +400,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) } }, }, @@ -483,7 +583,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) { @@ -533,3 +633,50 @@ 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: 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 { + t.Errorf("shortVal(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// 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 _, 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}}, + } { + 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" { + 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 ee8c30c3..4c260ee2 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 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 @@ -666,7 +668,9 @@ 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 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 { @@ -722,10 +726,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 (the symlink and shape tokens) 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 @@ -1090,15 +1103,31 @@ 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, why := destReadPath(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. 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 — "+ + "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 "+ + "(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 { // 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 // 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 - // 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 @@ -1112,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 164de60a..e7a3906e 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" @@ -287,7 +286,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 +314,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]++ } @@ -940,6 +938,22 @@ 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 +// (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. +const symlinkUnresolvableSentinel = "symlink-target-unresolvable" + func hashContent(b []byte) string { sum := sha256.Sum256(b) return hex.EncodeToString(sum[:]) @@ -951,41 +965,47 @@ 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 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 +// 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, 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 { - 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, 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 symlinkRefusedSentinel + case symlinkUnresolvable: + return symlinkUnresolvableSentinel } - // 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. - // + path = p // 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 are never shown; they exist only to never equal a content - // hash. 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. + // 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. if errors.Is(err, errDestNotRegular) || errors.Is(err, errDestUnstattable) { - return "not-a-regular-file" + return shapeSentinel } return "" } diff --git a/internal/cli/symlink_dest_test.go b/internal/cli/symlink_dest_test.go new file mode 100644 index 00000000..34f5dd43 --- /dev/null +++ b/internal/cli/symlink_dest_test.go @@ -0,0 +1,194 @@ +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 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 +// 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) + } + // 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) + } + // 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) + } +} 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..5ca43910 100644 --- a/internal/iox/atomic_test.go +++ b/internal/iox/atomic_test.go @@ -109,6 +109,36 @@ 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}, + {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 + 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) { diff --git a/website/src/content/docs/help/troubleshooting.mdx b/website/src/content/docs/help/troubleshooting.mdx index 1f8d860d..b782f804 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 `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 +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..5bb76521 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 `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` | 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 `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,4 +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`, + `status`, `diff`, `reconcile` and `explain` all need it, because it governs + reads as well as writes.