-
Notifications
You must be signed in to change notification settings - Fork 88
fix(assistant): skip empty-response nudge when prior turn had text #26164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -411,13 +411,34 @@ export class AgentLoop { | |
| // This can happen when the model fails to produce output after | ||
| // receiving a large tool result. Retry once with a nudge before | ||
| // the message is persisted. | ||
| // | ||
| // Only nudge when the model hasn't already delivered text to the user | ||
| // earlier in this tool-use chain. If a prior assistant turn in history | ||
| // contained visible text (e.g. the model said its piece before calling | ||
| // a side-effect tool like `remember`), an empty follow-up is the model | ||
| // correctly ending its turn — nudging would mislead it into thinking | ||
| // its earlier text didn't land and cause a verbatim re-send. | ||
| const hasVisibleText = response.content.some( | ||
| (block) => block.type === "text" && block.text.trim().length > 0, | ||
| ); | ||
| const priorAssistantHadVisibleText = (() => { | ||
| for (let i = history.length - 1; i >= 0; i--) { | ||
| const msg = history[i]; | ||
| if (msg.role !== "assistant") continue; | ||
| return msg.content.some( | ||
| (block) => | ||
| block.type === "text" && | ||
| typeof (block as { text?: unknown }).text === "string" && | ||
| (block as { text: string }).text.trim().length > 0, | ||
| ); | ||
| } | ||
| return false; | ||
| })(); | ||
| if ( | ||
| !hasVisibleText && | ||
| toolUseBlocks.length === 0 && | ||
| toolUseTurns > 0 && | ||
| !priorAssistantHadVisibleText && | ||
| emptyResponseRetries < MAX_EMPTY_RESPONSE_RETRIES | ||
|
Comment on lines
+441
to
442
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new Useful? React with 👍 / 👎. |
||
| ) { | ||
| emptyResponseRetries++; | ||
|
|
@@ -437,7 +458,12 @@ export class AgentLoop { | |
| continue; | ||
| } | ||
|
|
||
| if (!hasVisibleText && toolUseBlocks.length === 0 && toolUseTurns > 0) { | ||
| if ( | ||
| !hasVisibleText && | ||
| toolUseBlocks.length === 0 && | ||
| toolUseTurns > 0 && | ||
| !priorAssistantHadVisibleText | ||
| ) { | ||
| rlog.error( | ||
| { turn: toolUseTurns, retries: emptyResponseRetries }, | ||
| "Model returned empty response after tool results — retries exhausted", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴
priorAssistantHadVisibleTextonly checks the most recent assistant message instead of all prior onesThe backward scan at
assistant/src/agent/loop.ts:424-436usesreturnas soon as it finds the first (most recent) assistant message. If that message was a tool_use-only turn (no text), the function returnsfalse— even if an earlier assistant turn in the chain had visible text. This contradicts the stated intent in the comment (lines 415-420): "If a prior assistant turn in history contained visible text".Multi-step scenario where the bug manifests
At step 5, the backward scan finds the step-3 assistant message first (tool_use only, no text) and immediately returns
false. The text delivered in step 1 is never checked. The nudge fires incorrectly, potentially causing the model to re-send the step-1 text verbatim — the exact regression this PR aims to prevent.Was this helpful? React with 👍 or 👎 to provide feedback.