Per-session GitHub authentication for all SDK languages#1124
Per-session GitHub authentication for all SDK languages#1124SteveSandersonMS wants to merge 16 commits intomainfrom
Conversation
- Add githubToken to SessionConfig and ResumeSessionConfig
- Pass githubToken through in createSession/resumeSession RPC calls
- Auto-generated session.rpc.auth.getStatus() for querying session identity
- Auto-generated optional params for models.list and account.getQuota
Codegen improvements:
- Handle Zod optional params (anyOf with {not:{}}) without self-referential types
- Make function params optional when schema params are optional
- Remove unnecessary Omit<..., 'sessionId'> since sessionId is already
stripped from generated type definitions
Test infrastructure:
- Add /copilot_internal/user mock endpoint to ReplayingCapiProxy
- Add setCopilotUserByToken to CapiProxy for per-token identity responses
- Add COPILOT_DEBUG_GITHUB_API_URL to test env for token resolution
E2E tests:
- Single token auth status verification
- Two-session isolation with different tokens
- Unauthenticated session without token
- Invalid token error handling
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
- C# SetCopilotUserByTokenAsync: use typed records with source-generated
JsonSerializerContext instead of anonymous types (AOT requires it)
- Python codegen: fix _patch_model_capabilities regex to handle
params_dict in addition to empty {} for optional-params methods
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1124 · ● 1.4M
That is not true. All languages use those types. |
Avoids falsy-check inconsistency with other SDKs — empty string would now be sent rather than silently dropped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1124 · ● 847.7K
There was a problem hiding this comment.
Pull request overview
Adds per-session GitHub authentication support across the SDKs, plus test-harness plumbing and codegen updates to support new auth- and schema-related RPC surface changes.
Changes:
- Add a session-level
gitHubToken/GitHubToken/github_tokento session create/resume flows (all SDK languages). - Extend the shared replay proxy/harness to mock
/copilot_internal/userresponses per bearer token for E2E tests. - Update codegen outputs (RPC + session events) and add per-session-auth E2E tests across languages.
Show a summary per file
| File | Description |
|---|---|
| test/snapshots/session/should_set_model_with_reasoningeffort.yaml | New replay snapshot for reasoning-effort model selection scenario. |
| test/harness/replayingCapiProxy.ts | Add per-token /copilot_internal/user mocking and config endpoint for tests. |
| scripts/codegen/utils.ts | Improve schema resolution by filtering Zod’s { not: {} } optional wrapper branches. |
| scripts/codegen/typescript.ts | TS RPC codegen updates for optional params + schema wrapper handling. |
| scripts/codegen/python.ts | Python RPC codegen updates for optional params + models.list patch regex tweak. |
| scripts/codegen/go.ts | Go codegen schema wrapper handling to avoid self-referential types. |
| scripts/codegen/csharp.ts | C# codegen refactor to support custom property type resolution in polymorphic types. |
| python/e2e/testharness/proxy.py | Add proxy helper to configure per-token /copilot_internal/user responses. |
| python/e2e/testharness/context.py | Expose per-token user config helper via the E2E context. |
| python/e2e/test_per_session_auth.py | New Python E2E coverage for per-session GitHub auth isolation/error cases. |
| python/copilot/generated/session_events.py | Regenerated Python session event types (new events + usage schema alignment). |
| python/copilot/generated/rpc.py | Regenerated Python RPC surface (auth status, optional params, permissions updates). |
| python/copilot/client.py | Add github_token parameter to create/resume session payloads. |
| nodejs/test/e2e/streaming_fidelity.test.ts | Update to new Node auth option naming. |
| nodejs/test/e2e/session.test.ts | Update to new Node auth option naming. |
| nodejs/test/e2e/per_session_auth.test.ts | New Node E2E coverage for per-session GitHub auth isolation/error cases. |
| nodejs/test/e2e/harness/sdkTestContext.ts | Update test harness client options to new token option naming. |
| nodejs/test/e2e/harness/CapiProxy.ts | Add proxy helper to configure per-token /copilot_internal/user responses. |
| nodejs/test/client.test.ts | Update unit tests to new Node auth option naming + error messages. |
| nodejs/src/types.ts | Add session-level token; rename client-level token option. |
| nodejs/src/generated/session-events.ts | Regenerated TS session event types (new events + usage schema alignment). |
| nodejs/src/generated/rpc.ts | Regenerated TS RPC surface (auth status, optional params, permissions updates). |
| nodejs/src/client.ts | Wire session-level token into create/resume; rename client-level token option usage. |
| go/types.go | Add per-session GitHub token to session config/resume config (not directly JSON-serialized). |
| go/rpc/generated_rpc.go | Regenerated Go RPC surface (auth status, optional params, permissions updates). |
| go/internal/e2e/testharness/proxy.go | Add proxy helper to configure per-token /copilot_internal/user responses. |
| go/internal/e2e/testharness/context.go | Expose per-token user config helper via the Go E2E context. |
| go/internal/e2e/rpc_test.go | Update calls to newly-parameterized server RPC methods. |
| go/internal/e2e/per_session_auth_test.go | New Go E2E coverage for per-session GitHub auth isolation/error cases. |
| go/generated_session_events.go | Regenerated Go session event types (new events + usage schema alignment). |
| go/client.go | Wire session-level GitHub token into create/resume RPC requests. |
| dotnet/test/PerSessionAuthTests.cs | New .NET E2E coverage for per-session GitHub auth isolation/error cases. |
| dotnet/test/Harness/E2ETestContext.cs | Expose per-token user config helper via the .NET E2E context. |
| dotnet/test/Harness/CapiProxy.cs | Add typed/AOT-safe JSON payloads for per-token user config endpoint. |
| dotnet/src/Types.cs | Add session-level GitHubToken to public session/resume config types. |
| dotnet/src/Generated/SessionEvents.cs | Regenerated .NET session event types (new events + usage schema alignment). |
| dotnet/src/Generated/Rpc.cs | Regenerated .NET RPC surface (auth status, optional params, permissions updates). |
| dotnet/src/Client.cs | Wire session-level GitHub token into create/resume request records. |
Copilot's findings
- Files reviewed: 32/38 changed files
- Comments generated: 7
| /** True when the raw params schema uses `anyOf: [{ not: {} }, …]` — Zod's pattern for `.optional()`. */ | ||
| function isParamsOptional(method: RpcMethod): boolean { | ||
| const schema = method.params; | ||
| if (!schema?.anyOf) return false; | ||
| return schema.anyOf.some( | ||
| (item) => | ||
| typeof item === "object" && | ||
| (item as JSONSchema7).not !== undefined && | ||
| typeof (item as JSONSchema7).not === "object" && | ||
| Object.keys((item as JSONSchema7).not as object).length === 0 | ||
| ); |
There was a problem hiding this comment.
isParamsOptional can never return true as written: it checks typeof (item as JSONSchema7).not === "object" but not is a schema object in the { not: {} } pattern. This prevents optional params from being detected (and downstream signatures won’t be marked optional). Consider checking item !== null, verifying not is an object, and then testing Object.keys(not).length === 0.
| if (hasNonSessionParams) { | ||
| sigParams.push(`params: Omit<${paramsType}, "sessionId">`); | ||
| const optMark = isParamsOptional(value) ? "?" : ""; | ||
| // sessionId is already stripped from the generated type definition, | ||
| // so no need for Omit<..., "sessionId"> | ||
| sigParams.push(`params${optMark}: ${paramsType}`); | ||
| bodyArg = "{ sessionId, ...params }"; | ||
| } else { |
There was a problem hiding this comment.
For session RPC methods where params is optional (params?: T), the generated request body uses { sessionId, ...params }. In TS this is a type error because params is T | undefined (and can’t be spread). Use a nullish-coalesced spread (e.g., ...(params ?? {})) or keep params required and provide an overload.
| /** Detect the Zod optional params pattern: `anyOf: [{ not: {} }, { $ref }]` */ | ||
| function isParamsOptional(method: RpcMethod): boolean { | ||
| const schema = method.params; | ||
| if (!schema?.anyOf) return false; | ||
| return schema.anyOf.some( | ||
| (item) => | ||
| typeof item === "object" && | ||
| (item as JSONSchema7).not !== undefined && | ||
| typeof (item as JSONSchema7).not === "object" && | ||
| Object.keys((item as JSONSchema7).not as object).length === 0 | ||
| ); | ||
| } |
There was a problem hiding this comment.
isParamsOptional has the same logic issue as the TS generator: it checks typeof (item as JSONSchema7).not === "object", which makes the { not: {} } optional-params pattern impossible to match. This prevents emitting optional params: ... | None = None signatures for optional RPC params.
| ?.filter((item): item is JSONSchema7 => { | ||
| if (typeof item !== "object") return false; | ||
| const s = item as JSONSchema7; | ||
| // Filter out null types and `{ not: {} }` (Zod's representation of "nothing" in optional anyOf) | ||
| if (s.type === "null") return false; | ||
| if (s.not && typeof s.not === "object" && Object.keys(s.not).length === 0) return false; | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
resolveObjectSchema filters anyOf/oneOf entries with typeof item !== "object", but null also has typeof === "object". If an anyOf branch is ever null, const s = item as JSONSchema7 will cause property access on null. Add an explicit item !== null guard before casting.
| /** | ||
| * Per-token responses for `/copilot_internal/user` endpoint. | ||
| * Key is the Bearer token (without "Bearer " prefix), value is the response body. | ||
| * When a request arrives with `Authorization: Bearer <token>`, the matching response is returned. | ||
| * If no match is found, a default response is returned. | ||
| */ | ||
| private copilotUserByToken = new Map<string, CopilotUserResponse>(); |
There was a problem hiding this comment.
The doc comment for copilotUserByToken says that when no token match is found “a default response is returned”, but the implementation returns a 401 with { message: "Bad credentials" }. Either update the comment to match the behavior, or implement the documented default response to avoid confusion in future tests.
| /** | ||
| * GitHub token to use for authentication. | ||
| * When provided, the token is passed to the CLI server via environment variable. | ||
| * This takes priority over other authentication methods. | ||
| */ | ||
| githubToken?: string; | ||
| gitHubToken?: string; | ||
|
|
||
| /** | ||
| * Whether to use the logged-in user for authentication. | ||
| * When true, the CLI server will attempt to use stored OAuth tokens or gh CLI auth. | ||
| * When false, only explicit tokens (githubToken or environment variables) are used. | ||
| * @default true (but defaults to false when githubToken is provided) | ||
| * When false, only explicit tokens (gitHubToken or environment variables) are used. | ||
| * @default true (but defaults to false when gitHubToken is provided) | ||
| */ |
There was a problem hiding this comment.
Renaming the public Node option from githubToken to gitHubToken is a breaking change (and existing docs in the repo still reference githubToken). Consider supporting both names (with one deprecated) to preserve backward compatibility, or keep githubToken as the public API and map it internally to the gitHubToken wire field.
See below for a potential fix:
githubToken?: string;
/**
* @deprecated Use `githubToken` instead.
*/
gitHubToken?: string;
/**
* Whether to use the logged-in user for authentication.
* When true, the CLI server will attempt to use stored OAuth tokens or gh CLI auth.
* When false, only explicit tokens (githubToken, gitHubToken, or environment variables) are used.
* @default true (but defaults to false when githubToken or gitHubToken is provided)
| async def set_copilot_user_by_token(self, token: str, response: dict) -> None: | ||
| """Register a per-token response for the /copilot_internal/user endpoint.""" | ||
| if not self._proxy: | ||
| raise RuntimeError("Proxy not started") | ||
| await self._proxy.set_copilot_user_by_token(token, response) |
There was a problem hiding this comment.
Type annotation for response is just dict, while the underlying proxy API uses dict[str, Any]. Using the more precise type here keeps the harness consistent and avoids type-check noise under ty.
This comment has been minimized.
This comment has been minimized.
SteveSandersonMS
changed the title
This comment has been minimized.
This comment has been minimized.
Cross-SDK Consistency Review ✅This PR maintains excellent cross-SDK feature parity. All four language implementations receive the same additions in a consistent manner: Features added to all SDKs
Naming conventionsAll naming follows the expected per-language idioms:
Breaking change noteThe Node.js client-level option is renamed from No cross-SDK consistency issues found.
|
This reverts commit bf36938.
Summary
Adds per-session GitHub authentication support across all SDK languages (Node, Python, Go, C#). This allows SDK clients to pass a githubToken when creating a session, so each session can authenticate as a distinct GitHub user instead of sharing the process-level auth.
Changes
Node SDK
Python SDK
Go SDK
C# SDK
Codegen
Testing
All languages have E2E tests that verify: