fix(ci): make Rust Build + rust-ci able to go green; unmask clippy/fmt - #144
Merged
Merged
Conversation
…y/fmt - e2e.yml: `toolchain: v1` is not a toolchain name (rustup: "invalid toolchain name: 'v1'"), so "Rust Build + Unit Tests" died before building. Use `stable` with clippy + rustfmt components. - e2e.yml: drop `|| true` from the Clippy and Format steps; they could never fail. - cargo fmt over attestation.rs / keys_cli.rs (rust-ci's first red step). - Clear every `clippy --all-targets -D warnings` lint (each target's errors masked the next): rand 0.9 thread_rng -> rng, needless mut / borrow / parens, &PathBuf -> &Path, dead `root_path` field, dead store in the transactions bench. - jk-keys imported `attestation` and `keys` via `mod`, compiling a second private copy of each: the lib's public API read as dead code there and 10 unit tests ran twice. Import them from the lib crate instead. Unique test count is unchanged (107; was 117 listed with the 10 duplicates). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VxcAoyMQe7CjQwCKL18Mm4
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (17)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
|
| Layer / File(s) | Summary |
|---|---|
Workflow checks .github/workflows/e2e.yml |
The Rust jobs select the stable toolchain. Clippy checks all targets with warnings as errors, and Clippy and formatting failures fail the job. |
Key and attestation updates crates/januskey-cli/src/{keys.rs,attestation.rs,obliteration.rs,keys_cli.rs} |
Key and obliteration code uses rand::rng(). HMAC initialisation errors now map to I/O errors. KeyManager no longer stores root_path, and keys_cli.rs imports the modules from januskey. |
CLI command interfaces crates/januskey-cli/src/{keys_cli.rs,main.rs} |
CLI command helpers use &Path instead of &PathBuf. The changes also update optional-flag checks, error formatting, and status output. |
File reads and test updates crates/januskey-cli/src/{lib.rs,keys.rs,main.rs,obliteration.rs,operations.rs}, crates/januskey-cli/tests/*, crates/reversible-core/src/{metadata.rs,transaction.rs}, tests/aspect/cross_cutting_test.sh |
File reads retain their existing limits and parsing behaviour without mutable file bindings. Test helpers and filters are updated, and documentation checks accept .md or .adoc files. |
Benchmark and core call sites benches/januskey_benchmarks.rs, crates/reversible-core/src/manifest.rs |
The benchmark initialises its active flag directly and passes hash buffers by value. The manifest appends its version line as a literal and passes the finalized hash buffer by value. |
Priority: ➖ Normal
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Merge Risk: ⚪ Minimal · up to e6fee
The workflow will now fail on Clippy warnings and formatting errors, while the described code changes preserve existing behavior and return HMAC initialization errors. No merge-blocking regression is identified beyond normal CI validation.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 60.34% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 58 functions across 16 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Description check | ✅ Passed | The description clearly explains the Rust CI fixes, Clippy and rustfmt changes, test validation, aspect-check updates, and deferred checks. It is directly related to the changeset. |
| Title check | ✅ Passed | The title clearly identifies the primary change: fixing Rust CI and enabling Clippy and rustfmt failures to fail correctly. It is concise and related to the changeset. |
Full details: Docstring Coverage
Explanation
Docstring coverage is 60.34% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 58 functions across 16 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Autopilot is currently an internal CodeRabbit preview.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the stable build,
Then hops where Rust warnings once were stilled.
Key bytes whirl and hashes flow,
Bounded reads keep files in tow.
Markdown, AsciiDoc both appear,
The rabbit thumps: the checks are clear.
Comment @coderabbitai help to get the list of available commands.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
….adoc
- dtolnay/rust-toolchain@v1 needs a `toolchain` input; the Criterion,
E2E Lifecycle and Panic Attack jobs had none ("'toolchain' is a required
input"). Set `stable`.
- tests/aspect: SECURITY/ARCHITECTURE/PROOF-NEEDS/TOPOLOGY were migrated
to .adoc, so the .md-only existence checks failed 4/29. Accept either,
as README already did. Negative control: removing TOPOLOGY.adoc makes
the check FAIL.
- attestation: propagate HMAC new_from_slice's error instead of expect()
(Hypatia expect_in_hot_path; the fn already returns io::Result).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VxcAoyMQe7CjQwCKL18Mm4
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
|
Open the task to resolve the delivery issue or retry. |
Contributor
|
Autopilot could not be updated. Open Coding to check access and billing. |
hyperpolymath
added a commit
that referenced
this pull request
Oct 2, 2026
…counts (#146) ULTRAPLAN **P1-0, PR B of three** (B: ledger + counts; C: escape-hatch counter; D: `workflow_call`). > **Stacked on #144.** This branch contains #144's commit e6fee67, so review only **6f31e89** and the docs follow-up. It stays a draft until #144 merges; then it is rebased onto main and marked ready. ## What it does `PROOF-NEEDS.adoc` gains a `=== Claims ledger` table. Each row is `claim | status | artefact | command`, with status PROVEN / TESTED / ASSUMED / DESIGNED / OPEN, and each row fits on one line. `dashboard-check --check .` (already run by `dashboard-check.yml`) now does the following: | Status | Rule | |---|---| | TESTED | The artefact `file::test` must exist and name the test. The command must exit 0, **and** some output line must show that test passing (`ok`/`PASS`). That rejects `true` and a filter that runs 0 tests. | | PROVEN | The file must name the theorem, and the checker command must exit 0. | | ASSUMED / DESIGNED / OPEN | The command must be `-`. The artefact must be an existing file or `#N`. | | any | A wrapped row, an unknown status, or a missing table fails and names the line. | **Dashboard counts.** On README / EXPLAINME / TOPOLOGY / READINESS, a number followed within two words by *test(s)* must equal `cargo test --workspace --locked -- --list` (lines ending `: test`). A number followed by *proof(s)/theorem(s)* must equal the number of PROVEN rows. Percentages are excluded, and so are numbers in another table cell. **Ledger contents:** 7 TESTED rows (obliteration ×3, execute∘undo, content-store round-trip, rollback, audit-chain verify) and 2 OPEN rows (#145). **0 PROVEN**, because the Idris2 ABI does not typecheck. ## Doc corrections the checker forced | Where | Was | Now | |---|---|---| | READINESS:11, :67; TOPOLOGY:82 | 67 tests | 119 tests | | READINESS:11, :63, :67; TOPOLOGY:80 | 30 (Idris2) proofs, row 16 ✓ | 0 checked (#145), row 16 ✗ | TOPOLOGY "Last updated" → 2026-10-02. ## Scope limits (stated, not implied) - **Only test/proof counts are reconciled here.** The OVERALL percentage is reconciled against STATE as before, but the per-component `N%` figures on TOPOLOGY are **not** checked. - **The 119 tests are workspace-wide and include dashboard-check's own 31.** Adding a test anywhere now requires updating the dashboard count in the same PR. That is the intended ratchet. - **PROVEN is exercised only by unit tests (fake runner), not by a real row**, because the repo has none. ASSUMED/DESIGNED have no rows either. - **Escape-hatch counting** (Axiom/Admitted/sorry/believe_me/postulate) is PR C, not this one. ## Verification (local, rust 1.97.1) - `cargo fmt --all -- --check` ✓; `cargo clippy --workspace --all-targets --locked -- -D warnings` ✓; `cargo test --workspace --locked`: 119 passed, 0 failed. - `cargo run -p dashboard-check -- --check .` → rc=0: `✓ 119 tests measured, 0 PROVEN claims`, all 7 TESTED rows ran and passed. - **Positive control 1:** on the old docs the checker printed 6 divergences (67 vs 118, 30 vs 0 ×3, plus READINESS:11) with rc=1. - **Positive control 2:** adding one unit test made it report `claims 118 but there are 119` on three lines with rc=1. It was green again after the update. - **Fixtures** (in `claims.rs`): `lying_verifier_fails` (exit 0, no output), `unchecked_skip_fails` (`running 0 tests`), `inflated_counts_fail_and_true_counts_pass`, `wrapped_row_is_rejected`, `proven_row_needs_theorem_and_passing_checker`, `open_rows_run_nothing_and_need_a_real_artefact`. - **CI:** dashboard-check run 37010504568 passed in **39 s** (13:03:41Z to 13:04:20Z), well under the job's 15 min timeout. Its log shows all 7 `✓ Tested` rows, both `✓ Open` rows and `✓ 119 tests measured, 0 PROVEN claims`. ## Red checks: inherited from #144 (e6fee67), deferred, not introduced here The red set on this head equals #144's, which equals main 539e6f9's. The ledger commit adds none. - `lint-workflows`: deferred to #135 (acceptance item 5) - `governance / Workflow security linter`: deferred to #135 (acceptance item 5) - `governance / Allowlist Preflight`: deferred to #135 (acceptance item 6, `Check live Actions policy`) - `Validate DEED manifests`: deferred to #135 (acceptance item 6) - `estate-audit`: deferred to #135 (acceptance item 6, `Code Hygiene Gate`) - `idris-abi (expected red until J1-3)`: expected red by design (#142) until #145 (ULTRAPLAN J1-3) makes Types.idr typecheck. **CodeRabbit** posted the status "Review rate limited" on 6f31e89 and has not reviewed this PR yet. Its review will be read before this PR is marked ready. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01VxcAoyMQe7CjQwCKL18Mm4 --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Two Rust jobs are red on
main(539e6f9). The claims ledger (ULTRAPLAN P1-0) needs acargo testthat CI actually runs, so these are fixed first.error: invalid value 'v1' for '[TOOLCHAIN]...': invalid toolchain name: 'v1'(e2e.yml:24)cargo fmt --checkdiff inattestation.rs,keys_cli.rs. Behind it, a stack of clippy-D warningserrors, each target masking the nextWhat
e2e.yml:toolchain: stablewith the clippy and rustfmt components.|| truefrom the Clippy and Format steps, which previously could not fail.--all-targets, matching rust-ci.cargo fmt.clippy --all-targets -D warningslints cleared:thread_rng→rng.mut, borrows, parentheses andformat!.&PathBuf→&Path(20 sites).KeyManager.root_pathfield and a dead store in the bench.jk-keysnow importsattestation/keysfrom the lib. It used to compile them as private modules viamod. That made the lib's public methods read as dead code, and it ran 10 unit tests twice.Evidence (local, Rust 1.97.1)
cargo fmt --all --check→ cleancargo clippy --locked --workspace --all-targets -- -D warnings→ rc 0cargo test --locked --workspace→ 107 passed, 0 failed--listdiff againstmain:src/keys_cli.rscopies ofattestation::tests::*(6) andkeys::tests::*(4).src/lib.rs, so no test was lost.cargo bench --no-run→ builds.Not in this PR
e2e.yml:98runspython3(banned estate-wide), behind|| true. It is recorded indev-notes/inbox/findings.md, not changed here.Second push (e6fee67)
e2e.yml: the Criterion, E2E Lifecycle and Panic Attack jobs calleddtolnay/rust-toolchain@v1with notoolchaininput. That step failed with "'toolchain' is a required input", so these jobs now passtoolchain: stable.tests/aspect/cross_cutting_test.sh:.adoc, but the checks still looked for.md, so 4 of 29 failed.TOPOLOGY.adocremoved, the check fails (28/29).attestation.rs: the HMAC key error is now propagated withmap_err(std::io::Error::other)?instead ofexpect(). Hypatia flagged it asexpect_in_hot_path, and the function already returnsio::Result.Partly addresses #135 (acceptance items 1 and 2 and
Run aspect tests) and #133 (clippy/fmt|| true). Neither issue is closed.Red checks on e6fee67: deferred, not introduced here
Every red below also fails on
main539e6f9. The only required context isscan / gitleaks, which is green.governance / Workflow security linter: deferred to main is red on 7 of 12 workflows; four e2e jobs die atdtolnay/rust-toolchain@v1, so the unit tests have no CI measurement #135 (acceptance item 5)lint-workflows: deferred to main is red on 7 of 12 workflows; four e2e jobs die atdtolnay/rust-toolchain@v1, so the unit tests have no CI measurement #135 (acceptance item 5)governance / Allowlist Preflight: deferred to main is red on 7 of 12 workflows; four e2e jobs die atdtolnay/rust-toolchain@v1, so the unit tests have no CI measurement #135 (acceptance item 6,Check live Actions policy)Validate DEED manifests: deferred to main is red on 7 of 12 workflows; four e2e jobs die atdtolnay/rust-toolchain@v1, so the unit tests have no CI measurement #135 (acceptance item 6)estate-audit: deferred to main is red on 7 of 12 workflows; four e2e jobs die atdtolnay/rust-toolchain@v1, so the unit tests have no CI measurement #135 (acceptance item 6,Code Hygiene Gate)idris-abi (expected red until J1-3): expected red by design (docs(security): placeholder key ids so required gitleaks passes #142) until J1-3: make the Idris2 ABI typecheck; delete or honestly restate the six unproved security claims #145 (ULTRAPLAN J1-3) makes Types.idr typecheck.🤖 Generated with Claude Code
https://claude.ai/code/session_01VxcAoyMQe7CjQwCKL18Mm4