From dc074672ebb3d146e60fec7f1faa1ca7e654fe02 Mon Sep 17 00:00:00 2001 From: t Date: Sat, 5 Sep 2026 09:09:20 +0900 Subject: [PATCH 1/5] fix(claude): fall back to native launch when routing is off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ocx claude` hard-errored and returned 1 whenever Claude routing was disabled (`src/cli/claude.ts:420` on dev), so the command was unusable with the Claude toggle off even though a native `claude` binary was available. Launch natively in that case instead. Only an explicit `false` triggers the fallback — from config, or reported live by `GET /api/claude-code` — so a proxy predating the `enabled` field stays routed, and an absent proxy still starts one rather than falling back. The native session must not inherit proxy state, so it removes only values it can prove OpenCodex owns: `ANTHROPIC_BASE_URL` when it targets this proxy's own loopback host and configured port with a proxy-issued admission token, the `CLAUDE_CODE_*` discovery and auto-context levers, and model slots that resolve only through the proxy. An unrelated `http://localhost:8080` gateway and a user `sk-ant-` credential are preserved. Client-ownership preflight runs before any fallback, so an invalid or mismatched connected client still fails closed. Three fixes on top of the contributor's head: - Sync all eight `docs-site` `guides/claude-code.md` pages, which still described `ocx claude` as proxy-only. - Distinguish an absent `settings.json` from a corrupt one in `readPickerDefaultModel`. Swallowing both alike dropped the "saved model requires the proxy" warning exactly when the file was broken; a corrupt file now warns and names the path without echoing contents. - Restore the `#764 / SERVICE_STOP_LIVENESS` rationale comment above `ensureProxyForClaude`, which the diff deleted while keeping the behavior. Carry of #3519. Co-authored-by: everton-dgn <58889432+everton-dgn@users.noreply.github.com> --- .../src/content/docs/fr/guides/claude-code.md | 26 ++ .../src/content/docs/guides/claude-code.md | 25 ++ .../src/content/docs/ja/guides/claude-code.md | 27 ++ .../src/content/docs/ko/guides/claude-code.md | 25 ++ .../src/content/docs/ru/guides/claude-code.md | 28 ++ .../src/content/docs/tr/guides/claude-code.md | 31 ++ .../content/docs/zh-cn/guides/claude-code.md | 23 ++ .../content/docs/zh-tw/guides/claude-code.md | 23 ++ src/cli/claude.ts | 271 +++++++++++++++--- src/cli/registry.ts | 5 +- tests/claude-integration/claude-cli.test.ts | 133 ++++++++- 11 files changed, 575 insertions(+), 42 deletions(-) diff --git a/docs-site/src/content/docs/fr/guides/claude-code.md b/docs-site/src/content/docs/fr/guides/claude-code.md index 797f9d3554..9899797ea6 100644 --- a/docs-site/src/content/docs/fr/guides/claude-code.md +++ b/docs-site/src/content/docs/fr/guides/claude-code.md @@ -67,6 +67,32 @@ ignore les identifiants Anthropic introduits uniquement par un fichier dotenv du dans votre shell reste toujours prioritaire, quel que soit le mode d'authentification. Pour utiliser volontairement une clé API, exportez-la (`export ANTHROPIC_API_KEY=...`) au lieu de la laisser dans un fichier de projet. +### Lancement natif de repli quand le routage Claude est désactivé + +`ocx claude` échouait auparavant avec une erreur lorsque le routage Claude était désactivé. Il lance +désormais le binaire natif `claude` à la place, de sorte que la commande reste utile routage coupé : + +| Où le routage est désactivé | Ce qui se passe | +| --- | --- | +| `claudeCode.enabled: false` dans la configuration | Lancement natif, avec un avis indiquant que le routage est désactivé | +| Le proxy en cours renvoie `enabled: false` depuis `GET /api/claude-code` | Lancement natif, avec un avis de redémarrer le service après réactivation | +| `claudeCode.enabled` absent ou `true` | Routage par le proxy, inchangé | + +Seul un `false` explicite déclenche le repli : un proxy antérieur à ce champ reste donc routé. Un proxy +absent n'est pas non plus un déclencheur — routage activé, `ocx claude` démarre toujours le proxy. + +Une session native ne doit pas hériter de l'état du proxy. Le repli supprime donc uniquement les valeurs +dont OpenCodex peut **prouver** la propriété : `ANTHROPIC_BASE_URL` seulement lorsqu'elle pointe vers +l'adresse de bouclage et le port configuré de ce proxy *et* que le jeton d'admission associé a bien été +émis par lui ; les leviers `CLAUDE_CODE_*` de découverte et d'auto-contexte ; et les emplacements de +modèle qui ne se résolvent qu'à travers le proxy (alias routés et identifiants `provider/model`). Tout +le reste vous appartient et est préservé — une passerelle `http://localhost:8080` sans rapport et vos +propres identifiants `sk-ant-` survivent tous les deux. + +Si le modèle par défaut enregistré dans le sélecteur `/model` est réservé au proxy, la session native +bascule sur `claudeCode.model` lorsque celui-ci est utilisable nativement, et vous avertit sinon de +passer `--model `. Un argument `--model` explicite l'emporte toujours. + ## Mode d'authentification Claude Code a besoin d'un jeton dans `ANTHROPIC_AUTH_TOKEN` pour communiquer avec une passerelle, mais définir cette diff --git a/docs-site/src/content/docs/guides/claude-code.md b/docs-site/src/content/docs/guides/claude-code.md index 264d5d6fea..9dafc37c21 100644 --- a/docs-site/src/content/docs/guides/claude-code.md +++ b/docs-site/src/content/docs/guides/claude-code.md @@ -72,6 +72,31 @@ ignores Anthropic credentials that only a project dotenv introduced. A value you your shell still wins, in every auth mode. To use an API key deliberately, export it (`export ANTHROPIC_API_KEY=...`) rather than leaving it in a project file. +### Native fallback when Claude routing is off + +`ocx claude` used to exit with an error when Claude routing was disabled. It now launches the +native `claude` binary instead, so the command stays useful with routing off: + +| Where routing is off | What happens | +| --- | --- | +| `claudeCode.enabled: false` in config | Native launch, with a notice that routing is disabled | +| The running proxy reports `enabled: false` from `GET /api/claude-code` | Native launch, with a notice to restart the service after re-enabling | +| `claudeCode.enabled` absent or `true` | Routed through the proxy, unchanged | + +Only an explicit `false` triggers the fallback, so a proxy predating the field stays routed. A +missing proxy is not a trigger either — with routing on, `ocx claude` still starts the proxy. + +A native session must not inherit proxy state, so the fallback removes values it can **prove** +OpenCodex owns: `ANTHROPIC_BASE_URL` only when it points at this proxy's own loopback address +and configured port *and* the paired admission token is one the proxy issued; the +`CLAUDE_CODE_*` discovery and auto-context levers; and model slots that only resolve through the +proxy (routed aliases and `provider/model` ids). Anything else is yours and is preserved — an +unrelated `http://localhost:8080` gateway and your own `sk-ant-` credential both survive. + +If your saved `/model` picker default is a proxy-only model, the native session falls back to +`claudeCode.model` when that is natively usable, and otherwise warns you to pass +`--model `. An explicit `--model` argument always wins. + ## Auth mode Claude Code needs a token in `ANTHROPIC_AUTH_TOKEN` to talk to a gateway, but setting that diff --git a/docs-site/src/content/docs/ja/guides/claude-code.md b/docs-site/src/content/docs/ja/guides/claude-code.md index bc7e84e39d..6d45bf63af 100644 --- a/docs-site/src/content/docs/ja/guides/claude-code.md +++ b/docs-site/src/content/docs/ja/guides/claude-code.md @@ -28,6 +28,33 @@ ocx claude | `CLAUDE_CODE_MAX_CONTEXT_TOKENS` / `DISABLE_COMPACT` | `maxContextTokens` が設定された場合の従来コンテキスト上書き値 (条件付き) | 直接 export した変数が常に優先します。追加引数はそのまま渡されます: `ocx claude -p "hello"`。 +### Claude ルーティングが無効なときのネイティブフォールバック + +以前は Claude ルーティングが無効だと `ocx claude` はエラーで終了していました。現在は代わりに +ネイティブの `claude` バイナリを起動するため、ルーティングを切ったままでもこのコマンドを使えます。 + +| ルーティングが無効な場所 | 動作 | +| --- | --- | +| 設定の `claudeCode.enabled: false` | ルーティングが無効である旨の通知とともにネイティブ起動 | +| 実行中のプロキシが `GET /api/claude-code` で `enabled: false` を返す | ネイティブ起動 + 有効化後にサービスを再起動する案内 | +| `claudeCode.enabled` が無い、または `true` | 従来どおりプロキシ経由でルーティング | + +明示的な `false` のみがフォールバックの条件なので、このフィールドを持たない古いプロキシは +ルーティングのままです。プロキシが無いことも条件ではありません — ルーティングが有効なら +`ocx claude` はこれまでどおりプロキシを起動します。 + +ネイティブセッションがプロキシの状態を引き継いではならないため、フォールバックは OpenCodex の +所有だと**証明できる**値だけを削除します。`ANTHROPIC_BASE_URL` はこのプロキシ自身のループバック +アドレスと設定済みポートを指し、かつ対になる admission トークンがプロキシの発行したものである場合 +のみ削除します。加えて `CLAUDE_CODE_*` の検出・自動コンテキスト用スイッチと、プロキシ経由でしか +解決しないモデルスロット(ルーティング用エイリアスと `provider/model` 形式)も削除します。それ以外 +はあなたの値なので保持されます — 無関係な `http://localhost:8080` ゲートウェイと自分の +`sk-ant-` 資格情報はどちらも残ります。 + +保存された `/model` ピッカーの既定値がプロキシ専用モデルの場合、`claudeCode.model` がネイティブ +で使えるならそれにフォールバックし、使えなければ `--model ` を渡すよう警告します。 +明示的な `--model` 引数が常に優先します。 + ## システム環境統合(macOS) `claudeCode.systemEnv` を `true` に設定すると(デフォルト: **オフ`)`ocx start` が `launchctl setenv` を diff --git a/docs-site/src/content/docs/ko/guides/claude-code.md b/docs-site/src/content/docs/ko/guides/claude-code.md index 0183b5b72f..a7ed659c59 100644 --- a/docs-site/src/content/docs/ko/guides/claude-code.md +++ b/docs-site/src/content/docs/ko/guides/claude-code.md @@ -28,6 +28,31 @@ ocx claude | `CLAUDE_CODE_MAX_CONTEXT_TOKENS` / `DISABLE_COMPACT` | `maxContextTokens`가 설정된 경우 기존 컨텍스트 재정의 값 (조건부) | 직접 내보낸 변수가 항상 우선해요. 추가 인자는 그대로 전달돼요: `ocx claude -p "hello"`. +### Claude 라우팅이 꺼져 있을 때의 네이티브 폴백 + +예전에는 Claude 라우팅이 꺼져 있으면 `ocx claude`가 오류를 내고 종료했어요. 이제는 네이티브 +`claude` 실행 파일을 대신 실행하므로, 라우팅을 꺼 둔 상태에서도 이 명령을 그대로 쓸 수 있어요. + +| 라우팅이 꺼진 위치 | 동작 | +| --- | --- | +| 설정의 `claudeCode.enabled: false` | 라우팅이 비활성화되었다는 안내와 함께 네이티브 실행 | +| 실행 중인 프록시가 `GET /api/claude-code`에서 `enabled: false`를 보고 | 네이티브 실행 + 라우팅을 켠 뒤 서비스를 재시작하라는 안내 | +| `claudeCode.enabled`가 없거나 `true` | 기존과 동일하게 프록시로 라우팅 | + +명시적인 `false`만 폴백을 유발하므로, 이 필드를 모르는 예전 프록시는 계속 라우팅돼요. 프록시가 +없는 것도 폴백 조건이 아니에요 — 라우팅이 켜져 있으면 `ocx claude`가 프록시를 그대로 띄워요. + +네이티브 세션이 프록시 상태를 물려받으면 안 되므로, 폴백은 OpenCodex 소유임을 **증명할 수 있는** +값만 제거해요. `ANTHROPIC_BASE_URL`은 이 프록시의 루프백 주소와 설정된 포트를 정확히 가리키고 +짝이 되는 admission 토큰도 프록시가 발급한 것일 때만 제거하고, `CLAUDE_CODE_*` 검색·자동 컨텍스트 +레버와 프록시를 거쳐야만 해석되는 모델 슬롯(라우팅 별칭과 `provider/model` 형식)도 제거해요. +그 밖의 값은 사용자 것이라 그대로 유지돼요 — 관련 없는 `http://localhost:8080` 게이트웨이와 +직접 설정한 `sk-ant-` 자격 증명은 둘 다 살아남아요. + +저장된 `/model` 선택기 기본값이 프록시 전용 모델이면, `claudeCode.model`이 네이티브에서 쓸 수 +있을 때 그 값으로 대체하고, 그렇지 않으면 `--model `을 넘기라고 경고해요. +명시적인 `--model` 인자가 항상 우선해요. + ## 인증 모드 Claude Code가 게이트웨이와 통신하려면 `ANTHROPIC_AUTH_TOKEN`에 토큰이 필요해요. 그런데 이 변수를 diff --git a/docs-site/src/content/docs/ru/guides/claude-code.md b/docs-site/src/content/docs/ru/guides/claude-code.md index b04612625b..3f4285cfe6 100644 --- a/docs-site/src/content/docs/ru/guides/claude-code.md +++ b/docs-site/src/content/docs/ru/guides/claude-code.md @@ -29,6 +29,34 @@ ocx claude | `CLAUDE_CODE_MAX_CONTEXT_TOKENS` / `DISABLE_COMPACT` | Устаревшее переопределение контекста, когда задан `maxContextTokens` (условно) | Переменные, которые вы экспортируете сами, всегда имеют приоритет. Дополнительные аргументы передаются как есть: `ocx claude -p "hello"`. +### Нативный запасной запуск, когда маршрутизация Claude выключена + +Раньше `ocx claude` завершался с ошибкой, если маршрутизация Claude была выключена. Теперь вместо +этого запускается нативный бинарник `claude`, поэтому команда остаётся полезной и с выключенной +маршрутизацией: + +| Где выключена маршрутизация | Что происходит | +| --- | --- | +| `claudeCode.enabled: false` в конфигурации | Нативный запуск с уведомлением, что маршрутизация выключена | +| Запущенный прокси возвращает `enabled: false` из `GET /api/claude-code` | Нативный запуск и совет перезапустить службу после включения | +| `claudeCode.enabled` отсутствует или равен `true` | Маршрутизация через прокси, без изменений | + +Запасной запуск включает только явное `false`, поэтому прокси, не знающий об этом поле, остаётся +маршрутизируемым. Отсутствие прокси тоже не является триггером — при включённой маршрутизации +`ocx claude` по-прежнему запускает прокси. + +Нативная сессия не должна наследовать состояние прокси, поэтому запасной запуск удаляет только те +значения, принадлежность которых OpenCodex может **доказать**: `ANTHROPIC_BASE_URL` — лишь когда он +указывает на собственный локальный адрес и настроенный порт этого прокси, а парный admission-токен +выдан самим прокси; переключатели обнаружения и автоконтекста `CLAUDE_CODE_*`; и слоты моделей, +которые разрешаются только через прокси (маршрутные псевдонимы и форма `provider/model`). Всё +остальное — ваше и сохраняется: посторонний шлюз `http://localhost:8080` и ваши собственные +учётные данные `sk-ant-` остаются на месте. + +Если сохранённый выбор в селекторе `/model` — модель только для прокси, нативная сессия перейдёт на +`claudeCode.model`, когда та доступна нативно, а иначе предупредит передать +`--model <модель Anthropic>`. Явный аргумент `--model` всегда имеет приоритет. + ## Интеграция с системным окружением (macOS) Когда `claudeCode.systemEnv` установлен в `true` (по умолчанию: **выключено**), `ocx start` diff --git a/docs-site/src/content/docs/tr/guides/claude-code.md b/docs-site/src/content/docs/tr/guides/claude-code.md index bf955fba97..2b092dc621 100644 --- a/docs-site/src/content/docs/tr/guides/claude-code.md +++ b/docs-site/src/content/docs/tr/guides/claude-code.md @@ -80,6 +80,37 @@ kimlik doğrulama modunda her zaman geçerlidir. Bir API anahtarını kasıtlı kullanmak için, onu bir proje dosyasında bırakmak yerine dışa aktarın (`export ANTHROPIC_API_KEY=...`). +### Claude yönlendirmesi kapalıyken yerel geri dönüş + +`ocx claude` eskiden Claude yönlendirmesi kapalıyken hata vererek çıkardı. +Artık bunun yerine yerel `claude` ikili dosyasını başlatır; böylece komut, +yönlendirme kapalıyken de kullanışlı kalır: + +| Yönlendirmenin kapalı olduğu yer | Ne olur | +| --- | --- | +| Yapılandırmada `claudeCode.enabled: false` | Yönlendirmenin kapalı olduğunu bildiren bir uyarıyla yerel başlatma | +| Çalışan vekil `GET /api/claude-code` üzerinden `enabled: false` bildiriyor | Yerel başlatma ve yeniden etkinleştirdikten sonra servisi yeniden başlatma önerisi | +| `claudeCode.enabled` yok veya `true` | Değişmeden vekil üzerinden yönlendirme | + +Geri dönüşü yalnızca açık bir `false` tetikler; bu alandan önceki bir vekil +yönlendirilmiş kalır. Vekilin bulunmaması da bir tetikleyici değildir — +yönlendirme açıkken `ocx claude` vekili yine başlatır. + +Yerel bir oturum vekil durumunu devralmamalıdır; bu nedenle geri dönüş yalnızca +OpenCodex'in sahipliğini **kanıtlayabildiği** değerleri kaldırır: +`ANTHROPIC_BASE_URL` yalnızca bu vekilin kendi geri döngü adresini ve +yapılandırılmış bağlantı noktasını gösteriyorsa *ve* eşlenmiş kabul belirteci +vekilin verdiği bir belirteçse; `CLAUDE_CODE_*` keşif ve otomatik bağlam +anahtarları; ve yalnızca vekil üzerinden çözülen model yuvaları (yönlendirme +takma adları ve `provider/model` kimlikleri). Geri kalan her şey sizindir ve +korunur — ilgisiz bir `http://localhost:8080` ağ geçidi ve kendi `sk-ant-` +kimlik bilginiz birlikte hayatta kalır. + +Kaydedilmiş `/model` seçici varsayılanınız yalnızca vekile özgü bir modelse, +yerel oturum `claudeCode.model` yerel olarak kullanılabildiğinde ona döner; +aksi hâlde `--model ` geçmeniz için uyarır. Açık bir +`--model` argümanı her zaman kazanır. + ## Kimlik doğrulama modu (Auth mode) Claude Code'un bir ağ geçidiyle konuşabilmesi için `ANTHROPIC_AUTH_TOKEN` içinde diff --git a/docs-site/src/content/docs/zh-cn/guides/claude-code.md b/docs-site/src/content/docs/zh-cn/guides/claude-code.md index e5140cb8a7..e2db5ddec4 100644 --- a/docs-site/src/content/docs/zh-cn/guides/claude-code.md +++ b/docs-site/src/content/docs/zh-cn/guides/claude-code.md @@ -28,6 +28,29 @@ ocx claude | `CLAUDE_CODE_MAX_CONTEXT_TOKENS` / `DISABLE_COMPACT` | 设置 `maxContextTokens` 时使用的旧版上下文覆盖项（条件注入） | 你自行导出的变量始终优先。额外参数会直接透传：`ocx claude -p "hello"`。 +### Claude 路由关闭时的原生回退 + +以前当 Claude 路由被关闭时，`ocx claude` 会直接报错退出。现在它会改为启动原生 `claude` +可执行文件，因此在关闭路由的情况下该命令依然可用： + +| 路由关闭的位置 | 行为 | +| --- | --- | +| 配置中的 `claudeCode.enabled: false` | 原生启动，并提示路由已被禁用 | +| 运行中的代理在 `GET /api/claude-code` 中返回 `enabled: false` | 原生启动，并提示重新启用后重启服务 | +| `claudeCode.enabled` 缺失或为 `true` | 与以往一致，经代理路由 | + +只有显式的 `false` 才会触发回退，因此早于该字段的旧代理仍会保持路由。代理缺失同样不是触发条件 +——只要路由是开启的，`ocx claude` 仍会照常启动代理。 + +原生会话不应继承代理状态，因此回退只移除能够**证明**属于 OpenCodex 的值：仅当 +`ANTHROPIC_BASE_URL` 指向本代理自身的回环地址与配置端口、且配对的 admission 令牌确实由代理签发 +时才移除；此外还会移除 `CLAUDE_CODE_*` 的发现与自动上下文开关，以及只能经由代理解析的模型槽位 +（路由别名与 `provider/model` 形式）。其余都属于你自己的配置并被保留——无关的 +`http://localhost:8080` 网关和你自己的 `sk-ant-` 凭据都会保留。 + +如果保存的 `/model` 选择器默认值是仅限代理的模型，当 `claudeCode.model` 可在原生环境使用时会 +回退到它，否则会警告你传入 `--model `。显式的 `--model` 参数始终优先。 + ## 系统环境集成（macOS） 当 `claudeCode.systemEnv` 设置为 `true`（默认：**关闭**）时，`ocx start` 会使用 `launchctl setenv` diff --git a/docs-site/src/content/docs/zh-tw/guides/claude-code.md b/docs-site/src/content/docs/zh-tw/guides/claude-code.md index 7aea715953..f07b2f97c6 100644 --- a/docs-site/src/content/docs/zh-tw/guides/claude-code.md +++ b/docs-site/src/content/docs/zh-tw/guides/claude-code.md @@ -54,6 +54,29 @@ ocx claude | `CLAUDE_CODE_MAX_CONTEXT_TOKENS` / `DISABLE_COMPACT` | 設定 `maxContextTokens` 時使用的舊版上下文覆蓋項（條件注入） | 你自行匯出的變數始終優先。額外引數會直接透傳：`ocx claude -p "hello"`。 +### Claude 路由關閉時的原生回退 + +以前當 Claude 路由被關閉時，`ocx claude` 會直接報錯結束。現在它會改為啟動原生 `claude` +執行檔，因此在關閉路由的情況下該指令仍然可用： + +| 路由關閉的位置 | 行為 | +| --- | --- | +| 設定中的 `claudeCode.enabled: false` | 原生啟動，並提示路由已停用 | +| 執行中的代理在 `GET /api/claude-code` 回傳 `enabled: false` | 原生啟動，並提示重新啟用後重啟服務 | +| `claudeCode.enabled` 缺少或為 `true` | 與以往一致，經代理路由 | + +只有明確的 `false` 才會觸發回退，因此早於該欄位的舊代理仍會維持路由。代理不存在同樣不是觸發 +條件——只要路由是開啟的，`ocx claude` 仍會照常啟動代理。 + +原生工作階段不應繼承代理狀態，因此回退只移除能夠**證明**屬於 OpenCodex 的值：僅當 +`ANTHROPIC_BASE_URL` 指向本代理自身的回送位址與設定連接埠、且配對的 admission token 確實由代理 +簽發時才移除；此外還會移除 `CLAUDE_CODE_*` 的探索與自動上下文開關，以及只能經由代理解析的模型 +槽位（路由別名與 `provider/model` 形式）。其餘都屬於你自己的設定並會保留——無關的 +`http://localhost:8080` 閘道器和你自己的 `sk-ant-` 憑證都會保留。 + +若儲存的 `/model` 選擇器預設值是僅限代理的模型，當 `claudeCode.model` 可在原生環境使用時會 +回退到它，否則會警告你傳入 `--model `。明確的 `--model` 引數始終優先。 + ## 認證模式 Claude Code 需要在 `ANTHROPIC_AUTH_TOKEN` 中有 token 才能與閘道器通訊，但設定該變數也會停用 diff --git a/src/cli/claude.ts b/src/cli/claude.ts index a9e64fc1af..446485e2b7 100644 --- a/src/cli/claude.ts +++ b/src/cli/claude.ts @@ -1,5 +1,6 @@ /** - * `ocx claude [claude args...]` — launch Claude Code wired to the local proxy. + * `ocx claude [claude args...]` — launch Claude Code through the local proxy, + * or natively when Claude routing is explicitly disabled. * * Mirrors `ccr code` UX (devlog/260711_claude_inbound/020, 003 E1/E2/E5/G1): * ensures the proxy is running, injects the Anthropic env slots, then execs the @@ -9,8 +10,9 @@ import { spawn } from "node:child_process"; import { loadConfig } from "../config"; import { injectClaudeAgentDefs } from "../claude/agents-inject"; +import { CLAUDE_ALIAS_PREFIX_V1, CLAUDE_ALIAS_PREFIX_V2 } from "../claude/alias"; import { effectiveModelEnv, resolveAutoContext } from "../claude/context-windows"; -import { refreshGatewayModelCacheFromProxy } from "../claude/gateway-cache"; +import { claudeConfigDir, refreshGatewayModelCacheFromProxy } from "../claude/gateway-cache"; import { commandInvocation } from "../lib/win-exec"; import { isProxyAdmissionSecret } from "../server/auth-cors"; import { findLiveProxy } from "../server/proxy-liveness"; @@ -21,10 +23,11 @@ import { resolveClaudeAuthMode } from "../claude/auth-mode"; import { withProcessRuntimeProvenance } from "../lib/bun-runtime"; import { selfLaunchArgv } from "../lib/self-launch-argv"; import { ANTHROPIC_PARENT_ENV_SLOTS, trustedNodeLauncherContext, type AnthropicParentEnvSlot } from "./launcher-context"; -import { readClientConnectionState } from "../client/state"; -import { readServiceApiTokenState } from "../lib/service-secrets"; +import { readClientConnectionState, type ClientConnectionState } from "../client/state"; +import { readServiceApiTokenState, type ServiceApiTokenState } from "../lib/service-secrets"; import { DEFAULT_CATALOG_PATH } from "../codex/paths"; import { readFileSync } from "node:fs"; +import { join } from "node:path"; import { aliasForNative, aliasForRoute } from "../claude/alias"; import { desktop3pAlias } from "../claude/desktop-3p"; @@ -49,6 +52,48 @@ export type ClaudeEnvDeps = { allowRootSkipPermissions?: boolean; }; +function deleteUntrustedAnthropicSlots(env: ClaudeLaunchEnv, deps: ClaudeEnvDeps): void { + const explicitSlots = deps.preBunAnthropicSlots; + const trustedSlots = explicitSlots === undefined + ? trustedNodeLauncherContext()?.anthropicEnvSlots ?? [] + : explicitSlots ?? []; + const exported = new Set(trustedSlots); + for (const name of ANTHROPIC_PARENT_ENV_SLOTS) { + const value = env[name]; + if (value !== undefined && value !== "" && !exported.has(name)) delete env[name]; + } + delete env.OCX_PRE_BUN_ANTHROPIC_ENV; + delete env.OCX_NODE_LAUNCH_CONTEXT; +} + +/** + * Read Claude Code's own persisted `/model` picker default. + * + * An absent `settings.json` is the ordinary fresh-install case and stays silent. A + * present-but-unparseable one is not: swallowing it would drop the "saved model requires + * the proxy" warning exactly when the file is broken, so the native session would start + * on a model the user never chose with no explanation. Name the file, never its contents. + */ +export function readPickerDefaultModel(configDir: string): string | null { + const file = join(configDir, "settings.json"); + let raw: string; + try { + raw = readFileSync(file, "utf8"); + } catch (err) { // no-excuse-ok: catch -- an absent picker settings file is the default install state. + if ((err as NodeJS.ErrnoException).code !== "ENOENT") { + console.warn(`⚠ Could not read Claude Code settings at ${file}; the saved model check is skipped this run.`); + } + return null; + } + try { + const parsed = JSON.parse(raw) as Record; + return typeof parsed.model === "string" && parsed.model.trim() !== "" ? parsed.model.trim() : null; + } catch { // no-excuse-ok: catch -- a corrupt picker file must warn, not abort the launch. + console.warn(`⚠ Claude Code settings at ${file} are not valid JSON; the saved model check is skipped this run.`); + return null; + } +} + function isClaudeLoopbackHostname(hostname: string): boolean { const normalized = hostname.toLowerCase().replace(/\.$/, ""); return normalized === "localhost" @@ -128,18 +173,7 @@ export function buildClaudeEnv( // Direct `bun src/cli/index.ts` therefore loses ambient Anthropic values. That is a // real cost to a documented entry point, and the escape hatch is the launcher: run // through `ocx` (the published bin) and genuine shell exports are preserved by proof. - const explicitSlots = deps.preBunAnthropicSlots; - const trustedSlots = explicitSlots === undefined - ? trustedNodeLauncherContext()?.anthropicEnvSlots ?? [] - : explicitSlots ?? []; - const exported = new Set(trustedSlots); - for (const name of ANTHROPIC_PARENT_ENV_SLOTS) { - const value = env[name]; - if (value !== undefined && value !== "" && !exported.has(name)) delete env[name]; - } - // Never forward old or current provenance seams to Claude Code. - delete env.OCX_PRE_BUN_ANTHROPIC_ENV; - delete env.OCX_NODE_LAUNCH_CONTEXT; + deleteUntrustedAnthropicSlots(env, deps); const setDefault = (name: string, value: string | undefined) => { if (value === undefined || value.length === 0) return; if (env[name] !== undefined && env[name] !== "") return; // user wins @@ -302,7 +336,12 @@ export function buildClaudeEnv( * daemon registers every selector form — audit R3#1). 3s bound + management auth header. * (no [1m] marking, conservative). */ -export async function fetchClaudeContextWindows(config: OcxConfig, port: number, timeoutMs = 3_000): Promise> { +export interface ClaudeCodeLiveState { + contextWindows: Record; + enabled?: boolean; +} + +export async function fetchClaudeCodeState(config: OcxConfig, port: number, timeoutMs = 3_000): Promise { try { const headers = new Headers(); const token = configuredAdminToken(); @@ -311,15 +350,22 @@ export async function fetchClaudeContextWindows(config: OcxConfig, port: number, headers, signal: AbortSignal.timeout(timeoutMs), }); - if (!res.ok) return {}; - const body = await res.json() as { contextWindows?: Record }; - return body.contextWindows && typeof body.contextWindows === "object" ? body.contextWindows : {}; + if (!res.ok) return { contextWindows: {} }; + const body = await res.json() as { contextWindows?: Record; enabled?: boolean }; + return { + contextWindows: body.contextWindows && typeof body.contextWindows === "object" ? body.contextWindows : {}, + ...(typeof body.enabled === "boolean" ? { enabled: body.enabled } : {}), + }; } catch { console.error("⚠ 모델 컨텍스트 정보를 불러오지 못했습니다 — 1M 자동 표시는 이번 실행에서 생략됩니다."); - return {}; + return { contextWindows: {} }; } } +export async function fetchClaudeContextWindows(config: OcxConfig, port: number, timeoutMs = 3_000): Promise> { + return (await fetchClaudeCodeState(config, port, timeoutMs)).contextWindows; +} + export function readConnectedClaudeContextWindows(path = DEFAULT_CATALOG_PATH): Record { try { const parsed = JSON.parse(readFileSync(path, "utf8")) as { models?: unknown }; @@ -384,6 +430,139 @@ export async function ensureProxyForClaude(deps: ClaudeProxyEnsureDeps = {}): Pr return null; } +export const CLAUDE_NATIVE_ROUTING_OFF = + "ℹ️ Claude Code routing is disabled in OpenCodex. Launching Claude Code natively. Enable Claude routing to use the proxy again."; + +export const CLAUDE_NATIVE_LIVE_DISABLED = + "ℹ️ The running OpenCodex proxy has Claude Code routing disabled. Launching Claude Code natively. Restart the service after enabling routing."; + +export type ClaudeLaunchPlan = + | { kind: "routed" } + | { kind: "native"; notice: string }; + +export function claudeLaunchPlan( + configuredEnabled: boolean, + liveEnabled: boolean | undefined, +): ClaudeLaunchPlan { + if (!configuredEnabled) return { kind: "native", notice: CLAUDE_NATIVE_ROUTING_OFF }; + if (liveEnabled === false) return { kind: "native", notice: CLAUDE_NATIVE_LIVE_DISABLED }; + return { kind: "routed" }; +} + +export type ClaudeLaunchPreflight = + | { kind: "continue" } + | { kind: "native"; notice: string } + | { kind: "error"; message: string }; + +/** Validate connected-client ownership before any native fallback can run. */ +export function claudeLaunchPreflight( + configuredEnabled: boolean, + clientState: ClientConnectionState, + tokenState?: ServiceApiTokenState, +): ClaudeLaunchPreflight { + if (clientState.kind === "invalid" || clientState.kind === "mismatched") { + return { kind: "error", message: `Client state is ${clientState.kind}: ${clientState.reason}` }; + } + if (clientState.kind === "connected") { + if (!clientState.value.selectedClients.includes("claude")) { + return { kind: "error", message: "Claude is not selected for this remote hub connection." }; + } + if (tokenState?.kind !== "present" || tokenState.fingerprint !== clientState.value.tokenFingerprint) { + return { + kind: "error", + message: tokenState?.kind === "absent" + ? "Connected service token is missing." + : "Connected service token ownership changed.", + }; + } + } + return configuredEnabled + ? { kind: "continue" } + : { kind: "native", notice: CLAUDE_NATIVE_ROUTING_OFF }; +} + +const NATIVE_STRIPPED_LEVERS = [ + "CLAUDE_CODE_ENABLE_GATEWAY_MODEL_DISCOVERY", + "CLAUDE_CODE_PROVIDER_MANAGED_BY_HOST", + "CLAUDE_CODE_MAX_CONTEXT_TOKENS", + "CLAUDE_CODE_AUTO_COMPACT_WINDOW", + "CLAUDE_CODE_ALWAYS_ENABLE_EFFORT", + "DISABLE_COMPACT", +] as const; + +const MODEL_ENV_SLOT_NAMES = [ + "ANTHROPIC_MODEL", + "ANTHROPIC_DEFAULT_OPUS_MODEL", + "ANTHROPIC_DEFAULT_SONNET_MODEL", + "ANTHROPIC_DEFAULT_FABLE_MODEL", + "ANTHROPIC_DEFAULT_HAIKU_MODEL", + "ANTHROPIC_SMALL_FAST_MODEL", +] as const; + +const DESKTOP_3P_ALIAS = /^claude-opus-4(?:-8)?-[a-z][0-9a-z]{2}$/; + +export function isProxyOnlyModelId(value: string, providerNames: readonly string[] = []): boolean { + const id = value.trim().replace(/\[1m\]$/, ""); + if (!id) return false; + if (id.startsWith(CLAUDE_ALIAS_PREFIX_V1) || id.startsWith(CLAUDE_ALIAS_PREFIX_V2) || DESKTOP_3P_ALIAS.test(id)) { + return true; + } + const slash = id.indexOf("/"); + return slash > 0 && providerNames.includes(id.slice(0, slash)); +} + +export function buildNativeClaudeEnv( + config: OcxConfig, + base: ClaudeLaunchEnv, + deps: ClaudeEnvDeps = {}, +): ClaudeLaunchEnv { + const env: ClaudeLaunchEnv = { ...base }; + deleteUntrustedAnthropicSlots(env, deps); + + const admissionSlots = ["ANTHROPIC_AUTH_TOKEN", "ANTHROPIC_API_KEY"] as const; + const hasOwnedAdmission = admissionSlots.some(name => { + const value = env[name]?.trim(); + return Boolean(value && (value === PROXY_MARKER || isProxyAdmissionSecret(value, config))); + }); + const baseUrl = env.ANTHROPIC_BASE_URL; + if (hasOwnedAdmission && targetsLocalClaudeProxy(baseUrl, config.port)) { + delete env.ANTHROPIC_BASE_URL; + } + for (const name of admissionSlots) { + const value = env[name]?.trim(); + if (value && (value === PROXY_MARKER || isProxyAdmissionSecret(value, config))) delete env[name]; + } + + for (const name of NATIVE_STRIPPED_LEVERS) delete env[name]; + const providerNames = Object.keys(config.providers); + for (const name of MODEL_ENV_SLOT_NAMES) { + const value = env[name]; + if (value && isProxyOnlyModelId(value, providerNames)) delete env[name]; + } + if (deps.allowRootSkipPermissions === true && !env.IS_SANDBOX) env.IS_SANDBOX = "1"; + return env; +} + +export function nativeModelOverride( + pickedModel: string | null, + configuredModel: string | undefined, + args: readonly string[], + providerNames: readonly string[] = [], +): { flag?: string[]; warning?: string } { + if (!pickedModel || !isProxyOnlyModelId(pickedModel, providerNames)) return {}; + if (args.some(arg => arg === "--model" || arg.startsWith("--model="))) return {}; + const fallback = configuredModel?.trim(); + if (fallback && !isProxyOnlyModelId(fallback, providerNames)) { + return { + flag: ["--model", fallback], + warning: `ℹ️ The saved model (${pickedModel}) requires the proxy. This native session will use ${fallback}.`, + }; + } + return { + warning: `⚠ The saved model (${pickedModel}) requires the proxy. Use \`--model \` or select a native model in this session.`, + }; +} + const CLAUDE_INSTALL_HINT = "❌ `claude` CLI not found. Install it first: npm install -g @anthropic-ai/claude-code"; /** @@ -417,28 +596,19 @@ export function rootSkipPermissionsNotice(env: ClaudeLaunchEnv): string { export async function cmdClaude(args: string[]): Promise { const config = loadConfig(); - if (config.claudeCode?.enabled === false) { - console.error("Claude inbound is disabled (config.claudeCode.enabled=false — flip the Claude ON toggle in the GUI or edit config)."); - return 1; - } const clientState = readClientConnectionState(); - if (clientState.kind === "invalid" || clientState.kind === "mismatched") { - console.error(`Client state is ${clientState.kind}: ${clientState.reason}`); + const tokenState = clientState.kind === "connected" ? readServiceApiTokenState() : undefined; + const preflight = claudeLaunchPreflight(config.claudeCode?.enabled !== false, clientState, tokenState); + if (preflight.kind === "error") { + console.error(preflight.message); return 1; } + if (preflight.kind === "native") return launchNativeClaude(config, args, preflight.notice); let route: number | ClaudeRoutingTarget; let contextWindows: Record; if (clientState.kind === "connected") { - if (!clientState.value.selectedClients.includes("claude")) { - console.error("Claude is not selected for this remote hub connection."); - return 1; - } - const token = readServiceApiTokenState(); - if (token.kind !== "present" || token.fingerprint !== clientState.value.tokenFingerprint) { - console.error(token.kind === "absent" ? "Connected service token is missing." : "Connected service token ownership changed."); - return 1; - } - route = { baseUrl: clientState.value.serverUrl, admissionToken: token.token }; + if (tokenState?.kind !== "present") return 1; + route = { baseUrl: clientState.value.serverUrl, admissionToken: tokenState.token }; contextWindows = readConnectedClaudeContextWindows(); } else { const port = await ensureProxyForClaude(); @@ -446,8 +616,11 @@ export async function cmdClaude(args: string[]): Promise { console.error("❌ Proxy did not become healthy after starting."); return 1; } + const liveState = await fetchClaudeCodeState(config, port); + const plan = claudeLaunchPlan(true, liveState.enabled); + if (plan.kind === "native") return launchNativeClaude(config, args, plan.notice); route = port; - contextWindows = await fetchClaudeContextWindows(config, port); + contextWindows = liveState.contextWindows; } const allowRootSkipPermissions = shouldAllowRootSkipPermissions(args); const env = buildClaudeEnv(config, route, process.env, contextWindows, { allowRootSkipPermissions }); @@ -479,7 +652,27 @@ export async function cmdClaude(args: string[]): Promise { console.error(`⚠ Claude agent definitions could not be synced: ${message}`); } } - return await new Promise(resolve => { + return spawnClaude(args, env); +} + +async function launchNativeClaude(config: OcxConfig, args: string[], notice: string): Promise { + console.error(notice); + const providerNames = Object.keys(config.providers); + const override = nativeModelOverride( + readPickerDefaultModel(claudeConfigDir()), + config.claudeCode?.model, + args, + providerNames, + ); + if (override.warning) console.error(override.warning); + const allowRootSkipPermissions = shouldAllowRootSkipPermissions(args); + const env = buildNativeClaudeEnv(config, process.env, { allowRootSkipPermissions }); + if (allowRootSkipPermissions) console.error(rootSkipPermissionsNotice(env)); + return spawnClaude([...(override.flag ?? []), ...args], env); +} + +function spawnClaude(args: string[], env: ClaudeLaunchEnv): Promise { + return new Promise(resolve => { const inv = commandInvocation("claude", args); const child = spawn(inv.file, inv.args, { stdio: "inherit", env: env as NodeJS.ProcessEnv, ...inv.options }); child.on("error", (err: NodeJS.ErrnoException) => { diff --git a/src/cli/registry.ts b/src/cli/registry.ts index 5d6cd4391c..32d7481606 100644 --- a/src/cli/registry.ts +++ b/src/cli/registry.ts @@ -315,13 +315,14 @@ export const CLI_COMMANDS: CliCommandEntry[] = [ { name: "claude", usage: "ocx claude [claude args...]", - summary: "Launch Claude Code wired to the proxy (env injection + gateway model discovery).", + summary: "Launch Claude Code through the proxy, with native fallback when Claude routing is disabled.", details: [ "Ensures the proxy is running, then execs `claude` with ANTHROPIC_BASE_URL/ANTHROPIC_AUTH_TOKEN,", "CLAUDE_CODE_ENABLE_GATEWAY_MODEL_DISCOVERY=1 and model slots from config.claudeCode.", + "When Claude routing is explicitly disabled, it launches natively after removing proven OpenCodex-owned proxy state.", "Routed models appear in the native /model picker with stable claude-opus-4-8-2026MMDD slot aliases (Claude Code >= 2.1.129).", "Older versions: pick models via ANTHROPIC_MODEL or /model directly (any string passes through).", - "User-exported ANTHROPIC_* variables always take precedence.", + "User-exported ANTHROPIC_* variables take precedence for routed launches; native fallback removes only proven OpenCodex-owned proxy values.", "", "Claude Desktop profile:", " ocx claude desktop [apply] Save and apply the four-family profile", diff --git a/tests/claude-integration/claude-cli.test.ts b/tests/claude-integration/claude-cli.test.ts index 2e94faca5c..9bbe7cff95 100644 --- a/tests/claude-integration/claude-cli.test.ts +++ b/tests/claude-integration/claude-cli.test.ts @@ -1,6 +1,21 @@ import { describe, expect, test } from "bun:test"; -import { buildClaudeEnv, claudeNotFoundHint, ensureProxyForClaude, rootSkipPermissionsNotice, shouldAllowRootSkipPermissions } from "../../src/cli/claude"; +import { + buildClaudeEnv, + buildNativeClaudeEnv, + claudeLaunchPlan, + claudeLaunchPreflight, + claudeNotFoundHint, + ensureProxyForClaude, + isProxyOnlyModelId, + nativeModelOverride, + readPickerDefaultModel, + rootSkipPermissionsNotice, + shouldAllowRootSkipPermissions, +} from "../../src/cli/claude"; import { commandInvocation } from "../../src/lib/win-exec"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import type { LivenessIo, LiveProxy } from "../../src/server/proxy-liveness"; import type { OcxConfig } from "../../src/types"; @@ -40,6 +55,122 @@ describe("ocx claude proxy liveness", () => { }); }); +describe("ocx claude native fallback", () => { + test("routes unless configured or live Claude routing is explicitly disabled", () => { + expect(claudeLaunchPlan(true, true)).toEqual({ kind: "routed" }); + expect(claudeLaunchPlan(true, undefined)).toEqual({ kind: "routed" }); + expect(claudeLaunchPlan(false, true)).toMatchObject({ kind: "native" }); + expect(claudeLaunchPlan(true, false)).toMatchObject({ kind: "native" }); + }); + + test("rejects an invalid connected client before configuration-disabled fallback", () => { + expect(claudeLaunchPreflight(false, { kind: "invalid", reason: "bad client state" })) + .toEqual({ kind: "error", message: "Client state is invalid: bad client state" }); + expect(claudeLaunchPreflight(false, { + kind: "connected", + value: { + serverUrl: "https://hub.example.test", + apiKeyId: "remote", + tokenFingerprint: "expected", + selectedClients: ["claude"], + }, + }, { kind: "present", token: "secret", fingerprint: "changed" })) + .toEqual({ kind: "error", message: "Connected service token ownership changed." }); + }); + + test("removes proxy-owned state while preserving user credentials and native model ids", () => { + const config = cfg({ + apiKeys: [{ id: "local", name: "local", key: "ocx_data_local_key", createdAt: "2026-01-01" }], + providers: { mock: { adapter: "openai-chat", baseUrl: "http://x/v1" } }, + }); + const env = buildNativeClaudeEnv(config, { + PATH: "/usr/bin", + ANTHROPIC_BASE_URL: "http://127.0.0.1:10100", + ANTHROPIC_AUTH_TOKEN: "ocx_data_local_key", + ANTHROPIC_API_KEY: "sk-ant-user-key", + ANTHROPIC_MODEL: "claude-ocx-mock--model", + ANTHROPIC_DEFAULT_OPUS_MODEL: "mock/model", + ANTHROPIC_DEFAULT_SONNET_MODEL: "sonnet", + CLAUDE_CODE_PROVIDER_MANAGED_BY_HOST: "1", + CLAUDE_CODE_ENABLE_GATEWAY_MODEL_DISCOVERY: "1", + CLAUDE_CODE_AUTO_COMPACT_WINDOW: "829800", + }, { + preBunAnthropicSlots: ["ANTHROPIC_BASE_URL", "ANTHROPIC_AUTH_TOKEN", "ANTHROPIC_API_KEY"], + }); + + expect(env.PATH).toBe("/usr/bin"); + expect(env.ANTHROPIC_BASE_URL).toBeUndefined(); + expect(env.ANTHROPIC_AUTH_TOKEN).toBeUndefined(); + expect(env.ANTHROPIC_API_KEY).toBe("sk-ant-user-key"); + expect(env.ANTHROPIC_MODEL).toBeUndefined(); + expect(env.ANTHROPIC_DEFAULT_OPUS_MODEL).toBeUndefined(); + expect(env.ANTHROPIC_DEFAULT_SONNET_MODEL).toBe("sonnet"); + expect(env.CLAUDE_CODE_PROVIDER_MANAGED_BY_HOST).toBeUndefined(); + expect(env.CLAUDE_CODE_ENABLE_GATEWAY_MODEL_DISCOVERY).toBeUndefined(); + expect(env.CLAUDE_CODE_AUTO_COMPACT_WINDOW).toBeUndefined(); + }); + + test("preserves an unrelated loopback gateway and its user credential", () => { + for (const baseUrl of ["http://localhost:8080", "http://127.0.0.1:10100"]) { + const env = buildNativeClaudeEnv(cfg({ port: 10100 }), { + ANTHROPIC_BASE_URL: baseUrl, + ANTHROPIC_API_KEY: "sk-ant-user-key", + }, { + preBunAnthropicSlots: ["ANTHROPIC_BASE_URL", "ANTHROPIC_API_KEY"], + }); + + expect(env.ANTHROPIC_BASE_URL).toBe(baseUrl); + expect(env.ANTHROPIC_API_KEY).toBe("sk-ant-user-key"); + } + }); + + test("keeps unrelated slash model ids and recognizes configured provider routes", () => { + expect(isProxyOnlyModelId("mock/model", ["mock"])).toBe(true); + expect(isProxyOnlyModelId("claude-ocx2-abcd")).toBe(true); + expect(isProxyOnlyModelId("arn:aws:bedrock:region:acct:inference-profile/us.anthropic.model", ["mock"])).toBe(false); + expect(isProxyOnlyModelId("claude-opus-5")).toBe(false); + }); + + test("overrides a persisted proxy model only with a configured native model", () => { + expect(nativeModelOverride("claude-ocx2-abcd", "opus", [], ["mock"])) + .toMatchObject({ flag: ["--model", "opus"] }); + expect(nativeModelOverride("claude-ocx2-abcd", "mock/model", [], ["mock"]).flag).toBeUndefined(); + expect(nativeModelOverride("claude-ocx2-abcd", "opus", ["--model", "sonnet"], ["mock"])) + .toEqual({}); + }); + + test("preserves the root opt-in on native fallback", () => { + const env = buildNativeClaudeEnv(cfg(), {}, { allowRootSkipPermissions: true }); + expect(env.IS_SANDBOX).toBe("1"); + }); + + // A corrupt settings.json used to be indistinguishable from an absent one, so the + // "saved model requires the proxy" warning vanished exactly when the file was broken. + test("an absent picker settings file is silent, a corrupt one warns and names the file", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-claude-picker-")); + const warnings: string[] = []; + const realWarn = console.warn; + console.warn = (...parts: unknown[]) => { warnings.push(parts.join(" ")); }; + try { + expect(readPickerDefaultModel(dir)).toBeNull(); + expect(warnings).toEqual([]); + + writeFileSync(join(dir, "settings.json"), '{"model": "claude-ocx2-abcd"'); + expect(readPickerDefaultModel(dir)).toBeNull(); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain(join(dir, "settings.json")); + expect(warnings[0]).not.toContain("claude-ocx2-abcd"); + + writeFileSync(join(dir, "settings.json"), '{"model": "claude-ocx2-abcd"}'); + expect(readPickerDefaultModel(dir)).toBe("claude-ocx2-abcd"); + expect(warnings).toHaveLength(1); + } finally { + console.warn = realWarn; + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + describe("ocx claude env assembly", () => { test("connected target injects only the hub base and client admission token", () => { const env = buildClaudeEnv(cfg(), { From 29182deb64918340d03da321f1b1c7b0d4649f2a Mon Sep 17 00:00:00 2001 From: t Date: Sat, 5 Sep 2026 09:29:29 +0900 Subject: [PATCH 2/5] fix(oauth): rebase startup reconciliation on the persisted config reconcileOAuthProviders mutated the in-memory config and called saveConfig(config), so a startup snapshot overwrote any operator edit made after loadConfig() returned. runModelRenameStartupMigration had the same shape. Both now project onto a clone and commit through mutatePersistedConfig, which rebases the write on the newest on-disk snapshot, so a concurrent edit survives. Persistence failure degrades rather than throws. Both functions run inside startServer (src/server/index.ts:651 and :663), which is synchronous by design and wraps neither call in try/catch, so a throw there takes the whole proxy down over a config file the operator can still repair. A missing, malformed or contended config now warns once and adopts the projection in memory, matching every other mutatePersistedConfig consumer (src/storage/policy.ts, src/codex/plan-from-token.ts, src/server/management/agent-settings-routes.ts). Adoption is key by key over the touched keys only. A clear-and-reassign preserves the top-level object identity while silently detaching every nested sub-object a caller still holds a reference to. Tests: the concurrent-edit cases are the RED-on-dev proof of the defect (they fail against unmodified dev, which clobbers). The degrade-not-throw assertions are RED against #3524's head, which threw. The new tests/server/server-startup-reconcile-resilience.test.ts covers the boot path; its /healthz case binds a listener and is skipped where Bun.serve cannot bind, so it is a hosted-CI-only assertion. Co-authored-by: yansigit <44089734+yansigit@users.noreply.github.com> --- scripts/test-layout/layout.json | 1 + src/oauth/index.ts | 144 ++++++++++--- src/providers/model-rename-startup.ts | 80 ++++++- tests/fixtures/test-layout-expected.json | 1 + tests/oauth/oauth-provider-reconcile.test.ts | 199 +++++++++++++++++- .../providers/model-rename-migration.test.ts | 147 ++++++++++++- ...erver-startup-reconcile-resilience.test.ts | 147 +++++++++++++ 7 files changed, 680 insertions(+), 39 deletions(-) create mode 100644 tests/server/server-startup-reconcile-resilience.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 0d04e08ac5..b16e32881c 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1047,6 +1047,7 @@ "server-rate-limit-retry-e2e.test.ts": "server", "server-request-body-size.test.ts": "server", "server-search.test.ts": "server", + "server-startup-reconcile-resilience.test.ts": "server", "server-stop-config-hardening.test.ts": "server", "server-xai-chat-reasoning-streaming.test.ts": "server", "server-xai-header-parity.test.ts": "server", diff --git a/src/oauth/index.ts b/src/oauth/index.ts index 3ae6f4c01f..3623398309 100644 --- a/src/oauth/index.ts +++ b/src/oauth/index.ts @@ -1,7 +1,7 @@ import type { KiroOAuthMetadata, OAuthController, OAuthCredentials } from "./types"; import { parseCallbackInput } from "./callback-server"; import type { OcxConfig, OcxProviderConfig, RefreshPolicy } from "../types"; -import { ConfigMutationLockError, loadConfig, saveConfig } from "../config"; +import { ConfigMutationLockError, loadConfig, mutatePersistedConfig, saveConfig } from "../config"; import { resolveProviderApiKey } from "../providers/key-store"; import { maskEmail } from "../lib/privacy"; import { KiroTokenRefreshError, environmentKiroRoutingMetadata, loginKiro, refreshKiroToken, settleKiroLoginTransaction } from "./kiro"; @@ -1247,42 +1247,134 @@ function migrateLegacyAntigravityStaticCatalog(config: OcxConfig): boolean { return true; } -export function reconcileOAuthProviders(config: OcxConfig): boolean { - let changed = migrateLegacyAntigravityStaticCatalog(config); - for (const [name, prov] of Object.entries(config.providers)) { +interface OAuthReconcileProjection { + config: OcxConfig; + changed: boolean; + touchedProviders: string[]; + touchedAntigravityVersion: boolean; +} + +/** Pure projection over a clone: apply every reconciliation rule and report what it touched. */ +function projectOAuthProviderReconciliation(config: OcxConfig): OAuthReconcileProjection { + const projected = structuredClone(config); + const touchedProviders = new Set(); + const beforeAntigravity = JSON.stringify(projected.providers[GOOGLE_ANTIGRAVITY_PROVIDER]); + const beforeAntigravityVersion = projected.googleAntigravityStaticCatalogVersion; + let changed = migrateLegacyAntigravityStaticCatalog(projected); + if (JSON.stringify(projected.providers[GOOGLE_ANTIGRAVITY_PROVIDER]) !== beforeAntigravity) { + touchedProviders.add(GOOGLE_ANTIGRAVITY_PROVIDER); + } + const touchedAntigravityVersion = projected.googleAntigravityStaticCatalogVersion !== beforeAntigravityVersion; + + for (const [name, prov] of Object.entries(projected.providers)) { + const beforeProvider = JSON.stringify(prov); const def = OAUTH_PROVIDERS[name]; if (name === "command-code" && isLegacyCommandCodeStaticCatalog(prov)) { // The former experimental preset was the exact three-model seed above. It was not a user // choice to disable discovery, so promote only that shape to the account live catalog. prov.liveModels = true; - changed = true; } - if (!def || prov.authMode !== "oauth") continue; - const preset = def.providerConfig; - for (const field of OAUTH_RECONCILE_FIELDS) { - if (JSON.stringify(prov[field]) === JSON.stringify(preset[field])) continue; - if (preset[field] !== undefined) { - prov[field] = cloneProviderField(preset[field]) as never; - } else { - delete prov[field]; + if (def && prov.authMode === "oauth") { + const preset = def.providerConfig; + for (const field of OAUTH_RECONCILE_FIELDS) { + if (JSON.stringify(prov[field]) === JSON.stringify(preset[field])) continue; + if (preset[field] !== undefined) { + prov[field] = cloneProviderField(preset[field]) as never; + } else { + delete prov[field]; + } + } + if (prov.liveModels === undefined && preset.liveModels !== undefined) { + prov.liveModels = preset.liveModels; + } + // Heal a defaultModel that no longer exists in the refreshed list (e.g. a deprecated snapshot). + // Skip providers without a static preset `models` list: for live-discovery providers + // (e.g. command-code OAuth) the account-scoped catalog is not enumerable here, so any + // persisted defaultModel is a user selection and must not be overwritten by the seed. + if (prov.defaultModel && preset.defaultModel && preset.models && preset.models.length > 0 && !(prov.models ?? []).includes(prov.defaultModel)) { + prov.defaultModel = preset.defaultModel; } - changed = true; - } - if (prov.liveModels === undefined && preset.liveModels !== undefined) { - prov.liveModels = preset.liveModels; - changed = true; } - // Heal a defaultModel that no longer exists in the refreshed list (e.g. a deprecated snapshot). - // Skip providers without a static preset `models` list: for live-discovery providers - // (e.g. command-code OAuth) the account-scoped catalog is not enumerable here, so any - // persisted defaultModel is a user selection and must not be overwritten by the seed. - if (prov.defaultModel && preset.defaultModel && preset.models && preset.models.length > 0 && !(prov.models ?? []).includes(prov.defaultModel)) { - prov.defaultModel = preset.defaultModel; + if (JSON.stringify(prov) !== beforeProvider) { changed = true; + touchedProviders.add(name); } } - if (changed) saveConfig(config); - return changed; + + return { + config: projected, + changed, + touchedProviders: [...touchedProviders], + touchedAntigravityVersion, + }; +} + +/** + * Copy only the keys the projection actually touched back onto the caller's live object. + * + * Deliberately key-by-key rather than a wholesale clear-and-reassign: a live reference held + * elsewhere to an untouched provider sub-object must survive startup reconciliation. + */ +function adoptOAuthReconciliation(config: OcxConfig, projection: OAuthReconcileProjection): void { + for (const name of projection.touchedProviders) { + const provider = projection.config.providers[name]; + if (provider) config.providers[name] = structuredClone(provider); + else delete config.providers[name]; + } + if (projection.touchedAntigravityVersion) { + config.googleAntigravityStaticCatalogVersion = projection.config.googleAntigravityStaticCatalogVersion; + } +} + +/** + * Union the keys the on-disk rebase touched with the keys the live projection touched. + * + * The rebase runs against the persisted snapshot, which may already carry a reconciliation + * another process committed. Adopting only its touched set would leave the live object stale + * for a key it decided was already correct on disk. + */ +function withOAuthReconciliationTouchedKeys( + projection: OAuthReconcileProjection, + required: OAuthReconcileProjection, +): OAuthReconcileProjection { + return { + ...projection, + touchedProviders: [...new Set([...projection.touchedProviders, ...required.touchedProviders])], + touchedAntigravityVersion: projection.touchedAntigravityVersion || required.touchedAntigravityVersion, + }; +} + +/** + * Refresh OAuth provider presets against the registry, rebasing the write on the persisted config. + * + * This runs on the boot path (`startServer`), so persistence failure must never be fatal: a + * missing, malformed or contended config degrades to a warning plus an in-memory adopt, exactly + * as every other `mutatePersistedConfig` consumer does (`src/storage/policy.ts`, + * `src/codex/plan-from-token.ts`, `src/server/management/agent-settings-routes.ts`). Throwing + * here would take the whole proxy down over a config file the operator can still repair. + */ +export function reconcileOAuthProviders(config: OcxConfig, persist = true): boolean { + const projection = projectOAuthProviderReconciliation(config); + if (!projection.changed) return false; + if (!persist) { + adoptOAuthReconciliation(config, projection); + return true; + } + const outcome = mutatePersistedConfig(fresh => { + const next = projectOAuthProviderReconciliation(fresh); + if (next.changed) adoptOAuthReconciliation(fresh, next); + return { changed: next.changed, value: next }; + }); + if (outcome.status === "unavailable") { + console.warn( + `[opencodex] OAuth provider reconciliation could not be persisted (${outcome.reason}); ` + + "applying it in memory for this run only.", + ); + adoptOAuthReconciliation(config, projection); + return true; + } + adoptOAuthReconciliation(config, withOAuthReconciliationTouchedKeys(outcome.value, projection)); + return true; } /** Runtime guards: provider config is intentionally passthrough, so persisted fields may be malformed. */ diff --git a/src/providers/model-rename-startup.ts b/src/providers/model-rename-startup.ts index 1f983a6acb..7112e7cdd8 100644 --- a/src/providers/model-rename-startup.ts +++ b/src/providers/model-rename-startup.ts @@ -1,10 +1,47 @@ -import { saveConfig } from "../config"; +import { mutatePersistedConfig } from "../config"; import { projectModelRenames } from "./model-rename-migration"; import type { OcxConfig } from "../types"; export interface ModelRenameStartupDeps { project: typeof projectModelRenames; - save: (config: OcxConfig) => void; + /** Injected writer for tests and callers that own their own persistence. */ + save?: (config: OcxConfig) => void; +} + +function isPlainObject(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** Recursive key-by-key adopt: descend into a changed container, replace only changed leaves. */ +function adoptRecord(live: Record, next: Record): void { + for (const key of Object.keys(live)) { + if (!(key in next)) delete live[key]; + } + for (const [key, value] of Object.entries(next)) { + const current = live[key]; + if (JSON.stringify(current) === JSON.stringify(value)) continue; + if (isPlainObject(current) && isPlainObject(value)) { + adoptRecord(current, value); + continue; + } + live[key] = value; + } +} + +/** + * Copy `source` onto `target` in place, touching only the keys that actually differ. + * + * A clear-and-reassign preserves the top-level object identity while silently detaching every + * nested sub-object a caller still holds a live reference to. The renames rewrite one provider + * row at a time, so every sibling row — and the `providers` container itself — must survive + * with its identity intact. + */ +function adoptConfig(target: OcxConfig, source: OcxConfig): void { + if (target === source) return; + adoptRecord( + target as unknown as Record, + structuredClone(source) as unknown as Record, + ); } /** @@ -15,14 +52,41 @@ export interface ModelRenameStartupDeps { * recoverable from the config alone. This one only rewrites model ids that the * registry itself no longer seeds, and the pre-migration value is a string this * file still names, so the change is reversible by hand. + * + * Persistence failure is not fatal. This runs inside `startServer`, so a missing, malformed or + * contended config degrades to a warning plus the in-memory projection rather than taking the + * boot path down. */ export function runModelRenameStartupMigration( config: OcxConfig, - deps: ModelRenameStartupDeps = { project: projectModelRenames, save: saveConfig }, + deps: ModelRenameStartupDeps = { project: projectModelRenames }, ): OcxConfig { - const projection = deps.project(config); - for (const warning of projection.warnings) console.warn(`[model-rename-migration] ${warning}`); - if (!projection.changed) return projection.config; - deps.save(projection.config); - return projection.config; + const projection = deps.project(structuredClone(config)); + if (!projection.changed) { + for (const warning of projection.warnings) console.warn(`[model-rename-migration] ${warning}`); + return config; + } + if (deps.save) { + deps.save(projection.config); + adoptConfig(config, projection.config); + for (const warning of projection.warnings) console.warn(`[model-rename-migration] ${warning}`); + return config; + } + const outcome = mutatePersistedConfig(fresh => { + const next = deps.project(fresh); + if (next.changed) adoptConfig(fresh, next.config); + return { changed: next.changed, value: next }; + }); + if (outcome.status === "unavailable") { + console.warn( + `[model-rename-migration] persistence unavailable (${outcome.reason}); applying the renames ` + + "in memory for this run only.", + ); + adoptConfig(config, projection.config); + for (const warning of projection.warnings) console.warn(`[model-rename-migration] ${warning}`); + return config; + } + adoptConfig(config, outcome.value.config); + for (const warning of outcome.value.warnings) console.warn(`[model-rename-migration] ${warning}`); + return config; } diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index ef52b3720f..0b0c65d0ed 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -884,6 +884,7 @@ "server-rate-limit-retry-e2e.test.ts": "server", "server-request-body-size.test.ts": "server", "server-search.test.ts": "server", + "server-startup-reconcile-resilience.test.ts": "server", "server-stop-config-hardening.test.ts": "server", "server-xai-chat-reasoning-streaming.test.ts": "server", "server-xai-header-parity.test.ts": "server", diff --git a/tests/oauth/oauth-provider-reconcile.test.ts b/tests/oauth/oauth-provider-reconcile.test.ts index 4f1ef9768f..132dea3c7a 100644 --- a/tests/oauth/oauth-provider-reconcile.test.ts +++ b/tests/oauth/oauth-provider-reconcile.test.ts @@ -1,8 +1,13 @@ -import { afterEach, describe, expect, test } from "bun:test"; -import { mkdtempSync} from "node:fs"; +import { afterEach, describe, expect, spyOn, test } from "bun:test"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { loadConfig } from "../../src/config"; +import { + getConfigPath, + loadConfig, + saveConfig, + setPersistedConfigMutationBeforeCommitForTests, +} from "../../src/config"; import { OAUTH_PROVIDERS, reconcileOAuthProviders, upsertOAuthProvider } from "../../src/oauth"; import { getCredential, saveCredential } from "../../src/oauth/store"; import { routeModel } from "../../src/router"; @@ -15,6 +20,7 @@ const originalHome = process.env.OPENCODEX_HOME; const homes: string[] = []; afterEach(() => { + setPersistedConfigMutationBeforeCommitForTests(null); if (originalHome === undefined) delete process.env.OPENCODEX_HOME; else process.env.OPENCODEX_HOME = originalHome; for (const home of homes.splice(0)) removeTreeWithRetry(home); @@ -39,6 +45,7 @@ describe("OAuth provider reconciliation", () => { }, }, } satisfies OcxConfig; + saveConfig(config); expect(reconcileOAuthProviders(config)).toBe(true); expect(config.providers.cursor.noVisionModels).toEqual(preset.noVisionModels); @@ -48,6 +55,180 @@ describe("OAuth provider reconciliation", () => { expect(modelInList(config.providers.cursor.noVisionModels, "composer-2.5")).toBe(true); expect(reconcileOAuthProviders(config)).toBe(false); }); + + // RED on dev (#3524): dev mutates the live object then calls saveConfig(config), so the + // startup snapshot overwrites whatever an operator wrote after loadConfig() returned. + test("rebases startup reconciliation over a concurrent provider edit", () => { + const home = mkdtempSync(join(tmpdir(), "ocx-oauth-reconcile-race-")); + homes.push(home); + process.env.OPENCODEX_HOME = home; + const preset = OAUTH_PROVIDERS.cursor.providerConfig; + const config = { + port: 10100, + defaultProvider: "cursor", + providers: { + cursor: { + ...structuredClone(preset), + authMode: "oauth", + noVisionModels: cursorModelIds(CURSOR_STATIC_MODELS), + }, + }, + } satisfies OcxConfig; + saveConfig(config); + setPersistedConfigMutationBeforeCommitForTests(() => { + const concurrent = loadConfig(); + concurrent.providers.cursor.note = "concurrent-operator-edit"; + writeFileSync(getConfigPath(), JSON.stringify(concurrent, null, 2) + "\n"); + }); + + expect(reconcileOAuthProviders(config)).toBe(true); + + expect(config.providers.cursor.noVisionModels).toEqual(preset.noVisionModels); + expect(config.providers.cursor.note).toBe("concurrent-operator-edit"); + const persisted = loadConfig(); + expect(persisted.providers.cursor.noVisionModels).toEqual(preset.noVisionModels); + expect(persisted.providers.cursor.note).toBe("concurrent-operator-edit"); + }); + + // #3524 threw here, on a call site startServer() does not guard. Reconciliation is a + // best-effort startup refresh: an unwritable config must degrade, never kill boot. + test("a config removed between load and reconcile warns and degrades to an in-memory apply", () => { + const home = mkdtempSync(join(tmpdir(), "ocx-oauth-reconcile-unavailable-")); + homes.push(home); + process.env.OPENCODEX_HOME = home; + const preset = OAUTH_PROVIDERS.cursor.providerConfig; + const config = { + port: 10100, + defaultProvider: "cursor", + providers: { + cursor: { + ...structuredClone(preset), + authMode: "oauth", + noVisionModels: cursorModelIds(CURSOR_STATIC_MODELS), + }, + }, + } satisfies OcxConfig; + saveConfig(config); + rmSync(getConfigPath()); + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + let changed: boolean | undefined; + expect(() => { changed = reconcileOAuthProviders(config); }).not.toThrow(); + expect(changed).toBe(true); + // The running process still gets a correct catalog even though nothing was written. + expect(config.providers.cursor.noVisionModels).toEqual(preset.noVisionModels); + expect(warn.mock.calls.some(([first]) => typeof first === "string" + && first.includes("OAuth provider reconciliation could not be persisted (missing)"))).toBe(true); + } finally { + warn.mockRestore(); + } + }); + + test("a malformed config degrades without writing and without throwing", () => { + const home = mkdtempSync(join(tmpdir(), "ocx-oauth-reconcile-invalid-")); + homes.push(home); + process.env.OPENCODEX_HOME = home; + const preset = OAUTH_PROVIDERS.cursor.providerConfig; + const config = { + port: 10100, + defaultProvider: "cursor", + providers: { + cursor: { + ...structuredClone(preset), + authMode: "oauth", + noVisionModels: cursorModelIds(CURSOR_STATIC_MODELS), + }, + }, + } satisfies OcxConfig; + saveConfig(config); + writeFileSync(getConfigPath(), "{ this is not json"); + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + expect(reconcileOAuthProviders(config)).toBe(true); + expect(config.providers.cursor.noVisionModels).toEqual(preset.noVisionModels); + expect(warn.mock.calls.some(([first]) => typeof first === "string" + && first.includes("OAuth provider reconciliation could not be persisted"))).toBe(true); + } finally { + warn.mockRestore(); + } + }); + + test("a fresh install with nothing to reconcile neither writes nor warns", () => { + const home = mkdtempSync(join(tmpdir(), "ocx-oauth-reconcile-fresh-")); + homes.push(home); + process.env.OPENCODEX_HOME = home; + const config = { + port: 10100, + defaultProvider: "cursor", + providers: { cursor: { ...structuredClone(OAUTH_PROVIDERS.cursor.providerConfig), authMode: "oauth" } }, + } satisfies OcxConfig; + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + // No config.json on disk at all: an unchanged projection must short-circuit before + // persistence is ever consulted, so "missing" never becomes a startup warning. + expect(reconcileOAuthProviders(config)).toBe(false); + expect(warn.mock.calls.some(([first]) => typeof first === "string" + && first.includes("OAuth provider reconciliation"))).toBe(false); + } finally { + warn.mockRestore(); + } + }); + + test("persist=false adopts the projection in memory and leaves the file untouched", () => { + const home = mkdtempSync(join(tmpdir(), "ocx-oauth-reconcile-no-persist-")); + homes.push(home); + process.env.OPENCODEX_HOME = home; + const preset = OAUTH_PROVIDERS.cursor.providerConfig; + const config = { + port: 10100, + defaultProvider: "cursor", + providers: { + cursor: { + ...structuredClone(preset), + authMode: "oauth", + noVisionModels: cursorModelIds(CURSOR_STATIC_MODELS), + }, + }, + } satisfies OcxConfig; + saveConfig(config); + const before = Bun.file(getConfigPath()); + const beforeBytes = before.size; + + expect(reconcileOAuthProviders(config, false)).toBe(true); + expect(config.providers.cursor.noVisionModels).toEqual(preset.noVisionModels); + expect(Bun.file(getConfigPath()).size).toBe(beforeBytes); + }); + + test("an untouched provider row keeps its live object identity across reconciliation", () => { + const home = mkdtempSync(join(tmpdir(), "ocx-oauth-reconcile-identity-")); + homes.push(home); + process.env.OPENCODEX_HOME = home; + const preset = OAUTH_PROVIDERS.cursor.providerConfig; + const config = { + port: 10100, + defaultProvider: "cursor", + providers: { + cursor: { + ...structuredClone(preset), + authMode: "oauth", + noVisionModels: cursorModelIds(CURSOR_STATIC_MODELS), + }, + untouched: { + adapter: "openai", + baseUrl: "http://127.0.0.1:9999/v1", + allowPrivateNetwork: true, + models: ["local-live"], + }, + }, + } satisfies OcxConfig; + saveConfig(config); + const liveUntouched = config.providers.untouched; + + expect(reconcileOAuthProviders(config)).toBe(true); + // A wholesale clear-and-reassign would silently detach every reference a caller still + // holds; only the rows the projection actually changed may be replaced. + expect(config.providers.untouched).toBe(liveUntouched); + }); test("refreshes a saved Antigravity 3.5 preset without touching credentials or user fields", async () => { const home = mkdtempSync(join(tmpdir(), "ocx-gemini-36-reconcile-")); homes.push(home); @@ -76,6 +257,7 @@ describe("OAuth provider reconciliation", () => { }, }, } satisfies OcxConfig; + saveConfig(config); expect(reconcileOAuthProviders(config)).toBe(true); const provider = config.providers["google-antigravity"]; @@ -112,6 +294,9 @@ describe("OAuth provider reconciliation", () => { }); test("migrates the version-1 canonical Antigravity static row to live discovery", () => { + const home = mkdtempSync(join(tmpdir(), "ocx-antigravity-static-reconcile-")); + homes.push(home); + process.env.OPENCODEX_HOME = home; const config = { port: 10100, defaultProvider: "google-antigravity", @@ -134,6 +319,7 @@ describe("OAuth provider reconciliation", () => { }, }, } satisfies OcxConfig; + saveConfig(config); expect(reconcileOAuthProviders(config)).toBe(true); expect(config.providers["google-antigravity"].liveModels).toBe(true); @@ -149,6 +335,9 @@ describe("OAuth provider reconciliation", () => { // healing branch. This one is the opposite claim, and the one that matters for an // additive rollout: a user who deliberately chose 3.7 must still be on 3.7 afterwards. // Google still serves it, so healing it onto 3.8 would be silently overriding a choice. + const home = mkdtempSync(join(tmpdir(), "ocx-antigravity-explicit-default-")); + homes.push(home); + process.env.OPENCODEX_HOME = home; saveCredential("google-antigravity", { access: "a", refresh: "r", projectId: "p" }); const config = { port: 10100, @@ -165,6 +354,7 @@ describe("OAuth provider reconciliation", () => { }, }, } satisfies OcxConfig; + saveConfig(config); reconcileOAuthProviders(config); const provider = config.providers["google-antigravity"]; @@ -264,7 +454,7 @@ describe("OAuth provider reconciliation", () => { }, } satisfies OcxConfig; - reconcileOAuthProviders(config); + reconcileOAuthProviders(config, false); expect(config.providers.kimi.requiresReasoningPlaceholderModels).toEqual([]); }); @@ -288,6 +478,7 @@ describe("OAuth provider reconciliation", () => { }, }, } satisfies OcxConfig; + saveConfig(config); expect(reconcileOAuthProviders(config)).toBe(true); expect(config.providers.xai.modelReasoningEfforts?.["grok-4.6"]) diff --git a/tests/providers/model-rename-migration.test.ts b/tests/providers/model-rename-migration.test.ts index 40803a4ed1..34801e8b21 100644 --- a/tests/providers/model-rename-migration.test.ts +++ b/tests/providers/model-rename-migration.test.ts @@ -1,11 +1,17 @@ -import { describe, expect, test } from "bun:test"; +import { afterEach, describe, expect, spyOn, test } from "bun:test"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { MODEL_RENAMES, projectModelRenames, type ModelRename, } from "../../src/providers/model-rename-migration"; +import { runModelRenameStartupMigration } from "../../src/providers/model-rename-startup"; import { PROVIDER_REGISTRY } from "../../src/providers/registry"; +import { getConfigPath, loadConfig, saveConfig, setPersistedConfigMutationBeforeCommitForTests } from "../../src/config"; import type { OcxConfig } from "../../src/types"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; const INTL_BASE_URL = "https://token-plan.ap-southeast-1.maas.aliyuncs.com/compatible-mode/v1"; @@ -131,3 +137,142 @@ describe("registry model rename migration (#1610)", () => { } }); }); + +describe("model rename startup persistence", () => { + const homes: string[] = []; + const originalHome = process.env.OPENCODEX_HOME; + + function isolate(prefix: string): void { + const home = mkdtempSync(join(tmpdir(), prefix)); + homes.push(home); + process.env.OPENCODEX_HOME = home; + } + + /** A saved config the renames actually rewrite, valid enough for loadConfig to accept. */ + function persistableStale(): OcxConfig { + return { port: 10100, defaultProvider: "alibaba-token-plan-intl", ...staleConfig() } as OcxConfig; + } + + afterEach(() => { + setPersistedConfigMutationBeforeCommitForTests(null); + if (originalHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = originalHome; + for (const home of homes.splice(0)) removeTreeWithRetry(home); + }); + + test("startup no-op preserves the live config object identity", () => { + const clean = projectModelRenames(staleConfig(), [RENAME]).config; + const returned = runModelRenameStartupMigration(clean, { + project: config => projectModelRenames(config, [RENAME]), + save: () => { throw new Error("no-op must not save"); }, + }); + + expect(returned).toBe(clean); + }); + + test("startup no-op still reports projection warnings", () => { + const clean = projectModelRenames(staleConfig(), [RENAME]).config; + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + const returned = runModelRenameStartupMigration(clean, { + project: config => ({ config, changed: false, warnings: ["rename target is unavailable"] }), + save: () => { throw new Error("no-op must not save"); }, + }); + + expect(returned).toBe(clean); + expect(warn).toHaveBeenCalledWith("[model-rename-migration] rename target is unavailable"); + } finally { + warn.mockRestore(); + } + }); + + // RED on dev: dev hands `projectModelRenames` the live object and saves the projection + // wholesale, so an operator edit written after loadConfig() is silently discarded. + test("rebases the startup rename over a concurrent provider edit", () => { + isolate("ocx-model-rename-race-"); + const live = persistableStale(); + saveConfig(live); + setPersistedConfigMutationBeforeCommitForTests(() => { + const concurrent = loadConfig(); + concurrent.providers["alibaba-token-plan-intl"]!.note = "concurrent-operator-edit"; + writeFileSync(getConfigPath(), JSON.stringify(concurrent, null, 2) + "\n"); + }); + + const returned = runModelRenameStartupMigration(live); + + expect(returned).toBe(live); + expect(live.providers["alibaba-token-plan-intl"]!.models).toContain("qwen3.8-max"); + expect(live.providers["alibaba-token-plan-intl"]!.note).toBe("concurrent-operator-edit"); + const persisted = loadConfig(); + expect(persisted.providers["alibaba-token-plan-intl"]!.models).toContain("qwen3.8-max"); + expect(persisted.providers["alibaba-token-plan-intl"]!.note).toBe("concurrent-operator-edit"); + }); + + // #3524 threw here, on the unguarded startServer() call site at src/server/index.ts:651. + test("a config removed between load and migrate warns and degrades to an in-memory apply", () => { + isolate("ocx-model-rename-unavailable-"); + const live = persistableStale(); + saveConfig(live); + rmSync(getConfigPath()); + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + let returned: OcxConfig | undefined; + expect(() => { returned = runModelRenameStartupMigration(live); }).not.toThrow(); + + expect(returned).toBe(live); + expect(live.providers["alibaba-token-plan-intl"]!.models).toContain("qwen3.8-max"); + expect(warn.mock.calls.some(([first]) => typeof first === "string" + && first.includes("[model-rename-migration] persistence unavailable (missing)"))).toBe(true); + } finally { + warn.mockRestore(); + } + }); + + test("a malformed config degrades without throwing", () => { + isolate("ocx-model-rename-invalid-"); + const live = persistableStale(); + saveConfig(live); + writeFileSync(getConfigPath(), "{ this is not json"); + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + expect(() => runModelRenameStartupMigration(live)).not.toThrow(); + expect(live.providers["alibaba-token-plan-intl"]!.models).toContain("qwen3.8-max"); + expect(warn.mock.calls.some(([first]) => typeof first === "string" + && first.includes("[model-rename-migration] persistence unavailable"))).toBe(true); + } finally { + warn.mockRestore(); + } + }); + + test("a fresh install with nothing to rename neither writes nor warns about persistence", () => { + isolate("ocx-model-rename-fresh-"); + const clean = projectModelRenames(persistableStale()).config; + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + expect(runModelRenameStartupMigration(clean)).toBe(clean); + expect(warn.mock.calls.some(([first]) => typeof first === "string" + && first.includes("persistence unavailable"))).toBe(false); + } finally { + warn.mockRestore(); + } + }); + + test("an untouched top-level branch keeps its live object identity across the migration", () => { + isolate("ocx-model-rename-identity-"); + const live = persistableStale(); + live.providers.untouched = { + adapter: "openai", + baseUrl: "http://127.0.0.1:9999/v1", + allowPrivateNetwork: true, + models: ["local-live"], + }; + saveConfig(live); + const liveUntouched = live.providers.untouched; + + runModelRenameStartupMigration(live); + + // adoptConfig copies key by key, so a reference a caller still holds to an unchanged + // branch survives; a clear-and-reassign would silently detach it. + expect(live.providers.untouched).toBe(liveUntouched); + }); +}); diff --git a/tests/server/server-startup-reconcile-resilience.test.ts b/tests/server/server-startup-reconcile-resilience.test.ts new file mode 100644 index 0000000000..4b5db8ba64 --- /dev/null +++ b/tests/server/server-startup-reconcile-resilience.test.ts @@ -0,0 +1,147 @@ +/** + * Startup reconciliation must never be able to kill boot (#3524). + * + * `startServer` is synchronous by design and calls `runModelRenameStartupMigration` + * (src/server/index.ts:651) and `reconcileOAuthProviders` (:663) with no try/catch. Both + * now rebase their write on the persisted config, and a persistence failure there is a + * degrade-and-warn, not a throw: an operator whose config.json vanished, was hand-edited + * into invalid JSON, or sits on an unreadable volume still gets a running proxy. + * + * RED against #3524's head, which threw "OAuth provider reconciliation persistence + * unavailable: missing" from exactly this sequence. It is NOT red on unmodified dev — dev + * never throws, it silently overwrites — so the defect itself is proven by the + * concurrent-edit tests in tests/oauth/oauth-provider-reconcile.test.ts and + * tests/providers/model-rename-migration.test.ts, not here. + */ +import { afterEach, beforeEach, expect, spyOn, test } from "bun:test"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { getConfigPath, loadConfig, saveConfig } from "../../src/config"; +import { OAUTH_PROVIDERS, reconcileOAuthProviders } from "../../src/oauth"; +import { runModelRenameStartupMigration } from "../../src/providers/model-rename-startup"; +import { startServer } from "../../src/server"; +import { CURSOR_STATIC_MODELS, cursorModelIds } from "../../src/adapters/cursor/discovery"; +import { installIsolatedCodexHome, type IsolatedCodexHome } from "../helpers/isolated-codex-home"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; +import type { OcxConfig } from "../../src/types"; + +/** + * Sandboxed agent environments deny `Bun.serve` outright ("Is port 0 in use?", EADDRINUSE on + * every port), which is an environment artifact and not a regression — the same class already + * documented for tests/server/server-combo-failover-e2e.test.ts. Probe once so the + * listener-bound assertion is hosted-CI-only while the boot-sequence assertions always run. + * + * The probe has to be `Bun.serve` itself: a `node:net` listener still binds in an environment + * where Bun's does not, so probing with the wrong API reports a false green and the skip never + * fires. + */ +function canBindLoopback(): boolean { + try { + const probe = Bun.serve({ port: 0, hostname: "127.0.0.1", fetch: () => new Response("probe") }); + probe.stop(true); + return true; + } catch { + return false; + } +} + +const CAN_BIND = canBindLoopback(); + +let testDir = ""; +let previousHome: string | undefined; +let isolatedCodexHome: IsolatedCodexHome | null = null; + +/** A saved config that reconciliation genuinely rewrites, so the persistence path is reached. */ +function staleConfig(): OcxConfig { + return { + port: 0, + hostname: "127.0.0.1", + defaultProvider: "cursor", + providers: { + cursor: { + ...structuredClone(OAUTH_PROVIDERS.cursor.providerConfig), + authMode: "oauth", + noVisionModels: cursorModelIds(CURSOR_STATIC_MODELS), + }, + }, + } as OcxConfig; +} + +/** The exact startup sequence src/server/index.ts runs at :651 and :663, and nothing else. */ +function runStartupReconciliation(config: OcxConfig): void { + reconcileOAuthProviders(runModelRenameStartupMigration(config)); +} + +beforeEach(() => { + previousHome = process.env.OPENCODEX_HOME; + isolatedCodexHome = installIsolatedCodexHome("ocx-startup-reconcile-codex-"); + testDir = mkdtempSync(join(tmpdir(), "ocx-startup-reconcile-")); + process.env.OPENCODEX_HOME = testDir; +}); + +afterEach(() => { + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + isolatedCodexHome?.restore(); + isolatedCodexHome = null; + if (testDir) removeTreeWithRetry(testDir); +}); + +test("a config removed between loadConfig() and reconcile does not throw on the boot path", () => { + saveConfig(staleConfig()); + const config = loadConfig(); + rmSync(getConfigPath()); + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + expect(() => runStartupReconciliation(config)).not.toThrow(); + // Degrading is not the same as doing nothing: the running process still gets the + // reconciled catalog, it just never reaches disk. + expect(config.providers.cursor.noVisionModels) + .toEqual(OAUTH_PROVIDERS.cursor.providerConfig.noVisionModels); + } finally { + warn.mockRestore(); + } +}); + +test("a config hand-edited into invalid JSON does not throw on the boot path", () => { + saveConfig(staleConfig()); + const config = loadConfig(); + writeFileSync(getConfigPath(), "{ this is not json"); + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + expect(() => runStartupReconciliation(config)).not.toThrow(); + } finally { + warn.mockRestore(); + } +}); + +test("a fresh install with no config file at all does not throw on the boot path", () => { + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + expect(() => runStartupReconciliation(loadConfig())).not.toThrow(); + } finally { + warn.mockRestore(); + } +}); + +// Hosted-CI-only: binds a listener, which sandboxed agent environments refuse (see +// canBindLoopback above). The three assertions above cover the same claim without a port. +test.skipIf(!CAN_BIND)( + "startServer completes and serves /healthz when the config disappears before reconcile", + async () => { + saveConfig(staleConfig()); + // Removing the file after the save reproduces the operator-visible shape: every persisted + // mutation under startServer is "unavailable" for the rest of the boot. + rmSync(getConfigPath()); + const warn = spyOn(console, "warn").mockImplementation(() => {}); + const server = startServer(0); + try { + const response = await fetch(`http://127.0.0.1:${server.port}/healthz`); + expect(response.ok).toBe(true); + } finally { + await server.stop(true); + warn.mockRestore(); + } + }, +); From ca9ad0aacd9d29e045767784b9c90b818b1890a1 Mon Sep 17 00:00:00 2001 From: t Date: Sat, 5 Sep 2026 09:21:28 +0900 Subject: [PATCH 3/5] fix(combos): scope failover cooldowns to the failure's blast radius A combo failure recorded the same cooldown regardless of what the failure actually said. An oversized request cooled a healthy target, a per-request free-tier cap cooled the whole provider for every other combo, and a rejected credential cooled only the one target that happened to hit it. Meanwhile pickComboTarget never consulted the cooldown map at all, so a target cooled a moment earlier was picked again on the next attempt. ComboFailureCooldownScope gains "none" for request-shape failures (413, input_admission_refused, context_length_exceeded, tool_catalog_too_large, cursor_root_envelope_limit, target_incompatible, and the provider hard-cap overflow), and returns "provider" for 401/402/403 and credential/billing codes. free_rate_limited leaves isProviderScopedQuotaCap: it is evaluated per request, so it keeps its hop verdict but stops recording provider-wide evidence. comboFailureDecision additionally hops model-scoped rejections and 402/425. Generic 410 and 413 remain terminal, asserted explicitly so a future widening of the hop list cannot swallow them silently. "malformed upstream" now infers 502 rather than falling into the generic "malformed" 400 branch: bytes the upstream mangled are a provider protocol failure, not a bad client request. Scoped to that phrase, so plain "malformed" keeps its 400 verdict, and asserted on the message-only path where the existing structuredServerClass override in httpStatusFromTerminalError cannot absorb it. Carries the classification half of #3348. Disk persistence of cooldowns and the policy-fallback status synthesis are deliberately separate and not included. Co-authored-by: RHODIZSECURITY <180237049+RHODIZSECURITY@users.noreply.github.com> --- scripts/test-layout/layout.json | 1 + src/combos/failover.ts | 79 ++++++- src/combos/resolve.ts | 21 +- src/lib/errors.ts | 5 + tests/codex-integration/combos.test.ts | 8 +- tests/fixtures/test-layout-expected.json | 1 + ...uter-combo-failover-classification.test.ts | 195 ++++++++++++++++++ 7 files changed, 295 insertions(+), 15 deletions(-) create mode 100644 tests/routing/router-combo-failover-classification.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index b16e32881c..5ec5e305d5 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1009,6 +1009,7 @@ "retry-after-429.test.ts": "server", "route-decision-trace.test.ts": "server", "route-explainability.test.ts": "cli", + "router-combo-failover-classification.test.ts": "routing", "router-discarded-baseurl-warning.test.ts": "routing", "router-template-baseurl.test.ts": "routing", "router.test.ts": "routing", diff --git a/src/combos/failover.ts b/src/combos/failover.ts index 4cb9cdc55a..3e786ca2f7 100644 --- a/src/combos/failover.ts +++ b/src/combos/failover.ts @@ -253,7 +253,7 @@ export function clearComboTargetCooldowns(comboId?: string): void { } export type ComboFailureDecision = "hop" | "stop"; -export type ComboFailureCooldownScope = "target" | "provider"; +export type ComboFailureCooldownScope = "none" | "target" | "provider"; function normalizedFailureCode(code?: string | null): string { return code?.trim().toLowerCase().replaceAll("-", "_") ?? ""; @@ -272,17 +272,71 @@ function isProviderScopedQuotaCap( ) { return true; } - return normalizedCode === "free_rate_limited" - || text.includes("err_free_prompt_cap") + return text.includes("err_free_prompt_cap") || (text.includes("free tier") && text.includes("single request")); } +/** + * A free-tier cap the upstream evaluates PER REQUEST rather than per account window. These + * needles used to reach only `isProviderScopedQuotaCap`, so a single oversized free-tier prompt + * cooled the whole provider for every other combo — including the shorter requests that same + * provider would still have served. `free_rate_limited` also left the provider-scoped predicate + * for the same reason; it stays a hop signal, but stops recording provider-wide evidence. + */ +function isRequestLocalFreePromptCap( + status: number | undefined, + message: string, + code?: string | null, +): boolean { + if (status !== 400) return false; + const text = message.toLowerCase(); + if (normalizedFailureCode(code) === "free_rate_limited") return true; + if (text.includes("err_free_prompt_cap")) return true; + return text.includes("free tier") && (text.includes("single request") || text.includes("prompt")); +} + +/** + * Failures that describe the SHAPE of this request rather than the health of the target. + * Cooling anything for these is wrong twice over: the target is fine, and the next request + * (shorter prompt, smaller tool catalog) would have succeeded against it. + */ +const REQUEST_SHAPE_FAILURE_CODES = new Set([ + "input_admission_refused", + "context_length_exceeded", + "tool_catalog_too_large", + "cursor_root_envelope_limit", + "target_incompatible", +]); + +/** Credential/billing failures that every target sharing the provider inherits. */ +const PROVIDER_SCOPED_FAILURE_CODES = new Set([ + "invalid_api_key", + "insufficient_quota", + "subscription_required", + "payment_required", + "billing_error", + "insufficient_balance", +]); + export function comboFailureCooldownScope( status: number, message: string, options?: { code?: string | null }, ): ComboFailureCooldownScope { - return isProviderScopedQuotaCap(status, message, options?.code) ? "provider" : "target"; + const code = normalizedFailureCode(options?.code); + // Request-shape refusals first: an oversized request must not cool a healthy target. + if ( + status === 413 + || REQUEST_SHAPE_FAILURE_CODES.has(code) + || isRequestLocalFreePromptCap(status, message, options?.code) + || isProviderTargetContextOverflow(status, message, options?.code) + ) return "none"; + if (isProviderScopedQuotaCap(status, message, options?.code)) return "provider"; + // A rejected or unpaid credential is provider-wide evidence: every target that routes + // through the same provider row carries the same key and will fail identically. + if (status === 401 || status === 402 || status === 403) return "provider"; + if (PROVIDER_SCOPED_FAILURE_CODES.has(code)) return "provider"; + return "target"; } function isModelLifecycleGone( @@ -364,15 +418,30 @@ export function comboFailureDecision( if (isProviderScopedQuotaCap(status, message, options?.code || error.code)) { return "hop"; } + // A model-scoped rejection is target-local: this provider does not serve THIS model, which + // says nothing about the next combo target. Structured code only, plus the explicit prose + // form upstreams emit when they carry no code, so an unrelated 400 stays terminal. + const failureCode = normalizedFailureCode(options?.code || error.code); + if (["model_not_found", "model_unavailable", "unsupported_model"].includes(failureCode)) { + return "hop"; + } + // `free_rate_limited` no longer routes through `isProviderScopedQuotaCap` (it is a + // per-request cap, not provider-wide evidence), so keep its hop verdict explicit here. + if (failureCode === "free_rate_limited") return "hop"; if (["origin_rejected", "context_length_exceeded", "invalid_request_error"].includes(error.code ?? "")) { return "stop"; } - if ([401, 403, 404, 408, 429].includes(status) || status >= 500) return "hop"; + // 402 (payment required) and 425 (too early) are provider-state signals, not verdicts about + // the request: another combo target can still serve it. + if ([401, 402, 403, 404, 408, 425, 429].includes(status) || status >= 500) return "hop"; if ([ "permission_denied", "subscription_required", "invalid_api_key", "insufficient_quota", + "payment_required", + "billing_error", + "insufficient_balance", "rate_limit_exceeded", "server_is_overloaded", "upstream_server_error", diff --git a/src/combos/resolve.ts b/src/combos/resolve.ts index 56d7dd8fd1..5d41d511bf 100644 --- a/src/combos/resolve.ts +++ b/src/combos/resolve.ts @@ -155,6 +155,7 @@ export function pickComboTarget( const eligible = (target: Required): boolean => targetProviderIsUsable(config, target) && !cachedProviderQuotaIsExhausted(getCachedProviderQuota(target.provider, now), now) + && !isComboTargetInCooldown(comboId, target, now) && !excluded.has(targetKey(target)) && (options.eligible?.(target) ?? true); @@ -284,14 +285,18 @@ export function advanceComboAfterFailure( ): ComboPick | null { noteComboFailure(pick.comboId, pick.target, pick.writerGeneration); const combo = getCombo(config, pick.comboId); - const cooldownTargets = options.cooldownScope === "provider" && combo - ? combo.targets.filter(target => target.provider === pick.target.provider) - : [pick.target]; - for (const target of cooldownTargets) { - coolComboTarget(pick.comboId, target, { - ...options, - writerGeneration: pick.writerGeneration, - }); + // "none" records no cooldown at all: the failure described the request, not the target, so + // the target must stay immediately selectable for the next (differently shaped) request. + if (options.cooldownScope !== "none") { + const cooldownTargets = options.cooldownScope === "provider" && combo + ? combo.targets.filter(target => target.provider === pick.target.provider) + : [pick.target]; + for (const target of cooldownTargets) { + coolComboTarget(pick.comboId, target, { + ...options, + writerGeneration: pick.writerGeneration, + }); + } } return pickComboTarget(config, pick.comboId, { exclude: pick.attempted, diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 624917507c..384f7c2d44 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -359,6 +359,11 @@ export function inferHttpStatusFromAdapterMessage(message: string): number { // the message matched "unavailable" and returned a retryable 503, so clients kept retrying // a rejection that can never succeed. if (lower.includes("failed_precondition") || lower.includes("failed precondition")) return 400; + // Bytes the upstream itself produced and then mangled are a provider protocol failure, not a + // malformed client request. This must sit ahead of the generic "malformed" -> 400 branch so a + // combo can fail over instead of returning a terminal 4xx the caller cannot act on. Scoped to + // the "malformed upstream" phrase our adapters emit; plain "malformed" keeps its 400 verdict. + if (lower.includes("malformed upstream")) return 502; if ( lower.includes("unavailable") || lower.includes("overloaded") || diff --git a/tests/codex-integration/combos.test.ts b/tests/codex-integration/combos.test.ts index a50385c54c..a1c40616c0 100644 --- a/tests/codex-integration/combos.test.ts +++ b/tests/codex-integration/combos.test.ts @@ -511,15 +511,19 @@ describe("combo failure policy and advancement", () => { expect(comboFailureDecision(413, "request too large")).toBe("stop"); }); - test("provider-scoped free-tier and monthly quota failures hop without weakening generic 400 handling", () => { + test("free-tier and monthly quota failures hop with the right scope, without weakening generic 400 handling", () => { const orca = JSON.stringify({ error: { type: "invalid_request_error", code: "free_rate_limited", message: "This prompt is longer than the free tier allows for a single request.", }}); expect(comboFailureDecision(400, orca, { code: "free_rate_limited" })).toBe("hop"); - expect(comboFailureCooldownScope(400, orca, { code: "free_rate_limited" })).toBe("provider"); + // `free_rate_limited` is a PER-REQUEST cap ("longer than the free tier allows for a single + // request"), so it must not cool the provider for every other combo: a shorter prompt would + // still have been served. It keeps its hop verdict but records no cooldown evidence. + expect(comboFailureCooldownScope(400, orca, { code: "free_rate_limited" })).toBe("none"); expect(comboFailureDecision(400, "ordinary invalid request", { code: "invalid_request_error" })).toBe("stop"); + // The account-window quota cap is genuine provider-wide evidence and keeps its scope. expect(comboFailureCooldownScope(429, "Monthly usage limit reached. Resets in 14 days.", { code: "GoUsageLimitError", })).toBe("provider"); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 0b0c65d0ed..4909f508c5 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -846,6 +846,7 @@ "retry-after-429.test.ts": "server", "route-decision-trace.test.ts": "server", "route-explainability.test.ts": "cli", + "router-combo-failover-classification.test.ts": "routing", "router-discarded-baseurl-warning.test.ts": "routing", "router-template-baseurl.test.ts": "routing", "router.test.ts": "routing", diff --git a/tests/routing/router-combo-failover-classification.test.ts b/tests/routing/router-combo-failover-classification.test.ts new file mode 100644 index 0000000000..742f411bfb --- /dev/null +++ b/tests/routing/router-combo-failover-classification.test.ts @@ -0,0 +1,195 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { + advanceComboAfterFailure, + clearComboSelectionState, + clearComboTargetCooldowns, + coolComboTarget, + isComboTargetInCooldown, + pickComboTarget, + targetKey, +} from "../../src/combos"; +import { comboFailureCooldownScope, comboFailureDecision } from "../../src/combos/failover"; +import { adapterFailureFromMessage, inferHttpStatusFromAdapterMessage } from "../../src/lib/errors"; +import type { OcxConfig } from "../../src/types"; + +/** + * Cooldown scope and hop/stop verdicts must match a failure's actual blast radius. Before this + * suite, every non-quota failure cooled the target it hit — including request-shape refusals + * that say nothing about target health — and `pickComboTarget` never consulted the cooldown + * map at all, so a target cooled a moment earlier was picked again immediately. + */ + +function comboConfig(): OcxConfig { + return { + port: 10100, + defaultProvider: "a", + providers: { + a: { adapter: "openai-chat", baseUrl: "https://a.example/v1", apiKey: "ka", models: ["m1"] }, + b: { adapter: "openai-chat", baseUrl: "https://b.example/v1", apiKey: "kb", models: ["m2"] }, + }, + combos: { + free: { + strategy: "failover", + targets: [ + { provider: "a", model: "m1" }, + { provider: "b", model: "m2" }, + ], + }, + }, + }; +} + +const first = { provider: "a", model: "m1" }; + +afterEach(() => { + clearComboSelectionState(); + clearComboTargetCooldowns(); +}); + +describe("combo failure cooldown scope", () => { + test("request-shape refusals cool nothing at all", () => { + // An oversized request is a fact about the request, not the target: cooling here would make + // the next, shorter request skip a provider that would have served it. + expect(comboFailureCooldownScope(413, "request entity too large")).toBe("none"); + for (const code of [ + "input_admission_refused", + "context_length_exceeded", + "tool_catalog_too_large", + "cursor_root_envelope_limit", + "target_incompatible", + ]) { + expect(comboFailureCooldownScope(400, "refused locally", { code })).toBe("none"); + } + // Hyphenated spellings normalize to the same codes. + expect(comboFailureCooldownScope(400, "refused", { code: "input-admission-refused" })).toBe("none"); + // A provider's own per-target hard cap (vendor code 5059) is equally request-shaped. + expect(comboFailureCooldownScope( + 400, + "prompt 900000 > 200000 maximum context length", + { code: "5059" }, + )).toBe("none"); + }); + + test("a per-request free-tier cap does not cool the whole provider", () => { + // `free_rate_limited` is evaluated per request, so provider-wide cooldown punished every + // other combo for one oversized free-tier prompt. + expect(comboFailureCooldownScope(400, "prompt too long for the free tier", { + code: "free_rate_limited", + })).toBe("none"); + }); + + test("credential and billing failures cool the whole provider", () => { + expect(comboFailureCooldownScope(401, "invalid api key")).toBe("provider"); + expect(comboFailureCooldownScope(402, "payment required")).toBe("provider"); + expect(comboFailureCooldownScope(403, "forbidden")).toBe("provider"); + for (const code of [ + "invalid_api_key", + "insufficient_quota", + "subscription_required", + "payment_required", + "billing_error", + "insufficient_balance", + ]) { + expect(comboFailureCooldownScope(500, "upstream said no", { code })).toBe("provider"); + } + // The pre-existing account-window quota cap keeps its provider scope. + expect(comboFailureCooldownScope(429, "monthly usage limit reached")).toBe("provider"); + }); + + test("an ordinary target failure still cools only that target", () => { + expect(comboFailureCooldownScope(500, "internal server error")).toBe("target"); + expect(comboFailureCooldownScope(429, "rate limit reached for requests")).toBe("target"); + }); +}); + +describe("combo failure hop/stop verdicts", () => { + test("model-scoped rejections hop to the next target", () => { + for (const code of ["model_not_found", "model_unavailable", "unsupported_model"]) { + expect(comboFailureDecision(400, "upstream rejected the model", { code })).toBe("hop"); + } + }); + + test("402 and 425 hop instead of ending the chain", () => { + expect(comboFailureDecision(402, "payment required")).toBe("hop"); + expect(comboFailureDecision(425, "too early")).toBe("hop"); + }); + + test("a per-request free-tier cap still hops", () => { + expect(comboFailureDecision(400, "free tier prompt cap", { code: "free_rate_limited" })).toBe("hop"); + }); + + test("INVARIANT: generic 410 and 413 remain terminal", () => { + // These two are the tripwire for this change. A widened hop list must never swallow them: + // 410 without a structured lifecycle code is a real resource-gone verdict, and a generic + // 413 is a request the next target would reject identically. + expect(comboFailureDecision(410, "resource is gone")).toBe("stop"); + expect(comboFailureDecision(413, "request too large")).toBe("stop"); + }); + + test("INVARIANT: a structured model lifecycle 410 still hops", () => { + expect(comboFailureDecision(410, "model retired", { code: "model_end_of_life" })).toBe("hop"); + }); +}); + +describe("cooled targets are not selectable", () => { + test("a target inside its cooldown window is skipped by pickComboTarget", () => { + const config = comboConfig(); + const now = 10_000; + coolComboTarget("free", first, { now, cooldownMs: 60_000 }); + expect(isComboTargetInCooldown("free", first, now + 5_000)).toBe(true); + const pick = pickComboTarget(config, "free", { now: now + 5_000 }); + expect(pick && targetKey(pick.target)).toBe(targetKey({ provider: "b", model: "m2" })); + }); + + test("an expired cooldown makes the target selectable again", () => { + const config = comboConfig(); + const now = 10_000; + coolComboTarget("free", first, { now, cooldownMs: 1_000 }); + const pick = pickComboTarget(config, "free", { now: now + 1_000 }); + expect(pick && targetKey(pick.target)).toBe(targetKey(first)); + }); + + test("a \"none\" scope records no cooldown, so the target stays selectable", () => { + const config = comboConfig(); + const now = 10_000; + const pick = pickComboTarget(config, "free", { now })!; + expect(targetKey(pick.target)).toBe(targetKey(first)); + advanceComboAfterFailure(config, pick, { + now, + cooldownScope: comboFailureCooldownScope(413, "request entity too large"), + status: 413, + message: "request entity too large", + }); + expect(isComboTargetInCooldown("free", first, now)).toBe(false); + // The failed target is excluded from THIS request via `attempted`, but a fresh request + // (no exclusions) must still find it healthy. + expect(targetKey(pickComboTarget(config, "free", { now })!.target)).toBe(targetKey(first)); + }); + + test("a target-scoped failure does record a cooldown", () => { + const config = comboConfig(); + const now = 10_000; + const pick = pickComboTarget(config, "free", { now })!; + advanceComboAfterFailure(config, pick, { + now, + cooldownScope: comboFailureCooldownScope(500, "internal server error"), + status: 500, + message: "internal server error", + }); + expect(isComboTargetInCooldown("free", first, now)).toBe(true); + }); +}); + +describe("malformed upstream bytes are a provider failure", () => { + test("\"malformed upstream\" infers 502, not a client 4xx", () => { + // Message-only path: no structured `server_error` type, so the `structuredServerClass` + // override in httpStatusFromTerminalError cannot absorb this case. Plain "malformed" + // keeps its 400 verdict, which is what scopes the new branch. + expect(inferHttpStatusFromAdapterMessage("malformed upstream SSE data frame")).toBe(502); + expect(inferHttpStatusFromAdapterMessage("malformed request payload")).toBe(400); + expect(adapterFailureFromMessage("malformed upstream SSE data frame")).toMatchObject({ + httpStatus: 502, + error: { type: "server_error", code: "upstream_server_error" }, + }); + }); +}); From cc23704b9fa7bc29ce5c3504c46aff3a602dc848 Mon Sep 17 00:00:00 2001 From: t Date: Sat, 5 Sep 2026 09:21:43 +0900 Subject: [PATCH 4/5] fix(providers): recover a key-pool 401 by rotating instead of failing the request A static API-key pool already rotates on 429 but abandoned the provider on 401, even though one revoked or mistyped key says nothing about its siblings. Add rotateKeyOn401/rotateProviderTransportOn401 alongside the 429 pair (sharing the same persisted-config CAS and transport-rebuild rules) and consult them in the Responses recovery loop, after the OAuth replay so a refreshable token is never treated as a dead key. hasKeyPoolFailover already excludes oauth/forward modes. A 401 is a verdict about the credential, not a timing signal, and upstreams send no Retry-After for it, so the failed key is held for the full cap rather than the 429 default. The new key-401 recovery kind is a four-site chain, not one edit: the union in src/usage/log.ts, the ATTEMPT_RECOVERY_KINDS set that filters it back on read, the emit site in the Responses loop, and COOLDOWN_RECOVERY_KINDS in routing analytics. The regression round-trips a persisted attempt through the log file, because a kind added to the type but missing from the set writes fine and vanishes on read-back. Carries the key-401 half of #3348. Co-authored-by: RHODIZSECURITY <180237049+RHODIZSECURITY@users.noreply.github.com> --- src/providers/key-failover.ts | 63 ++++++++++++++++++++++++++++++++--- src/routing/analytics.ts | 1 + src/server/responses/core.ts | 32 ++++++++++++++++++ src/usage/log.ts | 2 ++ tests/usage/usage-log.test.ts | 28 ++++++++++++++++ 5 files changed, 121 insertions(+), 5 deletions(-) diff --git a/src/providers/key-failover.ts b/src/providers/key-failover.ts index 9bc8f8d210..9c7a42e11b 100644 --- a/src/providers/key-failover.ts +++ b/src/providers/key-failover.ts @@ -179,9 +179,10 @@ export function rateLimitRetryDelayMs( * rebuilds from this committed row, reapplies registry metadata, and retains only explicit * runtime transport state (`fetch` and generated OpenCode session affinity). */ -export function rotateKeyOn429( +function rotateKeyAfterFailure( config: OcxConfig, providerName: string, + failureStatus: 401 | 429, retryAfterHeader: string | null | undefined, now = Date.now(), attemptedKey?: string, @@ -235,12 +236,17 @@ export function rotateKeyOn429( }); if (outcome.status === "unavailable" || outcome.value === null) return null; if (outcome.value.failedId) { - const cooldownMs = parseRetryAfterMs(retryAfterHeader, now) ?? DEFAULT_COOLDOWN_MS; + // A 401 is a verdict about the credential itself, not a timing signal: the key is rejected + // until an operator replaces it, and upstreams send no Retry-After for it. Hold it for the + // full cap instead of the 429 default so a dead key is not re-tried once a minute. + const cooldownMs = failureStatus === 401 + ? MAX_COOLDOWN_MS + : parseRetryAfterMs(retryAfterHeader, now) ?? DEFAULT_COOLDOWN_MS; keyCooldowns.set(cooldownKey(providerName, outcome.value.failedId), { cooldownUntil: now + cooldownMs }); sweepExpiredOnWrite(now); } if ("exhaustedCount" in outcome.value) { - console.warn(`[key-failover] ${providerName}: all ${outcome.value.exhaustedCount} keys in cooldown; returning 429 to client`); + console.warn(`[key-failover] ${providerName}: all ${outcome.value.exhaustedCount} keys in cooldown after ${failureStatus}; returning the upstream status to the client`); return null; } @@ -249,12 +255,39 @@ export function rotateKeyOn429( if (outcome.value.candidateId) { console.warn( // Log ids only — labels are user-supplied free text and could carry secret material. - `[key-failover] ${providerName}: 429 on key ${outcome.value.failedId ?? "?"}; rotating to key ${outcome.value.candidateId}`, + `[key-failover] ${providerName}: ${failureStatus} on key ${outcome.value.failedId ?? "?"}; rotating to key ${outcome.value.candidateId}`, ); } return structuredClone(committed); } +export function rotateKeyOn429( + config: OcxConfig, + providerName: string, + retryAfterHeader: string | null | undefined, + now = Date.now(), + attemptedKey?: string, +): OcxProviderConfig | null { + return rotateKeyAfterFailure(config, providerName, 429, retryAfterHeader, now, attemptedKey); +} + +/** + * Record a 401 for the current key and attempt to switch to the next available one. + * + * A static key pool can recover a credential-scoped 401 without abandoning the provider: one + * revoked or mistyped key in a pool of several says nothing about its siblings. OAuth and + * forward providers never reach here — they refresh or re-authenticate instead, and + * `rotateKeyAfterFailure` rejects both auth modes outright. + */ +export function rotateKeyOn401( + config: OcxConfig, + providerName: string, + now = Date.now(), + attemptedKey?: string, +): OcxProviderConfig | null { + return rotateKeyAfterFailure(config, providerName, 401, null, now, attemptedKey); +} + export function sweepExpiredApiKeyCooldowns(now = Date.now()): number { let removed = 0; for (const [key, cooldown] of keyCooldowns) { @@ -292,7 +325,27 @@ export function rotateProviderTransportOn429( options.attemptedKey, ); if (!rotated) return null; + return applyRotatedTransport(providerName, routedProvider, rotated, options.promptCacheKey); +} +/** 401 counterpart of `rotateProviderTransportOn429`; shares its transport-rebuild rules. */ +export function rotateProviderTransportOn401( + config: OcxConfig, + providerName: string, + routedProvider: OcxProviderTransport, + options: Omit = {}, +): OcxProviderTransport | null { + const rotated = rotateKeyOn401(config, providerName, options.now, options.attemptedKey); + if (!rotated) return null; + return applyRotatedTransport(providerName, routedProvider, rotated, options.promptCacheKey); +} + +function applyRotatedTransport( + providerName: string, + routedProvider: OcxProviderTransport, + rotated: OcxProviderConfig, + promptCacheKey?: string, +): OcxProviderTransport { const committedRoute = routedProviderConfig(providerName, rotated); const routedSession = routedProvider.headers?.[OPENCODE_GO_SESSION_HEADER]; const retryProvider: OcxProviderTransport = { @@ -307,7 +360,7 @@ export function rotateProviderTransportOn429( } : {}), }; - return resolveProviderTransport(providerName, retryProvider, options.promptCacheKey); + return resolveProviderTransport(providerName, retryProvider, promptCacheKey); } /** Clear cooldown state for a provider (e.g. after manual key management). */ diff --git a/src/routing/analytics.ts b/src/routing/analytics.ts index 6dc217e734..b41e49442b 100644 --- a/src/routing/analytics.ts +++ b/src/routing/analytics.ts @@ -116,6 +116,7 @@ interface Bucket extends AnalyticsBreakdownRow { const COOLDOWN_RECOVERY_KINDS = new Set([ "rate-limit-429", + "key-401", "key-429", "oauth-401", "anthropic-oauth-429", diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index beb88fe7f2..1fbc853613 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -238,6 +238,7 @@ import { rateLimitRetryDelayMs, rateLimitRetryPolicyFor, rotateProviderTransportOn429, + rotateProviderTransportOn401, transientRetryPolicyFor, } from "../../providers/key-failover"; import { shouldAttemptImageTierRetry } from "../image-retry"; @@ -6154,6 +6155,37 @@ async function handleResponsesInner( continue recovery; } + // Static API-key pools can recover a credential-scoped 401 without abandoning the + // provider: one revoked or mistyped key says nothing about its siblings. OAuth providers + // refresh above and never enter here — `hasKeyPoolFailover` rejects oauth/forward modes. + // Runs after the OAuth replay so a refreshable token is never treated as a dead key. + while (upstreamResponse.status === 401 && hasKeyPoolFailover(route.provider)) { + const rotated = rotateProviderTransportOn401(config, route.providerName, route.provider, { + now: Date.now(), + attemptedKey: route.provider.apiKey, + promptCacheKey: parsed.options.promptCacheKey, + }); + if (!rotated) break; + // Release the failed response's socket before retrying; unread bodies otherwise linger + // until runtime cleanup (one per rotated key). + try { void upstreamResponse.body?.cancel().catch(() => {}); } catch { /* already consumed/closed */ } + route.provider = rotated; + invalidateSameTargetRequest(); + activeAdapter = resolveAdapter( + resolveWireProtocolOverride(route.providerName, route.modelId, route.provider, inboundWire), + config.cacheRetention, + ); + bindRouteReasoningReplayScope({ + parsed, + providerName: route.providerName, + provider: route.provider, + adapterName: activeAdapter.name, + }); + const result = await rebuildAndRefetch("key-401"); + if ("failed" in result) return result.failed; + upstreamResponse = result; + } + // Same-target 429 wait-and-retry (opt-in `retryOn429`, issue #487). Codex never retries // 429 itself (it retries 5xx only), and single-key pools cannot use the failover below, // so wait (Retry-After or the fixed interval) and replay the IDENTICAL request on the diff --git a/src/usage/log.ts b/src/usage/log.ts index 6a74ae7f96..acbe8a00db 100644 --- a/src/usage/log.ts +++ b/src/usage/log.ts @@ -46,6 +46,7 @@ export type AttemptRecoveryKind = | "transient-5xx" | "connection-reset" | "oauth-401" + | "key-401" | "key-429" | "rate-limit-429" | "anthropic-oauth-429" @@ -257,6 +258,7 @@ const ATTEMPT_RECOVERY_KINDS = new Set([ "transient-5xx", "connection-reset", "oauth-401", + "key-401", "key-429", "rate-limit-429", "anthropic-oauth-429", diff --git a/tests/usage/usage-log.test.ts b/tests/usage/usage-log.test.ts index 76c08c7212..42648e45bc 100644 --- a/tests/usage/usage-log.test.ts +++ b/tests/usage/usage-log.test.ts @@ -114,6 +114,34 @@ describe("usage log", () => { expect(readUsageEntries()[0]?.attempts?.[0]?.recoveryKinds).toEqual(["rate-limit-429"]); }); + test("persists the key-401 recovery kind on attempts", () => { + // ATTEMPT_RECOVERY_KINDS is the deserialization filter: a kind added to the type but not to + // the set writes fine and vanishes on read-back, so this must round-trip through the file + // rather than merely typecheck. + const entry: PersistedUsageEntry = { + requestId: "ocx-key-401-kind", + timestamp: 1, + provider: "blsc", + model: "blsc/DeepSeek-V4-Flash", + status: 200, + durationMs: 4, + usageStatus: "reported", + attempts: [{ + ordinal: 1, + provider: "blsc", + model: "blsc/DeepSeek-V4-Flash", + adapter: "openai-chat", + status: 200, + durationMs: 4, + sendCount: 2, + recoveryKinds: ["key-401"], + usageStatus: "reported", + }], + }; + appendUsageEntry(entry); + expect(readUsageEntries()[0]?.attempts?.[0]?.recoveryKinds).toEqual(["key-401"]); + }); + test("persists the empty-completion recovery kind on attempts", () => { const entry: PersistedUsageEntry = { requestId: "ocx-empty-completion-kind", From 6a31fcb7715a72bd8b3b4052998f711b1a6ffddc Mon Sep 17 00:00:00 2001 From: t Date: Sat, 5 Sep 2026 10:07:46 +0900 Subject: [PATCH 5/5] test(providers): pin the 401 rotation cooldown and the third key-pool recovery site Review round 1 (023): the rotator-count guard now records the pre-stream 401 site (key = 3) and rotateKeyOn401 / rotateProviderTransportOn401 get their own cooldown assertions (MAX_COOLDOWN_MS on 401 vs the 429 default). Co-authored-by: RHODIZSECURITY <180237049+RHODIZSECURITY@users.noreply.github.com> --- tests/adapters/key-failover.test.ts | 37 ++++++++++++++++++++++ tests/oauth/generic-oauth-failover.test.ts | 8 +++-- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/tests/adapters/key-failover.test.ts b/tests/adapters/key-failover.test.ts index e2595a84f0..7f4a77ef55 100644 --- a/tests/adapters/key-failover.test.ts +++ b/tests/adapters/key-failover.test.ts @@ -14,7 +14,9 @@ import { getKeyCooldownUntil, hasKeyPoolFailover, rotateKeyOn429, + rotateKeyOn401, rotateProviderTransportOn429, + rotateProviderTransportOn401, } from "../../src/providers/key-failover"; import { resolveOpenCodeGoTransport } from "../../src/providers/opencode-go-transport"; import { deriveXaiConvId } from "../../src/providers/xai-transport"; @@ -108,6 +110,7 @@ describe("rotateKeyOn429", () => { test("returns null for oauth/forward providers and single-key pools", () => { const oauth = makeConfig({ authMode: "oauth", apiKey: "t", apiKeyPool: pool3() }); + expect(rotateKeyOn401(oauth, "p")).toBeNull(); expect(rotateKeyOn429(oauth, "p", null)).toBeNull(); const single = makeConfig({ apiKey: "key-alpha-000111222333", apiKeyPool: [pool3()![0]] }); expect(rotateKeyOn429(single, "p", null)).toBeNull(); @@ -334,3 +337,37 @@ describe("rotateProviderTransportOn429", () => { expect(JSON.stringify(rotated?.headers)).not.toContain(promptCacheKey); }); }); + +describe("rotateKeyOn401", () => { + test("rotates to the next key and holds the rejected key for the full cap, not the 429 default", () => { + const config = makeConfig({ apiKey: "key-alpha-000111222333", apiKeyPool: pool3() }); + const now = 1_000_000; + const rotated = rotateKeyOn401(config, "p", now); + expect(rotated?.apiKey).toBe("key-beta-444555666777"); + expect(config.providers.p.apiKey).toBe("key-beta-444555666777"); + // A 401 is a verdict about the credential, so the cooldown is MAX_COOLDOWN_MS (10 min), + // not the 60s DEFAULT_COOLDOWN_MS a header-less 429 gets. + expect(getKeyCooldownUntil("p", "k1", now)).toBe(now + 10 * 60_000); + expect(getKeyCooldownUntil("p", "k1", now)).not.toBe(now + 60_000); + }); + + test("a rejected key is not retried once the 429 default window would have elapsed", () => { + const config = makeConfig({ apiKey: "key-alpha-000111222333", apiKeyPool: pool3() }); + const now = 1_000_000; + rotateKeyOn401(config, "p", now); + rotateKeyOn401(config, "p", now); + // alpha and beta are both rejected; gamma is the only candidate left. + expect(rotateKeyOn401(config, "p", now)?.apiKey ?? config.providers.p.apiKey).toBe("key-gamma-888999000111"); + // 61s later a 429 cooldown would have expired, but a 401 hold has not. + expect(getKeyCooldownUntil("p", "k1", now + 61_000)).toBe(now + 10 * 60_000); + }); + + test("rotateProviderTransportOn401 rebuilds the transport with the rotated key", () => { + const config = makeConfig({ apiKey: "key-alpha-000111222333", apiKeyPool: pool3() }); + const now = 1_000_000; + const routed = { ...config.providers.p } as Parameters[2]; + const rotated = rotateProviderTransportOn401(config, "p", routed, { now }); + expect(rotated?.apiKey).toBe("key-beta-444555666777"); + expect(getKeyCooldownUntil("p", "k1", now)).toBe(now + 10 * 60_000); + }); +}); diff --git a/tests/oauth/generic-oauth-failover.test.ts b/tests/oauth/generic-oauth-failover.test.ts index e5bd26e014..dc8fec9714 100644 --- a/tests/oauth/generic-oauth-failover.test.ts +++ b/tests/oauth/generic-oauth-failover.test.ts @@ -342,14 +342,16 @@ describe("sidecar on429 wiring", () => { // generic = 4: streaming loop, continuation loop, sidecar hook, runTurn preflight. // anthropic = 3: the same, MINUS runTurn -- that path is Cursor-only (cursor.ts is the // sole adapter implementing runTurn), so Anthropic cannot reach it. - // key = 2: hasKeyPoolFailover guards only the two response loops; the sidecar - // reaches the key pool through rotateProviderTransportOn429 instead. + // key = 3: hasKeyPoolFailover guards the two 429 response loops plus the + // pre-stream 401 recovery site (a rejected key rotates instead of + // failing the request); the sidecar reaches the key pool through + // rotateProviderTransportOn429 instead. // // Adding a fifth recovery site means deciding, deliberately, which rotators it needs and // updating the matching number. That decision is the thing this test exists to force. expect(counts.generic).toBe(4); expect(counts.anthropic).toBe(3); - expect(counts.key).toBe(2); + expect(counts.key).toBe(3); }); test("the helper fails closed rather than pairing a new bearer with an old identity", () => {