diff --git a/.claude/rules/policy-modules.md b/.claude/rules/policy-modules.md index 1135b1c6a..043e00000 100644 --- a/.claude/rules/policy-modules.md +++ b/.claude/rules/policy-modules.md @@ -127,8 +127,9 @@ the vendored preset that spelled it that way: `git push --force origin main` denied while `cd /tmp && git push --force origin main` was allowed, with a green suite over it. `input.call.segments` is `hook::segments` projected — the same quote-aware tokenizer `shape` and `pipeline` rows are decided by — one entry per -list element, each carrying `words`, `raw`, and the `terminator` that followed -it (`"&&"`, `";"`, `"||"`, `"|"`, `"&"`, or `null` where the command ended). So +list element, each carrying `words`, `raw`, the `terminator` that followed +it (`"&&"`, `";"`, `"||"`, `"|"`, `"&"`, or `null` where the command ended), and +`input-redirect`. So the correct predicate is the short one: ```rego @@ -136,6 +137,31 @@ some segment in input.call.segments segment.words[0] == "git" ``` +**`input-redirect` is per SEGMENT and that is the whole of it** (CLOUD-613): +whether THIS element binds stdin, by `<`, `<<` or `<<<`, outside a quoted span. +A heredoc binds to the element that writes it, so +`git commit -F - && mise run land <<'EOF'` gives `land` the message and git +`/dev/null` — and the command STRING carries an opener either way, which is why +no predicate over `command` can tell that from `git commit -F - <<'EOF'`. +Compare it with `== false`, never as `not segment["input-redirect"]`: Rego reads +an absent key as undefined and `not undefined` HOLDS, so the negated spelling +denies everything on an engine that stopped emitting the field, where the +comparison allows — the direction a miss is supposed to fail in. + +Segments arrive with heredoc **bodies already dropped**, which is the same +change read forwards. A body is data, not shell, so a `;` in a commit message no +longer splits the list and a `nohup` in a documentation paragraph is no longer an +invocation (CLOUD-723, measured twice in one session on the commands that were +documenting the rule). A module therefore does **not** scrub for heredocs, and a +new one copying `run-shape.rego`'s hand-rolled `openers`/`body` comprehensions is +copying the era before this projection. + +A **newline is whitespace, not a separator** — bash disagrees, and the bound is +deliberate rather than an oversight: promoting it would change every landed +`pipeline` verdict. So the shell following a heredoc's terminator joins the +segment its opener was written in, and a two-command call written across lines is +judged as one. It under-denies, which is the sanctioned direction. + There is **one parser**, and a module must not grow a second: no `split` of the command line, in Rego or in Rust. That is not style — without the projection it is ~60 lines of core-builtin string work per module (a list split, a pipe-stage diff --git a/batten.toml b/batten.toml index 31d0bdb51..a2794c34b 100644 --- a/batten.toml +++ b/batten.toml @@ -2897,8 +2897,14 @@ no_fix_reason = "a removal is announced by declaring the window, not by editing # `module` rather than `bundle`: one predicate, one file, and a folder would # enable whatever later lands beside it without a row saying so. # -# Mediated-call scoped, because the predicate is over a command line. The other -# four families of that guard stay in bash and its header says which and why. +# Mediated-call scoped, because every predicate in it is over a command line. +# THREE MORE LANDED WITH CLOUD-613 — `unsatisfiable-commit`, `foreground-sleep` +# and `background-timer`. The fourth family of that guard +# (`cargo-substitutes-for-a-task`) stays in bash on CLOUD-856, and because +# `shell-retirement` admits only a WHOLE-file deletion the guard keeps all four +# until that one can move: both authorities decide these three until then, which +# is CLOUD-1108's row rather than a drift nobody noticed. +# # The short flag cluster `git commit` accepts a message source in — `-m`, `-am`, # `-F`, `-C`, `-c` — as a shape rather than a literal (CLOUD-885). # @@ -2916,6 +2922,22 @@ no_fix_reason = "a removal is announced by declaring the window, not by editing id = "short-message-flag-cluster" regex = "^-[A-Za-z]*[mFCc]" +# CLOUD-613's half of the same predicate, and it is a DIFFERENT question from the +# row above. That one asks "does this cluster select a message source at all"; +# this asks "which flag takes the operand that would make it stdin", so it is +# anchored at BOTH ends — `-F` and `--file` exactly, never `-Fmsg.txt`, which +# carries its own value and cannot be followed by a bare `-`. +[[pattern]] +id = "commit-message-file-flag" +regex = "^(-[A-Za-z]*F|--file)$" + +# THE ID IS NARROWER THAN THE ROW AND STAYS THAT WAY. Three more predicates +# landed in this module with CLOUD-613, so `commit-message-obtainable` now names +# the first one to arrive rather than the set. Renaming it is a `rule-removed` +# smell to `config-lint`, whose only route is a `Weakens:` clause groomed into +# the issue BEFORE the work starts — and asserting one inside the change that +# performs it is precisely what that gate refuses. So the row keeps its name and +# this comment carries the correction; the module file is the honest label. [[rule]] id = "commit-message-obtainable" kind = "policy" @@ -3942,6 +3964,83 @@ id = "R-COMMIT-FROM-A-FILE" kind = "command" target = "git commit -F " +# CLOUD-613. The sibling of the class above, and the distinction is worth two +# rows rather than one: that one is "git was told nothing about where the message +# comes from", this one is "git was told STDIN and nothing was put there". The +# remedy happens to be the same file, but the diagnosis a reader needs is not — +# an author who reads "name a message source" while looking at their own `-F -` +# concludes the gate is wrong. +[[verdict]] +id = "V-COMMIT-STDIN-UNBOUND" +gloss = "a `git commit -F -` has nothing redirected into the element it is written in, so it reads /dev/null" +class = """ +The heredoc binds to the element that WRITES it. `git commit -F - && mise run \ +land <<'EOF'` hands the message to `land` and leaves git reading the harness's \ +/dev/null, so the commit is doomed at the instant it starts — and `githooks(5)` \ +runs `pre-commit` BEFORE git asks for the message, so the whole gate is spent \ +first and only then does git say "Aborting commit due to empty commit message". \ +Measured 2026-08-12 on PR #375: about four minutes of gate on a doomed commit, \ +and killing it took `kill -9` on the process group. A heredoc, `< msg.txt` or \ +`<<< "$msg"` bound to git's OWN element is a message source and is allowed. +""" + +[[verdict.route]] +id = "R-COMMIT-FROM-A-FILE-THAT-CANNOT-REBIND" +kind = "command" +target = "git commit -F " + +# CLOUD-613, CLOUD-482. The waste here is the SESSION rather than a verdict or a +# gate, which is why it is a class of its own rather than a row on either above. +[[verdict]] +id = "V-FOREGROUND-SLEEP" +gloss = "a foreground `sleep` spends the session's own turn waiting, and the call is killed at ~2 minutes" +class = """ +A wait longer than about two minutes does not run slowly, it FAILS — measured at \ +exit 143 and 144 over a hung commit, after which the container was reclaimed \ +with the work uncommitted. Waiting is the harness's job, not the command's: put \ +the work in the background by passing `run_in_background` on the tool call \ +itself and act on its exit, which is delivered (measured 523 of 524 in one \ +session). For a condition rather than a process, background a command that EXITS \ +when the condition holds — that is a background wait and is allowed. +""" + +[[verdict.route]] +id = "R-WAIT-ON-THE-CONDITION" +kind = "command" +target = "until ; do sleep 1; done" + +[[verdict.route]] +id = "R-ASK-WHAT-IS-RUNNING" +kind = "command" +target = "mise run alive" + +# CLOUD-821. NOT a narrower `V-FOREGROUND-SLEEP`: backgrounding is the remedy for +# that one and the subject of this one, so an author reading the wrong class here +# would be told to do the thing they already did. +[[verdict]] +id = "V-BACKGROUND-TIMER" +gloss = "a backgrounded `sleep` with no loop around it is a timer, not a wait" +class = """ +It exits when the clock says so, never when the thing being waited for happens, \ +so it reports the same whether that thing finished, failed, or never started. \ +The wake-up already exists: a backgrounded task's exit notification is delivered, \ +measured 523 of 524 in one session including every failure, so idling until it \ +arrives is the designed state rather than a turn wasted. Measured 2026-08-21: \ +490 of these in one session, 2 of which changed a decision. A backgrounded \ +command carrying an `until`/`while` construct waits on the condition itself and \ +is allowed. +""" + +[[verdict.route]] +id = "R-WAIT-ON-THE-CONDITION-NOT-THE-CLOCK" +kind = "command" +target = "until ; do sleep 1; done" + +[[verdict.route]] +id = "R-ASK-WHAT-IS-RUNNING-ONCE" +kind = "command" +target = "mise run alive" + [[verdict]] id = "V-WORKFLOW-UNPARSED" gloss = "a workflow could not be parsed, so its lanes were never judged" diff --git a/crates/batten/src/hook.rs b/crates/batten/src/hook.rs index 1b2c1608b..c18e76bb7 100644 --- a/crates/batten/src/hook.rs +++ b/crates/batten/src/hook.rs @@ -5146,6 +5146,12 @@ fn call_document(envelope: &Envelope, facts: &Facts<'_>) -> Result>(), @@ -5707,6 +5713,20 @@ struct Segment { /// parser used to split on exactly these operators and discard them, so the /// structure was destroyed before any rule could see it. terminator: Option, + /// Whether this span binds an input redirection — `<`, `<<` or `<<<`. + /// + /// **Per SEGMENT, which is the entire predicate** (CLOUD-613). A heredoc + /// opener binds to the element it is written in, so + /// `git commit -F - && mise run land <<'EOF'` hands the message to `land` + /// and leaves git reading the harness's `/dev/null`. The command STRING + /// carries an opener either way; only the segment that owns it can tell + /// those two apart, which is why this is a field here rather than a + /// question a module could ask of [`crate::hook`]'s `command`. + /// + /// Read outside quoted spans only, so a `<` written inside a commit message + /// is not a redirection — and heredoc BODIES are gone by the time this is + /// set, so prose in a body cannot set it either. + input_redirect: bool, } /// The shell operator between two segments — what happens to the first one's @@ -5764,17 +5784,50 @@ impl Separator { /// pair the policy matches on. It tightens exactly one case — `gh "pr" "merge"`, /// a real invocation, now denies. /// +/// **A heredoc BODY is not shell, and both directions of that are measured** +/// (CLOUD-613). Everything from the newline after a `< Vec { let mut out: Vec = Vec::new(); let mut words: Vec = Vec::new(); let mut word = String::new(); let mut has_word = false; let mut raw = String::new(); + let mut input_redirect = false; + // Delimiters whose bodies start at the next newline, in the order bash + // consumes them: `cat < = Vec::new(); let mut chars = command.chars().peekable(); while let Some(c) = chars.next() { @@ -5814,6 +5867,47 @@ fn segments(command: &str) -> Vec { has_word = true; } } + // AN INPUT REDIRECTION, and the heredoc opener that hides a body + // (CLOUD-613). `<` in any of its three spellings binds stdin, which + // is the whole of what `unsatisfiable-commit` needs to know: `git + // commit -F -` is a message source iff something is redirected into + // the SAME segment. + // + // `<<<` is a here-STRING and opens no body. Reading it as a heredoc + // starts a skip that never terminates, which would swallow the rest + // of the command — the same trap the bash guard's awk names. + '<' => { + input_redirect = true; + raw.push(c); + word.push(c); + has_word = true; + if chars.peek() == Some(&'<') { + chars.next(); + raw.push('<'); + word.push('<'); + if chars.peek() == Some(&'<') { + chars.next(); + raw.push('<'); + word.push('<'); + } else if let Some(delimiter) = + heredoc_delimiter(&mut chars, &mut raw, &mut word) + { + pending.push(delimiter); + } + } + } + // The newline that ENDS an opener line is where its bodies begin. + // Consuming them here, rather than scrubbing the string up front, + // is what lets the quote state above decide whether a `<<` was an + // opener at all: `echo "< { + raw.push(c); + if has_word { + words.push(std::mem::take(&mut word)); + has_word = false; + } + skip_heredoc_bodies(&mut chars, &mut pending); + } // An `&` belonging to a REDIRECTION is not a separator (CLOUD-443). // // `2>&1`, `>&2` and `&>log` all carry a literal `&` that says nothing @@ -5852,9 +5946,15 @@ fn segments(command: &str) -> Vec { words: std::mem::take(&mut words), raw: raw.trim().to_owned(), terminator: Some(separator), + input_redirect, }); } raw.clear(); + // The binding belongs to the segment that just closed. Carrying + // it forward is the exact defect the field exists to catch: + // `git commit -F - && mise run land <<'EOF'` would then read as + // if git had been given the heredoc. + input_redirect = false; } c if c.is_whitespace() => { raw.push(c); @@ -5881,11 +5981,103 @@ fn segments(command: &str) -> Vec { // status. `None` is what makes "alone in the call" — the prescribed // form — distinguishable from every shape that substitutes. terminator: None, + input_redirect, }); } out } +/// Read a heredoc delimiter off the front of `chars`, echoing what it consumes. +/// +/// Called with `<<` already consumed. Accepts the `<<-` tab-stripping form and +/// a delimiter in either quote style, which are the spellings that decide +/// whether the body is expanded — a distinction this parser does not care about, +/// since it drops the body either way. +/// +/// **Everything consumed is echoed into `raw` and `word`**, so the opener +/// survives in the segment exactly as written. That is what keeps `<<'EOF'` a +/// visible token rather than a hole, and it is why the caller does not also have +/// to remember what this ate. +/// +/// `None` where no delimiter word follows — `a << b` is an arithmetic shift or a +/// typo, and either way there is no body to skip. Reading one anyway would start +/// a skip that never terminates. +fn heredoc_delimiter( + chars: &mut std::iter::Peekable>, + raw: &mut String, + word: &mut String, +) -> Option { + let mut echo = |c: char| { + raw.push(c); + word.push(c); + }; + if chars.peek() == Some(&'-') { + chars.next(); + echo('-'); + } + while chars.peek().is_some_and(|c| *c == ' ' || *c == '\t') { + if let Some(c) = chars.next() { + echo(c); + } + } + let quote = match chars.peek() { + Some(&c @ ('\'' | '"')) => { + chars.next(); + echo(c); + Some(c) + } + _ => None, + }; + let mut delimiter = String::new(); + while let Some(&c) = chars.peek() { + if !(c.is_ascii_alphanumeric() || c == '_') { + break; + } + chars.next(); + echo(c); + delimiter.push(c); + } + if let Some(quote) = quote + && chars.peek() == Some("e) + { + chars.next(); + echo(quote); + } + (!delimiter.is_empty()).then_some(delimiter) +} + +/// Consume every pending heredoc body, leaving `chars` on the shell that follows. +/// +/// A line closes the front delimiter when it carries nothing but that word. +/// Trimmed rather than matched exactly, which is the reading both the bash +/// guard's awk and `policy/run-shape.rego` already take: `<<-` legitimately +/// indents its terminator, and being lenient here can only drop LESS text than +/// the shell would. +/// +/// An unterminated body runs to the end of the command, which is what bash does +/// with it — the alternative, treating the remainder as shell, is the CLOUD-723 +/// direction and is the one that produces a false refusal. +fn skip_heredoc_bodies( + chars: &mut std::iter::Peekable>, + pending: &mut Vec, +) { + let mut line = String::new(); + for c in chars.by_ref() { + if c != '\n' { + line.push(c); + continue; + } + if pending.first().is_some_and(|delim| line.trim() == delim) { + pending.remove(0); + if pending.is_empty() { + return; + } + } + line.clear(); + } + pending.clear(); +} + /// Do a row's operand words appear, adjacent and in order, in this command's /// words? /// @@ -7325,6 +7517,120 @@ mod tests { assert_eq!(parsed[0].words, ["rm", ".serena/memories/x.md"]); } + #[test] + fn a_heredoc_binds_to_the_element_that_writes_it() { + // THE MEASURED SHAPE (CLOUD-488, PR #375). The opener is present in the + // command STRING and absent from the element that needed it, so nothing + // short of a per-segment answer can tell this from the pair below. + let parsed = segments("git commit -F - && mise run land <<'EOF'\nmsg\nEOF\n"); + assert_eq!(parsed.len(), 2); + assert!( + !parsed[0].input_redirect, + "git got the harness's /dev/null: {:?}", + parsed[0] + ); + assert!(parsed[1].input_redirect, "`land` got the message"); + } + + #[test] + fn a_heredoc_in_the_same_element_binds_there() { + let parsed = segments("git commit -F - <<'EOF'\nmsg\nEOF\n"); + assert_eq!(parsed.len(), 1); + assert!(parsed[0].input_redirect); + assert_eq!(parsed[0].words, ["git", "commit", "-F", "-", "<<'EOF'"]); + } + + #[test] + fn every_spelling_of_an_input_redirection_binds() { + // One field for all three, because the predicate that reads it asks "did + // anything reach stdin here" and cannot be wrong about which. + for command in [ + "git commit -F - < msg.txt", + "git commit -F - < out.log") + .pop() + .unwrap() + .input_redirect + ); + } + + #[test] + fn a_heredoc_body_is_not_shell() { + // CLOUD-723, and it is a FALSE REFUSAL rather than a miss: every + // `pipeline` row decides over these segments, so a `;` in prose split + // the list and `verdict-not-discarded` refused correct commands — twice + // in one session, both times on the command documenting the rule. + let parsed = segments("cat > notes.md <<'EOF'\nfirst; then nohup x &\nEOF\n"); + assert_eq!(parsed.len(), 1, "the body carried `;` and `&`: {parsed:?}"); + assert_eq!(parsed[0].words, ["cat", ">", "notes.md", "<<'EOF'"]); + assert_eq!(parsed[0].terminator, None); + } + + #[test] + fn a_here_string_opens_no_body() { + // `<<<` read as a heredoc starts a skip that never terminates, which + // swallows the rest of the command — so the gate stops looking and the + // suite stays green. Everything after must still be judged. + let parsed = segments("echo x <<< \"$msg\" && git commit"); + assert_eq!(parsed.len(), 2, "the `&&` survived: {parsed:?}"); + assert_eq!(parsed[1].words, ["git", "commit"]); + } + + #[test] + fn a_quoted_heredoc_opener_is_not_one() { + // Decided by the SAME quote state the words are, which is why this walks + // the string once rather than scrubbing it first: a pre-pass has no + // quote state to consult and would skip to a delimiter that never comes. + let parsed = segments("echo \"< Result { "words": {"type": "array", "items": {"type": "string"}}, "raw": {"type": "string"}, "terminator": {"type": ["string", "null"]}, + "input-redirect": {"type": "boolean"}, }, }, }, diff --git a/crates/batten/tests/run_shape.rs b/crates/batten/tests/run_shape.rs index 781930be2..d7c3a3ac1 100644 --- a/crates/batten/tests/run_shape.rs +++ b/crates/batten/tests/run_shape.rs @@ -87,6 +87,9 @@ fn fixture(name: &str) -> PathBuf { "[[pattern]]\n", "id = \"short-message-flag-cluster\"\n", "regex = \"^-[A-Za-z]*[mFCc]\"\n\n", + "[[pattern]]\n", + "id = \"commit-message-file-flag\"\n", + "regex = \"^(-[A-Za-z]*F|--file)$\"\n\n", "[[verdict]]\n", "id = \"V-COMMIT-WITHOUT-A-MESSAGE-SOURCE\"\n", "gloss = \"a `git commit` names no message source, so git opens $EDITOR and blocks\"\n", @@ -98,7 +101,40 @@ fn fixture(name: &str) -> PathBuf { "[[verdict.route]]\n", "id = \"R-COMMIT-FROM-A-FILE\"\n", "kind = \"command\"\n", - "target = \"git commit -F \"\n", + "target = \"git commit -F \"\n\n", + "[[verdict]]\n", + "id = \"V-COMMIT-STDIN-UNBOUND\"\n", + "gloss = \"a `git commit -F -` has nothing redirected into the element it is written in\"\n", + "class = \"\"\"\n", + "The heredoc binds to the element that WRITES it, so git reads the harness's \\\n", + "/dev/null — after `pre-commit` has already spent the whole gate.\n", + "\"\"\"\n\n", + "[[verdict.route]]\n", + "id = \"R-COMMIT-FROM-A-FILE-THAT-CANNOT-REBIND\"\n", + "kind = \"command\"\n", + "target = \"git commit -F \"\n\n", + "[[verdict]]\n", + "id = \"V-FOREGROUND-SLEEP\"\n", + "gloss = \"a foreground `sleep` spends the session's own turn, and the call is killed at ~2 minutes\"\n", + "class = \"\"\"\n", + "A wait longer than about two minutes does not run slowly, it FAILS. Background \\\n", + "the work and act on its exit notification.\n", + "\"\"\"\n\n", + "[[verdict.route]]\n", + "id = \"R-WAIT-ON-THE-CONDITION\"\n", + "kind = \"command\"\n", + "target = \"until ; do sleep 1; done\"\n\n", + "[[verdict]]\n", + "id = \"V-BACKGROUND-TIMER\"\n", + "gloss = \"a backgrounded `sleep` with no loop around it is a timer, not a wait\"\n", + "class = \"\"\"\n", + "It exits when the clock says so, never when the thing being waited for happens. \\\n", + "The exit notification already fires.\n", + "\"\"\"\n\n", + "[[verdict.route]]\n", + "id = \"R-WAIT-ON-THE-CONDITION-NOT-THE-CLOCK\"\n", + "kind = \"command\"\n", + "target = \"until ; do sleep 1; done\"\n", ), ) .expect("write the fixture authority"); @@ -107,11 +143,30 @@ fn fixture(name: &str) -> PathBuf { } /// Hand `command` to the engine as a Claude Code `PreToolUse` envelope. +/// +/// No `run_in_background` key, which is the ordinary case rather than an +/// omission: most hosts send none, the engine projects `null`, and every +/// predicate here has to be correct about a host that said nothing. fn hook(root: &Path, command: &str) -> (bool, String) { + decide(root, command, None) +} + +/// The same, with the call's backgrounding stated (CLOUD-1094). +fn hook_background(root: &Path, command: &str, background: bool) -> (bool, String) { + decide(root, command, Some(background)) +} + +fn decide(root: &Path, command: &str, background: Option) -> (bool, String) { + let mut tool_input = serde_json::json!({"command": command}); + if let Some(flag) = background + && let Some(object) = tool_input.as_object_mut() + { + object.insert("run_in_background".to_owned(), flag.into()); + } let envelope = serde_json::json!({ "hook_event_name": "PreToolUse", "tool_name": "Bash", - "tool_input": {"command": command}, + "tool_input": tool_input, }) .to_string(); let output = common::run_with_stdin(root, &["hook", "--harness", "claude-code"], &envelope); @@ -138,6 +193,22 @@ fn allowed(root: &Path, command: &str) { assert!(!deny, "`{command}` should be allowed: {text}"); } +fn denied_background(root: &Path, command: &str, background: bool) { + let (deny, text) = hook_background(root, command, background); + assert!( + deny, + "`{command}` (background={background}) should be refused: {text}" + ); +} + +fn allowed_background(root: &Path, command: &str, background: bool) { + let (deny, text) = hook_background(root, command, background); + assert!( + !deny, + "`{command}` (background={background}) should be allowed: {text}" + ); +} + // --------------------------------------------------------------------------- // The predicate. // --------------------------------------------------------------------------- @@ -152,11 +223,19 @@ fn a_git_commit_naming_no_message_source_is_denied() { denied(&root, "git commit -a"); } -// carried: "every form that CAN obtain a message stays allowed" crates/batten/tests/run_shape.rs +// changed: "every form that CAN obtain a message stays allowed" crates/batten/tests/run_shape.rs a bare `git commit -F -` moved from this list to `a_commit_reading_unbound_stdin_is_refused`, because CLOUD-613 landed the predicate that tells the two apart — the retired case could not, so it asserted the weaker claim #[test] fn every_form_that_can_obtain_a_message_stays_allowed() { // The load-bearing half. A predicate that only ever denied would satisfy the // case above and be useless (CLOUD-418). + // + // WHAT LEFT THIS LIST, and why it is not a regression. `git commit -F -` was + // here because `-F` names a message source and the module could see nothing + // finer: heredoc binding is a property of the ELEMENT, and until CLOUD-613 + // the module had no element to ask. The bash guard has refused this exact + // string since 2026-08-12 (`run-shape-guard.sh:372-440`), so what changed is + // which authority answers, not the answer. `-F -` WITH a redirect bound to + // its own element is still allowed, below and in the module's own suite. let root = fixture("obtainable"); for command in [ "git commit -F /tmp/msg.txt", @@ -166,12 +245,190 @@ fn every_form_that_can_obtain_a_message_stays_allowed() { "git commit --fixup HEAD", "git commit -C HEAD@{1}", "git commit --message=hello", - "git commit -F -", ] { allowed(&root, command); } } +// --------------------------------------------------------------------------- +// CLOUD-613: heredoc BINDING, which needed a parser change to be askable at all. +// --------------------------------------------------------------------------- + +#[test] +fn a_commit_reading_unbound_stdin_is_refused() { + // THE MEASURED SHAPE (CLOUD-488, PR #375). The heredoc binds to the LAST + // element, so `mise run land` got the message and `git commit -F -` got the + // harness's /dev/null — about four minutes of gate on a commit git was + // always going to refuse, and killing it took `kill -9` on the process + // group. + // + // This is the case a command-string predicate cannot decide: the opener is + // PRESENT in the string and ABSENT from the element that needed it. + let root = fixture("stdin-unbound"); + denied( + &root, + "git add -A && git commit -F - && mise run land <<'EOF'\nmsg\nEOF\n", + ); + denied(&root, "git commit -F -"); + denied(&root, "git commit --file=-"); + denied(&root, "git commit --file -"); +} + +#[test] +fn a_redirect_bound_to_the_commits_own_element_is_a_message_source() { + // The discriminating half, and the pair above is the same four words in the + // same order — only the BINDING differs. All three spellings of `<`, because + // one test covers all three in the predicate and a suite that exercised one + // would not show that. + let root = fixture("stdin-bound"); + allowed(&root, "git commit -F - <<'EOF'\nmsg\nEOF\n"); + allowed(&root, "git commit -F - < /tmp/msg.txt"); + allowed(&root, "git commit -F - <<< \"$msg\""); + // A heredoc opened in an EARLIER element does not bind here either, which is + // the same rule read in the other direction. + allowed( + &root, + "cat <<'EOF' > /tmp/msg.txt\nmsg\nEOF\ngit commit -F /tmp/msg.txt", + ); +} + +#[test] +fn a_heredoc_body_is_not_shell() { + // CLOUD-723, the same parser change read in reverse. `verdict-not-discarded` + // and every `pipeline` row decide over these segments, so a body carrying a + // `;` used to split the list and turn a paragraph into its own command — + // measured twice in one session, both times on the command that was writing + // this rule down. + // + // The body here names `git commit` with no message source AND carries a list + // separator, so it would fire the first predicate in this file if it were + // read as shell at all. + let root = fixture("heredoc-prose"); + allowed( + &root, + "cat > notes.md <<'EOF'\nfirst; then git commit && nohup something &\nEOF\n", + ); + // `<<<` is a here-STRING and opens no body. Reading it as one starts a skip + // that never terminates, swallowing the rest of the command — so this + // `git commit` would VANISH rather than be judged, and the suite would go + // green on a gate that had stopped looking. The bash guard's awk carries the + // same `!/<<; do sleep 1; done` was allowed because no sleep resolved at all, +# so this conjunct decided nothing and read as coverage (CLOUD-1112). With the +# loop body reached, this is the only thing standing between that command and a +# refusal — which is what CLOUD-613's acceptance always claimed it was. +# +# `for` is NOT a wait. `for i in $(seq 60); do sleep 10; done` counts iterations +# rather than testing a condition, so it exits on the clock like any timer; the +# bash names it a deliberate non-catch "because narrowing that costs a real +# parser", and it costs none now. +waits_on_condition if { + some segment in input.call.segments + some word in segment.words + word in {"until", "while"} +} + +# `git commit`, resolved over WORDS the engine split rather than a string this +# module splits. Same rule as `git_commit` above and deliberately not shared with +# it: that one takes a stage string, and one function taking either would be a +# second parser wearing a signature. +# +# `git -C commit` resolves to the path and is NOT caught, which is the +# same deliberate false negative the bash carries — this repository commits from +# its own root, and a migration that silently fixed it would be changing the +# predicate rather than moving it. +git_commit_words(words) if { + idx := words_program_index(words) + basename(words[idx]) == "git" + subcommands := [w | + some i, w in words + i > idx + not startswith(w, "-") + not contains(w, ">") + not contains(w, "<") + ] + subcommands[0] == "commit" +} + +words_program_index(words) := idx if { + candidates := [i | + some i, w in words + not skippable(w) + ] + idx := candidates[0] +} + +# Does this segment tell git to read the message from STDIN? `-F -`, a cluster +# ending in F followed by `-`, `--file -`, or `--file=-`. +# +# The adjacency is the predicate: `-F` alone names a FILE and is fine, and it is +# only the `-` operand that makes stdin the source. `words[i + 1]` is undefined +# past the end, which Rego reads as *does not hold* — so a trailing `-F` allows. +names_stdin_as_the_source(words) if { + some i, w in words + regex.match(data.batten.patterns["commit-message-file-flag"], w) + words[i + 1] == "-" +} + +names_stdin_as_the_source(words) if { + some w in words + w == "--file=-" +} + # --------------------------------------------------------------------------- # Scrubbing: heredoc bodies, then quoted spans. Same order as the bash. +# +# EVERYTHING FROM HERE DOWN SERVES `commit-names-no-message-source` ALONE, and +# is the pre-`segments` era described in this file's header. Do not extend it. # --------------------------------------------------------------------------- lines := split(input.call.command, "\n") @@ -140,6 +308,26 @@ stages := [s | wrappers := {"env", "command", "nice", "stdbuf", "timeout", "xargs", "sudo", "doas", "nohup", "mise", "exec", "x"} +# SHELL KEYWORDS THAT INTRODUCE A COMMAND, looked through for the same reason +# every wrapper above is: what runs after them is the call being judged. +# +# `run-shape-guard.sh`'s `resolve()` has no such set, and CLOUD-1112 measured +# what that costs: `do sleep 1` resolved to the program `do`, so a sleep in a +# loop body was invisible — and `waits_on_condition` therefore exempted nothing, +# because the canonical `until ; do sleep 1; done` was already allowed for +# want of a resolvable sleep rather than for being a wait. The guard's own +# comment claims the opposite ("the one carrying the sleep has no keyword in +# it"), which only parses if that element IS reached. +# +# CLOUD-613's acceptance turns on that allow being LOAD-BEARING, so porting the +# gap would have satisfied the clause vacuously. This is the narrower reading: +# the engine resolves the loop body, and the exemption is what decides it. +# +# `until`/`while`/`if`/`for` are deliberately ABSENT. They introduce a condition +# list rather than the command, and `waits_on_condition` reads them as words — +# skipping them would blind the exemption to the thing it tests for. +keywords := {"do", "then", "else", "elif", "time"} + tokens(stage) := [t | some t in split(trim_space(stage), " "); t != ""] # The index the program sits at: the first token that is not something a @@ -160,6 +348,7 @@ program_index(stage) := idx if { skippable(tok) if { some answer in [ tok in wrappers, + tok in keywords, startswith(tok, "-"), contains(tok, "="), contains(tok, "@"), @@ -251,3 +440,186 @@ test_a_non_cluster_carrying_m_is_not_a_message_source if { some v in violation with input as {"call": {"command": "git commit -x=mfoo"}} v.rule == "commit-names-no-message-source" } + +# --------------------------------------------------------------------------- +# CLOUD-613's three. Every case carries a `command` as well as `segments`, +# because the first predicate reads the string and would otherwise fire on an +# undefined path and take these cases with it. +# --------------------------------------------------------------------------- + +seg(words, terminator, redirect) := { + "words": words, + "raw": concat(" ", words), + "terminator": terminator, + "input-redirect": redirect, +} + +# THE MEASURED SHAPE (CLOUD-488): the heredoc binds to the LAST element, so +# `land` gets the message and git gets /dev/null. +test_a_commit_whose_heredoc_binds_to_a_later_element_is_refused if { + some v in violation with input as {"call": { + "command": "git commit -F - && mise run land <<'EOF'", + "run-in-background": null, + "segments": [ + seg(["git", "commit", "-F", "-"], "&&", false), + seg(["mise", "run", "land", "<<'EOF'"], null, true), + ], + }} + v.rule == "unsatisfiable-commit" +} + +# THE DISCRIMINATING ALLOW, and it is the same two words in the same order — +# only the BINDING differs. A predicate reading the command string sees one +# string for both of these. +test_a_heredoc_bound_to_this_element_is_a_message_source if { + count(violation) == 0 with input as {"call": { + "command": "git commit -F - <<'EOF'", + "run-in-background": null, + "segments": [seg(["git", "commit", "-F", "-", "<<'EOF'"], null, true)], + }} +} + +test_a_file_redirect_is_a_message_source_too if { + count(violation) == 0 with input as {"call": { + "command": "git commit -F - < msg.txt", + "run-in-background": null, + "segments": [seg(["git", "commit", "-F", "-", "<", "msg.txt"], null, true)], + }} +} + +# `-F` naming a FILE is not stdin at all: the `-` operand is the predicate. +test_a_commit_reading_a_named_file_is_untouched if { + count(violation) == 0 with input as {"call": { + "command": "git commit -F /tmp/msg.txt", + "run-in-background": null, + "segments": [seg(["git", "commit", "-F", "/tmp/msg.txt"], null, false)], + }} +} + +test_the_long_flag_spelling_is_judged_too if { + some v in violation with input as {"call": { + "command": "git commit --file=-", + "run-in-background": null, + "segments": [seg(["git", "commit", "--file=-"], null, false)], + }} + v.rule == "unsatisfiable-commit" +} + +test_a_foreground_sleep_is_refused if { + some v in violation with input as {"call": { + "command": "sleep 90", + "run-in-background": null, + "segments": [seg(["sleep", "90"], null, false)], + }} + v.rule == "foreground-sleep" +} + +test_a_sleep_in_a_later_segment_is_refused_too if { + some v in violation with input as {"call": { + "command": "cd /tmp; sleep 90; git log", + "run-in-background": false, + "segments": [ + seg(["cd", "/tmp"], ";", false), + seg(["sleep", "90"], ";", false), + seg(["git", "log"], null, false), + ], + }} + v.rule == "foreground-sleep" +} + +test_a_backgrounded_bare_sleep_is_a_timer if { + some v in violation with input as {"call": { + "command": "sleep 590; tail -6 land.log", + "run-in-background": true, + "segments": [ + seg(["sleep", "590"], ";", false), + seg(["tail", "-6", "land.log"], null, false), + ], + }} + v.rule == "background-timer" +} + +# THE ALLOW THAT MATTERS. This is the form both refusals recommend, and denying +# it is what would get the rule switched off. +test_a_backgrounded_wait_on_a_condition_is_allowed if { + count(violation) == 0 with input as {"call": { + "command": "until [ -f /tmp/done ]; do sleep 1; done", + "run-in-background": true, + "segments": [ + seg(["until", "[", "-f", "/tmp/done", "]"], ";", false), + seg(["do", "sleep", "1"], ";", false), + seg(["done"], null, false), + ], + }} +} + +# A FOREGROUND loop spends the turn exactly as a foreground `sleep` does, and it +# is refused for that reason. Reaching it needs `keywords`: without the +# look-through `do sleep 1` resolves to `do` and this passes silently, which is +# how it stood in the bash (CLOUD-1112). +test_a_foreground_wait_on_a_condition_is_refused if { + some v in violation with input as {"call": { + "command": "until [ -f /tmp/done ]; do sleep 1; done", + "run-in-background": false, + "segments": [ + seg(["until", "[", "-f", "/tmp/done", "]"], ";", false), + seg(["do", "sleep", "1"], ";", false), + seg(["done"], null, false), + ], + }} + v.rule == "foreground-sleep" +} + +# A `for` LOOP IS A TIMER: it counts iterations rather than testing a condition, +# so it exits on the clock. Backgrounded, that is the shape CLOUD-821 measured. +test_a_backgrounded_counting_loop_is_a_timer if { + some v in violation with input as {"call": { + "command": "for i in $(seq 60); do sleep 10; done", + "run-in-background": true, + "segments": [ + seg(["for", "i", "in", "$(seq", "60)"], ";", false), + seg(["do", "sleep", "10"], ";", false), + seg(["done"], null, false), + ], + }} + v.rule == "background-timer" +} + +# The exemption's other reachable shape: a bare sleep and a loop keyword in one +# backgrounded call, where the sleep resolves without any look-through at all. +test_a_bare_sleep_beside_a_condition_loop_is_exempt if { + count(violation) == 0 with input as {"call": { + "command": "sleep 5; until [ -f /tmp/done ]; do :; done", + "run-in-background": true, + "segments": [ + seg(["sleep", "5"], ";", false), + seg(["until", "[", "-f", "/tmp/done", "]"], ";", false), + seg(["do", ":"], ";", false), + seg(["done"], null, false), + ], + }} +} + +# THE DISCRIMINATING CASE for `run-in-background`: both rules deny, so only the +# verdict tells them apart. A `foreground-sleep` that ignored the flag would +# raise TWO violations here. +test_a_backgrounded_bare_sleep_raises_only_the_timer if { + count(violation) == 1 with input as {"call": { + "command": "sleep 590; tail -6 land.log", + "run-in-background": true, + "segments": [ + seg(["sleep", "590"], ";", false), + seg(["tail", "-6", "land.log"], null, false), + ], + }} +} + +# THE ANCHORING CASE. `sleep` as an ARGUMENT is not an invocation, and a +# predicate scanning words rather than resolving the program refuses this. +test_a_mention_of_sleep_is_not_a_call if { + count(violation) == 0 with input as {"call": { + "command": "echo sleep 90", + "run-in-background": false, + "segments": [seg(["echo", "sleep", "90"], null, false)], + }} +} diff --git a/schema/policy-call.schema.json b/schema/policy-call.schema.json index da07fc428..6357bd94f 100644 --- a/schema/policy-call.schema.json +++ b/schema/policy-call.schema.json @@ -26,6 +26,9 @@ "items": { "additionalProperties": false, "properties": { + "input-redirect": { + "type": "boolean" + }, "raw": { "type": "string" },