Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
|
|
||
| export function taskResultText(result: MessageV2.WithParts, sessionID: string) { | ||
| if (result.info.role === "assistant" && result.info.error) { | ||
| const data = result.info.error.data |
There was a problem hiding this comment.
🟠 taskResultText crashes when error.data is undefined.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #116, packages/cli/src/tool/task.ts:30-31):
Problem: taskResultText crashes when error.data is undefined
Detail: taskResultText reads `const data = result.info.error.data` and then applies `"message" in data` without guarding against `data` being undefined or a non-object. Persisted child errors whose serialized shape lacks a `data` field (or carries a non-object data) make the `in` operator throw `TypeError: Cannot use 'in' operator`, replacing the intended "Subagent failed (task_id: ...): <reason>" message with a confusing TypeError — undermining this PR's goal of surfacing the real child failure cause. The added test only covers `MessageV2.APIError.toObject()`, which happens to include `data`, so the gap is untested. Repro: given a subagent whose final assistant message persists an error without a `data` payload, when the parent runs the Task tool, then taskResultText throws the raw TypeError instead of the child's actual error name/message.
Suggested fix: Guard the operand before using `in`: `const data = result.info.error.data as { message?: string } | undefined` then `const message = data && typeof data.message === "string" ? data.message : result.info.error.name` (or `"message" in (data ?? {})`). Add a test for an error serialized without `data`.
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 reads const data = result.info.error.data and then applies "message" in data without guarding against data being undefined or a non-object. Persisted child errors whose serialized shape lacks a data field (or carries a non-object data) make the in operator throw TypeError: Cannot use 'in' operator, replacing the intended "Subagent failed (task_id: ...): " message with a confusing TypeError — undermining this PR's goal of surfacing the real child failure cause. The added test only covers MessageV2.APIError.toObject(), which happens to include data, so the gap is untested. Repro: given a subagent whose final assistant message persists an error without a data payload, when the parent runs the Task tool, then taskResultText throws the raw TypeError instead of the child's actual error name/message.
export function taskResultText(result: MessageV2.WithParts, sessionID: string) {
if (result.info.role === "assistant" && result.info.error) {
const data = result.info.error.data
const message = "message" in data && typeof data.message === "string" ? data.message : result.info.error.name
throw new Error(`Subagent failed (task_id: ${sessionID}): ${message}`)
}
const failed = result.parts.findLast((part) => part.type === "tool" && part.state.status === "error")| }, | ||
| }, | ||
| metadata: value.providerMetadata, | ||
| metadata: { |
There was a problem hiding this comment.
🟡 providerExecuted magic string lacks shared constant.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #116, packages/cli/src/session/processor.ts:148-150):
Problem: providerExecuted magic string lacks shared constant
Detail: The `providerExecuted` key is written into tool-part metadata in processor.ts and read back with a magic string in prompt.ts (`part.metadata?.providerExecuted`, packages/cli/src/session/prompt.ts:66), creating an implicit cross-file contract with no shared constant or typed field. The new tests also hardcode the string. A typo or rename on either side would silently disable the headless-truth logic with no compiler error.
Suggested fix: Export a shared constant (e.g. `MessageV2.PROVIDER_EXECUTED_METADATA_KEY = "providerExecuted"`) or add a typed optional field on ToolPart, and use it in both processor.ts (write), prompt.ts (read), and the tests.
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 providerExecuted key is written into tool-part metadata in processor.ts and read back with a magic string in prompt.ts (part.metadata?.providerExecuted, packages/cli/src/session/prompt.ts:66), creating an implicit cross-file contract with no shared constant or typed field. The new tests also hardcode the string. A typo or rename on either side would silently disable the headless-truth logic with no compiler error.
start: Date.now(),
},
},
metadata: {
...value.providerMetadata,
...(value.providerExecuted ? { providerExecuted: true } : {}),
},
})
toolcalls[value.toolCallId] = part as MessageV2.ToolPart| }, | ||
| }, | ||
| metadata: value.providerMetadata, | ||
| metadata: { |
There was a problem hiding this comment.
🟡 providerMetadata can forge providerExecuted flag.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #116, packages/cli/src/session/processor.ts:148-151):
Problem: providerMetadata can forge providerExecuted flag
Detail: When `value.providerExecuted` is falsy, the persisted metadata is just `{ ...value.providerMetadata }`; a provider-supplied top-level key `providerExecuted` inside providerMetadata survives verbatim. SessionPrompt.hasToolCalls (packages/cli/src/session/prompt.ts:66) then treats the tool call as provider-executed, so the prompt loop can exit without executing the tool or taking the follow-up model turn — silently dropping a local tool call and, in json_schema mode, raising StructuredOutputError instead of continuing. Repro: given a provider adapter that surfaces a top-level `providerExecuted: true` entry in a tool-call part's providerMetadata while the part's own providerExecuted flag is false, when the model streams that local tool call and finishes with reason "stop", then the CLI persists metadata.providerExecuted=true, hasToolCalls returns false, and the loop exits without executing the tool.
Suggested fix: Overwrite the key unconditionally instead of conditionally spreading, e.g. `metadata: { ...value.providerMetadata, providerExecuted: value.providerExecuted === true }` (or strip any incoming `providerExecuted` key from providerMetadata before merging), so the flag can only come from the stream part itself.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
When value.providerExecuted is falsy, the persisted metadata is just { ...value.providerMetadata }; a provider-supplied top-level key providerExecuted inside providerMetadata survives verbatim. SessionPrompt.hasToolCalls (packages/cli/src/session/prompt.ts:66) then treats the tool call as provider-executed, so the prompt loop can exit without executing the tool or taking the follow-up model turn — silently dropping a local tool call and, in json_schema mode, raising StructuredOutputError instead of continuing. Repro: given a provider adapter that surfaces a top-level providerExecuted: true entry in a tool-call part's providerMetadata while the part's own providerExecuted flag is false, when the model streams that local tool call and finishes with reason "stop", then the CLI persists metadata.providerExecuted=true, hasToolCalls returns false, and the loop exits without executing the tool.
start: Date.now(),
},
},
metadata: {
...value.providerMetadata,
...(value.providerExecuted ? { providerExecuted: true } : {}),
},
})
toolcalls[value.toolCallId] = part as MessageV2.ToolPart|
|
||
| // Check if model finished (finish reason is not "tool-calls" or "unknown") | ||
| const modelFinished = processor.message.finish && !["tool-calls", "unknown"].includes(processor.message.finish) | ||
| const hasCurrentToolCalls = hasToolCalls(await MessageV2.parts(processor.message.id)) |
There was a problem hiding this comment.
🟡 Parts query runs even when model not finished.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #116, packages/cli/src/session/prompt.ts:718-720):
Problem: Parts query runs even when model not finished
Detail: hasCurrentToolCalls awaits `MessageV2.parts(processor.message.id)` unconditionally on every iteration of the structured-output retry loop, even when `modelFinished` is false or `processor.message.error` is set and the value is never used. This is inconsistent with the short-circuit `&&` chain it feeds and adds a redundant async persistence read per retry turn.
Suggested fix: Short-circuit inside the condition so the await only runs when the other guards pass: `if (modelFinished && !processor.message.error && !hasToolCalls(await MessageV2.parts(processor.message.id))) { ... }`, or compute hasCurrentToolCalls inside an `if (modelFinished)` guard.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
hasCurrentToolCalls awaits MessageV2.parts(processor.message.id) unconditionally on every iteration of the structured-output retry loop, even when modelFinished is false or processor.message.error is set and the value is never used. This is inconsistent with the short-circuit && chain it feeds and adds a redundant async persistence read per retry turn.
// Check if model finished (finish reason is not "tool-calls" or "unknown")
const modelFinished = processor.message.finish && !["tool-calls", "unknown"].includes(processor.message.finish)
const hasCurrentToolCalls = hasToolCalls(await MessageV2.parts(processor.message.id))
if (modelFinished && !hasCurrentToolCalls && !processor.message.error) {| @@ -150,7 +163,7 @@ export const TaskTool = Tool.define("task", async (ctx) => { | |||
| parts: promptParts, | |||
There was a problem hiding this comment.
🟡 agent.subtask.complete skipped on child failure.
Suggested change
| parts: promptParts, | |
| Trigger `agent.subtask.complete` (with a success/error flag) before throwing, or wrap the taskResultText call so the plugin event fires in a finally block on the failure path. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #116, packages/cli/src/tool/task.ts:163-165):
Problem: agent.subtask.complete skipped on child failure
Detail: taskResultText now throws on child failure before `await Plugin.trigger("agent.subtask.complete", ...)` executes, so plugins subscribed to subtask completion never observe failed subtasks, and any post-trigger accounting on this path is skipped. Previously the event fired for every completed subtask regardless of outcome; now failed subtasks become invisible to plugin-based stats/notifications. Repro: given a plugin subscribed to "agent.subtask.complete", when a child subagent fails (message-level error or error tool part), then taskResultText throws before Plugin.trigger runs and the plugin never receives the event.
Suggested fix: Trigger `agent.subtask.complete` (with a success/error flag) before throwing, or wrap the taskResultText call so the plugin event fires in a finally block on the failure path.
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 now throws on child failure before await Plugin.trigger("agent.subtask.complete", ...) executes, so plugins subscribed to subtask completion never observe failed subtasks, and any post-trigger accounting on this path is skipped. Previously the event fired for every completed subtask regardless of outcome; now failed subtasks become invisible to plugin-based stats/notifications. Repro: given a plugin subscribed to "agent.subtask.complete", when a child subagent fails (message-level error or error tool part), then taskResultText throws before Plugin.trigger runs and the plugin never receives the event.
parts: promptParts,
})
const text = taskResultText(result, session.id)
await Plugin.trigger(
"agent.subtask.complete",| @@ -709,8 +715,9 @@ export namespace SessionPrompt { | |||
|
|
|||
| // Check if model finished (finish reason is not "tool-calls" or "unknown") | |||
| const modelFinished = processor.message.finish && !["tool-calls", "unknown"].includes(processor.message.finish) | |||
There was a problem hiding this comment.
⚪ Finish-reason filter duplicated; extract helper.
Suggested change
| const modelFinished = processor.message.finish && !["tool-calls", "unknown"].includes(processor.message.finish) | |
| Add `export function isModelFinished(finish?: string) { return !!finish && !["tool-calls", "unknown"].includes(finish) }` next to hasToolCalls and use it in both the loop-exit condition and the modelFinished const. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #116, packages/cli/src/session/prompt.ts:717):
Problem: Finish-reason filter duplicated; extract helper
Detail: This PR extracts hasToolCalls as a shared helper, but the adjacent finish-reason filter `!["tool-calls", "unknown"].includes(finish)` still appears twice in prompt.ts (the loop-exit condition around line 342 and the modelFinished computation at line 717). Extracting both predicates keeps the two exit paths symmetric and single-sourced; the two sites must stay in agreement for the loop logic to be correct.
Suggested fix: Add `export function isModelFinished(finish?: string) { return !!finish && !["tool-calls", "unknown"].includes(finish) }` next to hasToolCalls and use it in both the loop-exit condition and the modelFinished const.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
This PR extracts hasToolCalls as a shared helper, but the adjacent finish-reason filter !["tool-calls", "unknown"].includes(finish) still appears twice in prompt.ts (the loop-exit condition around line 342 and the modelFinished computation at line 717). Extracting both predicates keeps the two exit paths symmetric and single-sourced; the two sites must stay in agreement for the loop logic to be correct.
// Check if model finished (finish reason is not "tool-calls" or "unknown")
const modelFinished = processor.message.finish && !["tool-calls", "unknown"].includes(processor.message.finish)
const hasCurrentToolCalls = hasToolCalls(await MessageV2.parts(processor.message.id))
if (modelFinished && !hasCurrentToolCalls && !processor.message.error) {| const message = "message" in data && typeof data.message === "string" ? data.message : result.info.error.name | ||
| throw new Error(`Subagent failed (task_id: ${sessionID}): ${message}`) | ||
| } | ||
| const failed = result.parts.findLast((part) => part.type === "tool" && part.state.status === "error") |
There was a problem hiding this comment.
⚪ Duplicated tool error check; use type predicate.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #116, packages/cli/src/tool/task.ts:34-35):
Problem: Duplicated tool error check; use type predicate
Detail: The findLast predicate and the immediately following if condition repeat the same compound check (`part.type === "tool" && part.state.status === "error"`) purely for TypeScript narrowing. A type predicate on findLast removes the duplicated condition.
Suggested fix: Use a type-guard predicate: `const failed = result.parts.findLast((part): part is MessageV2.ToolPart => part.type === "tool" && part.state.status === "error")` then `if (failed) { ... }`.
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 findLast predicate and the immediately following if condition repeat the same compound check (part.type === "tool" && part.state.status === "error") purely for TypeScript narrowing. A type predicate on findLast removes the duplicated condition.
throw new Error(`Subagent failed (task_id: ${sessionID}): ${message}`)
}
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 ?? ""
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 |
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.