From d4fb233a81e2ba50b4914eb1419a27593284c931 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 19:26:14 +0000 Subject: [PATCH 1/9] fix(cli): route every destination read through one shape gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A FIFO at a managed destination hangs agentsync. os.ReadFile on a FIFO does not fail, it BLOCKS in the open waiting for a writer that never comes, so the read's own error path never runs and the command never returns. Measured at f6aa686, with the destination swapped for a 0600 FIFO after a clean apply: whole-file dest diff, reconcile --auto-safe hang; status clean key-merge dest status, diff, reconcile hang status was already safe for the whole-file shape because hashFile applies render.IsRegularOrAbsent — and its comment claims it "shares render's predicate so the destination-read guards cannot disagree about what is safe to read". That was true of the hash and false of every other destination read. docs/components.md:394 made the same claim about internal/cli as a whole; it was written ahead of the code. All seven destination reads in internal/cli now go through readDestBytes, which applies the same predicate before the open. An ABSENT path is deliberately let through to os.ReadFile, whose ENOENT is the truthful answer every caller already handles; manufacturing a shape error for a file that is not there would name the wrong problem. Four of the seven are measured hangs, each pinned by a timeout-bounded test — this class does not fail, it hangs, so an unbounded test would wedge CI with no diagnostic rather than report in 5-8s: - readDestFile, the key-merge read ALL FOUR drift walks share - diff's per-op whole-file read - reconcile's per-op whole-file read - writeBackFileItem, which a user reaches one keystroke LATER: with only the classification reads guarded, a FIFO dest classifies as drift, the user presses [w], and the hang lands there instead The other three are import's state-seeding reads. They are NOT proven hangs: no fixture was found that reaches them with a non-regular destination, and with every guard removed `agentsync import` still returned. They were routed through the gate anyway, because three structurally identical unguarded reads beside four guarded ones invite the question "why those and not these", and "nobody found a fixture yet" is not an answer. TestEveryDestinationReadGoesThroughTheGate keeps that decision from rotting: a new bare os.ReadFile(op.Path) under internal/cli fails even where no hang can be demonstrated. Its proof-of-life arm earned its keep immediately — the first draft used os.ReadDir(".") and silently scanned zero files. Deliberately NOT touched: internal/render and the adapter Apply paths also read op.Path directly, but those are the write path with their own upstream handling (render.isRegularOrAbsent's doc names apply's pre-delete read). Unaudited here; the guard is scoped to internal/cli and claims nothing about them. Break-verified: removing the gate fails all four measured sites at their timeouts with the right diagnostic, and the guard test names the offending file when a bare read is planted back into diff.go. Groundwork for #229, which unifies these four drift walks; the guard has to land first so the shared walk inherits one dest-read policy instead of four. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 19 ++ docs/components.md | 8 +- internal/cli/dest_fifo_e2e_unix_test.go | 126 ++++++++++++++ internal/cli/destread.go | 43 +++++ internal/cli/destread_guard_internal_test.go | 70 ++++++++ internal/cli/destread_unix_internal_test.go | 174 +++++++++++++++++++ internal/cli/diff.go | 2 +- internal/cli/import.go | 8 +- internal/cli/reconcile.go | 4 +- 9 files changed, 446 insertions(+), 8 deletions(-) create mode 100644 internal/cli/dest_fifo_e2e_unix_test.go create mode 100644 internal/cli/destread.go create mode 100644 internal/cli/destread_guard_internal_test.go create mode 100644 internal/cli/destread_unix_internal_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f26ac95..8a88300f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,25 @@ source layout, CLI surface, and state schema are stabilizing but may still chang ### Fixed +- **A FIFO, directory, or other non-regular file at a managed destination no + longer hangs `status`, `diff` or `reconcile`.** `os.ReadFile` on a FIFO does + not fail — it blocks in the open waiting for a writer that never comes — so + the read's own error path never runs and the command never returns. Measured + on the previous release: a FIFO at a whole-file destination wedged `diff` and + `reconcile`, and a FIFO-shaped key-merge destination (a `~/.claude.json`, + say) additionally wedged `status`, which is advertised as read-only. Each had + to be killed. + + `status` was already safe for the whole-file shape, because its `hashFile` + applied `render.IsRegularOrAbsent` and said it "shares render's predicate so + the destination-read guards cannot disagree about what is safe to read". That + was true of the hash and false of every other destination read. All of them + now go through one gate, `readDestBytes`, which refuses a non-regular path + before the open and lets an ABSENT one through to the read, whose ENOENT is + the answer every caller already handles. A `reconcile` write-back (`[w]`) is + covered too — with only the classification reads guarded, a non-regular + destination would classify as drift and then hang one keystroke later. + - **`agentsync check` no longer rejects a `[secrets].backend` that `apply` accepts.** `secrets.SelectBackend` — the function `apply` actually resolves through — lower-cases the backend name, but `check` compared it against the diff --git a/docs/components.md b/docs/components.md index 42e3bba4..67dfe3c1 100644 --- a/docs/components.md +++ b/docs/components.md @@ -391,7 +391,13 @@ symmetric with the dest→source write boundary (see architecture §7). this component KIND reclaimed at all — drives reconcile's prompt wording) and `OrphanDeleteWillProceed` (will THIS destination actually be removed on this run — keeps the apply summary from counting a skipped delete). `IsRegularOrAbsent` - is shared with `internal/cli`'s destination reads so a FIFO cannot block them. + is shared with `internal/cli`'s destination reads so a FIFO cannot block them: + every one of them goes through `readDestBytes` (`internal/cli/destread.go`), + which applies this predicate before the open. Enforced, not asserted — + `TestEveryDestinationReadGoesThroughTheGate` fails on a bare + `os.ReadFile(op.Path)` anywhere under `internal/cli`. (The claim was written + ahead of the code: until then only `status`'s `hashFile` used the predicate, + and `diff`, `reconcile` and the shared key-merge read all blocked.) - **Depends on:** adapter, secrets, source, state, paths, iox, drift. - **Files:** `pipeline.go`, `writer.go`, `state_apply.go`, `report.go`. diff --git a/internal/cli/dest_fifo_e2e_unix_test.go b/internal/cli/dest_fifo_e2e_unix_test.go new file mode 100644 index 00000000..baaf9566 --- /dev/null +++ b/internal/cli/dest_fifo_e2e_unix_test.go @@ -0,0 +1,126 @@ +//go:build unix + +package cli_test + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/spxrogers/agentsync/internal/cli" +) + +// TestCommandsDoNotHangOnNonRegularDestination is the end-to-end half of the +// destination-read guard: it drives the real commands against a real applied +// home, which is the only way to prove the guard actually sits on the path the +// user reaches. +// +// Both shapes are covered because they are read by different code. A whole-file +// destination goes through the per-op reads in `diff` and `reconcile`; a +// key-merge destination (~/.claude.json) goes through the shared readDestFile +// that every drift walk uses, including `status` — which is advertised as +// read-only and hung on it. +// +// Every run is bounded. runCLI executes the command IN-PROCESS, so an +// unguarded read does not fail the test, it wedges the whole test binary until +// the package timeout kills it with a stack dump and no useful diagnostic. +func TestCommandsDoNotHangOnNonRegularDestination(t *testing.T) { + tmp := t.TempDir() + env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp} + + if _, err := runCLI(t, env, "init"); err != nil { + t.Fatal(err) + } + if _, err := runCLI(t, env, "agent", "add", "claude"); err != nil { + t.Fatal(err) + } + // A key-merge destination (~/.claude.json) … + mcp := filepath.Join(tmp, ".agentsync", "mcp", "github.toml") + if err := os.MkdirAll(filepath.Dir(mcp), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(mcp, []byte("[server]\ntype=\"stdio\"\ncommand=\"npx\"\n"), 0o644); err != nil { + t.Fatal(err) + } + // … and a whole-file one (a rendered skill). + skill := filepath.Join(tmp, ".agentsync", "skills", "demo", "SKILL.md") + if err := os.MkdirAll(filepath.Dir(skill), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(skill, []byte("---\nname: demo\ndescription: d\n---\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := runCLI(t, env, "apply"); err != nil { + t.Fatal(err) + } + + for _, dest := range []struct{ name, path string }{ + {"whole-file destination", filepath.Join(tmp, ".claude", "skills", "demo", "SKILL.md")}, + {"key-merge destination", filepath.Join(tmp, ".claude.json")}, + } { + t.Run(dest.name, func(t *testing.T) { + if _, err := os.Stat(dest.path); err != nil { + t.Fatalf("fixture never applied %s: %v — this test would pass vacuously", dest.path, err) + } + // Swap the applied destination for a FIFO. Nothing here opens it. + if err := os.Remove(dest.path); err != nil { + t.Fatal(err) + } + if err := syscall.Mkfifo(dest.path, 0o600); err != nil { + t.Skipf("mkfifo unsupported here: %v", err) + } + t.Cleanup(func() { _ = os.Remove(dest.path) }) + + for _, args := range [][]string{ + {"status"}, + {"diff"}, + {"reconcile", "--auto-safe"}, + {"import", "--dry-run"}, + // A REAL import too: importRun returns early on --dry-run + // (import.go:331), so the state-seeding destination reads in + // seedStateFromCurrentDest and unimportedDestPointers are only + // reachable without it. + {"import"}, + } { + t.Run(strings.Join(args, " "), func(t *testing.T) { + // Exit status is deliberately not asserted: a non-regular + // destination may legitimately report drift, or refuse. The + // contract under test is that the command RETURNS. + runBounded(t, 8*time.Second, args...) + }) + } + }) + } +} + +// runBounded executes the CLI in a goroutine and fails if it has not returned +// within d. The environment must already be set by the caller on the test +// goroutine (runCLI's t.Setenv persists for the whole test), so nothing here +// touches testing.T off the test goroutine except the final t.Fatalf. +func runBounded(t *testing.T, d time.Duration, args ...string) string { + t.Helper() + detachSlog(t) + done := make(chan string, 1) + go func() { + var buf bytes.Buffer + root := cli.NewRoot() + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs(args) + _ = root.Execute() + done <- buf.String() + }() + select { + case out := <-done: + return out + case <-time.After(d): + t.Fatalf("`agentsync %s` BLOCKED on a non-regular destination — os.ReadFile on a "+ + "FIFO waits for a writer that never comes, so the read's own error path never "+ + "runs and the command never returns", strings.Join(args, " ")) + return "" + } +} diff --git a/internal/cli/destread.go b/internal/cli/destread.go new file mode 100644 index 00000000..a5f72ec9 --- /dev/null +++ b/internal/cli/destread.go @@ -0,0 +1,43 @@ +package cli + +import ( + "errors" + "os" + + "github.com/spxrogers/agentsync/internal/render" +) + +// errDestNotRegular is returned by readDestBytes for a destination path that is +// present but is not a regular file. It is a sentinel so callers can match it +// with errors.Is; it deliberately carries NO path, because every caller either +// discards it or wraps it with the path itself, and embedding one here would +// double it in the wrapped message. +var errDestNotRegular = errors.New("not a regular file") + +// readDestBytes reads a destination file's bytes, refusing any path that is +// present but not a regular file BEFORE the open. +// +// The guard is not defensive tidying. os.ReadFile on a FIFO does not fail — it +// BLOCKS in the open, waiting for a writer that never comes, so no error path +// below the read ever runs and the command never returns. Measured on the +// unguarded code: a FIFO at a managed destination wedged `diff` and `reconcile` +// via their whole-file reads, and a FIFO-shaped key-merge destination (a +// ~/.claude.json, say) wedged `status` — which is advertised as read-only — +// through the shared readDestFile. +// +// render.hashFile already applied exactly this rule, and said so: "Shares +// render's predicate so the destination-read guards cannot disagree about what +// is safe to read." That was true of the HASH and false of every other +// destination read, which is the gap this closes. Same predicate, so the +// statement is now true of all of them. +// +// An ABSENT path is not refused: render.IsRegularOrAbsent passes it through to +// os.ReadFile, whose ENOENT is the truthful answer and is what every caller +// already handles. Manufacturing a shape error for a file that is not there +// would name the wrong problem. +func readDestBytes(path string) ([]byte, error) { + if !render.IsRegularOrAbsent(path) { + return nil, errDestNotRegular + } + return os.ReadFile(path) +} diff --git a/internal/cli/destread_guard_internal_test.go b/internal/cli/destread_guard_internal_test.go new file mode 100644 index 00000000..d82ab126 --- /dev/null +++ b/internal/cli/destread_guard_internal_test.go @@ -0,0 +1,70 @@ +package cli + +import ( + "strings" + "testing" +) + +// TestEveryDestinationReadGoesThroughTheGate enforces the invariant that makes +// the FIFO guard hold: no production code in this package reads a destination +// path with a bare os.ReadFile. +// +// Why an invariant and not just the behavior tests: the four reads that were +// MEASURED to hang (readDestFile, diff's and reconcile's per-op reads, and +// writeBackFileItem) are each pinned by a timeout-bounded test. The three in +// import's state-seeding path are not — no fixture was found that reaches them +// with a non-regular destination, and with every guard removed `agentsync +// import` still returned. They were routed through the gate anyway, because +// three structurally identical unguarded reads sitting beside four guarded ones +// invite exactly the question "why those and not these", and the honest answer +// — "nobody found a fixture yet" — is not a reason to leave them. This test is +// what keeps that decision from silently rotting: a new bare destination read +// fails here even where no hang can be demonstrated. +// +// LIMITS, stated so a reader does not over-trust it: +// - The pattern is TEXT-shaped. A read spelled through a variable +// (`p := op.Path; os.ReadFile(p)`), via afero, or with os.Open + io.ReadAll +// slips past. It catches the copy-paste that actually happened seven times, +// not every conceivable spelling. +// - It scans comments and string literals too, so a mention of the pattern in +// prose would trip it. That is the safe direction to fail. +func TestEveryDestinationReadGoesThroughTheGate(t *testing.T) { + // The forbidden spellings: a bare read of an op's destination path. + forbidden := []string{ + "os.ReadFile(op.Path)", + "os.ReadFile(it.op.Path)", + } + + repoRoot := repoRootFromCaller(t) + scanned := 0 + if err := walkRepoGoFiles(repoRoot, func(rel, src string) { + // Scoped to this package deliberately. internal/render and the adapter + // Apply paths also read op.Path, but those are the WRITE path, with + // their own upstream shape handling (see render.isRegularOrAbsent's doc + // comment, which names apply's pre-delete read). They were not audited + // here and this guard makes no claim about them. + if !strings.HasPrefix(rel, "internal/cli/") { + return + } + scanned++ + for _, pat := range forbidden { + if strings.Contains(src, pat) { + t.Errorf("%s contains %q — destination reads must go through readDestBytes "+ + "(internal/cli/destread.go), which refuses a non-regular path BEFORE the "+ + "open. os.ReadFile on a FIFO does not fail, it BLOCKS, so the read's own "+ + "error path never runs and the command never returns.", rel, pat) + } + } + }); err != nil { + t.Fatal(err) + } + + // Proof of life. Without this the guard passes vacuously the day the walk + // stops finding files — the failure mode that makes a green guard worthless. + // It already earned its keep once: the first draft used os.ReadDir(".") and + // silently scanned nothing. + if scanned < 10 { + t.Fatalf("scanned only %d production files under internal/cli/; the guard is not "+ + "looking at the code it claims to check", scanned) + } +} diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go new file mode 100644 index 00000000..cde6354f --- /dev/null +++ b/internal/cli/destread_unix_internal_test.go @@ -0,0 +1,174 @@ +//go:build unix + +package cli + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/spxrogers/agentsync/internal/adapter" +) + +// TestReadDestBytesShape pins the gate every destination read now passes +// through. +// +// Every assertion here is bounded by a timeout, and that is the point rather +// than caution: this defect class does not FAIL, it HANGS. os.ReadFile on a +// FIFO blocks in the open waiting for a writer that never comes, so a test +// written as a plain call would wedge the suite with no diagnostic instead of +// reporting in 5s. Nothing in this file ever opens the FIFO it creates — +// mkfifo, chmod and os.Stat do not. +func TestReadDestBytesShape(t *testing.T) { + cases := []struct { + name string + // setup returns the path to read. It must never OPEN that path. + setup func(t *testing.T, tmp string) string + wantErr error // errDestNotRegular, os.ErrNotExist, or nil + wantData string + }{ + { + // The sharp one: present, stats fine, and its open never returns. + name: "a FIFO destination is refused by shape", + setup: mkfifoDest, + wantErr: errDestNotRegular, + }, + { + name: "a directory destination is refused by shape", + setup: func(t *testing.T, tmp string) string { + t.Helper() + p := filepath.Join(tmp, "dest") + if err := os.Mkdir(p, 0o700); err != nil { + t.Fatal(err) + } + return p + }, + wantErr: errDestNotRegular, + }, + { + // The fail-open row, and the reason the gate does not refuse on any + // stat failure: an absent destination is ordinary, and every caller + // already handles its ENOENT. A shape error here would name the + // wrong problem. + name: "an absent destination still reports the truthful ENOENT", + setup: func(t *testing.T, tmp string) string { + return filepath.Join(tmp, "absent") + }, + wantErr: os.ErrNotExist, + }, + { + // The guard against an over-broad arm. + name: "an ordinary regular destination is read", + setup: func(t *testing.T, tmp string) string { + t.Helper() + p := filepath.Join(tmp, "dest") + if err := os.WriteFile(p, []byte("payload"), 0o644); err != nil { + t.Fatal(err) + } + return p + }, + wantData: "payload", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + path := tc.setup(t, t.TempDir()) + + type result struct { + data []byte + err error + } + // By channel, not a captured variable, so a read that DID block + // cannot race the assertions under -race. + ch := make(chan result, 1) + go func() { + d, err := readDestBytes(path) + ch <- result{d, err} + }() + var got result + select { + case got = <-ch: + case <-time.After(5 * time.Second): + t.Fatalf("readDestBytes BLOCKED on %s — the destination must be STAT'd for "+ + "shape before it is opened: os.ReadFile on a FIFO waits for a writer "+ + "that never comes, so the read's own error path never runs", path) + } + + if tc.wantErr == nil { + if got.err != nil { + t.Fatalf("readDestBytes(%s) = %v, want success", path, got.err) + } + if string(got.data) != tc.wantData { + t.Errorf("data = %q, want %q", got.data, tc.wantData) + } + return + } + if !errors.Is(got.err, tc.wantErr) { + t.Fatalf("readDestBytes(%s) error = %v, want errors.Is(_, %v)", path, got.err, tc.wantErr) + } + if got.data != nil { + t.Errorf("data = %q on an error, want nil", got.data) + } + }) + } +} + +// TestKeyMergeAndWriteBackReadsAreGuarded covers the two callers whose own read +// is not reachable from an end-to-end CLI run in this package. +// +// readDestFile is the read ALL FOUR drift walks share for key-merge ops, so an +// unguarded one wedged `status` — a command advertised as read-only — on a +// FIFO-shaped ~/.claude.json. writeBackFileItem is the one a user reaches one +// keystroke LATER: with only the classification reads guarded, a FIFO +// destination classifies as drift, the user presses [w], and the hang lands +// there instead. +func TestKeyMergeAndWriteBackReadsAreGuarded(t *testing.T) { + t.Run("readDestFile returns an empty map rather than blocking", func(t *testing.T) { + p := mkfifoDest(t, t.TempDir()) + ch := make(chan map[string]any, 1) + go func() { ch <- readDestFile("merge-json-keys", p) }() + select { + case got := <-ch: + if len(got) != 0 { + t.Errorf("readDestFile = %v, want an empty map", got) + } + case <-time.After(5 * time.Second): + t.Fatalf("readDestFile BLOCKED on a FIFO destination (%s) — this is the read "+ + "every key-merge drift walk shares, including read-only `status`", p) + } + }) + + t.Run("writeBackFileItem returns an error rather than blocking", func(t *testing.T) { + p := mkfifoDest(t, t.TempDir()) + it := reconcileItem{op: adapter.FileOp{Path: p, SourceID: "demo"}} + ch := make(chan error, 1) + go func() { ch <- writeBackFileItem(t.TempDir(), it) }() + select { + case err := <-ch: + if err == nil { + t.Fatal("writeBackFileItem = nil, want an error: a FIFO carries no dest content to write back") + } + case <-time.After(5 * time.Second): + t.Fatalf("writeBackFileItem BLOCKED on a FIFO destination (%s) — [w] must refuse, "+ + "not wedge the reconcile session", p) + } + }) +} + +// mkfifoDest creates a 0600 FIFO at a destination path and returns it. mkfifo +// and chmod do not open the FIFO; nothing in this file ever does. +func mkfifoDest(t *testing.T, tmp string) string { + t.Helper() + p := filepath.Join(tmp, "dest") + if err := syscall.Mkfifo(p, 0o600); err != nil { + t.Skipf("mkfifo unsupported here: %v", err) + } + if err := os.Chmod(p, 0o600); err != nil { + t.Fatal(err) + } + return p +} diff --git a/internal/cli/diff.go b/internal/cli/diff.go index a66609c5..6e0ab399 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -175,7 +175,7 @@ func newDiffCmd() *cobra.Command { } seen[op.Path] = true srcStr := secrets.MaskResolved(string(op.Content), redact) - dstBytes, readErr := os.ReadFile(op.Path) + dstBytes, readErr := readDestBytes(op.Path) dstStr := "" if readErr == nil { dstStr = secrets.MaskResolved(string(dstBytes), redact) diff --git a/internal/cli/import.go b/internal/cli/import.go index 8f3db00e..7220a97d 100644 --- a/internal/cli/import.go +++ b/internal/cli/import.go @@ -75,7 +75,7 @@ func decodeDestBytes(strategy string, data []byte, v *map[string]any) error { // unreadable dest classifies as "absent" rather than crashing. Replaces the // JSON-only readJSONFile so a TOML config.toml decodes correctly. func readDestFile(strategy, path string) map[string]any { - data, err := os.ReadFile(path) + data, err := readDestBytes(path) if err != nil { return map[string]any{} } @@ -599,7 +599,7 @@ func unimportedDestPointers(agentsyncHome, srcHome, agentName string, reg *adapt if !render.IsKeyMerge(op.MergeStrategy) { continue } - data, readErr := os.ReadFile(op.Path) + data, readErr := readDestBytes(op.Path) if readErr != nil { continue } @@ -714,7 +714,7 @@ func seedStateFromCurrentDest(agentsyncHome, srcHome, agentName string, reg *ada // Per-key seed: hash the *current* value at each pointer the // rendered op claims to own. The dest is decoded per strategy (TOML // for merge-toml-keys); op.Content is always JSON. - data, readErr := os.ReadFile(op.Path) + data, readErr := readDestBytes(op.Path) if readErr != nil { continue // dest doesn't exist yet; nothing to seed } @@ -742,7 +742,7 @@ func seedStateFromCurrentDest(agentsyncHome, srcHome, agentName string, reg *ada } } default: - data, readErr := os.ReadFile(op.Path) + data, readErr := readDestBytes(op.Path) if readErr != nil { continue } diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index 9828336d..ce465080 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -655,7 +655,7 @@ func collectItems(plan render.RenderPlan, reg *adapter.Registry, s *state.Target happlied := s.Files[stateFileKey(userHome, name, sc, projectRoot, op.Path)].SHA256 hdest := hashFile(op.Path) cls := drift.Classify(hsrc, happlied, hdest) - dstBytes, _ := os.ReadFile(op.Path) + dstBytes, _ := readDestBytes(op.Path) items = append(items, reconcileItem{ agentName: name, op: op, @@ -1151,7 +1151,7 @@ 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 := os.ReadFile(it.op.Path) + data, err := readDestBytes(it.op.Path) if err != nil { return fmt.Errorf("read dest %s: %w", it.op.Path, err) } From 269d5465c25f9d0321792c8256a6f98252d9d25b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 19:46:07 +0000 Subject: [PATCH 2/9] fix(cli): fold hashFile through the gate; scope the claims to what is fixed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes round-1 review findings on PR #240. Three of four lenses converged on the same two blockers, and both were mine. 1. The e2e import rows were VACUOUS, and the claim they backed was wrong. `import` is cobra.ExactArgs(1); the rows passed no agent selector, so cobra rejected them before RunE and they executed zero lines of import code. They passed identically with the fix reverted. That broken measurement is what the previous commit's "import's three sites are unproven hangs" rested on. A real `agentsync import claude` DOES hang, at d4fb233, with the gate in place — the read is upstream in the adapter Ingest paths, which this gate never covered. Rows removed rather than fixed, because asserting them would be asserting a bug. runBounded now fails any invocation cobra rejects, so this class cannot recur silently. 2. `apply` and `apply --dry-run` hang on the same fixture (measured rc=124), through render.Writer.Write's convergence read. `apply --dry-run` is advertised read-only. Not fixed here: between internal/render and the adapter Ingest paths that is ~60 sites across eleven packages, which is a sweep of its own, not a prerequisite bugfix. Filed as #241 and #242 with the measurements and the suggested shape. Consequently the PR's claims are narrowed to what it does: the drift-read path in internal/cli. The CHANGELOG, docs/components.md, destread.go's doc comment and the guard test's stated invariant all said or implied more. 3. hashFile was an EIGHTH destination read holding a second copy of the policy (guarded, so not a hang — but "the guards cannot disagree" was a coincidence rather than a property, and #229's shared walk would have inherited two policies since all four walks call it). It now goes through readDestBytes, which also gives errDestNotRegular its first production errors.Is consumer. Behavior-preserving: the existing TestHashFile_FIFODoesNotBlock catches the sentinel collapsing. 4. The symlink axis: destread.go claimed "Same predicate, so the statement is now true of all of them". False — hashFile Lstats and refuses a symlink, readDestBytes does not, so status calls a symlinked dest drifted while diff reads through it. Pre-existing (the old bare reads followed links too) and deliberately unchanged, because AGENTSYNC_ALLOW_SYMLINK_DEST=1 is a documented setup where apply writes through the link. Documented as a known divergence and left to #229's behavior pass. Also: the guard test gained the synthetic negative control both prior guards in this package run every time, driving the real matcher over planted sources in both directions; its LIMITS now name what it cannot see (readDestFile's own `os.ReadFile(path)`) and that it says nothing about apply/import. writeBackFileItem's refusal names a next step like its peers, and a test pins both the path wrap and the guidance — dropping the wrap previously left the suite green, so the pathless sentinel's rationale was untested. Break-verified: dropping the wrap fails on both arms; collapsing hashFile's sentinel fails TestHashFile_FIFODoesNotBlock. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 20 +++++--- docs/components.md | 18 ++++--- internal/cli/dest_fifo_e2e_unix_test.go | 20 +++++--- internal/cli/destread.go | 42 +++++++++++----- internal/cli/destread_guard_internal_test.go | 50 ++++++++++++++++---- internal/cli/destread_unix_internal_test.go | 14 ++++++ internal/cli/reconcile.go | 6 ++- internal/cli/status.go | 18 ++++--- 8 files changed, 141 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a88300f..0a600c6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,12 +23,20 @@ source layout, CLI surface, and state schema are stabilizing but may still chang `status` was already safe for the whole-file shape, because its `hashFile` applied `render.IsRegularOrAbsent` and said it "shares render's predicate so the destination-read guards cannot disagree about what is safe to read". That - was true of the hash and false of every other destination read. All of them - now go through one gate, `readDestBytes`, which refuses a non-regular path - before the open and lets an ABSENT one through to the read, whose ENOENT is - the answer every caller already handles. A `reconcile` write-back (`[w]`) is - covered too — with only the classification reads guarded, a non-regular - destination would classify as drift and then hang one keystroke later. + was true of the hash and false of every other destination read. Every + destination read in `internal/cli` — `hashFile` included, which held a second + copy of the rule — now goes through one gate, `readDestBytes`, which refuses a + non-regular path before the open and lets an ABSENT one through to the read, + whose ENOENT is the answer every caller already handles. A `reconcile` + write-back (`[w]`) is covered too: with only the classification reads guarded, + a non-regular destination would classify as drift and then hang one keystroke + later. + + **`apply`, `apply --dry-run` and `import ` are NOT fixed by this** and + still hang on the same fixture — their reads are in `internal/render` and the + adapter `Ingest` paths, a far wider sweep. Tracked as + [#241](https://github.com/spxrogers/agentsync/issues/241) and + [#242](https://github.com/spxrogers/agentsync/issues/242). - **`agentsync check` no longer rejects a `[secrets].backend` that `apply` accepts.** `secrets.SelectBackend` — the function `apply` actually resolves diff --git a/docs/components.md b/docs/components.md index 67dfe3c1..b8601e6e 100644 --- a/docs/components.md +++ b/docs/components.md @@ -391,13 +391,17 @@ symmetric with the dest→source write boundary (see architecture §7). this component KIND reclaimed at all — drives reconcile's prompt wording) and `OrphanDeleteWillProceed` (will THIS destination actually be removed on this run — keeps the apply summary from counting a skipped delete). `IsRegularOrAbsent` - is shared with `internal/cli`'s destination reads so a FIFO cannot block them: - every one of them goes through `readDestBytes` (`internal/cli/destread.go`), - which applies this predicate before the open. Enforced, not asserted — - `TestEveryDestinationReadGoesThroughTheGate` fails on a bare - `os.ReadFile(op.Path)` anywhere under `internal/cli`. (The claim was written - ahead of the code: until then only `status`'s `hashFile` used the predicate, - and `diff`, `reconcile` and the shared key-merge read all blocked.) + is shared with `internal/cli`'s destination reads so a FIFO cannot block + **those**: every one of them goes through `readDestBytes` + (`internal/cli/destread.go`), which applies this predicate before the open. + Enforced, not asserted — `TestEveryDestinationReadGoesThroughTheGate` fails on + a bare `os.ReadFile(op.Path)` anywhere under `internal/cli`. (The claim was + written ahead of the code: until then only `status`'s `hashFile` used the + predicate, and `diff`, `reconcile` and the shared key-merge read all blocked.) + It is **not** yet true of this package's own `Writer.Write` convergence read + or of the adapter `Ingest` paths, so `apply`, `apply --dry-run` and + `import ` still block on a non-regular destination — issues #241 and + #242. - **Depends on:** adapter, secrets, source, state, paths, iox, drift. - **Files:** `pipeline.go`, `writer.go`, `state_apply.go`, `report.go`. diff --git a/internal/cli/dest_fifo_e2e_unix_test.go b/internal/cli/dest_fifo_e2e_unix_test.go index baaf9566..c22ff5e1 100644 --- a/internal/cli/dest_fifo_e2e_unix_test.go +++ b/internal/cli/dest_fifo_e2e_unix_test.go @@ -75,16 +75,15 @@ func TestCommandsDoNotHangOnNonRegularDestination(t *testing.T) { } t.Cleanup(func() { _ = os.Remove(dest.path) }) + // Only the three commands this change actually fixes. `apply`, + // `apply --dry-run` and `import ` still hang on this exact + // fixture — their reads are in internal/render and the adapter + // Ingest paths, which this gate does not cover (issues #241, #242). + // Asserting them here would be asserting a bug. for _, args := range [][]string{ {"status"}, {"diff"}, {"reconcile", "--auto-safe"}, - {"import", "--dry-run"}, - // A REAL import too: importRun returns early on --dry-run - // (import.go:331), so the state-seeding destination reads in - // seedStateFromCurrentDest and unimportedDestPointers are only - // reachable without it. - {"import"}, } { t.Run(strings.Join(args, " "), func(t *testing.T) { // Exit status is deliberately not asserted: a non-regular @@ -116,6 +115,15 @@ func runBounded(t *testing.T, d time.Duration, args ...string) string { }() select { case out := <-done: + // Anti-vacuity. An earlier version of this test ran `import` with no + // agent selector; cobra rejected it at ExactArgs(1) before RunE, so the + // row executed zero lines of the code it named and passed identically + // with the fix reverted. A command that never starts cannot hang, so + // "it returned" is only meaningful once we know it ran. + if strings.Contains(out, "arg(s), received") || strings.Contains(out, "unknown command") { + t.Fatalf("`agentsync %s` never reached its RunE — cobra rejected the invocation: %s", + strings.Join(args, " "), strings.TrimSpace(out)) + } return out case <-time.After(d): t.Fatalf("`agentsync %s` BLOCKED on a non-regular destination — os.ReadFile on a "+ diff --git a/internal/cli/destread.go b/internal/cli/destread.go index a5f72ec9..0e89a844 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -14,8 +14,8 @@ import ( // double it in the wrapped message. var errDestNotRegular = errors.New("not a regular file") -// readDestBytes reads a destination file's bytes, refusing any path that is -// present but not a regular file BEFORE the open. +// readDestBytes reads a destination file's bytes, refusing before the open any +// path whose shape cannot be read as a file. // // The guard is not defensive tidying. os.ReadFile on a FIFO does not fail — it // BLOCKS in the open, waiting for a writer that never comes, so no error path @@ -25,16 +25,36 @@ var errDestNotRegular = errors.New("not a regular file") // ~/.claude.json, say) wedged `status` — which is advertised as read-only — // through the shared readDestFile. // -// render.hashFile already applied exactly this rule, and said so: "Shares -// render's predicate so the destination-read guards cannot disagree about what -// is safe to read." That was true of the HASH and false of every other -// destination read, which is the gap this closes. Same predicate, so the -// statement is now true of all of them. +// This gate covers THIS PACKAGE only. `apply`, `apply --dry-run` and +// `import ` still hang on the same fixture; their reads live in +// internal/render and the adapter Ingest paths (#241, #242). // -// An ABSENT path is not refused: render.IsRegularOrAbsent passes it through to -// os.ReadFile, whose ENOENT is the truthful answer and is what every caller -// already handles. Manufacturing a shape error for a file that is not there -// would name the wrong problem. +// cli.hashFile already applied exactly this rule, and said so: "Shares render's +// predicate so the destination-read guards cannot disagree about what is safe +// to read." That was true of the HASH and false of every other destination +// read. hashFile now calls this function instead of holding a second copy, so +// within this package that sentence describes a property rather than a +// coincidence. +// +// It does NOT hold across the symlink axis, and this comment previously claimed +// it did. hashFile Lstats first and refuses a symlink outright with its own +// sentinel; this function does not, so a read that reaches os.ReadFile follows +// the link. `status` therefore calls a symlinked destination drifted while +// `diff` reads through it and compares the target. That divergence predates the +// gate — every one of these reads was a bare os.ReadFile, which follows links +// too — and is deliberately left alone, because AGENTSYNC_ALLOW_SYMLINK_DEST=1 +// is a documented, supported setup in which apply writes THROUGH the link, so +// refusing links here would break it. Reconciling the two is a behavior +// decision, tracked with the drift-walk unification in #229. +// +// An ABSENT path is not refused: render.IsRegularOrAbsent reports absent as +// acceptable, so os.ReadFile runs and its ENOENT reaches the caller unchanged. +// Manufacturing a shape error for a file that is not there would name the wrong +// problem. Note the predicate also answers false for a stat failure that is NOT +// ENOENT (EACCES on a parent, ELOOP), so those surface as errDestNotRegular +// rather than as themselves — imprecise, but in the safe direction, and it +// keeps this function's answer identical to the one hashFile gave before it was +// folded in. func readDestBytes(path string) ([]byte, error) { if !render.IsRegularOrAbsent(path) { return nil, errDestNotRegular diff --git a/internal/cli/destread_guard_internal_test.go b/internal/cli/destread_guard_internal_test.go index d82ab126..b8342d1b 100644 --- a/internal/cli/destread_guard_internal_test.go +++ b/internal/cli/destread_guard_internal_test.go @@ -6,8 +6,9 @@ import ( ) // TestEveryDestinationReadGoesThroughTheGate enforces the invariant that makes -// the FIFO guard hold: no production code in this package reads a destination -// path with a bare os.ReadFile. +// the FIFO guard hold: no production code in this package reads an op's +// destination path with a bare os.ReadFile. It is an invariant over the two +// spellings below, not over every possible one — see LIMITS. // // Why an invariant and not just the behavior tests: the four reads that were // MEASURED to hang (readDestFile, diff's and reconcile's per-op reads, and @@ -25,7 +26,13 @@ import ( // - The pattern is TEXT-shaped. A read spelled through a variable // (`p := op.Path; os.ReadFile(p)`), via afero, or with os.Open + io.ReadAll // slips past. It catches the copy-paste that actually happened seven times, -// not every conceivable spelling. +// not every conceivable spelling. Concretely: reverting readDestFile's own +// read to `os.ReadFile(path)` is NOT caught here — the behavior tests are +// what cover that one. +// - It is scoped to THIS PACKAGE. `apply` and `import ` still hang on +// a non-regular destination through internal/render and the adapter Ingest +// paths (#241, #242); this guard says nothing about them, and passing it +// does not mean agentsync as a whole is safe. // - It scans comments and string literals too, so a mention of the pattern in // prose would trip it. That is the safe direction to fail. func TestEveryDestinationReadGoesThroughTheGate(t *testing.T) { @@ -34,6 +41,31 @@ func TestEveryDestinationReadGoesThroughTheGate(t *testing.T) { "os.ReadFile(op.Path)", "os.ReadFile(it.op.Path)", } + match := func(src string) string { + for _, pat := range forbidden { + if strings.Contains(src, pat) { + return pat + } + } + return "" + } + + // Negative control, run every time and not only when the tree is dirty: + // a guard that passes whatever the tree looks like is worse than none, + // because it reads as coverage. Both prior guards in this package do the + // same. This one drives the REAL matcher over synthetic sources — if the + // matcher is ever narrowed or a pattern is dropped, this fails here rather + // than silently stopping the repo walk from finding anything. + for _, pat := range forbidden { + if got := match("func f() { data, _ := " + pat + " }"); got != pat { + t.Fatalf("negative control: matcher missed the planted %q (got %q) — "+ + "the repo walk below cannot be trusted", pat, got) + } + } + if got := match("func f() { data, _ := readDestBytes(op.Path) }"); got != "" { + t.Fatalf("negative control: matcher flagged the CORRECT spelling as %q — "+ + "it would fail on a compliant tree", got) + } repoRoot := repoRootFromCaller(t) scanned := 0 @@ -47,13 +79,11 @@ func TestEveryDestinationReadGoesThroughTheGate(t *testing.T) { return } scanned++ - for _, pat := range forbidden { - if strings.Contains(src, pat) { - t.Errorf("%s contains %q — destination reads must go through readDestBytes "+ - "(internal/cli/destread.go), which refuses a non-regular path BEFORE the "+ - "open. os.ReadFile on a FIFO does not fail, it BLOCKS, so the read's own "+ - "error path never runs and the command never returns.", rel, pat) - } + if pat := match(src); pat != "" { + t.Errorf("%s contains %q — destination reads must go through readDestBytes "+ + "(internal/cli/destread.go), which refuses a non-regular path BEFORE the "+ + "open. os.ReadFile on a FIFO does not fail, it BLOCKS, so the read's own "+ + "error path never runs and the command never returns.", rel, pat) } }); err != nil { t.Fatal(err) diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index cde6354f..749dcb59 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -6,6 +6,7 @@ import ( "errors" "os" "path/filepath" + "strings" "syscall" "testing" "time" @@ -152,6 +153,19 @@ func TestKeyMergeAndWriteBackReadsAreGuarded(t *testing.T) { if err == nil { t.Fatal("writeBackFileItem = nil, want an error: a FIFO carries no dest content to write back") } + // The sentinel is pathless on the rationale that every caller wraps + // it with the path itself. This is the caller that does, and the one + // whose message a user reads mid-prompt, so both halves are pinned + // here — otherwise dropping the wrap leaves the suite green and the + // rationale untested. + if !strings.Contains(err.Error(), p) { + t.Errorf("error = %q, want it to name the destination %q: errDestNotRegular "+ + "carries no path, so this caller must supply it", err, p) + } + if !strings.Contains(err.Error(), "[o]verride") || !strings.Contains(err.Error(), "[i]gnore") { + t.Errorf("error = %q, want a next step: the user is mid-prompt choosing a "+ + "keystroke, and this function's other refusals all name one", err) + } case <-time.After(5 * time.Second): t.Fatalf("writeBackFileItem BLOCKED on a FIFO destination (%s) — [w] must refuse, "+ "not wedge the reconcile session", p) diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index ce465080..85366f7e 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -1153,7 +1153,11 @@ func writeBackKeyItem(cmd *cobra.Command, home string, it reconcileItem) error { func writeBackFileItem(home string, it reconcileItem) error { data, err := readDestBytes(it.op.Path) if err != nil { - return fmt.Errorf("read dest %s: %w", it.op.Path, err) + // 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. + return fmt.Errorf("read dest %s: %w — use [o]verride to push canonical to the dest, "+ + "or [i]gnore to suppress this item", it.op.Path, err) } srcID := it.op.SourceID if srcID == "" { diff --git a/internal/cli/status.go b/internal/cli/status.go index df7e61e1..933b5814 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -985,13 +986,18 @@ func hashFile(path string) string { // 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. Shares render's predicate so the destination-read guards cannot - // disagree about what is safe to read. - if !render.IsRegularOrAbsent(path) { - return "not-a-regular-file" - } - data, err := os.ReadFile(path) + // equality. + // + // The shape rule is NOT applied here: it comes from readDestBytes, the one + // gate every destination read in this package passes through. This function + // used to hold a second copy of it, which is the duplication the gate exists + // to remove — and while the two copies agreed, "the guards cannot disagree" + // was a claim rather than a property. + data, err := readDestBytes(path) if err != nil { + if errors.Is(err, errDestNotRegular) { + return "not-a-regular-file" + } return "" } return hashContent(data) From 54b01996b93a698f14c68ec8cbb4c3bda4a22f22 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 21:09:09 +0000 Subject: [PATCH 3/9] fix(cli): make the anti-vacuity check able to fire; stop pointing at a hang MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes round-2 review findings on PR #240. Both blockers were mine, and both are the same defect as round 1's, re-made inside the fix for it. 1. The anti-vacuity check added last round could NEVER FIRE. NewRoot sets SilenceErrors, so cobra prints nothing when it rejects an invocation — the text exists only in Execute()'s returned error, which runBounded discarded with `_ = root.Execute()`. Measured: `import` -> buffer "", error "accepts 1 arg(s), received 0". So the guard written to stop a vacuous row was itself vacuous, and `runBounded(t, 8s, "import")` still passed silently. Worse, the buffer carries ordinary stdout, so the substring match was live against user content: a rendered file containing the words "unknown command" failed a plain `diff`. Inert against its target, false-positive against real output. It now reads the returned error through isCobraRejection, and TestRunBoundedRejectsAnInvocationCobraRefuses is the positive control — the check only ever fires on a broken invocation, so without one nothing in a green suite proves it still works. That absence is exactly how the first version shipped. 2. `reconcile --auto-override` HANGS (measured rc=124): [o] re-applies through render.Writer.Write, whose convergence read is the unguarded one #241 covers. Two consequences, both introduced by this branch: - the CHANGELOG headline claimed `reconcile` was fixed, while the e2e only ever exercised --auto-safe, which never reaches that branch. Test and claim agreed with each other rather than with the code. - the guidance added LAST round told the user to "use [o]verride", walking them out of a clean refusal into an unbounded wedge. It now names the remedy that works, and a test asserts the message does NOT recommend [o] — the peer refusals in that file all do, so symmetry would quietly restore it. 3. Stale prose, instance #6: the guard test still said "with every guard removed `agentsync import` still returned", the sentence resting on the no-selector measurement — contradicted by its own LIMITS fifteen lines below. Rewritten: import's hang is real but upstream in the adapter Ingest reads (#242), so guarding these three sites neither fixed it nor could have. Instance #7: render.IsRegularOrAbsent's doc still named hashFile as its outside consumer. 4. Two behaviors were UNPINNED — the suite stayed green with hashFile returning the shape sentinel for every error (absent included, a branch its own suite takes 43 times) and green again with the symlink arm deleted, despite destread.go asserting that divergence in prose. TestHashFileSentinels pins all three sentinels and both halves of the symlink asymmetry. 5. The guard's negative control was tautological: built from the same slice it checked, so it could not notice a pattern being dropped. It now plants literals; break-verified by removing a pattern. Known-hanging commands are now SKIPPED rows rather than absent ones — greppable, visible in -v, and whoever closes #241/#242 deletes one line to inherit the assertion. Asserting the hang would cost 8s a row and fail when it is fixed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 11 +- docs/components.md | 3 +- internal/cli/dest_fifo_e2e_unix_test.go | 122 ++++++++++++++++--- internal/cli/destread.go | 22 ++-- internal/cli/destread_guard_internal_test.go | 41 ++++--- internal/cli/destread_unix_internal_test.go | 122 ++++++++++++++++++- internal/cli/reconcile.go | 12 +- internal/cli/status.go | 8 +- internal/render/writer.go | 3 +- 9 files changed, 285 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a600c6b..8657c8de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ source layout, CLI surface, and state schema are stabilizing but may still chang ### Fixed - **A FIFO, directory, or other non-regular file at a managed destination no - longer hangs `status`, `diff` or `reconcile`.** `os.ReadFile` on a FIFO does + longer hangs `status`, `diff`, or `reconcile`'s drift walk and write-back.** `os.ReadFile` on a FIFO does not fail — it blocks in the open waiting for a writer that never comes — so the read's own error path never runs and the command never returns. Measured on the previous release: a FIFO at a whole-file destination wedged `diff` and @@ -32,9 +32,12 @@ source layout, CLI surface, and state schema are stabilizing but may still chang a non-regular destination would classify as drift and then hang one keystroke later. - **`apply`, `apply --dry-run` and `import ` are NOT fixed by this** and - still hang on the same fixture — their reads are in `internal/render` and the - adapter `Ingest` paths, a far wider sweep. Tracked as + **`apply`, `apply --dry-run`, `reconcile`'s `[o]verride` and `import ` + are NOT fixed by this** and still hang on the same fixture — their reads are + in `internal/render` and the adapter `Ingest` paths, a far wider sweep. + `[o]verride` re-applies through `render.Writer.Write`, so it shares `apply`'s + unguarded read; the refusal message therefore points at removing or replacing + the file rather than at `[o]`, which would wedge. Tracked as [#241](https://github.com/spxrogers/agentsync/issues/241) and [#242](https://github.com/spxrogers/agentsync/issues/242). diff --git a/docs/components.md b/docs/components.md index b8601e6e..809b2f86 100644 --- a/docs/components.md +++ b/docs/components.md @@ -399,7 +399,8 @@ symmetric with the dest→source write boundary (see architecture §7). written ahead of the code: until then only `status`'s `hashFile` used the predicate, and `diff`, `reconcile` and the shared key-merge read all blocked.) It is **not** yet true of this package's own `Writer.Write` convergence read - or of the adapter `Ingest` paths, so `apply`, `apply --dry-run` and + or of the adapter `Ingest` paths, so `apply`, `apply --dry-run`, + `reconcile --auto-override` (which re-applies through `Writer.Write`) and `import ` still block on a non-regular destination — issues #241 and #242. - **Depends on:** adapter, secrets, source, state, paths, iox, drift. diff --git a/internal/cli/dest_fifo_e2e_unix_test.go b/internal/cli/dest_fifo_e2e_unix_test.go index c22ff5e1..743e0223 100644 --- a/internal/cli/dest_fifo_e2e_unix_test.go +++ b/internal/cli/dest_fifo_e2e_unix_test.go @@ -75,17 +75,30 @@ func TestCommandsDoNotHangOnNonRegularDestination(t *testing.T) { } t.Cleanup(func() { _ = os.Remove(dest.path) }) - // Only the three commands this change actually fixes. `apply`, - // `apply --dry-run` and `import ` still hang on this exact - // fixture — their reads are in internal/render and the adapter - // Ingest paths, which this gate does not cover (issues #241, #242). - // Asserting them here would be asserting a bug. - for _, args := range [][]string{ - {"status"}, - {"diff"}, - {"reconcile", "--auto-safe"}, + // The first group is what this change fixes. The second still hangs + // on this exact fixture and is SKIPPED rather than asserted or + // omitted: asserting the hang costs 8s a row and would start + // failing the day it is fixed, reading as a regression; omitting it + // leaves nothing to find. A skip is greppable, shows up in -v, and + // the person who closes the issue deletes one line to inherit a + // ready-made assertion. + for _, tc := range []struct { + args []string + skip string + }{ + {args: []string{"status"}}, + {args: []string{"diff"}}, + {args: []string{"reconcile", "--auto-safe"}}, + {args: []string{"apply", "--dry-run"}, skip: "#241: render.Writer.Write's convergence read is unguarded"}, + {args: []string{"apply"}, skip: "#241: render.Writer.Write's convergence read is unguarded"}, + {args: []string{"reconcile", "--auto-override"}, skip: "#241: [o]verride queues into render.Writer.Write"}, + {args: []string{"import", "claude"}, skip: "#242: the adapter Ingest reads are unguarded"}, } { + args := tc.args t.Run(strings.Join(args, " "), func(t *testing.T) { + if tc.skip != "" { + t.Skip(tc.skip) + } // Exit status is deliberately not asserted: a non-regular // destination may legitimately report drift, or refuse. The // contract under test is that the command RETURNS. @@ -103,28 +116,41 @@ func TestCommandsDoNotHangOnNonRegularDestination(t *testing.T) { func runBounded(t *testing.T, d time.Duration, args ...string) string { t.Helper() detachSlog(t) - done := make(chan string, 1) + type result struct { + out string + err error + } + done := make(chan result, 1) go func() { var buf bytes.Buffer root := cli.NewRoot() root.SetOut(&buf) root.SetErr(&buf) root.SetArgs(args) - _ = root.Execute() - done <- buf.String() + err := root.Execute() + done <- result{buf.String(), err} }() select { - case out := <-done: + case r := <-done: // Anti-vacuity. An earlier version of this test ran `import` with no // agent selector; cobra rejected it at ExactArgs(1) before RunE, so the // row executed zero lines of the code it named and passed identically // with the fix reverted. A command that never starts cannot hang, so // "it returned" is only meaningful once we know it ran. - if strings.Contains(out, "arg(s), received") || strings.Contains(out, "unknown command") { - t.Fatalf("`agentsync %s` never reached its RunE — cobra rejected the invocation: %s", - strings.Join(args, " "), strings.TrimSpace(out)) + // + // The check MUST read the returned error, not the output buffer. + // NewRoot sets SilenceErrors, so cobra prints nothing on a rejection — + // the first version of this guard inspected the buffer, could therefore + // never fire, and was itself the bug it was written to prevent. Worse, + // the buffer carries ordinary stdout, so matching these substrings + // against it made a plain `diff` fail whenever a rendered file happened + // to contain the words. TestRunBoundedRejectsAnInvocationCobraRefuses is + // the positive control that keeps this honest. + if r.err != nil && isCobraRejection(r.err) { + t.Fatalf("`agentsync %s` never reached its RunE — cobra rejected the invocation: %v", + strings.Join(args, " "), r.err) } - return out + return r.out case <-time.After(d): t.Fatalf("`agentsync %s` BLOCKED on a non-regular destination — os.ReadFile on a "+ "FIFO waits for a writer that never comes, so the read's own error path never "+ @@ -132,3 +158,65 @@ func runBounded(t *testing.T, d time.Duration, args ...string) string { return "" } } + +// isCobraRejection reports whether err is cobra refusing the invocation itself +// — a wrong argument count, an unknown command or flag — as opposed to a real +// failure from inside the command. +// +// It matches the returned error only. NewRoot sets SilenceErrors, so none of +// this text is ever printed, which is what made the first version of this check +// (written against the output buffer) unable to fire. +func isCobraRejection(err error) bool { + msg := err.Error() + for _, marker := range []string{ + "arg(s), received", + "unknown command", + "unknown flag", + "unknown shorthand flag", + "requires at least", + "accepts at most", + } { + if strings.Contains(msg, marker) { + return true + } + } + return false +} + +// TestRunBoundedRejectsAnInvocationCobraRefuses is the positive control for the +// anti-vacuity check in runBounded. Without it that check is unfalsifiable: it +// only ever fires on a broken invocation, so nothing in a green suite proves it +// still works, and its first version silently never fired at all. +// +// It asserts on isCobraRejection rather than by calling runBounded, because +// runBounded signals failure with t.Fatalf — driving it with a bad invocation +// would fail this test rather than pass it. +func TestRunBoundedRejectsAnInvocationCobraRefuses(t *testing.T) { + // Exactly the invocations that silently passed before: `import` without the + // agent selector its ExactArgs(1) requires, and an outright bad command. + for _, args := range [][]string{{"import"}, {"nosuchcommand"}} { + t.Run(strings.Join(args, " "), func(t *testing.T) { + var buf bytes.Buffer + root := cli.NewRoot() + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs(args) + err := root.Execute() + if err == nil { + t.Fatalf("`agentsync %s` returned nil; expected cobra to refuse it", + strings.Join(args, " ")) + } + if !isCobraRejection(err) { + t.Errorf("isCobraRejection(%q) = false, want true — runBounded would let this "+ + "invocation pass as if the command had run", err) + } + // The reason the check reads the error and not the buffer. + if strings.Contains(buf.String(), "arg(s), received") || + strings.Contains(buf.String(), "unknown command") { + t.Errorf("cobra printed its rejection into the output buffer (%q) — if that ever "+ + "becomes true, the simpler buffer-based check would work and this "+ + "indirection can go", buf.String()) + } + }) + } +} diff --git a/internal/cli/destread.go b/internal/cli/destread.go index 0e89a844..635395fe 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -29,23 +29,19 @@ var errDestNotRegular = errors.New("not a regular file") // `import ` still hang on the same fixture; their reads live in // internal/render and the adapter Ingest paths (#241, #242). // -// cli.hashFile already applied exactly this rule, and said so: "Shares render's -// predicate so the destination-read guards cannot disagree about what is safe -// to read." That was true of the HASH and false of every other destination -// read. hashFile now calls this function instead of holding a second copy, so -// within this package that sentence describes a property rather than a -// coincidence. +// Every destination read in this package routes here, cli.hashFile included, so +// they cannot disagree about what is safe to read. // -// It does NOT hold across the symlink axis, and this comment previously claimed -// it did. hashFile Lstats first and refuses a symlink outright with its own -// sentinel; this function does not, so a read that reaches os.ReadFile follows -// the link. `status` therefore calls a symlinked destination drifted while -// `diff` reads through it and compares the target. That divergence predates the -// gate — every one of these reads was a bare os.ReadFile, which follows links -// too — and is deliberately left alone, because AGENTSYNC_ALLOW_SYMLINK_DEST=1 +// They DO still disagree about symlinks, deliberately. hashFile Lstats first and +// refuses a link outright with its own sentinel; this function does not, so a +// read that reaches os.ReadFile follows it. `status` therefore calls a symlinked +// destination drifted while `diff` reads through it and compares the target. +// That predates this gate — the reads it replaced were bare os.ReadFile, which +// follows links too — and is left alone because AGENTSYNC_ALLOW_SYMLINK_DEST=1 // is a documented, supported setup in which apply writes THROUGH the link, so // refusing links here would break it. Reconciling the two is a behavior // decision, tracked with the drift-walk unification in #229. +// TestHashFileSentinels asserts both halves. // // An ABSENT path is not refused: render.IsRegularOrAbsent reports absent as // acceptable, so os.ReadFile runs and its ENOENT reaches the caller unchanged. diff --git a/internal/cli/destread_guard_internal_test.go b/internal/cli/destread_guard_internal_test.go index b8342d1b..6942f6ac 100644 --- a/internal/cli/destread_guard_internal_test.go +++ b/internal/cli/destread_guard_internal_test.go @@ -10,17 +10,20 @@ import ( // destination path with a bare os.ReadFile. It is an invariant over the two // spellings below, not over every possible one — see LIMITS. // -// Why an invariant and not just the behavior tests: the four reads that were -// MEASURED to hang (readDestFile, diff's and reconcile's per-op reads, and -// writeBackFileItem) are each pinned by a timeout-bounded test. The three in -// import's state-seeding path are not — no fixture was found that reaches them -// with a non-regular destination, and with every guard removed `agentsync -// import` still returned. They were routed through the gate anyway, because -// three structurally identical unguarded reads sitting beside four guarded ones -// invite exactly the question "why those and not these", and the honest answer -// — "nobody found a fixture yet" — is not a reason to leave them. This test is -// what keeps that decision from silently rotting: a new bare destination read -// fails here even where no hang can be demonstrated. +// Why an invariant and not just the behavior tests: the four reads MEASURED to +// hang (readDestFile, diff's and reconcile's per-op reads, and writeBackFileItem) +// are each pinned by a timeout-bounded test. The three in import's state-seeding +// path are not, and cannot be from here — `agentsync import ` does hang +// on a non-regular destination, but upstream of these sites, in the adapter +// Ingest reads this gate does not cover (#242). Guarding them neither fixed that +// nor could have. +// +// They were routed through the gate anyway, because three structurally +// identical unguarded reads sitting beside four guarded ones invite exactly the +// question "why those and not these", and there is no answer that survives +// being written down. This test is what keeps that decision from rotting: a new +// bare destination read fails here even where no hang is demonstrable at the +// site itself. // // LIMITS, stated so a reader does not over-trust it: // - The pattern is TEXT-shaped. A read spelled through a variable @@ -56,10 +59,18 @@ func TestEveryDestinationReadGoesThroughTheGate(t *testing.T) { // same. This one drives the REAL matcher over synthetic sources — if the // matcher is ever narrowed or a pattern is dropped, this fails here rather // than silently stopping the repo walk from finding anything. - for _, pat := range forbidden { - if got := match("func f() { data, _ := " + pat + " }"); got != pat { - t.Fatalf("negative control: matcher missed the planted %q (got %q) — "+ - "the repo walk below cannot be trusted", pat, got) + // The planted sources are LITERALS, deliberately not built from `forbidden`. + // A control assembled out of the same slice it is checking is tautological: + // it passes by construction and cannot notice a pattern being dropped, which + // is the most likely way this guard dies. + for _, planted := range []string{ + "func f() { data, _ := os.ReadFile(op.Path) }", + "func f(it reconcileItem) { data, _ := os.ReadFile(it.op.Path) }", + } { + if got := match(planted); got == "" { + t.Fatalf("negative control: matcher missed a planted bare destination read in %q — "+ + "a pattern has been dropped from `forbidden`, and the repo walk below "+ + "cannot be trusted", planted) } } if got := match("func f() { data, _ := readDestBytes(op.Path) }"); got != "" { diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index 749dcb59..3e9dfc0c 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -162,10 +162,21 @@ func TestKeyMergeAndWriteBackReadsAreGuarded(t *testing.T) { t.Errorf("error = %q, want it to name the destination %q: errDestNotRegular "+ "carries no path, so this caller must supply it", err, p) } - if !strings.Contains(err.Error(), "[o]verride") || !strings.Contains(err.Error(), "[i]gnore") { + if !strings.Contains(err.Error(), "[i]gnore") || !strings.Contains(err.Error(), "re-run") { t.Errorf("error = %q, want a next step: the user is mid-prompt choosing a "+ "keystroke, and this function's other refusals all name one", err) } + // The remedy must not be one that hangs. This function's peers all + // offer [o]verride, and an earlier version of this message copied + // them — but [o] re-applies through render.Writer.Write's unguarded + // convergence read, so on a non-regular destination it wedges + // instead of failing (#241). Suggesting it is worse than saying + // nothing, and this assertion is what stops it coming back by + // symmetry with the neighbours. + if strings.Contains(err.Error(), "[o]verride") { + t.Errorf("error = %q recommends [o]verride, which HANGS on a non-regular "+ + "destination (#241) — offer it again only once that is fixed", err) + } case <-time.After(5 * time.Second): t.Fatalf("writeBackFileItem BLOCKED on a FIFO destination (%s) — [w] must refuse, "+ "not wedge the reconcile session", p) @@ -186,3 +197,112 @@ func mkfifoDest(t *testing.T, tmp string) string { } return p } + +// TestHashFileSentinels pins hashFile's three-way answer, which every drift +// verdict in this package is built on. The values are opaque — callers only +// ever compare them for equality — so a changed sentinel does not fail a build +// or a type check. It silently changes what drift.Classify decides, at four +// call sites. +// +// Two of these rows cover behavior that a mutation sweep found UNPINNED: the +// whole suite stayed green with hashFile returning the shape sentinel for every +// error (absent included), and green again with the symlink arm removed +// entirely. +func TestHashFileSentinels(t *testing.T) { + cases := []struct { + name string + setup func(t *testing.T, tmp string) string + want string + }{ + { + // The absent contract. "" is what drift.Classify reads as "no + // destination", which is what makes an unrendered-but-recorded file + // an Orphan rather than drift. Returning the shape sentinel here + // instead would silently reclassify every absent destination — and + // the ENOENT branch is taken 43 times in this package's own suite, + // none of which asserted the value. + name: "an absent destination hashes to the empty sentinel", + setup: func(t *testing.T, tmp string) string { return filepath.Join(tmp, "gone") }, + 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", + setup: func(t *testing.T, tmp string) string { + t.Helper() + 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 + }, + want: "symlink-not-regular-file", + }, + { + name: "a FIFO is refused by shape", + setup: mkfifoDest, + want: "not-a-regular-file", + }, + { + name: "an ordinary regular file hashes its content", + setup: func(t *testing.T, tmp string) string { + t.Helper() + p := filepath.Join(tmp, "dest") + if err := os.WriteFile(p, []byte("payload"), 0o644); err != nil { + t.Fatal(err) + } + return p + }, + want: hashContent([]byte("payload")), + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + path := tc.setup(t, t.TempDir()) + // Bounded for the same reason as everything else here: a regression + // that reintroduces an unguarded read hangs rather than fails. + got := make(chan string, 1) + go func() { got <- hashFile(path) }() + select { + 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", + h, tc.want) + } + case <-time.After(5 * time.Second): + t.Fatalf("hashFile BLOCKED on %s", path) + } + }) + } + + // 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. + t.Run("readDestBytes follows the symlink hashFile refuses", func(t *testing.T) { + 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") + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + 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) + } + }) +} diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index 85366f7e..a50ba9f4 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -1156,8 +1156,16 @@ func writeBackFileItem(home string, it reconcileItem) error { // 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. - return fmt.Errorf("read dest %s: %w — use [o]verride to push canonical to the dest, "+ - "or [i]gnore to suppress this item", it.op.Path, err) + // + // [o]verride is deliberately NOT offered, unlike the peer refusals in + // this file. 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 + // user out of a clean refusal and into an unbounded wedge. Restore that + // suggestion only once #241 is fixed. + return fmt.Errorf("read dest %s: %w — remove or replace the non-regular file at that "+ + "path and re-run, or [i]gnore to suppress this item", it.op.Path, err) } srcID := it.op.SourceID if srcID == "" { diff --git a/internal/cli/status.go b/internal/cli/status.go index 933b5814..83c65577 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -988,11 +988,9 @@ func hashFile(path string) string { // symlink; both are opaque to callers, which only ever compare hashes for // equality. // - // The shape rule is NOT applied here: it comes from readDestBytes, the one - // gate every destination read in this package passes through. This function - // used to hold a second copy of it, which is the duplication the gate exists - // to remove — and while the two copies agreed, "the guards cannot disagree" - // was a claim rather than a property. + // 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. data, err := readDestBytes(path) if err != nil { if errors.Is(err, errDestNotRegular) { diff --git a/internal/render/writer.go b/internal/render/writer.go index a87db052..3d00ce52 100644 --- a/internal/render/writer.go +++ b/internal/render/writer.go @@ -366,7 +366,8 @@ func OrphanDeleteWillProceed(op adapter.FileOp) bool { } // IsRegularOrAbsent is the exported view of isRegularOrAbsent, for the sibling -// destination reads outside this package (internal/cli's hashFile). They face +// destination reads outside this package (internal/cli's readDestBytes, the one +// gate every destination read in that package passes through). They face // the identical hazard and must not answer it differently. func IsRegularOrAbsent(path string) bool { return isRegularOrAbsent(path) } From 37d8aafee325c6ac1f71e667542091d185dcc35d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 21:44:14 +0000 Subject: [PATCH 4/9] fix(cli): observe that a command ran; match the remedy to the failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes round-3 review findings on PR #240. 1. The anti-vacuity check was still inferring. It matched substrings against cobra's error prose, which covers only the arg/flag layer — anything rejecting later scored as "it ran", including this repo's own enforceScopeStance, a PersistentPreRunE refusal that never reaches RunE. runBounded now WRAPS the resolved command's RunE, so "did the body start" is observed rather than deduced, and cannot drift with cobra's wording. Third version of this check; the first two were unable to fire at all. It is also falsifiable now. runBoundedE reports instead of failing, so TestRunBoundedDetectsACommandThatNeverRan can assert ran==false for a missing argument, an unknown command AND a PersistentPreRunE refusal — the case the substring list structurally could not catch — plus ran==true for a command that does run. Previously nothing failed if the check were reverted. 2. Subtest fixture bleed. The cleanup unlinked the FIFO but never restored the applied file, so the key-merge subtest ran against a home whose whole-file destination was missing, contradicting the test's own "a real applied home". Measured: with the skips deleted, `import claude` and `reconcile --auto-override` PASSED in a full run and HUNG in isolation — whoever closes #241/#242 would have inherited a row green for the wrong reason. The destination is now restored, and after the fix the row hangs both ways. 3. writeBackFileItem appended "remove or replace the non-regular file at that path" to EVERY read failure, including ENOENT. Deleting a managed file is itself drift and offers [w], so the common path produced "no such file or directory — remove or replace the non-regular file at that path": advice for a situation the user is not in. Gated on errors.Is(err, errDestNotRegular), with a test for the absent case. Third round running that this one message has been the site of a new defect. 4. Prose, instances #8 and #9. The CHANGELOG headline said a "directory" no longer hangs — a directory never hung (os.ReadFile fails it in ~18us with EISDIR); only the diagnosis changes. And destread.go justified leaving the symlink split by claiming AGENTSYNC_ALLOW_SYMLINK_DEST=1 would break, but that variable is read only in internal/iox, on the WRITE path, so a read gate cannot affect it. The real reason is that changing it changes what diff and reconcile have always reported (#229) — and the real consequence, now named, is that under that supported setup `status` reports drift no apply can clear. 5. The hash row computed its expectation with the function under test; salting hashContent left it green. Pinned to a literal digest, break-verified. Also: docs/components.md's "Enforced, not asserted" overstated a two-spelling text matcher whose own LIMITS exempt a read; softened, and the review-audit parenthetical that had survived into a website-mirrored contract page is gone. destread.go now documents that `diff` and readDestFile swallow the refusal and render a refused destination as empty — a poorer diagnosis than it deserves, left to #229 because fixing it changes what those commands print. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 6 +- docs/components.md | 8 +- internal/cli/dest_fifo_e2e_unix_test.go | 197 ++++++++++++-------- internal/cli/destread.go | 36 +++- internal/cli/destread_unix_internal_test.go | 29 ++- internal/cli/reconcile.go | 11 +- 6 files changed, 190 insertions(+), 97 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8657c8de..6bca1dbd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,8 +11,10 @@ source layout, CLI surface, and state schema are stabilizing but may still chang ### Fixed -- **A FIFO, directory, or other non-regular file at a managed destination no - longer hangs `status`, `diff`, or `reconcile`'s drift walk and write-back.** `os.ReadFile` on a FIFO does +- **A FIFO at a managed destination no longer hangs `status`, `diff`, or + `reconcile`'s drift walk and write-back.** (A directory there never hung — + `os.ReadFile` fails it immediately with `EISDIR` — but it is now refused with + the same shape error instead of that surfacing from deeper in the decode.) `os.ReadFile` on a FIFO does not fail — it blocks in the open waiting for a writer that never comes — so the read's own error path never runs and the command never returns. Measured on the previous release: a FIFO at a whole-file destination wedged `diff` and diff --git a/docs/components.md b/docs/components.md index 809b2f86..f7895ccd 100644 --- a/docs/components.md +++ b/docs/components.md @@ -394,10 +394,10 @@ symmetric with the dest→source write boundary (see architecture §7). is shared with `internal/cli`'s destination reads so a FIFO cannot block **those**: every one of them goes through `readDestBytes` (`internal/cli/destread.go`), which applies this predicate before the open. - Enforced, not asserted — `TestEveryDestinationReadGoesThroughTheGate` fails on - a bare `os.ReadFile(op.Path)` anywhere under `internal/cli`. (The claim was - written ahead of the code: until then only `status`'s `hashFile` used the - predicate, and `diff`, `reconcile` and the shared key-merge read all blocked.) + `TestEveryDestinationReadGoesThroughTheGate` backs this up by failing on a + bare `os.ReadFile(op.Path)` anywhere under `internal/cli` — a two-spelling + text matcher, so it catches the copy-paste that happened rather than every + possible spelling. It is **not** yet true of this package's own `Writer.Write` convergence read or of the adapter `Ingest` paths, so `apply`, `apply --dry-run`, `reconcile --auto-override` (which re-applies through `Writer.Write`) and diff --git a/internal/cli/dest_fifo_e2e_unix_test.go b/internal/cli/dest_fifo_e2e_unix_test.go index 743e0223..10dbe15a 100644 --- a/internal/cli/dest_fifo_e2e_unix_test.go +++ b/internal/cli/dest_fifo_e2e_unix_test.go @@ -11,6 +11,8 @@ import ( "testing" "time" + "github.com/spf13/cobra" + "github.com/spxrogers/agentsync/internal/cli" ) @@ -66,14 +68,37 @@ func TestCommandsDoNotHangOnNonRegularDestination(t *testing.T) { if _, err := os.Stat(dest.path); err != nil { t.Fatalf("fixture never applied %s: %v — this test would pass vacuously", dest.path, err) } - // Swap the applied destination for a FIFO. Nothing here opens it. + // Swap the applied destination for a FIFO, and RESTORE it after. + // Removing the FIFO is not enough: the subtests share one applied + // home, so leaving the path absent means the next shape's subtest + // runs against a home that was never fully applied. Measured before + // this restore: with the skips deleted, `import claude` and + // `reconcile --auto-override` PASSED in a full run and HUNG when + // their subtest ran alone — so whoever closes #241/#242 would have + // inherited a row that was green for the wrong reason. + applied, err := os.ReadFile(dest.path) + if err != nil { + t.Fatal(err) + } + info, err := os.Stat(dest.path) + if err != nil { + t.Fatal(err) + } if err := os.Remove(dest.path); err != nil { t.Fatal(err) } if err := syscall.Mkfifo(dest.path, 0o600); err != nil { + // Put the destination back before bailing out. + _ = os.WriteFile(dest.path, applied, info.Mode().Perm()) t.Skipf("mkfifo unsupported here: %v", err) } - t.Cleanup(func() { _ = os.Remove(dest.path) }) + t.Cleanup(func() { + _ = os.Remove(dest.path) + if err := os.WriteFile(dest.path, applied, info.Mode().Perm()); err != nil { + t.Errorf("restoring %s: %v — the next subtest would run against a "+ + "partially-applied home", dest.path, err) + } + }) // The first group is what this change fixes. The second still hangs // on this exact fixture and is SKIPPED rather than asserted or @@ -110,15 +135,42 @@ func TestCommandsDoNotHangOnNonRegularDestination(t *testing.T) { } // runBounded executes the CLI in a goroutine and fails if it has not returned -// within d. The environment must already be set by the caller on the test -// goroutine (runCLI's t.Setenv persists for the whole test), so nothing here -// touches testing.T off the test goroutine except the final t.Fatalf. +// within d, or if the command never actually ran. The environment must already +// be set by the caller on the test goroutine (runCLI's t.Setenv persists for the +// whole test), so nothing here touches testing.T off the test goroutine except +// the final t.Fatalf. func runBounded(t *testing.T, d time.Duration, args ...string) string { + t.Helper() + out, ran, err := runBoundedE(t, d, args...) + if !ran { + t.Fatalf("`agentsync %s` never reached its command body (err=%v) — the row executed "+ + "none of the code it names, so \"it returned\" proves nothing", strings.Join(args, " "), err) + } + return out +} + +// runBoundedE runs the CLI in a goroutine, bounded by d, and reports whether the +// resolved command's body actually STARTED. It returns rather than failing, so +// the anti-vacuity check itself can be tested; runBounded is the fatal wrapper +// every real row uses. +// +// `ran` is OBSERVED, by wrapping the resolved command's RunE, rather than +// inferred from cobra's error text. Two earlier versions inferred it and both +// were wrong: the first matched substrings against the output buffer, which +// NewRoot's SilenceErrors leaves empty, so it could never fire — while the +// buffer's ordinary stdout made it fire on user content instead. The second +// matched cobra's error prose, which covers only the arg/flag layer: anything +// rejecting later still scored as "it ran", including this repo's own +// enforceScopeStance (internal/cli/scope_flags.go), a PersistentPreRunE refusal +// that never reaches RunE. Wrapping the body answers the actual question and +// cannot drift with cobra's wording. +func runBoundedE(t *testing.T, d time.Duration, args ...string) (out string, ran bool, err error) { t.Helper() detachSlog(t) type result struct { out string err error + ran bool } done := make(chan result, 1) go func() { @@ -127,96 +179,81 @@ func runBounded(t *testing.T, d time.Duration, args ...string) string { root.SetOut(&buf) root.SetErr(&buf) root.SetArgs(args) - err := root.Execute() - done <- result{buf.String(), err} + + started := false + if cmd, _, ferr := root.Find(args); ferr == nil && cmd != nil { + switch { + case cmd.RunE != nil: + inner := cmd.RunE + cmd.RunE = func(c *cobra.Command, a []string) error { + started = true + return inner(c, a) + } + case cmd.Run != nil: + inner := cmd.Run + cmd.Run = func(c *cobra.Command, a []string) { + started = true + inner(c, a) + } + } + } + rerr := root.Execute() + done <- result{buf.String(), rerr, started} }() select { case r := <-done: - // Anti-vacuity. An earlier version of this test ran `import` with no - // agent selector; cobra rejected it at ExactArgs(1) before RunE, so the - // row executed zero lines of the code it named and passed identically - // with the fix reverted. A command that never starts cannot hang, so - // "it returned" is only meaningful once we know it ran. - // - // The check MUST read the returned error, not the output buffer. - // NewRoot sets SilenceErrors, so cobra prints nothing on a rejection — - // the first version of this guard inspected the buffer, could therefore - // never fire, and was itself the bug it was written to prevent. Worse, - // the buffer carries ordinary stdout, so matching these substrings - // against it made a plain `diff` fail whenever a rendered file happened - // to contain the words. TestRunBoundedRejectsAnInvocationCobraRefuses is - // the positive control that keeps this honest. - if r.err != nil && isCobraRejection(r.err) { - t.Fatalf("`agentsync %s` never reached its RunE — cobra rejected the invocation: %v", - strings.Join(args, " "), r.err) - } - return r.out + return r.out, r.ran, r.err case <-time.After(d): t.Fatalf("`agentsync %s` BLOCKED on a non-regular destination — os.ReadFile on a "+ "FIFO waits for a writer that never comes, so the read's own error path never "+ "runs and the command never returns", strings.Join(args, " ")) - return "" - } -} - -// isCobraRejection reports whether err is cobra refusing the invocation itself -// — a wrong argument count, an unknown command or flag — as opposed to a real -// failure from inside the command. -// -// It matches the returned error only. NewRoot sets SilenceErrors, so none of -// this text is ever printed, which is what made the first version of this check -// (written against the output buffer) unable to fire. -func isCobraRejection(err error) bool { - msg := err.Error() - for _, marker := range []string{ - "arg(s), received", - "unknown command", - "unknown flag", - "unknown shorthand flag", - "requires at least", - "accepts at most", - } { - if strings.Contains(msg, marker) { - return true - } + return "", false, nil } - return false } -// TestRunBoundedRejectsAnInvocationCobraRefuses is the positive control for the -// anti-vacuity check in runBounded. Without it that check is unfalsifiable: it +// TestRunBoundedDetectsACommandThatNeverRan is the positive control for +// runBounded's anti-vacuity check. Without it the check is unfalsifiable: it // only ever fires on a broken invocation, so nothing in a green suite proves it -// still works, and its first version silently never fired at all. +// still works — and its first two versions shipped unable to fire at all. // -// It asserts on isCobraRejection rather than by calling runBounded, because -// runBounded signals failure with t.Fatalf — driving it with a bad invocation -// would fail this test rather than pass it. -func TestRunBoundedRejectsAnInvocationCobraRefuses(t *testing.T) { - // Exactly the invocations that silently passed before: `import` without the - // agent selector its ExactArgs(1) requires, and an outright bad command. - for _, args := range [][]string{{"import"}, {"nosuchcommand"}} { - t.Run(strings.Join(args, " "), func(t *testing.T) { - var buf bytes.Buffer - root := cli.NewRoot() - root.SetOut(&buf) - root.SetErr(&buf) - root.SetArgs(args) - err := root.Execute() +// It drives runBoundedE, which reports instead of failing, so a row that never +// ran can be asserted rather than crashing the test. +func TestRunBoundedDetectsACommandThatNeverRan(t *testing.T) { + cases := []struct { + name string + args []string + }{ + // Exactly the invocation that silently passed for two rounds: `import` + // without the agent selector its ExactArgs(1) requires. + {name: "a missing required argument", args: []string{"import"}}, + {name: "an unknown command", args: []string{"nosuchcommand"}}, + // The case cobra's error prose could not catch: a refusal from + // PersistentPreRunE, which runs AFTER argument validation and never + // reaches the command body. `secret list` is scope-unaware, so passing + // --scope is refused by enforceScopeStance. + {name: "a refusal from PersistentPreRunE", args: []string{"secret", "list", "--scope", "user"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, ran, err := runBoundedE(t, 8*time.Second, tc.args...) if err == nil { - t.Fatalf("`agentsync %s` returned nil; expected cobra to refuse it", - strings.Join(args, " ")) + t.Fatalf("`agentsync %s` returned nil; expected it to be refused", + strings.Join(tc.args, " ")) } - if !isCobraRejection(err) { - t.Errorf("isCobraRejection(%q) = false, want true — runBounded would let this "+ - "invocation pass as if the command had run", err) - } - // The reason the check reads the error and not the buffer. - if strings.Contains(buf.String(), "arg(s), received") || - strings.Contains(buf.String(), "unknown command") { - t.Errorf("cobra printed its rejection into the output buffer (%q) — if that ever "+ - "becomes true, the simpler buffer-based check would work and this "+ - "indirection can go", buf.String()) + if ran { + t.Errorf("runBoundedE reported ran=true for %q, which was refused with %v — "+ + "runBounded would let this row pass as if the command had executed", + strings.Join(tc.args, " "), err) } }) } + + // The other direction: a command that DOES run must report ran=true, or + // every real row would fail as vacuous. + t.Run("a command that runs is reported as having run", func(t *testing.T) { + if _, ran, _ := runBoundedE(t, 8*time.Second, "version"); !ran { + t.Error("runBoundedE reported ran=false for `version`, which has no arguments to " + + "get wrong — the wrap is not finding the resolved command") + } + }) } diff --git a/internal/cli/destread.go b/internal/cli/destread.go index 635395fe..2bde6923 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -25,8 +25,9 @@ var errDestNotRegular = errors.New("not a regular file") // ~/.claude.json, say) wedged `status` — which is advertised as read-only — // through the shared readDestFile. // -// This gate covers THIS PACKAGE only. `apply`, `apply --dry-run` and -// `import ` still hang on the same fixture; their reads live in +// This gate covers THIS PACKAGE only. `apply`, `apply --dry-run`, +// `reconcile --auto-override` (which re-applies through render.Writer.Write) +// and `import ` still hang on the same fixture; their reads live in // internal/render and the adapter Ingest paths (#241, #242). // // Every destination read in this package routes here, cli.hashFile included, so @@ -36,12 +37,31 @@ var errDestNotRegular = errors.New("not a regular file") // refuses a link outright with its own sentinel; this function does not, so a // read that reaches os.ReadFile follows it. `status` therefore calls a symlinked // destination drifted while `diff` reads through it and compares the target. -// That predates this gate — the reads it replaced were bare os.ReadFile, which -// follows links too — and is left alone because AGENTSYNC_ALLOW_SYMLINK_DEST=1 -// is a documented, supported setup in which apply writes THROUGH the link, so -// refusing links here would break it. Reconciling the two is a behavior -// decision, tracked with the drift-walk unification in #229. -// TestHashFileSentinels asserts both halves. +// +// It is left alone because changing it is a BEHAVIOR decision, not because the +// current split is right: the reads this gate replaced were bare os.ReadFile, +// which follows links too, so refusing links here would change what `diff` and +// `reconcile` have always reported. That belongs with the drift-walk +// unification in #229, where all four walks can be changed together. +// +// (An earlier version of this comment justified it differently, by claiming +// AGENTSYNC_ALLOW_SYMLINK_DEST=1 would break. That was wrong — the variable is +// read only in internal/iox, on the WRITE path, so a read gate cannot affect +// it. The real consequence of the split is worse and is worth naming: under +// that supported setup apply writes THROUGH the link, yet hashFile refuses +// links unconditionally, so `status` reports drift that no apply can ever +// clear. #229 owns that too.) +// TestHashFileSentinels asserts both halves of the split. +// +// What a caller DOES with the refusal is its own business, and two of them +// currently swallow it: `diff` (internal/cli/diff.go) leaves the destination +// text empty and `readDestFile` decodes to an empty map, so a refused +// destination renders as "every byte / every key removed" — indistinguishable +// from an absent one. That is a poorer diagnosis than the shape error deserves, +// and it is the same silent-drop shape this repo's own rules warn about; it is +// left as-is here because surfacing it means changing what those two commands +// print, which belongs with the drift-walk unification in #229. `status`, +// `doctor` and `reconcile`'s write-back all name the shape correctly. // // An ABSENT path is not refused: render.IsRegularOrAbsent reports absent as // acceptable, so os.ReadFile runs and its ENOENT reaches the caller unchanged. diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index 3e9dfc0c..a5af9e0b 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -184,6 +184,29 @@ func TestKeyMergeAndWriteBackReadsAreGuarded(t *testing.T) { }) } +// TestWriteBackFileItemMessageMatchesTheFailure pins that the remedy named in +// writeBackFileItem's refusal depends on WHY the read failed. +// +// The shape sentence ("remove or replace the non-regular file") was once +// appended to every readDestBytes error, so deleting a managed file — the +// common case, which is itself drift and offers [w] — produced +// "no such file or directory — remove or replace the non-regular file at that +// path", advice that describes a situation the user is not in. +func TestWriteBackFileItemMessageMatchesTheFailure(t *testing.T) { + absent := filepath.Join(t.TempDir(), "deleted.md") + err := writeBackFileItem(t.TempDir(), reconcileItem{op: adapter.FileOp{Path: absent, SourceID: "demo"}}) + if err == nil { + t.Fatal("writeBackFileItem = nil for an absent destination, want an error") + } + if strings.Contains(err.Error(), "non-regular") { + t.Errorf("error = %q calls an ABSENT destination non-regular; the remedy must match "+ + "the failure that actually occurred", err) + } + if !strings.Contains(err.Error(), "[i]gnore") { + t.Errorf("error = %q, want it to still name a next step", err) + } +} + // mkfifoDest creates a 0600 FIFO at a destination path and returns it. mkfifo // and chmod do not open the FIFO; nothing in this file ever does. func mkfifoDest(t *testing.T, tmp string) string { @@ -261,7 +284,11 @@ func TestHashFileSentinels(t *testing.T) { } return p }, - want: hashContent([]byte("payload")), + // A literal, not hashContent([]byte("payload")): computing the + // expectation with the function under test is a tautology — salting + // hashContent leaves this row green. This digest is what a state + // file records for that content. + want: "239f59ed55e737c77147cf55ad0c1b030b6d7ee748a7426952f9b852d5a935e5", }, } diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index a50ba9f4..4a74e546 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -1164,8 +1164,15 @@ func writeBackFileItem(home string, it reconcileItem) error { // An earlier version of this message recommended it, which walked the // user out of a clean refusal and into an unbounded wedge. Restore that // suggestion only once #241 is fixed. - return fmt.Errorf("read dest %s: %w — remove or replace the non-regular file at that "+ - "path and re-run, or [i]gnore to suppress this item", it.op.Path, err) + if errors.Is(err, errDestNotRegular) { + return fmt.Errorf("read dest %s: %w — remove or replace the non-regular file at "+ + "that path and re-run, or [i]gnore to suppress this item", it.op.Path, err) + } + // Any other read failure — an absent destination (the common case: the + // user deleted a managed file, which is itself drift and offers [w]), a + // permission error — must NOT be told to "remove the non-regular file". + // An earlier version appended that sentence unconditionally. + return fmt.Errorf("read dest %s: %w — [i]gnore suppresses this item", it.op.Path, err) } srcID := it.op.SourceID if srcID == "" { From aaa0fa6c9ff35595f51e54e08efaa08a4c865ae3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 22:29:11 +0000 Subject: [PATCH 5/9] fix(cli): restore by rename; correct three claims the last round introduced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes round-4 review findings on PR #240. Three of the four are defects that round 3's fixes introduced. 1. The fixture restore added last round could write INTO the FIFO it was meant to replace. os.WriteFile opens the destination; opening a FIFO blocks until the other end appears, and t.Cleanup has no per-row timeout, so with no reader it wedges the suite outright. With a reader — which is what a timed-out row leaves parked in open(2) — it SUCCEEDS, drains the bytes into the pipe, returns nil, and leaves the FIFO in place, so the error check never fires and the destination is silently not restored. restoreDest now renames, which never opens the target, and TestRestoreDestReplacesAFIFOEvenWithAReaderAttached parks a reader and measures it. Break-verified: writing directly fails with "destination is p--------- after restore". 2. That restore's comment also carried a measurement I did not make. It claimed un-skipped rows PASSED in a full run and HUNG in isolation; eight configurations could not reproduce it. What actually varies is goroutines left parked by PRECEDING timed-out rows, which is nondeterministic and orthogonal to the restore. The structural reason for restoring stands on its own and is all the comment now claims. This is the round-1 error repeated: relaying a reviewer's measurement as established fact. 3. destread.go claimed "status, doctor and reconcile's write-back all name the shape correctly". Measured false for two of three: doctor performs NO destination read at all and reports "all checks passed" over a FIFO, and status maps the refusal to an opaque hash sentinel that statusItem never carries, so the user sees a bare "drift". Only reconcile names it. That sentence was the contrast justifying leaving diff's silent swallow alone, and the truth argues the other way; the comment now lists what each surface actually does, and the gap is still #229's to close. 4. Withholding [o]verride was over-corrected. It is unsafe only for a NON-REGULAR destination, where it hangs (#241). For an ABSENT one — the common case, a deleted managed file — Writer.Write's convergence read gets ENOENT and falls through to the write, so [o] is safe and is the fix. The two arms now match their failure, both pinned. Also: runBounded/runBoundedE returned an output string no caller read; dropped. runBoundedE's doc said it "returns rather than failing" while still failing on the timeout path; it reports the vacuity verdict, and now says so. destread.go lost the round-3 retraction framing (keeping the fact), states that its stat is racy against a reshape rather than implying atomicity, and the CHANGELOG's directory parenthetical no longer credits a change to the decode path that never happened there. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 7 +- internal/cli/dest_fifo_e2e_unix_test.go | 129 ++++++++++++++++---- internal/cli/destread.go | 44 ++++--- internal/cli/destread_unix_internal_test.go | 8 ++ internal/cli/reconcile.go | 17 +-- 5 files changed, 152 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bca1dbd..ce23ae33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,10 @@ source layout, CLI surface, and state schema are stabilizing but may still chang ### Fixed - **A FIFO at a managed destination no longer hangs `status`, `diff`, or - `reconcile`'s drift walk and write-back.** (A directory there never hung — - `os.ReadFile` fails it immediately with `EISDIR` — but it is now refused with - the same shape error instead of that surfacing from deeper in the decode.) `os.ReadFile` on a FIFO does + `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 + it changes; only `reconcile`'s write-back now names the shape rather than + reporting `EISDIR`.) `os.ReadFile` on a FIFO does not fail — it blocks in the open waiting for a writer that never comes — so the read's own error path never runs and the command never returns. Measured on the previous release: a FIFO at a whole-file destination wedged `diff` and diff --git a/internal/cli/dest_fifo_e2e_unix_test.go b/internal/cli/dest_fifo_e2e_unix_test.go index 10dbe15a..e417b780 100644 --- a/internal/cli/dest_fifo_e2e_unix_test.go +++ b/internal/cli/dest_fifo_e2e_unix_test.go @@ -4,6 +4,7 @@ package cli_test import ( "bytes" + "io" "os" "path/filepath" "strings" @@ -71,11 +72,15 @@ func TestCommandsDoNotHangOnNonRegularDestination(t *testing.T) { // Swap the applied destination for a FIFO, and RESTORE it after. // Removing the FIFO is not enough: the subtests share one applied // home, so leaving the path absent means the next shape's subtest - // runs against a home that was never fully applied. Measured before - // this restore: with the skips deleted, `import claude` and - // `reconcile --auto-override` PASSED in a full run and HUNG when - // their subtest ran alone — so whoever closes #241/#242 would have - // inherited a row that was green for the wrong reason. + // runs against a home that was never fully applied — contradicting + // this test's own premise. + // + // That is a structural property, not a measured pass/hang flip. An + // earlier version of this comment claimed the un-skipped rows + // changed outcome without the restore; eight configurations could + // not reproduce that. What actually varies there is goroutines left + // parked in open(2) by PRECEDING timed-out rows, which can pair + // with a later opener — nondeterministic, and orthogonal to this. applied, err := os.ReadFile(dest.path) if err != nil { t.Fatal(err) @@ -88,17 +93,12 @@ func TestCommandsDoNotHangOnNonRegularDestination(t *testing.T) { t.Fatal(err) } if err := syscall.Mkfifo(dest.path, 0o600); err != nil { - // Put the destination back before bailing out. + // Safe to write directly here: mkfifo failed, so the path is + // still absent rather than a FIFO. _ = os.WriteFile(dest.path, applied, info.Mode().Perm()) t.Skipf("mkfifo unsupported here: %v", err) } - t.Cleanup(func() { - _ = os.Remove(dest.path) - if err := os.WriteFile(dest.path, applied, info.Mode().Perm()); err != nil { - t.Errorf("restoring %s: %v — the next subtest would run against a "+ - "partially-applied home", dest.path, err) - } - }) + t.Cleanup(func() { restoreDest(t, dest.path, applied, info.Mode().Perm()) }) // The first group is what this change fixes. The second still hangs // on this exact fixture and is SKIPPED rather than asserted or @@ -139,20 +139,20 @@ func TestCommandsDoNotHangOnNonRegularDestination(t *testing.T) { // be set by the caller on the test goroutine (runCLI's t.Setenv persists for the // whole test), so nothing here touches testing.T off the test goroutine except // the final t.Fatalf. -func runBounded(t *testing.T, d time.Duration, args ...string) string { +func runBounded(t *testing.T, d time.Duration, args ...string) { t.Helper() - out, ran, err := runBoundedE(t, d, args...) + ran, err := runBoundedE(t, d, args...) if !ran { t.Fatalf("`agentsync %s` never reached its command body (err=%v) — the row executed "+ "none of the code it names, so \"it returned\" proves nothing", strings.Join(args, " "), err) } - return out } // runBoundedE runs the CLI in a goroutine, bounded by d, and reports whether the -// resolved command's body actually STARTED. It returns rather than failing, so -// the anti-vacuity check itself can be tested; runBounded is the fatal wrapper -// every real row uses. +// resolved command's body actually STARTED. It REPORTS that verdict rather than +// failing on it, so the anti-vacuity check itself can be tested; runBounded is +// the fatal wrapper every real row uses. (The timeout path still fails here — +// a wedged command has no verdict to report.) // // `ran` is OBSERVED, by wrapping the resolved command's RunE, rather than // inferred from cobra's error text. Two earlier versions inferred it and both @@ -164,7 +164,7 @@ func runBounded(t *testing.T, d time.Duration, args ...string) string { // enforceScopeStance (internal/cli/scope_flags.go), a PersistentPreRunE refusal // that never reaches RunE. Wrapping the body answers the actual question and // cannot drift with cobra's wording. -func runBoundedE(t *testing.T, d time.Duration, args ...string) (out string, ran bool, err error) { +func runBoundedE(t *testing.T, d time.Duration, args ...string) (ran bool, err error) { t.Helper() detachSlog(t) type result struct { @@ -202,12 +202,12 @@ func runBoundedE(t *testing.T, d time.Duration, args ...string) (out string, ran }() select { case r := <-done: - return r.out, r.ran, r.err + return r.ran, r.err case <-time.After(d): t.Fatalf("`agentsync %s` BLOCKED on a non-regular destination — os.ReadFile on a "+ "FIFO waits for a writer that never comes, so the read's own error path never "+ "runs and the command never returns", strings.Join(args, " ")) - return "", false, nil + return false, nil } } @@ -235,7 +235,7 @@ func TestRunBoundedDetectsACommandThatNeverRan(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - _, ran, err := runBoundedE(t, 8*time.Second, tc.args...) + ran, err := runBoundedE(t, 8*time.Second, tc.args...) if err == nil { t.Fatalf("`agentsync %s` returned nil; expected it to be refused", strings.Join(tc.args, " ")) @@ -251,9 +251,90 @@ func TestRunBoundedDetectsACommandThatNeverRan(t *testing.T) { // The other direction: a command that DOES run must report ran=true, or // every real row would fail as vacuous. t.Run("a command that runs is reported as having run", func(t *testing.T) { - if _, ran, _ := runBoundedE(t, 8*time.Second, "version"); !ran { + if ran, _ := runBoundedE(t, 8*time.Second, "version"); !ran { t.Error("runBoundedE reported ran=false for `version`, which has no arguments to " + "get wrong — the wrap is not finding the resolved command") } }) } + +// restoreDest puts a captured destination back, by RENAME rather than by +// writing to the path. +// +// os.WriteFile opens the destination, and opening a FIFO blocks until the other +// end appears — inside t.Cleanup, where no per-row timeout applies, that wedges +// the suite outright. Worse, when a reader IS present — which is what a +// timed-out row leaves parked in open(2), and becomes common once the #241/#242 +// skips are deleted — the write succeeds, drains into the pipe, returns nil, +// and leaves the FIFO in place, so an error check never fires and the +// destination is silently not restored. Rename never opens the target. +// TestRestoreDestReplacesAFIFOEvenWithAReaderAttached measures exactly that. +func restoreDest(t *testing.T, path string, data []byte, mode os.FileMode) { + t.Helper() + tmp := path + ".restore" + if err := os.WriteFile(tmp, data, mode); err != nil { + t.Errorf("restoring %s: %v", path, err) + return + } + // os.WriteFile's mode is masked by umask on create; chmod pins it. + if err := os.Chmod(tmp, mode); err != nil { + t.Errorf("restoring %s: %v", path, err) + return + } + if err := os.Rename(tmp, path); err != nil { + t.Errorf("restoring %s: %v — the next subtest would run against a "+ + "partially-applied home", path, err) + } +} + +// TestRestoreDestReplacesAFIFOEvenWithAReaderAttached pins the reason +// restoreDest renames instead of writing. +// +// The reader goroutine reproduces what a timed-out row leaves behind. Without +// the rename, os.WriteFile succeeds here — the bytes vanish into the pipe, the +// error is nil, and the FIFO survives, so the destination is silently NOT +// restored and the next subtest runs against a partially-applied home. +func TestRestoreDestReplacesAFIFOEvenWithAReaderAttached(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "dest") + if err := syscall.Mkfifo(path, 0o600); err != nil { + t.Skipf("mkfifo unsupported here: %v", err) + } + + // A reader parked on the FIFO, as an abandoned CLI goroutine would be. + opened := make(chan struct{}) + go func() { + f, err := os.Open(path) + close(opened) + if err == nil { + _, _ = io.Copy(io.Discard, f) + _ = f.Close() + } + }() + + done := make(chan struct{}) + go func() { + restoreDest(t, path, []byte("applied"), 0o644) + close(done) + }() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatalf("restoreDest BLOCKED on %s — it must not open the destination", path) + } + + info, err := os.Lstat(path) + if err != nil { + t.Fatal(err) + } + if !info.Mode().IsRegular() { + t.Fatalf("destination is %s after restore, want a regular file: the write went INTO "+ + "the FIFO instead of replacing it, so the fixture was never restored", + info.Mode().Type()) + } + got, err := os.ReadFile(path) + if err != nil || string(got) != "applied" { + t.Errorf("restored content = (%q, %v), want %q", got, err, "applied") + } + <-opened +} diff --git a/internal/cli/destread.go b/internal/cli/destread.go index 2bde6923..3a6034c5 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -14,8 +14,11 @@ import ( // double it in the wrapped message. var errDestNotRegular = errors.New("not a regular file") -// readDestBytes reads a destination file's bytes, refusing before the open any -// path whose shape cannot be read as a file. +// readDestBytes reads a destination file's bytes, refusing any path whose shape +// cannot be read as a file. The check is a stat before the open, so it is +// racy against something reshaping the path in between — that window needs +// write access to the destination's own directory, which is already game over, +// and closing it properly needs O_RDONLY|O_NONBLOCK + fstat. // // The guard is not defensive tidying. os.ReadFile on a FIFO does not fail — it // BLOCKS in the open, waiting for a writer that never comes, so no error path @@ -44,24 +47,27 @@ var errDestNotRegular = errors.New("not a regular file") // `reconcile` have always reported. That belongs with the drift-walk // unification in #229, where all four walks can be changed together. // -// (An earlier version of this comment justified it differently, by claiming -// AGENTSYNC_ALLOW_SYMLINK_DEST=1 would break. That was wrong — the variable is -// read only in internal/iox, on the WRITE path, so a read gate cannot affect -// it. The real consequence of the split is worse and is worth naming: under -// that supported setup apply writes THROUGH the link, yet hashFile refuses -// links unconditionally, so `status` reports drift that no apply can ever -// clear. #229 owns that too.) -// TestHashFileSentinels asserts both halves of the split. +// The split has a real cost worth naming: under AGENTSYNC_ALLOW_SYMLINK_DEST=1 +// apply writes THROUGH a symlinked destination, yet hashFile refuses links +// unconditionally, so `status` reports drift that no apply can ever clear. #229 +// owns that too. TestHashFileSentinels asserts both halves of the split. // -// What a caller DOES with the refusal is its own business, and two of them -// currently swallow it: `diff` (internal/cli/diff.go) leaves the destination -// text empty and `readDestFile` decodes to an empty map, so a refused -// destination renders as "every byte / every key removed" — indistinguishable -// from an absent one. That is a poorer diagnosis than the shape error deserves, -// and it is the same silent-drop shape this repo's own rules warn about; it is -// left as-is here because surfacing it means changing what those two commands -// print, which belongs with the drift-walk unification in #229. `status`, -// `doctor` and `reconcile`'s write-back all name the shape correctly. +// What a caller DOES with the refusal is its own business, and today almost +// none of them surface it. Measured against a FIFO destination: +// +// - `reconcile`'s write-back is the ONLY surface that names the shape. +// - `status` maps it to the opaque not-a-regular-file hash, which exists only +// to never equal a content hash; statusItem carries no reason, so the user +// sees a bare "drift". +// - `diff` leaves the destination text empty and readDestFile decodes to an +// empty map, so a refused destination renders as "every byte / every key +// removed" — indistinguishable from an absent one. `explain` inherits that. +// - `doctor` reads no destination at all and reports "all checks passed". +// +// So the refusal is mostly a better DIAGNOSIS available to callers rather than +// one they give the user, and that gap is wider than this gate. Narrowing it +// means changing what several commands print, which belongs with the drift-walk +// unification in #229 rather than here. // // An ABSENT path is not refused: render.IsRegularOrAbsent reports absent as // acceptable, so os.ReadFile runs and its ENOENT reaches the caller unchanged. diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index a5af9e0b..99aa3ab0 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -205,6 +205,14 @@ 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. + 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) + } } // mkfifoDest creates a 0600 FIFO at a destination path and returns it. mkfifo diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index 4a74e546..9d515f7a 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -1157,8 +1157,8 @@ func writeBackFileItem(home string, it reconcileItem) error { // mid-prompt with a keystroke to choose, and "read dest X: not a regular // file" alone does not tell them which one gets them unstuck. // - // [o]verride is deliberately NOT offered, unlike the peer refusals in - // this file. It re-applies through render.Writer.Write, whose + // [o]verride is deliberately NOT offered for THIS arm, unlike the peer + // refusals in this file and unlike the absent-destination 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 @@ -1168,11 +1168,14 @@ func writeBackFileItem(home string, it reconcileItem) error { return fmt.Errorf("read dest %s: %w — remove or replace the non-regular file at "+ "that path and re-run, or [i]gnore to suppress this item", it.op.Path, err) } - // Any other read failure — an absent destination (the common case: the - // user deleted a managed file, which is itself drift and offers [w]), a - // permission error — must NOT be told to "remove the non-regular file". - // An earlier version appended that sentence unconditionally. - return fmt.Errorf("read dest %s: %w — [i]gnore suppresses this item", it.op.Path, err) + // Any other read failure keeps the peers' remedy set. The common one is + // 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. + 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) } srcID := it.op.SourceID if srcID == "" { From 7dd8d9fcd2e60779600034525c0d92059d810dd5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 00:34:43 +0000 Subject: [PATCH 6/9] fix(cli): de-race the restore test; stop the sentinel lying about stat errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes round-5 review findings on PR #240. 1. BLOCKER, and it was my own test: TestRestoreDestReplacesAFIFOEvenWith AReaderAttached was RACY and usually hung. The reader goroutine did a blocking os.Open on the FIFO and raced restoreDest — if the rename landed first the open returned instantly on a regular file and the test passed; if the reader won it parked forever, because restoreDest's whole purpose is never to open the target. So the test written to prevent an unbounded hang reproduced one, and my single green run had simply won the race. Two lenses lost it; so did every run since. The reader now attaches with syscall.O_RDONLY|O_NONBLOCK, which returns immediately and leaves a genuinely attached reader instead of a racing one, and restoreDest is called on the test goroutine (its t.Errorf no longer fires off-goroutine). Deterministic both ways: 3/3 pass, 3/3 fail when reverted to writing the path. 2. errDestNotRegular was returned for stat failures that are not absence. render.IsRegularOrAbsent answers false for EACCES-on-a-parent and ELOOP too, so a symlink loop reached reconcile's refusal and told the user to "remove or replace the non-regular file at that path" — a false statement about their destination, with the real errno discarded. Those now report as themselves. render.IsRegularOrAbsent is deliberately still the authority on SHAPE, at the cost of one extra stat on an error path: inlining the predicate would have falsified docs/components.md's claim that it is shared with this package's destination reads. Pinned with an ELOOP fixture (root in the container makes an EACCES fixture unenforceable). 3. Prose #13: "doctor reads no destination at all" is false. Its plugin check reaches one through claude.IngestPlugins -> a bare os.ReadFile of settings.json, a managed destination. A reviewer measured doctor hanging there; a second fixture exited 1 because another issue was reported first, so the comment records the read as unguarded by inspection and leaves reachability to #242, which now lists doctor in scope. 4. The guard test cited render.isRegularOrAbsent's doc as authority for the write path being handled. That doc says the predicate is what stops `apply --dry-run` hanging, which is false — Writer.Write never calls it and apply --dry-run still hangs (#241). The comment now says so rather than leaning on it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- internal/cli/dest_fifo_e2e_unix_test.go | 43 ++++++++++---------- internal/cli/destread.go | 21 +++++++--- internal/cli/destread_guard_internal_test.go | 10 +++-- internal/cli/destread_unix_internal_test.go | 35 ++++++++++++++++ 4 files changed, 77 insertions(+), 32 deletions(-) diff --git a/internal/cli/dest_fifo_e2e_unix_test.go b/internal/cli/dest_fifo_e2e_unix_test.go index e417b780..f17f695f 100644 --- a/internal/cli/dest_fifo_e2e_unix_test.go +++ b/internal/cli/dest_fifo_e2e_unix_test.go @@ -4,7 +4,6 @@ package cli_test import ( "bytes" - "io" "os" "path/filepath" "strings" @@ -301,27 +300,28 @@ func TestRestoreDestReplacesAFIFOEvenWithAReaderAttached(t *testing.T) { t.Skipf("mkfifo unsupported here: %v", err) } - // A reader parked on the FIFO, as an abandoned CLI goroutine would be. - opened := make(chan struct{}) - go func() { - f, err := os.Open(path) - close(opened) - if err == nil { - _, _ = io.Copy(io.Discard, f) - _ = f.Close() - } - }() - - done := make(chan struct{}) - go func() { - restoreDest(t, path, []byte("applied"), 0o644) - close(done) - }() - select { - case <-done: - case <-time.After(5 * time.Second): - t.Fatalf("restoreDest BLOCKED on %s — it must not open the destination", path) + // A reader ATTACHED to the FIFO, standing in for what a timed-out row leaves + // parked in open(2). + // + // O_NONBLOCK is load-bearing. A blocking os.Open in a goroutine looks like + // the real thing but is a RACE against restoreDest: if the rename lands + // first the open returns instantly on a regular file, and if the reader + // wins it parks forever, because restoreDest's entire purpose is never to + // open the target. The first version of this test did exactly that and hung + // most of the time — reproducing, in the test meant to prevent it, the + // failure mode this whole PR is about. O_NONBLOCK returns immediately and + // leaves a genuinely attached reader, so the counterfactual below (a direct + // os.WriteFile succeeding into the pipe) is real and deterministic. + fd, err := syscall.Open(path, syscall.O_RDONLY|syscall.O_NONBLOCK, 0) + if err != nil { + t.Fatalf("attaching a reader to %s: %v", path, err) } + defer func() { _ = syscall.Close(fd) }() + + // Called on the test goroutine: with a reader attached neither restoreDest + // nor the os.WriteFile variant can block, so no timeout wrapper is needed — + // and restoreDest's t.Errorf stays on the goroutine that owns the test. + restoreDest(t, path, []byte("applied"), 0o644) info, err := os.Lstat(path) if err != nil { @@ -336,5 +336,4 @@ func TestRestoreDestReplacesAFIFOEvenWithAReaderAttached(t *testing.T) { if err != nil || string(got) != "applied" { t.Errorf("restored content = (%q, %v), want %q", got, err, "applied") } - <-opened } diff --git a/internal/cli/destread.go b/internal/cli/destread.go index 3a6034c5..dbbb332f 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -62,7 +62,10 @@ var errDestNotRegular = errors.New("not a regular file") // - `diff` leaves the destination text empty and readDestFile decodes to an // empty map, so a refused destination renders as "every byte / every key // removed" — indistinguishable from an absent one. `explain` inherits that. -// - `doctor` reads no destination at all and reports "all checks passed". +// - `doctor` performs no read through this gate, but its plugin check reaches +// one anyway, in the adapter Ingest path (`IngestPlugins` -> a bare +// os.ReadFile of the agent's settings). That read is unguarded, so `doctor` +// is exposed to the same hang as `import` (#242), not immune to it. // // So the refusal is mostly a better DIAGNOSIS available to callers rather than // one they give the user, and that gap is wider than this gate. Narrowing it @@ -72,12 +75,18 @@ var errDestNotRegular = errors.New("not a regular file") // An ABSENT path is not refused: render.IsRegularOrAbsent reports absent as // acceptable, so os.ReadFile runs and its ENOENT reaches the caller unchanged. // Manufacturing a shape error for a file that is not there would name the wrong -// problem. Note the predicate also answers false for a stat failure that is NOT -// ENOENT (EACCES on a parent, ELOOP), so those surface as errDestNotRegular -// rather than as themselves — imprecise, but in the safe direction, and it -// keeps this function's answer identical to the one hashFile gave before it was -// folded in. +// problem. func readDestBytes(path string) ([]byte, error) { + // A stat failure that is NOT absence — EACCES on a parent, ELOOP — is + // reported as itself. render.IsRegularOrAbsent answers false for those too, + // and letting them through as errDestNotRegular would put a false statement + // in front of the user: reconcile's refusal names that sentinel and would + // tell someone with a permission problem to "remove or replace the + // non-regular file at that path". The extra stat costs one syscall on an + // error path and keeps render's predicate the single authority on SHAPE. + if _, serr := os.Stat(path); serr != nil && !os.IsNotExist(serr) { + return nil, serr + } if !render.IsRegularOrAbsent(path) { return nil, errDestNotRegular } diff --git a/internal/cli/destread_guard_internal_test.go b/internal/cli/destread_guard_internal_test.go index 6942f6ac..970ae4b1 100644 --- a/internal/cli/destread_guard_internal_test.go +++ b/internal/cli/destread_guard_internal_test.go @@ -82,10 +82,12 @@ func TestEveryDestinationReadGoesThroughTheGate(t *testing.T) { scanned := 0 if err := walkRepoGoFiles(repoRoot, func(rel, src string) { // Scoped to this package deliberately. internal/render and the adapter - // Apply paths also read op.Path, but those are the WRITE path, with - // their own upstream shape handling (see render.isRegularOrAbsent's doc - // comment, which names apply's pre-delete read). They were not audited - // here and this guard makes no claim about them. + // Apply paths also read op.Path, but on the WRITE path, which this + // change does not cover. Do NOT read that as "handled elsewhere": + // render.isRegularOrAbsent's own doc says it is what stops + // `apply --dry-run` hanging on a FIFO, and that is false — Writer.Write's + // convergence read never calls it, and `apply --dry-run` still hangs + // (#241). This guard makes no claim about those reads either way. if !strings.HasPrefix(rel, "internal/cli/") { return } diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index 99aa3ab0..e7e36bc7 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -341,3 +341,38 @@ func TestHashFileSentinels(t *testing.T) { } }) } + +// TestReadDestBytesReportsAStatFailureAsItself pins that a stat failure which +// is not absence surfaces as the real error rather than as errDestNotRegular. +// +// It matters because the sentinel is not private: reconcile's write-back names +// it and tells the user to "remove or replace the non-regular file at that +// path". Answering it for a symlink loop or a permission problem states +// something false about the destination. +// +// ELOOP rather than EACCES because these tests run as root in the container, +// where permission bits are not enforced and an EACCES fixture would not fail. +func TestReadDestBytesReportsAStatFailureAsItself(t *testing.T) { + dir := t.TempDir() + a := filepath.Join(dir, "a") + b := filepath.Join(dir, "b") + if err := os.Symlink(b, a); err != nil { + t.Fatal(err) + } + if err := os.Symlink(a, b); err != nil { + t.Fatal(err) + } + + _, err := readDestBytes(a) + if err == nil { + t.Fatal("readDestBytes on a symlink loop = nil, want ELOOP") + } + if errors.Is(err, errDestNotRegular) { + t.Errorf("error = %v, want the real stat failure: a symlink loop is not a shape "+ + "problem, and reporting it as one makes reconcile tell the user to remove a "+ + "non-regular file that isn't there", err) + } + if !errors.Is(err, syscall.ELOOP) { + t.Errorf("error = %v, want it to wrap ELOOP", err) + } +} From d31f62309c374e105423fb40f775b95d2e63183f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 00:38:10 +0000 Subject: [PATCH 7/9] test(cli): pin the chmod; move the per-surface catalogue to #229 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the rest of round 5. The deadlocking test and the lying sentinel went in 7dd8d9f; these are the two remaining findings. 1. restoreDest's chmod was UNPINNED — no test asserted the restored mode, so dropping it left the suite green. os.WriteFile's mode argument is masked by umask on create, so without the chmod a restored destination can come back more restrictive than the one captured. Now asserted; break-verified by dropping the chmod under `umask 0077`, which fails with "restored mode = 0600, want 0644". 2. Proportionality. The test-rigor lens' verdict was that the behavior tests are proportionate but the prose is not — 68 comment lines to 13 code lines in destread.go, with each round adding scaffolding and then scaffolding for the scaffolding. Acted on: the doc is 52/16, and the biggest block, a catalogue of what every OTHER command does with a refused destination, is gone from here entirely. It documented other packages' behavior in the wrong place and it is only actionable in the shared walk, so it now lives on #229 with the measurements, alongside the symlink split and the EACCES-classifies-as-Orphan case. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- internal/cli/dest_fifo_e2e_unix_test.go | 7 +++ internal/cli/destread.go | 84 +++++++++---------------- 2 files changed, 38 insertions(+), 53 deletions(-) diff --git a/internal/cli/dest_fifo_e2e_unix_test.go b/internal/cli/dest_fifo_e2e_unix_test.go index f17f695f..29de69a3 100644 --- a/internal/cli/dest_fifo_e2e_unix_test.go +++ b/internal/cli/dest_fifo_e2e_unix_test.go @@ -336,4 +336,11 @@ func TestRestoreDestReplacesAFIFOEvenWithAReaderAttached(t *testing.T) { if err != nil || string(got) != "applied" { t.Errorf("restored content = (%q, %v), want %q", got, err, "applied") } + // The chmod, which os.WriteFile alone cannot guarantee: its mode argument is + // masked by umask on create, so without the explicit chmod a restored + // destination can come back more restrictive than the one that was captured. + if perm := info.Mode().Perm(); perm != 0o644 { + t.Errorf("restored mode = %04o, want 0644: os.WriteFile's mode is umask-masked, so "+ + "restoreDest must chmod to pin what it captured", perm) + } } diff --git a/internal/cli/destread.go b/internal/cli/destread.go index dbbb332f..d305d520 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -14,68 +14,46 @@ import ( // double it in the wrapped message. var errDestNotRegular = errors.New("not a regular file") -// readDestBytes reads a destination file's bytes, refusing any path whose shape -// cannot be read as a file. The check is a stat before the open, so it is -// racy against something reshaping the path in between — that window needs -// write access to the destination's own directory, which is already game over, -// and closing it properly needs O_RDONLY|O_NONBLOCK + fstat. +// readDestBytes reads a destination file's bytes, refusing before the open any +// path whose shape cannot be read as a file. // // The guard is not defensive tidying. os.ReadFile on a FIFO does not fail — it // BLOCKS in the open, waiting for a writer that never comes, so no error path // below the read ever runs and the command never returns. Measured on the -// unguarded code: a FIFO at a managed destination wedged `diff` and `reconcile` -// via their whole-file reads, and a FIFO-shaped key-merge destination (a -// ~/.claude.json, say) wedged `status` — which is advertised as read-only — -// through the shared readDestFile. +// unguarded code: a FIFO at a managed destination wedged `diff` and +// `reconcile`, and a FIFO-shaped key-merge destination (a ~/.claude.json, say) +// wedged `status` — which is advertised as read-only — through the shared +// readDestFile. // -// This gate covers THIS PACKAGE only. `apply`, `apply --dry-run`, -// `reconcile --auto-override` (which re-applies through render.Writer.Write) -// and `import ` still hang on the same fixture; their reads live in -// internal/render and the adapter Ingest paths (#241, #242). +// Every destination read in THIS PACKAGE routes here, cli.hashFile included, so +// they cannot disagree about what is safe to read. Other packages are not +// covered: `apply`, `apply --dry-run`, `reconcile --auto-override` and +// `import ` still hang, and `doctor` is exposed through its plugin +// check, because those reads live in internal/render and the adapter Ingest +// paths (#241, #242). // -// Every destination read in this package routes here, cli.hashFile included, so -// they cannot disagree about what is safe to read. +// Two deliberate limits: // -// They DO still disagree about symlinks, deliberately. hashFile Lstats first and -// refuses a link outright with its own sentinel; this function does not, so a -// read that reaches os.ReadFile follows it. `status` therefore calls a symlinked -// destination drifted while `diff` reads through it and compares the target. +// - The check is a stat, so it is racy against a reshape between stat and +// open. That window needs write access to the destination's own directory, +// which is already game over; closing it properly needs O_RDONLY|O_NONBLOCK +// plus fstat. +// - 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. // -// It is left alone because changing it is a BEHAVIOR decision, not because the -// current split is right: the reads this gate replaced were bare os.ReadFile, -// which follows links too, so refusing links here would change what `diff` and -// `reconcile` have always reported. That belongs with the drift-walk -// unification in #229, where all four walks can be changed together. +// 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. // -// The split has a real cost worth naming: under AGENTSYNC_ALLOW_SYMLINK_DEST=1 -// apply writes THROUGH a symlinked destination, yet hashFile refuses links -// unconditionally, so `status` reports drift that no apply can ever clear. #229 -// owns that too. TestHashFileSentinels asserts both halves of the split. -// -// What a caller DOES with the refusal is its own business, and today almost -// none of them surface it. Measured against a FIFO destination: -// -// - `reconcile`'s write-back is the ONLY surface that names the shape. -// - `status` maps it to the opaque not-a-regular-file hash, which exists only -// to never equal a content hash; statusItem carries no reason, so the user -// sees a bare "drift". -// - `diff` leaves the destination text empty and readDestFile decodes to an -// empty map, so a refused destination renders as "every byte / every key -// removed" — indistinguishable from an absent one. `explain` inherits that. -// - `doctor` performs no read through this gate, but its plugin check reaches -// one anyway, in the adapter Ingest path (`IngestPlugins` -> a bare -// os.ReadFile of the agent's settings). That read is unguarded, so `doctor` -// is exposed to the same hang as `import` (#242), not immune to it. -// -// So the refusal is mostly a better DIAGNOSIS available to callers rather than -// one they give the user, and that gap is wider than this gate. Narrowing it -// means changing what several commands print, which belongs with the drift-walk -// unification in #229 rather than here. -// -// An ABSENT path is not refused: render.IsRegularOrAbsent reports absent as -// acceptable, so os.ReadFile runs and its ENOENT reaches the caller unchanged. -// Manufacturing a shape error for a file that is not there would name the wrong -// problem. +// An ABSENT path is not refused: os.ReadFile runs and its ENOENT reaches the +// caller unchanged, because manufacturing a shape error for a file that is not +// there would name the wrong problem. func readDestBytes(path string) ([]byte, error) { // A stat failure that is NOT absence — EACCES on a parent, ELOOP — is // reported as itself. render.IsRegularOrAbsent answers false for those too, From 43f4464a6438f3354c7485eeeee38e81994a69f0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 01:00:03 +0000 Subject: [PATCH 8/9] fix(cli): split the sentinel; make the chmod pin fire at CI's umask MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes round-6 review findings on PR #240. All four lenses reported: two CLEAN, two holding on the same item. 1. The chmod pin was VACUOUS in CI. Three lenses caught it. At the ambient umask 0022, os.WriteFile(_, _, 0o644) already yields 0644, so deleting restoreDest's chmod left the assertion green — and my own break-verification had "passed" only because I ran it under `umask 0077` in a subshell. I verified my shell, not the test. The fixture now captures 0o666, which masks down to 0644 on create, so only the chmod can produce it; break-verified at the DEFAULT umask, no process-global umask manipulation needed. 2. A base-parity regression I introduced in round 5. The stat-error arm moved hashFile's answer for an unstattable destination (parent ENOTDIR/ELOOP/ EACCES) from "not-a-regular-file" to "", which drift.Classify reads as absent — turning ForeignCollision into New when nothing was applied, and New is SafeForAutoApply. The naive fix breaks a different case: hashFile cannot tell a stat failure from a read failure by errno alone, and EACCES on the FILE answered "" at base. So the gate now has TWO sentinels. errDestNotRegular means the shape is wrong; errDestUnstattable wraps a real stat errno. reconcile keys on the first alone, so its "remove or replace the non-regular file" line stays truthful for a permission problem, while hashFile — whose sentinels are opaque tokens compared only for equality — maps both alike and regains exact base parity. Pinned by a new unstattable row; the fix had been UNPINNED, which is the same defect it was fixing. 3. Inverting the stat order fixed two findings at once. render.IsRegularOrAbsent is asked FIRST, so the ordinary read costs one stat and only the refusal path pays a second to tell shape from stat failure. That makes the comment's cost claim true rather than merely reworded — three lenses flagged it as false, since the stat had been unconditional. 4. Prose #15: my round-5 CORRECTION was over-broad. The guard test said render.isRegularOrAbsent's `apply --dry-run` claim "is false"; it is true of the orphan-delete read that predicate guards (writer.go:346, reached via OrphanDeleteWillProceed) and false only of Writer.Write's convergence read. Scoped. 5. doctor is a measured hang, not merely "exposed": mkfifo ~/.claude/settings.json then `doctor` wedges at rc=124 after printing "Plugins", via claude.IngestPlugins. My two earlier failures to reproduce were fixture bugs — .claude did not exist, so mkfifo itself failed and I measured a run with no FIFO in it. Stated as measured, and added to CHANGELOG's not-fixed list. No e2e row: neither existing destination shape is settings.json, so a row would pass vacuously. Also: restoreDest no longer leaves its temp file behind on a failure path (the contamination it exists to prevent); `explain` joins the CHANGELOG's fixed list (it reads destinations through readDestFile too); hashFile's doc describes what it now returns; and reconcile.go is back to the base count of over-long lines. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 12 ++++--- internal/cli/dest_fifo_e2e_unix_test.go | 15 ++++++-- internal/cli/destread.go | 39 +++++++++++++++------ internal/cli/destread_unix_internal_test.go | 18 ++++++++++ internal/cli/reconcile.go | 8 +++-- internal/cli/status.go | 14 ++++++-- 6 files changed, 81 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce23ae33..f3306617 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,8 +11,8 @@ source layout, CLI surface, and state schema are stabilizing but may still chang ### Fixed -- **A FIFO at a managed destination no longer hangs `status`, `diff`, or - `reconcile`'s drift walk and write-back.** (A directory there never hung — `os.ReadFile` +- **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 it changes; only `reconcile`'s write-back now names the shape rather than reporting `EISDIR`.) `os.ReadFile` on a FIFO does @@ -35,12 +35,14 @@ source layout, CLI surface, and state schema are stabilizing but may still chang a non-regular destination would classify as drift and then hang one keystroke later. - **`apply`, `apply --dry-run`, `reconcile`'s `[o]verride` and `import ` - are NOT fixed by this** and still hang on the same fixture — their reads are + **`apply`, `apply --dry-run`, `reconcile`'s `[o]verride`, `import ` and + `doctor` are NOT fixed by this** and still hang on the same fixture — their reads are in `internal/render` and the adapter `Ingest` paths, a far wider sweep. `[o]verride` re-applies through `render.Writer.Write`, so it shares `apply`'s unguarded read; the refusal message therefore points at removing or replacing - the file rather than at `[o]`, which would wedge. Tracked as + the file rather than at `[o]`, which would wedge. `doctor` reads no + destination itself but reaches one through its plugin check — a FIFO at + `~/.claude/settings.json` wedges it after it prints `Plugins`. Tracked as [#241](https://github.com/spxrogers/agentsync/issues/241) and [#242](https://github.com/spxrogers/agentsync/issues/242). diff --git a/internal/cli/dest_fifo_e2e_unix_test.go b/internal/cli/dest_fifo_e2e_unix_test.go index 29de69a3..e6a1e841 100644 --- a/internal/cli/dest_fifo_e2e_unix_test.go +++ b/internal/cli/dest_fifo_e2e_unix_test.go @@ -275,6 +275,10 @@ func restoreDest(t *testing.T, path string, data []byte, mode os.FileMode) { t.Errorf("restoring %s: %v", path, err) return } + // On success the rename consumes tmp and this is a no-op; on any failure + // below it stops a stray .restore file being left in the applied home — + // which is the very contamination this helper exists to prevent. + defer func() { _ = os.Remove(tmp) }() // os.WriteFile's mode is masked by umask on create; chmod pins it. if err := os.Chmod(tmp, mode); err != nil { t.Errorf("restoring %s: %v", path, err) @@ -321,7 +325,12 @@ func TestRestoreDestReplacesAFIFOEvenWithAReaderAttached(t *testing.T) { // Called on the test goroutine: with a reader attached neither restoreDest // nor the os.WriteFile variant can block, so no timeout wrapper is needed — // and restoreDest's t.Errorf stays on the goroutine that owns the test. - restoreDest(t, path, []byte("applied"), 0o644) + // 0666, not 0644, and the choice is load-bearing: at the ambient umask 0022 + // os.WriteFile(_, _, 0o644) already yields 0644, so the mode assertion below + // would hold with or without the chmod — the pin would be vacuous in CI, + // which is exactly where it needs to work. 0666 masks down to 0644 on + // create, so only the chmod can produce it. + restoreDest(t, path, []byte("applied"), 0o666) info, err := os.Lstat(path) if err != nil { @@ -339,8 +348,8 @@ func TestRestoreDestReplacesAFIFOEvenWithAReaderAttached(t *testing.T) { // The chmod, which os.WriteFile alone cannot guarantee: its mode argument is // masked by umask on create, so without the explicit chmod a restored // destination can come back more restrictive than the one that was captured. - if perm := info.Mode().Perm(); perm != 0o644 { - t.Errorf("restored mode = %04o, want 0644: os.WriteFile's mode is umask-masked, so "+ + if perm := info.Mode().Perm(); perm != 0o666 { + t.Errorf("restored mode = %04o, want 0666: os.WriteFile's mode is umask-masked, so "+ "restoreDest must chmod to pin what it captured", perm) } } diff --git a/internal/cli/destread.go b/internal/cli/destread.go index d305d520..a8c7f96e 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -2,6 +2,7 @@ package cli import ( "errors" + "fmt" "os" "github.com/spxrogers/agentsync/internal/render" @@ -14,6 +15,17 @@ import ( // double it in the wrapped message. var errDestNotRegular = errors.New("not a regular file") +// errDestUnstattable is returned when the destination cannot be STAT'd for a +// reason other than absence — EACCES on a parent, ELOOP, ENOTDIR. It wraps the +// 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. +var errDestUnstattable = errors.New("cannot stat destination") + // readDestBytes reads a destination file's bytes, refusing before the open any // path whose shape cannot be read as a file. // @@ -30,7 +42,9 @@ var errDestNotRegular = errors.New("not a regular file") // covered: `apply`, `apply --dry-run`, `reconcile --auto-override` and // `import ` still hang, and `doctor` is exposed through its plugin // check, because those reads live in internal/render and the adapter Ingest -// paths (#241, #242). +// paths (#241, #242). `doctor` is not merely exposed there: with a FIFO at +// ~/.claude/settings.json it hangs outright (measured, rc=124, wedged after +// printing "Plugins"), through claude.IngestPlugins. // // Two deliberate limits: // @@ -38,6 +52,9 @@ var errDestNotRegular = errors.New("not a regular file") // open. That window needs write access to the destination's own directory, // which is already game over; closing it properly needs O_RDONLY|O_NONBLOCK // plus fstat. +// - A destination that cannot be stat'd comes back as errDestUnstattable +// wrapping the real errno, NOT as a shape error, because those are +// different claims and one of them is shown to 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 @@ -55,17 +72,17 @@ var errDestNotRegular = errors.New("not a regular file") // caller unchanged, because manufacturing a shape error for a file that is not // there would name the wrong problem. func readDestBytes(path string) ([]byte, error) { - // A stat failure that is NOT absence — EACCES on a parent, ELOOP — is - // reported as itself. render.IsRegularOrAbsent answers false for those too, - // and letting them through as errDestNotRegular would put a false statement - // in front of the user: reconcile's refusal names that sentinel and would - // tell someone with a permission problem to "remove or replace the - // non-regular file at that path". The extra stat costs one syscall on an - // error path and keeps render's predicate the single authority on SHAPE. - if _, serr := os.Stat(path); serr != nil && !os.IsNotExist(serr) { - return nil, serr - } + // 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". if !render.IsRegularOrAbsent(path) { + if _, serr := os.Stat(path); serr != nil { + return nil, fmt.Errorf("%w: %w", errDestUnstattable, serr) + } return nil, errDestNotRegular } return os.ReadFile(path) diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index e7e36bc7..ee64bd84 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -282,6 +282,24 @@ func TestHashFileSentinels(t *testing.T) { setup: mkfifoDest, want: "not-a-regular-file", }, + { + // The parity row. A destination that cannot be STAT'd at all — + // here because a parent component is a regular file, so the walk + // gets ENOTDIR — answered "not-a-regular-file" before this package + // had a read gate, and must still. Splitting it off to "" would + // read as absent, moving such a destination from ForeignCollision + // to New when nothing was applied — and New is SafeForAutoApply. + name: "an unstattable destination keeps the shape sentinel", + setup: func(t *testing.T, tmp string) string { + t.Helper() + blocker := filepath.Join(tmp, "notadir") + if err := os.WriteFile(blocker, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + return filepath.Join(blocker, "dest") + }, + want: "not-a-regular-file", + }, { name: "an ordinary regular file hashes its content", setup: func(t *testing.T, tmp string) string { diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index 9d515f7a..b0dc36eb 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -1158,9 +1158,11 @@ func writeBackFileItem(home string, it reconcileItem) error { // 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 absent-destination 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). + // refusals in this file and unlike the 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 // user out of a clean refusal and into an unbounded wedge. Restore that // suggestion only once #241 is fixed. diff --git a/internal/cli/status.go b/internal/cli/status.go index 83c65577..662ce8a3 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -963,8 +963,10 @@ func hashContent(b []byte) string { } // hashFile returns the SHA-256 hex digest of the file at path. Returns -// the empty string on missing-file errors (which `drift.Classify` reads -// as "absent" — the expected signal for Orphan / OrphanDrifted). +// the empty string when the destination cannot be read as content at all — +// absent, or present-and-unreadable — which `drift.Classify` reads as "absent", +// 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 @@ -993,7 +995,13 @@ func hashFile(path string) string { // refusal onto the sentinel above rather than re-deciding it. data, err := readDestBytes(path) if err != nil { - if errors.Is(err, errDestNotRegular) { + // 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. + if errors.Is(err, errDestNotRegular) || errors.Is(err, errDestUnstattable) { return "not-a-regular-file" } return "" From 0a45d0d11bff4059aa7954dc43d7179b9db0f0fb Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 12:29:05 +0000 Subject: [PATCH 9/9] fix(cli): land the round-6 fix that never applied; stop doubling the path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes round-7 review findings on PR #240. Two lenses returned CLEAN; the one real finding is that a fix I reported as done was not in the tree. 1. `43f4464`'s message said prose #15 was "Scoped." It was not. That commit does not touch destread_guard_internal_test.go at all — my script applied three replacements to dest_fifo_e2e_unix_test.go, but the third pattern lives in the guard test, and str.replace() silently no-ops on a missing pattern. Every other edit in that script asserted count==1; that one did not, so a silent miss became a claim of work done. Caught only by `git show --name-only`. Now actually applied: render.isRegularOrAbsent's `apply --dry-run` claim is TRUE of the orphan-delete read it guards and false only of Writer.Write's convergence read. Every edit in this commit went through an assertion, and a post-hoc audit greps the tree for each one. 2. The second sentinel violated the first's stated principle. errDestNotRegular is deliberately pathless because callers wrap it with the path; but errDestUnstattable wrapped a *fs.PathError, so reconcile printed "read dest X: cannot stat destination: stat X: not a directory". It now unwraps to the bare errno via pathlessStatErr, mirroring secrets.pathlessErr, and errors.Is still matches both the sentinel and the underlying syscall error. Pinned by counting path occurrences rather than matching a literal, so a reworded message will not break it; break-verified. 3. `internal/render/writer.go`'s doc claimed the predicate is what stops `apply --dry-run` hanging on a FIFO — flagged by two lenses and disproved by this PR's own skipped rows. It is true of the pre-delete read it guards and false of the convergence read; the sentence now says exactly that rather than being left for #241 to correct later. Also: "Two deliberate limits:" headed three bullets (the second was added in round 6 without bumping the count, and is a design statement rather than a limit); a reflow had orphaned "// whose" on its own line; and docs/components.md's not-fixed list omitted `doctor`, which the CHANGELOG and destread.go already named. Correctness validated base parity empirically rather than by reading: a verbatim copy of f6aa686's hashFile compared against head across 13 input classes, run as root AND as an unprivileged uid so the EACCES rows genuinely denied. All 13 identical. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- docs/components.md | 6 ++--- internal/cli/destread.go | 25 ++++++++++++++++---- internal/cli/destread_guard_internal_test.go | 7 +++--- internal/cli/destread_unix_internal_test.go | 12 ++++++++++ internal/cli/reconcile.go | 9 ++++--- internal/render/writer.go | 8 ++++--- 6 files changed, 49 insertions(+), 18 deletions(-) diff --git a/docs/components.md b/docs/components.md index f7895ccd..11237e28 100644 --- a/docs/components.md +++ b/docs/components.md @@ -400,9 +400,9 @@ symmetric with the dest→source write boundary (see architecture §7). possible spelling. It is **not** yet true of this package's own `Writer.Write` convergence read or of the adapter `Ingest` paths, so `apply`, `apply --dry-run`, - `reconcile --auto-override` (which re-applies through `Writer.Write`) and - `import ` still block on a non-regular destination — issues #241 and - #242. + `reconcile --auto-override` (which re-applies through `Writer.Write`), + `import ` and `doctor` (through its plugin check) still block on a + non-regular destination — issues #241 and #242. - **Depends on:** adapter, secrets, source, state, paths, iox, drift. - **Files:** `pipeline.go`, `writer.go`, `state_apply.go`, `report.go`. diff --git a/internal/cli/destread.go b/internal/cli/destread.go index a8c7f96e..d8aa3992 100644 --- a/internal/cli/destread.go +++ b/internal/cli/destread.go @@ -3,6 +3,7 @@ package cli import ( "errors" "fmt" + "io/fs" "os" "github.com/spxrogers/agentsync/internal/render" @@ -46,15 +47,16 @@ var errDestUnstattable = errors.New("cannot stat destination") // ~/.claude/settings.json it hangs outright (measured, rc=124, wedged after // printing "Plugins"), through claude.IngestPlugins. // -// Two deliberate limits: +// Three things to know about the shape check: // // - The check is a stat, so it is racy against a reshape between stat and // open. That window needs write access to the destination's own directory, // which is already game over; closing it properly needs O_RDONLY|O_NONBLOCK // plus fstat. // - A destination that cannot be stat'd comes back as errDestUnstattable -// wrapping the real errno, NOT as a shape error, because those are -// different claims and one of them is shown to a user. +// 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 @@ -71,6 +73,16 @@ var errDestUnstattable = errors.New("cannot stat destination") // An ABSENT path is not refused: os.ReadFile runs and its ENOENT reaches the // caller unchanged, because manufacturing a shape error for a file that is not // there would name the wrong problem. +// pathlessStatErr strips the redundant path from a *fs.PathError, mirroring +// secrets.pathlessErr. errors.Is still matches the underlying errno. +func pathlessStatErr(err error) error { + var pe *fs.PathError + if errors.As(err, &pe) { + return pe.Err + } + return err +} + func readDestBytes(path string) ([]byte, error) { // render.IsRegularOrAbsent stays the single authority on SHAPE, and it is // asked FIRST so the ordinary read costs exactly one stat. It answers false @@ -81,7 +93,12 @@ func readDestBytes(path string) ([]byte, error) { // permission problem to "remove or replace the non-regular file". if !render.IsRegularOrAbsent(path) { if _, serr := os.Stat(path); serr != nil { - return nil, fmt.Errorf("%w: %w", errDestUnstattable, serr) + // Pathless, for the reason errDestNotRegular carries no path: the + // caller supplies it, and a *fs.PathError would make reconcile print + // "read dest X: cannot stat destination: stat X: ...". Unwrapping to + // the bare errno keeps errors.Is matching BOTH this sentinel and the + // underlying syscall error. + return nil, fmt.Errorf("%w: %w", errDestUnstattable, pathlessStatErr(serr)) } return nil, errDestNotRegular } diff --git a/internal/cli/destread_guard_internal_test.go b/internal/cli/destread_guard_internal_test.go index 970ae4b1..40c12d26 100644 --- a/internal/cli/destread_guard_internal_test.go +++ b/internal/cli/destread_guard_internal_test.go @@ -85,9 +85,10 @@ func TestEveryDestinationReadGoesThroughTheGate(t *testing.T) { // Apply paths also read op.Path, but on the WRITE path, which this // change does not cover. Do NOT read that as "handled elsewhere": // render.isRegularOrAbsent's own doc says it is what stops - // `apply --dry-run` hanging on a FIFO, and that is false — Writer.Write's - // convergence read never calls it, and `apply --dry-run` still hangs - // (#241). This guard makes no claim about those reads either way. + // `apply --dry-run` hanging on a FIFO. That is true of the orphan-delete + // read it actually guards, and false of Writer.Write's convergence read, + // which never calls it — so `apply --dry-run` still hangs (#241). This + // guard makes no claim about those reads either way. if !strings.HasPrefix(rel, "internal/cli/") { return } diff --git a/internal/cli/destread_unix_internal_test.go b/internal/cli/destread_unix_internal_test.go index ee64bd84..b940dc50 100644 --- a/internal/cli/destread_unix_internal_test.go +++ b/internal/cli/destread_unix_internal_test.go @@ -393,4 +393,16 @@ func TestReadDestBytesReportsAStatFailureAsItself(t *testing.T) { if !errors.Is(err, syscall.ELOOP) { t.Errorf("error = %v, want it to wrap ELOOP", err) } + if !errors.Is(err, errDestUnstattable) { + t.Errorf("error = %v, want it to wrap errDestUnstattable so hashFile can map it "+ + "to the shape sentinel and keep base parity", err) + } + // Pathless, like its sibling sentinel: the caller supplies the path, and a + // *fs.PathError here makes reconcile print "read dest X: cannot stat + // destination: stat X: ...". Counting occurrences rather than asserting a + // literal keeps this from breaking on a reworded message. + if n := strings.Count(err.Error(), a); n != 0 { + t.Errorf("error = %q names the path %d time(s); it must carry none — the caller "+ + "wraps it with the path and a *fs.PathError would double it", err, n) + } } diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index b0dc36eb..419d480a 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -1158,11 +1158,10 @@ func writeBackFileItem(home string, it reconcileItem) error { // 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 shape-guarded, so on this exact item it - // does not fail — it HANGS (measured: `reconcile --auto-override` - // rc=124). + // refusals in this file and unlike the 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 // user out of a clean refusal and into an unbounded wedge. Restore that // suggestion only once #241 is fixed. diff --git a/internal/render/writer.go b/internal/render/writer.go index 3d00ce52..15d6efa3 100644 --- a/internal/render/writer.go +++ b/internal/render/writer.go @@ -377,9 +377,11 @@ func IsRegularOrAbsent(path string) bool { return isRegularOrAbsent(path) } // // It uses Stat, which FOLLOWS symlinks, so a symlink pointing at a FIFO is // caught as well as a bare one. That matters because os.Open on a FIFO with no -// writer BLOCKS rather than failing: without this, `apply --dry-run` (advertised -// as read-only) and the real apply's pre-delete read would both hang forever on -// a FIFO left at a destination path. A dangling symlink reports absent, which is +// writer BLOCKS rather than failing: without this, the pre-delete read below +// would hang forever on a FIFO left at a destination path, in the real apply and +// in `apply --dry-run` alike. It does NOT cover Writer.Write's convergence read, +// which never calls it — so `apply --dry-run` still hangs on a FIFO at a +// RENDERED destination (#241). A dangling symlink reports absent, which is // the right answer — the link is removable and carries nothing to preserve. func isRegularOrAbsent(path string) bool { fi, err := os.Stat(path)