docs(python): correct the binding docs and make the type stub live (#97) - #132
Merged
Xof merged 2 commits intoJul 31, 2026
Merged
Conversation
Six findings from the 2026-07-29 review, all verified against the
running binding before and after.
savepoint.rs said the engine "pops the savepoint stack down to AND
including this savepoint, so the mark itself is gone after one
successful call". The engine does the opposite: `rollback_to_inner`
ends with `savepoints.truncate(idx + 1)`, retaining the named savepoint
so it can be rolled back to again — its own doc says so. That mattered
because the wrong claim was the stated justification for the Python
guard ("a cleaner, more specific AlreadyFinishedError"), inviting a
maintainer to delete a guard that is in fact the only thing enforcing
the single-use contract. The comment now says what the engine does and
re-justifies the guard on its own terms. Two test comments repeating
the same wrong model are corrected.
README Tags documented the pre-I126 API: tag 0 as an "untagged"
sentinel and `tag()` returning an int. Tag 0 raises ValueError on every
tagged method and `tag()` returns None. A reader following the old text
would write `if db.handles_with_tag(0)` (ValueError) or
`if tx.tag(h) == 0` (never matches, so untagged handles are silently
misclassified).
The README's only defrag example could not be run: `max_pages` is not a
field — `DefragOptions(max_pages=0)` raises TypeError before reaching
the engine, and the name appears nowhere in the codebase. Corrected to
`max_values`, dropped the pointer to a nonexistent docstring, and
removed the claim that defrag "lives on the Chisel object, not the
Transaction object" — `PyTransaction::defrag` exists and is tested.
The type stub was inert. Named `chisel/chisel.pyi`, PEP 561 resolves it
to module `chisel.chisel`, which does not exist, so no checker ever read
its 244 lines and drift accumulated undetected — `__exit__` was declared
`-> None` in all three classes while every implementation returns
`bool`. Renamed to `chisel/__init__.pyi` (pyproject include and header
updated), fixed the `__exit__` return types, and added a CI step that
type-checks it, which is what keeps it from going inert again. Verified:
mypy now resolves `import chisel` through the stub and reports
`--strict` clean at 3.13; the step is pinned to 3.13 because the stub
uses `collections.abc.Buffer` (PEP 688, 3.12+) while the package itself
still runs on 3.11.
README "Thread safety" forbade what the suite certifies —
`test_two_thread_mutex_contention` runs 1600 concurrent reads across
two threads and asserts no error — while omitting the property that
actually bites. Rewritten to state what holds: calls serialize (GIL +
Mutex) so concurrency cannot corrupt, transaction state is what must
not be shared, and since only `open()` releases the GIL, a long commit
or defrag blocks every Python thread in the process.
Encryption was undocumented: the README named five encryption errors in
its tables without ever mentioning the `encryption_key` kwarg or
add_key/rotate_key/remove_key. Adds an Encryption section covering the
bytes-vs-str key vocabulary, the three open-time mismatch errors, the
8-slot table, and the between-transaction restriction.
Every claim in the new section was executed against a built wheel,
which caught one of my own: a raw key is HKDF input keying material of
any non-empty length, not a fixed 32 bytes.
Closes #97.
The preceding commit staged only the chisel.pyi -> __init__.pyi rename: the `git add` that should have carried the rest listed the old stub path, which no longer existed after `git mv`, so the whole invocation failed and added nothing. This commit is the content that belongs with it — the README corrections (tags, defrag, thread safety, the new Encryption section), the savepoint comment fix and its two test comments, the stub's `__exit__` return types and header, and the CI type-check step. No change in intent from the message on the previous commit; see it for the reasoning on each finding.
This was referenced Jul 29, 2026
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
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.
Closes #97 (6 findings, DESIGN/docs).
Every claim below was verified against a built wheel (
maturin develop --release,Python 3.13) before and after the change — including one of my own that turned
out to be wrong (see the last section).
PYTHON-2 — the savepoint comment described the opposite of the engine
savepoint.rssaid the engine "pops the savepoint stack down to AND includingthis savepoint, so the mark itself is gone after one successful call". It does
the opposite:
rollback_to_innerends withsavepoints.truncate(idx + 1),which retains the named savepoint, and the engine's own doc says so.
This mattered because the wrong claim was the stated justification for the
Python-side guard — "turns that into a cleaner, more specific
AlreadyFinishedError" — reading as if the guard merely re-labels an error the
engine would raise anyway. It doesn't: a second engine-level
rollback_towould succeed, so the guard is the only thing enforcing the documented
single-use contract. Comment corrected and the guard re-justified on its own
terms. Two test comments repeating the same wrong model are fixed.
PYTHON-4 — Tags section documented the pre-I126 API
The README described tag
0as an "untagged" sentinel andtag()as returningan int. Neither is true:
require_tagrejects0withValueError: tag must be non-zeroon every tagged method, andtag()returnsOptional[int].python/chisel/__init__.pyalready documented it correctly, sothe two docs contradicted each other.
A reader following the old text writes
if db.handles_with_tag(0):and gets anunhandled
ValueError, orif tx.tag(h) == 0:which never matches — silentlymisclassifying every untagged handle.
PYTHON-5 — the only defrag example could not be run
DefragOptions(sparse_threshold=0.25, max_pages=0)raisesTypeErrorbeforereaching the engine; the field is
max_values, andmax_pagesappears nowherein the codebase. Also dropped the pointer to "
DefragOptions.max_pages'sdocstring" (does not exist) and the claim that defrag "lives on the Chisel
object, not the Transaction object" —
PyTransaction::defragexists and istested by
test_transaction_defrag_mid_tx.PYTHON-6 — the type stub was inert; now it is checked in CI
Named
chisel/chisel.pyi, PEP 561 resolves the stub to modulechisel.chisel,which does not exist. No checker ever read its 244 lines, and drift accumulated
undetected —
__exit__was declared-> Nonein all three classes while everyimplementation returns
bool(db.rs:311,transaction.rs:92,savepoint.rs:65, eachOk(false)).chisel/__init__.pyi, pyproject include and header comment updated__exit__return typesVerified: mypy now resolves
import chiselthrough the stub (it reports errorsagainst the stub's declarations, which it previously could not see) and is
--strictclean at 3.13. The step is pinned to--python-version 3.13becausethe stub uses
collections.abc.Buffer(PEP 688, 3.12+) while the package stillruns on 3.11 — the stub's content doesn't vary by interpreter, so one pinned
check is enough.
PYTHON-8 — Thread safety forbade what the test suite certifies
The README said two threads "must never call into the same
Chiselat the sametime", while
test_two_thread_mutex_contentionruns 1600 concurrentread()calls across two threads and asserts no error, no corruption, no poison.
Meanwhile the property that actually bites was documented only in a Rust
comment users never see.
Rewritten to state what holds: calls serialize (GIL +
Mutex) so concurrencycannot corrupt; transaction state is what must not be shared across threads;
and since only
open()releases the GIL, a long commit ordefrag()blocksevery Python thread in the process — not just ones touching this database.
PYTHON-9 — encryption was named only in the error tables
The README listed
NoEncryptionKeyError,InvalidEncryptionKeyError,EncryptionNotSupportedError,NoFreeKeySlotErrorandLastKeySlotErrorwithout ever mentioning the
encryption_keykwarg oradd_key/rotate_key/remove_key. Combined with the inert stub, there was noin-tree source short of the Rust code. Adds an Encryption section and puts
encryption_keyin theopen()signature block.Verification
Every example and claim in the new section was executed against the built wheel:
the encryption round-trip, all three open-time mismatch errors, the 8-slot limit
(
NoFreeKeySlotError raised with 8 slots occupied),LastKeySlotError,InvalidEncryptionKeyErroron each of the three key methods, andTransactionInProgressErroron each mid-transaction.That caught an error of my own: I first wrote that a raw key "must be 32 bytes".
It doesn't —
derive_kekuses the bytes as HKDF input keying material andrejects only empty input, so any non-empty length works. I also wrote that
b"hunter2"and"hunter2"derive different keys; they don't, because KDFdispatch is on the slot's recorded
kdf_idrather than theKeyvariant. Bothclaims are corrected in the shipped text.
cargo clippy --workspace -- -D warningsclean,cargo fmt --checkclean,138 Python tests passing, 682 Rust tests passing.
Noted for later in this stack
max_pagesalso appears stale inREADME.md:188andARCHITECTURE.md:609(the Rust
DefragOptionsfield ismax_valuestoo). Those belong to #99 and#98 respectively and are handled there, to keep this PR to the Python binding.