From 85dcd4fd2850ff5db4da3fdbf5d555fa16bc424c Mon Sep 17 00:00:00 2001 From: Logan Lindquist Land Date: Tue, 20 Oct 2026 18:27:31 -0500 Subject: [PATCH] fix: exit 1 when scan --fail-on-risk or inventory could not read every path `scan --fail-on-risk` now exits 1 when the scan skipped a path it could not read or classify, in text and `--json`. Plain `scan` still exits 0. Before, a CI gate built on it passed on an incomplete scan. `inventory` still completes every mode (write, `--check`, `--stdout`) when paths were skipped, then lists them on stderr and exits 1. A mode that already failed keeps its own exit code. `optimize` keeps exit 2 for an unreadable path, since it refuses before any paid call. docs/exit-codes.md, scan, inventory, README and usage text now agree on these codes. --- README.md | 8 ++- cmd/agentsync/inventory.go | 47 +++++++++---- cmd/agentsync/inventory_test.go | 113 ++++++++++++++++++++++++++++++++ cmd/agentsync/main.go | 20 ++++-- cmd/agentsync/main_test.go | 50 ++++++++++++++ docs/exit-codes.md | 14 ++-- docs/inventory.md | 7 +- docs/scan.md | 8 +-- 8 files changed, 235 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index af3a179..c271756 100644 --- a/README.md +++ b/README.md @@ -11,11 +11,15 @@ replacing the file with a link loses nothing. It is built to be safe to run: -- Every command that writes is a dry run until you pass `--apply`. +- Every command that writes is a dry run until you pass `--apply`, except that + `init` in a terminal writes once you confirm, `inventory` rewrites its own + derived file, and `reconcile` records a deliberately deleted link in the + state file. - It refuses to link a project holding a file that is backed up nowhere. - It removes a symlink only if it recorded creating it and the link is broken, and deletes a captured file only if git still holds its bytes. -- Anything it cannot determine is a refusal with exit 2, never a guessed default. +- Anything it cannot determine is a refusal, never a guessed default (see + [exit codes](docs/exit-codes.md)). ## Install diff --git a/cmd/agentsync/inventory.go b/cmd/agentsync/inventory.go index 6f214ca..5bc3375 100644 --- a/cmd/agentsync/inventory.go +++ b/cmd/agentsync/inventory.go @@ -46,7 +46,8 @@ func runInventory(args []string, stdout, stderr io.Writer) int { out := fs.String("out", "", "write here instead of /"+inventoryFileName) toStdout := fs.Bool("stdout", false, "write to stdout and leave the file alone") check := fs.Bool("check", false, - "write nothing; exit 1 if the file on disk is not what a regeneration would produce") + "write nothing; exit 1 if the file on disk is not what a regeneration would produce, "+ + "or a path could not be read or classified") allowOutsideHome := fs.Bool(scan.AllowOutsideHomeFlag, false, "projects legitimately live outside $HOME; suppress the home warning") @@ -122,30 +123,50 @@ func runInventory(args []string, stdout, stderr io.Writer) int { emitRepoChoice(reportStream(*toStdout, stdout, stderr), repo, repoFrom) emitRootChoice(reportStream(*toStdout, stdout, stderr), repo.Home, root) - if *toStdout { - if _, err := stdout.Write(want); err != nil { - fmt.Fprintf(stderr, "agentsync: writing to stdout: %v\n", err) - return exitFailure - } - return exitOK - } - dest := *out if dest == "" { dest = filepath.Join(repo.Root, inventoryFileName) } + code, wrote := emitInventory(*toStdout, *check, dest, want, inv, stdout, stderr) - if *check { - return checkInventory(dest, want, stdout, stderr) + // Each mode finishes its job first; a skipped path then makes an + // otherwise clean run exit 1, since the document is missing what it hides. + if n := len(res.Problems); n > 0 { + emitProblems(stderr, res.Problems) + paths := fmt.Sprintf("%d %s could not be read or classified and %s missing", + n, plural(n, "path", "paths"), plural(n, "is", "are")) + if wrote { + fmt.Fprintf(stderr, "agentsync: inventory written, but %s from it\n", paths) + } else { + fmt.Fprintf(stderr, "agentsync: %s from the inventory\n", paths) + } + if code == exitOK { + code = exitAtRisk + } } + return code +} +// emitInventory runs the selected mode and reports whether dest was written. +func emitInventory(toStdout, check bool, dest string, want []byte, inv inventory.Inventory, + stdout, stderr io.Writer) (code int, wrote bool) { + if toStdout { + if _, err := stdout.Write(want); err != nil { + fmt.Fprintf(stderr, "agentsync: writing to stdout: %v\n", err) + return exitFailure, false + } + return exitOK, false + } + if check { + return checkInventory(dest, want, stdout, stderr), false + } if err := writeInventory(dest, want); err != nil { fmt.Fprintf(stderr, "agentsync: %v\n", err) - return exitFailure + return exitFailure, false } fmt.Fprintf(stdout, "Wrote %s\n", dest) emitInventorySummary(stdout, inv) - return exitOK + return exitOK, true } // checkInventory compares the file on disk against a fresh render. diff --git a/cmd/agentsync/inventory_test.go b/cmd/agentsync/inventory_test.go index f8560e6..685720e 100644 --- a/cmd/agentsync/inventory_test.go +++ b/cmd/agentsync/inventory_test.go @@ -343,3 +343,116 @@ func TestRunInventory_DoesNotWarnWithNoPluginTree(t *testing.T) { t.Errorf("warned with no plugin tree at all:\n%s", stderr) } } + +// --- unreadable paths ------------------------------------------------------- + +// Every mode still does its job over what it could read, then names the +// skipped path and exits 1: the document is silently short by whatever it hides. +func TestRunInventory_UnreadablePathIsReportedAndExitsOne(t *testing.T) { + t.Run("default writes the readable projects", func(t *testing.T) { + base, repoRoot, root := inventoryFixture(t) + mustWriteT(t, projectAgentPath(base, "Web/one", "solo"), "only here\n") + unreadable := lockedSubtree(t, root) + + code, stdout, stderr := runCLI(t, inventoryArgs(t, repoRoot, root)...) + if code != exitAtRisk { + t.Fatalf("exit = %d, want %d\nstdout:\n%s\nstderr:\n%s", code, exitAtRisk, stdout, stderr) + } + if !strings.Contains(readFileT(t, inventoryPath(repoRoot)), "solo") { + t.Error("the readable project is missing from the written document") + } + if !strings.Contains(stdout, "Wrote ") { + t.Errorf("stdout does not report the write:\n%s", stdout) + } + if !strings.Contains(stderr, unreadable) { + t.Errorf("stderr does not name the unreadable path:\n%s", stderr) + } + want := "agentsync: inventory written, but 1 path could not be read or classified and is missing from it" + if !strings.Contains(stderr, want) { + t.Errorf("stderr lacks %q:\n%s", want, stderr) + } + }) + + t.Run("--check exits 1 against a file that otherwise matches", func(t *testing.T) { + _, repoRoot, root := inventoryFixture(t) + if code, _, stderr := runCLI(t, inventoryArgs(t, repoRoot, root)...); code != exitOK { + t.Fatalf("write exit = %d: %s", code, stderr) + } + unreadable := lockedSubtree(t, root) + + code, stdout, stderr := runCLI(t, inventoryArgs(t, repoRoot, root, "--check")...) + if code != exitAtRisk { + t.Fatalf("exit = %d, want %d\nstdout:\n%s\nstderr:\n%s", code, exitAtRisk, stdout, stderr) + } + // The comparison itself passed, so the 1 comes from the skipped path alone. + if !strings.Contains(stdout, "up to date") { + t.Errorf("the file was expected to match the readable tree:\n%s", stdout) + } + if !strings.Contains(stderr, unreadable) { + t.Errorf("stderr does not name the unreadable path:\n%s", stderr) + } + want := "agentsync: 1 path could not be read or classified and is missing from the inventory" + if !strings.Contains(stderr, want) { + t.Errorf("stderr lacks %q:\n%s", want, stderr) + } + }) + + t.Run("--stdout prints the document", func(t *testing.T) { + base, repoRoot, root := inventoryFixture(t) + mustWriteT(t, projectAgentPath(base, "Web/one", "solo"), "only here\n") + unreadable := lockedSubtree(t, root) + + code, stdout, stderr := runCLI(t, inventoryArgs(t, repoRoot, root, "--stdout")...) + if code != exitAtRisk { + t.Fatalf("exit = %d, want %d\nstderr:\n%s", code, exitAtRisk, stderr) + } + var doc map[string]any + if err := json.Unmarshal([]byte(stdout), &doc); err != nil { + t.Fatalf("stdout is not the JSON document: %v\n%s", err, stdout) + } + if !strings.Contains(stdout, "solo") { + t.Errorf("the readable project is missing from the document:\n%s", stdout) + } + if !strings.Contains(stderr, unreadable) { + t.Errorf("stderr does not name the unreadable path:\n%s", stderr) + } + if _, err := os.Stat(inventoryPath(repoRoot)); !os.IsNotExist(err) { + t.Errorf("--stdout touched the file: stat err = %v", err) + } + }) + + // A mode that already failed keeps its 2; the skipped path must not lower it. + t.Run("--check that cannot read the file keeps exit 2", func(t *testing.T) { + _, repoRoot, root := inventoryFixture(t) + unreadable := lockedSubtree(t, root) + dest := filepath.Join(t.TempDir(), "in-the-way") + mustMkdirT(t, dest) + + code, _, stderr := runCLI(t, inventoryArgs(t, repoRoot, root, "--check", "--out", dest)...) + if code != exitFailure { + t.Fatalf("exit = %d, want %d\nstderr:\n%s", code, exitFailure, stderr) + } + if !strings.Contains(stderr, unreadable) { + t.Errorf("stderr does not name the unreadable path:\n%s", stderr) + } + }) + + t.Run("a failed write keeps exit 2 and does not claim it wrote", func(t *testing.T) { + _, repoRoot, root := inventoryFixture(t) + unreadable := lockedSubtree(t, root) + file := filepath.Join(t.TempDir(), "a-file") + mustWriteT(t, file, "not a directory\n") + + code, _, stderr := runCLI(t, inventoryArgs(t, repoRoot, root, "--out", filepath.Join(file, "inv.json"))...) + if code != exitFailure { + t.Fatalf("exit = %d, want %d\nstderr:\n%s", code, exitFailure, stderr) + } + if strings.Contains(stderr, "inventory written") { + t.Errorf("stderr claims a write that failed:\n%s", stderr) + } + want := "agentsync: 1 path could not be read or classified and is missing from the inventory" + if !strings.Contains(stderr, want) || !strings.Contains(stderr, unreadable) { + t.Errorf("stderr lacks %q or the unreadable path:\n%s", want, stderr) + } + }) +} diff --git a/cmd/agentsync/main.go b/cmd/agentsync/main.go index 667e0b0..ed374af 100644 --- a/cmd/agentsync/main.go +++ b/cmd/agentsync/main.go @@ -89,7 +89,8 @@ Common flags: --all scan the whole default tree ($AGENTSYNC_ROOT, else root in the config file) rather than the project the working directory is in - --json emit machine-readable JSON + --json emit machine-readable JSON (scan, import, link, doctor, + reconcile, optimize) --allow-outside-home your projects legitimately live outside $HOME; do not warn (scan, doctor, and a reconcile dry run) or refuse (import, @@ -124,7 +125,8 @@ the candidates and prints the exact command to run, and it writes only with scan flags: --at-risk list only entries whose content is protected nowhere - --fail-on-risk exit 1 if any at-risk entry is found (for CI) + --fail-on-risk exit 1 if any at-risk entry is found or a path could not + be read or classified (for CI) import flags: --apply write the captures; without it import is a dry run @@ -225,7 +227,8 @@ inventory flags: regeneration would produce. The document holds nothing volatile — no timestamp, no tool version — so a difference is a real change in the tree, which is what makes this a - freshness test rather than a clock reading. + freshness test rather than a clock reading. Also exit 1 + if a path could not be read or classified. optimize flags: --advisor MODEL OpenRouter decision model to ask (default ~typesafe/jev-latest; @@ -270,6 +273,8 @@ output is inventory.json, which it owns, recomputes from scratch every run, and overwrites atomically. That file is derived — the filesystem is the source of truth and regenerating is the documented fix for any inconsistency — so nothing may decide a write from it. Read it for orientation; re-scan before acting. +A path it could not read or classify is skipped and reported, and the run +exits 1. doctor never writes anywhere at all: it reports broken links, foreign links, links pointing at the wrong file in this repo, captures whose project no longer @@ -285,8 +290,8 @@ bytes are in no commit. doctor's exit code, not reconcile's, is what asserts that nothing has drifted. Exit codes: 0 clean, 1 something needs resolving (at-risk files, conflicts, a -blocked or broken project, or a path the scan could not read), 2 the tool could -not do its job. +blocked or broken project, or a path the scan could not read: scan only with +--fail-on-risk, optimize 2 instead), 2 the tool could not do its job. ` // Exit codes. These are part of the CLI contract: CI keys off 1 meaning @@ -430,7 +435,8 @@ func runScan(args []string, stdout, stderr io.Writer) int { all := registerAll(fs) asJSON := fs.Bool("json", false, "emit JSON") atRiskOnly := fs.Bool("at-risk", false, "list only at-risk entries") - failOnRisk := fs.Bool("fail-on-risk", false, "exit 1 if any at-risk entry is found") + failOnRisk := fs.Bool("fail-on-risk", false, + "exit 1 if any at-risk entry is found or a path could not be read or classified") allowOutsideHome := fs.Bool(scan.AllowOutsideHomeFlag, false, "projects legitimately live outside $HOME; suppress the home warning") @@ -481,7 +487,7 @@ func runScan(args []string, stdout, stderr io.Writer) int { emitText(stdout, res, repo.Root, root.Path, *atRiskOnly, entryOverrides(res, stderr)) } - if *failOnRisk && len(res.AtRisk()) > 0 { + if *failOnRisk && (len(res.AtRisk()) > 0 || len(res.Problems) > 0) { return exitAtRisk } return exitOK diff --git a/cmd/agentsync/main_test.go b/cmd/agentsync/main_test.go index 7d0e5e9..1130abc 100644 --- a/cmd/agentsync/main_test.go +++ b/cmd/agentsync/main_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/agentic-tooling/agentsync/internal/scan" + "github.com/agentic-tooling/agentsync/internal/testfs" ) // --- helpers --------------------------------------------------------------- @@ -591,6 +592,44 @@ func TestRunScan_EndToEnd_ProblemDoesNotHideRisk(t *testing.T) { } } +// An unreadable path fails --fail-on-risk even with nothing at risk, since it +// may hide at-risk files; without the flag scan stays a report and exits 0. +func TestRunScan_UnreadablePathFailsOnRisk(t *testing.T) { + base := t.TempDir() + repoRoot := filepath.Join(base, "agents-repo") + mustMkdirT(t, filepath.Join(repoRoot, "agents")) + unreadable := lockedSubtree(t, base) + + if code, stdout, stderr := runCLI(t, scanArgs(t, repoRoot, base)...); code != exitOK { + t.Errorf("without --fail-on-risk exit = %d, want %d\nstdout:\n%s\nstderr:\n%s", + code, exitOK, stdout, stderr) + } + + code, stdout, stderr := runCLI(t, scanArgs(t, repoRoot, base, "--fail-on-risk")...) + if code != exitAtRisk { + t.Errorf("--fail-on-risk exit = %d, want %d\nstdout:\n%s\nstderr:\n%s", + code, exitAtRisk, stdout, stderr) + } + if !strings.Contains(stdout, "1 path could not be checked") { + t.Errorf("the summary does not count the unread path:\n%s", stdout) + } + + code, stdout, stderr = runCLI(t, scanArgs(t, repoRoot, base, "--json", "--fail-on-risk")...) + if code != exitAtRisk { + t.Errorf("--json --fail-on-risk exit = %d, want %d\nstderr:\n%s", code, exitAtRisk, stderr) + } + var res scan.Result + if err := json.Unmarshal([]byte(stdout), &res); err != nil { + t.Fatalf("--json output does not parse: %v\n%s", err, stdout) + } + if n := len(res.AtRisk()); n != 0 { + t.Errorf("fixture has %d at-risk entries, want 0", n) + } + if len(res.Problems) != 1 || res.Problems[0].Path != unreadable { + t.Errorf("problems = %v, want exactly %s", res.Problems, unreadable) + } +} + // useRepo names the repo the way a user's environment does, and points the // config lookup at an empty directory so the author's real file is never read. func useRepo(t *testing.T, repoRoot string) { @@ -616,6 +655,17 @@ func inTree(t *testing.T, repoRoot, root string) { // --- local fs helpers (the scan package's own are unexported) -------------- +// lockedSubtree plants a project the scan cannot enter under root and returns +// the path it will report. +func lockedSubtree(t *testing.T, root string) string { + t.Helper() + locked := filepath.Join(root, "locked") + mustMkdirT(t, locked) + unreadable, inside := testfs.UnreadableSubtree(t, locked) + testfs.PlantAgentsDir(t, inside, "proj") + return unreadable +} + func mustMkdirT(t *testing.T, dir string) { t.Helper() if err := os.MkdirAll(dir, 0o755); err != nil { diff --git a/docs/exit-codes.md b/docs/exit-codes.md index 6987622..52eea6d 100644 --- a/docs/exit-codes.md +++ b/docs/exit-codes.md @@ -4,20 +4,24 @@ |---|---| | `0` | clean | | `1` | the run completed and found something a human must settle | -| `2` | agentsync could not do its job: an unknown flag or a stray argument, missing configuration, an unreadable path, a refused write | +| `2` | agentsync could not do its job: an unknown flag or a stray argument, missing configuration, a refused write | + +A path the scan could not read is exit 1 for every command that scans, except +`scan`, which exits 1 for it only with `--fail-on-risk`, and `optimize`, which +refuses with 2 before spending anything. What counts as exit 1, by command: | Command | Exit 1 when | |---|---| | `init` | a config file already exists, no candidate repo was found, or the user stopped at a prompt | -| `scan` | never by itself; with `--fail-on-risk`, any at-risk file | +| `scan` | never by itself; with `--fail-on-risk`, any at-risk file or unreadable path | | `import` | a conflicting capture, an unreadable path, or (with `--fail-on-pending`) anything uncaptured | | `link` | a blocked project, a broken or mismatched link, or an unreadable path | | `doctor` | a broken or mismatched link, a stale capture, an orphaned capture directory, an override that cannot run, or a project path the scan could not read (a capture or settings file it could not judge is exit 2) | -| `reconcile` | a refused selector, a conflict, a diverged or dereferenced link, or a removable link left in place | -| `inventory --check` | the file is stale or absent | -| `optimize` | an agent's advice is unresolved, or it was skipped by the cost cap or a stopped run; with `--apply`, a change was refused (a foreign link, a dereferenced link, a read-only or changed file, or frontmatter that cannot be edited safely) | +| `reconcile` | a refused selector, a conflict, a diverged or dereferenced link, a removable link left in place, or a path the scan could not read | +| `inventory` | a path could not be read or classified (the rest is still written, checked or printed); with `--check`, the file is stale or absent | +| `optimize` | an agent's advice is unresolved, or it was skipped by the cost cap or a stopped run; with `--apply`, a change was refused (a foreign link, a dereferenced link, a read-only or changed file, or frontmatter that cannot be edited safely); a path the scan could not read is exit 2 | | `override` | never | One exception: the hidden `agentsync hook claude-agent`, which Claude Code runs diff --git a/docs/inventory.md b/docs/inventory.md index c987aea..35ebce4 100644 --- a/docs/inventory.md +++ b/docs/inventory.md @@ -16,13 +16,18 @@ agentsync inventory --out ./inv.json |---|---| | `--out PATH` | write here instead of `/inventory.json` | | `--stdout` | write the document to stdout and leave the file alone | -| `--check` | write nothing; exit 1 if the file is not what a regeneration would produce | +| `--check` | write nothing; exit 1 if the file is not what a regeneration would produce, or a path could not be read or classified | `inventory` takes no `--apply`. It only ever rewrites its own derived file, recomputed from scratch and replaced atomically, so re-running is always safe. It never derives its root from the working directory; see [configuration](configuration.md). +**Unreadable paths.** A path the scan cannot read or classify is skipped. Every +mode still does its job over the rest (writes, compares or prints), then lists +each skipped path and its error on stderr and exits 1, since the document is +missing whatever that path holds. + ## Derived, never authoritative The filesystem is the source of truth. Nothing decides from `inventory.json`: diff --git a/docs/scan.md b/docs/scan.md index b13fa13..06cdd52 100644 --- a/docs/scan.md +++ b/docs/scan.md @@ -7,7 +7,7 @@ repo. It is read-only. agentsync scan # the project you are in agentsync scan --all # every project under the default tree agentsync scan --at-risk # only files protected nowhere else -agentsync scan --all --fail-on-risk # exit 1 if any at-risk file exists (CI) +agentsync scan --all --fail-on-risk # exit 1 on any at-risk file or unread path (CI) ``` An entry is "at risk" when its content exists nowhere else: no copy in the @@ -18,12 +18,12 @@ are captured. | Flag | Meaning | |---|---| | `--at-risk` | list only entries whose content is protected nowhere | -| `--fail-on-risk` | exit 1 if any at-risk entry is found | +| `--fail-on-risk` | exit 1 if any at-risk entry is found or a path could not be read or classified | `scan` exits 0 even when it reports at-risk files, because its exit code is a report. Use `--fail-on-risk` when you want an assertion. A path the scan could -not read or classify is reported and qualifies the summary, but does not -change the exit code. +not read or classify is reported and qualifies the summary; it changes the exit +code only with `--fail-on-risk`, which exits 1 for it, as it does with `--json`. An agent with an [override](override.md) in its project's `.claude/settings.local.json` shows it on its line, as `override: sonnet /