feat(ingest): distinguish T3 app Codex sessions - #633
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.
📝 WalkthroughWalkthroughThe change adds T3-linked Codex session detection, provenance classification, deferred watcher handling, a retagging and rollback CLI, and tests. Two variable-name mismatches affect the report and watcher paths. ChangesT3 app provenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Watcher
participant T3StateDB
participant ProvenanceClassifier
participant BrainLayerDB
Watcher->>T3StateDB: load linked Codex session IDs
T3StateDB-->>Watcher: linked IDs or linkage alarm
Watcher->>ProvenanceClassifier: classify message with linked IDs
ProvenanceClassifier-->>Watcher: T3 or existing provenance
Watcher->>BrainLayerDB: persist confirmed message
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 20e0108. Configure here.
![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: 20e0108fa0
ℹ️ 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".
| for batch_start in range(0, len(candidates), batch_size): | ||
| batch = candidates[batch_start : batch_start + batch_size] | ||
| connection.executemany( | ||
| "UPDATE chunks SET provenance_class = ? WHERE id = ?", |
There was a problem hiding this comment.
When the watcher or enrichment changes a candidate after _candidates() reads it but before this batch executes, the unconditional update overwrites that newer provenance with t3-app-session. The busy timeout only serializes individual SQLite statements, while each batch commit leaves a race across the scan-to-update interval; add the expected codex-session value to the WHERE clause or hold the repository's writer lock for the operation.
AGENTS.md reference: AGENTS.md:L28-L30
Useful? React with 👍 / 👎.
| else {} | ||
| ) | ||
| existing.update(dict(candidates)) | ||
| with path.open("w", encoding="utf-8") as artifact: |
There was a problem hiding this comment.
When --apply is rerun with an existing artifact, this "w" open truncates the only rollback record before the preserved entries are rewritten. A crash, interruption, or ENOSPC during that rewrite can therefore destroy recovery data for rows changed by the earlier run even though the merge behavior explicitly supports artifact reuse; write a temporary file and atomically replace the artifact instead.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
| pre_restore_path.parent.mkdir(parents=True, exist_ok=True) | ||
| with pre_restore_path.open("x", encoding="utf-8") as artifact: | ||
| for chunk_id, _ in candidates: | ||
| artifact.write(json.dumps({"id": chunk_id, "provenance_class": current_values[chunk_id]}) + "\n") |
There was a problem hiding this comment.
If any chunk recorded in an older rollback artifact has since been deleted, current_values has no entry for that ID and this lookup raises KeyError. Because the pre-restore file has already been opened with "x", the failed rollback also leaves an empty or partial artifact that blocks retry, so later rollback of the remaining rows cannot proceed without manual cleanup; skip absent IDs or represent their absence explicitly.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
|
VERDICT: ITERATE |
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_c56bff01-49fd-4531-a378-51d94e69a48a) |
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_4d748502-2014-4a2e-9860-6aa39a776b64) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@codex review CI compatibility follow-up: constrain MCP to the supported 1.x API range after MCP 2.0 removed Server.list_tools. |
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_5db5e816-8ed7-4cde-a982-9766c34634e5) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17e293b630
ℹ️ 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".
| is_t3_app_initiated_codex_session( | ||
| source_file, | ||
| state_db=t3_state_db, | ||
| linked_session_ids=t3_linked_session_ids, |
There was a problem hiding this comment.
When a T3 state DB exists, scripts/provenance_classify_report.py:32-37 calls classify_provenance once for every chunk without supplying t3_linked_session_ids, so every Codex chunk reaching this branch reopens and rescans provider_session_runtime. Since a single session can contain hundreds of chunks, the canonical report now performs O(C×R) SQLite work and may classify one session inconsistently if T3 state changes mid-scan; load the linked IDs once in build_report and pass that set to each classification.
Useful? React with 👍 / 👎.
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_4966663d-fa30-4043-b672-dc9ec824ebf9) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@codex review Routed by The pin question is settled — do not re-litigate it, but do verify the reasoning holds:
Where the real risk is — not the pin: this branch predates the SDK v2 migration entirely, so any MCP SDK call site it touches could still be v1. The worker reports no v1 call sites survived; please verify that independently rather than accept it. Also check: this branch retags Codex sessions as Full gate on the branch: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aeaa5f01b6
ℹ️ 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".
| resume_cursor = json.loads(resume_cursor_json) | ||
| except (TypeError, json.JSONDecodeError) as exc: |
There was a problem hiding this comment.
When a Codex runtime exists before it has a resume cursor, resume_cursor_json can legitimately be NULL (the T3 schema leaves it nullable, and src/brainlayer/ingest/t3.py:280-286 already treats an absent cursor as no provider session). Passing that value to json.loads raises the fatal t3_runtime_linkage_invalid alarm; because watcher_bridge.py:352-353 loads all linked IDs before every flush, one such runtime prevents ingestion of every source, not merely the unlinked T3 session. Skip null/empty cursors while retaining the alarm for genuinely malformed non-empty JSON.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
Routed independent review — status: ITERATE resolved locally, awaiting pushPosting the routed verdict here so it is visible on the PR rather than only on the sprint board. This is not a GitHub approval — every fleet agent authenticates as Reviewer:
The blocking finding — F1
And the trigger is wider than a t3layer release. The guard is Local fix What the reviewer verified independently, not on claim
Out of scope, deliberatelyThe |
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_ffdb6cca-9c6e-467b-970c-77d662b38e48) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
| policies: Counter[str] = Counter() | ||
| visibility: Counter[str] = Counter() | ||
| total = 0 | ||
| t3_state_db = Path(os.environ.get("BRAINLAYER_T3_STATE_DB", DEFAULT_T3_STATE_DB)).expanduser() |
There was a problem hiding this comment.
🟠 High scripts/provenance_classify_report.py:32
build_report uses os.environ.get("BRAINLAYER_T3_STATE_DB", DEFAULT_T3_STATE_DB), so when the env var is set but empty, the value is Path('') — the current directory. That path exists, so line 33 calls t3_app_codex_session_ids on it, which fails to open a directory as SQLite and raises a fatal alarm. This diverges from is_t3_app_initiated_codex_session, which guards the same lookup with or DEFAULT_T3_STATE_DB. An empty-but-present env var therefore crashes the report instead of falling back to the default DB.
- t3_state_db = Path(os.environ.get("BRAINLAYER_T3_STATE_DB", DEFAULT_T3_STATE_DB)).expanduser()
+ t3_state_db = Path(os.environ.get("BRAINLAYER_T3_STATE_DB") or DEFAULT_T3_STATE_DB).expanduser()🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @scripts/provenance_classify_report.py around line 32:
`build_report` uses `os.environ.get("BRAINLAYER_T3_STATE_DB", DEFAULT_T3_STATE_DB)`, so when the env var is set but empty, the value is `Path('')` — the current directory. That path exists, so line 33 calls `t3_app_codex_session_ids` on it, which fails to open a directory as SQLite and raises a fatal alarm. This diverges from `is_t3_app_initiated_codex_session`, which guards the same lookup with `or DEFAULT_T3_STATE_DB`. An empty-but-present env var therefore crashes the report instead of falling back to the default DB.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fefbcb8e5
ℹ️ 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".
| except BrainLayerAlarm: | ||
| linked_t3_session_ids = set() |
There was a problem hiding this comment.
When the T3 state database is temporarily unreadable, malformed, or still being initialized, this new handler catches the alarm and substitutes an empty linkage set. Every Codex entry in that flush is then persisted as codex-session and its source offset is confirmed, so it is never retried after the T3 state recovers; continue processing non-Codex entries, but defer Codex entries whose provenance cannot be determined. The fresh evidence in this commit is the new alarm catch that converts the previously reported failure into a permanent misclassification.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
Choose recovery shape (a): when the once-per-flush T3 linkage lookup alarms, source-only provenance classification defers Codex entries without persisting or confirming their offsets, while non-Codex entries in the same flush continue normally. A later pass after runtime repair can then apply T3 precedence. This avoids adding an unresolved schema marker or widening the retag migration over legitimate provenance classes. Add a real temp-queue regression with recon-signature Codex content that forces the alarm, proves plain Claude persistence and confirmation remain live, then proves repaired linkage replays as t3-app-session.
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_965c4af6-b60a-401d-bbb8-d696e241832e) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47bb3a3787
ℹ️ 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".
| source_file, | ||
| base_content_class, | ||
| content=clean_content, | ||
| t3_linked_session_ids=linked_t3_session_ids, |
There was a problem hiding this comment.
When a T3-linked transcript contains text that exact- or near-duplicates an existing ordinary Codex chunk, this classification is discarded: the direct path passes an incoming dict without provenance_class to merge_duplicate_chunk (watcher_bridge.py:554-581), while the arbitrated path reaches _insert_or_merge_chunk, whose duplicate merge also leaves the canonical provenance unchanged (drain.py:348-377). The surviving row therefore remains codex-session with the earlier source file, and the retag migration cannot recover it because that source points to the unlinked session; either prevent cross-provenance deduplication or preserve both origins when merging.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/retag_t3_app_provenance.py`:
- Around line 98-123: Update the apply flow around the retagging operation to
require enrichment workers to be stopped before bulk updates, either by adding
the established preflight check or documenting the operational requirement.
Preserve the existing PRAGMA wal_checkpoint(FULL) calls before and after the
batch updates, and verify whether an FTS trigger on chunks causes reindexing so
the migration’s operational cost is accounted for.
- Around line 30-36: Remove the contradictory provenance_class check in the
_candidates comprehension, retaining only the condition that expresses which
records should be selected. If PROVENANCE_RANK is intended to gate replacement,
compare the rank of T3_APP_SESSION with the existing provenance_class rank
instead, ensuring future additions to PROVENANCE_RANK do not make --apply
silently return no candidates.
- Around line 217-234: Update main’s argument validation before the
rollback/apply branch so --rollback-artifact is required whenever the command
runs, including with --apply, and report the omission through parser.error
instead of allowing retag_t3_app_chunks to raise ValueError. Preserve the
existing rollback validation behavior and normal execution when the argument is
provided.
- Around line 158-170: Update the current_values lookup in the rollback flow to
query candidates in chunks rather than binding every candidate in one SELECT
statement. Use a chunk size capped at 900 (or otherwise below the lowest
supported SQLite parameter limit), execute the existing query for each chunk,
and merge all returned rows into current_values while preserving the
empty-candidates behavior.
In `@src/brainlayer/agent_provenance.py`:
- Around line 149-157: Update the watcher classification flow around
classify_provenance in watcher_bridge.py to explicitly catch BrainLayerAlarm,
which inherits from BaseException, when T3 linkage resolution performs SQLite
I/O. Ensure T3 schema or connection failures are handled by the existing watcher
error path, or preload t3_linked_session_ids before classification so those
failures cannot escape.
In `@src/brainlayer/t3_provenance.py`:
- Around line 56-62: The read-only connection setup must handle live WAL
databases without making every linkage attempt unavailable. In the T3
database-opening flow around sqlite3.connect, verify the actual journal mode and
either read a temporary database copy when WAL is used or add an immutable=1
fallback; when both approaches fail, treat the result as no linkage instead of
raising t3_runtime_unavailable via raise_alarm.
In `@src/brainlayer/watcher_bridge.py`:
- Around line 410-414: In the watcher processing flow around
t3_linkage_resolved, track deferred Codex-provenance entries separately from
ordinary skipped entries, incrementing the counter before continuing. Update
emit_watcher_flush to report the deferred count and add one per-flush log
message so repeated deferrals remain observable without logging each entry.
In `@tests/test_agent_provenance.py`:
- Around line 286-291: Update the monkeypatch in the provenance report test to
use the default raising=True behavior by removing raising=False. After
build_report, assert the expected tag distribution in report so the test
verifies the loaded session-ID set is forwarded to classify_provenance, while
retaining the existing total_chunks and calls assertions.
In `@tests/test_retag_t3_app_provenance.py`:
- Around line 224-230: Update the test around _write_rollback_artifact to patch
a module-local replace symbol rather than retag_module.os.replace, preventing
process-wide os.replace mutation; expose os.replace as a module-level imported
name in scripts.retag_t3_app_provenance and have the implementation use it. Also
remove the unused **kwargs from the wrapper near line 179, since
_atomic_write_jsonl accepts only positional parameters.
In `@tests/test_t3_app_provenance.py`:
- Around line 14-38: Update the SQLite connection usage in _state_db and the
analogous connection blocks at the referenced locations to wrap sqlite3.connect
with contextlib.closing, while retaining the transaction context manager for
commit behavior. Follow the existing closing pattern already used nearby so
every connection is explicitly closed.
🪄 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 Plus
Run ID: 530e9e92-9a32-4664-b25c-543bd5f6f9e7
📒 Files selected for processing (9)
scripts/provenance_classify_report.pyscripts/retag_t3_app_provenance.pysrc/brainlayer/agent_provenance.pysrc/brainlayer/t3_provenance.pysrc/brainlayer/watcher_bridge.pytests/conftest.pytests/test_agent_provenance.pytests/test_retag_t3_app_provenance.pytests/test_t3_app_provenance.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
🧰 Additional context used
📓 Path-based instructions (2)
src/brainlayer/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/brainlayer/**/*.py: Treat retrieval correctness, write safety, and MCP stability as critical-path concerns; prioritize regressions in search quality, lock handling, and tool contracts over style refactors.
Usepaths.py:get_db_path()for database path resolution rather than hardcoding paths; the canonical database is~/.local/share/brainlayer/brainlayer.db.
Ensure concurrent database access retriesSQLITE_BUSY, uses one connection per worker, permits one writer at a time, and does not runbrain_digestconcurrently with other MCP work.
Run secret scrubbing inwatcher_bridge.pybefore chunk persistence; redact provider-prefixed and labeled high-entropy secrets and record unlabeled high-entropy tokens in quarantine metadata.
Preserveai_code,stack_trace, anduser_messageverbatim; skip noise, summarize build logs, retain only structure for directory listings, use AST-aware tree-sitter chunking, never split stack traces, and mask large tool output.
Do not change the source denylist semantics: provider sessions and ordinary Claude subagents ingest by default; only exactbrain-workersubagents andwf_*workflow paths are excluded, without deleting source JSONL.
For bulk database operations, stop enrichment workers, runPRAGMA wal_checkpoint(FULL)before and after, drop and recreate FTS triggers around large deletes, batch deletes in 5–10K chunks, checkpoint every three batches, and never delete large datasets while the FTS delete trigger is active.
Respect chunk lifecycle fieldssuperseded_by,aggregated_into, andarchived_at; exclude lifecycle-managed chunks from default search, supportinclude_archived=True, use safety gates for personal data inbrain_supersede, and implementbrain_archiveas a timestamped soft delete.
Use Groq as the primary enrichment backend, Gemini as fallback, and Ollama as the offline last resort; honorBRAINLAYER_ENRICH_BACKENDandBRAINLAYER_ENRICH_RATE.
The real-time watcher must persist offsets, dete...
Files:
src/brainlayer/agent_provenance.pysrc/brainlayer/t3_provenance.pysrc/brainlayer/watcher_bridge.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run
pytestbefore claiming behavior changes are safe; avoid running real-production-DB teststests/test_vector_store.pyandtests/test_engine.pyas part of worker pre-push checks unless deliberately performing production-DB checks.
Files:
tests/conftest.pytests/test_agent_provenance.pytests/test_t3_app_provenance.pytests/test_retag_t3_app_provenance.py
🪛 ast-grep (0.45.0)
src/brainlayer/t3_provenance.py
[warning] 20-20: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: _CODEX_SESSION_ID_RE.findall(Path(source_file).stem)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
scripts/retag_t3_app_provenance.py
[info] 46-46: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"id": chunk_id, "provenance_class": provenance_class})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 234-234: use jsonify instead of json.dumps for JSON output
Context: json.dumps(report, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/test_t3_app_provenance.py
[info] 47-47: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"threadId": "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 62-62: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"threadId": session_id})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 74-74: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"threadId": session_id})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 90-90: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"threadId": session_id})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 119-119: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"threadId": session_id})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 253-253: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"threadId": session_id})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 271-271: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"threadId": session_id})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/test_retag_t3_app_provenance.py
[info] 23-23: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"threadId": session_id})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 104-104: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"id": "prior-null", "provenance_class": None})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 105-105: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"id": "prior-codex", "provenance_class": "codex-session"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 175-175: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"id": "app-1", "provenance_class": "codex-session"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 203-203: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"id": "deleted", "provenance_class": "codex-session"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 220-220: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"id": "old", "provenance_class": "codex-session"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (10)
src/brainlayer/t3_provenance.py (2)
19-48: LGTM!
55-97: 🩺 Stability & AvailabilityNo change needed.
raise_alarm()is annotated withtyping.NoReturn, so the surrounding control flow is provable.tests/test_t3_app_provenance.py (1)
186-265: LGTM!src/brainlayer/agent_provenance.py (1)
142-143: The line-range change details state thatsrc/brainlayer/watcher_bridge.pyandscripts/provenance_classify_report.pyreference an "undefined namet3_linked_session_ids". That is incorrect.t3_linked_session_idsis the keyword parameter declared here at line 143. Both call sites pass it as a keyword argument and assign the locally definedlinked_t3_session_idsto it. No name error exists.src/brainlayer/watcher_bridge.py (2)
353-359: LGTM!
515-520: LGTM!scripts/provenance_classify_report.py (1)
8-8: LGTM!Also applies to: 22-22, 32-43
tests/conftest.py (1)
90-93: LGTM!scripts/retag_t3_app_provenance.py (2)
39-73: LGTM!
113-124: 🗄️ Data Integrity & IntegrationNo change needed.
This script targets Python 3.11, where
sqlite3.Cursor.rowcountafterexecutemanyreports the total affected rows for these UPDATE operations.> Likely an incorrect or invalid review comment.
| return [ | ||
| (chunk_id, provenance_class) | ||
| for chunk_id, source_file, provenance_class in rows | ||
| if codex_session_id_from_source(source_file) in linked_session_ids | ||
| and provenance_class == "codex-session" | ||
| and provenance_class not in PROVENANCE_RANK | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the contradictory candidate condition.
Lines 34 and 35 test the same value against two conditions that can both hold only when "codex-session" is absent from PROVENANCE_RANK. Today it is absent, so line 35 is always true and dead. If a future change adds "codex-session" to PROVENANCE_RANK, _candidates returns an empty list, and --apply becomes a silent no-op that reports success.
Keep one condition and state the intent explicitly.
♻️ Proposed change
return [
(chunk_id, provenance_class)
for chunk_id, source_file, provenance_class in rows
- if codex_session_id_from_source(source_file) in linked_session_ids
- and provenance_class == "codex-session"
- and provenance_class not in PROVENANCE_RANK
+ # Only the neutral "codex-session" tag is safe to replace; any ranked or
+ # manually assigned provenance is left untouched.
+ if provenance_class == "codex-session"
+ and codex_session_id_from_source(source_file) in linked_session_ids
]If PROVENANCE_RANK is meant to gate the replacement instead, compare the rank of T3_APP_SESSION against the rank of the existing value.
📝 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.
| return [ | |
| (chunk_id, provenance_class) | |
| for chunk_id, source_file, provenance_class in rows | |
| if codex_session_id_from_source(source_file) in linked_session_ids | |
| and provenance_class == "codex-session" | |
| and provenance_class not in PROVENANCE_RANK | |
| ] | |
| return [ | |
| (chunk_id, provenance_class) | |
| for chunk_id, source_file, provenance_class in rows | |
| # Only the neutral "codex-session" tag is safe to replace; any ranked or | |
| # manually assigned provenance is left untouched. | |
| if provenance_class == "codex-session" | |
| and codex_session_id_from_source(source_file) in linked_session_ids | |
| ] |
🤖 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/retag_t3_app_provenance.py` around lines 30 - 36, Remove the
contradictory provenance_class check in the _candidates comprehension, retaining
only the condition that expresses which records should be selected. If
PROVENANCE_RANK is intended to gate replacement, compare the rank of
T3_APP_SESSION with the existing provenance_class rank instead, ensuring future
additions to PROVENANCE_RANK do not make --apply silently return no candidates.
| if apply: | ||
| connection.execute("PRAGMA busy_timeout = 30000") | ||
| candidates = _candidates(connection, linked_session_ids) | ||
| report = { | ||
| "linked_sessions": len(linked_session_ids), | ||
| "candidate_chunks": len(candidates), | ||
| "matched_chunks": 0, | ||
| "retagged_chunks": 0, | ||
| } | ||
| if not apply: | ||
| return report | ||
|
|
||
| artifact_path = Path(rollback_artifact).expanduser() | ||
| _write_rollback_artifact(artifact_path, candidates) | ||
| connection.execute("PRAGMA wal_checkpoint(FULL)") | ||
| for batch_start in range(0, len(candidates), batch_size): | ||
| batch = candidates[batch_start : batch_start + batch_size] | ||
| cursor = connection.executemany( | ||
| "UPDATE chunks SET provenance_class = ? WHERE id = ? AND provenance_class = ?", | ||
| [(T3_APP_SESSION, chunk_id, "codex-session") for chunk_id, _ in batch], | ||
| ) | ||
| report["matched_chunks"] += cursor.rowcount | ||
| connection.commit() | ||
| if ((batch_start // batch_size) + 1) % 3 == 0: | ||
| connection.execute("PRAGMA wal_checkpoint(FULL)") | ||
| connection.execute("PRAGMA wal_checkpoint(FULL)") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Operational note: enrichment workers and FTS triggers during the retag.
The function updates chunks.provenance_class in batches and checkpoints the WAL. It does not stop enrichment workers first. The PR objectives state that chunks.provenance_class has multiple writers and no precedence rule, so an enrichment pass that runs during or after the retag can overwrite the T3 tag. The guarded UPDATE ... AND provenance_class = 'codex-session' protects the write itself, but it does not protect the result afterwards.
Two recommendations:
- Document that operators must stop enrichment workers before
--applyand restart them after, or add a preflight check that fails when workers are running. - Confirm whether an FTS update trigger exists on
chunks. If it does, each batch reindexes every touched row, which changes the cost profile of the migration.
Do you want me to open an issue to track a precedence rule for chunks.provenance_class writers?
As per coding guidelines: "For bulk database operations, stop enrichment workers, run PRAGMA wal_checkpoint(FULL) before and after".
🤖 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/retag_t3_app_provenance.py` around lines 98 - 123, Update the apply
flow around the retagging operation to require enrichment workers to be stopped
before bulk updates, either by adding the established preflight check or
documenting the operational requirement. Preserve the existing PRAGMA
wal_checkpoint(FULL) calls before and after the batch updates, and verify
whether an FTS trigger on chunks causes reindexing so the migration’s
operational cost is accounted for.
Source: Coding guidelines
| current_values = ( | ||
| { | ||
| chunk_id: provenance_class | ||
| for chunk_id, provenance_class in connection.execute( | ||
| "SELECT id, provenance_class FROM chunks WHERE id IN ({})".format( | ||
| ", ".join("?" for _ in candidates) | ||
| ), | ||
| [chunk_id for chunk_id, _ in candidates], | ||
| ) | ||
| } | ||
| if candidates | ||
| else {} | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
The rollback lookup exceeds the SQLite parameter limit.
Lines 162-165 create one ? placeholder for every candidate in the artifact. SQLite limits bound parameters per statement through SQLITE_MAX_VARIABLE_NUMBER: 999 before SQLite 3.32 and 32766 from 3.32. A rollback artifact produced by a real retag can hold more rows than that. The statement then raises sqlite3.OperationalError: too many SQL variables, and the entire rollback fails before any row is restored. batch_size does not apply to this lookup.
Read the current values in chunks of batch_size.
🐛 Proposed fix
- current_values = (
- {
- chunk_id: provenance_class
- for chunk_id, provenance_class in connection.execute(
- "SELECT id, provenance_class FROM chunks WHERE id IN ({})".format(
- ", ".join("?" for _ in candidates)
- ),
- [chunk_id for chunk_id, _ in candidates],
- )
- }
- if candidates
- else {}
- )
+ current_values: dict[str, str | None] = {}
+ candidate_ids = [chunk_id for chunk_id, _ in candidates]
+ for lookup_start in range(0, len(candidate_ids), batch_size):
+ lookup_ids = candidate_ids[lookup_start : lookup_start + batch_size]
+ placeholders = ", ".join("?" for _ in lookup_ids)
+ current_values.update(
+ dict(
+ connection.execute(
+ f"SELECT id, provenance_class FROM chunks WHERE id IN ({placeholders})",
+ lookup_ids,
+ )
+ )
+ )Note that batch_size defaults to 5000, which still exceeds the 999 limit on SQLite builds before 3.32. Cap the lookup chunk at 900 if the target SQLite version is not guaranteed.
🤖 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/retag_t3_app_provenance.py` around lines 158 - 170, Update the
current_values lookup in the rollback flow to query candidates in chunks rather
than binding every candidate in one SELECT statement. Use a chunk size capped at
900 (or otherwise below the lowest supported SQLite parameter limit), execute
the existing query for each chunk, and merge all returned rows into
current_values while preserving the empty-candidates behavior.
| if args.rollback: | ||
| if args.rollback_artifact is None: | ||
| parser.error("--rollback-artifact is required with --rollback") | ||
| report: dict[str, Any] = rollback_t3_app_chunks( | ||
| db_path=args.db_path, | ||
| rollback_artifact=args.rollback_artifact, | ||
| only_null_prior_values=args.only_null_prior_values, | ||
| pre_restore_artifact=args.pre_restore_artifact, | ||
| batch_size=args.batch_size, | ||
| ) | ||
| else: | ||
| report = retag_t3_app_chunks( | ||
| db_path=args.db_path, | ||
| state_db=args.state_db, | ||
| apply=args.apply, | ||
| rollback_artifact=args.rollback_artifact, | ||
| batch_size=args.batch_size, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Report the missing --rollback-artifact as a usage error.
Line 219 uses parser.error for the rollback path. The apply path does not. If a user runs --apply without --rollback-artifact, retag_t3_app_chunks raises ValueError at line 88 and the user sees a traceback. Validate the argument in main so both paths behave the same.
♻️ Proposed change
else:
+ if args.apply and args.rollback_artifact is None:
+ parser.error("--rollback-artifact is required with --apply")
report = retag_t3_app_chunks(📝 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.
| if args.rollback: | |
| if args.rollback_artifact is None: | |
| parser.error("--rollback-artifact is required with --rollback") | |
| report: dict[str, Any] = rollback_t3_app_chunks( | |
| db_path=args.db_path, | |
| rollback_artifact=args.rollback_artifact, | |
| only_null_prior_values=args.only_null_prior_values, | |
| pre_restore_artifact=args.pre_restore_artifact, | |
| batch_size=args.batch_size, | |
| ) | |
| else: | |
| report = retag_t3_app_chunks( | |
| db_path=args.db_path, | |
| state_db=args.state_db, | |
| apply=args.apply, | |
| rollback_artifact=args.rollback_artifact, | |
| batch_size=args.batch_size, | |
| ) | |
| if args.rollback: | |
| if args.rollback_artifact is None: | |
| parser.error("--rollback-artifact is required with --rollback") | |
| report: dict[str, Any] = rollback_t3_app_chunks( | |
| db_path=args.db_path, | |
| rollback_artifact=args.rollback_artifact, | |
| only_null_prior_values=args.only_null_prior_values, | |
| pre_restore_artifact=args.pre_restore_artifact, | |
| batch_size=args.batch_size, | |
| ) | |
| else: | |
| if args.apply and args.rollback_artifact is None: | |
| parser.error("--rollback-artifact is required with --apply") | |
| report = retag_t3_app_chunks( | |
| db_path=args.db_path, | |
| state_db=args.state_db, | |
| apply=args.apply, | |
| rollback_artifact=args.rollback_artifact, | |
| batch_size=args.batch_size, | |
| ) |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 234-234: use jsonify instead of json.dumps for JSON output
Context: json.dumps(report, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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/retag_t3_app_provenance.py` around lines 217 - 234, Update main’s
argument validation before the rollback/apply branch so --rollback-artifact is
required whenever the command runs, including with --apply, and report the
omission through parser.error instead of allowing retag_t3_app_chunks to raise
ValueError. Preserve the existing rollback validation behavior and normal
execution when the argument is provided.
| is_t3_app_session = _under_provider_sessions(path, ".codex") and ( | ||
| is_t3_app_initiated_codex_session( | ||
| source_file, | ||
| state_db=t3_state_db, | ||
| linked_session_ids=t3_linked_session_ids, | ||
| ) | ||
| ) | ||
| if is_t3_app_session: | ||
| return ProvenanceDecision(T3_APP_SESSION, "KEEP", "T3 runtime cursor links Codex session") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List every classify_provenance call site and show whether T3 arguments are passed.
rg -nP -C 6 '\bclassify_provenance\s*\(' --type=py -g '!tests/**'
# Show which of those modules handle BrainLayerAlarm.
rg -nP -C 3 'except\s+BrainLayerAlarm' --type=pyRepository: EtanHey/brainlayer
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'tracked python files around agent_provenance:\n'
git ls-files 'src/brainlayer/**/*.py' | sed -n '1,200p'
printf '\nSearch classify_provenance (all tracked files):\n'
rg -n -C 4 'classify_provenance\s*\(' . --glob '*.py' | sed -n '1,220p' || true
printf '\nSearch BrainLayerAlarm handling (all tracked files):\n'
rg -n -C 4 'except\s+BrainLayerAlarm|--\|--\|.*BrainLayerAlarm|--' . --glob '*.py' | sed -n '1,240p' || trueRepository: EtanHey/brainlayer
Length of output: 32820
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'agent_provenance outline:\n'
ast-grep outline src/brainlayer/agent_provenance.py --view expanded | sed -n '1,240p' || true
printf '\nagent_provenance lines 1-240:\n'
cat -n src/brainlayer/agent_provenance.py | sed -n '1,240p'
printf '\nWatcher relevant lines 360-540:\n'
cat -n src/brainlayer/watcher_bridge.py | sed -n '360,540p'Repository: EtanHey/brainlayer
Length of output: 19383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Scripts provenance call context:\n'
cat -n scripts/provenance_classify_report.py | sed -n '1,90p'
printf '\nNon-test classify_provenance calls outside tests/scripts:\n'
rg -n -C 3 'classify_provenance\s*\(' src scripts --glob '*.py' | rg -v 'tests' | sed -n '1,220p' || true
printf '\nSearch BrainLayerAlarm definitions/usages:\n'
rg -n -C 4 'class BrainLayerAlarm|BrainLayerAlarm' src scripts tests --glob '*.py' | sed -n '1,300p' || trueRepository: EtanHey/brainlayer
Length of output: 24477
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 't3_provenance relevant lines:\n'
fd -a -i '^t3_provenance.py$' . | while read -r f; do
echo "----- ${f#"$PWD/"} -----"
wc -l "$f"
cat -n "$f" | sed -n '1,220p'
done
printf '\nInspector for BrainLayerAlarm inheritance:\n'
python3 - <<'PY'
import ast
from pathlib import Path
for p in Path("src/brainlayer").rglob("alarm.py"):
print(p)
tree = ast.parse(p.read_text())
for node in ast.walk(tree):
if isinstance(node, ast.ClassDef) and node.name == "BrainLayerAlarm":
bases = [ast.unparse(n) for n in node.bases]
print("bases:", bases)
for subnode in ast.walk(node):
if isinstance(subnode, ast.ClassDef):
print("nested:", subnode.name, ast.unparse(subnode.bases))
PYRepository: EtanHey/brainlayer
Length of output: 5007
Handle BrainLayerAlarm around classify_provenance in watcher_bridge.py.
The watcher caller at src/brainlayer/watcher_bridge.py:410-411 now runs SQLite I/O when t3_linked_session_ids is missing from env resolution. Since BrainLayerAlarm inherits from BaseException, an existing except Exception block around watcher classification/can not catch T3 schema or connection failures. Catch it explicitly or ensure this path preloads T3 linkage IDs.
🤖 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/brainlayer/agent_provenance.py` around lines 149 - 157, Update the
watcher classification flow around classify_provenance in watcher_bridge.py to
explicitly catch BrainLayerAlarm, which inherits from BaseException, when T3
linkage resolution performs SQLite I/O. Ensure T3 schema or connection failures
are handled by the existing watcher error path, or preload t3_linked_session_ids
before classification so those failures cannot escape.
Source: Coding guidelines
| connection = sqlite3.connect(f"{path.absolute().as_uri()}?mode=ro&immutable=0", uri=True, timeout=1.0) | ||
| except sqlite3.Error as exc: | ||
| raise_alarm( | ||
| "t3_runtime_unavailable", | ||
| "could not open T3 runtime state read-only", | ||
| {"path": str(path), "error": str(exc)}, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Read-only opening of a live WAL database can fail.
The T3 state database belongs to another application. If that application uses WAL journaling, SQLite needs write access to the -shm and -wal files to attach a reader, and needs write access to recover a hot journal. With mode=ro, SQLite then returns SQLITE_READONLY or SQLITE_CANTOPEN. This raises t3_runtime_unavailable, and flush_to_db in src/brainlayer/watcher_bridge.py then defers every Codex entry for as long as the condition persists.
Verify the journal mode of the real T3 database. If it is WAL, copy the database to a temporary path before reading, or open with mode=ro plus a fallback to immutable=1 and treat a failure as "no linkage" rather than an alarm.
🤖 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/brainlayer/t3_provenance.py` around lines 56 - 62, The read-only
connection setup must handle live WAL databases without making every linkage
attempt unavailable. In the T3 database-opening flow around sqlite3.connect,
verify the actual journal mode and either read a temporary database copy when
WAL is used or add an immutable=1 fallback; when both approaches fail, treat the
result as no linkage instead of raising t3_runtime_unavailable via raise_alarm.
| if not t3_linkage_resolved: | ||
| source_only_provenance = classify_provenance(source_file, t3_linked_session_ids=set()) | ||
| if source_only_provenance.provenance_tag == "codex-session": | ||
| # Do not persist or confirm ambiguous Codex provenance; a later pass can replay this offset. | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Record the deferral. A silent stop to Codex ingestion is not observable.
The branch defers Codex entries without any counter or log. skipped stays unchanged, and emit_watcher_flush at line 722 reports only chunks_indexed and chunks_skipped. If the T3 state database stays unusable, Codex ingestion stops indefinitely and every flush still looks healthy.
Count the deferred entries and log once per flush.
🔭 Proposed change
t3_linkage_resolved = True
try:
linked_t3_session_ids = t3_app_codex_session_ids(t3_state_db) if t3_state_db.exists() else set()
except BrainLayerAlarm:
linked_t3_session_ids = set()
t3_linkage_resolved = False
+ deferred_codex_entries = 0 if not t3_linkage_resolved:
source_only_provenance = classify_provenance(source_file, t3_linked_session_ids=set())
if source_only_provenance.provenance_tag == "codex-session":
# Do not persist or confirm ambiguous Codex provenance; a later pass can replay this offset.
+ deferred_codex_entries += 1
continue+ if deferred_codex_entries:
+ logger.warning(
+ "T3 linkage unresolved at %s; deferred %d Codex entries for replay",
+ t3_state_db,
+ deferred_codex_entries,
+ )🤖 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/brainlayer/watcher_bridge.py` around lines 410 - 414, In the watcher
processing flow around t3_linkage_resolved, track deferred Codex-provenance
entries separately from ordinary skipped entries, incrementing the counter
before continuing. Update emit_watcher_flush to report the deferred count and
add one per-flush log message so repeated deferrals remain observable without
logging each entry.
| monkeypatch.setattr(provenance_classify_report, "t3_app_codex_session_ids", load_once, raising=False) | ||
|
|
||
| report = provenance_classify_report.build_report(db_path) | ||
|
|
||
| assert report["total_chunks"] == 3 | ||
| assert calls == [state_db] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Drop raising=False and assert the resulting tags.
scripts/provenance_classify_report.py imports t3_app_codex_session_ids at module level, so the attribute always exists. raising=False makes the patch silently do nothing if that import is renamed or removed, and the test would still pass. Use the default raising=True.
Also assert the tag distribution. The current assertions pass even if build_report stops forwarding the loaded set to classify_provenance.
💚 Proposed change
- monkeypatch.setattr(provenance_classify_report, "t3_app_codex_session_ids", load_once, raising=False)
+ monkeypatch.setattr(provenance_classify_report, "t3_app_codex_session_ids", load_once)
report = provenance_classify_report.build_report(db_path)
assert report["total_chunks"] == 3
+ assert report["tags"] == {"codex-session": 3}
assert calls == [state_db]📝 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.
| monkeypatch.setattr(provenance_classify_report, "t3_app_codex_session_ids", load_once, raising=False) | |
| report = provenance_classify_report.build_report(db_path) | |
| assert report["total_chunks"] == 3 | |
| assert calls == [state_db] | |
| monkeypatch.setattr(provenance_classify_report, "t3_app_codex_session_ids", load_once) | |
| report = provenance_classify_report.build_report(db_path) | |
| assert report["total_chunks"] == 3 | |
| assert report["tags"] == {"codex-session": 3} | |
| assert calls == [state_db] |
🤖 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_agent_provenance.py` around lines 286 - 291, Update the
monkeypatch in the provenance report test to use the default raising=True
behavior by removing raising=False. After build_report, assert the expected tag
distribution in report so the test verifies the loaded session-ID set is
forwarded to classify_provenance, while retaining the existing total_chunks and
calls assertions.
| def fail_replace(source: str, destination: str) -> None: | ||
| raise OSError("simulated replace failure") | ||
|
|
||
| monkeypatch.setattr(retag_module.os, "replace", fail_replace) | ||
|
|
||
| with pytest.raises(OSError, match="simulated replace failure"): | ||
| retag_module._write_rollback_artifact(artifact, [("new", "codex-session")]) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The patch replaces os.replace process-wide.
retag_module.os is the global os module object, not a module-local alias. Line 227 therefore replaces os.replace for the whole interpreter while the test runs. Any other code that calls os.replace during the test fails with the simulated error.
Patch the function inside scripts.retag_t3_app_provenance instead, so the scope matches the intent.
💚 Proposed change
- monkeypatch.setattr(retag_module.os, "replace", fail_replace)
+ monkeypatch.setattr("scripts.retag_t3_app_provenance.os.replace", fail_replace)That target resolves to the same global attribute. To scope the failure to this module only, import replace into scripts/retag_t3_app_provenance.py as a module-level name and patch that name.
Also drop the unused **kwargs from the wrapper at line 179. _atomic_write_jsonl declares only positional parameters.
🤖 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_retag_t3_app_provenance.py` around lines 224 - 230, Update the
test around _write_rollback_artifact to patch a module-local replace symbol
rather than retag_module.os.replace, preventing process-wide os.replace
mutation; expose os.replace as a module-level imported name in
scripts.retag_t3_app_provenance and have the implementation use it. Also remove
the unused **kwargs from the wrapper near line 179, since _atomic_write_jsonl
accepts only positional parameters.
Choose durability shape (a): the flush result now names the exact Codex entries deferred when T3 linkage is unavailable, and BatchIndexer retains only those payloads while confirming unrelated plain Claude work. Confirmations for a source with a deferred entry are withheld so a later record cannot advance the registry across the gap. This keeps the fix inside the existing watcher/indexer contract without adding an unresolved queue schema or widening recovery migrations.
Failure isolation must honor the same deferred-entry contract as normal flushes. Retain deferred payloads, suppress their watermarks, and count only actual callback exceptions as failed inputs so mixed exception/deferral batches cannot drop a consumed Codex record.
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_cd869dd6-9d58-4900-8acf-d2037cca4f6e) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
| self._buffer = retained | ||
| self._flush_failures = self._flush_failures + 1 if retained else 0 | ||
| return len(retained) | ||
| self._flush_failures = self._flush_failures + 1 if failed_inputs else 0 |
There was a problem hiding this comment.
🟡 Medium brainlayer/watcher.py:991
In _isolate_failed_flush, deferred entries are appended to retained alongside genuinely failing items, and _flush_failures is incremented whenever failed_inputs is nonzero. The quarantine path that drops the buffer after repeated failures only triggers when len(batch) <= 1, but with a deferred entry plus a failing entry the batch size stays at 2, so the failing entry is retried forever and never quarantined. The deferred entry also gets reprocessed on every retry, and new input keeps accumulating in the buffer.
The retain-limit/quarantine check should also apply to multi-item batches where only a subset fails, so persistent failures are quarantined instead of looping indefinitely. Alternatively, exclude deferred entries from the failure count so they don't keep the batch size above the quarantine threshold.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/watcher.py around line 991:
In `_isolate_failed_flush`, deferred entries are appended to `retained` alongside genuinely failing items, and `_flush_failures` is incremented whenever `failed_inputs` is nonzero. The quarantine path that drops the buffer after repeated failures only triggers when `len(batch) <= 1`, but with a deferred entry plus a failing entry the batch size stays at 2, so the failing entry is retried forever and never quarantined. The deferred entry also gets reprocessed on every retry, and new input keeps accumulating in the buffer.
The retain-limit/quarantine check should also apply to multi-item batches where only a subset fails, so persistent failures are quarantined instead of looping indefinitely. Alternatively, exclude deferred entries from the failure count so they don't keep the batch size above the quarantine threshold.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdd0a74270
ℹ️ 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".
| source_file, | ||
| base_content_class, | ||
| content=clean_content, | ||
| t3_linked_session_ids=linked_t3_session_ids, |
There was a problem hiding this comment.
When a T3-linked watcher chunk later completes normal enrichment, this classification is lost: the watcher stores t3-app-session in chunks.provenance_class, but _apply_enrichment in enrichment_controller.py:1547-1552 unconditionally derives a trust-lattice value such as RAW-ETAN-DIRECT or AGENT-PARAPHRASE and overwrites that same column. The migration's retagged rows are erased by the same path, so the new distinction survives only until the next enrichment; preserve the agent-origin tag in a separate field or explicitly retain it during enrichment.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/brainlayer/watcher_bridge.py (1)
356-371: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRepeated T3 database reads during failure isolation.
t3_app_codex_session_idsopens a new SQLite connection every timeflush_to_dbruns.BatchIndexer._isolate_failed_flushinwatcher.pycallsself.on_flush([item])once per item after a batch flush fails, so a failed N-item batch triggers N fresh T3 database opens instead of one. This reintroduces the per-chunk T3 database scan pattern the earlier fix removed, just scoped to the failure-isolation path instead of the normal batch path.Cache the linkage result (with a short TTL, or compute it once per
_isolate_failed_flushcall and pass it through) so isolation retries do not re-open the T3 database per item.🤖 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/brainlayer/watcher_bridge.py` around lines 356 - 371, Update the failure-isolation flow around BatchIndexer._isolate_failed_flush and flush_to_db so all per-item retries reuse one T3 linkage lookup instead of calling t3_app_codex_session_ids for each item. Compute the linkage result once per isolation call and pass it through, or introduce a short-lived cache with equivalent scope, while preserving the existing BrainLayerAlarm unresolved-linkage behavior.src/brainlayer/watcher.py (1)
884-897: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap entries deferred across repeated flush cycles.
_do_flush()sets_buffer = deferred_entriesand resets_flush_failures, so entries deferred for unresolved T3 linkage are not covered by the retry-limit/quarantine path. Add an age- or count-based limit for retained deferred entries, and quarantine them when the limit is reached.🤖 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/brainlayer/watcher.py` around lines 884 - 897, Update _do_flush so entries retained in deferred_entries across repeated flush cycles are tracked by age or retry count rather than resetting their retry state via _flush_failures. When the configured limit is reached, quarantine those unresolved entries using the existing quarantine mechanism, while preserving normal retention for entries still below the limit and resetting only successful flush state.
🤖 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.
Outside diff comments:
In `@src/brainlayer/watcher_bridge.py`:
- Around line 356-371: Update the failure-isolation flow around
BatchIndexer._isolate_failed_flush and flush_to_db so all per-item retries reuse
one T3 linkage lookup instead of calling t3_app_codex_session_ids for each item.
Compute the linkage result once per isolation call and pass it through, or
introduce a short-lived cache with equivalent scope, while preserving the
existing BrainLayerAlarm unresolved-linkage behavior.
In `@src/brainlayer/watcher.py`:
- Around line 884-897: Update _do_flush so entries retained in deferred_entries
across repeated flush cycles are tracked by age or retry count rather than
resetting their retry state via _flush_failures. When the configured limit is
reached, quarantine those unresolved entries using the existing quarantine
mechanism, while preserving normal retention for entries still below the limit
and resetting only successful flush state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e3fb9fa6-0271-44c4-83b8-b21eafa5b3b3
📒 Files selected for processing (4)
src/brainlayer/watcher.pysrc/brainlayer/watcher_bridge.pytests/test_jsonl_watcher.pytests/test_t3_app_provenance.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: Macroscope - Correctness Check
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run
pytestbefore claiming behavior changes are safe; avoid running real-production-DB teststests/test_vector_store.pyandtests/test_engine.pyas part of worker pre-push checks unless deliberately performing production-DB checks.
Files:
tests/test_jsonl_watcher.pytests/test_t3_app_provenance.py
src/brainlayer/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/brainlayer/**/*.py: Treat retrieval correctness, write safety, and MCP stability as critical-path concerns; prioritize regressions in search quality, lock handling, and tool contracts over style refactors.
Usepaths.py:get_db_path()for database path resolution rather than hardcoding paths; the canonical database is~/.local/share/brainlayer/brainlayer.db.
Ensure concurrent database access retriesSQLITE_BUSY, uses one connection per worker, permits one writer at a time, and does not runbrain_digestconcurrently with other MCP work.
Run secret scrubbing inwatcher_bridge.pybefore chunk persistence; redact provider-prefixed and labeled high-entropy secrets and record unlabeled high-entropy tokens in quarantine metadata.
Preserveai_code,stack_trace, anduser_messageverbatim; skip noise, summarize build logs, retain only structure for directory listings, use AST-aware tree-sitter chunking, never split stack traces, and mask large tool output.
Do not change the source denylist semantics: provider sessions and ordinary Claude subagents ingest by default; only exactbrain-workersubagents andwf_*workflow paths are excluded, without deleting source JSONL.
For bulk database operations, stop enrichment workers, runPRAGMA wal_checkpoint(FULL)before and after, drop and recreate FTS triggers around large deletes, batch deletes in 5–10K chunks, checkpoint every three batches, and never delete large datasets while the FTS delete trigger is active.
Respect chunk lifecycle fieldssuperseded_by,aggregated_into, andarchived_at; exclude lifecycle-managed chunks from default search, supportinclude_archived=True, use safety gates for personal data inbrain_supersede, and implementbrain_archiveas a timestamped soft delete.
Use Groq as the primary enrichment backend, Gemini as fallback, and Ollama as the offline last resort; honorBRAINLAYER_ENRICH_BACKENDandBRAINLAYER_ENRICH_RATE.
The real-time watcher must persist offsets, dete...
Files:
src/brainlayer/watcher_bridge.pysrc/brainlayer/watcher.py
🪛 ast-grep (0.45.0)
tests/test_jsonl_watcher.py
[info] 756-756: Do not hardcode temporary file or directory names
Context: "/tmp/codex.jsonl"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 759-759: Do not hardcode temporary file or directory names
Context: "/tmp/other.jsonl"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
tests/test_t3_app_provenance.py
[info] 291-291: use jsonify instead of json.dumps for JSON output
Context: json.dumps(entry)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 295-295: use jsonify instead of json.dumps for JSON output
Context: json.dumps(entry)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 308-316: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "user",
"message": {
"content": [{"type": "text", "text": "Keep ordinary Claude ingestion available."}]
},
"timestamp": "2026-08-01T12:00:00Z",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 373-373: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"threadId": session_id})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (7)
src/brainlayer/watcher_bridge.py (3)
719-739: Log the deferred-entry count per flush.
deferred_entriesis now tracked as a list, but neither the log statement at Line 721-727 noremit_watcher_flushat Line 732-737 reports its size. If the T3 state database stays unusable, Codex ingestion is deferred indefinitely with no observable signal; every flush still looks healthy.Add the deferred count to the log line and to
emit_watcher_flush.🔭 Proposed change
- if inserted > 0: + if inserted > 0 or deferred_entries: logger.info( - "Flushed %d chunks (%d skipped) in %.1fms", + "Flushed %d chunks (%d skipped, %d deferred) in %.1fms", inserted, skipped, + len(deferred_entries), latency_ms, )emit_watcher_flush( chunks_indexed=inserted, chunks_skipped=skipped, + chunks_deferred=len(deferred_entries), latency_ms=latency_ms, source_files=list(source_files_seen), )
77-90: LGTM!Also applies to: 361-371, 741-746
419-424: 🎯 Functional CorrectnessNo change needed for the
classify_provenancekeyword argument.
classify_provenanceacceptst3_linked_session_ids, so this call does not introduce aTypeError.> Likely an incorrect or invalid review comment.tests/test_t3_app_provenance.py (1)
11-11: LGTM!Also applies to: 269-390
src/brainlayer/watcher.py (2)
908-935: LGTM!Also applies to: 967-992
884-897: 🗄️ Data Integrity & IntegrationNo change needed for
on_confirm_batchitem filtering.
JSONLWatcher._advance_confirmed_batchonly reads item offsets to compute the highest eligible offset per file; deferred entries are not registered inwatermarks, so their offsets are not exposed to this path as confirmed progress.tests/test_jsonl_watcher.py (1)
754-786: LGTM!
The 7 admin merges of 2026-08-04 (#645 #644 #643 #641 #633 #605 #602) bypassed CI, landing import-order and formatting drift on main. Every PR's lint job inherits it. Rebased onto #647 (949334a), which cleared the watchdog test blocking this push. ruff check --fix + ruff format only; no behavioural change.
1 participant
What broke and how it was found
T3-app-initiated Codex transcripts were indistinguishable from ordinary terminal Codex sessions: provenance_class LIKE %t3% returned 0. The explicit discriminator is provider_session_runtime.resume_cursor_json.threadId in the read-only T3 state DB.
Evidence
Review
This branch was pair-reviewed by a separate agent under the taste gate: smallest correct solution, behavioural tests, and no speculative machinery.
Provenance-column warning
chunks.provenance_class currently serves both as a trust ladder and a source class, with independent writers and no precedence rule. This PR is correct, but enrichment can overwrite its source tag until that collision is resolved. See design collision.
Enrichment and drain are currently disabled with launchctl to stop that decay; re-enable deliberately only after the column question is settled.
Note
Medium Risk
Touches realtime ingestion and offset confirmation for Codex when T3 state is partial or alarming; mis-linkage or prolonged deferral could delay Codex indexing until T3 DB is healthy.
Overview
Adds a T3 runtime linkage path so Codex transcripts started from the T3 Code app are tagged
t3-app-sessioninstead of genericcodex-session, using read-only queries ofprovider_session_runtime.resume_cursor_json.threadIdin the T3 state DB (with alarms on missing/unreadable schema).classify_provenancechecks that linkage before recon signatures and ordinary Codex rules. The watcher flush loads linked session IDs once per batch; if linkage cannot be resolved, ambiguous Codex lines are deferred (not persisted, offsets not advanced) while other sources still ingest.BatchIndexerkeeps deferred entries in the buffer and excludes them from confirmed watermarks.A new
retag_t3_app_provenancescript dry-runs or applies batched updates for existing rows, with JSONL rollback and safe restore. The provenance classify report and tests are updated; unit tests isolateBRAINLAYER_T3_STATE_DB.Reviewed by Cursor Bugbot for commit cdd0a74. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Distinguish T3 app-initiated Codex sessions with a
t3-app-sessionprovenance classprovider_session_runtimetable in a SQLite state DB.classify_provenancein agent_provenance.py to tag T3-linked Codex sessions ast3-app-sessionwithKEEPpolicy, taking precedence over recon-agent signature rules.t3-app-sessionwith batched updates, rollback artifact generation, and a separate rollback command.Macroscope summarized cdd0a74.
Summary by CodeRabbit
New Features
Bug Fixes
Tests