agentHost: remove the claudeUseCopilotProxy setting - #329650
agentHost: remove the claudeUseCopilotProxy setting#329650Tyler James Leonhardt (TylerLeonhardt) merged 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Removes the obsolete host-wide Claude proxy setting now that transport is selected per session from the model provider.
Changes:
- Removes
claudeUseCopilotProxyfrom configuration and transport resolution. - Updates Claude transport and authentication tests.
- Adds and updates authentication decision documentation.
Show a summary per file
| File | Description |
|---|---|
src/vs/platform/agentHost/common/agentHostCustomizationConfig.ts |
Removes the configuration key and schema. |
src/vs/platform/agentHost/node/claude/claudeAgent.ts |
Removes the explicit proxy override lookup. |
src/vs/platform/agentHost/node/claude/claudeTransportMode.ts |
Reduces fallback resolution to three inputs. |
src/vs/platform/agentHost/test/node/claudeAgent.test.ts |
Updates Claude routing tests and fixtures. |
src/vs/platform/agentHost/test/node/claudeTransportMode.test.ts |
Updates the transport decision matrix. |
src/vs/platform/agentHost/test/node/agentHostGitHubEndpointService.test.ts |
Replaces the removed test configuration key. |
src/vs/sessions/services/sessions/common/session.ts |
Updates Unusable documentation. |
src/vs/sessions/contrib/providers/agentHost/test/browser/sessionTypeAuthRequirement.test.ts |
Updates merged-catalog test context. |
src/vs/sessions/CONTEXT.md |
Adds an authentication glossary. |
src/vs/sessions/docs/adr/0001-conditional-agent-window-auth.md |
Adds the authentication decision record and amendments. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 8
- Review effort level: Balanced
5825fca to
a8844f0
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (9)
src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts:285
- This state is now recoverable by GitHub sign-in: signed-out Claude with no local setup advertises an optional Copilot resource and zero models, but
authenticate()starts the proxy and refreshes the Copilot catalog. Classifying that state asUnusablesuppresses the per-type sign-in affordance (getSessionTypeAvailabilityseesrequired: falseand reports “No models”), notably when another native provider has already opened the window. Represent the empty, proxy-recoverable state asGitHubor carry an explicit recoverability signal instead.
* regardless of credentials (the Claude SDK's `supportedModels()` does exactly
* this). Providers are therefore expected to publish an empty catalog when they
* genuinely cannot run, and an empty catalog is what distinguishes
* {@link SessionTypeAuthRequirement.Unusable} from
* {@link SessionTypeAuthRequirement.None} here.
src/vs/platform/agentHost/common/agentHostCustomizationConfig.ts:23
- The setting remains documented throughout
node/claude/phase18-plan.md,phase19-plan.md, androadmap.md, including instructions to set it and assertions that it controls the current transport. Removing the schema while leaving those source documents unchanged makes the clean deletion incomplete and sends maintainers toward a nonexistent key; remove the obsolete plans or clearly update them for per-session provider routing.
DefaultShell = 'defaultShell',
/**
src/vs/sessions/browser/sessionsAuthGate.ts:35
- This file-level block is much longer than the repository's 1–2 sentence JSDoc convention and duplicates details already documented on the individual predicates below. Keep only the distinction between the window and per-type gates so the implementation remains scannable.
/**
* Predicates behind the Agents window's conditional authentication — when the
* window may open for a user who is signed out of GitHub.
*
* Two gates, at different altitudes, are easy to confuse:
src/vs/sessions/services/sessions/common/session.ts:47
- This enum JSDoc exceeds the repository's 1–2 sentence limit and embeds provider implementation details that are already drifting from the enum contract. Keep the type-level documentation to the contract and document provider-specific derivation at its implementation.
/**
* What a session type needs before it can serve a request.
*
* Deliberately three states rather than a boolean. A boolean collapses
* {@link Unusable} into {@link GitHub}, which turns "this agent cannot run" into
src/vs/platform/agentHost/node/claude/claudeTransportMode.ts:33
- This function JSDoc grows into a full decision record, exceeding the repository's 1–2 sentence JSDoc convention and making a four-branch pure function harder to scan. Keep the API contract here and retain longer architectural rationale in a dedicated design document.
/**
* Which transport should the Claude provider fall back to right now? Pure
* decision; precedence, highest first:
*
* 1. Feature flag off means today's default behavior (always proxy).
src/vs/platform/agentHost/node/claude/claudeModelSelection.ts:158
- The newly added picker-default narrative substantially extends an already long JSDoc and records an end-to-end observation that can drift with picker sorting. Keep only the invariant relevant to this merge function: input order does not determine the rendered default.
* Array order is *not* what picks the session default. The picker re-buckets the
* flat list by the `_meta` vendor token and renders group-by-group, so which
* model is pre-selected follows the group ordering — verified end-to-end: with
* both halves populated the Anthropic group sorts first and
* `@provider=anthropic:default` is pre-selected, i.e. the default routes native
src/vs/platform/agentHost/node/claude/claudeAgent.ts:689
- This added paragraph narrates downstream UI behavior and exceeds the repository's 1–2 sentence JSDoc convention. The load-bearing constraint can be stated directly without embedding the full auth-state rationale here.
* Gating the native half on {@link detectExistingClaudeSetup} is deliberate and
* load-bearing, not just an optimization. `supportedModels()` returns a *static*
* list of models the SDK understands — it is not an entitlement or credential
* check, and it answers even with no `ANTHROPIC_API_KEY`, no
* `CLAUDE_CODE_OAUTH_TOKEN` and an empty `HOME`. Publishing it unconditionally
src/vs/platform/agentHost/test/node/claudeAgent.test.ts:903
- This helper JSDoc narrates setup and cleanup mechanics that are already obvious from the implementation, exceeding the repository's 1–2 sentence convention. A short purpose statement is sufficient.
/**
* Run `body` against a temp `$HOME/.claude/settings.json` carrying an Anthropic
* key so {@link detectExistingClaudeSetup} reports a usable native setup, then
* always clean the directory up. Pair with `allowSignedOutWhenUsable` to make a
* signed-out agent resolve its model-less default to native.
src/vs/sessions/browser/sessionsSetUpService.ts:277
- This method JSDoc expands into a multi-paragraph design rationale, contrary to the repository's 1–2 sentence JSDoc convention. The method name and called observable already convey the last-resort behavior; keep the contract concise.
/**
* The **window gate**: whether the Agents window must fall back to forcing
* GitHub sign-in before showing any of the sessions UI. Every caller is on a
* signed-out path, so this is simply the inverse of "can work without GitHub"
* — always true while the opt-in is off, which is today's mandatory-sign-in
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Since #329331 the Claude model picker shows Copilot-proxy and native-Anthropic models together, each carrying a provider-qualified id, and a session routes on the provider of the model it was started with (resolveClaudeSessionTransport). That makes the host-global override redundant — and, worse, incoherent. Model enumeration (_refreshModels) gates the proxy half on holding a GitHub token and the native half on detectExistingClaudeSetup; neither consulted the setting. getProtectedResources() advertises the Copilot resource required:false unconditionally. So `claudeUseCopilotProxy: false` never stopped Copilot-routed Claude from being offered or used by a signed-in user, which is exactly what its title ("Route Claude Through Copilot") promised. Its only remaining effect was on the model-less fallback, where an explicit `true` broke the one state the signed-out feature exists to serve: opt-in on, signed out, local Anthropic credential present — forced to proxy and dead-ended on AHP_AUTH_REQUIRED. resolveClaudeTransportMode drops its explicitProxy input and reduces to three: opt-in off => proxy; signed in => proxy; else local setup => native; else proxy. Clean deletion, not a deprecation: the key was never forwarded from a VS Code setting (AgentHostRootConfigForwarder never managed it) and was only ever hand-written into agent-host-config.json. A profile carrying a stale value starts with no error and drops the key on the first config write. Also corrects mergeClaudeModelCatalogs' claim that proxy-first ordering makes Copilot the session default. It does not: the picker re-buckets by the _meta vendor token, so the Anthropic group sorts first and the pre-selected model routes native. Verified end-to-end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a8844f0 to
617ece8
Compare
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Sandeep Somavarapu (@sandy081)Matched files:
Ladislau Szomoru (@lszomoru)Matched files:
|
Since #329331 the Claude model picker shows Copilot-proxy and native-Anthropic models together, each carrying a provider-qualified id (
@provider=copilot:…/@provider=anthropic:…), and a session routes on the provider of the model it was started with (resolveClaudeSessionTransport). The host-globalclaudeUseCopilotProxyoverride is no longer needed.Why it had to go — it was incoherent, not merely redundant
Model enumeration (
_refreshModels) gates the proxy half of the catalog on holding a GitHub token and the native half ondetectExistingClaudeSetup— neither consulted the setting — andgetProtectedResources()advertises the Copilot resourcerequired: falseunconditionally.So
claudeUseCopilotProxy: falsedid not stop Copilot-routed Claude from being offered or used by a signed-in user, which is precisely what its title ("Route Claude Through Copilot") promised it would do. A setting that contradicts its own description is worse than no setting.What it still did, post-#329331, was decide the fallback for a session whose model names no provider (model-less, or a bare/legacy id) — and there it was only ever load-bearing in one state: opt-in on and signed out and a local Anthropic credential present, where an explicit
trueforced proxy and produced a dead-endAHP_AUTH_REQUIREDfor a user who could have just worked. That is the state the signed-out feature exists to serve, so the override's only remaining effect was to break it.Change
resolveClaudeTransportModedrops itsexplicitProxyinput and reduces from five rules to four (three inputs):Removal is a clean deletion, not a deprecation: the key was never forwarded from a VS Code setting (
AgentHostRootConfigForwardernever managed it) and was only ever hand-written intoagent-host-config.json.Two intentional behavior changes
required: falseunconditionally, so with neither half of the catalog enumerable the published catalog is empty and the type resolves toUnusable("no models"). The proxy fallback only bites at use time, when a model-less/bare session materializes with no proxy handle and_ensureAuthenticatedraisesAHP_AUTH_REQUIRED.Unusablesurvives, on a different route — Copilot advertised as optional while publishing an empty model catalog (signed out, no local credential, so neither half of the merged catalog enumerates). The empty-catalog behaviour is unchanged; only the example that produces it moved.Drive-by fix:
mergeClaudeModelCatalogsdocumented the wrong defaultIts JSDoc claimed proxy-first ordering "preserves the picker's
models[0]-is-default convention for the common (Copilot) case." The e2e disproved this. The picker re-buckets the flat list by the_metavendor token and renders group-by-group, so array order does not select the default — the Anthropic group sorts first and@provider=anthropic:defaultis pre-selected, which routes native. Comment corrected, with an explicit warning not to infer the default from the merge order.Validation
npm run typecheck-clientclean. 241 targeted unit tests passing (ClaudeAgent,Claude transport mode,AgentHostGitHubEndpointService,Agent Host - session type auth requirement).End-to-end in the Agents window on a machine signed in to GitHub and holding local Anthropic credentials, so both halves of the catalog enumerate:
@provider=anthropic:default→ reply OK; log showsclaude-opus-5[1m], noClaudeProxyService POST /v1/messages→ direct to Anthropic@provider=copilot:claude-opus-4.6→ reply OK; log shows[ClaudeProxyService] POST /v1/messages+ a Copilot credits entryclaudeUseCopilotProxy: false(27 keys) into a profile → starts with no error; first config write emits 26 keys, that key the only difference. It self-cleans; no migration neededKnown gap (follow-up, not a regression)
No lever remains to express a transport preference, and the picker's default is not obviously the safe one — the pre-selected model routes native and bills the user's own Anthropic account (a one-word reply cost $0.38 on
claude-opus-5[1m]).The residual gap is not "everyone gets Copilot"; it is that whichever vendor group sorts first silently decides who pays. The fix is a default/sticky model preference (adjacent to #323049), not a transport flag — the transport is downstream of the model, and a flag would once again disagree with what the picker shows. That reasoning is recorded on
resolveClaudeTransportMode.Note on the docs
The rationale here previously lived in two markdown files under
src/vs/sessions/(an auth glossary and an ADR). They were never committed upstream, and they had already drifted from the code. This PR keeps the durable half and puts it on the code it constrains instead:SessionTypeAuthRequirement— why it is three states and not a boolean.resolveAgentAuthRequirement— why auth is derived from advertised protected resources rather than a static flag, whyrequired: falsebeats omitting the resource, and why the model count is the load-bearing second half._refreshModels— why gating the native catalog is load-bearing, not an optimization:supportedModels()is a static list that answers with no credentials at all, so an empty catalog is the honest signal.sessionsAuthGate.ts— window gate vs per-type gate._mustForceGitHubSignIn— the window gate as a deliberate last resort.resolveClaudeTransportMode— why no host-global transport preference exists.🤖 Generated with Claude Code