fix(runtime-host): page Session traces at the source - #3133
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review. 📝 WalkthroughWhat problem this solvesThe Runtime Host previously loaded complete Session traces before display. Large or corrupt Sessions could exceed live Host evidence limits and fail inspection. This PR paginates traces at the Storage source. Each page uses evidence, turn, and result budgets. Opaque cursors support continuation. Oversized and unreadable evidence produces partial coverage instead of failure. The Desktop loads earlier pages on demand. Usage and cache summaries load independently from the timeline. Source of truthThis PR extends the existing Storage and usage sources of truth. It does not create a parallel trace dataset. The Runtime Host reads paginated Durable usage writes publish session-scoped invalidations. The Runtime Host coalesces these events. The Desktop consumes them through the current Host epoch. Scope and complexityThis is the smallest coherent solution for bounded inspection:
Cursor validation, refresh guards, refresh coalescers, coverage tracking, session scoping, and migration logic are necessary for continuation, refresh races, corrupt evidence, usage isolation, and mixed-version boundaries. Deletion and simplification opportunitiesThe obsolete complete-trace loader, revision/offset pagination, timeline filter state, filter models, aggregate trace totals, and related styles were removed. No further deletion is evident from the supplied diff. The added tests cover pagination, refresh races, unreadable and oversized evidence, composite identity, usage scoping, invalidation, migrations, protocol validation, and stale retry supersession. Risks and validationUser-visible behavior changes include:
Public contracts changed in the preload bridge, Runtime Host inspection protocol, Storage interfaces, usage query types, and session-domain notifications. The Runtime Host compatibility epoch changed from 22 to 23. The SQLite usage schema changed from version 3 to 4 and adds session-scoped indexes. Final test, build, lint, typecheck, and Storybook results remain unverified because no direct check output was provided. Review-relevant risks
The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe session inspector now uses session-scoped usage summaries and cursor-based trace pages. Runtime usage changes propagate through continuity and IPC notifications. The desktop UI displays cost, cache-hit rate, coverage status, timestamps, and earlier trace pages. ChangesSession inspector data flow
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🔵 Low · up to The PR makes Session traces paged and keeps summaries Session-wide, with focused tests and builds passing. It is mergeable with owner awareness that one test fixture depends on the node:sqlite runtime and storage-internal names, which could cause unrelated test failures if those interfaces change. Sequence Diagram(s)sequenceDiagram
participant UsageWriter
participant RuntimeHost
participant InspectorBridge
participant UseSessionTrace
participant InspectorPanel
UsageWriter->>RuntimeHost: publish session usage change
RuntimeHost->>InspectorBridge: usage:changed(sessionId)
InspectorBridge->>UseSessionTrace: notify usage change
UseSessionTrace->>InspectorBridge: request summary or trace page
InspectorBridge-->>UseSessionTrace: return summary or nextCursor
UseSessionTrace->>InspectorPanel: provide inspector snapshot
<f_fixed_issue_severity>Low</f_fixed_issue_severity> 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
bc35fc6 to
8096203
Compare
There was a problem hiding this comment.
Pull request overview
This PR reworks Session inspection to page trace data at the Storage source (instead of materializing entire Sessions in the Runtime Host), while keeping Session-wide usage/cost summaries independent of timeline pagination. It also introduces Session-scoped usage invalidations so Desktop can refresh usage estimates from the Usage authority rather than inferring from Session events.
Changes:
- Add Session-scoped indexing/querying for usage + model-call ledgers (including schema migration/backfill) and publish Session usage-change notifications after durable writes.
- Introduce cursor-based, bounded Session trace paging in Runtime Host and propagate a new
usagesession domain for continuity invalidations. - Update Desktop Inspector to load only a bounded trace window initially, append earlier pages on demand, and fetch Session-wide usage summary independently.
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/storage/src/usage-stores.ts | Adds Session-scoped model-call paging and a Session usage-change subscription API on the usage writer facade. |
| packages/storage/src/sqlite-usage-store.ts | Stores session_id for legacy LLM rows, clamps cache-read tokens, and scopes reads by (session_id, ts) where applicable. |
| packages/storage/src/sqlite-usage-schema.ts | Bumps schema version and migrates/backfills session_id columns + supporting indexes. |
| packages/storage/src/model-call-ledger.ts | Adds optional Session scoping for model-call reads and pending reprojection queries; persists session_id. |
| packages/storage/src/execution-stores.ts | Plumbs a new AgentRun paging API through execution stores. |
| packages/storage/src/agent-run-store.ts | Implements listSessionRunsPage with stable (created_at, run_id) cursor semantics. |
| packages/storage/src/tests/usage-stores.test.ts | Adds coverage for Session usage-change publication and legacy summary clamping/scoping behavior. |
| packages/storage/src/tests/sqlite-usage-schema.test.ts | Verifies migration backfills Session identity + creates indexes. |
| packages/storage/src/tests/sqlite-core-execution-store.test.ts | Tests stable AgentRun paging order and cursor behavior. |
| packages/storage/src/tests/model-call-ledger.test.ts | Ensures Session-scoped ledger reads exclude other Sessions (including corrupt rows). |
| packages/runtime/src/session-trace-projection.ts | Moves trace identity to (runId, turnId) and introduces oversizedRuns coverage. |
| packages/runtime/src/tests/session-trace-projection.test.ts | Adds tests for per-run turn identity separation and partial coverage classification. |
| packages/runtime-host/src/server/execution-inspect-coordinator.ts | Replaces revision/offset paging with cursor-based, bounded Session trace pages assembled from run pages. |
| packages/runtime-host/src/server/execution-composition.ts | Subscribes to usage-change notifications and emits usage session-domain invalidations. |
| packages/runtime-host/src/server/canonical-usage-reader.ts | Scopes pending repairs and attempt reads by sessionId where requested. |
| packages/runtime-host/src/protocol/usage-pricing.ts | Allows sessionId in usage query decoding. |
| packages/runtime-host/src/protocol/session-continuity.ts | Adds usage to the set of Session domains. |
| packages/runtime-host/src/protocol/index.ts | Advances compatibility epoch to 23. |
| packages/runtime-host/src/protocol/execution-inspect.ts | Updates Session trace inspect protocol to use an opaque cursor and nextCursor (removing revision/offset). |
| packages/runtime-host/src/tests/usage-pricing-protocol.test.ts | Adds test ensuring Session summary doesn’t repair/report other Sessions’ pending projections. |
| packages/runtime-host/src/tests/session-continuity-coordinator.test.ts | Extends domain invalidation coalescing tests to include usage. |
| packages/runtime-host/src/tests/protocol.test.ts | Adjusts imports/order due to protocol constant changes. |
| packages/runtime-host/src/tests/execution-inspect-protocol.test.ts | Updates protocol tests for cursor-based paging and expanded coverage fields. |
| packages/runtime-host/src/tests/execution-inspect-coordinator.test.ts | Adds end-to-end tests for paging, oversized runs/results, corrupt evidence handling, and cursor validation. |
| packages/core/src/usage-stats/types.ts | Adds optional sessionId to usage queries. |
| packages/core/src/session-trace.ts | Introduces (runId, turnId) identity helpers and adds oversizedRuns; adds trace/coverage merge helpers. |
| packages/core/src/model-call-usage-projection.ts | Adds Session filtering + clamps cache-read tokens via exported helper. |
| packages/core/src/tests/session-trace.test.ts | Tests coverage merge semantics and multi-page trace merge behavior. |
| packages/core/src/tests/model-call-usage-projection.test.ts | Tests cache-read clamping and Session scoping in selection/projection. |
| docs/astryx-surface-file-inventory.md | Updates Astryx surface inventory to reflect Inspector component usage changes. |
| apps/desktop/stories/session-workbar.stories.tsx | Updates story fixtures for new trace identity/coverage and adds a “load more history” story path. |
| apps/desktop/src/renderer/use-session-trace.ts | Refactors trace loading into paged windows + independent summary/context reads and refresh signals. |
| apps/desktop/src/renderer/styles/chat-detail.css | Removes CSS tied to Inspector search/filter UI that was deleted. |
| apps/desktop/src/renderer/session-trace-refresh.ts | Generalizes refresh coalescing for authority invalidations and keeps trace-specific event coalescer. |
| apps/desktop/src/renderer/session-inspector-panel.tsx | Removes in-memory search/filter UI, adds “Load earlier records”, and switches turn labeling to stable timestamps. |
| apps/desktop/src/renderer/session-inspector-panel-model.ts | Updates panel model to carry run identity + startedAt and to surface oversizedRuns. |
| apps/desktop/src/renderer/session-inspector-overview-model.ts | Moves cache hit rate + cost estimate to be derived from Session-wide usage summary (not paged trace). |
| apps/desktop/src/renderer/session-inspector-filter.ts | Removes the Inspector timeline filter implementation (no longer supported with paged data). |
| apps/desktop/src/renderer/locales/conversation-copy.ts | Updates copy for paged timeline/usage summary states and new coverage wording/fields. |
| apps/desktop/src/preload/preload.ts | Changes Inspector trace API to return { trace, nextCursor }; adds usage summary call and usage-change subscription. |
| apps/desktop/src/preload/bridge-contract.d.ts | Extends bridge contract with trace-page + Session usage summary types and usage-change subscription. |
| apps/desktop/src/main/runtime-host-session-domains-ipc-main.ts | Routes usage domain invalidations to a usage:changed renderer event and includes it in resync. |
| apps/desktop/src/main/tests/use-session-trace.test.ts | Adds tests for usage-only refresh, page-depth rebuild on refresh, cursor stability, and summary failure behavior. |
| apps/desktop/src/main/tests/session-inspector-panel-model.test.ts | Adds tests for cost-estimate semantics and availability heuristics; updates coverage expectations. |
| apps/desktop/src/main/tests/runtime-host-session-domains-ipc-main.test.ts | Verifies usage:changed dispatch and resync behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/core/src/session-trace.ts (1)
676-699: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAdd a
turnIdtiebreak to the page merge comparator.The comparator compares
startedAtthenrunId. Two turns of the same run that sharestartedAtkeep insertion order, so the merged order depends on which page arrived first.orderedTurnIdentitiesinpackages/runtime/src/session-trace-projection.ts(lines 494-501) already breaks the same tie byrunIdthenturnId. Aligning both comparators makes the merged order independent of page arrival order.♻️ Proposed comparator alignment
const ordered = [...turns.values()].sort( - (left, right) => left.startedAt - right.startedAt || left.runId.localeCompare(right.runId), + (left, right) => + left.startedAt - right.startedAt || + left.runId.localeCompare(right.runId) || + left.turnId.localeCompare(right.turnId), );packages/runtime-host/src/__tests__/execution-inspect-coordinator.test.ts (2)
296-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the continuation cursor instead of returning early.
Line 301 returns when
first.result.nextCursoris falsy. If pagination stops emitting a cursor for fractionalcreatedAtvalues, this test passes without checking anything, which is the exact regression it exists to catch. Assert the cursor, then continue.💚 Proposed assertion
assert.equal(first.ok, true); - if (!first.ok || first.result.kind !== 'session_trace_page' || !first.result.nextCursor) - return; + if (!first.ok || first.result.kind !== 'session_trace_page') return; + assert.ok(first.result.nextCursor, 'a 17th run must leave a continuation cursor');As per path instructions: "Flag tests that duplicate existing coverage, assert implementation details, or do not protect observable behavior."
Source: Path instructions
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the corruption update changes one row.
Capture
run()and assertchanges === 1so storage schema drift fails with an explicit fixture error.packages/core/src/__tests__/session-trace.test.ts (1)
26-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the untested
nonefold direction and the two page-merge rejections.The table omits
['absent', 'none', 'absent'], so thenext.modelCalls === 'none'branch inmergeDisjointTraceCoverage(packages/core/src/session-trace.tsline 648) is never executed. One extra row closes that.
mergeSessionTracesalso guarantees two rejections that no test exercises: an empty page list, and pages that disagree onsessionIdorschemaVersion. Both are stated contracts and both are cheap to assert.💚 Proposed additions
const cases = [ ['none', 'absent', 'absent'], + ['absent', 'none', 'absent'], ['absent', 'absent', 'absent'], ['no_known_gap', 'no_known_gap', 'no_known_gap'], ['absent', 'no_known_gap', 'partial'], ['partial', 'no_known_gap', 'partial'], ] as const;assert.equal(merged.totals.inputTokens, 3); + + assert.throws(() => mergeSessionTraces([]), /At least one Session trace page/); + assert.throws( + () => mergeSessionTraces([page('run-1', 1, 1), { ...page('run-2', 2, 2), sessionId: 'other' }]), + /same Session/, + ); });Also applies to: 66-76
apps/desktop/src/preload/preload.ts (1)
798-807: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrap
loadSessionUsageSummaryinbridgeResultso the declaredResultcontract always holds.
inspector.traceandinspector.contextconvert a thrown error into{ ok: false, error }.inspector.summaryreturns the rawipcRenderer.invokepromise cast toResult<DesktopSessionUsageSummary>. IfruntimeHostSessionRefthrows, or theusage:summaryhandler rejects, the returned promise rejects instead of resolving to aResult. The declared return type then does not describe the runtime behavior.
use-session-trace.tscurrently passes a rejection handler, so the UI still recovers. The contract inconsistency remains.♻️ Uniform Result envelope
-async function loadSessionUsageSummary( - sessionId: string, -): Promise<Result<DesktopSessionUsageSummary>> { - const session = await runtimeHostSessionRef(sessionId); - return ipcRenderer.invoke( - 'usage:summary', - session.scope, - { range: 'all', sessionId: session.sessionId }, - ) as Promise<Result<DesktopSessionUsageSummary>>; -} +function loadSessionUsageSummary( + sessionId: string, +): Promise<Result<DesktopSessionUsageSummary>> { + return bridgeResult(async () => { + const session = await runtimeHostSessionRef(sessionId); + const result = await ipcRenderer.invoke( + 'usage:summary', + session.scope, + { range: 'all', sessionId: session.sessionId }, + ) as Result<DesktopSessionUsageSummary>; + if (!result.ok) throw new Error(result.error.message); + return result.data; + }, 'INSPECTOR_SUMMARY_FAILED'); +}Also applies to: 2281-2283
apps/desktop/src/main/__tests__/use-session-trace.test.ts (1)
183-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
tracePageproducesstartedAt: NaNfor non-numeric run ids, so the ordering assertions rely on the merge tie-break.Line 188 computes
Number(runId.replace(/\D/g, '')). For'run-z','run-m', and'run-n'the digit set is empty, soNumber('')is0… no:runId.replace(/\D/g,'')yields''andNumber('')is0. For these ids the value is0, so every turn sharesstartedAt: 0andmergeSessionTracesfalls back torunId.localeCompare. The expected orders['run-m','run-z']and['run-n','run-z']therefore assert the tie-break, not chronological ordering.Give the fixture an explicit
startedAtso the test states the ordering it means.♻️ Explicit timestamps in the fixture
function tracePage( sessionId: string, runId: string, nextCursor: string | null, + startedAt = Number(runId.replace(/\D/g, '')) || 0, ): DesktopSessionTracePage { - const startedAt = Number(runId.replace(/\D/g, '')); return {Also applies to: 400-440
apps/desktop/src/main/__tests__/session-inspector-panel-model.test.ts (1)
159-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the cast so the assertion still protects the
InspectorTurnRowcontract.
InspectorTurnRowdeclaresstartedAt: number. The cast to{ startedAt?: number } | undefinedmakes the test compile even ifstartedAtis removed from the model, which is the field this test exists to protect.♻️ Assert the typed field directly
- const turn = deriveInspectorPanelModel(trace).turns[0]; - assert.equal((turn as { startedAt?: number } | undefined)?.startedAt, 1); + const turn = deriveInspectorPanelModel(trace).turns[0]; + assert.ok(turn); + assert.equal(turn.startedAt, 1);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e684a90-c382-4a33-8c4c-e884bcd01081
📒 Files selected for processing (45)
apps/desktop/src/main/__tests__/runtime-host-session-domains-ipc-main.test.tsapps/desktop/src/main/__tests__/session-inspector-panel-model.test.tsapps/desktop/src/main/__tests__/use-session-trace.test.tsapps/desktop/src/main/runtime-host-session-domains-ipc-main.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/locales/conversation-copy.tsapps/desktop/src/renderer/session-inspector-filter.tsapps/desktop/src/renderer/session-inspector-overview-model.tsapps/desktop/src/renderer/session-inspector-panel-model.tsapps/desktop/src/renderer/session-inspector-panel.tsxapps/desktop/src/renderer/session-trace-refresh.tsapps/desktop/src/renderer/styles/chat-detail.cssapps/desktop/src/renderer/use-session-trace.tsapps/desktop/stories/session-workbar.stories.tsxdocs/astryx-surface-file-inventory.mdpackages/core/src/__tests__/model-call-usage-projection.test.tspackages/core/src/__tests__/session-trace.test.tspackages/core/src/model-call-usage-projection.tspackages/core/src/session-trace.tspackages/core/src/usage-stats/types.tspackages/runtime-host/src/__tests__/execution-inspect-coordinator.test.tspackages/runtime-host/src/__tests__/execution-inspect-protocol.test.tspackages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/__tests__/session-continuity-coordinator.test.tspackages/runtime-host/src/__tests__/usage-pricing-protocol.test.tspackages/runtime-host/src/protocol/execution-inspect.tspackages/runtime-host/src/protocol/index.tspackages/runtime-host/src/protocol/session-continuity.tspackages/runtime-host/src/protocol/usage-pricing.tspackages/runtime-host/src/server/canonical-usage-reader.tspackages/runtime-host/src/server/execution-composition.tspackages/runtime-host/src/server/execution-inspect-coordinator.tspackages/runtime/src/__tests__/session-trace-projection.test.tspackages/runtime/src/session-trace-projection.tspackages/storage/src/__tests__/model-call-ledger.test.tspackages/storage/src/__tests__/sqlite-core-execution-store.test.tspackages/storage/src/__tests__/sqlite-usage-schema.test.tspackages/storage/src/__tests__/usage-stores.test.tspackages/storage/src/agent-run-store.tspackages/storage/src/execution-stores.tspackages/storage/src/model-call-ledger.tspackages/storage/src/sqlite-usage-schema.tspackages/storage/src/sqlite-usage-store.tspackages/storage/src/usage-stores.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/styles/chat-detail.css
- apps/desktop/src/renderer/session-inspector-filter.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 366d7faa-58e8-4deb-a552-6d3defde1b32
📒 Files selected for processing (8)
apps/desktop/src/main/__tests__/use-session-trace.test.tsapps/desktop/src/renderer/use-session-trace.tspackages/core/src/__tests__/session-trace.test.tspackages/core/src/session-trace.tspackages/storage/src/__tests__/sqlite-core-execution-store.test.tspackages/storage/src/__tests__/usage-stores.test.tspackages/storage/src/agent-run-store.tspackages/storage/src/sqlite-usage-store.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/src/tests/session-trace.test.ts
- packages/storage/src/tests/sqlite-core-execution-store.test.ts
- packages/storage/src/tests/usage-stores.test.ts
- packages/storage/src/sqlite-usage-store.ts
- packages/core/src/session-trace.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/desktop/src/renderer/session-inspector-panel.tsx:639
formatTurnStartedAtallocates a newIntl.DateTimeFormaton every row render. For long traces this can become a measurable hotspot; consider caching the formatter per-locale (similar tonumberFormatter) and only formatting theDateper call.
function formatTurnStartedAt(startedAt: number, locale: UiLocale): string {
const date = new Date(startedAt);
if (!Number.isFinite(date.getTime())) return '—';
return new Intl.DateTimeFormat(uiLocaleToIntlLocale(locale), {
dateStyle: 'short',
|
/agentic_review Automated request by Codex on behalf of @Astro-Han to verify the newly installed Qodo OSS review integration. |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/storage/src/sqlite-usage-schema.ts:88
ensureColumnbuilds SQL by interpolatingtable/columndirectly into the statement. Even though current callers pass constants, this helper is now a footgun (and potential injection vector) if reused with non-validated identifiers later. Consider validatingtable/columnagainst a strict identifier regex before composing SQL.
9d276d1 to
55e0516
Compare
|
Heads-up on a cross-PR collision — not a review comment on your change.
The trap is that this does not conflict. All three branches write the same text to that line, so git's three-way merge takes it silently; only the adjacent comment block conflicts, and keeping both comments is the natural resolution. Each PR's own Please re-check against (Posted with Claude Code (Opus 5) assistance; the epoch values were read from each branch head.) |
55e0516 to
7b69b81
Compare
|
@hqhq1025 @M4n5ter — re-review requested on the rebased head The latest head addresses the three confirmed findings:
The Focused Core, Storage, Runtime Host, and Desktop checks pass, including 49 affected tests, Desktop typecheck, Biome, and This update and comment were prepared with Codex assistance. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
One merge-blocking finding is attached inline.
Problem and mechanism: this PR correctly addresses unbounded Session inspection by paging AgentRuns at Storage, while keeping Session-wide Usage as an independent authority and refreshing it through Session-scoped invalidations. The problem definition is supported by the prior all-at-once materialization path, and source paging is the right first-principles boundary.
First principles and Occam: the pagination, composite trace identity, and independent Usage summary are materially simpler than loading an entire Session and inferring totals from a partial timeline. The previous invalidation loop and partial-summary presentation findings are fixed on this head. However, the repair intent is still represented by one unversioned row per run. That representation cannot distinguish an idempotent replay from a newer authority append while an older repair is in flight, so an old repair can delete the only durable recovery signal for a new billed call.
Optimality/refactor: preserve marker generations and compare-and-swap clear the exact generation observed by repair, or use per-attempt repair intents. The current insert/no-op plus unconditional clear is not a maintainable final state. Add a deterministic concurrency regression covering old repair snapshot -> new authority append/mark -> crash before projection -> old repair clear.
Deletion opportunities: SessionTrace.totals appears to have no Session-level production consumer after Usage became authoritative; removing that projection/wire/merge surface is a reasonable follow-up, not a blocker. I did not find a low-quality test that must be deleted for this fix; the missing test is the concurrent marker-generation case above.
Merge verdict: not ready to merge. Residual non-blocking gaps include corrupt persisted AgentRun/RuntimeEvent rows pinning pagination, the preload summary method not uniformly wrapping rejected promises in its declared Result envelope, and CI still running at review time.
Verification: reviewed head 7b69b81; 164 focused Core, Storage, Runtime, Runtime Host, and Desktop tests passed; the full affected build completed after applying repository dependency patches (one initial sequential workspace resolution retry was required).
| `) | ||
| .run(sessionId, runId, Date.now()); | ||
| .run(sessionId, runId, Date.now()).changes; | ||
| return changed > 0; |
There was a problem hiding this comment.
[P2] Preserve a newer repair intent when this marker already exists
Returning false here conflates an idempotent replay with a newer attempt reusing an existing run marker. A repair can snapshot the old AgentRun events, a later authority append can hit the existing marker and make this mark a no-op, and the stale repair then clears that only marker. If the later Usage projection fails or the process exits before it, the new billed call remains only in AgentRun while pendingReprojections() is empty; no implemented full-stream sweep rediscovers it. I reproduced exactly authority=[old,new], stored=[old], pending=[]. Please version the marker and CAS-clear the generation read by repair, or use per-attempt repair intents.
7b69b81 to
6f52b7b
Compare
|
Rebased onto the latest The new invariant is: a run is pending exactly while its canonical AgentRun model-call high-water exceeds the Usage checkpoint. Projection rows and checkpoint advancement commit atomically, so the ABA race and the authority-append-before-marker crash window are both removed. A rebuildable run-level high-water index, maintained in the AgentRun append transaction and backfilled on migration, keeps lagging-run discovery from rescanning the full event history. Current head: Validation:
@hqhq1025, could you please re-review the updated approach and confirm whether the original race is resolved? Posted by Codex on behalf of the contributor. |
Read the latest trace page without materializing the full Session, expose stable older-history cursors, and keep the Inspector's usage estimate scoped to the complete Session. Generated-by: Codex
Degrade Session trace pages that exceed result or turn limits into explicit unreadable coverage while preserving their continuation cursor. Validate opaque cursor payloads before they reach Storage so malformed input is reported as invalid_request. Generated-by: Codex
Keep Session trace pages keyed by their request cursor, reconnect refreshed head pages to the existing tail, and derive totals and coverage only from the connected disjoint window. Separate Session lifetime from head refresh revisions so earlier-page reads survive live updates, and remove stale Session summaries after a failed refresh. Generated-by: Codex
Route Inspector summaries through the existing Host-scoped usage IPC instead of widening the generic renderer allowlist. Clamp cache-read tokens per canonical and legacy record so one malformed attempt cannot inflate the full-Session cache rate. Generated-by: Codex
Cover Storage keyset ordering and append stability directly, replace the 2,800-write aggregate evidence fixture with two projection-ignored bounded records, and remove the literal compatibility-epoch assertion that only repeated a production constant. Generated-by: Codex
Delete the unused overview trace input, panel-level trace totals, dead duration copy, and search-era comments and classes now that the complete Session summary and paged timeline have distinct owners. Generated-by: Codex
Provide Session usage summary fixtures through the Storybook inspector bridge so trace stories exercise the same scoped IPC contract as the desktop renderer. Generated-by: Codex
Rebuild the visible trace prefix from the source on refresh, fill Host pages by the existing evidence budgets, and keep continuation cursors valid for every accepted AgentRun timestamp. Push Session identity into the Usage indexes so summaries, repair, and provenance are scoped before decoding, while keeping timeline and summary refresh policies independent. Remove window-relative turn numbering and move pure page merging to Core. Generated-by: Codex
Keep Session usage refreshes on the Usage authority, distinguish oversized evidence from unreadable records, and use composite Run/Turn identities throughout trace pagination. Generated-by: Codex
Append one earlier page per user request, keep Usage invalidations live across Host epochs, and preserve known unreadable evidence in trace coverage. Generated-by: Codex
Validate pagination cursors, make trace ordering deterministic, keep usage bucket cache accounting consistent, and settle superseded loading state. Generated-by: Codex
Generated-by: Codex
Publish Session usage changes only after durable usage mutations succeed, including pending-reprojection marker creation and removal. Generated-by: Codex
Keep the mainline pricing-key regression fixture valid after Session trace coverage gains oversized-run accounting. Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Replace the independent reprojection marker lifecycle with a durable per-run checkpoint derived from the canonical AgentRun event sequence. Normal accounting and query-time repair now share one bounded catch-up path, preserving unreadable evidence without allowing projection failures to advance progress. Generated-by: Codex
Maintain a rebuildable per-run model-call sequence index in the same transaction as the canonical AgentRun append. Backfill existing runs during migration so Usage catch-up discovers lagging projections without rescanning the complete event history. Generated-by: Codex
|
Rebased again onto the latest The prior exact-head CI was fully green, including Desktop E2E, Storybook smoke, and installed CLI validation. Fresh CI is now running on the rebased head. @hqhq1025, when it is convenient, please re-review the sequence-derived catch-up architecture and dismiss the superseded changes-requested review if the current head addresses the findings. Posted by Codex on behalf of the contributor. |
6f52b7b to
1f50987
Compare
Summary
session_idbefore decoding, so the summary remains Session-wide while the visible timeline stays bounded.usagedomain, and Desktop subscribes synchronously through the current Host epoch instead of inferring cost changes from Session events.(runId, turnId)as the trace identity throughout projection, coverage, merge, and rendering. Distinguish oversized evidence from corrupt/unreadable evidence, and treat either known gap as partial rather than claiming the backend records no call details.This changes user-visible Inspector behavior: long Sessions open normally, summary figures remain Session-wide estimates, each click loads one bounded earlier page, and incomplete accounting is never presented as a known zero or a complete timeline.
Screenshots
The same 944-record, 1.18 MB Session is shown before and after the change.
Additional Storybook verification covers the continuation control above the ascending timeline, one-page loading, stable timestamp labels, and explicit incomplete coverage. A clean isolated run produced no console warnings or errors.
Verification
npm --workspace @maka/core run buildnpm --workspace @maka/runtime run buildnpm --workspace @maka/storage run buildnpm --workspace @maka/runtime-host run buildnpm --workspace @maka/desktop run build:mainnpm --workspace @maka/desktop run typechecknpm run astryx:surface-inventorygit diff --check origin/main...HEADTraceMoreHistory: click-to-load appends the older page and a fresh isolated page reports no console warnings or errors.Review
AI use
Select exactly one:
Tool(s) and scope: Codex diagnosed the failure, shaped and authored the implementation and tests, adjudicated adversarial reviews, resolved the latest-main rebase, and performed focused and visual verification. Claude Fable provided earlier read-only architecture and code reviews. The human contributor must review the final diff, screenshots, and commit messages before merge.
Checklist
Does this PR entail a change in behavior?