fix(oauth): rebase startup reconciliation on the persisted config (reimplementation of #3524) - #3564
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 68 / 80이 PR은 부팅 경로에서 OAuth 프로바이더 재조정·모델 rename 마이그레이션이 메모리 투영을 디스크에 덮어써서 동시에 고친 운영자 설정을 지우던 문제를 고칩니다. 지금 src/oauth/index.ts src/oauth/index.ts src/oauth/index.ts src/providers/model-rename-startup.ts tests/server/server-startup-reconcile-resilience.test.ts - Bun.serve bind 실패 시 skip은 로컬 샌드박스용. hosted CI에서 실제로 도는지 머지 전 확인. #3524 - OPEN leftover. 랜딩 후 Landed via + landed-via-maintainer. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
`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>
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>
e9e9ebd to
dc07467
Compare
589347f to
29182de
Compare
|
Maintainer admin merge (ruleset bypass recorded per MAINTAINERS.md): reimplementation of #3524, exact-head CI green on 29182de (24 pass / 2 skipped / 0 fail), stack layer above the merged #3563 (adcf8a7). Security-surface note: src/oauth/index.ts persistence ordering only — no credential, token, or flow change; reviewer 023 read mutatePersistedConfig in full and confirmed no partial write is possible. wp2 Stack B of the 260905 open-work closeout. |
1 participant
Summary
Startup OAuth provider reconciliation and the model-rename startup migration both wrote their in-memory projection over the on-disk config, silently discarding a concurrent operator edit (
src/oauth/index.ts:1284on dev). Both now commit throughmutatePersistedConfigand adopt the persisted result key by key (recursively, so sibling provider rows keep identity). #3524's version added an unguardedthrowwhen persistence was unavailable, which turned a survivable state into a boot failure; this reimplementation warns and adopts instead, and a startup resilience test provesstartServersurvives a config removed between load and reconcile.Carries #3524 (author @yansigit) onto current
dev; reimplemented per the campaign plan. Supersedes #3524.Security boundary: touches
src/oauth/index.ts(restricted surface) — persistence ordering only, no credential or flow change.Stack (merge bottom-up; each layer targets the branch below):
Depends on the layers below it. Review this PR's diff only. Unit:
devlog/_plan/260905_open_work_closeout/(020, 021, 022).Verification
bun run typecheck— exit 0 on this layer and on the stack top (d0f80e85f).tests/oauth/oauth-provider-reconcile.test.ts11 pass / 4 fail andtests/providers/model-rename-migration.test.ts13 pass / 3 fail (concurrent-edit rebase — the RED-on-dev defect proof).tests/server/server-startup-reconcile-resilience.test.tsis RED against fix(oauth): persist startup reconciliation before adoption #3524's head only (dev never throws) and skips locally whenBun.servecannot bind (hosted CI runs it). GREEN 52 pass / 0 fail unsandboxed;bun run test:changed10747 pass / 0 fail unsandboxed. Layout guard 17/0.tests/lab/core-lab-boundary.test.ts.Checklist
codex/260905-claude-native-fallback(stack layer)Co-authored-bytrailer in the branch commitCo-authored-by: yansigit 44089734+yansigit@users.noreply.github.com