fix(tracing): trace waterfall + summary prompt + chat tab all lose data after each agent turn#895
Conversation
…-finish
Symptom: opening `/traces` mid-session and watching the waterfall view
during agent execution shows the rich trace populating correctly, but
the moment the agent finishes its turn the view collapses to a single
"system-prompt" span — and the data is genuinely lost on disk, not
just from the viewer.
Chain (verified by reading worker.ts + routes/session/index.tsx + tracing.ts):
1. `routes/session/index.tsx` has
`createEffect(() => session()?.workspaceID && sdk.setWorkspace(...))`.
SolidJS dirty-tracks any signal read inside the effect — so it
re-runs on EVERY `session()` change (message count, status, parts
updates), including the cascade at agent-finish.
2. Each fire calls `worker.setWorkspace` via RPC.
3. `worker.setWorkspace` unconditionally calls `startEventStream`.
4. `startEventStream`:
for (const [, trace] of sessionTraces) {
void trace.endTrace().catch(() => {}) // fire-and-forget
}
sessionTraces.clear()
5. On the next event for the same session, `getOrCreateTrace` hits a
cache miss, calls `Trace.create()` + `startTrace(sessionID, {})`,
which pushes a single root span into a freshly-empty `this.spans`
and calls `this.snapshot()`. The snapshot path is derived purely
from sessionID (`tracing.ts:836`), so it overwrites the rich
`ses_<id>.json` with a 1-span file.
Distinct from #867 — that PR fixed intra-Trace-instance concurrency
(snapshot debounce M2; FileExporter ↔ flushSync race M3). This bug is
at the worker-level cache lifecycle: a new Trace instance gets
constructed and its near-empty initial state clobbers the previous
instance's rich state on disk.
Two minimal fixes lock the contract:
* `worker.ts` — make `setWorkspace` idempotent. Track
`currentWorkspaceID` at module scope; early-return when the incoming
value matches. Spurious calls from the UI no longer destroy traces.
* `routes/session/index.tsx` — switch the workspaceID effect to
`createEffect(on(() => session()?.workspaceID, ...))`. The `on()`
projector restricts SolidJS dirty-tracking to that one field, so the
effect only fires when workspaceID actually changes — defense in
depth at the upstream trigger.
Regression test in `test/cli/tui/worker-trace-clearing.test.ts`
locks both contracts via source-grep (the worker-import side has
top-level side effects that make in-process unit testing awkward).
Out of scope (follow-up): `getOrCreateTrace` on cache miss does not
rehydrate `this.spans` from the existing `ses_<id>.json` file. After
the two fixes above, this matters only on worker restart or
MAX_TRACES=100 eviction — both uncommon. Worth tracking as defense in
depth so the disk file is always authoritative.
Typecheck clean. 152 TUI tests pass; 35 existing tracing tests pass
unchanged.
…e Trace every turn
Previous commit on this branch was correct but not load-bearing. The
actual hot path that collapsed the on-disk trace after every agent
turn is in `worker.ts`:
if (event.type === "session.status") {
if (status === "idle" && sid) {
const trace = sessionTraces.get(sid)
if (trace) {
void trace.endTrace().catch(() => {})
sessionTraces.delete(sid) // ← every turn
sessionUserMsgIds.delete(sid)
}
}
}
`session.status === "idle"` fires after every busy→idle transition,
which happens once per turn — not once per session. Each fire ended
the trace AND deleted the cache entry. The next event for the same
session in the next turn hit a cache miss, constructed a fresh
`Trace.create()`, called `startTrace(sessionID, {})` (which pushes a
single root span into empty `this.spans`), and the immediate
`snapshot()` clobbered the rich on-disk `ses_<id>.json` with a 1-span
file.
This also explains the "What was asked / No prompt recorded" symptom:
`metadata.prompt` was captured on the now-destroyed first instance and
never persisted into the replacement.
Fixes in this commit:
* `worker.ts`: removed the destructive `session.status === "idle"`
handler. Sessions in altimate-code are long-lived; the Trace lives
as long as the worker has the session in cache. Finalization
happens on `shutdown` and on MAX_TRACES eviction only — both
already correct.
* `tracing.ts`: new `Trace.rehydrateFromFile(sessionId)` that reads
the existing on-disk file and restores `this.spans`, `this.metadata`,
`this.rootSpanId`, `this.startTime`, counters, and clears the root's
endTime so the trace renders as still-in-progress. Returns true on
success; false on missing/mismatched/malformed file.
* `worker.ts:getOrCreateTrace`: on cache miss, calls
`rehydrateFromFile` before falling back to `startTrace`. Defense in
depth for the worker-restart / MAX_TRACES-eviction paths — even if
some future path destroys the in-memory instance, the new instance
loads disk state instead of overwriting.
Verification
* Behavioral test (`test/altimate/tracing-rehydrate.test.ts`, 4 cases):
proves end-to-end that rehydrate preserves spans+metadata across
Trace-instance reconstruction, returns false on missing/mismatched
files, and clears the root endTime so re-opened traces accept new
events.
* Source-grep regression tests for worker.ts continue to lock the
no-idle-clobber and rehydrate-before-startTrace contracts.
* 449 pass / 0 fail across the full tracing + TUI suites (1 network
flake in tracing-adversarial-2 unrelated to this change, passes on
re-run).
What I traced before declaring done
* Inventoried every `sessionTraces.delete`, `sessionTraces.clear`,
`endTrace()`, `Trace.create()`, `Trace.withExporters()`, and direct
trace-file write across the whole repo.
* Confirmed `this.spans` is never reassigned mid-instance (only
pushed) except by the new `rehydrateFromFile`.
* Confirmed no external callers of `trace.snapshot()` outside
`tracing.ts`.
Post-fix, the only ways a new `Trace` instance can replace an existing
session's in-memory Trace are: worker boot (once), `setWorkspace` with
an actually-changed workspaceID (rare, also idempotency-guarded), and
MAX_TRACES eviction (uncommon at 100 sessions). All three now go
through `rehydrateFromFile` first.
Per codex review (2026-06-05): the previous user-text branch in worker.ts
gated on `part.time?.end` (which user-input parts never have set) AND
called `trace.setTitle(text, text)` which would have regressed the
auto-generated session title from Path C (`session.updated`) back to
the raw user input ('Greeting' → 'hi') if the ordering went sideways.
* New `Trace.setPrompt(prompt)` method that only mutates `metadata.prompt`.
Decouples prompt capture from title mutation.
* Path B in worker.ts now: (a) accepts user text parts without
`part.time?.end` (it's an assistant-side concept only); (b) calls
`setPrompt(text)` only — never `setTitle` — for user-identified
messages. Assistant text still requires `time.end` for `logText`.
* Source-grep regression tests lock both contracts: no more
`part.time?.end` on the user branch, no `setTitle` from the user
branch, `setPrompt` exists and doesn't touch title.
…-turn The chat tab in the trace viewer renders 'metadata.prompt' as a single 'You' bubble at the top, then iterates 'kind: generation' spans for assistant replies. There's no place for any user message beyond the first to land — `setPrompt` overwrites on every call, and the viewer only reads the latest value. Symptom: a 3-turn session shows only the LAST user prompt followed by all earlier assistant responses, with the older user messages dropped. Fix splits the data and the rendering: * New `Trace.logUserMessage(text)` pushes a `kind: 'user-message'` span with the user text as input. Snapshots immediately like other log* methods. * New 'user-message' variant added to the TraceSpan.kind union. * worker.ts Path B: now also calls `logUserMessage(text)` alongside `setPrompt(text)` for user-identified text parts (the first one populates metadata.prompt for the Summary tab; all of them populate per-turn spans for the Chat tab). * viewer.ts chat-view: builds a chronologically sorted list of user-message + generation spans and walks it in startTime order. Older traces without user-message spans fall back to rendering metadata.prompt as before. * Behavioral test in tracing-rehydrate.test.ts proves the spans are written, ordered chronologically, and preserve the user text. 10 unit tests in worker-trace-clearing + 8 in tracing-rehydrate pass; 35 existing tracing tests pass unchanged; typecheck clean.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds disk rehydration and per-turn ChangesTrace Persistence and Multi-turn Lifecycle
Sequence Diagram(s)sequenceDiagram
participant Worker
participant Trace
participant FileExporter
Worker->>Trace: rehydrateFromFile(sessionId)
Trace->>FileExporter: read session JSON
FileExporter-->>Trace: spans + metadata
Trace->>Trace: clear rootSpan.endTime & mark open generation spans interrupted
Worker->>Trace: setPrompt(prompt)
Worker->>Trace: logUserMessage(text)
Trace->>FileExporter: snapshot() (persist updated spans)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![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 (1)
packages/opencode/test/altimate/tracing-rehydrate.test.ts (1)
59-61: 💤 Low valueConsider improving type safety by avoiding
as anycasts.Multiple test cases use
as anycasts to bypass type checking when callinglogToolCall,logText, andlogUserMessage. While this works for testing, it reduces type safety and could hide issues if the actual method signatures change.Consider using
Partial<T>or creating minimal properly-typed test fixtures instead ofanycasts.Example:
// Instead of: original.logToolCall({ tool: "read", state: { status: "completed", input: { f: "a" } } } as any) // Consider: const mockToolCall = { tool: "read", callID: "test-call-1", state: { status: "completed" as const, input: { f: "a" }, output: "", time: { start: Date.now(), end: Date.now() } } } original.logToolCall(mockToolCall)Also applies to: 86-86, 141-144, 166-166, 177-177
🤖 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 `@packages/opencode/test/altimate/tracing-rehydrate.test.ts` around lines 59 - 61, Replace the unsafe "as any" casts in the test by constructing minimal properly-typed fixtures (or using Partial<T>) that conform to the expected parameter types for original.logToolCall, original.logText, and original.logUserMessage; specifically, build small objects that include required fields like tool, callID, state (with status, input, output, time) for logToolCall and include time and text for logText (and message/time for logUserMessage), then pass those typed fixtures into the calls instead of casting—this preserves type safety and will surface signature changes for functions logToolCall, logText, and logUserMessage in the tracing-rehydrate.test.ts tests.
🤖 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 `@packages/opencode/test/altimate/tracing-rehydrate.test.ts`:
- Around line 23-32: The test currently manages a manual tmpDir variable with
beforeEach/afterEach; replace this by removing the tmpDir module variable and
the beforeEach/afterEach hooks and use the fixture tmpdir() helper instead: in
each test, add an "await using tmp = await tmpdir()" (import tmpdir from
fixture/fixture.ts) and reference tmp.path (or the helper's API) where tmpDir
was used so cleanup is automatic when tmp goes out of scope.
In `@packages/opencode/test/cli/tui/worker-trace-clearing.test.ts`:
- Around line 60-67: The current assertions on routeSrc only catch one spelling
of the old inline createEffect; update the negative regexes in
worker-trace-clearing.test.ts (the expect(...) checks using routeSrc) to broadly
reject any inline createEffect arrow form that accesses optional-chained values
or uses short-circuiting (e.g., patterns like createEffect(() =>
session()?.workspaceID && ...), createEffect(() => session()?.workspaceID ? ...
: ...), or createEffect(() => { if (session()?.workspaceID) ... }) ) and also
add guards to reject nested optional-check if-statements inside user-text
branches (the pattern if (part.time?.end) or similar) so the tests fail if code
reverts to any of those buggy shapes; keep positive check that
createEffect(on(() => session()?.workspaceID, ...) ) still appears.
---
Nitpick comments:
In `@packages/opencode/test/altimate/tracing-rehydrate.test.ts`:
- Around line 59-61: Replace the unsafe "as any" casts in the test by
constructing minimal properly-typed fixtures (or using Partial<T>) that conform
to the expected parameter types for original.logToolCall, original.logText, and
original.logUserMessage; specifically, build small objects that include required
fields like tool, callID, state (with status, input, output, time) for
logToolCall and include time and text for logText (and message/time for
logUserMessage), then pass those typed fixtures into the calls instead of
casting—this preserves type safety and will surface signature changes for
functions logToolCall, logText, and logUserMessage in the
tracing-rehydrate.test.ts tests.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d8954327-c7af-42d8-b70b-20de542b2632
📒 Files selected for processing (7)
packages/opencode/specs/trace-bugs-followup-867.mdpackages/opencode/src/altimate/observability/tracing.tspackages/opencode/src/altimate/observability/viewer.tspackages/opencode/src/cli/cmd/tui/routes/session/index.tsxpackages/opencode/src/cli/cmd/tui/worker.tspackages/opencode/test/altimate/tracing-rehydrate.test.tspackages/opencode/test/cli/tui/worker-trace-clearing.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
sahrizvi
changed the title
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
✅ Tests — All PassedTypeScript — passedcc @sahrizvi |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
* Trace.rehydrateFromFile (cubic P2): restore traceId from the on-disk file so post-rehydrate snapshots/exports preserve trace identity across instance lifetimes. Without this, every snapshot after rehydration writes a fresh random traceId. * Trace.rehydrateFromFile (cubic P2): normalize sessionId via the same sanitization buildTraceFile applies before comparing to trace.sessionId. Previously, sessions with /, \, ., or : in the id would be falsely rejected and recreated. * viewer chat-view (cubic P2): always render metadata.prompt at the top unless an existing user-message span carries the same text. Pre-fix traces (only metadata.prompt) and mixed traces (rehydrated pre-fix data + new turns) now render the first user turn correctly. Previously, the fallback was gated on userMsgs.length === 0 and dropped the legacy first turn in mixed traces. * worker-trace-clearing.test.ts (CodeRabbit, cubic P3): broaden the negative regression guards to catch all three flagged bug spellings — inline expression, inline ternary, block body with if — for the workspaceID effect; and to reject part.time?.end nested inside the user-text branch (identified by sessionUserMsgIds.get(...).has(...)). * routes/session/index.tsx: paraphrased the bug-shape literal that was in a comment so the broadened test regex doesn't catch our own documentation as the bug. * tracing-rehydrate.test.ts: behavioral tests for the traceId preservation and sanitized-sessionId match. Skipped CodeRabbit's tmpdir-fixture-style suggestion — the convention isn't followed by the existing tracing tests (tracing-display-crash, tracing-rename-race) so changing this one file alone would be inconsistent and out of scope for this PR. 48 affected tests pass; typecheck clean.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
CodeRabbit pointed out that `packages/opencode/test/AGENTS.md` documents `tmpdir()` from `fixture/fixture.ts` as the project convention. My earlier reply skipped the migration on the grounds that sibling tracing tests don't follow the convention — but for code I'm adding fresh, the right move is to follow the documented standard. The sibling tests predate it and should be swept in a separate cleanup PR. Replaces the manual `os.tmpdir() + beforeEach/afterEach` pattern with `await using tmp = await tmpdir()` per test. Helpers `makeTrace` and `readTraceFile` now take `dir` as a first parameter so each test threads its own tmp directory through. 46 affected tests pass.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
cubic flagged that `Trace.logUserMessage` slices user text at 4000 chars when persisting to the span, while `metadata.prompt` keeps the full string. The strict equality check in viewer chat-view (`u.input === t.metadata.prompt`) misses the dedupe for prompts longer than 4000 chars and renders the same text twice — once as the top-level "You" fallback bubble, once as the first user-message span. Match against both the full and the truncated form. Same fix shape cubic suggested.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
The PR description already carries the bug-by-bug origin table and the explanation of why #867's scope was disjoint from these bugs. Keeping a 295-line spec file in the repo to say the same thing again is bloat — the PR is the right place for this content.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
3 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…uncation constant Three follow-ups from the multi-LLM consensus review of PR #895, codex-validated: * Worker user-text branch now skips parts with `synthetic` or `ignored` set (Major #1 in the review, codex-confirmed). `Session.createUserMessage` in prompt.ts attaches many synthetic text parts to the user messageID for MCP resource banners, decoded file contents, retry/reminder text, plan-mode reminders, and agent-handoff tags. Without the gate they pass the `sessionUserMsgIds.has(messageID)` check, `metadata.prompt` ends up holding the LAST synthetic part (typically a file blob), and the chat tab renders one fake "▶ You" bubble per synthetic span — defeating the two display surfaces this PR fixes. Gated on an `isAuthoredText` predicate so the symmetric assistant-text branch is also protected. Continue via predicate rather than `continue` keyword so the outer event loop still forwards the event downstream via `Rpc.emit`. * `Trace.rehydrateFromFile` now marks any open generation span as interrupted (Major #2). The transient state `logStepStart` populated (`currentGenerationSpanId`, `generationText`, `generationToolCalls`, `pendingToolResults`) is memory-only and can't be reconstructed from disk. If we leave open spans open, the next `step-finish` for that turn drops at the `!this.currentGenerationSpanId` guard and follow-up `logToolCall` mis-parents tool spans to the root, silently degrading the trace shape. Closing the span with `status: "error"` and a `statusMessage` describing the interruption preserves the partial data and makes the boundary visible in the viewer. * Extracted the 4000-char truncation cap as `USER_MESSAGE_INPUT_MAX_CHARS` exported from tracing.ts (new cubic P3 on viewer.ts:1331). The viewer's chat-tab dedupe now interpolates the same constant so the two sides can't drift if the truncation cap ever changes. Also reused a single `Date.now()` call for the user-message span's start/end timestamps (cosmetic, addresses review nit #16). Skipped from the review: - Major #3 (no per-messageID dedupe in logUserMessage): codex confirmed user text doesn't stream/chunk — message.part.delta is assistant-only — so the symptom the review described is subsumed by Major #1's synthetic gate. No separate fix needed. - Major #5 (Path A `setTitle(text, text)` couples title and prompt): codex grep-verified that no in-repo code path populates `userMsg.summary.title` or `summary.body`; the branch is inert. Cleanup risk only, tracked in #896. Tests - New behavioral test in tracing-rehydrate.test.ts asserts that an open generation span (no `step-finish` before reconstruction) ends up with `endTime` set, `status: "error"`, and a statusMessage matching `/interrupted/i` after rehydrate + a snapshot-triggering call. - New source-grep test in worker-trace-clearing.test.ts locks both the synthetic-gate literal (`!part.synthetic && !part.ignored`) and the requirement that both `trace.setPrompt` and `trace.logUserMessage` sit inside the `isAuthoredText &&` guard. 42 affected tests pass; typecheck clean.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/opencode/test/cli/tui/worker-trace-clearing.test.ts`:
- Around line 147-153: The test's regexes match across the whole file and can be
satisfied by unrelated code; update the assertions to scope both checks to the
same guarded block by matching a single guard that contains "!part.synthetic &&
!part.ignored" and then, within that same non-greedy span, require
"isAuthoredText &&" followed by both "trace.setPrompt" and
"trace.logUserMessage" (or better, scope around the "sessionUserMsgIds...has("
branch) so the pattern ensures all these tokens appear inside the same
branch/guard rather than anywhere in the file; modify the
expect(workerSrc).toMatch(...) regex to start at the guard
(!part.synthetic\s*&&\s*!part.ignored) and then use a non-greedy [\s\S]*? to
assert isAuthoredText\s*&&.*?trace\.setPrompt.*?trace\.logUserMessage within
that block.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b46b10de-eca5-4af0-b455-6a99c7e5ee69
📒 Files selected for processing (5)
packages/opencode/src/altimate/observability/tracing.tspackages/opencode/src/altimate/observability/viewer.tspackages/opencode/src/cli/cmd/tui/worker.tspackages/opencode/test/altimate/tracing-rehydrate.test.tspackages/opencode/test/cli/tui/worker-trace-clearing.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/opencode/src/altimate/observability/viewer.ts
- packages/opencode/src/cli/cmd/tui/worker.ts
- packages/opencode/src/altimate/observability/tracing.ts
- packages/opencode/test/altimate/tracing-rehydrate.test.ts
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
Multi-Persona Review — Verdict: ship
The PR implements a well-structured feature to add a Cluster column and enhanced filtering to Databricks Job Run History, following React best practices, API conventions, and defensive rendering patterns. All adversarial tests passed, no critical or high-severity findings were identified, and the codebase hygiene is clean.
15/15 agents completed · 249s · 5 findings (0 critical, 0 high, 0 medium)
Low
- [web-researcher] PR handles cluster_id as string to preserve leading zeros and avoid precision loss, aligning with frontend best practices for non-numeric identifiers. →
apps/web/src/helpers/apis.ts:5712- 💡 Consider adding a type annotation for cluster_id as string in the API contract for clarity.
- [web-researcher] Defensive rendering of null/undefined cluster_name with fallbacks ('-' or cluster_id) and data-testid mitigates potential XSS risks from untrusted data. →
apps/web/src/pages/jobs/components/JobRunHistory.tsx:113- 💡 Consider adding a unit test that explicitly verifies the fallback rendering of null/undefined cluster_name.
- [web-researcher] Use of @altimateai/lego v2.1.0 Stack component with row layout and justify='between' leverages recent accessibility improvements in the design system. →
apps/web/src/components/tables/DataTableHeaderPortals.tsx:25- 💡 Verify that the Stack component's updated spacing behavior is visually consistent across all breakpoints.
- [context-pack] Extracted DataTableHeaderPortals component reduces duplication between JobsTable and JobRunHistory, improving maintainability and consistency. →
apps/web/src/components/tables/DataTableHeaderPortals.tsx:1- 💡 Consider documenting this component in the design system wiki for future reuse.
- [web-researcher] Adding result_states and cluster_ids query parameters aligns with Databricks API conventions for efficient server-side filtering. →
apps/web/src/helpers/apis.ts:5714- 💡 Ensure backend PR AltimateAI/altimate-backend#5455 is merged in sync to avoid API mismatch.
Multi-Persona Review · vllm:qwen3-next-80b (waves) + vllm-fallback (synth) ·
…eRabbit)
CodeRabbit flagged that the previous source-grep assertions matched
across the entire `worker.ts` file, so unrelated code positioning
could satisfy them without the synthetic-gate fix being present in
the actual user-text branch.
* The `isAuthoredText` declaration check now asserts the const is
built from BOTH flags (`!part.synthetic && !part.ignored`), not
just that the literal exists somewhere.
* The two write-path checks now require `trace.setPrompt` and
`trace.logUserMessage` to sit inside the same `if (text) { ... }`
body within the `sessionUserMsgIds...has(part.messageID)` branch.
The `[^{}]` bounds on the inner spans prevent the match from
extending past the closing brace of that body, so calls elsewhere
in the file can't false-green the assertion.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
@anandgupta42 Ready to be merged. |
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/cli/cmd/tui/worker.ts">
<violation number="1" location="packages/opencode/src/cli/cmd/tui/worker.ts:239">
P2: User text may be silently dropped if `message.part.updated` arrives before `message.updated` for the same user message, because `sessionUserMsgIds` won't yet contain the messageID. The else-if assistant branch (`part.time?.end`) won't catch it either, since user text doesn't set `time.end`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if ( | ||
| isAuthoredText && | ||
| part.messageID && | ||
| sessionUserMsgIds.get(part.sessionID)?.has(part.messageID) |
There was a problem hiding this comment.
P2: User text may be silently dropped if message.part.updated arrives before message.updated for the same user message, because sessionUserMsgIds won't yet contain the messageID. The else-if assistant branch (part.time?.end) won't catch it either, since user text doesn't set time.end.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/cli/cmd/tui/worker.ts, line 239:
<comment>User text may be silently dropped if `message.part.updated` arrives before `message.updated` for the same user message, because `sessionUserMsgIds` won't yet contain the messageID. The else-if assistant branch (`part.time?.end`) won't catch it either, since user text doesn't set `time.end`.</comment>
<file context>
@@ -203,16 +211,50 @@ const startEventStream = (input: { directory: string; workspaceID?: string }) =>
+ if (
+ isAuthoredText &&
+ part.messageID &&
+ sessionUserMsgIds.get(part.sessionID)?.has(part.messageID)
+ ) {
const text = String(part.text || "")
</file context>
There was a problem hiding this comment.
Real race in the abstract, but bounded in practice. Producer side: Session.createUserMessage in session/prompt.ts calls Session.updateMessage (which emits message.updated) BEFORE Session.updatePart (which emits message.part.updated) for each text part. Consumer side: the worker reads for await (const event of events.stream) — events are processed in receipt order from a single stream. So as long as the producer ordering holds, sessionUserMsgIds is populated before the part-handler fires.
Not closing the door entirely — if the producer ever reorders (parallel batched emit, retry-on-reconnect, server-side reordering), this would manifest as silently dropped user text. The defensive shape would be to buffer parts by messageID until we see a message.updated that classifies them, then flush. I'd rather land that as a focused follow-up with a real reproducer than speculate the implementation here — happy to open an issue if you'd like.
There was a problem hiding this comment.
Got it — thanks for the clarification.
…hot path cubic flagged that `fsSync.readFileSync` in `rehydrateFromFile` blocks the worker event loop during a trace cache miss. The hot path is bounded (cache miss only fires on worker restart, MAX_TRACES eviction, or initial-boot resumption of a stale session), but a multi-MB trace file makes the pause visible. * `Trace.rehydrateFromFile` now returns `Promise<boolean>` and uses the async `fs.readFile`. * `getOrCreateTrace` in worker.ts becomes `async` and the three call sites in the event-stream loop now `await` it. The loop body was already async (`for await (const event of events.stream)`), so the conversion is local. * Behavioural tests in `tracing-rehydrate.test.ts` converted to `await` the now-async method (7 call sites). * The source-grep contract test for `getOrCreateTrace`'s rehydrate-before-startTrace shape now matches the `await` form. Not addressed in this commit (will reply on PR): - cubic also flagged a theoretical event-ordering race where `message.part.updated` could arrive before `message.updated`, leaving `sessionUserMsgIds` empty when the part handler runs. The producer (`Session.createUserMessage` → `updateMessage` THEN `updatePart` for each part) emits in order, and the consumer reads the event stream sequentially. The race is theoretical given the current producer; if we ever reorder, the defensive fix is to buffer unrouted parts by messageID. Out of scope for this PR.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
3 participants
Closes #896.
Follow-up to #867. Fixes the trace lifecycle and rendering bugs that survived #867's concurrency hardening because they live at a different layer of the system.
What this fixes
session.status === "idle"handler inworker.tscalledendTrace + sessionTraces.deleteafter every turn.idleis a per-turn busy→idle transition, not session-end. Next event hit a cache miss,Trace.create()+startTracesnapshotted an empty 1-span file, clobbering the rich on-disk trace.sessionTraces. Finalization on shutdown + MAX_TRACES eviction only.getOrCreateTracecache-miss path never rehydrated from disk — worker restart / eviction always overwrote the existing trace.Trace.rehydrateFromFile(sessionId)restores spans / metadata / counters / clears rootendTime.getOrCreateTraceprefers it before falling back tostartTrace.worker.setWorkspacewas not idempotent → spurious calls clearedsessionTraces.currentWorkspaceIDat module scope; early-return on unchanged value.routes/session/index.tsxworkspaceID effect re-ran on everysession()signal change.createEffect(on(() => session()?.workspaceID, ...))so SolidJS dirty-tracks onlyworkspaceID.worker.tscould feedmetadata.prompt: (a) the user-rolemessage.updatedhandler only fired ifinfo.summary.title || info.summary.bodywas populated, which doesn't happen on the first turn; (b) the text-part branch ofmessage.part.updatedgated onpart.time?.end, which user-input parts never have set; (c) thesession.updatedhandler calledsetTitle(info.title)with one argument only, never touching prompt. Codex review caught a second risk: dropping thetime.endgate while keepingsetTitle(text, text)would regress the auto-generated session title to raw user input, becausesetTitle(t, p)mutates both fields.Trace.setPrompt(prompt)that only mutatesmetadata.prompt. The user-text branch ofmessage.part.updateddrops thetime.endprecondition and callssetPrompt(text)instead ofsetTitle(text, text).metadata.prompt(singular) at the top + assistant generations. Every user message after the first was dropped from the viewer.Trace.logUserMessage(text)pushes akind: "user-message"span. The user-text branch ofmessage.part.updatedalso calls it for each user-identified text. Viewer chat-view rebuilt to interleave user-message and generation spans instartTimeorder. Backward compatible with traces that lack user-message spans.Why #867 didn't catch these
PR #867 closed M2 (snapshot-debounce dropping events) and M3 (FileExporter racing flushSync). Both are intra-Trace-instance concurrency — within one Trace object's state machine. The reproducers in
tracing-rename-race.test.tsandtracing-display-crash.test.tsall hold one instance for the test's lifetime and exercise crash/burst conditions against that instance.The bugs in this PR are at three other layers:
sessionID. The on-disk file clobber happens via the new instance's first snapshot, not from any race within an old one.Beyond the layer mismatch, the per-turn idle handler (row 1) was intentional code — the original comment read "Finalize trace when session reaches idle (completed)". It was a semantic misunderstanding of what
idlemeans at the event level (per-turn vs session-end), not a concurrency bug, so #867's reproducer matrix wouldn't have surfaced it. And no existing test exercises the full event-stream-to-trace-file path across multiplebusy → idlecycles, so the cross-turn nature was invisible to CI.Independent of #867's scope
Every bug fixed here predates #867 (the prior trace-corruption PR). Listing
this explicitly so reviewers can see this PR is orthogonal work — not a
follow-up cleaning up regressions #867 introduced.
session.status === "idle"destroying the Trace (worker.ts)git log -S 'status === "idle"' -- worker.ts→ introduced by #183 ("feat: replace Langfuse with local-first tracing system"), the original local-tracing PR. Predates #867 by months.getOrCreateTraceno disk rehydration (worker.ts)getOrCreateTracewithout rehydration. #867 never touchedworker.ts.setWorkspacenot idempotent (worker.ts)worker.ts.routes/session/index.tsx)worker.ts)message.updateduser-role branch,message.part.updatedtext branch,session.updated) live inworker.ts, never touched by #867.tracing.tskind union,viewer.tschat-view)tracing.tsbut only added M2/M3 concurrency fixes — did NOT change thekindunion, did NOT touchviewer.ts(zero changes to the viewer file in the #867 commits).Tests
test/cli/tui/worker-trace-clearing.test.ts— source-grep contract tests: no-idle-clobber, rehydrate-before-startTrace,setPrompt-not-setTitle,Trace.setPromptpurity.test/altimate/tracing-rehydrate.test.ts— behavioral tests forrehydrateFromFile(preserve spans/metadata/counters, reject missing/mismatched/malformed, clear endTime on rehydrate) andlogUserMessage(chronological ordering).47 affected tests pass. Typecheck clean. 35 existing tracing tests (
tracing-display-crash,tracing-rename-race) pass unchanged.Verification on a live build
Built locally as
0.0.0-fix/trace-clearing-on-workspace-set-202606051512. Confirmed against a real multi-turn TUI session that:busy → idletransitions (5+ spans, full metadata, real counter values where v0.8.3 produced a 1-span /metadata: {}/ null-summary file).Out of scope
session.statusidle events through the real worker event loop. Tracked separately as a follow-up.message.updatedhandler in worker.ts still callssetTitle(title, title)withsummary.title || summary.body. SamesetTitle(x, x)shape Codex caught for the user-text branch in this PR; if the title-agent's summary update lands aftersession.updated, it can overwritemetadata.promptwith the title-agent's text instead of the user's actual input. Smaller surface than the bug fixed here; not addressed in this PR.🤖 Generated with Claude Code
Summary by cubic
Fixes trace lifecycle bugs that overwrote traces after each turn and preserves trace identity across restarts. Also makes the chat tab show multi-turn conversations, improves prompt capture (skipping synthetic parts), and prevents duplicate top-level prompts for long messages.
Bug Fixes
session.status === "idle"; traces persist across turns and only finalize on shutdown or eviction.Trace.rehydrateFromFile(sessionId)and call it beforestartTraceon cache miss; preserves spans/metadata/counters/traceId, normalizes session ID, clears rootendTime, and marks any open generation spans as “interrupted” to keep structure.setWorkspaceidempotent viacurrentWorkspaceID; UI effect now useson(() => session()?.workspaceID, ...)so it fires only on real changes, preventingsessionTracesclearing.Trace.setPrompt(prompt)and dropped thetime.endgate for user text; never callsetTitle(text, text)and skipsynthetic/ignoredtext parts so prompts stay accurate.Trace.logUserMessage(text)and updated the viewer to interleaveuser-messageandgenerationspans by time; falls back tometadata.promptwhen needed and dedupes using the sharedUSER_MESSAGE_INPUT_MAX_CHARScap to avoid duplicate long prompts.Refactors
Trace.rehydrateFromFileasync and awaited it ingetOrCreateTraceand stream handlers to avoid blocking the event loop on cache misses; tests updated accordingly.tracing-rehydratetests to thetmpdir()fixture for consistency.worker-trace-clearingtest regex to be scope-bounded, ensuring thesynthetic/ignoredgate andsetPrompt/logUserMessagechecks apply to the correct branch.specs/trace-bugs-followup-867.mdto avoid duplicated documentation.Written for commit 7f71154. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests