fix(client): refuse a hub catalog the local Codex CLI cannot parse - #4240
Conversation
A connected client reported connected, catalog present, token owned and rotation clean, while its installed Codex CLI exited before making a single request: failed to parse model_catalog_json ... unknown variant `max`, expected one of `none`, `minimal`, `low`, `medium`, `high`, `xhigh` The connection state answered a different question from the one the operator was asking. It proved the hub was reachable and the credential worked; nothing on that path proved the selected local runtime could consume what was written. validateRemoteCatalog checks JSON shape only, and connect wrote the hub's bytes verbatim, so the effort clamp that already exists for local catalog sync never saw the downloaded file. The client path now establishes compatibility before it materialises anything. catalogEffortCompatibility reports which reasoning levels the observed local ladder would reject, without mutating the catalog - the clamp beside it is mutate-and-continue, which is right when this process owns the file it is about to write and wrong for a catalog that must keep agreeing with hub truth. Both hub-download writes, connect and sync, are gated on it and fail closed: the download is refused before the write, so the previous known-good catalog is still there, no connection is committed, and no success is printed. The two restore paths stay ungated, since refusing to restore a catalog this machine already accepted would strand the client with none at all. The refusal names the level, what was preserved, and both ways out - upgrade the Codex CLI, or point CODEX_CLI_PATH at a build that supports it and run `ocx sync`. It never suggests editing the hub catalog and it does not touch running Codex processes, both of which the issue rules out. An unobservable local ladder is reported as unverified rather than incompatible, and does not block. A client machine may legitimately have no Codex CLI to observe, and refusing on absent evidence would break a working configuration; the issue asks to preserve the prior catalog when compatibility cannot be established, which is the incompatible case, not the inconclusive one. That call is mine and is recorded here. Closes #4207
|
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: Advanced 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 |
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. |
![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: 9627fe2d61
ℹ️ 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".
| // Fail closed BEFORE the write (#4207). The hub being reachable and the credential working | ||
| // does not mean the selected local Codex runtime can consume what arrived: an older CLI | ||
| // exits on an unknown reasoning level before making a single request, while connect | ||
| // reports success. Refusing here leaves the previous catalog in place untouched, rather |
There was a problem hiding this comment.
When selectedClients is ["claude"], this unconditional check can reject the hub catalog solely because an installed, older Codex CLI does not support a level such as max, even though Codex will not consume the catalog and the Claude path only reads context-window metadata from it. The same problem occurs during syncConnectedClient, where the check also runs regardless of initial.connection.selectedClients; guard both checks with the corresponding selectedClients.includes("codex") condition and add a Claude-only regression case.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
리뷰 · 우선순위 71 / 80이 PR은 지금 제보 장면은 이렇게 읽힙니다. 원격 클라이언트가 이 PR은 그 구멍을 닫습니다. 테스트
베이스 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
A remote client reported
connected, catalog present and freshly synced, token owned, rotation clean — and its installed Codex CLI still exited before making a single request:The connection state answered a different question from the one the operator was asking. It proved the hub was reachable and the credential worked; nothing on that path proved the selected local runtime could consume what was written.
validateRemoteCatalogchecks JSON shape only, and connect writes the hub's bytes verbatim, so the effort clamp that already exists for local catalog sync never saw the downloaded file. That is whyocx statuscould report an active effort clamp for the older runtime while the materialized client catalog still failed to parse — the two paths never met.The client path now establishes compatibility before it materializes anything, and fails closed, which is the decision recorded for this issue:
catalogEffortCompatibilityreports which reasoning levels the observed local ladder would reject, without mutating the catalog. The clamp beside it is mutate-and-continue, which is right when this process owns the file it is about to write and wrong for a catalog that has to keep agreeing with hub truth.connectClientandsyncConnectedClient— are gated on it. The download is refused before the write, so the previous known-good catalog is still on disk, no connection is committed, and no success is printed. Refusing before the write is stronger than writing and restoring: there is no window in which an unparseable catalog exists.The refusal names the level, what was preserved, and both ways out — upgrade the Codex CLI, or point
CODEX_CLI_PATHat a build that supports it and runocx sync. It never suggests editing the hub catalog and it does not touch running Codex processes; the issue rules both out.One decision the issue left open. An unobservable local ladder is reported as unverified rather than incompatible, and does not block. A client machine may legitimately have no Codex CLI to observe, and refusing on absent evidence would break a working configuration. I read "preserve the prior known-good catalog if compatibility cannot be established" as the incompatible case — the alternative to the compatible-projection branch offered in the same sentence — not as the inconclusive one. Flagging it because the other reading is defensible and would be a one-line change.
Verification
The local product suite, typecheck and GUI build were NOT RUN, by operator instruction for this dispatch round.
bun test,bun run test,bun run test:changed,bun run typecheck,bun run build:guiandbun installwere all NOT RUN. Hosted CI on this exact pushed head is the product evidence for this PR.What was checked instead, by read-only review agents and deterministic source scans:
atomicWriteFile(DEFAULT_CATALOG_PATH, …)sites were enumerated and classified: the two hub-download writes are gated and the two restores are not. No other code in the repository writes a hub download to that path.commitClientConnection, and throwing leaveswrittenCatalogFingerprintnull so the restore path is correctly skipped — the bytes are never written rather than written and rolled back.src/line was reviewed against stricttscrules by hand, since typecheck was not run: theRawEntrytype-only import, theunknownnarrowing onentry.slugandentry.default_reasoning_level, and the new optional dep field.src/clientand adds no module-load side effect;src/client/connect.tsalready reachedsrc/codex/catalog/sync.ts, which already importseffort.ts. The Lab boundary files are untouched.ocx codex-runtime, which does not exist; it was replaced withCODEX_CLI_PATH+ocx sync, both anchored in the CLI registry and indoctor.ts's existing advice.Checklist
This targets
codex/260911-l4-stop-refusal(PR #4237) as a stacked child and will be retargeted todevonce its parents land.#4204was removed from this round by the feasibility audit, so the effort clamp is not bound to the Desktop runtime here; this PR only adds a read-only compatibility view of the existing observed ladder.Closes #4207