diff --git a/crates/batten/src/facts.rs b/crates/batten/src/facts.rs index f4dbea85e..017e8e016 100644 --- a/crates/batten/src/facts.rs +++ b/crates/batten/src/facts.rs @@ -1536,58 +1536,180 @@ pub fn sourced(record: Option<&Sourced>, asked_for: &str) -> Look { } } -/// Reduce a tool result buffer to a row count, or [`Look::CouldNotLook`] if its -/// shape is not one this build recognises. -/// -/// The buffer's shape is per-tool and only partly surveyed: an MCP tool returns a -/// content-block array (measured — `tests/board-write-record.bats`), and a shell -/// tool returns something this repository has not measured. **Answering `0` for a -/// shape this build cannot read would be a guessed envelope becoming a silent -/// fact**, which is the failure the capability table exists to prevent, so an -/// unrecognised shape is could-not-look instead. -/// -/// Two shapes are read, both with evidence in this tree: a **bare row array**, -/// and the **content-block envelope**, whose text blocks are counted where they -/// parse as JSON arrays — the same `fromjson?` posture `board-write-record` takes -/// over the same bytes, so a block that is not JSON is skipped rather than -/// aborting the read. +/// Reduce a tool result buffer to a row count by **normalising it to an array**, +/// or [`Look::CouldNotLook`] where no count can honestly be given. +/// +/// # Why normalise rather than refuse (CLOUD-992) +/// +/// This function used to read exactly two shapes — a bare row array and the +/// content-block envelope — and answer could-not-look for everything else. The +/// reasoning was sound and is preserved below: **answering `0` for a buffer this +/// build cannot read would be a guessed envelope becoming a silent fact.** +/// +/// What was wrong is that refusing is not the only way to avoid that, and it made +/// the channel **unusable from any shell tool**. Measured on real Bash responses: +/// every one is raw text, so the first match arm rejected it. The channel's first +/// intended consumer declares a `gh` command, so it would have seen could-not-look +/// on every call — a gate refusing every `gh pr ready`, unsatisfiable by running +/// the very command its deny prints. +/// +/// The alternative considered and rejected was making each declared command +/// project an array (`gh … --jq '[…]'`). That puts the obligation on every future +/// fact row, fails as could-not-look rather than loudly when forgotten, and is +/// invisible in the rule that consumes it. +/// +/// # The table +/// +/// | buffer | rows | +/// | -- | -- | +/// | JSON array that is not wholly content blocks | its length — an empty one is a genuine zero | +/// | content-block envelope (EVERY item a content block) | the sum over its blocks, each normalised by the rules below — could-not-look if ANY block says nothing | +/// | a shell tool's envelope object (`{stdout, stderr}`) | its stream text, normalised by the rules below — the buffer is a MEMBER, never the object | +/// | any other JSON object, or any non-string scalar | `1` — one element, wrapped | +/// | text that parses as a JSON array | that array's length | +/// | text that parses as any other JSON value | `1` | +/// | text that is not JSON at all | `1` — one opaque row | +/// | text that is empty or whitespace | could-not-look — see below | +/// | [`serde_json::Value::Null`] | could-not-look — an absent buffer is not a reading | +/// +/// **The invariant that must not regress: no unread shape is ever reported as +/// `0`.** Wrapping preserves it exactly as refusing did — an opaque buffer counts +/// as one row, never none — so a `rows == 0` predicate stays fail-closed. +/// +/// **The empty buffer is the one place the three-valued reading survives, and it +/// earns it.** A command that failed and printed nothing is indistinguishable +/// from one that legitimately found nothing, so `0` and `1` are both guesses: +/// `0` would let an unreviewed head through, `1` would deny a gate forever. Only +/// could-not-look states what is actually known. +/// +/// A tool whose output cannot be parsed as JSON wants an **adapter** rather than +/// this fallback — the one-opaque-row reading keeps a gate fail-closed in the +/// meantime and is inventoried as debt (CLOUD-993), not left as the design. /// /// No byte of the buffer is returned (rule 4). The count is the whole answer. #[must_use] pub fn rows_in(result: &serde_json::Value) -> Look { - let serde_json::Value::Array(items) = result else { - return Look::CouldNotLook; - }; - let blocks: Vec<&serde_json::Value> = items.iter().filter(|item| is_text_block(item)).collect(); - if blocks.is_empty() { - // A bare array of rows. An EMPTY array reaches here too and is a genuine - // zero — it is a shape we read that carried nothing, not a shape we - // failed to read. - return Look::Is(items.len()); - } - let mut rows = 0; - let mut parsed_any = false; - for text in blocks - .iter() - .filter_map(|block| block.get("text").and_then(serde_json::Value::as_str)) - { - if let Ok(serde_json::Value::Array(inner)) = serde_json::from_str::(text) - { - rows += inner.len(); - parsed_any = true; + match result { + // An absent buffer is not a reading. Distinct from an empty one, which + // at least says a tool answered. + serde_json::Value::Null => Look::CouldNotLook, + serde_json::Value::Array(items) => { + // ENVELOPE SEMANTICS DEMAND THE WHOLE ARRAY, NOT A MEMBER OF IT. A + // row array may legitimately contain a row that happens to be + // text-shaped, and treating that array as an envelope reads the one + // block and DROPS every other row: `[{"type":"text","text":"[]"}, + // {"id":1}]` answered `Is(0)` for two rows, breaking the fail-closed + // invariant this function exists to hold. So an array is an envelope + // only when every item is a content block; anything else is a bare + // row array and counts its items. + // + // An EMPTY array lands here too and is a genuine zero — a shape we + // read that carried nothing, not a shape we failed to read. + if items.is_empty() || !items.iter().all(is_text_block) { + return Look::Is(items.len()); + } + // The content-block envelope an MCP tool returns. Each block's text + // goes through the same normalisation a bare text buffer does, so the + // two entry points cannot disagree about one string. + // NO BLOCK IS SKIPPED, and that is the whole of this loop's contract. + // `is_text_block` guarantees a string `text`, so nothing can be + // silently filtered out here — a `filter_map` over the field would + // drop a block it could not read and then report the SUM OF THE REST + // as the buffer's count, which is how a partly-unreadable envelope + // came back `Is(0)`. + // + // And one unreadable block condemns the whole envelope. A block + // saying nothing contributes no knowable count, so folding it in as + // `0` would be a guess presented as a reading; could-not-look is the + // only honest answer for the buffer as a whole. That is also what + // keeps the single-empty-block envelope could-not-look, as it has + // always been. + let mut rows = 0; + for block in items { + let Some(text) = block.get("text").and_then(serde_json::Value::as_str) else { + return Look::CouldNotLook; + }; + let Some(count) = rows_in_text(text) else { + return Look::CouldNotLook; + }; + rows += count; + } + Look::Is(rows) } + serde_json::Value::String(text) => rows_in_text(text).map_or(Look::CouldNotLook, Look::Is), + // A SHELL TOOL'S BUFFER IS A MEMBER OF THIS OBJECT, NEVER THE OBJECT. + // This is the arm the whole capability turned on and the one a shape + // table could not see: Claude Code hands a Bash call's response back as + // `{stdout, stderr, …}`, so counting the object gives `1` for every + // shell command ever declared — measured, `printf '[1,2,3]'` recorded + // `rows 1`. Normalising buffers was necessary and not sufficient, + // because the buffer never arrived as one. + // + // `capture::decode_response` is the ONE authority on that shape. It + // already reads `stdout` then `stderr` in a declared order, already + // says so against the measured corpus, and is already the reader the + // capture store trusts. A second copy of the field list here is the + // "two copies drifted" failure this repository keeps recording — so + // this defers to it rather than restating it. + serde_json::Value::Object(_) => match crate::capture::decode_response(result) { + // A shell envelope: count what the tool actually printed. + Ok(decoded) if decoded.blocks > 0 => match std::str::from_utf8(&decoded.bytes) { + Ok(text) => rows_in_text(text).map_or(Look::CouldNotLook, Look::Is), + // Bytes that are not text are a shape this build did not read, + // never a buffer that carried nothing. + Err(_) => Look::CouldNotLook, + }, + // An object carrying no readable stream member is not an envelope at + // all — it is a single JSON row, which is one element wrapped. + Ok(_) | Err(_) => Look::Is(1), + }, + // A single number or boolean is one element. Wrapping it is what makes + // the count obvious rather than making the caller project. + serde_json::Value::Number(_) | serde_json::Value::Bool(_) => Look::Is(1), } - // Nothing parsed is NOT "zero rows": it is a shape this build did not - // understand, which is could not look. - if parsed_any { - Look::Is(rows) - } else { - Look::CouldNotLook +} + +/// How many rows one text buffer carries, by the table on [`rows_in`], or +/// `None` when it says nothing at all. +/// +/// Split out because the bare-string arm and every text block inside a +/// content-block envelope must answer identically — a second copy of this rule +/// is how the two entry points come to disagree about the same string. +/// +/// `Option` rather than [`Look`]: a count has only two outcomes +/// here, and `Look`'s third — [`Look::IsNot`] — is not one of them. Returning +/// `Look` would put an arm on every caller that no input can reach, and a +/// wildcard over it is exactly how a future third outcome would be silently +/// folded into "nothing to add". +fn rows_in_text(text: &str) -> Option { + if text.trim().is_empty() { + // Nothing was said. Neither `0` nor `1` is knowable here; see the doc. + return None; + } + match serde_json::from_str::(text) { + Ok(serde_json::Value::Array(inner)) => Some(inner.len()), + // Any other JSON value is one element; prose that does not parse is one + // opaque row. Both are "we saw something", which is what matters. + Ok(_) | Err(_) => Some(1), } } +/// Whether one array item is a content block this build can read in full. +/// +/// **A STRING `text` IS PART OF THE SHAPE, not a detail the reader checks +/// later.** Keying only on `type` admitted `{"type":"text","text":7}` as a +/// block, and the envelope loop then had to cope with a block it could not +/// read — which it did by skipping it, so +/// `[{"type":"text","text":"[]"},{"type":"text","text":7}]` answered `Is(0)` +/// for a buffer half of which was never read. +/// +/// Requiring the string here fixes it in the one place that decides, and it +/// decides in the more useful direction: a malformed block is not a content +/// block, so its array is not an envelope at all and counts as ordinary rows. +/// Two rows is what that buffer carries, and saying so beats refusing it. fn is_text_block(value: &serde_json::Value) -> bool { value.get("type").and_then(serde_json::Value::as_str) == Some("text") + && value.get("text").is_some_and(serde_json::Value::is_string) } /// One agent-sourced fact a consumer declares: its name, and the command whose diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 044bb7be0..d9acf5c0d 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -3012,10 +3012,13 @@ fn drain_advisories( /// reason work stops: the next attempt denies again with the same `Fix::Run`, /// which is the safe direction and one the agent can see. fn record_agent_fact(overrides: &Overrides, envelope: &hook::Envelope) { - // An unrecognised buffer shape is `CouldNotLook` and records NOTHING. Writing - // a zero here would turn a shape this build cannot read into the fact "there - // are none", which is the guessed-envelope failure the whole capability table - // exists to prevent. + // A buffer that said nothing at all — absent, or empty — is `CouldNotLook` + // and records NOTHING. Writing a zero here would turn "the tool printed + // nothing" into the fact "there are none", which is the guessed-envelope + // failure the whole capability table exists to prevent. Every buffer that + // DID say something is a count now (CLOUD-992), down to one opaque row for + // prose that is not JSON, so the declared command no longer has to project + // its own output into an array to be readable here. let facts::Look::Is(rows) = facts::rows_in(&envelope.result) else { return; }; diff --git a/crates/batten/tests/agent_facts.rs b/crates/batten/tests/agent_facts.rs index c4a5e8a75..96847b7cf 100644 --- a/crates/batten/tests/agent_facts.rs +++ b/crates/batten/tests/agent_facts.rs @@ -136,22 +136,187 @@ fn no_byte_of_the_buffer_reaches_the_stored_record() { } #[test] -fn an_unrecognised_buffer_shape_is_could_not_look_rather_than_zero() { - // The shape is per-tool and only partly surveyed. Answering `Is(0)` for a - // shape this build does not read would be a guessed envelope becoming a - // silent fact — the failure the capability table exists to prevent. - for unreadable in [ - serde_json::Value::Null, +fn no_buffer_is_ever_reported_as_zero_unless_it_really_carried_nothing() { + // THE INVARIANT, and the reason the refusal this test used to assert could be + // replaced (CLOUD-992). Answering `Is(0)` for a buffer this build cannot + // decompose would be a guessed envelope becoming a silent fact. Normalising + // to an array preserves that just as refusing did: an opaque buffer is one + // row, never none, so a `rows == 0` predicate stays fail-closed. + for wrapped in [ serde_json::json!({ "stdout": "whatever" }), serde_json::json!("a string"), - // A block envelope whose text is not JSON parses nothing, which is not - // "zero rows" either. + serde_json::json!(7), + serde_json::json!(true), serde_json::json!([{ "type": "text", "text": "not json" }]), ] { assert_eq!( - facts::rows_in(&unreadable), + facts::rows_in(&wrapped), + Look::Is(1), + "{wrapped} carries one opaque row, and must never read as zero" + ); + } +} + +#[test] +fn only_an_absent_or_empty_buffer_is_could_not_look() { + // `Null` is no buffer at all. An empty or whitespace-only one is the one + // place the three-valued reading survives and earns it: a command that + // failed silently and one that legitimately found nothing are + // indistinguishable, so `0` would let an unreviewed head through and `1` + // would deny a gate forever. Only could-not-look states what is known. + for unknowable in [ + serde_json::Value::Null, + serde_json::json!(""), + serde_json::json!(" \n\t "), + serde_json::json!([{ "type": "text", "text": "" }]), + ] { + assert_eq!( + facts::rows_in(&unknowable), + Look::CouldNotLook, + "{unknowable} says nothing, and a count would be a guess" + ); + } +} + +#[test] +fn a_shell_buffer_carrying_json_is_counted_without_the_command_projecting_it() { + // The whole point of CLOUD-992: a `gh … --json` buffer arrives as TEXT, and + // the engine parses it rather than obliging every declared command to append + // `--jq '[…]'`. Real lengths, not 1, or the suite would pass over a change + // that made everything read as one row. + assert_eq!( + facts::rows_in(&serde_json::json!("[{\"n\":1},{\"n\":2},{\"n\":3}]")), + Look::Is(3) + ); + // An empty JSON array in text is a genuine zero — the reading a review gate + // needs for "reviewed and addressed". + assert_eq!(facts::rows_in(&serde_json::json!("[]")), Look::Is(0)); + // A single JSON object in text is one element, wrapped. + assert_eq!(facts::rows_in(&serde_json::json!("{\"n\":1}")), Look::Is(1)); + // And an envelope AGGREGATES its blocks rather than reading the first: two + // arrays of two and one sum to three. Without this the loop could return any + // single block's count and the suite would not notice. + assert_eq!( + facts::rows_in(&serde_json::json!([ + { "type": "text", "text": "[{\"n\":1},{\"n\":2}]" }, + { "type": "text", "text": "[{\"n\":3}]" } + ])), + Look::Is(3) + ); +} + +#[test] +fn a_mixed_array_is_a_row_array_and_never_an_envelope() { + // THE INVARIANT'S OWN COUNTEREXAMPLE (CodeRabbit on PR #672, confirmed). + // Envelope semantics used to be selected by ANY item being a content block, + // which read the one block and dropped every other row — so an array + // carrying two rows answered `Is(0)`, the exact fail-closed collapse this + // function exists to prevent, produced by the code meant to prevent it. + // + // A row that merely LOOKS like a content block is an ordinary row: a tool + // emitting `{"type":"text", …}` records is not thereby emitting an envelope. + assert_eq!( + facts::rows_in(&serde_json::json!([ + { "type": "text", "text": "[]" }, + { "id": 1 } + ])), + Look::Is(2), + "an array whose items are not ALL content blocks counts its items" + ); + // The same shape with a genuine zero-row envelope beside it: still two rows, + // because the array is not an envelope at all. + assert_eq!( + facts::rows_in(&serde_json::json!([{ "id": 1 }, { "type": "text", "text": "[]" }])), + Look::Is(2) + ); + // A one-item array that IS wholly content blocks keeps envelope semantics — + // the narrowing must not cost the shape an MCP tool actually returns. + assert_eq!( + facts::rows_in(&serde_json::json!([{ "type": "text", "text": "[]" }])), + Look::Is(0) + ); + // A `text` that is not a STRING is not a content block, so its array is not + // an envelope. Keying the shape on `type` alone admitted this block and the + // loop then skipped it, answering `Is(0)` for a buffer half of which was + // never read (CodeRabbit on PR #672, confirmed). + assert_eq!( + facts::rows_in(&serde_json::json!([ + { "type": "text", "text": "[]" }, + { "type": "text", "text": 7 } + ])), + Look::Is(2), + "a malformed block is an ordinary row, and two rows is what this carries" + ); + // Likewise a block with no `text` field at all. + assert_eq!( + facts::rows_in(&serde_json::json!([ + { "type": "text", "text": "[]" }, + { "type": "text" } + ])), + Look::Is(2) + ); +} + +#[test] +fn a_shell_tools_buffer_is_a_member_of_its_envelope_and_is_counted_there() { + // THE ARM CLOUD-992 EXISTS FOR, and the one normalising buffers did not + // reach. Claude Code hands a Bash call's response back as an OBJECT — + // `capture::decode_response` states that shape against the measured corpus + // — so `rows_in` never sees the stdout text as a buffer. Counting the + // object gave `1` for every shell command ever declared. + // + // MEASURED, not reasoned: with a `[[fact]]` row declaring + // `printf '[1,2,3]\n'`, the record written by the real hook read `rows 1`. + // That is the whole capability failing, and no buffer-shaped test could see + // it, because the buffer was never the value under test. + assert_eq!( + facts::rows_in(&serde_json::json!({ "stdout": "[1,2,3]\n", "stderr": "" })), + Look::Is(3), + "the count is what the command printed, not the envelope wrapping it" + ); + // The reading a review gate needs: an empty JSON array on stdout is a + // genuine zero, which is what makes `rows == 0` mean "reviewed and clear". + assert_eq!( + facts::rows_in(&serde_json::json!({ "stdout": "[]", "stderr": "" })), + Look::Is(0) + ); + // Prose on stdout is one opaque row — fail-closed, never zero. + assert_eq!( + facts::rows_in(&serde_json::json!({ "stdout": "gh version 2.97.0\n" })), + Look::Is(1) + ); + // A command that printed nothing at all is could-not-look, exactly as an + // empty bare buffer is. `0` here would let an unreviewed head through. + assert_eq!( + facts::rows_in(&serde_json::json!({ "stdout": "", "stderr": "" })), + Look::CouldNotLook + ); + // An object with no stream member is NOT an envelope — it is a single JSON + // row, and stays one element wrapped. + assert_eq!(facts::rows_in(&serde_json::json!({ "n": 1 })), Look::Is(1)); +} + +#[test] +fn one_unreadable_block_condemns_the_whole_envelope() { + // The sibling of the shape check above, on the axis `is_text_block` cannot + // reach: every item IS a well-formed content block, and one of them says + // nothing. Folding that in as `0` would be a guess presented as a reading, + // so the buffer as a whole is could-not-look — never the sum of the rest, + // which is what silently under-counted before. + for envelope in [ + serde_json::json!([ + { "type": "text", "text": "[{\"n\":1},{\"n\":2},{\"n\":3}]" }, + { "type": "text", "text": "" } + ]), + serde_json::json!([ + { "type": "text", "text": " \n\t " }, + { "type": "text", "text": "[{\"n\":1}]" } + ]), + ] { + assert_eq!( + facts::rows_in(&envelope), Look::CouldNotLook, - "{unreadable} is not a shape this build reads" + "{envelope} carries a block that said nothing, so its total is unknown" ); } }