fix(store,bench): a read-only open mode, so a diagnostic cannot mutate its subject (#1328) - #1330
Conversation
… subject (#1328) `MemoryStore(path)` is a write open. It runs the DDL battery, any pending migrations, the `schema_meta` generation seed, `_resolve_local_scope_id` (which mints and persists a federation identity on a store that has none), eleven guarded one-shot passes, and -- since #1314 -- `sweep_expired_locks`, which flips expired user locks back to unlocked. That is correct for a real open and wrong for a diagnostic. Two shipped benchmarks point their store argument at the live `.git/aelfrice/memory.db` in their own usage text, so running them mutated the corpus they existed to measure. Reproduced on a copy of the live store: planting one expired time-boxed lock and then doing nothing but constructing a MemoryStore and closing it flipped `lock_level` from `user` to `none`, wrote a sweep marker, and changed the file bytes. `read_only=True` opens `file:<path>?mode=ro` and skips the whole open-time write window. The engine refuses writes, so the guarantee does not rest on the caller's discipline -- which matters, because the previous guarantee was a sentence in a benchmark docstring and it did not hold. Three details worth stating: - `_run_guarded_migration` returns early on a read-only handle. Not needed for correctness (mode=ro already refuses, and the guard swallows the raise), but without it eleven passes raise and log at ERROR on every read-only open. Pinned by a log-quiet test, since the correctness tests cannot see it. - `_read_only_scope_id` reads the persisted scope id and returns "" when absent rather than minting one. The value is consumed only by write paths, which cannot run here. - A missing file raises instead of being created. The read-write default creates an empty store, which reads as "the corpus is empty" rather than as "that is not the corpus". The schema is taken as found: no migration runs, so an older store is read at whatever shape it has. Correct for a diagnostic, wrong for anything needing the current schema, hence opt-in.
`r3_idf_clip_bound.py` documents `--store .git/aelfrice/memory.db` and `temporal_spine_shadow.py` points `--db` at the same file; both opened it read-write. `temporal_spine_shadow` keeps a write handle when `--backfill` is passed, because that flag is a deliberate write and the honest rule is not "never construct a MemoryStore" but "open for write only when writes were asked for". `r3_idf_clip_bound` has no write mode and is read-only outright. `benchmarks/consolidate_blocking_recall.py` was already fixed this way in 684c089 after review; this closes the other two instances of the same class.
…t regress (#1328) Behavioural arms prove the specific mutation observed is prevented -- the expired lock survives a read-only open -- with a read-write control so the assertion cannot pass on a fixture that never armed. A byte-comparison arm catches the writes that change nothing a query can see (migrations, the generation seed, the scope-id mint), and an engine-refusal arm covers the writes nobody thought of, which is the difference between this and the docstring it replaces. The static guard enumerates `benchmarks/*.py` from the directory rather than from a literal list, and parses with `ast` rather than grepping, so it flags any `MemoryStore(...)` call lacking `read_only=` in a module that names a live store path. A literal list would pass forever while the directory grew around it, and growth is the failure mode: three benchmarks reached for the convenient call independently. Paired with a self-test that feeds the detector a synthetic offender, so a refactor that stops matching fails there instead of turning the real check green.
Insert-only at the head of Unreleased/Fixed; bullet count +1, dupes gate clean. Carries the reproduction rather than the claim, since the previous read-only guarantee was also written down and was not true.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
robotrocketscience
added
the
attn:review
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesRead-only diagnostic access
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Diagnostic
participant MemoryStore
participant SQLite
Diagnostic->>MemoryStore: open with read_only=True
MemoryStore->>SQLite: connect using mode=ro
SQLite-->>MemoryStore: provide existing database data
MemoryStore-->>Diagnostic: return read-only store
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Reviewer's GuideIntroduce a read-only mode for MemoryStore and ensure shipped benchmarks and tests use it so diagnostics cannot mutate the store they measure, while adding guards around migrations and live-store usage. Sequence diagram for benchmark store opening with read-only modesequenceDiagram
actor Operator
participant temporal_spine_shadow as temporal_spine_shadow_main
participant r3_idf_clip_bound as r3_idf_clip_bound_main
participant MemoryStore
participant SQLiteEngine
Operator->>temporal_spine_shadow: run with argv
temporal_spine_shadow->>temporal_spine_shadow: parse_args()
temporal_spine_shadow->>MemoryStore: MemoryStore(args.db, read_only=not args.backfill)
alt args.backfill is True
MemoryStore->>SQLiteEngine: sqlite3.connect(path)
MemoryStore->>SQLiteEngine: PRAGMA journal_mode=WAL
MemoryStore->>SQLiteEngine: PRAGMA synchronous=NORMAL
MemoryStore->>MemoryStore: _retry_on_schema_change(_apply_open_schema)
MemoryStore->>MemoryStore: _run_guarded_migration(pass_*)
MemoryStore->>MemoryStore: sweep_expired_locks()
else args.backfill is False (diagnostic)
MemoryStore->>SQLiteEngine: sqlite3.connect(file_path_mode_ro, uri=True)
MemoryStore->>MemoryStore: _read_only_scope_id()
Note over MemoryStore,SQLiteEngine: no DDL, migrations, seeds, or sweeps run
end
Operator->>r3_idf_clip_bound: run with argv
r3_idf_clip_bound->>r3_idf_clip_bound: parse_args()
r3_idf_clip_bound->>MemoryStore: MemoryStore(args.store, read_only=True)
MemoryStore->>SQLiteEngine: sqlite3.connect(file_path_mode_ro, uri=True)
MemoryStore->>MemoryStore: _read_only_scope_id()
Note over MemoryStore,SQLiteEngine: diagnostic reads without mutating store
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
|
[claim:review:Kulili:2026-08-04T22:05:19Z] |
…ontract The docstring says True means the pass completed and False means it raised. The #1328 read-only gate adds a third case — skipped — and returns True for it, which the contract as written does not cover. True is the right choice of the two available: False is consumed as "record a failure marker", and writing that marker is itself a write. Worth stating, since a reader checking the return value has no way to tell a skip from a success.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/aelfrice/store.py (2)
1719-1726: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the
Returnsline for the read-only branch.Line 1719 states that
Truemeans the pass completed. On a read-only store the method returnsTruewithout running the pass. No call site reads the value today, so behavior is unaffected, but the docstring contract is now inaccurate.♻️ Proposed docstring correction
- Returns True if the pass completed, False if it raised. + Returns True if the pass completed or was skipped (read-only + store), False if it raised. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aelfrice/store.py` around lines 1719 - 1726, Update the docstring for the method containing the _read_only guard to state that it returns True when the pass completes or is skipped for a read-only store, and False when the pass raises. Leave the existing return behavior unchanged.
1341-1347: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEscape the file path before building the SQLite URI.
file:{path}?mode=roparses everything after the first?as query parameters. A path such as.../memory.db#fragment.dbdrops themode=roquery, so the handle can be writable. UsePath(path).as_uri()or percent-encode the path before appending the query.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aelfrice/store.py` around lines 1341 - 1347, Update the read-only connection branch in the store initializer to construct a properly escaped SQLite URI from path before appending the mode=ro query, using Path(path).as_uri() or equivalent percent-encoding. Preserve the existing read-only behavior for non-memory paths and the current connection options.tests/test_readonly_diagnostics_1328.py (2)
167-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe guard misses an aliased import and an indirectly resolved store path.
Two shapes bypass this detector.
_bare_memorystore_callsmatches only the literal nameMemoryStore. An aliased import evades it:from aelfrice.store import MemoryStore as Store s = Store(".git/aelfrice/memory.db") # not reported
_live_store_pathsis a substring match on the module text. A benchmark that resolves the live path throughdb_pathsinstead of writing the literal is skipped entirely, so itsMemoryStore(...)calls are never inspected.Both gaps sit on the axis the class docstring names: a new benchmark reaching for the convenient call. Resolving the import alias in
_bare_memorystore_calls, and addingdb_pathsto the_live_store_pathstrigger, closes them.♻️ Proposed detector widening
`@staticmethod` def _live_store_paths(module: Path) -> bool: text = module.read_text(encoding="utf-8", errors="replace") - return "aelfrice/memory.db" in text or "AELFRICE_DB" in text + return ( + "aelfrice/memory.db" in text + or "AELFRICE_DB" in text + or "db_paths" in text + ) `@staticmethod` def _bare_memorystore_calls(module: Path) -> list[int]: """Line numbers of `MemoryStore(...)` calls with no `read_only=`.""" try: tree = ast.parse(module.read_text(encoding="utf-8")) except SyntaxError: # pragma: no cover - a broken benchmark return [] + # Resolve `import ... as` so an alias cannot slip past the name + # match below. The local binding is what the call site uses. + names = {"MemoryStore"} + for node in ast.walk(tree): + if isinstance(node, (ast.Import, ast.ImportFrom)): + for alias in node.names: + if alias.name.rsplit(".", 1)[-1] == "MemoryStore": + names.add(alias.asname or alias.name) bare: list[int] = [] for node in ast.walk(tree): if not isinstance(node, ast.Call): continue fn = node.func name = ( fn.id if isinstance(fn, ast.Name) else fn.attr if isinstance(fn, ast.Attribute) else None ) - if name != "MemoryStore": + if name not in names: continue if not any(k.arg == "read_only" for k in node.keywords): bare.append(node.lineno) return bareExtend
test_the_guard_can_actually_firewith an aliased offender so the new branch has its own control.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_readonly_diagnostics_1328.py` around lines 167 - 193, Update _bare_memorystore_calls to resolve imported aliases, including calls such as Store(...) when Store aliases MemoryStore, while preserving detection of direct and attribute forms. Expand _live_store_paths to trigger when module text references db_paths, and extend test_the_guard_can_actually_fire with a distinct aliased MemoryStore offender covering the new detection branch.
232-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an arm for a store with no
schema_metascope id.Every arm here seeds the database through
_store_with_an_expired_lock, which opens read-write and therefore mints and persists a local scope id. Both defensive branches of_read_only_scope_idstay unexecuted: theor ""fallback for an absent key, and theexcept sqlite3.DatabaseErrorfor a missing or incompatibleschema_metatable.
store.pydocuments the second branch as the legacy-store case that read-only mode exists to support, because a read-only open runs no DDL. A refactor that narrows the except clause or drops the fallback would pass this file unchanged.💚 Proposed test arm
def test_a_read_only_open_of_a_store_without_schema_meta_yields_no_scope_id( tmp_path: Path, ) -> None: """A store written by a pre-schema_meta binary must still open. A read-only open runs no DDL, so `schema_meta` may be absent. The honest answer for the scope id is then "none", not a mint and not a raise. """ db = tmp_path / "legacy.db" con = sqlite3.connect(str(db)) try: con.execute("CREATE TABLE beliefs (id TEXT PRIMARY KEY)") con.commit() finally: con.close() store = MemoryStore(str(db), read_only=True) try: assert store.local_scope_id == "" finally: store.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_readonly_diagnostics_1328.py` around lines 232 - 263, Add a test alongside test_a_read_only_open_logs_no_migration_failures that creates a legacy SQLite database containing beliefs but no schema_meta table, opens it with MemoryStore(read_only=True), and asserts local_scope_id is the empty string without raising. Ensure the setup avoids _store_with_an_expired_lock so both the missing-key fallback and DatabaseError path in _read_only_scope_id are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/aelfrice/store.py`:
- Around line 1719-1726: Update the docstring for the method containing the
_read_only guard to state that it returns True when the pass completes or is
skipped for a read-only store, and False when the pass raises. Leave the
existing return behavior unchanged.
- Around line 1341-1347: Update the read-only connection branch in the store
initializer to construct a properly escaped SQLite URI from path before
appending the mode=ro query, using Path(path).as_uri() or equivalent
percent-encoding. Preserve the existing read-only behavior for non-memory paths
and the current connection options.
In `@tests/test_readonly_diagnostics_1328.py`:
- Around line 167-193: Update _bare_memorystore_calls to resolve imported
aliases, including calls such as Store(...) when Store aliases MemoryStore,
while preserving detection of direct and attribute forms. Expand
_live_store_paths to trigger when module text references db_paths, and extend
test_the_guard_can_actually_fire with a distinct aliased MemoryStore offender
covering the new detection branch.
- Around line 232-263: Add a test alongside
test_a_read_only_open_logs_no_migration_failures that creates a legacy SQLite
database containing beliefs but no schema_meta table, opens it with
MemoryStore(read_only=True), and asserts local_scope_id is the empty string
without raising. Ensure the setup avoids _store_with_an_expired_lock so both the
missing-key fallback and DatabaseError path in _read_only_scope_id are
exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 11ea63ed-a8f1-4530-98ad-71aa0137fe31
📒 Files selected for processing (5)
CHANGELOG/v4.mdbenchmarks/r3_idf_clip_bound.pybenchmarks/temporal_spine_shadow.pysrc/aelfrice/store.pytests/test_readonly_diagnostics_1328.py
Review — approve. One docstring fix pushed (
|
| mutation | result |
|---|---|
| plant a benchmark that names the live store and opens it read-write | test_no_benchmark_opens_a_named_live_store_read_write fails |
revert the _run_guarded_migration read-only gate |
test_a_read_only_open_logs_no_migration_failures fails |
The second is the one worth calling out. The gate is not needed for correctness — mode=ro refuses and the guard swallows the raise — so the correctness arms stay green with it removed, and only the log-quiet arm catches it. The PR body says this was found because the mutation came back green. That is the right way round: a mutation that fails to kill anything is a finding about the tests, not a licence to skip the gate.
The AST-over-the-directory approach is also right, and the body names the reason: three benchmarks reached for the convenient call independently. A literal list of the two known offenders would pass forever while the directory grew around it. I can confirm the growth is real — the third instance it mentions (684c0899) was mine, caught in review of #1316 today.
Forward-compatibility with two benchmarks landing now
I checked the guard against the files about to arrive rather than assuming they were fine:
benchmarks/ingest_log_ulid_clusters.py(exp(1283): characterise the ULID-prefix clusters before AC2 keys on them #1331, open) — passes.benchmarks/consolidate_blocking_recall.py(merged today in fix(consolidate): blocking discarded whole near-duplicate families — 0 reported exactly when the family is largest (#1316) #1317) — passes.
Both use a raw mode=ro connection with no MemoryStore call, so the detector has nothing to flag. Worth noting that this is a second correct answer the guard permits, and permitting it is right: #1331 needs no store interface at all, whereas temporal_spine_shadow.py would have had to re-implement BM25Index.build's. The body's argument against a raw-sqlite3 shim is about that script, not a general rule, and the guard correctly doesn't try to impose one.
Pushed: the return contract now has three cases
_run_guarded_migration's docstring says True means the pass completed and False means it raised. The read-only gate adds skipped and returns True for it, which the contract as written does not cover — a reader checking the return value cannot distinguish a skip from a success. Stated it, with the reason True is the right choice of the two available: False is consumed as "record a failure marker", and writing that marker is itself a write. Docstring only.
On the revised AC1
Agreed, and the revision is better than what was filed. "Neither constructs a MemoryStore" is wrong for temporal_spine_shadow.py, which has a legitimate --backfill write path; read_only=not args.backfill is the rule that survives contact. Worth having recorded on the issue rather than silently satisfied by a weaker criterion.
Verification
- Full suite on the branch: 7,081 passed, 69 skipped, 71 xfailed. No contention flake this run.
- FF on main, discretion grep clean on added lines, all five commits
G-signed.
One note, no action
The schema is taken as found, so a read-only open of an older store reads whatever shape it has. The docstring says this and says why it is opt-in. The consequence worth remembering downstream: a diagnostic that reads a column added by a pending migration will get a bare sqlite3.DatabaseError, not a graceful degrade. _read_only_scope_id already handles that for its own read; nothing else has to yet.
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged f288578 → |
|
[release:review:Kulili:2026-08-04T22:14:47Z] |
1 participant
Closes #1328.
MemoryStore(path)is a write open. Two shipped diagnostics point their store argument at the live.git/aelfrice/memory.dbin their own usage text and open it that way, so running either mutates the corpus it exists to measure.Reproduced before fixing
On a
.backup()copy of the live store, planting one time-boxed lock whose window had already closed, then doing nothing but constructing aMemoryStoreand closing it:No analysis code ran. The constructor did it.
What a bare open does
Not just the #1314 sweep — this is a class of exposure, which is why the fix is a mode rather than three edits:
_MIGRATIONS/_POST_MIGRATION_INDEXES;schema_metastore-generation seed and the origin backfill;_resolve_local_scope_id, which mints and persists a federation identity on a store that has none;_run_guarded_migrationpasses, each able to write amigration_failed:marker;sweep_expired_locks, the one that changes user-visible state.It also takes write locks, so a diagnostic run alongside a live session contends with it.
The fix
MemoryStore(..., read_only=True)opensfile:<path>?mode=roand skips the whole open-time write window. The engine refuses writes, so the guarantee does not rest on the caller's discipline — which matters here, because the previous guarantee was a sentence in a benchmark docstring and it did not hold.Three details worth surfacing for review:
_run_guarded_migrationreturns early on a read-only handle. This is not needed for correctness —mode=roalready refuses and the guard already swallows the raise. It is needed for quiet: without it, eleven passes raise and log at ERROR on every read-only open. I only found that because the mutation test for the gate came back green; there is now a log-quiet arm that fails when the gate is removed, because the correctness arms cannot see it._read_only_scope_idreads the persisted id and returns""when absent rather than minting one. The value is consumed only by write paths, which cannot run here.The schema is taken as found — no migration runs, so an older store is read at whatever shape it has. Correct for a diagnostic (observe, don't upgrade), wrong for anything needing the current schema, hence opt-in.
One revision to the issue's acceptance criteria
I filed AC1 as "neither constructs a
MemoryStore". That is wrong fortemporal_spine_shadow.py, which has a--backfillflag that legitimately writes. The rule that survives contact is open for write only when writes were asked for, so that script usesread_only=not args.backfillandr3_idf_clip_bound.py— which has no write mode — is read-only outright. A raw-sqlite3shim would also have meant re-implementingBM25Index.build's store interface, and the next diagnostic would have re-implemented it again.The guard
The static test enumerates
benchmarks/*.pyfrom the directory and parses each withast, flagging anyMemoryStore(...)call withoutread_only=in a module that names a live-store path. A literal list of the two offenders would pass forever while the directory grew around it, and growth is the failure mode — three benchmarks reached for the convenient call independently, the third caught in review of #1316 (684c0899). Paired with a self-test that feeds the detector a synthetic offender, so a refactor that stops matching fails there rather than turning the real check green.Verification
Mutations, all verified red:
read_onlystops openingmode=roFull suite: 7,081 passed, 69 skipped, 71 xfailed. CHANGELOG insert-only at the head of
Unreleased/Fixed(104 → 105, dupes gate clean). Four atomic signed commits.Reproduction run against a read-only
.backup()copy of the repo-local live store (44,594 active beliefs), 2026-08-04.Summary by Sourcery
Introduce a read-only mode for MemoryStore to ensure diagnostics and benchmarks can inspect stores without mutating them, and add guards and tests to prevent future accidental write opens against live data.
New Features:
read_onlyflag to MemoryStore to support opening SQLite stores in engine-enforced read-only mode for diagnostic use.Bug Fixes:
Enhancements:
Documentation:
Tests:
read_onlyargument, with a self-test to validate the detector and prevent regressions.Summary by CodeRabbit
New Features
Bug Fixes
Tests