diff --git a/specs/016-native-tools/tasks.md b/specs/016-native-tools/tasks.md index 56c60dd..71498bd 100644 --- a/specs/016-native-tools/tasks.md +++ b/specs/016-native-tools/tasks.md @@ -227,7 +227,7 @@ the slice; all are recorded in `quickstart.md` beside the check that found them. - [X] T070 `Finding.advisory_level` is dead for the one shipped adapter. Opengrep reports severity in `tool.driver.rules[].defaultConfiguration.level`, not on the result, and `src/sarif.rs` deliberately never materialises `tool.driver.rules` — that array is the 99.96% of the report research R4 says not to read. Either reach the level without materialising the catalogue (a streaming id→level pass over the rules array in `sarif-worker`, which is in-scope and bounded), or drop the field and say why. Harmless today (an advisory level never becomes `Severity`, FR-005), but a field that can never be populated is a lie in the data model. *(Fixed by the first option: `retain_rule_levels` in `src/sarif.rs` is a streaming seq visitor that collapses the catalogue to `id → level` as it passes — two short strings per rule, capped at `MAX_RULE_LEVELS` (4096) — so help text, descriptions, and tags are still parsed and dropped rather than allocated. A result's own `level` wins over the rule default (SARIF §3.27.10); a rule past the cap costs only its advisory level, never the finding, because the visitor drains the sequence rather than aborting the deserializer and turning a big catalogue into "unparseable report". FR-005 is unmoved: `to_finding` carries the level and leaves `severity` `None`, asserted in `a_rule_level_reaches_the_ledger_without_becoming_a_severity`.)* - [X] T071 The FR-005 score guard keys on the flat `severity_score`. A caller who nests `severity = { score = … }` has the object dropped by serde, so no score reaches the ledger — FR-005 is not violated — but the caller is *silently ignored* rather than refused, which is precisely the failure mode the guard's own doc-comment says it exists to prevent. *(Fixed: `#[serde(deny_unknown_fields)]` on `RecordArgs`, so there is no spelling of "here is my score" bee accepts quietly. Covered by `a_score_nested_under_another_name_is_refused_too`.)* -- [ ] T072 `repl_command::no_provider_at_all_reports_what_is_missing` fails on any non-`enforce` build — the unenforced-session refusal fires before the configuration-completeness check the test asserts on. **Pre-existing on `main`**, not a 016 regression (verified by running the test on both branches at default features), and the only failure in the whole suite. It needs `--host`, or the `skip_on_enforcement_build()` treatment its neighbours have, inverted. +- [X] T072 `repl_command::no_provider_at_all_reports_what_is_missing` fails on any non-`enforce` build — the unenforced-session refusal fires before the configuration-completeness check the test asserts on. **Pre-existing on `main`**, not a 016 regression (verified by running the test on both branches at default features), and the only failure in the whole suite. It needs `--host`, or the `skip_on_enforcement_build()` treatment its neighbours have, inverted. *(Fixed with `--host`, which settles enforcement identically on both builds and lets the completeness failure be about the provider — an `enforce` build would otherwise have named the missing **policy** instead, so the assertion held on neither. No skip guard: this case exits at configuration and never starts a session. The fix exposed a second cause — the child inherited the developer's `~/.config/bee/config.toml`, where a provider silently completes the session the test means to leave incomplete — so `run_with_stdin` now runs in an empty directory with `HOME`/`XDG_CONFIG_HOME` pointed at it. Verified on both builds; it runs rather than skips under `enforce`.)* --- diff --git a/src/tui/markdown.rs b/src/tui/markdown.rs index 1b64464..19a403f 100644 --- a/src/tui/markdown.rs +++ b/src/tui/markdown.rs @@ -192,6 +192,7 @@ mod tests { #[test] fn a_recognized_fence_is_highlighted_and_no_color_strips_it() { + let _no_color = crate::viz::palette::lock_no_color(); // NO_COLOR is process-global, so both states live in one serial test (mirrors the // theme_bridge and palette tests in this binary). let md = "```rust\nfn main() { let answer = 42; }\n```\n"; diff --git a/src/tui/theme_bridge.rs b/src/tui/theme_bridge.rs index 89e4506..6cc5b40 100644 --- a/src/tui/theme_bridge.rs +++ b/src/tui/theme_bridge.rs @@ -50,6 +50,7 @@ mod tests { #[test] fn no_color_toggles_the_foreground() { + let _no_color = crate::viz::palette::lock_no_color(); // NO_COLOR is process-global, so both states are checked in one serial test to avoid racing a // parallel test in this binary (mirrors the palette tests). // With NO_COLOR set (SC-005): no foreground color (monochrome floor); bold still applies. diff --git a/src/viz/grid.rs b/src/viz/grid.rs index d5a85b3..361ead3 100644 --- a/src/viz/grid.rs +++ b/src/viz/grid.rs @@ -75,6 +75,7 @@ mod tests { // parallel tests in the same binary would race (SC-013 + AS-3). #[test] fn status_grid_color_and_no_color() { + let _no_color = crate::viz::palette::lock_no_color(); // Color on (SC-013): 3 green + 1 red dot, fraction present. std::env::remove_var("NO_COLOR"); let rows = vec![( diff --git a/src/viz/palette.rs b/src/viz/palette.rs index a10722e..15a04b8 100644 --- a/src/viz/palette.rs +++ b/src/viz/palette.rs @@ -147,6 +147,24 @@ pub fn bold_if(color: bool, code: &str, text: &str) -> String { } } +/// Serialises the tests that toggle the process-global `NO_COLOR`. +/// +/// Each such test already checks both colour states in one body, precisely to avoid racing — but +/// they live in different modules of the *same* test binary (`viz::grid`, `tui::theme_bridge`, +/// `tui::markdown`), so they still run concurrently with **each other**. One clearing the variable +/// while another has it set makes the suite's outcome depend on thread scheduling, which is how a +/// green run and a red run come out of identical code. Every site that mutates `NO_COLOR` takes +/// this first. +#[cfg(test)] +pub(crate) static NO_COLOR_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + +/// Take [`NO_COLOR_LOCK`], ignoring poisoning: a panicking test has already reported its own +/// failure, and refusing the lock afterwards would turn that one failure into every failure. +#[cfg(test)] +pub(crate) fn lock_no_color() -> std::sync::MutexGuard<'static, ()> { + NO_COLOR_LOCK.lock().unwrap_or_else(|e| e.into_inner()) +} + #[cfg(test)] mod tests { use super::*; diff --git a/tests/repl_command.rs b/tests/repl_command.rs index 83553c1..23ac532 100644 --- a/tests/repl_command.rs +++ b/tests/repl_command.rs @@ -31,9 +31,19 @@ fn mock_provider(dir: &Path, name: &str, reply: &str) -> PathBuf { } /// Run a command with `input` on stdin, closing it so the session ends. +/// +/// The child runs in an empty directory with `XDG_CONFIG_HOME` and `HOME` pointed at it, so it +/// discovers **no** user or project configuration. Without that, these assertions depend on the +/// developer's own `~/.config/bee/config.toml`: a provider there silently completes a session these +/// tests mean to leave incomplete, and a policy there contradicts `--host` outright. What the +/// session resolves from flags is the whole subject here, so the ambient layers have to be absent. fn run_with_stdin(bin: &Path, args: &[&str], input: &str) -> Output { + let empty = tempfile::tempdir().unwrap(); let mut child = Command::new(bin) .args(args) + .current_dir(empty.path()) + .env("XDG_CONFIG_HOME", empty.path()) + .env("HOME", empty.path()) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) @@ -132,7 +142,14 @@ fn no_provider_at_all_reports_what_is_missing() { // The resolver's contribution: `bee-repl` required `--provider` as a clap argument and said so // in usage. `bee repl` can take it from configuration, so its absence is a *completeness* // failure that names the places it could come from. - let out = bee_repl(&["--no-bee"], ""); + // + // `--host` is not incidental. Enforcement is resolved before configuration completeness, so + // without it a non-`enforce` build refuses the unenforced session first and this assertion + // never reaches the resolver — and an `enforce` build reports the *policy* as the missing + // piece. Settling enforcement is what lets the completeness failure be about the provider. + // No `skip_on_enforcement_build()` guard for the same reason its neighbours have one: this + // case exits at configuration and never starts a session, so no kernel is involved either way. + let out = bee_repl(&["--no-bee", "--host"], ""); assert_eq!(out.status.code(), Some(64)); let stderr = String::from_utf8_lossy(&out.stderr); assert!(stderr.contains("incomplete configuration"), "{stderr}");