fix: preserve headless tool-call failure truth - #116
Merged
Merged
Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 1 · 🟡 4 · ⚪ 2 · 0/7 resolved
🤖 Fix all 7 open findings with your agent📋 Out-of-diff findings (7)
Reviewed 6 files · 0 inline · view all 7 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
byapparov
force-pushed
the
backport/headless-truth
branch
from
September 30, 2026 14:00
532f00f to
4a96e51
Compare
Contributor
Author
Review response — PR #116All seven findings from the latest automated review have a verdict. Six are fixed in Issues addressed (pushed to this PR)
Also aligned with #122 (delivered tool calls): a local tool call that ended with partial pending or running input and a synthetic Verification: Review claims verified false (no change needed)
Not addressed here
|
| }, | ||
| }, | ||
| metadata: value.providerMetadata, | ||
| metadata: { |
There was a problem hiding this comment.
🟡 Persisted providerExecuted key skips SDK/share consumers.
Suggested change
| metadata: { | |
| Audit part.metadata consumers: confirm SDK generated types (packages/sdk/src/gen/types.gen.ts) and share/export part schemas treat metadata as an open Record<string, JSONValue>, regenerating types if needed; if the flag is meant to stay internal, consider stripping it (like toolProviderMetadata does) or moving it to a dedicated typed field on ToolPart instead of the generic metadata bag. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #116, packages/cli/src/session/processor.ts:189-192):
Problem: Persisted providerExecuted key skips SDK/share consumers
Detail: Every new ToolPart now persists metadata.providerExecuted, but the coupled neighbours that consume persisted part metadata are untouched in this PR: packages/sdk/src/gen/types.gen.ts is a weight-1.0 co-change partner of both edited files in recent PRs, and share/export surfaces (e.g. src/share/share-next.ts, which imports MessageV2) serialize part metadata verbatim. If any of those schemas type ToolPart.metadata as a closed shape rather than an open record, serialized sessions carrying the new key fail validation; at minimum an internal execution flag is now exposed on shared/exported session payloads with no scrubbing (toolProviderMetadata strips it only at the toModelMessages boundary).
Suggested fix: Audit part.metadata consumers: confirm SDK generated types (packages/sdk/src/gen/types.gen.ts) and share/export part schemas treat metadata as an open Record<string, JSONValue>, regenerating types if needed; if the flag is meant to stay internal, consider stripping it (like toolProviderMetadata does) or moving it to a dedicated typed field on ToolPart instead of the generic metadata bag.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Every new ToolPart now persists metadata.providerExecuted, but the coupled neighbours that consume persisted part metadata are untouched in this PR: packages/sdk/src/gen/types.gen.ts is a weight-1.0 co-change partner of both edited files in recent PRs, and share/export surfaces (e.g. src/share/share-next.ts, which imports MessageV2) serialize part metadata verbatim. If any of those schemas type ToolPart.metadata as a closed shape rather than an open record, serialized sessions carrying the new key fail validation; at minimum an internal execution flag is now exposed on shared/exported session payloads with no scrubbing (toolProviderMetadata strips it only at the toModelMessages boundary).
metadata: {
...value.providerMetadata,
[MessageV2.PROVIDER_EXECUTED_METADATA_KEY]: value.providerExecuted === true,
},
})
toolcalls[value.toolCallId] = part as MessageV2.ToolPart
parts.add(part.id)| return parts.some( | ||
| (part) => | ||
| part.type === "tool" && | ||
| part.metadata?.[MessageV2.PROVIDER_EXECUTED_METADATA_KEY] !== true && |
There was a problem hiding this comment.
🟡 Legacy parts default to client-executed in hasToolCalls.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #116, packages/cli/src/session/prompt.ts:69-75):
Problem: Legacy parts default to client-executed in hasToolCalls
Detail: The gate reads part.metadata?.[PROVIDER_EXECUTED_METADATA_KEY] with two different comparisons: `!== true` for the completed branch but `=== false` for the aborted branch. Tool parts persisted before this PR never carry the key (metadata was just value.providerMetadata), so a legacy completed tool part is always classified as client-executed. On resume/retry of a pre-PR session whose last assistant message has a terminal finish plus completed tool parts, the top-of-loop check that previously exited now returns hasToolCalls=true and triggers an extra model turn that re-sends tool results — including results for tools the provider already executed server-side, which some providers reject. The undefined-vs-false asymmetry between the two branches is implicit rather than a documented tri-state decision.
Suggested fix: Make the tri-state explicit: normalize once (`const executed = part.metadata?.[MessageV2.PROVIDER_EXECUTED_METADATA_KEY]`) and decide deliberately how `undefined` (legacy parts) behaves per status branch — e.g. treat undefined as client-executed for completed parts only if that resume behavior is intended, otherwise fall back to the pre-PR finish-only exit for parts without the marker.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The gate reads part.metadata?.[PROVIDER_EXECUTED_METADATA_KEY] with two different comparisons: !== true for the completed branch but === false for the aborted branch. Tool parts persisted before this PR never carry the key (metadata was just value.providerMetadata), so a legacy completed tool part is always classified as client-executed. On resume/retry of a pre-PR session whose last assistant message has a terminal finish plus completed tool parts, the top-of-loop check that previously exited now returns hasToolCalls=true and triggers an extra model turn that re-sends tool results — including results for tools the provider already executed server-side, which some providers reject. The undefined-vs-false asymmetry between the two branches is implicit rather than a documented tri-state decision.
export function hasToolCalls(parts: MessageV2.Part[]) {
return parts.some(
(part) =>
part.type === "tool" &&
part.metadata?.[MessageV2.PROVIDER_EXECUTED_METADATA_KEY] !== true &&
(part.state.status === "completed" ||
(part.state.status === "error" &&
(part.state.error !== MessageV2.TOOL_EXECUTION_ABORTED ||
part.metadata?.[MessageV2.PROVIDER_EXECUTED_METADATA_KEY] === false))),
)
}| ? data.message | ||
| : result.info.error.name | ||
| throw new Error(`Subagent failed (task_id: ${sessionID}): ${message}`) | ||
| } |
There was a problem hiding this comment.
🟡 Trailing orphan error part discards valid child text.
Suggested change
| } | |
| Only throw when the errored tool part has no subsequent text answer in the message (or tolerate parts whose error is MessageV2.TOOL_EXECUTION_ABORTED, mirroring hasToolCalls), otherwise return the text — optionally annotated with the failure — so the parent keeps the child's answer while still surfacing genuine failures. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #116, packages/cli/src/tool/task.ts:36-39):
Problem: Trailing orphan error part discards valid child text
Detail: taskResultText hard-fails the subagent when the child's final message contains ANY error-status tool part, even when the same message also carries the model's textual answer. The prompt layer added in this PR treats exactly such a case as a clean stop: an orphan partial tool input (tool-input-start with no tool-call, finish "stop") is persisted as an errored part while the loop exits after one model call with no message.error — the PR's own prompt-tool-loop test pins that behavior. The two layers therefore disagree: prompt says "terminal but clean", TaskTool throws "Subagent failed (task_id: …)", and the parent model loses the child's valid text for what may be a provider truncation glitch.
Suggested fix: Only throw when the errored tool part has no subsequent text answer in the message (or tolerate parts whose error is MessageV2.TOOL_EXECUTION_ABORTED, mirroring hasToolCalls), otherwise return the text — optionally annotated with the failure — so the parent keeps the child's answer while still surfacing genuine failures.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
taskResultText hard-fails the subagent when the child's final message contains ANY error-status tool part, even when the same message also carries the model's textual answer. The prompt layer added in this PR treats exactly such a case as a clean stop: an orphan partial tool input (tool-input-start with no tool-call, finish "stop") is persisted as an errored part while the loop exits after one model call with no message.error — the PR's own prompt-tool-loop test pins that behavior. The two layers therefore disagree: prompt says "terminal but clean", TaskTool throws "Subagent failed (task_id: …)", and the parent model loses the child's valid text for what may be a provider truncation glitch.
const failed = result.parts.findLast((part) => part.type === "tool" && part.state.status === "error")
if (failed?.type === "tool" && failed.state.status === "error") {
throw new Error(`Subagent failed (task_id: ${sessionID}): ${failed.state.error}`)
}
return result.parts.findLast((part) => part.type === "text")?.text ?? ""
}| return result.parts.findLast((part) => part.type === "text")?.text ?? "" | ||
| } | ||
|
|
||
| export async function completeTask(result: MessageV2.WithParts, subagentSessionID: string, parentSessionID: string) { |
There was a problem hiding this comment.
⚪ Last-text extraction duplicated in task helpers.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #116, packages/cli/src/tool/task.ts:44-46):
Problem: Last-text extraction duplicated in task helpers
Detail: completeTask computes `result.parts.findLast((part) => part.type === "text")?.text ?? ""` for the plugin payload, then delegates to taskResultText which repeats the identical expression for its return value. The two copies can drift if text selection ever changes (e.g. trimming or filtering empty texts) and the duplication is avoidable.
Suggested fix: Extract a single helper (e.g. `const lastText = (parts) => parts.findLast((p) => p.type === "text")?.text ?? ""`) and use it in both completeTask and taskResultText, or have taskResultText accept the precomputed text.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
completeTask computes result.parts.findLast((part) => part.type === "text")?.text ?? "" for the plugin payload, then delegates to taskResultText which repeats the identical expression for its return value. The two copies can drift if text selection ever changes (e.g. trimming or filtering empty texts) and the duplication is avoidable.
export async function completeTask(result: MessageV2.WithParts, subagentSessionID: string, parentSessionID: string) {
const text = result.parts.findLast((part) => part.type === "text")?.text ?? ""
await Plugin.trigger("agent.subtask.complete", { subagentSessionID, parentSessionID }, { result: text })
return taskResultText(result, subagentSessionID)
}
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 3 · ⚪ 1 · 0/4 resolved
🤖 Fix all 4 open findings with your agent📋 Out-of-diff findings (4)
Reviewed 7 files · 0 inline · view all 4 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #89
Intent
A model can return a terminal-looking stop after a local tool call, and a child task can fail while its parent only sees empty text. Those cases can terminate headless execution with incomplete truth.
Expected Impact on Users
Headless runs continue after ordinary local tool calls when structured output is still pending, and failed child tasks are surfaced to the parent instead of being presented as successful empty output.
Expected Outcomes
Implementation
TaskToolresults while retaining the existing success output shape.Scope Caveat
This prepares the malformed-tool-call recovery work in #111; it does not ship automatic retries or claim live recovery effectiveness.
Test Plan
Verification
Risks and Rollout
The change is limited to headless continuation and child-task error propagation. No event schema removal or retry policy change is involved.