fix(agent): give each reasoning span its own AG-UI messageId - #3471
Conversation
Providers restart reasoning part ids at `reasoning-0` on every step, and both AG-UI browser encoders composed the run-global reasoning messageId straight from that part id. Every reasoning span in a multi-step run therefore shared one id. Production run 2f7ae3d4 emitted five REASONING_MESSAGE_START events all keyed `msg-2ba40c86...:reasoning:reasoning-0`, so any consumer keying by messageId merges or overwrites four of the five, and a trace UI cannot tell the five thinking blocks apart. The part id was never a run-global identifier. src/agent/react/use-chat/ streaming/handler.ts:739 already documents the reuse and compensates for it client-side; the encoders did not. A span is now identified by its position in the run. Both encoders count reasoning spans and only advance on a genuine span open, so deltas and ends stay on the id their own span opened rather than recomposing from whatever part id they carry. Ordinals rather than a disambiguating suffix on the part id, because veryfront-api already rebuilds these same events with an ordinal (stream-snapshot-helpers.ts:217 and terminal-public-replay.ts:217). Keeping the part id would have left a run emitting one id live and a different id on replay -- the same class of defect. Nothing parses these ids; they are opaque correlation keys, so the shape is free to change. Historical events keep their colliding ids either way, since no format choice repairs a value that is already ambiguous. lifecycle-browser-adapter carries the identical defect on the flag-gated v2 stream-lifecycle path and is fixed alongside, so the bug does not return when VF_STREAM_LIFECYCLE_MODE flips. Not addressed: veryfront-api indexes into the whole parts array while the encoders count reasoning spans, so the two paths now share a shape but not necessarily a value. Aligning them needs a follow-up.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe AG-UI browser encoder and lifecycle adapter now assign run-scoped ordinal IDs to reasoning spans. Tests cover repeated provider IDs, stable span identities, and unmatched end events. API reference links were updated for shifted source lines. ChangesReasoning span identity
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RuntimeStream
participant LifecycleBrowserAdapter
participant BrowserEncoder
participant AGUIClient
RuntimeStream->>LifecycleBrowserAdapter: reasoning stream event
LifecycleBrowserAdapter->>BrowserEncoder: request reasoning identity
BrowserEncoder->>AGUIClient: emit event with stable ordinal ID
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fef5d99936
ℹ️ 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".
| // Providers restart part ids at `reasoning-0` in every step, so composing the | ||
| // AG-UI id from the part id alone collides across a multi-step run. Observed | ||
| // in production run 2f7ae3d4, where all five reasoning blocks shared | ||
| // `msg-2ba40c86...:reasoning:reasoning-0`. |
There was a problem hiding this comment.
This comment records a production run identifier and part of its message identifier in a test. Replace both with placeholders or a synthetic example so repository history does not retain identifiers from production activity.
AGENTS.md reference: AGENTS.md:L96-L101
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in da9a0ed. Dropped the run id and the message-id prefix; the comment now states the collision shape generically (<messageId>:reasoning:reasoning-0), which is what the test actually pins. Also redacted the same identifiers from the PR description, since AGENTS.md:96 covers those too.
| activeTextContentId: string | null; | ||
| textContentIndex: number; | ||
| reasoningMessageId: string | null; | ||
| reasoningSpanIndex: number; |
There was a problem hiding this comment.
AgUiBrowserEncoderState is re-exported from the public veryfront/agent surface, so making reasoningSpanIndex required breaks downstream callers that construct a previously valid state object and pass it to the public mapping or finalization functions. Make the new counter optional with a zero fallback, or otherwise avoid adding a required property to the public state contract.
AGENTS.md reference: AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in da9a0ed. reasoningSpanIndex is now optional and reads as 0 when absent, so a hand-built state that predates the counter stays valid through the public mapping and finalization functions:
const index = state.reasoningSpanIndex ?? 0;
state.reasoningSpanIndex = index + 1;createAgUiBrowserEncoderState() still seeds it to 0, so the normal path is unchanged. The v2 path in lifecycle-browser-adapter.ts keeps its counter in closure state and never touched a public type, so it needed no change.
…ct ids - reasoningSpanIndex is optional on the public AgUiBrowserEncoderState so a state object built before the counter existed stays valid; absent reads as 0. - Drop the production run and message identifiers from the test comment. - Regenerate docs/api-reference for the shifted source line numbers.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agent/ag-ui/browser-encoder.ts (1)
740-743: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not open a reasoning span for an unmatched end event.
Both paths create a new reasoning ID when they receive an end event without an active span. They then emit
ReasoningMessageEndwithout a matching start. Return no event for an unmatched end. Add regression coverage for this case.
src/agent/ag-ui/browser-encoder.ts#L740-L743: return an empty event list whenstate.reasoningMessageIdisnull.src/agent/ag-ui/lifecycle-browser-adapter.ts#L133-L137: makeendReasoning()return only the active ID. Do not callcontinueReasoning().src/agent/ag-ui/lifecycle-browser-adapter.ts#L201-L204: emitReasoningMessageEndonly whenendReasoning()returns an ID.🤖 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 `@src/agent/ag-ui/browser-encoder.ts` around lines 740 - 743, Prevent unmatched reasoning-end events from producing end events: in src/agent/ag-ui/browser-encoder.ts lines 740-743, return an empty event list when state.reasoningMessageId is null; in src/agent/ag-ui/lifecycle-browser-adapter.ts lines 133-137, update endReasoning() to return only the active ID without calling continueReasoning(); and in lines 201-204, emit ReasoningMessageEnd only when endReasoning() returns an ID. Add regression coverage for unmatched end events.
🤖 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.
Outside diff comments:
In `@src/agent/ag-ui/browser-encoder.ts`:
- Around line 740-743: Prevent unmatched reasoning-end events from producing end
events: in src/agent/ag-ui/browser-encoder.ts lines 740-743, return an empty
event list when state.reasoningMessageId is null; in
src/agent/ag-ui/lifecycle-browser-adapter.ts lines 133-137, update
endReasoning() to return only the active ID without calling continueReasoning();
and in lines 201-204, emit ReasoningMessageEnd only when endReasoning() returns
an ID. Add regression coverage for unmatched end events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3665c267-e798-4742-bfbb-558e1777651b
📒 Files selected for processing (6)
docs/api-reference/veryfront/agent.mdsrc/agent/ag-ui/browser-encoder.test.tssrc/agent/ag-ui/browser-encoder.tssrc/agent/ag-ui/lifecycle-browser-adapter.test.tssrc/agent/ag-ui/lifecycle-browser-adapter.tssrc/internal-agents/ag-ui-sse.test.ts
An unmatched reasoning end fell through to opening a span, so it emitted a ReasoningMessageEnd with no matching start and consumed a span ordinal, shifting every later span's id. Both encoders now report nothing to close. The browser encoder reuses closeOpenReasoningEvent, which already had this guard for the tool-input-start path.
|
@coderabbitai Confirmed and fixed in Verified the finding against the code first:
Regression coverage added to both suites ( One related case I did not change, to keep this diff scoped: a |
|
Tip For best results, initiate chat on the files or code changes.
Would you like me to create a GitHub follow-up issue for the pre-existing ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai Thanks for confirming. No follow-up issue for now — I'd rather not open a tracking issue that the repo owner hasn't asked for. The |
|
Tip For best results, initiate chat on the files or code changes.
🐇 You are interacting with an AI system. |
Found while reconstructing a trace from a production run with five reasoning blocks across five steps. All five blocks carried the same
messageId:Root cause
Both AG-UI browser encoders composed the run-global reasoning
messageIdfrom the provider's part id:Part ids are only unique within a step — providers restart them at
reasoning-0on every step.src/agent/react/use-chat/streaming/handler.ts:739already documents this and compensates client-side; the encoders did not. Any consumer keying bymessageIdmerges or overwrites four of the five spans, and a trace UI cannot tell the thinking blocks apart.The doubled segment in
:reasoning:reasoning-0is the tell — a part id sitting where the code reads as an index.Fix
A span is identified by its position in the run. Both encoders count reasoning spans and only advance on a genuine span open, so deltas and ends stay on the id their own span opened instead of recomposing from whatever part id they carry.
Two call sites, not one —
lifecycle-browser-adapter.tshas the identical defect on the flag-gated v2 stream-lifecycle path, so the bug would return whenVF_STREAM_LIFECYCLE_MODEflips.Why ordinals (wire-format note)
This changes the emitted id shape. Deliberate:
stream-snapshot-helpers.ts:217andterminal-public-replay.ts:217both compose:reasoning:${index}. Keeping the part id would leave a run emitting one id live and a different id on replay — the same class of defect this PR fixes.split(":reasoning:")or equivalent in either repo; they are opaque correlation keys.An earlier revision used a
:2occurrence suffix on the part id to keep the first span byte-identical. Dropped:…:reasoning:reasoning-0:5embeds two competing ordinals and is not a legible id.Not addressed
veryfront-api indexes into the whole parts array while these encoders count reasoning spans, so the two paths now share a shape but not necessarily a value. Aligning them needs a follow-up.
Tests
Red-first, three new tests:
browser-encoder: distinct id per span across 3 stepsbrowser-encoder: delta/end stay on their span's idlifecycle-browser-adapter: distinct id per span across 2 steps["…:reasoning-0","…:reasoning-0"]Existing assertions updated to the ordinal scheme in
browser-encoder.test.tsandinternal-agents/ag-ui-sse.test.ts— each was a first span in a fresh encoder state, so each becomes:reasoning:0.Full suite green via the pre-push gate: 3,776 passed, 0 failed.
deno check/fmt/lintclean.Summary by CodeRabbit
Bug Fixes
Documentation