Skip to content

fix(providers): preserve explicit key mode when credentials are unavailable - #7304

Closed
luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:freshcodex/preserve-explicit-key-mode-20261018
Closed

luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:freshcodex/preserve-explicit-key-mode-20261018

Conversation

@luvs01

@luvs01 luvs01 commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Preserve an explicitly configured API-key auth lane for dual-auth providers when the active key is paused or cannot be resolved, instead of unintentionally switching to a stored OAuth account.
  • Keep explicit OAuth and registry-default behavior unchanged. Cover xAI and Copilot pause/resume, unavailable environment references, and healthy key-pool rotation; align initial model selection with the same auth-mode decision.

Verification

Recorded implementation checks; no tests were rerun during publication:

  • Focused provider/routing regressions on the initial candidate: baseline 100 passed, 9 failed; candidate 109 passed, 0 failed.
  • Initial-candidate TypeScript, privacy scan, and structure checks: PASS.
  • Previous-head Linux CI: at 8354d6fb, test 4/4 batch 33/62 reported 215 passed, 2 failed. Both failures were in tests/oauth/oauth-upsert-preserves-api-key.test.ts: the unavailable-key cases still expected oauth although they explicitly preserved saved authMode: "key". This follow-up updates those two expectations and their descriptions, and restores the split-env fixture in finally. Explicit OAuth behavior and env-key recovery contrasts are preserved.
  • Follow-up focused verification: official Bun 1.4.3, through scripts/test.ts ./tests/oauth/oauth-upsert-preserves-api-key.test.ts with repository preload and shared test lock: 33 passed, 0 failed, 169 assertions. The explicit OAuth contrast passed. The earlier 109-test set was not repeated.
  • Follow-up tsc --noEmit and privacy scan: PASS, executed with the existing Bun 1.4.2 launcher. Scoped file-size ratchet for all 12 candidate files: PASS using Bun 1.4.3. git diff --check and full-patch reverse applicability check: PASS.
  • Separate local Windows adjacent OAuth/account-auto-pause set: 29 passed, 43 failed, 44 errors. These failures remain unresolved.
  • One isolated adjacent case failed under its original 5-second timeout on both baseline and candidate. An extended 30-second baseline diagnostic measured startServer at 6.75 seconds and mocks at 0.5 seconds; this diagnostic does not replace or turn the original failure into a pass.
  • test:changed and the full suite remain unrun under the shared-host resource exception. The previous failed CI run was not rerun.
  • Publication readback verified the follow-up changes exactly one file, has the previous PR head as its sole parent, carries a GitHub-verified signature, and leaves all 12 remote blobs matching the candidate manifest.
  • Current-head Linux CI: run 39119217985 completed successfully at 4644880893953aab7e733ed4f9571889c00703aa, including test shards 1–4 and the aggregate ci check. This current-head success is separate from the previous-head CI failure and the local 33-test result. Skipped platform lanes are not passing cross-platform evidence.
  • Review readiness uses the documented shared-host resource exception and the scoped local regressions plus current-head Linux CI. The separate Windows adjacent failures and unrun full/import-connected suites above remain limitations. There are no current review threads or submitted reviews; CodeRabbit explicitly skipped Draft review and still needs to review after Ready. Maintainer review, including the required authentication/security review, remains outstanding.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • The owning structure documentation was updated.
  • Authentication changes were independently reviewed for secrets and unsafe defaults.

Review readiness

  • Required local validation passed with its scope documented.
  • Branch is on the latest dev commit.
  • All correct Codex and CodeRabbit findings are fixed.
  • Ready-for-review confirmation.

Summary by CodeRabbit

  • Bug Fixes
    • Explicit key authentication remains selected even when a configured credential is temporarily unavailable, instead of silently switching to OAuth.
    • Requests without a usable key are rejected; restored or alternate available keys can be used when present. Explicit OAuth mode and default provider behavior remain unchanged.

…ilable

Keep an explicitly configured API-key auth lane when its credential is paused or cannot be resolved. Cover dual-auth provider pause/resume, unavailable references, and key-pool rotation.
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 885f6b1d-5215-4044-8b35-e4234400d504
📥 Commits

Reviewing files that changed from the base of the PR and between 8640c2f and 4644880.

📒 Files selected for processing: 12
  • scripts/test-layout/layout.json
  • src/providers/initial-model-selection.ts
  • src/providers/key-store.ts
  • src/providers/quota-routing-cache.ts
  • src/providers/routed-config.ts
  • structure/subagents.md
  • tests/fixtures/test-layout-expected.json
  • tests/oauth/oauth-upsert-preserves-api-key.test.ts
  • tests/providers/explicit-key-pause-oauth-boundary.test.ts
  • tests/providers/initial-model-selection.test.ts
  • tests/providers/resolved-model-policy.test.ts
  • tests/routing/router.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Provider override checks now preserve configured key mode for eligible OAuth providers even when a key is unavailable. Routing, registration, and tests cover unresolved keys, paused keys, and OAuth-mode behavior.

Changes: Explicit key mode

Layer / File(s) Summary
Apply configured key-mode override
src/providers/key-store.ts, src/providers/initial-model-selection.ts, src/providers/quota-routing-cache.ts, src/providers/routed-config.ts, structure/subagents.md
providerUsesKeyAuthOverride now checks registry eligibility and configured authMode, without requiring a resolved key. Routing and initial model selection use the updated check. The registration documentation describes unresolved key-mode providers as pending.
Verify key-mode routing boundaries
tests/providers/explicit-key-pause-oauth-boundary.test.ts, tests/oauth/oauth-upsert-preserves-api-key.test.ts, tests/providers/initial-model-selection.test.ts, tests/providers/resolved-model-policy.test.ts, tests/routing/router.test.ts, tests/fixtures/test-layout-expected.json, scripts/test-layout/layout.json
Tests cover unresolved credentials, paused and restored keys, multiple-key pools, and OAuth-mode behavior. Updated expectations verify key mode remains selected when a key cannot resolve, and transport construction rejects a missing credential. The fixture mapping adds the new test; the layout mapping has no semantic change.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 46448

The change preserves explicit key mode when a credential is unavailable. No actionable issue has been identified that should block merge after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 9 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preserving explicit key mode when provider credentials are unavailable. This matches the routing, model-selection, and OAuth regression-test …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title clearly and concisely describes the main change: preserving explicit key mode when provider credentials are unavailable. This matches the routing, model-selection, and OAuth regression-test changes.

Full details: Docstring Coverage

Explanation

Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 9 files. (3 skipped: 3 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 18, 2026
Align the two unavailable-key routing expectations with the explicit stored key-mode contract. Preserve explicit OAuth and environment-recovery contrasts, and restore the split-environment fixture in finally.
@luvs01
luvs01 marked this pull request as ready for review October 18, 2026 10:41
@Ingwannu

Copy link
Copy Markdown
Owner

Scoped code review at 4644880893: treating explicit authMode:key as intent rather than successful credential resolution addresses the lane-switch trigger; the four updated consumers now use the same predicate. The important acceptance boundary is zero OAuth selection/spend when the last key is paused, missing from env/keychain or disabled, with an OAuth record still present. Preserve explicit OAuth and registry rows that disallow key override as controls, including initial catalog selection and quota binding. Current Ubuntu CI is successful. I have not completed the credential-boundary review or independently executed the regression suite, so this comment is not approval/sponsorship.

lidge-jun added a commit that referenced this pull request Oct 19, 2026
…ate paths and shim token handling (B5) (#7397)

* fix(oauth): allow dismissing a settled failed login flow (carry #7379)

Co-authored-by: Ingwannu <186453546+Ingwannu@users.noreply.github.com>

* fix(providers): preserve explicit API-key mode when keys are unavailable (carry #7304)

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* fix(dsh): bound account profile response reads (carry #7333)

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* test(dsh): drain server fixtures before removing homes (carry #7305)

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* fix(claude-cli): keep local errors out of subscription cooldown (carry #7343)

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* fix(claude): bound Desktop library reads and preflight writes (carry #7301)

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* fix(service): stop scheduler wrappers before Windows reinstall (carry #7313)

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* fix(startup): require RunAtLoad before Desktop relaunch (carry #7277)

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* fix(codex): strip OpenSSL startup variables from updater child env (carry #7342)

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* fix(codex): keep the service token out of bypassed shim launches

All three Codex shims loaded the service API token into OPENCODEX_API_AUTH_TOKEN before checking OCX_SHIM_BYPASS, so a bypassed native passthrough launch handed the proxy data-plane token to the real Codex and its descendants. The POSIX, CMD and PowerShell shims now skip the token load under bypass; the Unix revision marker moves to 4 so installed wrappers are refreshed.

loadServiceTokenFromFile reads the inherited OCX_API_TOKEN_FILE through a no-follow, nonblocking descriptor, accepts only a regular file of at most 4 KiB and returns only a single-line printable token.

* test(claude): wait for a real ctime transition before requiring rejection

Re-applying the same utimes moves ctime only after the kernel clock ticks, so the real ctime case from #7301 often observed no change and the reader correctly returned {}. Repeat the call until ctime has advanced (bounded), assert that precondition, then require desktop_config_not_regular.

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* fix(google-antigravity): admit a probed project_id fallback in account import (carry #7385)

An Antigravity account whose Cloud Code Assist project is not auto-discovered was rejected with missing_project even when the operator knows its project. A Cockpit record may now carry a bounded project_id (Google Cloud project grammar, 6-30 chars, exact match). Discovery keeps precedence; the fallback is admitted only after the account quota probe succeeds with that account own access token against that project. Probe refusal, probe errors and cancellation write nothing. No shared default project.

Co-authored-by: yobushka <51312852+yobushka@users.noreply.github.com>

* fix(quota): name a closed reason when an OAuth account quota probe fails

Anthropic per-account quota rows reported quotaUnavailable with no reason, and Antigravity reported account_unavailable for any credential-side throw. The Anthropic usage read now reports a closed code for an HTTP status class, an unusable body or a thrown dispatch, and a probe that cannot obtain a credential reports account_unavailable without dispatching. Antigravity distinguishes a credential refresh deadline (timeout). Codes reuse the existing closed QUOTA_FAILURE_CODES set, carry no upstream text and never change account health.

* test(claude): match the symlink-resolved rollback target in the picker restore case

The atomic writer renames onto the symlink-resolved target, so on macOS (/var -> /private/var) the rollback rename never matched the fixture path and the restore-failure injection did not fire. Compare both spellings.

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* fix(google-antigravity): cancel the import project probe with the import request

probeAntigravityUsageQuota accepts an optional caller signal that cancels every request of the probe and stops before the models fallback; the import verifier forwards the import signal and never admits after cancellation.

* fix(codex): upgrade installed Windows shims that load the token before bypass

CMD and PowerShell wrappers now carry a Windows revision marker. A marked wrapper without it (installed before the bypass-token fix) is obsolete: installCodexShim and autoRestoreCodexShim rewrite it in place over the same backing launcher instead of reporting it healthy or already installed, and diagnostics report it unhealthy. On Windows the service token reader also binds the opened descriptor to the path by identity, so a link swapped in after lstat is refused.

* test(codex): mark the in-place Windows diagnostics fixture with the current revision

* fix(codex): refresh obsolete Windows shims without clobbering a concurrent replacement

The obsolete-wrapper refresh truncated whatever occupied the wrapper path. It now moves the observed wrapper aside and proves it is the probed file, stages the current wrapper beside it, and publishes with a hard link that refuses an occupied path. A concurrent replacement is restored or kept untouched and the refresh reports deferred.

* docs(structure): record the Windows shim refresh transaction

* fix(codex): create the Windows shim refresh stage exclusively and resume partial upgrades

The refresh stage is now created with O_CREAT|O_EXCL (and O_NOFOLLOW on POSIX) under an independent random name, and publication, withdrawal and cleanup are bound to the identity of the file this refresh created, so a planted link is never written through. Auto-restore resumes a Windows revision-only sibling mix (current plus obsolete wrappers) instead of deferring forever after one transient publish failure.

* fix(codex): bind Windows shim refresh rollback and recovery to observed identities

Publication keeps a linked entry only if it is the created stage, and withdraws a link to a swapped stage (file or symlink) while leaving independent replacements. Restoring the moved wrapper requires the aside to still be the moved file. Automatic revision recovery now refreshes only the admitted obsolete wrappers and first re-verifies every current sibling, so a replacement during the stability wait is never adopted.

* fix(codex): never displace a Windows shim where its replacement cannot be published

The refresh proves hard-link support in the wrapper directory with a throwaway pair before moving the wrapper aside, and if a restore link still fails it falls back to an identity-checked rename onto the empty path, so the launcher never stays missing.

---------

Co-authored-by: Ingwannu <186453546+Ingwannu@users.noreply.github.com>
Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Co-authored-by: yobushka <51312852+yobushka@users.noreply.github.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Carried into #7397 (squash dbb908f) on dev with a Co-authored-by trailer, as part of the B5 OAuth/accounts/service hardening bundle. It ships in the next release (2.90.0). Thanks!

@lidge-jun lidge-jun closed this Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants