diff --git a/.abcd/development/agents/CHANGELOG.md b/.abcd/development/agents/CHANGELOG.md index abb56a08f..2f962c278 100644 --- a/.abcd/development/agents/CHANGELOG.md +++ b/.abcd/development/agents/CHANGELOG.md @@ -12,6 +12,167 @@ over the brief's earlier `1.0.0`-at-close expectation). The four M6 synthesis agents below entered at `0.1.0`, wired to their `abcd disembark` verbs and unmeasured; `lifeboat-oracle` has since become `lifeboat-reviewer` at `0.1.1`. +## 2026-10-10 (iss-2610091942156774 — sub-agents are taught the rm the guard refuses) + +### cold-reading-comparative 0.1.4 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### cold-reading-detection 0.1.5 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### cold-reading-entailment 0.1.4 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### cold-reading-widening 0.2.4 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### docs-currency-reviewer 0.2.1 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### graveyard-interpreter 0.1.2 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### intent-auditor 0.5.1 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### lifeboat-reviewer 0.1.2 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### planning-interviewer 0.1.1 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### press-release-composer 0.1.2 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### principle-distiller 0.2.1 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### question-drafter 0.1.2 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### reflection-composer 0.3.1 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### release-changelog-composer 0.4.2 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### ruthless-reviewer 0.2.1 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. + +### security-reviewer 0.3.1 + +PATCH: the definition ends with the generated guard-lessons block, which +restates the guard's `rm-unguarded-variable-path` rule as the SHELL domain +renders it: an `rm` whose path starts with a variable that can be empty +followed by `/` is refused, and the operand is written `"${VAR:?}"/...` or as +a literal path instead. A sub-agent is handed none of the session's SHELL +rules, so the block is where it learns the rule before the guard refuses the +command. Nothing else changes. Unmeasured, in the `0.x` band. ## 2026-10-10 (iss-2610100626211810 — a question's chip sets the mode) ### question-drafter 0.1.1 diff --git a/.abcd/work/issues/open/iss-2610100938485695-the-rm-unguarded-variable-path-guard-entry-misjudges-three.md b/.abcd/work/issues/open/iss-2610100938485695-the-rm-unguarded-variable-path-guard-entry-misjudges-three.md new file mode 100644 index 000000000..9953f6f77 --- /dev/null +++ b/.abcd/work/issues/open/iss-2610100938485695-the-rm-unguarded-variable-path-guard-entry-misjudges-three.md @@ -0,0 +1,15 @@ +--- +schema_version: 1 +id: "iss-2610100938485695" +slug: "the-rm-unguarded-variable-path-guard-entry-misjudges-three" +severity: "minor" +category: "bug" +source: "impl-review" +found_during: "Fable review of the rm-unguarded-variable-path entry, 2026-10-10" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/guard/varpath.go" +remedy: "Carry the non-empty guarantee of :? / :- / := through the named re-read of a double-quoted shell string, read nested defaults recursively, and treat a variable followed by a substitution before the slash as emptyable; one fixture per shape, watched fail first." +--- + +The rm-unguarded-variable-path guard entry misjudges three edge shapes found in its review. (1) Inside a double-quoted shell string the successor's own rewrite is refused: sh -c "rm -rf ${X:?}/y" blocks, because the re-read string spells ${X:?} as ${X} (payload.go spellParameterAt), so the refusal tells the agent to do what it did; single quotes avoid it. (2) A nested guard is not read: "${VAR:-${OTHER:?}}"/y and "${VAR:-${OTHER:-/tmp}}"/y block although both are guarded (varpath.go stripEmptyableRefs strips the inner reference). (3) A site followed by a substitution is not followed: rm -rf $VAR$(true)/x is allowed (varpath.go). diff --git a/.abcd/work/issues/resolved/iss-2610091942156774-guard-allows-rm-on-unguarded-variable-path.md b/.abcd/work/issues/resolved/iss-2610091942156774-guard-allows-rm-on-unguarded-variable-path.md new file mode 100644 index 000000000..99fa4a7b3 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2610091942156774-guard-allows-rm-on-unguarded-variable-path.md @@ -0,0 +1,17 @@ +--- +schema_version: 1 +id: "iss-2610091942156774" +slug: "guard-allows-rm-on-unguarded-variable-path" +severity: "minor" +category: "ux" +source: "user-observation" +found_during: "2026-10-09 security-drain coordination, product thinker report" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/guard" +remedy: "Product thinker's ruling 2026-10-09, options 1 and 2: (1) add a guard registry entry that blocks `rm` whose operand is an unguarded variable path (`\"$VAR\"/...`, `$VAR/...`, `\"${VAR}\"/...`) and allows `\"${VAR:?}\"/...` and literal paths, with that rewrite as its successor, so the agent rewrites the command before the host prompts; prove in a live host session that the hook's refusal lands before the host's prompt; (2) teach the same rule through the SHELL domain (generated from the entry) and in abcd's agent definition files, which sub-agents read. Build after the v0.13.4 cut; never a silent rewrite of the command." +resolution: "The bundled registry gains the blocker rm-unguarded-variable-path: an rm whose operand starts with a variable that can be empty directly followed by / is refused, with the \"${VAR:?}\"/... rewrite or a literal path as its successor, through a new arg_shapes operand predicate. The SHELL domain teaches it from the entry, and every shell-capable agent definition but the scribe carries a block generated from the same entry." +impact: fix +--- + +An agent's `rm -f "$VAR"/*` triggers the host's own dangerous-rm permission prompt ("possibly-empty variable path ... rewrite it as "${VAR:?}"/* or use a literal path"), even with permissions skipped. The person cannot tell whether the variable is empty and cannot make the agent take the suggested form, so they must approve blind. abcd's guard allows the shape today, and sub-agents never receive the injected SHELL rules, so nothing steers an agent away from it. Reported by the product thinker on 2026-10-09 from two prompts raised by a review sub-agent. diff --git a/agents/cold-reading-comparative.md b/agents/cold-reading-comparative.md index c5835118c..1fe1d47dc 100644 --- a/agents/cold-reading-comparative.md +++ b/agents/cold-reading-comparative.md @@ -4,7 +4,7 @@ description: >- Cold reading at the comparative position. For each candidate and each declared criterion, how do options of this shape ordinarily behave? Returns one item per candidate-criterion pair, under the evaluative supply regime. -prompt_version: 0.1.3 +prompt_version: 0.1.4 reads_untrusted_input: true capability_scope: task_classes: [cold_reading] @@ -132,3 +132,13 @@ ingest verb's to compose rather than yours. "characterisation": "" } ``` + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/cold-reading-detection.md b/agents/cold-reading-detection.md index fc701a76e..9243fa3b2 100644 --- a/agents/cold-reading-detection.md +++ b/agents/cold-reading-detection.md @@ -4,7 +4,7 @@ description: >- Cold reading at the detection position. Where is the shipped tree in tension with the claim record? Returns tensions, each with the constraint in play and why it is a tension, under the registrative supply regime. -prompt_version: 0.1.4 +prompt_version: 0.1.5 reads_untrusted_input: true capability_scope: task_classes: [cold_reading] @@ -137,3 +137,13 @@ ingest verb's to compose rather than yours. "why_a_tension": "" } ``` + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/cold-reading-entailment.md b/agents/cold-reading-entailment.md index 6af05f189..11b612e66 100644 --- a/agents/cold-reading-entailment.md +++ b/agents/cold-reading-entailment.md @@ -5,7 +5,7 @@ description: >- being the kind of thing it is, that its articulation does not state? Returns surfaced claims, each with its claim type and what implies it, under the explicative supply regime. -prompt_version: 0.1.3 +prompt_version: 0.1.4 reads_untrusted_input: true capability_scope: task_classes: [cold_reading] @@ -136,3 +136,13 @@ rather than yours. "what_implies_it": "" } ``` + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/cold-reading-widening.md b/agents/cold-reading-widening.md index d258150ce..230f39feb 100644 --- a/agents/cold-reading-widening.md +++ b/agents/cold-reading-widening.md @@ -5,7 +5,7 @@ description: >- construes it, what configurations does the construal admit that are not present in what has been committed to? Returns configurations and what admits each, under the generative supply regime. -prompt_version: 0.2.3 +prompt_version: 0.2.4 reads_untrusted_input: true capability_scope: task_classes: [cold_reading] @@ -136,3 +136,13 @@ the record identity — is the ingest verb's to compose rather than yours. "what_admits_it": "" } ``` + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/docs-currency-reviewer.md b/agents/docs-currency-reviewer.md index 7d34b9e92..249e98b14 100644 --- a/agents/docs-currency-reviewer.md +++ b/agents/docs-currency-reviewer.md @@ -2,7 +2,7 @@ name: docs-currency-reviewer description: Semantic docs-currency review — verifies every user-facing claim against the code that implements it. Use PROACTIVELY before tagging any release, and after merging behaviour changes that docs describe. tools: Read, Grep, Glob, Bash -prompt_version: 0.2.0 +prompt_version: 0.2.1 reads_untrusted_input: true capability_scope: task_classes: [cross_document_audit] @@ -55,3 +55,13 @@ the fixes (or flagged judgement calls) in severity order. End with an explicit statement: either "docs are current" or the list of what blocks that statement. If you found nothing, say so plainly; do not invent findings to justify the pass. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/graveyard-interpreter.md b/agents/graveyard-interpreter.md index b313e4b0c..ced7fa416 100644 --- a/agents/graveyard-interpreter.md +++ b/agents/graveyard-interpreter.md @@ -1,7 +1,7 @@ --- name: graveyard-interpreter description: Interpret a packed lifeboat's graveyard — say what was tried and why it was left behind, each lesson citing the layer-1/2 finding ids it rests on. Host-delegated; feeds `abcd disembark graveyard --lessons-json`. -prompt_version: 0.1.1 +prompt_version: 0.1.2 reads_untrusted_input: true capability_scope: task_classes: [cross_document_audit] @@ -99,3 +99,13 @@ files' finding ids ground a lesson. The binary reports every drop with its reason and still **exits 0**. A graveyard that yields no groundable lesson is an honest outcome. Emit only lessons you can tie to a finding id; never manufacture an id to pass the gate. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/intent-auditor.md b/agents/intent-auditor.md index b8ed3eb2a..62f5504b1 100644 --- a/agents/intent-auditor.md +++ b/agents/intent-auditor.md @@ -9,7 +9,7 @@ description: >- file:line evidence pointer. Role 2 (cross-document): reads the assembled brief-and-intents corpus and emits one findings JSON naming each contradiction between two documents, both ends quoted verbatim. -prompt_version: 0.5.0 +prompt_version: 0.5.1 reads_untrusted_input: true capability_scope: task_classes: [intent_audit, intent_consistency] @@ -357,3 +357,13 @@ nothing written): 7. At most 100 findings. 8. The corpus must not have moved between the request and the ingest; if it has, the request is re-emitted and the pass run again. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/lifeboat-reviewer.md b/agents/lifeboat-reviewer.md index 395b525df..44706cbf6 100644 --- a/agents/lifeboat-reviewer.md +++ b/agents/lifeboat-reviewer.md @@ -1,7 +1,7 @@ --- name: lifeboat-reviewer description: Audit a packed lifeboat against its source repo; return a registered verdict (SHIP / NEEDS_WORK / MAJOR_RETHINK) and findings that each cite a packed lifeboat file. Host-delegated; feeds `abcd disembark review --review-json`. -prompt_version: 0.1.1 +prompt_version: 0.1.2 reads_untrusted_input: true capability_scope: task_classes: [oracle_review, audit] @@ -50,7 +50,7 @@ fabricate a manifest hash or claim a verification you did not run. Emit: { "schema_version": 1, "mode": "delegated", - "prompt_version": "0.1.1", + "prompt_version": "0.1.2", "verdict": "NEEDS_WORK", "findings": [ { @@ -109,3 +109,13 @@ Uncitable findings drop and the audit still writes (the binary reports each drop and exits 0); only an out-of-enum verdict or a structural fault refuses the whole payload. State the verdict you can defend and the findings you can pin to a file — never manufacture a path to pass the gate. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/planning-interviewer.md b/agents/planning-interviewer.md index 01ddbc792..fb931c18d 100644 --- a/agents/planning-interviewer.md +++ b/agents/planning-interviewer.md @@ -1,7 +1,7 @@ --- name: planning-interviewer description: Run an intent's planning interview in a plain Terminal on the person's own route, writing each question for abcd to draw and editing the intent record after each answer. Started by `abcd intent interview `, never dispatched by the host. -prompt_version: 0.1.0 +prompt_version: 0.1.1 reads_untrusted_input: true capability_scope: task_classes: [spec_planning] @@ -75,3 +75,13 @@ Exactly one JSON object, written to the receipt path, each turn: abcd refuses a receipt that is not exactly one of these, or whose question breaks an asking rule, and the interview stops with the refusal recorded. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/press-release-composer.md b/agents/press-release-composer.md index cc54af28e..58f8d26fc 100644 --- a/agents/press-release-composer.md +++ b/agents/press-release-composer.md @@ -1,7 +1,7 @@ --- name: press-release-composer description: Compose a lifeboat's press release from its packed brief, spine, and distilled principles — a single grounded document that must cite at least one resolvable source. Host-delegated; feeds `abcd disembark press-release --press-release-json`. -prompt_version: 0.1.1 +prompt_version: 0.1.2 reads_untrusted_input: true capability_scope: task_classes: [surface_render] @@ -41,7 +41,7 @@ reject the **whole payload**. Use exactly these keys: { "schema_version": 1, "mode": "delegated", - "prompt_version": "0.1.1", + "prompt_version": "0.1.2", "headline": "abcd carries a project's theory across a session boundary.", "subhead": "A host-agnostic configuration layer for development.", "body": "The full press-release prose, composed from the packed brief and spine.", @@ -84,3 +84,13 @@ Compose a press release a product-minded reader would come away from with an *accurate* mental model — not a marketing gloss. If the brief is thin, say less rather than inventing. The gate rewards a short, grounded document over a long, uncitable one. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/principle-distiller.md b/agents/principle-distiller.md index 3c1000d2e..fda694ba8 100644 --- a/agents/principle-distiller.md +++ b/agents/principle-distiller.md @@ -1,7 +1,7 @@ --- name: principle-distiller description: Distil durable principles from a packed lifeboat's decision record — each principle citing the record ids or lifeboat paths it rests on. Host-delegated; feeds `abcd disembark principles --principles-json`. -prompt_version: 0.2.0 +prompt_version: 0.2.1 reads_untrusted_input: true capability_scope: task_classes: [principle_distillation] @@ -44,7 +44,7 @@ the **whole payload**, not just the offending entry. Use exactly these keys: { "schema_version": 2, "mode": "delegated", - "prompt_version": "0.2.0", + "prompt_version": "0.2.1", "principles": [ { "id": "prn-oracle-cascade-fixed", @@ -122,3 +122,13 @@ The binary reports every drop with its reason and still **exits 0**. A distillat that grounds nothing is a valid, honest outcome — an empty `"principles": []` said plainly beats a padded list of uncited assertions. Emit only principles you can cite. Never manufacture an id to satisfy the gate. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/question-drafter.md b/agents/question-drafter.md index 5d4431e65..d7a654ff6 100644 --- a/agents/question-drafter.md +++ b/agents/question-drafter.md @@ -1,7 +1,7 @@ --- name: question-drafter description: Draft one of abcd's questions so it follows every asking rule and fits the rows limit — given the material to quote, the addressee, the decision and the defensible answers, return the host question tool's input and the rows each tab takes. Never puts anything to the person itself. -prompt_version: 0.1.1 +prompt_version: 0.1.2 reads_untrusted_input: true capability_scope: task_classes: [surface_render] @@ -114,3 +114,13 @@ The check estimates the rows one tab takes in the host's question view in a wind The tab's rows are the sum of the three. To wrap a text at a width, take each of its lines on its own and fill it word by word, as a terminal fills a line: a word goes on the current line when it fits beside the words already there, separated by one space, and starts a new line when it does not. Each line of the text takes at least one row, so a blank line is one row; blank lines at the start and the end of the text are dropped; an empty text is 0 rows. A word wider than the width stands alone on its line and the host hard-wraps it, so that line counts its width divided by the width, rounded up. Width is counted in terminal columns: most characters take one, and East Asian wide and fullwidth characters take two. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/reflection-composer.md b/agents/reflection-composer.md index 72e340d5f..725a5fbe3 100644 --- a/agents/reflection-composer.md +++ b/agents/reflection-composer.md @@ -1,7 +1,7 @@ --- name: reflection-composer description: Run a cut release's retrospective interview from the seed the binary renders, and draft the four asked sections' answers, asking a clarifying question wherever an answer is thin. Host-delegated; feeds `abcd reflect write --answers `. -prompt_version: 0.3.0 +prompt_version: 0.3.1 reads_untrusted_input: true capability_scope: task_classes: [surface_render] @@ -114,3 +114,13 @@ answer. The words are the person's, lightly joined into sentences where they answered in fragments; never a claim they did not make. The binary passes every answer through the secret scanner before it writes, and refuses, writing nothing, while an answer is under the floor without its follow-up. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/release-changelog-composer.md b/agents/release-changelog-composer.md index 809827830..55add0802 100644 --- a/agents/release-changelog-composer.md +++ b/agents/release-changelog-composer.md @@ -1,7 +1,7 @@ --- name: release-changelog-composer description: Compose the prose of one release cut from the records that shipped in it — the changelog lines and the release page, every line and headline citing the record id it reports, so the binary can prove both documents describe exactly the cut. Host-delegated; feeds `abcd launch ship --changelog-json`. -prompt_version: 0.4.1 +prompt_version: 0.4.2 reads_untrusted_input: true capability_scope: task_classes: [surface_render] @@ -268,3 +268,13 @@ No other keys, at any level. There is no `mode` field here. `breaking` record says what stops working. - Entries keep your order **within** a section; the section order is the binary's. So group your lines by importance inside each section. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/ruthless-reviewer.md b/agents/ruthless-reviewer.md index e61ddb446..de334af49 100644 --- a/agents/ruthless-reviewer.md +++ b/agents/ruthless-reviewer.md @@ -3,7 +3,7 @@ name: ruthless-reviewer description: Demanding senior code review at a Linus/Carmack bar. Use PROACTIVELY on any non-trivial diff before presenting it to the user — correctness, resource handling, error paths, API misuse, dead code. tools: Read, Grep, Glob, Bash model: opus -prompt_version: 0.2.0 +prompt_version: 0.2.1 reads_untrusted_input: true capability_scope: task_classes: [oracle_review] @@ -97,3 +97,13 @@ Propose fixes only for FIX-FIRST findings, and keep them to the minimal correct alternative. Do not attach a suggested rewrite to every observation; elaborating a fix for a marginal finding is how a marginal finding gets mistaken for a real one. No compliment sandwiches, no summary of what the diff does well. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/agents/security-reviewer.md b/agents/security-reviewer.md index 1507623fc..f6be9088f 100644 --- a/agents/security-reviewer.md +++ b/agents/security-reviewer.md @@ -3,7 +3,7 @@ name: security-reviewer description: Adversarial security review of a diff or design. Use PROACTIVELY before presenting any change that touches a trust boundary — auth, secrets, network exposure, input parsing, file/DB access, subprocess execution — or any invariant declared in the project's AGENTS.md. Authorized defensive review of the user's own code only. tools: Read, Grep, Glob, Bash model: opus -prompt_version: 0.3.0 +prompt_version: 0.3.1 reads_untrusted_input: true capability_scope: task_classes: [oracle_review] @@ -93,3 +93,13 @@ Exactly one of: - **NEEDS-INPUT** — what you could not determine, and what would settle it. No praise, no padding, no findings invented to justify the time spent. + + + + +## Shell commands the guard refuses + +A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start. + +- Refused by the guard (rm-unguarded-variable-path): `rm` with an operand that starts with a variable that can be empty followed by `/` (`"$VAR"/…`, `$VAR/…`, `"${VAR}"/…`, `${VAR}/…`). A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f "$VAR"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either. Instead: Write the variable as `"${VAR:?}"/...` (`rm -f -- "${VAR:?}"/*`), which stops the shell with an error when it is empty or unset, or use a literal path. + diff --git a/commands/guard.md b/commands/guard.md index a2a4b34d1..69b1dcca9 100644 --- a/commands/guard.md +++ b/commands/guard.md @@ -503,6 +503,24 @@ as `~/*` does); a final dot-glob (`~/.?`) is not read so, since rm refuses a last segment `..`. Quoted, `'/**'` and `"~/.?"/*` block as `"/*"` does. +An `rm` whose path starts with a variable that can be empty, directly followed +by `/`, is a **block** whatever its flags (`rm-unguarded-variable-path`): +`"$VAR"/*`, `$VAR/x`, `"${VAR}"/x`, `${VAR}/x` and `"$VAR/x"` name a path from +the filesystem root when the variable is empty or unset, and nothing on the +line says which. So are `"$1"/x`, `"$A$B"/x`, an expansion that can print +nothing (`${VAR:-}/x`, `${VAR:+x}/x`), and a default whose word is such a +variable (`"${X:-$Y}"/x`), inside a string a shell runs as on the line. The +successor is the rewrite, `"${VAR:?}"/...`, which stops the shell on an empty +value, or a literal path; neither is refused, and nor is a default that cannot +be empty (`"${TMPDIR:-/tmp}"/x`), a variable later in the path +(`./build/$name`), a bare `"$VAR"`, which empties to no path at all, a trim, a +replacement, a substring or a case change (`"${DIR%/}"/x`), or `$HOME` and +`$PWD`, whose deletes the two entries above name. The guard never rewrites the +command itself. The same rule is restated in every agent definition that can +run a shell, in a block generated from this entry, because a sub-agent is +handed none of the `SHELL` rules the session is taught; the scribe's is the +one exception, its definition being held to ledger content alone. + What an allow still does not see is a hazard that never reaches command position at all: a delete target printed whole by a substitution (`rm -rf $(echo /)`), read by its known text the way `rm -rf $(find …)` names its targets every day, diff --git a/hooks/bootstrap.sh b/hooks/bootstrap.sh index c21db7927..bb12004ee 100755 --- a/hooks/bootstrap.sh +++ b/hooks/bootstrap.sh @@ -583,10 +583,10 @@ staged=yes # SIGKILL runs no trap, so a killed run leaves its PID-stamped temp directory # behind holding a partially downloaded, UNVERIFIED binary. The lock is held from # here on, so sweeping them cannot touch a live run's directory. -rm -rf "$plugin_root"/.bootstrap.tmp.* 2>/dev/null +rm -rf "${plugin_root:?}"/.bootstrap.tmp.* 2>/dev/null if [ -n "$cache_mode" ]; then - rm -rf "$data_dir"/.bootstrap.tmp.* 2>/dev/null - rm -rf "$data_dir"/.bootstrap.auth.* 2>/dev/null + rm -rf "${data_dir:?}"/.bootstrap.tmp.* 2>/dev/null + rm -rf "${data_dir:?}"/.bootstrap.auth.* 2>/dev/null fi # 5. Refresh detector (spc-35): the recorded cache state plus ONE best-effort @@ -1025,7 +1025,7 @@ else # No data dir provisioned this root, so no stamp may say one did: a stale # record from an earlier cache-mode provision of the same root would route # a terminal `ahoy install` to a cache this binary did not come from. - rm -f "$plugin_root/.data-dir" 2>/dev/null + rm -f "${plugin_root:?}/.data-dir" 2>/dev/null # The root-local provenance record, written only on this path: cache-mode # roots read the cache meta instead, and the skew notice compares the LIVE diff --git a/internal/core/guard/agentlessons_test.go b/internal/core/guard/agentlessons_test.go new file mode 100644 index 000000000..a634eaa47 --- /dev/null +++ b/internal/core/guard/agentlessons_test.go @@ -0,0 +1,188 @@ +package guard + +import ( + "flag" + "os" + "path/filepath" + "sort" + "strings" + "testing" +) + +// The agent definitions' guard lessons (iss-2610091942156774, the ruling's +// option 2). The rules loader injects the SHELL domain into the session, and +// a sub-agent the session starts is handed none of it: it reads its own +// definition under agents/ and nothing else, so an rm the guard refuses +// reaches it only as a refusal after the fact. Every definition that can run +// a shell — one that declares Bash among its tools, or declares no tools and +// so is given all of them — carries a generated block restating the lessons +// named in agentLessonEntries, rendered from the registry exactly as the +// SHELL domain renders them (Entry.Lesson), so the two planes teach one text. +// An entry's words are edited in defaults/guard.json; this test then fails +// until the block is regenerated with -update. + +// updateAgentLessons rewrites the agent definitions' blocks from the registry +// instead of failing on a stale one. +var updateAgentLessons = flag.Bool("update", false, "rewrite the agent definitions' guard-lessons blocks from the registry") + +// agentLessonEntries are the registry entries every shell-capable agent +// definition restates: the hazards a sub-agent is likely to write and the +// host otherwise asks the person to approve blind. It is the one list; add an +// entry id here and run -update to teach it to every definition. +var agentLessonEntries = []string{"rm-unguarded-variable-path"} + +// agentLessonsExempt are the shell-capable definitions that cannot carry the +// block, each with the reason. A definition is exempt only where another +// contract over its text forbids what the block holds; the guard still +// refuses the command it would have taught. +var agentLessonsExempt = map[string]string{ + "scribe.md": "the scribe's definition is held to ledger content alone (internal/core/lint's " + + "scribecontract_test): it may name no path outside .abcd/work/issues/ and no code point " + + "outside its own marks, and the lesson names `/*` and the registry's path", +} + +const ( + agentLessonsBegin = "" + agentLessonsEnd = "" +) + +// renderAgentLessons is the block a shell-capable definition carries, from +// its begin marker through its end marker and the newline after it. +func renderAgentLessons(t *testing.T, r Registry) string { + t.Helper() + var b strings.Builder + b.WriteString(agentLessonsBegin + "\n") + b.WriteString("\n\n") + b.WriteString("## Shell commands the guard refuses\n\n") + b.WriteString("A sub-agent is not handed the shell rules the session is taught, so the rules for the commands you are most likely to write are restated here. The guard refuses a command that breaks one before it runs; write it the way the rule says from the start.\n\n") + for _, id := range agentLessonEntries { + e, ok := r.Entries[id] + if !ok { + t.Fatalf("agentLessonEntries names %s, which the bundled registry does not carry", id) + } + e.ID = id + b.WriteString("- " + e.Lesson() + "\n") + } + b.WriteString(agentLessonsEnd + "\n") + return b.String() +} + +// runsShell reports whether an agent definition can run a shell: its +// frontmatter declares no tools line, which gives it every tool, or declares +// Bash among them. +func runsShell(def string) bool { + if !strings.HasPrefix(def, "---\n") { + return true + } + front, _, ok := strings.Cut(def[len("---\n"):], "\n---\n") + if !ok { + return true + } + for _, line := range strings.Split(front, "\n") { + if v, found := strings.CutPrefix(line, "tools:"); found { + for _, tool := range strings.Split(v, ",") { + if strings.TrimSpace(tool) == "Bash" { + return true + } + } + return false + } + } + return true +} + +// spliceAgentLessons is def with its guard-lessons block replaced by block, +// or with block appended after a blank line where it carries none. ok is +// false for a definition carrying the begin marker more than once, or one +// it never closes: the test never guesses where the block belongs. +func spliceAgentLessons(def, block string) (string, bool) { + switch strings.Count(def, agentLessonsBegin) { + case 0: + return strings.TrimRight(def, "\n") + "\n\n" + block, true + case 1: + default: + return "", false + } + start := strings.Index(def, agentLessonsBegin) + stop := strings.Index(def[start:], agentLessonsEnd) + if stop < 0 { + return "", false + } + end := start + stop + len(agentLessonsEnd) + if end < len(def) && def[end] == '\n' { + end++ + } + return def[:start] + block + def[end:], true +} + +// TestAgentDefinitionsCarryTheGuardLessons holds every agent definition to +// the block its tools call for: a shell-capable one carries it exactly as the +// registry renders it, and one that cannot run a shell carries none. +func TestAgentDefinitionsCarryTheGuardLessons(t *testing.T) { + root, err := filepath.Abs(filepath.Join("..", "..", "..")) + if err != nil { + t.Fatal(err) + } + defs, _ := filepath.Glob(filepath.Join(root, "agents", "*.md")) + sort.Strings(defs) + if len(defs) < 10 { + t.Fatalf("found %d agent definitions under %s; the tree has gone missing", len(defs), filepath.Join(root, "agents")) + } + block := renderAgentLessons(t, Defaults()) + carrying := 0 + for _, path := range defs { + rel, _ := filepath.Rel(root, path) + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + def := string(data) + if _, exempt := agentLessonsExempt[filepath.Base(path)]; exempt || !runsShell(def) { + if strings.Contains(def, agentLessonsBegin) { + t.Errorf("%s is exempt or cannot run a shell, and carries the guard-lessons block; remove it", rel) + } + continue + } + carrying++ + want, ok := spliceAgentLessons(def, block) + if !ok { + t.Errorf("%s carries %s more than once, or never closes it with %s", rel, agentLessonsBegin, agentLessonsEnd) + continue + } + if want == def { + continue + } + if *updateAgentLessons { + if err := os.WriteFile(path, []byte(want), 0o644); err != nil { + t.Fatal(err) + } + t.Logf("rewrote the guard-lessons block of %s", rel) + continue + } + t.Errorf("%s does not carry the guard-lessons block the registry renders; run "+ + "`go test ./internal/core/guard -run TestAgentDefinitionsCarryTheGuardLessons -update`, "+ + "then bump the definition's prompt_version and add its entry to .abcd/development/agents/CHANGELOG.md", rel) + } + if carrying == 0 { + t.Fatal("no agent definition can run a shell; the tools reading is broken") + } +} + +// TestRunsShellReadsTheToolsLine pins the frontmatter reading the test above +// rests on. +func TestRunsShellReadsTheToolsLine(t *testing.T) { + for _, tc := range []struct { + def string + want bool + }{ + {"---\nname: a\n---\nbody\n", true}, + {"---\nname: a\ntools: Read, Grep, Glob, Bash\n---\nbody\n", true}, + {"---\nname: a\ntools: WebSearch, Read, Grep\n---\nbody\n", false}, + {"---\nname: a\ntools: Bashful\n---\nbody\n", false}, + {"no frontmatter\n", true}, + } { + if got := runsShell(tc.def); got != tc.want { + t.Errorf("runsShell(%q) = %v, want %v", tc.def, got, tc.want) + } + } +} diff --git a/internal/core/guard/argspelling_test.go b/internal/core/guard/argspelling_test.go index 395ef1b0d..975b2409d 100644 --- a/internal/core/guard/argspelling_test.go +++ b/internal/core/guard/argspelling_test.go @@ -28,8 +28,10 @@ func TestArgValuesReadAVariableAsWritten(t *testing.T) { {`rm -rf "$PWD"`, VerdictWarn, cwd}, {`rm -rf $PWD`, VerdictWarn, cwd}, {`rm -rf ${PWD}/*`, VerdictWarn, cwd}, - {`rm -rf "$BUILD_DIR"/*`, VerdictAllow, ""}, - {`rm -rf $OUT/`, VerdictAllow, ""}, + // Not a delete of the root as written; a path from the root when the + // variable is empty, which its own entry refuses (iss-2610091942156774). + {`rm -rf "$BUILD_DIR"/*`, VerdictBlock, "rm-unguarded-variable-path"}, + {`rm -rf $OUT/`, VerdictBlock, "rm-unguarded-variable-path"}, {`rm -rf /`, VerdictBlock, home}, {`rm -rf ~`, VerdictBlock, home}, {`rm -rf ~/.*`, VerdictBlock, home}, diff --git a/internal/core/guard/config.go b/internal/core/guard/config.go index 0108b485f..5a67cbaeb 100644 --- a/internal/core/guard/config.go +++ b/internal/core/guard/config.go @@ -273,6 +273,9 @@ func mergePattern(base, over Pattern) Pattern { if over.ArgValues != nil { r.ArgValues = append([]string(nil), over.ArgValues...) } + if over.ArgShapes != nil { + r.ArgShapes = append([]string(nil), over.ArgShapes...) + } if over.FlagValues != nil { r.FlagValues = cloneFlagValues(over.FlagValues) } diff --git a/internal/core/guard/defaults/guard.json b/internal/core/guard/defaults/guard.json index a92b65632..b62ffb084 100644 --- a/internal/core/guard/defaults/guard.json +++ b/internal/core/guard/defaults/guard.json @@ -123,6 +123,44 @@ ] } }, + "rm-unguarded-variable-path": { + "tier": "blocker", + "pattern": { + "command": "rm", + "arg_shapes": ["unguarded-variable-path"] + }, + "why": "A path that starts with a variable followed by `/` names a path from the filesystem root when the variable is empty or unset — `rm -f \"$VAR\"/*` becomes `rm -f /*` — and nothing on the line says which, so whoever is asked to approve it cannot tell either.", + "successor": "Write the variable as `\"${VAR:?}\"/...` (`rm -f -- \"${VAR:?}\"/*`), which stops the shell with an error when it is empty or unset, or use a literal path.", + "fixtures": { + "known_bad": [ + "rm -f \"$VAR\"/*", + "rm -rf $VAR/build", + "rm -rf \"${VAR}\"/build", + "rm -rf ${VAR}/build", + "rm -rf \"$TMPDIR/abcd-x\"", + "rm -rf -- \"$VAR\"/*", + "sudo rm -rf \"$VAR\"/*", + "make clean && rm -rf \"$OUT\"/*", + "sh -c 'rm -rf \"$VAR\"/*'", + "rm -rf \"$1\"/build", + "rm -rf ${VAR:-}/build" + ], + "known_good": [ + "rm -rf \"${VAR:?}\"/build", + "rm -f -- \"${VAR:?}\"/*", + "rm -rf ${VAR:?VAR is unset}/build", + "rm -rf \"${TMPDIR:-/tmp}/abcd-x\"", + "rm -rf /tmp/build", + "rm -rf ./build/$name", + "rm -rf \"$tmp\"", + "rm -rf \"$HOME/.cache/abcd-test\"", + "cat > NOTES.md <<'EOF'\nrm -rf \"$VAR\"/*\nEOF", + "git commit -m 'never run rm -rf \"$VAR\"/* again'", + "abcd capture \"an agent ran rm -f \\\"$VAR\\\"/* and the host asked to approve it blind\"", + "ls \"$VAR\"/" + ] + } + }, "git-push-force": { "tier": "blocker", "pattern": { diff --git a/internal/core/guard/globsegments_test.go b/internal/core/guard/globsegments_test.go index c6a4aceaa..3a226f5fa 100644 --- a/internal/core/guard/globsegments_test.go +++ b/internal/core/guard/globsegments_test.go @@ -115,7 +115,7 @@ func TestWorkingDirectoryParentThroughPWD(t *testing.T) { } allows := []string{ `rm -rf $PWD/../x`, `rm -rf x/../*`, `rm -rf x/..`, `rm -rf x/../y`, - `rm -rf ../x`, `rm -rf $X/../../*`, `rm -rf ~user/../../*`, `rm -f $PWD/../*`, + `rm -rf ../x`, `rm -rf ~user/../../*`, `rm -f $PWD/../*`, } for _, c := range allows { for n, cmd := range globSegmentSpellings(c) { @@ -125,6 +125,14 @@ func TestWorkingDirectoryParentThroughPWD(t *testing.T) { } } } + // `$X/../../*` climbs from a directory the line does not name, so it is + // neither the working directory nor its parent; it is a path from the + // root when X is empty, which its own entry refuses (iss-2610091942156774). + for _, cmd := range globSegmentSpellings(`rm -rf $X/../../*`) { + if d := verdictOf(t, cmd); d.Verdict != VerdictBlock || d.EntryID != "rm-unguarded-variable-path" { + t.Errorf("Check(%q) = %q via %q, want block via rm-unguarded-variable-path", cmd, d.Verdict, d.EntryID) + } + } } // TestTrailingSlashGlobsOfTheRootOrTheHome — iss-2609290929129725. `/*/` diff --git a/internal/core/guard/guard.go b/internal/core/guard/guard.go index 324e9de2a..e92b43f06 100644 --- a/internal/core/guard/guard.go +++ b/internal/core/guard/guard.go @@ -87,6 +87,21 @@ type Pattern struct { // compared as a path with its redundant separators taken out: `//*` is // the word `/*`, and `$HOME/./` the word `$HOME/`. ArgValues []string `json:"arg_values,omitempty"` + // ArgShapes constrain an OPERAND by a shape of the word as the line wrote + // it, which neither a fixed prefix nor an exact word can say: every listed + // shape must be carried by some non-flag argument. The shapes are a closed + // set the matcher knows (argShapes), so a misspelt name is refused at load + // rather than describing an operand nothing can be. + // + // ShapeUnguardedVariablePath is an operand that begins with a variable + // whose value can be empty, directly followed by `/` (`"$VAR"/*`, + // `${VAR}/x`, `"$VAR/x"`): empty or unset, it names a path from the + // filesystem root. `"${VAR:?}"/x` is not it, nor is a default that cannot + // be empty (`"${TMPDIR:-/tmp}"/x`), a variable later in the path + // (`./build/$name`), or a bare `"$VAR"`, which empties to no path at all. + // `$HOME` and `$PWD` are not counted: the login and the shell set them, + // and the entries that name them own a delete of them. + ArgShapes []string `json:"arg_shapes,omitempty"` // MinOperands, when set, requires at least that many non-flag arguments // (value_flags stepped over). It is what separates a kill BY PATTERN — // `pkill make`, whose operand is the pattern — from `pkill -g 4242`, which @@ -387,6 +402,13 @@ func validatePattern(id string, p Pattern) error { return fmt.Errorf("%w: entry %s argument value %q starts with a dash and could never match a non-flag argument", ErrInvalidEntry, id, value) } } + // A shape the matcher does not know describes an operand nothing can be: + // the silent defang again, one field along. + for i, shape := range p.ArgShapes { + if !argShapes[shape] { + return fmt.Errorf("%w: entry %s argument shape %d is %q, which is not a shape the guard knows", ErrInvalidEntry, id, i, shape) + } + } // A flag-value constraint with no flag, or with no accepted value, can // never be satisfied — the silent defang again, one field along. for i, fv := range p.FlagValues { @@ -916,6 +938,7 @@ func clonePattern(p Pattern) Pattern { out.Flags = append([]string(nil), p.Flags...) out.ArgPrefixes = append([]string(nil), p.ArgPrefixes...) out.ArgValues = append([]string(nil), p.ArgValues...) + out.ArgShapes = append([]string(nil), p.ArgShapes...) out.ArgPaths = append([]PathArg(nil), p.ArgPaths...) out.FlagValues = cloneFlagValues(p.FlagValues) out.ArgsFrom = clonePatterns(p.ArgsFrom) diff --git a/internal/core/guard/guardset_test.go b/internal/core/guard/guardset_test.go index a796e765f..fabcb8877 100644 --- a/internal/core/guard/guardset_test.go +++ b/internal/core/guard/guardset_test.go @@ -217,11 +217,14 @@ func TestParametersThatPrintNothingTheWrittenCompareReads(t *testing.T) { {`rm -rf $#/`, shellBare | shellSQ, VerdictAllow, ""}, {`rm -rf $0/`, shellBare | shellSQ, VerdictAllow, ""}, {`rm -rf ${0}/`, shellBare | shellSQ, VerdictAllow, ""}, - {`rm -rf $_x/`, shellBare | shellSQ, VerdictAllow, ""}, + // `$_x` is the variable `_x`, not `$_`: no delete of the root as + // written, and a path from the root when it is empty, which its own + // entry refuses (iss-2610091942156774). + {`rm -rf $_x/`, shellBare | shellSQ, VerdictBlock, "rm-unguarded-variable-path"}, {`rm -rf $!`, shellBare | shellSQ, VerdictAllow, ""}, {`rm -rf "$1"`, shellBare | shellSQ, VerdictAllow, ""}, {`rm -f "$tmp.$!"`, shellBare | shellSQ, VerdictAllow, ""}, - {`rm -rf "$d/$!"`, shellBare | shellSQ, VerdictAllow, ""}, + {`rm -rf "$d/$!"`, shellBare | shellSQ, VerdictBlock, "rm-unguarded-variable-path"}, {`kill $!`, shellBare | shellSQ, VerdictAllow, ""}, {`wait $!`, shellBare | shellSQ, VerdictAllow, ""}, {`echo $!`, shellBare | shellSQ, VerdictAllow, ""}, @@ -483,8 +486,11 @@ func TestExpansionsThatCanPrintNothingTheWrittenCompareReads(t *testing.T) { {`rm -rf ${X@U}/`, all, VerdictBlock, home}, // The look-alikes: text after the empty text, a quoted array, and // an error message. - {`rm -rf "${TMPDIR:-}/abcd-x"`, shellBare | shellSQ, VerdictAllow, ""}, - {`rm -rf "${A[0]}/build"`, shellBare | shellSQ, VerdictAllow, ""}, + // Neither is the root or the home as written; each is a path from + // the root when it prints nothing, which its own entry refuses + // (iss-2610091942156774). + {`rm -rf "${TMPDIR:-}/abcd-x"`, shellBare | shellSQ, VerdictBlock, "rm-unguarded-variable-path"}, + {`rm -rf "${A[0]}/build"`, shellBare | shellSQ, VerdictBlock, "rm-unguarded-variable-path"}, {`rm -rf "${files[@]}"`, shellBare | shellSQ, VerdictAllow, ""}, {`rm -rf ${X^^}.txt`, shellBare | shellSQ, VerdictAllow, ""}, {`rm -rf "${BUILD_DIR:?}/"*`, shellBare | shellSQ, VerdictAllow, ""}, diff --git a/internal/core/guard/homeresiduals_test.go b/internal/core/guard/homeresiduals_test.go index 009e64f58..f09da2543 100644 --- a/internal/core/guard/homeresiduals_test.go +++ b/internal/core/guard/homeresiduals_test.go @@ -135,7 +135,9 @@ func TestHomeSpellingsTheWrittenCompareReads(t *testing.T) { {`rm -rf "$HO"{ME,}`, bare | sq, VerdictAllow, ""}, {`rm -rf ${HO}{ME,}`, bare | sq, VerdictAllow, ""}, {"rm -rf $HO\\ME", bare | sq, VerdictAllow, ""}, - {`rm -rf {$OUT,x}/`, bare | sq, VerdictAllow, ""}, + // No home in either word; `$OUT/` is a path from the root when OUT + // is empty, which its own entry refuses (iss-2610091942156774). + {`rm -rf {$OUT,x}/`, bare | sq, VerdictBlock, "rm-unguarded-variable-path"}, {`rm -rf ${X:+$HOME/x}`, bare | sq, VerdictAllow, ""}, {`rm -rf ${X:+$HOMEx}`, bare | sq, VerdictAllow, ""}, {`rm -rf ${X:+$HO}ME`, bare | sq, VerdictAllow, ""}, diff --git a/internal/core/guard/match.go b/internal/core/guard/match.go index 7a41f94e8..1454cfa5a 100644 --- a/internal/core/guard/match.go +++ b/internal/core/guard/match.go @@ -358,7 +358,7 @@ func matchSegmentNamed(p Pattern, s segment) (hit, named bool) { tally(len(s.tokens)) // glob reports, per TOKEN index, whether bash would expand that token. glob := func(i int) bool { return !noglob && s.globAt(i) } - m := newEntryMatcher(p, s.tokens, s.spelled, glob) + m := newEntryMatcher(p, s.tokens, s.spelled, s.varLead, glob) for _, a := range group { if m.matchesAfter(a.idx) && argsFed(p, s, a.idx) { hit = true @@ -580,14 +580,16 @@ type entryMatcher struct { // a value (`git -$(x) /tmp push`); a word that may print nothing both is and is // not an operand (`git $(true) push`). The subcommands, the count, the prefix // and the path are all met by one reading. spelled is the segment's -// segment.spelled, which only the arg_values clause reads (writtenMatches). -func newEntryMatcher(p Pattern, tokens []string, spelled map[int][]string, glob func(int) bool) entryMatcher { +// segment.spelled, which only the arg_values clause reads (writtenMatches), +// and varLead the segment's segment.varLead, which only the arg_shapes +// clause reads (ShapeUnguardedVariablePath). +func newEntryMatcher(p Pattern, tokens []string, spelled map[int][]string, varLead map[int]bool, glob func(int) bool) entryMatcher { n := len(tokens) want := operandWant{ sub: p.Subcommand, sub2: p.Subcommand2, min: p.MinOperands, - prefixes: p.ArgPrefixes, paths: p.ArgPaths, values: p.ArgValues, + prefixes: p.ArgPrefixes, paths: p.ArgPaths, values: p.ArgValues, shapes: p.ArgShapes, } - m := entryMatcher{accept: operandAcceptance(tokens, spelled, p.ValueFlags, want, glob), nextStop: make([]int, n+1)} + m := entryMatcher{accept: operandAcceptance(tokens, spelled, varLead, p.ValueFlags, want, glob), nextStop: make([]int, n+1)} m.nextStop[n] = n for i := n - 1; i >= 0; i-- { m.nextStop[i] = m.nextStop[i+1] diff --git a/internal/core/guard/payload.go b/internal/core/guard/payload.go index 394a39e4c..1c3c6c86a 100644 --- a/internal/core/guard/payload.go +++ b/internal/core/guard/payload.go @@ -369,9 +369,14 @@ func spelledViews(s segment) []segment { // the mark-view word's known text in the same order around it (fitsWritten); // a word no reading pairs keeps the spelling it has, which names no variable. // A word paired with more than maxSpellings texts, or with a text past a -// bound, is spellCapped. Nothing else of psegs is changed. +// bound, is spellCapped. A paired word also takes its segment.varLead from +// the readings it was paired with, any one of which opening a path with a +// variable that can be empty marks it: the mark-view word holds the value's +// mark, whose name it no longer has (opensUnguardedPath). Nothing else of +// psegs is changed. func spellPayload(psegs []segment, named []string) { paired := make([]map[int][]string, len(psegs)) + leads := make([]map[int]bool, len(psegs)) for _, nm := range named { nsegs, err := tokenize(nm) if err != nil || len(nsegs) != len(psegs) { @@ -398,6 +403,10 @@ func spellPayload(psegs []segment, named []string) { if !fitsWritten(m.tokens[j], n.tokens[j]) { continue } + if leads[i] == nil { + leads[i] = map[int]bool{} + } + leads[i][j] = leads[i][j] || n.varLead[j] for _, w := range ws { if fitsWritten(m.tokens[j], w) { if paired[i] == nil { @@ -417,6 +426,18 @@ func spellPayload(psegs []segment, named []string) { psegs[i].spelled[j] = texts } } + for i, words := range leads { + for j, lead := range words { + switch { + case lead && psegs[i].varLead == nil: + psegs[i].varLead = map[int]bool{j: true} + case lead: + psegs[i].varLead[j] = true + default: + delete(psegs[i].varLead, j) + } + } + } } // fitsWritten reports whether word, read from a string's named text, can be diff --git a/internal/core/guard/teach.go b/internal/core/guard/teach.go index a297bd0e1..7859f04aa 100644 --- a/internal/core/guard/teach.go +++ b/internal/core/guard/teach.go @@ -238,6 +238,11 @@ func (p Pattern) Describe() string { for _, pre := range p.ArgPrefixes { with = append(with, "an operand starting `"+pre+"`") } + for _, shape := range p.ArgShapes { + if shape == ShapeUnguardedVariablePath { + with = append(with, "an operand that starts with a variable that can be empty followed by `/` (`\"$VAR\"/…`, `$VAR/…`, `\"${VAR}\"/…`, `${VAR}/…`)") + } + } switch { case p.MinOperands == 1: with = append(with, "an operand") diff --git a/internal/core/guard/tokenize.go b/internal/core/guard/tokenize.go index 112a828b5..e8ed4825c 100644 --- a/internal/core/guard/tokenize.go +++ b/internal/core/guard/tokenize.go @@ -102,6 +102,12 @@ type segment struct { // the default IFS (ifsSplits in unknown.go), which a line that names IFS // reads as past its bound (capIFSSplits). nil when no word is. ifsSplit map[int]bool + // varLead records, per token index, a word that opens with a variable + // whose value can be empty directly followed by `/` (opensUnguardedPath + // in varpath.go), which empty names a path from the filesystem root. Only + // an entry's arg_shapes read it (ShapeUnguardedVariablePath). nil when no + // word does. + varLead map[int]bool // arrivals caches commandArrivals(tokens) once Check has its final // segments (walked records that it is set), so the walk to command position // is paid once per segment rather than once per entry. A segment built @@ -491,6 +497,8 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { curVarAt []varSite // splits rides with the segment (segment.ifsSplit). splits map[int]bool + // leads rides with the segment (segment.varLead). + leads map[int]bool // curMask is parallel to cur and records, per byte, whether it reached // the tokenizer unquoted (wordStruct) and whether it began its word // (wordRawStart) — what the brace expander needs to read a word the way @@ -767,6 +775,18 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { } splits[len(toks)] = true } + // markVarLead files the word being built under segment.varLead when it + // opens with a variable that can be empty, directly followed by `/` + // (opensUnguardedPath in varpath.go). + markVarLead := func(word []byte, sites []varSite) { + if !opensUnguardedPath(word, sites) { + return + } + if leads == nil { + leads = map[int]bool{} + } + leads[len(toks)] = true + } // recordSpelling files the word being built under segment.spelled when a // variable's mark is in it: word is the token it becomes, and whole // reports that the token is cur as built, so each mark's place is known. @@ -784,6 +804,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { case whole: spells[len(toks)] = spellWritten(cur, curVarAt, nil) markIFSSplit(curVarAt) + markVarLead(cur, curVarAt) case isUnknown(word): spells[len(toks)] = []string{unknownText} } @@ -813,6 +834,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { } spells[len(toks)] = spellWritten(w.b, sites, w.m) markIFSSplit(sites) + markVarLead(w.b, sites) } flushToken := func() { if !hasCur { @@ -897,7 +919,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { tokens: toks, chain: chain, braceGroup: braceGroup, globbed: globsOrNil(globs), stdinStream: curStdin || pipeNext || len(groupIn) > 0, literal: lits, feeds: feeds, piped: piped, stdinIn: groupIn, home: list, at: len(segs), variable: vars, spelled: spells, - ifsSplit: splits, redirects: curRedirs, afterAnd: andNext, end: pos, + ifsSplit: splits, varLead: leads, redirects: curRedirs, afterAnd: andNext, end: pos, }) curRedirs, andNext = nil, false toks = nil @@ -906,6 +928,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { vars = nil spells = nil splits = nil + leads = nil feeds = nil braceGroup = false pipeNext = false @@ -1052,7 +1075,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { openSubstitution := func(kind parenKind, pos int, procSub bool) { saved := &enclosing{ toks: toks, globs: globs, lits: lits, vars: vars, curVar: curVar, curSub: curSub, - spells: spells, curVarAt: curVarAt, splits: splits, + spells: spells, curVarAt: curVarAt, splits: splits, leads: leads, cur: cur, curMask: curMask, hasCur: hasCur, curGlob: curGlob, curBrace: curBrace, braceGroup: braceGroup, chain: chain, procSub: procSub, curStdin: curStdin, pipeNext: pipeNext, curDocs: curDocs, pieces: curPieces, @@ -1062,7 +1085,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { toks, globs, lits, cur, curMask, hasCur, curGlob, curBrace, braceGroup = nil, nil, nil, nil, nil, false, false, false, false curRedirs, andNext = nil, false curPieces, vars, curVar, curSub = nil, nil, false, false - spells, curVarAt, splits = nil, nil, nil + spells, curVarAt, splits, leads = nil, nil, nil, nil // A substitution is a command string of its own: its pipelines begin // inside it. Its standard input is its command's: what was piped into // the groups around it, and the pipe into the command it sits in @@ -1134,7 +1157,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { curStdin, pipeNext, curDocs, curPieces = e.curStdin, e.pipeNext, e.curDocs, e.pieces feeds, curFeeds, pipeFrom, braceFrom, groupIn = e.feeds, e.curFeeds, e.pipeFrom, e.braceFrom, e.groupIn vars, curVar, curSub = e.vars, e.curVar, e.curSub - spells, curVarAt, splits = e.spells, e.curVarAt, e.splits + spells, curVarAt, splits, leads = e.spells, e.curVarAt, e.splits, e.leads curRedirs, andNext = e.redirs, e.andNext resumeDocs(e) if !f.bare { @@ -1159,7 +1182,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { curStdin, pipeNext, curDocs, curPieces = e.curStdin, e.pipeNext, e.curDocs, e.pieces feeds, curFeeds, pipeFrom, braceFrom, groupIn = e.feeds, e.curFeeds, e.pipeFrom, e.braceFrom, e.groupIn vars, curVar, curSub = e.vars, e.curVar, e.curSub - spells, curVarAt, splits = e.spells, e.curVarAt, e.splits + spells, curVarAt, splits, leads = e.spells, e.curVarAt, e.splits, e.leads curRedirs, andNext = e.redirs, e.andNext feedFrom(e.segStart) if e.procSub { @@ -1190,6 +1213,8 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { addVar(spellParameter(body, split)...) site := &curVarAt[len(curVarAt)-1] site.split = split + site.guarded = guardedValue(body) + site.transform = transformsValue(body) if len(segs) > start { curSub = true } @@ -2380,6 +2405,7 @@ type enclosing struct { spells map[int][]string curVarAt []varSite splits map[int]bool + leads map[int]bool cur []byte curMask []byte hasCur bool diff --git a/internal/core/guard/unguardedvar_test.go b/internal/core/guard/unguardedvar_test.go new file mode 100644 index 000000000..fbacd0807 --- /dev/null +++ b/internal/core/guard/unguardedvar_test.go @@ -0,0 +1,132 @@ +package guard + +import ( + "strings" + "testing" +) + +// TestRmOnAnUnguardedVariablePathBlocks — iss-2610091942156774. An rm whose +// operand begins with a variable followed by `/` (`"$VAR"/*`) deletes from the +// filesystem root when the variable is empty or unset, and the host raises its +// own dangerous-rm prompt for exactly that shape, which the person must +// approve blind. The guard refuses it first, with the rewrite as its +// successor, so the agent changes the command before the host prompts: +// `"${VAR:?}"/...` stops the shell on an empty value, and a literal path +// cannot be empty. A variable later in the path cannot climb to the root, and +// a bare `"$VAR"` operand empties to no path at all, so neither is it. +func TestRmOnAnUnguardedVariablePathBlocks(t *testing.T) { + const id = "rm-unguarded-variable-path" + runVerdictCases(t, []verdictCase{ + // The four spellings the ruling names, with and without -r. + {`rm -f "$VAR"/*`, VerdictBlock, id}, + {`rm -rf $VAR/build`, VerdictBlock, id}, + {`rm -rf "${VAR}"/build`, VerdictBlock, id}, + {`rm -rf ${VAR}/build`, VerdictBlock, id}, + {`rm "$VAR"/notes.txt`, VerdictBlock, id}, + // The slash inside the quotes is the same word to bash. + {`rm -rf "$VAR/build"`, VerdictBlock, id}, + {`rm -rf "$TMPDIR/abcd-x"`, VerdictBlock, id}, + // A trailing slash alone empties to the root itself. + {`rm -rf "$VAR"/`, VerdictBlock, id}, + // After the operand terminator, behind a launcher, in a chain, among + // other operands. + {`rm -rf -- "$VAR"/*`, VerdictBlock, id}, + {`sudo rm -rf "$VAR"/*`, VerdictBlock, id}, + {`/bin/rm -f "$VAR"/*`, VerdictBlock, id}, + {`make clean && rm -rf "$OUT"/*`, VerdictBlock, id}, + {`rm -rf ./build "$OUT"/cache`, VerdictBlock, id}, + // Inside a string handed to a shell. + {`sh -c 'rm -rf "$VAR"/*'`, VerdictBlock, id}, + {`bash -c "rm -rf \"\$VAR\"/build"`, VerdictBlock, id}, + {`sh -c "rm -rf $VAR/build"`, VerdictBlock, id}, + // A positional parameter, two variables that can both be empty, an + // expansion that can print nothing, and a default whose word is itself + // a variable that can be empty. + {`rm -rf "$1"/build`, VerdictBlock, id}, + {`rm -rf "$A$B"/x`, VerdictBlock, id}, + {`rm -rf ${VAR:-}/build`, VerdictBlock, id}, + {`rm -rf ${VAR:+x}/build`, VerdictBlock, id}, + {`rm -rf "${XDG:-$CACHE}"/x`, VerdictBlock, id}, + + // The rewrites the successor names. + {`rm -rf "${VAR:?}"/build`, VerdictAllow, ""}, + {`rm -f "${VAR:?}"/*`, VerdictAllow, ""}, + {`rm -rf "${VAR:?VAR is unset}"/build`, VerdictAllow, ""}, + {`rm -rf ${VAR:?}/build`, VerdictAllow, ""}, + {`/bin/rm -f -- "${VAR:?}"/*`, VerdictAllow, ""}, + {`rm -rf /tmp/build`, VerdictAllow, ""}, + // A default that cannot be empty guards the value as `:?` does. + {`rm -rf "${TMPDIR:-/tmp}/abcd-x"`, VerdictAllow, ""}, + {`rm -rf "${XDG_CACHE_HOME:-$HOME/.cache}/abcd"`, VerdictAllow, ""}, + // A trim, a replacement, a substring or a case change is an everyday + // expansion the guard leaves allowed. + {`rm -rf "${VAR%/}"/x`, VerdictAllow, ""}, + {`rm -rf "${p%/*}/build"`, VerdictAllow, ""}, + // A variable later in the path cannot climb to the root. + {`rm -rf ./build/$name`, VerdictAllow, ""}, + {`rm -rf build/"$name"`, VerdictAllow, ""}, + {`rm -rf "./out/$VAR"/cache`, VerdictAllow, ""}, + // A bare variable operand empties to no path at all. + {`rm -rf "$tmp"`, VerdictAllow, ""}, + {`rm -f $file`, VerdictAllow, ""}, + {`rm -rf "${VAR}"`, VerdictAllow, ""}, + // The home and the working directory are set by the login and the + // shell; their own entries own a delete of them. + {`rm -rf "$HOME/.cache/abcd-test"`, VerdictAllow, ""}, + {`rm -rf "$PWD/build"`, VerdictAllow, ""}, + // The words as data: a here-document body, a commit message, a quoted + // argument, and other commands. + {"cat > NOTES.md <<'EOF'\nrm -rf \"$VAR\"/*\nEOF", VerdictAllow, ""}, + {"tee NOTES.md <<'EOF'\nrm -f $VAR/build\nEOF", VerdictAllow, ""}, + {`git commit -m 'never run rm -rf "$VAR"/* again'`, VerdictAllow, ""}, + {`printf '%s\n' 'rm -rf "$VAR"/*'`, VerdictAllow, ""}, + {`echo "$VAR"/build`, VerdictAllow, ""}, + {`ls "$VAR"/`, VerdictAllow, ""}, + }) +} + +// TestRmOnAnUnguardedVariablePathTeachesTheRewrite pins what the refusal says: +// the successor names the `"${VAR:?}"/...` rewrite and the literal path, so the +// block message is the lesson the host's prompt would otherwise give the +// person instead of the agent. +func TestRmOnAnUnguardedVariablePathTeachesTheRewrite(t *testing.T) { + d := verdictOf(t, `rm -f "$VAR"/*`) + if d.Verdict != VerdictBlock || d.EntryID != "rm-unguarded-variable-path" { + t.Fatalf("verdict = %q via %q, want a block via rm-unguarded-variable-path", d.Verdict, d.EntryID) + } + for _, want := range []string{`"${VAR:?}"/`, "literal path"} { + if !strings.Contains(d.Successor, want) { + t.Errorf("successor %q does not name %q", d.Successor, want) + } + } +} + +// TestRmRewriteInsideAShellStringIsNotThisEntry: the successor's rewrite +// handed to a shell does not fire the entry. The string as a whole is still +// a warn of the execute-a-string family, which reads `${VAR:?}` there as +// syntax it does not inspect; that verdict is not this entry's and is not +// pinned here. +func TestRmRewriteInsideAShellStringIsNotThisEntry(t *testing.T) { + for _, cmd := range []string{ + `sh -c 'rm -rf "${VAR:?}"/build'`, + `bash -c 'rm -f "${VAR:?}"/*'`, + } { + d := verdictOf(t, cmd) + if d.Verdict == VerdictBlock || contains(d.Matches, "rm-unguarded-variable-path") { + t.Errorf("Check(%q) = %q via %v, want no rm-unguarded-variable-path match", cmd, d.Verdict, d.Matches) + } + } +} + +// TestArgShapesRejectsAnUnknownShape: a shape name the matcher does not know +// would describe an operand nothing can be, the silent defang the other +// operand checks refuse. +func TestArgShapesRejectsAnUnknownShape(t *testing.T) { + r := Defaults() + e := r.Entries["rm-unguarded-variable-path"] + e.Pattern.ArgShapes = []string{"unguarded-variabel-path"} + r.Entries["rm-unguarded-variable-path"] = e + if err := Validate(r); err == nil { + t.Fatal("Validate accepted an unknown arg_shapes name") + } +} diff --git a/internal/core/guard/unknown.go b/internal/core/guard/unknown.go index cd5bdb030..d5e1cc20a 100644 --- a/internal/core/guard/unknown.go +++ b/internal/core/guard/unknown.go @@ -122,6 +122,13 @@ type varSite struct { bare bool split bool width int + // guarded is the text of the variable's own value (`${X}`) where the + // expansion never prints that value empty — `${X:?}`, `${X:-w}`, + // `${X:=w}` — and "" otherwise (guardedValue in varpath.go). + guarded string + // transform records a trim, a replacement, a substring or a case + // change (transformsValue in varpath.go). + transform bool } // emptyable reports whether the parameter named name — the text after its @@ -1702,14 +1709,16 @@ func sitesNamed(s segment, name string) []arrival { } // operandWant is what an entry asks of a command's operands: operand 0 and 1 -// by name, a count, an argument prefix and a resource path carried by some -// operand, and one of a set of exact words standing as some operand. +// by name, a count, an argument prefix, a resource path and a written shape +// carried by some operand, and one of a set of exact words standing as some +// operand. type operandWant struct { sub, sub2 string min int prefixes []string paths []PathArg values []string + shapes []string } // operandAcceptance returns, for each index i of tokens, whether some reading @@ -1721,14 +1730,15 @@ type operandWant struct { // the table's state is (word, operands so far, clauses met), filled from the // end once: linear in the words, whatever the number of places a command can // sit. spelled is the segment's segment.spelled, read by the arg_values -// clause alone (writtenMatches). -func operandAcceptance(tokens []string, spelled map[int][]string, valueFlags []string, want operandWant, glob func(int) bool) []bool { +// clause alone (writtenMatches), and varLead its segment.varLead, read by +// the arg_shapes clause alone. +func operandAcceptance(tokens []string, spelled map[int][]string, varLead map[int]bool, valueFlags []string, want operandWant, glob func(int) bool) []bool { need := want.need() nv := 0 if len(want.values) > 0 { nv = 1 // the values are one clause: any one of them meets it } - nb := uint(len(want.prefixes) + len(want.paths) + nv) + nb := uint(len(want.prefixes) + len(want.paths) + nv + len(want.shapes)) full := 1< 0 && writtenMatches(want.values, tokens, spelled, i) { hits |= 1 << (len(want.prefixes) + len(want.paths)) } + for j, shape := range want.shapes { + if shape == ShapeUnguardedVariablePath && varLead[i] { + hits |= 1 << (len(want.prefixes) + len(want.paths) + nv + j) + } + } } for k := 0; k <= need; k++ { operand := r.operand && diff --git a/internal/core/guard/varpath.go b/internal/core/guard/varpath.go new file mode 100644 index 000000000..7b9fda3b6 --- /dev/null +++ b/internal/core/guard/varpath.go @@ -0,0 +1,203 @@ +package guard + +import "strings" + +// varpath.go reads the one operand shape an entry's arg_shapes can name +// (Pattern.ArgShapes): a word that opens with a variable whose value can be +// empty, directly followed by `/` (iss-2610091942156774). `rm -f "$VAR"/*` +// with VAR empty or unset is `rm -f /*`, and nothing on the line says which: +// the host asks the person to approve that blind, so the guard refuses it +// first and names the rewrite, `"${VAR:?}"/*`, which stops the shell on an +// empty value instead. +// +// The shape is decided while the word is read, from the expansions the +// tokenizer recorded for it (varSite), because the written spelling the +// arg_values compare reads (segment.spelled) cannot tell `${VAR:?}` from +// `${VAR}`: both can print only VAR's value, and the difference is what +// happens when it is empty. + +// ShapeUnguardedVariablePath names the shape in an entry's arg_shapes. +const ShapeUnguardedVariablePath = "unguarded-variable-path" + +// argShapes are the shapes arg_shapes may name; Validate refuses any other. +var argShapes = map[string]bool{ + ShapeUnguardedVariablePath: true, +} + +// guardedValue is the text of the variable's own value, as spellParameter +// writes it (`${X}`), where the `${…}` whose text between the braces is body +// never prints that value empty, and "" otherwise: `${X:?}` stops the shell +// on an empty or unset X, and `${X:-w}` and `${X:=w}` print w then. Whether +// w can be empty is read from w's own texts, which the expansion's texts +// hold beside the value. A plain reference (`${X}`), a test without the +// colon (`${X?}`, `${X-w}`, `${X=w}`, which print a value set to empty), an +// alternative and a subscript keep "". An indirection is not read: the +// value it names is not X's. +func guardedValue(body string) string { + body = paramText(body) + indirect, n := paramNameEnd(body) + if indirect || n <= 0 { + return "" + } + rest := body[n:] + if strings.HasPrefix(rest, ":?") || strings.HasPrefix(rest, ":-") || strings.HasPrefix(rest, ":=") { + return "${" + body[:n] + "}" + } + return "" +} + +// transformsValue reports whether the `${…}` whose text between the braces +// is body is a trim, a replacement, a substring or a case change +// (`${DIR%/}`, `${p%/*}`, `${X/a/b}`, `${X:1}`, `${X^^}`): an expression +// over a value rather than the variable written as a path, which the +// guard leaves allowed as an everyday expansion +// (TestEverydayExpansionsStayAllowed), and which the shape does not read. +// What such an expansion makes of the root is the arg_values compare's to +// read (`${X^}/` is `/`, rm-rf-root-or-home). +func transformsValue(body string) bool { + body = paramText(body) + indirect, n := paramNameEnd(body) + if indirect || n <= 0 || n >= len(body) { + return false + } + rest := body[n:] + if rest[0] == '[' { + // An element's trim (`${X[0]%zzz}`) is a trim. + k := subscriptEnd(rest) + if k < 0 || k+1 >= len(rest) { + return false + } + rest = rest[k+1:] + } + switch { + case strings.HasPrefix(rest, ":?"), strings.HasPrefix(rest, ":-"), strings.HasPrefix(rest, ":="), strings.HasPrefix(rest, ":+"): + return false + } + return strings.IndexByte(":%#/^,@", rest[0]) >= 0 +} + +// opensUnguardedPath reports whether a word, as the tokenizer built it (each +// variable's mark at its site), opens with a variable that can print nothing, +// directly followed by `/`, so the word read with the variable empty is a +// path from the root. A word whose first byte is not a variable's is not it, +// however many variables follow (`./build/$name`), and neither is a word the +// variable ends (`"$VAR"`), which empty is no path at all. +func opensUnguardedPath(word []byte, sites []varSite) bool { + if len(sites) == 0 || sites[0].at != 0 { + return false + } + return siteOpensPath(word, sites, 0) +} + +// siteOpensPath reports whether the site sites[k] can print nothing, or a +// text that opens with a variable that can, where what follows in the word +// or the text is `/` — or is another site of which the same holds +// (`"$A$B"/x`). A transformsValue expansion is not read. Each of the +// expansion's texts is read: the value a guardedValue expansion guards is +// skipped, and a text past a bound is read +// as able to print nothing, so a bound refuses only where a `/` follows. A +// site whose texts are not known is not read: a raw 0x01 byte at the top of +// a line is text, and a mark a payload's text carries from the enclosing +// shell is read from the string with its variables written out instead +// (spellPayload). +func siteOpensPath(word []byte, sites []varSite, k int) bool { + s := sites[k] + next := s.at + max(s.width, 1) + // followed is asked once whatever the number of texts, so a run of + // sites is read once each, never once per combination of their texts. + asked, answer := false, false + followed := func() bool { + if asked { + return answer + } + asked = true + switch { + case next >= len(word): + case word[next] == '/': + answer = true + case k+1 < len(sites) && sites[k+1].at == next: + answer = siteOpensPath(word, sites, k+1) + } + return answer + } + if len(s.texts) == 0 || s.transform { + return false + } + if capped(s.texts) { + return followed() + } + for _, t := range s.texts { + if s.guarded != "" && t == s.guarded { + continue + } + rest, stripped := stripEmptyableRefs(t) + switch { + case rest == "": + if followed() { + return true + } + case stripped && rest[0] == '/': + return true + } + } + return false +} + +// stripEmptyableRefs is t with its leading run of variable references that +// can print nothing taken off (`$X`, `${X}`, `$1`), and whether any was. +// A reference that cannot be empty ends the run: a length (`${#X}`), the +// shell's own numbers (`$$`, `$?`, `$#`, `$0`), and the home and the working +// directory, which the login and the shell set to absolute paths and whose +// own entries name a delete of them. +func stripEmptyableRefs(t string) (string, bool) { + stripped := false + for { + end, empty := leadingRef(t) + if end <= 0 || !empty { + return t, stripped + } + t, stripped = t[end:], true + } +} + +// leadingRef returns the length of the variable reference t opens with, 0 +// when it opens with none, and whether its value can be empty. +func leadingRef(t string) (end int, empty bool) { + if len(t) < 2 || t[0] != '$' { + return 0, false + } + if t[1] == '{' { + depth := 0 + for i := 1; i < len(t); i++ { + switch t[i] { + case '{': + depth++ + case '}': + depth-- + if depth == 0 { + return i + 1, refCanBeEmpty(t[2:i]) + } + } + } + return 0, false + } + if strings.IndexByte("$?#!", t[1]) >= 0 { + return 2, refCanBeEmpty(t[1:2]) + } + if e := simpleParamEnd(t, 1); e > 1 { + return e, refCanBeEmpty(t[1:e]) + } + return 0, false +} + +// refCanBeEmpty reports whether the reference whose text after its `$`, or +// between its braces, is body can print nothing. Every form the spelling +// keeps as written other than a length (`${!X}`, `${X@Q}`) is read as able +// to: a guard refuses where it cannot read. +func refCanBeEmpty(body string) bool { + switch body { + case "HOME", "PWD", "$", "?", "#", "0": + return false + } + return !(len(body) > 1 && body[0] == '#') +} diff --git a/internal/core/reading/definitions_test.go b/internal/core/reading/definitions_test.go index badfafe9e..f6426b05f 100644 --- a/internal/core/reading/definitions_test.go +++ b/internal/core/reading/definitions_test.go @@ -698,11 +698,16 @@ const retiredFourthConditionSentence = "Items come back unordered and unweighted // with spc-2609020626042471: their object's source list gains the knowledge // record, which the include table admits there and not at comparative, so the // comparative definition did not move. +// +// All four moved PATCH together with iss-2610091942156774: each ends with the +// guard-lessons block generated from the shell guard's registry, outside the +// blindness core, which restates the rm the guard refuses for a sub-agent the +// session's shell rules never reach. var promptVersions = map[Position]string{ - PositionWidening: "0.2.3", - PositionEntailment: "0.1.3", - PositionComparative: "0.1.3", - PositionDetection: "0.1.4", + PositionWidening: "0.2.4", + PositionEntailment: "0.1.4", + PositionComparative: "0.1.4", + PositionDetection: "0.1.5", } // TestDetectionItemShapeCitesAConditionIdentity is spc-2609020626046252's diff --git a/scripts/preflight-receipt.sh b/scripts/preflight-receipt.sh index aab6df9b7..ee54786a5 100755 --- a/scripts/preflight-receipt.sh +++ b/scripts/preflight-receipt.sh @@ -119,7 +119,7 @@ mint() { # listing is safe to word-split. # shellcheck disable=SC2012 ls -t "$receipts_rel" | tail -n +"$((keep + 1))" | while IFS= read -r old; do - rm -f "$receipts_rel/$old" + rm -f "${receipts_rel:?}/$old" done echo "preflight: push receipt minted for ${now_head:0:12} — a push of that commit passes the pre-push gate." }