This issue groups 11 related findings.
DOCS-COMMENTS-7 — page.rs and python/README.md both say the MINOR-newer write-refusal gate is deferred/a no-op; it is implemented and forces read-only on open
Location: src/page.rs:104 · Severity: SMELL · Category: comment-accuracy
What the code does. src/page.rs:104-108 documents FORMAT_MAJOR_VERSION/FORMAT_MINOR_VERSION with: "Write safety across minors is a separate concern — a binary at minor M opening a file at minor M' > M can read but not safely write without clobbering fields it doesn't know about; this check is deferred until the first 1.1 release (at which point the gate grows a "newer minor ⇒ refuse writes" arm). See ISSUES.md I29." python/README.md:423 says the same: "starting with the first post-1.0 minor bump, a binary at MINOR = m opening a file at MINOR = m' > m will be restricted to read-only ... Until 1.1 ships this check is a no-op because no minor variants exist." The gate is live: src/transaction/recovery.rs:421-423 runs if page::format_minor(sb.format_version) > page::FORMAT_MINOR_VERSION { cache.io_mut().force_read_only(); }, and PageIo::force_read_only (src/page_io.rs:202) exists solely for it ("Used by the I29 format-MINOR write-gate"). The root README already says it is done — README.md:363: "The write-refusal arm (refuse writes when file MINOR > binary MINOR) is implemented (I29)".
Why it is a problem. Three documents disagree about whether a safety gate exists. A maintainer reading the constants block in page.rs — the natural place to look when bumping FORMAT_MINOR_VERSION from 1 to 2 — concludes the write-refusal arm still has to be built and may implement a second, conflicting one, or may bump MINOR believing older binaries will happily write the file. The Python doc likewise tells users a real behaviour (a mutation returning ReadOnlyModeError after opening a newer-minor file) "cannot happen yet".
Direction of a fix. Update src/page.rs:104-108 to state the gate is implemented in TransactionManager::open_existing and that a MINOR-newer file opens read-only rather than being rejected, and correct python/README.md:423 to drop "Until 1.1 ships this check is a no-op".
DOCS-COMMENTS-8 — DataPage::insert's comment claims the transaction layer allocates a fresh page per insert and that the function is "underutilized" — R1 slot packing has been implemented since
Location: src/data_page.rs:162 · Severity: SMELL · Category: comment-accuracy
What the code does. src/data_page.rs:162-165: "Note (v1 simplification per ARCHITECTURE.md): the transaction layer calls PageCache::new_page() for every insert rather than scanning existing pages for free slots. Intentional, not a bug — this function itself is correct; it's just underutilized." The transaction layer no longer does that. SlotPacker::insert (src/transaction/packing.rs:119-134) first tries the packing cursor — if let Some(cursor_page_id) = self.insert_cursor { let buf = cache.get_mut(cursor_page_id)?; let result = DataPage::insert(buf, value); ... } — and only allocates when the cursor is absent or full (line 141). ARCHITECTURE.md now documents the opposite of what the comment attributes to it (line 579-583, "Slot packing (R1) means a single data page can hold many small values").
Why it is a problem. The comment cites ARCHITECTURE as its authority for a behaviour ARCHITECTURE no longer describes, so a reader cross-checking the two gets a self-consistent-looking but false picture: they would expect one value per data page and would not understand why SlotPacker exists, why the cursor must be cleared under savepoints (src/transaction/packing.rs:157-162), or why release() decrements a count instead of freeing a page. It is the only in-code statement of the data-page allocation policy, and it describes the pre-R1 engine.
Direction of a fix. Replace the note with the current model: inserts pack into the transaction's insert cursor when packing is enabled, falling back to a fresh page when the cursor is full or a savepoint is active; DataPage::insert is the per-page primitive that model is built on.
DOCS-COMMENTS-9 — THEORY.md says the Cargo workspace was deferred and that bench/ is not a workspace member; Cargo.toml declares a workspace with bench in default-members
Location: THEORY.md:178 · Severity: SMELL · Category: docs-vs-reality
What the code does. THEORY.md:178 lists among rejected alternatives: "Adopting a real Cargo workspace — deferred (I61): members would share edition/rust-version/feature resolution, too restrictive for the PyO3 abi3 binding, and the bench floats its floor faster than the engine wants." THEORY.md:190 adds: "It lives in a bench/ subcrate that is a sibling to python/, not a plain workspace member drawn into the engine's own build in a way that would auto-run its 10–25 minutes of tests on every cargo test." Cargo.toml:19-31 declares exactly that workspace: [workspace] members = [".", "python", "bench"] with default-members = [".", "bench"], headed by the comment "I61 (ISSUES.md, 2026-05-22): Cargo workspace declaration. Members share one Cargo.lock (at workspace root) and one target/ directory". README.md:73 and ARCHITECTURE.md:133 both describe it as landed: "bench/ is a default-members workspace member (I58/I61), so a root cargo test runs its tests too."
Why it is a problem. THEORY is the document a maintainer reads to understand why the build is shaped as it is; it asserts a decision ("deferred") that was subsequently reversed under the same issue number, and further asserts a build property (bench tests not run by a root cargo test) that the repo's own contributor instructions contradict. Someone acting on THEORY.md:190 would not expect cargo test at the root to run the bench crate, and would not understand why a bench-crate compile error breaks the engine's test command.
Direction of a fix. Rewrite the MSRV entry's rejected-alternatives line to record that the workspace was adopted (I61) with python excluded from default-members for the PyO3 linker reason, and correct the "sibling, not a workspace member" sentence in the implementation-history section to match default-members = [".", "bench"].
DOCS-COMMENTS-10 — ARCHITECTURE documents the HKDF info string as "chisel-kek"; the code uses "chisel-kek-v1"
Location: ARCHITECTURE.md:673 · Severity: SMELL · Category: docs-vs-reality
What the code does. ARCHITECTURE.md:673, in the on-disk-encryption section's key-derivation spec: "Raw key (Key::Raw): KEK = HKDF-SHA256(ikm=key material, salt=slot salt, info=\"chisel-kek\")." The code's constant is const KEK_INFO: &[u8] = b"chisel-kek-v1"; (src/crypto/mod.rs:140), used at src/crypto/mod.rs:181 (hk.expand(KEK_INFO, okm.as_mut())) and pinned by the reference test at src/crypto/mod.rs:455 (hk.expand(b"chisel-kek-v1", &mut expect)).
Why it is a problem. The info string is a hard on-disk format parameter — src/crypto/mod.rs:138-140 says so explicitly ("Changing it is a format break (existing slots would stop unwrapping)"). ARCHITECTURE is the stated byte-level reference for the crypto layout, so anyone reimplementing the unwrap path (a recovery tool, a second-language binding, a security audit) from the documented value derives a different KEK and every existing key slot fails to unwrap — presenting as InvalidEncryptionKey on a correct passphrase, with no clue where the divergence came from.
Direction of a fix. Correct ARCHITECTURE.md:673 to info="chisel-kek-v1", and note that the -v1 suffix is the KDF-construction version so a future revision can coexist (the rationale already recorded at src/crypto/mod.rs:138-140).
DOCS-COMMENTS-11 — ARCHITECTURE states superblock ties break by lowest slot index (the code's max_by_key picks the highest), and presents the #[cfg(test)]-only Superblock::select as the production recovery path
Location: ARCHITECTURE.md:310 · Severity: SMELL · Category: docs-vs-reality
What the code does. ARCHITECTURE.md:310: "Superblock::select reads up to MAX_SUPERBLOCKS (= 16) candidate pages ... and max_by_keys on txn_counter. Ties break by lowest slot index". The function's own doc says the opposite — src/superblock/mod.rs:559-561: "Tie-break: max_by_key returns the LAST maximum in iteration order (highest page index on a tie)" — which is Iterator::max_by_key's documented behaviour. Separately, select is gated #[cfg(test)] (src/superblock/mod.rs:562) and its doc says "Used in tests only. Production code (open_existing) inlines the same filter_map + max_by_key"; the production selection is src/transaction/recovery.rs:319-325. README.md:378 and the recovery doc comment at src/transaction/recovery.rs:172-175 both name Superblock::select as the thing that runs on open.
Why it is a problem. Two separate wrongnesses in one paragraph. The tie-break direction is inverted, and ties are not hypothetical — ARCHITECTURE itself notes they occur "during the create_new seeding window", and create_new seeds slot i with counter N-1-i (src/transaction/recovery.rs:86), so a reader reasoning about which slot wins on a fresh encrypted database gets the wrong buffer. That is precisely the failure src/transaction/recovery.rs:311-318 warns about ("decrypt_body builds its AAD from the winner's txn_counter but reads the sealed body from the wrong buffer ... the correct key wrongly fails to open"). The select-as-production claim sends a reader auditing crash recovery to a test-only function that is not the code that runs.
Direction of a fix. Fix the tie-break sentence to "highest slot index (max_by_key returns the last maximum)", and reword ARCHITECTURE/README to say recovery inlines the same filter+max in TransactionManager::open_existing (keeping the winning buffer for decrypt_body), with Superblock::select noted as the test-only mirror.
DOCS-COMMENTS-12 — Stats is documented as three fields in its own module header, its #[non_exhaustive] rationale, ARCHITECTURE, and the README API table — it has five
Location: src/stats.rs:12 · Severity: SMELL · Category: comment-accuracy
What the code does. Stats (src/stats.rs:16-48) has five public fields: handle_count, total_pages, file_size_bytes, spillway_logical_bytes, spillway_max_bytes — the last two added by I74 and populated at src/lib.rs:860-861. Four doc sites still describe three. src/stats.rs:1-4: "A plain snapshot struct returned by Chisel::stats() for observability: handle count, page count, and raw file size ... so that lib.rs and the public API don't have to pull in transaction.rs just to expose these three numbers." src/stats.rs:12-15: "#[non_exhaustive] so adding a fifth summary field (e.g. live-handle/total-handle ratio for retirement pressure) is not a breaking change" — there are already five. ARCHITECTURE.md:130: "Stats (handle_count, total_pages, file_size_bytes)". README.md:280: "stats() | Handle count, page count, file size (takes &self)".
Why it is a problem. The spillway gauges are the only way to observe spillway pressure before SpillwayFull fires, and stats.rs's own field docs (lines 42-47) call that out as the intended operator workflow ("Operators predict SpillwayFull by watching spillway_logical_bytes / spillway_max_bytes climb across commits"). Every summary-level doc omits them, so an operator reading the README API table or ARCHITECTURE's module table never learns the capability exists. The "adding a fifth" rationale is also self-refuting and would confuse the next person deciding whether a sixth field is breaking.
Direction of a fix. Add the two spillway fields to the README API-table entry and ARCHITECTURE's module table, and update src/stats.rs's header and the #[non_exhaustive] note to say "five" (or drop the count and phrase it as "a future field").
DOCS-COMMENTS-13 — README's chunk-tags example passes bare u32 literals where the API takes Tag, and the API table says tag() returns 0 when untagged instead of None
Location: README.md:191 · Severity: SMELL · Category: docs-vs-reality
What the code does. The public tag API is typed: pub fn allocate_tagged(&mut self, value: &[u8], tag: Tag) (src/lib.rs:609), pub fn tag(&self, handle: Handle) -> Result<Option<Tag>> (src/lib.rs:618), handles_with_tag(&self, tag: Tag) (line 655), delete_with_tag(&mut self, tag: Tag, max: usize) (line 742). Tag wraps NonZeroU32 and has no From<u32> — only Tag::new(v) -> Option<Tag> and TryFrom<u32> (src/handle.rs:96-133). README.md:191-206 nevertheless writes db.allocate_tagged(b"row-a", 42)?, db.handles_with_tag(42)?, and db.delete_with_tag(42, 256)?, plus assert_eq!(db.tag(a)?, 42); at line 195 — the latter comparing an Option<Tag> against an integer. README.md:270's API table says: "tag(handle) | Read a handle's tag, 0 if untagged". src/handle.rs:5-7 states the design intent the README contradicts: "'no tag' is the absence of a Tag (Option<Tag>), never the in-band sentinel 0."
Why it is a problem. None of the four calls in the example compile — Tag cannot be built from an integer literal — so the section's entire code block is unusable as written, and it is the only documentation of how to construct a tag. The API-table entry teaches the exact sentinel model (0 = untagged) that handle.rs deliberately removed from the public surface, so a reader writes if db.tag(h)? == 0 and finds no such comparison exists.
Direction of a fix. Rewrite the example to construct tags (let t = Tag::new(42).expect("non-zero");) and to match on Option<Tag> for tag(); change the API-table row to "Read a handle's tag; None if untagged". Mention that Tag is non-zero and that untagged values use plain allocate.
DOCS-COMMENTS-14 — overflow.rs's module header and ARCHITECTURE both attribute chain freeing to a delete() that does not exist; the real function has no side effects
Location: src/overflow.rs:40 · Severity: SMELL · Category: comment-accuracy
What the code does. src/overflow.rs:40 states "A chain is reachable from exactly one handle; delete() frees all pages", and lines 46-48: "delete() returns the list of page ids to free; the caller (transaction.rs) folds those into txn_freed_pages". ARCHITECTURE.md:373 repeats it: "Cycle detection in read/delete bounds the walk by total_length / OVERFLOW_PAYLOAD (I14)". Overflow exposes only write, read, and collect_chain_pages (src/overflow.rs:82, 152, 247); there is no delete. collect_chain_pages's own doc is explicit that it is not the freeing step — src/overflow.rs:238-240: "The caller (transaction layer) is responsible for actually releasing the pages — this function deliberately has no side effect on page state so it's safe to call speculatively and discard the result."
Why it is a problem. Two adjacent comments in the same file disagree about whether the enumeration function frees anything, and the header's version is the one a reader hits first. Someone adding a new overflow-release path could reasonably call collect_chain_pages and assume the pages are now freed, silently leaking the whole chain (the ids never reach txn_freed_pages, so persist_freemap never marks them free). The stale delete name also defeats grep from ARCHITECTURE into the source.
Direction of a fix. Rename the references in src/overflow.rs's header and ARCHITECTURE.md:373 to collect_chain_pages, and state that it only enumerates — the transaction layer pushes the returned ids onto txn_freed_pages, and commit's persist_freemap is what actually frees them.
DOCS-COMMENTS-15 — freemap.rs's dead-code note is wrong in both directions: is_free has a production caller and allocate_first has none
Location: src/freemap.rs:59 · Severity: SMELL · Category: comment-accuracy
What the code does. src/freemap.rs:59-65: "I35 reshape note: capacity and is_free are reached only from src-tests today (capacity from src/freemap.rs's own tests; is_free from src/transaction.rs's I27/I28 regression tests). The production allocator path uses allocate_first + mark_free." Both halves are false today. FreeMap::is_free is on a production path — FreeMapTree::is_free (src/freemap_tree.rs:240-244) wraps it, and reclaim_freemap_orphans calls that at src/transaction/freemap.rs:465 (&& !tree.is_free(cache, id)?). FreeMap::allocate_first has no non-test caller at all: grep -rn "FreeMap::allocate_first" src/ outside freemap.rs returns nothing; the tree claims a bit with FreeMap::first_free_bit_from + FreeMap::clear_bit (src/freemap_tree.rs:508 and 437). ARCHITECTURE.md:122 carries the same error: "freemap.rs | Single-page bitmap primitive: allocate_first / mark_free on one [u8; PAGE_SIZE] buffer."
Why it is a problem. The whole impl FreeMap block sits under #[allow(dead_code)] (src/freemap.rs:66), so the compiler will never correct this note — the comment is the only signal about which primitives are live. A maintainer pruning dead code would trust it and delete first_free_bit_from/clear_bit (the two that actually run) while keeping allocate_first (the one that does not), breaking the freemap tree's claim path. ARCHITECTURE's module table points the same reader at the wrong primitive as "the" allocator.
Direction of a fix. Rewrite the note to name the current split — first_free_bit_from + clear_bit are the tree's find-and-claim pair, mark_free is the reclaim primitive, is_free backs FreeMapTree::is_free in the orphan sweep, and allocate_first/capacity are test-only leftovers (delete allocate_first or say plainly that it is retained unused). Update ARCHITECTURE.md:122 to match.
DOCS-COMMENTS-16 — python/README's open() signature omits the encryption_key keyword it documents elsewhere, and its pages_allocated definition misses freemap-reuse allocations
Location: python/README.md:281 · Severity: SMELL · Category: docs-vs-reality
What the code does. python/README.md:280-288 presents the full chisel.open(...) signature — path, cache_max_bytes, spillway_max_bytes, drain_insertion, create_if_missing, read_only, superblock_count — and stops there. The binding's actual signature (python/src/db.rs:167-178) ends with encryption_key = None, and the stub file confirms it (python/chisel/chisel.pyi:139: encryption_key: bytes | str | None = None). The same README's error table already references the keyword (line 352: "NoEncryptionKeyError | Opened an encrypted database without supplying encryption_key"), and the root README.md:396 says "Encryption is exposed through the open() encryption_key keyword". Separately, python/README.md:261 defines "pages_allocated — PageCache.new_page invocations", while the authoritative description counts both extension and reuse — src/stats.rs:68-72: "page allocations, counting BOTH file extensions (PageCache::new_page ...) AND freemap reuses (PageCache::claim_page ...). Reuse is the common case once the handle table / membership index allocate COW pages through the freemap-aware path, so a counter that ignored it would read ~0 for a steady-state mutating workload."
Why it is a problem. The signature block is the section a Python user reads to discover open-time options; omitting encryption_key means encryption looks unreachable from Python even though the error table implies it exists — the two halves of the same document disagree. The counter definition understates what pages_allocated measures in exactly the steady-state mutating workload stats.rs calls out, so a user benchmarking allocation behaviour from the Python side misreads the number.
Direction of a fix. Add encryption_key=None # bytes (raw 32-byte key) or str (passphrase) to the signature block with a one-line create-vs-reopen note, and restate pages_allocated as "page allocations: both file extensions (new_page) and freemap reuses (claim_page)", matching src/stats.rs.
DOCS-COMMENTS-17 — txn_freed_pages's field comment names a current_freemap field that no longer exists
Location: src/transaction/mod.rs:181 · Severity: NIT · Category: comment-accuracy
What the code does. src/transaction/mod.rs:180-182 documents the field as: "Pages whose contents are no longer reachable from the new roots. Merged into current_freemap at commit time so subsequent transactions can reuse the space (ISSUES.md I9 / I10 / I11 / R2)." There is no current_freemap field on TransactionManager (the struct's fields are listed at src/transaction/mod.rs:146-237); the committed freemap is {freemap_page, freemap_depth} inside Roots (lines 99-103), reconstructed on demand, and the merge is done by FreemapRecycle::persist (called from src/transaction/commit.rs:94-95), with commit.rs:185-187 spelling it out: "The committed freemap tree advances automatically: its {root, depth} ride in current_roots ... No separate in-memory freemap copy to advance."
Why it is a problem. A reader grepping for current_freemap to understand where freed pages land finds nothing, and the phrasing implies an in-memory freemap mirror that the design deliberately removed — the same misconception ARCHITECTURE warns against in its freemap section.
Direction of a fix. Reword to: merged into the COW freemap tree by FreemapRecycle::persist during commit, which updates current_roots.{freemap_page, freemap_depth}.
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.
This issue groups 11 related findings.
DOCS-COMMENTS-7 —
page.rsandpython/README.mdboth say the MINOR-newer write-refusal gate is deferred/a no-op; it is implemented and forces read-only on openLocation:
src/page.rs:104· Severity: SMELL · Category: comment-accuracyWhat the code does. src/page.rs:104-108 documents
FORMAT_MAJOR_VERSION/FORMAT_MINOR_VERSIONwith: "Write safety across minors is a separate concern — a binary at minor M opening a file at minor M' > M can read but not safely write without clobbering fields it doesn't know about; this check is deferred until the first 1.1 release (at which point the gate grows a "newer minor ⇒ refuse writes" arm). See ISSUES.md I29." python/README.md:423 says the same: "starting with the first post-1.0 minor bump, a binary at MINOR = m opening a file at MINOR = m' > m will be restricted to read-only ... Until 1.1 ships this check is a no-op because no minor variants exist." The gate is live: src/transaction/recovery.rs:421-423 runsif page::format_minor(sb.format_version) > page::FORMAT_MINOR_VERSION { cache.io_mut().force_read_only(); }, andPageIo::force_read_only(src/page_io.rs:202) exists solely for it ("Used by the I29 format-MINOR write-gate"). The root README already says it is done — README.md:363: "The write-refusal arm (refuse writes when file MINOR > binary MINOR) is implemented (I29)".Why it is a problem. Three documents disagree about whether a safety gate exists. A maintainer reading the constants block in page.rs — the natural place to look when bumping
FORMAT_MINOR_VERSIONfrom 1 to 2 — concludes the write-refusal arm still has to be built and may implement a second, conflicting one, or may bump MINOR believing older binaries will happily write the file. The Python doc likewise tells users a real behaviour (a mutation returningReadOnlyModeErrorafter opening a newer-minor file) "cannot happen yet".Direction of a fix. Update src/page.rs:104-108 to state the gate is implemented in
TransactionManager::open_existingand that a MINOR-newer file opens read-only rather than being rejected, and correct python/README.md:423 to drop "Until 1.1 ships this check is a no-op".DOCS-COMMENTS-8 —
DataPage::insert's comment claims the transaction layer allocates a fresh page per insert and that the function is "underutilized" — R1 slot packing has been implemented sinceLocation:
src/data_page.rs:162· Severity: SMELL · Category: comment-accuracyWhat the code does. src/data_page.rs:162-165: "Note (v1 simplification per ARCHITECTURE.md): the transaction layer calls PageCache::new_page() for every insert rather than scanning existing pages for free slots. Intentional, not a bug — this function itself is correct; it's just underutilized." The transaction layer no longer does that.
SlotPacker::insert(src/transaction/packing.rs:119-134) first tries the packing cursor —if let Some(cursor_page_id) = self.insert_cursor { let buf = cache.get_mut(cursor_page_id)?; let result = DataPage::insert(buf, value); ... }— and only allocates when the cursor is absent or full (line 141). ARCHITECTURE.md now documents the opposite of what the comment attributes to it (line 579-583, "Slot packing (R1) means a single data page can hold many small values").Why it is a problem. The comment cites ARCHITECTURE as its authority for a behaviour ARCHITECTURE no longer describes, so a reader cross-checking the two gets a self-consistent-looking but false picture: they would expect one value per data page and would not understand why
SlotPackerexists, why the cursor must be cleared under savepoints (src/transaction/packing.rs:157-162), or whyrelease()decrements a count instead of freeing a page. It is the only in-code statement of the data-page allocation policy, and it describes the pre-R1 engine.Direction of a fix. Replace the note with the current model: inserts pack into the transaction's insert cursor when packing is enabled, falling back to a fresh page when the cursor is full or a savepoint is active;
DataPage::insertis the per-page primitive that model is built on.DOCS-COMMENTS-9 — THEORY.md says the Cargo workspace was deferred and that
bench/is not a workspace member; Cargo.toml declares a workspace withbenchindefault-membersLocation:
THEORY.md:178· Severity: SMELL · Category: docs-vs-realityWhat the code does. THEORY.md:178 lists among rejected alternatives: "Adopting a real Cargo workspace — deferred (I61): members would share edition/rust-version/feature resolution, too restrictive for the PyO3 abi3 binding, and the bench floats its floor faster than the engine wants." THEORY.md:190 adds: "It lives in a
bench/subcrate that is a sibling topython/, not a plain workspace member drawn into the engine's own build in a way that would auto-run its 10–25 minutes of tests on everycargo test." Cargo.toml:19-31 declares exactly that workspace:[workspace] members = [".", "python", "bench"]withdefault-members = [".", "bench"], headed by the comment "I61 (ISSUES.md, 2026-05-22): Cargo workspace declaration. Members share one Cargo.lock (at workspace root) and one target/ directory". README.md:73 and ARCHITECTURE.md:133 both describe it as landed: "bench/is adefault-membersworkspace member (I58/I61), so a rootcargo testruns its tests too."Why it is a problem. THEORY is the document a maintainer reads to understand why the build is shaped as it is; it asserts a decision ("deferred") that was subsequently reversed under the same issue number, and further asserts a build property (bench tests not run by a root
cargo test) that the repo's own contributor instructions contradict. Someone acting on THEORY.md:190 would not expectcargo testat the root to run the bench crate, and would not understand why a bench-crate compile error breaks the engine's test command.Direction of a fix. Rewrite the MSRV entry's rejected-alternatives line to record that the workspace was adopted (I61) with
pythonexcluded fromdefault-membersfor the PyO3 linker reason, and correct the "sibling, not a workspace member" sentence in the implementation-history section to matchdefault-members = [".", "bench"].DOCS-COMMENTS-10 — ARCHITECTURE documents the HKDF info string as "chisel-kek"; the code uses "chisel-kek-v1"
Location:
ARCHITECTURE.md:673· Severity: SMELL · Category: docs-vs-realityWhat the code does. ARCHITECTURE.md:673, in the on-disk-encryption section's key-derivation spec: "Raw key (
Key::Raw): KEK = HKDF-SHA256(ikm=key material, salt=slot salt, info=\"chisel-kek\")." The code's constant isconst KEK_INFO: &[u8] = b"chisel-kek-v1";(src/crypto/mod.rs:140), used at src/crypto/mod.rs:181 (hk.expand(KEK_INFO, okm.as_mut())) and pinned by the reference test at src/crypto/mod.rs:455 (hk.expand(b"chisel-kek-v1", &mut expect)).Why it is a problem. The info string is a hard on-disk format parameter — src/crypto/mod.rs:138-140 says so explicitly ("Changing it is a format break (existing slots would stop unwrapping)"). ARCHITECTURE is the stated byte-level reference for the crypto layout, so anyone reimplementing the unwrap path (a recovery tool, a second-language binding, a security audit) from the documented value derives a different KEK and every existing key slot fails to unwrap — presenting as
InvalidEncryptionKeyon a correct passphrase, with no clue where the divergence came from.Direction of a fix. Correct ARCHITECTURE.md:673 to
info="chisel-kek-v1", and note that the-v1suffix is the KDF-construction version so a future revision can coexist (the rationale already recorded at src/crypto/mod.rs:138-140).DOCS-COMMENTS-11 — ARCHITECTURE states superblock ties break by lowest slot index (the code's
max_by_keypicks the highest), and presents the#[cfg(test)]-onlySuperblock::selectas the production recovery pathLocation:
ARCHITECTURE.md:310· Severity: SMELL · Category: docs-vs-realityWhat the code does. ARCHITECTURE.md:310: "
Superblock::selectreads up toMAX_SUPERBLOCKS(= 16) candidate pages ... andmax_by_keys ontxn_counter. Ties break by lowest slot index". The function's own doc says the opposite — src/superblock/mod.rs:559-561: "Tie-break:max_by_keyreturns the LAST maximum in iteration order (highest page index on a tie)" — which isIterator::max_by_key's documented behaviour. Separately,selectis gated#[cfg(test)](src/superblock/mod.rs:562) and its doc says "Used in tests only. Production code (open_existing) inlines the samefilter_map+max_by_key"; the production selection is src/transaction/recovery.rs:319-325. README.md:378 and the recovery doc comment at src/transaction/recovery.rs:172-175 both nameSuperblock::selectas the thing that runs on open.Why it is a problem. Two separate wrongnesses in one paragraph. The tie-break direction is inverted, and ties are not hypothetical — ARCHITECTURE itself notes they occur "during the
create_newseeding window", andcreate_newseeds slot i with counter N-1-i (src/transaction/recovery.rs:86), so a reader reasoning about which slot wins on a fresh encrypted database gets the wrong buffer. That is precisely the failure src/transaction/recovery.rs:311-318 warns about ("decrypt_body builds its AAD from the winner's txn_counter but reads the sealed body from the wrong buffer ... the correct key wrongly fails to open"). Theselect-as-production claim sends a reader auditing crash recovery to a test-only function that is not the code that runs.Direction of a fix. Fix the tie-break sentence to "highest slot index (
max_by_keyreturns the last maximum)", and reword ARCHITECTURE/README to say recovery inlines the same filter+max inTransactionManager::open_existing(keeping the winning buffer fordecrypt_body), withSuperblock::selectnoted as the test-only mirror.DOCS-COMMENTS-12 —
Statsis documented as three fields in its own module header, its#[non_exhaustive]rationale, ARCHITECTURE, and the README API table — it has fiveLocation:
src/stats.rs:12· Severity: SMELL · Category: comment-accuracyWhat the code does.
Stats(src/stats.rs:16-48) has five public fields:handle_count,total_pages,file_size_bytes,spillway_logical_bytes,spillway_max_bytes— the last two added by I74 and populated at src/lib.rs:860-861. Four doc sites still describe three. src/stats.rs:1-4: "A plain snapshot struct returned by Chisel::stats() for observability: handle count, page count, and raw file size ... so that lib.rs and the public API don't have to pull in transaction.rs just to expose these three numbers." src/stats.rs:12-15: "#[non_exhaustive]so adding a fifth summary field (e.g. live-handle/total-handle ratio for retirement pressure) is not a breaking change" — there are already five. ARCHITECTURE.md:130: "Stats(handle_count,total_pages,file_size_bytes)". README.md:280: "stats()| Handle count, page count, file size (takes&self)".Why it is a problem. The spillway gauges are the only way to observe spillway pressure before
SpillwayFullfires, and stats.rs's own field docs (lines 42-47) call that out as the intended operator workflow ("Operators predictSpillwayFullby watchingspillway_logical_bytes / spillway_max_bytesclimb across commits"). Every summary-level doc omits them, so an operator reading the README API table or ARCHITECTURE's module table never learns the capability exists. The "adding a fifth" rationale is also self-refuting and would confuse the next person deciding whether a sixth field is breaking.Direction of a fix. Add the two spillway fields to the README API-table entry and ARCHITECTURE's module table, and update src/stats.rs's header and the
#[non_exhaustive]note to say "five" (or drop the count and phrase it as "a future field").DOCS-COMMENTS-13 — README's chunk-tags example passes bare
u32literals where the API takesTag, and the API table saystag()returns0when untagged instead ofNoneLocation:
README.md:191· Severity: SMELL · Category: docs-vs-realityWhat the code does. The public tag API is typed:
pub fn allocate_tagged(&mut self, value: &[u8], tag: Tag)(src/lib.rs:609),pub fn tag(&self, handle: Handle) -> Result<Option<Tag>>(src/lib.rs:618),handles_with_tag(&self, tag: Tag)(line 655),delete_with_tag(&mut self, tag: Tag, max: usize)(line 742).TagwrapsNonZeroU32and has noFrom<u32>— onlyTag::new(v) -> Option<Tag>andTryFrom<u32>(src/handle.rs:96-133). README.md:191-206 nevertheless writesdb.allocate_tagged(b"row-a", 42)?,db.handles_with_tag(42)?, anddb.delete_with_tag(42, 256)?, plusassert_eq!(db.tag(a)?, 42);at line 195 — the latter comparing anOption<Tag>against an integer. README.md:270's API table says: "tag(handle)| Read a handle's tag,0if untagged". src/handle.rs:5-7 states the design intent the README contradicts: "'no tag' is the absence of aTag(Option<Tag>), never the in-band sentinel0."Why it is a problem. None of the four calls in the example compile —
Tagcannot be built from an integer literal — so the section's entire code block is unusable as written, and it is the only documentation of how to construct a tag. The API-table entry teaches the exact sentinel model (0= untagged) thathandle.rsdeliberately removed from the public surface, so a reader writesif db.tag(h)? == 0and finds no such comparison exists.Direction of a fix. Rewrite the example to construct tags (
let t = Tag::new(42).expect("non-zero");) and to match onOption<Tag>fortag(); change the API-table row to "Read a handle's tag;Noneif untagged". Mention thatTagis non-zero and that untagged values use plainallocate.DOCS-COMMENTS-14 —
overflow.rs's module header and ARCHITECTURE both attribute chain freeing to adelete()that does not exist; the real function has no side effectsLocation:
src/overflow.rs:40· Severity: SMELL · Category: comment-accuracyWhat the code does. src/overflow.rs:40 states "A chain is reachable from exactly one handle; delete() frees all pages", and lines 46-48: "delete() returns the list of page ids to free; the caller (transaction.rs) folds those into txn_freed_pages". ARCHITECTURE.md:373 repeats it: "Cycle detection in
read/deletebounds the walk bytotal_length / OVERFLOW_PAYLOAD(I14)".Overflowexposes onlywrite,read, andcollect_chain_pages(src/overflow.rs:82, 152, 247); there is nodelete.collect_chain_pages's own doc is explicit that it is not the freeing step — src/overflow.rs:238-240: "The caller (transaction layer) is responsible for actually releasing the pages — this function deliberately has no side effect on page state so it's safe to call speculatively and discard the result."Why it is a problem. Two adjacent comments in the same file disagree about whether the enumeration function frees anything, and the header's version is the one a reader hits first. Someone adding a new overflow-release path could reasonably call
collect_chain_pagesand assume the pages are now freed, silently leaking the whole chain (the ids never reachtxn_freed_pages, sopersist_freemapnever marks them free). The staledeletename also defeats grep from ARCHITECTURE into the source.Direction of a fix. Rename the references in src/overflow.rs's header and ARCHITECTURE.md:373 to
collect_chain_pages, and state that it only enumerates — the transaction layer pushes the returned ids ontotxn_freed_pages, and commit'spersist_freemapis what actually frees them.DOCS-COMMENTS-15 —
freemap.rs's dead-code note is wrong in both directions:is_freehas a production caller andallocate_firsthas noneLocation:
src/freemap.rs:59· Severity: SMELL · Category: comment-accuracyWhat the code does. src/freemap.rs:59-65: "I35 reshape note: capacity and is_free are reached only from src-tests today (capacity from src/freemap.rs's own tests; is_free from src/transaction.rs's I27/I28 regression tests). The production allocator path uses
allocate_first+mark_free." Both halves are false today.FreeMap::is_freeis on a production path —FreeMapTree::is_free(src/freemap_tree.rs:240-244) wraps it, andreclaim_freemap_orphanscalls that at src/transaction/freemap.rs:465 (&& !tree.is_free(cache, id)?).FreeMap::allocate_firsthas no non-test caller at all:grep -rn "FreeMap::allocate_first" src/outside freemap.rs returns nothing; the tree claims a bit withFreeMap::first_free_bit_from+FreeMap::clear_bit(src/freemap_tree.rs:508 and 437). ARCHITECTURE.md:122 carries the same error: "freemap.rs| Single-page bitmap primitive:allocate_first/mark_freeon one[u8; PAGE_SIZE]buffer."Why it is a problem. The whole
impl FreeMapblock sits under#[allow(dead_code)](src/freemap.rs:66), so the compiler will never correct this note — the comment is the only signal about which primitives are live. A maintainer pruning dead code would trust it and deletefirst_free_bit_from/clear_bit(the two that actually run) while keepingallocate_first(the one that does not), breaking the freemap tree's claim path. ARCHITECTURE's module table points the same reader at the wrong primitive as "the" allocator.Direction of a fix. Rewrite the note to name the current split —
first_free_bit_from+clear_bitare the tree's find-and-claim pair,mark_freeis the reclaim primitive,is_freebacksFreeMapTree::is_freein the orphan sweep, andallocate_first/capacityare test-only leftovers (deleteallocate_firstor say plainly that it is retained unused). Update ARCHITECTURE.md:122 to match.DOCS-COMMENTS-16 — python/README's
open()signature omits theencryption_keykeyword it documents elsewhere, and itspages_allocateddefinition misses freemap-reuse allocationsLocation:
python/README.md:281· Severity: SMELL · Category: docs-vs-realityWhat the code does. python/README.md:280-288 presents the full
chisel.open(...)signature — path, cache_max_bytes, spillway_max_bytes, drain_insertion, create_if_missing, read_only, superblock_count — and stops there. The binding's actual signature (python/src/db.rs:167-178) ends withencryption_key = None, and the stub file confirms it (python/chisel/chisel.pyi:139:encryption_key: bytes | str | None = None). The same README's error table already references the keyword (line 352: "NoEncryptionKeyError| Opened an encrypted database without supplyingencryption_key"), and the root README.md:396 says "Encryption is exposed through theopen()encryption_keykeyword". Separately, python/README.md:261 defines "pages_allocated—PageCache.new_pageinvocations", while the authoritative description counts both extension and reuse — src/stats.rs:68-72: "page allocations, counting BOTH file extensions (PageCache::new_page...) AND freemap reuses (PageCache::claim_page...). Reuse is the common case once the handle table / membership index allocate COW pages through the freemap-aware path, so a counter that ignored it would read ~0 for a steady-state mutating workload."Why it is a problem. The signature block is the section a Python user reads to discover open-time options; omitting
encryption_keymeans encryption looks unreachable from Python even though the error table implies it exists — the two halves of the same document disagree. The counter definition understates whatpages_allocatedmeasures in exactly the steady-state mutating workload stats.rs calls out, so a user benchmarking allocation behaviour from the Python side misreads the number.Direction of a fix. Add
encryption_key=None # bytes (raw 32-byte key) or str (passphrase)to the signature block with a one-line create-vs-reopen note, and restatepages_allocatedas "page allocations: both file extensions (new_page) and freemap reuses (claim_page)", matching src/stats.rs.DOCS-COMMENTS-17 —
txn_freed_pages's field comment names acurrent_freemapfield that no longer existsLocation:
src/transaction/mod.rs:181· Severity: NIT · Category: comment-accuracyWhat the code does. src/transaction/mod.rs:180-182 documents the field as: "Pages whose contents are no longer reachable from the new roots. Merged into
current_freemapat commit time so subsequent transactions can reuse the space (ISSUES.md I9 / I10 / I11 / R2)." There is nocurrent_freemapfield onTransactionManager(the struct's fields are listed at src/transaction/mod.rs:146-237); the committed freemap is{freemap_page, freemap_depth}insideRoots(lines 99-103), reconstructed on demand, and the merge is done byFreemapRecycle::persist(called from src/transaction/commit.rs:94-95), with commit.rs:185-187 spelling it out: "The committed freemap tree advances automatically: its {root, depth} ride in current_roots ... No separate in-memory freemap copy to advance."Why it is a problem. A reader grepping for
current_freemapto understand where freed pages land finds nothing, and the phrasing implies an in-memory freemap mirror that the design deliberately removed — the same misconception ARCHITECTURE warns against in its freemap section.Direction of a fix. Reword to: merged into the COW freemap tree by
FreemapRecycle::persistduring commit, which updatescurrent_roots.{freemap_page, freemap_depth}.Filed from the clean-slate deep review of 2026-07-29. Full context, verification notes, and the delta against
ISSUES.mdare indocs/reviews/review-20260729-183138.md. Baseline at review time: 681 tests passing, clippy and fmt clean — none of these are toolchain-visible.