perf: rebuild quarantine apply from scratch#575
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_98cb6e1d-1229-459c-89ac-33fd8b8e1d2f) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7101018395
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cursor.execute("DELETE FROM chunks_fts") | ||
| cursor.execute( | ||
| f""" | ||
| INSERT INTO chunks_fts_operational({FTS_COLUMNS}) | ||
| INSERT INTO chunks_fts({FTS_COLUMNS}) | ||
| SELECT {FTS_SELECT_COLUMNS} |
There was a problem hiding this comment.
When any quarantined chunk originally has an FTS rowid that is lower than a preserved knowledge chunk, this full rebuild deletes chunks_fts and reinserts preserved rows with implicit rowids, so the preserved row can reuse the quarantined chunk's saved original_fts_rowid; a later unquarantine_ids() then tries to reinsert the quarantined row at that saved rowid and fails with an APSW ConstraintError. This breaks the rollback path for normal mixed databases, so the rebuild needs to either preserve rowids for non-quarantined rows or avoid restoring conflicting rowids from the manifest.
Useful? React with 👍 / 👎.
|
|
||
| incremental_state = _snapshot_apply_state(script, legacy_db, run_id=run_id, chunk_ids=all_ids) | ||
| rebuilt_state = _snapshot_apply_state(script, rebuild_db, run_id=run_id, chunk_ids=all_ids) | ||
| baseline_state = _snapshot_apply_state(script, base_db, run_id="baseline", chunk_ids=all_ids) |
There was a problem hiding this comment.
This test calls _snapshot_apply_state() on base_db before any quarantine code has created retro_self_pollution_quarantine_manifest, but the helper unconditionally queries that table, so pytest tests/test_retro_self_pollution_quarantine.py currently fails with apsw.SQLError: no such table: retro_self_pollution_quarantine_manifest before it can validate the rebuild behavior. The baseline path should create the manifest table or treat a missing manifest as None.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughThe retro quarantine script is refactored to use set-based reconciliation for applying chunk quarantine: chunk IDs are loaded once into a temp table, and manifest recording, FTS row deletion, class updates, and FTS/rowid rebuilds occur in a single transaction rather than per-batch incremental updates. A trigram-existence check conditionally includes trigram FTS rebuilding. A new test suite adds DB snapshot/copy helpers and a parity test comparing legacy incremental behavior against the new rebuild path. ChangesReconcile-driven quarantine apply
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| (AGENT_INFERENCE,), | ||
| ) | ||
| _insert_operational_fts(cursor) | ||
| _rebuild_knowledge_fts(cursor) |
There was a problem hiding this comment.
🟠 High scripts/retro_quarantine_self_pollution.py:591
apply_quarantine_ids() calls _rebuild_knowledge_fts(), _rebuild_trigram_fts(), and _rebuild_chunk_fts_rowids(), which delete and re-insert every non-operational FTS row across the entire database, assigning fresh rowid values to all of them. The manifest only stores the pre-quarantine original_fts_rowid/original_trigram_rowid for the chunks in the current run. After a second quarantine run rebuilds the FTS tables, those saved rowids may now belong to unrelated chunks. A subsequent unquarantine_ids(..., run_id=<old run>) reinserts rows with those stale rowids, producing a duplicate-rowid conflict or an incorrect FTS-rowid mapping. Consider not rebuilding all FTS tables on every run, or storing enough state to remap stale rowids during unquarantine.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @scripts/retro_quarantine_self_pollution.py around line 591:
`apply_quarantine_ids()` calls `_rebuild_knowledge_fts()`, `_rebuild_trigram_fts()`, and `_rebuild_chunk_fts_rowids()`, which delete and re-insert every non-operational FTS row across the entire database, assigning fresh `rowid` values to all of them. The manifest only stores the pre-quarantine `original_fts_rowid`/`original_trigram_rowid` for the chunks in the current run. After a second quarantine run rebuilds the FTS tables, those saved `rowid`s may now belong to unrelated chunks. A subsequent `unquarantine_ids(..., run_id=<old run>)` reinserts rows with those stale `rowid`s, producing a duplicate-`rowid` conflict or an incorrect FTS-rowid mapping. Consider not rebuilding all FTS tables on every run, or storing enough state to remap stale `rowid`s during unquarantine.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@scripts/retro_quarantine_self_pollution.py`:
- Around line 572-599: The single-transaction flow in
retro_quarantine_self_pollution.py leaves `batches_since_checkpoint` and the
`checkpoint_every` check in `_reconcile_chunks` effectively dead, and
`batch_size` is validated but never used. Remove the unused counter and
unreachable mid-flow `_checkpoint` logic, and either drop `batch_size` from the
function path or explicitly note it is kept only for API compatibility so the
`reconcile`/`finalize` behavior is clear.
- Around line 361-391: The _record_manifest function still unconditionally reads
from chunks_fts_trigram, which breaks runs on databases where that table is
absent. Update _record_manifest to accept or derive the same include_trigram
condition used by apply_quarantine_ids, and make the original_trigram_rowid
field return NULL when trigram support is not available. Keep the existing rowid
lookups for chunks_fts and chunks_fts_operational intact, and ensure the
manifest insert only references chunks_fts_trigram when that table exists.
In `@tests/test_retro_self_pollution_quarantine.py`:
- Around line 105-113: The UPDATE in the quarantine test is still hardcoding the
provenance value instead of using the shared constant. Replace the literal in
the `cursor.execute` call with `script.AGENT_INFERENCE` so the test stays
aligned with `apply_quarantine_ids` and any future constant changes, and keep
the change localized to the batch update logic in
`test_retro_self_pollution_quarantine`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3f72722-e574-4116-a972-b57b11d42da1
📒 Files selected for processing (2)
scripts/retro_quarantine_self_pollution.pytests/test_retro_self_pollution_quarantine.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Flag risky DB or concurrency changes explicitly and do not hand-wave lock behavior
Enforce one-write-at-a-time concurrency constraint; reads are safe but brain_digest is write-heavy and must not run in parallel with other MCP work
Run pytest before claiming behavior changed safely; current test suite has 929 tests
Files:
tests/test_retro_self_pollution_quarantine.pyscripts/retro_quarantine_self_pollution.py
🪛 OpenGrep (1.23.0)
tests/test_retro_self_pollution_quarantine.py
[ERROR] 70-70: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 73-81: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 105-113: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 114-123: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 124-132: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 133-138: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
scripts/retro_quarantine_self_pollution.py
[ERROR] 362-369: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 372-391: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 401-401: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 454-456: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 457-465: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 466-472: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 477-484: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 489-496: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 581-589: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🔇 Additional comments (8)
scripts/retro_quarantine_self_pollution.py (3)
39-39: LGTM!Also applies to: 136-145
453-521: LGTM!
573-599: 🩺 Stability & AvailabilityNo concurrency issue here
run_apply()already requires--confirm-workers-stoppedand--confirm-watcher-paused, so this transaction is meant to run in an offline maintenance window.> Likely an incorrect or invalid review comment.tests/test_retro_self_pollution_quarantine.py (5)
4-9: LGTM!
46-52: LGTM!
55-85: LGTM!
258-326: LGTM!Also applies to: 341-352
327-340: 🎯 Functional CorrectnessNo rowid gap here:
capture_restore_state()includes each FTS table’srowidin the returned tuples, andassert restored == baselinecompares those tuples directly, so the restore check already covers rowid assignment.> Likely an incorrect or invalid review comment.
| def _record_manifest(cursor: apsw.Cursor, *, run_id: str, timestamp: str) -> None: | ||
| missing = cursor.execute( | ||
| f""" | ||
| SELECT r.chunk_id | ||
| FROM {RECONCILE_CHUNK_ID_TABLE} r | ||
| LEFT JOIN chunks c ON c.id = r.chunk_id | ||
| WHERE c.id IS NULL | ||
| """, | ||
| ).fetchall() | ||
| if missing: | ||
| raise ValueError(f"missing chunk: {missing[0][0]!r}") | ||
| cursor.execute( | ||
| f""" | ||
| INSERT OR REPLACE INTO {QUARANTINE_MANIFEST_TABLE} ( | ||
| run_id, chunk_id, original_content_class, original_provenance_class, | ||
| original_fts_rowid, original_trigram_rowid, original_operational_rowid, quarantined_at | ||
| ) | ||
| SELECT | ||
| ? AS run_id, | ||
| c.id, | ||
| c.content_class, | ||
| c.provenance_class, | ||
| (SELECT rowid FROM chunks_fts WHERE chunk_id = c.id ORDER BY rowid LIMIT 1), | ||
| (SELECT rowid FROM chunks_fts_trigram WHERE chunk_id = c.id ORDER BY rowid LIMIT 1), | ||
| (SELECT rowid FROM chunks_fts_operational WHERE chunk_id = c.id ORDER BY rowid LIMIT 1), | ||
| ? | ||
| FROM chunks c | ||
| INNER JOIN {RECONCILE_CHUNK_ID_TABLE} r ON r.chunk_id = c.id | ||
| """, | ||
| (run_id, timestamp), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'chunks_fts_trigram' scripts/retro_quarantine_self_pollution.py
rg -nP 'CREATE\s+VIRTUAL\s+TABLE.*chunks_fts_trigram|chunks_fts_trigram' -g '!*.pyc' -C2Repository: EtanHey/brainlayer
Length of output: 1259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant sections around _record_manifest and apply_quarantine_ids.
sed -n '330,620p' scripts/retro_quarantine_self_pollution.py
printf '\n--- schema/search ---\n'
# Search for manifest table creation and trigram-related schema references.
rg -n "QUARANTINE_MANIFEST_TABLE|original_trigram_rowid|chunks_fts_trigram|include_trigram" scripts/retro_quarantine_self_pollution.pyRepository: EtanHey/brainlayer
Length of output: 12756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '330,620p' scripts/retro_quarantine_self_pollution.py
printf '\n--- schema/search ---\n'
rg -n "QUARANTINE_MANIFEST_TABLE|original_trigram_rowid|chunks_fts_trigram|include_trigram" scripts/retro_quarantine_self_pollution.pyRepository: EtanHey/brainlayer
Length of output: 12756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact call order and manifest schema.
nl -ba scripts/retro_quarantine_self_pollution.py | sed -n '540,610p'
printf '\n---\n'
nl -ba scripts/retro_quarantine_self_pollution.py | sed -n '250,330p'Repository: EtanHey/brainlayer
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "_table_exists\\(cursor, \"chunks_fts_trigram\"\\)|chunks_fts_trigram" -g '!*.pyc' .Repository: EtanHey/brainlayer
Length of output: 20991
Guard the trigram rowid lookup in _record_manifest
scripts/retro_quarantine_self_pollution.py:384 still queries chunks_fts_trigram unconditionally, even though apply_quarantine_ids() checks _table_exists(cursor, "chunks_fts_trigram") and skips the trigram path when that table is absent. Pass include_trigram through here or return NULL for original_trigram_rowid; otherwise the quarantine aborts with no such table: chunks_fts_trigram on databases without that table.
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 362-369: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 372-391: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🤖 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 `@scripts/retro_quarantine_self_pollution.py` around lines 361 - 391, The
_record_manifest function still unconditionally reads from chunks_fts_trigram,
which breaks runs on databases where that table is absent. Update
_record_manifest to accept or derive the same include_trigram condition used by
apply_quarantine_ids, and make the original_trigram_rowid field return NULL when
trigram support is not available. Keep the existing rowid lookups for chunks_fts
and chunks_fts_operational intact, and ensure the manifest insert only
references chunks_fts_trigram when that table exists.
Source: Linters/SAST tools
| batches_since_checkpoint = 0 | ||
| try: | ||
| _load_chunk_ids_reconcile_table(cursor, chunk_ids) | ||
| for ids in _batch(chunk_ids, batch_size): | ||
| placeholders = _placeholders(ids) | ||
| cursor.execute("BEGIN IMMEDIATE") | ||
| _record_manifest(cursor, ids, run_id=run_id, timestamp=timestamp) | ||
| _delete_fts_rows(cursor, ids, table_names=("chunks_fts", "chunks_fts_operational")) | ||
| cursor.execute( | ||
| f""" | ||
| UPDATE chunks | ||
| SET content_class = 'operational', | ||
| provenance_class = ? | ||
| WHERE id IN ({placeholders}) | ||
| """, | ||
| (AGENT_INFERENCE, *ids), | ||
| ) | ||
| _insert_operational_fts(cursor, ids) | ||
| cursor.execute("COMMIT") | ||
| batches_since_checkpoint += 1 | ||
| if batches_since_checkpoint >= checkpoint_every: | ||
| _checkpoint(cursor) | ||
| batches_since_checkpoint = 0 | ||
| cursor.execute("BEGIN IMMEDIATE") | ||
| _clear_trigram_fts(cursor) | ||
| _record_manifest(cursor, run_id=run_id, timestamp=timestamp) | ||
| table_names = ["chunks_fts", "chunks_fts_operational"] | ||
| if include_trigram: | ||
| table_names.append("chunks_fts_trigram") | ||
| _delete_fts_rows_for_reconcile(cursor, table_names=tuple(table_names)) | ||
| cursor.execute( | ||
| f""" | ||
| UPDATE chunks | ||
| SET content_class = 'operational', | ||
| provenance_class = ? | ||
| WHERE id IN (SELECT chunk_id FROM {RECONCILE_CHUNK_ID_TABLE}) | ||
| """, | ||
| (AGENT_INFERENCE,), | ||
| ) | ||
| _insert_operational_fts(cursor) | ||
| _rebuild_knowledge_fts(cursor) | ||
| if include_trigram: | ||
| _rebuild_trigram_fts(cursor) | ||
| _rebuild_chunk_fts_rowids(cursor, include_trigram=include_trigram) | ||
| cursor.execute("COMMIT") | ||
| batches_since_checkpoint += 1 | ||
| if batches_since_checkpoint >= checkpoint_every: | ||
| _checkpoint(cursor) | ||
| batches_since_checkpoint = 0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Vestigial checkpoint counter and unused batch_size after single-transaction rewrite.
With batching removed, batches_since_checkpoint can only reach 1 after the lone COMMIT, so the >= checkpoint_every (default 3) guard at Line 597 never fires — the mid-flow _checkpoint is now dead (checkpointing effectively only happens in the finalize block). Likewise batch_size is validated at Line 555 but no longer used in the body. Consider dropping the counter and either removing batch_size or documenting it as retained for API compatibility.
♻️ Proposed cleanup
include_trigram = _table_exists(cursor, "chunks_fts_trigram")
- batches_since_checkpoint = 0
try:
_load_chunk_ids_reconcile_table(cursor, chunk_ids)
cursor.execute("BEGIN IMMEDIATE")
_record_manifest(cursor, run_id=run_id, timestamp=timestamp)
@@
_rebuild_chunk_fts_rowids(cursor, include_trigram=include_trigram)
cursor.execute("COMMIT")
- batches_since_checkpoint += 1
- if batches_since_checkpoint >= checkpoint_every:
- _checkpoint(cursor)
- batches_since_checkpoint = 0
+ _checkpoint(cursor)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| batches_since_checkpoint = 0 | |
| try: | |
| _load_chunk_ids_reconcile_table(cursor, chunk_ids) | |
| for ids in _batch(chunk_ids, batch_size): | |
| placeholders = _placeholders(ids) | |
| cursor.execute("BEGIN IMMEDIATE") | |
| _record_manifest(cursor, ids, run_id=run_id, timestamp=timestamp) | |
| _delete_fts_rows(cursor, ids, table_names=("chunks_fts", "chunks_fts_operational")) | |
| cursor.execute( | |
| f""" | |
| UPDATE chunks | |
| SET content_class = 'operational', | |
| provenance_class = ? | |
| WHERE id IN ({placeholders}) | |
| """, | |
| (AGENT_INFERENCE, *ids), | |
| ) | |
| _insert_operational_fts(cursor, ids) | |
| cursor.execute("COMMIT") | |
| batches_since_checkpoint += 1 | |
| if batches_since_checkpoint >= checkpoint_every: | |
| _checkpoint(cursor) | |
| batches_since_checkpoint = 0 | |
| cursor.execute("BEGIN IMMEDIATE") | |
| _clear_trigram_fts(cursor) | |
| _record_manifest(cursor, run_id=run_id, timestamp=timestamp) | |
| table_names = ["chunks_fts", "chunks_fts_operational"] | |
| if include_trigram: | |
| table_names.append("chunks_fts_trigram") | |
| _delete_fts_rows_for_reconcile(cursor, table_names=tuple(table_names)) | |
| cursor.execute( | |
| f""" | |
| UPDATE chunks | |
| SET content_class = 'operational', | |
| provenance_class = ? | |
| WHERE id IN (SELECT chunk_id FROM {RECONCILE_CHUNK_ID_TABLE}) | |
| """, | |
| (AGENT_INFERENCE,), | |
| ) | |
| _insert_operational_fts(cursor) | |
| _rebuild_knowledge_fts(cursor) | |
| if include_trigram: | |
| _rebuild_trigram_fts(cursor) | |
| _rebuild_chunk_fts_rowids(cursor, include_trigram=include_trigram) | |
| cursor.execute("COMMIT") | |
| batches_since_checkpoint += 1 | |
| if batches_since_checkpoint >= checkpoint_every: | |
| _checkpoint(cursor) | |
| batches_since_checkpoint = 0 | |
| try: | |
| _load_chunk_ids_reconcile_table(cursor, chunk_ids) | |
| cursor.execute("BEGIN IMMEDIATE") | |
| _record_manifest(cursor, run_id=run_id, timestamp=timestamp) | |
| table_names = ["chunks_fts", "chunks_fts_operational"] | |
| if include_trigram: | |
| table_names.append("chunks_fts_trigram") | |
| _delete_fts_rows_for_reconcile(cursor, table_names=tuple(table_names)) | |
| cursor.execute( | |
| f""" | |
| UPDATE chunks | |
| SET content_class = 'operational', | |
| provenance_class = ? | |
| WHERE id IN (SELECT chunk_id FROM {RECONCILE_CHUNK_ID_TABLE}) | |
| """, | |
| (AGENT_INFERENCE,), | |
| ) | |
| _insert_operational_fts(cursor) | |
| _rebuild_knowledge_fts(cursor) | |
| if include_trigram: | |
| _rebuild_trigram_fts(cursor) | |
| _rebuild_chunk_fts_rowids(cursor, include_trigram=include_trigram) | |
| cursor.execute("COMMIT") | |
| _checkpoint(cursor) |
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 581-589: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🤖 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 `@scripts/retro_quarantine_self_pollution.py` around lines 572 - 599, The
single-transaction flow in retro_quarantine_self_pollution.py leaves
`batches_since_checkpoint` and the `checkpoint_every` check in
`_reconcile_chunks` effectively dead, and `batch_size` is validated but never
used. Remove the unused counter and unreachable mid-flow `_checkpoint` logic,
and either drop `batch_size` from the function path or explicitly note it is
kept only for API compatibility so the `reconcile`/`finalize` behavior is clear.
| cursor.execute( | ||
| f""" | ||
| UPDATE chunks | ||
| SET content_class = 'operational', | ||
| provenance_class = ? | ||
| WHERE id IN ({placeholders}) | ||
| """, | ||
| ("AGENT-INFERENCE", *batch_ids), | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\bAGENT_INFERENCE\b\s*=' scripts/retro_quarantine_self_pollution.pyRepository: EtanHey/brainlayer
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== AGENT_INFERENCE references ==\n'
rg -n --hidden --glob '!.git' '\bAGENT_INFERENCE\b' .
printf '\n== relevant file list ==\n'
git ls-files | rg 'retro|quarantine|script|scripts'Repository: EtanHey/brainlayer
Length of output: 5202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== tests/test_retro_self_pollution_quarantine.py (relevant ranges) ==\n'
cat -n tests/test_retro_self_pollution_quarantine.py | sed -n '1,170p'
printf '\n---\n'
cat -n tests/test_retro_self_pollution_quarantine.py | sed -n '300,350p'
printf '\n== scripts/retro_quarantine_self_pollution.py (constant usage) ==\n'
cat -n scripts/retro_quarantine_self_pollution.py | sed -n '1,80p'
printf '\n---\n'
cat -n scripts/retro_quarantine_self_pollution.py | sed -n '570,610p'Repository: EtanHey/brainlayer
Length of output: 15536
Use script.AGENT_INFERENCE here instead of the hardcoded string. This keeps the legacy simulation aligned with apply_quarantine_ids and avoids drift if the provenance constant changes.
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 105-113: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🤖 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_retro_self_pollution_quarantine.py` around lines 105 - 113, The
UPDATE in the quarantine test is still hardcoding the provenance value instead
of using the shared constant. Replace the literal in the `cursor.execute` call
with `script.AGENT_INFERENCE` so the test stays aligned with
`apply_quarantine_ids` and any future constant changes, and keep the change
localized to the batch update logic in `test_retro_self_pollution_quarantine`.
Source: Linters/SAST tools
1 participant
Refactor retro quarantine apply to rebuild FTS indexes from scratch for non-operational chunks and add end-state parity test. Includes manifest record/revert compatibility and real backup performance work.
Note
Medium Risk
Apply still mutates chunk classification and full-text search indexes on production SQLite paths, but behavior is locked to the prior incremental model via parity and round-trip tests; manifest shape stays compatible with unquarantine.
Overview
Retro quarantine apply no longer walks denylisted chunk IDs in batches to patch FTS row-by-row. It loads IDs into a temp reconcile table, records the manifest in one set-oriented query, reclassifies those chunks to operational, refreshes their operational FTS rows, then clears and repopulates
chunks_ftsand (when present)chunks_fts_trigramfor all non-operational/test/benchmark chunks viaNON_OPERATIONAL_FTS_CLASS_SQL, and rebuildschunk_fts_rowidsfrom the FTS tables.Manifest capture now joins the reconcile table instead of looping per ID; FTS deletion for apply uses reconcile-scoped helpers. Trigram handling is gated on whether
chunks_fts_trigramexists.Tests add a fixture parity check: a simulated legacy incremental apply on a DB copy is compared to the new
apply_quarantine_idspath (chunk classes, FTS membership, manifest), with unquarantine restoring byte-identical FTS state to baseline.Reviewed by Cursor Bugbot for commit 7101018. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Rebuild quarantine apply from scratch using global FTS rebuild instead of per-batch incremental updates
apply_quarantine_idswith a single-transaction rebuild that truncates and repopulateschunks_fts,chunks_fts_trigram(when present), andchunk_fts_rowidsglobally._table_exists,_delete_fts_rows_for_reconcile,_insert_operational_fts,_rebuild_knowledge_fts,_rebuild_trigram_fts, and_rebuild_chunk_fts_rowids._record_manifestto validate chunk IDs against the reconcile table via LEFT JOIN rather than accepting explicit chunk IDs.📊 Macroscope summarized 7101018. 1 file reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.
Summary by CodeRabbit