From a6b6d99b4bbabbe0361a91b69624eddac11beba3 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:49:20 +0100 Subject: [PATCH 01/17] docs: plan itd-103 and write spc-16 shell-hazard registry spec The 2026-07-27 grill settled the design; the spec records the settlements plus the mechanism and decides the one delegated spec-time question: guard overrides live in a dedicated committed .abcd/guard.json, independent of the rules loader kill switch. Assisted-by: Claude:claude-fable-5 --- ...ents-the-shell-commands-they-must-never.md | 4 +- ...ents-the-shell-commands-they-must-never.md | 130 ++++++++++++++++++ .abcd/work/DECISIONS.md | 1 + 3 files changed, 133 insertions(+), 2 deletions(-) rename .abcd/development/intents/{drafts => planned}/itd-103-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md (99%) create mode 100644 .abcd/development/specs/open/spc-16-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md diff --git a/.abcd/development/intents/drafts/itd-103-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md b/.abcd/development/intents/planned/itd-103-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md similarity index 99% rename from .abcd/development/intents/drafts/itd-103-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md rename to .abcd/development/intents/planned/itd-103-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md index ea3e42a..6732e30 100644 --- a/.abcd/development/intents/drafts/itd-103-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md +++ b/.abcd/development/intents/planned/itd-103-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md @@ -1,8 +1,8 @@ --- id: itd-103 slug: abcd-teaches-repo-agents-the-shell-commands-they-must-never -spec_id: null -kind: null +spec_id: spc-16 +kind: standalone suggested_kind: null reclassification_history: [] builds_on: [] diff --git a/.abcd/development/specs/open/spc-16-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md b/.abcd/development/specs/open/spc-16-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md new file mode 100644 index 0000000..7676c68 --- /dev/null +++ b/.abcd/development/specs/open/spc-16-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md @@ -0,0 +1,130 @@ +--- +id: spc-16 +slug: abcd-teaches-repo-agents-the-shell-commands-they-must-never +intent: itd-103 +--- +# abcd-teaches-repo-agents-the-shell-commands-they-must-never + +## Summary + +spc-16 delivers the shell-hazard registry for itd-103: one bundled registry of +dangerous command patterns, surfaced through two planes. The teaching plane +injects the matched safety rules through the existing rules loader before +shell-heavy work; the guard plane is a deterministic core verb +(`abcd guard check`) that a harness hook calls at execution time to refuse a +matching command and reply with its safe successor. The design decisions below +were settled by the 2026-07-27 grill (recorded in the intent's Grill +Settlements); this spec records them together with the mechanism — it does not +reopen them. + +## Settled constraints (from the grill) + +- **Fail-open-loud.** If the guard hook cannot execute the abcd binary, the + session is never blocked and never silently unguarded: the hook emits an + unmissable in-session warning, and `abcd ahoy` status reports guard health. +- **Two tiers, mirroring the docs-lint family.** `blocker` refuses the command + with the safe successor as the block message; `warn` lets the command pass + with the warning injected. Entry shape follows the lint `banned_tokens` + family: id, pattern, tier, successor, plain-language why. +- **No in-session override.** The only escape is a committed, reviewable + per-repo config override. +- **Shell-token-aware matching in command position only.** A raw-regex guard + would have blocked this repository's own incident-capture command (a ledger + capture whose quoted text mentions `rm -rf *`) — the known-good corpus is + not optional. + +## Mechanism + +### Registry + +Bundled defaults are embedded in the binary, like the rules-loader default +domains. Each entry declares: + +- `id` — stable slug (e.g. `rm-rf-after-cd-chain`). +- `pattern` — a command-position match expressed over shell tokens, not raw + text (see Matching). +- `tier` — `blocker` or `warn`. +- `successor` — the safe form the agent is told to run instead. +- `why` — one plain-language sentence a non-expert facilitator understands. +- `fixtures` — known-bad and known-good command lines proving the entry's + behaviour (see Admission gate). + +### Config home (spec-time decision) + +Per-repo overrides live in a dedicated committed file, `.abcd/guard.json` +(`schema_version`, `disabled`, `entries` keyed by id for per-field override or +addition) — not in a new `.abcd/rules.json` domain. Two reasons: the guard +entry schema is structured (tier/successor/fixtures) where rules-domain +entries are prose strings, and the rules loader's kill switch must not +silently disable a safety guard — the two features keep independent switches. +Disabling the guard is itself a committed, reviewable act: `"disabled": true` +in `.abcd/guard.json`. + +### Two planes, one registry + +- **Teaching plane (all hosts).** The rules loader gains a guard-backed safety + domain: prompts that recall-match shell-heavy work get the matched registry + entries rendered as rules (pattern, why, safe successor) injected before the + agent acts. Hosts without hook support get this plane at minimum. +- **Guard plane (hosts with hooks).** `abcd guard check` is a core verb behind + the transport-agnostic boundary: core takes a candidate command string and + returns a decision (allow / warn+message / block+successor+why); the CLI + front door formats it, and the host-hook adapter parses the host's hook + payload into that call and maps the decision onto the host's block/allow + protocol. Hook wiring is installed by ahoy for hosts that support it. +- Both planes are wired at delivery: the verb is reachable from the CLI and + the plugin markdown surface (`commands/abcd/guard.md`), and the hook + executes it in a live session — no dead scaffolding. + +### Matching + +Matching is shell-token-aware and applies in command position only: + +- The candidate line is tokenised with shell quoting honoured; a hazard + pattern inside a quoted string argument never fires (AC 3 — the + incident-capture command is known-good fixture #1). +- Command position is tracked across compound separators (`&&`, `;`, `||`, + pipes), so `cd scratch && rm -rf *` matches the cd-chain structure the + registry entry describes. +- String payloads inside `eval` or `sh -c '...'` are a **documented v1 gap**, + stated in the verb's reference doc and the registry README — not a silent + one. + +### Fail-open-loud and health + +The installed hook is a thin shim: if the abcd binary is missing, fails to +execute, or exits abnormally, the shim exits with the host's allow status +while emitting an unmissable warning into the session transcript. `abcd ahoy` +status gains a guard-health line (hook installed, binary reachable, registry +loadable) so a broken guard is visible outside the session too — never a +silent no-op, never a bricked session. + +### Admission gate (bundled defaults) + +A registry entry is admitted to the bundled defaults only when: + +- it ships known-bad and known-good fixtures, with known-good at least 40% of + the entry's corpus; +- it clears the declared true-negative-rate floor: **100% on its known-good + corpus** — a single known-good fixture that fires blocks admission. The + floor is enforced by a test over the embedded corpus, so an entry cannot + ship without its proof. + +The registry grows from reality: `abcd capture` is the single move a +facilitator needs, and recurring captures are promoted into the bundled +defaults through this gate. + +## Acceptance-criteria mapping + +- AC 1 (fail open loud, health in ahoy) → Fail-open-loud and health. +- AC 2 (blocker refuses with successor, warn passes, committed override only) + → Registry tiers + Config home. +- AC 3 (quoted-string non-firing, command position, cd-chain) → Matching. +- AC 4 (fixture corpus, ≥40% known-good, TNR floor, documented eval gap) → + Admission gate + Matching. + +## Out of scope + +- Non-shell tool calls (a later guard-family question, per the intent). +- Parsing string payloads inside `eval` / `shell -c` (documented v1 gap). +- Any scheduled or CI-side execution of the guard. diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 7cc9a06..1fa81af 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -862,3 +862,4 @@ parallel-agent merge contention bites. - 2026-07-27 — ADOPTED the canonical tagline: "A host-agnostic configuration layer for intent-driven development." Canonical identity home is the brief product chapter (01-product README, Identity section: title / tagline / pitch); README strapline, plugin manifest description, and AGENTS.md opening render from it. README's former strapline ("An opinionated, intent-driven development framework for product thinkers") is retired as a surface line; the product-thinker framing lives on in the README body. Rejected wordings: "for agent harnesses" (object shift, host/harness redundancy, plural over-promise), "over any agent harness" (drops the domain). Resolves iss-143; itd-102 generalises the drift check for managed repos. - 2026-07-27 — itd-93 parity tension resolved: Branch A. The scaffold template is the single source for release.yml/auto-release.yml; abcd-cli's live workflows are regenerated from it; the only sanctioned live diff is the additive workflow_dispatch rehearsal (trigger + non-publishing dry-run job). Rejected: substitution-gating the rehearsal off for abcd (weakens the parity guarantee on the one novel component). - 2026-07-27 — GRILLED itd-101/102/103/104 to settlement (six decisions, each now in its intent's Grill Settlements + acceptance criteria): itd-103 guard fails open loudly, blocker/warn tiers with committed-only overrides, shell-token-aware command-position matching with an itd-81-style TNR-floored corpus; itd-101 refresh is manual-with-nagging (scheduled CI later, own sign-off), staleness warns at 180d and blocks releases at 365d, human verifications age on the same clock; itd-102 identity lives in a parseable markdown block (config points at it), check is warn-tier and never rewrites; itd-104 ideate is an optional verb, never a pre-capture gate, with a fresh-context off-policy adversary. All four drafts are grill-settled and await intent plan. +- 2026-07-27: itd-103 guard overrides live in dedicated committed .abcd/guard.json, not a rules.json domain — structured entry schema, and the rules kill switch must not silently disable a safety guard (spc-16). From 56de5156026d0fc1bb9be48f5a5fb673a70f99d6 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:49:21 +0100 Subject: [PATCH 02/17] chore: commit iss-146 capture (guard-bash gh-write false positive) Captured last session but left untracked; rides the run's first record branch per the handover note. Assisted-by: Claude:claude-fable-5 --- .../iss-146-guard-bash-gh-write-local-path-fp.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 .abcd/work/issues/open/iss-146-guard-bash-gh-write-local-path-fp.md diff --git a/.abcd/work/issues/open/iss-146-guard-bash-gh-write-local-path-fp.md b/.abcd/work/issues/open/iss-146-guard-bash-gh-write-local-path-fp.md new file mode 100644 index 0000000..d79478c --- /dev/null +++ b/.abcd/work/issues/open/iss-146-guard-bash-gh-write-local-path-fp.md @@ -0,0 +1,12 @@ +--- +schema_version: 1 +id: "iss-146" +slug: "guard-bash-gh-write-local-path-fp" +severity: "minor" +category: "observation" +source: "user-observation" +found_during: "2026-07-27 session" +found_at: "guard hook, agent shell usage" +--- + +guard-bash false positive class: a compound command chaining a gh write verb with a LOCAL command taking an absolute path argument (gh pr merge ... ; git worktree remove /abs/path) is blocked by the line-level privacy regex, which cannot see that the path never reaches GitHub. Second guard incident of 2026-07-27 (the first, blocking --no-verify, was a true positive). Both are fixture material for itd-103's calibration corpus: this one is a known-good case the TNR floor must protect; the workaround (split the compound) is recorded here so the friction is not silent. \ No newline at end of file From 5e2c813e389ff0d2bb092ecb1a4f4d497342516d Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:14:05 +0100 Subject: [PATCH 03/17] feat: add shell tokenizer for the guard hazard registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The registry matches in command position only, so it needs the candidate command split into simple commands with shell quoting honoured: a hazard named inside a quoted argument (an incident capture whose text mentions "cd scratch && rm -rf *") must never reach command position, and the cd-chain structure must stay visible across compound separators. Quoting affects tokenisation, not argument semantics, so a quoted flag still yields the same token — no bypass. String payloads inside eval or sh -c are a documented v1 gap, marked where the tokenizer stops. Assisted-by: Claude:claude-opus-5[1m] --- internal/core/guard/doc.go | 24 +++++ internal/core/guard/errors.go | 28 ++++++ internal/core/guard/tokenize.go | 135 ++++++++++++++++++++++++++ internal/core/guard/tokenize_test.go | 136 +++++++++++++++++++++++++++ 4 files changed, 323 insertions(+) create mode 100644 internal/core/guard/doc.go create mode 100644 internal/core/guard/errors.go create mode 100644 internal/core/guard/tokenize.go create mode 100644 internal/core/guard/tokenize_test.go diff --git a/internal/core/guard/doc.go b/internal/core/guard/doc.go new file mode 100644 index 0000000..55b3525 --- /dev/null +++ b/internal/core/guard/doc.go @@ -0,0 +1,24 @@ +// Package guard is abcd's transport-agnostic shell-hazard registry (itd-103, +// spc-16). It owns one registry of dangerous command patterns and the +// deterministic decision that a harness hook calls at execution time: given a +// candidate command line it returns allow, warn, or block — never writing to +// stdout, never exiting, never knowing a transport. Front doors under +// internal/surface format the decision, and the fail-open-loud behaviour on a +// broken guard lives in the hook shim, not here. +// +// The model is a set of binary-bundled default entries (embedded from +// defaults/guard.json) merged with an optional per-repo +// /.abcd/guard.json override. Each entry declares an id, a +// command-position pattern over shell tokens, a tier (blocker/warn), the safe +// successor, a plain-language why, and the known-bad/known-good fixtures that +// prove it. Bundled entries are held to an admission gate (fixtures present, +// known-good at least 40% of the corpus, 100% true-negative rate) enforced by +// TestBundledEntriesPassAdmissionGate, so an entry cannot ship without proof. +// +// Matching is shell-token-aware and applies in COMMAND POSITION only: the +// candidate is tokenised with shell quoting honoured, so a hazard named inside a +// quoted string argument (an incident capture whose text mentions +// "cd scratch && rm -rf *") never fires. Quoting affects tokenisation, not +// argument semantics — `git push '--force'` still fires, because the shell hands +// the same argv either way. +package guard diff --git a/internal/core/guard/errors.go b/internal/core/guard/errors.go new file mode 100644 index 0000000..979041e --- /dev/null +++ b/internal/core/guard/errors.go @@ -0,0 +1,28 @@ +package guard + +import "errors" + +// The guard sentinels. Every error the package returns wraps one of these, so a +// front door can render the failure loudly (and, for the hook shim, fail OPEN) +// without string-matching. Core never decides what a failure means for a +// session — it only names it. +var ( + // ErrUnparsableCommand is a candidate command line the shell tokenizer + // cannot split (an unterminated quote, a trailing backslash). + ErrUnparsableCommand = errors.New("guard: unparsable command line") + + // ErrMalformedConfig is a per-repo .abcd/guard.json that is unreadable or + // is not valid JSON. + ErrMalformedConfig = errors.New("guard: malformed config") + + // ErrSchemaVersion is a config declaring a schema_version this build does + // not implement. + ErrSchemaVersion = errors.New("guard: unsupported schema_version") + + // ErrUnknownTier is an entry whose tier is neither blocker nor warn. + ErrUnknownTier = errors.New("guard: unknown tier") + + // ErrInvalidEntry is an entry that fails the registry schema (bad id, no + // pattern command, missing successor or why). + ErrInvalidEntry = errors.New("guard: invalid entry") +) diff --git a/internal/core/guard/tokenize.go b/internal/core/guard/tokenize.go new file mode 100644 index 0000000..2810cd7 --- /dev/null +++ b/internal/core/guard/tokenize.go @@ -0,0 +1,135 @@ +package guard + +import "fmt" + +// segment is one command in command position: the tokens of a single simple +// command, plus the index of the logical line (chain) it belongs to. Segments in +// the same chain are separated by `&&`, `;`, `||`, `|`, `&`, or a subshell +// parenthesis; a newline starts a new chain. The chain is what lets an entry +// require the cd-chain structure (`cd scratch && rm -rf *`) without matching an +// unrelated `rm` on the next line. +type segment struct { + tokens []string + chain int +} + +// tokenize splits a candidate command line into command-position segments, +// honouring shell quoting: single quotes are literal, double quotes take the +// POSIX backslash escapes, and a backslash outside quotes escapes the next +// character. Operators and comments are recognised OUTSIDE quotes only, which is +// exactly what keeps a hazard named inside a quoted argument from ever reaching +// command position. +// +// v1 GAP (documented, not silent): the tokenizer stops at the token boundary. A +// command string carried as a DATA argument — `sh -c ''`, `eval +// ''`, `bash -lc ""` — stays one opaque token and its payload +// is never parsed, so a hazard hidden there is not matched. Descending into +// those payloads is out of scope for v1 (spc-16 "Out of scope"); it is stated in +// the verb's reference doc and the registry README rather than silently assumed. +func tokenize(line string) ([]segment, error) { + var ( + segs []segment + toks []string + cur []byte + hasCur bool + chain int + ) + flushToken := func() { + if hasCur { + toks = append(toks, string(cur)) + cur = nil + hasCur = false + } + } + flushSegment := func() { + flushToken() + if len(toks) > 0 { + segs = append(segs, segment{tokens: toks, chain: chain}) + toks = nil + } + } + + for i := 0; i < len(line); { + c := line[i] + switch { + case c == '\\': + if i+1 >= len(line) { + return nil, fmt.Errorf("%w: trailing backslash", ErrUnparsableCommand) + } + if line[i+1] == '\n' { + // Line continuation: the newline is removed, the chain continues. + i += 2 + continue + } + cur = append(cur, line[i+1]) + hasCur = true + i += 2 + case c == '\'': + j := i + 1 + for j < len(line) && line[j] != '\'' { + j++ + } + if j >= len(line) { + return nil, fmt.Errorf("%w: unterminated single quote", ErrUnparsableCommand) + } + cur = append(cur, line[i+1:j]...) + hasCur = true + i = j + 1 + case c == '"': + j := i + 1 + closed := false + for j < len(line) { + if line[j] == '\\' && j+1 < len(line) { + switch line[j+1] { + case '"', '\\', '$', '`': + cur = append(cur, line[j+1]) + case '\n': + // Line continuation inside double quotes: both dropped. + default: + // Backslash is literal before any other character. + cur = append(cur, '\\', line[j+1]) + } + j += 2 + continue + } + if line[j] == '"' { + closed = true + break + } + cur = append(cur, line[j]) + j++ + } + if !closed { + return nil, fmt.Errorf("%w: unterminated double quote", ErrUnparsableCommand) + } + hasCur = true + i = j + 1 + case c == ' ' || c == '\t' || c == '\r': + flushToken() + i++ + case c == '\n': + flushSegment() + chain++ + i++ + case c == '#' && !hasCur: + // A comment starts only at a word boundary (POSIX): `url/#frag` is + // part of the token, a bare `#` runs to the end of the line. + for i < len(line) && line[i] != '\n' { + i++ + } + case c == '&' || c == '|' || c == ';' || c == '(' || c == ')': + flushSegment() + if (c == '&' || c == '|') && i+1 < len(line) && line[i+1] == c { + i += 2 + continue + } + i++ + default: + cur = append(cur, c) + hasCur = true + i++ + } + } + flushSegment() + return segs, nil +} diff --git a/internal/core/guard/tokenize_test.go b/internal/core/guard/tokenize_test.go new file mode 100644 index 0000000..4714960 --- /dev/null +++ b/internal/core/guard/tokenize_test.go @@ -0,0 +1,136 @@ +package guard + +import ( + "errors" + "strings" + "testing" +) + +// render flattens segments to a comparable form: chain index, then the tokens +// joined with "|" so an empty or embedded-space token is still visible. +func render(segs []segment) []string { + out := make([]string, len(segs)) + for i, s := range segs { + out[i] = string(rune('0'+s.chain)) + ":" + strings.Join(s.tokens, "|") + } + return out +} + +func TestTokenizeSegments(t *testing.T) { + tests := []struct { + name string + line string + want []string + }{ + { + name: "single command", + line: "rm -rf *", + want: []string{"0:rm|-rf|*"}, + }, + { + name: "cd chain across &&", + line: "cd scratch && rm -rf *", + want: []string{"0:cd|scratch", "0:rm|-rf|*"}, + }, + { + name: "quoted hazard is one argument token", + line: `abcd capture "agent ran cd scratch && rm -rf * — one failed cd from disaster"`, + want: []string{"0:abcd|capture|agent ran cd scratch && rm -rf * — one failed cd from disaster"}, + }, + { + name: "single quotes suppress operators", + line: `echo 'a && b'`, + want: []string{"0:echo|a && b"}, + }, + { + name: "backslash escapes a space", + line: `echo a\ b`, + want: []string{`0:echo|a b`}, + }, + { + name: "backslash escapes a quote inside double quotes", + line: `echo "a\"b"`, + want: []string{`0:echo|a"b`}, + }, + { + name: "backslash escapes a separator", + line: `echo a\&\&b`, + want: []string{`0:echo|a&&b`}, + }, + { + name: "all compound separators split command position", + line: "a; b | c || d && e", + want: []string{"0:a", "0:b", "0:c", "0:d", "0:e"}, + }, + { + name: "background operator splits", + line: "sleep 1 & rm -rf *", + want: []string{"0:sleep|1", "0:rm|-rf|*"}, + }, + { + name: "subshell parentheses split", + line: "(cd x && rm -rf *)", + want: []string{"0:cd|x", "0:rm|-rf|*"}, + }, + { + name: "newline starts a new chain", + line: "cd x\nrm -rf *", + want: []string{"0:cd|x", "1:rm|-rf|*"}, + }, + { + name: "comment at word start is stripped", + line: "echo hi # rm -rf *", + want: []string{"0:echo|hi"}, + }, + { + name: "hash inside a word is not a comment", + line: "curl https://example.test/#frag", + want: []string{"0:curl|https://example.test/#frag"}, + }, + { + name: "quoted hash is not a comment", + line: `git commit -m "fix #123"`, + want: []string{"0:git|commit|-m|fix #123"}, + }, + { + name: "empty line yields no segments", + line: " \t ", + want: nil, + }, + { + name: "adjacent quoting concatenates into one token", + line: `git push '--force' origin`, + want: []string{"0:git|push|--force|origin"}, + }, + { + name: "empty quoted token is preserved", + line: `grep "" file`, + want: []string{"0:grep||file"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + segs, err := tokenize(tc.line) + if err != nil { + t.Fatalf("tokenize(%q): unexpected error: %v", tc.line, err) + } + got := render(segs) + if len(got) != len(tc.want) { + t.Fatalf("tokenize(%q) = %q, want %q", tc.line, got, tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Fatalf("tokenize(%q) segment %d = %q, want %q", tc.line, i, got[i], tc.want[i]) + } + } + }) + } +} + +func TestTokenizeRejectsUnterminatedQuote(t *testing.T) { + for _, line := range []string{`echo "unterminated`, `echo 'unterminated`, `echo trailing\`} { + if _, err := tokenize(line); !errors.Is(err, ErrUnparsableCommand) { + t.Fatalf("tokenize(%q) error = %v, want ErrUnparsableCommand", line, err) + } + } +} From 4a8ac2eecfb7b6f7d0a8258e0ba96a0c7e416bcb Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:20:07 +0100 Subject: [PATCH 04/17] feat: add the bundled shell-hazard registry and its decision function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The registry is the itd-103 core: entries drawn only from this repo's own documented never-list, each carrying a command-position pattern over shell tokens, a blocker/warn tier mirroring the record-lint severity family, a safe successor, and a plain-language why — because the refusal itself is the lesson. Check returns allow / warn / block as values only; no stdout, no exit status. Fail-open-loud on a broken guard is the hook shim's job, so core only names the failure (ErrUnparsableCommand) and lets the caller decide. Matching is command-position only, so a hazard named inside a quoted argument never fires, while quoting is not a bypass and wrapper or environment prefixes do not hide the command. The admission gate test holds every bundled entry to its proof: each known-bad fixture fires at the declared tier, every known-good fixture fires nothing (100% true-negative floor), and known-good is at least 40% of the corpus. Known-good fixture #1 is this repo's own incident capture, the command a raw-regex guard would have refused. Assisted-by: Claude:claude-opus-5[1m] --- internal/core/guard/defaults/guard.json | 146 ++++++++++++ internal/core/guard/fixtures_test.go | 125 +++++++++++ internal/core/guard/guard.go | 284 ++++++++++++++++++++++++ internal/core/guard/guard_test.go | 190 ++++++++++++++++ internal/core/guard/match.go | 178 +++++++++++++++ 5 files changed, 923 insertions(+) create mode 100644 internal/core/guard/defaults/guard.json create mode 100644 internal/core/guard/fixtures_test.go create mode 100644 internal/core/guard/guard.go create mode 100644 internal/core/guard/guard_test.go create mode 100644 internal/core/guard/match.go diff --git a/internal/core/guard/defaults/guard.json b/internal/core/guard/defaults/guard.json new file mode 100644 index 0000000..514e30c --- /dev/null +++ b/internal/core/guard/defaults/guard.json @@ -0,0 +1,146 @@ +{ + "schema_version": 1, + "disabled": false, + "entries": { + "rm-rf-after-cd-chain": { + "tier": "blocker", + "pattern": { + "command": "rm", + "flags": ["-r|-R|--recursive", "-f|--force"], + "after_cd": true + }, + "why": "If the `cd` fails, the delete still runs — in whatever directory the shell was already in — so a command meant for a scratch folder can wipe the working tree instead.", + "successor": "Run the delete as its own command with an absolute path (`rm -rf -- /absolute/path/to/target`), so a failed `cd` cannot redirect it.", + "fixtures": { + "known_bad": [ + "cd scratch && rm -rf *", + "cd /tmp/build; rm -rf .", + "cd scratch && sudo rm -fr ./*", + "cd worktree && rm -r -f build" + ], + "known_good": [ + "./bin/abcd-darwin-arm64 capture \"agent ran cd scratch && rm -rf * — one failed cd from disaster\"", + "rm -rf ./build", + "cd scratch && ls -la", + "cd scratch && rm ./notes.txt" + ] + } + }, + "git-push-force": { + "tier": "blocker", + "pattern": { + "command": "git", + "subcommand": "push", + "value_flags": ["-C", "-c", "--git-dir", "--work-tree", "--namespace", "--exec-path", "--super-prefix"], + "flags": ["--force|-f|--force-with-lease|--force-if-includes"] + }, + "why": "A force push rewrites the shared branch history, so commits other people already have can disappear with no copy left anywhere.", + "successor": "Push without a force flag; if the remote rejects it, integrate first (`git pull --rebase`), or push a new branch and open a pull request.", + "fixtures": { + "known_bad": [ + "git push --force origin main", + "git push -f", + "git push --force-with-lease origin main", + "git -C /repo push --force origin main", + "git push '--force' origin main" + ], + "known_good": [ + "git push origin main", + "git push --set-upstream origin feat/shell-hazard-registry", + "abcd capture \"an agent tried git push --force on main\"", + "git log --oneline -5" + ] + } + }, + "git-commit-no-verify": { + "tier": "blocker", + "pattern": { + "command": "git", + "subcommand": "commit", + "value_flags": ["-C", "-c", "--git-dir", "--work-tree", "--namespace", "--exec-path", "--super-prefix"], + "flags": ["--no-verify|-n"] + }, + "why": "`--no-verify` skips the checks the repository installed as commit hooks, so a broken or unsafe change is recorded without ever being looked at.", + "successor": "Commit without the flag and fix whatever the hooks report; if a hook is wrong, fix the hook.", + "fixtures": { + "known_bad": [ + "git commit --no-verify -m \"wip\"", + "git commit -n -m \"wip\"", + "git -c core.hooksPath=/dev/null commit --no-verify" + ], + "known_good": [ + "git commit -m \"wip\"", + "git commit --amend --no-edit", + "abcd capture \"agent used git commit --no-verify to skip the hooks\"" + ] + } + }, + "git-push-no-verify": { + "tier": "blocker", + "pattern": { + "command": "git", + "subcommand": "push", + "value_flags": ["-C", "-c", "--git-dir", "--work-tree", "--namespace", "--exec-path", "--super-prefix"], + "flags": ["--no-verify"] + }, + "why": "`--no-verify` skips the pre-push checks, so the branch is published without the tests and scans the repository requires before anything leaves the machine.", + "successor": "Push without the flag and fix what the pre-push hook reports; if a hook is wrong, fix the hook.", + "fixtures": { + "known_bad": [ + "git push --no-verify", + "git push --no-verify origin main" + ], + "known_good": [ + "git push -n origin main", + "git push origin main", + "abcd capture \"the agent ran git push --no-verify\"" + ] + } + }, + "git-reset-hard": { + "tier": "warn", + "pattern": { + "command": "git", + "subcommand": "reset", + "value_flags": ["-C", "-c", "--git-dir", "--work-tree", "--namespace", "--exec-path", "--super-prefix"], + "flags": ["--hard"] + }, + "why": "`--hard` throws away every uncommitted change in the working tree, and nothing in git can bring back work that was never committed.", + "successor": "Save the changes first (`git stash`), or undo one file with `git restore `; use `--hard` only on work you created yourself.", + "fixtures": { + "known_bad": [ + "git reset --hard origin/main", + "git reset --hard", + "cd repo && git reset --hard HEAD~1" + ], + "known_good": [ + "git reset --soft HEAD~1", + "git reset HEAD -- notes.md", + "abcd capture \"agent ran git reset --hard and lost my uncommitted edits\"" + ] + } + }, + "git-clean": { + "tier": "warn", + "pattern": { + "command": "git", + "subcommand": "clean", + "value_flags": ["-C", "-c", "--git-dir", "--work-tree", "--namespace", "--exec-path", "--super-prefix"] + }, + "why": "`git clean` deletes untracked files outright — anything never added to git, including local notes and scratch output, is gone with no copy in the repository.", + "successor": "List what would go first (`git clean --dry-run`) and move anything worth keeping aside; delete only files you created yourself.", + "fixtures": { + "known_bad": [ + "git clean -fd", + "git clean -xfd", + "cd worktree && git clean -f" + ], + "known_good": [ + "git status --porcelain", + "git stash --include-untracked", + "abcd capture \"git clean -fd wiped my scratch notes\"" + ] + } + } + } +} diff --git a/internal/core/guard/fixtures_test.go b/internal/core/guard/fixtures_test.go new file mode 100644 index 0000000..bf224b4 --- /dev/null +++ b/internal/core/guard/fixtures_test.go @@ -0,0 +1,125 @@ +package guard + +import ( + "sort" + "strings" + "testing" +) + +// TestBundledEntriesPassAdmissionGate is the admission gate of spc-16: a +// registry entry is admitted to the bundled defaults only when it ships the +// fixtures that prove it. The gate is a test over the embedded corpus, so an +// entry cannot ship without its proof and a regression in the matcher fails the +// build rather than silently un-guarding a session. +// +// - every known-bad fixture fires ITS entry at the entry's declared tier; +// - every known-good fixture fires NOTHING (a 100% true-negative rate — one +// known-good that fires blocks admission); +// - known-good is at least 40% of the entry's corpus. +func TestBundledEntriesPassAdmissionGate(t *testing.T) { + r := Defaults() + ids := make([]string, 0, len(r.Entries)) + for id := range r.Entries { + ids = append(ids, id) + } + sort.Strings(ids) + if len(ids) == 0 { + t.Fatal("the bundled registry is empty") + } + + for _, id := range ids { + e := r.Entries[id] + t.Run(id, func(t *testing.T) { + bad, good := len(e.Fixtures.KnownBad), len(e.Fixtures.KnownGood) + if bad == 0 { + t.Fatalf("entry %s ships no known-bad fixtures", id) + } + if good == 0 { + t.Fatalf("entry %s ships no known-good fixtures", id) + } + // good/(good+bad) >= 0.4, in integer arithmetic. + if 10*good < 4*(good+bad) { + t.Fatalf("entry %s: known-good is %d of %d fixtures, below the 40%% floor", id, good, good+bad) + } + + want := VerdictBlock + if e.Tier == TierWarn { + want = VerdictWarn + } + for _, cmd := range e.Fixtures.KnownBad { + d, err := r.Check(cmd) + if err != nil { + t.Fatalf("known-bad %q: unexpected error: %v", cmd, err) + } + if !contains(d.Matches, id) { + t.Fatalf("known-bad %q did not fire entry %s (matches: %v)", cmd, id, d.Matches) + } + if d.Verdict != want { + t.Fatalf("known-bad %q verdict = %q, want %q for a %s entry", cmd, d.Verdict, want, e.Tier) + } + } + for _, cmd := range e.Fixtures.KnownGood { + d, err := r.Check(cmd) + if err != nil { + t.Fatalf("known-good %q: unexpected error: %v", cmd, err) + } + if d.Verdict != VerdictAllow { + t.Fatalf("known-good %q fired %v (verdict %q); the true-negative floor is 100%%", cmd, d.Matches, d.Verdict) + } + } + }) + } +} + +// TestBundledEntriesAreExplainable holds the teaching plane's end of the +// bargain: every entry carries a safe successor and a plain-language why, since +// the block message IS the lesson. +func TestBundledEntriesAreExplainable(t *testing.T) { + for id, e := range Defaults().Entries { + if e.Successor == "" { + t.Errorf("entry %s has no successor; a refusal must say what to run instead", id) + } + if e.Why == "" { + t.Errorf("entry %s has no why; a refusal must explain itself in plain language", id) + } + if e.ID != id { + t.Errorf("entry keyed %s carries ID %q; the map key is the id", id, e.ID) + } + } +} + +// TestIncidentCaptureIsKnownGoodFixtureOne pins the corpus case the whole +// design turns on: this repo's own incident-capture command, whose quoted text +// names a hazard, must be the first known-good fixture of the rm entry and must +// not fire. A raw-regex guard would have blocked it. +func TestIncidentCaptureIsKnownGoodFixtureOne(t *testing.T) { + e, ok := Defaults().Entries["rm-rf-after-cd-chain"] + if !ok { + t.Fatal("rm-rf-after-cd-chain is missing from the bundled registry") + } + if len(e.Fixtures.KnownGood) == 0 { + t.Fatal("rm-rf-after-cd-chain ships no known-good fixtures") + } + first := e.Fixtures.KnownGood[0] + if !containsAll(first, "capture", "cd scratch && rm -rf *") { + t.Fatalf("known-good fixture #1 = %q, want the incident-capture shape", first) + } +} + +func contains(xs []string, want string) bool { + for _, x := range xs { + if x == want { + return true + } + } + return false +} + +func containsAll(s string, subs ...string) bool { + for _, sub := range subs { + if !strings.Contains(s, sub) { + return false + } + } + return true +} diff --git a/internal/core/guard/guard.go b/internal/core/guard/guard.go new file mode 100644 index 0000000..9efa14b --- /dev/null +++ b/internal/core/guard/guard.go @@ -0,0 +1,284 @@ +package guard + +import ( + _ "embed" + "encoding/json" + "fmt" + "sort" + "strings" +) + +// SchemaVersion is the guard config schema this build implements. It is +// independent of the rules loader's schema: the two features keep separate +// switches so a rules kill switch can never silently disable a safety guard. +const SchemaVersion = 1 + +// Tier values, mirroring the record-lint severity family: a blocker refuses the +// command, a warn lets it through with the warning attached. +const ( + TierBlocker = "blocker" + TierWarn = "warn" +) + +// Verdict is the decision a front door acts on. +type Verdict string + +const ( + // VerdictAllow means no entry matched: run the command. + VerdictAllow Verdict = "allow" + // VerdictWarn means a warn-tier entry matched: run the command with the + // warning surfaced. + VerdictWarn Verdict = "warn" + // VerdictBlock means a blocker-tier entry matched: refuse, and reply with + // the successor as the block message. + VerdictBlock Verdict = "block" +) + +// Pattern is a command-position match expressed over shell tokens, never raw +// text. All declared constraints must hold for the pattern to fire. +type Pattern struct { + // Command is the executable that must stand in command position. It is + // compared against the segment's argv[0] by basename, so /bin/rm matches rm. + Command string `json:"command"` + // Subcommand, when set, must be the first non-flag argument (git push, + // git reset). ValueFlags lets a global option that consumes the next token + // be skipped while looking for it. + Subcommand string `json:"subcommand,omitempty"` + // ValueFlags are flags of Command that consume the FOLLOWING token, so the + // scan for Subcommand steps over the value rather than reading it as the + // subcommand (`git -C /repo push`). + ValueFlags []string `json:"value_flags,omitempty"` + // Flags are argument constraints. Each element is one alternation group + // written with "|" ("--force|-f"); every group must be satisfied by some + // argument token. A long alternative also matches its --flag=value form; a + // single-letter short alternative also matches inside a bundled cluster + // (-rf satisfies -r and -f). + Flags []string `json:"flags,omitempty"` + // AfterCD, when true, additionally requires that some EARLIER command in the + // same chain is a `cd` — the cd-chain structure (`cd scratch && rm -rf *`) + // whose hazard is that a failed cd silently redirects the command. A nil + // pointer means false; it is a pointer so a per-repo override can turn the + // requirement off as well as on. + AfterCD *bool `json:"after_cd,omitempty"` +} + +// Fixtures is an entry's proof corpus: command lines it must fire on, and +// command lines it must not. The admission gate in fixtures_test.go enforces +// both sides plus the 40% known-good floor. +type Fixtures struct { + KnownBad []string `json:"known_bad,omitempty"` + KnownGood []string `json:"known_good,omitempty"` +} + +// Entry is one hazard. The map key in Registry.Entries is its id; ID is filled +// from that key on load, so the key is always authoritative. +type Entry struct { + ID string `json:"-"` + Pattern Pattern `json:"pattern"` + Tier string `json:"tier"` + Successor string `json:"successor"` + Why string `json:"why"` + Fixtures Fixtures `json:"fixtures,omitempty"` +} + +// Registry is the merged, validated hazard model: the bundled defaults overlaid +// with the per-repo override. +type Registry struct { + SchemaVersion int `json:"schema_version"` + Disabled bool `json:"disabled"` + Entries map[string]Entry `json:"entries"` +} + +// Decision is what core returns for one candidate command. It carries no +// formatting and no exit status — the front door decides how to render it, and +// the hook shim decides what to do with a broken guard. +type Decision struct { + Verdict Verdict `json:"verdict"` + // EntryID, Tier, Successor, and Why describe the winning entry; they are + // empty on an allow. + EntryID string `json:"entry_id,omitempty"` + Tier string `json:"tier,omitempty"` + Successor string `json:"successor,omitempty"` + Why string `json:"why,omitempty"` + // Message is the ready-to-surface sentence: the why plus the successor, so + // the refusal itself teaches the safe form. + Message string `json:"message,omitempty"` + // Matches lists every entry the command tripped (blocker ids first, then + // warn, each in id order), so a warn plane can surface all of them. + Matches []string `json:"matches,omitempty"` +} + +//go:embed defaults/guard.json +var defaultsJSON []byte + +// defaultRegistry is parsed once at init; a malformed embedded asset is a build +// error surfaced as a panic (it can never happen at runtime). +var defaultRegistry = mustParseDefaults() + +func mustParseDefaults() Registry { + r, err := parse(defaultsJSON) + if err != nil { + panic("guard: bundled defaults are malformed: " + err.Error()) + } + if err := Validate(r); err != nil { + panic("guard: bundled defaults fail validation: " + err.Error()) + } + return r +} + +// Defaults returns a deep copy of the binary-bundled registry, safe for the +// caller to mutate. +func Defaults() Registry { return cloneRegistry(defaultRegistry) } + +// parse decodes registry JSON and stamps each entry's ID from its map key. +func parse(data []byte) (Registry, error) { + var r Registry + if err := json.Unmarshal(data, &r); err != nil { + return Registry{}, fmt.Errorf("%w: %v", ErrMalformedConfig, err) + } + for id, e := range r.Entries { + e.ID = id + r.Entries[id] = e + } + return r, nil +} + +// Validate checks the registry schema: the schema version, every entry id, and +// every entry's tier, pattern command, successor, and why. A per-repo override +// is validated AFTER merging, so an override can never produce an entry the +// bundled defaults would not have been allowed to ship. +func Validate(r Registry) error { + if r.SchemaVersion != SchemaVersion { + return fmt.Errorf("%w: schema_version must be %d, got %d", ErrSchemaVersion, SchemaVersion, r.SchemaVersion) + } + ids := make([]string, 0, len(r.Entries)) + for id := range r.Entries { + ids = append(ids, id) + } + sort.Strings(ids) + for _, id := range ids { + if !validEntryID(id) { + return fmt.Errorf("%w: entry id %q must match [a-z0-9][a-z0-9-]*", ErrInvalidEntry, id) + } + e := r.Entries[id] + switch e.Tier { + case TierBlocker, TierWarn: + default: + return fmt.Errorf("%w: entry %s has tier %q, want %q or %q", ErrUnknownTier, id, e.Tier, TierBlocker, TierWarn) + } + if strings.TrimSpace(e.Pattern.Command) == "" { + return fmt.Errorf("%w: entry %s has no pattern command", ErrInvalidEntry, id) + } + // A refusal with no successor leaves its replacement in prose only, and + // one with no why cannot teach — both are load-time rejections, as in + // the record-lint banned_tokens family (iss-51). + if strings.TrimSpace(e.Successor) == "" { + return fmt.Errorf("%w: entry %s has no successor", ErrInvalidEntry, id) + } + if strings.TrimSpace(e.Why) == "" { + return fmt.Errorf("%w: entry %s has no why", ErrInvalidEntry, id) + } + } + return nil +} + +func validEntryID(id string) bool { + if id == "" { + return false + } + for i := 0; i < len(id); i++ { + c := id[i] + switch { + case c >= 'a' && c <= 'z', c >= '0' && c <= '9': + case c == '-' && i > 0: + default: + return false + } + } + return true +} + +// Check evaluates one candidate command line against the registry. A disabled +// registry allows everything (the escape is committed and reviewable, never an +// in-session override). A command line the tokenizer cannot split is an error, +// not an allow — core names the failure and the caller decides what it means. +func (r Registry) Check(command string) (Decision, error) { + if r.Disabled { + return Decision{Verdict: VerdictAllow}, nil + } + segs, err := tokenize(command) + if err != nil { + return Decision{}, err + } + ids := make([]string, 0, len(r.Entries)) + for id := range r.Entries { + ids = append(ids, id) + } + sort.Strings(ids) + + var blockers, warns []string + for _, id := range ids { + if matchesAny(r.Entries[id].Pattern, segs) { + if r.Entries[id].Tier == TierBlocker { + blockers = append(blockers, id) + } else { + warns = append(warns, id) + } + } + } + matches := append(append([]string(nil), blockers...), warns...) + if len(matches) == 0 { + return Decision{Verdict: VerdictAllow}, nil + } + // A blocker outranks a warn; within a tier the id order is the tiebreak, so + // the decision is deterministic for the same registry and command. + win := r.Entries[matches[0]] + verdict := VerdictWarn + if win.Tier == TierBlocker { + verdict = VerdictBlock + } + return Decision{ + Verdict: verdict, + EntryID: win.ID, + Tier: win.Tier, + Successor: win.Successor, + Why: win.Why, + Message: message(verdict, win), + Matches: matches, + }, nil +} + +// message renders the sentence a front door surfaces: what happened, why in +// plain language, and what to run instead. The refusal is the lesson, so the +// successor is always part of it. +func message(v Verdict, e Entry) string { + lead := "Blocked" + if v == VerdictWarn { + lead = "Warning" + } + return fmt.Sprintf("%s by the abcd guard (%s): %s Run instead: %s", lead, e.ID, e.Why, e.Successor) +} + +func cloneRegistry(r Registry) Registry { + out := Registry{SchemaVersion: r.SchemaVersion, Disabled: r.Disabled} + if r.Entries != nil { + out.Entries = make(map[string]Entry, len(r.Entries)) + for id, e := range r.Entries { + out.Entries[id] = cloneEntry(e) + } + } + return out +} + +func cloneEntry(e Entry) Entry { + out := e + out.Pattern.ValueFlags = append([]string(nil), e.Pattern.ValueFlags...) + out.Pattern.Flags = append([]string(nil), e.Pattern.Flags...) + if e.Pattern.AfterCD != nil { + v := *e.Pattern.AfterCD + out.Pattern.AfterCD = &v + } + out.Fixtures.KnownBad = append([]string(nil), e.Fixtures.KnownBad...) + out.Fixtures.KnownGood = append([]string(nil), e.Fixtures.KnownGood...) + return out +} diff --git a/internal/core/guard/guard_test.go b/internal/core/guard/guard_test.go new file mode 100644 index 0000000..864ad49 --- /dev/null +++ b/internal/core/guard/guard_test.go @@ -0,0 +1,190 @@ +package guard + +import ( + "errors" + "strings" + "testing" +) + +// checkOK runs Check against the bundled defaults, failing the test on a +// tokenizer error (the cases that expect one assert it directly). +func checkOK(t *testing.T, command string) Decision { + t.Helper() + d, err := Defaults().Check(command) + if err != nil { + t.Fatalf("Check(%q): unexpected error: %v", command, err) + } + return d +} + +func TestCheckDecisions(t *testing.T) { + tests := []struct { + name string + command string + verdict Verdict + entryID string // expected winning entry ("" when allowed) + }{ + { + name: "cd chain then rm -rf is blocked", + command: "cd scratch && rm -rf *", + verdict: VerdictBlock, + entryID: "rm-rf-after-cd-chain", + }, + { + name: "hazard quoted inside an incident capture never fires", + command: `./bin/abcd-darwin-arm64 capture "agent ran cd scratch && rm -rf * — one failed cd from disaster"`, + verdict: VerdictAllow, + }, + { + name: "rm -rf without a cd chain is allowed", + command: "rm -rf ./build", + verdict: VerdictAllow, + }, + { + name: "a quoted flag still fires (quoting is not a bypass)", + command: `git push '--force' origin main`, + verdict: VerdictBlock, + entryID: "git-push-force", + }, + { + name: "git global value flags do not hide the subcommand", + command: "git -C /repo push --force origin main", + verdict: VerdictBlock, + entryID: "git-push-force", + }, + { + name: "bundled short flags match (-fr satisfies -r and -f)", + command: "cd scratch && rm -fr ./*", + verdict: VerdictBlock, + entryID: "rm-rf-after-cd-chain", + }, + { + name: "a sudo wrapper does not hide the command", + command: "cd scratch && sudo rm -rf *", + verdict: VerdictBlock, + entryID: "rm-rf-after-cd-chain", + }, + { + name: "an environment assignment prefix does not hide the command", + command: "cd scratch && FOO=bar rm -rf *", + verdict: VerdictBlock, + entryID: "rm-rf-after-cd-chain", + }, + { + name: "push -n is dry-run, not --no-verify", + command: "git push -n origin main", + verdict: VerdictAllow, + }, + { + name: "commit -n is --no-verify and is blocked", + command: `git commit -n -m "wip"`, + verdict: VerdictBlock, + entryID: "git-commit-no-verify", + }, + { + name: "reset --hard warns rather than blocks", + command: "git reset --hard origin/main", + verdict: VerdictWarn, + entryID: "git-reset-hard", + }, + { + name: "an unrelated command is allowed", + command: "git status --porcelain", + verdict: VerdictAllow, + }, + { + name: "a cd on the previous line does not chain into the next line", + command: "cd scratch\nrm -rf ./build", + verdict: VerdictAllow, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + d := checkOK(t, tc.command) + if d.Verdict != tc.verdict { + t.Fatalf("Check(%q).Verdict = %q, want %q (matches: %v)", tc.command, d.Verdict, tc.verdict, d.Matches) + } + if d.EntryID != tc.entryID { + t.Fatalf("Check(%q).EntryID = %q, want %q", tc.command, d.EntryID, tc.entryID) + } + if tc.verdict == VerdictAllow { + return + } + if d.Successor == "" || d.Why == "" { + t.Fatalf("Check(%q): a firing decision must carry a successor and a why, got %+v", tc.command, d) + } + if !strings.Contains(d.Message, d.Successor) { + t.Fatalf("Check(%q).Message = %q, must cite the successor %q", tc.command, d.Message, d.Successor) + } + }) + } +} + +// TestBlockerWinsOverWarn pins the severity ordering: a line that trips both +// tiers is refused, and both matches are reported. +func TestBlockerWinsOverWarn(t *testing.T) { + d := checkOK(t, "cd repo && git reset --hard && rm -rf *") + if d.Verdict != VerdictBlock || d.EntryID != "rm-rf-after-cd-chain" { + t.Fatalf("blocker must win over warn, got %+v", d) + } + if len(d.Matches) != 2 { + t.Fatalf("Matches = %v, want both the blocker and the warn entry", d.Matches) + } +} + +// TestEvalPayloadIsDocumentedV1Gap records the v1 gap explicitly: a hazard +// carried as a string payload to sh -c is NOT matched. It is a stated gap, not a +// silent one — this test is what makes it visible if the behaviour ever changes. +func TestEvalPayloadIsDocumentedV1Gap(t *testing.T) { + d := checkOK(t, `sh -c 'cd scratch && rm -rf *'`) + if d.Verdict != VerdictAllow { + t.Fatalf("v1 parses no payload strings; got %+v — update the documented gap before changing this", d) + } +} + +func TestCheckRejectsUnparsableCommand(t *testing.T) { + if _, err := Defaults().Check(`rm -rf "unterminated`); !errors.Is(err, ErrUnparsableCommand) { + t.Fatalf("error = %v, want ErrUnparsableCommand", err) + } +} + +func TestDisabledRegistryAllowsEverything(t *testing.T) { + r := Defaults() + r.Disabled = true + d, err := r.Check("cd scratch && rm -rf *") + if err != nil { + t.Fatal(err) + } + if d.Verdict != VerdictAllow { + t.Fatalf("a disabled registry must allow everything, got %+v", d) + } +} + +// TestDefaultsAreACopy proves the bundled registry cannot be mutated through a +// caller's handle — the next Check must not inherit another caller's edit. +func TestDefaultsAreACopy(t *testing.T) { + r := Defaults() + e := r.Entries["git-reset-hard"] + e.Tier = TierBlocker + r.Entries["git-reset-hard"] = e + delete(r.Entries, "git-push-force") + + fresh := Defaults() + if fresh.Entries["git-reset-hard"].Tier != TierWarn { + t.Fatal("mutating a Defaults() handle changed the bundled registry") + } + if _, ok := fresh.Entries["git-push-force"]; !ok { + t.Fatal("deleting from a Defaults() handle removed a bundled entry") + } +} + +// TestDefaultsValidate proves the embedded registry passes the same validation a +// per-repo override must pass. +func TestDefaultsValidate(t *testing.T) { + if err := Validate(Defaults()); err != nil { + t.Fatalf("bundled defaults fail validation: %v", err) + } + if Defaults().SchemaVersion != SchemaVersion { + t.Fatalf("bundled schema_version = %d, want %d", Defaults().SchemaVersion, SchemaVersion) + } +} diff --git a/internal/core/guard/match.go b/internal/core/guard/match.go new file mode 100644 index 0000000..63cbc9b --- /dev/null +++ b/internal/core/guard/match.go @@ -0,0 +1,178 @@ +package guard + +import ( + "path" + "strings" +) + +// wrappers are commands that RUN another command with the same intent, so the +// hazard sits one token further along: `sudo rm -rf *` is an `rm`. Only the +// wrapper NAME is stepped over — a wrapper's own flags are not parsed (`sudo -u +// bob rm -rf *` reads `-u` as the command), a narrow v1 limitation of the same +// family as the eval gap. +var wrappers = map[string]bool{ + "sudo": true, + "doas": true, + "command": true, + "env": true, + "nohup": true, + "time": true, +} + +// matchesAny reports whether the pattern fires on any segment of the candidate. +func matchesAny(p Pattern, segs []segment) bool { + for i, s := range segs { + if matchSegment(p, s) && (p.AfterCD == nil || !*p.AfterCD || precededByCD(segs[:i], s.chain)) { + return true + } + } + return false +} + +// precededByCD reports whether an earlier command in the SAME chain is a `cd`. +// A cd on a previous logical line does not chain: a new line is a new shell +// command, and its failure cannot redirect this one. +func precededByCD(before []segment, chain int) bool { + for _, s := range before { + if s.chain != chain { + continue + } + if cmd, _ := commandOf(s); cmd == "cd" { + return true + } + } + return false +} + +// commandOf returns the segment's command name (basename, wrappers and +// environment-assignment prefixes stepped over) and the arguments that follow +// it. An empty name means the segment holds no command (assignments only). +func commandOf(s segment) (string, []string) { + i := 0 + for i < len(s.tokens) { + tok := s.tokens[i] + if isAssignment(tok) || wrappers[path.Base(tok)] { + i++ + continue + } + break + } + if i >= len(s.tokens) { + return "", nil + } + return path.Base(s.tokens[i]), s.tokens[i+1:] +} + +// isAssignment reports whether a token is a NAME=VALUE environment prefix, +// which precedes the command rather than being one. +func isAssignment(tok string) bool { + eq := strings.IndexByte(tok, '=') + if eq <= 0 { + return false + } + for i := 0; i < eq; i++ { + c := tok[i] + switch { + case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c >= '0' && c <= '9', c == '_': + default: + return false + } + } + return !(tok[0] >= '0' && tok[0] <= '9') +} + +// matchSegment applies the pattern's command, subcommand, and flag constraints +// to one command-position segment. +func matchSegment(p Pattern, s segment) bool { + cmd, args := commandOf(s) + if cmd == "" || cmd != p.Command { + return false + } + if p.Subcommand != "" && subcommandOf(args, p.ValueFlags) != p.Subcommand { + return false + } + for _, group := range p.Flags { + if !flagGroupMatches(group, args) { + return false + } + } + return true +} + +// subcommandOf returns the first non-flag argument, stepping over the value of +// any flag listed in valueFlags (`git -C /repo push` is a push). An unknown +// value-taking flag is not stepped over — the miss is a non-match, never a +// false block. +func subcommandOf(args []string, valueFlags []string) string { + for i := 0; i < len(args); i++ { + a := args[i] + if !strings.HasPrefix(a, "-") { + return a + } + if a == "--" { + continue + } + if !strings.Contains(a, "=") && containsString(valueFlags, a) { + i++ // its value is not the subcommand + } + } + return "" +} + +// flagGroupMatches reports whether any alternative in one "a|b" group is present +// among the argument tokens. +func flagGroupMatches(group string, args []string) bool { + for _, alt := range strings.Split(group, "|") { + if alt == "" { + continue + } + for _, arg := range args { + if flagMatches(alt, arg) { + return true + } + } + } + return false +} + +// flagMatches compares one flag alternative with one argument token. A long +// flag also matches its --flag=value form; a single-letter short flag also +// matches inside a bundled cluster, so -rf satisfies both -r and -f. +func flagMatches(alt, arg string) bool { + if alt == arg { + return true + } + if strings.HasPrefix(alt, "--") { + return strings.HasPrefix(arg, alt+"=") + } + if len(alt) == 2 && alt[0] == '-' { + return isShortCluster(arg) && strings.ContainsRune(arg[1:], rune(alt[1])) + } + return false +} + +// isShortCluster reports whether a token is a bundled short-flag cluster +// (-rf, -xfd): a single leading dash followed by letters or digits only. +func isShortCluster(arg string) bool { + if len(arg) < 2 || arg[0] != '-' || arg[1] == '-' { + return false + } + for i := 1; i < len(arg); i++ { + c := arg[i] + switch { + case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c >= '0' && c <= '9': + default: + return false + } + } + return true +} + +func containsString(xs []string, want string) bool { + for _, x := range xs { + if x == want { + return true + } + } + return false +} From 7f4259cb3594765734f8feefe10303e602e878ad Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:22:22 +0100 Subject: [PATCH 05/17] feat: load per-repo guard overrides from .abcd/guard.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only escape from a blocker is a committed, reviewable override, so the registry needs a per-repo file — a dedicated one rather than a rules.json domain, because the entry schema is structured and the rules kill switch must never silently disable a safety guard. The file is repo-tree content, so it is read across a trust boundary: the shared guarded read refuses a symlinked .abcd or leaf, a non-regular leaf, and anything over the size cap, and it cannot block the guard hook on a FIFO. A file that cannot be read, parsed, or validated is a typed error, never a silent fallback to the defaults or to no guard; deciding what a broken guard means for a session belongs to the hook shim, not to core. Merging is per-field so an override can retier an entry while keeping its pattern, successor, and why. AfterCD is a pointer for the same reason: a plain bool could only tighten the cd-chain requirement, never lift it. Assisted-by: Claude:claude-opus-5[1m] --- internal/core/guard/config.go | 129 +++++++++++++++ internal/core/guard/config_test.go | 241 +++++++++++++++++++++++++++++ 2 files changed, 370 insertions(+) create mode 100644 internal/core/guard/config.go create mode 100644 internal/core/guard/config_test.go diff --git a/internal/core/guard/config.go b/internal/core/guard/config.go new file mode 100644 index 0000000..bb44762 --- /dev/null +++ b/internal/core/guard/config.go @@ -0,0 +1,129 @@ +package guard + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "syscall" + + "github.com/REPPL/abcd-cli/internal/fsutil" +) + +// RepoRelPath is the per-repo override file, relative to the repo worktree. It +// is a dedicated committed file rather than a rules.json domain: the entry schema +// is structured, and the rules loader's kill switch must never silently disable a +// safety guard (spc-16, "Config home"). Disabling the guard is itself a +// committed, reviewable act. +const RepoRelPath = ".abcd/guard.json" + +// maxGuardFileBytes caps the per-repo override (trust boundary). +const maxGuardFileBytes = 256 * 1024 + +// Load returns the bundled defaults merged with /.abcd/guard.json when +// that file exists. An absent file yields the defaults unchanged. A file that +// cannot be read, parsed, or validated is an ERROR, never a silent fallback to +// the defaults or to no guard at all — core names the failure and the caller +// (the hook shim) decides what it means for the session. +func Load(repoRoot string) (Registry, error) { + // Refuse a symlinked .abcd directory component before touching the leaf, so a + // swapped .abcd cannot redirect the read (trust boundary). + if di, err := os.Lstat(filepath.Join(repoRoot, ".abcd")); err == nil && di.Mode()&os.ModeSymlink != 0 { + return Registry{}, fmt.Errorf("%w: .abcd is a symlink (refusing to follow)", ErrMalformedConfig) + } + data, err := fsutil.ReadGuarded(filepath.Join(repoRoot, ".abcd", "guard.json"), maxGuardFileBytes) + if err != nil { + switch { + case os.IsNotExist(err): + return Defaults(), nil + case errors.Is(err, syscall.ELOOP): + return Registry{}, fmt.Errorf("%w: %s is a symlink (refusing to follow)", ErrMalformedConfig, RepoRelPath) + case errors.Is(err, fsutil.ErrNotRegular): + return Registry{}, fmt.Errorf("%w: %s is not a regular file", ErrMalformedConfig, RepoRelPath) + case errors.Is(err, fsutil.ErrTooBig): + return Registry{}, fmt.Errorf("%w: %s exceeds the %d-byte cap", ErrMalformedConfig, RepoRelPath, maxGuardFileBytes) + default: + return Registry{}, fmt.Errorf("%w: reading %s failed", ErrMalformedConfig, RepoRelPath) + } + } + over, err := parse(data) + if err != nil { + return Registry{}, fmt.Errorf("%s: %w", RepoRelPath, err) + } + // The override declares its schema version explicitly: an absent version is a + // truncated or hand-mangled file, not an invitation to guess (a safety config + // fails closed on unrecognised input). + if over.SchemaVersion != SchemaVersion { + return Registry{}, fmt.Errorf("%w: %s must declare schema_version %d, got %d", ErrSchemaVersion, RepoRelPath, SchemaVersion, over.SchemaVersion) + } + merged := Merge(Defaults(), over) + if err := Validate(merged); err != nil { + return Registry{}, fmt.Errorf("%s: %w", RepoRelPath, err) + } + return merged, nil +} + +// Merge overlays over onto base. Entry fields are per-field: a field set on the +// override wins, an absent field inherits the bundled entry (so {"tier":"warn"} +// retiers an entry while keeping its pattern, successor, and why). New entry keys +// are added. The kill switch is sticky — either side can disable the guard, and +// neither can silently re-enable the other's refusal to run it. +func Merge(base, over Registry) Registry { + out := cloneRegistry(base) + if over.SchemaVersion != 0 { + out.SchemaVersion = over.SchemaVersion + } + out.Disabled = base.Disabled || over.Disabled + if out.Entries == nil && len(over.Entries) > 0 { + out.Entries = make(map[string]Entry, len(over.Entries)) + } + for id, oe := range over.Entries { + out.Entries[id] = mergeEntry(out.Entries[id], oe) + } + return out +} + +func mergeEntry(base, over Entry) Entry { + r := cloneEntry(base) + r.ID = over.ID + if over.Tier != "" { + r.Tier = over.Tier + } + if over.Successor != "" { + r.Successor = over.Successor + } + if over.Why != "" { + r.Why = over.Why + } + if over.Fixtures.KnownBad != nil { + r.Fixtures.KnownBad = append([]string(nil), over.Fixtures.KnownBad...) + } + if over.Fixtures.KnownGood != nil { + r.Fixtures.KnownGood = append([]string(nil), over.Fixtures.KnownGood...) + } + r.Pattern = mergePattern(r.Pattern, over.Pattern) + return r +} + +func mergePattern(base, over Pattern) Pattern { + r := base + if over.Command != "" { + r.Command = over.Command + } + if over.Subcommand != "" { + r.Subcommand = over.Subcommand + } + if over.ValueFlags != nil { + r.ValueFlags = append([]string(nil), over.ValueFlags...) + } + if over.Flags != nil { + r.Flags = append([]string(nil), over.Flags...) + } + // AfterCD is a pointer precisely so an override can set it to false — a + // bool field could only ever tighten the requirement, never lift it. + if over.AfterCD != nil { + v := *over.AfterCD + r.AfterCD = &v + } + return r +} diff --git a/internal/core/guard/config_test.go b/internal/core/guard/config_test.go new file mode 100644 index 0000000..ff65492 --- /dev/null +++ b/internal/core/guard/config_test.go @@ -0,0 +1,241 @@ +package guard + +import ( + "errors" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + "time" +) + +// writeOverride lays down a repo root with .abcd/guard.json and returns the root. +func writeOverride(t *testing.T, body string) string { + t.Helper() + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".abcd"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, ".abcd", "guard.json"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + return dir +} + +func TestLoadWithoutOverrideReturnsDefaults(t *testing.T) { + r, err := Load(t.TempDir()) + if err != nil { + t.Fatalf("an absent %s must yield the defaults, got %v", RepoRelPath, err) + } + if len(r.Entries) != len(Defaults().Entries) { + t.Fatalf("Entries = %d, want the %d bundled entries", len(r.Entries), len(Defaults().Entries)) + } +} + +// TestLoadOverridesEntryPerField proves the committed override is the only +// escape hatch AND that it is per-field: retiering an entry keeps its pattern, +// successor, and why. +func TestLoadOverridesEntryPerField(t *testing.T) { + root := writeOverride(t, `{"schema_version":1,"entries":{"git-reset-hard":{"tier":"blocker"}}}`) + r, err := Load(root) + if err != nil { + t.Fatal(err) + } + e := r.Entries["git-reset-hard"] + if e.Tier != TierBlocker { + t.Fatalf("tier = %q, want the override's %q", e.Tier, TierBlocker) + } + if e.Why != Defaults().Entries["git-reset-hard"].Why || e.Successor == "" { + t.Fatal("an unset override field must inherit the bundled entry") + } + d, err := r.Check("git reset --hard origin/main") + if err != nil { + t.Fatal(err) + } + if d.Verdict != VerdictBlock { + t.Fatalf("verdict = %q, want the retiered %q", d.Verdict, VerdictBlock) + } +} + +// TestLoadOverridesPatternPerField pins the pointer semantics of after_cd: an +// override can turn the cd-chain requirement OFF as well as on. +func TestLoadOverridesPatternPerField(t *testing.T) { + root := writeOverride(t, `{"schema_version":1,"entries":{"rm-rf-after-cd-chain":{"pattern":{"after_cd":false}}}}`) + r, err := Load(root) + if err != nil { + t.Fatal(err) + } + if got := r.Entries["rm-rf-after-cd-chain"].Pattern.Command; got != "rm" { + t.Fatalf("pattern command = %q, want the inherited %q", got, "rm") + } + d, err := r.Check("rm -rf ./build") + if err != nil { + t.Fatal(err) + } + if d.Verdict != VerdictBlock { + t.Fatalf("verdict = %q, want %q once the cd-chain requirement is lifted", d.Verdict, VerdictBlock) + } +} + +func TestLoadAddsRepoEntry(t *testing.T) { + root := writeOverride(t, `{"schema_version":1,"entries":{"drop-database":{ + "tier":"blocker", + "pattern":{"command":"dropdb"}, + "why":"It deletes the whole database.", + "successor":"Take a dump first."}}}`) + r, err := Load(root) + if err != nil { + t.Fatal(err) + } + d, err := r.Check("dropdb production") + if err != nil { + t.Fatal(err) + } + if d.Verdict != VerdictBlock || d.EntryID != "drop-database" { + t.Fatalf("a repo-added entry must fire, got %+v", d) + } + if len(r.Entries) != len(Defaults().Entries)+1 { + t.Fatal("adding an entry must not drop the bundled ones") + } +} + +func TestLoadHonoursCommittedKillSwitch(t *testing.T) { + root := writeOverride(t, `{"schema_version":1,"disabled":true}`) + r, err := Load(root) + if err != nil { + t.Fatal(err) + } + d, err := r.Check("cd scratch && rm -rf *") + if err != nil { + t.Fatal(err) + } + if d.Verdict != VerdictAllow { + t.Fatalf("a committed kill switch must disable the guard, got %+v", d) + } +} + +func TestLoadRejectsBadConfig(t *testing.T) { + tests := []struct { + name string + body string + want error + }{ + { + name: "unknown tier", + body: `{"schema_version":1,"entries":{"git-clean":{"tier":"fatal"}}}`, + want: ErrUnknownTier, + }, + { + name: "unsupported schema version", + body: `{"schema_version":2,"entries":{}}`, + want: ErrSchemaVersion, + }, + { + name: "absent schema version", + body: `{"entries":{}}`, + want: ErrSchemaVersion, + }, + { + name: "malformed json", + body: `{"schema_version":1,`, + want: ErrMalformedConfig, + }, + { + name: "new entry without a successor", + body: `{"schema_version":1,"entries":{"drop-database":{"tier":"blocker","pattern":{"command":"dropdb"},"why":"It deletes everything."}}}`, + want: ErrInvalidEntry, + }, + { + name: "new entry without a pattern command", + body: `{"schema_version":1,"entries":{"drop-database":{"tier":"blocker","why":"It deletes everything.","successor":"Take a dump first."}}}`, + want: ErrInvalidEntry, + }, + { + name: "entry id that could build a path", + body: `{"schema_version":1,"entries":{"../escape":{"tier":"warn","pattern":{"command":"x"},"why":"w","successor":"s"}}}`, + want: ErrInvalidEntry, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := Load(writeOverride(t, tc.body)) + if !errors.Is(err, tc.want) { + t.Fatalf("Load error = %v, want %v", err, tc.want) + } + if strings.Contains(err.Error(), os.TempDir()) { + t.Fatalf("error text leaks an absolute local path: %v", err) + } + }) + } +} + +// TestLoadRefusesSymlinkedGuardFile keeps the trust-boundary read honest: the +// override is repo-tree content, so a symlinked leaf is refused rather than +// followed to an arbitrary target. +func TestLoadRefusesSymlinkedGuardFile(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".abcd"), 0o755); err != nil { + t.Fatal(err) + } + target := filepath.Join(dir, "real-guard.json") + if err := os.WriteFile(target, []byte(`{"schema_version":1}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, filepath.Join(dir, ".abcd", "guard.json")); err != nil { + t.Fatal(err) + } + if _, err := Load(dir); !errors.Is(err, ErrMalformedConfig) { + t.Fatalf("a symlinked guard.json leaf must be refused, not followed; got %v", err) + } +} + +func TestLoadRefusesSymlinkedAbcdDir(t *testing.T) { + dir := t.TempDir() + real := filepath.Join(dir, "elsewhere") + if err := os.MkdirAll(real, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(real, "guard.json"), []byte(`{"schema_version":1}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(real, filepath.Join(dir, ".abcd")); err != nil { + t.Fatal(err) + } + if _, err := Load(dir); !errors.Is(err, ErrMalformedConfig) { + t.Fatalf("a symlinked .abcd directory must be refused, not followed; got %v", err) + } +} + +func TestLoadRefusesOversizeConfig(t *testing.T) { + root := writeOverride(t, `{"schema_version":1,"entries":{}}`+strings.Repeat(" ", maxGuardFileBytes)) + if _, err := Load(root); !errors.Is(err, ErrMalformedConfig) { + t.Fatalf("Load error = %v, want ErrMalformedConfig for an oversize file", err) + } +} + +// TestLoadDoesNotBlockOnAFifo proves the read cannot wedge a hook: the guard is +// called on the execution path of every command, so a non-regular leaf must be +// refused promptly rather than blocking the open forever. +func TestLoadDoesNotBlockOnAFifo(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".abcd"), 0o755); err != nil { + t.Fatal(err) + } + if err := syscall.Mkfifo(filepath.Join(dir, ".abcd", "guard.json"), 0o644); err != nil { + t.Skipf("mkfifo unsupported: %v", err) + } + done := make(chan error, 1) + go func() { + _, err := Load(dir) + done <- err + }() + select { + case err := <-done: + if err == nil { + t.Fatal("a FIFO guard.json must be refused, not read") + } + case <-time.After(3 * time.Second): + t.Fatal("Load hung on a FIFO guard.json leaf (must never block the guard hook)") + } +} From 2640b50dc3d9d335753c74ac15a8f76a14fd8ec9 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:24:57 +0100 Subject: [PATCH 06/17] feat: reject a guard flag group that could never match An override could write a flag constraint with no usable alternative ("" or "|"), which is unsatisfiable: the entry would stop refusing while still being listed as active. That is the one failure mode a guard must not have, because a guard that has quietly stopped refusing looks exactly like one with nothing to refuse. It is now a load-time rejection, like a missing successor or why. Assisted-by: Claude:claude-opus-5[1m] --- internal/core/guard/config_test.go | 9 +++++++++ internal/core/guard/guard.go | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/internal/core/guard/config_test.go b/internal/core/guard/config_test.go index ff65492..745f532 100644 --- a/internal/core/guard/config_test.go +++ b/internal/core/guard/config_test.go @@ -151,6 +151,15 @@ func TestLoadRejectsBadConfig(t *testing.T) { body: `{"schema_version":1,"entries":{"drop-database":{"tier":"blocker","why":"It deletes everything.","successor":"Take a dump first."}}}`, want: ErrInvalidEntry, }, + { + // An empty flag group can never be satisfied, so it would silently + // defang the entry it belongs to — the exact failure a guard must + // never have (a guard that stops refusing looks identical to one + // with nothing to refuse). + name: "flag group that can never match", + body: `{"schema_version":1,"entries":{"git-clean":{"pattern":{"flags":["-f","|"]}}}}`, + want: ErrInvalidEntry, + }, { name: "entry id that could build a path", body: `{"schema_version":1,"entries":{"../escape":{"tier":"warn","pattern":{"command":"x"},"why":"w","successor":"s"}}}`, diff --git a/internal/core/guard/guard.go b/internal/core/guard/guard.go index 9efa14b..f6d13ac 100644 --- a/internal/core/guard/guard.go +++ b/internal/core/guard/guard.go @@ -178,10 +178,29 @@ func Validate(r Registry) error { if strings.TrimSpace(e.Why) == "" { return fmt.Errorf("%w: entry %s has no why", ErrInvalidEntry, id) } + // An empty flag group can never be satisfied, so it would silently + // defang the entry rather than fail loudly — the one failure mode a + // guard must not have. + for i, group := range e.Pattern.Flags { + if !hasAlternative(group) { + return fmt.Errorf("%w: entry %s flag group %d is empty and could never match", ErrInvalidEntry, id, i) + } + } } return nil } +// hasAlternative reports whether a flag group holds at least one usable +// alternative, so neither "" nor "|" can pass as a constraint. +func hasAlternative(group string) bool { + for _, alt := range strings.Split(group, "|") { + if strings.TrimSpace(alt) != "" { + return true + } + } + return false +} + func validEntryID(id string) bool { if id == "" { return false From 3adde8cc1b8b621bba89a54229b947063626484e Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:39:39 +0100 Subject: [PATCH 07/17] fix: close four guard matching and validation holes found in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-line input was wrong in both directions, and no fixture could see it because every bundled fixture was a single line. - Heredoc bodies were tokenised as commands, so writing a document that NAMES a hazard was refused at blocker tier. That is the same class as the quoted argument the whole design turns on, one syntax over: the body is data. The tokenizer now consumes pending heredoc bodies at the newline that starts them, honouring the quoted and tab-stripping delimiter forms, and leaves a herestring as the ordinary argument it is. - A newline after a list operator started a new chain, so the flagship entry missed its own hazard in the wrapped form agents most often emit. - Shell keywords were read as the command, so a hazard inside a conditional or a loop body never reached command position. - Validation admitted patterns nothing could satisfy (a command with a slash or a space, a subcommand starting with a dash), the same silent defang the empty flag-group check exists to prevent. An unrecognised key in a per-repo override is now rejected too, rather than dropping a blocker the repo believed it had. Each class gains a fixture, so the admission gate — not a reviewer — is what holds them from here. Assisted-by: Claude:claude-opus-5[1m] --- internal/core/guard/config_test.go | 20 ++++ internal/core/guard/defaults/guard.json | 12 ++- internal/core/guard/guard.go | 22 +++- internal/core/guard/guard_test.go | 23 ++++ internal/core/guard/match.go | 18 +++- internal/core/guard/tokenize.go | 136 ++++++++++++++++++++++-- internal/core/guard/tokenize_test.go | 40 +++++++ 7 files changed, 259 insertions(+), 12 deletions(-) diff --git a/internal/core/guard/config_test.go b/internal/core/guard/config_test.go index 745f532..1d75b92 100644 --- a/internal/core/guard/config_test.go +++ b/internal/core/guard/config_test.go @@ -160,6 +160,26 @@ func TestLoadRejectsBadConfig(t *testing.T) { body: `{"schema_version":1,"entries":{"git-clean":{"pattern":{"flags":["-f","|"]}}}}`, want: ErrInvalidEntry, }, + { + // The command is compared against a token's basename, so a path or a + // phrase could never be satisfied — the same silent defang as an + // empty flag group. + name: "pattern command that could never match", + body: `{"schema_version":1,"entries":{"drop-database":{"tier":"blocker","pattern":{"command":"/usr/bin/dropdb"},"why":"It deletes everything.","successor":"Take a dump first."}}}`, + want: ErrInvalidEntry, + }, + { + name: "subcommand that could never match", + body: `{"schema_version":1,"entries":{"drop-database":{"tier":"blocker","pattern":{"command":"dropdb","subcommand":"--all"},"why":"It deletes everything.","successor":"Take a dump first."}}}`, + want: ErrInvalidEntry, + }, + { + // A misspelt top-level key would otherwise be dropped in silence, and + // the repo's intended blocker would simply not exist. + name: "unrecognised key", + body: `{"schema_version":1,"entrys":{"git-clean":{"tier":"blocker"}}}`, + want: ErrMalformedConfig, + }, { name: "entry id that could build a path", body: `{"schema_version":1,"entries":{"../escape":{"tier":"warn","pattern":{"command":"x"},"why":"w","successor":"s"}}}`, diff --git a/internal/core/guard/defaults/guard.json b/internal/core/guard/defaults/guard.json index 514e30c..909d9c7 100644 --- a/internal/core/guard/defaults/guard.json +++ b/internal/core/guard/defaults/guard.json @@ -16,13 +16,18 @@ "cd scratch && rm -rf *", "cd /tmp/build; rm -rf .", "cd scratch && sudo rm -fr ./*", - "cd worktree && rm -r -f build" + "cd worktree && rm -r -f build", + "cd scratch &&\nrm -rf *", + "if cd scratch; then rm -rf *; fi", + "for d in a b; do cd $d && rm -rf *; done" ], "known_good": [ "./bin/abcd-darwin-arm64 capture \"agent ran cd scratch && rm -rf * — one failed cd from disaster\"", "rm -rf ./build", "cd scratch && ls -la", - "cd scratch && rm ./notes.txt" + "cd scratch && rm ./notes.txt", + "cat > NOTES.md <<'EOF'\ncd scratch && rm -rf *\nEOF", + "cd scratch\nrm -rf ./build" ] } }, @@ -48,7 +53,8 @@ "git push origin main", "git push --set-upstream origin feat/shell-hazard-registry", "abcd capture \"an agent tried git push --force on main\"", - "git log --oneline -5" + "git log --oneline -5", + "cat > CONTRIBUTING.md <<'EOF'\nNever run git push --force on main.\nEOF" ] } }, diff --git a/internal/core/guard/guard.go b/internal/core/guard/guard.go index f6d13ac..abd1af8 100644 --- a/internal/core/guard/guard.go +++ b/internal/core/guard/guard.go @@ -1,6 +1,7 @@ package guard import ( + "bytes" _ "embed" "encoding/json" "fmt" @@ -131,11 +132,20 @@ func mustParseDefaults() Registry { func Defaults() Registry { return cloneRegistry(defaultRegistry) } // parse decodes registry JSON and stamps each entry's ID from its map key. +// Unknown fields and trailing content are rejected: a misspelt key would +// otherwise be dropped in silence, and the blocker a repo believed it had +// declared would simply not exist (a safety config fails closed on +// unrecognised input). func parse(data []byte) (Registry, error) { var r Registry - if err := json.Unmarshal(data, &r); err != nil { + dec := json.NewDecoder(bytes.NewReader(data)) + dec.DisallowUnknownFields() + if err := dec.Decode(&r); err != nil { return Registry{}, fmt.Errorf("%w: %v", ErrMalformedConfig, err) } + if dec.More() { + return Registry{}, fmt.Errorf("%w: trailing content after the JSON object", ErrMalformedConfig) + } for id, e := range r.Entries { e.ID = id r.Entries[id] = e @@ -169,6 +179,16 @@ func Validate(r Registry) error { if strings.TrimSpace(e.Pattern.Command) == "" { return fmt.Errorf("%w: entry %s has no pattern command", ErrInvalidEntry, id) } + // The command is compared against a token's BASENAME, and a subcommand is + // only ever a non-flag argument, so a path, a phrase, or a leading dash + // describes a pattern nothing can satisfy. Reject it here rather than + // ship an entry that looks armed and never fires. + if strings.ContainsAny(e.Pattern.Command, "/ \t") { + return fmt.Errorf("%w: entry %s pattern command %q is a path or phrase and could never match a command name", ErrInvalidEntry, id, e.Pattern.Command) + } + if strings.HasPrefix(e.Pattern.Subcommand, "-") { + return fmt.Errorf("%w: entry %s subcommand %q starts with a dash and could never match a non-flag argument", ErrInvalidEntry, id, e.Pattern.Subcommand) + } // A refusal with no successor leaves its replacement in prose only, and // one with no why cannot teach — both are load-time rejections, as in // the record-lint banned_tokens family (iss-51). diff --git a/internal/core/guard/guard_test.go b/internal/core/guard/guard_test.go index 864ad49..8579902 100644 --- a/internal/core/guard/guard_test.go +++ b/internal/core/guard/guard_test.go @@ -92,6 +92,29 @@ func TestCheckDecisions(t *testing.T) { command: "git status --porcelain", verdict: VerdictAllow, }, + { + name: "a cd chain wrapped over a newline still fires", + command: "cd scratch &&\nrm -rf *", + verdict: VerdictBlock, + entryID: "rm-rf-after-cd-chain", + }, + { + name: "a hazard inside a shell conditional is still in command position", + command: "if cd scratch; then rm -rf *; fi", + verdict: VerdictBlock, + entryID: "rm-rf-after-cd-chain", + }, + { + name: "a hazard inside a loop body is still in command position", + command: "for d in a b; do cd $d && rm -rf *; done", + verdict: VerdictBlock, + entryID: "rm-rf-after-cd-chain", + }, + { + name: "writing a document that names a hazard is not running one", + command: "cat > CONTRIBUTING.md <<'EOF'\nNever run git push --force on main.\nEOF", + verdict: VerdictAllow, + }, { name: "a cd on the previous line does not chain into the next line", command: "cd scratch\nrm -rf ./build", diff --git a/internal/core/guard/match.go b/internal/core/guard/match.go index 63cbc9b..8239284 100644 --- a/internal/core/guard/match.go +++ b/internal/core/guard/match.go @@ -19,6 +19,22 @@ var wrappers = map[string]bool{ "time": true, } +// reserved are shell keywords and grouping tokens that PRECEDE a command rather +// than being one: without stepping over them, `if cd scratch; then rm -rf *; fi` +// reads `then` as argv[0] and every hazard inside a conditional or a loop body +// escapes command position. +var reserved = map[string]bool{ + "if": true, + "then": true, + "elif": true, + "else": true, + "do": true, + "while": true, + "until": true, + "{": true, + "!": true, +} + // matchesAny reports whether the pattern fires on any segment of the candidate. func matchesAny(p Pattern, segs []segment) bool { for i, s := range segs { @@ -51,7 +67,7 @@ func commandOf(s segment) (string, []string) { i := 0 for i < len(s.tokens) { tok := s.tokens[i] - if isAssignment(tok) || wrappers[path.Base(tok)] { + if isAssignment(tok) || reserved[tok] || wrappers[path.Base(tok)] { i++ continue } diff --git a/internal/core/guard/tokenize.go b/internal/core/guard/tokenize.go index 2810cd7..2b4c311 100644 --- a/internal/core/guard/tokenize.go +++ b/internal/core/guard/tokenize.go @@ -1,6 +1,9 @@ package guard -import "fmt" +import ( + "fmt" + "strings" +) // segment is one command in command position: the tokens of a single simple // command, plus the index of the logical line (chain) it belongs to. Segments in @@ -28,11 +31,16 @@ type segment struct { // the verb's reference doc and the registry README rather than silently assumed. func tokenize(line string) ([]segment, error) { var ( - segs []segment - toks []string - cur []byte - hasCur bool - chain int + segs []segment + toks []string + cur []byte + hasCur bool + chain int + pending []heredoc + // lastList records that the previous operator was a list operator + // (`&&`, `||`, `|`), whose newline is a line continuation rather than a + // new command — `cd scratch &&\nrm -rf *` is one chain, not two. + lastList bool ) flushToken := func() { if hasCur { @@ -63,6 +71,7 @@ func tokenize(line string) ([]segment, error) { } cur = append(cur, line[i+1]) hasCur = true + lastList = false i += 2 case c == '\'': j := i + 1 @@ -74,6 +83,7 @@ func tokenize(line string) ([]segment, error) { } cur = append(cur, line[i+1:j]...) hasCur = true + lastList = false i = j + 1 case c == '"': j := i + 1 @@ -103,33 +113,145 @@ func tokenize(line string) ([]segment, error) { return nil, fmt.Errorf("%w: unterminated double quote", ErrUnparsableCommand) } hasCur = true + lastList = false i = j + 1 case c == ' ' || c == '\t' || c == '\r': flushToken() i++ case c == '\n': flushSegment() - chain++ i++ + // A pending heredoc body starts on the next line and is DATA, not + // commands: writing a document that names a hazard must never read + // as running one. + if len(pending) > 0 { + i = skipHeredocBodies(line, i, pending) + pending = nil + } + if !lastList { + chain++ + } + lastList = false case c == '#' && !hasCur: // A comment starts only at a word boundary (POSIX): `url/#frag` is // part of the token, a bare `#` runs to the end of the line. for i < len(line) && line[i] != '\n' { i++ } + case c == '<' && strings.HasPrefix(line[i:], "<<<"): + // A herestring, not a heredoc: its payload is an ordinary argument + // token, so the operator is kept as plain token text. + cur = append(cur, '<', '<', '<') + hasCur = true + lastList = false + i += 3 + case c == '<' && strings.HasPrefix(line[i:], "<<"): + // A heredoc redirection (`<<`, `<<-`). + flushToken() + hd, next, err := readHeredocDelim(line, i+2) + if err != nil { + return nil, err + } + pending = append(pending, hd) + i = next case c == '&' || c == '|' || c == ';' || c == '(' || c == ')': flushSegment() if (c == '&' || c == '|') && i+1 < len(line) && line[i+1] == c { + lastList = true i += 2 continue } + // A single pipe continues the list across a newline; `;`, `&`, and + // the grouping parens do not. + lastList = c == '|' i++ default: cur = append(cur, c) hasCur = true + lastList = false i++ } } flushSegment() return segs, nil } + +// heredoc is one pending here-document: the delimiter word that ends its body, +// and whether the `<<-` form allows the delimiter line to be tab-indented. +type heredoc struct { + delim string + stripTabs bool +} + +// readHeredocDelim reads the delimiter word after a `<<` at pos, honouring the +// `<<-` variant and any quoting of the word itself (`<<'EOF'`, `<<"EOF"`), and +// returns the position just past it. +func readHeredocDelim(line string, pos int) (heredoc, int, error) { + hd := heredoc{} + if pos < len(line) && line[pos] == '-' { + hd.stripTabs = true + pos++ + } + for pos < len(line) && (line[pos] == ' ' || line[pos] == '\t') { + pos++ + } + var w []byte + for pos < len(line) { + c := line[pos] + switch c { + case '\'', '"': + j := pos + 1 + for j < len(line) && line[j] != c { + j++ + } + if j >= len(line) { + return heredoc{}, 0, fmt.Errorf("%w: unterminated quote in heredoc delimiter", ErrUnparsableCommand) + } + w = append(w, line[pos+1:j]...) + pos = j + 1 + continue + case '\\': + if pos+1 >= len(line) { + return heredoc{}, 0, fmt.Errorf("%w: trailing backslash", ErrUnparsableCommand) + } + w = append(w, line[pos+1]) + pos += 2 + continue + case ' ', '\t', '\n', ';', '&', '|', '(', ')', '<', '>': + hd.delim = string(w) + return hd, pos, nil + } + w = append(w, c) + pos++ + } + hd.delim = string(w) + return hd, pos, nil +} + +// skipHeredocBodies consumes the body of every pending here-document, starting +// at pos (the first byte after the newline that ended the command line), and +// returns the position just past the last body. An unterminated body swallows +// the rest of the input — exactly as a shell would treat it. +func skipHeredocBodies(line string, pos int, pending []heredoc) int { + for _, hd := range pending { + for pos < len(line) { + end := pos + for end < len(line) && line[end] != '\n' { + end++ + } + text := line[pos:end] + if hd.stripTabs { + text = strings.TrimLeft(text, "\t") + } + if end < len(line) { + pos = end + 1 + } else { + pos = end + } + if text == hd.delim { + break + } + } + } + return pos +} diff --git a/internal/core/guard/tokenize_test.go b/internal/core/guard/tokenize_test.go index 4714960..2292e7c 100644 --- a/internal/core/guard/tokenize_test.go +++ b/internal/core/guard/tokenize_test.go @@ -92,6 +92,46 @@ func TestTokenizeSegments(t *testing.T) { line: `git commit -m "fix #123"`, want: []string{"0:git|commit|-m|fix #123"}, }, + { + name: "a newline after a list operator continues the same chain", + line: "cd scratch &&\nrm -rf *", + want: []string{"0:cd|scratch", "0:rm|-rf|*"}, + }, + { + name: "a newline after a pipe continues the same chain", + line: "cd scratch |\nrm -rf *", + want: []string{"0:cd|scratch", "0:rm|-rf|*"}, + }, + { + name: "heredoc body is data, not commands", + line: "cat > doc.md <<'EOF'\ngit push --force\nEOF", + want: []string{"0:cat|>|doc.md"}, + }, + { + name: "heredoc body ends at its delimiter line", + line: "cat < Date: Mon, 27 Jul 2026 20:41:13 +0100 Subject: [PATCH 08/17] docs: map the guard package in internal/README The package map is where a reader learns what each core package owns; a new capability that is absent from it is invisible until someone greps for it. Assisted-by: Claude:claude-opus-5[1m] --- internal/README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/README.md b/internal/README.md index 1601fd9..221eb25 100644 --- a/internal/README.md +++ b/internal/README.md @@ -30,6 +30,15 @@ plugin surface, and a future MCP server share one engine. the two agree. The table is a *hypothesis*: `abcd disembark probe` measures the same sections against real repositories in the same `grounded`/`partial`/`blank` vocabulary, and the evidence is expected to revise it (adr-35, itd-88). +- **`core/guard/`** — the shell-hazard registry. Bundled hazard entries (id, + command-position pattern over shell tokens, blocker/warn tier, safe successor, + plain-language why) plus the deterministic allow/warn/block decision a harness + hook calls before a command runs. Matching is token-aware and command-position + only, so a hazard named inside a quoted argument never fires; every bundled + entry ships known-bad and known-good fixtures, and the admission gate test + holds each one to its own corpus (100% true-negative floor, at least 40% + known-good). Fail-open-loud on a broken guard belongs to the hook shim, not + here. - **`surface/cli/`** — the default front door: a Cobra command tree that calls `core` and formats results as text or `--json`. Holds no business logic. - **`surface/mcp/`** *(later)* — an additive front door exposing the same core From 9bae2c36ec340ea813af4b7a18e7c2bd1da0630a Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:42:05 +0100 Subject: [PATCH 09/17] fix: hold a heredoc body until its command line is complete A here-document body starts after the command LINE ends, not after the newline that queued it. On a line ending in a list operator the command is unfinished, so consuming the body there swallowed the continuation as body text and every command on it escaped matching. Assisted-by: Claude:claude-opus-5[1m] --- internal/core/guard/tokenize.go | 10 ++++++---- internal/core/guard/tokenize_test.go | 7 +++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/internal/core/guard/tokenize.go b/internal/core/guard/tokenize.go index 2b4c311..97a38bd 100644 --- a/internal/core/guard/tokenize.go +++ b/internal/core/guard/tokenize.go @@ -121,10 +121,12 @@ func tokenize(line string) ([]segment, error) { case c == '\n': flushSegment() i++ - // A pending heredoc body starts on the next line and is DATA, not - // commands: writing a document that names a hazard must never read - // as running one. - if len(pending) > 0 { + // A pending heredoc body starts once the command LINE is complete, + // and is DATA, not commands: writing a document that names a hazard + // must never read as running one. A line ending in a list operator + // is not complete, so the body waits — what follows it is still + // command text. + if len(pending) > 0 && !lastList { i = skipHeredocBodies(line, i, pending) pending = nil } diff --git a/internal/core/guard/tokenize_test.go b/internal/core/guard/tokenize_test.go index 2292e7c..97e60fe 100644 --- a/internal/core/guard/tokenize_test.go +++ b/internal/core/guard/tokenize_test.go @@ -122,6 +122,13 @@ func TestTokenizeSegments(t *testing.T) { line: "cat < Date: Mon, 27 Jul 2026 20:43:07 +0100 Subject: [PATCH 10/17] fix: stop the git-clean successor recommending a warned command The entry warns on any git clean, so telling the reader to run git clean --dry-run instead offered a successor that trips the same warning. The advice now points at a command the guard is silent on. Assisted-by: Claude:claude-opus-5[1m] --- internal/core/guard/defaults/guard.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/core/guard/defaults/guard.json b/internal/core/guard/defaults/guard.json index 909d9c7..4b105a4 100644 --- a/internal/core/guard/defaults/guard.json +++ b/internal/core/guard/defaults/guard.json @@ -134,7 +134,7 @@ "value_flags": ["-C", "-c", "--git-dir", "--work-tree", "--namespace", "--exec-path", "--super-prefix"] }, "why": "`git clean` deletes untracked files outright — anything never added to git, including local notes and scratch output, is gone with no copy in the repository.", - "successor": "List what would go first (`git clean --dry-run`) and move anything worth keeping aside; delete only files you created yourself.", + "successor": "See what is untracked first (`git status --ignored`), move anything worth keeping aside, and delete only files you created yourself.", "fixtures": { "known_bad": [ "git clean -fd", From 83f20a1a7d92b896198c7493aa8f9a38411b023e Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:48:34 +0100 Subject: [PATCH 11/17] fix: keep an arithmetic shift and a blank line from unguarding a session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two holes in the heredoc and continuation state machine, both found in the second review pass and both silent. - `$((1<<20))` was read as a here-document whose delimiter never arrives, so every following line was swallowed as body text and matched nothing. An unquoted delimiter must now start with a letter or an underscore; a quoted one is a delimiter however exotic it looks. - Clearing the continuation flag at the newline ended the list across a blank or comment-only line, so the wrapped cd chain was missed one blank line over. Every token-producing branch already clears it when real content arrives. An override may also declare the id it is keyed by: the documented entry shape spells one out, and rejecting it would have failed a repo's whole guard config — which, under fail-open-loud, means an unguarded session. Assisted-by: Claude:claude-opus-5[1m] --- internal/core/guard/config_test.go | 14 +++++++++++ internal/core/guard/defaults/guard.json | 8 ++++-- internal/core/guard/guard.go | 6 +++-- internal/core/guard/guard_test.go | 12 +++++++++ internal/core/guard/tokenize.go | 33 ++++++++++++++++++++++--- internal/core/guard/tokenize_test.go | 20 +++++++++++++++ 6 files changed, 86 insertions(+), 7 deletions(-) diff --git a/internal/core/guard/config_test.go b/internal/core/guard/config_test.go index 1d75b92..6516cd6 100644 --- a/internal/core/guard/config_test.go +++ b/internal/core/guard/config_test.go @@ -100,6 +100,20 @@ func TestLoadAddsRepoEntry(t *testing.T) { } } +// TestLoadAcceptsADeclaredEntryID keeps the documented entry shape loadable: an +// override may spell out the id it is already keyed by. The map key stays +// authoritative — a mismatched id must not rename the entry. +func TestLoadAcceptsADeclaredEntryID(t *testing.T) { + root := writeOverride(t, `{"schema_version":1,"entries":{"git-clean":{"id":"git-clean","tier":"blocker"}}}`) + r, err := Load(root) + if err != nil { + t.Fatalf("an entry declaring its own id must load, got %v", err) + } + if r.Entries["git-clean"].ID != "git-clean" || r.Entries["git-clean"].Tier != TierBlocker { + t.Fatalf("entry = %+v, want the keyed id and the override's tier", r.Entries["git-clean"]) + } +} + func TestLoadHonoursCommittedKillSwitch(t *testing.T) { root := writeOverride(t, `{"schema_version":1,"disabled":true}`) r, err := Load(root) diff --git a/internal/core/guard/defaults/guard.json b/internal/core/guard/defaults/guard.json index 4b105a4..1e04a9e 100644 --- a/internal/core/guard/defaults/guard.json +++ b/internal/core/guard/defaults/guard.json @@ -19,7 +19,9 @@ "cd worktree && rm -r -f build", "cd scratch &&\nrm -rf *", "if cd scratch; then rm -rf *; fi", - "for d in a b; do cd $d && rm -rf *; done" + "for d in a b; do cd $d && rm -rf *; done", + "cd scratch &&\n\nrm -rf *", + "echo $((1<<20)) > size.txt\ncd scratch && rm -rf *" ], "known_good": [ "./bin/abcd-darwin-arm64 capture \"agent ran cd scratch && rm -rf * — one failed cd from disaster\"", @@ -27,7 +29,9 @@ "cd scratch && ls -la", "cd scratch && rm ./notes.txt", "cat > NOTES.md <<'EOF'\ncd scratch && rm -rf *\nEOF", - "cd scratch\nrm -rf ./build" + "cd scratch\nrm -rf ./build", + "tee NOTES.md <<'---'\ncd scratch && rm -rf *\n---", + "cd scratch && rm -f ./notes.txt" ] } }, diff --git a/internal/core/guard/guard.go b/internal/core/guard/guard.go index abd1af8..cd386a7 100644 --- a/internal/core/guard/guard.go +++ b/internal/core/guard/guard.go @@ -72,9 +72,11 @@ type Fixtures struct { } // Entry is one hazard. The map key in Registry.Entries is its id; ID is filled -// from that key on load, so the key is always authoritative. +// from that key on load, so the key is always authoritative — a declared id is +// accepted (the documented entry shape spells one out) and then overwritten, +// never used to rename the entry. type Entry struct { - ID string `json:"-"` + ID string `json:"id,omitempty"` Pattern Pattern `json:"pattern"` Tier string `json:"tier"` Successor string `json:"successor"` diff --git a/internal/core/guard/guard_test.go b/internal/core/guard/guard_test.go index 8579902..ab0bc16 100644 --- a/internal/core/guard/guard_test.go +++ b/internal/core/guard/guard_test.go @@ -98,6 +98,18 @@ func TestCheckDecisions(t *testing.T) { verdict: VerdictBlock, entryID: "rm-rf-after-cd-chain", }, + { + name: "a blank line does not break the cd chain", + command: "cd scratch &&\n\nrm -rf *", + verdict: VerdictBlock, + entryID: "rm-rf-after-cd-chain", + }, + { + name: "an arithmetic shift does not swallow the lines after it", + command: "echo $((1<<20)) > size.txt\ncd scratch && rm -rf *", + verdict: VerdictBlock, + entryID: "rm-rf-after-cd-chain", + }, { name: "a hazard inside a shell conditional is still in command position", command: "if cd scratch; then rm -rf *; fi", diff --git a/internal/core/guard/tokenize.go b/internal/core/guard/tokenize.go index 97a38bd..69ff9d3 100644 --- a/internal/core/guard/tokenize.go +++ b/internal/core/guard/tokenize.go @@ -130,10 +130,12 @@ func tokenize(line string) ([]segment, error) { i = skipHeredocBodies(line, i, pending) pending = nil } + // lastList is NOT cleared here: a blank or comment-only line after a + // list operator does not end the list, and every token-producing + // branch clears the flag as soon as real content arrives. if !lastList { chain++ } - lastList = false case c == '#' && !hasCur: // A comment starts only at a word boundary (POSIX): `url/#frag` is // part of the token, a bare `#` runs to the end of the line. @@ -148,12 +150,22 @@ func tokenize(line string) ([]segment, error) { lastList = false i += 3 case c == '<' && strings.HasPrefix(line[i:], "<<"): - // A heredoc redirection (`<<`, `<<-`). - flushToken() + // A heredoc redirection (`<<`, `<<-`) — but only when a delimiter + // word follows. `$((1<<20))` is an arithmetic shift, and taking it + // for a heredoc would swallow every later line as body text and + // silently unguard them. hd, next, err := readHeredocDelim(line, i+2) if err != nil { return nil, err } + if !hd.quoted && !isDelimStart(hd.delim) { + cur = append(cur, '<', '<') + hasCur = true + lastList = false + i += 2 + continue + } + flushToken() pending = append(pending, hd) i = next case c == '&' || c == '|' || c == ';' || c == '(' || c == ')': @@ -183,6 +195,20 @@ func tokenize(line string) ([]segment, error) { type heredoc struct { delim string stripTabs bool + // quoted records that the delimiter word carried quotes, which makes it a + // delimiter beyond doubt however exotic it looks (`<<'---'`). + quoted bool +} + +// isDelimStart reports whether a word looks like a here-document delimiter: an +// unquoted one starts with a letter or an underscore, which is what separates +// `cat <= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') } // readHeredocDelim reads the delimiter word after a `<<` at pos, honouring the @@ -210,6 +236,7 @@ func readHeredocDelim(line string, pos int) (heredoc, int, error) { return heredoc{}, 0, fmt.Errorf("%w: unterminated quote in heredoc delimiter", ErrUnparsableCommand) } w = append(w, line[pos+1:j]...) + hd.quoted = true pos = j + 1 continue case '\\': diff --git a/internal/core/guard/tokenize_test.go b/internal/core/guard/tokenize_test.go index 97e60fe..79b02cf 100644 --- a/internal/core/guard/tokenize_test.go +++ b/internal/core/guard/tokenize_test.go @@ -134,6 +134,26 @@ func TestTokenizeSegments(t *testing.T) { line: "cat < Date: Mon, 27 Jul 2026 21:06:42 +0100 Subject: [PATCH 12/17] feat: wire the shell-hazard guard into the CLI, the host hook, and ahoy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit itd-103 / spc-16 delivered a registry with no way to reach it. This makes it executable: two front doors onto the one core decision, a hook entry that runs it before a shell command executes, and a health line so a guard that is not running is visible from outside the session. `abcd guard check --command ""` renders the decision — allow, warn, or block with the plain-language why and the safe successor — and exits 0 / 0 / 1. `abcd guard hook` reads a pre-tool-use payload from stdin and maps the same decision onto the host's protocol: the blocking status with the message on stderr for a block, exit 0 for a warn or an allow. The two front doors part company on exactly one case, deliberately. A guard that cannot be EVALUATED — an unparsable command line, a registry that will not load — is a fault for `check` (exit 2, so a script never reads silence as clearance) and fail-OPEN for `hook` (exit 0 with a loud stderr warning, so a broken guard is never the reason a session stops). The installed PreToolUse entry wraps the binary call in a shim that passes only the binary's own allow and block statuses through; every other outcome — binary absent, non-executable, crashed, killed — allows the command and emits an UNGUARDED warning. That shim is what makes fail-open-loud true for the failure Go cannot cover: the binary not running at all. It is tested by driving the committed manifest string under a shell against a fake plugin root. `abcd ahoy` gains a guard line and three diagnostic gaps reporting the three things that can independently be false — hook installed, binary reachable, registry loadable — plus a deliberately disabled registry, which is not a fault but must not read as protection either. The hook manifest decoder is factored out of verifyHookManifest so the prompt-router check and the guard-health check cannot disagree about what a well-formed manifest is. Assisted-by: Claude:claude-opus-5[1m] --- .abcd/development/release/surface.json | 23 +++ docs/reference/cli/commands.md | 50 +++++ hooks/hooks.json | 3 + internal/core/ahoy/ahoy.go | 6 +- internal/core/ahoy/detect.go | 5 + internal/core/ahoy/guard_health.go | 147 +++++++++++++ internal/core/ahoy/guard_health_test.go | 168 +++++++++++++++ internal/core/ahoy/store.go | 40 +++- internal/surface/cli/cli.go | 4 + internal/surface/cli/guard.go | 262 ++++++++++++++++++++++++ internal/surface/cli/guard_hook_test.go | 134 ++++++++++++ internal/surface/cli/guard_shim_test.go | 147 +++++++++++++ internal/surface/cli/guard_verb_test.go | 173 ++++++++++++++++ 13 files changed, 1154 insertions(+), 8 deletions(-) create mode 100644 internal/core/ahoy/guard_health.go create mode 100644 internal/core/ahoy/guard_health_test.go create mode 100644 internal/surface/cli/guard.go create mode 100644 internal/surface/cli/guard_hook_test.go create mode 100644 internal/surface/cli/guard_shim_test.go create mode 100644 internal/surface/cli/guard_verb_test.go diff --git a/.abcd/development/release/surface.json b/.abcd/development/release/surface.json index e40c0b2..c54a67f 100644 --- a/.abcd/development/release/surface.json +++ b/.abcd/development/release/surface.json @@ -343,6 +343,29 @@ "hidden": false, "flags": [] }, + { + "path": "abcd guard", + "hidden": false, + "flags": [] + }, + { + "path": "abcd guard check", + "hidden": false, + "flags": [ + { + "name": "command", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd guard hook", + "hidden": false, + "flags": [] + }, { "path": "abcd history", "hidden": false, diff --git a/docs/reference/cli/commands.md b/docs/reference/cli/commands.md index 4309010..4ba04d1 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -254,6 +254,56 @@ Report what a lifeboat would write into a target, read-only (coverage blanks fir **Usage:** `abcd embark probe [target-dir]` +### `abcd guard` + +Check a shell command against the hazard registry before it runs + +**Usage:** `abcd guard` + +#### `abcd guard check` + +Decide whether a candidate shell command is safe to run + +**Usage:** `abcd guard check [flags]` + +Evaluates one candidate command line against the hazard registry — the +bundled defaults merged with this repo's `.abcd/guard.json` — and reports +allow, warn, or block. A blocker exits 1 and names the safe successor; a +warn exits 0 with the warning rendered; an allow exits 0. A guard that +cannot be evaluated at all (an unparsable command line, a malformed +registry) exits 2, so a caller never reads silence as clearance. + +Matching is shell-token-aware and applies in command position only, so a +hazard named inside a quoted argument never fires. Command strings passed +to `eval` or `sh -c` are not parsed: a hazard hidden inside one is not +seen. That is a known limit of this version, not a claim of coverage. + +The candidate comes from --command, or from stdin when the flag is absent. + +**Flags:** + +``` + --command string the candidate command line (default: read from stdin) +``` + +#### `abcd guard hook` + +Host pre-tool-use adapter: decide a shell command from a hook payload + +**Usage:** `abcd guard hook` + +Reads a host pre-tool-use hook payload on stdin and evaluates its shell +command against the hazard registry. A blocker exits with the host's +blocking status and puts the safe successor and the plain-language why on +stderr, which is the channel the host replays to the agent. A warn and an +allow both let the command run. + +Anything the adapter cannot turn into a decision — an unreadable payload, a +tool call that is not a shell command, an unparsable command line, a +registry that will not load — allows the command and warns loudly on +stderr. A guard that cannot answer never stops a session, and is never +silently absent. + ### `abcd history` Manage the native session-transcript store diff --git a/hooks/hooks.json b/hooks/hooks.json index 6f03182..fd09283 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -9,6 +9,9 @@ {"type": "command", "command": "\"$CLAUDE_PLUGIN_ROOT/abcd\" hook session-start"} ]} ], + "PreToolUse": [ + {"matcher": "Bash", "hooks": [{"type": "command", "command": "\"$CLAUDE_PLUGIN_ROOT/abcd\" guard hook; s=$?; [ $s -eq 0 ] || [ $s -eq 2 ] || { echo \"abcd guard: FAILED TO RUN (exit $s) — shell commands run UNGUARDED in this session; run 'abcd ahoy' to see guard health\" >&2; exit 1; }; exit $s"}]} + ], "PreCompact": [ {"hooks": [{"type": "command", "command": "\"$CLAUDE_PLUGIN_ROOT/abcd\" hook prompt-router-reset"}]} ], diff --git a/internal/core/ahoy/ahoy.go b/internal/core/ahoy/ahoy.go index 0018047..ea85696 100644 --- a/internal/core/ahoy/ahoy.go +++ b/internal/core/ahoy/ahoy.go @@ -70,7 +70,11 @@ type DetectionResult struct { PluginRootStatus string `json:"plugin_root_status"` // "resolved" | "missing" RepoIdentity RepoIdentity `json:"repo_identity"` Signals map[string]any `json:"signals"` - Gaps []Gap `json:"gaps"` + // Guard is the execution-time shell guard's health. It is reported on every + // pass, healthy or not: a guard that fails open looks exactly like a working + // one from inside a session, so the state has to be legible from outside. + Guard GuardHealth `json:"guard"` + Gaps []Gap `json:"gaps"` // pluginRoot is the resolved plugin root; not serialized. pluginRoot string diff --git a/internal/core/ahoy/detect.go b/internal/core/ahoy/detect.go index e0fdaed..bb53c4c 100644 --- a/internal/core/ahoy/detect.go +++ b/internal/core/ahoy/detect.go @@ -72,6 +72,11 @@ func Detect(cwd string) (DetectionResult, error) { gaps = append(gaps, detectPathSymlink(pluginRoot, pluginOK)...) gaps = append(gaps, detectHookManifest(pluginRoot, pluginOK)...) gaps = append(gaps, detectVersion(abs)...) + // Guard health is computed for every managed or adoptable repo, so a + // broken guard is visible on the status board and not only from inside a + // session that has already stopped being protected (itd-103 AC 1). + res.Guard = detectGuardHealth(abs, pluginRoot, pluginOK) + gaps = append(gaps, detectGuardGaps(res.Guard)...) } sortGaps(gaps) diff --git a/internal/core/ahoy/guard_health.go b/internal/core/ahoy/guard_health.go new file mode 100644 index 0000000..1dc87cd --- /dev/null +++ b/internal/core/ahoy/guard_health.go @@ -0,0 +1,147 @@ +package ahoy + +import ( + "os" + "strings" + + "github.com/REPPL/abcd-cli/internal/core/guard" +) + +// guardHookCommand is the substring the PreToolUse manifest entry must contain +// for the execution-time guard to be armed. It mirrors requiredHookCommand's +// shape, but is checked separately and reported separately: a missing guard hook +// is a guard-health fact, not evidence that the plugin install is broken. +const guardHookCommand = "guard hook" + +// GuardHealth is the answer to "is the execution-time guard actually on?", split +// into the three things that can independently be false. It exists because a +// guard that fails open is indistinguishable from a guard that is working, from +// inside the session — so the state has to be visible from outside it too +// (itd-103 AC 1, spc-16 "Fail-open-loud and health"). +type GuardHealth struct { + // HookInstalled reports whether the plugin's hook manifest declares the + // pre-tool-use entry that runs the guard. + HookInstalled bool `json:"hook_installed"` + // BinaryReachable reports whether the binary that entry invokes exists and + // is executable. When it is not, the shim fails open on every command. + BinaryReachable bool `json:"binary_reachable"` + // RegistryLoadable reports whether the bundled defaults merged with this + // repo's .abcd/guard.json parse and validate. When they do not, the guard + // declines to answer and every command runs unchecked. + RegistryLoadable bool `json:"registry_loadable"` + // Disabled reports a deliberately switched-off registry. It is not a fault — + // the committed kill switch is the only sanctioned escape — but a disabled + // guard that looks armed is exactly the state this report exists to prevent. + Disabled bool `json:"disabled"` + // Entries is how many hazards the loaded registry holds, so "loadable" is + // backed by a number rather than a boolean nobody can check. + Entries int `json:"entries"` + // Detail is a one-line human reason when something is false; empty when the + // guard is healthy. + Detail string `json:"detail,omitempty"` +} + +// Healthy reports whether commands in a session are actually being checked: the +// hook runs, the binary it calls exists, and the registry loads. +func (g GuardHealth) Healthy() bool { + return g.HookInstalled && g.BinaryReachable && g.RegistryLoadable +} + +// detectGuardHealth answers the three questions for one repo. It never returns an +// error: an unanswerable question is a false with a reason, which is the whole +// point — a health check that could itself fail silently would report nothing +// where it matters most. +func detectGuardHealth(cwd, pluginRoot string, pluginOK bool) GuardHealth { + var h GuardHealth + var reasons []string + if !pluginOK { + reasons = append(reasons, "plugin root not resolvable, so the hook manifest cannot be read") + } else { + h.HookInstalled = manifestArmsGuard(pluginRoot) + h.BinaryReachable = isExecutableFile(pluginBinaryPath(pluginRoot)) + if !h.HookInstalled { + reasons = append(reasons, "hooks/hooks.json declares no PreToolUse entry running `abcd guard hook`") + } + if !h.BinaryReachable { + reasons = append(reasons, "the plugin-root abcd binary is missing or not executable, so the hook shim fails open on every command") + } + } + + if reg, err := guard.Load(cwd); err != nil { + // The raw error can name a per-repo path, and the action a human takes is + // the same whatever the parse failure was: `abcd guard check` prints it. + reasons = append(reasons, guardRegistryUnloadableReason) + } else { + h.RegistryLoadable = true + h.Disabled = reg.Disabled + h.Entries = len(reg.Entries) + } + + h.Detail = strings.Join(reasons, "; ") + return h +} + +// guardRegistryUnloadableReason is the one reason string for a registry that will +// not load, shared by the health detail and the gap so a reader is never told two +// different things about one fault. +const guardRegistryUnloadableReason = guard.RepoRelPath + " does not load, so the guard declines to answer and commands run unchecked" + +// detectGuardGaps turns an unhealthy guard into gaps, so a broken guard shows up +// on the same status board as every other discrepancy rather than only in a line +// a reader might skip. They are diagnostic: ahoy does not write a repo's guard +// config, and a missing hook entry means a broken or stale plugin install. +func detectGuardGaps(h GuardHealth) []Gap { + var gaps []Gap + if !h.HookInstalled { + gaps = append(gaps, Gap{ + ID: "guard.hook_missing", Category: PluginOwned, Scope: "machine", + Title: "shell-hazard guard not armed", + Detail: "No PreToolUse hook runs `abcd guard hook`, so dangerous shell commands are not checked before they execute.", + FixHint: "Update or reinstall the abcd plugin.", Required: false, Resolvable: false, + }) + } + if h.HookInstalled && !h.BinaryReachable { + gaps = append(gaps, Gap{ + ID: "guard.binary_unreachable", Category: PluginOwned, Scope: "machine", + Title: "guard hook installed but its binary is unreachable", + Detail: "The guard hook is wired but the abcd binary it calls is missing or not executable, so it fails open on every command.", + FixHint: "Reinstall the abcd plugin, or run `abcd ahoy install`.", Required: false, Resolvable: false, + }) + } + if !h.RegistryLoadable { + gaps = append(gaps, Gap{ + ID: "guard.registry_unloadable", Category: ConfigChange, Scope: "repo", + Title: "hazard registry does not load", + Detail: guardRegistryUnloadableReason, + FixHint: "Fix or remove " + guard.RepoRelPath + "; `abcd guard check --command ls` names the parse error.", + Required: false, Resolvable: false, + }) + } + return gaps +} + +// manifestArmsGuard reports whether the plugin's hook manifest declares a +// PreToolUse entry running the guard. It reuses the same guarded read and the +// same command-substring test as the prompt-router manifest check, so the two +// cannot disagree about what "installed" means. +func manifestArmsGuard(pluginRoot string) bool { + hooks, ok := readHookEvents(pluginRoot) + if !ok { + return false + } + entries, ok := hooks["PreToolUse"].([]any) + if !ok { + return false + } + return eventHasCommand(entries, guardHookCommand) +} + +// isExecutableFile reports whether path is a regular file with an execute bit — +// what the hook shim needs in order to run at all. +func isExecutableFile(path string) bool { + fi, err := os.Stat(path) + if err != nil || !fi.Mode().IsRegular() { + return false + } + return fi.Mode().Perm()&0o111 != 0 +} diff --git a/internal/core/ahoy/guard_health_test.go b/internal/core/ahoy/guard_health_test.go new file mode 100644 index 0000000..8f5e16c --- /dev/null +++ b/internal/core/ahoy/guard_health_test.go @@ -0,0 +1,168 @@ +package ahoy + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// hooksJSONWithGuard is a manifest that also arms the execution-time guard. +const hooksJSONWithGuard = `{ + "hooks": { + "UserPromptSubmit": [{"hooks": [{"type": "command", "command": "\"$CLAUDE_PLUGIN_ROOT/abcd\" hook prompt-router"}]}], + "SessionStart": [{"hooks": [{"type": "command", "command": "\"$CLAUDE_PLUGIN_ROOT/abcd\" hook prompt-router-reset"}]}], + "PreCompact": [{"hooks": [{"type": "command", "command": "\"$CLAUDE_PLUGIN_ROOT/abcd\" hook prompt-router-reset"}]}], + "PreToolUse": [{"matcher": "Bash", "hooks": [{"type": "command", "command": "\"$CLAUDE_PLUGIN_ROOT/abcd\" guard hook"}]}] + } +}` + +// managedRepoAt turns dir into a folder ahoy classifies as a managed repo, so the +// full detection pass runs over it. +func managedRepoAt(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(dir, ".git"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(dir, ".abcd"), 0o755); err != nil { + t.Fatal(err) + } + body := append(append(append([]byte(nil), markerBegin...), []byte("\nx\n")...), markerEnd...) + if err := os.WriteFile(filepath.Join(dir, "CLAUDE.md"), append(body, '\n'), 0o644); err != nil { + t.Fatal(err) + } +} + +// TestGuardHealthArmed is the good state: the manifest carries the PreToolUse +// entry, the plugin-root binary is present, and the registry loads. Everything a +// facilitator needs to believe the guard is actually on. +func TestGuardHealthArmed(t *testing.T) { + _, pluginRoot := setupHermetic(t) + if err := os.WriteFile(filepath.Join(pluginRoot, "hooks", "hooks.json"), []byte(hooksJSONWithGuard), 0o644); err != nil { + t.Fatal(err) + } + dir := t.TempDir() + managedRepoAt(t, dir) + + det, err := Detect(dir) + if err != nil { + t.Fatal(err) + } + g := det.Guard + if !g.HookInstalled || !g.BinaryReachable || !g.RegistryLoadable { + t.Errorf("a fully armed guard must report healthy; got %+v", g) + } + if !g.Healthy() { + t.Errorf("Healthy() must agree with the three checks; got %+v", g) + } + for _, gap := range det.Gaps { + if strings.HasPrefix(gap.ID, "guard.") { + t.Errorf("a healthy guard must emit no guard gap; got %q", gap.ID) + } + } +} + +// TestGuardHealthHookNotInstalled is AC 1's visibility half: a manifest without +// the PreToolUse entry means commands run unguarded, and ahoy must say so rather +// than report a clean bill of health. +func TestGuardHealthHookNotInstalled(t *testing.T) { + setupHermetic(t) // validHooksJSON: no PreToolUse entry + dir := t.TempDir() + managedRepoAt(t, dir) + + det, err := Detect(dir) + if err != nil { + t.Fatal(err) + } + if det.Guard.HookInstalled { + t.Error("a manifest with no PreToolUse guard entry must report hook_installed=false") + } + if det.Guard.Healthy() { + t.Error("a guard with no hook is not healthy") + } + if !hasGap(det.Gaps, "guard.hook_missing") { + t.Errorf("a missing guard hook must surface as a gap; gaps = %v", gapIDs(det.Gaps)) + } +} + +// TestGuardHealthBinaryUnreachable covers the fail-open case ahoy exists to make +// visible: the hook is installed but the binary it calls is gone, so every +// session runs unguarded behind a shim warning. +func TestGuardHealthBinaryUnreachable(t *testing.T) { + _, pluginRoot := setupHermetic(t) + if err := os.WriteFile(filepath.Join(pluginRoot, "hooks", "hooks.json"), []byte(hooksJSONWithGuard), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Remove(filepath.Join(pluginRoot, "abcd")); err != nil { + t.Fatal(err) + } + dir := t.TempDir() + managedRepoAt(t, dir) + + det, err := Detect(dir) + if err != nil { + t.Fatal(err) + } + if det.Guard.BinaryReachable { + t.Error("an absent plugin-root binary must report binary_reachable=false") + } + if !hasGap(det.Gaps, "guard.binary_unreachable") { + t.Errorf("an unreachable guard binary must surface as a gap; gaps = %v", gapIDs(det.Gaps)) + } +} + +// TestGuardHealthRegistryUnloadable is the third failure mode: the hook runs, the +// binary runs, and the registry the repo committed does not parse — so the guard +// refuses to answer and the session is unguarded. Never silent. +func TestGuardHealthRegistryUnloadable(t *testing.T) { + _, pluginRoot := setupHermetic(t) + if err := os.WriteFile(filepath.Join(pluginRoot, "hooks", "hooks.json"), []byte(hooksJSONWithGuard), 0o644); err != nil { + t.Fatal(err) + } + dir := t.TempDir() + managedRepoAt(t, dir) + if err := os.WriteFile(filepath.Join(dir, ".abcd", "guard.json"), []byte("{not json"), 0o644); err != nil { + t.Fatal(err) + } + + det, err := Detect(dir) + if err != nil { + t.Fatal(err) + } + if det.Guard.RegistryLoadable { + t.Error("a malformed .abcd/guard.json must report registry_loadable=false") + } + if det.Guard.Detail == "" { + t.Error("an unloadable registry must carry a reason a human can act on") + } + if !hasGap(det.Gaps, "guard.registry_unloadable") { + t.Errorf("an unloadable registry must surface as a gap; gaps = %v", gapIDs(det.Gaps)) + } +} + +// TestGuardHealthDisabledIsReported is the committed escape hatch: a repo that +// deliberately turned the guard off is not broken, but the fact must still be on +// the status board — a disabled guard that looks armed is the failure mode. +func TestGuardHealthDisabledIsReported(t *testing.T) { + _, pluginRoot := setupHermetic(t) + if err := os.WriteFile(filepath.Join(pluginRoot, "hooks", "hooks.json"), []byte(hooksJSONWithGuard), 0o644); err != nil { + t.Fatal(err) + } + dir := t.TempDir() + managedRepoAt(t, dir) + cfg := `{"schema_version":1,"disabled":true,"entries":{}}` + if err := os.WriteFile(filepath.Join(dir, ".abcd", "guard.json"), []byte(cfg), 0o644); err != nil { + t.Fatal(err) + } + + det, err := Detect(dir) + if err != nil { + t.Fatal(err) + } + if !det.Guard.RegistryLoadable { + t.Error("a deliberately disabled registry still loads; it is not a fault") + } + if !det.Guard.Disabled { + t.Error("a committed kill switch must be reported by ahoy, not hidden") + } +} diff --git a/internal/core/ahoy/store.go b/internal/core/ahoy/store.go index 449290c..bdd139e 100644 --- a/internal/core/ahoy/store.go +++ b/internal/core/ahoy/store.go @@ -537,13 +537,9 @@ func verifyHookManifest(pluginRoot string) string { return "read failed" } } - var parsed map[string]any - if err := json.Unmarshal(data, &parsed); err != nil { - return "JSON parse failed" - } - hooks, ok := parsed["hooks"].(map[string]any) - if !ok { - return "missing or non-object `hooks` key" + hooks, reason := decodeHookEvents(data) + if reason != "" { + return reason } for _, event := range []string{"UserPromptSubmit", "SessionStart", "PreCompact"} { entries, ok := hooks[event].([]any) @@ -557,6 +553,36 @@ func verifyHookManifest(pluginRoot string) string { return "" } +// decodeHookEvents decodes a hook manifest's `hooks` object, returning the +// per-event entries or a one-line reason. It is the single decoder for the +// manifest: verifyHookManifest checks the prompt-router events with it, and the +// guard-health check reads the PreToolUse event with it, so the two can never +// disagree about what a well-formed manifest is. +func decodeHookEvents(data []byte) (map[string]any, string) { + var parsed map[string]any + if err := json.Unmarshal(data, &parsed); err != nil { + return nil, "JSON parse failed" + } + hooks, ok := parsed["hooks"].(map[string]any) + if !ok { + return nil, "missing or non-object `hooks` key" + } + return hooks, "" +} + +// readHookEvents reads and decodes the plugin's hook manifest, returning the +// per-event entries. A manifest that cannot be read or decoded yields ok=false — +// the caller reports "not installed", which is the honest answer when nothing can +// be proven about the wiring. +func readHookEvents(pluginRoot string) (map[string]any, bool) { + data, err := fsutil.ReadGuarded(filepath.Join(pluginRoot, "hooks", "hooks.json"), 256*1024) + if err != nil { + return nil, false + } + hooks, reason := decodeHookEvents(data) + return hooks, reason == "" +} + // eventHasCommand reports whether any nested command string contains substring. func eventHasCommand(entries []any, substring string) bool { for _, entry := range entries { diff --git a/internal/surface/cli/cli.go b/internal/surface/cli/cli.go index 85d4f37..e891dbb 100644 --- a/internal/surface/cli/cli.go +++ b/internal/surface/cli/cli.go @@ -93,6 +93,7 @@ func NewRootCommand() *cobra.Command { root.AddCommand(newAhoyCommand(&asJSON)) root.AddCommand(newAuditCommand(&asJSON)) + root.AddCommand(newGuardCommand(&asJSON)) var launchDryRun bool launchCmd := &cobra.Command{ @@ -1435,6 +1436,9 @@ func newAhoyCommand(asJSON *bool) *cobra.Command { fmt.Fprintf(w, " install: %s\n", mode) } fmt.Fprintf(w, " gaps: %d\n", len(res.Gaps)) + if res.FolderKind != ahoy.UnmanagedFolder { + fmt.Fprintf(w, " guard: %s\n", guardHealthLine(res.Guard)) + } // Classification is read-only; the human report names the // next step per folder kind (itd-40 AC2/AC3). switch res.FolderKind { diff --git a/internal/surface/cli/guard.go b/internal/surface/cli/guard.go new file mode 100644 index 0000000..1b548fe --- /dev/null +++ b/internal/surface/cli/guard.go @@ -0,0 +1,262 @@ +package cli + +import ( + "encoding/json" + "fmt" + "io" + "os" + "strings" + + "github.com/REPPL/abcd-cli/internal/core/ahoy" + "github.com/REPPL/abcd-cli/internal/core/guard" + "github.com/REPPL/abcd-cli/internal/termsafe" + "github.com/spf13/cobra" +) + +// maxGuardStdinBytes caps the candidate command read from stdin. A command line +// is short; anything larger is not one, and an unbounded read on a hook's stdin +// would let a pathological payload stall the session it is guarding. +const maxGuardStdinBytes = 1 << 20 // 1 MiB + +// newGuardCommand builds the `guard` sub-tree: the two front doors onto +// internal/core/guard's one decision. `check` is the human/scriptable verb; +// `hook` is the host adapter that a PreToolUse-style hook runs before a shell +// command executes. +// +// Both are thin: core decides, this file only formats the decision and maps it +// onto an exit status. The two differ in exactly one respect, and it is the whole +// point of the split — `check` treats a guard it cannot evaluate as a FAULT +// (loud, non-zero, so a script never mistakes silence for clearance), while +// `hook` treats the same condition as fail-OPEN (loud, exit 0, so a broken guard +// can never brick a session). spc-16, "Fail-open-loud and health". +func newGuardCommand(asJSON *bool) *cobra.Command { + guardCmd := &cobra.Command{ + Use: "guard", + Short: "Check a shell command against the hazard registry before it runs", + Args: cobra.NoArgs, + } + + var command string + checkCmd := &cobra.Command{ + Use: "check", + Short: "Decide whether a candidate shell command is safe to run", + Long: "Evaluates one candidate command line against the hazard registry — the\n" + + "bundled defaults merged with this repo's `.abcd/guard.json` — and reports\n" + + "allow, warn, or block. A blocker exits 1 and names the safe successor; a\n" + + "warn exits 0 with the warning rendered; an allow exits 0. A guard that\n" + + "cannot be evaluated at all (an unparsable command line, a malformed\n" + + "registry) exits 2, so a caller never reads silence as clearance.\n\n" + + "Matching is shell-token-aware and applies in command position only, so a\n" + + "hazard named inside a quoted argument never fires. Command strings passed\n" + + "to `eval` or `sh -c` are not parsed: a hazard hidden inside one is not\n" + + "seen. That is a known limit of this version, not a claim of coverage.\n\n" + + "The candidate comes from --command, or from stdin when the flag is absent.", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + candidate, err := guardCandidate(cmd, command) + if err != nil { + return &exitError{Code: 2, Msg: fmt.Sprintf("guard check: %s", scrubPaths(err))} + } + reg, err := loadGuardRegistry() + if err != nil { + return &exitError{Code: 2, Msg: fmt.Sprintf("guard check: %s", scrubPaths(err))} + } + dec, err := reg.Check(candidate) + if err != nil { + return &exitError{Code: 2, Msg: fmt.Sprintf("guard check: %s", scrubPaths(err))} + } + if rerr := render(cmd.OutOrStdout(), *asJSON, dec, func(w io.Writer) { + writeGuardDecision(w, dec) + }); rerr != nil { + return rerr + } + // A refusal is an EXPECTED outcome, not a fault: the render is the + // output and the exit code the only extra signal (exit 1), matching + // how `embark from` and `intent ready` report a refusal. + if dec.Verdict == guard.VerdictBlock { + return &exitError{Code: 1} + } + return nil + }, + } + checkCmd.Flags().StringVar(&command, "command", "", "the candidate command line (default: read from stdin)") + guardCmd.AddCommand(checkCmd) + + guardCmd.AddCommand(newGuardHookCommand()) + return guardCmd +} + +// newGuardHookCommand builds `guard hook`: the adapter between a host's +// pre-tool-use hook payload and the core decision. +// +// The mapping is the host's own convention for a pre-execution hook: exit 2 is +// the blocking status and the hook's stderr is what the host replays to the +// agent, so the refusal — successor and why — goes to stderr and nowhere else. A +// warn and an allow both exit 0, which is what lets the command proceed; the +// warning still goes to stderr so it is on the record. +// +// Every path that is NOT a decision — an unreadable payload, a tool that is not +// the shell, a command the tokenizer cannot split, a registry that will not +// load — allows the command and says so unmissably. This is the fail-open-loud +// contract: a guard that cannot answer must never be the reason a session stops, +// and must never be silently absent either (itd-103 AC 1). +func newGuardHookCommand() *cobra.Command { + return &cobra.Command{ + Use: "hook", + Short: "Host pre-tool-use adapter: decide a shell command from a hook payload", + Long: "Reads a host pre-tool-use hook payload on stdin and evaluates its shell\n" + + "command against the hazard registry. A blocker exits with the host's\n" + + "blocking status and puts the safe successor and the plain-language why on\n" + + "stderr, which is the channel the host replays to the agent. A warn and an\n" + + "allow both let the command run.\n\n" + + "Anything the adapter cannot turn into a decision — an unreadable payload, a\n" + + "tool call that is not a shell command, an unparsable command line, a\n" + + "registry that will not load — allows the command and warns loudly on\n" + + "stderr. A guard that cannot answer never stops a session, and is never\n" + + "silently absent.", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + // failOpen is the single exit for every non-decision path, so the + // contract cannot be violated by forgetting one of them. + failOpen := func(format string, a ...any) error { + fmt.Fprintf(cmd.ErrOrStderr(), + "abcd guard: NOT CHECKED — "+format+". This command runs UNGUARDED.\n", a...) + return nil + } + + raw, err := io.ReadAll(io.LimitReader(cmd.InOrStdin(), maxHookStdinBytes)) + if err != nil { + return failOpen("the hook payload could not be read (%v)", err) + } + var in guardHookInput + if err := json.Unmarshal(raw, &in); err != nil { + return failOpen("the hook payload is not readable JSON (%v)", err) + } + // The manifest scopes this hook to the shell tool, so a different tool + // name means the wiring is wrong — worth saying rather than ignoring. + if !strings.EqualFold(in.ToolName, "Bash") { + return failOpen("the payload is a %q tool call, not a shell command", termsafe.Sanitize(in.ToolName)) + } + candidate := in.ToolInput.Command + if strings.TrimSpace(candidate) == "" { + return failOpen("the payload carries no command to check") + } + + // The payload names the session's working directory; fall back to the + // process cwd only when it does not, exactly as the other hooks do. + cwd := in.Cwd + if cwd == "" { + if wd, err := os.Getwd(); err == nil { + cwd = wd + } + } + reg, err := guard.Load(rulesRoot(cwd)) + if err != nil { + return failOpen("the hazard registry did not load (%s)", scrubPaths(err)) + } + dec, err := reg.Check(candidate) + if err != nil { + return failOpen("the command line could not be parsed (%s)", scrubPaths(err)) + } + + switch dec.Verdict { + case guard.VerdictBlock: + // Exit 2 is the host's blocking status; stderr is the message it + // replays to the agent, so the refusal itself is the lesson. + fmt.Fprintln(cmd.ErrOrStderr(), termsafe.Sanitize(dec.Message)) + return &exitError{Code: 2} + case guard.VerdictWarn: + fmt.Fprintln(cmd.ErrOrStderr(), termsafe.Sanitize(dec.Message)) + return nil + default: + return nil // allow: silent, and cheap + } + }, + } +} + +// guardHookInput is the subset of the host's pre-tool-use payload the adapter +// reads. Unknown fields are ignored: the host owns this schema and adds to it. +type guardHookInput struct { + Cwd string `json:"cwd"` + ToolName string `json:"tool_name"` + ToolInput struct { + Command string `json:"command"` + } `json:"tool_input"` +} + +// guardCandidate resolves the command to check: the flag when given, otherwise +// stdin. An empty candidate from either channel is a usage error, never an +// implicit allow — "nothing to check" and "checked and clear" must not look the +// same to a caller. +func guardCandidate(cmd *cobra.Command, flag string) (string, error) { + if strings.TrimSpace(flag) != "" { + return flag, nil + } + if flag != "" { + return "", fmt.Errorf("--command is empty; pass a command line to check") + } + raw, err := io.ReadAll(io.LimitReader(cmd.InOrStdin(), maxGuardStdinBytes)) + if err != nil { + return "", fmt.Errorf("reading the candidate command from stdin failed: %w", err) + } + candidate := strings.TrimRight(string(raw), "\r\n") + if strings.TrimSpace(candidate) == "" { + return "", fmt.Errorf("no command to check: pass --command \"\" or pipe one on stdin") + } + return candidate, nil +} + +// loadGuardRegistry resolves the repo root the same way the modular-rules loader +// does — the nearest ancestor holding a .abcd directory — so `.abcd/guard.json` +// is honoured from any nested working directory, kill switch included. +func loadGuardRegistry() (guard.Registry, error) { + cwd, err := os.Getwd() + if err != nil { + return guard.Registry{}, err + } + return guard.Load(rulesRoot(cwd)) +} + +// guardHealthLine renders ahoy's one-line guard-health verdict. A guard that +// cannot check anything fails OPEN inside the session, which is invisible there +// by design — so this line is the place a broken guard becomes visible, and it +// says which of the three parts is missing rather than a bare "unhealthy". +func guardHealthLine(h ahoy.GuardHealth) string { + if h.Healthy() { + state := fmt.Sprintf("armed (%d hazards)", h.Entries) + if h.Disabled { + // Loadable and wired, but switched off by a committed override. Not a + // fault, and not something to report as protection either. + state = "OFF — disabled in " + guard.RepoRelPath + } + return state + } + var missing []string + if !h.HookInstalled { + missing = append(missing, "hook not installed") + } + if !h.BinaryReachable { + missing = append(missing, "binary unreachable") + } + if !h.RegistryLoadable { + missing = append(missing, "registry does not load") + } + return "NOT ARMED — " + strings.Join(missing, ", ") + " (shell commands run unchecked)" +} + +// writeGuardDecision renders one decision as the human report. Entry text is +// registry-supplied (a per-repo file can add entries), so it is sanitised before +// it reaches a terminal. +func writeGuardDecision(w io.Writer, dec guard.Decision) { + fmt.Fprintf(w, "abcd guard — %s\n", dec.Verdict) + if dec.Verdict == guard.VerdictAllow { + return + } + fmt.Fprintf(w, " entry: %s (%s)\n", termsafe.Sanitize(dec.EntryID), termsafe.Sanitize(dec.Tier)) + fmt.Fprintf(w, " why: %s\n", termsafe.Sanitize(dec.Why)) + fmt.Fprintf(w, " run instead: %s\n", termsafe.Sanitize(dec.Successor)) + if len(dec.Matches) > 1 { + fmt.Fprintf(w, " also matched: %s\n", termsafe.Sanitize(strings.Join(dec.Matches[1:], ", "))) + } +} diff --git a/internal/surface/cli/guard_hook_test.go b/internal/surface/cli/guard_hook_test.go new file mode 100644 index 0000000..e25b6e2 --- /dev/null +++ b/internal/surface/cli/guard_hook_test.go @@ -0,0 +1,134 @@ +package cli + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// preToolUse builds the host's PreToolUse payload for a Bash tool call. The +// adapter reads exactly two things out of it — the tool name and +// tool_input.command — so the fixture spells both out rather than hiding them +// behind a helper's defaults. +func preToolUse(t *testing.T, tool, command, cwd string) string { + t.Helper() + payload := map[string]any{ + "session_id": "s1", + "cwd": cwd, + "hook_event_name": "PreToolUse", + "tool_name": tool, + "tool_input": map[string]any{"command": command, "description": "x"}, + } + b, err := json.Marshal(payload) + if err != nil { + t.Fatal(err) + } + return string(b) +} + +// TestGuardHookBlocksWithHostExitCode is AC 2 on the guard plane: a blocker match +// exits 2 — the host's blocking status — and puts the successor and the why on +// stderr, which is the channel the host feeds back to the agent. The block is the +// lesson, so both must be present. +func TestGuardHookBlocksWithHostExitCode(t *testing.T) { + dir := guardRepo(t) + _, stderr, code := runGuard(preToolUse(t, "Bash", "cd scratch && rm -rf *", dir), "guard", "hook") + + if code != 2 { + t.Errorf("a blocker must map to the host's blocking exit status 2; got %d (stderr %q)", code, stderr) + } + if !strings.Contains(stderr, "absolute path") || !strings.Contains(stderr, "the delete still runs") { + t.Errorf("the block message must carry the successor and the why; stderr = %q", stderr) + } +} + +// TestGuardHookAllowsSilently is the common case, and the one that must stay +// cheap and quiet: an allowed command produces no output at all. +func TestGuardHookAllowsSilently(t *testing.T) { + dir := guardRepo(t) + stdout, stderr, code := runGuard(preToolUse(t, "Bash", "git status --porcelain", dir), "guard", "hook") + + if code != 0 { + t.Errorf("an allow must exit 0; got %d (stderr %q)", code, stderr) + } + if stdout != "" || stderr != "" { + t.Errorf("an allow must be silent; stdout=%q stderr=%q", stdout, stderr) + } +} + +// TestGuardHookWarnAllowsAndSurfaces is AC 2's warn half on the guard plane: the +// command runs (exit 0, never the blocking status) and the warning is still said +// out loud. +func TestGuardHookWarnAllowsAndSurfaces(t *testing.T) { + dir := guardRepo(t) + _, stderr, code := runGuard(preToolUse(t, "Bash", "git clean -fd", dir), "guard", "hook") + + if code != 0 { + t.Errorf("a warn must never block: want exit 0, got %d", code) + } + if !strings.Contains(stderr, "git-clean") { + t.Errorf("the warning must be surfaced and name its entry; stderr = %q", stderr) + } +} + +// TestGuardHookFailsOpenLoud is AC 1 at the adapter: every input the adapter +// cannot turn into a decision allows the command AND says so unmissably. A silent +// allow would leave a session unguarded with nobody told; a block would brick it. +func TestGuardHookFailsOpenLoud(t *testing.T) { + cases := []struct { + name string + stdin func(t *testing.T, dir string) string + }{ + {"unparsable JSON", func(t *testing.T, _ string) string { return "{not json" }}, + {"empty payload", func(t *testing.T, _ string) string { return "" }}, + {"non-Bash tool call", func(t *testing.T, dir string) string { + return preToolUse(t, "Write", "cd scratch && rm -rf *", dir) + }}, + {"Bash call with no command", func(t *testing.T, dir string) string { + return preToolUse(t, "Bash", "", dir) + }}, + {"command the tokenizer cannot split", func(t *testing.T, dir string) string { + return preToolUse(t, "Bash", `rm -rf "unterminated`, dir) + }}, + {"malformed per-repo registry", func(t *testing.T, dir string) string { + if err := os.WriteFile(filepath.Join(dir, ".abcd", "guard.json"), []byte("{not json"), 0o644); err != nil { + t.Fatal(err) + } + return preToolUse(t, "Bash", "cd scratch && rm -rf *", dir) + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := guardRepo(t) + _, stderr, code := runGuard(tc.stdin(t, dir), "guard", "hook") + + if code != 0 { + t.Errorf("must fail OPEN (exit 0), got %d", code) + } + if !strings.Contains(stderr, "abcd guard") { + t.Errorf("must fail LOUD: stderr must carry an abcd guard warning; stderr = %q", stderr) + } + }) + } +} + +// TestGuardHookHonoursTheCommittedKillSwitch is AC 2's escape hatch: the only way +// out is a committed, reviewable per-repo config, and when it is set the guard +// allows without pretending to be broken. +func TestGuardHookHonoursTheCommittedKillSwitch(t *testing.T) { + dir := guardRepo(t) + cfg := `{"schema_version":1,"disabled":true,"entries":{}}` + if err := os.WriteFile(filepath.Join(dir, ".abcd", "guard.json"), []byte(cfg), 0o644); err != nil { + t.Fatal(err) + } + stdout, stderr, code := runGuard(preToolUse(t, "Bash", "cd scratch && rm -rf *", dir), "guard", "hook") + + if code != 0 { + t.Errorf("a committed kill switch must allow; got exit %d (stderr %q)", code, stderr) + } + if stdout != "" || stderr != "" { + t.Errorf("a deliberate disable is not a fault and must stay quiet; stdout=%q stderr=%q", stdout, stderr) + } +} diff --git a/internal/surface/cli/guard_shim_test.go b/internal/surface/cli/guard_shim_test.go new file mode 100644 index 0000000..18b9c7b --- /dev/null +++ b/internal/surface/cli/guard_shim_test.go @@ -0,0 +1,147 @@ +package cli + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// preToolUseGuardCommand reads the committed hook manifest and returns the shell +// command the host runs for the guard's PreToolUse entry, plus the matcher it is +// scoped to. The test drives the REAL installed string: a shim that fails open in +// theory and not in the manifest would be worth nothing. +func preToolUseGuardCommand(t *testing.T) (matcher, command string) { + t.Helper() + data, err := os.ReadFile(filepath.Join("..", "..", "..", "hooks", "hooks.json")) + if err != nil { + t.Fatalf("cannot read hooks/hooks.json: %v", err) + } + var doc struct { + Hooks map[string][]struct { + Matcher string `json:"matcher"` + Hooks []struct { + Type string `json:"type"` + Command string `json:"command"` + } `json:"hooks"` + } `json:"hooks"` + } + if err := json.Unmarshal(data, &doc); err != nil { + t.Fatalf("hooks/hooks.json is not valid JSON: %v", err) + } + for _, group := range doc.Hooks["PreToolUse"] { + for _, h := range group.Hooks { + if strings.Contains(h.Command, "guard hook") { + return group.Matcher, h.Command + } + } + } + t.Fatal("hooks/hooks.json declares no PreToolUse entry running `guard hook`; the guard plane is not installed") + return "", "" +} + +// fakePluginRoot builds a plugin root holding an `abcd` executable that exits +// with the given status, so the shim can be driven through every outcome the real +// binary can produce — including not existing at all (script == ""). +func fakePluginRoot(t *testing.T, script string) string { + t.Helper() + root := t.TempDir() + if script == "" { + return root + } + bin := filepath.Join(root, "abcd") + if err := os.WriteFile(bin, []byte("#!/bin/sh\n"+script+"\n"), 0o755); err != nil { + t.Fatal(err) + } + return bin[:len(bin)-len("/abcd")] +} + +// runShim executes the manifest's command string the way the host does: a POSIX +// shell, with CLAUDE_PLUGIN_ROOT pointing at the plugin root. +func runShim(t *testing.T, command, pluginRoot string) (stderr string, code int) { + t.Helper() + cmd := exec.Command("/bin/sh", "-c", command) + cmd.Env = append(os.Environ(), "CLAUDE_PLUGIN_ROOT="+pluginRoot) + cmd.Stdin = strings.NewReader(`{"tool_name":"Bash","tool_input":{"command":"ls"}}`) + var se strings.Builder + cmd.Stderr = &se + cmd.Stdout = &strings.Builder{} + err := cmd.Run() + if err == nil { + return se.String(), 0 + } + var ee *exec.ExitError + if ok := asExitError(err, &ee); ok { + return se.String(), ee.ExitCode() + } + t.Fatalf("running the shim failed structurally: %v", err) + return "", 0 +} + +func asExitError(err error, out **exec.ExitError) bool { + e, ok := err.(*exec.ExitError) + if ok { + *out = e + } + return ok +} + +// TestGuardHookIsInstalledForBashCalls holds the wiring itself: the guard is +// reachable from a live session or it does not exist. The entry must be scoped to +// the shell tool, so the guard is not asked about every unrelated tool call. +func TestGuardHookIsInstalledForBashCalls(t *testing.T) { + matcher, command := preToolUseGuardCommand(t) + if matcher != "Bash" { + t.Errorf("the guard entry must be scoped to the shell tool; matcher = %q", matcher) + } + if !strings.Contains(command, "CLAUDE_PLUGIN_ROOT") { + t.Errorf("the entry must invoke the plugin-root binary; command = %q", command) + } +} + +// TestGuardShimFailsOpenLoud is AC 1's other half — the one that cannot be +// satisfied in Go, because the failure being guarded against is the Go binary not +// running at all. A missing or broken binary must never produce the host's +// blocking status, and must never be silent about it. +func TestGuardShimFailsOpenLoud(t *testing.T) { + _, command := preToolUseGuardCommand(t) + cases := []struct { + name string + script string + }{ + {"binary absent", ""}, + {"binary not executable as a program", "exit 127"}, + {"binary crashes", "exit 3"}, + {"binary killed", "kill -TERM $$"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + stderr, code := runShim(t, command, fakePluginRoot(t, tc.script)) + if code == 2 { + t.Errorf("a broken guard must never block the session (exit 2); stderr = %q", stderr) + } + if !strings.Contains(stderr, "UNGUARDED") { + t.Errorf("a broken guard must be unmissable; stderr = %q", stderr) + } + }) + } +} + +// TestGuardShimPropagatesRealDecisions is the other side of the same contract: the +// shim must not swallow the decisions the binary DID make. A block stays a block +// and an allow stays an allow. +func TestGuardShimPropagatesRealDecisions(t *testing.T) { + _, command := preToolUseGuardCommand(t) + + if stderr, code := runShim(t, command, fakePluginRoot(t, `echo "blocked" >&2; exit 2`)); code != 2 { + t.Errorf("a real block must reach the host as exit 2; got %d (stderr %q)", code, stderr) + } + if stderr, code := runShim(t, command, fakePluginRoot(t, "exit 0")); code != 0 { + t.Errorf("a real allow must reach the host as exit 0; got %d (stderr %q)", code, stderr) + } + if stderr, _ := runShim(t, command, fakePluginRoot(t, "exit 0")); strings.Contains(stderr, "UNGUARDED") { + t.Errorf("a working guard must not cry wolf; stderr = %q", stderr) + } +} diff --git a/internal/surface/cli/guard_verb_test.go b/internal/surface/cli/guard_verb_test.go new file mode 100644 index 0000000..6a78c6c --- /dev/null +++ b/internal/surface/cli/guard_verb_test.go @@ -0,0 +1,173 @@ +package cli + +import ( + "bytes" + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// runGuard drives the guard verbs through the real exit-code mapping with stdin +// wired, and returns what a caller (a terminal or a host hook runner) would see: +// stdout, stderr, and the process exit code. The code is load-bearing on every +// path here — it is the whole protocol the hook speaks — so it is returned +// rather than asserted away. +func runGuard(stdin string, args ...string) (stdout, stderr string, code int) { + root := NewRootCommand() + root.SetArgs(args) + var so, se bytes.Buffer + root.SetOut(&so) + root.SetErr(&se) + root.SetIn(strings.NewReader(stdin)) + err := root.Execute() + if err == nil { + return so.String(), se.String(), 0 + } + code = 1 + var coded interface{ ExitCode() int } + if errors.As(err, &coded) { + code = coded.ExitCode() + } + // Mirror Run's error surface, so a test sees the diagnostic line a caller + // sees rather than an error value the process never prints. + if msg := scrubPaths(err); msg != "" { + se.WriteString("abcd: " + msg + "\n") + } + return so.String(), se.String(), code +} + +// guardRepo is a bare directory with a .abcd/ dir, so rulesRoot resolves there +// and the registry is the bundled default with no per-repo override. Chdir keeps +// the verb reading the fixture rather than the developer's own repo. +func guardRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".abcd"), 0o755); err != nil { + t.Fatal(err) + } + t.Chdir(dir) + return dir +} + +// TestGuardCheckBlocksHazard is AC 2's blocker half at the CLI front door: a +// command matching a blocker entry is refused, the refusal names the safe +// successor and the plain-language why, and the exit code says "refused" (1 — +// an expected outcome), never 0. +func TestGuardCheckBlocksHazard(t *testing.T) { + guardRepo(t) + stdout, _, code := runGuard("", "guard", "check", "--command", "cd scratch && rm -rf *") + + if code == 0 { + t.Errorf("a blocker match must exit non-zero; got 0 (stdout %q)", stdout) + } + if !strings.Contains(stdout, "rm-rf-after-cd-chain") { + t.Errorf("the refusal must name the entry; stdout = %q", stdout) + } + if !strings.Contains(stdout, "absolute path") { + t.Errorf("the refusal must carry the safe successor; stdout = %q", stdout) + } + if !strings.Contains(stdout, "the delete still runs") { + t.Errorf("the refusal must carry the plain-language why; stdout = %q", stdout) + } +} + +// TestGuardCheckAllowsIncidentCapture is AC 3 at the front door: this repo's own +// incident-capture command names a hazard inside a quoted argument and must run. +// It is the known-good fixture the whole matching design exists for. +func TestGuardCheckAllowsIncidentCapture(t *testing.T) { + guardRepo(t) + capture := `abcd capture "agent ran cd scratch && rm -rf * — one failed cd from disaster"` + stdout, stderr, code := runGuard("", "guard", "check", "--command", capture) + + if code != 0 { + t.Errorf("the incident-capture command must be allowed; exit %d (stdout %q stderr %q)", code, stdout, stderr) + } + if !strings.Contains(stdout, "allow") { + t.Errorf("an allow must say so; stdout = %q", stdout) + } +} + +// TestGuardCheckWarnPasses is AC 2's warn half: a warn-tier match passes (exit 0) +// with the warning rendered, so the command runs and the lesson still lands. +func TestGuardCheckWarnPasses(t *testing.T) { + guardRepo(t) + stdout, _, code := runGuard("", "guard", "check", "--command", "git reset --hard origin/main") + + if code != 0 { + t.Errorf("a warn match must exit 0; got %d (stdout %q)", code, stdout) + } + if !strings.Contains(stdout, "warn") || !strings.Contains(stdout, "git-reset-hard") { + t.Errorf("the warning must be rendered and name its entry; stdout = %q", stdout) + } +} + +// TestGuardCheckReadsStdin covers the second input channel: with no --command the +// candidate is read from stdin, so a caller can pipe a command line in. +func TestGuardCheckReadsStdin(t *testing.T) { + guardRepo(t) + stdout, _, code := runGuard("git push --force origin main\n", "guard", "check") + + if code == 0 { + t.Errorf("a blocker read from stdin must still refuse; exit 0 (stdout %q)", stdout) + } + if !strings.Contains(stdout, "git-push-force") { + t.Errorf("stdin candidate must be evaluated; stdout = %q", stdout) + } +} + +// TestGuardCheckJSON holds the machine surface: --json emits the core decision +// verbatim, so another front door reads the same fields the text render shows. +func TestGuardCheckJSON(t *testing.T) { + guardRepo(t) + stdout, _, code := runGuard("", "guard", "check", "--json", "--command", "cd scratch && rm -rf *") + + if code == 0 { + t.Errorf("--json must not change the exit code; got 0") + } + var got struct { + Verdict string `json:"verdict"` + EntryID string `json:"entry_id"` + Successor string `json:"successor"` + } + if err := json.Unmarshal([]byte(stdout), &got); err != nil { + t.Fatalf("--json must emit a JSON decision, got %q (%v)", stdout, err) + } + if got.Verdict != "block" || got.EntryID != "rm-rf-after-cd-chain" || got.Successor == "" { + t.Errorf("decision JSON is incomplete: %+v", got) + } +} + +// TestGuardCheckUnparsableCommandIsAFault separates "refused" from "broken": a +// command line the tokenizer cannot split is a structural fault (exit 2), never +// a silent allow that would leave the caller believing the guard had cleared it. +func TestGuardCheckUnparsableCommandIsAFault(t *testing.T) { + guardRepo(t) + _, stderr, code := runGuard("", "guard", "check", "--command", `rm -rf "unterminated`) + + if code != 2 { + t.Errorf("an unparsable command must be a fault (exit 2), got %d", code) + } + if !strings.Contains(stderr, "guard") { + t.Errorf("the fault must be named on stderr; stderr = %q", stderr) + } +} + +// TestGuardCheckMalformedRepoConfigIsAFault holds the same line for the config: +// a broken .abcd/guard.json must be loud, never a fallback to "no guard". +func TestGuardCheckMalformedRepoConfigIsAFault(t *testing.T) { + dir := guardRepo(t) + if err := os.WriteFile(filepath.Join(dir, ".abcd", "guard.json"), []byte("{not json"), 0o644); err != nil { + t.Fatal(err) + } + _, stderr, code := runGuard("", "guard", "check", "--command", "ls") + + if code != 2 { + t.Errorf("a malformed .abcd/guard.json must be a fault (exit 2), got %d", code) + } + if !strings.Contains(stderr, "guard") { + t.Errorf("the fault must be named on stderr; stderr = %q", stderr) + } +} From e8ba05305892f59d8cdaf0a6592fee7e15e62522 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:06:52 +0100 Subject: [PATCH 13/17] docs: document the shell-hazard guard surface Adds the plugin command doc teaching both sub-verbs, the brief's surface contract and registry row (the record-lint surface_coverage rule requires one), the package-map note that eval and sh -c payloads are a stated limit rather than an implicit gap, and the CHANGELOG entry for the user-facing addition. Assisted-by: Claude:claude-opus-5[1m] --- .../development/brief/04-surfaces/17-guard.md | 75 +++++++++++++++++++ .abcd/development/brief/04-surfaces/README.md | 1 + CHANGELOG.md | 18 +++++ commands/abcd/guard.md | 74 ++++++++++++++++++ internal/README.md | 7 +- 5 files changed, 173 insertions(+), 2 deletions(-) create mode 100644 .abcd/development/brief/04-surfaces/17-guard.md create mode 100644 commands/abcd/guard.md diff --git a/.abcd/development/brief/04-surfaces/17-guard.md b/.abcd/development/brief/04-surfaces/17-guard.md new file mode 100644 index 0000000..6c70c9c --- /dev/null +++ b/.abcd/development/brief/04-surfaces/17-guard.md @@ -0,0 +1,75 @@ +# `/abcd:guard` — Shell-Hazard Guard + +`/abcd:guard` decides whether a shell command is safe to run, against abcd's +hazard registry. It is **strictly read-only** — it performs zero writes, it only +answers. It is the execution-time half of itd-103; the teaching half is the rules +loader injecting the same registry's entries before shell-heavy work. + +## Sub-verbs + +- **`/abcd:guard check`** — evaluate one candidate command line and report the + decision. The candidate comes from `--command ""` or from stdin. The + plugin command (`commands/abcd/guard.md`) invokes + `abcd guard check --json --command ""` and reports the decision. +- **`abcd guard hook`** — the host adapter. It reads a pre-tool-use hook payload + on stdin, applies the same decision, and maps it onto the host's block/allow + protocol. It is invoked by `hooks/hooks.json`, not by hand. + +Bare `abcd guard` prints command usage — it does **not** render a status board; +guard health is reported by `abcd ahoy`, which is where install/health state +lives for every other part of abcd too. + +## The decision + +Core (`internal/core/guard`) returns one of three verdicts, and the front doors +only format it: + +| Verdict | `guard check` | `guard hook` | +|---|---|---| +| `allow` | exit 0 | exit 0, silent | +| `warn` | exit 0, warning rendered | exit 0, warning on stderr | +| `block` | exit 1, why + successor rendered | the host's blocking status, why + successor as the message | + +A guard that cannot be **evaluated** is a fourth case, and the two front doors +part company on it deliberately: + +- `guard check` exits **2**. A script that asked the guard a question must never + read silence as clearance. +- `guard hook` exits **0** and warns loudly. A guard that cannot answer must + never be the reason a session stops. + +## Fail-open-loud + +The installed hook entry wraps the binary call in a shim: if the abcd binary is +missing, not executable, crashes, or is killed, the shim allows the command and +emits an unmissable `UNGUARDED` warning. Only the binary's own exit statuses — +allow and block — propagate. A session is therefore never bricked by a broken +guard, and never silently unprotected either. + +The outside-the-session half is `abcd ahoy`'s `guard:` line, which reports the +three things that can independently be false: whether the hook is installed, +whether the binary it calls is reachable, and whether the registry loads. Each +also surfaces as a gap (`guard.hook_missing`, `guard.binary_unreachable`, +`guard.registry_unloadable`). + +## Registry and overrides + +The hazard entries are bundled in the binary and merged with a repo's committed +`.abcd/guard.json` — a dedicated file rather than a rules-loader domain, so a +rules kill switch can never silently disable a safety guard. An entry key +overrides one field or declares a new hazard; `{"disabled": true}` switches the +guard off. There is **no in-session override**: turning a guard off is a change +someone can review. + +## Known limit + +Command strings passed to `eval` or `sh -c` are not parsed, so a hazard hidden +inside one is not seen. It is stated in the `abcd guard check` reference doc and +in the package map, never left implicit. + +## References + +- Plugin command: [`commands/abcd/guard.md`](../../../../commands/abcd/guard.md) +- Spec: [`spc-16`](../../specs/open/spc-16-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md) +- Intent: [`itd-103`](../../intents/planned/itd-103-abcd-teaches-repo-agents-the-shell-commands-they-must-never.md) +- Install/health surface: [`01-ahoy.md`](01-ahoy.md) diff --git a/.abcd/development/brief/04-surfaces/README.md b/.abcd/development/brief/04-surfaces/README.md index 4d6f353..814196f 100644 --- a/.abcd/development/brief/04-surfaces/README.md +++ b/.abcd/development/brief/04-surfaces/README.md @@ -20,6 +20,7 @@ The brief's user-facing command surface is the set enumerated below (not all are | 14 | `/abcd:ingest` | shipped | Register a URL or document into the local sources corpus — host-delegated command, no Go verb | [`14-ingest.md`](14-ingest.md) | | 15 | `/abcd:prepare-this-repo` | shipped | Bring an owned repo up to abcd's conventions (interim bridge) — host-delegated command, no Go verb | [`15-prepare-this-repo.md`](15-prepare-this-repo.md) | | 16 | `/abcd:audit` | shipped | Check whether a repo conforms to the working conventions (three-tier layout, AGENTS.md router, durable decisions, docs currency, privacy hygiene) — read-only, tri-state exit; backs `prepare-this-repo` and gates CI (itd-85) | [`16-audit.md`](16-audit.md) | +| 17 | `/abcd:guard` | shipped | Shell-hazard guard (`check`) — decide one candidate command line against the bundled hazard registry merged with the repo's committed `.abcd/guard.json`, and answer allow / warn / block with the plain-language why and the safe successor. Read-only. The `hook` sub-verb is the host pre-tool-use adapter, live-wired from `hooks/hooks.json` behind a fail-open-loud shim; guard health (hook installed, binary reachable, registry loadable) is reported by `abcd ahoy` (itd-103, spc-16) | [`17-guard.md`](17-guard.md) | The **Status** column is machine-checked: the `surface_coverage` record-lint rule asserts every `shipped` row has a backing surface (`commands/abcd/.md` or `skills//`) and every `staged` row (a design target) has none — and, in reverse, that every real surface has a row here. The bare `/abcd` top-level is binary-backed (its command file is `commands/abcd.md`, not a `commands/abcd/.md` entry) and is exempt from the file check. Keeping this column honest is how the brief's surface set stays reconciled with the shipped binary; the semantic half — whether each row's *prose* matches binary behaviour — stays a release-gate agent check. diff --git a/CHANGELOG.md b/CHANGELOG.md index 4130a66..a1c9047 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,24 @@ called out in a **Breaking** section. ### Added +- **`abcd guard` — the shell-hazard guard, wired into a live session** (itd-103, + spc-16). `abcd guard check --command ""` evaluates one candidate command + against the hazard registry and reports allow, warn, or block: a blocker exits + 1 and answers with the plain-language why and the safe successor, a warn exits + 0 with the warning rendered, and a guard that cannot be evaluated at all exits + 2 rather than letting silence read as clearance. `abcd guard hook` is the host + adapter: it reads a pre-tool-use hook payload, refuses a matching command with + the successor as the block message, and lets everything else through. The + installed hook entry wraps the binary call so a missing or broken abcd **fails + open, loudly** — the command runs and the session carries an unmissable + UNGUARDED warning, never a silent no-op and never a stuck session. `abcd ahoy` + gains a `guard:` line reporting the three things that can independently be + false — hook installed, binary reachable, registry loadable — plus a + deliberately disabled registry, so a broken guard is visible from outside the + session too. Per-repo overrides stay committed and reviewable in + `.abcd/guard.json`; there is no in-session escape. Command strings passed to + `eval` or `sh -c` are not parsed — a stated limit of this version, carried in + the command reference. - **`abcd launch scaffold` — the changelog-driven release-gate scaffolder** (itd-93, spc-14). Writes the fixed release machinery into a managed repo that lacks it: `.github/workflows/release.yml` (verify → build → publish, the verify diff --git a/commands/abcd/guard.md b/commands/abcd/guard.md new file mode 100644 index 0000000..daa8c8f --- /dev/null +++ b/commands/abcd/guard.md @@ -0,0 +1,74 @@ +--- +name: guard +description: Check a shell command against abcd's hazard registry before it runs, by invoking the abcd binary. Read-only; performs zero writes. +argument-hint: "[check | hook]" +--- + +# `/abcd:guard` shell-hazard check + +Decide whether a shell command is safe to run, using abcd's hazard registry — +the bundled hazard entries merged with this repo's `.abcd/guard.json`. This +command performs **zero writes**. + +## `check` — decide one command + +Run: + +```bash +abcd guard check --json --command "" +``` + +The candidate may also be piped on stdin when no `--command` is given. + +Then report the JSON to the user: + +- `verdict` — `allow`, `warn`, or `block`. +- `entry_id`, `tier` — which hazard matched, and how severe it is. +- `why` — the plain-language reason, written for a non-expert. +- `successor` — the safe form to run instead. +- `matches` — every entry the command tripped, blockers first. + +Exit codes: `0` for allow and warn, `1` for a block, `2` when the guard could +not be evaluated at all (an unparsable command line, or a `.abcd/guard.json` +that does not load). Treat `2` as a fault to report, never as a clearance. + +On a `block`, do not run the command. Tell the user the `why`, then run the +`successor` instead — the refusal is the lesson, so pass it on in full. On a +`warn`, the command may run; surface the warning first so the user can stop it. + +## `hook` — the host adapter + +```bash +abcd guard hook +``` + +Reads a host pre-tool-use hook payload on stdin and applies the same decision +before a shell command executes. It is invoked by the plugin's hook manifest, +not by hand; a blocker returns the host's blocking status with the successor and +the why as the message, and a warn or an allow lets the command run. + +Anything the adapter cannot turn into a decision — an unreadable payload, a tool +call that is not a shell command, a registry that does not load — allows the +command and warns loudly. A guard that cannot answer never stops a session, and +is never silently absent. + +## Registry and overrides + +The bundled hazards ship inside the binary. A repo overrides them in a committed, +reviewable file, `.abcd/guard.json`: add an entry key to change one field (for +example `{"tier": "warn"}`) or to declare a new hazard, and set +`{"disabled": true}` to switch the guard off entirely. There is no in-session +override — turning a guard off is a change someone can review. + +Command strings passed to `eval` or `sh -c` are not parsed, so a hazard hidden +inside one is not seen. Say so if a user asks about coverage. + +To check whether the guard is actually armed in this repo, run `abcd ahoy` and +read its `guard:` line: it reports whether the hook is installed, whether the +binary is reachable, and whether the registry loads. + +If the `abcd` binary is not on `PATH`, fall back to +`go run ./cmd/abcd guard check --json --command ""` from the repo root, +or tell the user to build it with `make build`. + +**User input:** $ARGUMENTS diff --git a/internal/README.md b/internal/README.md index 221eb25..1f457c6 100644 --- a/internal/README.md +++ b/internal/README.md @@ -37,8 +37,11 @@ plugin surface, and a future MCP server share one engine. only, so a hazard named inside a quoted argument never fires; every bundled entry ships known-bad and known-good fixtures, and the admission gate test holds each one to its own corpus (100% true-negative floor, at least 40% - known-good). Fail-open-loud on a broken guard belongs to the hook shim, not - here. + known-good). Command strings passed to `eval` or `sh -c` are not parsed, so a + hazard hidden inside one is not seen — a stated limit of this version, carried + in the `abcd guard check` reference doc as well as here. Fail-open-loud on a + broken guard belongs to the hook shim (`hooks/hooks.json`) and the + `abcd guard hook` adapter, not here. - **`surface/cli/`** — the default front door: a Cobra command tree that calls `core` and formats results as text or `--json`. Holds no business logic. - **`surface/mcp/`** *(later)* — an additive front door exposing the same core From d4dd46c2e14d954324e1bee3bdd02181cf22867b Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:09:19 +0100 Subject: [PATCH 14/17] fix: answer an empty --command instead of waiting on stdin `abcd guard check --command ""` fell through to the stdin channel, because the channel was picked on the flag's VALUE rather than on whether it was given. In a terminal that is a silent hang; with something on stdin it answers a question nobody asked. The flag being present now decides the channel, and an empty value is the usage fault it is. Also records the three wiring decisions a future session would otherwise re-litigate: why `guard hook` sits under the user-facing verb rather than the hidden hook subtree, why the two front doors disagree on an unevaluatable guard, and why the fail-open shim is shell rather than Go. Assisted-by: Claude:claude-opus-5[1m] --- .abcd/work/DECISIONS.md | 3 +++ internal/surface/cli/guard.go | 12 ++++++++---- internal/surface/cli/guard_verb_test.go | 15 +++++++++++++++ 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 1fa81af..5a16e2f 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -863,3 +863,6 @@ parallel-agent merge contention bites. - 2026-07-27 — itd-93 parity tension resolved: Branch A. The scaffold template is the single source for release.yml/auto-release.yml; abcd-cli's live workflows are regenerated from it; the only sanctioned live diff is the additive workflow_dispatch rehearsal (trigger + non-publishing dry-run job). Rejected: substitution-gating the rehearsal off for abcd (weakens the parity guarantee on the one novel component). - 2026-07-27 — GRILLED itd-101/102/103/104 to settlement (six decisions, each now in its intent's Grill Settlements + acceptance criteria): itd-103 guard fails open loudly, blocker/warn tiers with committed-only overrides, shell-token-aware command-position matching with an itd-81-style TNR-floored corpus; itd-101 refresh is manual-with-nagging (scheduled CI later, own sign-off), staleness warns at 180d and blocks releases at 365d, human verifications age on the same clock; itd-102 identity lives in a parseable markdown block (config points at it), check is warn-tier and never rewrites; itd-104 ideate is an optional verb, never a pre-capture gate, with a fresh-context off-policy adversary. All four drafts are grill-settled and await intent plan. - 2026-07-27: itd-103 guard overrides live in dedicated committed .abcd/guard.json, not a rules.json domain — structured entry schema, and the rules kill switch must not silently disable a safety guard (spc-16). +- 2026-07-27: itd-103 guard wiring — `abcd guard hook` is a sub-verb of the USER-facing `guard` verb, not a fifth entrypoint under the hidden `hook` subtree. Reason: guard health has to be legible (AC 1), and a hidden adapter documents nothing; the other `hook` entrypoints are injection transport with no user-facing counterpart, this one is the same decision the user can also ask for by hand. Consequence: it appears in the generated CLI reference and must stay host-agnostic in its prose. +- 2026-07-27: itd-103 guard wiring — the two front doors disagree on ONE case by design. A guard that cannot be evaluated (unparsable command, registry that will not load) is a FAULT for `guard check` (exit 2: a script must never read silence as clearance) and fail-OPEN for `guard hook` (exit 0 + loud stderr: a broken guard must never stop a session). Rejected: one shared behaviour, which would either brick sessions or teach scripts that silence means safe. +- 2026-07-27: itd-103 guard wiring — the fail-open-loud shim is a shell wrapper in the plugin's `hooks/hooks.json` PreToolUse entry (pass through only the binary's own 0/2; everything else warns UNGUARDED and allows), not Go code. The failure it guards against is the Go binary not running at all, so it cannot live in Go. Tested by executing the committed manifest string under /bin/sh against a fake plugin root. diff --git a/internal/surface/cli/guard.go b/internal/surface/cli/guard.go index 1b548fe..4b206e5 100644 --- a/internal/surface/cli/guard.go +++ b/internal/surface/cli/guard.go @@ -190,12 +190,16 @@ type guardHookInput struct { // implicit allow — "nothing to check" and "checked and clear" must not look the // same to a caller. func guardCandidate(cmd *cobra.Command, flag string) (string, error) { - if strings.TrimSpace(flag) != "" { + // Whether the flag was GIVEN decides the channel, not whether its value is + // non-empty: `--command ""` is an empty question, and falling through to stdin + // there would leave the verb waiting on a terminal that is never going to + // answer. + if cmd.Flags().Changed("command") { + if strings.TrimSpace(flag) == "" { + return "", fmt.Errorf("--command is empty; pass a command line to check") + } return flag, nil } - if flag != "" { - return "", fmt.Errorf("--command is empty; pass a command line to check") - } raw, err := io.ReadAll(io.LimitReader(cmd.InOrStdin(), maxGuardStdinBytes)) if err != nil { return "", fmt.Errorf("reading the candidate command from stdin failed: %w", err) diff --git a/internal/surface/cli/guard_verb_test.go b/internal/surface/cli/guard_verb_test.go index 6a78c6c..cbdacbf 100644 --- a/internal/surface/cli/guard_verb_test.go +++ b/internal/surface/cli/guard_verb_test.go @@ -155,6 +155,21 @@ func TestGuardCheckUnparsableCommandIsAFault(t *testing.T) { } } +// TestGuardCheckEmptyFlagDoesNotFallThroughToStdin pins the channel choice: an +// explicitly empty --command is an empty question, answered immediately. Falling +// through to stdin would leave the verb waiting on a terminal that never answers. +func TestGuardCheckEmptyFlagDoesNotFallThroughToStdin(t *testing.T) { + guardRepo(t) + _, stderr, code := runGuard("cd scratch && rm -rf *\n", "guard", "check", "--command", "") + + if code != 2 { + t.Errorf("an empty --command is a usage fault (exit 2), got %d", code) + } + if !strings.Contains(stderr, "--command is empty") { + t.Errorf("the fault must say the flag was empty, not read stdin; stderr = %q", stderr) + } +} + // TestGuardCheckMalformedRepoConfigIsAFault holds the same line for the config: // a broken .abcd/guard.json must be loud, never a fallback to "no guard". func TestGuardCheckMalformedRepoConfigIsAFault(t *testing.T) { From 6698f69951f76b71e0ee195c370c643a635e3ebc Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:20:44 +0100 Subject: [PATCH 15/17] fix: stop the guard report asserting what it never checked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from review, all on the same theme — a safety report that is wrong or unheard is worse than none. **ahoy accused an install it never inspected.** With an unresolvable plugin root the manifest is never opened and the binary is never looked for, but HookInstalled and BinaryReachable kept their zero values, so the status line read `NOT ARMED — hook not installed, binary unreachable` while the JSON in the same run said `plugin root not resolvable`. Every dev build running from source hit it, including the fallback `commands/abcd/guard.md` itself prescribes. The state is now carried explicitly: no wiring gaps are emitted, and the line reports what is unknown. The registry half stays answerable — it is read from the repo, not the plugin. **The fail-open warning was unheard.** A pre-tool-use hook that exits 0 has its stderr discarded, so every non-decision path warned into a void — the "silently absent" state the contract says cannot happen. The adapter now exits 1: still non-blocking, but a status the host surfaces. The shim passes 1 through alongside 0 and 2, so a binary that ran and reported is no longer described as having failed to run. **The documented invocation could execute the command it was asked to check.** `abcd guard check --command ""` is expanded by the shell before the verb starts, so a candidate containing a command substitution runs at check time — the one moment the check exists to prevent — and an embedded quote breaks the quoting outright. The plugin command now passes the candidate on stdin in a quoted-delimiter heredoc; `--command` stays for literals a human typed, and the reference says why. Assisted-by: Claude:claude-opus-5[1m] --- .../development/brief/04-surfaces/17-guard.md | 7 +- commands/abcd/guard.md | 20 ++++-- docs/reference/cli/commands.md | 4 ++ hooks/hooks.json | 2 +- internal/core/ahoy/guard_health.go | 47 +++++++++---- internal/core/ahoy/guard_health_test.go | 33 +++++++++ internal/surface/cli/guard.go | 19 +++++- .../surface/cli/guard_health_line_test.go | 67 +++++++++++++++++++ internal/surface/cli/guard_hook_test.go | 12 +++- internal/surface/cli/guard_shim_test.go | 11 +++ 10 files changed, 197 insertions(+), 25 deletions(-) create mode 100644 internal/surface/cli/guard_health_line_test.go diff --git a/.abcd/development/brief/04-surfaces/17-guard.md b/.abcd/development/brief/04-surfaces/17-guard.md index 6c70c9c..52a40e4 100644 --- a/.abcd/development/brief/04-surfaces/17-guard.md +++ b/.abcd/development/brief/04-surfaces/17-guard.md @@ -9,8 +9,11 @@ loader injecting the same registry's entries before shell-heavy work. - **`/abcd:guard check`** — evaluate one candidate command line and report the decision. The candidate comes from `--command ""` or from stdin. The - plugin command (`commands/abcd/guard.md`) invokes - `abcd guard check --json --command ""` and reports the decision. + plugin command (`commands/abcd/guard.md`) passes it on **stdin**, inside a + quoted-delimiter heredoc: a candidate interpolated into a double-quoted + `--command` argument is expanded by the shell before the guard starts, so a + command substitution inside it would run at check time — the one moment the + check exists to prevent. `--command` stays for literals a human typed. - **`abcd guard hook`** — the host adapter. It reads a pre-tool-use hook payload on stdin, applies the same decision, and maps it onto the host's block/allow protocol. It is invoked by `hooks/hooks.json`, not by hand. diff --git a/commands/abcd/guard.md b/commands/abcd/guard.md index daa8c8f..696019f 100644 --- a/commands/abcd/guard.md +++ b/commands/abcd/guard.md @@ -12,13 +12,21 @@ command performs **zero writes**. ## `check` — decide one command -Run: +Pass the candidate on **stdin**, inside a quoted-delimiter heredoc: ```bash -abcd guard check --json --command "" +abcd guard check --json <<'ABCD_GUARD_EOF' + +ABCD_GUARD_EOF ``` -The candidate may also be piped on stdin when no `--command` is given. +Never interpolate the candidate into `--command "…"`. The shell expands a +double-quoted argument before `abcd` ever starts, so a candidate containing +`$(…)` or a backtick would **execute at check time** — the exact moment the check +exists to prevent — and an embedded `"` would break the quoting outright. The +quoted delimiter (`<<'ABCD_GUARD_EOF'`, quotes included) switches expansion off, +so the candidate reaches the guard as written. Use `--command` only for a literal +you typed yourself. Then report the JSON to the user: @@ -67,8 +75,8 @@ To check whether the guard is actually armed in this repo, run `abcd ahoy` and read its `guard:` line: it reports whether the hook is installed, whether the binary is reachable, and whether the registry loads. -If the `abcd` binary is not on `PATH`, fall back to -`go run ./cmd/abcd guard check --json --command ""` from the repo root, -or tell the user to build it with `make build`. +If the `abcd` binary is not on `PATH`, fall back to `go run ./cmd/abcd guard +check --json` from the repo root, with the same quoted-delimiter heredoc on +stdin, or tell the user to build it with `make build`. **User input:** $ARGUMENTS diff --git a/docs/reference/cli/commands.md b/docs/reference/cli/commands.md index 4ba04d1..8111e35 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -279,6 +279,10 @@ to `eval` or `sh -c` are not parsed: a hazard hidden inside one is not seen. That is a known limit of this version, not a claim of coverage. The candidate comes from --command, or from stdin when the flag is absent. +Prefer stdin for a command line you did not type yourself: the shell expands +a double-quoted --command argument before this verb starts, so a candidate +containing a command substitution would run at check time. A quoted-delimiter +heredoc (`abcd guard check <<'EOF'` ... `EOF`) passes it through untouched. **Flags:** diff --git a/hooks/hooks.json b/hooks/hooks.json index fd09283..9d256f1 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -10,7 +10,7 @@ ]} ], "PreToolUse": [ - {"matcher": "Bash", "hooks": [{"type": "command", "command": "\"$CLAUDE_PLUGIN_ROOT/abcd\" guard hook; s=$?; [ $s -eq 0 ] || [ $s -eq 2 ] || { echo \"abcd guard: FAILED TO RUN (exit $s) — shell commands run UNGUARDED in this session; run 'abcd ahoy' to see guard health\" >&2; exit 1; }; exit $s"}]} + {"matcher": "Bash", "hooks": [{"type": "command", "command": "\"$CLAUDE_PLUGIN_ROOT/abcd\" guard hook; s=$?; [ $s -eq 0 ] || [ $s -eq 1 ] || [ $s -eq 2 ] || { echo \"abcd guard: FAILED TO RUN (exit $s) — shell commands run UNGUARDED in this session; run 'abcd ahoy' to see guard health\" >&2; exit 1; }; exit $s"}]} ], "PreCompact": [ {"hooks": [{"type": "command", "command": "\"$CLAUDE_PLUGIN_ROOT/abcd\" hook prompt-router-reset"}]} diff --git a/internal/core/ahoy/guard_health.go b/internal/core/ahoy/guard_health.go index 1dc87cd..0b66f7d 100644 --- a/internal/core/ahoy/guard_health.go +++ b/internal/core/ahoy/guard_health.go @@ -19,11 +19,20 @@ const guardHookCommand = "guard hook" // inside the session — so the state has to be visible from outside it too // (itd-103 AC 1, spc-16 "Fail-open-loud and health"). type GuardHealth struct { + // PluginRootResolved reports whether the plugin root was found at all. It + // gates the two checks below: without it the manifest is never opened and the + // binary is never looked for, so HookInstalled and BinaryReachable carry no + // information. A report that folded "not checked" into "false" would accuse a + // plugin install that may be perfectly armed — the state every dev build that + // runs from source is in. + PluginRootResolved bool `json:"plugin_root_resolved"` // HookInstalled reports whether the plugin's hook manifest declares the - // pre-tool-use entry that runs the guard. + // pre-tool-use entry that runs the guard. Meaningful only when + // PluginRootResolved. HookInstalled bool `json:"hook_installed"` // BinaryReachable reports whether the binary that entry invokes exists and // is executable. When it is not, the shim fails open on every command. + // Meaningful only when PluginRootResolved. BinaryReachable bool `json:"binary_reachable"` // RegistryLoadable reports whether the bundled defaults merged with this // repo's .abcd/guard.json parse and validate. When they do not, the guard @@ -44,7 +53,7 @@ type GuardHealth struct { // Healthy reports whether commands in a session are actually being checked: the // hook runs, the binary it calls exists, and the registry loads. func (g GuardHealth) Healthy() bool { - return g.HookInstalled && g.BinaryReachable && g.RegistryLoadable + return g.PluginRootResolved && g.HookInstalled && g.BinaryReachable && g.RegistryLoadable } // detectGuardHealth answers the three questions for one repo. It never returns an @@ -54,8 +63,9 @@ func (g GuardHealth) Healthy() bool { func detectGuardHealth(cwd, pluginRoot string, pluginOK bool) GuardHealth { var h GuardHealth var reasons []string + h.PluginRootResolved = pluginOK if !pluginOK { - reasons = append(reasons, "plugin root not resolvable, so the hook manifest cannot be read") + reasons = append(reasons, "plugin root not resolvable, so the hook manifest cannot be read and the guard wiring is unknown") } else { h.HookInstalled = manifestArmsGuard(pluginRoot) h.BinaryReachable = isExecutableFile(pluginBinaryPath(pluginRoot)) @@ -92,6 +102,12 @@ const guardRegistryUnloadableReason = guard.RepoRelPath + " does not load, so th // config, and a missing hook entry means a broken or stale plugin install. func detectGuardGaps(h GuardHealth) []Gap { var gaps []Gap + // Without a plugin root neither wiring check ran, and a gap is an assertion. + // The sibling detections (detectPathSymlink, detectHookManifest) stay silent + // in this state for the same reason; plugin.root_missing already reports it. + if !h.PluginRootResolved { + return registryGap(h) + } if !h.HookInstalled { gaps = append(gaps, Gap{ ID: "guard.hook_missing", Category: PluginOwned, Scope: "machine", @@ -108,16 +124,23 @@ func detectGuardGaps(h GuardHealth) []Gap { FixHint: "Reinstall the abcd plugin, or run `abcd ahoy install`.", Required: false, Resolvable: false, }) } - if !h.RegistryLoadable { - gaps = append(gaps, Gap{ - ID: "guard.registry_unloadable", Category: ConfigChange, Scope: "repo", - Title: "hazard registry does not load", - Detail: guardRegistryUnloadableReason, - FixHint: "Fix or remove " + guard.RepoRelPath + "; `abcd guard check --command ls` names the parse error.", - Required: false, Resolvable: false, - }) + return append(gaps, registryGap(h)...) +} + +// registryGap is the repo-scoped half of the guard report. It is separate because +// it stays answerable whatever the plugin state is: the registry is read from the +// repo, not from the plugin. +func registryGap(h GuardHealth) []Gap { + if h.RegistryLoadable { + return nil } - return gaps + return []Gap{{ + ID: "guard.registry_unloadable", Category: ConfigChange, Scope: "repo", + Title: "hazard registry does not load", + Detail: guardRegistryUnloadableReason, + FixHint: "Fix or remove " + guard.RepoRelPath + "; `abcd guard check --command ls` names the parse error.", + Required: false, Resolvable: false, + }} } // manifestArmsGuard reports whether the plugin's hook manifest declares a diff --git a/internal/core/ahoy/guard_health_test.go b/internal/core/ahoy/guard_health_test.go index 8f5e16c..7fc3b63 100644 --- a/internal/core/ahoy/guard_health_test.go +++ b/internal/core/ahoy/guard_health_test.go @@ -140,6 +140,39 @@ func TestGuardHealthRegistryUnloadable(t *testing.T) { } } +// TestGuardHealthUnresolvablePluginRootAssertsNothing is the honesty rule: when +// the plugin root cannot be resolved, the manifest is never opened and the binary +// is never looked for, so ahoy knows NOTHING about the hook wiring. Reporting +// "hook not installed" there would accuse a plugin install that may be perfectly +// armed — the state a dev build (`go run ./cmd/abcd`) is in every time. +func TestGuardHealthUnresolvablePluginRootAssertsNothing(t *testing.T) { + setupHermetic(t) + t.Setenv("ABCD_PLUGIN_ROOT", t.TempDir()) // no hooks/ layout: not a plugin root + dir := t.TempDir() + managedRepoAt(t, dir) + + det, err := Detect(dir) + if err != nil { + t.Fatal(err) + } + if det.PluginRootStatus != "missing" { + t.Fatalf("fixture is wrong: plugin root resolved as %q", det.PluginRootStatus) + } + if det.Guard.PluginRootResolved { + t.Error("an unresolvable plugin root must be reported as such, not folded into the other checks") + } + if hasGap(det.Gaps, "guard.hook_missing") || hasGap(det.Gaps, "guard.binary_unreachable") { + t.Errorf("ahoy must not accuse a manifest it never opened; gaps = %v", gapIDs(det.Gaps)) + } + if !strings.Contains(det.Guard.Detail, "plugin root") { + t.Errorf("the reason must name what is actually unknown; detail = %q", det.Guard.Detail) + } + // The registry is a repo fact and stays answerable regardless of the plugin. + if !det.Guard.RegistryLoadable { + t.Error("the registry is readable without a plugin root and must still be reported") + } +} + // TestGuardHealthDisabledIsReported is the committed escape hatch: a repo that // deliberately turned the guard off is not broken, but the fact must still be on // the status board — a disabled guard that looks armed is the failure mode. diff --git a/internal/surface/cli/guard.go b/internal/surface/cli/guard.go index 4b206e5..b4c52cb 100644 --- a/internal/surface/cli/guard.go +++ b/internal/surface/cli/guard.go @@ -50,7 +50,11 @@ func newGuardCommand(asJSON *bool) *cobra.Command { "hazard named inside a quoted argument never fires. Command strings passed\n" + "to `eval` or `sh -c` are not parsed: a hazard hidden inside one is not\n" + "seen. That is a known limit of this version, not a claim of coverage.\n\n" + - "The candidate comes from --command, or from stdin when the flag is absent.", + "The candidate comes from --command, or from stdin when the flag is absent.\n" + + "Prefer stdin for a command line you did not type yourself: the shell expands\n" + + "a double-quoted --command argument before this verb starts, so a candidate\n" + + "containing a command substitution would run at check time. A quoted-delimiter\n" + + "heredoc (`abcd guard check <<'EOF'` ... `EOF`) passes it through untouched.", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { candidate, err := guardCandidate(cmd, command) @@ -118,10 +122,15 @@ func newGuardHookCommand() *cobra.Command { RunE: func(cmd *cobra.Command, _ []string) error { // failOpen is the single exit for every non-decision path, so the // contract cannot be violated by forgetting one of them. + // Exit 1, not 0, is what makes this LOUD. A pre-tool-use hook that + // exits 0 has its stderr discarded, so the warning would exist and + // nobody would ever see it; a non-zero, non-blocking status both lets + // the command run and puts the warning in front of a human. Only the + // blocking status (2) stops anything. failOpen := func(format string, a ...any) error { fmt.Fprintf(cmd.ErrOrStderr(), "abcd guard: NOT CHECKED — "+format+". This command runs UNGUARDED.\n", a...) - return nil + return &exitError{Code: 1} } raw, err := io.ReadAll(io.LimitReader(cmd.InOrStdin(), maxHookStdinBytes)) @@ -236,6 +245,12 @@ func guardHealthLine(h ahoy.GuardHealth) string { } return state } + // Without a plugin root the manifest was never opened and the binary was never + // looked for. Saying "hook not installed" here would accuse an install that + // may be perfectly armed, so the line reports what is unknown instead. + if !h.PluginRootResolved { + return "UNKNOWN — " + h.Detail + } var missing []string if !h.HookInstalled { missing = append(missing, "hook not installed") diff --git a/internal/surface/cli/guard_health_line_test.go b/internal/surface/cli/guard_health_line_test.go new file mode 100644 index 0000000..5835c21 --- /dev/null +++ b/internal/surface/cli/guard_health_line_test.go @@ -0,0 +1,67 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/REPPL/abcd-cli/internal/core/ahoy" +) + +// TestGuardHealthLineNeverAssertsWhatItCannotKnow pins the human half of the +// status board to the same honesty rule as the JSON half. When the plugin root +// is unresolvable, ahoy has not opened the manifest and has not looked for the +// binary, so the line must say what is unknown — not synthesise "hook not +// installed, binary unreachable" out of two booleans nobody ever set. +func TestGuardHealthLineNeverAssertsWhatItCannotKnow(t *testing.T) { + h := ahoy.GuardHealth{ + PluginRootResolved: false, + RegistryLoadable: true, + Entries: 6, + Detail: "plugin root not resolvable, so the hook manifest cannot be read", + } + line := guardHealthLine(h) + + if strings.Contains(line, "hook not installed") || strings.Contains(line, "binary unreachable") { + t.Errorf("the line asserts facts that were never checked: %q", line) + } + if !strings.Contains(line, "plugin root") { + t.Errorf("the line must carry the reason the state is unknown; got %q", line) + } +} + +// TestGuardHealthLineReportsEachRealFailure keeps the useful half: when the +// plugin root IS resolved, every check that actually ran and failed is named, so +// a reader knows which of the three parts to fix. +func TestGuardHealthLineReportsEachRealFailure(t *testing.T) { + line := guardHealthLine(ahoy.GuardHealth{ + PluginRootResolved: true, + HookInstalled: true, + BinaryReachable: false, + RegistryLoadable: true, + }) + if !strings.Contains(line, "binary unreachable") || strings.Contains(line, "hook not installed") { + t.Errorf("only the failing check must be named; got %q", line) + } + if !strings.Contains(line, "NOT ARMED") { + t.Errorf("an unarmed guard must say so; got %q", line) + } +} + +// TestGuardHealthLineArmedAndDisabled covers the two healthy states, which must +// read differently: a guard that is protecting the session, and one a committed +// override switched off. +func TestGuardHealthLineArmedAndDisabled(t *testing.T) { + armed := ahoy.GuardHealth{ + PluginRootResolved: true, HookInstalled: true, BinaryReachable: true, + RegistryLoadable: true, Entries: 6, + } + if line := guardHealthLine(armed); !strings.Contains(line, "armed") || !strings.Contains(line, "6") { + t.Errorf("an armed guard must say so and count its hazards; got %q", line) + } + + off := armed + off.Disabled = true + if line := guardHealthLine(off); !strings.Contains(line, "OFF") { + t.Errorf("a disabled guard must never read as protection; got %q", line) + } +} diff --git a/internal/surface/cli/guard_hook_test.go b/internal/surface/cli/guard_hook_test.go index e25b6e2..65f368c 100644 --- a/internal/surface/cli/guard_hook_test.go +++ b/internal/surface/cli/guard_hook_test.go @@ -76,6 +76,11 @@ func TestGuardHookWarnAllowsAndSurfaces(t *testing.T) { // TestGuardHookFailsOpenLoud is AC 1 at the adapter: every input the adapter // cannot turn into a decision allows the command AND says so unmissably. A silent // allow would leave a session unguarded with nobody told; a block would brick it. +// +// "Unmissable" is why the exit code is 1 and not 0. A pre-tool-use hook that +// exits 0 has its stderr discarded — the warning would exist and nobody would +// ever see it. A non-zero, non-blocking status is the one channel that both lets +// the command run and puts the warning in front of a human. func TestGuardHookFailsOpenLoud(t *testing.T) { cases := []struct { name string @@ -104,8 +109,11 @@ func TestGuardHookFailsOpenLoud(t *testing.T) { dir := guardRepo(t) _, stderr, code := runGuard(tc.stdin(t, dir), "guard", "hook") - if code != 0 { - t.Errorf("must fail OPEN (exit 0), got %d", code) + if code == 2 { + t.Errorf("must fail OPEN: the blocking status must never come from a non-decision") + } + if code == 0 { + t.Errorf("must fail LOUD: exit 0 discards the hook's stderr, so the warning would never be seen") } if !strings.Contains(stderr, "abcd guard") { t.Errorf("must fail LOUD: stderr must carry an abcd guard warning; stderr = %q", stderr) diff --git a/internal/surface/cli/guard_shim_test.go b/internal/surface/cli/guard_shim_test.go index 18b9c7b..e3a33a4 100644 --- a/internal/surface/cli/guard_shim_test.go +++ b/internal/surface/cli/guard_shim_test.go @@ -144,4 +144,15 @@ func TestGuardShimPropagatesRealDecisions(t *testing.T) { if stderr, _ := runShim(t, command, fakePluginRoot(t, "exit 0")); strings.Contains(stderr, "UNGUARDED") { t.Errorf("a working guard must not cry wolf; stderr = %q", stderr) } + // Exit 1 is the adapter's own fail-open-loud status: it RAN, it could not + // decide, and it allowed. The shim must pass that through untouched — adding + // its own "FAILED TO RUN" on top would tell the reader a lie about which part + // broke. + stderr, code := runShim(t, command, fakePluginRoot(t, `echo "abcd guard: NOT CHECKED" >&2; exit 1`)) + if code == 2 { + t.Errorf("the adapter's fail-open status must never become a block; got %d", code) + } + if strings.Contains(stderr, "FAILED TO RUN") { + t.Errorf("a binary that ran and reported must not be described as failing to run; stderr = %q", stderr) + } } From c6333d4773b2021b054743f1fc4fe39671626730 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:26:28 +0100 Subject: [PATCH 16/17] fix: close the two silent ways past the guard, and stop overclaiming coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security review, BLOCK verdict. Two ways to get an unearned clearance in silence, and one claim the code does not support. **A switched-off registry ran in silence.** `disabled: true` allows everything, and the adapter's allow branch returned nil — so the one unguarded state that needs no broken install, just a single file write the guard itself permits, was also the only one that said nothing. It now warns like every other unguarded state, naming the file that turned it off. Off can no longer pass for clear. **An oversized candidate was answered on its prefix.** `guard check` read the stdin cap and discarded the overflow undetected, so padding a command past 1 MiB returned `allow` on the part that fitted — via the stdin channel the plugin command now mandates. It reads one byte past the cap and refuses (exit 2): a candidate that does not fit is a candidate that was not checked. **"eval or sh -c" was written as if it were the whole gap.** It is not: a wrapper outside the small known set (`xargs`, `timeout`, `exec`), a backtick substitution, or simply a dangerous form no entry describes all pass too. The docs now say what an allow means — no entry matched, never "safe" — and enumerate the routes as examples rather than a boundary. The claim that disabling is "a committed, reviewable act" is likewise corrected rather than repeated: `guard.Load` reads the working tree, so an edit takes effect before review. What is true — the file is the only route, so the change lands in a diff, and the resulting state is loud — is what the docs now say. Enforcing the committed half is core-side and tracked as iss-147; the registry coverage gaps the review surfaced are iss-148. Assisted-by: Claude:claude-opus-5[1m] --- .../development/brief/04-surfaces/17-guard.md | 36 ++++++++++++++----- ...d-guard-json-from-the-working-tree-so-a.md | 11 ++++++ ...ge-gaps-found-while-wiring-itd-103-regi.md | 11 ++++++ CHANGELOG.md | 13 ++++--- commands/abcd/guard.md | 34 ++++++++++++++---- docs/reference/cli/commands.md | 11 ++++-- internal/README.md | 14 +++++--- internal/surface/cli/guard.go | 27 +++++++++++--- internal/surface/cli/guard_hook_test.go | 31 +++++++++++----- internal/surface/cli/guard_verb_test.go | 20 +++++++++++ 10 files changed, 167 insertions(+), 41 deletions(-) create mode 100644 .abcd/work/issues/open/iss-147-guard-load-reads-abcd-guard-json-from-the-working-tree-so-a.md create mode 100644 .abcd/work/issues/open/iss-148-guard-registry-coverage-gaps-found-while-wiring-itd-103-regi.md diff --git a/.abcd/development/brief/04-surfaces/17-guard.md b/.abcd/development/brief/04-surfaces/17-guard.md index 52a40e4..0061e9b 100644 --- a/.abcd/development/brief/04-surfaces/17-guard.md +++ b/.abcd/development/brief/04-surfaces/17-guard.md @@ -57,18 +57,36 @@ also surfaces as a gap (`guard.hook_missing`, `guard.binary_unreachable`, ## Registry and overrides -The hazard entries are bundled in the binary and merged with a repo's committed +The hazard entries are bundled in the binary and merged with a repo's `.abcd/guard.json` — a dedicated file rather than a rules-loader domain, so a rules kill switch can never silently disable a safety guard. An entry key overrides one field or declares a new hazard; `{"disabled": true}` switches the -guard off. There is **no in-session override**: turning a guard off is a change -someone can review. - -## Known limit - -Command strings passed to `eval` or `sh -c` are not parsed, so a hazard hidden -inside one is not seen. It is stated in the `abcd guard check` reference doc and -in the package map, never left implicit. +guard off. + +There is **no flag, environment variable, or prompt** that disarms the guard for +a session: the file is the only route, so the change lands in a diff. What the +file is *not*, today, is verified as committed — `guard.Load` reads the working +tree, so an edit takes effect on the next command, before review. The mitigation +is loudness rather than enforcement: a disabled registry makes every command it +lets through carry an UNGUARDED warning naming the file, and `abcd ahoy` reports +`OFF`. Closing the gap properly (refusing a `disabled: true` that is not in +`HEAD`) is a core-side change to `guard.Load`, tracked as an issue. + +## What an allow means + +An allow means **no registry entry matched** — never that a command is safe. The +guard reads command names it can see in command position, so a hazard reached any +other way is not seen: + +- a command string handed to an interpreter (`eval`, `sh -c`); +- one launched through a wrapper outside the small known set (`sudo`, `doas`, + `command`, `env`, `nohup`, `time`) — `xargs`, `timeout`, `exec` are not in it; +- one inside a backtick substitution (`$(…)` is followed, backticks are not); +- a dangerous form no entry describes. + +Coverage is what the registry names. The registry grows from reality: a +facilitator who sees something scary captures it, and recurring captures are +promoted into the bundled defaults through the admission gate. ## References diff --git a/.abcd/work/issues/open/iss-147-guard-load-reads-abcd-guard-json-from-the-working-tree-so-a.md b/.abcd/work/issues/open/iss-147-guard-load-reads-abcd-guard-json-from-the-working-tree-so-a.md new file mode 100644 index 0000000..127c40e --- /dev/null +++ b/.abcd/work/issues/open/iss-147-guard-load-reads-abcd-guard-json-from-the-working-tree-so-a.md @@ -0,0 +1,11 @@ +--- +schema_version: 1 +id: "iss-147" +slug: "guard-load-reads-abcd-guard-json-from-the-working-tree-so-a" +severity: "minor" +category: "observation" +source: "user-observation" +found_during: "manual-capture" +--- + +guard.Load reads .abcd/guard.json from the WORKING TREE, so a disabled:true (or a retiered blocker) takes effect on the very next command, before anyone reviews it — spc-16 and the shipped docs both say the only escape is a committed, reviewable override, and nothing enforces the committed half. Reachable in one move: an agent writes .abcd/guard.json and the guard itself allows that write. Mitigated at the front door in itd-103 wiring (a disabled registry now warns UNGUARDED on every command, and abcd ahoy reports OFF) but not enforced. Proper fix is core-side: refuse a disabled:true that is not in HEAD, or drop the committed claim from spc-16. Found by the security reviewer on the itd-103 wiring branch. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-148-guard-registry-coverage-gaps-found-while-wiring-itd-103-regi.md b/.abcd/work/issues/open/iss-148-guard-registry-coverage-gaps-found-while-wiring-itd-103-regi.md new file mode 100644 index 0000000..283ab63 --- /dev/null +++ b/.abcd/work/issues/open/iss-148-guard-registry-coverage-gaps-found-while-wiring-itd-103-regi.md @@ -0,0 +1,11 @@ +--- +schema_version: 1 +id: "iss-148" +slug: "guard-registry-coverage-gaps-found-while-wiring-itd-103-regi" +severity: "minor" +category: "observation" +source: "user-observation" +found_during: "manual-capture" +--- + +guard registry coverage gaps found while wiring itd-103 (registry content, not matching semantics): a push whose refspec carries a leading plus is a force in disguise and no entry describes it; xargs, timeout and exec are absent from the matcher's wrappers set, so a hazard launched through one of them is not seen; a backtick command substitution is not followed, while the dollar-paren form is. Candidates for the admission gate as the registry grows from reality. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index a1c9047..2a1aa01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,10 +26,15 @@ called out in a **Breaking** section. gains a `guard:` line reporting the three things that can independently be false — hook installed, binary reachable, registry loadable — plus a deliberately disabled registry, so a broken guard is visible from outside the - session too. Per-repo overrides stay committed and reviewable in - `.abcd/guard.json`; there is no in-session escape. Command strings passed to - `eval` or `sh -c` are not parsed — a stated limit of this version, carried in - the command reference. + session too. Every unguarded state is loud, including the deliberate one: a + registry switched off in `.abcd/guard.json` makes each command it lets through + carry the warning, so "off" can never pass for "clear". Per-repo overrides live + in that one file and nowhere else — no flag, environment variable, or prompt + disarms the guard for a session, so the change lands in a diff. An allow means + no entry matched, never that a command is safe: a hazard reached another way — + a string handed to an interpreter (`eval`, `sh -c`), a launcher outside the + small known set, a backtick substitution, or a form no entry describes — is not + seen. Coverage is what the registry names, and the command reference says so. - **`abcd launch scaffold` — the changelog-driven release-gate scaffolder** (itd-93, spc-14). Writes the fixed release machinery into a managed repo that lacks it: `.github/workflows/release.yml` (verify → build → publish, the verify diff --git a/commands/abcd/guard.md b/commands/abcd/guard.md index 696019f..89f762d 100644 --- a/commands/abcd/guard.md +++ b/commands/abcd/guard.md @@ -62,14 +62,34 @@ is never silently absent. ## Registry and overrides -The bundled hazards ship inside the binary. A repo overrides them in a committed, -reviewable file, `.abcd/guard.json`: add an entry key to change one field (for +The bundled hazards ship inside the binary. A repo overrides them in one file in +the repository, `.abcd/guard.json`: add an entry key to change one field (for example `{"tier": "warn"}`) or to declare a new hazard, and set -`{"disabled": true}` to switch the guard off entirely. There is no in-session -override — turning a guard off is a change someone can review. - -Command strings passed to `eval` or `sh -c` are not parsed, so a hazard hidden -inside one is not seen. Say so if a user asks about coverage. +`{"disabled": true}` to switch the guard off entirely. + +There is no flag, environment variable, or prompt that turns the guard off for a +session — the file is the only route, so the change lands in a diff someone +reviews. Two things follow, and both must be said plainly if a user asks. The +file is read from the working tree, so an edit takes effect on the very next +command, before anyone has reviewed it. And a repo whose guard is switched off is +an unguarded session: every command it lets through carries an UNGUARDED warning +naming the file, so the state cannot pass unnoticed. + +**Never write `.abcd/guard.json` on your own initiative.** Disabling or +retiering a hazard is the user's decision to make and to review. + +### What an allow does and does not mean + +An allow means **no registry entry matched**. It is never a statement that a +command is safe. The guard reads command names it can see in command position, so +a hazard reached any other way is not seen: a command string handed to an +interpreter (`eval`, `sh -c`), one launched through a wrapper outside the small +known set, one inside a backtick substitution, or a dangerous form no entry +describes. Coverage is what the registry names. Say exactly this if a user asks +about coverage — never that the guard cleared the command. + +A candidate too long to read is refused (exit 2), not answered on the part that +fitted. To check whether the guard is actually armed in this repo, run `abcd ahoy` and read its `guard:` line: it reports whether the hook is installed, whether the diff --git a/docs/reference/cli/commands.md b/docs/reference/cli/commands.md index 8111e35..5aa2253 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -274,9 +274,14 @@ cannot be evaluated at all (an unparsable command line, a malformed registry) exits 2, so a caller never reads silence as clearance. Matching is shell-token-aware and applies in command position only, so a -hazard named inside a quoted argument never fires. Command strings passed -to `eval` or `sh -c` are not parsed: a hazard hidden inside one is not -seen. That is a known limit of this version, not a claim of coverage. +hazard named inside a quoted argument never fires. + +An allow means no registry entry matched — it is never a statement that a +command is safe. The guard reads command names it can see in command +position, so a hazard reached any other way is not seen: a command string +handed to an interpreter (`eval`, `sh -c`), one launched through a wrapper +outside the small known set, one inside a backtick substitution, or a +dangerous form no entry describes. Coverage is what the registry names. The candidate comes from --command, or from stdin when the flag is absent. Prefer stdin for a command line you did not type yourself: the shell expands diff --git a/internal/README.md b/internal/README.md index 1f457c6..84c4f8f 100644 --- a/internal/README.md +++ b/internal/README.md @@ -37,11 +37,15 @@ plugin surface, and a future MCP server share one engine. only, so a hazard named inside a quoted argument never fires; every bundled entry ships known-bad and known-good fixtures, and the admission gate test holds each one to its own corpus (100% true-negative floor, at least 40% - known-good). Command strings passed to `eval` or `sh -c` are not parsed, so a - hazard hidden inside one is not seen — a stated limit of this version, carried - in the `abcd guard check` reference doc as well as here. Fail-open-loud on a - broken guard belongs to the hook shim (`hooks/hooks.json`) and the - `abcd guard hook` adapter, not here. + known-good). An allow means no entry matched, never that a command is safe: a + hazard reached another way — a string handed to an interpreter (`eval`, + `sh -c`), a launcher outside the small `wrappers` set, a backtick substitution, + or a form no entry describes — is not seen. That limit is stated here and in + the `abcd guard check` reference doc, never left implicit. `Load` reads the + working tree, so a `disabled: true` takes effect before it is reviewed; the + front door compensates by making a disabled registry loud rather than silent. + Fail-open-loud on a broken guard belongs to the hook shim (`hooks/hooks.json`) + and the `abcd guard hook` adapter, not here. - **`surface/cli/`** — the default front door: a Cobra command tree that calls `core` and formats results as text or `--json`. Holds no business logic. - **`surface/mcp/`** *(later)* — an additive front door exposing the same core diff --git a/internal/surface/cli/guard.go b/internal/surface/cli/guard.go index b4c52cb..e0e761b 100644 --- a/internal/surface/cli/guard.go +++ b/internal/surface/cli/guard.go @@ -47,9 +47,13 @@ func newGuardCommand(asJSON *bool) *cobra.Command { "cannot be evaluated at all (an unparsable command line, a malformed\n" + "registry) exits 2, so a caller never reads silence as clearance.\n\n" + "Matching is shell-token-aware and applies in command position only, so a\n" + - "hazard named inside a quoted argument never fires. Command strings passed\n" + - "to `eval` or `sh -c` are not parsed: a hazard hidden inside one is not\n" + - "seen. That is a known limit of this version, not a claim of coverage.\n\n" + + "hazard named inside a quoted argument never fires.\n\n" + + "An allow means no registry entry matched — it is never a statement that a\n" + + "command is safe. The guard reads command names it can see in command\n" + + "position, so a hazard reached any other way is not seen: a command string\n" + + "handed to an interpreter (`eval`, `sh -c`), one launched through a wrapper\n" + + "outside the small known set, one inside a backtick substitution, or a\n" + + "dangerous form no entry describes. Coverage is what the registry names.\n\n" + "The candidate comes from --command, or from stdin when the flag is absent.\n" + "Prefer stdin for a command line you did not type yourself: the shell expands\n" + "a double-quoted --command argument before this verb starts, so a candidate\n" + @@ -163,6 +167,14 @@ func newGuardHookCommand() *cobra.Command { if err != nil { return failOpen("the hazard registry did not load (%s)", scrubPaths(err)) } + // A disabled registry allows everything, which makes it an unguarded + // session — and it is the CHEAPEST one to reach: the other unguarded + // states need a broken install, this one needs a single file write. + // It is not a fault (someone chose it, and the choice sits in a file a + // reviewer can read), but it must never pass for protection. + if reg.Disabled { + return failOpen("the hazard registry is switched off in %s", guard.RepoRelPath) + } dec, err := reg.Check(candidate) if err != nil { return failOpen("the command line could not be parsed (%s)", scrubPaths(err)) @@ -209,10 +221,17 @@ func guardCandidate(cmd *cobra.Command, flag string) (string, error) { } return flag, nil } - raw, err := io.ReadAll(io.LimitReader(cmd.InOrStdin(), maxGuardStdinBytes)) + // Read ONE byte past the cap so an overflow is detectable. Truncating to the + // cap and answering on the prefix is the quietest possible way to hand out a + // clearance nobody earned: pad a command past the limit and the tail — the + // part that matters — is never tokenised. + raw, err := io.ReadAll(io.LimitReader(cmd.InOrStdin(), maxGuardStdinBytes+1)) if err != nil { return "", fmt.Errorf("reading the candidate command from stdin failed: %w", err) } + if len(raw) > maxGuardStdinBytes { + return "", fmt.Errorf("the candidate command is too long (over %d bytes); it was not checked", maxGuardStdinBytes) + } candidate := strings.TrimRight(string(raw), "\r\n") if strings.TrimSpace(candidate) == "" { return "", fmt.Errorf("no command to check: pass --command \"\" or pipe one on stdin") diff --git a/internal/surface/cli/guard_hook_test.go b/internal/surface/cli/guard_hook_test.go index 65f368c..1d1f507 100644 --- a/internal/surface/cli/guard_hook_test.go +++ b/internal/surface/cli/guard_hook_test.go @@ -6,6 +6,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/REPPL/abcd-cli/internal/core/guard" ) // preToolUse builds the host's PreToolUse payload for a Bash tool call. The @@ -122,21 +124,32 @@ func TestGuardHookFailsOpenLoud(t *testing.T) { } } -// TestGuardHookHonoursTheCommittedKillSwitch is AC 2's escape hatch: the only way -// out is a committed, reviewable per-repo config, and when it is set the guard -// allows without pretending to be broken. -func TestGuardHookHonoursTheCommittedKillSwitch(t *testing.T) { +// TestGuardHookAnnouncesADisabledRegistry is AC 1 applied to the escape hatch. +// A disabled registry allows everything, which makes it an unguarded session — +// and an unguarded session that says nothing is the one failure mode this whole +// feature exists to prevent. It is not a FAULT (someone chose it, and the choice +// is in a file a reviewer can see), but it must never pass for protection. +// +// It matters more than the other unguarded states, not less: those need a broken +// install, while this one needs a single file write that the guard itself allows. +func TestGuardHookAnnouncesADisabledRegistry(t *testing.T) { dir := guardRepo(t) cfg := `{"schema_version":1,"disabled":true,"entries":{}}` if err := os.WriteFile(filepath.Join(dir, ".abcd", "guard.json"), []byte(cfg), 0o644); err != nil { t.Fatal(err) } - stdout, stderr, code := runGuard(preToolUse(t, "Bash", "cd scratch && rm -rf *", dir), "guard", "hook") + _, stderr, code := runGuard(preToolUse(t, "Bash", "cd scratch && rm -rf *", dir), "guard", "hook") - if code != 0 { - t.Errorf("a committed kill switch must allow; got exit %d (stderr %q)", code, stderr) + if code == 2 { + t.Error("a disabled registry allows: it must never produce the blocking status") } - if stdout != "" || stderr != "" { - t.Errorf("a deliberate disable is not a fault and must stay quiet; stdout=%q stderr=%q", stdout, stderr) + if code == 0 { + t.Error("exit 0 discards the hook's stderr, so a disabled guard would run in silence") + } + if !strings.Contains(stderr, guard.RepoRelPath) { + t.Errorf("the warning must name the file that turned the guard off; stderr = %q", stderr) + } + if !strings.Contains(stderr, "UNGUARDED") { + t.Errorf("a disabled guard is an unguarded session and must say so; stderr = %q", stderr) } } diff --git a/internal/surface/cli/guard_verb_test.go b/internal/surface/cli/guard_verb_test.go index cbdacbf..96a6016 100644 --- a/internal/surface/cli/guard_verb_test.go +++ b/internal/surface/cli/guard_verb_test.go @@ -170,6 +170,26 @@ func TestGuardCheckEmptyFlagDoesNotFallThroughToStdin(t *testing.T) { } } +// TestGuardCheckOversizedCandidateIsAFault closes the quietest way to get a +// clearance you did not earn: pad a command past the stdin cap and the guard used +// to answer on the prefix it happened to read. A candidate that does not fit is +// a candidate that was not checked. +func TestGuardCheckOversizedCandidateIsAFault(t *testing.T) { + guardRepo(t) + padded := strings.Repeat("# padding\n", (maxGuardStdinBytes/10)+1) + "cd scratch && rm -rf *\n" + stdout, stderr, code := runGuard(padded, "guard", "check") + + if code != 2 { + t.Errorf("a truncated candidate must be a fault (exit 2), got %d (stdout %q)", code, stdout) + } + if strings.Contains(stdout, "allow") { + t.Errorf("a prefix must never be rendered as a clearance; stdout = %q", stdout) + } + if !strings.Contains(stderr, "too long") { + t.Errorf("the fault must say the candidate did not fit; stderr = %q", stderr) + } +} + // TestGuardCheckMalformedRepoConfigIsAFault holds the same line for the config: // a broken .abcd/guard.json must be loud, never a fallback to "no guard". func TestGuardCheckMalformedRepoConfigIsAFault(t *testing.T) { From 4288532cb91b457ada0293e5d84b3197e0681546 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:36:46 +0100 Subject: [PATCH 17/17] fix: refuse to answer from a switched-off registry, and correct four claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups from the security re-review (verdict APPROVE, none of these blocking). **`guard check` answered `allow` from a disabled registry.** The hook path warns, but the scriptable verb rendered a bare allow, exit 0, indistinguishable from a real clearance — so a CI gate could be waved through by an edit to `.abcd/guard.json`. A disabled registry evaluated nothing, which is this verb's own definition of a fault: it exits 2 and names the file. Three claims in text this change ships were wrong: - The brief documented the fail-open path as exit 0 and said only allow and block propagate. Both backwards — exit 1 is what keeps the warning visible, and the shim passes it through precisely so a binary that ran and reported is not described as having failed to run. - The corrected coverage list named `sudo`/`env`/`time` as known wrappers, implying hazards launched through them are seen. Only the wrapper NAME is stepped over, so one flag defangs an entry the registry does describe: `sudo ` is seen, `sudo -u bob ` is not. Stated everywhere the list appears, and added to iss-148 as the sharpest of its items. - Two comments in new code still called the kill switch committed, the claim the docs had just retired. Assisted-by: Claude:claude-opus-5[1m] --- .../development/brief/04-surfaces/17-guard.md | 26 ++++++++++++------- ...ge-gaps-found-while-wiring-itd-103-regi.md | 2 +- CHANGELOG.md | 7 +++-- commands/abcd/guard.md | 6 +++-- docs/reference/cli/commands.md | 6 +++-- internal/README.md | 5 ++-- internal/core/ahoy/guard_health.go | 6 +++-- internal/surface/cli/guard.go | 21 ++++++++++++--- internal/surface/cli/guard_verb_test.go | 26 +++++++++++++++++++ 9 files changed, 81 insertions(+), 24 deletions(-) diff --git a/.abcd/development/brief/04-surfaces/17-guard.md b/.abcd/development/brief/04-surfaces/17-guard.md index 0061e9b..cba0d1c 100644 --- a/.abcd/development/brief/04-surfaces/17-guard.md +++ b/.abcd/development/brief/04-surfaces/17-guard.md @@ -33,21 +33,26 @@ only format it: | `warn` | exit 0, warning rendered | exit 0, warning on stderr | | `block` | exit 1, why + successor rendered | the host's blocking status, why + successor as the message | -A guard that cannot be **evaluated** is a fourth case, and the two front doors -part company on it deliberately: +A guard that cannot be **evaluated** — an unparsable command line, a registry +that will not load, a candidate too long to read, a registry switched off — is a +fourth case, and the two front doors part company on it deliberately: - `guard check` exits **2**. A script that asked the guard a question must never read silence as clearance. -- `guard hook` exits **0** and warns loudly. A guard that cannot answer must - never be the reason a session stops. +- `guard hook` exits **1** and warns loudly. Exit 1 rather than 0 is the whole + point: a pre-tool-use hook that exits 0 has its stderr discarded, so the + warning would exist and nobody would ever see it. Exit 1 is non-blocking, so + the command still runs — a guard that cannot answer never stops a session. ## Fail-open-loud -The installed hook entry wraps the binary call in a shim: if the abcd binary is -missing, not executable, crashes, or is killed, the shim allows the command and -emits an unmissable `UNGUARDED` warning. Only the binary's own exit statuses — -allow and block — propagate. A session is therefore never bricked by a broken -guard, and never silently unprotected either. +The installed hook entry wraps the binary call in a shim. The binary's own three +statuses pass through untouched — 0 (allow), 1 (ran, could not decide, allowed +loudly), 2 (block). Anything else means the binary did not run at all: missing, +not executable, crashed, killed. The shim then allows the command and emits an +unmissable `UNGUARDED` warning of its own. A session is therefore never bricked +by a broken guard, and never silently unprotected either — and a binary that ran +and reported is never described as having failed to run. The outside-the-session half is `abcd ahoy`'s `guard:` line, which reports the three things that can independently be false: whether the hook is installed, @@ -81,6 +86,9 @@ other way is not seen: - a command string handed to an interpreter (`eval`, `sh -c`); - one launched through a wrapper outside the small known set (`sudo`, `doas`, `command`, `env`, `nohup`, `time`) — `xargs`, `timeout`, `exec` are not in it; +- one launched through a wrapper that *is* in that set but carries its own flags: + only the wrapper NAME is stepped over, so `sudo ` is seen and + `sudo -u bob ` is not (`-u` is read as the command); - one inside a backtick substitution (`$(…)` is followed, backticks are not); - a dangerous form no entry describes. diff --git a/.abcd/work/issues/open/iss-148-guard-registry-coverage-gaps-found-while-wiring-itd-103-regi.md b/.abcd/work/issues/open/iss-148-guard-registry-coverage-gaps-found-while-wiring-itd-103-regi.md index 283ab63..e6acb7e 100644 --- a/.abcd/work/issues/open/iss-148-guard-registry-coverage-gaps-found-while-wiring-itd-103-regi.md +++ b/.abcd/work/issues/open/iss-148-guard-registry-coverage-gaps-found-while-wiring-itd-103-regi.md @@ -8,4 +8,4 @@ source: "user-observation" found_during: "manual-capture" --- -guard registry coverage gaps found while wiring itd-103 (registry content, not matching semantics): a push whose refspec carries a leading plus is a force in disguise and no entry describes it; xargs, timeout and exec are absent from the matcher's wrappers set, so a hazard launched through one of them is not seen; a backtick command substitution is not followed, while the dollar-paren form is. Candidates for the admission gate as the registry grows from reality. \ No newline at end of file +guard registry coverage gaps found while wiring itd-103 (registry content, not matching semantics): a push whose refspec carries a leading plus is a force in disguise and no entry describes it; xargs, timeout and exec are absent from the matcher's wrappers set, so a hazard launched through one of them is not seen; a backtick command substitution is not followed, while the dollar-paren form is; and a wrapper that IS in the set defangs an entry the moment it carries its own flags, because only the wrapper name is stepped over — `sudo ` is seen, `sudo -u bob ` is not, and the same holds for `env -i` and `time -p`. That last one is the sharpest: it turns an entry the registry does describe into an allow with one extra token, and it is the only item here that a facilitator would reasonably assume was covered. Candidates for the admission gate as the registry grows from reality; the wrapper-flag item is matcher-side (`commandOf` in internal/core/guard/match.go), not registry content. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a1aa01..7c3aca5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,9 +32,12 @@ called out in a **Breaking** section. in that one file and nowhere else — no flag, environment variable, or prompt disarms the guard for a session, so the change lands in a diff. An allow means no entry matched, never that a command is safe: a hazard reached another way — - a string handed to an interpreter (`eval`, `sh -c`), a launcher outside the - small known set, a backtick substitution, or a form no entry describes — is not + a string handed to an interpreter (`eval`, `sh -c`), a launcher the guard does + not step over, a backtick substitution, or a form no entry describes — is not seen. Coverage is what the registry names, and the command reference says so. + A registry that is switched off answers `abcd guard check` with a fault rather + than a clearance, so a script using the verb as a gate cannot be waved through + by an edit to `.abcd/guard.json`. - **`abcd launch scaffold` — the changelog-driven release-gate scaffolder** (itd-93, spc-14). Writes the fixed release machinery into a managed repo that lacks it: `.github/workflows/release.yml` (verify → build → publish, the verify diff --git a/commands/abcd/guard.md b/commands/abcd/guard.md index 89f762d..b53a965 100644 --- a/commands/abcd/guard.md +++ b/commands/abcd/guard.md @@ -83,8 +83,10 @@ retiering a hazard is the user's decision to make and to review. An allow means **no registry entry matched**. It is never a statement that a command is safe. The guard reads command names it can see in command position, so a hazard reached any other way is not seen: a command string handed to an -interpreter (`eval`, `sh -c`), one launched through a wrapper outside the small -known set, one inside a backtick substitution, or a dangerous form no entry +interpreter (`eval`, `sh -c`), one launched through a wrapper the guard does not +step over (`xargs`, `timeout`, `exec` — or a wrapper it does know, such as +`sudo`, carrying its own flags: `sudo ` is seen, `sudo -u bob ` +is not), one inside a backtick substitution, or a dangerous form no entry describes. Coverage is what the registry names. Say exactly this if a user asks about coverage — never that the guard cleared the command. diff --git a/docs/reference/cli/commands.md b/docs/reference/cli/commands.md index 5aa2253..322292c 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -280,8 +280,10 @@ An allow means no registry entry matched — it is never a statement that a command is safe. The guard reads command names it can see in command position, so a hazard reached any other way is not seen: a command string handed to an interpreter (`eval`, `sh -c`), one launched through a wrapper -outside the small known set, one inside a backtick substitution, or a -dangerous form no entry describes. Coverage is what the registry names. +the guard does not step over (or a known wrapper carrying its own flags — +`sudo ` is seen, `sudo -u bob ` is not), one inside a +backtick substitution, or a dangerous form no entry describes. Coverage is +what the registry names. The candidate comes from --command, or from stdin when the flag is absent. Prefer stdin for a command line you did not type yourself: the shell expands diff --git a/internal/README.md b/internal/README.md index 84c4f8f..46a1b9d 100644 --- a/internal/README.md +++ b/internal/README.md @@ -39,8 +39,9 @@ plugin surface, and a future MCP server share one engine. holds each one to its own corpus (100% true-negative floor, at least 40% known-good). An allow means no entry matched, never that a command is safe: a hazard reached another way — a string handed to an interpreter (`eval`, - `sh -c`), a launcher outside the small `wrappers` set, a backtick substitution, - or a form no entry describes — is not seen. That limit is stated here and in + `sh -c`), a launcher outside the small `wrappers` set, a launcher inside it + carrying its own flags (only the wrapper NAME is stepped over), a backtick + substitution, or a form no entry describes — is not seen. That limit is stated here and in the `abcd guard check` reference doc, never left implicit. `Load` reads the working tree, so a `disabled: true` takes effect before it is reviewed; the front door compensates by making a disabled registry loud rather than silent. diff --git a/internal/core/ahoy/guard_health.go b/internal/core/ahoy/guard_health.go index 0b66f7d..94111af 100644 --- a/internal/core/ahoy/guard_health.go +++ b/internal/core/ahoy/guard_health.go @@ -39,8 +39,10 @@ type GuardHealth struct { // declines to answer and every command runs unchecked. RegistryLoadable bool `json:"registry_loadable"` // Disabled reports a deliberately switched-off registry. It is not a fault — - // the committed kill switch is the only sanctioned escape — but a disabled - // guard that looks armed is exactly the state this report exists to prevent. + // .abcd/guard.json is the only route, so the change lands in a diff — but the + // file is read from the WORKING TREE, so this can be true before anyone has + // reviewed the edit that made it true (iss-147). A disabled guard that looks + // armed is exactly the state this report exists to prevent. Disabled bool `json:"disabled"` // Entries is how many hazards the loaded registry holds, so "loadable" is // backed by a number rather than a boolean nobody can check. diff --git a/internal/surface/cli/guard.go b/internal/surface/cli/guard.go index e0e761b..ebb879c 100644 --- a/internal/surface/cli/guard.go +++ b/internal/surface/cli/guard.go @@ -52,8 +52,10 @@ func newGuardCommand(asJSON *bool) *cobra.Command { "command is safe. The guard reads command names it can see in command\n" + "position, so a hazard reached any other way is not seen: a command string\n" + "handed to an interpreter (`eval`, `sh -c`), one launched through a wrapper\n" + - "outside the small known set, one inside a backtick substitution, or a\n" + - "dangerous form no entry describes. Coverage is what the registry names.\n\n" + + "the guard does not step over (or a known wrapper carrying its own flags —\n" + + "`sudo ` is seen, `sudo -u bob ` is not), one inside a\n" + + "backtick substitution, or a dangerous form no entry describes. Coverage is\n" + + "what the registry names.\n\n" + "The candidate comes from --command, or from stdin when the flag is absent.\n" + "Prefer stdin for a command line you did not type yourself: the shell expands\n" + "a double-quoted --command argument before this verb starts, so a candidate\n" + @@ -69,6 +71,15 @@ func newGuardCommand(asJSON *bool) *cobra.Command { if err != nil { return &exitError{Code: 2, Msg: fmt.Sprintf("guard check: %s", scrubPaths(err))} } + // A disabled registry evaluated nothing, so there is no answer to + // render — and a bare `allow` here is indistinguishable from a real + // clearance to the CI job or script using this verb as a gate. Same + // category as an unparsable command line: the question could not be + // answered, so say that rather than answer it wrongly. + if reg.Disabled { + return &exitError{Code: 2, Msg: fmt.Sprintf( + "guard check: the hazard registry is switched off in %s; nothing was checked", guard.RepoRelPath)} + } dec, err := reg.Check(candidate) if err != nil { return &exitError{Code: 2, Msg: fmt.Sprintf("guard check: %s", scrubPaths(err))} @@ -258,8 +269,10 @@ func guardHealthLine(h ahoy.GuardHealth) string { if h.Healthy() { state := fmt.Sprintf("armed (%d hazards)", h.Entries) if h.Disabled { - // Loadable and wired, but switched off by a committed override. Not a - // fault, and not something to report as protection either. + // Loadable and wired, but switched off in .abcd/guard.json. Not a + // fault, and not something to report as protection either. The file is + // read from the working tree, so this can be true before anyone has + // reviewed the edit that made it true (iss-147). state = "OFF — disabled in " + guard.RepoRelPath } return state diff --git a/internal/surface/cli/guard_verb_test.go b/internal/surface/cli/guard_verb_test.go index 96a6016..4cff695 100644 --- a/internal/surface/cli/guard_verb_test.go +++ b/internal/surface/cli/guard_verb_test.go @@ -8,6 +8,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/REPPL/abcd-cli/internal/core/guard" ) // runGuard drives the guard verbs through the real exit-code mapping with stdin @@ -190,6 +192,30 @@ func TestGuardCheckOversizedCandidateIsAFault(t *testing.T) { } } +// TestGuardCheckRefusesToAnswerFromADisabledRegistry closes the scriptable half +// of the kill switch. A CI job or script using this verb as a gate got a bare +// `allow`, exit 0, indistinguishable from a real clearance, for a command an +// entry blocks. A disabled registry means nothing was evaluated, which is the +// verb's own definition of a fault — not an answer. +func TestGuardCheckRefusesToAnswerFromADisabledRegistry(t *testing.T) { + dir := guardRepo(t) + cfg := `{"schema_version":1,"disabled":true,"entries":{}}` + if err := os.WriteFile(filepath.Join(dir, ".abcd", "guard.json"), []byte(cfg), 0o644); err != nil { + t.Fatal(err) + } + stdout, stderr, code := runGuard("", "guard", "check", "--command", "cd scratch && rm -rf *") + + if code != 2 { + t.Errorf("a disabled registry checked nothing and must be a fault (exit 2), got %d", code) + } + if strings.Contains(stdout, "allow") { + t.Errorf("a disabled registry must never render as a clearance; stdout = %q", stdout) + } + if !strings.Contains(stderr, guard.RepoRelPath) { + t.Errorf("the fault must name the file that turned the guard off; stderr = %q", stderr) + } +} + // TestGuardCheckMalformedRepoConfigIsAFault holds the same line for the config: // a broken .abcd/guard.json must be loud, never a fallback to "no guard". func TestGuardCheckMalformedRepoConfigIsAFault(t *testing.T) {