refactor: dissolve private session insight lifecycle - #4649
Conversation
|
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: 49b57855ca
ℹ️ 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".
| ) -> SessionInsightStatusSnapshot: | ||
| ready_flags: dict[SessionInsightReadyFlag, bool] = { | ||
| descriptor.ready_key: descriptor.ready(tables, counts) for descriptor in _READY_DESCRIPTORS | ||
| } | ||
| ready_flags.update({descriptor.ready_key: descriptor.ready(tables, counts) for descriptor in _FTS_DESCRIPTORS}) | ||
| return SessionInsightStatusSnapshot( | ||
| **counts, # type: ignore[arg-type] | ||
| profile_rows_ready=ready_flags["profile_rows_ready"], | ||
| latency_profile_rows_ready=ready_flags["latency_profile_rows_ready"], | ||
| work_event_inference_rows_ready=ready_flags["work_event_inference_rows_ready"], | ||
| work_event_inference_fts_ready=ready_flags["work_event_inference_fts_ready"], | ||
| phase_inference_rows_ready=ready_flags["phase_inference_rows_ready"], | ||
| run_rows_ready=ready_flags["run_rows_ready"], | ||
| observed_event_rows_ready=ready_flags["observed_event_rows_ready"], | ||
| context_snapshot_rows_ready=ready_flags["context_snapshot_rows_ready"], | ||
| threads_ready=ready_flags["threads_ready"], | ||
| tag_rollups_ready=ready_flags["tag_rollups_ready"], | ||
| **counts, | ||
| ) |
There was a problem hiding this comment.
For every archive, session_insight_status_sync() and its async twin still iterate the run, observed-event, and context-snapshot descriptors whose table_key is insight_materialization, but that key was removed from _TABLE_DESCRIPTORS; consequently the lookup in _should_query() raises KeyError('insight_materialization') and status, health, readiness, and repair calls fail instead of returning a snapshot. The remaining missing_thread_materialization_count would also be rejected by the reduced SessionInsightStatusSnapshot when expanded here, so all retired ledger counts need to be removed from the descriptor output.
Useful? React with 👍 / 👎.
| # ddl-lifecycle-waiver: derived CREATE TABLE insight_materialization retirement removes the obsolete marker; ordinary convergence regenerates surviving rows. | ||
| INDEX_DDL = f""" |
There was a problem hiding this comment.
Opening an existing v90 index created by the parent revision now fails: removing this table changes the INDEX_DDL-based derived schema identity while INDEX_SCHEMA_VERSION remains 90, and the same-version bootstrap neither drops the table nor refreshes the old identity before ensure_derived_schema_identity() compares it and raises SchemaSkew. Register the retirement in benign same-version convergence with a safe identity refresh, or bump the version and declare the lifecycle delta, so existing archives can reach the daemon rebuild route.
AGENTS.md reference: AGENTS.md:L85-L92
Useful? React with 👍 / 👎.
| work_events_ready = True | ||
| phases_ready = True | ||
| thread_ready = True | ||
| latency_ready = True |
There was a problem hiding this comment.
For an incompletely converged archive—for example, one whose profile expects a work event but whose session_work_events row has not been produced—profile_work_event_count_mismatch is nonzero, yet this code unconditionally reports timeline_work_events.ready=true; phases, threads, and latency are likewise always declared ready even when their rows are absent. This makes the daemon workload/readiness payload hide the retryable derived backlog rather than exposing a negative verdict for the affected surface.
AGENTS.md reference: AGENTS.md:L109-L115
Useful? React with 👍 / 👎.
| # ddl-lifecycle-waiver: derived CREATE TABLE insight_materialization retirement removes the obsolete marker; ordinary convergence regenerates surviving rows. | ||
| INDEX_DDL = f""" |
There was a problem hiding this comment.
On a fresh index created by this revision, resolving a previously written child into a prefix-sharing lineage reaches _reconcile_child_prefix_storage() and executes DELETE FROM insight_materialization, even though the table has now been removed from the canonical DDL. That raises sqlite3.OperationalError: no such table: insight_materialization, aborting the parent/child reconciliation transaction instead of persisting the topology edge; the retirement must also remove this late-parent invalidation statement.
AGENTS.md reference: AGENTS.md:L56-L63
Useful? React with 👍 / 👎.
| if provenance is None: | ||
| return ArchiveInsightProvenance( | ||
| materializer_version=1, | ||
| materializer_version=SESSION_INSIGHT_MATERIALIZER_VERSION, | ||
| materialized_at=None, | ||
| input_high_water_mark=input_high_water_mark, |
There was a problem hiding this comment.
When an existing work-event, phase, latency, or thread row has no corresponding session_profiles row, _read_session_insight_provenance() returns None and this fallback now labels the result with the current materializer version despite having no durable evidence for that version. The async timeline query still uses version 1 for the same missing-profile input, so public read paths disagree and the archive-backed path falsely presents an unproven row as current; retain a legacy/unknown version until row provenance exists.
AGENTS.md reference: AGENTS.md:L174-L178
Useful? React with 👍 / 👎.
| unique_ids + (SESSION_INSIGHT_MATERIALIZER_VERSION,), | ||
| ).fetchall() |
There was a problem hiding this comment.
For an archive containing the known zero-token session_model_usage projection while its session_profile is otherwise current, this reduced predicate selects no session, so _refresh_provider_usage_rollup() is never invoked by daemon convergence; repair_session_insights() also sees zero row debt and returns "already ready." The archive-debt surface still identifies this as usage-coverage debt and claims a daemon pass self-heals it, but after removing the provider-usage ledger signal the incorrect token and cost results persist until a full index rebuild unless another row-native candidate check is added.
AGENTS.md reference: AGENTS.md:L109-L115
Useful? React with 👍 / 👎.
`_reextract_prefix_tail_db` deleted from `insight_materialization`, which #4649 retired from the index DDL, so every deferred-tail resolution raised `no such table`. Delete the child's `session_profiles`, `session_latency_profiles`, `session_work_events` and `session_phases` instead: their staleness predicate compares the session's sort key, updated-at and content hash, none of which re-extraction moves, so a profile materialized over the whole child would report fresh indefinitely. The same retirement left four `SessionInsightCountDescriptor`s whose `count_key`s are no longer snapshot fields and whose `table_key` names the retired table, making every `session_insight_status_sync`/`_async` call raise `KeyError`. Remove them and their SQL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#4649 removed the readiness descriptor whose `zero_counts` short-circuited `stale_thread_count`, `orphan_thread_count` and `stale_tag_rollup_count` when their product tables were absent, leaving the three count descriptors querying `session_profiles`, `threads` and `session_tag_rollups` unguarded. Restore the gates as `table_key`, so a status call on an archive without those relations falls back to zero instead of raising `no such table`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
Remove the session-insight materialization ledger and converge derived session rows through the generic
derivedstage.Problem
The ledger duplicated derived freshness state and could drift from normal convergence.
Solution
Remove the marker schema, repair path, readiness flags, and private periodic drain. Use source-bound provenance and migrate legacy
insightsdebt toderived. Remove stale tests for the retired lifecycle and regenerate schema disposition evidence.Verification
uv run devtools verify --quickpassed all gates, including schema-versioning. The packet commanduv run devtools test tests/unit/storage/insights tests/unit/insightsfailed before collection because the first path does not exist. The corrected broad selection was killed by the host pytest queue after unrelated failures; it has no completion receipt.Residual risk
The broad focused selection needs a fresh managed run if full storage and insight test coverage is required.