fix(core): resume delegated needsApproval approvals in subagent flows#1107
Closed
omeraplak wants to merge 3 commits into
Closed
fix(core): resume delegated needsApproval approvals in subagent flows#1107omeraplak wants to merge 3 commits into
omeraplak wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 317690f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment has been minimized.
This comment has been minimized.
Contributor
📝 WalkthroughWalkthroughAdds Human-in-the-Loop tool-approval flows: agents can emit approval requests, approvals propagate across delegated sub-agent handoffs with preserved parent context, forwarded stream events include approval types, and resumption executes the guarded tool once approval is received. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
end
rect rgba(200,255,200,0.5)
participant ParentAgent
participant SubAgent
end
rect rgba(255,200,200,0.5)
participant ToolApprovalHandler
participant Tool
end
Client->>ParentAgent: send chat / request
ParentAgent->>SubAgent: delegate_task (includes parent tool-context)
SubAgent->>ToolApprovalHandler: evaluate tool call -> needs approval
ToolApprovalHandler-->>SubAgent: approval pending (tool-approval-request)
SubAgent->>ParentAgent: raise ToolDeniedError with approval metadata
ParentAgent->>Client: emit UI part (tool-approval-request)
Client->>ParentAgent: submit UI response (tool-approval-response)
ParentAgent->>SubAgent: resume with approval response
SubAgent->>Tool: execute guarded tool
Tool-->>SubAgent: tool-result
SubAgent->>ParentAgent: forward tool-result (tool-result)
ParentAgent->>Client: final chat response with result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 27 files
Prompt for AI agents (all 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="packages/core/src/agent/subagent/index.ts">
<violation number="1" location="packages/core/src/agent/subagent/index.ts:810">
P2: The `toolApproval` property is smuggled onto `ToolDeniedError` via `Object.assign` but is not part of the class definition. This bypasses TypeScript's type system, making the approval data invisible to consumers and fragile to refactoring. Consider extending `ToolDeniedError` (or creating a subclass like `ToolApprovalPendingError`) with a typed `toolApproval` field instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| httpStatus: 403, | ||
| }); | ||
|
|
||
| Object.assign(error, { |
Contributor
There was a problem hiding this comment.
P2: The toolApproval property is smuggled onto ToolDeniedError via Object.assign but is not part of the class definition. This bypasses TypeScript's type system, making the approval data invisible to consumers and fragile to refactoring. Consider extending ToolDeniedError (or creating a subclass like ToolApprovalPendingError) with a typed toolApproval field instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/agent/subagent/index.ts, line 810:
<comment>The `toolApproval` property is smuggled onto `ToolDeniedError` via `Object.assign` but is not part of the class definition. This bypasses TypeScript's type system, making the approval data invisible to consumers and fragile to refactoring. Consider extending `ToolDeniedError` (or creating a subclass like `ToolApprovalPendingError`) with a typed `toolApproval` field instead.</comment>
<file context>
@@ -683,27 +718,109 @@ ${task}\n\nContext: ${safeStringify(contextObj, { indentation: 2 })}`;
+ httpStatus: 403,
+ });
+
+ Object.assign(error, {
+ toolApproval: {
+ status,
</file context>
Deploying voltagent with
|
| Latest commit: |
317690f
|
| Status: | 🚫 Build failed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
website/docs/agents/overview.md (1)
596-610:⚠️ Potential issue | 🟡 MinorCode example will fail TypeScript compilation until
FullStreamEventForwardingConfig.typesis widened.The
"tool-approval-response"literal on line 599 (insidetypes: [...]) is not assignable toStreamEventTypebecause it is not part of the AI SDK'sTextStreamPartunion. Users copying this example will get a TypeScript error. This will be resolved by the fix suggested intypes.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/docs/agents/overview.md` around lines 596 - 610, The example uses a literal "tool-approval-response" in the FullStreamEventForwardingConfig.types array which isn't included in the SDK's StreamEventType/TextStreamPart union and causes TypeScript errors; update the type union in types.ts (the FullStreamEventForwardingConfig / StreamEventType/TextStreamPart definitions) to include "tool-approval-response" (and any other missing event literals used in examples) so FullStreamEventForwardingConfig.types accepts that string, or alternatively adjust the example to only use existing StreamEventType members; locate the symbol FullStreamEventForwardingConfig.types and the union/type declarations in types.ts and add the missing literal(s) to the union.packages/core/src/agent/types.ts (1)
328-342:⚠️ Potential issue | 🟠 Major
FullStreamEventForwardingConfig.typesfield type is incompatible with documented usage and internal function signatures.The field is typed as
StreamEventType[], which is derived fromTextStreamPart<any>["type"]and excludes"tool-approval-response". However,createMetadataEnrichedStream()instream-metadata-enricher.tsexplicitly acceptsForwardableStreamEventType[]— a union that intentionally includes"tool-approval-response"as a VoltAgent extension.The code in
subagent/index.ts(lines 650–655 and 683–688) passes arrays containing"tool-approval-response"to this function, and the documentation (overview.mdlines 583, 599, 623) instructs users to include"tool-approval-response"in the types array. The current field type prevents users from following the documented pattern without a type error.Change
types?: StreamEventType[]totypes?: ForwardableStreamEventType[]by importingForwardableStreamEventTypefrom./subagent/stream-metadata-enricher.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/agent/types.ts` around lines 328 - 342, The FullStreamEventForwardingConfig.types field is currently typed as StreamEventType[] which excludes the VoltAgent extension "tool-approval-response" used by createMetadataEnrichedStream and by subagent/index.ts; change the declaration of FullStreamEventForwardingConfig (the types?: field) to use ForwardableStreamEventType[] instead, import ForwardableStreamEventType from ./subagent/stream-metadata-enricher, and update any affected references so callers can pass "tool-approval-response" without type errors (ensure createMetadataEnrichedStream and subagent usages remain compatible).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/with-hitl/.gitignore`:
- Around line 1-3: The .gitignore in examples/with-hitl currently lists
node_modules, dist, and .DS_Store but misses .env which may contain sensitive
API keys; update the .gitignore by adding a line for .env so the
examples/with-hitl/.env file is ignored and cannot be accidentally committed
(add ".env" to the existing list).
In `@packages/core/src/agent/agent.ts`:
- Around line 6608-6814: The resolver resolveToolApprovalDecisionFromMessages
currently falls back to matching historical fingerprints and can auto-approve
new requests; thread the current toolCallId through the call chain and limit
fingerprint-based matches to approvals that are tied to the same toolCallId (or
to delegate_task approvals originating from the same active delegate toolCallId)
instead of any history. Concretely: add an extra param toolCallId to
resolveToolApprovalDecisionFromMessages (and mirror it in
resolveToolApprovalDecisionFromUIMessages and ensureToolApproval), then when
building decisionByFingerprint/decisionByDelegateInputFingerprint only insert or
consider decisions whose approvalId maps to the same toolCallId via
approvalIdToToolCallId (or whose sourceToolName is "delegate_task" and
approvalIdToToolCallId equals the active delegate toolCallId). This uses
existing maps approvalIdToToolCallId, approvalIdToFingerprint,
approvalIdToInputFingerprint, and approvalIdToToolName to enforce the scoping.
In `@website/recipes/hitl.md`:
- Around line 73-127: Add a brief note that callers must capture the
conversationId returned by the initial POST to /agents/crmHitlAgent/chat (or
include one in the first request's options) and then supply that same
conversationId in the options of the follow-up resume payload (the payload using
the tool-approval-response entry and approval.id) so the approval response
targets the correct conversation; mention extracting conversationId from the
first response if not provided up-front.
---
Outside diff comments:
In `@packages/core/src/agent/types.ts`:
- Around line 328-342: The FullStreamEventForwardingConfig.types field is
currently typed as StreamEventType[] which excludes the VoltAgent extension
"tool-approval-response" used by createMetadataEnrichedStream and by
subagent/index.ts; change the declaration of FullStreamEventForwardingConfig
(the types?: field) to use ForwardableStreamEventType[] instead, import
ForwardableStreamEventType from ./subagent/stream-metadata-enricher, and update
any affected references so callers can pass "tool-approval-response" without
type errors (ensure createMetadataEnrichedStream and subagent usages remain
compatible).
In `@website/docs/agents/overview.md`:
- Around line 596-610: The example uses a literal "tool-approval-response" in
the FullStreamEventForwardingConfig.types array which isn't included in the
SDK's StreamEventType/TextStreamPart union and causes TypeScript errors; update
the type union in types.ts (the FullStreamEventForwardingConfig /
StreamEventType/TextStreamPart definitions) to include "tool-approval-response"
(and any other missing event literals used in examples) so
FullStreamEventForwardingConfig.types accepts that string, or alternatively
adjust the example to only use existing StreamEventType members; locate the
symbol FullStreamEventForwardingConfig.types and the union/type declarations in
types.ts and add the missing literal(s) to the union.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.changeset/curly-rivers-march.mdexamples/README.mdexamples/with-hitl/.env.exampleexamples/with-hitl/.gitignoreexamples/with-hitl/README.mdexamples/with-hitl/package.jsonexamples/with-hitl/src/index.tsexamples/with-hitl/tsconfig.jsonpackages/core/src/agent/agent.spec.tspackages/core/src/agent/agent.tspackages/core/src/agent/subagent/index.spec.tspackages/core/src/agent/subagent/index.tspackages/core/src/agent/subagent/stream-metadata-enricher.tspackages/core/src/agent/types.tspackages/core/src/tool/manager/ToolManager.tspackages/core/src/utils/message-converter.spec.tspackages/core/src/utils/message-converter.tspackages/core/src/utils/tool-approval.tswebsite/docs/agents/overview.mdwebsite/docs/agents/subagents.mdwebsite/docs/agents/tools.mdwebsite/docs/api/endpoints/agents.mdwebsite/recipes/hitl.mdwebsite/recipes/overview.mdwebsite/recipes/tools.mdwebsite/sidebarsRecipes.ts
💤 Files with no reviewable changes (1)
- packages/core/src/tool/manager/ToolManager.ts
Comment on lines
+1
to
+3
| node_modules | ||
| dist | ||
| .DS_Store |
Contributor
There was a problem hiding this comment.
Add .env to ignore list to avoid leaking API keys.
The README instructs users to create examples/with-hitl/.env containing OPENAI_API_KEY. Without ignoring it here, the key can be committed accidentally.
🔒 Suggested fix
node_modules
dist
.DS_Store
+.env📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| node_modules | |
| dist | |
| .DS_Store | |
| node_modules | |
| dist | |
| .DS_Store | |
| .env |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/with-hitl/.gitignore` around lines 1 - 3, The .gitignore in
examples/with-hitl currently lists node_modules, dist, and .DS_Store but misses
.env which may contain sensitive API keys; update the .gitignore by adding a
line for .env so the examples/with-hitl/.env file is ignored and cannot be
accidentally committed (add ".env" to the existing list).
Comment on lines
+6608
to
+6814
| private resolveToolApprovalDecisionFromMessages(params: { | ||
| messages: ModelMessage[]; | ||
| toolName: string; | ||
| args: Record<string, unknown>; | ||
| approvalId: string; | ||
| }): { approved: boolean; reason?: string } | null { | ||
| const { messages, toolName, args, approvalId } = params; | ||
| if (!messages.length) { | ||
| return null; | ||
| } | ||
|
|
||
| const toolCallFingerprintById = new Map<string, string>(); | ||
| const toolCallInputFingerprintById = new Map<string, string>(); | ||
| const toolCallNameById = new Map<string, string>(); | ||
| const approvalIdToToolCallId = new Map<string, string>(); | ||
| const approvalIdToFingerprint = new Map<string, string>(); | ||
| const approvalIdToInputFingerprint = new Map<string, string>(); | ||
| const approvalIdToToolName = new Map<string, string>(); | ||
| const decisionByApprovalId = new Map<string, { approved: boolean; reason?: string }>(); | ||
|
|
||
| for (const message of messages) { | ||
| if (message.role === "assistant") { | ||
| if (!Array.isArray(message.content)) { | ||
| continue; | ||
| } | ||
|
|
||
| for (const rawPart of message.content as unknown[]) { | ||
| if (!isRecord(rawPart)) { | ||
| continue; | ||
| } | ||
|
|
||
| const part = rawPart as Record<string, unknown>; | ||
| const partType = hasNonEmptyString(part.type) ? part.type : null; | ||
| if (partType === "tool-call") { | ||
| const toolCallId = hasNonEmptyString(part.toolCallId) ? part.toolCallId : null; | ||
| const toolCallName = hasNonEmptyString(part.toolName) ? part.toolName : null; | ||
| const input = | ||
| isRecord(part.input) && !Array.isArray(part.input) | ||
| ? (part.input as Record<string, unknown>) | ||
| : {}; | ||
| if (toolCallId && toolCallName) { | ||
| toolCallFingerprintById.set( | ||
| toolCallId, | ||
| createToolApprovalFingerprint(toolCallName, input), | ||
| ); | ||
| toolCallInputFingerprintById.set( | ||
| toolCallId, | ||
| this.createToolApprovalInputFingerprint(input), | ||
| ); | ||
| toolCallNameById.set(toolCallId, toolCallName); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (partType === "tool-approval-request") { | ||
| const approvalRequestId = hasNonEmptyString(part.approvalId) ? part.approvalId : null; | ||
| const toolCallId = hasNonEmptyString(part.toolCallId) ? part.toolCallId : null; | ||
| if (!approvalRequestId || !toolCallId) { | ||
| continue; | ||
| } | ||
|
|
||
| approvalIdToToolCallId.set(approvalRequestId, toolCallId); | ||
| const fingerprint = toolCallFingerprintById.get(toolCallId); | ||
| if (fingerprint) { | ||
| approvalIdToFingerprint.set(approvalRequestId, fingerprint); | ||
| } | ||
| const inputFingerprint = toolCallInputFingerprintById.get(toolCallId); | ||
| if (inputFingerprint) { | ||
| approvalIdToInputFingerprint.set(approvalRequestId, inputFingerprint); | ||
| } | ||
| const toolCallName = toolCallNameById.get(toolCallId); | ||
| if (toolCallName) { | ||
| approvalIdToToolName.set(approvalRequestId, toolCallName); | ||
| } | ||
| } | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (message.role !== "tool" || !Array.isArray(message.content)) { | ||
| continue; | ||
| } | ||
|
|
||
| for (const rawPart of message.content as unknown[]) { | ||
| if (!isRecord(rawPart)) { | ||
| continue; | ||
| } | ||
|
|
||
| const part = rawPart as Record<string, unknown>; | ||
| const partType = hasNonEmptyString(part.type) ? part.type : null; | ||
| if (partType === "tool-result") { | ||
| const approvalOutput = extractToolApprovalOutput(part.output); | ||
| if (!approvalOutput) { | ||
| continue; | ||
| } | ||
|
|
||
| approvalIdToToolCallId.set(approvalOutput.approvalId, approvalOutput.toolCallId); | ||
| approvalIdToFingerprint.set( | ||
| approvalOutput.approvalId, | ||
| createToolApprovalFingerprint(approvalOutput.toolName, approvalOutput.input || {}), | ||
| ); | ||
| approvalIdToInputFingerprint.set( | ||
| approvalOutput.approvalId, | ||
| this.createToolApprovalInputFingerprint(approvalOutput.input || {}), | ||
| ); | ||
| approvalIdToToolName.set(approvalOutput.approvalId, approvalOutput.toolName); | ||
|
|
||
| if (approvalOutput.status === "denied") { | ||
| decisionByApprovalId.set(approvalOutput.approvalId, { | ||
| approved: false, | ||
| reason: approvalOutput.reason, | ||
| }); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (partType !== "tool-approval-response") { | ||
| continue; | ||
| } | ||
|
|
||
| const approvalResponseId = hasNonEmptyString(part.approvalId) ? part.approvalId : null; | ||
| if (!approvalResponseId || typeof part.approved !== "boolean") { | ||
| continue; | ||
| } | ||
|
|
||
| decisionByApprovalId.set(approvalResponseId, { | ||
| approved: part.approved, | ||
| reason: hasNonEmptyString(part.reason) ? part.reason : undefined, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| if (!approvalIdToFingerprint.has(approvalId)) { | ||
| const toolCallId = approvalIdToToolCallId.get(approvalId); | ||
| if (toolCallId) { | ||
| const fingerprint = toolCallFingerprintById.get(toolCallId); | ||
| if (fingerprint) { | ||
| approvalIdToFingerprint.set(approvalId, fingerprint); | ||
| } | ||
| const inputFingerprint = toolCallInputFingerprintById.get(toolCallId); | ||
| if (inputFingerprint) { | ||
| approvalIdToInputFingerprint.set(approvalId, inputFingerprint); | ||
| } | ||
| const toolNameFromCall = toolCallNameById.get(toolCallId); | ||
| if (toolNameFromCall) { | ||
| approvalIdToToolName.set(approvalId, toolNameFromCall); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const directDecision = decisionByApprovalId.get(approvalId); | ||
| if (directDecision) { | ||
| return directDecision; | ||
| } | ||
|
|
||
| const decisionByFingerprint = new Map<string, { approved: boolean; reason?: string }>(); | ||
| const decisionByDelegateInputFingerprint = new Map< | ||
| string, | ||
| { approved: boolean; reason?: string } | ||
| >(); | ||
| for (const [approvalResponseId, decision] of decisionByApprovalId.entries()) { | ||
| if (!approvalIdToFingerprint.has(approvalResponseId)) { | ||
| const toolCallId = approvalIdToToolCallId.get(approvalResponseId); | ||
| if (toolCallId) { | ||
| const fingerprint = toolCallFingerprintById.get(toolCallId); | ||
| if (fingerprint) { | ||
| approvalIdToFingerprint.set(approvalResponseId, fingerprint); | ||
| } | ||
| const inputFingerprint = toolCallInputFingerprintById.get(toolCallId); | ||
| if (inputFingerprint) { | ||
| approvalIdToInputFingerprint.set(approvalResponseId, inputFingerprint); | ||
| } | ||
| const toolNameFromCall = toolCallNameById.get(toolCallId); | ||
| if (toolNameFromCall) { | ||
| approvalIdToToolName.set(approvalResponseId, toolNameFromCall); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const fingerprint = approvalIdToFingerprint.get(approvalResponseId); | ||
| if (fingerprint) { | ||
| decisionByFingerprint.set(fingerprint, decision); | ||
| } | ||
|
|
||
| const inputFingerprint = approvalIdToInputFingerprint.get(approvalResponseId); | ||
| const sourceToolName = approvalIdToToolName.get(approvalResponseId); | ||
| if (inputFingerprint && sourceToolName === "delegate_task") { | ||
| decisionByDelegateInputFingerprint.set(inputFingerprint, decision); | ||
| } | ||
| } | ||
|
|
||
| const currentFingerprint = createToolApprovalFingerprint(toolName, args); | ||
| const directFingerprintDecision = decisionByFingerprint.get(currentFingerprint); | ||
| if (directFingerprintDecision) { | ||
| return directFingerprintDecision; | ||
| } | ||
|
|
||
| if (toolName !== "delegate_task") { | ||
| const currentInputFingerprint = this.createToolApprovalInputFingerprint(args); | ||
| const delegateDecision = decisionByDelegateInputFingerprint.get(currentInputFingerprint); | ||
| if (delegateDecision) { | ||
| return delegateDecision; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
Potential approval bypass via historical fingerprint matches.
When approvalId doesn’t match, the fallback accepts any prior approval response with the same toolName+args (or delegate input). If the conversation history contains an older approval with the same arguments, a new tool call can be auto-approved without explicit user intent. That weakens needsApproval gating and can skip required HITL confirmation.
Consider scoping fingerprint fallback to the current approval request (e.g., only approvals tied to the current toolCallId, or only delegate-task approvals from the active delegate call). One way is to thread toolCallId into the resolver and filter fingerprint matches by that id:
Proposed guard to avoid historical auto-approvals
- private resolveToolApprovalDecisionFromMessages(params: {
- messages: ModelMessage[];
- toolName: string;
- args: Record<string, unknown>;
- approvalId: string;
- }): { approved: boolean; reason?: string } | null {
- const { messages, toolName, args, approvalId } = params;
+ private resolveToolApprovalDecisionFromMessages(params: {
+ messages: ModelMessage[];
+ toolName: string;
+ args: Record<string, unknown>;
+ approvalId: string;
+ toolCallId: string;
+ }): { approved: boolean; reason?: string } | null {
+ const { messages, toolName, args, approvalId, toolCallId } = params;
...
- for (const [approvalResponseId, decision] of decisionByApprovalId.entries()) {
+ for (const [approvalResponseId, decision] of decisionByApprovalId.entries()) {
+ const responseToolCallId = approvalIdToToolCallId.get(approvalResponseId);
+ if (responseToolCallId && responseToolCallId !== toolCallId) {
+ continue;
+ }
const fingerprint = approvalIdToFingerprint.get(approvalResponseId);
if (fingerprint) {
decisionByFingerprint.set(fingerprint, decision);
}
...
}You’d mirror the same toolCallId threading + filter in resolveToolApprovalDecisionFromUIMessages and pass toolCallId from ensureToolApproval.
Also applies to: 6853-6981
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/agent/agent.ts` around lines 6608 - 6814, The resolver
resolveToolApprovalDecisionFromMessages currently falls back to matching
historical fingerprints and can auto-approve new requests; thread the current
toolCallId through the call chain and limit fingerprint-based matches to
approvals that are tied to the same toolCallId (or to delegate_task approvals
originating from the same active delegate toolCallId) instead of any history.
Concretely: add an extra param toolCallId to
resolveToolApprovalDecisionFromMessages (and mirror it in
resolveToolApprovalDecisionFromUIMessages and ensureToolApproval), then when
building decisionByFingerprint/decisionByDelegateInputFingerprint only insert or
consider decisions whose approvalId maps to the same toolCallId via
approvalIdToToolCallId (or whose sourceToolName is "delegate_task" and
approvalIdToToolCallId equals the active delegate toolCallId). This uses
existing maps approvalIdToToolCallId, approvalIdToFingerprint,
approvalIdToInputFingerprint, and approvalIdToToolName to enforce the scoping.
Comment on lines
718
to
+796
| private async resolveStreamResult( | ||
| response: Awaited<ReturnType<Agent["streamText"]>>, | ||
| parentOperationContext?: OperationContext, | ||
| ): Promise<{ finalResult: string; usage: any }> { | ||
| ): Promise<{ | ||
| finalResult: string; | ||
| usage: any; | ||
| pendingApproval: ToolApprovalOutputPayload | null; | ||
| }> { | ||
| const bailedResultFromContext = parentOperationContext?.systemContext?.get("bailedResult") as | ||
| | { agentName: string; response: string } | ||
| | undefined; | ||
| const [pendingApproval, usage] = await Promise.all([ | ||
| this.resolvePendingApprovalFromStream(response), | ||
| response.usage, | ||
| ]); | ||
| const finalResult = bailedResultFromContext?.response || (await response.text); | ||
| const usage = await response.usage; | ||
| return { finalResult, usage }; | ||
| return { finalResult, usage, pendingApproval }; | ||
| } | ||
|
|
||
| private async streamTextWithForwarding( | ||
| targetAgent: Agent, | ||
| messages: UIMessage[], | ||
| options: StreamTextOptions, | ||
| parentOperationContext: OperationContext | undefined, | ||
| ): Promise<{ finalResult: string; usage: any }> { | ||
| ): Promise<{ | ||
| finalResult: string; | ||
| usage: any; | ||
| pendingApproval: ToolApprovalOutputPayload | null; | ||
| }> { | ||
| const response = await targetAgent.streamText(messages, options); | ||
| const forwardingMetadata = this.buildForwardingMetadata(targetAgent, parentOperationContext); | ||
| this.forwardStreamEvents(response, forwardingMetadata, parentOperationContext, targetAgent); | ||
| return this.resolveStreamResult(response, parentOperationContext); | ||
| } | ||
|
|
||
| private async resolvePendingApprovalFromStream( | ||
| response: Awaited<ReturnType<Agent["streamText"]>>, | ||
| ): Promise<ToolApprovalOutputPayload | null> { | ||
| const stepsPromise = ( | ||
| response as { | ||
| steps?: | ||
| | PromiseLike<Array<{ toolResults?: Array<{ output?: unknown }> }> | undefined> | ||
| | undefined; | ||
| } | ||
| ).steps; | ||
|
|
||
| if (!stepsPromise) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| const steps = await stepsPromise; | ||
| if (!Array.isArray(steps) || steps.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| const lastStep = steps.at(-1); | ||
| return this.extractPendingApprovalFromToolResults(lastStep?.toolResults); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| private extractPendingApprovalFromToolResults( | ||
| toolResults: Array<{ output?: unknown }> | undefined, | ||
| ): ToolApprovalOutputPayload | null { | ||
| if (!Array.isArray(toolResults) || toolResults.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| for (const toolResult of toolResults) { | ||
| const approvalOutput = extractToolApprovalOutput(toolResult?.output); | ||
| if (approvalOutput?.status === "requested") { | ||
| return approvalOutput; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
Add fallback approval detection beyond response.steps or document SDK dependency.
resolvePendingApprovalFromStream (lines 753-782) relies solely on response.steps to detect pending approvals. While the code defensively returns null if steps is missing, this creates a silent failure: if the AI SDK's streamText method doesn't populate steps for certain providers or streaming modes, pending approvals won't be detected and delegate_task may proceed without surfacing required approvals.
The code should either:
- Document explicitly which AI SDK versions and providers guarantee
stepswill be populated, or - Implement an alternative approval detection path (e.g., from response metadata, tool execution logs) to ensure approvals are never silently lost.
Comment on lines
+73
to
+127
| ```bash | ||
| curl -N -X POST http://localhost:3141/agents/crmHitlAgent/chat \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"input":"Delete CRM user user_123."}' | ||
| ``` | ||
|
|
||
| Expected behavior: | ||
|
|
||
| 1. Tool call is paused with `approval-requested`. | ||
| 2. UI/API sends approval response. | ||
| 3. Tool executes and agent continues. | ||
|
|
||
| ## Resume with Raw API (`tool-approval-response`) | ||
|
|
||
| After you receive `approval.id` from the first response, resume like this: | ||
|
|
||
| ```bash | ||
| curl -N -X POST http://localhost:3141/agents/crmHitlAgent/chat \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{ | ||
| "input": [ | ||
| { | ||
| "id": "asst-approval-1", | ||
| "role": "assistant", | ||
| "parts": [ | ||
| { | ||
| "type": "tool-deleteCrmUser", | ||
| "toolCallId": "call_123", | ||
| "state": "approval-requested", | ||
| "input": { "userId": "user_123" }, | ||
| "approval": { "id": "approval-call_123" } | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "id": "tool-approval-1", | ||
| "role": "tool", | ||
| "parts": [ | ||
| { | ||
| "type": "tool-approval-response", | ||
| "approvalId": "approval-call_123", | ||
| "approved": true, | ||
| "reason": "approved by operator" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "id": "user-continue-1", | ||
| "role": "user", | ||
| "parts": [{ "type": "text", "text": "Continue." }] | ||
| } | ||
| ], | ||
| "options": { "conversationId": "conv-hitl-1", "userId": "user-1" } | ||
| }' | ||
| ``` |
Contributor
There was a problem hiding this comment.
Minor documentation gap: initial call doesn't show how to capture conversationId needed for the resume payload.
The first curl sends no conversationId, but the resume payload on line 125 uses "conversationId": "conv-hitl-1". Readers may not realise they need to send conversationId on the first call (or extract it from the response) to correctly target the conversation when resuming. Consider adding a brief note that the conversationId from the first request must be passed in the options of the follow-up call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/recipes/hitl.md` around lines 73 - 127, Add a brief note that callers
must capture the conversationId returned by the initial POST to
/agents/crmHitlAgent/chat (or include one in the first request's options) and
then supply that same conversationId in the options of the follow-up resume
payload (the payload using the tool-approval-response entry and approval.id) so
the approval response targets the correct conversation; mention extracting
conversationId from the first response if not provided up-front.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/agent/subagent/index.ts (1)
753-779: Verify approval detection whenresponse.stepsis unavailable.resolvePendingApprovalFromStream returns null if steps are missing; if some providers/SDK modes don’t populate steps, approvals may be missed. Consider documenting the dependency or adding a fallback source.
AI SDK streamText "steps" availability across providers and streaming modes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/agent/subagent/index.ts` around lines 753 - 779, resolvePendingApprovalFromStream currently returns null if response.steps is missing which can silently miss approvals when certain providers or streaming modes don't populate steps; update resolvePendingApprovalFromStream (and callers if needed) to either (1) attempt a fallback approval extraction from response (e.g., inspect response.text, response.chunks, or any raw stream payload fields available in the Agent streamText response) by passing that data into extractPendingApprovalFromToolResults or a new helper, or (2) explicitly log/document the dependency and throw or surface a warning when steps is absent so callers know approvals cannot be detected; reference the resolvePendingApprovalFromStream method and extractPendingApprovalFromToolResults for where to add the fallback parsing or the warning behavior.
🧹 Nitpick comments (2)
packages/core/src/agent/subagent/index.ts (1)
997-1009: UseModelMessage[]instead ofas anyfor type consistency.The
as anycast weakens type safety in this conversion. While the code already has runtime safeguards (array check and try-catch), usingas ModelMessage[]would align with the pattern used elsewhere in the codebase (seeagent.tslines 7051, 7061) and better express intent to TypeScript.Current code pattern
const uiMessages = convertModelMessagesToUIMessages(toolMessages as any);Consider casting to
ModelMessage[]instead, which is the expected parameter type and matches the conversion pattern used in similar code throughout the agent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/agent/subagent/index.ts` around lines 997 - 1009, The convert call in resolveSharedContextFromExecuteOptions weakly casts toolMessages with "as any"; change this to "as ModelMessage[]" to preserve type safety and match the codebase pattern used elsewhere, and ensure the ModelMessage type is imported where resolveSharedContextFromExecuteOptions and convertModelMessagesToUIMessages are defined so the signature aligns with convertModelMessagesToUIMessages(ModelMessage[]).packages/core/src/utils/message-converter.ts (1)
298-341: Remove unnecessaryas anycasts and maintain type consistency for ToolUIPart mutations.The code inconsistently casts to
anywhen mutating ToolUIPart. However, direct property assignment works elsewhere in the same file (e.g.,toolPart.state = "output-available"at line 363), indicating these properties are assignable without casting. Refactor these three functions to assign properties directly—forstate,output,errorText, andproviderExecuted—all of which are part of ToolUIPart. For theapprovalproperty (not in the base type), consider a local type or module augmentation to keep the mutations typed consistently. This aligns with the coding guideline to "maintain type safety in TypeScript-first codebase."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/message-converter.ts` around lines 298 - 341, The three functions applyOutputDeniedToToolPart, applyApprovalRequestToToolPart, and applyApprovalResponseToToolPart should stop using (toolPart as any) casts: assign state, output, errorText, and providerExecuted directly on ToolUIPart (they are already part of the type) and introduce a narrow local interface or module augmentation that extends ToolUIPart with an optional approval?: { id: string; approved?: boolean; reason?: string } to type the approval property; then replace all casted assignments with properly typed property assignments and keep the same conditional logic for setting state ("output-denied", "approval-requested", "approval-responded") and copying approval payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/utils/message-converter.ts`:
- Around line 277-296: The tool output error text isn't being mapped to the
ToolUIPart "output-error" state, so in both applyToolResultToPart and
createToolPartFromResult call extractErrorTextFromToolOutput(output) early and
if it returns a string set the part/state to "output-error" and populate
errorText with that string (return/emit that part immediately), otherwise
continue the existing approval checks and fall back to "output-available" as
before; this ensures outputs with type: "error-text" or message fields are
preserved as errors rather than treated as available output.
---
Duplicate comments:
In `@packages/core/src/agent/subagent/index.ts`:
- Around line 753-779: resolvePendingApprovalFromStream currently returns null
if response.steps is missing which can silently miss approvals when certain
providers or streaming modes don't populate steps; update
resolvePendingApprovalFromStream (and callers if needed) to either (1) attempt a
fallback approval extraction from response (e.g., inspect response.text,
response.chunks, or any raw stream payload fields available in the Agent
streamText response) by passing that data into
extractPendingApprovalFromToolResults or a new helper, or (2) explicitly
log/document the dependency and throw or surface a warning when steps is absent
so callers know approvals cannot be detected; reference the
resolvePendingApprovalFromStream method and
extractPendingApprovalFromToolResults for where to add the fallback parsing or
the warning behavior.
---
Nitpick comments:
In `@packages/core/src/agent/subagent/index.ts`:
- Around line 997-1009: The convert call in
resolveSharedContextFromExecuteOptions weakly casts toolMessages with "as any";
change this to "as ModelMessage[]" to preserve type safety and match the
codebase pattern used elsewhere, and ensure the ModelMessage type is imported
where resolveSharedContextFromExecuteOptions and
convertModelMessagesToUIMessages are defined so the signature aligns with
convertModelMessagesToUIMessages(ModelMessage[]).
In `@packages/core/src/utils/message-converter.ts`:
- Around line 298-341: The three functions applyOutputDeniedToToolPart,
applyApprovalRequestToToolPart, and applyApprovalResponseToToolPart should stop
using (toolPart as any) casts: assign state, output, errorText, and
providerExecuted directly on ToolUIPart (they are already part of the type) and
introduce a narrow local interface or module augmentation that extends
ToolUIPart with an optional approval?: { id: string; approved?: boolean;
reason?: string } to type the approval property; then replace all casted
assignments with properly typed property assignments and keep the same
conditional logic for setting state ("output-denied", "approval-requested",
"approval-responded") and copying approval payloads.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/agent/subagent/index.spec.tspackages/core/src/agent/subagent/index.tspackages/core/src/utils/message-converter.spec.tspackages/core/src/utils/message-converter.ts
Comment on lines
+277
to
+296
| function extractErrorTextFromToolOutput(output: unknown): string | undefined { | ||
| if (!output || typeof output !== "object" || Array.isArray(output)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const value = output as Record<string, unknown>; | ||
| if ( | ||
| value.type === "error-text" && | ||
| typeof value.value === "string" && | ||
| value.value.trim().length > 0 | ||
| ) { | ||
| return value.value; | ||
| } | ||
|
|
||
| if (typeof value.message === "string" && value.message.trim().length > 0) { | ||
| return value.message; | ||
| } | ||
|
|
||
| return undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and examine the message-converter.ts file
fd "message-converter.ts" --type fRepository: VoltAgent/voltagent
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Read the functions mentioned in the review (lines 344-433)
cat -n packages/core/src/utils/message-converter.ts | sed -n '340,440p'Repository: VoltAgent/voltagent
Length of output: 3787
🏁 Script executed:
#!/bin/bash
# Also search for how output-error is used in the codebase
rg -n "output-error" --type tsRepository: VoltAgent/voltagent
Length of output: 1653
🏁 Script executed:
#!/bin/bash
# Search for ToolUIPart type definition
rg -n "type ToolUIPart|interface ToolUIPart" --type tsRepository: VoltAgent/voltagent
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for ToolUIPart type definition more broadly
rg -n "ToolUIPart" --type ts | head -20Repository: VoltAgent/voltagent
Length of output: 2081
🏁 Script executed:
#!/bin/bash
# Get the type definition for ToolUIPart from types.ts
cat -n packages/core/src/agent/types.ts | sed -n '1040,1080p'Repository: VoltAgent/voltagent
Length of output: 1143
🏁 Script executed:
#!/bin/bash
# Check what states are valid for ToolUIPart by searching the types file
rg -B5 -A5 "state.*output-error" packages/core/src/agent/types.tsRepository: VoltAgent/voltagent
Length of output: 446
🏁 Script executed:
#!/bin/bash
# Let's check the actual usage of extractErrorTextFromToolOutput and where it's called
rg -n "extractErrorTextFromToolOutput" packages/core/src/utils/message-converter.tsRepository: VoltAgent/voltagent
Length of output: 214
🏁 Script executed:
#!/bin/bash
# Let's check if there are any test cases that show expected behavior for error outputs
rg -B10 -A10 "error-text|errorText" packages/core/src/agent/message-normalizer.spec.ts | head -50Repository: VoltAgent/voltagent
Length of output: 601
🏁 Script executed:
#!/bin/bash
# Check the AI SDK ToolUIPart definition to see if output-error is a valid state
# Since it's imported from "ai", let's see what states are actually used
rg -n "state.*output" packages/core/src/utils/message-converter.ts | grep -E "(output-available|output-error|output-denied)"Repository: VoltAgent/voltagent
Length of output: 344
Check if error outputs should map to output-error state instead of output-available.
Currently, extractErrorTextFromToolOutput() is only used in applyToolResultToPart for handling denied approvals. Both applyToolResultToPart and createToolPartFromResult default non-approval outputs to output-available, which means error-encoded outputs (with type: "error-text" or message fields) lose their error state. Since output-error is a valid ToolUIPart state with errorText field, error outputs should be detected and mapped to output-error before falling through to output-available.
Consider adding error detection to both functions using the suggested diff pattern:
- Call
extractErrorTextFromToolOutput()early in both functions - If an error is detected, set state to
output-errorwith the extractederrorText - Otherwise, proceed with the existing approval and
output-availablelogic
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/utils/message-converter.ts` around lines 277 - 296, The
tool output error text isn't being mapped to the ToolUIPart "output-error"
state, so in both applyToolResultToPart and createToolPartFromResult call
extractErrorTextFromToolOutput(output) early and if it returns a string set the
part/state to "output-error" and populate errorText with that string
(return/emit that part immediately), otherwise continue the existing approval
checks and fall back to "output-available" as before; this ensures outputs with
type: "error-text" or message fields are preserved as errors rather than treated
as available output.
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.
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
In delegated subagent flows, approvals requested by guarded tools (
needsApproval) could fail to resume correctly when the UI approval response was attached totool-delegate_taskparts. This caused repeated approval prompts instead of executing the approved guarded tool.There was also a context propagation gap where parent tool-context messages were not consistently forwarded to delegated subagents, so required inputs (such as
userId) could be lost.Additionally, docs were missing raw API examples for
tool-approval-response, and there was no dedicated Recipes & Guides HITL page covering both direct agent and subagent scenarios.What is the new behavior?
tool-delegate_taskare now matched to the underlying guarded subagent tool call and correctly resume execution.tool-approval-request,tool-approval-response) alongside tool call/result events.tool-approval-response) and a new Recipes & Guides HITL page covering both direct and subagent usage.fixes #1105
Notes for reviewers
Core changes:
packages/core/src/agent/agent.tspackages/core/src/agent/subagent/index.tspackages/core/src/utils/tool-approval.tspackages/core/src/utils/message-converter.tsTest coverage:
packages/core/src/agent/agent.spec.tspackages/core/src/agent/subagent/index.spec.tspackages/core/src/utils/message-converter.spec.tsDocs & recipe updates:
website/docs/agents/tools.mdwebsite/docs/api/endpoints/agents.mdwebsite/docs/agents/subagents.mdwebsite/docs/agents/overview.mdwebsite/recipes/hitl.mdwebsite/recipes/overview.mdwebsite/recipes/tools.mdwebsite/sidebarsRecipes.tsValidation run:
cd packages/core && pnpm vitest run src/agent/agent.spec.ts --testNamePattern "Tool Approval"cd packages/core && pnpm vitest run src/agent/subagent/index.spec.tscd packages/core && pnpm typecheckSummary by cubic
Fixes HITL resume in delegated subagent flows by matching approval responses to the guarded tool call and forwarding context/events so approved actions run without repeat prompts. Also normalizes denied approvals to a clear UI state and error, and strips OpenAI item IDs from delegated shared context.
Bug Fixes
New Features
Written for commit 317690f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests