Do not mark a denied subagent tool call as a subagent - #334549
Open
Ryan Ewen (RyanEwen) wants to merge 2 commits into
Open
Do not mark a denied subagent tool call as a subagent#334549Ryan Ewen (RyanEwen) wants to merge 2 commits into
Ryan Ewen (RyanEwen) wants to merge 2 commits into
Conversation
Subagent-ness is taken from the tool name, so a denied `Agent` or `Task` call still has a subagent marker attached to its result. On restore that marker registers a subagent chat whose transcript can never exist, because nothing was ever spawned. `is_error` is already computed immediately above the push and was simply not consulted. Successful calls are unaffected.
Contributor
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: TylerLeonhardtMatched files:
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
is_error also covers spawned subagents that later fail, potentially hiding valid transcripts; denial-specific handling and failure coverage are needed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents denied Claude subagent calls from creating phantom restored chats.
Changes:
- Suppresses subagent markers for errored tool results.
- Adds regression coverage for denied
Agentcalls.
File summaries
| File | Description |
|---|---|
src/vs/platform/agentHost/test/node/claudeReplayMapper.test.ts |
Adds denied-call replay coverage. |
src/vs/platform/agentHost/node/claude/claudeReplayMapper.ts |
Conditionally attaches restored subagent markers; the condition is currently too broad. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`is_error` also covers a subagent that started and then failed, and those children have real transcripts. Keying on it alone hid every failed subagent on restore. The SDK stamps `agentId: <hex>` on the tool_result of a call that actually spawned, which is what `scanTranscriptForAgentIds` already keys on, so a pre-spawn denial is the case with an error and no agentId.
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
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.
A denied
AgentorTaskcall still gets a subagent marker attached to its result, so on restore the host registers a subagent chat whose transcript can never exist.What happens
toolKindis derived from the tool name, with no regard for the outcome, and_attachToolResultpushesToolResultContentType.Subagentunconditionally. On restore,_registerRestoredSubagentSummariesscans replayed turns for that marker and registers a chat with a lazy resolver, and the resolver throwsSubagent transcript is not available yetforever, because nothing was ever spawned.Since #334312 that is no longer fatal to the session, but the phantom chat is still registered, still fails to
subscribe, and still shows in the session's chat list as a subagent that never produces anything.How it was hit
Auto mode could not classify an
Agentcall and denied it:The SDK transcript for that session is 47 lines with zero
isSidechainentries, confirming no subagent ran. Any denial reaches this the same way, so a permission rule or a user rejecting the call does too.The change
is_erroralone is not the right test: it also covers a subagent that started and then failed, and those children have real transcripts that must still be linked on restore.The SDK stamps
agentId: <hex>on the tool_result of a call that actually spawned, which is whatscanTranscriptForAgentIdsalready keys on through the exportedSUBAGENT_ID_SUFFIX_REGEX. A pre-spawn denial is therefore the case with an error and no agentId:resultTextis already computed immediately above for the past-tense message, so nothing new is derived.Tests
Fixture 5c: a denied Agent tool_use produces no subagent markerasserts the call is still recognised as a subagent by name, thatsuccessisfalse, and that no marker is attached.Fixture 5d: an Agent that spawned and then failed keeps its subagent markeris the failed-after-spawn case, and fails against anis_error-only condition.Fixtures 5 and 5b continue to pass, so successful spawns are unchanged. The suite is 30 passing. No new type errors.
AI disclosure: this comment and the related code were written with the assistance of AI.