Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 34 additions & 4 deletions crates/batten/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1386,7 +1386,8 @@ pub const RETIRED_KEYS: &[(&str, &str)] = &[(
///
/// [`trust::load_base`]: crate::trust::load_base
pub fn parse_base(text: &str, source: &str) -> Result<Config> {
let mut table: toml::Table = toml::from_str(text).map_err(|err| config_error(source, &err))?;
let mut table: toml::Table =
toml::from_str(text).map_err(|err| config_error(source, text, &err))?;
// Nothing is reported when a key is dropped: the report this feeds is a
// comparison of two policies, and "the base declared a key this build no
// longer has" is a fact about the build rather than about either policy.
Expand Down Expand Up @@ -1518,7 +1519,8 @@ pub fn parse_override(text: &str, source: &str) -> Result<OverrideConfig> {
prune_unresolvable::<OverrideConfig>(text, binary_is_behind_the_config(source, text));
let config = match pruned.config {
Some(config) => config,
None => toml::from_str(&pruned.text).map_err(|err| config_error(source, &err))?,
None => toml::from_str(&pruned.text)
.map_err(|err| config_error(source, &pruned.text, &err))?,
};
(config, pruned.dropped)
};
Expand Down Expand Up @@ -1967,8 +1969,35 @@ fn names_an_unknown_key(rendered: &str) -> bool {
//MUTANT-SUITE crates/batten/tests/it/config_skew.rs
//MUTANT skew-reads-as-malformed|s@ if !names_an_unknown_key(&rendered) {@ if true {@|an_unknown_key_names_the_rebuild
//MUTANT every-parse-error-blames-skew|s@ if !names_an_unknown_key(&rendered) {@ if false {@|a_malformed_config_does_not_mention_a_rebuild
pub(crate) fn config_error(source: &str, err: &toml::de::Error) -> anyhow::Error {
pub(crate) fn config_error(source: &str, text: &str, err: &toml::de::Error) -> anyhow::Error {
let rendered = err.to_string();
// THE SYNTAX PROBE, AND IT RUNS ONLY HERE — ON THE ERROR PATH (CLOUD-1677).
//
// `toml::de::Error` is the one type for two very different faults, and the
// rendering hides it: a missing field and an invalid type both arrive as
// "TOML parse error at line N, column C", exactly like a stray brace. Reading
// the message cannot tell them apart, and `names_an_unknown_key` answers a
// third question again — measured on the `[[fact]]`-with-no-`returns` fixture,
// which is a SCHEMA fault that renders as a parse error and was classed as
// unreadable by the message alone.
//
// A `Table` parse answers it exactly: if the bytes are well-formed TOML then
// whatever failed was the SCHEMA over them, and the file still has rows a
// build can read. This is the probe the comment in `parse_ungated` records as
// removed for costing a parse on the hot path — it is free here, because
// nothing reaches this function until a parse has already failed.
if toml::from_str::<toml::Table>(text).is_err() {
// CLASSED, AND THE CLASS IS A DISCRIMINATOR RATHER THAN A LABEL. This is
// the one config fault with no partial function left to preserve — the
// file is not TOML, so no row is readable and none can be enforced. Every
// other fault leaves the rest of the file deciding, which is what lets an
// agent be told to repair the broken part instead of losing the gate
// surface entirely. `Native::ConfigUnreadable` carries the full argument.
return UsageError::raise_as(
crate::verdict::Native::ConfigUnreadable,
format!("invalid config {source}: {err}"),
);
}
if !names_an_unknown_key(&rendered) {
return UsageError::raise(format!("invalid config {source}: {err}"));
}
Expand Down Expand Up @@ -3042,7 +3071,8 @@ fn parse_ungated(text: &str, source: &str) -> Result<Config> {
let pruned = prune_unresolvable::<Config>(text, binary_is_behind_the_config(source, text));
let config = match pruned.config {
Some(config) => config,
None => toml::from_str(&pruned.text).map_err(|err| config_error(source, &err))?,
None => toml::from_str(&pruned.text)
.map_err(|err| config_error(source, &pruned.text, &err))?,
};
(config, pruned.dropped)
};
Expand Down
49 changes: 44 additions & 5 deletions crates/batten/src/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1742,7 +1742,15 @@ pub(crate) fn piped(
// relative name cannot arise and handing a directory would only be a guess at
// one.
// `Drop`: both callers of this entry point parse the string it returns.
piped_through(root, None, path.to_str()?, args, stdin, Diagnostics::Drop)
piped_through(
root,
None,
path.to_str()?,
args,
stdin,
Diagnostics::Drop,
&[],
)
}

/// The one spawn both piped entry points share.
Expand Down Expand Up @@ -1777,16 +1785,38 @@ fn piped_through(
args: &[String],
stdin: &str,
diagnostics: Diagnostics,
published: &[(String, Option<String>)],
) -> Option<(i32, String)> {
let mut child = crate::rules::spawn_resolving(resolve_root, program, |resolved, extra| {
Command::new(OsString::from(resolved))
let mut command = Command::new(OsString::from(resolved));
command
.args(extra.iter().map(OsString::from))
.args(args)
.current_dir(root)
.stdin(Stdio::piped())
.stdout(Stdio::piped())
.stderr(diagnostics.redirection())
.spawn()
.stderr(diagnostics.redirection());
// THE BET TRAVELS TO EVERY GATE THAT READS THE COMMIT RANGE, not only to
// `verify`'s (CLOUD-1770). `BATTEN_SPEC_BASE` is the boundary
// `claimed-keys` narrows on, and it was published into the verify child's
// environment alone — so `closing-key-check`, which runs at a later step
// and delegates to the same reader, saw no bet and counted the holder's
// borrowed keys as this branch's own. Measured: two lease acquisitions
// spent and handed back on keys the speculation adopted.
// A `None` REMOVES rather than skips, and the difference is the whole
// reason this is `Option` (measured: this file's own suite runs INSIDE a
// `land` gate, which exports the variable, so a child that merely
// inherited it read a bet that was not its own). Publishing must be a
// FUNCTION of the bet — a lap with none outstanding has to say so, or a
// stale value from an outer process narrows the inner lap's commit range
// against a base it never borrowed.
for (name, value) in published {
match value {
Some(value) => command.env(name, value),
None => command.env_remove(name),
};
}
command.spawn()
})
.ok()?;
// TAKEN AND DROPPED EVEN WHEN EMPTY, because a gate that reads stdin blocks
Expand Down Expand Up @@ -1975,13 +2005,22 @@ pub(crate) fn piped_argv(
argv: &[String],
stdin: &str,
diagnostics: Diagnostics,
published: &[(String, Option<String>)],
) -> Option<(i32, String)> {
let (program, operands) = argv.split_first()?;
// `Some(root)`, where [`piped`] passes `None`: the first word here is a NAME
// the ladder resolves, so rung 3 needs a directory to read a shebang out of.
// That one argument IS the difference between the two entry points, which is
// why they share [`piped_through`] and not a signature.
piped_through(root, Some(root), program, operands, stdin, diagnostics)
piped_through(
root,
Some(root),
program,
operands,
stdin,
diagnostics,
published,
)
}

/// This process's next dispatch number, for the live-capture key.
Expand Down
81 changes: 74 additions & 7 deletions crates/batten/src/land.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1597,7 +1597,12 @@ pub enum Readied {
/// right way round: a gate that cannot run has not passed, and treating it as
/// clean is exactly how a retired or renamed gate goes silently dead.
#[must_use]
pub fn ready(root: &Path, gates: &[Vec<String>], body: &str) -> Readied {
pub fn ready(
root: &Path,
gates: &[Vec<String>],
body: &str,
published: &[(String, Option<String>)],
) -> Readied {
if body.trim().is_empty() {
return Readied::Clear;
}
Expand All @@ -1611,8 +1616,15 @@ pub fn ready(root: &Path, gates: &[Vec<String>], body: &str) -> Readied {
// is identical across every possible finding is not a pointer (review of
// #848).
let gate = argv.join(" ");
// `published` CARRIES THE BET (CLOUD-1770). A body gate that reads the
// PR's commit range — `closing-key-check` does, through `claimed-keys` —
// cannot otherwise tell a commit this branch authored from one the
// speculation adopted, and counts the holder's keys as this branch's
// stranded ones. CLOUD-748 fixed that for `claim-race-check` by
// publishing the base into `verify`'s child; this is the same boundary
// reaching the same reader through its other caller.
let Some((code, output)) =
crate::exec::piped_argv(root, argv, body, crate::exec::Diagnostics::Keep)
crate::exec::piped_argv(root, argv, body, crate::exec::Diagnostics::Keep, published)
else {
return Readied::Unrunnable { gate };
};
Expand Down Expand Up @@ -1838,7 +1850,7 @@ pub fn admits_the_landing(root: &Path, gates: &[Vec<String>], pr: &str) -> Admit
let gate = with_pr.join(" ");
with_pr.push(pr.to_owned());
let Some((code, output)) =
crate::exec::piped_argv(root, &with_pr, "", crate::exec::Diagnostics::Keep)
crate::exec::piped_argv(root, &with_pr, "", crate::exec::Diagnostics::Keep, &[])
else {
// AN ADVISORY GATE THAT WILL NOT RUN IS NOT A REFUSAL EITHER, which
// is the same reading one line down rather than a separate decision:
Expand Down Expand Up @@ -3041,13 +3053,13 @@ mod tests {
)]];

assert_eq!(
super::ready(&root, &gate, " \n "),
super::ready(&root, &gate, " \n ", &[]),
super::Readied::Clear,
"a body the fetch never produced says nothing, so there is nothing to judge"
);

assert_eq!(
super::ready(&root, &gate, "Closes CLOUD-1"),
super::ready(&root, &gate, "Closes CLOUD-1", &[]),
super::Readied::Unrunnable {
gate: String::from("batten-no-such-program-for-the-ready-phase"),
},
Expand Down Expand Up @@ -3077,7 +3089,7 @@ mod tests {
]];

assert_eq!(
super::ready(&root, &gate, "Closes CLOUD-1"),
super::ready(&root, &gate, "Closes CLOUD-1", &[]),
super::Readied::Unrunnable {
gate: String::from(
"batten-no-such-runner-for-the-ready-phase run closing-key-check"
Expand All @@ -3087,12 +3099,67 @@ mod tests {
);
}

/// **THE BET REACHES A BODY GATE, AND FOR ITS WHOLE LIFE IT DID NOT**
/// (CLOUD-1770).
///
/// `BATTEN_SPEC_BASE` is the boundary `claimed-keys` narrows the commit range
/// on. It was published into `verify`'s child environment alone, so
/// `closing-key-check` — a LATER step delegating to that same reader — saw no
/// bet and counted the lease holder's borrowed commits as keys this branch
/// had served and stranded. Measured over one session: two lease acquisitions
/// taken, spent on a 21–28 minute gate, and handed straight back.
///
/// The gate here is `sh -c` over the variable, so its verdict is a fact about
/// the CHILD's environment rather than about this process's — a case reading
/// `std::env::var` would pass against the defect it exists to catch.
#[test]
fn a_body_gate_is_told_which_base_the_lap_borrowed() {
let root = std::env::temp_dir();
let gate = vec![vec![
String::from("sh"),
String::from("-c"),
String::from("test -n \"$BATTEN_SPEC_BASE\""),
]];
let published = vec![(
String::from(crate::speculation::PUBLISHED_AS),
Some(String::from("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")),
)];
// A lap with NO bet, spelled as the removal it has to be. `&[]` would
// leave whatever the parent exported in place — and this suite runs
// inside a `land` gate that exports exactly this variable, which is how
// the first version of this case failed against its own subject.
let unpublished = vec![(String::from(crate::speculation::PUBLISHED_AS), None)];

assert_eq!(
super::ready(&root, &gate, "Closes CLOUD-1", &published),
super::Readied::Clear,
"a speculative lap must tell its body gates which base it borrowed"
);

// **THE MIRROR, and without it the case above passes on any environment
// that happens to carry the variable** — a developer's shell, an outer
// `land`, or a sibling test leaking one. That is not hypothetical: this
// suite runs as a child of the gate `mise run land` drives, which
// publishes this very variable, and the first version of this case read
// that outer bet and failed.
//
// So "no bet" is an explicit REMOVAL rather than an omission, and the
// mechanism now matches the claim: publication is a function of the bet.
assert!(
matches!(
super::ready(&root, &gate, "Closes CLOUD-1", &unpublished),
super::Readied::Refused { .. }
),
"a lap carrying no bet must publish no base"
);
}

/// No declared gates is a clear ready, and the distinction from `Unrunnable`
/// is the optional-versus-dead one the driver's own header states.
#[test]
fn a_consumer_declaring_no_body_gates_is_clear_rather_than_unrunnable() {
assert_eq!(
super::ready(&std::env::temp_dir(), &[], "Closes CLOUD-1"),
super::ready(&std::env::temp_dir(), &[], "Closes CLOUD-1", &[]),
super::Readied::Clear
);
}
Expand Down
Loading
Loading