Fix chat tool result continuation race#175
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 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="src/server/aiChat.ts">
<violation number="1" location="src/server/aiChat.ts:482">
P3: Streaming-part normalization logic is duplicated instead of reusing `finalizeStreamingParts`, creating avoidable divergence risk.</violation>
</file>
<file name="src/components/chat/ChatSession.tsx">
<violation number="1" location="src/components/chat/ChatSession.tsx:345">
P1: Persistence failures in the success branch are misclassified as compilation failures because `onToolOutput` rejection is not isolated.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } catch (persistError) { | ||
| console.warn('Failed to persist tool output to DB:', persistError); | ||
| } | ||
| await onToolOutput(assistant.id, nextParts); |
There was a problem hiding this comment.
P1: Persistence failures in the success branch are misclassified as compilation failures because onToolOutput rejection is not isolated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/chat/ChatSession.tsx, line 345:
<comment>Persistence failures in the success branch are misclassified as compilation failures because `onToolOutput` rejection is not isolated.</comment>
<file context>
@@ -341,11 +342,7 @@ export function ChatSession({
- } catch (persistError) {
- console.warn('Failed to persist tool output to DB:', persistError);
- }
+ await onToolOutput(assistant.id, nextParts);
}
</file context>
| await onToolOutput(assistant.id, nextParts); | |
| try { | |
| await onToolOutput(assistant.id, nextParts); | |
| } catch (persistError) { | |
| console.warn('Failed to persist tool output to DB:', persistError); | |
| } |
| }; | ||
| } | ||
| if ( | ||
| (part.type === 'reasoning' || part.type === 'text') && |
There was a problem hiding this comment.
P3: Streaming-part normalization logic is duplicated instead of reusing finalizeStreamingParts, creating avoidable divergence risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/server/aiChat.ts, line 482:
<comment>Streaming-part normalization logic is duplicated instead of reusing `finalizeStreamingParts`, creating avoidable divergence risk.</comment>
<file context>
@@ -459,6 +459,39 @@ function finalizeStreamingParts(
+ };
+ }
+ if (
+ (part.type === 'reasoning' || part.type === 'text') &&
+ part.state === 'streaming'
+ ) {
</file context>
Greptile SummaryThis PR fixes a race condition where
Confidence Score: 3/5The race fix is correct in the happy path, but removing the persist error guard means any transient DB write failure will freeze the conversation UI with no recovery short of a page refresh. The reordering in finishWithError and the success path correctly addresses the original race. The server-side normalizeIncompleteToolCalls is a solid defensive addition. However, dropping the try/catch without adding equivalent error handling means an onToolOutput failure now silently blocks chat.addToolOutput — the tool call never completes in the SDK view, the spinner hangs indefinitely, and auto-continuation never fires. src/components/chat/ChatSession.tsx — the finishWithError rewrite and the success-path onToolOutput call both need error handling restored so addToolOutput always runs regardless of persist outcome. Important Files Changed
Sequence DiagramsequenceDiagram
participant SDK as AI SDK (Client)
participant TC as handleToolCall
participant DB as onToolOutput (DB)
participant UI as chat.addToolOutput
Note over TC: Tool call fires (model done streaming input)
TC->>DB: await onToolOutput(assistant.id, nextParts)
alt DB persist succeeds
DB-->>TC: ok
TC->>UI: chat.addToolOutput(...)
UI-->>SDK: triggers auto-continuation
else DB persist throws (NEW: unhandled)
DB-->>TC: throws
Note over TC,UI: addToolOutput never called — UI stuck
end
Note over SDK: Next request to server
SDK->>SDK: normalizeIncompleteToolCalls(branchMessages)
Note over SDK: input-available → output-error (server safety net)
Reviews (1): Last reviewed commit: "Fix chat tool result continuation race" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (assistant && nextParts) { | ||
| await onToolOutput(assistant.id, nextParts); | ||
| } catch (persistError) { | ||
| console.warn('Failed to persist tool error to DB:', persistError); | ||
| } | ||
|
|
||
| chat.addToolOutput({ | ||
| state: 'output-error', | ||
| tool: 'build_parametric_model', | ||
| toolCallId: toolCall.toolCallId, | ||
| errorText, | ||
| }); |
There was a problem hiding this comment.
addToolOutput silently skipped on persist failure
If onToolOutput throws (e.g., a transient network or DB error), the await propagates out of finishWithError and chat.addToolOutput is never called. The SDK's tool call stays in input-available forever, the spinner never clears, and auto-continuation never fires — the conversation is permanently stuck until the user refreshes. The old try/catch existed to ensure the UI always advances regardless of persist success. Removing it trades the race bug for a stuck-UI bug under any persist failure. The correct fix is to persist first but still call addToolOutput in the catch branch (so the UI always unblocks), relying on the new server-side normalizeIncompleteToolCalls to cover any gap in the persisted state.
| }) as AppUIMessage['parts']; | ||
|
|
||
| return dirty ? { ...message, parts } : message; |
There was a problem hiding this comment.
parts array is cast with as rather than typed correctly from the start. The team's rule is to avoid as casts and find type-safe alternatives — for example, give the .map() call an explicit return-type annotation or extract a typed mapper function instead of widening after the fact.
| }) as AppUIMessage['parts']; | |
| return dirty ? { ...message, parts } : message; | |
| }); | |
| const typedParts: AppUIMessage['parts'] = parts as AppUIMessage['parts']; | |
| return dirty ? { ...message, parts: typedParts } : message; |
Rule Used: Avoid using 'as' type casting in TypeScript code. ... (source)
Learned From
Adam-CAD/desktop-backend#1
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary by cubic
Fixes a race where chat auto-continuation could run before a tool result was persisted, causing mismatched tool calls and stuck threads. Also hardens server recovery by normalizing incomplete messages before continuing.
onToolOutputbefore callingchat.addToolOutputto ensure the server sees the matching result when auto-continuation triggers.normalizeIncompleteToolCallsto:tool-build_parametric_modelparts stuck atinput-availabletooutput-errorwith a clear message.reasoningandtextparts stuck instreamingtodone.Written for commit fdf2730. Summary will update on new commits.
Review in cubic