From 3a801d6616213d196b6e0dc529e59f909be3137a Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:46:45 +0100 Subject: [PATCH 1/3] refactor: extract cli.Run for testable error rendering Move the error->exit-code mapping out of main and into a cli.Run(args, stdout, stderr) int seam so the front door's error surface is injectable and testable. Behaviour-preserving: main becomes a one-line delegate and the raw-text diagnostic is unchanged. Assisted-by: Claude:claude-opus-4-8 --- cmd/abcd/main.go | 17 +---------------- internal/surface/cli/cli.go | 29 +++++++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/cmd/abcd/main.go b/cmd/abcd/main.go index 34527d3e..82e070e5 100644 --- a/cmd/abcd/main.go +++ b/cmd/abcd/main.go @@ -3,26 +3,11 @@ package main import ( - "errors" - "fmt" "os" "github.com/REPPL/abcd-cli/internal/surface/cli" ) func main() { - if err := cli.Execute(); err != nil { - // A command may request a specific exit code (usage errors, the memory - // lint curator contract). An empty message means it already rendered its - // output and only the exit code should propagate. - var coded interface{ ExitCode() int } - if errors.As(err, &coded) { - if msg := err.Error(); msg != "" { - fmt.Fprintln(os.Stderr, "abcd:", msg) - } - os.Exit(coded.ExitCode()) - } - fmt.Fprintln(os.Stderr, "abcd:", err) - os.Exit(1) - } + os.Exit(cli.Run(os.Args[1:], os.Stdout, os.Stderr)) } diff --git a/internal/surface/cli/cli.go b/internal/surface/cli/cli.go index 53b882e9..0dbecf69 100644 --- a/internal/surface/cli/cli.go +++ b/internal/surface/cli/cli.go @@ -8,6 +8,7 @@ package cli import ( "bufio" "encoding/json" + "errors" "fmt" "io" "os" @@ -1447,8 +1448,32 @@ func repoRootSHA() (string, error) { } // Execute runs the root command; main sets the process exit code on error. -func Execute() error { - return NewRootCommand().Execute() +// Run builds the command tree, executes it against args, and renders any error +// as a single diagnostic line — the one place that maps a command error to a +// process exit code, so main stays a thin shell. stdout/stderr are injected so +// the whole front door (including its error surface) is testable. +func Run(args []string, stdout, stderr io.Writer) int { + root := NewRootCommand() + root.SetArgs(args) + root.SetOut(stdout) + root.SetErr(stderr) + + err := root.Execute() + if err == nil { + return 0 + } + // A command may request a specific exit code (usage errors, the memory-lint + // curator contract). An empty message means it already rendered its output + // and only the exit code should propagate. + var coded interface{ ExitCode() int } + if errors.As(err, &coded) { + if msg := err.Error(); msg != "" { + fmt.Fprintln(stderr, "abcd:", msg) + } + return coded.ExitCode() + } + fmt.Fprintln(stderr, "abcd:", err) + return 1 } // render writes v as indented JSON when asJSON is set, otherwise delegates to From 5c4542d894560799f00cabd2b6e0a535b6203288 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:57:48 +0100 Subject: [PATCH 2/3] fix: fail-closed capture surface (iss-29) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three coupled instances behind the unrecognized-input-never-writes detector, each with a watched-fail acceptance test: - A mistyped capture subcommand (e.g. `capture resovle iss-1 note`) was swallowed as free text and filed as a new issue. suspectedTypoedSubcommand now refuses it with a did-you-mean and writes nothing. The guard is high-precision — it fires only on a near-miss token that is alone or followed by an iss-id, so genuine free-text prose still files. - Errors requested with --json emitted raw Go text. cli.Run now renders the diagnostic as a {"error": ...} JSON envelope under --json. - `docs lint` with a missing/unreadable config surfaced a raw os.ReadFile error that leaked the absolute config path. It now reports a clean, repo-relative diagnostic; a *PathError's inner cause is reported without its path. Resolves iss-29 (folded into the branch). The systemic PathError-into-json leak across other verbs is split out as iss-76. Assisted-by: Claude:claude-opus-4-8 --- .../iss-29-fail-closed-capture-surface.md | 1 + CHANGELOG.md | 7 + internal/surface/cli/capture_surface_test.go | 166 ++++++++++++++++++ internal/surface/cli/cli.go | 106 ++++++++++- 4 files changed, 275 insertions(+), 5 deletions(-) rename .abcd/work/issues/{open => resolved}/iss-29-fail-closed-capture-surface.md (70%) create mode 100644 internal/surface/cli/capture_surface_test.go diff --git a/.abcd/work/issues/open/iss-29-fail-closed-capture-surface.md b/.abcd/work/issues/resolved/iss-29-fail-closed-capture-surface.md similarity index 70% rename from .abcd/work/issues/open/iss-29-fail-closed-capture-surface.md rename to .abcd/work/issues/resolved/iss-29-fail-closed-capture-surface.md index 2a0a1bcc..5989b9bc 100644 --- a/.abcd/work/issues/open/iss-29-fail-closed-capture-surface.md +++ b/.abcd/work/issues/resolved/iss-29-fail-closed-capture-surface.md @@ -7,6 +7,7 @@ category: "bug" source: "agent-finding" found_during: "2026-07-08 multi-agent review" found_at: "internal/surface/cli/cli.go" +resolution: "Fixed the three fail-closed-capture instances behind the unrecognized-input-never-writes detector: (1) typo'd subcommand refused with did-you-mean + no write (suspectedTypoedSubcommand); (2) --json errors JSON-shaped via cli.Run; (3) docs-lint missing/unreadable config surfaces a repo-relative, path-safe error. Systemic PathError-into-json leak split out as iss-76." --- fail-closed capture surface: a misspelled capture subcommand (e.g. capture resovle) is swallowed as capture text and files a NEW issue instead of erroring (internal/surface/cli/cli.go:455) — a typo becomes a ledger mutation; --json errors emit raw Go text, not JSON (internal/surface/cli/cli.go:165), and abcd docs lint without a config surfaces a raw file error. Detector (per unrecognized-input-never-writes): a surface test convention where every mutating verb has a malformed-input case asserting an error, a did-you-mean for near-misses, and no write occurred, plus a --json error-shape contract test. Acceptance corpus: the three instances above — the detector is proven when it flags all three. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a75bea8..cb309aa8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -137,6 +137,13 @@ called out in a **Breaking** section. ### Fixed +- **Fail-closed capture surface** (iss-29). A mistyped `capture` subcommand + (e.g. `abcd capture resovle iss-1 …`) is no longer swallowed as free text and + filed as a new issue; it is refused with a did-you-mean and writes nothing. + Errors requested with `--json` are now emitted as a `{"error": …}` envelope + rather than raw Go text, and `abcd docs lint` with a missing or unreadable + config reports a clean, repo-relative diagnostic instead of a raw file error + that leaked the absolute config path. - `abcd` status now reports `IsGitRepo` correctly in a linked git worktree or a submodule, where `.git` is a regular gitfile rather than a directory (iss-72). - `abcd intent plan` now refuses an `## Acceptance Criteria` section with no diff --git a/internal/surface/cli/capture_surface_test.go b/internal/surface/cli/capture_surface_test.go new file mode 100644 index 00000000..36386459 --- /dev/null +++ b/internal/surface/cli/capture_surface_test.go @@ -0,0 +1,166 @@ +package cli + +import ( + "bytes" + "encoding/json" + "io/fs" + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// The three tests below are iss-29's acceptance corpus for the +// "unrecognized-input-never-writes" detector: a mistyped mutating subcommand +// must error without writing, --json errors must be JSON-shaped, and a missing +// config must surface a clean, path-safe error rather than raw Go text. + +var reIssueFile = regexp.MustCompile(`^iss-\d+-.*\.md$`) + +// ledgerIssueCount walks a repo tree and counts written ledger issue files. +func ledgerIssueCount(t *testing.T, root string) int { + t.Helper() + n := 0 + err := filepath.WalkDir(root, func(_ string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if !d.IsDir() && reIssueFile.MatchString(d.Name()) { + n++ + } + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", root, err) + } + return n +} + +// TestCaptureTypoSubcommandNeverWrites is the headline: `capture resovle iss-1 +// note` (a typo for `resolve`) must be refused with a did-you-mean, and must +// not file a new issue. Before the fix it was swallowed as free text and wrote. +func TestCaptureTypoSubcommandNeverWrites(t *testing.T) { + repo := t.TempDir() + t.Chdir(repo) + + out, err := runCLIErr(t, "capture", "resovle", "iss-1", "clear the flake") + if err == nil { + t.Fatalf("expected an error for the mistyped subcommand, got success:\n%s", out) + } + if !strings.Contains(err.Error(), "resolve") { + t.Fatalf("expected a did-you-mean pointing at %q, got: %v", "resolve", err) + } + if n := ledgerIssueCount(t, repo); n != 0 { + t.Fatalf("a mistyped subcommand filed %d issue(s); it must write nothing", n) + } +} + +// TestCaptureFreeTextStillWrites guards the contract: a genuine free-text +// capture whose first word merely resembles a subcommand (but is followed by +// prose, not an iss-id) still files. The typo guard must be high-precision. +func TestCaptureFreeTextStillWrites(t *testing.T) { + repo := t.TempDir() + t.Chdir(repo) + + out := runCLI(t, "capture", "resolved a flaky parser test by widening the timeout", "--json") + var r struct { + ID string `json:"id"` + } + if err := json.Unmarshal(out, &r); err != nil { + t.Fatalf("capture output not JSON: %v\n%s", err, out) + } + if r.ID != "iss-1" { + t.Fatalf("free-text capture id = %q, want iss-1", r.ID) + } + if n := ledgerIssueCount(t, repo); n != 1 { + t.Fatalf("free-text capture wrote %d issue(s), want 1", n) + } +} + +// TestJSONErrorShapeIsJSON is the --json error-shape contract: when the caller +// asked for --json, a command error is emitted as a JSON envelope, not raw Go +// text. `capture list --json` (no state flag) is a stable erroring case. +func TestJSONErrorShapeIsJSON(t *testing.T) { + repo := t.TempDir() + t.Chdir(repo) + + var stdout, stderr bytes.Buffer + code := Run([]string{"capture", "list", "--json"}, &stdout, &stderr) + if code == 0 { + t.Fatalf("expected a non-zero exit for `capture list` with no state flag") + } + var env struct { + Error string `json:"error"` + } + if err := json.Unmarshal(stderr.Bytes(), &env); err != nil { + t.Fatalf("--json error not JSON-shaped: %v\nstderr: %q", err, stderr.String()) + } + if env.Error == "" { + t.Fatalf("--json error envelope has an empty message:\n%s", stderr.String()) + } +} + +// TestDocsLintMissingConfigCleanError proves the third instance: a missing +// docs-lint config yields a clean, repo-relative diagnostic — never a raw +// os.Open error leaking the absolute config path. +func TestDocsLintMissingConfigCleanError(t *testing.T) { + repo := t.TempDir() + t.Chdir(repo) + + var stdout, stderr bytes.Buffer + code := Run([]string{"docs", "lint"}, &stdout, &stderr) + if code == 0 { + t.Fatalf("expected a non-zero exit for `docs lint` with no config") + } + msg := stderr.String() + if strings.Contains(msg, repo) { + t.Fatalf("docs lint error leaked the absolute repo path %q:\n%s", repo, msg) + } + if !strings.Contains(msg, filepath.Join(".abcd", "docs-lint.json")) { + t.Fatalf("docs lint error should name the repo-relative config path:\n%s", msg) + } + + // And under --json it is JSON-shaped, not raw text. + stdout.Reset() + stderr.Reset() + if code := Run([]string{"docs", "lint", "--json"}, &stdout, &stderr); code == 0 { + t.Fatalf("expected a non-zero exit for `docs lint --json` with no config") + } + var env struct { + Error string `json:"error"` + } + if err := json.Unmarshal(stderr.Bytes(), &env); err != nil { + t.Fatalf("--json docs lint error not JSON-shaped: %v\nstderr: %q", err, stderr.String()) + } +} + +// TestDocsLintUnreadableConfigNoPathLeak covers a non-not-exist load failure +// (the config path is a directory → EISDIR): a *PathError's Error() embeds the +// absolute path, so the branch must strip it. Guards the security-review BLOCK. +func TestDocsLintUnreadableConfigNoPathLeak(t *testing.T) { + repo := t.TempDir() + t.Chdir(repo) + // Make .abcd/docs-lint.json a directory so os.ReadFile fails with EISDIR. + if err := os.MkdirAll(filepath.Join(repo, ".abcd", "docs-lint.json"), 0o755); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + if code := Run([]string{"docs", "lint"}, &stdout, &stderr); code == 0 { + t.Fatalf("expected a non-zero exit for an unreadable docs-lint config") + } + if msg := stderr.String(); strings.Contains(msg, repo) { + t.Fatalf("docs lint error leaked the absolute repo path %q:\n%s", repo, msg) + } + + // Same guarantee under --json. + stdout.Reset() + stderr.Reset() + if code := Run([]string{"docs", "lint", "--json"}, &stdout, &stderr); code == 0 { + t.Fatalf("expected a non-zero exit under --json for an unreadable config") + } + if msg := stderr.String(); strings.Contains(msg, repo) { + t.Fatalf("--json docs lint error leaked the absolute repo path %q:\n%s", repo, msg) + } +} diff --git a/internal/surface/cli/cli.go b/internal/surface/cli/cli.go index 0dbecf69..84803b22 100644 --- a/internal/surface/cli/cli.go +++ b/internal/surface/cli/cli.go @@ -173,7 +173,27 @@ func newDocsCommand(asJSON *bool) *cobra.Command { } cfg, err := lint.LoadConfig(cfgPath) if err != nil { - return err + // Surface config-load failures as clean, repo-relative + // diagnostics — never a raw os.Open/os.ReadFile error, whose + // *PathError embeds the absolute path (iss-29: no absolute path + // in machine output). Reference what the user typed when they + // passed --config, else the relative default. + ref := filepath.Join(".abcd", "docs-lint.json") + if configPath != "" { + ref = configPath + } + if os.IsNotExist(err) { + return &exitError{Code: 2, Msg: fmt.Sprintf( + "docs lint: config not found at %s — run in a prepared repo or pass --config", ref)} + } + // Strip the path-bearing wrapper: a *PathError's inner Err is the + // bare cause ("is a directory", "permission denied"), no path. + detail := err.Error() + var pe *os.PathError + if errors.As(err, &pe) { + detail = pe.Err.Error() + } + return &exitError{Code: 2, Msg: fmt.Sprintf("docs lint: cannot read config %s: %s", ref, detail)} } findings, err := lint.Lint(cfg, root) if err != nil { @@ -904,6 +924,17 @@ func newCaptureCommand(asJSON *bool) *cobra.Command { } }) } + // Guard: a mistyped subcommand (e.g. `capture resovle iss-1 …`) + // must not be swallowed as free text and filed. When args[0] is a + // near-miss to a real subverb and the shape looks like a subcommand + // call — a lone token, or a token followed by an issue id — refuse + // with a did-you-mean and write nothing (unrecognized-input-never- + // writes, iss-29). Genuine prose still files. + if sug, ok := suspectedTypoedSubcommand(cmd, args); ok { + return &exitError{Code: 2, Msg: fmt.Sprintf( + "unknown capture subcommand %q; did you mean %q? (nothing captured — reword the text if you meant to file it)", + args[0], sug)} + } // Fast path: append a structured issue from the free-form text. text := strings.Join(args, " ") sl := slug @@ -1064,6 +1095,57 @@ var slugNonAlnumRe = regexp.MustCompile(`[^a-z0-9]+`) // ^iss-[0-9]+$ schema constraint). var issIDRe = regexp.MustCompile(`^iss-[0-9]+$`) +// suspectedTypoedSubcommand reports the nearest real subverb when args[0] is a +// near-miss for one (edit distance 1–2) and the invocation shape resembles a +// subcommand call rather than free-text prose: a lone token, or a token +// followed by an issue id. It is deliberately high-precision so it never +// refuses a legitimate free-text capture whose first word merely resembles a +// verb — those carry no trailing iss-id and are multi-word. +func suspectedTypoedSubcommand(parent *cobra.Command, args []string) (string, bool) { + if len(args) == 0 { + return "", false + } + shapedLikeSubcommand := len(args) == 1 || issIDRe.MatchString(args[1]) + if !shapedLikeSubcommand { + return "", false + } + best, bestDist := "", 3 // accept edit distances 1 and 2 + for _, c := range parent.Commands() { + name := c.Name() + if c.Hidden || name == "help" || name == "completion" { + continue + } + if d := levenshtein(args[0], name); d > 0 && d < bestDist { + best, bestDist = name, d + } + } + return best, best != "" +} + +// levenshtein is the classic edit distance (insert/delete/substitute each cost +// 1). Inputs are subcommand-name sized, so the simple O(n·m) two-row form is +// more than fast enough. +func levenshtein(a, b string) int { + ra, rb := []rune(a), []rune(b) + prev := make([]int, len(rb)+1) + for j := range prev { + prev[j] = j + } + for i := 1; i <= len(ra); i++ { + cur := make([]int, len(rb)+1) + cur[0] = i + for j := 1; j <= len(rb); j++ { + cost := 1 + if ra[i-1] == rb[j-1] { + cost = 0 + } + cur[j] = min(prev[j]+1, cur[j-1]+1, prev[j-1]+cost) + } + prev = cur + } + return prev[len(rb)] +} + // parseBlockedBy splits the comma-separated --blocked-by value into iss-ids, // dropping blanks and rejecting any token that is not ^iss-[0-9]+$. An empty // input yields a nil slice (the field is omitted). @@ -1465,15 +1547,29 @@ func Run(args []string, stdout, stderr io.Writer) int { // A command may request a specific exit code (usage errors, the memory-lint // curator contract). An empty message means it already rendered its output // and only the exit code should propagate. + code := 1 var coded interface{ ExitCode() int } if errors.As(err, &coded) { - if msg := err.Error(); msg != "" { + code = coded.ExitCode() + } + if msg := err.Error(); msg != "" { + // Honour --json for the error surface too: a caller that asked for + // machine output must get a JSON envelope, never raw Go text (iss-29). + if asJSON, _ := root.PersistentFlags().GetBool("json"); asJSON { + enc := json.NewEncoder(stderr) + enc.SetIndent("", " ") + _ = enc.Encode(errorEnvelope{Error: msg}) + } else { fmt.Fprintln(stderr, "abcd:", msg) } - return coded.ExitCode() } - fmt.Fprintln(stderr, "abcd:", err) - return 1 + return code +} + +// errorEnvelope is the --json error shape: a single {"error": "..."} object so +// a machine caller can parse a failure the same way it parses a success. +type errorEnvelope struct { + Error string `json:"error"` } // render writes v as indented JSON when asJSON is set, otherwise delegates to From effa17c51eaee552dd258441cbd945aba6dcb799 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:57:54 +0100 Subject: [PATCH 3/3] chore: capture iss-76 (json error surface leaks absolute paths) Filed during the iss-29 security review: cli.Run routes all command errors through the --json envelope, so any verb returning a bare *fs.PathError emits the absolute local path into machine JSON. iss-29 sanitised the docs-lint branch; the systemic boundary fix is iss-76. Assisted-by: Claude:claude-opus-4-8 --- .../issues/open/iss-76-json-error-abspath-leak.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 .abcd/work/issues/open/iss-76-json-error-abspath-leak.md diff --git a/.abcd/work/issues/open/iss-76-json-error-abspath-leak.md b/.abcd/work/issues/open/iss-76-json-error-abspath-leak.md new file mode 100644 index 00000000..5022d24a --- /dev/null +++ b/.abcd/work/issues/open/iss-76-json-error-abspath-leak.md @@ -0,0 +1,11 @@ +--- +schema_version: 1 +id: "iss-76" +slug: "json-error-abspath-leak" +severity: "minor" +category: "bug" +source: "agent-finding" +found_during: "2026-07-12 /abcd:run iss-29 security review" +--- + +cli.Run now routes all command errors through the --json envelope, so any verb that returns a bare *fs.PathError (os.ReadFile/os.Open failures not wrapped by core) emits the absolute local path into machine JSON output, violating no-absolute-paths-in-machine-output. iss-29 sanitised the docs-lint config-load branch specifically; the systemic fix is to sanitise PathError-bearing errors at the Run() boundary (or audit which errors reach the envelope). Detector: a table test that runs each verb's known filesystem-error path under --json and asserts the envelope carries no absolute path. Pre-existing as stderr text; newly widened to --json by cli.Run. \ No newline at end of file