Cleanups in the test suite and CI

[Xof]

This issue groups 9 related findings.

TESTS-CI-2 — counters_snapshot_does_not_mutate_when_engine_continues is a tautology that can never fail

Location: tests/counters.rs:108-124 · Severity: SMELL · Category: tests · Status: NEW

What the code does. The test does let snap = db.counters().unwrap(); let snap_copy = snap.clone();, performs a commit, then assert_eq!(snap, snap_copy, "ChiselCounters is a snapshot, not a live view"). ChiselCounters (src/stats.rs:81-88) is #[derive(Debug, Clone, Default, PartialEq, Eq)] over four plain u64 fields — no Cell, no reference, no interior mutability.

Why it is a problem. snap_copy is a bitwise copy of snap made before any work; nothing in the language could make two owned u64-only structs diverge. If counters() were changed tomorrow to return a live view (e.g. Rc<Cell<...>> fields, or a borrow of the engine's counters), this test would still pass, because the test only compares the snapshot against a clone of itself rather than against a freshly-read db.counters(). The test occupies the slot reserved for the snapshot contract while verifying nothing.

Direction of a fix. Compare the pre-work snapshot against the value of the field it should differ from: take snap before, do the commit, then assert snap.fsync_calls < db.counters().unwrap().fsync_calls AND snap.fsync_calls == snap_copy.fsync_calls. The second half is only meaningful once the first half proves the engine actually moved.

TESTS-CI-3 — ci.yml's audit job carries a 14-line "Permissions notes" comment describing a permissions: block and an annotation-posting action that the job no longer has

Location: .github/workflows/ci.yml:65-72 (comment) vs :77-84 (audit job) · Severity: SMELL · Category: ci

What the code does. Lines 66-72 read: "# Permissions notes: # * pull-requests: write lets the action post per-line annotations on PR runs. # * checks: write is needed on push-to-main runs — without it the action's annotation-posting step trips "Resource not accessible by integration"...". The audit: job that follows (lines 80-89) declares no permissions: key at all, and lines 85-86 record that rustsec/audit-check was replaced by a bare run: cargo audit, so there is no action and no annotation-posting step left to need those scopes.

Why it is a problem. A maintainer tightening token scope reads this block, believes audit depends on pull-requests: write / checks: write, and either grants them repo-wide (a real over-permission on a job that compiles and runs cargo install cargo-audit --locked, i.e. arbitrary third-party build scripts) or spends time chasing a permission failure that cannot occur. It also masks the fact that neither ci.yml nor wheels.yml declares any permissions: block, so every job silently inherits the repository default token scope.

Direction of a fix. Delete the stale Permissions notes block, and add an explicit top-level permissions: contents: read to ci.yml and wheels.yml (bench.yml already scopes per-job), overriding only where a job genuinely needs more.

TESTS-CI-4 — tests/counters.rs documents a 2-fsync commit protocol; the protocol is 3 fsyncs, and the assertion is one fsync weaker than the invariant

Location: tests/counters.rs:38-45 · Severity: SMELL · Category: comment-accuracy

What the code does. tests/counters.rs:39-40 says "// Commit calls fsync twice (data pages + superblock). Anything below this is a regression in the commit protocol." and asserts after.fsync_calls >= baseline.fsync_calls + 2 with the message "commit must perform at least 2 fsyncs (data + superblock)". The actual protocol is three: src/transaction/commit.rs:1 titles the module "the 3-fsync commit protocol", commit.rs:78 issues the I28 pre-drain cache.flush(), commit.rs:105 the data flush, commit.rs:180 the superblock fsync(); PageCache::flush fsyncs unconditionally at src/page_cache.rs:586 (self.io.fsync()?;, outside any dirty-page conditional), and src/page_cache.rs:578-580 names this "the SECOND of three fsyncs". tests/spillway_integration.rs:166 asserts the delta is exactly 3.

Why it is a problem. Two defects in one place. (1) The comment contradicts three other in-repo sources and would lead a maintainer editing the commit path to believe removing a flush is legal. (2) The assertion's own stated purpose — "anything below this is a regression in the commit protocol" — is not met: deleting the I28 pre-drain flush at commit.rs:78 (whose only job is keeping CacheFull off the persist_freemap path) drops the count to 2 and this test still passes. Only the spillway test catches it, and only incidentally.

Direction of a fix. Correct the comment to three fsyncs and change the assertion to == baseline + 3, matching tests/spillway_integration.rs:166 so the two tests pin the same number.

TESTS-CI-9 — The named-root suite is duplicated between src/recovery_tests.rs and tests/error_and_format.rs, contradicting recovery_tests.rs's own stated reason for existing

Location: src/recovery_tests.rs:867-1035 — five of the six duplicate, not six · Severity: SMELL · Category: tests · Status: NEW

What the code does. src/recovery_tests.rs:1-7 states the file "Lives in src/ rather than tests/ because the I35 pub→pub(crate) reshape locks these internals down; integration tests no longer have access." But test_named_roots_survive_commit_and_reopen (:868), test_named_roots_revert_on_rollback (:893), test_named_roots_revert_on_rollback_to_savepoint (:918), test_named_roots_validation_errors (:945), test_named_roots_table_full (:981) and test_named_roots_clear_is_idempotent (:1023) touch nothing but Chisel, ChiselError and crate::Handle — all public. Every one has a near-identical twin in tests/error_and_format.rs:141-292 (test_named_root_empty_name_rejected, _too_long_rejected, _max_length_accepted, _embedded_nul_rejected, _table_full, _clear_missing_name_is_noop, _rollback_reverts_set, test_named_root_survives_commit_and_reopen).

Why it is a problem. Two suites assert the same contract with different names and different hardcoded bounds — recovery_tests.rs:993 hardcodes the literal 8 (with a comment warning it must move if NAMED_ROOT_COUNT changes) while tests/error_and_format.rs:214 uses the exported NAMED_ROOT_COUNT. A change to NAMED_ROOT_COUNT silently stops testing the boundary in one file while the other keeps working, so the duplicate provides false reassurance rather than redundancy. It also stretches recovery_tests.rs to 1652 lines of "crash recovery" that is substantially not about crash recovery.

Direction of a fix. Delete the six named-root tests from src/recovery_tests.rs; the public-API versions in tests/error_and_format.rs already cover them and use the exported constant. Keep recovery_tests.rs to the tests that genuinely need Superblock, PageType, and page::stamp_checksum.

TESTS-CI-10 — Test comments point readers at src/transaction.rs and tests/crash_recovery.rs, neither of which exists

Location: tests/client_byte.rs:9, tests/api_edge_cases.rs:410, tests/transactions.rs:140, tests/tag_ops.rs:12, tests/freemap_multipage.rs:210, tests/error_and_format.rs:2 · Severity: SMELL · Category: comment-accuracy · Status: NEW

What the code does. Six test files cite paths that were removed by the module split. tests/client_byte.rs:8-10: "Poison-path coverage lives in the in-crate unit test poisoned_manager_rejects_every_public_entry_point (src/transaction.rs)" — the test is real but lives at src/transaction/tests.rs:210; src/transaction.rs does not exist (the module is the directory src/transaction/). tests/api_edge_cases.rs:410: "the existing crash_recovery.rs suite covers poison via real fatal-I/O injection" — there is no tests/crash_recovery.rs; that suite is src/recovery_tests.rs. Likewise tests/transactions.rs:140 ("migrated ... to src/transaction.rs as reopen_preserves_committed_data" — actually src/transaction/tests.rs:2290), tests/tag_ops.rs:12 ("persist_freemap_* in src/transaction.rs"), tests/freemap_multipage.rs:210 ("see the unit-test coverage in transaction.rs"), tests/error_and_format.rs:2 ("the named-root validation path in transaction.rs").

Why it is a problem. Every one of these comments exists to route a maintainer to the complementary coverage before they widen or delete a test. Following them lands on a missing file, so the reader concludes the referenced coverage was deleted and either re-adds a duplicate or, worse, removes the integration test believing the in-crate one is gone. This is the same class of rot that produced the duplicated named-root suite in TESTS-CI-9.

Direction of a fix. Sweep the six references: src/transaction.rs → src/transaction/tests.rs (with the function name, which is still accurate in all cases), crash_recovery.rs → src/recovery_tests.rs.

TESTS-CI-11 — test_defrag_respects_max_values asserts values_moved <= 2, so a defrag that moves nothing passes

Location: tests/defrag.rs:218, tests/defrag.rs:220 · Severity: SMELL · Category: tests

What the code does. After deleting 48 of 50 values, the test runs defrag(DefragOptions::default().sparse_threshold(0.25).max_values(2)) and asserts only result.values_moved <= 2 with the message "max_values=2 should cap values_moved to 2, got {}". The two surviving handles are then read back, which passes whether or not defrag relocated anything.

Why it is a problem. The message states the expectation is 2, but the assertion accepts 0, 1 or 2. A regression in max_values handling that turns the cap into "stop before the first move" (an off-by-one on the budget check — remaining == 0 evaluated before rather than after the decrement) makes values_moved == 0 and this test still passes. The sibling test test_defrag_skips_dense_pages (tests/defrag.rs:124-129) asserts values_moved == 0 for the do-nothing case, so the suite cannot distinguish "cap respected" from "cap broke defrag entirely".

Direction of a fix. Assert values_moved == 2 — the setup is deterministic (50 values × 200 bytes, 48 deleted, threshold 0.25), exactly as the sibling test_defrag_reclaims_space_after_deletes already asserts exact counts at tests/defrag.rs:66-74.

TESTS-CI-1 — error_and_format.rs claims exhaustive ChiselError coverage but tests 19 of 30 variants; the "compile error" safety net it describes does not exist

Location: tests/error_and_format.rs:7-8 and :81-83 (comments only); the real guard is src/error.rs:524-637 · Severity: NIT · Category: tests · Status: NEW

What the code does. The file header asserts "is_fatal() classification of every ChiselError variant" and "Display output is non-empty for every variant", and the Display test's body says "We enumerate every variant explicitly so a newly added variant forces a compile error in the match below, reminding the author to add a Display arm." There is no match in that test — it is a let variants: Vec<ChiselError> = vec![...] literal, which a new variant never breaks. ChiselError has 30 variants; the Display list carries 19. Uncovered: CacheFull, SpillwayFull, TransactionInProgress, TagMismatch, UnsupportedPageSize, NoEncryptionKey, InvalidEncryptionKey, EncryptionNotSupported, NoFreeKeySlot, LastKeySlot, DecryptionFailed. test_is_fatal_storage_integrity_variants_are_fatal omits UnsupportedPageSize and DecryptionFailed, both of which is_fatal (src/error.rs:225-226) classifies as fatal; the non-fatal test omits CacheFull, SpillwayFull, TransactionInProgress, TagMismatch and every encryption variant.

Why it is a problem. Concretely: drop ChiselError::DecryptionFailed { .. } from the matches! in src/error.rs:214-227 and the whole suite still passes. A page whose AEAD tag fails under the correct session DEK — i.e. on-disk tampering — would then be classified operational, so TransactionManager would not poison and the caller would keep issuing reads against a store known to be corrupt. The same silent hole exists for UnsupportedPageSize. The header comment is worse than no comment: a maintainer adding a variant reads "forces a compile error" and does not add a case.

Direction of a fix. Make the enumeration real: build the variant list inside a function that takes &ChiselError and matches exhaustively (an #[allow(unreachable_patterns)]-free match on the crate-internal type, or a fn all_variants() -> Vec<ChiselError> fed by a match on a marker enum), and add the 11 missing variants plus the two missing is_fatal classifications. Failing that, delete the "forces a compile error" claim so nobody relies on it.

TESTS-CI-8 — No CI job measures test coverage, so the suite's blind spots are invisible

Location: .github/workflows/ci.yml:12, .github/workflows/ci.yml:129 · Severity: NIT · Category: ci · Status: NEW

What the code does. The complete job inventory across all three workflows is: ci.yml — test (cargo build + cargo test, debug, ubuntu), clippy (cargo clippy --workspace -- -D warnings), fmt (cargo fmt -- --check), audit (cargo audit), msrv (cargo build -p chisel on 1.82), python (maturin + pytest, 2 OS × 2 Python); bench.yml — bench (report-only PR comment); wheels.yml — cargo-test-gate (cargo test --release + cargo audit), wheels, sdist. None of them runs cargo llvm-cov, cargo tarpaulin, or any equivalent, and no coverage artifact or badge is produced.

Why it is a problem. The gaps this review found are exactly the kind a coverage run surfaces mechanically: 11 ChiselError variants with no Display test (TESTS-CI-1), the Fault::FailReadPage arm at src/page_io.rs:290 exercised only from one unit test, and every #[cfg(test)] fault path that integration tests cannot reach. Without a coverage number, "is this path tested?" is answered by grep and hope, and a refactor that deletes the last caller of a branch produces no signal.

Direction of a fix. Add a non-gating coverage job running cargo llvm-cov --workspace --lcov and uploading the report as an artifact (report-only, same posture as bench). Gate later, once a baseline exists.

TESTS-CI-12 — create_tests.rs module doc lists a cleartext-leak check that no test in the file performs

Location: src/transaction/create_tests.rs:7, src/transaction/create_tests.rs:140 · Severity: NIT · Category: comment-accuracy · Status: NEW

What the code does. The module header enumerates the file's scope, including "- sensitive fields (named_roots names) are NOT in cleartext" (create_tests.rs:7). The only test that could do so, create_encrypted_db_sealed_body_is_present, explicitly disclaims it at create_tests.rs:140-142: "Bytes 52..308 (plaintext named_roots) ARE intentionally zeroed by serialize_encrypted ... We do NOT check those here." It checks only that the 24-byte nonce region is non-zero.

Why it is a problem. A reader auditing encryption coverage reads the header, ticks off "cleartext leak — covered", and moves on. The property is in fact covered, but by encrypted_named_root_name_absent_from_cleartext in src/superblock/mod.rs:1081 — a file the header does not mention. If that superblock test were ever deleted, the header would still claim coverage that no longer exists anywhere.

Direction of a fix. Change the bullet to point at src/superblock/mod.rs:1081 ("cleartext-leak coverage lives in superblock::tests::encrypted_named_root_name_absent_from_cleartext") rather than claiming it as this file's scope.

---

_{Filed from the clean-slate deep review of 2026-07-29. Full context, verification notes, and the delta against ISSUES.md are in docs/reviews/review-20260729-183138.md. Baseline at review time: 681 tests passing, clippy and fmt clean — none of these are toolchain-visible.}