This issue groups 6 related findings.
PYTHON-2 — savepoint.rs claims the engine pops the savepoint itself on rollback_to; the engine explicitly keeps it
Location: python/src/savepoint.rs:99 · Severity: DESIGN · Category: comment-accuracy
What the code does. savepoint.rs:99-104 states: "The engine pops the savepoint stack down to AND including this savepoint, so the mark itself is gone after one successful call — a second call would fail at the engine layer with SavepointNotFound regardless." The engine does the opposite: rollback_to_inner ends with self.savepoints.truncate(idx + 1); (src/transaction/savepoints.rs:103), which RETAINS index idx, and the engine's own doc comment says so — "The named savepoint itself remains on the stack and can be rolled back to again or released" (src/transaction/savepoints.rs:47-49). Verified: after sp.rollback_to(), tx.savepoint('s') raises DuplicateSavepointError: duplicate savepoint: s, proving the mark is still there. The same wrong model is repeated in tests/test_exception_contract.py:26 and :34 ("sp1.rollback_to() # pops both sp1 and sp2 from the engine stack" — it pops sp2 only).
Why it is a problem. The comment is the stated justification for the Python-side guard ('The guard here turns that into a cleaner, more specific AlreadyFinishedError'). A maintainer who trusts it would conclude the guard is a cosmetic re-labelling of an engine error and could delete it — which would then expose repeated rollback_to as a working operation, silently changing the documented Python contract. It also misdescribes why test_exception_contract's SavepointNotFound tests pass.
Direction of a fix. Correct the comment to match src/transaction/savepoints.rs:47-49 and :103 (the named savepoint survives; only savepoints layered on top are popped), and fix the two test comments. Then re-justify the guard on its own terms (or drop it — see PYTHON-3).
PYTHON-4 — python/README.md Tags section documents the pre-I126 tag API: tag 0 and an int-valued tag() that no longer exist
Location: python/README.md:131 · Severity: DESIGN · Category: docs-vs-reality
What the code does. README.md:131-134: "Tag 0 is the "untagged" sentinel: it is never indexed, so handles_with_tag(0) is always empty -- use plain allocate() for untagged values." README.md:144: "assert tx.tag(h) == 42 # 0 if untagged". The binding rejects tag 0 outright — require_tag (python/src/db.rs:84-87) returns PyValueError::new_err("tag must be non-zero") for every tagged method — and tag() returns Option<u32> (db.rs:415-420), i.e. Python None. Verified: db.handles_with_tag(0) raises ValueError: tag must be non-zero; db.tag(h) on an untagged handle returns None. python/chisel/init.py:3-7 documents the correct rules, so the two docs contradict each other.
Why it is a problem. A user writing the documented defensive pattern if db.handles_with_tag(0): ... gets an unhandled ValueError, and if tx.tag(h) == 0: silently never matches (None != 0) so untagged handles are misclassified as tagged. The README is the wrong side here — the code and init.py agree.
Direction of a fix. Rewrite README.md:131-134 and the inline comment at :144 to match init.py's docstring: tags are >= 1, tag 0 raises ValueError on every tagged method, tag() returns None for untagged handles.
PYTHON-5 — README defrag example uses a max_pages kwarg that does not exist and denies that Transaction.defrag exists
Location: python/README.md:242 · Severity: DESIGN · Category: docs-vs-reality
What the code does. README.md:242: result = db.defrag(chisel.DefragOptions(sparse_threshold=0.25, max_pages=0)). The dataclass field is max_values (python/chisel/init.py:119-120), and the binding reads exactly sparse_threshold and max_values (python/src/db.rs:518-519). Verified: chisel.DefragOptions(sparse_threshold=0.25, max_pages=0) raises TypeError: DefragOptions.__init__() got an unexpected keyword argument 'max_pages'. Did you mean 'max_values'?. README.md:246 compounds it by pointing at "DefragOptions.max_pages's docstring", which does not exist. README.md:240 also asserts "defrag lives on the Chisel object, not the Transaction object", but PyTransaction::defrag exists (python/src/transaction.rs:211-214) and is tested (tests/test_stats_defrag.py::test_transaction_defrag_mid_tx).
Why it is a problem. The only defrag example in the binding's documentation cannot be run: copy-pasting it raises TypeError before reaching the engine. The max_pages name also does not appear anywhere in the codebase, so a reader cannot map it to the real knob without reading the Rust source.
Direction of a fix. Change the example and the surrounding prose to max_values, drop the 'legacy carry-over / see max_pages's docstring' sentence, and delete the incorrect 'defrag lives on the Chisel object, not the Transaction object' claim (or restate it as 'available on both').
PYTHON-6 — The type stub is named chisel/chisel.pyi, so PEP 561 resolves it to a nonexistent module chisel.chisel and no checker ever reads it
Location: python/chisel/chisel.pyi:7 · Severity: DESIGN · Category: docs-vs-reality
What the code does. The stub file is python/chisel/chisel.pyi and its header asserts "Why this file lives alongside init.py rather than at the package root: the stubs describe the chisel package namespace as users see it. The py.typed marker next to this file signals PEP 561 inline-typed package so type checkers pick these up." PEP 561 resolves stubs by module path: inside package chisel, the file chisel.pyi is the stub for module chisel.chisel, which does not exist; the stub for the package itself must be chisel/__init__.pyi. py.typed marks the package inline-typed, which sends checkers to chisel/__init__.py — whose entire public surface comes from from chisel._chisel import (...), a compiled module with no stub of its own. The file is nonetheless shipped into the wheel (pyproject.toml:31). No CI job runs mypy or pyright (.github/workflows/ci.yml has cargo test/clippy/fmt and pytest only), so the misnaming is unverifiable in-tree.
Why it is a problem. Every type declaration in the 244-line stub — the whole Chisel/Transaction/Savepoint API, the exception hierarchy, IoError.errno/.kind — is inert. A user type-checking chisel.open('db').allocate(b'x') gets 'Cannot find implementation or library stub for module named chisel._chisel' and no signatures, despite the package advertising py.typed. Stub/impl drift also accumulates undetected (e.g. __exit__ is declared -> None in the stub while the implementation returns bool).
Direction of a fix. Rename to python/chisel/__init__.pyi (updating the pyproject include and the header comment), or move the declarations into a chisel/_chisel.pyi stub for the extension module. Add a mypy/pyright job to the python CI so the stub is actually exercised.
PYTHON-8 — README 'Thread safety' contradicts the shipped concurrency test and omits that the GIL is held across every blocking engine call
Location: python/README.md:403 · Severity: DESIGN · Category: docs-vs-reality
What the code does. README.md:403: "A Chisel instance is not safe for concurrent use from multiple threads ... two threads must never call into the same Chisel at the same time." The suite asserts the opposite for reads: test_two_thread_mutex_contention (tests/test_exception_contract.py:229-283) runs two threads doing 200×8 concurrent db.read() calls on one handle and asserts no error, no corruption, no poison. That is sound because per-op methods never release the GIL — with_inner_io/with_inner_mut_io take no Python token and call the engine with the GIL held (db.rs:636-672) — plus the Mutex<Option<Chisel>>. The genuine per-op consequence is documented only in a Rust comment the user never sees: "long-running engine calls (e.g. a large commit's fsync, a big defrag) will block ALL other Python threads in this process" (db.rs:643-645). The README never mentions the GIL.
Why it is a problem. The two statements cannot both guide a user: the README forbids what the test suite certifies, so a reader cannot tell whether a concurrent read is undefined behaviour or supported. Meanwhile the property that actually bites — every commit's three fsyncs stall every other thread in the process, since only open() releases the GIL (db.rs:276) — is undocumented for users, who will reasonably assume a Rust extension releases the GIL around blocking I/O.
Direction of a fix. Rewrite the Thread safety section to state what holds: calls are serialized (GIL + Mutex) so concurrent calls cannot corrupt memory, but the single-writer transaction state is shared, so two threads must not interleave transactions; and the GIL is held for the duration of every engine call, so long commits/defrags block all Python threads.
PYTHON-9 — python/README.md documents the encryption and key-rotation ERRORS but never documents the feature, the encryption_key kwarg, or add_key/rotate_key/remove_key
Location: python/README.md:279 · Severity: DESIGN · Category: docs-vs-reality
What the code does. The open() signature block (README.md:277-288) lists path, cache_max_bytes, spillway_max_bytes, drain_insertion, create_if_missing, read_only, superblock_count — and stops. encryption_key is a real parameter (python/src/db.rs:176, :194, :222-240) and add_key/rotate_key/remove_key are real methods (db.rs:555-578), all covered by tests (tests/test_encryption.py, tests/test_encryption_keys.py). Their names appear in the README exactly once each, inside the error tables (README.md:352-356: NoEncryptionKeyError / InvalidEncryptionKeyError / EncryptionNotSupportedError / NoFreeKeySlotError / LastKeySlotError). There is no ## Encryption section (section list: Status, Install, Quick start, Transactions, Savepoints, Values, Handles, Tags, Client byte, Named roots, Stats and defrag, Engine counters, Opening a database, Errors, Recovery, Thread safety, In-memory mode, On-disk format compatibility, Design).
Why it is a problem. The binding's only user-facing documentation makes an entire shipped subsystem undiscoverable: a reader learns that NoFreeKeySlotError fires when 'all 8 key-slot table entries are in use' without ever being told the API that fills them, or that passing a str means passphrase and bytes means a raw key (db.rs:63-77). Combined with the inert type stub (PYTHON-6), there is no in-tree source a user can read to discover these methods short of the Rust source.
Direction of a fix. Add an Encryption section covering the encryption_key kwarg (bytes = raw key, str = passphrase), add it to the open() signature block, and document add_key/rotate_key/remove_key with the 8-slot limit and the between-transactions restriction.
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 6 related findings.
PYTHON-2 — savepoint.rs claims the engine pops the savepoint itself on rollback_to; the engine explicitly keeps it
Location:
python/src/savepoint.rs:99· Severity: DESIGN · Category: comment-accuracyWhat the code does. savepoint.rs:99-104 states: "The engine pops the savepoint stack down to AND including this savepoint, so the mark itself is gone after one successful call — a second call would fail at the engine layer with SavepointNotFound regardless." The engine does the opposite:
rollback_to_innerends withself.savepoints.truncate(idx + 1);(src/transaction/savepoints.rs:103), which RETAINS indexidx, and the engine's own doc comment says so — "The named savepoint itself remains on the stack and can be rolled back to again or released" (src/transaction/savepoints.rs:47-49). Verified: aftersp.rollback_to(),tx.savepoint('s')raisesDuplicateSavepointError: duplicate savepoint: s, proving the mark is still there. The same wrong model is repeated in tests/test_exception_contract.py:26 and :34 ("sp1.rollback_to() # pops both sp1 and sp2 from the engine stack" — it pops sp2 only).Why it is a problem. The comment is the stated justification for the Python-side guard ('The guard here turns that into a cleaner, more specific AlreadyFinishedError'). A maintainer who trusts it would conclude the guard is a cosmetic re-labelling of an engine error and could delete it — which would then expose repeated rollback_to as a working operation, silently changing the documented Python contract. It also misdescribes why test_exception_contract's SavepointNotFound tests pass.
Direction of a fix. Correct the comment to match src/transaction/savepoints.rs:47-49 and :103 (the named savepoint survives; only savepoints layered on top are popped), and fix the two test comments. Then re-justify the guard on its own terms (or drop it — see PYTHON-3).
PYTHON-4 — python/README.md Tags section documents the pre-I126 tag API: tag 0 and an int-valued tag() that no longer exist
Location:
python/README.md:131· Severity: DESIGN · Category: docs-vs-realityWhat the code does. README.md:131-134: "Tag
0is the "untagged" sentinel: it is never indexed, sohandles_with_tag(0)is always empty -- use plainallocate()for untagged values." README.md:144: "assert tx.tag(h) == 42 # 0 if untagged". The binding rejects tag 0 outright —require_tag(python/src/db.rs:84-87) returnsPyValueError::new_err("tag must be non-zero")for every tagged method — andtag()returnsOption<u32>(db.rs:415-420), i.e. PythonNone. Verified:db.handles_with_tag(0)raisesValueError: tag must be non-zero;db.tag(h)on an untagged handle returnsNone. python/chisel/init.py:3-7 documents the correct rules, so the two docs contradict each other.Why it is a problem. A user writing the documented defensive pattern
if db.handles_with_tag(0): ...gets an unhandled ValueError, andif tx.tag(h) == 0:silently never matches (None != 0) so untagged handles are misclassified as tagged. The README is the wrong side here — the code and init.py agree.Direction of a fix. Rewrite README.md:131-134 and the inline comment at :144 to match init.py's docstring: tags are >= 1, tag 0 raises ValueError on every tagged method,
tag()returns None for untagged handles.PYTHON-5 — README defrag example uses a
max_pageskwarg that does not exist and denies that Transaction.defrag existsLocation:
python/README.md:242· Severity: DESIGN · Category: docs-vs-realityWhat the code does. README.md:242:
result = db.defrag(chisel.DefragOptions(sparse_threshold=0.25, max_pages=0)). The dataclass field ismax_values(python/chisel/init.py:119-120), and the binding reads exactlysparse_thresholdandmax_values(python/src/db.rs:518-519). Verified:chisel.DefragOptions(sparse_threshold=0.25, max_pages=0)raisesTypeError: DefragOptions.__init__() got an unexpected keyword argument 'max_pages'. Did you mean 'max_values'?. README.md:246 compounds it by pointing at "DefragOptions.max_pages's docstring", which does not exist. README.md:240 also asserts "defrag lives on the Chisel object, not the Transaction object", butPyTransaction::defragexists (python/src/transaction.rs:211-214) and is tested (tests/test_stats_defrag.py::test_transaction_defrag_mid_tx).Why it is a problem. The only defrag example in the binding's documentation cannot be run: copy-pasting it raises TypeError before reaching the engine. The
max_pagesname also does not appear anywhere in the codebase, so a reader cannot map it to the real knob without reading the Rust source.Direction of a fix. Change the example and the surrounding prose to
max_values, drop the 'legacy carry-over / see max_pages's docstring' sentence, and delete the incorrect 'defrag lives on the Chisel object, not the Transaction object' claim (or restate it as 'available on both').PYTHON-6 — The type stub is named chisel/chisel.pyi, so PEP 561 resolves it to a nonexistent module
chisel.chiseland no checker ever reads itLocation:
python/chisel/chisel.pyi:7· Severity: DESIGN · Category: docs-vs-realityWhat the code does. The stub file is
python/chisel/chisel.pyiand its header asserts "Why this file lives alongside init.py rather than at the package root: the stubs describe thechiselpackage namespace as users see it. Thepy.typedmarker next to this file signals PEP 561 inline-typed package so type checkers pick these up." PEP 561 resolves stubs by module path: inside packagechisel, the filechisel.pyiis the stub for modulechisel.chisel, which does not exist; the stub for the package itself must bechisel/__init__.pyi.py.typedmarks the package inline-typed, which sends checkers tochisel/__init__.py— whose entire public surface comes fromfrom chisel._chisel import (...), a compiled module with no stub of its own. The file is nonetheless shipped into the wheel (pyproject.toml:31). No CI job runs mypy or pyright (.github/workflows/ci.yml has cargo test/clippy/fmt and pytest only), so the misnaming is unverifiable in-tree.Why it is a problem. Every type declaration in the 244-line stub — the whole
Chisel/Transaction/SavepointAPI, the exception hierarchy,IoError.errno/.kind— is inert. A user type-checkingchisel.open('db').allocate(b'x')gets 'Cannot find implementation or library stub for module named chisel._chisel' and no signatures, despite the package advertising py.typed. Stub/impl drift also accumulates undetected (e.g.__exit__is declared-> Nonein the stub while the implementation returnsbool).Direction of a fix. Rename to
python/chisel/__init__.pyi(updating the pyproject include and the header comment), or move the declarations into achisel/_chisel.pyistub for the extension module. Add a mypy/pyright job to the python CI so the stub is actually exercised.PYTHON-8 — README 'Thread safety' contradicts the shipped concurrency test and omits that the GIL is held across every blocking engine call
Location:
python/README.md:403· Severity: DESIGN · Category: docs-vs-realityWhat the code does. README.md:403: "A
Chiselinstance is not safe for concurrent use from multiple threads ... two threads must never call into the sameChiselat the same time." The suite asserts the opposite for reads:test_two_thread_mutex_contention(tests/test_exception_contract.py:229-283) runs two threads doing 200×8 concurrentdb.read()calls on one handle and asserts no error, no corruption, no poison. That is sound because per-op methods never release the GIL —with_inner_io/with_inner_mut_iotake noPythontoken and call the engine with the GIL held (db.rs:636-672) — plus theMutex<Option<Chisel>>. The genuine per-op consequence is documented only in a Rust comment the user never sees: "long-running engine calls (e.g. a large commit's fsync, a big defrag) will block ALL other Python threads in this process" (db.rs:643-645). The README never mentions the GIL.Why it is a problem. The two statements cannot both guide a user: the README forbids what the test suite certifies, so a reader cannot tell whether a concurrent read is undefined behaviour or supported. Meanwhile the property that actually bites — every commit's three fsyncs stall every other thread in the process, since only
open()releases the GIL (db.rs:276) — is undocumented for users, who will reasonably assume a Rust extension releases the GIL around blocking I/O.Direction of a fix. Rewrite the Thread safety section to state what holds: calls are serialized (GIL + Mutex) so concurrent calls cannot corrupt memory, but the single-writer transaction state is shared, so two threads must not interleave transactions; and the GIL is held for the duration of every engine call, so long commits/defrags block all Python threads.
PYTHON-9 — python/README.md documents the encryption and key-rotation ERRORS but never documents the feature, the
encryption_keykwarg, or add_key/rotate_key/remove_keyLocation:
python/README.md:279· Severity: DESIGN · Category: docs-vs-realityWhat the code does. The
open()signature block (README.md:277-288) lists path, cache_max_bytes, spillway_max_bytes, drain_insertion, create_if_missing, read_only, superblock_count — and stops.encryption_keyis a real parameter (python/src/db.rs:176, :194, :222-240) andadd_key/rotate_key/remove_keyare real methods (db.rs:555-578), all covered by tests (tests/test_encryption.py, tests/test_encryption_keys.py). Their names appear in the README exactly once each, inside the error tables (README.md:352-356: NoEncryptionKeyError / InvalidEncryptionKeyError / EncryptionNotSupportedError / NoFreeKeySlotError / LastKeySlotError). There is no## Encryptionsection (section list: Status, Install, Quick start, Transactions, Savepoints, Values, Handles, Tags, Client byte, Named roots, Stats and defrag, Engine counters, Opening a database, Errors, Recovery, Thread safety, In-memory mode, On-disk format compatibility, Design).Why it is a problem. The binding's only user-facing documentation makes an entire shipped subsystem undiscoverable: a reader learns that
NoFreeKeySlotErrorfires when 'all 8 key-slot table entries are in use' without ever being told the API that fills them, or that passing astrmeans passphrase andbytesmeans a raw key (db.rs:63-77). Combined with the inert type stub (PYTHON-6), there is no in-tree source a user can read to discover these methods short of the Rust source.Direction of a fix. Add an Encryption section covering the
encryption_keykwarg (bytes = raw key, str = passphrase), add it to theopen()signature block, and document add_key/rotate_key/remove_key with the 8-slot limit and the between-transactions restriction.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.