fix(providers): route GitHub Copilot Responses-only models off chat completions

[mushikingh]

Closes #748

Problem

The github-copilot preset configures a provider-wide openai-chat adapter. That is correct for most of the Copilot catalog, but Copilot fronts a mixed-wire catalog and several newer OpenAI models are only served by the Responses API:

model "gpt-5.6-sol" is not accessible via the /chat/completions endpoint

gpt-5.4 hides the same problem behind a passing smoke test. A minimal text-only Chat Completions request succeeds; a real Codex request fails, because Codex supplies function tools and a reasoning effort:

Function tools with reasoning_effort are not supported for gpt-5.4 in
/v1/chat/completions. To use function tools, use /v1/responses or set
reasoning_effort to 'none'.

modelAdapters already solves this, but every Copilot user has to discover the failure and hand-write the same map. The gap is the missing built-in mapping.

Change

Built-in per-model wire defaults — new src/providers/model-wire-defaults.ts holds a per-provider default map, consulted from resolveWireProtocolOverride. Precedence becomes:

hard pin → explicit modelAdapters → built-in default → provider adapter

Design notes:

• An explicit modelAdapters entry always wins, including one that names the provider-wide adapter — that is how a user opts a model back out of a default. The previous requested !== providerConfig.adapter short-circuit would have silently dropped such an entry into the default path, so the override is now resolved before that comparison.
• Defaults only apply while the provider is on an OpenAI-shaped wire (MODEL_ADAPTER_OVERRIDE_ALLOWED), so they never override a provider a user moved to some other adapter.
• Distinct from the hard pins in types.ts: a pin means the upstream speaks exactly one wire and a user override must not apply. These are defaults.
• Dated snapshots of a listed family (gpt-5.4-2026-07-30) resolve the same way — Copilot publishes dated ids (gpt-4o-2024-05-13).
• Behaviour for every provider without a map entry is byte-for-byte unchanged.

Models routed to openai-responses: gpt-5.3-codex, gpt-5.4, gpt-5.4-mini, gpt-5.5, gpt-5.6-luna, gpt-5.6-sol, gpt-5.6-terra.

Cold-start seed refresh — the Copilot registry seed listed gpt-4.1-mini and claude-sonnet-4, neither of which is in the live catalog. Refreshed from the verified catalog (embeddings and the internal trajectory-compaction entry excluded — they are not chat models) so a qualified selector resolves before live discovery lands.

Selector error hint — bare OpenAI-family ids are reserved for the canonical openai provider, so -m gpt-5.6-sol cannot mean Copilot and fails with Model gpt-5.6-sol requires the canonical openai provider. NoEnabledOpenAiProviderError now also names a qualified selector that would work when another enabled provider serves that id, instead of only suggesting ocx provider add openai. Message-only; routing is unchanged.

Verification

Field verification was done on OpenCodex 2.7.43 / Codex CLI 0.145.0 against a live 37-entry Copilot catalog, with real codex exec runs rather than minimal text probes — presence in GET /models does not prove a model works on /chat/completions, and a text-only success does not prove it survives function tools plus reasoning_effort. All 33 generative models returned successfully under the resulting mapping; before it, the seven above returned unsupported_api_for_model or the function-tools/reasoning incompatibility error.

In-repo:

• bun run typecheck clean.
• New tests/github-copilot-wire-defaults.test.ts covers both wire groups, dated snapshots, explicit-override precedence in both directions, an out-of-allow-list override falling through to the default, resolve idempotence, credential/destination survival across the swap, non-OpenAI-wire providers being left alone, and seed coverage.
• Additions to tests/adapter-resolve.test.ts (defaults do not leak to other providers) and tests/router.test.ts (the selector hint).
• tests/{adapter-resolve,router,reasoning-effort,effort-policy}.test.ts + the new file: 122 pass / 0 fail. The full bun run test suite was not run locally — please let CI be the gate.

Not changed

With adapter: "openai-responses" and no responsesPath, the URL builds as https://api.githubcopilot.com/v1/responses. Copilot's own documented path is /responses. The field verification reported HTTP 200 on real Codex runs through /v1/responses, so Copilot evidently accepts both and I left it alone rather than guess without a Copilot subscription to test against. Worth a follow-up if a maintainer can confirm.

Maintenance

The map needs a recheck whenever Copilot adds or changes models; the rationale and the verification caveat are recorded in the source comment. Automatic derivation from live discovery (if Copilot exposes per-model supported_endpoints) would be the durable fix and is deliberately out of scope here.

Docs

English guides/providers.md gains a "GitHub Copilot model wires" section (affected models, both error shapes, the opt-out example, and the qualified selector); reference/configuration.md notes built-in defaults on the modelAdapters row. ko/ja/zh-cn/ru get matching notes so the locales do not contradict the English source.

Summary by CodeRabbit

• New Features

  • GitHub Copilot model routing is now more accurate for newer Responses-only models, with clarified provider-qualified model selection and per-model adapter overrides.
  • Routed Responses requests can now recover from upstream auth failures (token refresh) and rate limits (key rotation).
  • Routing errors now suggest valid provider/model alternatives for bare OpenAI-family IDs.

• Bug Fixes

  • Improved handling of incompatible sampling parameters for routed Responses requests.

• Documentation

  • Updated provider/config guidance across languages on Copilot/GPT-5.6 preview routing rules and modelAdapters precedence.

• Tests

  • Added coverage for Copilot wire defaults, error messaging, and Responses recovery behavior.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b566a5d4-f73c-416a-89f4-36b26c15c6ce

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

GitHub Copilot now maps mixed model catalogs to openai-chat or openai-responses per model. Routing errors suggest qualified selectors, Responses requests gain bounded credential recovery, and documentation and tests cover configuration precedence, snapshots, request shaping, and provider usage.

Changes

GitHub Copilot mixed-wire routing

Layer / File(s)	Summary
Model wire defaults and adapter resolution src/providers/registry.ts, src/server/adapter-resolve.ts, tests/github-copilot-wire-defaults.test.ts, tests/adapter-resolve.test.ts, docs-site/src/content/docs/reference/configuration.md, docs-site/src/content/docs/ko/reference/configuration.md	Copilot model metadata maps Responses-only models to openai-responses, supports dated snapshots, preserves explicit overrides, and validates registry consistency and provider isolation.
Qualified model routing src/router.ts, tests/router.test.ts	Configured adapter keys become known model IDs, and bare OpenAI-family failures include usable provider/model alternatives.
Responses request handling and recovery src/server/responses/core.ts, src/server/responses-wire-params.ts, src/server/chat-completions.ts, src/server/claude-messages.ts, tests/github-copilot-responses-wire-recovery.test.ts	Routed Responses requests centralize unsupported-parameter stripping, preserve max_output_tokens outside forward mode, and retry bounded OAuth 401 and key-pool 429 failures with rebuilt transports.
Provider guidance docs-site/src/content/docs/guides/providers.md, docs-site/src/content/docs/ja/guides/providers.md, docs-site/src/content/docs/ko/guides/providers.md, docs-site/src/content/docs/ru/guides/providers.md, docs-site/src/content/docs/zh-cn/guides/providers.md	Documentation describes Copilot wire selection, modelAdapters, authentication details, failure behavior, and provider-qualified model selectors across locales.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Router
  participant AdapterResolver
  participant CopilotResponses
  participant CredentialRecovery
  Client->>Router: select github-copilot/model
  Router->>AdapterResolver: resolve model adapter
  AdapterResolver->>CopilotResponses: route Responses-only model
  CopilotResponses-->>CredentialRecovery: return 401 or 429
  CredentialRecovery->>CopilotResponses: refresh or rotate and replay
  CopilotResponses-->>Client: return response or final failure

Loading

Possibly related PRs

• lidge-jun/opencodex#463: Both changes modify routed Responses passthrough handling for non-2xx upstream results.
• lidge-jun/opencodex#749: Both changes modify replay-related control flow in src/server/responses/core.ts.

Suggested reviewers: lidge-jun, wibias, ingwannu

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ 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 states the main change: routing GitHub Copilot Responses-only models away from Chat Completions.
Linked Issues check	✅ Passed	The PR routes all listed Copilot models to the correct wire, preserves compatible chat models, and enforces the required github-copilot/ selector.
Out of Scope Changes check	✅ Passed	The extra routing, retry, and docs changes match the PR objectives and support the Copilot mapping fix, so no unrelated scope stands out.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Fix failing CI checks

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

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 `@docs-site/src/content/docs/guides/providers.md`:
- Line 93: Change the “GitHub Copilot model wires” heading from an h4 to an h3
so it is correctly nested beneath the surrounding “## 2. Account login (OAuth)”
section and satisfies the heading hierarchy.

In `@docs-site/src/content/docs/ja/guides/providers.md`:
- Around line 201-204: Update the model-routing descriptions in
docs-site/src/content/docs/ja/guides/providers.md lines 201-204 and
docs-site/src/content/docs/ko/guides/providers.md lines 201-204 to state that
gpt-5.4 remains routed to Responses by default, while text-only Chat Completions
are supported; retain the requirement for Responses when real Codex requests use
function tools and reasoning, without changing the handling of the other listed
models.

In `@docs-site/src/content/docs/ru/guides/providers.md`:
- Line 94: The Copilot provider guidance is incomplete in both translated pages.
Update docs-site/src/content/docs/ru/guides/providers.md lines 94-94 and
docs-site/src/content/docs/zh-cn/guides/providers.md lines 187-191 to include
the /chat/completions failure message, the gpt-5.4 text-only caveat, remaining
openai-chat behavior, wire-default details, and the manual selection command
codex -m github-copilot/...; keep each translation synchronized with the actual
CLI/API behavior.

In `@src/router.ts`:
- Around line 312-318: Update knownModelIdsForProvider, used by
providersServingModelId, to include the keys from prov.modelAdapters in the
returned model-ID set alongside existing sources. Add a router test covering a
provider configured only with modelAdapters, verifying its adapted model appears
among bare-model selector alternatives.

🪄 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 Plus

Run ID: 6da0615d-686e-4480-a737-ce612a5682da

📥 Commits

Reviewing files that changed from the base of the PR and between e449521 and ae0f9c4.

📒 Files selected for processing (14)

• docs-site/src/content/docs/guides/providers.md
• docs-site/src/content/docs/ja/guides/providers.md
• docs-site/src/content/docs/ko/guides/providers.md
• docs-site/src/content/docs/ko/reference/configuration.md
• docs-site/src/content/docs/reference/configuration.md
• docs-site/src/content/docs/ru/guides/providers.md
• docs-site/src/content/docs/zh-cn/guides/providers.md
• src/providers/model-wire-defaults.ts
• src/providers/registry.ts
• src/router.ts
• src/server/adapter-resolve.ts
• tests/adapter-resolve.test.ts
• tests/github-copilot-wire-defaults.test.ts
• tests/router.test.ts

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ae0f9c41dc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[mushikingh]

Codex review — all four findings verified and addressed (27a2b4e3)

I verified each finding against the code rather than taking it at face value. All four were real; two are resolved differently than suggested, with reasoning below.

1. Preserve compat sampling for Copilot Responses routes — confirmed, fixed

Confirmed at src/server/chat-completions.ts:87 and src/server/claude-messages.ts:593. The intent was already documented — claude-messages.ts says "Native ChatGPT passthrough (openai-responses forward) … routed providers keep them" — but the condition only tested adapter === "openai-responses", so it fired for every Responses route.

Gated on authMode === "forward" rather than isCanonicalOpenAiForwardProvider, because the adapter's own stripUnsupportedForwardParams (openai-responses.ts:495) already drops max_output_tokens on exactly that condition — so the two layers now agree instead of one being a narrower duplicate. The canonical check would also have broken tests/claude-messages-endpoint.test.ts, whose mock forward provider cannot match the pinned chatgpt.com/backend-api/codex URL.

I deliberately did not un-gate temperature / top_p / stop / user. stop is not a Responses parameter at all; every model this PR newly routes to Responses is a GPT-5.x reasoning model that rejects temperature/top_p; and noTemperatureModels and friends are honored only by the openai-chat adapter (openai-chat.ts:541), so the passthrough wire has no per-model sampling filter. Un-gating those would trade a silent drop for an upstream 400.

2. Keep OAuth 401 refresh for Copilot Responses routes — confirmed, fixed

4. Preserve key-pool 429 retry for Copilot Responses routes — confirmed, fixed

Both confirmed: core.ts:1393 enters the passthrough branch for any openai-responses provider, and the !upstreamResponse.ok path returned formatPassthroughUpstreamError immediately, with the 401 replay and 429 rotation living only in the normal adapter loop further down.

Added one bounded recovery block before the error formatting: at most one forced OAuth refresh, then one replay per remaining pool key, reusing the same helpers as the normal loop. Placed before sanitizePassthroughHeaders so resolvedModel, passthroughCt and isEventStream all read the final response, mirroring how the Codex pool retry already replaces upstreamResponse in place. Preconditions hold: nothing has streamed and the body is a replayable string — the same basis the transient-5xx retry above relies on. The failed response's socket is released before the refresh round-trip, not only before the replay, since a failed refresh returns early. noteAttemptSend is called per replay so the request log's attempt count stays honest.

Scoped to non-forward passthrough, so the canonical ChatGPT forward path is byte-for-byte unchanged.

I confirmed these actually fire rather than trusting green tests: instrumenting the 401 case showed the real sequence (/v1/responses 401 → copilot_internal/v2/token → identity lookup → replay with the new token), and the 429 case logs [key-failover] github-copilot: 429 on key k1; rotating to key k2.

3. Move Copilot wire defaults into the registry — confirmed, fixed

Agreed, and it was a direct src/AGENTS.md:18 violation. src/providers/model-wire-defaults.ts is deleted; the map is now modelWireDefaults on the github-copilot PROVIDER_REGISTRY entry, read at resolve time via providerModelWireDefault.

It is registry-only metadata, deliberately not in ProviderConfigSeed: seeding it into a saved config would freeze today's map on disk and make it indistinguishable from an explicit user modelAdapters entry, which must always win over a default. A new invariant test pins every default to a model the same entry seeds and to an allowed wire, which is what the two lists drifting would have cost.

Also folded in (overlapping CodeRabbit items)

• registry.modelWireDefaults and configured prov.modelAdapters keys now feed knownModelIdsForProvider, so a provider naming a model only in modelAdapters is selectable as provider/model and shows up in the bare-id selector hint. Covered in tests/router.test.ts.
• #### → ### heading nesting.
• The gpt-5.4 text-only caveat and the /chat/completions failure message carried into the ko/ja/zh-cn/ru locales.

Verification

bun x tsc --noEmit and bun run privacy:scan clean. New tests/github-copilot-responses-wire-recovery.test.ts covers 401 refresh+replay, repeated-401 replaying only once, key-pool 429 rotation, and max_output_tokens surviving on a routed gateway.

Full suite: 6069 pass, 6 fail. I checked every failure against the unmodified base by stashing:

Failing test	Branch	Base
claude-desktop-cli — persisted profile	fail	fail
claude-desktop-cli — import rejects invalid	fail	fail
cli-help — service usage remove alias	fail	fail (expectation predates repair)
provider-workspace-auth — logout/delete states	fail	fail (greps GUI for setToast(...), now showActionFeedback(...))
decodeSchtasksOutput — UTF-16LE BOM	fail	fail
cli-restore-back — ambiguous cleanup	fail (5017ms timeout)	passes in isolation — 5s-timeout flake under load

None are caused by this PR, and I did not touch GUI or Windows-scheduler code.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/server/chat-completions.ts (1)

86-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated Responses-wire sampling-param stripping across two handlers — extract a shared helper.

src/server/chat-completions.ts and src/server/claude-messages.ts both carry an identical block that conditionally strips max_output_tokens (forward-only) and unconditionally strips temperature/top_p/stop/user when route.provider.adapter === "openai-responses". This PR itself had to patch both copies in lockstep to fix the Copilot routed-gateway bug — proof that keeping this logic duplicated risks the two call sites silently diverging again the next time a Responses-wire nuance needs handling.

• src/server/chat-completions.ts#L86-L103: extract the store/max_output_tokens/temperature/top_p/stop/user shaping block (currently inline here) into a shared helper, e.g. applyResponsesWireParamShaping(internalBody, route.provider).
• src/server/claude-messages.ts#L600-L616: replace the identical inline block with a call to the same shared helper so both handlers stay in sync going forward.

♻️ Suggested shared helper sketch

// e.g. src/server/responses/param-shaping.ts
export function stripUnsupportedResponsesSamplingParams(
  body: Rec,
  provider: Pick<OcxProviderConfig, "authMode">,
): void {
  // Forward mode is the only route that 400s on max_output_tokens; a routed Responses
  // gateway (Copilot, api.openai.com, ...) honors the field.
  if (provider.authMode === "forward") delete body.max_output_tokens;
  // temperature/top_p/stop/user stay stripped on every Responses route.
  delete body.temperature;
  delete body.top_p;
  delete body.stop;
  delete body.user;
}

🤖 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/server/chat-completions.ts` around lines 86 - 103, Extract the
Responses-wire parameter shaping from the inline block near the route handling
in src/server/chat-completions.ts:86-103 into a shared helper that applies the
forward-only max_output_tokens removal and unconditional temperature, top_p,
stop, and user removal, while preserving store handling. Replace the duplicate
block in src/server/claude-messages.ts:600-616 with the same helper call so both
handlers use one implementation.

🤖 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/server/responses/core.ts`:
- Around line 1519-1576: Update the recovery replay loop around noteAttemptSend
in the routed Responses passthrough to track the recovery kind that triggered
each replay. Set the kind to "oauth-401" for the OAuth refresh branch and
"key-429" for the key-rotation branch, then pass it as the recovery argument to
noteAttemptSend so recoveryKinds reflects the corresponding send.

---

Outside diff comments:
In `@src/server/chat-completions.ts`:
- Around line 86-103: Extract the Responses-wire parameter shaping from the
inline block near the route handling in src/server/chat-completions.ts:86-103
into a shared helper that applies the forward-only max_output_tokens removal and
unconditional temperature, top_p, stop, and user removal, while preserving store
handling. Replace the duplicate block in src/server/claude-messages.ts:600-616
with the same helper call so both handlers use one implementation.

🪄 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 Plus

Run ID: c589c86f-972b-48df-96dc-b348f4d1ae25

📥 Commits

Reviewing files that changed from the base of the PR and between ae0f9c4 and 27a2b4e.

📒 Files selected for processing (14)

• docs-site/src/content/docs/guides/providers.md
• docs-site/src/content/docs/ja/guides/providers.md
• docs-site/src/content/docs/ko/guides/providers.md
• docs-site/src/content/docs/ru/guides/providers.md
• docs-site/src/content/docs/zh-cn/guides/providers.md
• src/providers/registry.ts
• src/router.ts
• src/server/adapter-resolve.ts
• src/server/chat-completions.ts
• src/server/claude-messages.ts
• src/server/responses/core.ts
• tests/github-copilot-responses-wire-recovery.test.ts
• tests/github-copilot-wire-defaults.test.ts
• tests/router.test.ts

[mushikingh]

CodeRabbit follow-up review — both findings applied (5e4bf388)

1. Recovery replay doesn't record which recovery kind fired — valid, fixed

Correct, and worth fixing on its own terms: "oauth-401" and "key-429" are already members of AttemptRecoveryKind (src/usage/log.ts:10), and noteAttemptSend pushes them onto attempt.recoveryKinds (request-log.ts:842). Omitting the third argument meant a request that silently self-healed would show an inflated sendCount with no reason attached — the opposite of what this path is for.

Threaded through as suggested, typed as AttemptRecoveryKind (already imported at core.ts:125) rather than a fresh string-literal union, so it stays tied to the shared enum.

2. Duplicated Responses-wire sampling stripping — valid, fixed

Fair, and the evidence was in the diff itself: I had to patch both handlers in lockstep. Extracted to stripUnsupportedResponsesSamplingParams in a new src/server/responses-wire-params.ts, called from both.

Two deliberate deviations from the sketch:

• store stays inline. Only the Chat Completions handler has a non-Responses default (else if (internalBody.store === undefined) internalBody.store = false), so folding it in would give the Anthropic handler behavior it never had.
• Placed at src/server/responses-wire-params.ts, not src/server/responses/param-shaping.ts. Everything under src/server/responses/ is internals of handleResponses; this helper belongs to the inbound compat handlers that sit beside it, and a flat sibling module keeps that boundary honest per src/AGENTS.md.

Verification

bun x tsc --noEmit and bun run privacy:scan clean. Focused run of the wire-default, recovery, router, chat-completions and claude-messages suites: 134 pass / 0 fail.

Full suite: 6069 pass, 6 fail — byte-identical to the previous run and to the base: claude-desktop-cli (x2), cli-help service usage, provider-workspace-auth GUI grep, decodeSchtasksOutput, and cli-restore-back (passes in isolation, 5s-timeout flake under load). No new failures.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/server/responses/core.ts (1)

1543-1549: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Propagate refreshed Kiro auth context before replay.

The new replay updates only route.provider.apiKey, but passthroughAdapter.buildRequest(parsed, ...) also consumes parsed._kiroAuthContext. The existing recovery path updates this field from refreshed.kiro at Lines 2237-2239; without the same update here, a Kiro OAuth refresh can replay with stale context and fail again.

🔧 Proposed fix

          sentOAuthSnapshot = refreshed;
+         if (route.providerName === "kiro") {
+           parsed._kiroAuthContext = { ...(refreshed.kiro ?? {}) };
+         }
          nextProvider = resolveProviderTransport(

As per path instructions, src/** changes must flag provider/adapter contract drift and preserve shared routing/config behavior.

🤖 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/server/responses/core.ts` around lines 1543 - 1549, Update the OAuth
refresh replay path around sentOAuthSnapshot and nextProvider to also propagate
refreshed.kiro into parsed._kiroAuthContext before
passthroughAdapter.buildRequest(parsed, ...); preserve the existing
route.provider apiKey update, provider transport resolution, and shared
routing/config behavior.

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/server/responses/core.ts`:
- Around line 1543-1549: Update the OAuth refresh replay path around
sentOAuthSnapshot and nextProvider to also propagate refreshed.kiro into
parsed._kiroAuthContext before passthroughAdapter.buildRequest(parsed, ...);
preserve the existing route.provider apiKey update, provider transport
resolution, and shared routing/config behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 52683971-e920-4ef5-baf6-5b8f22962172

📥 Commits

Reviewing files that changed from the base of the PR and between 27a2b4e and 5e4bf38.

📒 Files selected for processing (4)

• src/server/chat-completions.ts
• src/server/claude-messages.ts
• src/server/responses-wire-params.ts
• src/server/responses/core.ts

[lidge-jun]

NEEDS-SECURITY-REVIEW — blocked on review, not on changes.

This is a bigger change than the title suggests, and that is why it needs a reviewer rather than a merge. Beyond src/providers/registry.ts:1198-1228 selecting the Responses wire for seven Copilot models and src/server/adapter-resolve.ts:26-38 applying the built-in destination defaults, src/server/responses/core.ts:1509-1579 adds OAuth token refresh with request replay on 401 and API-key pool rotation with replay on 429.

Replay-after-credential-change is the part that needs someone hostile looking at it. The questions a reviewer should be able to answer from the code:

• is the replay bounded, or can a persistently-401 upstream drive it in a loop
• is the original request body still intact at replay time, and is the failed response fully drained first
• does the rotated or refreshed credential replace the old one everywhere the request carries it
• can a Copilot credential reach a non-Copilot provider or the canonical forward path through this code

MAINTAINERS.md:33-34 requires explicit security review for credential handling, and :35 adds that a promoted provider preset is itself a credential-destination change. Both apply.

Your tests cover wire selection, override precedence, destination and credential preservation, OAuth replay, pool rotation, and non-Copilot isolation. That is a genuinely useful starting point for the reviewer — it is not a substitute for the approval.

What happens next: request explicit security review. No changes are being asked for ahead of it.

---

CI context (shared across the current review round). dev is now at 62e937614. The baseline breakage that made every required check red is fixed: run 30545575865 has macos-latest, ubuntu-latest, and all three npm-global jobs green. The one remaining red is windows-latest, and it is not a test failure — it is a Bun 1.3.14 runtime panic (heap.zig:deleteMin reached through spawnSync with a timeout, triggered by our Windows ACL hardening path). That is a runtime defect being tracked separately, so please do not treat a red windows-latest as a signal about your change. Rebase onto current dev before your next push so your checks run against the repaired baseline.

[Wibias]

@mushikingh Pleasse resolve conflicts & CI Errors, then we will security review it.

[mushikingh]

Rebased onto current dev (675eb6ac) — conflicts resolved, diff substantially smaller

@Wibias @lidge-jun — rebased and force-pushed. The PR is MERGEABLE again.

Rebasing surfaced something worth flagging before the security review: dev has since landed the registry wire-default mechanism itself, so a large part of what this PR previously carried is now redundant and has been dropped rather than merged.

What dev already provides now

providerModelWireDefault in src/providers/registry.ts and the precedence in src/server/adapter-resolve.ts are already there — the same hard-pin → explicit modelAdapters → registry default → provider adapter ordering this PR proposed, plus an inbound: InboundWire parameter the earlier version did not have. I dropped my equivalents entirely and adopted yours; only the Copilot data remains.

Two details I took from your DeepSeek precedent:

• Copilot's entries are bare strings, not {wire, inbound}. DeepSeek scopes Flash to a responses inbound because it serves Chat natively too, so translating would add a hop for no gain. Copilot serves no chat route for these seven, so every inbound must be translated. Covered by a test asserting all three inbounds resolve to Responses.
• I did not add responsesPath. DeepSeek needs /responses with no /v1; the reporter's field verification showed Copilot returning HTTP 200 on real codex exec runs through the default /v1/responses construction, so I left it alone rather than guess without a Copilot subscription to test against.

What is still this PR

1. Copilot modelWireDefaults (7 Responses-only models) plus a cold-start seed refreshed from the verified 37-entry catalog — the old seed named gpt-4.1-mini and claude-sonnet-4, neither of which the catalog contains.
2. Sampling params on routed Responses gateways. Both inbound handlers still strip max_output_tokens for any openai-responses route, though claude-messages.ts documents the intent as forward-only ("routed providers keep them"). This became reachable the moment a registry default can put a chat/anthropic inbound on the Responses wire. Gated on authMode === "forward", matching stripUnsupportedForwardParams, and the duplicated block is now one helper.
3. Credential recovery in the passthrough branch — the part flagged NEEDS-SECURITY-REVIEW, unchanged in substance and now in its own commit (17e5263f) to make the review target clean.

The four security questions, answered from the code

• Bounded? Yes. oauth401Replayed latches after one refresh and rotateProviderTransportOn429 returns null when keys are exhausted; both guards are monotonic, so a persistently-401/429 upstream exits the loop rather than spinning.
• Body intact, failed response drained? The body is a replayable string and nothing has streamed — the same precondition the existing transient-5xx retry relies on. The socket is released before the refresh round-trip, not only before the replay, because a failed refresh returns early.
• Credential replaced everywhere? The adapter closes over the provider, so the request is rebuilt from the rotated provider via buildRequest rather than reusing the old one.
• Can a Copilot credential reach another provider or the forward path? No. The transport is rebuilt through resolveProviderTransport under the same route.providerName, which for Copilot re-validates the base URL to *.githubcopilot.com; and the whole block is gated on !isCanonicalOpenAiForwardProvider, so the forward path never enters it.

CI

The GUI baseline breakage is confirmed gone — gui && tsc -b is clean on this branch, where it had 6 errors before.

Full suite on the rebased branch: 6803 pass, 3 fail. I checked all three against upstream/dev in a clean worktree: the two claude-desktop-cli failures reproduce there identically, and cli-restore-back > sync exits nonzero when managed-default cleanup is ambiguous passes in isolation and only times out under full-suite load. bun x tsc --noEmit and bun run privacy:scan clean. Per your note I am treating a red windows-latest as the tracked Bun spawnSync panic, not a signal about this change.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 17e5263f87

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[Wibias]

[shipping-github] Security review

PR: #746 @ 8ea61aef
Decision: Pass after fixes
Risk: Medium

Findings

none confirmed

Summary

Reviewed the final head 8ea61aef, including the new replay-wire delta. The provider registry, routed Responses passthrough, OAuth refresh/replay, key-pool rotation, request rebuilding, Copilot destination validation, and compatibility-handler parameter shaping remain bounded and fail closed. Failed response bodies are cancelled before refresh/replay; refreshed or rotated credentials are carried into rebuilt requests; the replay now also preserves the original inbound wire when resolving an inbound-scoped default. The canonical ChatGPT forward path remains excluded, Copilot OAuth destinations remain constrained to the validated Copilot host policy, and no new secrets, permissions, workflows, or dependencies were introduced.

Residual

• The bridge depends on GitHub Copilot's unofficial upstream transport and catalog behavior; the PR documents that operational dependency.

Final-head delta reviewed

• 8ea61aef — routed Responses recovery rebuild now passes inboundWire, preventing an inbound-scoped registry default from changing protocol on replay.
• 2d131a9b — added the missing gpt-5.4-nano Responses mapping and kept Kiro OAuth context consistent during replay.
• 17e5263f — bounded routed Responses credential recovery and compatibility shaping.

[mushikingh]

Codex review 4834755084 — finding verified and addressed

@Wibias had already pushed 2d131a9b for this an hour before I got to it, so the nano and Kiro changes on the branch are yours, not mine. I rebased onto it rather than force-pushing over it, dropped my two now-redundant commits, and kept only the one delta that survived review of your version (8ea61aef).

The finding was real — verified, not taken at face value

gpt-5.4-nano appears nowhere in this repo, including src/codex/data/upstream-models.json, which carries the rest of the gpt-5.x family — so I checked the cited source rather than the third-party metadata link. GitHub's supported-models page does list GPT-5.4 nano as GA for Copilot. The reporter's 37-entry capture missed it because GET /models is plan-scoped.

One correction to the landed change

gpt-5.4-nano sits directly beneath the comment "Verified 2026-07-30 with real codex exec runs, not minimal text probes: presence in GET /models proves neither" — and it is the one entry that sentence is not true of. It rests on GitHub's published list plus the family pattern (every GPT-5.3+ entry Copilot serves is Responses-only; only the gpt-4o/4.1/gpt-5-mini tier answers chat). I have no Copilot subscription to confirm it, same reason I left responsesPath alone earlier.

I noted that at the entry rather than changing the value. The inference is sound and modelAdapters opts back out either way, but a reviewer signing off on a credential-destination table should be able to see which rows are measured and which are inferred.

The delta in 8ea61aef — a bug in my own earlier commit

The passthrough recovery loop rebuilt its adapter with:

resolveWireProtocolOverride(route.providerName, route.modelId, route.provider)

with no inbound argument, so it fell back to the "responses" default instead of the inbound the request arrived on. It was the only one of nine call sites in the file missing it — including the pre-existing 429 handler at core.ts:2116, which threads inboundWire through the identical call. A default scoped to a chat or anthropic inbound would resolve one wire on the first send and a different one on the replay, putting the retried request on a protocol the original never used.

Not reachable from a shipped entry today — Copilot's defaults are bare strings applying to every inbound, and DeepSeek's is scoped to responses, which is also the fallback — so no test catches it. It is what the next inbound-scoped default would inherit silently.

On the boundedness question from the security review

Chasing an apparent hang, I re-derived the bound from the implementation instead of restating my earlier claim. It holds, and here is the actual mechanism, which is worth having on record:

rotateKeyOn429 terminates the loop only because it cools the failed key (key-failover.ts:96-104) before selecting the next one, so each iteration retires exactly one pool key and the pool is exhausted after pool.length rotations. That depends on attemptedKey being findable in the pool — if it were not, currentEntry is undefined, nothing is cooled, currentIndex is -1, and the selector returns pool[0] forever. It is findable: resolveProviderTransport spreads ...provider and never rewrites apiKey (Copilot's transport only replaces baseUrl/headers). So the loop is bounded, but by the cooldown bookkeeping rather than by a counter — worth knowing if that helper ever changes.

The apparent hang was my own error: I ran the suite as bare bun test, which skips scripts/test.ts and so runs without --isolate and against the real $HOME instead of the sandboxed one. That leaked state across 477 files and produced a spin plus a handful of bogus failures. scripts/test.ts:38-51 documents this exact trap. No real config was written (~/.opencodex/config.json mtime unchanged).

Verification

bun x tsc --noEmit and bun run privacy:scan clean. Full suite on the merged tip: 6831 tests, 6819 pass, 2 fail, 309s — both failures are the pre-existing claude-desktop-cli pair (show --json … times out at 5s, and its unhandled JSON.parse is the reported "1 error"), which reproduce on upstream/dev in a clean worktree and are untouched by this branch.

[mushikingh]

Heads-up: the head moved 10 minutes after the security review

@Wibias — the review above is against 0ada48da. I pushed 8ea61aef at 00:00Z, ten minutes after it was posted, so the PR head is no longer the reviewed commit, and the change is in src/server/responses/core.ts — the replay path the review covers. Flagging rather than letting a passing review look like it covers code it never saw. I had not seen your review when I pushed.

The entire delta

 passthroughAdapter = resolveAdapter(
-  resolveWireProtocolOverride(route.providerName, route.modelId, route.provider),
+  resolveWireProtocolOverride(route.providerName, route.modelId, route.provider, inboundWire),
   config.cacheRetention,
 );

plus a comment on the gpt-5.4-nano registry row. No other executable change.

Against the properties you verified

• Replay bounded — untouched; no loop guard, counter, or exit condition changed.
• Bodies cancelled before refresh/replay — untouched.
• Refreshed/rotated credential carried into the rebuilt request — this is the only property in contact with the change, and it moves in the safe direction. The argument decides which wire the replay rebuilds on; passing inboundWire makes the replay resolve the same wire as the original send instead of silently falling back to the "responses" default. resolveWireProtocolOverride only ever selects between openai-chat and openai-responses and returns the config untouched for a canonical forward provider, so it cannot redirect a credential.
• Canonical ChatGPT forward path excluded — untouched; the enclosing !isCanonicalOpenAiForwardProvider gate is unchanged.
• Copilot destinations fail-closed — untouched; resolveProviderTransport and the base-URL validation are not in this diff.

So I read it as security-neutral, and mildly positive on the third point. That is my assessment of my own patch, not a substitute for yours.

Your call: re-run the review at 8ea61aef if you want the sign-off to match the head, or say the word and I will drop the commit so the branch is exactly the tree you passed. It is a latent-only fix — no shipped registry entry is inbound-scoped in a way that reaches it today — so deferring it to a follow-up PR costs nothing.

[lidge-jun]

Thanks for the careful work here and for following up on the review feedback. Before we can move this forward, please:

1. Resolve the open P2 catalog/selector findings from CodeRabbit.
2. Rebase this branch onto the current dev branch.
3. Because this change touches credential replay and failover behavior, it needs an explicit credential-replay security sign-off from a maintainer. Please request that review after the rebase so the sign-off covers the final diff.
4. Rerun CI after rebasing. The earlier Windows failure looked like the Bun crash pattern recently fixed on dev via test(storage): cap macOS isolate worker churn to avoid Bun segfault #849, so the rebase may clear it.

The routing fix itself is correct and wanted. Thank you for pushing it forward.