Fix tool approval denial reason not propagating to LLM or parent agent#2500
Fix tool approval denial reason not propagating to LLM or parent agent#2500
Conversation
- Return denial reason string from tool execute so the sub-agent LLM sees a clean human-readable message instead of the raw sentinel object - Track denial redirect events on Agent instances during execution - Prepend denial context to delegation responses so the parent agent knows the user redirected the task mid-execution (e.g. SF → Tokyo)
🦋 Changeset detectedLatest commit: 532e5bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Fixes two bugs in the tool approval denial flow: (1) sub-agent LLMs received the raw
|
There was a problem hiding this comment.
The sentinel-to-string fix in wrapToolWithStreaming is correct — the LLM will now see a clean human-readable denial message. However, the redirect tracking has a semantic issue: approvalResult.reason is always truthy on any denial (timeouts, expiries, explicit denials), so the parent agent will receive misleading "user redirected this task" notes even for timeout/expiry denials. The redirect note path in generateTaskHandler.ts also lacks test coverage.
There was a problem hiding this comment.
PR Review Summary
(6) Total Issues | Risk: Medium
🟠⚠️ Major (4) 🟠⚠️
Inline Comments:
- 🟠 Major:
Agent.ts:1027-1033Denials without explicit reasons are silently excluded fromtaskDenialRedirects, creating inconsistent parent agent visibility - 🟠 Major:
Agent.ts:1353-1359Same issue for function tools path - 🟠 Major:
generateTaskHandler.ts:506-513User-provided denial reason injected into parent agent prompt without sanitization - 🟠 Major:
generateTaskHandler.test.ts:202-204Core Bug #2 fix (note injection) has zero test coverage
🟠 5) file Missing changeset for bug fix
Issue: Per AGENTS.md, any bug fix affecting runtime behavior requires a changeset. This PR fixes two bugs in agents-api that affect how tool denials propagate through the system.
Why: Changesets document changes for release notes and help consumers understand what changed between versions.
Fix: Run:
pnpm bump patch --pkg agents-api "Fix tool approval denial reason not propagating to LLM or parent agent"Refs:
- AGENTS.md — changeset requirements
- changeset-bot comment already flagged this
🟡 Minor (1) 🟡
🟡 1) functionToolApprovals.test.ts No test for denial-without-reason fallback
Issue: The code at Agent.ts:753 has a fallback result.reason ?? 'Tool call was denied by the user.' for denials without explicit reasons. The existing test only covers denial WITH a reason (reason: 'no'). The fallback path is untested.
Why: If the fallback is removed or broken, LLMs would receive undefined as a tool result, potentially confusing the model.
Fix: Add a test case where waitForApproval returns { approved: false, reason: undefined } and verify the result equals the fallback string.
Refs:
💭 Consider (2) 💭
💭 1) Agent.ts:297 taskDenialRedirects array never cleared
Issue: The array accumulates throughout the agent's lifetime. If an Agent instance were reused across multiple delegated tasks, denials from earlier tasks would persist in later responses.
Why: Currently agents appear to be instantiated per-task (cleanup() is called in finally block), but this is an implicit assumption. If agent reuse patterns change, this becomes a bug.
Fix: Either clear the array at the start of generate(), add a comment documenting the single-use assumption, or clear it in cleanup().
Refs:
- generateTaskHandler.ts:506 — where array is consumed
💭 2) Agent.ts:297 Naming: "redirects" vs "reasons"
Issue: The variable is named taskDenialRedirects but the implementation tracks all denials with any reason, not specifically "redirect-like" denials. A user saying "no, I changed my mind" is different from "no, use Tokyo instead".
Why: The PR description focuses on "redirects" but the code captures all denial reasons. This is fine if intentional, but the naming could confuse future maintainers.
Fix: Consider renaming to taskDenialReasons for accuracy, or add a comment clarifying that all reasons are captured.
🚫 REQUEST CHANGES
Summary: The fix addresses both bugs well at a high level, but has gaps that could leave Bug #2 partially unresolved. Specifically: (1) denials without explicit reasons won't propagate to parent agents since tracking is conditional, and (2) the core fix for Bug #2 has no test coverage. The prompt injection concern with unsanitized reasons is also worth addressing. Once these are resolved, this is a solid fix.
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
Agent.ts:1027-1038 |
Code organization — tracking in different location than transformation | Valid observation but code works correctly; purely stylistic preference |
Agent.ts:753 |
Fallback lacks tool-specific context | Low confidence; generic message is acceptable for edge case |
Agent.ts:1027 |
Denials should track even without reason (duplicate) | Merged into main Major finding |
generateTaskHandler.test.ts:202 |
No test coverage (duplicate) | Merged into main Major finding |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-errors |
3 | 0 | 0 | 0 | 2 | 0 | 1 |
pr-review-tests |
3 | 1 | 0 | 0 | 1 | 0 | 1 |
pr-review-llm |
2 | 0 | 0 | 0 | 1 | 0 | 1 |
pr-review-precision |
3 | 0 | 2 | 0 | 0 | 0 | 1 |
pr-review-standards |
1 | 1 | 0 | 0 | 0 | 0 | 0 |
| Total | 12 | 2 | 2 | 0 | 4 | 0 | 4 |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta re-review scoped to changes since commit 1f6e0f786b25. The new commit cf2bcf3a3 addresses all prior review feedback.
✅ Prior Feedback Addressed
| Prior Issue | Resolution in Delta |
|---|---|
| 🟠 Denials without reasons silently excluded | ✅ recordDenial() now always records with fallback message |
| 🟠 Same issue for function tools path | ✅ Both paths now use the same recordDenial() helper |
| 🟠 Unsanitized reasons in parent prompt | ✅ Added sanitize() — removes newlines, caps at 200 chars |
| 🟠 No test coverage for redirect note | ✅ New test validates note injection with toolName, toolCallId, reason |
| 🟠 Missing changeset | ✅ Added .changeset/impossible-sapphire-elk.md |
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
generateTaskHandler.test.ts:584-611Test cleanup pattern could cause test pollution on assertion failure
🔍 Delta Changes Verified
Agent.ts (lines 554-560):
private recordDenial(toolName: string, toolCallId: string, reason: string | undefined): void {
this.taskDenialRedirects.push({
toolName,
toolCallId,
reason: reason ?? 'Tool call was denied by the user.', // ← Always records
});
}- Centralized denial recording with fallback ✅
- Applied to both MCP (line 1037) and function tool (line 1357) paths ✅
generateTaskHandler.ts (lines 506-516):
- Sanitization added:
s.replace(/\n/g, ' ').slice(0, 200)✅ - toolCallId now included in note format ✅
- Note wording refined to be more accurate ✅
generateTaskHandler.test.ts (lines 583-611):
- New test exercises the denial redirect note injection ✅
- Validates note contains toolName, toolCallId, and reason ✅
✅ APPROVE
Summary: All four Major issues from the prior review have been addressed. The fix is now complete: denials are always tracked (regardless of reason truthiness), reasons are sanitized before injection into parent agent context, and test coverage validates the behavior. The one remaining suggestion is a minor test cleanup pattern improvement that doesn't affect production behavior.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
generateTaskHandler.ts:508 |
Truncation should add ellipsis | Low confidence; current behavior is acceptable, purely stylistic preference |
generateTaskHandler.ts:506 |
Optional chaining masks null agent | Low confidence; agent is created in same try block and would have thrown earlier |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
2 | 0 | 1 | 0 | 1 | 0 | 0 |
pr-review-errors |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-precision |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
| Total | 5 | 0 | 1 | 0 | 1 | 0 | 3 |
Note: pr-review-precision finding was duplicate of pr-review-tests finding (test cleanup pattern).
| vi.mocked(Agent).prototype.getTaskDenialRedirects = vi.fn().mockReturnValue([ | ||
| { | ||
| toolName: 'get_coordinates', | ||
| toolCallId: 'call-abc', | ||
| reason: 'I want the coordinates for tokyo instead', | ||
| }, | ||
| ]); | ||
|
|
||
| const taskHandler = createTaskHandler(mockConfig); | ||
| const task: A2ATask = { | ||
| id: 'task-123', | ||
| input: { parts: [{ kind: 'text', text: 'Get coordinates for San Francisco' }] }, | ||
| context: { conversationId: 'conv-123' }, | ||
| }; | ||
|
|
||
| const result = await taskHandler(task); | ||
|
|
||
| expect(result.status.state).toBe(TaskState.Completed); | ||
| expect(result.artifacts?.[0].parts).toHaveLength(2); | ||
| const notePart = result.artifacts?.[0].parts[0]; | ||
| expect(notePart?.kind).toBe('text'); | ||
| expect((notePart as any)?.text).toContain('[NOTE: Some tool calls were denied'); | ||
| expect((notePart as any)?.text).toContain( | ||
| 'get_coordinates (call-abc): I want the coordinates for tokyo instead' | ||
| ); | ||
|
|
||
| vi.mocked(Agent).prototype.getTaskDenialRedirects = vi.fn().mockReturnValue([]); | ||
| }); |
There was a problem hiding this comment.
💭 Consider: Test cleanup pattern
Issue: The mock is restored at the end of the test body (line 610) rather than in an afterEach hook. If the test fails before reaching line 610, subsequent tests may inherit the mocked getTaskDenialRedirects.
Why: This is a low-risk issue since the base MockAgent class at line 203-205 already returns [] by default and most tests create fresh instances. However, the current pattern could cause confusing cascading failures if assertions fail mid-test.
Fix: Consider wrapping in try/finally or moving cleanup to afterEach:
it('prepends denial redirect note when task had denied tool calls', async () => {
const originalMethod = vi.mocked(Agent).prototype.getTaskDenialRedirects;
vi.mocked(Agent).prototype.getTaskDenialRedirects = vi.fn().mockReturnValue([...]);
try {
// ... test body ...
} finally {
vi.mocked(Agent).prototype.getTaskDenialRedirects = originalMethod;
}
});Refs:
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta re-review scoped to changes since commit cf2bcf3a3b2c. The delta consists solely of a merge from main (commit f58484d55).
🔍 Delta Analysis
The only changes since the last approved review are from PR #2502 (Trace improvements), merged via main:
| File | Delta Change | Relevance to This PR |
|---|---|---|
Agent.ts:204 |
Added agentName?: string to AgentConfig type |
❌ Unrelated — trace attribute fix |
Agent.ts:2432 |
Changed span attribute to use this.config.agentName |
❌ Unrelated — trace attribute fix |
generateTaskHandler.ts:197 |
Added agentName: currentAgent.name to Agent constructor |
❌ Unrelated — trace attribute fix |
This PR's bug fix code is unchanged since the last approved review:
- ✅
recordDenial()helper with fallback message — intact - ✅ MCP and function tool paths both call
recordDenial()— intact - ✅ Sanitization in
generateTaskHandler.ts— intact - ✅ Test coverage for denial redirect note — intact
- ✅ Changeset present — intact
🕐 Pending Recommendations (1)
- 💭
generateTaskHandler.test.ts:611Test cleanup pattern could cause test pollution (Consider — from prior review)
✅ APPROVE
Summary: The merge from main introduced only trace-related changes (agent name attribute) that are orthogonal to this PR's bug fix. The denial reason propagation fix remains complete and unchanged. All prior feedback has been addressed. Ship it! 🚀
Reviewers (0)
No reviewers dispatched — delta contained no PR-specific changes requiring domain review.
| 'Tool execution denied by user' | ||
| ); | ||
|
|
||
| this.recordDenial(toolName, toolCallId, approvalResult.reason); |
There was a problem hiding this comment.
I have a concern about our tool approval flow in general. I bring it up here because this is a continuation of what I think is the problematic pattern.
What we are doing is aggregating the denials in memory. I think all tool related states should be persisted beyond application memory so that we can have potentially long running operations.
Let's discuss tomorrow and either we can fix it in this PR or as a separate PR.
@tim-inkeep is refactoring Agent.ts so I think we should implement persistent storage later
There was a problem hiding this comment.
Worth thinking about how this is related to durable execution. I.e., are the tool related states persisted via durable execution state primatives (using e.g. Vercel Workflows) and/or the runtime DB.
I shared context with Abraham on durable execution, that's the next project.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta re-review scoped to changes since commit cf2bcf3a3b2c. The delta consists solely of a merge from main (commit 30e85841d).
🔍 Delta Analysis
The only changes since the last approved review are from merging main, which brought in:
- PR #2516: Docker-compose dependency bumps
- PR #2514-2515: Docs updates for scheduled triggers
- PR #2513: Version packages
- PR #2512: Errors-only view in traces
- PR #2502: Trace improvements (
agentNameattribute)
Changes to PR-specific files from the merge:
| File | Delta Change | Relevance to This PR |
|---|---|---|
Agent.ts:204 |
Added agentName?: string to AgentConfig type |
❌ Unrelated — trace attribute fix |
Agent.ts:2432 |
Changed span attribute to use this.config.agentName |
❌ Unrelated — trace attribute fix |
generateTaskHandler.ts:197 |
Added agentName: currentAgent.name to Agent constructor |
❌ Unrelated — trace attribute fix |
This PR's bug fix code is unchanged since the last approved review:
- ✅
recordDenial()helper with fallback message — intact - ✅ MCP and function tool paths both call
recordDenial()— intact - ✅ Sanitization in
generateTaskHandler.ts— intact - ✅ Test coverage for denial redirect note — intact
- ✅ Changeset present — intact
🕐 Pending Recommendations (1)
- 💭
generateTaskHandler.test.ts:611Test cleanup pattern could cause test pollution (Consider — from prior review)
📝 Human Review Discussion (Acknowledged)
@amikofalvy and @nick-inkeep raised valid points about in-memory denial tracking vs persistent storage. This is acknowledged as a future consideration for the durable execution project — not blocking for this bug fix PR.
✅ APPROVE
Summary: The merge from main introduced only trace-related changes (agent name attribute) that are orthogonal to this PR's bug fix. The denial reason propagation fix remains complete and unchanged. All prior feedback has been addressed. Ship it! 🚀
Reviewers (0)
No reviewers dispatched — delta contained no PR-specific changes requiring domain review.
Document that parent agents now receive a note in the delegation response when a user redirects a delegated sub-agent's task mid-execution (e.g. by denying a tool call with a reason). This gives the parent full context about how the task evolved. Ref: PR #2500
* docs: clarify denial reason propagation in tool approvals runtime behavior * docs: document denial reason propagation for tool approvals * docs: add task redirect visibility to delegation relationships Document that parent agents now receive a note in the delegation response when a user redirects a delegated sub-agent's task mid-execution (e.g. by denying a tool call with a reason). This gives the parent full context about how the task evolved. Ref: PR #2500 * Apply suggestion from @pullfrog[bot] Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> --------- Co-authored-by: inkeep[bot] <257615677+inkeep[bot]@users.noreply.github.com> Co-authored-by: Abraham <[email protected]> Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
In durable execution, each step creates a fresh Agent instance, so taskDenialRedirects accumulated during executeToolStep were lost before the next callLlmStep. This meant the LLM had no awareness that a tool was denied and the generateTaskHandler delegation note from #2500 never fired in durable mode. Carry denial state in the workflow loop, pass it into callLlmStep, and give the LLM an explicit post-approval message when denials occurred. Also improve conversation history format for denied tools.
Summary
{ __inkeepToolDenied: true, ... })Agentinstance during executionget_coordinates(San Francisco)with reason "I want tokyo instead"), the delegation response now includes a note so the parent agent knows the task evolvedProblem
Two bugs in the approval denial flow:
Bug 1 — Sub-agent LLM received a raw JSON sentinel instead of the reason string, so it would see
{"__inkeepToolDenied":true,"toolCallId":"...","reason":"..."}and sometimes ignore the redirect.Bug 2 — Parent agent had no visibility into user-triggered redirects during delegation. In the trace:
activities-planneraskedget-coordinates-agentfor San Francisco, the user denied and redirected to Tokyo,get-coordinates-agentcorrectly fetched Tokyo — butactivities-plannerreceived the Tokyo result without context, got confused by the mismatch, and hallucinated San Francisco's coordinates from training data.Changes
Agent.ts— Returnsresult.reasonstring (not the sentinel object) fromwrapToolWithStreamingso the AI SDK passes a clean message to the LLM; tracks denial events intaskDenialRedirects[]; exposesgetTaskDenialRedirects()gettergenerateTaskHandler.ts— Prepends a[NOTE: The user redirected this task...]prefix to delegation responses when denials occurred, giving the parent agent full contextCleanShot.2026-03-03.at.13.49.07.mp4