Skip to content

Notify hooks after response delivery - #2457

Draft
gimenete wants to merge 1 commit into
mainfrom
hook-response-receipt
Draft

Notify hooks after response delivery#2457
gimenete wants to merge 1 commit into
mainfrom
hook-response-receipt

Conversation

@gimenete

@gimenete gimenete commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • expose JSON-RPC request IDs to hook handlers
  • notify session hooks only after a hook response is written successfully
  • cover the response-delivery callback in the Rust SDK session test

Testing

  • cargo test --features test-support hooks_invoke_dispatches_to_session_hooks --test session_test

Required by github/github-app#14176.

Copilot AI balanced review requested due to automatic review settings September 1, 2026 12:29
@gimenete
gimenete requested a review from a team as a code owner September 1, 2026 12:29
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@gimenete
gimenete force-pushed the hook-response-receipt branch from 4f23cbc to b66cec8 Compare September 1, 2026 12:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The new public payload needs API hardening, and response-delivery ordering and failure behavior require coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 2 Medium severity

New issues introduced by this change (2)
Severity Finding
Medium severity rust/​src/​hooks.rs — This SDK-produced callback payload is a new public struct with public fields, so downstream code…
Medium severity rust/​tests/​session_test.rs — This test covers callback contents only; because the receiver is not polled until after…
What changed in this PR

Adds hook request correlation and post-delivery notifications to the Rust SDK.

Changes:

  • Exposes JSON-RPC request IDs through hook contexts.
  • Notifies hooks after successful response writes.
  • Extends session tests for callback data.
File Description
rust/​src/​hooks.rs Adds request IDs and response-sent callback APIs.
rust/​src/​session.rs Dispatches IDs and invokes callbacks after writes.
rust/​tests/​session_test.rs Tests request IDs and callback payloads.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread rust/src/hooks.rs
Comment on lines +27 to +28
#[derive(Debug, Clone)]
pub struct HookResponseSent {
Comment on lines +4158 to +4163
let response_sent = timeout(TIMEOUT, response_sent_rx.recv())
.await
.unwrap()
.unwrap();
assert_eq!(response_sent.request_id, 300);
assert_eq!(response_sent.hook_type, "preToolUse");

@SteveSandersonMS SteveSandersonMS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together. Before we add this API, we need clarification on the intended design and scope.

Public API design

Why is this implemented by exposing the JSON-RPC call ID through HookContext and HookResponseSent? The SDKs currently keep the SDK↔runtime JSON-RPC envelope IDs internal. Hook invocation context in Rust, Node.js, Python, Go, .NET, and Java exposes the session ID, but not the transport request ID. Public requestId values elsewhere in the SDK represent explicit product/protocol concepts such as permission or sampling requests; they are not the JSON-RPC envelope identifier.

Please explain the application-level operation consumers need to perform and why exposing this transport detail is the appropriate, consistent extension to the public API.

Correlation model

If the goal is to correlate hook responses with source events displayed in an application UI—for example, matching preToolUse to a tool-call request—could we instead add toolCallId to the ordinary tool hook payloads? toolCallId is already an established public product concept in the SDK (including tool invocations, permission requests, and preMcpToolCall) and expresses the application relationship directly, without exposing JSON-RPC internals.

Cross-language scope

This capability does not appear inherently Rust-specific. Our default is consistent capabilities across all six SDK languages. Unless there is a concrete reason this can only or should only exist in Rust, the change should cover Node.js, .NET, Python, Go, Rust, and Java, with corresponding E2E coverage that validates the real application flow. If the intended exception is that only a Rust consumer currently needs it, that alone is not sufficient justification for creating a Rust-only public capability; please document the product or platform constraint.

Once the intended API is settled, tests should also prove the claimed timing and failed-write behavior rather than only checking callback payload contents.

Of course we want to support what you need here. Whenever we add public APIs to the SDK, we have to ensure they have a shape we want to support permanently and consistently across the SDKs. Hope that's OK.

@SteveSandersonMS

Copy link
Copy Markdown
Contributor

Marking as draft - please set back to ready to review when you're ready.

@SteveSandersonMS
SteveSandersonMS marked this pull request as draft September 1, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants