Skip to content

Proposal: Stop treating COPILOT_API_KEY as a BYOK upstream credential #4161

@lpcox

Description

@lpcox

Problem

resolveCopilotApiKey() in src/copilot-api-resolver.ts currently uses a simple fallback chain:

return env.COPILOT_API_KEY || env.COPILOT_PROVIDER_API_KEY;

This is structurally wrong because COPILOT_API_KEY is owned by the Copilot CLI as its session-token slot. In BYOK mode, gh-aw injects a placeholder sentinel (dummy-byok-key-for-offline-mode) into it so the CLI starts — that value is never a real upstream credential. Forwarding it to the api-proxy sidecar caused issue #4040 (upstream of gh-aw#35575).

The documented BYOK entry point is COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL (changelog). Non-BYOK Copilot auth uses COPILOT_GITHUB_TOKEN through a separate path (copilotGithubToken).

Proposed Change (from PR #4143)

PR #4143 (by @zarenner, from fork) proposes:

  1. resolveCopilotApiKey() only consults COPILOT_PROVIDER_API_KEYCOPILOT_API_KEY is no longer read as a BYOK credential
  2. One-time deprecation warning when COPILOT_API_KEY is set without COPILOT_PROVIDER_API_KEY, directing operators to use the correct env vars
  3. Latch mechanism (copilotApiKeyDeprecationWarned) prevents repeated warnings within a process, plus a __resetCopilotApiKeyDeprecationLatchForTesting() export for test isolation

Exact Code Changes

src/copilot-api-resolver.ts

  • Adds module-level copilotApiKeyDeprecationWarned latch
  • Exports __resetCopilotApiKeyDeprecationLatchForTesting() for test use
  • Rewrites resolveCopilotApiKey():
    • If COPILOT_PROVIDER_API_KEY is defined → return it (no warning)
    • If only COPILOT_API_KEY is defined → log warning once, return undefined
    • If neither is defined → return undefined

src/copilot-api-resolver.test.ts

  • Adds beforeEach to reset latch + spy on logger.warn
  • New test: "prefer COPILOT_PROVIDER_API_KEY when both set — no warn"
  • New test: "ignore COPILOT_API_KEY alone — resolves undefined + warns once + latch prevents re-warn"
  • Updated: "empty COPILOT_PROVIDER_API_KEY preserves empty string"
  • Updated: process.env default test uses COPILOT_PROVIDER_API_KEY

src/cli.test.ts

  • BYOK resolution test renamed and updated: asserts COPILOT_PROVIDER_API_KEY wins, legacy key alone returns undefined

Compatibility Impact

User config Before After
COPILOT_GITHUB_TOKEN only (common case) ✅ works ✅ works
COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL ✅ works ✅ works
Both COPILOT_API_KEY + COPILOT_PROVIDER_API_KEY ⚠️ COPILOT_API_KEY wins (wrong) COPILOT_PROVIDER_API_KEY wins
COPILOT_API_KEY only (no provider key, no GitHub token) ⚠️ forwarded as BYOK key ❌ returns undefined + deprecation warning

Only the fourth row is breaking — likely empty in practice since real Copilot auth always uses COPILOT_GITHUB_TOKEN and BYOK always uses COPILOT_PROVIDER_API_KEY.

Relationship to Other PRs

ADR Alignment

This change brings AWF into conformance with gh-aw ADR-26544 (BYOK-Copilot composition), which states the dummy key "MUST NOT be treated as a real credential" and the real credential path "SHALL remain isolated in the sidecar."

Source

PR #4143 by @zarenner — branch refactor/copilot-byok-provider-key-only on fork zarenner/gh-aw-firewall

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions