diff --git a/crates/batten/src/config.rs b/crates/batten/src/config.rs index aaea17f54..a5107ea3d 100644 --- a/crates/batten/src/config.rs +++ b/crates/batten/src/config.rs @@ -2465,9 +2465,26 @@ impl<'a> Header<'a> { self.name().split('.').next().unwrap_or(self.name()) } - /// Whether this header is a dotless `[[name]]` — the only droppable shape. + /// Whether this header is a `[[name]]` — an array-of-tables element. + /// + /// **Dotted or not (CLOUD-1775).** This asked `!name.contains('.')` and so + /// equated *droppable* with *dotless*, which is true of one dotted shape and + /// false of the other. `[[hook.handler]]` is an array under a plain table, + /// addressable as the path `hook.handler` and droppable a row at a time; + /// `[[provision.env]]` is an array inside the last `[[provision]]` ELEMENT, + /// where the droppable unit is that element. Conflating them cost the whole + /// file: `owning_row` declined the handler, so the row arm never ran, and + /// `drop_key` then looked for a top-level key spelled `hook.handler` and found + /// `hook`, so the key arm declined too. A `[[hook.handler]]` is where the + /// stale-binary detector lives, so the first schema addition a running binary + /// predates unregistered the detector for exactly that staleness. + /// + /// Which of the two shapes a dotted row is cannot be read off the header + /// alone — it is a fact about the DOCUMENT, namely whether the root is itself + /// an array row — so that question lives in [`owning_row`], which has the text. + //MUTANT dotted-row-not-droppable|s@ matches!(self, Self::Row(_))@ matches!(self, Self::Row(name) if !name.contains('.'))@|an_unknown_key_in_a_dotted_row_costs_the_row_not_the_file fn is_row(self) -> bool { - matches!(self, Self::Row(name) if !name.contains('.')) + matches!(self, Self::Row(_)) } } @@ -2808,6 +2825,87 @@ fn key_extent(text: &str, at: usize) -> (usize, usize) { /// counts this section's rows as the author wrote them, so two rows dropped in /// one load still report distinct pointers. Reading it off the working copy is /// how two `[[verb]]` rows both reported `#0`, caught in review. +/// Walk a dotted path of plain tables, from `table` down. +/// +/// `None` the moment a segment is missing or is not a table, which is the +/// fail-closed direction: the caller abandons the drop rather than guessing +/// where the row it is charging actually lives. +fn table_at<'a>(table: &'a mut toml::Table, path: &str) -> Option<&'a mut toml::Table> { + let mut at = table; + for segment in path.split('.') { + at = at.get_mut(segment)?.as_table_mut()?; + } + Some(at) +} + +/// Remove every table along `path` that the drop above left empty, root-ward. +/// +/// A file whose only `hook` headers were `[[hook.handler]]` rows parses, once +/// they are all blanked, to a document with no `hook` key at all — not to +/// `hook = {}`. So the husk goes too, and its own holder is asked the same +/// question in turn. Leaving one behind would fail [`prune_unresolvable`]'s +/// exactness guard and abandon a prune that was otherwise right. +/// +/// It stops at the first table that is NOT empty, which is the whole of its +/// caution: a section that still holds something the author wrote is never +/// removed because a sibling array emptied. +fn drop_empty_ancestors(table: &mut toml::Table, path: Option<&str>) { + let mut path = path; + while let Some(above) = path { + let (next, key) = above + .rsplit_once('.') + .map_or((None, above), |(next, key)| (Some(next), key)); + let emptied = match next { + Some(next) => table_at(table, next), + None => Some(&mut *table), + } + .filter(|holder| { + holder + .get(key) + .and_then(toml::Value::as_table) + .is_some_and(toml::Table::is_empty) + }); + let Some(emptied) = emptied else { break }; + emptied.remove(key); + path = next; + } +} + +/// Whether `header` names a row that can be addressed — and therefore dropped — +/// as a path from the top-level table. +/// +/// A dotless `[[name]]` always can. A dotted one can only when its root is NOT +/// itself an array row in this document: `[[hook.handler]]` sits under a plain +/// `hook` table and is reachable as `hook.handler`, while `[[provision.env]]` +/// sits inside whichever `[[provision]]` ELEMENT precedes it, and no path from +/// the root names it — the element that holds it is the unit, which is what the +/// caller's walk resolves to. +/// +/// **The answer is a property of the document, not of the header**, which is why +/// [`Header::is_row`] cannot decide it. It is also the fail-closed direction: a +/// root that is a row anywhere makes the dotted header un-droppable and the +/// caller falls back to the owning element, so a misread here costs a wider unit +/// rather than a wrong blank. +//MUTANT every-dotted-row-droppable|s@ !nests_in_a_row(text, header.root())@ true@|a_dotted_row_nested_in_an_array_element_still_charges_its_owner +fn addressable_row(text: &str, header: Header<'_>) -> bool { + if !header.is_row() { + return false; + } + if !header.name().contains('.') { + return true; + } + !nests_in_a_row(text, header.root()) +} + +/// Whether `root` is itself declared as an array-of-tables anywhere in `text`. +/// +/// Named rather than inlined for the `//MUTANT` row above's reason: a mutation +/// row is `id|script|case` split on `|`, so an anchor carrying a closure yields +/// five fields and the declaration can only ever report `malformed-row`. +fn nests_in_a_row(text: &str, root: &str) -> bool { + headers(text).any(|(_, other)| other.is_row() && other.name() == root) +} + fn owning_row(text: &str, at: usize) -> Option<(String, usize, usize)> { // The nearest header at or before `at`, then back through any dotted // headers it nests under. @@ -2817,7 +2915,7 @@ fn owning_row(text: &str, at: usize) -> Option<(String, usize, usize)> { if offset > at { continue; } - if header.is_row() && want.is_none_or(|root| root == header.name()) { + if addressable_row(text, header) && want.is_none_or(|root| root == header.name()) { found = Some((offset, header.name())); break; } @@ -2988,6 +3086,13 @@ fn drop_key( // A key under a `[[row]]` header that `owning_row` declined for some other // reason is not this arm's business: dropping one key out of a row would // leave that row enforcing something nobody wrote. + // + // DOTTED OR NOT (CLOUD-1775). This guard read `is_row` when that predicate + // meant *dotless* row, so a key inside a `[[hook.handler]]` reached the + // paragraph below and asked the top-level table for a key spelled + // `hook.handler` — which decided nothing, since the answer was `Declined` + // either way. It is a guard rather than an accident now: the row arm owns + // every `[[name]]`, and key granularity never reaches one. if header.is_row() { return KeyDrop::Declined; } @@ -3197,10 +3302,24 @@ fn prune_unresolvable(source: &str, behind: bool }) .count(); let mut expected = current.clone(); - let Some(rows) = expected - .get_mut(§ion) - .and_then(toml::Value::as_array_mut) - else { + // A SECTION IS A PATH, NOT A KEY (CLOUD-1775). `hook.handler` is a key + // `handler` inside the table `hook`; asking the top-level table for it + // finds nothing, which is half of why the handler rows fell through both + // granularities. A dotless section still resolves in one step, so this is + // the same lookup for every row that already worked. + let (holder_path, leaf) = section + .rsplit_once('.') + .map_or((None, section.as_str()), |(holder, leaf)| { + (Some(holder), leaf) + }); + let Some(holder) = (match holder_path { + Some(path) => table_at(&mut expected, path), + None => Some(&mut expected), + }) else { + break; + }; + //MUTANT dotted-section-read-as-a-key|s@ let Some(rows) = holder.get_mut(leaf).and_then(toml::Value::as_array_mut) else {@ let Some(rows) = holder.get_mut(section.as_str()).and_then(toml::Value::as_array_mut) else {@|an_unknown_key_in_a_dotted_row_costs_the_row_not_the_file + let Some(rows) = holder.get_mut(leaf).and_then(toml::Value::as_array_mut) else { break; }; let Some(position) = index.checked_sub(shift).filter(|at| *at < rows.len()) else { @@ -3209,8 +3328,16 @@ fn prune_unresolvable(source: &str, behind: bool rows.remove(position); // A section whose last row goes leaves no header at all, so the parsed // document loses the key rather than keeping an empty array. + // + // Under a DOTTED section that reaches one table further (CLOUD-1775): a + // file whose only `hook` headers were `[[hook.handler]]` rows parses, + // once they are all blanked, to a document with no `hook` key at all — + // not to `hook = {}`. So an emptied holder is removed from ITS holder in + // turn, up to the root. Leaving the husk behind would fail this loop's + // own exactness guard and abandon a prune that was otherwise right. if rows.is_empty() { - expected.remove(§ion); + holder.remove(leaf); + drop_empty_ancestors(&mut expected, holder_path); } let mut candidate = text.clone(); diff --git a/crates/batten/tests/it/cli.rs b/crates/batten/tests/it/cli.rs index 72d8de4b9..44d6d4ea7 100644 --- a/crates/batten/tests/it/cli.rs +++ b/crates/batten/tests/it/cli.rs @@ -9974,10 +9974,27 @@ fn an_action_on_the_adjudicated_event_is_a_config_error() { } #[test] -fn an_unknown_key_in_an_action_row_stays_a_hard_config_error() { - // Acceptance (d). `deny_unknown_fields` on the row, so a mistyped key is - // refused rather than silently dropped — an action nobody notices is - // half-declared is a side effect that never fires. +fn an_unknown_key_in_an_action_row_costs_the_row_and_is_reported() { + // Acceptance (d), AND THE SUBJECT MOVED UNDER IT — the concern is unchanged + // and what answers it is not. + // + // (d) asks that a mistyped key never be SILENTLY dropped, because an action + // nobody notices is half-declared is a side effect that never fires. This + // case read that as "the whole file is refused", and until CLOUD-1775 that + // was also what happened: `[[hook.action]]` is a dotted header, `is_row` + // equated droppable with dotless, and neither prune granularity could name + // the unit — so the only granularity left was the file. + // + // CLOUD-1428 had already overruled that trade for `[[rule]]`: one row off and + // named beats every row off and silent, and only the first is a posture a + // reader can act on. A dotted row is a row, so the action row takes the same + // disposition — and `deny_unknown_fields` STAYS on the row, which is what + // makes the key cost it at all rather than being ignored. + // + // So the assertion is the PAIR, and neither half alone is the criterion: the + // file loads, AND the drop is reported by id. A build that dropped the key + // quietly satisfies the first and fails the second, which is exactly the + // reading (d) exists to refuse. let dir = repo_with_config( "action-unknown-key", "version = 1\n\n[[hook.action]]\nid = \"probe\"\non = \"stop\"\nrun = [\"true\"]\nwhen = \"always\"\n", @@ -9988,7 +10005,23 @@ fn an_unknown_key_in_an_action_row_stays_a_hard_config_error() { &serde_json::json!({ "hook_event_name": "Stop" }).to_string(), false, ); - assert_eq!(output.status.code(), Some(1), "a usage error, never a deny"); + assert_eq!( + output.status.code(), + Some(0), + "one unreadable row is not an unreadable file: {}", + common::stderr(&output) + ); + + let shown = common::batten() + .args(["config", "show"]) + .current_dir(&dir) + .output() + .expect("run batten config show"); + let said = format!("{}{}", common::stdout(&shown), common::stderr(&shown)); + assert!( + said.contains("probe"), + "the dropped action is named by id, or the drop is the silence (d) refuses: {said}" + ); } /// CLOUD-437's fixture: two rows, one declaring its own hatch and one not. diff --git a/crates/batten/tests/it/config_skew.rs b/crates/batten/tests/it/config_skew.rs index eec2daa7c..a1ff65126 100644 --- a/crates/batten/tests/it/config_skew.rs +++ b/crates/batten/tests/it/config_skew.rs @@ -140,6 +140,157 @@ fn a_malformed_config_does_not_mention_a_rebuild() { ); } +/// Two `[[hook.handler]]` rows, the second carrying a key no build has. +/// +/// `[hook]` is never declared as a header of its own, which is the shape +/// `batten.toml` itself has: the only `hook` headers in the authority are the +/// sixteen `[[hook.handler]]` rows. That is what makes the section addressable +/// as a dotted path rather than as a nest inside some `[[hook]]` element. +const HANDLERS: &str = r#"version = 1 + +[[hook.handler]] +id = "readable-handler" +on = "user-prompt-submit" +run = ["true"] +owner = "CLOUD-1775" +expires = "2027-02-28" + +[[hook.handler]] +id = "from-a-newer-schema" +on = "user-prompt-submit" +run = ["true"] +owner = "CLOUD-1775" +expires = "2027-02-28" +this_key_does_not_exist_in_any_version = true +"#; + +/// THE CASE THE DEFECT FAILS (CLOUD-1775), and the one the bootstrap turns on. +/// +/// `[[hook.handler]]` fell through BOTH prune granularities. `Header::is_row` +/// equated droppable with dotless, so `owning_row` declined and the row arm never +/// ran; `drop_key` then asked the top-level table for a key spelled +/// `hook.handler`, found `hook` instead, and declined too. The file was refused +/// whole. +/// +/// That is the detector disabled by the staleness it detects: the skew handler is +/// declared in the artifact whose vocabulary it polices, so the first schema +/// addition a running binary predates unregisters it. Measured 2026-09-10 on +/// `command_matcher`, three hours after the same shape on `[wiring]`; between +/// them, eight `land` laps at ~900s of local `verify` failed for reasons that had +/// nothing to do with the branch. +/// +/// Fails by: `dotted-row-not-droppable`, which restores the dotless requirement. +#[test] +fn an_unknown_key_in_a_dotted_row_costs_the_row_not_the_file() { + let dir = scratch("dotted-row", HANDLERS); + + let output = check(&dir); + let said = stderr(&output); + assert_eq!( + output.status.code(), + Some(0), + "the rows this build CAN read must still load, or the detector is \ + disabled by exactly the skew it exists to report: {said}" + ); + assert!( + !said.contains("invalid config"), + "one unreadable row is not an unreadable file: {said}" + ); +} + +/// THE ANTI-VACUITY HALF. A partial load must not become a silent one. +/// +/// Without this, the case above is satisfied by a build that drops the key and +/// says nothing — which is the permissive fallback CLOUD-251 names, and would let +/// a typo switch a handler off with no reader ever learning of it. It is also the +/// second of the row's own Done criteria, stated in its words: *the unknown key +/// is still REPORTED*. +/// +/// The good row is asserted present in the same breath, because a report naming +/// the dropped row would also be produced by a build that dropped both. +#[test] +fn the_dropped_handler_is_named_and_its_neighbour_survives() { + let dir = scratch("dotted-row-named", HANDLERS); + + let out = batten() + .args(["config", "show"]) + .current_dir(&dir) + .env_remove("BATTEN_STRICTNESS") + .env_remove("BATTEN_CONFIG_FROM") + .output() + .expect("run batten config show"); + let said = format!("{}{}", String::from_utf8_lossy(&out.stdout), stderr(&out)); + assert!( + said.contains("from-a-newer-schema"), + "a dropped row is a handler that is NOT running, and must be reported by \ + id: {said}" + ); + // THE SURVIVOR IS COUNTED, NOT NAMED, and the count is the whole assertion. + // `config show` is pointer-only (rule 4), so it renders `hook ` rather + // than each handler's id — which is what makes the number load-bearing here: + // `2` is a build that dropped nothing, `0` is a prune that took the section + // instead of the row, and only `1` is the repair. + let counted = said.lines().find_map(|line| { + let mut fields = line.split_whitespace(); + (fields.next() == Some("hook")).then(|| fields.next())? + }); + assert_eq!( + counted, + Some("1"), + "exactly the readable handler survives — 2 is no prune at all and 0 is \ + the section taken rather than the row: {said}" + ); + // POINTER, NEVER THE PAYLOAD (rule 4): the report names the row, never the + // bytes that could not be read. + assert!( + !said.contains("this_key_does_not_exist_in_any_version"), + "the report is a pointer, not the file's contents: {said}" + ); +} + +/// A DOTTED ROW NESTED INSIDE AN ARRAY ELEMENT IS STILL NOT THE DROPPABLE UNIT. +/// +/// `[[provision.env]]` is an array inside the last `[[provision]]` element, so +/// the row that owns a key under it is the `[[provision]]` row — dropping the +/// `env` block alone would leave a provision row provisioning something nobody +/// wrote. The discriminator is whether the dotted header's ROOT is itself an +/// array row in the same document; `hook` is not, `provision` is. +/// +/// This is the mirror that keeps the fix from becoming "every dotted header is +/// droppable", which would pass the two cases above and silently change what a +/// `[[provision]]` row provisions. +#[test] +fn a_dotted_row_nested_in_an_array_element_still_charges_its_owner() { + let dir = scratch( + "dotted-row-nested", + r#"version = 1 + +[[provision]] +id = "readable-provision" +run = ["true"] + +[[provision.env]] +name = "SOME_VAR" +value = "x" +this_key_does_not_exist_in_any_version = true +"#, + ); + + let out = batten() + .args(["config", "show"]) + .current_dir(&dir) + .env_remove("BATTEN_STRICTNESS") + .env_remove("BATTEN_CONFIG_FROM") + .output() + .expect("run batten config show"); + let said = format!("{}{}", String::from_utf8_lossy(&out.stdout), stderr(&out)); + assert!( + said.contains("readable-provision"), + "the unit charged is the [[provision]] row that owns the nested block, \ + named by its own id: {said}" + ); +} + /// The wording this predicate matches on is serde's, and nothing types it. /// /// `config_error` discriminates on the rendered message because serde exposes no