chore: Collapse redundant derived tables before the wipe - #4630
Conversation
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. |
|
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 |
![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: 672d0dd804
ℹ️ 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".
|
|
||
| CREATE INDEX IF NOT EXISTS idx_threads_time | ||
| ON threads(end_time DESC, start_time DESC); | ||
| CREATE VIEW IF NOT EXISTS threads AS |
There was a problem hiding this comment.
After threads becomes a view, _insight_readiness_entry() still probes it and thread_sessions through storage.introspection.table_exists(), which only matches type='table'. Consequently every v87 thread-readiness response reports the thread surface as missing and both artifacts as absent even when these views are queryable, causing consumers such as insight-bundle export to withhold otherwise available thread data. Use relation_exists() for these probes or retain a table-backed readiness artifact.
AGENTS.md reference: AGENTS.md:L170-L172
Useful? React with 👍 / 👎.
| conn.execute("DELETE FROM session_model_usage WHERE session_id = ?", (child_session_id,)) | ||
| _aggregate_message_tokens_into_model_usage(conn, child_session_id) | ||
| _aggregate_provider_usage_into_model_usage(conn, child_session_id) | ||
| refresh_delegation_facts_for_session(conn, child_session_id) | ||
| conn.execute("DELETE FROM insight_materialization WHERE session_id = ?", (child_session_id,)) |
There was a problem hiding this comment.
When a child is ingested before its parent and its replayed prefix contains a subagent dispatch, late-parent resolution deletes that inherited prefix from the child here. delegation_facts is still a materialized table, while the later outer refresh targets the newly ingested parent rather than the mutated child, so rows with parent_session_id = child_session_id can continue exposing dispatches that were just removed. Restore refresh_delegation_facts_for_session(conn, child_session_id) after these mutations, or actually convert the relation to a query-time view.
AGENTS.md reference: AGENTS.md:L56-L62
Useful? React with 👍 / 👎.
| WHEN ap.is_error IS NULL AND ap.exit_code IS NULL THEN 'outcome_unknown' | ||
| WHEN ap.exit_code IS NOT NULL AND ap.exit_code != 0 THEN 'outcome_error' | ||
| WHEN ap.exit_code IS NULL AND ap.is_error = 1 THEN 'outcome_error' | ||
| WHEN ap.is_error = 0 OR ap.exit_code = 0 THEN 'outcome_success' | ||
| ELSE 'outcome_unknown' | ||
| ELSE 'outcome_success' |
There was a problem hiding this comment.
When a result block's canonical tool_outcome is more precise than stale or nullable compatibility fields—for example tool_outcome='unknown' with legacy false/zero values—this CASE now reports outcome_success; a canonical error with both legacy fields null is reported unknown. This makes action filters and reports depend on compatibility columns instead of the declared authority. Keep the tr.tool_outcome branches ahead of the legacy fallback so an unknown outcome can never become success.
AGENTS.md reference: AGENTS.md:L46-L50
Useful? React with 👍 / 👎.
| _raw_column("source_url", """source_url TEXT"""), | ||
| _raw_column("caption", """caption TEXT"""), |
There was a problem hiding this comment.
For any session containing an attachment, the unchanged writer executes an INSERT INTO attachment_refs (..., direction, producer_ref, ...), but this table spec no longer declares either column. A fresh v87 index therefore fails attachment ingestion with no column named direction before it can persist the reference; the same missing columns also break the existing carry-forward SELECTs on replacement. Restore both columns to the canonical spec so fresh DDL still matches its readers and writers.
AGENTS.md reference: AGENTS.md:L170-L172
Useful? React with 👍 / 👎.
| (SELECT CASE WHEN COUNT(u.model_name) = 0 THEN NULL | ||
| WHEN COUNT(u.provider_cost_usd) > 0 THEN 0 | ||
| WHEN COUNT(u.catalog_cost_usd) = COUNT(u.model_name) THEN 0 ELSE 1 END | ||
| FROM session_model_usage u |
There was a problem hiding this comment.
When a delegated child has provider_cost_usd rows but no complete catalog pricing, child_cost_usd immediately above correctly selects the provider-reported dollars, yet this CASE now sets child_cost_is_estimated to 1. That makes the public delegation payload label exact provider evidence as estimated. Preserve the removed COUNT(provider_cost_usd) > 0 exactness branch before checking catalog coverage.
Useful? React with 👍 / 👎.
| root_ids = [str(row["thread_id"]) for row in rows] | ||
| records = await build_thread_records_for_roots_async(conn, root_ids) | ||
| return [records[root_id] for root_id in root_ids if root_id in records] |
There was a problem hiding this comment.
When a newer root has no session_profiles row but an older root does, a request such as list_threads(limit=1) applies the limit to both roots in the SQL view, selects the unmaterialized root, and then silently drops it when build_thread_records_for_roots_async() returns no record. The caller receives an empty page even though a valid thread exists immediately after it, and offsets likewise skip valid records. Restrict the SQL candidates to hydratable roots or paginate after hydration.
AGENTS.md reference: AGENTS.md:L170-L172
Useful? React with 👍 / 👎.
| lower(g.thread_id || char(10) || COALESCE(r.dominant_repo, '') || char(10) || g.session_ids_json) | ||
| AS search_text, |
There was a problem hiding this comment.
When list_threads(query=...) searches for evidence represented only by the derived thread record—such as a work-event type like implementation—the SQL prefilter now misses it because this replacement search_text contains only the thread ID, dominant repo, and session IDs. The prior build_thread_record() text also included work-event keys, support level, support signals, and member signals, and that builder still produces those fields after the SQL filtering step. Include the same searchable fields in the view or perform the query against the hydrated records.
AGENTS.md reference: AGENTS.md:L170-L172
Useful? React with 👍 / 👎.
| ), | ||
| ) | ||
|
|
||
| WEB_CONTENT_CONSTRUCTS_SPEC = _make_table_spec( |
There was a problem hiding this comment.
After the SESSIONS_SPEC = replace(...) projection block is removed, every raw session column has record_name=None and SESSIONS_SPEC.record_select_column_names() returns an empty string. The async session readers consequently construct statements such as SELECT FROM sessions WHERE session_id = ?, so get_session, batch reads, and list queries fail with a SQL syntax error on every archive. Restore the record-name, timestamp, hash, metadata, and working-directory projections before declaring the next table spec.
AGENTS.md reference: AGENTS.md:L170-L172
Useful? React with 👍 / 👎.
672d0dd to
d0b1f36
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0b1f36acd
ℹ️ 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".
| SELECT dm.tag, | ||
| dm.bucket_day, | ||
| dm.source_name, | ||
| 0 AS materializer_version, 'query-time' AS materialized_at, |
There was a problem hiding this comment.
For any archive with at least one profile-backed tag, list_session_tag_rollup_records() maps this literal into SessionTagRollupRecord.materialized_at; passing those records to the standard aggregate_session_tag_rollup_insights() reducer then reaches records_provenance(), which unconditionally parses every nonempty materialization value as ISO-8601 and raises ValueError for query-time. Return an ISO timestamp or nullable value, or explicitly teach the provenance reducer about query-time rows, so tag-rollup records remain consumable.
AGENTS.md reference: AGENTS.md:L170-L172
Useful? React with 👍 / 👎.
| CREATE VIEW IF NOT EXISTS threads AS | ||
| WITH RECURSIVE members AS ( | ||
| SELECT s.session_id, s.parent_session_id, | ||
| COALESCE(s.root_session_id, s.session_id) AS thread_id, |
There was a problem hiding this comment.
When parent A is ingested after child B, and B already has a resolved child C, _resolve_session_graph() refreshes the stored projection only for A and B; C therefore retains root_session_id = B after B moves under A. Grouping directly on this cached value exposes one A thread containing A/B and a phantom B thread containing C, instead of the single A/B/C lineage that the previous recursive thread queries derived from parent_session_id. Propagate root changes through all descendants or derive thread membership recursively rather than trusting the stale transitive projection.
AGENTS.md reference: AGENTS.md:L56-L63
Useful? React with 👍 / 👎.
| thread_count = int( | ||
| conn.execute( | ||
| f"SELECT COUNT(*) FROM threads WHERE thread_id IN ({placeholders})", | ||
| affected_roots, | ||
| ).fetchone()[0] |
There was a problem hiding this comment.
When a scoped insight rebuild targets only a child session, _stamp_bundle_materialization() updates that child's thread marker, but this replacement for _refresh_thread_roots_sync() only counts the affected root and no longer refreshes its marker. _thread_insight_from_id() still reads provenance from insight_materialization using the root ID, so the returned thread includes the child's newly derived data while reporting the root's old materialization time and high-water mark. Refresh or derive root-level provenance whenever any member changes; the async scoped path needs the same treatment.
AGENTS.md reference: AGENTS.md:L103-L111
Useful? React with 👍 / 👎.
…4692) ## Summary Fixes seven root causes from the 2026-09-05 master corpus reds (106 failed / 20,455 passed), profile/delegation/storage cluster: - fix(storage): select the input_row_count the profile record reads (`no such column: total_input_tokens`) - fix(storage): insight presence counts views, not only tables (`threads`/`delegations`/`actions` are views; exports withheld every row) - fix(storage): retain an ambiguous-owner attachment as typed unowned (PR #4644 dropped it before its row was written; bead polylogue-c9qxh) - fix(maintenance): absent stats for an empty table are not missing coverage - test: supply parent-dispatch evidence delegation resolution now requires; read profile totals and ingest seams at their current locations; follow embedding reuse and the declared content-hash partition; classify new index tables and satisfy the threads view dependency ## Verification Lane batches: `450 passed` (batch 1620), then `5 failed, 664 passed in 654 s` (batch 1628) on the listed cluster plus touched modules. `devtools verify --quick` green 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_layout` still raises the pre-migration `SchemaVersionMismatchError` on skew (#4626 half-landed); flipping it changes MCP error codes; needs a caller audit. - `test_refresh_thread_reorder_only_touches_changed_span`: #4630 made `_refresh_thread` a no-op; design decision, not a patch. - `test_insights_status_plain`, two `test_reindex_campaign` tests, `test_full_reindex_and_incremental_convergence_have_equal_derived_models`: still red in batch 1628. - `perf_floors` and `codex_804` timed out under five-lane host contention. Beads: polylogue-7r19s, polylogue-xtstx, polylogue-c9qxh 🤖 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>
1 participant
Summary
Collapse action pairing, delegation facts, thread membership, thread summaries, and session tag rollups into query-time views over canonical index evidence. Remove their write-side refresh and bulk rebuild paths, and group thread search hits through
messages_fts.Problem
These index-tier tables duplicated derivable facts and imposed convergence and delete work against large archives. The measured redundancies were
thread_sessionsas a root-session restatement,action_pairsas a materialized action pairing,delegation_factsas a duplicate delegation projection, andsession_tag_rollupsas an oversized aggregate cache. Attachments remain normalized canonical evidence.Solution
Fresh index DDL declares five compatibility views and no corresponding tables. Session insight rebuild and refresh paths retain only per-session materializations. Delegation and action readers continue through the public relation names. Schema disposition marks the five views
DERIVE, and index schema v81 declares the lifecycle as a semantic reparse because existing persisted rows cannot be retained when tables become views.Verification
nix develop --accept-flake-config --command devtools verify --quick: passed.nix develop --accept-flake-config --command devtools test tests/unit/storage/test_archive_tiers_archive.py::test_read_open_rejects_stale_index_with_generation_and_lifecycle_action: 1 passed.git diff --check: passed.