Repository navigation
fix(codex): stop a stopped proxy from locking users out of Codex sign-in - #5267
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change makes stale Codex routing identifiable and reversible. It adds an undo-aware routing marker, dead-proxy guidance in ChangesCodex routing recovery
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Codex
participant ocx status
participant RoutingAdvice
participant Operator
Codex->>ocx status: report startup routing and proxy state
ocx status->>RoutingAdvice: evaluate proxyUp=false and routingKind=opencodex-local
RoutingAdvice-->>ocx status: return sign-in failure and ocx restore guidance
ocx status-->>Operator: print recovery instructions
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab485792da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const line of deadProxyRoutingAdviceLines({ | ||
| proxyUp: false, | ||
| routingKind: status.json.startup.routingKind, |
There was a problem hiding this comment.
When the proxy crashes or the machine reboots without deleting ocx.pid, collectStatus() preserves that stale PID while health.ok and proxy.running are false. The surrounding if (!(status.json.proxy.pid || status.json.proxy.health.ok)) therefore skips this newly added advice entirely, even though this is a primary lockout scenario. Gate the not-running block on verified liveness such as proxy.running rather than PID-file presence, and cover the actual status output rather than only testing the helper directly.
Useful? React with 👍 / 👎.
| You can undo the routing by hand. Open `$CODEX_HOME/config.toml` and delete | ||
| each `# Auto-injected by opencodex` comment together with the single line | ||
| directly below it, plus any `model_catalog_json` line ending in | ||
| `opencodex-catalog.json`. Leave the rest of the file alone. |
There was a problem hiding this comment.
For a non-loopback installation, this marker is directly above [model_providers.opencodex]; following these instructions deletes only the marker and table header, leaving the root model_provider = "opencodex" without a provider definition and leaving the table fields misplaced, so Codex can remain unable to load. The same broad marker text can also match prompt-layer ownership comments. Limit the simple pair deletion to the two root routing keys and separately explain removal of the entire provider-table form and its root selector.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| * | ||
| * Scope is routing only. Prompt layers keep the bare marker: `ocx restore` is not their undo. | ||
| */ | ||
| export const OCX_ROUTING_MARKER_LINE = `${OCX_SECTION_MARKER} (undo: ocx restore)`; |
There was a problem hiding this comment.
This changes persisted Codex routing configuration and CLI status behavior, but the commit updates no files under structure/. The source-area map assigns src/codex/ and src/cli/ to multiple architecture documents, and the scoped repository rule requires those documents to be updated in the same change; synchronize the applicable routing/configuration and runtime descriptions with the new marker and status behavior.
AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
리뷰 · 우선순위 61 / 80이 PR은 #5261에서 나온 “프록시가 죽었는데 Codex 로그인까지 막힌” 상태를, 잠금 자체를 없애기보다 빠져나가는 길을 보이게 만드는 레인 H다. 베이스는 이번 변경은 세 갈래다. (1) 라우팅 소유 마커를 라인 - 라인 - 라인 - tip 메인테이너의 판단이 필요한 지점 status 게이트를 너의 추천 방향은 맞고 범위도 잠금 발견 가능성에 잘 맞춰져 있다. 머지 전 status 게이트만 tip에 짧게 고치는 쪽을 권한다 — #5261 재부팅 장면과 정확히 겹치고, 이 댓글은 grok-bot이 작성했습니다 |
A Windows user whose proxy had stopped was locked out of Codex sign-in (#5261). The root openai_base_url opencodex writes keeps pointing Codex's built-in openai provider at 127.0.0.1:10100 after the proxy is gone, and the injection survives reboot, so the lockout persists. The only surface such a user can still read is config.toml, and it named no way out: the marker said "Auto-injected by opencodex" and nothing else. The recovery they found was to hand-delete the routing lines and the catalog file, which is worse than "ocx restore" -- a model_catalog_json target that no longer exists makes Codex fail on a missing file. Routing markers now read "# Auto-injected by opencodex (undo: ocx restore)". Every ownership predicate matches OCX_SECTION_MARKER as a substring rather than by equality, so both the new line and markers written by earlier builds are still recognized, stripped and restored. An in-place rewrite refreshes the marker, so an existing install gains the hint on its next start instead of keeping a bare marker. Scope is routing only. Prompt layers keep the bare marker, because "ocx restore" is not what undoes them.
When the proxy is down, "ocx status" says Codex requests will fail and then offers only ways to bring the proxy back: restart it, install the service, repair the service. For the user in #5261 that was the wrong half of the choice. Their injected routing points Codex's own built-in openai provider at a dead loopback port, so they were stopped at Codex sign-in, and every suggestion on screen asked them to fix opencodex first. Add the other half. When the proxy is down and the routing is one opencodex owns, the report now says that sign-in fails too, and names "ocx restore", which needs no proxy, no management API and no network. The sentences live in a pure function beside unusedProxyWarningLines so they are testable without spawning the CLI. Routing opencodex does not own is excluded: "ocx restore" would not remove somebody else's local gateway, so advertising it there would be a false promise.
Covers the state #5261 was actually reported in: routing on disk, proxy gone, nothing listening on the loopback port Codex is pointed at. The tests never start a proxy or bind a port, because recovery has to work without one, and a test that needed a live proxy would be exercising the wrong state. What it holds: - Routing written before the recovery hint existed is still recognized, and recovery from the old and new marker forms is byte-identical, so an install that upgrades mid-incident restores the same way. - Removal clears the dead base URL, the realtime sideband override and the catalog pointer together, while leaving the user's own keys. The catalog pointer matters as much as the routing: left behind, it names a file only opencodex maintains and Codex fails on a missing target. - An install that predates the hint gains it in place on the next injection, without adding an ownership line or breaking idempotency. - A user-owned root override is still untouched and gains no hint. - The dead-proxy advice appears only for routing opencodex owns. - Both recovery surfaces name a command the CLI registry actually has. That one is derived from the marker rather than restated, so renaming the command in one place fails here instead of shipping a config file that points at nothing.
… proxy There was no page for the state in #5261, and it is the one a user in it can actually reach: Codex is unusable, so the docs site and the config file are what is left. The page names the mechanism, both ways out, and the manual edit for someone without the CLI. It warns specifically against deleting the catalog pointer on its own, which is the repair people reach for and which produces the same symptom from a second cause. Account-pool failures are covered separately on the same page rather than folded into the lockout. They happened in the same session in the report, but the pool needs a live management API and a fixed loopback callback port, so they are a different problem with a different fix.
Names the mechanism, the four independent source reads that agreed on it, and the three gaps this lane deliberately leaves open. Also records that the account-pool failures which opened the report are a second cause with a different fix, so a later reader does not merge them.
… steps Three existing cases asserted that injecting over routing we already own returns the file byte for byte apart from the URL. Refreshing the ownership marker breaks that literal expectation, and hosted CI failed on exactly those three. The contract they protect still holds -- injection is idempotent and no unrelated value moves -- so they now expect our own marker to refresh and assert everything else unchanged, including the malformed tail that must be returned verbatim. The troubleshooting page told a stuck user to delete every "# Auto-injected by opencodex" comment and the line below it. That same comment sits above other managed keys, such as an injected developer_instructions, so following it would have cost configuration that has nothing to do with sign-in. It now names the three keys to remove and says to go by the key rather than the comment. The lockout test claimed more migration than it exercised: the fixture carries two markers and only the routing writer had run. It now asserts the exact marker list at each step, which pins the real behaviour -- each writer refreshes only the marker it owns -- and covers the realtime override as well. Dropped one assertion that restated how the constant is defined rather than testing behaviour.
Swept every marker occurrence under tests/ and classified each one as routing output, prompt-layer output, or an input fixture. The three cases hosted CI failed on are already fixed; this closes the class that produced them rather than the three instances. Assertions on what the injector WRITES above a routing key now come from OCX_ROUTING_MARKER_LINE, and the two that checked a substring now assert the whole line. A substring check passes even when the wrong ownership line is written above a routing key, which is exactly the defect that would have to be caught here. The four prompt-layer files kept a private literal copy of the bare marker. They now derive it from OCX_SECTION_MARKER and say why: prompt layers keep the short marker because "ocx restore" is not their undo, so the two scopes cannot drift apart silently. Input fixtures are deliberately left as literals. A hand-written config or one from an older build is what those tests exist to exercise, and rewriting them to the current constant would delete the backward compatibility coverage instead of strengthening it.
702295a to
e3f490a
Compare
추가 리뷰 · 우선순위 56 / 80이전 리뷰(head 라인 - 라인 - 라인 - 라인 - status 출력 계약 테스트: tip의 lockout 테스트는 헬퍼·restore·마커 갱신을 잘 핀하지만, “PID 파일만 남은 collectStatus → 사람 출력에 advice가 나온다”는 경로는 여전히 없다. 게이트를 고치면 그 한 케이스가 회귀를 막는다. 메인테이너의 판단이 필요한 지점 status 게이트를 너의 추천 테스트 상수화는 방향이 맞고, 앞선 CI 실패 클래스를 잘 닫았다. 머지 전에는 status 게이트만 tip에 짧게 고치는 쪽을 다시 권한다 — 이전 추천과 같고, tip이 그 구멍을 안 건드렸기 때문이다. 고친 뒤 “pid 파일만 남은 상태면 advice가 출력된다” 한 케이스를 붙이면 된다. provider-table 수동 안내·structure 한 절은 가능하면 같이, 아니면 follow-up 이슈로 명시. 리눅스 CI가 이미 초록이니 macos pending만 기다리면 머지 후보다다. #5261은 OPEN 유지. 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
A Windows 11 user on 2.59.0 was locked out of Codex sign-in after setting up opencodex (#5261). Their screenshots show why: the default loopback integration does not add a provider, it points Codex's own built-in
openaiprovider at the proxy by writing the codex-rs root keyopenai_base_url = "http://127.0.0.1:10100/v1"into~/.codex/config.toml. That file is on disk and survives a reboot, so once the proxy process was gone Codex had one endpoint, it answered nothing, and there was no fallback. The screen said nothing about opencodex.Recovery already existed and already worked with the proxy down —
ocx restoreneeds no proxy, no management API and no network. Nothing told the user it existed. The injected config named no command,ocx statuson a dead proxy offered only ways to restart the proxy, and no troubleshooting page covered the state. The reporter's assistant read the config, found three unexplained lines, and hand-deleted them along with the catalog file — which is the one repair that makes things worse, because amodel_catalog_jsonpointing at a file that no longer exists stops Codex loading its config at all.This makes the failure detectable and the recovery discoverable. Excluding sign-in from the proxy path is not expressible:
openai_base_urlis a single key for a single built-in provider, and when the proxy is down no scoping of it helps.# Auto-injected by opencodex (undo: ocx restore). Every ownership predicate matches the marker as a substring rather than by equality, so configs written by earlier builds are still recognized, stripped and restored unchanged; an in-place rewrite refreshes the line, so an existing install gains the hint on its nextocx start. Scope is routing only — prompt layers keep the bare marker, becauseocx restoreis not what undoes those.ocx statusoffers the other half of the choice. With the proxy down over routing opencodex owns, the report now says sign-in fails too and names the command that does not require the proxy to come back. Routing opencodex does not own is excluded, sinceocx restorewould not remove someone else's local gateway.Before / after, same file, proxy stopped:
Not closing #5261
This removes the lockout's dead end, not every path into it. Left open deliberately, each being a separate change with its own risk and none of them what traps the user on its own:
ocx ensurewith output discarded and|| true, then launches Codex regardless (src/codex/shim-templates.ts:134), so a failed auto-start is silent. It is also CLI-only, so it never covered the reporter, who was in the Codex app.model_catalog_jsonafter injection. The inject-time chooser refuses a missing owned catalog (src/codex/inject/config-toml.ts:597), but a file removed afterwards leaves a pointer that stops Codex loading.src/service/windows-taskxml.ts:225), and applying the integration does not install a service at all, so injection present with nothing listening stays an ordinary post-reboot state.The account-pool failures that opened the report are a second cause, documented rather than changed: the pool is served by the management API and so needs a live proxy (
src/cli/runtime-api.ts:68), the browser flow needs the fixed callback port 1455 which cannot move (src/oauth/callback-server.ts:137), the Windows browser launch swallows its own failure (src/lib/open-url.ts:20), and the dashboard keeps last-good rows after a failed refresh (gui/src/hooks/useCodexAccountPool.ts:325), which is why a new account can be absent while older ones still show.Verification
tests/codex-integration/codex-signin-lockout.test.tsreconstructs the reported config — Windows catalog path, both root overrides, proxy gone — and runs recovery against it. Nothing in it starts a proxy or binds a port, because recovery has to work without one; a test that needed a live proxy would be exercising the wrong state. It holds that legacy and hinted markers are recognized identically and restore byte-identically, that removal clears the base URL, the realtime override and the catalog pointer together while keeping the user's own keys, that re-injection refreshes the marker in place without breaking idempotency, that a user-owned root override is still untouched, and that the advice appears only for routing we own.findCommandin the CLI registry, so renaming the command in one place fails the test instead of shipping a config file that points at nothing.scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json.tests/fixtures/file-size-baseline.json: no tracked file is over its cap, and none of the 20 files this PR touches is ratcheted..includes(OCX_SECTION_MARKER), which is what makes older configs keep working.tests/codex-integration/codex-inject.test.tsthat asserted an injection over routing we already own returns the file byte for byte apart from the URL. Refreshing our own ownership marker breaks that literal expectation while preserving the contract behind it, so those cases now expect the marker to refresh and assert everything else unchanged, including a malformed tail that must come back verbatim.tests/and classified each as routing output, prompt-layer output, or an input fixture, to close that class rather than its three instances. Routing-output assertions now derive fromOCX_ROUTING_MARKER_LINE, and two that checked a substring now assert the whole line, since a substring check passes even when the wrong ownership line is written. The four prompt-layer files that kept a private literal copy now derive it fromOCX_SECTION_MARKER. Input fixtures stay literal on purpose: a config from an older build is what those tests exist to exercise.ocxinvocation was used to establish any claim here. This incident is a configuration change that locked a user out, so verification was static source review plus exact-head hosted CI on this branch.Checklist
Security review
This touches the Codex auth/routing surface, so stating it explicitly: no credential, token or account identifier is read, written, logged or serialized by any line in this PR. The only values added to output are two fixed English sentences and one fixed TOML comment, none of which interpolate user data. No default changes: injection still writes the same base URL to the same key for the same users, and the added marker text is a TOML comment with no semantic effect. Recovery is not loosened —
ocx restorebehaviour is untouched, and marker ownership still requires our exact marker substring directly above the key, so a user-owned override remains unclaimable. The widened marker cannot cause a config to be misclassified as ours, since it is strictly longer than and contains the string every predicate already matched.Summary by CodeRabbit
New Features
ocx statusnow provides recovery guidance when Codex routing points to a stopped local proxy.ocx restoreas the undo command.Documentation
Bug Fixes