Fix sub-agent hook propagation: expose sessionId on hook inputs#1290
Merged
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1290 · ● 1.9M
This comment has been minimized.
This comment has been minimized.
Adds an E2E test that verifies preToolUse and postToolUse hooks fire for tool calls made by sub-agents spawned via the task tool. The test spawns an explore agent that reads a file using the view tool and asserts that hooks are invoked for the sub-agent's view tool call. This test requires the runtime fix (getEffectiveHooks() propagation) from github/copilot-agent-runtime and the SESSION_BASED_SUBAGENTS feature flag to be enabled. Relates to #1097 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The runtime fix (getEffectiveHooks) only applies to the session-based subagent path. The legacy callback-bridge path does not support SDK preToolUse/postToolUse hooks for sub-agent tool calls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Track hook invocations with sequential indices to assert that: - 'task' hooks fire for the parent agent - 'view' hooks fire for the sub-agent - Sub-agent 'view' hooks fire after the parent's 'task' pre-hook, proving they come from the spawned sub-agent Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The runtime sends sessionId in hook inputs identifying which session (parent or sub-agent) invoked the tool. Map this to agentSessionId on BaseHookInput so SDK consumers can distinguish parent tool calls from sub-agent tool calls. Update the E2E test to assert that parent 'task' hooks and sub-agent 'view' hooks carry different agentSessionIds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace agentSessionId with sessionId on BaseHookInput — this is the runtime session ID passed through from the wire, which differs between parent and sub-agent tool calls. Simplify the E2E test to focus on verifying hooks fire for both parent and sub-agent tools and that their sessionIds differ. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…thon, Go, .NET - Add sessionId field to BaseHookInput/PreToolUseHookInput/PostToolUseHookInput in Python, Go, and .NET SDKs (already present in Node.js) - Create subagent_hooks E2E tests for all three languages sharing the same snapshot as the Node.js test - Tests verify hooks fire for both parent and sub-agent tool calls with distinct sessionIds Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add session_id field to all 6 hook input structs (PreToolUseInput, PostToolUseInput, UserPromptSubmittedInput, SessionStartInput, SessionEndInput, ErrorOccurredInput) - Update existing unit test JSON fixtures with sessionId - Create subagent_hooks E2E test sharing the same snapshot as other SDKs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
….NET - Python: Add sessionId to UserPromptSubmittedHookInput, SessionStartHookInput, SessionEndHookInput, ErrorOccurredHookInput. Remove unused BaseHookInput. - .NET: Add SessionId to UserPromptSubmittedHookInput, SessionStartHookInput, SessionEndHookInput, ErrorOccurredHookInput. All 6 hook input types now have sessionId in every SDK (Node, Python, Go, .NET, Rust). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Instead of polluting global os.environ at module import time, create a dedicated client with the feature flag in its env dict. This matches how the .NET and Go tests scope the env var to the client instance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
force-pushed
the
stevesa/subagent-permission-hooks
branch
from
e10f695 to
7021a8e
Compare
This comment has been minimized.
This comment has been minimized.
The bundled runtime changed 'retrieve the full results' to 'retrieve unread results' in agent completion notifications. Also fix ruff formatting on the Python E2E test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
The session_test hooks tests were missing the sessionId field in their input JSON, causing deserialization failures after adding the required session_id field to hook input structs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Cross-SDK Consistency Review ✅This PR maintains excellent cross-SDK consistency. Here's what was verified:
|
| Hook Input Type | Node.js | Python | Go | .NET | Rust |
|---|---|---|---|---|---|
PreToolUseHookInput |
✅ | ✅ | ✅ | ✅ | ✅ |
PostToolUseHookInput |
✅ | ✅ | ✅ | ✅ | ✅ |
UserPromptSubmittedHookInput |
✅ | ✅ | ✅ | ✅ | ✅ |
SessionStartHookInput |
✅ | ✅ | ✅ | ✅ | ✅ |
SessionEndHookInput |
✅ | ✅ | ✅ | ✅ | ✅ |
ErrorOccurredHookInput |
✅ | ✅ | ✅ | ✅ | ✅ |
Naming follows language conventions correctly
- Node.js/TypeScript:
sessionId(camelCase onBaseHookInput) ✅ - Python:
sessionId(TypedDict key, matching JSON wire format) ✅ - Go:
SessionIDwithjson:"sessionId"tag (idiomatic Go:IDnotId) ✅ - .NET:
SessionIdwith[JsonPropertyName("sessionId")](idiomatic C# PascalCase) ✅ - Rust:
session_idwith#[serde(rename_all = "camelCase")](idiomatic snake_case + serde) ✅
E2E tests added for all 5 SDKs, sharing a single snapshot ✅
No consistency issues found. The PR is a model example of a cross-SDK feature addition.
Generated by SDK Consistency Review Agent for issue #1290 · ● 662.4K · ◷
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.
Summary
Fixes #1097 --
preToolUse/postToolUsehooks were not being invoked for tool calls made by sub-agents spawned via the task tool.Changes
Hook input types (all SDKs)
sessionIdto all 6 hook input types (PreToolUseHookInput,PostToolUseHookInput,UserPromptSubmittedHookInput,SessionStartHookInput,SessionEndHookInput,ErrorOccurredHookInput) across Node.js, Python, Go, .NET, and Rust.input.sessionIdwithinvocation.sessionId.E2E tests (all SDKs)
subagent_hooksE2E tests for all five SDKs, sharing a single snapshot.preToolUse/postToolUsehooks, sends a prompt that spawns a sub-agent (task tool -> view tool), and asserts:tasktool callviewtool callsessionIddiffers between parent and sub-agent hook invocationsRequirements
COPILOT_EXP_COPILOT_CLI_SESSION_BASED_SUBAGENTS=truefeature flag (the legacy callback-bridge path does not support SDK tool hooks for sub-agents).