fix: profile, delegation and storage reds from the 2026-09-05 corpus - #4692
Merged
Conversation
session_profiles carries no token columns; session_model_usage is the sole authority and the reader overlays it. _profile_totals now composes the production read path instead of selecting deleted columns. wal_checkpoint opens through open_daemon_connection, and the batch drain prelude reads the connection for stale-session cleanup and an FTS staleness verdict; both monkeypatch targets follow. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
…erage ANALYZE writes no sqlite_stat1 row for a table with no rows, so planner-stats warned on every archive whose action_pairs is empty. Demand coverage only for populated tables. The per-table coverage test targeted session_links, which the covered-table list has never contained. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
Resolution joins a child to its dispatch through session_links.parent_tool_use_block_id, which the writer sets only from a claude_delegation_progress event on the parent. Fixtures that predate that contract resolved to unresolved/edge_only. The provider fixture carries the evidence as a real progress record so the Claude Code parser still produces it; the materializer fixture, which inserts session_links directly, sets the block id directly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
get_session_profile_record indexes row["input_row_count"] unconditionally, but the profile SELECT never projected the column, so every read raised IndexError and the daemon session-insights route answered 500. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
threads, delegations and actions are query-time views. Testing presence with a table-only lookup reported them absent, which marks the insight diverged and withholds every row from an export as "insight table is absent". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
schema_identity stamps the DDL's own hash and is not comparable; session_identity_claims is ordinary derived data. The status fixture needs session_work_events to exist before threads counts as readable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
Embedding finalization moved to finalize_embedding_attempt_success, and content-addressed vectors are reused across roots, so identical content in a second session makes no second provider call. source_name is a declared member of the ParsedSession semantic partition. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
An attachment whose owning message cannot be resolved was skipped before its own row was written, so identity and bytes were lost rather than kept without a guessed ref. Write the row and keep it out of the ref-count sweep, which collects rows whose refs went away. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
Sinity
enabled auto-merge (squash)
September 5, 2026 15:36
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: c0b77901a6
ℹ️ 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".
| # attachment's identity and bytes are still evidence. The row is | ||
| # written unreferenced and kept out of the ref-count sweep, which | ||
| # exists to collect rows whose refs went away. | ||
| _write_attachment_row(conn, attachment_id, attachment, preacquired_blobs) |
There was a problem hiding this comment.
When an ambiguous-owner ParsedAttachment carries inline_bytes, ArchiveStore preacquires it with acquisition_status='acquired', but this call persists it without any attachment_refs row. _check_attachment_coverage explicitly treats every acquired attachment without a reference as unreachable and returns a blocking ERROR, so verify_archive(..., checks=('attachment-coverage',)) rejects an archive immediately after otherwise successful ingestion. Represent the unowned disposition through a relation/state recognized by readers and verification, or avoid creating an acquired row until it can be referenced.
AGENTS.md reference: AGENTS.md:L175-L179
Useful? React with 👍 / 👎.
Comment on lines
+3772
to
+3776
| # The owner is ambiguous, so no ref may be guessed, but the | ||
| # attachment's identity and bytes are still evidence. The row is | ||
| # written unreferenced and kept out of the ref-count sweep, which | ||
| # exists to collect rows whose refs went away. | ||
| _write_attachment_row(conn, attachment_id, attachment, preacquired_blobs) |
There was a problem hiding this comment.
On a full replacement where this attachment was previously resolved but the new message set makes its owner ambiguous, stale_attachment_ids still contains the attachment ID after the old reference is deleted. This branch upserts the supposedly retained row but does not exempt its ID from refresh_attachment_ids, so refresh_and_sweep_attachment_rows later deletes it at ref-count zero; consequently the same ambiguous attachment is retained on first ingest but disappears after reingest. Track incoming unowned IDs and exclude them from that stale-row sweep.
AGENTS.md reference: AGENTS.md:L175-L179
Useful? React with 👍 / 👎.
| (SELECT CASE WHEN COUNT(u.model_name) = 0 THEN NULL WHEN COUNT(u.catalog_cost_usd) = COUNT(u.model_name) THEN 0 ELSE 1 END FROM session_model_usage u WHERE u.session_id = s.session_id) AS cost_is_estimated, | ||
| COALESCE((SELECT CASE WHEN MAX(u.provider_cost_usd) IS NOT NULL THEN 'origin_reported' WHEN MAX(u.catalog_cost_usd) IS NOT NULL THEN 'priced' END FROM session_model_usage u WHERE u.session_id = s.session_id), CASE WHEN s.reported_cost_usd IS NOT NULL THEN 'origin_reported' END) AS cost_provenance, | ||
| (SELECT COALESCE(SUM(u.provider_cost_usd), SUM(u.catalog_cost_usd), s.reported_cost_usd) FROM session_model_usage u WHERE u.session_id = s.session_id) AS total_cost_usd, sp.total_duration_ms, | ||
| sp.input_row_count, |
There was a problem hiding this comment.
Adding only sp.input_row_count makes ArchiveStore.get_session_profile_record stop raising, but the returned SessionProfileRecord still never receives sp.input_content_hash: this query does not select it and _session_profile_record_from_archive_row therefore leaves the field at its None default. For any materialized profile with a non-null content binding, the public Polylogue.get_session_profile_record route now reports input_content_hash=None, defeating the provenance-based content comparison this method promises. Select and map the content hash alongside the row count.
AGENTS.md reference: AGENTS.md:L175-L179
Useful? React with 👍 / 👎.
Sinity
added a commit
that referenced
this pull request
Sep 5, 2026
…4697) ## Summary Restores the archive verification registry to green on seeded fixtures. Two merged PRs each left one declaration non-green, and 16 corpus tests assert the whole registry at a single seam (`tests/infra/convergence_harness.py:371`), so both surfaced as the same failure. ## Problem Corpus run on master 754553b (`/realm/tmp/work/corpus-2026-09-05c.log`): ``` AssertionError: archive verification registry is not green: [('planner-stats', 'warning', 'sqlite_stat1 missing coverage for: session_links (polylogue-l3tk class)', ...), ('source-conservation', 'error', '5 raw item(s), 0 hook event(s), 4 index session(s); revision_superseded=1; attachment_unreferenced=1!', ...)] ``` **source-conservation.** #4692 (917e793) stopped dropping an attachment whose owning message is ambiguous and began retaining it as a typed unowned row (`ref_count` 0, deliberately excluded from the ref-count sweep). It has no `attachment_refs` row, so `source-conservation` counted it under `attachment_unreferenced`, which blocks. A deliberate, explained retention was reported as archive corruption. **planner-stats.** #4685 (0fa2a9a) added `session_links` to the verified covered set. The empty-table exemption added by #4692 is working correctly — the warning fires because `session_links` is *populated* and un-ANALYZEd. The production rebuild route (`rebuild_index.py`) did ANALYZE it; the test archive builders ANALYZEd a separately hand-copied list (`tests/infra/convergence_harness.py`, `tests/infra/schema_inference.py`) that still read `blocks, messages, action_pairs`. One route drifted from the verified set with nothing to catch it. ## Solution **source-conservation** splits the ref-less attachment population on `ref_count`, which is the discriminator the writer already establishes: - `ref_count = 0` — inserted unreferenced and kept out of the sweep, so it never had a ref: the new non-blocking `attachment_unowned` term. - `ref_count != 0` — refreshed while refs existed, then lost them without the sweep running. This is exactly the state `refresh_and_sweep_attachment_rows` documents as unreachable from every read path; it stays `attachment_unreferenced` and keeps blocking. No schema change: adding a marker column to the index tier would have forced an `INDEX_SCHEMA_VERSION` bump, regenerated schema-disposition artifacts, and a rebuild plan, to record a fact two existing columns already determine. **planner-stats** moves the covered set to one constant, `PLANNER_STATS_COVERED_TABLES` in `polylogue/storage/sqlite/maintenance.py`, with an `analyze_planner_stats_tables()` helper. The verification check, the rebuild replay, and both test builders now read it, so a table added to the verified set cannot go uncovered on a single route. `rebuild_index` still ANALYZEs `sessions` in addition, for its session-scoped replay lookups. ## Verification Red-first, measured against the same fixtures with the pre-fix modules loaded side by side: ``` === owner-ambiguous (ref_count=0) === OLD: blocking_count=1 ... attachment_unreferenced=1! NEW: blocking_count=0 ... attachment_unowned=1 === stale ref_count (ref_count=2) === OLD: blocking_count=1 ... attachment_unreferenced=1! NEW: blocking_count=1 ... attachment_unreferenced=1! ``` ``` OLD builder ANALYZE set : warning missing=['session_links'] NEW builder ANALYZE seam: ok missing=[] ``` The `ref_count=2` row is the control: the split does not disable the blocking term, it only stops it claiming the explained case. Four new tests, each naming its anti-vacuity condition: `4 passed, 3 warnings in 13.81s`. `.venv/bin/python -m devtools verify --quick` — exit 0, all 13 gates ok, no `out of sync`. `ruff check` / `ruff format --check` clean across `polylogue/` and `tests/`; `mypy` clean on the changed production modules. ## Residuals - The 13-file affected-set run (the previously-red convergence/property/reindex files) is still queued behind an unrelated full `verify` on the host's single pytest slot. Both non-green terms that produced the assertion are proven resolved above, but those 16 tests have not yet been observed green in one run; I will post the result on this PR when it lands. - `test_convergence_builder_analyze_covers_every_verified_table` imports the builder's private `_analyze_registry_tables` deliberately: asserting against a restatement of the table list would not catch a builder reverting to a hand-copied one. - Unrelated, found while researching: CLAUDE.md and `docs/internals.md` still point at `polylogue/storage/sqlite/lifecycle.py` and the delta-class lifecycle mechanism, deleted in #4666 (879a995). Wants its own sweep. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Sinity
added a commit
that referenced
this pull request
Sep 5, 2026
Master's side kept for every conflicted hunk: #4690's dispatch-link functions and #4692's attachment writer in write.py, INDEX_SCHEMA_VERSION 94, the daemon-probe status path, and the tests master advanced with them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
Sinity
added a commit
that referenced
this pull request
Sep 5, 2026
## Summary Integrates the reindex-recovery work: the physical blob disposition plan and its guarded apply, embedding-vector preservation, archive-ingest parse-pool sizing, the query-execution envelope lab check, the conversational-evidence rule in the one-shot importer, AI Studio attachment ownership on same-timestamp document turns, and attachment-reacquisition coverage. Rebased onto master f80542b. ## Review outcome The adversarial review's blocking finding was the branch's own attachment writer: retaining an owner-ambiguous attachment wrote a ref-less row that `blob-reference-closure` and `attachment-coverage` classify as blocking debt, and that crashed `polylogue ops maintenance blob-reference-closure`. Master #4692 has since shipped its own version of that writer change, so this branch keeps master's side of `write.py` in full -- the file is now byte-identical to master, which also preserves #4690's dispatch-link functions and `INDEX_SCHEMA_VERSION = 94`. What remained was the independent crash the review proved, which master does not fix: `attachment_relink._match_session_payload` called `attachment_message_owner_key` with no handler, unlike `_write_attachments`. Any ref-less attachments row makes `_read_orphaned_attachment_ids` non-empty, so the raw re-parse scan reaches the same ambiguity and raises `MessageOwnerAmbiguityError` out of `blob_reference_closure.py`, past the CLI's `BlobReferenceClosureError`-only handler. ## Solution `_match_session_payload` classifies the ambiguity as `UnrecoverableAttachmentReason.OWNER_AMBIGUOUS` -- the same typed-unrecoverable shape as `NO_AUTHORITATIVE_RAW` and `MESSAGE_MISSING`. No consumer change is needed: closure blockers are already filtered to `acquired` orphans, and both required archive-verification checks key on acquired-and-unreferenced, which the unowned row is not. Two tests, each naming its anti-vacuity condition: - `test_owner_ambiguous_orphan_is_reported_typed_not_raised` -- writes the ambiguous session through the production writer, then runs plan and relink. Removing the handler makes it red with the review's traceback. - `test_unowned_attachment_evidence_keeps_closure_and_coverage_clean` -- `blob-reference-closure` and `attachment-coverage` on a fixture archive carrying the #4692 unowned shape. Giving the row `acquired` turns both ERROR. ## Verification ``` .venv/bin/python -m devtools test tests/unit/maintenance/test_archive_verification.py tests/unit/storage/test_attachment_relink.py 1 failed, 118 passed, 3 warnings in 77.00s .venv/bin/python -m devtools verify --quick 13 gates ok, no "out of sync" ``` The single failure is `test_empty_covered_table_without_stats_is_not_missing_coverage`, confirmed failing on master itself and tracked separately. ## Residuals - The review's two should-fix items are untouched and have beads: the destructive branch of `blob_disposition_apply` (the `unlink_unreferenced_blob_hashes_under_exclusion` call) has no test that reaches a real unlink, and `embedding_preservation` still has no production caller. - An owner-ambiguous attachment carrying inline bytes reaches `_write_attachment_row` with a preacquired `acquired` tuple, which would produce the acquired-and-unreferenced shape both checks reject. The new test covers the unfetched shape master produces without preacquired bytes; the acquired variant is master's behavior, not this branch's. - Envelope `_temp_used_bytes` measures the TMPDIR filesystem, not the process (review nit 5), and the one-shot importer's evidence refusal increments no `ParseResult` counter (nit 6). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sinity
added a commit
that referenced
this pull request
Sep 5, 2026
#4701) ## Summary Follow-up to #4697. That PR typed the writer's owner-ambiguous attachment as `attachment_unowned` in `source-conservation`, but two further declarations count the same row, so seeded archives still verify non-green and the convergence tests stay red. ## Problem Running the previously-red files at #4697's head, the registry assertion is still tripped, now by a different pair: ``` archive verification registry is not green: [ ('attachment-coverage', 'error', 'acquired attachment debt: missing_blob=0, unreachable=1', ...), ('blob-reference-closure', 'error', '0 raw session(s) and 1 acquired attachment(s) lack canonical refs', ...)] ``` 14 of the 19 failures in that run reduce to this one attachment. Both checks ask "is this acquired attachment reachable through a ref?", and both answer no for a row the writer deliberately created without one. Three separate call sites carried their own copy of the "acquired and ref-less" SQL, so fixing one would have left the others reporting the same row. ## Solution One shared predicate, `acquired_attachment_missing_ref_predicate()` in `polylogue/storage/blob_liveness.py`, used by `closure_counts`, the `blob-reference-closure` sample query, and `scan_attachment_coverage`. An acquired ref-less attachment is debt only when its `ref_count` is non-zero. That is the polylogue-w06b state these checks exist to catch: the sweep set the count while refs existed, then the refs disappeared without the sweep running, leaving a row unreachable from every read path. A row inserted with `ref_count` 0 and deliberately kept out of the sweep never had a ref to lose. `scan_attachment_coverage` reports those as their own `unowned_count` dimension, and the OK summary names them rather than claiming every acquired attachment has a live reference. ## Verification All three declarations, same fixture, only `ref_count` varying: ``` === owner-ambiguous (ref_count=0) === attachment-coverage ok all 1 acquired attachment(s) have bytes and a live attachment reference; 1 retained unowned blob-reference-closure ok every raw session and acquired attachment has canonical reference closure source-conservation ok ... non_session_artifact=1; attachment_unowned=1 === stale ref_count (ref_count=2) === attachment-coverage error acquired attachment debt: missing_blob=0, unreachable=1 blob-reference-closure error 0 raw session(s) and 1 acquired attachment(s) lack canonical refs source-conservation error ... attachment_unreferenced=1! ``` The `ref_count=2` column is the anti-vacuity control: the exemption does not disable any of the three checks, it only stops them claiming the explained case. `.venv/bin/python -m devtools verify --quick` — exit 0, all 13 gates ok, no `out of sync`. `ruff` and `mypy` clean on the changed modules. ## Residuals - `test_acquired_unreachable_attachment_debt_is_blocking` previously seeded `ref_count` 0. That is now precisely the writer's deliberate shape, so the test seeded a case that is no longer debt; it seeds the stale non-zero count that actually witnesses lost refs. The scenario it describes is unchanged. - The affected-file run is queued behind a full `verify_all` on the host's single pytest slot; I will post it here when it lands and will not enable auto-merge before then. - Two failures in that run are inherited from master, not addressed here: `test_empty_covered_table_without_stats_is_not_missing_coverage` (already red in the 2026-09-05 corpus; `sqlite_stat1` holds 3 rows for an empty `action_pairs`, contradicting the premise #4692 documented) and a `dispatch_reason` key added to `session_links.evidence_json` by #4690. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes seven root causes from the 2026-09-05 master corpus reds (106 failed / 20,455 passed), profile/delegation/storage cluster:
no such column: total_input_tokens)threads/delegations/actionsare views; exports withheld every row)Verification
Lane batches:
450 passed(batch 1620), then5 failed, 664 passed in 654 s(batch 1628) on the listed cluster plus touched modules.devtools verify --quickgreen after one typing fix by the coordinator.Residuals (deliberately not fixed, with reasons)
test_session_revision_projection_golden_hashes: message-level hash drift unexplained by any diff in the identity modules; regenerating would launder a possible identity regression.assert_readable_archive_layoutstill raises the pre-migrationSchemaVersionMismatchErroron skew (fix: Validate schema identity on readable opens #4626 half-landed); flipping it changes MCP error codes; needs a caller audit.test_refresh_thread_reorder_only_touches_changed_span: chore: Collapse redundant derived tables before the wipe #4630 made_refresh_threada no-op; design decision, not a patch.test_insights_status_plain, twotest_reindex_campaigntests,test_full_reindex_and_incremental_convergence_have_equal_derived_models: still red in batch 1628.perf_floorsandcodex_804timed out under five-lane host contention.Beads: polylogue-7r19s, polylogue-xtstx, polylogue-c9qxh
🤖 Generated with Claude Code
https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid