From d6cffec20bea2aa42e7e633b2ed65deefda3b288 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 01:47:19 +0000 Subject: [PATCH 1/5] ci: gate the steel-detailer-lookup crate, and fix what it had drifted into MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `20-agents/aeco/engineering/steel-detailer-lookup` is the repo's second Rust crate. It ships in every install archive next to `aware` (release.yml builds it; install.sh, install.ps1 and packaging/wix/aware.wxs all place it), and no CI gate has ever touched it: it declares its own `[workspace]`, so `cd cli && cargo …` — the only Rust CI runs — never reaches it. The only thing that ever compiled it was `cargo build --release` at release time, which runs neither clippy nor its five unit tests. With nothing watching, it drifted from CLAUDE.md while CI stayed green: * six `.unwrap()` calls in non-test code (`run_lookup`, `run_describe`), against §Code style "No `unwrap()` outside of tests + main entry"; * edition 2021, against §Tech stack's "Rust (edition 2024)". Fixes, then the gate that keeps them fixed: * every result now goes through `print_json`, which reports a serialization failure and exits 2 like the binary's other hard errors instead of panicking with exit 101 — a code the cli transport reads as a failed invocation with no parseable reason; * edition bumped to 2024 (rustfmt's 2024 style edition reflows three statements; no behaviour change); * the crate root carries the same `#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))]` gate as `cli/src/main.rs`, and `Cargo.toml` the same `undocumented_unsafe_blocks = "deny"` as `cli/Cargo.toml`; * ci.yml runs fmt + clippy + test on it with the pinned toolchain. `tests/lint_gates.rs` is the negative control, mirroring `cli/tests/lint_gates.rs`: it compiles scratch crates carrying the gate and asserts clippy rejects `unwrap()`/`expect()` in non-test code *for that lint*, accepts clean code, still permits `unwrap()` under `cfg(test)`, and that `src/main.rs` and `Cargo.toml` still carry the gates unrelaxed. Verified by hand as well: restoring the original `to_string_pretty(&info).unwrap()` fails clippy with `error: used unwrap() on a Result value`, citing the gate line. Behaviour is unchanged — `describe`, `lookup --list/--rule/--category` and the `--json-stdin` transport all emit identical JSON and identical exit codes (0 found / 1 not-found standalone / 0 not-found under `--json-stdin` / 2 hard error). --- .github/workflows/ci.yml | 26 +- .../steel-detailer-lookup/Cargo.lock | 105 ++++++ .../steel-detailer-lookup/Cargo.toml | 19 +- .../steel-detailer-lookup/src/main.rs | 75 +++- .../steel-detailer-lookup/tests/lint_gates.rs | 319 ++++++++++++++++++ 5 files changed, 530 insertions(+), 14 deletions(-) create mode 100644 20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fc82fecbc..659e224aa 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,7 +60,11 @@ jobs: - name: Cache cargo registry + build uses: Swatinem/rust-cache@v2 with: - workspaces: cli + # Two workspaces, not one: `20-agents/.../steel-detailer-lookup` + # declares its own `[workspace]`, so `cli`'s cache never covered it. + workspaces: | + cli + 20-agents/aeco/engineering/steel-detailer-lookup # Same Linux build deps release.yml installs — the crate links libsecret # and libdbus for credential storage. @@ -92,6 +96,26 @@ jobs: working-directory: cli run: cargo test + # `20-agents/aeco/engineering/steel-detailer-lookup` is the repo's *other* + # Rust crate — the deterministic clause/section lookup that ships inside + # every install archive next to `aware` (release.yml builds it; install.sh, + # install.ps1 and packaging/wix/aware.wxs all place it). It declares its own + # `[workspace]`, so `cd cli && cargo …` has never reached it and no gate ran + # on it at all: not fmt, not clippy, not its five unit tests. The only thing + # that ever compiled it was `cargo build --release` at release time, which + # runs neither clippy nor the suite. + # + # That is how it accumulated six `unwrap()` calls in non-test code against + # CLAUDE.md §Code style, and stayed on edition 2021 against the 2024 pin in + # §Tech stack, with CI green throughout. Same three gates as `cli/` above, + # same pinned toolchain; it links nothing, so it needs none of the apt deps. + - name: steel-detailer-lookup — fmt + clippy + test + working-directory: 20-agents/aeco/engineering/steel-detailer-lookup + run: | + cargo fmt --all -- --check + cargo clippy --all-targets -- -D warnings + cargo test + # The IFC bridge is a Node package, so `cargo test` never touches it — its suite only ever ran on # whoever last remembered to. That is how `read-model` shipped returning a different coordinate # frame from `probe` (#343): the bridge's own tests encoded the wrong frame and nothing re-read diff --git a/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.lock b/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.lock index 6556841b8..4742bc862 100644 --- a/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.lock +++ b/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.lock @@ -2,18 +2,75 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "bitflags" +version = "2.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b588b76d00fde79687d7646a9b5bdf3cc0f655e0bbd080335a95d7e96f3587da" + +[[package]] +name = "cfg-if" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" + +[[package]] +name = "errno" +version = "0.3.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" +dependencies = [ + "libc", + "windows-sys", +] + +[[package]] +name = "fastrand" +version = "2.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da7c62ceae207dd37ea5b845da6a0696c799f85e97da1ab5b7910be3c1c80223" + +[[package]] +name = "getrandom" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "300e883d756b2e4ec94e02791f39b04b522276138852cfc41d9fb7e904106099" +dependencies = [ + "cfg-if", + "libc", + "r-efi", +] + [[package]] name = "itoa" version = "1.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8f42a60cbdf9a97f5d2305f08a87dc4e09308d1276d28c869c684d7777685682" +[[package]] +name = "libc" +version = "0.2.189" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3eaf3ede3fee6db1a4c2ee091bf8a8b4dccdc6d17f656fb07896ee72867612f2" + +[[package]] +name = "linux-raw-sys" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25d58a53" + [[package]] name = "memchr" version = "2.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "88904434abc2901f197fe8cc55f0445e7ded921dba5911dad2e2b39b48e663c4" +[[package]] +name = "once_cell" +version = "1.21.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" + [[package]] name = "proc-macro2" version = "1.0.106" @@ -32,6 +89,25 @@ dependencies = [ "proc-macro2", ] +[[package]] +name = "r-efi" +version = "6.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8dcc9c7d52a811697d2151c701e0d08956f92b0e24136cf4cf27b57a6a0d9bf" + +[[package]] +name = "rustix" +version = "1.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e9930ba190" +dependencies = [ + "bitflags", + "errno", + "libc", + "linux-raw-sys", + "windows-sys", +] + [[package]] name = "serde" version = "1.0.228" @@ -79,6 +155,7 @@ name = "steel-detailer-lookup" version = "0.1.0" dependencies = [ "serde_json", + "tempfile", ] [[package]] @@ -92,12 +169,40 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "tempfile" +version = "3.27.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" +dependencies = [ + "fastrand", + "getrandom", + "once_cell", + "rustix", + "windows-sys", +] + [[package]] name = "unicode-ident" version = "1.0.24" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" +[[package]] +name = "windows-link" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" + +[[package]] +name = "windows-sys" +version = "0.61.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae137229bcbd6cdf0f7b80a31df61766145077ddf49416a728b02cb3921ff3fc" +dependencies = [ + "windows-link", +] + [[package]] name = "zmij" version = "1.0.21" diff --git a/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.toml b/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.toml index 59ce4702b..ccbb30232 100644 --- a/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.toml +++ b/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.toml @@ -4,7 +4,10 @@ members = ["."] [package] name = "steel-detailer-lookup" version = "0.1.0" -edition = "2021" +# CLAUDE.md §Tech stack pins the language at "Rust (edition 2024)"; this crate +# was still on 2021 because no gate compiled it. Bumped with the CI wiring that +# now does. +edition = "2024" description = "Deterministic clause-lookup CLI for steel-detailer-us, steel-detailer-uk and steel-detailer-eu AWARE agents" [[bin]] @@ -21,3 +24,17 @@ path = "src/main.rs" [dependencies] serde_json = "1" + +[dev-dependencies] +# Scratch crates for the lint-gate negative control in `tests/lint_gates.rs`. +# Dev-only, so `cargo build --release` — what release.yml runs — never sees it. +tempfile = "3.14" + +# CLAUDE.md §Code style: "No `unsafe` unless explicitly justified with a comment +# block explaining the invariant." `cargo clippy -D warnings` does not enable +# `undocumented_unsafe_blocks` — it lives in the `restriction` group — so the +# rule needs denying by name. Mirrors `cli/Cargo.toml`, which took the same step +# for the same reason. This crate has no `unsafe` today; the gate is what keeps +# it that way without a review catching it. +[lints.clippy] +undocumented_unsafe_blocks = "deny" diff --git a/20-agents/aeco/engineering/steel-detailer-lookup/src/main.rs b/20-agents/aeco/engineering/steel-detailer-lookup/src/main.rs index 68d8f572c..708ac253e 100644 --- a/20-agents/aeco/engineering/steel-detailer-lookup/src/main.rs +++ b/20-agents/aeco/engineering/steel-detailer-lookup/src/main.rs @@ -1,3 +1,24 @@ +//! Deterministic clause/section lookup for the steel-detailer-us / -uk / -eu +//! AWARE agents. One source, three `[[bin]]` names; the binary picks its rule +//! set from `argv[0]`. Built and shipped by `.github/workflows/release.yml` +//! next to `aware` in every install archive. + +// CLAUDE.md §Code style: "Errors as data, not exceptions … No `unwrap()` +// outside of tests + main entry." `cli/` gates that with this exact attribute +// (see `cli/src/main.rs`); this crate is a separate cargo workspace under +// `20-agents/`, so nothing here inherited it and six `unwrap()` calls had +// accumulated in `run_lookup` / `run_describe` with every gate green — because +// until now no gate ran on this crate at all. +// +// Same form as `cli/`, for the same reason stated there: `[lints.clippy]` +// applies to every target in the package and would also fire on the unit tests +// below, where CLAUDE.md explicitly permits `unwrap()`. `cfg_attr(not(test), …)` +// encodes the carve-out exactly — the bin targets are linted, the same crate +// compiled under `cfg(test)` is not. +// +// `tests/lint_gates.rs` is this gate's negative control. +#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))] + use std::collections::HashSet; use std::env; use std::fs; @@ -144,7 +165,9 @@ fn main() { eprintln!(" lookup --rule look up a specific rule by id"); eprintln!(" lookup --category list all rules in a category"); eprintln!(" lookup --list list all rule ids"); - eprintln!(" lookup --json-stdin read {{rule|category}} as JSON on stdin (AWARE cli transport)"); + eprintln!( + " lookup --json-stdin read {{rule|category}} as JSON on stdin (AWARE cli transport)" + ); eprintln!(" describe show agent metadata and category list"); process::exit(2); } @@ -217,15 +240,21 @@ fn run_lookup(rules: &[serde_json::Value], args: &[String], _db: &serde_json::Va let not_found_code = if json_mode { 0 } else { 1 }; if inputs.list_all { - let ids: Vec<&str> = rules.iter().filter_map(|r| r["id"].as_str()).collect(); - println!("{}", serde_json::to_string_pretty(&ids).unwrap()); + // Built as a `Value` rather than serialized from `Vec<&str>` so it can go + // through `print_json` with the rest — `Value::from(&str)` is infallible. + let ids: serde_json::Value = rules + .iter() + .filter_map(|r| r["id"].as_str()) + .map(serde_json::Value::from) + .collect(); + print_json(&ids); return; } if let Some(id) = inputs.rule_id.as_deref() { match find_rule(rules, id) { Some(rule) => { - println!("{}", serde_json::to_string_pretty(rule).unwrap()); + print_json(rule); // exit 0 = found } None => { @@ -238,7 +267,7 @@ fn run_lookup(rules: &[serde_json::Value], args: &[String], _db: &serde_json::Va "source_quote": null, "found": false }); - println!("{}", serde_json::to_string_pretty(¬_found).unwrap()); + print_json(¬_found); process::exit(not_found_code); } } @@ -249,16 +278,18 @@ fn run_lookup(rules: &[serde_json::Value], args: &[String], _db: &serde_json::Va let filtered = rules_in_category(rules, cat); if filtered.is_empty() { let result = serde_json::json!({ "category": cat, "rules": [], "found": false }); - println!("{}", serde_json::to_string_pretty(&result).unwrap()); + print_json(&result); process::exit(not_found_code); } else { let result = serde_json::json!({ "category": cat, "rules": filtered }); - println!("{}", serde_json::to_string_pretty(&result).unwrap()); + print_json(&result); } return; } - eprintln!("lookup requires --rule , --category , or --list (or a JSON {{rule|category}} on stdin with --json-stdin)"); + eprintln!( + "lookup requires --rule , --category , or --list (or a JSON {{rule|category}} on stdin with --json-stdin)" + ); process::exit(2); } @@ -278,7 +309,26 @@ fn run_describe(rules: &[serde_json::Value], db: &serde_json::Value) { "rule_count": rules.len(), "categories": cat_list }); - println!("{}", serde_json::to_string_pretty(&info).unwrap()); + print_json(&info); +} + +/// Print `value` as pretty JSON on stdout, or report the failure and exit 2. +/// +/// Every result this binary emits goes through here. `to_string_pretty` returns +/// a `Result`, and the six call sites used to `unwrap()` it — a panic message +/// with no context on stderr, and exit code 101 rather than the 2 this binary +/// uses for every other hard error, which the cli transport reads as a failed +/// invocation with an unparseable reason (see `cli/src/runtime/invoker.rs`). +/// Serializing a `Value` is not expected to fail; "not expected to" is exactly +/// the claim the gate at the top of this file no longer takes on trust. +fn print_json(value: &serde_json::Value) { + match serde_json::to_string_pretty(value) { + Ok(text) => println!("{text}"), + Err(e) => { + eprintln!("error: cannot serialize result to JSON: {e}"); + process::exit(2); + } + } } /// Find a rule by its exact `id`. Pure — the curated connection rules and the merged @@ -352,9 +402,10 @@ mod tests { let rules = merged(); let secs = rules_in_category(&rules, "sections"); assert_eq!(secs.len(), 2); - assert!(secs - .iter() - .all(|r| r["id"].as_str().unwrap().starts_with("section."))); + assert!( + secs.iter() + .all(|r| r["id"].as_str().unwrap().starts_with("section.")) + ); assert!(rules_in_category(&rules, "nonexistent").is_empty()); } } diff --git a/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs b/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs new file mode 100644 index 000000000..45ba0e388 --- /dev/null +++ b/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs @@ -0,0 +1,319 @@ +//! Negative control for the lint gate in `src/main.rs`. +//! +//! A `deny` attribute is only worth anything if it still fires. These tests +//! compile a throwaway crate carrying the *same* attribute this crate carries +//! and assert that clippy rejects a violation and accepts its absence — so a +//! future edit that removes or narrows the gate fails the suite instead of +//! passing quietly. +//! +//! This mirrors `cli/tests/lint_gates.rs`, which guards the identical attribute +//! on the `aware` binary. Deliberately a copy rather than a shared helper: the +//! two live in separate cargo workspaces (this crate declares its own +//! `[workspace]`), so there is no crate they could both depend on without +//! inventing one, and a gate's negative control that can be broken from another +//! workspace is not much of a control. +//! +//! What is covered, precisely: +//! * the attribute denies `unwrap()` / `expect()` in non-test code, and it is +//! the *denied lint* that rejects — not an unrelated probe failure; +//! * clean code is still accepted, so a rejection above means something; +//! * the `cfg(test)` carve-out still holds, witnessed by a companion probe so +//! the assertion cannot pass by never compiling a test target at all; +//! * `src/main.rs` still carries the gate verbatim; +//! * nobody has re-opened it with an `#[allow]` / `#[expect]` / a +//! `clippy::restriction` group allow, nor from `[lints.clippy]` in the +//! manifest, where a group entry that outranks a specific `deny` switches +//! it off. +//! +//! The last two scan an artefact that is correct today — the real `src/main.rs`, +//! the real `Cargo.toml` — so they would report clean both when they work and +//! when they have stopped matching anything at all. Hence +//! `gate_reopener_classifier_matches_its_contract`, which drives the classifier +//! over synthetic input with known answers. +//! +//! The clippy-backed probes shell out to `cargo clippy` on a scratch crate with +//! no dependencies (hence `--offline`); the rest are pure file and string +//! checks. If clippy is missing the probes skip — except under `CI`, where the +//! workflow installs the component explicitly and a silent skip would be a hole +//! rather than a courtesy. + +use std::path::Path; +use std::process::Command; + +/// The gate under test, kept byte-identical to the attribute in `src/main.rs`. +const GATE: &str = "#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))]"; + +/// `true` when the probe can run. Panics instead of skipping under `CI`. +fn clippy_available() -> bool { + let available = Command::new("cargo") + .args(["clippy", "--version"]) + .output() + .is_ok_and(|out| out.status.success()); + assert!( + available || std::env::var_os("CI").is_none(), + "cargo clippy is unavailable under CI, where the workflow installs it — \ + skipping here would silently drop the gate's only negative control" + ); + available +} + +/// Run `cargo clippy` over a scratch crate whose `main.rs` is `body`, prefixed +/// with the gate attribute. Returns `(clippy succeeded, combined diagnostics)`. +/// +/// The diagnostics matter: asserting only on the exit status would let these +/// tests pass for the wrong reason — a probe that fails to build at all also +/// "fails", which would make a dead gate look enforced. +fn run_gate(body: &str) -> (bool, String) { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::create_dir_all(root.join("src")).expect("src dir"); + std::fs::write( + root.join("Cargo.toml"), + "[package]\nname = \"gate_probe\"\nversion = \"0.0.0\"\nedition = \"2024\"\n\n[workspace]\n", + ) + .expect("write manifest"); + std::fs::write(root.join("src/main.rs"), format!("{GATE}\n\n{body}")).expect("write main"); + + let out = Command::new("cargo") + // `--all-targets` matches how CI invokes clippy, and is what makes the + // `cfg(test)` carve-out testable at all: without it the probe never + // compiles a `#[cfg(test)]` module, so that assertion would pass + // vacuously. + .args(["clippy", "--all-targets", "--offline", "--quiet"]) + .current_dir(root) + // Keep the probe's build products inside the tempdir so it neither + // pollutes nor contends on this crate's target directory. + .env("CARGO_TARGET_DIR", root.join("target")) + .output() + .expect("run cargo clippy"); + let diagnostics = format!( + "{}{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + (out.status.success(), diagnostics) +} + +/// Assert clippy rejected `body`, and that it rejected it *because of the gate* +/// rather than because the probe crate failed to build. +fn assert_gate_rejects(body: &str, expected_lint: &str, what: &str) { + let (accepted, diagnostics) = run_gate(body); + assert!( + !accepted, + "clippy accepted {what} in non-test code — the gate is not enforcing" + ); + assert!( + diagnostics.contains(expected_lint), + "clippy rejected {what}, but not for `{expected_lint}` — the probe may be \ + failing for an unrelated reason. Diagnostics:\n{diagnostics}" + ); +} + +#[test] +fn unwrap_gate_rejects_unwrap_and_expect_outside_tests() { + if !clippy_available() { + eprintln!("skipping: cargo clippy unavailable"); + return; + } + + // Positive control: the same crate, same attribute, no violation. If this + // fails, the probe itself is broken and the rejections below prove nothing. + let clean = "fn main() {\n let v: Option = std::env::args().count().try_into().ok();\n println!(\"{v:?}\");\n}\n"; + let (accepted, diagnostics) = run_gate(clean); + assert!( + accepted, + "gate probe rejected clean code — the probe is broken, not the gate:\n{diagnostics}" + ); + + // `Option` from a runtime value, not a `Some(1)` literal: a literal also + // trips `unnecessary_literal_unwrap`, which would muddy which lint fired. + assert_gate_rejects( + "fn main() {\n let v: Option = std::env::args().count().try_into().ok();\n println!(\"{}\", v.unwrap());\n}\n", + "unwrap_used", + "`.unwrap()`", + ); + + assert_gate_rejects( + "fn main() {\n let v: Option = std::env::args().count().try_into().ok();\n println!(\"{}\", v.expect(\"set\"));\n}\n", + "expect_used", + "`.expect()`", + ); +} + +#[test] +fn unwrap_gate_still_permits_unwrap_in_tests() { + if !clippy_available() { + eprintln!("skipping: cargo clippy unavailable"); + return; + } + + // Witness first. This probe puts a hard type error inside `#[cfg(test)]`; + // clippy must reject it. If it does not, the probe never compiled a test + // target — and the carve-out assertion below would then pass for that + // reason alone rather than because the carve-out held. + let (witness_accepted, witness_diagnostics) = run_gate( + "fn main() {}\n\n#[cfg(test)]\nmod tests {\n #[test]\n fn t() {\n let _x: u8 = \"not a u8\";\n }\n}\n", + ); + assert!( + !witness_accepted, + "a type error inside `#[cfg(test)]` was not caught, so the probe is not \ + compiling test targets — the carve-out assertion below would be vacuous.\n\ + Check that `run_gate` still passes `--all-targets`.\n{witness_diagnostics}" + ); + + // CLAUDE.md permits `unwrap()` in tests. `cfg_attr(not(test), …)` is what + // buys that carve-out; this asserts the carve-out survives, so nobody + // "fixes" the gate into one that would force `unwrap`-free test code. + let (accepted, diagnostics) = run_gate( + "fn main() {}\n\n#[cfg(test)]\nmod tests {\n #[test]\n fn t() {\n let v: Option = Some(1);\n assert_eq!(v.unwrap(), 1);\n }\n}\n", + ); + assert!( + accepted, + "the gate fired inside `#[cfg(test)]`, where CLAUDE.md permits `unwrap()`:\n{diagnostics}" + ); +} + +#[test] +fn crate_root_actually_carries_the_gate() { + // The probes above prove the attribute denies. This proves the shipped + // crate is the thing carrying it — otherwise the gate could be deleted from + // `src/main.rs` and every behavioural assertion above would still pass. + let source = std::fs::read_to_string(manifest_dir().join("src/main.rs")).expect("read main.rs"); + assert!( + source.contains(GATE), + "`src/main.rs` no longer carries the gate verbatim:\n {GATE}\n\ + If it was deliberately reworded, update `GATE` here in the same change \ + so the probes keep testing what the crate actually carries." + ); +} + +#[test] +fn nobody_reopened_the_gate_from_source() { + let source = std::fs::read_to_string(manifest_dir().join("src/main.rs")).expect("read main.rs"); + let reopened: Vec<(usize, &str)> = source + .lines() + .enumerate() + .filter(|(_, line)| reopens_gate(line)) + .map(|(i, line)| (i + 1, line.trim())) + .collect(); + assert!( + reopened.is_empty(), + "`src/main.rs` re-opens the gate it is supposed to be under. \ + CLAUDE.md forbids silencing a gate rather than fixing the violation \ + under it:\n{reopened:#?}" + ); +} + +#[test] +fn nobody_reopened_the_gate_from_the_manifest() { + // `[lints.clippy]` outranks a crate-root attribute, so a group-level allow + // there switches the gate off with nothing in `src/` to show for it. + let manifest = std::fs::read_to_string(manifest_dir().join("Cargo.toml")).expect("read mani"); + let reopened: Vec<&str> = manifest + .lines() + .map(str::trim) + .filter(|line| manifest_reopens_gate(line)) + .collect(); + assert!( + reopened.is_empty(), + "`Cargo.toml` switches off a lint the crate root denies:\n{reopened:#?}" + ); + assert!( + manifest.contains("undocumented_unsafe_blocks = \"deny\""), + "`Cargo.toml` no longer denies `undocumented_unsafe_blocks` — CLAUDE.md's \ + \"no `unsafe` without a justification\" rule has lost its only gate here." + ); +} + +/// Negative control for the two scanners above. They read artefacts that are +/// clean today, so they report success both when they work and when they have +/// stopped matching anything; this drives them over inputs with known answers. +#[test] +fn gate_reopener_classifier_matches_its_contract() { + for line in [ + "#[allow(clippy::unwrap_used)]", + " #![allow(clippy::expect_used)]", + "#[expect(clippy::unwrap_used)]", + "#![allow(clippy::restriction)]", + "#[cfg_attr(windows, allow(clippy::unwrap_used))]", + "#[allow(clippy::undocumented_unsafe_blocks)]", + ] { + assert!(reopens_gate(line), "should have been flagged: {line}"); + } + for line in [ + "#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))]", + "#[allow(dead_code)]", + "// an allow(clippy::unwrap_used) mentioned in prose, not an attribute", + "let unwrap_used = 1;", + ] { + assert!(!reopens_gate(line), "should not have been flagged: {line}"); + } + + for line in [ + "unwrap_used = \"allow\"", + "expect_used = 'allow'", + "restriction = \"allow\"", + "undocumented_unsafe_blocks = \"warn\"", + ] { + assert!( + manifest_reopens_gate(line), + "manifest line should have been flagged: {line}" + ); + } + for line in [ + "undocumented_unsafe_blocks = \"deny\"", + "serde_json = \"1\"", + "# unwrap_used = \"allow\" — a commented-out example, not active", + ] { + assert!( + !manifest_reopens_gate(line), + "manifest line should not have been flagged: {line}" + ); + } +} + +/// This crate's root directory. +fn manifest_dir() -> &'static Path { + Path::new(env!("CARGO_MANIFEST_DIR")) +} + +/// `true` when `line` is an attribute that relaxes one of the denied lints. +/// +/// Matches on the attribute opener so prose and identifiers that merely contain +/// a lint name do not trip it, and covers `cfg_attr`-wrapped levels because the +/// level is what takes effect, not the predicate around it. +fn reopens_gate(line: &str) -> bool { + let trimmed = line.trim(); + if !(trimmed.starts_with("#[") || trimmed.starts_with("#![")) { + return false; + } + let relaxes = ["allow(", "expect(", "warn("]; + let targets = [ + "clippy::unwrap_used", + "clippy::expect_used", + "clippy::undocumented_unsafe_blocks", + "clippy::restriction", + ]; + relaxes.iter().any(|level| trimmed.contains(level)) + && targets.iter().any(|target| trimmed.contains(target)) +} + +/// `true` when `line` is a `[lints.clippy]` entry that relaxes a denied lint. +fn manifest_reopens_gate(line: &str) -> bool { + let trimmed = line.trim(); + if trimmed.starts_with('#') { + return false; + } + let Some((key, value)) = trimmed.split_once('=') else { + return false; + }; + let key = key.trim(); + let value = value.trim().trim_matches(['"', '\'']); + let gated = [ + "unwrap_used", + "expect_used", + "undocumented_unsafe_blocks", + "restriction", + ]; + gated.contains(&key) && matches!(value, "allow" | "warn") +} From 91fa082652c3832663fb6c16307f47bccea950c7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 01:58:25 +0000 Subject: [PATCH 2/5] fix(tests): parse whole attributes and inline-table lint entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings from Codex's review of d6cffec20b, both real, both in the negative control rather than the gate it guards — a control that misses the form it is supposed to catch is the worst kind of green. P2, `reopens_gate` scanned line by line. rustfmt wraps an `#[allow(…)]` that carries a long `reason`, so `allow(` and `clippy::unwrap_used` land on different lines; a predicate requiring both on one line reported clean while clippy honoured the attribute in full and accepted a production `unwrap()`. Replaced with `gate_reopeners`, ported from `cli/tests/lint_gates.rs` as the review suggested: it matches complete bracketed spans over a copy of the source with comment and string *contents* blanked, so wrapped attributes, attributes that are not line-initial, and levels nested in a `cfg_attr` predicate all count, while a lint merely named in prose or in a `reason` string does not. P2, `manifest_reopens_gate` compared the raw value against "allow"/"warn", so cargo's inline-table form — `restriction = { level = "allow", priority = 1 }` — left the whole table in `value`, matched nothing, and switched the unsafe gate off with the test still green. Replaced with `manifest_clippy_entry`, also ported: it reads both value forms, tracks the table header, skips commented-out entries, accepts the dotted `[lints] clippy.` key, and returns priority so a group entry is compared against the specific lint's priority rather than against zero. Verified end-to-end, not just by the classifier's own fixtures: injecting a rustfmt-wrapped `#[allow(clippy::unwrap_used, reason = "…")]` into the real `src/main.rs` and `restriction = { level = "allow", priority = 1 }` into the real `Cargo.toml` fails `nobody_reopened_the_gate_from_source` and `nobody_reopened_the_gate_from_the_manifest` respectively; both pass again once restored. `manifest_lint_reader_matches_its_contract` is new and covers the inline-table, single-quote, dotted-key and negative-priority cases. --- .../steel-detailer-lookup/tests/lint_gates.rs | 610 +++++++++++++++--- 1 file changed, 517 insertions(+), 93 deletions(-) diff --git a/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs b/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs index 45ba0e388..674df0ed5 100644 --- a/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs +++ b/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs @@ -21,15 +21,17 @@ //! the assertion cannot pass by never compiling a test target at all; //! * `src/main.rs` still carries the gate verbatim; //! * nobody has re-opened it with an `#[allow]` / `#[expect]` / a -//! `clippy::restriction` group allow, nor from `[lints.clippy]` in the -//! manifest, where a group entry that outranks a specific `deny` switches -//! it off. +//! `clippy::restriction` group allow — including one rustfmt has wrapped +//! across lines, or nested in a `cfg_attr` predicate — nor from +//! `[lints.clippy]` in the manifest, where a group entry that outranks a +//! specific `deny` switches it off. //! -//! The last two scan an artefact that is correct today — the real `src/main.rs`, +//! The last two scan artefacts that are correct today — the real `src/main.rs`, //! the real `Cargo.toml` — so they would report clean both when they work and -//! when they have stopped matching anything at all. Hence -//! `gate_reopener_classifier_matches_its_contract`, which drives the classifier -//! over synthetic input with known answers. +//! when they have stopped matching anything at all. Each therefore has a +//! negative control driving its classifier over synthetic input: +//! `gate_reopener_classifier_matches_its_contract` and +//! `manifest_lint_reader_matches_its_contract`. //! //! The clippy-backed probes shell out to `cargo clippy` on a scratch crate with //! no dependencies (hence `--offline`); the rest are pure file and string @@ -190,12 +192,7 @@ fn crate_root_actually_carries_the_gate() { #[test] fn nobody_reopened_the_gate_from_source() { let source = std::fs::read_to_string(manifest_dir().join("src/main.rs")).expect("read main.rs"); - let reopened: Vec<(usize, &str)> = source - .lines() - .enumerate() - .filter(|(_, line)| reopens_gate(line)) - .map(|(i, line)| (i + 1, line.trim())) - .collect(); + let reopened = gate_reopeners(&source); assert!( reopened.is_empty(), "`src/main.rs` re-opens the gate it is supposed to be under. \ @@ -206,70 +203,207 @@ fn nobody_reopened_the_gate_from_source() { #[test] fn nobody_reopened_the_gate_from_the_manifest() { - // `[lints.clippy]` outranks a crate-root attribute, so a group-level allow - // there switches the gate off with nothing in `src/` to show for it. + // `[lints.clippy]` reaches every target in the package, so an entry here can + // re-open a gate as effectively as an `#[allow]` — and from a file the + // source scan above never reads. let manifest = std::fs::read_to_string(manifest_dir().join("Cargo.toml")).expect("read mani"); - let reopened: Vec<&str> = manifest - .lines() - .map(str::trim) - .filter(|line| manifest_reopens_gate(line)) + + assert_eq!( + manifest_clippy_entry(&manifest, "undocumented_unsafe_blocks") + .map(|(level, _)| level) + .as_deref(), + Some("deny"), + "`Cargo.toml` no longer actively denies `undocumented_unsafe_blocks` under \ + `[lints.clippy]` — CLAUDE.md §Code style requires every `unsafe` block to \ + carry a justification, and `cargo clippy -D warnings` does not enable \ + that lint on its own" + ); + + let mut reopened: Vec = GATED_LINTS + .iter() + .filter_map(|lint| { + let (level, _) = manifest_clippy_entry(&manifest, lint)?; + (!level_enforces(&level)).then(|| format!("{lint} = \"{level}\"")) + }) .collect(); + + // Groups need a priority comparison, not a priority threshold. + // `undocumented_unsafe_blocks` is the one gate that lives only in this + // manifest, so a group entry that outranks it switches it off with every + // other check in this file still green. Cargo applies the higher-priority + // entry last, so the group wins exactly when its priority is strictly + // greater than the lint's — measured on clippy 1.95: + // + // deny @ 0 vs allow @ 1 -> gate OFF deny @ 0 vs allow @ 0 -> holds + // deny @ -2 vs allow @ -1 -> gate OFF deny @ -1 vs allow @ -2 -> holds + // + // Comparing the group against zero instead would miss the second row, where + // both priorities are negative, and comparing it against nothing at all + // would reject the legitimate manifests on the right. + for group in GATED_GROUPS { + let Some((group_level, group_priority)) = manifest_clippy_entry(&manifest, group) else { + continue; + }; + if level_enforces(&group_level) { + continue; + } + for lint in GATED_LINTS { + let Some((lint_level, lint_priority)) = manifest_clippy_entry(&manifest, lint) else { + continue; + }; + if level_enforces(&lint_level) && group_priority > lint_priority { + reopened.push(format!( + "{group} = {{ level = \"{group_level}\", priority = {group_priority} }} \ + outranks {lint} = {{ level = \"{lint_level}\", priority = {lint_priority} }}" + )); + } + } + } + assert!( reopened.is_empty(), - "`Cargo.toml` switches off a lint the crate root denies:\n{reopened:#?}" - ); - assert!( - manifest.contains("undocumented_unsafe_blocks = \"deny\""), - "`Cargo.toml` no longer denies `undocumented_unsafe_blocks` — CLAUDE.md's \ - \"no `unsafe` without a justification\" rule has lost its only gate here." + "these `[lints.clippy]` entries put a gated lint below `deny`:\n {}", + reopened.join("\n ") ); } -/// Negative control for the two scanners above. They read artefacts that are -/// clean today, so they report success both when they work and when they have -/// stopped matching anything; this drives them over inputs with known answers. +/// Negative control for [`gate_reopeners`]. The scan above reads a `src/main.rs` +/// that is clean today, so it reports success both when the classifier works and +/// when it has stopped matching anything; this drives it over known answers. #[test] fn gate_reopener_classifier_matches_its_contract() { - for line in [ - "#[allow(clippy::unwrap_used)]", - " #![allow(clippy::expect_used)]", - "#[expect(clippy::unwrap_used)]", - "#![allow(clippy::restriction)]", - "#[cfg_attr(windows, allow(clippy::unwrap_used))]", - "#[allow(clippy::undocumented_unsafe_blocks)]", - ] { - assert!(reopens_gate(line), "should have been flagged: {line}"); - } - for line in [ - "#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))]", - "#[allow(dead_code)]", - "// an allow(clippy::unwrap_used) mentioned in prose, not an attribute", - "let unwrap_used = 1;", + for source in [ + "#[allow(clippy::unwrap_used)]\nfn f() {}\n", + " #![allow(clippy::expect_used)]\n", + "#[expect(clippy::unwrap_used)]\nfn f() {}\n", + "#![allow(clippy::restriction)]\n", + "#[cfg_attr(windows, allow(clippy::unwrap_used))]\nfn f() {}\n", + "#[allow(clippy::undocumented_unsafe_blocks)]\nfn f() {}\n", + // Anywhere, not just line-initial — clippy honours this form too. + "fn f() { let w = { #[allow(clippy::unwrap_used)] g() }; }\n", + // Whitespace between the name and its list is legal and honoured. + "#[allow (clippy::unwrap_used)]\nfn f() {}\n", + // The form Codex flagged on this PR (#408): rustfmt wraps a long + // `reason` across lines, splitting `allow(` from the lint name. A + // line-at-a-time classifier reads every line as clean while clippy + // honours the attribute in full. + "#[allow(\n clippy::unwrap_used,\n reason = \"a long justification that pushes rustfmt into wrapping this attribute\"\n)]\nfn f() {}\n", + "#[cfg_attr(\n not(test),\n allow(clippy::expect_used)\n)]\nfn f() {}\n", ] { - assert!(!reopens_gate(line), "should not have been flagged: {line}"); + assert!( + !gate_reopeners(source).is_empty(), + "should have been flagged:\n{source}" + ); } - for line in [ - "unwrap_used = \"allow\"", - "expect_used = 'allow'", - "restriction = \"allow\"", - "undocumented_unsafe_blocks = \"warn\"", + for source in [ + "#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))]\n", + "#[allow(dead_code)]\nfn f() {}\n", + "// an #[allow(clippy::unwrap_used)] mentioned in prose, not an attribute\n", + "/* #[allow(clippy::unwrap_used)] in a block comment */\n", + "let s = \"#[allow(clippy::unwrap_used)]\";\n", + // Naming the lint in a `reason` string is not setting its level. + "#[expect(dead_code, reason = \"unlike clippy::unwrap_used, harmless\")]\nfn f() {}\n", + "let unwrap_used = 1;\n", + // Another tool's lint of the same bare name is not clippy's. + "#[allow(othertool::unwrap_used)]\nfn f() {}\n", + // An unbalanced bracket in a comment must not swallow what follows. + "// TODO(#412): first element is items[0\nfn f() {}\n", ] { assert!( - manifest_reopens_gate(line), - "manifest line should have been flagged: {line}" + gate_reopeners(source).is_empty(), + "should not have been flagged:\n{source}" ); } - for line in [ - "undocumented_unsafe_blocks = \"deny\"", - "serde_json = \"1\"", - "# unwrap_used = \"allow\" — a commented-out example, not active", - ] { - assert!( - !manifest_reopens_gate(line), - "manifest line should not have been flagged: {line}" + + // The reported line is the attribute's own, so a maintainer can go to it. + let hits = gate_reopeners("fn a() {}\n\n#[allow(clippy::unwrap_used)]\nfn b() {}\n"); + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].line, 3); +} + +/// Negative control for [`manifest_clippy_entry`]. Same reasoning: the test +/// above reads a `Cargo.toml` that is correct today. +#[test] +fn manifest_lint_reader_matches_its_contract() { + let cases = [ + ( + "the gate as it actually ships", + "[lints.clippy]\nundocumented_unsafe_blocks = \"deny\"\n", + Some(("deny", 0)), + ), + ( + "commented out — the gate is off, however the line reads", + "[lints.clippy]\n# undocumented_unsafe_blocks = \"deny\"\n", + None, + ), + ( + "downgraded", + "[lints.clippy]\nundocumented_unsafe_blocks = \"allow\"\n", + Some(("allow", 0)), + ), + ( + "single quotes, which are valid TOML", + "[lints.clippy]\nundocumented_unsafe_blocks = 'allow'\n", + Some(("allow", 0)), + ), + ( + // The form Codex flagged on this PR (#408): a `value == \"allow\"` + // comparison reads the whole inline table and matches nothing. + "the inline-table form cargo also accepts", + "[lints.clippy]\nundocumented_unsafe_blocks = { level = \"warn\", priority = 1 }\n", + Some(("warn", 1)), + ), + ( + "a negative priority, which loses to the specific lint", + "[lints.clippy]\nundocumented_unsafe_blocks = { level = \"allow\", priority = -1 }\n", + Some(("allow", -1)), + ), + ( + "the dotted key cargo accepts under `[lints]`", + "[lints]\nclippy.undocumented_unsafe_blocks = \"allow\"\n", + Some(("allow", 0)), + ), + ( + "a different tool's table cannot answer for clippy's", + "[lints.rust]\nundocumented_unsafe_blocks = \"deny\"\n", + None, + ), + ( + "the entry moved out from under the table header", + "[lints.clippy]\nother = \"deny\"\n\n[profile.release]\nundocumented_unsafe_blocks = \"deny\"\n", + None, + ), + ( + "a longer lint name must not answer for this one", + "[lints.clippy]\nundocumented_unsafe_blocks_extra = \"deny\"\n", + None, + ), + ("absent entirely", "[lints.clippy]\n", None), + ]; + for (what, manifest, expected) in cases { + let expected = expected.map(|(level, priority)| (level.to_string(), priority)); + assert_eq!( + manifest_clippy_entry(manifest, "undocumented_unsafe_blocks"), + expected, + "misread the manifest with {what}:\n{manifest}" ); } + + // The group form the priority comparison exists for, including the negative + // pair a threshold-against-zero test reads as harmless. + for (manifest, expected) in [ + ( + "[lints.clippy]\nrestriction = { level = \"allow\", priority = 1 }\n", + Some(("allow".to_string(), 1)), + ), + ( + "[lints.clippy]\nrestriction = { level = \"allow\", priority = -1 }\n", + Some(("allow".to_string(), -1)), + ), + ] { + assert_eq!(manifest_clippy_entry(manifest, "restriction"), expected); + } } /// This crate's root directory. @@ -277,43 +411,333 @@ fn manifest_dir() -> &'static Path { Path::new(env!("CARGO_MANIFEST_DIR")) } -/// `true` when `line` is an attribute that relaxes one of the denied lints. +/// The lints this crate's gates rest on. All three live in clippy's +/// `restriction` group, which is why `restriction` appears in [`GATED_GROUPS`] +/// and the other groups do not. /// -/// Matches on the attribute opener so prose and identifiers that merely contain -/// a lint name do not trip it, and covers `cfg_attr`-wrapped levels because the -/// level is what takes effect, not the predicate around it. -fn reopens_gate(line: &str) -> bool { - let trimmed = line.trim(); - if !(trimmed.starts_with("#[") || trimmed.starts_with("#![")) { - return false; - } - let relaxes = ["allow(", "expect(", "warn("]; - let targets = [ - "clippy::unwrap_used", - "clippy::expect_used", - "clippy::undocumented_unsafe_blocks", - "clippy::restriction", - ]; - relaxes.iter().any(|level| trimmed.contains(level)) - && targets.iter().any(|target| trimmed.contains(target)) +/// Bare, not `clippy::`-qualified, because both lists are matched two ways — +/// against qualified attribute text in [`gate_reopeners`], and against the +/// unqualified keys a `[lints.clippy]` table uses. +const GATED_LINTS: [&str; 3] = ["unwrap_used", "expect_used", "undocumented_unsafe_blocks"]; + +/// Lint *groups* whose allow-level suppresses a gated lint. `clippy::all`, +/// `pedantic`, `nursery` and `correctness` do not contain these lints, so +/// listing them would make this reject code that is in fact gated. +const GATED_GROUPS: [&str; 1] = ["restriction"]; + +/// A lint-level attribute that would re-open one of the gates. +#[derive(Debug, PartialEq, Eq)] +struct Reopener { + /// 1-indexed line the attribute starts on. + line: usize, + /// The attribute, whitespace-collapsed onto one line. + text: String, } -/// `true` when `line` is a `[lints.clippy]` entry that relaxes a denied lint. -fn manifest_reopens_gate(line: &str) -> bool { - let trimmed = line.trim(); - if trimmed.starts_with('#') { - return false; +/// Report every `allow`/`expect` attribute in `source` that re-opens a gate. +/// +/// Pure, so [`gate_reopener_classifier_matches_its_contract`] can drive it with +/// synthetic sources — the scan over the real `src/main.rs` has nothing to say +/// about whether the classifier still matches anything at all. +/// +/// Attributes are matched as *complete bracketed spans*, not line by line. +/// Codex flagged the line-at-a-time version on this PR (#408) and was right: an +/// `#[allow(…)]` carrying a long `reason` gets wrapped by rustfmt so that +/// `allow(` and `clippy::unwrap_used` land on different lines, and a predicate +/// requiring both on one line then reports clean while clippy honours the +/// attribute in full. Bracket matching over +/// [`blank_comments_and_strings`] also catches attributes that are not +/// line-initial (`let w = { #[allow(clippy::unwrap_used)] v.unwrap() };`), and +/// [`opens_a_lint`] finds the level by position, so one nested in a `cfg_attr` +/// predicate counts too. +/// +/// The rule is deliberately absolute: no allow/expect of a gated lint (or of +/// `clippy::restriction`) anywhere in the file, test module included. Inside +/// `#[cfg(test)]` the `cfg_attr(not(test), …)` gate is not in force, so such an +/// attribute is redundant — delete it. `undocumented_unsafe_blocks` is denied +/// from `Cargo.toml` and so applies to every target; the fix for that one is a +/// `// SAFETY:` comment, never a deletion. +fn gate_reopeners(source: &str) -> Vec { + let chars: Vec = source.chars().collect(); + let code = blank_comments_and_strings(&chars); + let mut found = Vec::new(); + let mut i = 0; + while i < code.len() { + // An attribute opens with `#[` or `#![` and nothing else. + let open = match (code.get(i), code.get(i + 1), code.get(i + 2)) { + (Some('#'), Some('['), _) => i + 1, + (Some('#'), Some('!'), Some('[')) => i + 2, + _ => { + i += 1; + continue; + } + }; + + let mut depth = 0usize; + let mut end = None; + for (k, c) in code.iter().enumerate().skip(open) { + match c { + '[' => depth += 1, + ']' => { + depth -= 1; + if depth == 0 { + end = Some(k); + break; + } + } + _ => {} + } + } + let Some(end) = end else { + // Unterminated — not an attribute this can reason about. Step past + // the `#` only, so nothing after it is swallowed. + i += 1; + continue; + }; + + let collapsed = collapse(&code[i..=end]); + // Qualified, so `#[allow(othertool::unwrap_used)]` is another tool's + // business rather than a finding here. + let names_gated = GATED_LINTS + .iter() + .chain(GATED_GROUPS.iter()) + .any(|name| collapsed.contains(&format!("clippy::{name}"))); + if opens_a_lint(&collapsed) && names_gated { + found.push(Reopener { + line: code[..i].iter().filter(|c| **c == '\n').count() + 1, + // Reported from the original, so a maintainer sees the text as + // written rather than with its strings blanked out. + text: collapse(&chars[i..=end]), + }); + } + i = end + 1; } - let Some((key, value)) = trimmed.split_once('=') else { - return false; + found +} + +/// `true` when `collapsed` sets an `allow` or `expect` lint level *anywhere* +/// inside it, including nested in a `cfg_attr` payload. +/// +/// Nesting is the point. A prefix test (`starts_with("#[allow(")`) misses +/// `#[cfg_attr(not(test), allow(clippy::unwrap_used))]`, which clippy honours in +/// full — and that form is the exact mirror of this crate's own gate, so it is +/// the first thing someone reaching for a target-conditional override would +/// write. +/// +/// A lint level is recognised by its position rather than by enumerating +/// wrappers: with whitespace removed, `allow(`/`expect(` counts only when the +/// character before it opens a list — `[`, `(` or `,`. That admits `#[allow(`, +/// `#![allow(` and any `cfg_attr(, allow(` depth, while the crate's own +/// `deny(…)` gate is left alone. Whitespace is stripped rather than trusted, so +/// `#[allow (clippy::unwrap_used)]` — which clippy honours — still counts. +fn opens_a_lint(collapsed: &str) -> bool { + let dense: String = collapsed.chars().filter(|c| !c.is_whitespace()).collect(); + ["allow(", "expect("].iter().any(|level| { + dense.match_indices(level).any(|(at, _)| { + // A level at index 0 has no opening bracket before it, so it is not + // an attribute at all. + matches!(dense[..at].chars().next_back(), Some('[' | '(' | ',')) + }) + }) +} + +/// Collapse a span onto one whitespace-normalized line. +fn collapse(span: &[char]) -> String { + span.iter() + .collect::() + .split_whitespace() + .collect::>() + .join(" ") +} + +/// Blank out comment and string-literal *contents*, preserving every character +/// position and newline so offsets still map to lines. +/// +/// This is what makes bracket matching trustworthy, and it is not a nicety. A +/// scanner that counts `[` and `]` over raw text is thrown by one unbalanced +/// bracket inside a comment — `// TODO(#412): first element is items[0` — and +/// then runs on until the brackets happen to rebalance, swallowing every +/// attribute in between. +/// +/// Blanking string contents earns its keep twice more: it removes the +/// false-positive class where a comment or an `#[expect(…, reason = "…")]` +/// merely *names* a gated lint, and it disarms `#[doc = "…[…"]`. +fn blank_comments_and_strings(chars: &[char]) -> Vec { + let mut out = chars.to_vec(); + let mut i = 0; + let at = |k: usize| chars.get(k).copied(); + // Blank `chars[k]` unless it is the newline that keeps line numbers aligned. + let blank = |out: &mut Vec, k: usize| { + if chars.get(k).is_some_and(|c| *c != '\n') + && let Some(slot) = out.get_mut(k) + { + *slot = ' '; + } }; - let key = key.trim(); - let value = value.trim().trim_matches(['"', '\'']); - let gated = [ - "unwrap_used", - "expect_used", - "undocumented_unsafe_blocks", - "restriction", - ]; - gated.contains(&key) && matches!(value, "allow" | "warn") + while i < chars.len() { + match (chars[i], at(i + 1)) { + ('/', Some('/')) => { + while i < chars.len() && chars[i] != '\n' { + blank(&mut out, i); + i += 1; + } + } + ('/', Some('*')) => { + let mut depth = 0usize; + while i < chars.len() { + if chars[i] == '/' && at(i + 1) == Some('*') { + depth += 1; + blank(&mut out, i); + blank(&mut out, i + 1); + i += 2; + continue; + } + if chars[i] == '*' && at(i + 1) == Some('/') { + depth -= 1; + blank(&mut out, i); + blank(&mut out, i + 1); + i += 2; + if depth == 0 { + break; + } + continue; + } + blank(&mut out, i); + i += 1; + } + } + // Raw string: `r`, then any number of `#`, then `"`. Anything else + // beginning with `r` is an ordinary identifier. + ('r', Some('"' | '#')) => { + let mut hashes = 0; + let mut j = i + 1; + while at(j) == Some('#') { + hashes += 1; + j += 1; + } + if at(j) != Some('"') { + i += 1; + continue; + } + j += 1; + while j < chars.len() { + if chars[j] == '"' { + let closed = (1..=hashes).all(|n| at(j + n) == Some('#')); + if closed { + j += hashes + 1; + break; + } + } + blank(&mut out, j); + j += 1; + } + i = j; + } + ('"', _) => { + let mut j = i + 1; + while j < chars.len() && chars[j] != '"' { + // Skip the character an escape protects, so `\"` does not + // look like the closing quote. + let step = usize::from(chars[j] == '\\') + 1; + for k in j..j + step { + blank(&mut out, k); + } + j += step; + } + i = j + 1; + } + // A char literal, not a lifetime: `'x'`, `'\n'`. A lifetime (`'a`) + // has no closing quote and must be left alone. + ('\'', _) => { + let width = if at(i + 1) == Some('\\') { 3 } else { 2 }; + if at(i + width) == Some('\'') { + for k in i + 1..i + width { + blank(&mut out, k); + } + i += width + 1; + } else { + i += 1; + } + } + _ => i += 1, + } + } + out +} + +/// The level and priority a manifest `[lints.clippy]` table assigns to `name`, +/// or `None` when it assigns none. Priority defaults to 0, as cargo does. +/// +/// Reads *active* entries only. A `contains` over the whole file cannot tell +/// `undocumented_unsafe_blocks = "deny"` from the same line commented out, so +/// the gate could be switched off by prefixing one `#` and the anchor would stay +/// green — the precise failure it exists to prevent. Tracking the table header +/// also keeps a same-named `[lints.rust]` entry from answering for the clippy +/// one. +/// +/// Both value forms cargo accepts are read: `name = "level"` and the inline +/// table `name = { level = "level", priority = N }`. Codex flagged the missing +/// second form on this PR (#408) — a `value == "allow"` comparison sees the +/// whole table and matches nothing, so `restriction = { level = "allow", +/// priority = 1 }` would switch the unsafe gate off with this test still green. +/// +/// Priority is read because it decides whether a *group* entry wins. Measured +/// against the real gate: with `undocumented_unsafe_blocks = "deny"` present, +/// `restriction = { level = "allow", priority = 1 }` turns the unsafe gate off, +/// while the same entry at `priority = -1` leaves it enforcing. Flagging the +/// second would reject a legitimate manifest. +/// +/// Deliberately not a TOML parser: this crate has no TOML dependency, and both +/// forms carry what is needed in quotes on the entry's first line. +fn manifest_clippy_entry(manifest: &str, name: &str) -> Option<(String, i64)> { + let dotted = format!("clippy.{name}"); + let mut table = ""; + for line in manifest.lines().map(str::trim) { + if line.starts_with('#') { + continue; + } + if let Some(header) = line.strip_prefix('[') { + table = header.split(']').next().unwrap_or_default(); + continue; + } + // Under `[lints]`, cargo also accepts the dotted key `clippy.` — + // a form a bare `starts_with(name)` never sees. + let rest = match table { + "lints.clippy" => line.strip_prefix(name), + "lints" => line.strip_prefix(dotted.as_str()), + _ => None, + }; + // `strip_prefix` alone would match `unwrap_used_extra`; require the + // assignment to begin right after the name. + let Some(rest) = rest.map(str::trim_start).filter(|r| r.starts_with('=')) else { + continue; + }; + // Single quotes are valid TOML, so a quote-specific split would read + // `= 'allow'` as no level at all and call a downgraded lint clean. + let level = rest + .split(['"', '\'']) + .nth(1) + .unwrap_or_default() + .to_string(); + let priority = rest + .split_once("priority") + .and_then(|(_, tail)| { + let digits: String = tail + .trim_start() + .trim_start_matches('=') + .trim() + .chars() + .take_while(|c| c.is_ascii_digit() || *c == '-') + .collect(); + digits.parse::().ok() + }) + .unwrap_or(0); + return Some((level, priority)); + } + None +} + +/// `true` when `level` still enforces. +fn level_enforces(level: &str) -> bool { + level == "deny" || level == "forbid" } From 1b61bdd019daadc7ccea1a5e9ab15652b948d9ef Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 02:06:58 +0000 Subject: [PATCH 3/5] fix(tests): parse the manifest as TOML instead of scanning it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings from Codex's review of 91fa082652, both real. `priority = +1` is valid TOML and a digits-and-`-` filter rejected the `+`, so the parse failed and defaulted to 0; and a multiline inline table left the reader looking at `restriction = {` alone, recording an empty level at priority 0. Each was measured to switch the unsafe gate off with the test still green. Checking those two turned up a third the review had not named: `[lints.clippy.restriction]` as its own sub-table does the same thing. Four forms in two rounds is the answer to whether a hand-rolled reader can cover TOML — so `manifest_clippy_entry` now parses with the `toml` crate (a dev-dependency; `cargo build --release`, what release.yml runs, never builds it) and navigates `lints.clippy.`. That path resolves `[lints.clippy]`, the dotted `[lints] clippy.` key and the `[lints.clippy.]` sub-table alike, because the parser rather than this function decides what each spelling means. Commented-out entries stop needing a special case too: a comment is not data. The real fix, though, is that the reader no longer has the last word. `the_manifest_reader_agrees_with_clippy` runs clippy over a scratch crate with an undocumented `unsafe` block for each form and fails if the reader's verdict differs from what clippy actually did. Hand-written fixtures encode what their author believes cargo accepts, and that belief was wrong four times here; this measures instead. It earned its keep immediately by rejecting one of its own fixtures — `[lints.clippy]` followed by a second `[lints]` is a duplicate key — so `unsafe_gate_holds` now also asserts the probe failed *for the lint* rather than for a manifest cargo could not read, which would otherwise have looked like an open gate. Verified against the real Cargo.toml: `priority = +1`, the multiline table and the sub-table each fail `nobody_reopened_the_gate_from_the_manifest`, naming the outranking, and all 8 tests pass once restored. --- .../steel-detailer-lookup/Cargo.lock | 77 +++++ .../steel-detailer-lookup/Cargo.toml | 13 +- .../steel-detailer-lookup/tests/lint_gates.rs | 268 ++++++++++++++---- 3 files changed, 296 insertions(+), 62 deletions(-) diff --git a/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.lock b/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.lock index 4742bc862..4b61edcfd 100644 --- a/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.lock +++ b/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.lock @@ -14,6 +14,12 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" +[[package]] +name = "equivalent" +version = "1.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "877a4ace8713b0bcf2a4e7eec82529c029f1d0619886d18145fea96c3ffe5c0f" + [[package]] name = "errno" version = "0.3.14" @@ -41,6 +47,22 @@ dependencies = [ "r-efi", ] +[[package]] +name = "hashbrown" +version = "0.17.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ed5909b6e89a2db4456e54cd5f673791d7eca6732202bbf2a9cc504fe2f9b84a" + +[[package]] +name = "indexmap" +version = "2.14.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d466e9454f08e4a911e14806c24e16fba1b4c121d1ea474396f396069cf949d9" +dependencies = [ + "equivalent", + "hashbrown", +] + [[package]] name = "itoa" version = "1.0.18" @@ -150,12 +172,22 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_spanned" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6662b5879511e06e8999a8a235d848113e942c9124f211511b16466ee2995f26" +dependencies = [ + "serde_core", +] + [[package]] name = "steel-detailer-lookup" version = "0.1.0" dependencies = [ "serde_json", "tempfile", + "toml", ] [[package]] @@ -182,6 +214,45 @@ dependencies = [ "windows-sys", ] +[[package]] +name = "toml" +version = "1.1.4+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3aace63f4bbcdfc2c965b059de67119c89c4017a70d633be6c104910f67056f5" +dependencies = [ + "indexmap", + "serde_core", + "serde_spanned", + "toml_datetime", + "toml_parser", + "toml_writer", + "winnow", +] + +[[package]] +name = "toml_datetime" +version = "1.1.1+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7" +dependencies = [ + "serde_core", +] + +[[package]] +name = "toml_parser" +version = "1.1.3+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d38ac1cf9b95face32296c0a3ede1fdc270627c9d9c02a7274dd6d960dc4d56" +dependencies = [ + "winnow", +] + +[[package]] +name = "toml_writer" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" + [[package]] name = "unicode-ident" version = "1.0.24" @@ -203,6 +274,12 @@ dependencies = [ "windows-link", ] +[[package]] +name = "winnow" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23b97319f7b8343df12cc98938e5c3eb436064524c8d2b4e30a1d3a36eecdf81" + [[package]] name = "zmij" version = "1.0.21" diff --git a/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.toml b/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.toml index ccbb30232..ceb9ba605 100644 --- a/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.toml +++ b/20-agents/aeco/engineering/steel-detailer-lookup/Cargo.toml @@ -26,9 +26,18 @@ path = "src/main.rs" serde_json = "1" [dev-dependencies] -# Scratch crates for the lint-gate negative control in `tests/lint_gates.rs`. -# Dev-only, so `cargo build --release` — what release.yml runs — never sees it. +# Both are for the lint-gate negative control in `tests/lint_gates.rs`, and both +# are dev-only — `cargo build --release`, what release.yml runs, never sees them. +# +# `tempfile` builds the scratch crates the clippy probes lint. `toml` reads the +# `[lints.clippy]` table below, replacing a hand-rolled line-at-a-time reader +# that missed three separate forms cargo accepts — a single-line inline table, a +# multiline one, and a `+`-signed priority — each of which was *measured* to +# switch the unsafe gate off with the test still green (#408). A guard for +# "nobody re-opened this gate" has to read the manifest the way cargo does, so +# it parses rather than pattern-matches. tempfile = "3.14" +toml = "1" # CLAUDE.md §Code style: "No `unsafe` unless explicitly justified with a comment # block explaining the invariant." `cargo clippy -D warnings` does not enable diff --git a/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs b/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs index 674df0ed5..c1e0c4276 100644 --- a/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs +++ b/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs @@ -219,16 +219,29 @@ fn nobody_reopened_the_gate_from_the_manifest() { that lint on its own" ); + let reopened = manifest_reopenings(&manifest); + assert!( + reopened.is_empty(), + "these `[lints.clippy]` entries put a gated lint below `deny`:\n {}", + reopened.join("\n ") + ); +} + +/// Every way `manifest` puts a gated lint below `deny`, directly or by group. +/// +/// Pure, so `the_manifest_reader_agrees_with_clippy` can drive it over forms +/// whose real effect has been measured. +fn manifest_reopenings(manifest: &str) -> Vec { let mut reopened: Vec = GATED_LINTS .iter() .filter_map(|lint| { - let (level, _) = manifest_clippy_entry(&manifest, lint)?; + let (level, _) = manifest_clippy_entry(manifest, lint)?; (!level_enforces(&level)).then(|| format!("{lint} = \"{level}\"")) }) .collect(); // Groups need a priority comparison, not a priority threshold. - // `undocumented_unsafe_blocks` is the one gate that lives only in this + // `undocumented_unsafe_blocks` is the one gate that lives only in the // manifest, so a group entry that outranks it switches it off with every // other check in this file still green. Cargo applies the higher-priority // entry last, so the group wins exactly when its priority is strictly @@ -241,14 +254,14 @@ fn nobody_reopened_the_gate_from_the_manifest() { // both priorities are negative, and comparing it against nothing at all // would reject the legitimate manifests on the right. for group in GATED_GROUPS { - let Some((group_level, group_priority)) = manifest_clippy_entry(&manifest, group) else { + let Some((group_level, group_priority)) = manifest_clippy_entry(manifest, group) else { continue; }; if level_enforces(&group_level) { continue; } for lint in GATED_LINTS { - let Some((lint_level, lint_priority)) = manifest_clippy_entry(&manifest, lint) else { + let Some((lint_level, lint_priority)) = manifest_clippy_entry(manifest, lint) else { continue; }; if level_enforces(&lint_level) && group_priority > lint_priority { @@ -259,12 +272,144 @@ fn nobody_reopened_the_gate_from_the_manifest() { } } } + reopened +} +/// The `[lints.clippy]` table as this crate ships it. +const SHIPPED_LINTS: &str = "[lints.clippy]\nundocumented_unsafe_blocks = \"deny\"\n"; + +/// `true` when clippy still rejects an undocumented `unsafe` block in a scratch +/// crate carrying `lints` as its `[lints.clippy]` table. +/// +/// This is the oracle. Every other assertion about the manifest reader is a +/// claim about TOML syntax written by hand, and two rounds of review on #408 +/// found forms those claims had missed. Asking clippy directly cannot miss one. +fn unsafe_gate_holds(lints: &str) -> bool { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::create_dir_all(root.join("src")).expect("src dir"); + std::fs::write( + root.join("Cargo.toml"), + format!( + "[package]\nname = \"gate_probe\"\nversion = \"0.0.0\"\nedition = \"2024\"\n\n[workspace]\n\n{lints}" + ), + ) + .expect("write manifest"); + // An `unsafe` block with no `// SAFETY:` above it — the exact thing + // `undocumented_unsafe_blocks` exists to reject. Never executed: only + // clippy runs here, the binary is not. + std::fs::write( + root.join("src/main.rs"), + "fn main() {\n unsafe {\n std::ptr::null::();\n }\n}\n", + ) + .expect("write main"); + + let out = Command::new("cargo") + .args(["clippy", "--offline", "--quiet"]) + .current_dir(root) + .env("CARGO_TARGET_DIR", root.join("target")) + .output() + .expect("run cargo clippy"); + let diagnostics = format!( + "{}{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + // Named, not merely non-zero: a scratch crate that failed to build for some + // unrelated reason would otherwise read as an enforcing gate. + let named = diagnostics.contains("undocumented_unsafe_blocks"); + // And the converse trap, which this test walked into while being written: a + // manifest cargo *rejects* names no lint either, so it would read as "gate + // off" and quietly agree with whatever the reader said. A failure that is + // not the lint firing means the probe broke, not that the gate is open. assert!( - reopened.is_empty(), - "these `[lints.clippy]` entries put a gated lint below `deny`:\n {}", - reopened.join("\n ") + named || out.status.success(), + "the probe crate failed for a reason other than the gate — the manifest \ + under test may not be valid TOML at all:\n{lints}\n{diagnostics}" ); + named +} + +/// The reader's verdict must match what clippy actually does, form by form. +/// +/// Hand-written fixtures encode what their author believed cargo accepts, and +/// on #408 that belief was wrong four times running — a single-line inline +/// table, a multiline one, a `+`-signed priority, and a lint promoted to its own +/// sub-table. Each was measured to switch the unsafe gate off while the reader +/// called the manifest clean. This test measures instead of believing: it runs +/// clippy over each form and fails when the reader disagrees with the result. +#[test] +fn the_manifest_reader_agrees_with_clippy() { + if !clippy_available() { + eprintln!("skipping: cargo clippy unavailable"); + return; + } + let deny = "undocumented_unsafe_blocks = \"deny\""; + let cases = [ + ( + "the table as this crate ships it", + SHIPPED_LINTS.to_string(), + ), + ( + "a single-line inline table outranking the deny", + format!( + "[lints.clippy]\n{deny}\nrestriction = {{ level = \"allow\", priority = 1 }}\n" + ), + ), + ( + "a multiline inline table", + format!( + "[lints.clippy]\n{deny}\nrestriction = {{\n level = \"allow\",\n priority = 1,\n}}\n" + ), + ), + ( + "an explicitly `+`-signed priority", + format!( + "[lints.clippy]\n{deny}\nrestriction = {{ level = \"allow\", priority = +1 }}\n" + ), + ), + ( + "the group promoted to its own sub-table", + format!( + "[lints.clippy]\n{deny}\n\n[lints.clippy.restriction]\nlevel = \"allow\"\npriority = 1\n" + ), + ), + ( + "a group that loses the priority comparison", + format!( + "[lints.clippy]\n{deny}\nrestriction = {{ level = \"allow\", priority = -1 }}\n" + ), + ), + ( + "the lint downgraded outright", + "[lints.clippy]\nundocumented_unsafe_blocks = \"allow\"\n".to_string(), + ), + ( + // Both dotted under one `[lints]` header. `[lints.clippy]` followed + // by a second `[lints]` is a duplicate key and not valid TOML — a + // fixture this test was first written with, and which the hardened + // `unsafe_gate_holds` now refuses to read as an open gate. + "the dotted keys cargo accepts under `[lints]`", + "[lints]\nclippy.undocumented_unsafe_blocks = \"deny\"\nclippy.restriction = { level = \"allow\", priority = 1 }\n".to_string(), + ), + ]; + + for (what, lints) in cases { + let holds = unsafe_gate_holds(&lints); + let reader_says_clean = manifest_reopenings(&lints).is_empty(); + assert_eq!( + reader_says_clean, + holds, + "the manifest reader and clippy disagree about {what}. \ + clippy: gate {}. reader: manifest {}.\n{lints}", + if holds { "HOLDS" } else { "is OFF" }, + if reader_says_clean { + "is clean" + } else { + "re-opens the gate" + } + ); + } } /// Negative control for [`gate_reopeners`]. The scan above reads a `src/main.rs` @@ -348,12 +493,27 @@ fn manifest_lint_reader_matches_its_contract() { Some(("allow", 0)), ), ( - // The form Codex flagged on this PR (#408): a `value == \"allow\"` - // comparison reads the whole inline table and matches nothing. "the inline-table form cargo also accepts", "[lints.clippy]\nundocumented_unsafe_blocks = { level = \"warn\", priority = 1 }\n", Some(("warn", 1)), ), + ( + // Every row below was measured against the real gate before being + // written down — see `cargo_accepts_the_forms_this_reader_claims`. + "the multiline inline table (#408, round 2)", + "[lints.clippy]\nundocumented_unsafe_blocks = {\n level = \"allow\",\n priority = 1,\n}\n", + Some(("allow", 1)), + ), + ( + "an explicitly `+`-signed priority (#408, round 2)", + "[lints.clippy]\nundocumented_unsafe_blocks = { level = \"allow\", priority = +1 }\n", + Some(("allow", 1)), + ), + ( + "the lint promoted to its own sub-table", + "[lints.clippy.undocumented_unsafe_blocks]\nlevel = \"allow\"\npriority = 1\n", + Some(("allow", 1)), + ), ( "a negative priority, which loses to the specific lint", "[lints.clippy]\nundocumented_unsafe_blocks = { level = \"allow\", priority = -1 }\n", @@ -675,66 +835,54 @@ fn blank_comments_and_strings(chars: &[char]) -> Vec { /// also keeps a same-named `[lints.rust]` entry from answering for the clippy /// one. /// -/// Both value forms cargo accepts are read: `name = "level"` and the inline -/// table `name = { level = "level", priority = N }`. Codex flagged the missing -/// second form on this PR (#408) — a `value == "allow"` comparison sees the -/// whole table and matches nothing, so `restriction = { level = "allow", -/// priority = 1 }` would switch the unsafe gate off with this test still green. +/// Parsed as TOML rather than scanned, and that is the whole point. A +/// line-at-a-time reader was tried first and shipped three separate holes, +/// each of which cargo 1.95.0 accepts and each of which was *measured* to +/// switch the unsafe gate off with this test still green: +/// +/// | form | what the scanner did | +/// |---|---| +/// | `restriction = { level = "allow", priority = 1 }` | compared the whole inline table against `"allow"`, matched nothing | +/// | `restriction = {`⏎` level = "allow",`⏎` priority = 1,`⏎`}` | read only the first line, recorded an empty level at priority 0 | +/// | `priority = +1` | a digits-and-`-` filter rejected `+`, so the parse failed and defaulted to 0 | +/// +/// A fourth, `[lints.clippy.restriction]` as its own sub-table, was found while +/// checking the first three. That is four for four against a hand-rolled +/// reader, so this reads the manifest the way cargo does instead of guessing at +/// its syntax — `+1`, multiline tables, sub-tables and dotted keys all come out +/// right because the parser, not this function, decides what they mean. +/// +/// One thing the parse gives for free that the scanner needed a special case +/// for: a commented-out `undocumented_unsafe_blocks = "deny"` is not data, so +/// it cannot answer for the live entry. A `contains` over the file could not +/// tell the two apart, and the gate would have been switchable off by prefixing +/// one `#`. /// /// Priority is read because it decides whether a *group* entry wins. Measured /// against the real gate: with `undocumented_unsafe_blocks = "deny"` present, /// `restriction = { level = "allow", priority = 1 }` turns the unsafe gate off, /// while the same entry at `priority = -1` leaves it enforcing. Flagging the /// second would reject a legitimate manifest. -/// -/// Deliberately not a TOML parser: this crate has no TOML dependency, and both -/// forms carry what is needed in quotes on the entry's first line. fn manifest_clippy_entry(manifest: &str, name: &str) -> Option<(String, i64)> { - let dotted = format!("clippy.{name}"); - let mut table = ""; - for line in manifest.lines().map(str::trim) { - if line.starts_with('#') { - continue; + // `Table`, not `Value`: in toml 1.x `Value`'s `FromStr` parses a single TOML + // *value*, so a whole document fails with "unexpected content" at the first + // table header. + let parsed: toml::Table = manifest.parse().expect("manifest is not valid TOML"); + // `[lints.clippy]`, `[lints] clippy. = …` and `[lints.clippy.]` + // are three spellings of one path, and the parser has already reconciled + // them — so navigating the path covers all three with no cases here. + let entry = parsed.get("lints")?.get("clippy")?.get(name)?; + match entry { + // `name = "level"`. Priority defaults to 0, as cargo does. + toml::Value::String(level) => Some((level.clone(), 0)), + // `name = { level = "…", priority = N }`, however it is laid out. + toml::Value::Table(table) => { + let level = table.get("level")?.as_str()?.to_string(); + let priority = table.get("priority").and_then(toml::Value::as_integer); + Some((level, priority.unwrap_or(0))) } - if let Some(header) = line.strip_prefix('[') { - table = header.split(']').next().unwrap_or_default(); - continue; - } - // Under `[lints]`, cargo also accepts the dotted key `clippy.` — - // a form a bare `starts_with(name)` never sees. - let rest = match table { - "lints.clippy" => line.strip_prefix(name), - "lints" => line.strip_prefix(dotted.as_str()), - _ => None, - }; - // `strip_prefix` alone would match `unwrap_used_extra`; require the - // assignment to begin right after the name. - let Some(rest) = rest.map(str::trim_start).filter(|r| r.starts_with('=')) else { - continue; - }; - // Single quotes are valid TOML, so a quote-specific split would read - // `= 'allow'` as no level at all and call a downgraded lint clean. - let level = rest - .split(['"', '\'']) - .nth(1) - .unwrap_or_default() - .to_string(); - let priority = rest - .split_once("priority") - .and_then(|(_, tail)| { - let digits: String = tail - .trim_start() - .trim_start_matches('=') - .trim() - .chars() - .take_while(|c| c.is_ascii_digit() || *c == '-') - .collect(); - digits.parse::().ok() - }) - .unwrap_or(0); - return Some((level, priority)); + _ => None, } - None } /// `true` when `level` still enforces. From a8a49a982c0ac56557780fbd67e3d8894d101204 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 05:39:59 +0000 Subject: [PATCH 4/5] fix(tests): scan all of src, and require rejection rather than a diagnostic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings from Codex's review of 1b61bdd0, both real. `nobody_reopened_the_gate_from_source` read `src/main.rs` and nothing else. The crate-root `deny` is overridden by an `#![allow(...)]` at the top of any other module, so the first `src/helper.rs` this crate grew could re-open the gate with the guard still green. It now walks every `.rs` file under `src/`, and asserts the walk reached `src/main.rs` so a collapsed walk cannot read as a clean scan. Measured: a scratch `src/helper.rs` carrying `#![allow(clippy::unwrap_used)]` is reported before this change and flagged after it. `unsafe_gate_holds` returned "the gate holds" whenever clippy named the lint. At `warn` the lint fires and prints its name while clippy exits 0 and the undocumented `unsafe` ships, so a warning read as enforcement. It now requires a failing status as well, matching `level_enforces`, which has always counted only `deny`/`forbid`. `warn` joins the measured cases in `the_manifest_reader_agrees_with_clippy` — that case fails against the old oracle, which is what makes it worth having. --- .../steel-detailer-lookup/tests/lint_gates.rs | 105 +++++++++++++++--- 1 file changed, 90 insertions(+), 15 deletions(-) diff --git a/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs b/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs index c1e0c4276..7e902532d 100644 --- a/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs +++ b/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs @@ -20,15 +20,16 @@ //! * the `cfg(test)` carve-out still holds, witnessed by a companion probe so //! the assertion cannot pass by never compiling a test target at all; //! * `src/main.rs` still carries the gate verbatim; -//! * nobody has re-opened it with an `#[allow]` / `#[expect]` / a -//! `clippy::restriction` group allow — including one rustfmt has wrapped -//! across lines, or nested in a `cfg_attr` predicate — nor from -//! `[lints.clippy]` in the manifest, where a group entry that outranks a -//! specific `deny` switches it off. +//! * no production source under `src/` — every `.rs` file, not just the crate +//! root, since a module can `#![allow]` its way out from under one — has +//! re-opened it with an `#[allow]` / `#[expect]` / a `clippy::restriction` +//! group allow, including one rustfmt has wrapped across lines or nested in +//! a `cfg_attr` predicate, nor from `[lints.clippy]` in the manifest, where +//! a group entry that outranks a specific `deny` switches it off. //! -//! The last two scan artefacts that are correct today — the real `src/main.rs`, -//! the real `Cargo.toml` — so they would report clean both when they work and -//! when they have stopped matching anything at all. Each therefore has a +//! The last two scan artefacts that are correct today — the real sources under +//! `src/`, the real `Cargo.toml` — so they would report clean both when they +//! work and when they have stopped matching anything at all. Each therefore has a //! negative control driving its classifier over synthetic input: //! `gate_reopener_classifier_matches_its_contract` and //! `manifest_lint_reader_matches_its_contract`. @@ -189,18 +190,77 @@ fn crate_root_actually_carries_the_gate() { ); } +/// Every production `.rs` file, not just `src/main.rs`. +/// +/// The crate is a single file today, and scanning only that file was the gate's +/// blind spot: a crate-root `deny` is overridden by an `#![allow(…)]` at the top +/// of any *other* module, so the first `src/helper.rs` this crate grows could +/// re-open the gate with nothing reading it. Walking `src/` means the guard +/// survives the crate being split into modules, which is precisely when a +/// hand-listed path stops covering it. #[test] fn nobody_reopened_the_gate_from_source() { - let source = std::fs::read_to_string(manifest_dir().join("src/main.rs")).expect("read main.rs"); - let reopened = gate_reopeners(&source); + let root = manifest_dir(); + let mut files = Vec::new(); + collect_rs_files(&root.join("src"), &mut files); + files.sort(); + + // A floor, not an is-empty check: a walk that silently recovered nothing + // would report a clean scan. `src/main.rs` is the file the crate cannot + // exist without, so its presence is what proves the walk actually ran. assert!( - reopened.is_empty(), - "`src/main.rs` re-opens the gate it is supposed to be under. \ + files.contains(&root.join("src/main.rs")), + "the walk of {} did not reach `src/main.rs`, so a clean result means \ + nothing:\n{files:#?}", + root.join("src").display() + ); + + let mut offenders = Vec::new(); + for file in &files { + // Not `else { continue }`. A file this gate cannot read is a file it + // cannot clear, and turning that into silence is the exact failure mode + // CLAUDE.md §Engineering rules forbids — in the file that enforces it. + let source = std::fs::read_to_string(file) + .unwrap_or_else(|e| panic!("cannot read {}: {e}", file.display())); + let relative = file + .strip_prefix(root) + .unwrap_or(file) + .display() + .to_string(); + for hit in gate_reopeners(&source) { + offenders.push(format!("{relative}:{}: {}", hit.line, hit.text)); + } + } + + assert!( + offenders.is_empty(), + "these attributes re-open the gate the crate is supposed to be under. \ CLAUDE.md forbids silencing a gate rather than fixing the violation \ - under it:\n{reopened:#?}" + under it:\n {}", + offenders.join("\n ") ); } +/// Collect every `.rs` file under `dir`, recursively. +/// +/// Panics rather than returning early on an unreadable directory. Turning a +/// failed walk into a short file list would hand the caller a clean scan of +/// almost nothing — coverage collapse looks exactly like coverage. +fn collect_rs_files(dir: &Path, out: &mut Vec) { + let entries = + std::fs::read_dir(dir).unwrap_or_else(|e| panic!("cannot walk {}: {e}", dir.display())); + for entry in entries { + let path = entry + .unwrap_or_else(|e| panic!("cannot read an entry in {}: {e}", dir.display())) + .path(); + if path.is_dir() { + collect_rs_files(&path, out); + } else if path.extension().is_some_and(|ext| ext == "rs") { + out.push(path); + } + } +} + #[test] fn nobody_reopened_the_gate_from_the_manifest() { // `[lints.clippy]` reaches every target in the package, so an entry here can @@ -318,16 +378,21 @@ fn unsafe_gate_holds(lints: &str) -> bool { // Named, not merely non-zero: a scratch crate that failed to build for some // unrelated reason would otherwise read as an enforcing gate. let named = diagnostics.contains("undocumented_unsafe_blocks"); + // And rejected, not merely named: at `warn` the lint still fires and still + // prints its name, but clippy exits 0 and the undocumented `unsafe` ships. + // A gate that only warns is not a gate, so naming alone must not read as + // one — `level_enforces` agrees, counting `deny`/`forbid` and nothing else. + let rejected = !out.status.success(); // And the converse trap, which this test walked into while being written: a // manifest cargo *rejects* names no lint either, so it would read as "gate // off" and quietly agree with whatever the reader said. A failure that is // not the lint firing means the probe broke, not that the gate is open. assert!( - named || out.status.success(), + named || !rejected, "the probe crate failed for a reason other than the gate — the manifest \ under test may not be valid TOML at all:\n{lints}\n{diagnostics}" ); - named + rejected && named } /// The reader's verdict must match what clippy actually does, form by form. @@ -384,6 +449,16 @@ fn the_manifest_reader_agrees_with_clippy() { "the lint downgraded outright", "[lints.clippy]\nundocumented_unsafe_blocks = \"allow\"\n".to_string(), ), + ( + // Measured, not assumed: at `warn` the lint still fires and still + // prints its own name, but clippy exits 0 and the undocumented + // `unsafe` compiles. An oracle keyed on the name alone read that as + // an enforcing gate. The reader has always counted `deny`/`forbid` + // and nothing else, so this case is what holds the two to the same + // meaning of "enforcing". + "the lint downgraded to a warning", + "[lints.clippy]\nundocumented_unsafe_blocks = \"warn\"\n".to_string(), + ), ( // Both dotted under one `[lints]` header. `[lints.clippy]` followed // by a second `[lints]` is a duplicate key and not valid TOML — a From 776ebb71f163478598826f9c707d89e9520c3145 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 05:47:34 +0000 Subject: [PATCH 5/5] fix(tests): count a warn attribute as re-opening the gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gate_reopeners` recognised `allow` and `expect` but not `warn`, so a module-level `#![warn(clippy::unwrap_used)]` — which overrides the crate-root `deny` — read as clean. Measured against the pinned 1.95.0 rather than assumed, and the mechanism is not quite the one the review described: with that attribute in a `src/helper.rs`, clippy demotes `unwrap_used` to a warning, but CI's `cargo clippy --all-targets -- -D warnings` promotes it straight back to an error, so the build still fails today. The downgrade is worth flagging anyway. A `deny` in the source is enforced by the source; a `warn` is enforced by a flag in a workflow file, so the downgrade moves the gate out of the file this test can see and makes the `deny` it guards decorative. `level_enforces` already draws that line in the same place for the manifest, counting `deny`/`forbid` and nothing else — this keeps the two readers saying the same thing about the same word. Flagged and not-flagged fixtures both extended: `#![warn(clippy::…)]`, a `cfg_attr`-nested warn and a warn of the whole `restriction` group count; `#[warn(dead_code)]` and a *tightening* `#[deny(clippy::…)]` do not. The flagged set fails against the old classifier. --- .../steel-detailer-lookup/tests/lint_gates.rs | 51 ++++++++++++++----- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs b/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs index 7e902532d..795244023 100644 --- a/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs +++ b/20-agents/aeco/engineering/steel-detailer-lookup/tests/lint_gates.rs @@ -22,10 +22,12 @@ //! * `src/main.rs` still carries the gate verbatim; //! * no production source under `src/` — every `.rs` file, not just the crate //! root, since a module can `#![allow]` its way out from under one — has -//! re-opened it with an `#[allow]` / `#[expect]` / a `clippy::restriction` -//! group allow, including one rustfmt has wrapped across lines or nested in -//! a `cfg_attr` predicate, nor from `[lints.clippy]` in the manifest, where -//! a group entry that outranks a specific `deny` switches it off. +//! re-opened it with an `#[allow]` / `#[expect]` / `#[warn]` — a `warn` +//! overrides an outer `deny` and leaves only CI's `-D warnings` enforcing +//! it — or a `clippy::restriction` group override, including one rustfmt +//! has wrapped across lines or nested in a `cfg_attr` predicate, nor from +//! `[lints.clippy]` in the manifest, where a group entry that outranks a +//! specific `deny` switches it off. //! //! The last two scan artefacts that are correct today — the real sources under //! `src/`, the real `Cargo.toml` — so they would report clean both when they @@ -509,6 +511,14 @@ fn gate_reopener_classifier_matches_its_contract() { // honours the attribute in full. "#[allow(\n clippy::unwrap_used,\n reason = \"a long justification that pushes rustfmt into wrapping this attribute\"\n)]\nfn f() {}\n", "#[cfg_attr(\n not(test),\n allow(clippy::expect_used)\n)]\nfn f() {}\n", + // A module-level `warn` overrides the crate-root `deny`, leaving only + // CI's `-D warnings` between the `unwrap()` and a green build. Measured + // against the pinned 1.95.0: with this in a `src/helper.rs`, clippy + // demotes `unwrap_used` to a warning, and `cargo clippy` on its own + // exits 0. + "#![warn(clippy::unwrap_used)]\n\npub fn f(s: &str) -> i32 { s.parse().unwrap() }\n", + "#[cfg_attr(windows, warn(clippy::expect_used))]\nfn f() {}\n", + "#![warn(clippy::restriction)]\n", ] { assert!( !gate_reopeners(source).is_empty(), @@ -519,6 +529,11 @@ fn gate_reopener_classifier_matches_its_contract() { for source in [ "#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))]\n", "#[allow(dead_code)]\nfn f() {}\n", + // `warn` counts only for a gated lint. Raising an ungated one is + // ordinary code, not a gate being taken apart. + "#[warn(dead_code)]\nfn f() {}\n", + // Nor is tightening a gated lint further. + "#[deny(clippy::unwrap_used)]\nfn f() {}\n", "// an #[allow(clippy::unwrap_used)] mentioned in prose, not an attribute\n", "/* #[allow(clippy::unwrap_used)] in a block comment */\n", "let s = \"#[allow(clippy::unwrap_used)]\";\n", @@ -750,8 +765,8 @@ fn gate_reopeners(source: &str) -> Vec { found } -/// `true` when `collapsed` sets an `allow` or `expect` lint level *anywhere* -/// inside it, including nested in a `cfg_attr` payload. +/// `true` when `collapsed` sets an `allow`, `expect` or `warn` lint level +/// *anywhere* inside it, including nested in a `cfg_attr` payload. /// /// Nesting is the point. A prefix test (`starts_with("#[allow(")`) misses /// `#[cfg_attr(not(test), allow(clippy::unwrap_used))]`, which clippy honours in @@ -759,15 +774,27 @@ fn gate_reopeners(source: &str) -> Vec { /// the first thing someone reaching for a target-conditional override would /// write. /// +/// `warn` counts alongside `allow` and `expect`. It reads like a lesser thing +/// and is not: a module-level `#![warn(clippy::unwrap_used)]` overrides the +/// crate-root `deny`, after which the only thing still rejecting the `unwrap()` +/// is the `-D warnings` in CI's clippy invocation — measured, and it does still +/// reject today. That is the point. A `deny` in the source is enforced by the +/// source; a `warn` is enforced by a flag in a workflow file, so downgrading one +/// to the other moves the gate somewhere this test cannot see and makes the +/// `deny` it is guarding decorative. [`level_enforces`] already draws the line +/// in the same place for the manifest, counting `deny`/`forbid` and nothing +/// else; this keeps the two readers saying the same thing about the same word. +/// /// A lint level is recognised by its position rather than by enumerating -/// wrappers: with whitespace removed, `allow(`/`expect(` counts only when the -/// character before it opens a list — `[`, `(` or `,`. That admits `#[allow(`, -/// `#![allow(` and any `cfg_attr(, allow(` depth, while the crate's own -/// `deny(…)` gate is left alone. Whitespace is stripped rather than trusted, so -/// `#[allow (clippy::unwrap_used)]` — which clippy honours — still counts. +/// wrappers: with whitespace removed, `allow(`/`expect(`/`warn(` counts only +/// when the character before it opens a list — `[`, `(` or `,`. That admits +/// `#[allow(`, `#![allow(` and any `cfg_attr(, allow(` depth, while the +/// crate's own `deny(…)` gate is left alone. Whitespace is stripped rather than +/// trusted, so `#[allow (clippy::unwrap_used)]` — which clippy honours — still +/// counts. fn opens_a_lint(collapsed: &str) -> bool { let dense: String = collapsed.chars().filter(|c| !c.is_whitespace()).collect(); - ["allow(", "expect("].iter().any(|level| { + ["allow(", "expect(", "warn("].iter().any(|level| { dense.match_indices(level).any(|(at, _)| { // A level at index 0 has no opening bracket before it, so it is not // an attribute at all.