perf(replay): N+1 belief loop → set-based query in _compute_replay_drift_report (#725) - #726
Conversation
Reviewer's GuideOptimizes canonical-orphan detection in replay drift reports by introducing a set-based store query and wiring it into replay, with targeted tests and changelog documentation. Sequence diagram for canonical orphan computation in replay drift reportsequenceDiagram
participant ReplayModule as replay_full_equality
participant MemoryStore
participant SQLite
ReplayModule->>MemoryStore: list_canonical_orphans()
MemoryStore->>SQLite: execute(SELECT b.id, b.content_hash ...)
SQLite-->>MemoryStore: rows(belief_id, content_hash)
MemoryStore-->>ReplayModule: all_orphans
ReplayModule->>ReplayModule: canonical_orphan = len(all_orphans)
loop for bid, content_hash in all_orphans[:drift_examples]
ReplayModule->>ReplayModule: append to examples_canonical_orphan
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces per-belief ingest-log scans in replay drift reporting with a set-based store query. Adds ChangesCanonical orphan query optimization
Sequence Diagram(s)sequenceDiagram
participant Replay as replay._compute_replay_drift_report
participant Store as MemoryStore.list_canonical_orphans
participant DB as SQLite
Replay->>Store: call list_canonical_orphans(limit)
Store->>DB: SELECT b.id, b.content_hash WHERE b.id NOT IN (SELECT belief_id FROM ingest_log WHERE source_kind != 'legacy_unknown')
DB-->>Store: rows (id, content_hash)
Store-->>Replay: list[(belief_id, content_hash)...]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labelsattn:review, author-Setr 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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 |
There was a problem hiding this comment.
Hey - I've found 1 security issue, and left some high level feedback:
Security issues:
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
General comments:
- The new
list_canonical_orphansquery changes the handling ofNULLsource_kindrows vs. the previous Python logic (str(r.get('source_kind', '')) != INGEST_SOURCE_LEGACY_UNKNOWNtreats missing/None as non-legacy), so ifsource_kindcan beNULLin practice you may want to explicitlyCOALESCE(il.source_kind, '') != ?(or similar) to preserve the old behavior. - To avoid subtle issues with
NOT INifjson_eachever yieldedNULLvalues (or future schema changes allow them), consider switching the subquery to aSELECT DISTINCT+WHERE je.value IS NOT NULLor using aNOT EXISTSpattern instead. - The
limit_clauseis interpolated directly into the SQL string; even thoughlimitis an internal int, you could use a parameterizedLIMIT ?for consistency with the rest of the store queries and to avoid hand-crafted SQL fragments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `list_canonical_orphans` query changes the handling of `NULL` `source_kind` rows vs. the previous Python logic (`str(r.get('source_kind', '')) != INGEST_SOURCE_LEGACY_UNKNOWN` treats missing/None as non-legacy), so if `source_kind` can be `NULL` in practice you may want to explicitly `COALESCE(il.source_kind, '') != ?` (or similar) to preserve the old behavior.
- To avoid subtle issues with `NOT IN` if `json_each` ever yielded `NULL` values (or future schema changes allow them), consider switching the subquery to a `SELECT DISTINCT` + `WHERE je.value IS NOT NULL` or using a `NOT EXISTS` pattern instead.
- The `limit_clause` is interpolated directly into the SQL string; even though `limit` is an internal int, you could use a parameterized `LIMIT ?` for consistency with the rest of the store queries and to avoid hand-crafted SQL fragments.
## Individual Comments
### Comment 1
<location path="src/aelfrice/store.py" line_range="1500-1513" />
<code_context>
cur = self._conn.execute(
f"""
SELECT b.id, b.content_hash
FROM beliefs b
WHERE b.id NOT IN (
SELECT je.value
FROM ingest_log il, json_each(il.derived_belief_ids) je
WHERE il.derived_belief_ids IS NOT NULL
AND il.source_kind != ?
)
ORDER BY b.id ASC{limit_clause}
""",
(INGEST_SOURCE_LEGACY_UNKNOWN,),
)
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/aelfrice/store.py (1)
1499-1517: ⚡ Quick winThis is a read-only query not covered by the file's write-path guideline.
The
list_canonical_orphans()method performs only a SELECT query and is not part of the belief write path, so the file's coding guideline (which applies to SQL bypassing broker-confidence attenuation in WRITE operations) does not apply here. While the f-string LIMIT clause creates a style risk that static analysis flags, theint(limit)conversion provides runtime safety.If you want to refactor this for consistency with general SQL best practices, Option 2 (sentinel value) is simpler than Option 1 (conditional queries).
🤖 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 1499 - 1517, The query in list_canonical_orphans() is read-only and the file-level write-path guideline doesn't apply; to remove the f-string style risk while keeping runtime safety, replace the dynamic f-string LIMIT construction (limit_clause) with a parameterized LIMIT: build the SQL without interpolated strings and, if limit is provided, append " LIMIT ?" to the query and pass int(limit) as an extra parameter alongside INGEST_SOURCE_LEGACY_UNKNOWN; keep handling of NULL content_hash and the same returned tuple format. Reference: list_canonical_orphans, limit_clause, INGEST_SOURCE_LEGACY_UNKNOWN.
🤖 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 1499-1517: The query in list_canonical_orphans() is read-only and
the file-level write-path guideline doesn't apply; to remove the f-string style
risk while keeping runtime safety, replace the dynamic f-string LIMIT
construction (limit_clause) with a parameterized LIMIT: build the SQL without
interpolated strings and, if limit is provided, append " LIMIT ?" to the query
and pass int(limit) as an extra parameter alongside
INGEST_SOURCE_LEGACY_UNKNOWN; keep handling of NULL content_hash and the same
returned tuple format. Reference: list_canonical_orphans, limit_clause,
INGEST_SOURCE_LEGACY_UNKNOWN.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ce52232-2496-4bf2-9b34-24825b676a74
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/CHANGELOG.md
📒 Files selected for processing (3)
src/aelfrice/replay.pysrc/aelfrice/store.pytests/test_store_crud.py
|
Addressed Sourcery's review on f20288a:
All 9 |
|
[claim:review:Newton:2026-05-13T02:07:42Z] |
|
Newton review — LGTM. VerdictApprove. Adding What I checked
On the bot comments
Mechanical
|
|
[release:review:Newton:2026-05-13T02:09:43Z] |
|
merge-train: blocked branch is not fast-forward on The |
f20288a to
a7fa14f
Compare
|
Rebased onto current Re-added |
|
merge-train: blocked required check(s) failed: The |
Add MemoryStore.list_canonical_orphans(limit) that returns (belief_id, content_hash) tuples for beliefs where every ingest_log row has source_kind='legacy_unknown' or no row exists at all. Single SQL pass: NOT IN (SELECT je.value FROM ingest_log il, json_each(il.derived_belief_ids) je WHERE source_kind != 'legacy_unknown') replaces the N+1 per-belief iteration. Results ordered ORDER BY b.id ASC to preserve sample stability matching list_belief_ids().
Nine tests in test_store_crud.py covering: - empty store, no log rows (pre-#205), all-legacy rows - non-legacy row excludes belief - mixed beliefs (orphans vs non-orphans) - mixed log rows on same belief (both legacy + non-legacy → not orphan) - limit= cap, ORDER BY id ASC ordering, content_hash tuple field
…ort (#725) Replace the N+1 per-belief loop (list_belief_ids + iter_ingest_log_for_belief per id + get_belief per orphan) with a single call to the new set-based list_canonical_orphans(). Removes the TODO(perf) comment. Output is byte-identical: canonical_orphan count and examples_canonical_orphan sample are produced from the same ordered set, capped at drift_examples.
Add Performance entry under [3.0.0] - Unreleased describing the list_canonical_orphans optimization and its byte-identical contract.
…ourcery) opengrep flagged the f-string LIMIT interpolation as a SQL-injection shape even though int(limit) made it runtime-safe. Switch to bound parameter for static-analysis cleanliness and consistency with the rest of the store query layer. No behaviour change. Sourcery's adjacent suggestions to COALESCE source_kind for NULL parity and switch NOT IN to NOT EXISTS for NULL-safe je.value are moot under the current schema: source_kind is TEXT NOT NULL and derived_belief_ids holds list[str]. Not adding defensive code for guaranteed invariants.
a7fa14f to
a8d0b8d
Compare
|
merge-train: merged a8d0b8d → |
Closes #725.
Summary
Replaces the N+1 belief-iteration loop in
src/aelfrice/replay.py:_compute_replay_drift_reportwith a single set-based store query. AddsMemoryStore.list_canonical_orphans(limit: int | None) -> list[tuple[str, str | None]]to encapsulate the SQL, matching the rest ofstore.py's pattern (the existingsynthesize_legacy_logpath already uses the samejson_each(derived_belief_ids)+NOT INshape).Diff
Four atomic SSH-signed commits:
feat(store): list_canonical_orphans set-based query (#725)— new method insrc/aelfrice/store.py. Single SQL pass: build the set of belief ids that appear inderived_belief_idsof at least one non-legacy_unknownlog row, thenNOT INselects the complement.ORDER BY b.id ASCfor sample stability (matcheslist_belief_ids()).test(store): unit tests for list_canonical_orphans (#725)— 9 new tests intests/test_store_crud.pycovering empty store, all-legacy beliefs, mixed, with/withoutlimit, ordering, and pre-[v2.0] Write log as source of truth — append-only ingest_log + replay-capable derivation #205 store with no log rows.perf(replay): use list_canonical_orphans in _compute_replay_drift_report (#725)— replaces the N+1 loop. Removes theTODO(perf):comment. The function output (canonical_orphancount +examples_canonical_orphansample) is byte-identical.docs(changelog): #725 N+1 → set-based query in replay drift report— entry under### Performancein## [3.0.0] - Unreleased.Touched files:
src/aelfrice/store.py(+43),src/aelfrice/replay.py(+7/-19),tests/test_store_crud.py(+128),CHANGELOG.md(+2).Acceptance (from #725)
MemoryStore.list_canonical_orphans()method with unit tests in thetests/test_store_crud.pypattern.replay.py:339-360consumes the new method; N+1 loop andTODO(perf):comment removed.tests/test_replay.py::test_compute_replay_drift_report_*pass byte-identically (all 20test_replay_full_equalitytests + 22test_store_crudtests green; full suite 3764 passed, 59 skipped, 75 xfailed).Bench
Synthetic store, 50% canonical-orphans, wall-clock for the canonical-orphan section only (averaged across 3 runs):
The old shape is O(N²) due to a full
ingest_logtable scan per belief (iter_ingest_log_for_beliefis documented as "Linear scan — v2.0 first slice has no inverted index"). New shape is O(N) — one set-build pass.The public v0.1 soak corpus has
canonical_orphan == 0(all beliefs have non-legacy log rows), sorun_replay_soakwall-clock (~9 ms) is noise-dominated before/after; the gain only surfaces at operator-store scale.Verification
git log --format='%G?' github/main..HEAD— all four commits signed (G).github/main.uv run pytest -x -q: 3764 passed, 59 skipped, 75 xfailed.iter_ingest_log_for_beliefuntouched (other callers exist; out of scope per perf(replay): replace N+1 belief loop in _compute_replay_drift_report with set-based query #725).list_belief_ids()usesORDER BY id ASC(store.py:1474); newlist_canonical_orphans()usesORDER BY b.id ASC. Order preserved.Out of scope (per #725)
iter_ingest_log_for_beliefmethod — kept; other callers exist.Summary by Sourcery
Replace the canonical-orphan computation in the replay drift report with a set-based store query for better performance while preserving output.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Chores
Refactor
Tests