Spare a background subagent's in-flight tool call from the turn-end wipe - #334559
Open
Ryan Ewen (RyanEwen) wants to merge 2 commits into
Open
Spare a background subagent's in-flight tool call from the turn-end wipe#334559Ryan Ewen (RyanEwen) wants to merge 2 commits into
Ryan Ewen (RyanEwen) wants to merge 2 commits into
Conversation
`clearPending` drops all cross-message tool attribution on every `result` envelope. A background subagent outlives the turn that spawned it, so its inner tool call is still in flight and its result is still coming, but its entry is wiped along with the genuinely orphaned ones. When that result arrives the tool name and input are gone, so the call renders with a generic past-tense label and live diverges from replay. `drainForegroundSpawns`, called on the same envelope two lines later, already spares background spawns for exactly this reason.
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
The tests bypass the production mapper wiring, leaving the reported regression insufficiently covered.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Preserves in-flight tool attribution for background Claude subagents across turn cleanup.
Changes:
- Skips background-subagent tools during pending-call cleanup.
- Passes subagent state into result handling.
- Adds background and foreground cleanup tests.
File summaries
| File | Description |
|---|---|
claudeToolCallRegistry.ts |
Adds background-aware cleanup. |
claudeMapSessionEvents.ts |
Connects subagent lookup to cleanup. |
claudeToolCallRegistry.test.ts |
Tests selective cleanup behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The registry-level tests pass a synthetic lookup straight to `clearPending`, so dropping the `registry` argument at the `mapResult` call site would not fail them and the divergence would return unnoticed. Adds a test that drives the real path: a background spawn with its inner tool, a streamed `tool_use`, then a `result` envelope through `mapSDKMessageToAgentSignals`.
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.
clearPendingdrops all cross-message tool attribution on everyresultenvelope. A background subagent outlives the turn that spawned it, so its inner tool call is still in flight and its result is still coming, but its entry is wiped along with the genuinely orphaned ones.Why it matters
The registry exists because a
tool_useand itstool_resultarrive in different SDK messages, so the tool name, parsed input and invocation message have to survive between them. Once the entry is gone, the late result renders with the generic past-tense message instead of the real one, and live output diverges from what replay produces from the same transcript.The precedent
drainForegroundSpawnsis called frommapResulttwo lines afterclearPendingToolCalls, and already makes exactly this distinction:The subagent registry keeps that state; the tool call registry simply never consulted it.
The change
clearPendingtakes an optional lookup and skips entries whose parent spawn is background, leaving every other orphan drained and warned exactly as before. The lookup is declared as a narrow local interface rather than importing the subagent registry, to avoid a cycle between the two modules.clearPendingToolCallsalready runs beforedrainForegroundSpawns, so the spawn and its inner edges are still present when the check happens.Tests
clearPending spares an inner tool of a background subagent and still drains the restfails without the change.clearPending still drains an inner tool whose parent subagent is foregroundis the control and passes either way.The suite goes from 12 passing with 1 failing to 13 passing, and both pre-existing
clearPendingtests are unaffected. No new type errors.AI disclosure: this comment and the related code were written with the assistance of AI.