ci: gate the steel-detailer-lookup crate, and fix what it had drifted into - #408
Conversation
… into
`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).
|
@codex review Generated by Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6cffec20b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "undocumented_unsafe_blocks", | ||
| "restriction", | ||
| ]; | ||
| gated.contains(&key) && matches!(value, "allow" | "warn") |
There was a problem hiding this comment.
When Cargo.toml contains Cargo's valid inline-table form, such as restriction = { level = "allow", priority = 1 }, value remains the whole table and never matches "allow" or "warn". With the pinned Cargo/Clippy 1.95, that group entry outranks the existing undocumented_unsafe_blocks = "deny", allowing undocumented unsafe code while nobody_reopened_the_gate_from_the_manifest remains green. Parse both the level and priority, as the existing reader in cli/tests/lint_gates.rs does.
Useful? React with 👍 / 👎.
| relaxes.iter().any(|level| trimmed.contains(level)) | ||
| && targets.iter().any(|target| trimmed.contains(target)) |
There was a problem hiding this comment.
When an override is formatted across lines—for example an #[allow(...)] carrying a long reason—rustfmt 1.95 can leave allow( on one line and clippy::unwrap_used on another. Because the caller checks each line separately and this predicate requires both tokens on the same line, nobody_reopened_the_gate_from_source reports no reopening even though Clippy honors the attribute and accepts the production unwrap(). Parse complete attributes rather than individual lines, as gate_reopeners in cli/tests/lint_gates.rs already does.
Useful? React with 👍 / 👎.
Both findings from Codex's review of d6cffec, 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.<lint>` 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.
|
Both P2 findings were real, and both were in the negative control rather than in the gate it guards — a control that misses the form it exists to catch is the worst kind of green. Fixed in Wrapped attributes — Inline-table lint entries — Verified end-to-end, not only against the classifiers' own fixtures — injecting each finding's exact shape into the real files:
Both pass again once restored. @codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91fa082652
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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) { |
There was a problem hiding this comment.
With the pinned Cargo 1.95.0, [lints.clippy] restriction = {\n level = "allow",\n priority = 1,\n} is accepted and the group outranks undocumented_unsafe_blocks = "deny" at priority 0, but this line-by-line reader evaluates only restriction = {, records an empty level at priority 0, and ignores the continuation lines, leaving nobody_reopened_the_gate_from_the_manifest green. Fresh evidence beyond the previously fixed single-line inline-table case is Cargo's accepted multiline form; accumulate the complete value or parse the manifest as TOML.
Useful? React with 👍 / 👎.
| .trim_start_matches('=') | ||
| .trim() | ||
| .chars() | ||
| .take_while(|c| c.is_ascii_digit() || *c == '-') |
There was a problem hiding this comment.
With the pinned Cargo 1.95.0, restriction = { level = "allow", priority = +1 } is valid and disables the priority-0 specific deny, but this filter rejects +, causing the priority parse to default to 0 and the strict outranking comparison to miss the reopening. Fresh evidence beyond the previously fixed unsigned-priority case is Cargo's accepted explicitly positive integer syntax; include + when parsing the signed priority.
Useful? React with 👍 / 👎.
Both findings from Codex's review of 91fa082, 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.<name>`. That path resolves `[lints.clippy]`, the dotted `[lints] clippy.<lint>` key and the `[lints.clippy.<lint>]` 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.
|
Both round-2 findings were real, and checking them turned up a third the review hadn't named. Fixed in I verified each against the pinned cargo/clippy 1.95.0 before changing anything — building a scratch crate with an undocumented
Four missed forms across two rounds is a sufficient answer to whether a hand-rolled reader can cover TOML, so The more important change is that the reader no longer has the last word. It earned its keep immediately by rejecting one of its own fixtures: Verified end-to-end against the real One thing I deliberately did not do: @codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b61bdd019
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| #[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"); |
There was a problem hiding this comment.
When implementation is moved into a new module such as src/helper.rs, that module can use #![allow(clippy::unwrap_used)] to override the crate-root deny; with the pinned Clippy 1.95.0 this permits an unwrap() while this test remains green because it reads only src/main.rs. Recursively scan all production .rs files under src so the guard continues to cover the crate after it is split into modules.
Useful? React with 👍 / 👎.
| "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 |
There was a problem hiding this comment.
When the lint is configured as warn or force-warn, Clippy exits successfully but still emits a diagnostic containing undocumented_unsafe_blocks, so this function returns true and reports that the gate “HOLDS” even though the probe was not rejected. The pinned cargo clippy --help describes -W / --warn [LINT] as “Set lint warnings”; require both a failing status and the named diagnostic before treating the gate as enforcing.
Useful? React with 👍 / 👎.
Stopping here — both round-3 findings are real, and I'm at my round capI reproduced both against the pinned 1.95.0 rather than take them on faith. Both are genuine, and I am not rejecting either. 1. The source scan reads only #[allow(clippy::unwrap_used)]
pub fn parse(s: &str) -> i32 { s.parse::<i32>().unwrap() }…called from 2. Why I'm not pushing a third fix. The routine that opened this PR caps me at two fix-and-re-review rounds, then requires me to stop and leave the PR for a human rather than keep cycling unattended. I've used both rounds ( State as it stands on Carried over from the last round, still open: Generated by Claude Code |
…nostic Both findings from Codex's review of 1b61bdd, 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.
|
@codex review Both P2 findings on
Verified on the pinned toolchain (1.95.0) with CI's apt deps: Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8a49a982c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// `#[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| { |
There was a problem hiding this comment.
warn attributes as gate reopeners
When a production module uses #![warn(clippy::unwrap_used)] (or the corresponding expect_used, undocumented_unsafe_blocks, or restriction form), it overrides the outer deny with a warning, so Clippy exits successfully and the prohibited code compiles. This classifier only recognizes allow and expect, leaving nobody_reopened_the_gate_from_source green in that scenario; include warning-level overrides in the reopener detection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed on the real crate at a8a49a98, with one correction to the stated impact that changes the severity.
The classifier gap is real. opens_a_lint (line 770) matches only allow( and expect(. I added src/helper.rs with #[warn(clippy::unwrap_used)] and an unwrap(), called it from run_describe, and nobody_reopened_the_gate_from_source passed — so a warn-level override does slip past the guard. Worth fixing.
But "Clippy exits successfully and the prohibited code compiles" only holds without -D warnings, and that is not how this gate is invoked. Measured on the same crate, same module:
| clippy invocation | result |
|---|---|
cargo clippy (bare) |
exit 0 — unwrap() accepted, as the comment says |
cargo clippy --all-targets -- -D warnings (what ci.yml runs) |
exit 101, error: used unwrap()on aResult value |
-D warnings promotes the downgraded lint straight back to an error, which is exactly what it cannot do for allow/expect — and that asymmetry is why those two are the reopeners that actually bite. So this is defense-in-depth (it would matter to someone running bare cargo clippy locally, or if -D warnings were ever dropped from CI), not a live hole. CI catches it today.
The fix is a one-liner — add "warn(" to the list at line 770, plus a fixture in gate_reopener_classifier_matches_its_contract. Note force-warn as mentioned isn't an attribute form, only a --force-warn flag / [lints] level, so the attribute classifier doesn't need it; the manifest side already handles it, since level_enforces counts only deny/forbid.
I'm not pushing it. This routine caps me at two fix-and-re-review rounds and I hit that at 1b61bdd0; a later session did round three in a8a49a98, and two agents pushing to one branch is a worse failure mode than one clear note. Flagging for whoever takes the next round.
Same hole in cli/tests/lint_gates.rs:301 — identical ["allow(", "expect("], since this file was ported from it. That's now the second defect this PR has found in cli/'s copy (the four manifest-parsing holes being the first), which is a decent argument that the follow-up PR should port the fixed version back rather than patch it twice.
Generated by Claude Code
`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.
|
@codex review Fixed in Treat The claim was that a module-level
So the gate does still reject this today, and the PR was not shipping a live hole. I made the change regardless, because the finding is right about the shape even where it is wrong about the blast radius: a Both fixture sets grew: Gates on the pinned toolchain with CI's apt deps: Round 2 of 2 for this pass. If a further finding comes back, I stop and leave this for a human rather than keep cycling unattended. Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
20-agents/aeco/engineering/steel-detailer-lookupis the repo's second Rust crate — it ships in every install archive next toaware— and no CI gate has ever run on it. It declares its own[workspace], socd cli && cargo …(the only Rust CI runs) never reaches it. The only thing that ever compiled it wascargo build --releaseat release time, which runs neither clippy nor its five unit tests..unwrap()calls in non-test code (§Code style: "Nounwrap()outside of tests + main entry") and edition 2021 (§Tech stack: "Rust (edition 2024)").cli/already carries, wires it intoci.yml, and adds a negative control so the gate can't be quietly removed.Type of change
Decalog check
The gap
release.ymlbuilds this crate on all three runners and stages it into the archive;scripts/install.sh,scripts/install.ps1andpackaging/wix/aware.wxsall place the three binaries. It is shipped code. Butci.yml'sgatesjob is entirelyworking-directory: cli, and this crate's[workspace]isolates it, so:cli/steel-detailer-lookup(before)cargo fmt --checkcargo clippy -D warningscargo testunwrap_used/expect_useddenycli/src/main.rsundocumented_unsafe_blocksdenycli/Cargo.tomlWhat changed
Violations fixed at root cause (no lint weakened, no
#[allow], no test deleted):print_json, which reports a serialization failure andexit(2)— the code this binary already uses for every other hard error — instead ofunwrap()-panicking with exit 101. 101 is a code the cli transport reads as a failed invocation with no parseable reason (cli/src/runtime/invoker.rs).edition = "2024". rustfmt's 2024 style edition reflows three long statements; no behaviour change.Gates added, mirroring
cli/rather than inventing anything:#[cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))]on the crate root — byte-identical to the attribute incli/src/main.rs, and for the reason stated there:[lints.clippy]would also fire on the unit tests, where CLAUDE.md permitsunwrap().[lints.clippy] undocumented_unsafe_blocks = "deny"in itsCargo.toml, matchingcli/Cargo.toml. The crate has nounsafetoday; the gate is what keeps it that way.ci.yml: fmt + clippy + test on the crate with the pinned toolchain, and the rust-cache extended to both workspaces.Negative control
tests/lint_gates.rs(new), mirroringcli/tests/lint_gates.rs. It compiles scratch crates carrying the gate and asserts clippy rejectsunwrap()/expect()in non-test code and rejects for that lint specifically (so a probe that merely fails to build can't look like an enforcing gate), accepts clean code, still permitsunwrap()under#[cfg(test)]— with a type-error witness so that assertion can't pass by never compiling a test target — and thatsrc/main.rsandCargo.tomlstill carry the gates unrelaxed.gate_reopener_classifier_matches_its_contractdrives both scanners over synthetic input with known answers, since they otherwise report clean whether they work or have stopped matching anything.Also verified by hand. Restoring one of the original calls:
Verification
Run locally on Linux with the pinned toolchain (1.95.0) and CI's apt deps:
cli/:cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings,cargo test— all green on the base commit and unchanged by this PR (onlyci.ymlis touched there).steel-detailer-lookup: fmt clean, clippy clean,CI=1 cargo test→ 5 + 5 + 5 unit tests and 6 lint-gate tests pass.describe,lookup --list,lookup --rule <hit>,lookup --rule <miss>,lookup --category, and the--json-stdintransport all emit identical JSON and identical exit codes (0 found / 1 not-found standalone / 0 not-found under--json-stdin/ 2 hard error).Notes for reviewers
rust-toolchain.tomlfor this crate, deliberately.cli/rust-toolchain.tomlsays it is the single source of truth for the pin, andci.ymlreads it rather than restating it. A second pin file would be a second thing to drift. In CI this is exact —dtolnay/rust-toolchainsets the pinned channel as the rustup default, so the new step uses it. Locally, a developer in this directory gets their own default toolchain; that residual gap is stated rather than papered over.tests/lint_gates.rsis a copy ofcli/'s, not a shared helper. The two live in separate cargo workspaces, 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. The duplication is the point.tempfileadded as a dev-dependency for the scratch crates. Dev-only, socargo build --release— whatrelease.ymlruns — never builds it.serde_json::Valueis not realistically fallible, so the sixunwrap()s were unlikely to fire in practice. They were still the rule violation, and "not expected to fail" is precisely the claim the gate no longer takes on trust.Generated by Claude Code