Repository navigation
fix(copilot): auto-refresh Copilot token on 401 instead of only showing re-auth hint - #1766
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (10)src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{src/services/**/*.ts,src/utils/**/*.ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{src/integrations/**/*.ts,src/services/**/*.ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,py,json,md}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{ts,tsx,js,jsx,py}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**⚙️ CodeRabbit configuration file
Files:
**/*⚙️ CodeRabbit configuration file
Files:
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds Copilot token refresh on expired-token 401 responses and retries Copilot requests with the refreshed token. Includes helper logic plus tests for refresh success, failure, and retry behavior. ChangesCopilot 401 Token Auto-Refresh
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a70d581 to
f7013eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api/openaiShim.ts`:
- Around line 3325-3335: The token refresh logic in the GitHub Copilot 401
handler is gated on `isGithub`, which covers both Copilot and non-Copilot GitHub
routes, and does not verify that the failing credential is actually the stored
Copilot token. This can silently swap credentials from other sources (custom
headers, route credentials, or provider overrides) with the secure-storage
Copilot token. Change the condition from `isGithub && response.status === 401 &&
!didRefreshCopilotToken` to use `isGithubCopilot` instead of `isGithub`, and add
a check to ensure the failing credential is the stored Copilot token before
attempting to refresh and update headers.Authorization.
- Line 3336: The `continue` statement in the retry loop is consuming the final
iteration slot when it executes a refresh-only operation, causing the loop to
exit prematurely and hit the generic 500 error handler at line 3391 instead of
retrying with the refreshed token. Add a retry-budget guard before the
`continue` statement to prevent it from executing on the final iteration,
ensuring at least one retry attempt remains after token refresh occurs. This
will allow the original 401 error to be properly retried or surfaced instead of
being masked by the generic 500.
- Around line 3325-3340: The 401 "token expired" refresh logic with retry that
was added to _doOpenAIRequest is not applied to the performCodexRequest
function, which handles GitHub Copilot requests via the codex_responses path.
When performCodexRequest receives a 401 response with "token expired" in the
error body, it will throw an APIError without attempting token refresh and retry
like _doOpenAIRequest does. Add equivalent 401 token-refresh-and-retry handling
to performCodexRequest similar to what exists in _doOpenAIRequest (checking
isGithub, response status of 401, didRefreshCopilotToken flag, and "token
expired" message), call refreshCopilotTokenOn401(), update the Authorization
header with the new token, and retry the request. Alternatively, document why
the codex_responses path will not encounter this 401 scenario.
In `@src/utils/githubModelsCredentials.ts`:
- Around line 237-265: The function refreshCopilotTokenOn401() is used in the
401 retry logic but currently lacks test coverage. Add comprehensive tests in
the githubModelsCredentials.refresh.test.ts file covering these scenarios: the
success path where a token is refreshed and environment variables are updated,
early exit conditions for bare mode and direct API key usage, early exit for
direct-key credential types, handling missing or empty OAuth tokens, failure
cases when exchangeForCopilotToken throws an error, and failure cases when
saveGithubModelsToken returns an unsuccessful result. Ensure each test path
through the function (including try and catch blocks) is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bccb6db2-1196-4aa6-b19c-0913a79b3519
📒 Files selected for processing (2)
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim.ts
🔇 Additional comments (2)
src/services/api/openaiShim.ts (2)
42-45: LGTM!
3000-3000: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api/openaiShim.ts`:
- Line 14: Remove all code related to the OPENAI_API_KEYS credential-pool
rotation feature from the openaiShim.ts file. This includes deleting the
documentation comment about the OPENAI_API_KEYS environment variable at line 14,
removing the credential precedence logic around lines 102-112, deleting the
leasing and cooldown mechanism implementation in the 2202-2225 and 2874-3008
ranges, and removing the auth-failure retry behavior additions at lines
3186-3189, 3273-3288, and 3467-3486. Keep only the code necessary for the 401
token refresh fix, ensuring the changes remain focused on the single
auto-refresh problem.
- Around line 2998-3008: The issue is that `apiKeyRaw` and `singleAuthValue` are
computed once before the retry loop, so when `buildHeadersForAttempt()` is
called during retries after a 401 error, it still uses the stale expired token
instead of the refreshed one. Refactor `refreshCopilotTokenOn401()` to return
the refreshed token value directly instead of relying on implicit process.env
mutations, then capture and use this returned refreshed token in the retry path.
Update `buildHeadersForAttempt()` to accept and use the refreshed token when
available, ensuring that subsequent retries send the new token instead of the
originally captured stale credential.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 702de0fa-73fb-434c-943d-21eebc95aff2
📒 Files selected for processing (2)
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim.ts
🔇 Additional comments (1)
src/services/api/openaiShim.ts (1)
3452-3462: Previous 401-handler blockers still apply here.This branch is still gated by
isGithubinstead ofisGithubCopilotplus the original Copilot credential source, and the refresh-onlycontinuestill lacks a retry-budget guard for the final attempt.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api/openaiShim.ts`:
- Line 14: Remove all code related to the OPENAI_API_KEYS credential-pool
rotation feature from the openaiShim.ts file. This includes deleting the
documentation comment about the OPENAI_API_KEYS environment variable at line 14,
removing the credential precedence logic around lines 102-112, deleting the
leasing and cooldown mechanism implementation in the 2202-2225 and 2874-3008
ranges, and removing the auth-failure retry behavior additions at lines
3186-3189, 3273-3288, and 3467-3486. Keep only the code necessary for the 401
token refresh fix, ensuring the changes remain focused on the single
auto-refresh problem.
- Around line 2998-3008: The issue is that `apiKeyRaw` and `singleAuthValue` are
computed once before the retry loop, so when `buildHeadersForAttempt()` is
called during retries after a 401 error, it still uses the stale expired token
instead of the refreshed one. Refactor `refreshCopilotTokenOn401()` to return
the refreshed token value directly instead of relying on implicit process.env
mutations, then capture and use this returned refreshed token in the retry path.
Update `buildHeadersForAttempt()` to accept and use the refreshed token when
available, ensuring that subsequent retries send the new token instead of the
originally captured stale credential.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 702de0fa-73fb-434c-943d-21eebc95aff2
📒 Files selected for processing (2)
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.ts
📜 Review details
🔇 Additional comments (1)
src/services/api/openaiShim.ts (1)
3452-3462: Previous 401-handler blockers still apply here.This branch is still gated by
isGithubinstead ofisGithubCopilotplus the original Copilot credential source, and the refresh-onlycontinuestill lacks a retry-budget guard for the final attempt.
🛑 Comments failed to post (2)
src/services/api/openaiShim.ts (2)
14-14: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split
OPENAI_API_KEYScredential-pool rotation out of this PR.These hunks add a new env var, credential precedence, leasing/cooldown, and auth-failure retry behavior unrelated to the Copilot 401 refresh. That expands auth behavior in a PR scoped to one fix and needs its own issue, tests, and docs.
As per path instructions, “keep the change focused on the single ‘401 token expired → auto-refresh’ problem; avoid unrelated formatting/renames/dependency changes.”
Also applies to: 102-112, 2202-2225, 2874-3008, 3186-3189, 3273-3288, 3467-3486
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/openaiShim.ts` at line 14, Remove all code related to the OPENAI_API_KEYS credential-pool rotation feature from the openaiShim.ts file. This includes deleting the documentation comment about the OPENAI_API_KEYS environment variable at line 14, removing the credential precedence logic around lines 102-112, deleting the leasing and cooldown mechanism implementation in the 2202-2225 and 2874-3008 ranges, and removing the auth-failure retry behavior additions at lines 3186-3189, 3273-3288, and 3467-3486. Keep only the code necessary for the 401 token refresh fix, ensuring the changes remain focused on the single auto-refresh problem.Source: Path instructions
2998-3008: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Retry with the refreshed token, not the captured stale credential.
apiKeyRaw/singleAuthValueare computed before the retry loop, and eachcontinuerebuilds headers viabuildHeadersForAttempt(). Mutating this iteration’sheaders.Authorizationis discarded, so the retry sends the old expired token again.Minimal shape for a scoped retry token
const singleAuthValue = explicitCustomAuthHeaderValue || parseCredentialList(apiKeyRaw)[0] || apiKeyRaw + let refreshedCopilotApiKey: string | undefined const buildHeadersForAttempt = async ( credentialLease: CredentialLease | null, ): Promise<Record<string, string>> => { @@ const authValue = explicitCustomAuthHeaderValue || + (isGithubCopilot ? refreshedCopilotApiKey : undefined) || credentialLease?.value || (credentialPool ? '' : singleAuthValue) @@ const refreshed = await refreshCopilotTokenOn401() if (refreshed) { const newApiKey = process.env.OPENAI_API_KEY?.trim() || '' if (newApiKey) { - headers.Authorization = `Bearer ${newApiKey}` + refreshedCopilotApiKey = newApiKey + continue } - continue }Prefer having
refreshCopilotTokenOn401()return the refreshed token directly so the retry path does not useprocess.envas implicit IPC.Also applies to: 3456-3462
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/openaiShim.ts` around lines 2998 - 3008, The issue is that `apiKeyRaw` and `singleAuthValue` are computed once before the retry loop, so when `buildHeadersForAttempt()` is called during retries after a 401 error, it still uses the stale expired token instead of the refreshed one. Refactor `refreshCopilotTokenOn401()` to return the refreshed token value directly instead of relying on implicit process.env mutations, then capture and use this returned refreshed token in the retry path. Update `buildHeadersForAttempt()` to accept and use the refreshed token when available, ensuring that subsequent retries send the new token instead of the originally captured stale credential.
f7013eb to
6f86167
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api/openaiShim.ts`:
- Around line 3459-3465: The refreshed token obtained from
refreshCopilotTokenOn401() is stored locally but the next loop iteration calls
buildHeadersForAttempt() which rebuilds headers from pre-refresh credential
values, causing the expired token to be retried. Store the refreshed token from
the OPENAI_API_KEY environment variable in a persistent retry state variable
before the continue statement, then modify buildHeadersForAttempt() to use this
stored refreshed token when available instead of rebuilding from original
credentials. Additionally, validate that the stored refreshed token is different
from the Authorization header that failed to prevent reusing the same expired
credential.
In `@src/utils/githubModelsCredentials.refresh.test.ts`:
- Around line 166-183: The beforeEach hook acquires the shared mutation lock but
does not reset the auth environment variables to a known baseline before each
test. This causes tests to potentially skip the refresh path if
GITHUB_COPILOT_KEY or CLAUDE_CODE_SIMPLE are set in the local or CI environment.
In the beforeEach hook, after calling acquireSharedMutationLock, explicitly
clear or delete the GITHUB_COPILOT_KEY and CLAUDE_CODE_SIMPLE environment
variables to ensure each test starts with a clean, isolated state regardless of
the runner's starting environment configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 039668c1-771a-4bc1-927b-680a3ae4eba0
📒 Files selected for processing (3)
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.tssrc/utils/githubModelsCredentials.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (13)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/utils/githubModelsCredentials.refresh.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/utils/githubModelsCredentials.refresh.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/githubModelsCredentials.refresh.test.ts
…ng re-auth hint When GitHub Copilot returns 401 'token expired', the existing flow only showed a hint to run /onboard-github (the Twigpine#1042 fix) but never actually refreshed the token mid-session. Now _doRequest detects the 401 + 'token expired' body in GitHub mode, calls refreshCopilotTokenOn401() to exchange the stored OAuth token for a fresh Copilot token, updates process.env, and retries the request. Fixes Twigpine#1746
6f86167 to
4607516
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/services/api/openaiShim.ts (1)
3459-3470: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly continue when another attempt can use a different token.
This block can still consume the final loop slot, and it also
continues when refresh returns true butnewApiKeyis empty or equal tooldToken. In those cases the retry either exits as the generic 500 or repeats stale auth. Reserve a retry slot and movecontinueinside the branch that installs a different token.🐛 Proposed fix
- if (isGithubCopilot && response.status === 401 && !didRefreshCopilotToken) { + if ( + isGithubCopilot && + response.status === 401 && + !didRefreshCopilotToken && + attempt < maxAttempts - 1 + ) { const lowerBody = errorBody.toLowerCase() if (lowerBody.includes('token expired') || lowerBody.includes('token has expired')) { didRefreshCopilotToken = true - const oldToken = headers.Authorization?.replace(/^Bearer\s+/i, '') || '' + const oldToken = headers.Authorization?.replace(/^Bearer\s+/i, '').trim() || '' const refreshed = await refreshCopilotTokenOn401() if (refreshed) { const newApiKey = process.env.OPENAI_API_KEY?.trim() || '' if (newApiKey && newApiKey !== oldToken) { refreshedCopilotToken = newApiKey + continue } - continue } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/openaiShim.ts` around lines 3459 - 3470, In the GitHub Copilot 401 token refresh block within the retry loop, the `continue` statement is executed even when no new token is available or the token hasn't changed, wasting a retry slot. Move the `continue` statement from after the `if (newApiKey && newApiKey !== oldToken)` block to inside that conditional block so that the loop only continues when refreshedCopilotToken is actually set to a different value. This ensures retries only happen when a new token is available to use.src/utils/githubModelsCredentials.ts (1)
231-252: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCompare against the actual failed bearer, not global
OPENAI_API_KEY.This helper says it only refreshes when the failing credential matches the stored Copilot token, but it checks
process.env.OPENAI_API_KEY. In the shim, the sent header can come from a provider override, route credential, credential pool, or custom auth whileOPENAI_API_KEYstill contains the stored Copilot token, so this can refresh and swap credential sources. Pass the failed bearer into the helper and compare that toblob.accessToken.🛡️ Proposed fix
/** * Force-refresh the Copilot token on 401. + * + * `@param` failedCredential Bearer token from the failed request. * * Only refreshes when the failing credential matches the stored Copilot @@ -export async function refreshCopilotTokenOn401(): Promise<boolean> { +export async function refreshCopilotTokenOn401( + failedCredential: string, +): Promise<boolean> { @@ - const currentCredential = process.env.OPENAI_API_KEY?.trim() + const currentCredential = failedCredential.trim() if (!currentCredential || currentCredential !== blob.accessToken) return falseCall site:
- const refreshed = await refreshCopilotTokenOn401() + const refreshed = oldToken + ? await refreshCopilotTokenOn401(oldToken) + : falseAs per path instructions,
src/services/api/**auth/token handling must be reviewed with high scrutiny and block credential reuse mistakes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/githubModelsCredentials.ts` around lines 231 - 252, The refreshCopilotTokenOn401() function currently checks process.env.OPENAI_API_KEY to determine if the failing credential matches the stored Copilot token, but this is incorrect because the actual credential sent in the request might come from other sources like provider overrides, route credentials, or credential pools, while OPENAI_API_KEY still contains the stored Copilot token. This can cause the function to incorrectly refresh and swap credential sources. Modify the refreshCopilotTokenOn401() function to accept the actual failed bearer token as a parameter and compare that parameter directly against blob.accessToken instead of comparing process.env.OPENAI_API_KEY, ensuring refresh only occurs when the credential that actually failed matches the stored Copilot token.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/services/api/openaiShim.ts`:
- Around line 3459-3470: In the GitHub Copilot 401 token refresh block within
the retry loop, the `continue` statement is executed even when no new token is
available or the token hasn't changed, wasting a retry slot. Move the `continue`
statement from after the `if (newApiKey && newApiKey !== oldToken)` block to
inside that conditional block so that the loop only continues when
refreshedCopilotToken is actually set to a different value. This ensures retries
only happen when a new token is available to use.
In `@src/utils/githubModelsCredentials.ts`:
- Around line 231-252: The refreshCopilotTokenOn401() function currently checks
process.env.OPENAI_API_KEY to determine if the failing credential matches the
stored Copilot token, but this is incorrect because the actual credential sent
in the request might come from other sources like provider overrides, route
credentials, or credential pools, while OPENAI_API_KEY still contains the stored
Copilot token. This can cause the function to incorrectly refresh and swap
credential sources. Modify the refreshCopilotTokenOn401() function to accept the
actual failed bearer token as a parameter and compare that parameter directly
against blob.accessToken instead of comparing process.env.OPENAI_API_KEY,
ensuring refresh only occurs when the credential that actually failed matches
the stored Copilot token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66226b28-2734-41d6-8f4f-09757429ae2c
📒 Files selected for processing (3)
src/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.tssrc/utils/githubModelsCredentials.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.refresh.test.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/utils/githubModelsCredentials.refresh.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/utils/githubModelsCredentials.refresh.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/githubModelsCredentials.refresh.test.ts
🔇 Additional comments (2)
src/utils/githubModelsCredentials.refresh.test.ts (1)
141-422: LGTM!src/services/api/openaiShim.ts (1)
43-46: LGTM!Also applies to: 3008-3008, 3106-3107
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P1] Complete CodeRabbit's request to add 401 token-refresh handling for the GitHub Copilot
codex_responsespath
src/services/api/openaiShim.ts:2346-2369andsrc/services/api/codexShim.ts:645-668
The new 401 refresh logic is only installed inside_doRequest()for thechat_completionsflow. When GitHub Copilot routes usecodex_responses,openaiShim.tsdelegates toperformCodexRequest()incodexShim.ts, which throws aCodex API error 401without any refresh or retry. BecauseproviderConfig.tsselectscodex_responsesfor newer GitHub Copilot/GPT-5 Codex models, those models will still hit the stale-token 401 described in #1746. Please add equivalent "token expired" 401 handling in theisGithubWithCodexTransportbranch (or insideperformCodexRequest) so the refreshedOPENAI_API_KEY/GITHUB_TOKENis used and the request is retried, mirroring the_doRequestbehavior. -
[P2] Add integration coverage for the new Copilot 401 refresh retry path
src/services/api/openaiShim.ts:3452-3473
The unit tests forrefreshCopilotTokenOn401()cover the helper, but the actual retry loop that detects "token expired" and calls the helper has no test. Add anopenaiShim.test.tstest that simulates a GitHub Copilotchat_completionsrequest returning 401 with a "token expired" body, verifies thatrefreshCopilotTokenOn401()is invoked andprocess.env.OPENAI_API_KEY/GITHUB_TOKENare updated, and confirms the request is retried with the new token.
…ry integration test - Fix auth value precedence in buildHeadersForAttempt so refreshedCopilotToken takes effect even when a CredentialPool is active (single key still creates a pool, shadowing the refreshed token on retry) - Add integration test verifying GitHub Copilot 401 'token expired' triggers refreshCopilotTokenOn401() and retries chat_completions with the new token (P2 for PR Twigpine#1766 review)
23de8ec
P1 + P2 review items doneP1 — Codex transport 401 retry loop already addressed(line 2348-2391) catches with and for the path, calls , and retries. P2 — Integration test addedin :
Bug found during P2 — auth precedence in buildHeadersForAttemptwas placed after in the auth chain (). Since even a single API key creates a via , was always truthy on retry and shadowed the refreshed token. Fixed by promoting above . |
P1 + P2 review items doneP1 — Codex transport 401 retry loop already addressed
P2 — Integration test added
Bug found during P2 — auth precedence in buildHeadersForAttempt
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/services/api/openaiShim.ts (2)
2351-2384: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMatch the failed bearer before refreshing or overriding it.
refreshCopilotTokenOn401()validatesprocess.env.OPENAI_API_KEY, but Line 2351 and Line 3029 can send a provider override or credential-pool lease instead. A Copilot 401 from that credential can refresh the stored token and, through Line 3028, retry with a different account’s token. Gate refresh on the bearer that actually failed.Suggested fix direction
@@ - if (lowerMsg.includes('token expired') || lowerMsg.includes('token has expired')) { + const failedToken = apiKey.trim() + const currentCopilotToken = process.env.OPENAI_API_KEY?.trim() || '' + if ( + (lowerMsg.includes('token expired') || lowerMsg.includes('token has expired')) && + failedToken && + failedToken === currentCopilotToken + ) { didRefreshCopilotCodexToken = true const refreshed = await refreshCopilotTokenOn401() if (refreshed && process.env.OPENAI_API_KEY?.trim()) { continue @@ - didRefreshCopilotToken = true - const oldToken = headers.Authorization?.replace(/^Bearer\s+/i, '') || '' - const refreshed = await refreshCopilotTokenOn401() - if (refreshed) { - const newApiKey = process.env.OPENAI_API_KEY?.trim() || '' - if (newApiKey && newApiKey !== oldToken) { - refreshedCopilotToken = newApiKey + const oldToken = headers.Authorization?.replace(/^Bearer\s+/i, '').trim() || '' + const currentCopilotToken = process.env.OPENAI_API_KEY?.trim() || '' + if (oldToken && oldToken === currentCopilotToken) { + didRefreshCopilotToken = true + const refreshed = await refreshCopilotTokenOn401() + if (refreshed) { + const newApiKey = process.env.OPENAI_API_KEY?.trim() || '' + if (newApiKey && newApiKey !== oldToken) { + refreshedCopilotToken = newApiKey + continue + } } - continue }As per path instructions,
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}requires high scrutiny for “env precedence” and “auth/token handling” and blocks on “credential reuse mistakes.”Also applies to: 3026-3030, 3481-3492
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/openaiShim.ts` around lines 2351 - 2384, The error handling in the catch block that calls refreshCopilotTokenOn401() needs to verify that the bearer token which actually failed matches the one stored in process.env.OPENAI_API_KEY before attempting to refresh it. Currently, the request made by performCodexRequest could have used either this.providerOverride?.apiKey or the env var, but refreshCopilotTokenOn401() only refreshes process.env.OPENAI_API_KEY. Add a guard to check that the apiKey variable (which could be from the provider override) matches process.env.OPENAI_API_KEY before proceeding with the refresh, ensuring you only refresh the token that actually failed and avoiding credential reuse mistakes.Source: Path instructions
3481-3492: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLeave a retry slot for the refresh-only
continue.This branch can run on the last loop iteration after earlier 429, credential-pool, or local self-heal attempts. In that case, Line 3492 exits the loop and throws the generic 500 at Line 3568 instead of retrying or surfacing the original 401.
Minimal guard
- if (isGithubCopilot && response.status === 401 && !didRefreshCopilotToken) { + if ( + isGithubCopilot && + response.status === 401 && + !didRefreshCopilotToken && + attempt < maxAttempts - 1 + ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/openaiShim.ts` around lines 3481 - 3492, In the GitHub Copilot 401 token refresh handler within openaiShim.ts, the continue statement at line 3492 can execute on the final loop iteration, which causes the loop to exit prematurely and throw a generic 500 error instead of properly handling or retrying the request with the refreshed token. Add a guard condition before the continue statement to verify that there are remaining retry attempts available, ensuring the refresh-only path only proceeds when a retry iteration can actually be executed. Check the loop's retry counter or remaining attempts to implement this guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api/openaiShim.test.ts`:
- Around line 7702-7770: Add a new regression test for the GitHub Copilot 401
retry logic in the codex_responses transport path. Create a test similar to the
existing test named "GitHub Copilot 401 chat_completions retries with refreshed
token" but instead route through the codex_responses endpoint. Mock the fetch
calls to return a 401 status with "token expired" from the /responses endpoint
on the first call, verify that refreshCopilotTokenOn401 is invoked, and confirm
that the second attempt uses the refreshed token in the Authorization header.
This ensures the 401 retry behavior changed in the codex_responses branch
(referenced in lines 2349-2390 of openaiShim.ts) is properly covered.
---
Duplicate comments:
In `@src/services/api/openaiShim.ts`:
- Around line 2351-2384: The error handling in the catch block that calls
refreshCopilotTokenOn401() needs to verify that the bearer token which actually
failed matches the one stored in process.env.OPENAI_API_KEY before attempting to
refresh it. Currently, the request made by performCodexRequest could have used
either this.providerOverride?.apiKey or the env var, but
refreshCopilotTokenOn401() only refreshes process.env.OPENAI_API_KEY. Add a
guard to check that the apiKey variable (which could be from the provider
override) matches process.env.OPENAI_API_KEY before proceeding with the refresh,
ensuring you only refresh the token that actually failed and avoiding credential
reuse mistakes.
- Around line 3481-3492: In the GitHub Copilot 401 token refresh handler within
openaiShim.ts, the continue statement at line 3492 can execute on the final loop
iteration, which causes the loop to exit prematurely and throw a generic 500
error instead of properly handling or retrying the request with the refreshed
token. Add a guard condition before the continue statement to verify that there
are remaining retry attempts available, ensuring the refresh-only path only
proceeds when a retry iteration can actually be executed. Check the loop's retry
counter or remaining attempts to implement this guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ed1e1ef-d8e3-4a98-a04d-6271b4cd79dc
📒 Files selected for processing (2)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/services/api/openaiShim.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/services/api/openaiShim.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/api/openaiShim.test.ts
🔇 Additional comments (2)
src/services/api/openaiShim.ts (1)
43-46: LGTM!Also applies to: 3128-3129
src/services/api/openaiShim.test.ts (1)
8-8: LGTM!Also applies to: 102-106
Adds a new test 'GitHub Copilot 401 codex_responses retries with refreshed token' that verifies the 401 retry path for the codex_responses transport (lines 2349-2390). Mocks performCodexRequest to throw APIError(401, 'token expired') on first call and return a valid SSE response on second, verifies refreshCopilotTokenOn401 is invoked and the retry uses the refreshed token.
CodeRabbit: added codex_responses 401 retry integration testAdded
All 159 tests pass. |
jatmn
left a comment
There was a problem hiding this comment.
Findings
1. Inconsistent token-expired detection between chat_completions and codex_responses
Location: src/services/api/openaiShim.ts
_doOpenAIRequest(regular path) inspects the raw response body:
errorBody.toLowerCase().includes('token expired') || lowerBody.includes('token has expired')(lines 3482-3483)._doRequest(codex transport) inspects only the thrownAPIError.message:
lowerMsg.includes('token expired') || lowerMsg.includes('token has expired')(lines 2379-2380).
If GitHub Copilot returns a 401 where the expired-token signal lives in the response body but not in the top-level APIError.message, the codex transport will miss the refresh and surface the raw 401 to the user. The two transport paths should use the same source of truth for the expired-token signal — ideally the same lower-cased body check used by the error classifier (openaiErrorClassification.ts).
Suggestion: Promote the body check into a shared helper and call it from both paths, passing the original response body to the codex path.
2. Missing test for the credential-pool precedence fix
Commit 23de8ec explicitly promotes refreshedCopilotToken above credentialLease?.value in buildHeadersForAttempt because a CredentialPool could otherwise shadow the refreshed token on retry (lines 3027-3030). There is no test that exercises the refresh path while OPENAI_API_KEYS (the credential-pool env var) is active.
Suggestion: Add an integration test that sets OPENAI_API_KEYS to a pool containing the stale token, simulates a 401 "token expired", and asserts the retry uses the refreshed token, not the next key in the pool.
3. Edge cases and coverage gaps in the 401 refresh guard
The code checks for both "token expired" and "token has expired", but the test suite only uses "token expired". There is also no test verifying that a 401 with a non-expired body (e.g., "invalid token") does not trigger refresh.
Additionally, in _doOpenAIRequest (lines 3487-3492), if refreshCopilotTokenOn401() returns true but the new OPENAI_API_KEY equals the old token, refreshedCopilotToken is left undefined and the retry reuses the same stale token. Because didRefreshCopilotToken is already true, no second refresh is attempted; the request fails with 401 rather than a clearer signal that the refresh did not produce a different token.
Suggestion:
- Add tests for
"token has expired", for a 401 without the expired substring, and for the case where the refreshed token equals the stale token. - Consider logging a warning when the refresh returns the same token so debugging is easier.
…tests Finding 1: Promote token-expired detection into isCopilotTokenExpiredError() helper used by both chat_completions and codex_responses paths. Finding 2: Add credential-pool integration test verifying that refreshedCopilotToken takes precedence over credentialPool.next() when OPENAI_API_KEYS is active. Finding 3: Add edge case tests - 'token has expired' variant, 401 without expired substring (no refresh), and same-token refresh (retries with original token, exhausted, then fails).
Review findings addressedFinding 1 — Shared helper for token-expired detection Finding 2 — Credential-pool precedence test Finding 3 — Edge case coverage
All 163 tests pass. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/services/api/openaiShim.ts (1)
2384-2388: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winVerify the failed credential before refreshing.
Both retry paths still call
refreshCopilotTokenOn401()without proving that the 401 came from the stored Copilot token. That helper only checksprocess.env.OPENAI_API_KEY, so a Copilot request sent withproviderOverride.apiKeyor a custom auth header can still refresh against secure storage, and_doOpenAIRequest()can then retry withrefreshedCopilotTokeninstead of the credential that actually failed. Pass the failed bearer token into the refresh gate, or compare it here before calling the helper. As per path instructions,src/services/api/**changes must block on “credential reuse mistakes” in auth/token handling.Also applies to: 3485-3495
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/openaiShim.ts` around lines 2384 - 2388, The Copilot 401 retry flow in openaiShim is refreshing the stored token without first confirming that the failed request used the stored Copilot credential. Update the retry logic around isCopilotTokenExpiredError, refreshCopilotTokenOn401, and _doOpenAIRequest so the failed bearer token is checked against the stored OpenAI_API_KEY or passed into the refresh gate before refreshing. This should prevent retrying with refreshedCopilotToken when providerOverride.apiKey or a custom auth header was the credential that actually failed, and the same fix should be applied to both retry paths mentioned in the diff.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/services/api/openaiShim.ts`:
- Around line 2384-2388: The Copilot 401 retry flow in openaiShim is refreshing
the stored token without first confirming that the failed request used the
stored Copilot credential. Update the retry logic around
isCopilotTokenExpiredError, refreshCopilotTokenOn401, and _doOpenAIRequest so
the failed bearer token is checked against the stored OpenAI_API_KEY or passed
into the refresh gate before refreshing. This should prevent retrying with
refreshedCopilotToken when providerOverride.apiKey or a custom auth header was
the credential that actually failed, and the same fix should be applied to both
retry paths mentioned in the diff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ba6a030-5064-48eb-a7df-74b5a6110da1
📒 Files selected for processing (2)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (13)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/services/api/openaiShim.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/services/api/openaiShim.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/api/openaiShim.test.ts
Review findings P2 + P3P2 — refreshedCopilotCodexToken override for stale providerOverride.apiKey P3 — Guard All 164 tests pass, typecheck clean. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/api/openaiShim.ts (1)
3493-3497: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winVerify the failed Bearer token before refreshing.
Line 3497 calls
refreshCopilotTokenOn401()before provingoldTokenis the env-backed Copilot token. If a route/provider override or credential-pool token fails whileOPENAI_API_KEYstill matches secure storage, this can refresh and retry with a different Copilot credential. Gate the refresh on the actual failed header first.Minimal guard
if (isGithubCopilot && response.status === 401 && !didRefreshCopilotToken) { if (isCopilotTokenExpiredError(errorBody)) { - didRefreshCopilotToken = true const oldToken = headers.Authorization?.replace(/^Bearer\s+/i, '') || '' + const originalCopilotToken = openAIApiKey?.trim() || '' + if (!oldToken || oldToken !== originalCopilotToken) { + throwClassifiedHttpError( + response.status, + errorBody, + undefined, + response.headers as unknown as Headers, + requestUrl, + rateHint, + failure, + ) + } + didRefreshCopilotToken = true const refreshed = await refreshCopilotTokenOn401()As per path instructions,
src/services/api/**auth/token handling must block credential reuse mistakes and hidden fallback expansion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/openaiShim.ts` around lines 3493 - 3497, The Copilot 401 retry path in openaiShim’s token refresh flow is refreshing too early without confirming the failed Authorization header matches the env-backed Copilot token. Update the isGithubCopilot / isCopilotTokenExpiredError branch to compare the failed bearer token from headers.Authorization against the expected Copilot credential before calling refreshCopilotTokenOn401(), and only set didRefreshCopilotToken/retry when that match is proven.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 3493-3497: The Copilot 401 retry path in openaiShim’s token
refresh flow is refreshing too early without confirming the failed Authorization
header matches the env-backed Copilot token. Update the isGithubCopilot /
isCopilotTokenExpiredError branch to compare the failed bearer token from
headers.Authorization against the expected Copilot credential before calling
refreshCopilotTokenOn401(), and only set didRefreshCopilotToken/retry when that
match is proven.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf6b9284-1dba-4643-ac5e-4112600e70d6
📒 Files selected for processing (1)
src/services/api/openaiShim.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/services/api/openaiShim.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/api/openaiShim.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/services/api/openaiShim.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/services/api/openaiShim.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/services/api/openaiShim.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/openaiShim.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim.ts
🔇 Additional comments (1)
src/services/api/openaiShim.ts (1)
2354-2402: LGTM!Also applies to: 3038-3042, 3140-3141
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Verify failed credential matches env token before refreshing in chat path
src/services/api/openaiShim.ts:3493-3508
The chat path's 401 refresh branch callsrefreshCopilotTokenOn401()without first confirming that the failedAuthorizationheader matchesprocess.env.OPENAI_API_KEY. If the request failed because of aproviderOverride.apiKey, a route credential, or a custom auth header, the code can still refresh the env-backed Copilot token and retry withrefreshedCopilotToken, silently substituting the credential that actually failed. Thecodex_responsespath already guards this at line 2386 by comparingapiKey === (process.env.OPENAI_API_KEY ?? ''); please add the same guard in_doOpenAIRequestand add a regression test for the chat path with a mismatched provider override (for example, setproviderOverride.apiKeyto a stale token whileOPENAI_API_KEYmatches secure storage, and assert that no refresh occurs). -
[P3] Log refresh failures in
refreshCopilotTokenOn401
src/utils/githubModelsCredentials.ts:237-268
The entire refresh body is wrapped intry { ... } catch { return false }, so secure storage read failures, token exchange rejections, and persistence failures are swallowed without logging. This makes it hard to diagnose in production why a 401 "token expired" was not retried. Please log the caught error withlogForDebugging(or a similar project logging utility) before returning false.
P1: Add oldToken === (process.env.OPENAI_API_KEY ?? '') check in chat path before calling refreshCopilotTokenOn401(), matching the codex path guard. Prevents credential substitution when providerOverride.apiKey, route credential, or custom auth header was the failing credential. Add regression test for chat path with mismatched providerOverride. P3: Log caught error with logForDebugging before returning false in refreshCopilotTokenOn401() catch block, so secure storage read failures, token exchange rejections, and persistence failures are visible in debug logs instead of silently swallowed.
Review findings P1 + P3P1 — Chat path credential source gate P3 — Log refresh failures All 165 tests pass, typecheck clean. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/api/openaiShim.ts (1)
2349-2352: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGate the Codex refresh path to Copilot/GHE endpoints.
Line 2351 treats any GitHub-mode
codex_responsesrequest as Copilot, then appliesCOPILOT_HEADERSand Copilot refresh handling. Use the already-computedgithubEndpointTypeso GitHub Models/custom routes cannot enter this Copilot-only auth path.Suggested fix
const githubEndpointType = getGithubEndpointType(request.baseUrl) const isGithubMode = isGithubModelsMode() - const isGithubWithCodexTransport = isGithubMode && request.transport === 'codex_responses' + const isGithubCopilotEndpoint = + githubEndpointType === 'copilot' || githubEndpointType === 'ghe' + const isGithubWithCodexTransport = + isGithubMode && isGithubCopilotEndpoint && request.transport === 'codex_responses'As per path instructions, provider auth/token handling in
src/services/api/**must block credential reuse mistakes and hidden fallback expansion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/openaiShim.ts` around lines 2349 - 2352, The Codex refresh branch currently relies only on isGithubModelsMode() and request.transport, which can incorrectly send GitHub Models or custom GitHub routes through Copilot-only auth handling. Update the logic around githubEndpointType, isGithubMode, and isGithubWithCodexTransport so the Copilot refresh path is only entered for Copilot/GHE endpoints, and keep GitHub Models/custom endpoints out of the COPILOT_HEADERS and refresh-token reuse flow.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 2349-2352: The Codex refresh branch currently relies only on
isGithubModelsMode() and request.transport, which can incorrectly send GitHub
Models or custom GitHub routes through Copilot-only auth handling. Update the
logic around githubEndpointType, isGithubMode, and isGithubWithCodexTransport so
the Copilot refresh path is only entered for Copilot/GHE endpoints, and keep
GitHub Models/custom endpoints out of the COPILOT_HEADERS and refresh-token
reuse flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcb4ae4a-fba2-4be6-89e3-890b942c61fd
📒 Files selected for processing (3)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/utils/githubModelsCredentials.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (13)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/githubModelsCredentials.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/services/api/openaiShim.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/services/api/openaiShim.test.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/api/openaiShim.test.ts
🔇 Additional comments (3)
src/utils/githubModelsCredentials.ts (1)
4-4: LGTM!Also applies to: 238-273
src/services/api/openaiShim.ts (1)
3038-3041: LGTM!Also applies to: 3140-3141, 3493-3506
src/services/api/openaiShim.test.ts (1)
8129-8178: LGTM!
isGithubWithCodexTransport was using isGithubMode which is true for all GitHub modes including GitHub Models API and custom routes. Changed to isGithubCopilotEndpoint that checks githubEndpointType === 'copilot' || githubEndpointType === 'ghe', matching the chat path's isGithubCopilot guard. Prevents COPILOT_HEADERS and refreshCopilotTokenOn401 from being triggered for non-Copilot endpoints.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/api/openaiShim.ts (1)
3494-3498: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly mark Copilot refresh as consumed after confirming the failed token is eligible.
Line 3496 sets
didRefreshCopilotTokenbefore the bearer token is matched to the current Copilot env token. If a credential-pool or custom-auth attempt returns an expired-token 401 first, it burns the single refresh chance and can block a later eligible credential from refreshing.🐛 Minimal guard adjustment
if (isGithubCopilot && response.status === 401 && !didRefreshCopilotToken) { if (isCopilotTokenExpiredError(errorBody)) { - didRefreshCopilotToken = true const oldToken = headers.Authorization?.replace(/^Bearer\s+/i, '') || '' - if (oldToken === (process.env.OPENAI_API_KEY ?? '')) { + const currentCopilotToken = process.env.OPENAI_API_KEY?.trim() || '' + if (oldToken && oldToken === currentCopilotToken) { + didRefreshCopilotToken = true const refreshed = await refreshCopilotTokenOn401()As per path instructions,
src/services/api/**must review auth/token handling with high scrutiny and block credential reuse mistakes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/openaiShim.ts` around lines 3494 - 3498, The Copilot refresh flag is being consumed too early in the 401 handling path inside openaiShim.ts. In the branch that checks isGithubCopilot, response.status === 401, and isCopilotTokenExpiredError(errorBody), move the didRefreshCopilotToken assignment so it only happens after confirming the failed bearer token matches the current Copilot env token via the oldToken comparison. Keep the refresh attempt tied to the eligible token only, and ensure other auth attempts do not burn the single refresh chance before eligibility is verified.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 3494-3498: The Copilot refresh flag is being consumed too early in
the 401 handling path inside openaiShim.ts. In the branch that checks
isGithubCopilot, response.status === 401, and
isCopilotTokenExpiredError(errorBody), move the didRefreshCopilotToken
assignment so it only happens after confirming the failed bearer token matches
the current Copilot env token via the oldToken comparison. Keep the refresh
attempt tied to the eligible token only, and ensure other auth attempts do not
burn the single refresh chance before eligibility is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73fddb63-f885-4037-a3a3-79b2851fca1c
📒 Files selected for processing (1)
src/services/api/openaiShim.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/services/api/openaiShim.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/api/openaiShim.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/services/api/openaiShim.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/services/api/openaiShim.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/services/api/openaiShim.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/openaiShim.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim.ts
🔇 Additional comments (1)
src/services/api/openaiShim.ts (1)
2351-2403: LGTM!Also applies to: 3041-3042, 3141-3142
…heck The refresh flag was set before confirming oldToken matches process.env.OPENAI_API_KEY, burning the single refresh chance when the credential pool rotates to a non-matching key first. Moved inside the credential source check so it's only consumed for eligible credentials.
jatmn
left a comment
There was a problem hiding this comment.
This PR adds automatic GitHub Copilot token refresh on 401 "token expired" responses for both chat and Codex paths, with credential-source guards and retry-once behavior. I verified that the prior valid-reviewer requests from jatmn and CodeRabbit are addressed, and the focused and adjacent test suites, typecheck, build, and smoke all pass. One small hardening item remains in the chat path.
Findings
- [P3] Tighten the chat-path credential-source guard against empty Authorization headers
src/services/api/openaiShim.ts:3497
The guardoldToken === (process.env.OPENAI_API_KEY ?? '')is true when bothheaders.Authorizationandprocess.env.OPENAI_API_KEYare empty, so an unauthenticated Copilot request that returns 401 can still setdidRefreshCopilotToken = trueand callrefreshCopilotTokenOn401(). The refresh call will return false because no stored token matches, but the single refresh chance is burned for that loop. Please add anoldToken &&check (matching CodeRabbit's suggestion) so the refresh flag is only consumed for an actual eligible bearer token.
Prevents '' === '' match when both Authorization and OPENAI_API_KEY are empty from burning the refresh flag on an unauthenticated request.
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
@kevincodex1 LGTM
Fixes #1746
Problem
When GitHub Copilot returns 401 "token expired", the previous fix (#1042) only improved the error message hint to say "Run /onboard-github" — but never actually refreshed the token. Users had to manually notice the hint and re-authenticate, and even then the re-auth might not stick.
Solution
Added in that reads the stored OAuth token and exchanges it for a fresh Copilot token, then updates
process.env.Hooked this into
openaiShim.ts:_doRequest()retry loop: when GitHub mode is active, a 401 is returned, and the body contains "token expired", the token is refreshed and the request retried automatically.Changes
exchangeForCopilotToken(), persists the new token, updates env varsdidRefreshCopilotTokenguard to prevent infinite loopsSummary by CodeRabbit
Bug Fixes
401“token expired”/“token has expired” (case-insensitive), refreshing credentials when eligible, and retrying once with rotated authorization—covering both chat and Codex-style requests, including credential-pool scenarios. Refresh is skipped for bare/direct key usage, mismatched tokens, provider overrides, and non-matching401messages, with at-most-once refresh per retry cycle.Tests
openaiShimretry flow, includingrefreshCopilotTokenOn401success/failure cases, refresh trigger/skip conditions, same-token behavior, authorization header rotation, credential-pool handling, and enterprise/GHE refresh support.