Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
198 changes: 137 additions & 61 deletions scripts/retro_quarantine_self_pollution.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
RECONCILE_CHUNK_ID_TABLE = "_retro_quarantine_reconcile_chunk_ids"
DEFAULT_ESTIMATE = 244_152
RETRIEVABILITY_TOKEN_RE = re.compile(r"[A-Za-z0-9]{4,}")
NON_OPERATIONAL_FTS_CLASS_SQL = "COALESCE(content_class, 'knowledge') NOT IN ('operational', 'test', 'benchmark')"


def _utc_now() -> str:
Expand Down Expand Up @@ -132,6 +133,16 @@ def _drop_fts_triggers(cursor: apsw.Cursor) -> None:
cursor.execute(f"DROP TRIGGER IF EXISTS {trigger_name}")


def _table_exists(cursor: apsw.Cursor, table_name: str) -> bool:
return (
cursor.execute(
"SELECT 1 FROM sqlite_master WHERE type IN ('table','view') AND name = ?",
(table_name,),
).fetchone()
is not None
)


def _ensure_manifest(cursor: apsw.Cursor) -> None:
existing = cursor.execute(
"SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = ?",
Expand Down Expand Up @@ -347,30 +358,61 @@ def capture_restore_state(cursor: apsw.Cursor, chunk_ids: list[str]) -> dict[str
return state


def _record_manifest(cursor: apsw.Cursor, chunk_ids: list[str], *, run_id: str, timestamp: str) -> None:
for chunk_id in chunk_ids:
row = cursor.execute(
"""
SELECT 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
WHERE c.id = ?
""",
(chunk_id,),
).fetchone()
if row is None:
raise ValueError(f"missing chunk: {chunk_id}")
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),
)
Comment on lines +361 to +391

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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' -C2

Repository: 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.py

Repository: 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.py

Repository: 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



def _delete_fts_rows_for_reconcile(
cursor: apsw.Cursor,
*,
table_names: tuple[str, ...] = ("chunks_fts", "chunks_fts_operational", "chunks_fts_trigram"),
) -> None:
placeholders = f"SELECT chunk_id FROM {RECONCILE_CHUNK_ID_TABLE}"
for table_name in table_names:
cursor.execute(f"DELETE FROM {table_name} WHERE chunk_id IN ({placeholders})")
updates = []
if "chunks_fts" in table_names:
updates.append("fts_rowid = NULL")
if "chunks_fts_trigram" in table_names:
updates.append("trigram_rowid = NULL")
if "chunks_fts_operational" in table_names:
updates.append("operational_rowid = NULL")
if updates:
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
) VALUES (?, ?, ?, ?, ?, ?, ?, ?)
""",
(run_id, chunk_id, row[0], row[1], row[2], row[3], row[4], timestamp),
UPDATE chunk_fts_rowids
SET {", ".join(updates)}
WHERE chunk_id IN ({placeholders})
"""
)


Expand Down Expand Up @@ -408,44 +450,75 @@ def _load_chunk_ids_reconcile_table(cursor: apsw.Cursor, chunk_ids: list[str]) -
)


def _clear_trigram_fts(cursor: apsw.Cursor) -> None:
def _insert_operational_fts(cursor: apsw.Cursor) -> None:
cursor.execute(
f"DELETE FROM chunks_fts_operational WHERE chunk_id IN (SELECT chunk_id FROM {RECONCILE_CHUNK_ID_TABLE})"
)
cursor.execute(
f"""
DELETE FROM chunks_fts_trigram
WHERE chunk_id IN (SELECT chunk_id FROM {RECONCILE_CHUNK_ID_TABLE})
INSERT INTO chunks_fts_operational({FTS_COLUMNS})
SELECT {FTS_SELECT_COLUMNS}
FROM chunks
WHERE id IN (SELECT chunk_id FROM {RECONCILE_CHUNK_ID_TABLE})
ORDER BY id
"""
)
cursor.execute(
f"""
UPDATE chunk_fts_rowids
SET trigram_rowid = NULL
WHERE chunk_id IN (SELECT chunk_id FROM {RECONCILE_CHUNK_ID_TABLE})
INSERT INTO chunk_fts_rowids(chunk_id, operational_rowid)
SELECT chunk_id, rowid FROM chunks_fts_operational WHERE chunk_id IN (SELECT chunk_id FROM {RECONCILE_CHUNK_ID_TABLE})
ON CONFLICT(chunk_id) DO UPDATE SET operational_rowid = excluded.operational_rowid
"""
)


def _insert_operational_fts(cursor: apsw.Cursor, chunk_ids: list[str]) -> None:
placeholders = _placeholders(chunk_ids)
def _rebuild_knowledge_fts(cursor: apsw.Cursor) -> None:
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}
Comment on lines +476 to 480

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve original FTS rowids during rebuild

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 👍 / 👎.

FROM chunks
WHERE id IN ({placeholders})
ORDER BY id
""",
chunk_ids,
WHERE {NON_OPERATIONAL_FTS_CLASS_SQL}
"""
)


def _rebuild_trigram_fts(cursor: apsw.Cursor) -> None:
cursor.execute("DELETE FROM chunks_fts_trigram")
cursor.execute(
f"""
INSERT INTO chunks_fts_trigram({FTS_COLUMNS})
SELECT {FTS_SELECT_COLUMNS}
FROM chunks
WHERE {NON_OPERATIONAL_FTS_CLASS_SQL}
"""
)


def _rebuild_chunk_fts_rowids(cursor: apsw.Cursor, *, include_trigram: bool) -> None:
cursor.execute("DELETE FROM chunk_fts_rowids")
cursor.execute(
"""
INSERT INTO chunk_fts_rowids(chunk_id, fts_rowid)
SELECT chunk_id, rowid FROM chunks_fts WHERE chunk_id IS NOT NULL
"""
)
cursor.execute(
"""
INSERT INTO chunk_fts_rowids(chunk_id, operational_rowid)
SELECT chunk_id, rowid
FROM chunks_fts_operational
WHERE chunk_id IN ({placeholders})
SELECT chunk_id, rowid FROM chunks_fts_operational WHERE chunk_id IS NOT NULL
ON CONFLICT(chunk_id) DO UPDATE SET operational_rowid = excluded.operational_rowid
""",
chunk_ids,
"""
)
if include_trigram:
cursor.execute(
"""
INSERT INTO chunk_fts_rowids(chunk_id, trigram_rowid)
SELECT chunk_id, rowid FROM chunks_fts_trigram WHERE chunk_id IS NOT NULL
ON CONFLICT(chunk_id) DO UPDATE SET trigram_rowid = excluded.trigram_rowid
"""
)


def _insert_fts_row(
Expand Down Expand Up @@ -495,32 +568,35 @@ def apply_quarantine_ids(
_drop_fts_triggers(cursor)
_ensure_manifest(cursor)
timestamp = _utc_now()
include_trigram = _table_exists(cursor, "chunks_fts_trigram")
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

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
Comment on lines 572 to +599

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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.

except Exception:
if not conn.getautocommit():
cursor.execute("ROLLBACK")
Expand Down
Loading
Loading