Add sessionId to BYOK provider-token callback; rename to bearerTokenProvider#1796
Merged
Conversation
Surface the owning session id on the hand-written ProviderTokenArgs in every language SDK (Rust, Node, Go, Python, Java, .NET) and wire the dispatch glue to populate it from the generated request (which the runtime already injects on the wire for session-scoped RPCs). A client-level shared getBearerToken callback registered across many sessions can now resolve which session triggered the request and scope token acquisition or caching accordingly. No runtime changes required: the runtime already merges sessionId into the wire params for every clientSession-scoped method, and codegen already includes it on each generated ProviderTokenAcquireRequest. Only the consumer-facing args type and the glue that builds it from the request were missing the field. Per-language e2e tests now assert a non-empty sessionId is surfaced. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the BYOK bearer-token provider callback argument type (ProviderTokenArgs) across all SDKs to include the owning sessionId, wiring it through each language’s provider-token dispatch path and updating E2E coverage to assert it is present.
Changes:
- Added
sessionId(session_id/SessionID/SessionId) toProviderTokenArgsin Rust, Node.js, Python, Go, Java, and .NET. - Updated each provider-token dispatch/adapter to populate
sessionIdfrom theProviderTokenAcquirerequest. - Updated per-language BYOK E2E tests to assert a non-empty session id reaches the callback.
Show a summary per file
| File | Description |
|---|---|
| rust/tests/e2e/byok_bearer_token_provider.rs | Asserts session_id is non-empty in the provider callback. |
| rust/src/provider_token.rs | Adds session_id field to Rust ProviderTokenArgs. |
| rust/src/provider_token_dispatch.rs | Populates session_id when invoking the token provider. |
| python/e2e/test_byok_bearer_token_provider_e2e.py | Asserts session_id is present and non-empty in callback args. |
| python/copilot/session.py | Adds session_id to ProviderTokenArgs TypedDict and populates it at dispatch. |
| nodejs/test/e2e/byok_bearer_token_provider.e2e.test.ts | Asserts sessionId is a non-empty string in callback args. |
| nodejs/src/types.ts | Adds sessionId to the public ProviderTokenArgs interface. |
| nodejs/src/session.ts | Passes sessionId through to the registered token callback. |
| java/src/test/java/com/github/copilot/ByokBearerTokenProviderE2ETest.java | Asserts non-empty sessionId in callback args. |
| java/src/main/java/com/github/copilot/RpcHandlerDispatcher.java | Passes sessionId into constructed ProviderTokenArgs. |
| java/src/main/java/com/github/copilot/rpc/ProviderTokenArgs.java | Adds sessionId field/accessors and updates constructor usage. |
| go/types.go | Adds SessionID to ProviderTokenArgs. |
| go/session.go | Populates SessionID when invoking the token callback. |
| go/internal/e2e/byok_bearer_token_provider_e2e_test.go | Asserts SessionID is non-empty in callback args. |
| dotnet/test/E2E/ByokBearerTokenProviderE2ETests.cs | Asserts SessionId is non-empty in callback args. |
| dotnet/src/Session.cs | Populates SessionId when invoking the token callback. |
| dotnet/src/BearerTokenProvider.cs | Adds SessionId to .NET ProviderTokenArgs. |
Copilot's findings
- Files reviewed: 17/17 changed files
- Comments generated: 2
This comment has been minimized.
This comment has been minimized.
… docs and CodeQL findings Address post-merge review feedback from #1748 across all 6 SDKs: - Rename the BYOK token callback field getBearerToken/get_bearer_token/ GetBearerToken to bearerTokenProvider/bearer_token_provider/ BearerTokenProvider, and the callback type to BearerTokenProvider. The Provider suffix distinguishes the dynamic token source from the static bearerToken credential and aligns with the existing Rust trait and the SDK's *Provider value-producer precedent. In Java this also drops the double-get accessor (getGetBearerToken -> getBearerTokenProvider). - Fix docs that incorrectly described the callback and static apiKey/ bearerToken as mutually exclusive; the runtime applies precedence (the callback wins and the static credential is not sent). - Resolve 4 CodeQL findings: empty except in python; LINQ .Where filter, specific catch type, and HttpResponseMessage disposal in dotnet. Java was not build-verified locally (requires JDK 25); CI validates it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
changed the title
This comment has been minimized.
This comment has been minimized.
The args object is constructed solely by the SDK and only read inside the consumer callback, so it should not be mutable by consumers. - Java: make fields final, keep only the all-args constructor, and drop the no-arg constructor and the fluent setters. The runtime already builds it via new ProviderTokenArgs(providerName, sessionId). - Node: mark the interface fields readonly. .NET already uses init-only required properties and Rust passes a shared reference, so both are already immutable. Go and Python have no idiomatic readonly equivalent for these DTO shapes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Apply the exact reflows spotless computed (javadoc wrapping in the e2e test and ProviderTokenArgs, and the provider.getToken call in the dispatcher), and move BearerTokenProvider to its correct alphabetical position in the python package exports after the getBearerToken rename. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Cross-SDK Consistency Review ✅This PR touches all six SDK implementations (Node.js, Python, Go, .NET, Java, Rust). The changes are fully consistent across all languages. 1. Rename
|
| SDK | Old | New |
|---|---|---|
| Node.js | getBearerToken?: GetBearerToken / type GetBearerToken |
bearerTokenProvider?: BearerTokenProvider / type BearerTokenProvider |
| Python | get_bearer_token: GetBearerToken / alias GetBearerToken |
bearer_token_provider: BearerTokenProvider / alias BearerTokenProvider |
| Go | GetBearerToken GetBearerToken field + type |
BearerTokenProvider BearerTokenProvider field + type |
| .NET | GetBearerToken property |
BearerTokenProvider property |
| Java | interface GetBearerToken / accessor getGetBearerToken() / setter setGetBearerToken() |
interface BearerTokenProvider / accessor getBearerTokenProvider() / setter setBearerTokenProvider() |
| Rust | field get_bearer_token / builder with_get_bearer_token() |
field bearer_token_provider / builder with_bearer_token_provider() |
2. Add sessionId to ProviderTokenArgs
All six SDKs updated with idiomatic field names and wired from the generated ProviderTokenAcquireRequest:
| SDK | Field | Casing rationale |
|---|---|---|
| Node.js | readonly sessionId: string |
camelCase + readonly for immutability |
| Python | session_id: str |
snake_case |
| Go | SessionID string |
Go initialism convention (ID not Id) |
| .NET | public required string SessionId |
PascalCase, Id per .NET naming |
| Java | getSessionId() / final field |
immutable value class pattern |
| Rust | pub session_id: String |
snake_case |
3. Docs: precedence vs. "mutually exclusive"
Node.js, Python, Go, and .NET — which all had the incorrect "mutually exclusive" wording — have been corrected to describe the actual precedence behaviour. Rust and Java correctly had no such wording, so no doc change was needed for them.
4. CodeQL fixes
.NET: Where() LINQ filter, specific catch type, and HttpResponseMessage disposal. Python: intentional bare except now has an explanatory comment.
5. E2E tests
All six SDK E2E test files updated to use the new names and assert that sessionId/session_id/SessionID is non-empty in the callback arguments.
No cross-SDK inconsistencies identified. The API surface is parallel across all languages, accounting for idiomatic naming in each.
Generated by SDK Consistency Review Agent for issue #1796 · sonnet46 1.4M · ◷
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+268
to
+274
| var response = new HttpResponseMessage(HttpStatusCode.NotFound) | ||
| { | ||
| Content = new StringContent( | ||
| "{\"error\":{\"message\":\"fake byok endpoint\"}}", | ||
| System.Text.Encoding.UTF8, | ||
| "application/json"), | ||
| }); | ||
| }; |
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.
Follow-up to #1748 that addresses post-merge review feedback on the BYOK bearer-token (managed identity) support, across all six language SDKs (Rust, Node, Go, Python, Java, .NET). No runtime-repo changes are required.
1. Add
sessionIdto the provider-token callback argsA bearer-token provider callback can be registered at the client level and shared across many sessions. Previously such a callback had no way to tell which session triggered a given token request, making per-session token scoping/caching impossible. This surfaces the
sessionIdthe runtime already provides.sessionId(session_id/SessionID/SessionIdper language idiom) to the hand-writtenProviderTokenArgstype in each SDK.ProviderTokenAcquirerequest.sessionIdreaches the callback.The runtime already injects
sessionIdinto the wire params for every session-scoped RPC, and codegen already carries it on each generatedProviderTokenAcquireRequest. Only the consumer-facing args type and the glue that builds it from the request were missing the field.2. Rename
getBearerToken->bearerTokenProviderRename the callback config field
getBearerToken/get_bearer_token/GetBearerTokentobearerTokenProvider/bearer_token_provider/BearerTokenProvider, and the callback type toBearerTokenProvider.Providersuffix distinguishes the dynamic token source from the staticbearerTokencredential, matching the Azure SDK getBearerTokenProvider terminology raised in review.*Providervalue-producer precedent and the Rust pre-existingBearerTokenProvidertrait.getaccessor (getGetBearerToken()->getBearerTokenProvider()). The distinct staticgetBearerToken()String getter is unchanged.3. Fix precedence docs
The hand-written SDK docs incorrectly described the callback and the static
apiKey/bearerTokenas mutually exclusive. The runtime actually applies precedence: when a provider callback is set, it wins and the static credential is not sent. Docs updated (node/python/go/dotnet; rust/java had no such wording).4. CodeQL fixes
Resolve 4 findings flagged on #1748: empty
exceptin python; LINQ.Where()filter, specific catch type, andHttpResponseMessagedisposal in dotnet.Validation
Compiled on the rebased base: Rust (
cargo check --tests), Go (go build+ e2ego vet), Node (tsc --noEmit), Python (py_compile+ruff), .NET (dotnet build+ tests). Java verified by inspection (build requires JDK 25+, unavailable locally; CI validates).