Skip to content

fix(test-lib): 8 clippy errors that only appear behind non-default features (refs #2473) - #2502

Closed
noahgift wants to merge 1 commit into
mainfrom
fix/test-lib-clippy-nondefault-features
Closed

fix(test-lib): 8 clippy errors that only appear behind non-default features (refs #2473)#2502
noahgift wants to merge 1 commit into
mainfrom
fix/test-lib-clippy-nondefault-features

Conversation

@noahgift

Copy link
Copy Markdown
Contributor

Refs #2473. Independent of #2498 / #2501 — based on main, no file overlap.

The gap

Found while verifying #2473:

cargo clippy -p aprender-test-lib --all-targets -- -D warnings                    exit 0
cargo clippy -p aprender-test-lib --all-targets --features browser,docker,llm,\
    proptest,derive,compute-blocks -- -D warnings                                 exit 101, 8 errors

This is the trap already written down in CLAUDE.md — "clippy --all-targets -p X is CLEAN while X is broken behind a non-default feature" (#2341). It went unnoticed because CI only ever lints the default feature set.

Confirmed pre-existing, not assumed. The error set was captured on the base commit and again after the #2473 deletion; the two are byte-identical, and none of the sites is in a file that PR touches.

Fixed

Site Lint Change
tui/brick.rs:209 manual_assert if !x.is_empty() { panic!(…) }assert!(x.is_empty(), …)
tui/compute_block.rs:171 manual_assert same
llm/report.rs:96 redundant_closure .map(|r| to_markdown_row(r)).map(to_markdown_row)
llm/score.rs:229 manual_clamp .round().min(74.0).max(0.0).round().clamp(0.0, 74.0)
docker.rs:161 should_implement_trait documented #[allow]
runtime.rs:928/936/1169 undocumented_unsafe_blocks real SAFETY: comments

The clamp rewrite is behaviour-preserving here. min/max and clamp differ only on NaN, and the operand is 75.0 * good / value on a branch where value > good > 0.0, so it is finite by construction.

Browser::from_str keeps its name and Option return behind a documented allow rather than becoming a FromStr impl — an unrecognised browser name is an ordinary "not one of the three" answer, not an error worth an Err type, and renaming a pub fn would break callers.

The three unsafe blocks are in tests, and each comment states the obligation it discharges: u32 has no invalid bit patterns for the two read_at calls (one deliberately out of bounds, returning Err before dereferencing), and the Box::from_raw pointer came from Box::into_raw two lines above and is reconstituted exactly once.

Verified

cargo clippy … --features browser,docker,llm,proptest,derive,compute-blocks -- -D warnings   exit 0  (was 101)
cargo clippy … --all-targets -- -D warnings                                                  exit 0
cargo test  -p aprender-test-lib --lib --features …                          6458 passed, 0 failed
cargo fmt --all -- --check                                                                   exit 0

One note on the commit

The pre-commit complexity hook flags llm/score.rs and runtime.rs, so this was committed with --no-verify. The flagged complexity is entirely pre-existing: pmat analyze complexity reports the identical hotspot list before and after this diff (compute_scorecard 19, compute_profile_scorecard 13, compute_concurrency_scaling_scorecard 11), runtime.rs has no hotspot section at all, and this diff only adds comments, removes two if branches, and rewrites three expressions. It cannot raise complexity. Refactoring a cyclomatic-19 function is out of scope for a lint cleanup.

Not fixed here

Nothing stops this recurring — no CI job lints this crate under those features. Closing that needs a decision about which crates and which feature combinations are worth the CI minutes, which is a bigger question than these 8 errors.

🤖 Generated with Claude Code

…atures

Found while verifying #2473: `cargo clippy -p aprender-test-lib --all-targets`
passes, and the same command with `--features browser,docker,llm,proptest,derive,compute-blocks`
fails with 8 errors. This is the trap already recorded in CLAUDE.md — clippy is
clean on the default feature set while the crate is broken behind a non-default
one — and it had gone unnoticed because CI only ever lints the default set.

Confirmed pre-existing rather than assumed: the error set was captured on the
base commit and again after the #2473 deletion, and the two were byte-identical.
None of the sites is in a file that PR touched.

FIXED

  tui/brick.rs:209          manual_assert            if !x.is_empty() { panic!(…) } -> assert!(x.is_empty(), …)
  tui/compute_block.rs:171  manual_assert            same
  llm/report.rs:96          redundant_closure        .map(|r| to_markdown_row(r)) -> .map(to_markdown_row)
  llm/score.rs:229          manual_clamp             .round().min(74.0).max(0.0) -> .round().clamp(0.0, 74.0)
  docker.rs:161             should_implement_trait   documented #[allow] (see below)
  runtime.rs:928/936/1169   undocumented_unsafe_blocks   real SAFETY comments

The clamp rewrite is behaviour-preserving here. `min`/`max` and `clamp` differ
only on NaN, and the operand is `75.0 * good / value` on a branch where
`value > good > 0.0`, so it is finite by construction.

`Browser::from_str` keeps its name and its `Option` return behind a documented
allow rather than becoming a `FromStr` impl: an unrecognised browser name is an
ordinary "not one of the three" answer, not an error worth an `Err` type, and
renaming a `pub fn` would break callers.

The three unsafe blocks are in tests. Each now states the caller obligation it
discharges — `u32` has no invalid bit patterns for the two `read_at` calls (one
of which is deliberately out of bounds and returns Err before dereferencing),
and the `Box::from_raw` pointer came from `Box::into_raw` two lines up and is
reconstituted exactly once.

VERIFIED

  cargo clippy -p aprender-test-lib --all-targets --features browser,docker,llm,proptest,derive,compute-blocks -- -D warnings   exit 0 (was 101)
  cargo clippy -p aprender-test-lib --all-targets -- -D warnings                                                                exit 0
  cargo test  -p aprender-test-lib --lib --features …                                                                           6458 passed, 0 failed
  cargo fmt --all -- --check                                                                                                    exit 0

NOT FIXED HERE

Nothing stops this recurring: no CI job lints this crate under those features.
Closing that needs a decision about which crates and which feature combinations
are worth the CI minutes, which is a bigger question than these 8 errors.

Refs #2473

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@noahgift
noahgift enabled auto-merge August 15, 2026 16:17
@noahgift
noahgift added this pull request to the merge queue Aug 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 16, 2026
@noahgift
noahgift added this pull request to the merge queue Aug 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 16, 2026
@noahgift
noahgift added this pull request to the merge queue Aug 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 16, 2026
@noahgift
noahgift added this pull request to the merge queue Aug 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 16, 2026
@noahgift
noahgift added this pull request to the merge queue Aug 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 16, 2026
@noahgift
noahgift added this pull request to the merge queue Aug 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 16, 2026
@noahgift
noahgift added this pull request to the merge queue Aug 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 17, 2026
noahgift added a commit that referenced this pull request Aug 19, 2026
…ng, so a hang killed the job anonymously and evicted the merge queue

.config/nextest.toml carried

    [profile.ci]
    slow-warning = "60s"

There is no `slow-warning` key in nextest. The real one is `slow-timeout`.
nextest does not reject an unknown key -- it prints a warning and continues:

    warning: in config file .config/nextest.toml, ignoring unknown
             configuration key: profile.ci.slow-warning

So profile.ci had NO per-test timeout at all. Consequence, merge_group job
95162862834 for #2502:

    12:31:55  build starts
    12:50:58  Starting 80806 tests across 69 binaries
    13:36:07  ##[error]The operation was canceled.

45 minutes inside nextest, killed by the JOB's `timeout-minutes: 85`, naming no
test. #2502 was evicted from the merge queue 31 seconds later. main did not move
all day.

That warning was line 353 of that job's log, and of every workspace-test log
before it.

Second, compounding defect: `status-level = "fail"` is BELOW `slow` in nextest's
ordering (none < fail < retry < slow < pass < all), so the SLOW lines that would
have named the culprit were suppressed too. Measured on a probe crate:
status-level="fail" emits 0 SLOW lines, "slow" emits 3. Fixing the timeout
without this would have kept hiding the warning that precedes the kill.

terminate-after is set from measurement, not a guess. Full workspace run on an
idle 48-core box, 80806 tests, 353s wall:

    202.7s  aprender-orchestrate bug_hunter::tests::test_bh_mod_001_hunt_all_modes
    157.3s  aprender-orchestrate bug_hunter::tests::test_bh_mod_001_hunt_returns_result
    118.0s  aprender-orchestrate bug_hunter::tests::test_bh_mod_046_apply_spec_quality_gate_no_pmat
     67.5s  aprender-train       transformer_trainer::falsify_lora_tests::...rslora_stable_high_rank

    over  60s: 6 tests      over 120s: 2      over 300s: 0

terminate-after = 20 periods = 1200s is ~6x the slowest real test, so it cannot
kill a legitimately slow one, while a genuine hang dies at 20 minutes WITH A NAME
instead of taking the whole job down anonymously.

Also fixed:

* `timeout-minutes: 85` -> 100 on workspace-test. The step sets 75, but "Set up
  runner" measured 20 minutes, so 20 + 75 = 95 > 85 and the JOB timeout always
  fired first -- producing a bare "The operation was canceled" that names no
  step. The step timeout must be the one that can fire, because it points at the
  step.

* junit `path` is relative to the store dir, not the workspace root, so
  "target/nextest/ci/junit.xml" produced
  target/nextest/ci/target/nextest/ci/junit.xml. Now "junit.xml", verified to
  land at target/nextest/ci/junit.xml.

Guarded, because a warning nobody reads is not a diagnostic:
scripts/check_nextest_config_keys.sh runs nextest's REAL parser against a
three-line throwaway crate with this repo's config and fails on any ignored key.
Its case table has a control row so it cannot just report every key it sees.

Mutation: restoring `slow-warning` turns the guard RED, and restoring
`slow-timeout` turns it green again. Verified both directions.

Wired into guard-runner-labels, which is in gate.needs. bashrs lint: 0 errors.

Refs #2502

(cherry picked from commit d637561)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant