-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(client): refuse a hub catalog the local Codex CLI cannot parse #4240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
66 changes: 66 additions & 0 deletions
66
devlog/_plan/260911_l4_service_cli/030_wp3_client_catalog.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| # wp3 — #4207: connected catalog reports success while the local Codex CLI rejects it | ||
|
|
||
| Work-phase 3 of the L4 lane, stacked on wp2. No carried PR: this issue had none. | ||
|
|
||
| ## The gap | ||
|
|
||
| Subagent Bernoulli mapped the client path. `connectClient` downloads at `connect.ts:542` and | ||
| writes the hub's bytes verbatim at `:545-549`; `syncConnectedClient` writes the same way at | ||
| `:667`. The only validation in between is `validateRemoteCatalog` | ||
| (`hub-client.ts:145`), which checks JSON shape — object, `models` array, unique non-empty | ||
| slugs — and nothing about reasoning levels. `src/client` never imports the effort clamp. | ||
|
|
||
| So the connection state proves the hub is reachable and the credential works, and is then | ||
| reported as readiness. The reporter's Codex CLI 0.135.0 exited before its first request on | ||
| `unknown variant \`max\``, while `ocx connect status --json` said `connected` with the catalog | ||
| present. `ocx status` even reported an active effort clamp for that same older runtime: the | ||
| local machinery already knew the ladder, and the client path simply never consulted it. | ||
|
|
||
| ## Decision | ||
|
|
||
| The packet records it: **fail closed — block local readiness rather than reporting success.** | ||
| Not a locally clamped projection, which would make the client silently disagree with hub truth. | ||
|
|
||
| ## Shape | ||
|
|
||
| - `catalogEffortCompatibility(models, supported)` in `src/codex/catalog/effort.ts` — pure, no | ||
| mutation, reports the rejected efforts and the models carrying them. It sits beside | ||
| `clampCatalogModelsToObservedCodexSupport`, which mutates; that is correct for a file this | ||
| process owns and wrong for one that must keep matching the hub. | ||
| - `src/client/catalog-compatibility.ts` — assesses a downloaded body against the observed local | ||
| ladder and throws `ClientCatalogIncompatibleError` when it cannot be consumed. | ||
| - Both hub-download writes are gated **before** the write. Refusing before the write is stronger | ||
| than writing and restoring: there is no window in which an unparseable catalog exists on disk, | ||
| and `writtenCatalogFingerprint` stays null so the existing rollback correctly does nothing. | ||
| - The two restore paths (`connect.ts:126`, `:729`) are deliberately **not** gated. Refusing to | ||
| restore a catalog this machine already accepted would strand the client with none at all. | ||
|
|
||
| ## Decisions I had to make | ||
|
|
||
| **An unobservable ladder does not block.** `codexSupportedReasoningEfforts` returns null when | ||
| `codex debug models --bundled` cannot be observed. That is not evidence of incompatibility, and a | ||
| client machine may legitimately have no Codex CLI. I read the issue's *"preserve the prior | ||
| known-good catalog if compatibility cannot be established"* as the incompatible branch — the | ||
| alternative to the compatible-projection branch offered in the same sentence — not as the | ||
| inconclusive one. Recorded in the PR body too, because the other reading is defensible. | ||
|
|
||
| **An invented command was caught before it shipped.** The first draft of the refusal recommended | ||
| `ocx codex-runtime`, which does not exist. `AGENTS.md` records this exact failure mode — a | ||
| documented `ocx request-history` that never existed — so every command in the message was | ||
| checked against the CLI registry. It now names `CODEX_CLI_PATH` and `ocx sync`, with `ocx doctor` | ||
| for diagnosis, matching `doctor.ts`'s existing advice. | ||
|
|
||
| ## Audit | ||
|
|
||
| Bohr reviewed the diff adversarially and returned `SAFE_TO_PUSH`: no strict-tsc failure on the | ||
| new `src/` lines (checked by hand, since typecheck was NOT RUN), no import cycle into | ||
| `src/client` and no module-load side effect, Lab boundary untouched, all four | ||
| `atomicWriteFile(DEFAULT_CATALOG_PATH, …)` sites classified, and the gate proven to precede | ||
| `commitClientConnection`. One nit folded: a test title claimed write ordering that only the | ||
| source-scan test actually asserts, and was renamed. | ||
|
|
||
| ## Not run | ||
|
|
||
| `bun test`, `bun run test`, `bun run test:changed`, `bun run typecheck`, `bun run build:gui` | ||
| and `bun install` are NOT RUN by operator instruction. Hosted CI on the exact pushed head is | ||
| the only product evidence this round accepts. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| /** | ||
| * #4207: a connected client reported `connected` with a present, freshly synced catalog while | ||
| * its installed Codex CLI exited before making a request, because the hub's catalog contained a | ||
| * reasoning level that CLI does not know: | ||
| * | ||
| * 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; it never proved the selected local | ||
| * runtime could consume what was downloaded. This module supplies the missing half, and the | ||
| * connect path fails closed on it: an incompatible catalog is refused before it is written, so | ||
| * the previous known-good file survives and no success is reported. | ||
| * | ||
| * What it deliberately does not do: rewrite the hub's catalog into a locally compatible | ||
| * projection (the client would then silently disagree with hub truth) and terminate running | ||
| * Codex processes. Both are ruled out by the issue. | ||
| */ | ||
| import { catalogEffortCompatibility, codexSupportedReasoningEfforts } from "../codex/catalog/effort"; | ||
| import type { RawEntry } from "../codex/catalog/parsing"; | ||
|
|
||
| export type ClientCatalogCompatibility = | ||
| | { kind: "compatible" } | ||
| /** The runtime ladder could not be observed, so incompatibility cannot be established. */ | ||
| | { kind: "unverified"; reason: string } | ||
| | { | ||
| kind: "incompatible"; | ||
| unsupportedEfforts: readonly string[]; | ||
| affectedModels: readonly string[]; | ||
| }; | ||
|
|
||
| export interface CatalogCompatibilityDeps { | ||
| /** Injected in tests; defaults to observing the selected local Codex runtime. */ | ||
| supportedEfforts?: () => ReadonlySet<string> | null; | ||
| } | ||
|
|
||
| function parseModels(body: string): RawEntry[] | null { | ||
| try { | ||
| const parsed = JSON.parse(body) as { models?: unknown }; | ||
| if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) return null; | ||
| return Array.isArray(parsed.models) ? parsed.models as RawEntry[] : []; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Assess a downloaded catalog against the reasoning efforts the selected local Codex runtime | ||
| * accepts. Unreadable bytes are reported as unverified rather than incompatible: the hub | ||
| * client already rejects a malformed body, and inventing a second cause for it here would | ||
| * repeat the mistake #4169 was filed for. | ||
| */ | ||
| export function assessClientCatalogCompatibility( | ||
| body: string, | ||
| deps: CatalogCompatibilityDeps = {}, | ||
| ): ClientCatalogCompatibility { | ||
| const models = parseModels(body); | ||
| if (!models) return { kind: "unverified", reason: "the downloaded catalog could not be read" }; | ||
| const supported = (deps.supportedEfforts ?? (() => codexSupportedReasoningEfforts()))(); | ||
| if (!supported) { | ||
| return { | ||
| kind: "unverified", | ||
| reason: "the selected local Codex runtime did not report the reasoning levels it supports", | ||
| }; | ||
| } | ||
| const result = catalogEffortCompatibility(models, supported); | ||
| if (result.compatible) return { kind: "compatible" }; | ||
| return { | ||
| kind: "incompatible", | ||
| unsupportedEfforts: result.unsupportedEfforts, | ||
| affectedModels: result.affectedModels, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Raised instead of writing an incompatible catalog. It names both remedies the issue asks | ||
| * for, because the operator cannot act on "incompatible" alone, and it never suggests editing | ||
| * the hub. | ||
| */ | ||
| export class ClientCatalogIncompatibleError extends Error { | ||
| readonly unsupportedEfforts: readonly string[]; | ||
| readonly affectedModels: readonly string[]; | ||
|
|
||
| constructor(unsupportedEfforts: readonly string[], affectedModels: readonly string[]) { | ||
| const efforts = unsupportedEfforts.join(", "); | ||
| const models = affectedModels.length > 3 | ||
| ? `${affectedModels.slice(0, 3).join(", ")} and ${affectedModels.length - 3} more` | ||
| : affectedModels.join(", "); | ||
| super( | ||
| `catalog_incompatible: the hub catalog uses reasoning ${unsupportedEfforts.length === 1 ? "level" : "levels"} ` | ||
| + `${efforts}, which the selected local Codex CLI rejects${models ? ` (${models})` : ""}. ` | ||
| + "The previous catalog was kept and nothing was changed. Upgrade the Codex CLI to a " | ||
| + "version that supports those levels, or point CODEX_CLI_PATH at one that does and run " | ||
| + "`ocx sync`, then retry. `ocx doctor` reports which runtime is selected.", | ||
| ); | ||
| this.name = "ClientCatalogIncompatibleError"; | ||
| this.unsupportedEfforts = unsupportedEfforts; | ||
| this.affectedModels = affectedModels; | ||
| } | ||
| } | ||
|
|
||
| /** Fail closed: refuse an incompatible catalog before anything is written. */ | ||
| export function assertClientCatalogCompatible(body: string, deps: CatalogCompatibilityDeps = {}): void { | ||
| const assessment = assessClientCatalogCompatibility(body, deps); | ||
| if (assessment.kind !== "incompatible") return; | ||
| throw new ClientCatalogIncompatibleError(assessment.unsupportedEfforts, assessment.affectedModels); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
selectedClientsis["claude"], this unconditional check can reject the hub catalog solely because an installed, older Codex CLI does not support a level such asmax, even though Codex will not consume the catalog and the Claude path only reads context-window metadata from it. The same problem occurs duringsyncConnectedClient, where the check also runs regardless ofinitial.connection.selectedClients; guard both checks with the correspondingselectedClients.includes("codex")condition and add a Claude-only regression case.AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.