feat: implement endsTurn behavior for ask-question action and enhance… - #2774
feat: implement endsTurn behavior for ask-question action and enhance…#2774manucorporat wants to merge 2 commits into
Conversation
… guided question handling
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Code Review Summary
PR #2774 adds explicit endsTurn handling for ask-question, prevents later tool calls in the same assistant message from executing after a successful user-blocking action, and scopes agent-written guided-question payloads to the asking chat thread. The overall approach is sound: the loop reports a typed input_required outcome, preserves the existing integration message prefix, and the new tests cover both stopping and non-stopping actions plus thread visibility. This is a standard-risk change because it modifies shared agent-loop behavior and client state synchronization.
Key Findings
🟡 MEDIUM: The guided-question state retention check does not include threadId, so identical questions can remain associated with the previous chat when one thread replaces another.
🧪 Browser testing: Will run after this review (PR touches UI code)
| // user can still answer it by going back to that chat — but it must not | ||
| // render here. | ||
| const visiblePayload = | ||
| payload && payloadBelongsToThread(payload, threadId) ? payload : null; |
There was a problem hiding this comment.
🟡 Include thread identity when retaining a question payload
When setPayload preserves the previous payload, its equality check compares only the questions fingerprint and clientResolveId, not threadId. If another chat writes an otherwise identical question, the old payload can remain associated with the first thread: it is hidden in the new chat and reappears when returning to the old one. Include threadId in the preservation condition and add a replacement-flow regression test.
Additional Info
Found by 1 of 3 code-review agents; confirmed by inspecting the state-retention effect and thread filtering.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest PR head adds the package changeset; the implementation diff for endsTurn, guided-question thread filtering, and chat propagation remains otherwise unchanged. The previously reported thread-retention issue is still present and remains open, so it was intentionally not reposted. This is a standard-risk change because it modifies shared agent-loop control flow, action replay behavior, and user-facing guided-question delivery.
New Findings
🟡 MEDIUM: endsTurn actions can be included in parallel-safe batches, allowing later calls to execute before the yielding action sets the flag.
🟡 MEDIUM: Successful journal/ledger replay paths can bypass the new endsTurn transition and continue a resumed run.
🟡 MEDIUM: Validation failures returned as ordinary strings by ask-question are treated as successful yields, leaving the user waiting for a card that was never persisted.
🟡 MEDIUM: Synthetic tool_start events for skipped calls can cause Slack extraction to select a skipped question instead of the executed one.
🧪 Browser testing: Will run after this review (PR touches UI code)
| if (turnYieldedToUser) { | ||
| await flushParallelBatch(); | ||
| toolResultParts.push(skipToolCallAfterYield(toolCall)); | ||
| continue; |
There was a problem hiding this comment.
🟡 Treat endsTurn actions as parallel-batch boundaries
turnYieldedToUser is checked only while building batches. If an endsTurn action is also marked read-only or parallel-safe, it can be grouped with later compatible calls and Promise.all starts them before the yielding action completes, violating the contract that remaining calls must not execute. Make endsTurn actions a batch boundary or reject incompatible parallel flags, and add a regression test.
Additional Info
New finding from 1 of 3 incremental review agents; the generic exported ActionEntry contract permits these combinations.
| ...(actionEntry.chatUI ? { chatUI: actionEntry.chatUI } : {}), | ||
| }); | ||
| recordToolResult(result, isError); | ||
| if (!isError && actionEntry.endsTurn === true) { |
There was a problem hiding this comment.
🟡 Apply endsTurn when replaying completed actions
Successful journal replay and interrupted-write recovery paths return before this normal execution branch, even though they represent an action that already completed successfully. On a resumed run, a replayed ask-question can therefore continue into another model iteration and issue another question. Centralize the successful endsTurn transition or invoke it in each successful replay/recovery path, with a resume regression test.
Additional Info
New finding from 1 of 3 incremental review agents; replay/recovery paths were traced in production-agent.ts.
| ...(actionEntry.chatUI ? { chatUI: actionEntry.chatUI } : {}), | ||
| }); | ||
| recordToolResult(result, isError); | ||
| if (!isError && actionEntry.endsTurn === true) { |
There was a problem hiding this comment.
🟡 Do not yield when ask-question delivery fails validation
ask-question returns several validation failures as strings (for example, invalid JSON in options) rather than throwing, so isError remains false and this branch reports input_required even though no question card was persisted. Use a typed action error or an explicit successful-delivery signal so malformed calls can be corrected instead of leaving the user waiting for a nonexistent card.
Additional Info
New finding from 1 of 3 incremental review agents; validation-return behavior is in context-tools.ts.
| const result = | ||
| `Not executed: ${toolCall.name} was called after an action that ends the turn. ` + | ||
| `The turn is paused for the user's answer — call it again on a later turn if still needed.`; | ||
| send({ |
There was a problem hiding this comment.
🟡 Do not emit real tool starts for skipped calls
The skip path emits a synthetic tool_start for a call that never ran. Slack’s input-request extraction can select the last ask-question tool start, so when a duplicate question is skipped it may deliver the skipped question text instead of the executed question that actually paused the run. Omit the synthetic start or make extraction select the successful call’s id, and add a duplicate-question Slack regression test.
Additional Info
New finding from 1 of 3 incremental review agents; cross-checked against the skipped-call event construction and Slack extraction behavior.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 participants
… guided question handling