From a0d78f2dd0eafc0bc533eafefdabfcd47f0ee216 Mon Sep 17 00:00:00 2001 From: Zachary Renner Date: Mon, 1 Jun 2026 12:07:08 -0700 Subject: [PATCH 1/2] fix(copilot-api-resolver): handle BYOK dummy sentinel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flip precedence so an explicitly-set COPILOT_PROVIDER_API_KEY wins over an inherited COPILOT_API_KEY, and treat gh-aw's BYOK dummy sentinel ('dummy-byok-key-for-offline-mode', introduced in github/gh-aw#33116) as 'not set' so it never shadows the real upstream key. Without this, gh-aw-compiled lock files run export COPILOT_API_KEY="$COPILOT_DUMMY_BYOK" before invoking awf, the dummy short-circuits the '||' fallback in resolveCopilotApiKey, and the api-proxy sidecar forwards the placeholder upstream — producing 401/503 'Credentials … are not configured' against Azure OpenAI / OpenAI-compatible BYOK endpoints. Fixes #4040. Refs github/gh-aw#35575. --- src/cli.test.ts | 6 ++++- src/copilot-api-resolver.test.ts | 26 +++++++++++++++++-- src/copilot-api-resolver.ts | 43 ++++++++++++++++++++++++++++++-- 3 files changed, 70 insertions(+), 5 deletions(-) diff --git a/src/cli.test.ts b/src/cli.test.ts index 99263afdf..698638145 100644 --- a/src/cli.test.ts +++ b/src/cli.test.ts @@ -349,10 +349,14 @@ describe('cli', () => { }); describe('Copilot BYOK env resolution', () => { - it('prefers COPILOT_API_KEY and falls back to COPILOT_PROVIDER_API_KEY', () => { + it('prefers COPILOT_PROVIDER_API_KEY and falls back to COPILOT_API_KEY', () => { expect(resolveCopilotApiKey({ COPILOT_API_KEY: 'primary-key', COPILOT_PROVIDER_API_KEY: 'fallback-key', + })).toBe('fallback-key'); + + expect(resolveCopilotApiKey({ + COPILOT_API_KEY: 'primary-key', })).toBe('primary-key'); expect(resolveCopilotApiKey({ diff --git a/src/copilot-api-resolver.test.ts b/src/copilot-api-resolver.test.ts index 234b4f685..72701e399 100644 --- a/src/copilot-api-resolver.test.ts +++ b/src/copilot-api-resolver.test.ts @@ -25,12 +25,32 @@ describe('resolveCopilotApiKey', () => { expect(resolveCopilotApiKey(env)).toBe('provider_key456'); }); - it('should prefer COPILOT_API_KEY over COPILOT_PROVIDER_API_KEY', () => { + it('should prefer COPILOT_PROVIDER_API_KEY over COPILOT_API_KEY', () => { + // COPILOT_PROVIDER_API_KEY is an explicit BYOK opt-in signal, so it + // takes precedence over an inherited COPILOT_API_KEY. This also covers + // the common case where gh-aw injects the BYOK dummy sentinel into + // COPILOT_API_KEY alongside a real COPILOT_PROVIDER_API_KEY. const env = { COPILOT_API_KEY: 'key123', COPILOT_PROVIDER_API_KEY: 'provider_key456', }; - expect(resolveCopilotApiKey(env)).toBe('key123'); + expect(resolveCopilotApiKey(env)).toBe('provider_key456'); + }); + + it('should treat the BYOK dummy sentinel in COPILOT_API_KEY as unset', () => { + // gh-aw's compiled lock injects this sentinel into COPILOT_API_KEY to + // satisfy the Copilot CLI's "must be set" check, but it must not be + // forwarded upstream. See github/gh-aw#33116 / github/gh-aw#35575. + const env = { + COPILOT_API_KEY: 'dummy-byok-key-for-offline-mode', + COPILOT_PROVIDER_API_KEY: 'real_provider_key', + }; + expect(resolveCopilotApiKey(env)).toBe('real_provider_key'); + }); + + it('should return undefined when only the BYOK dummy sentinel is set', () => { + const env = { COPILOT_API_KEY: 'dummy-byok-key-for-offline-mode' }; + expect(resolveCopilotApiKey(env)).toBeUndefined(); }); it('should return undefined when neither key is set', () => { @@ -39,6 +59,8 @@ describe('resolveCopilotApiKey', () => { }); it('should return empty string when keys are empty strings', () => { + // Empty string is preserved (distinct from "not set") so callers can + // distinguish an explicit clear from an absent variable. const env = { COPILOT_API_KEY: '', COPILOT_PROVIDER_API_KEY: '', diff --git a/src/copilot-api-resolver.ts b/src/copilot-api-resolver.ts index 52ffba427..5995d1111 100644 --- a/src/copilot-api-resolver.ts +++ b/src/copilot-api-resolver.ts @@ -3,14 +3,53 @@ import { deriveCopilotApiTargetFromProviderBaseUrl, } from './copilot-api-resolver.internal'; +/** + * Sentinel value injected by gh-aw's compiled lock-file `pathSetup` step as + * `COPILOT_API_KEY` whenever the user is operating in BYOK mode. The Copilot + * CLI requires *some* value in `COPILOT_API_KEY` to start up, but in BYOK + * mode the real upstream credential is `COPILOT_PROVIDER_API_KEY`, which is + * what the api-proxy sidecar must actually forward upstream. + * + * Treating this sentinel as "unset" lets the resolver fall through to + * `COPILOT_PROVIDER_API_KEY` instead of forwarding the placeholder, which + * would otherwise produce upstream 401/503 responses. + * + * See: github/gh-aw#33116 (introduces the sentinel) and + * github/gh-aw#35575 / github/gh-aw-firewall#4040 (the resulting BYOK + * regression this constant guards against). + */ +const COPILOT_BYOK_DUMMY_API_KEY = 'dummy-byok-key-for-offline-mode'; + +function cleanApiKey(value: string | undefined): string | undefined { + if (value === undefined) return undefined; + if (value === COPILOT_BYOK_DUMMY_API_KEY) return undefined; + return value; +} + /** * Resolve the Copilot BYOK key from supported environment variables. - * COPILOT_API_KEY takes precedence over COPILOT_PROVIDER_API_KEY. + * + * Precedence: + * 1. `COPILOT_PROVIDER_API_KEY` (explicit BYOK signal; user opted in to a + * non-Copilot upstream provider) + * 2. `COPILOT_API_KEY` (real Copilot token in non-BYOK mode) + * + * The BYOK dummy sentinel injected into `COPILOT_API_KEY` by gh-aw is treated + * as unset, so it never shadows a real `COPILOT_PROVIDER_API_KEY`. + * + * Empty string is preserved (not coerced to undefined) for callers that + * distinguish "explicitly set to empty" from "not set". */ export function resolveCopilotApiKey( env: Record = process.env ): string | undefined { - return env.COPILOT_API_KEY || env.COPILOT_PROVIDER_API_KEY; + const providerKey = cleanApiKey(env.COPILOT_PROVIDER_API_KEY); + if (providerKey !== undefined) return providerKey; + + const apiKey = cleanApiKey(env.COPILOT_API_KEY); + if (apiKey !== undefined) return apiKey; + + return undefined; } /** From 00a929c3b5eece23fd134f39b7381175bc805c8f Mon Sep 17 00:00:00 2001 From: Zachary Renner Date: Mon, 1 Jun 2026 12:36:15 -0700 Subject: [PATCH 2/2] refactor: inline sentinel check; only apply to COPILOT_API_KEY Drop the cleanApiKey() helper. The BYOK dummy sentinel is only ever written into COPILOT_API_KEY (by gh-aw's pathSetup), so checking it on COPILOT_PROVIDER_API_KEY was speculative defense-in-depth that created an asymmetry between code and rationale. Behavior change vs. the prior commit on this branch: if COPILOT_PROVIDER_API_KEY is literally set to the sentinel string (no current code path does this), it is now forwarded verbatim rather than ignored. --- src/copilot-api-resolver.ts | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/copilot-api-resolver.ts b/src/copilot-api-resolver.ts index 5995d1111..8cc3da43c 100644 --- a/src/copilot-api-resolver.ts +++ b/src/copilot-api-resolver.ts @@ -20,12 +20,6 @@ import { */ const COPILOT_BYOK_DUMMY_API_KEY = 'dummy-byok-key-for-offline-mode'; -function cleanApiKey(value: string | undefined): string | undefined { - if (value === undefined) return undefined; - if (value === COPILOT_BYOK_DUMMY_API_KEY) return undefined; - return value; -} - /** * Resolve the Copilot BYOK key from supported environment variables. * @@ -35,7 +29,10 @@ function cleanApiKey(value: string | undefined): string | undefined { * 2. `COPILOT_API_KEY` (real Copilot token in non-BYOK mode) * * The BYOK dummy sentinel injected into `COPILOT_API_KEY` by gh-aw is treated - * as unset, so it never shadows a real `COPILOT_PROVIDER_API_KEY`. + * as unset, so it never shadows a real `COPILOT_PROVIDER_API_KEY`. The + * sentinel check is only applied to `COPILOT_API_KEY` since that is the only + * variable gh-aw writes it into; `COPILOT_PROVIDER_API_KEY` is forwarded + * verbatim. * * Empty string is preserved (not coerced to undefined) for callers that * distinguish "explicitly set to empty" from "not set". @@ -43,11 +40,10 @@ function cleanApiKey(value: string | undefined): string | undefined { export function resolveCopilotApiKey( env: Record = process.env ): string | undefined { - const providerKey = cleanApiKey(env.COPILOT_PROVIDER_API_KEY); - if (providerKey !== undefined) return providerKey; + if (env.COPILOT_PROVIDER_API_KEY !== undefined) return env.COPILOT_PROVIDER_API_KEY; - const apiKey = cleanApiKey(env.COPILOT_API_KEY); - if (apiKey !== undefined) return apiKey; + const apiKey = env.COPILOT_API_KEY; + if (apiKey !== undefined && apiKey !== COPILOT_BYOK_DUMMY_API_KEY) return apiKey; return undefined; }