Repository navigation
fix(cli): tell a client-role listener apart from a missing management route #4959
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,10 +42,39 @@ export class RuntimeApiError extends Error { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Refusal for a management request that resolved to a connected-client machine listener. | ||
| * | ||
| * A connected client runs `src/client/machine-listener.ts`, which binds the SAME address the | ||
| * standalone proxy would (`port ?? config.port ?? 10100`) and answers `/healthz` as opencodex. | ||
| * Liveness therefore finds it — correctly, it is our process — but it serves only | ||
| * `/api/machine/*` and returns a JSON 404 for every other `/api/*` path. Before this refusal | ||
| * every management-backed subcommand on such a machine died on that opaque 404: | ||
| * `{"error":"not_found","method":"PUT","path":"/api/custom-models/<id>"}`, one character away | ||
| * from the real handler's unknown-id `{"error":"not found"}` and indistinguishable from it | ||
| * (#4662). The role was already on the wire; only the parser was throwing it away. | ||
| * | ||
| * 503 rather than 404: the management plane is unavailable here, the resource is not missing, | ||
| * and `runCliAction` maps 404 to exit 4 ("no such thing") — the wrong answer to give a script. | ||
| */ | ||
| function clientRoleManagementRefusal(port: number): string { | ||
| return [ | ||
| `The opencodex listener on port ${port} is running in the client role. It serves only the machine routes (/api/machine/*), so this machine has no management API to call.`, | ||
| "Custom-model edits and other management changes are made on the hub this machine is connected to: run the command there, or use the hub dashboard (the dashboard on this machine relays to it when the connection uses the relay transport).", | ||
| "To change this machine's own configuration instead: edit customModels in config.json, then run: ocx sync", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On a connected client, editing local Useful? React with 👍 / 👎. |
||
| ].join("\n"); | ||
| } | ||
|
|
||
| export async function runtimeBaseUrl(deps: RuntimeApiDeps = {}): Promise<string> { | ||
| if (deps.baseUrl) return deps.baseUrl.replace(/\/$/, ""); | ||
| const live = await (deps.findLiveProxy ?? findLiveProxy)(); | ||
| if (!live) throw new RuntimeApiError("Proxy is not running. Start it with: ocx start", 503, null); | ||
| // The role comes from the same identity-checked /healthz body liveness already parsed, so | ||
| // this costs no extra request. Only the client role is refused: an absent role is a | ||
| // standalone or hub proxy (or a legacy body that predates the field), and both serve /api/*. | ||
| if (live.role === "client") { | ||
| throw new RuntimeApiError(clientRoleManagementRefusal(live.port), 503, null); | ||
|
Comment on lines
+75
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On a connected client, the entire Useful? React with 👍 / 👎. |
||
| } | ||
| return `http://${probeHostname(live.hostname)}:${live.port}`; | ||
| } | ||
|
|
||
|
|
@@ -62,11 +91,26 @@ function stringField(record: Record<string, unknown>, key: string): string | und | |
| * is unavailable). Both were dropped here, so a fenced management plane was | ||
| * indistinguishable from a generic failure and an operator had no way to tell a port | ||
| * collision from an ACL refusal from a stopped proxy (#2698). | ||
| * | ||
| * A 404 body that carries both `method` and `path` is a different statement again: some | ||
| * opencodex listener answered, and it does not route that request at all. Only the | ||
| * connected-client machine listener emits that shape today (`json404` in | ||
| * src/client/machine-listener.ts), and printing its bare `not_found` token read as though the | ||
| * resource were missing — a real handler's unknown-id 404 says `not found`, one space apart | ||
| * (#4662). Name the route instead, so any listener that does not serve a path stays legible | ||
| * even if another one starts answering this way. | ||
| */ | ||
| function responseMessage(body: unknown, status: number): string { | ||
| if (typeof body === "string" && body.trim()) return body.trim().slice(0, 400); | ||
| if (!body || typeof body !== "object") return `Management request failed (${status})`; | ||
| const record = body as Record<string, unknown>; | ||
| if (status === 404) { | ||
| const method = stringField(record, "method"); | ||
| const path = stringField(record, "path"); | ||
| if (method && path) { | ||
| return `This opencodex listener does not serve ${method.slice(0, 16)} ${path.slice(0, 200)}, so the request was refused before any handler ran (it is not a missing record). Check that the command is pointed at a proxy that serves the management API.`; | ||
| } | ||
| } | ||
| let primary: string | undefined; | ||
| for (const key of ["error", "message", "detail"]) { | ||
| primary = stringField(record, key); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -370,6 +370,24 @@ Usage consumers preserve positive incomplete-history metadata as specified in [u | |
| `dropCodexSafetyBuffering` is an optional boolean, default false. Invalid API candidates reject; | ||
| malformed persisted values stay disabled. It controls only the allowlisted client-output hints | ||
| described in [Responses transport](transports/responses.md), not upstream policy or model selection. | ||
|
|
||
| ## Management-backed CLI commands need a management plane | ||
|
|
||
| `src/cli/runtime-api.ts` is the single client every headless management subcommand calls through, so | ||
| it owns the refusal as well as the request. `runtimeBaseUrl` resolves the live listener through | ||
| `findLiveProxy`, and when that listener reports the client role — see | ||
| [the client role owns no management plane](gui-and-management-api.md#the-client-role-owns-no-management-plane) | ||
| — it refuses with `RuntimeApiError` status 503 instead of dialing it. The message names the port, | ||
|
Comment on lines
+374
to
+380
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new user-visible refusal and its recovery workflow are documented only in AGENTS.md reference: structure/AGENTS.md:L9-L13 Useful? React with 👍 / 👎. |
||
| states that the listener serves only the machine routes, points custom-model and other management | ||
| edits at the hub the machine is connected to, and gives the on-machine alternative: edit | ||
| `customModels` in `config.json`, then run `ocx sync`. The status is also the honest exit code, since | ||
| `runCliAction` maps 404 to exit 4 and would otherwise report a missing record. | ||
|
|
||
| A 404 body carrying both `method` and `path` is rendered as the route that listener does not serve, | ||
| so any not-served-here answer stays legible rather than printing a bare token. `ocx models edit` in | ||
| `src/cli/models-runtime.ts` narrows the opposite case: only a 404 without those keys is the | ||
| management handler's own unknown-id answer, and it names the id and `ocx models list-custom`. | ||
|
|
||
| ## Paginated history writer boundary | ||
|
|
||
| `src/codex/history-provider.ts` refuses external writes to paginated or migration-capable history. `src/codex/inject.ts` checks affected rows and manifest-owned restore targets before and after config/profile/journal changes, including successful journal and fallback restores, and compensates detected migration. Failed config restore stops later catalog/history work and rolls back a coordinated remove transition. See the [history writer contract](codex-home.md#paginated-history-writer-boundary) for guarantees and concurrent-writer limits. | ||
|
|
||
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 21844
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 20834
🏁 Script executed:
sed -n '17,70p' src/bridge/errors.tsRepository: lidge-jun/opencodex
Length of output: 2112
Match the management handler’s affirmative 404 discriminator.
unknownCustomModelIdtreats any object 404 withoutmethodandpathas a missing custom-model ID. The unauthenticated loopback listener rejects/api/custom-models/<id>before dispatch and callsformatErrorResponse(404, "not_found", ...); that formatter returns a JSON body with a structurederrorobject and nomethodorpath. This response does not mean that the custom-model ID is absent.The management handler returns the distinct top-level
{ error: "not found" }only when its ID lookup fails. Match that exact value:📝 Committable suggestion
🤖 Prompt for AI Agents