diff --git a/src/cli/models-runtime.ts b/src/cli/models-runtime.ts index 0608c324c85..1be92b6c301 100644 --- a/src/cli/models-runtime.ts +++ b/src/cli/models-runtime.ts @@ -4,6 +4,7 @@ import { printData, rejectArgs, runCliAction, + RuntimeApiError, runtimeRequest, summaryLines, takeBooleanOption, @@ -165,6 +166,22 @@ async function priceRequest(write: boolean, argv: string[], deps: RuntimeApiDeps [auto ? `${selector}: automatic pricing restored.` : `${selector}: manual pricing saved.`]); } +/** + * True for the management handler's own unknown-id 404, and only that. + * + * Two different listeners answer 404 on this route. `src/server/management/model-routes.ts` + * means "no custom model with that id"; a listener that does not route the request at all + * reports `{error, method, path}` (src/client/machine-listener.ts), and runtime-api.ts already + * renders that shape as a routing statement. Narrowing on the absence of `method`/`path` keeps + * this rewrite from relabelling a not-served-here 404 as a missing record — the exact confusion + * #4662 was reported as. + */ +function unknownCustomModelId(body: unknown): boolean { + if (!body || typeof body !== "object") return false; + const record = body as Record; + return record.method === undefined && record.path === undefined; +} + async function edit(argv: string[], deps: RuntimeApiDeps): Promise { const args = [...argv]; const id = args.shift()?.trim(); @@ -204,10 +221,22 @@ async function edit(argv: string[], deps: RuntimeApiDeps): Promise { } if (defaultEffortRaw !== undefined) patch.defaultReasoningEffort = defaultEffortRaw === "-" ? null : defaultEffortRaw; if (Object.keys(patch).length === 0) throw new CliUsageError("at least one edit option is required", USAGE); - const result = await runtimeRequest(`/api/custom-models/${encodeURIComponent(id)}`, { - method: "PUT", - body: JSON.stringify(patch), - }, deps); + let result: unknown; + try { + result = await runtimeRequest(`/api/custom-models/${encodeURIComponent(id)}`, { + method: "PUT", + body: JSON.stringify(patch), + }, deps); + } catch (error) { + if (error instanceof RuntimeApiError && error.status === 404 && unknownCustomModelId(error.body)) { + throw new RuntimeApiError( + `No custom model has id ${id}. Edits address the custom-model id, not the provider/model slug; list the ids with: ocx models list-custom`, + 404, + error.body, + ); + } + throw error; + } printData(result, wantsJson, [`Updated custom model ${id}.`]); } diff --git a/src/cli/runtime-api.ts b/src/cli/runtime-api.ts index 3da3fb29cc5..290d92eefa9 100644 --- a/src/cli/runtime-api.ts +++ b/src/cli/runtime-api.ts @@ -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/"}`, 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", + ].join("\n"); +} + export async function runtimeBaseUrl(deps: RuntimeApiDeps = {}): Promise { 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); + } return `http://${probeHostname(live.hostname)}:${live.port}`; } @@ -62,11 +91,26 @@ function stringField(record: Record, 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; + 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); diff --git a/src/server/proxy-liveness.ts b/src/server/proxy-liveness.ts index 045e4cddf7e..d9d8693c411 100644 --- a/src/server/proxy-liveness.ts +++ b/src/server/proxy-liveness.ts @@ -21,6 +21,12 @@ export interface HealthzIdentity { uptime?: unknown; pid?: unknown; port?: unknown; + /** + * Which listener answered: the standalone/hub server omits it, and the connected-client + * machine listener reports `"client"` (src/client/machine-listener.ts). It is the only + * on-the-wire way to tell those two apart, because both bind `config.port ?? 10100`. + */ + role?: unknown; restartCapability?: unknown; providerReloadCapability?: unknown; guiPairCapability?: unknown; @@ -78,6 +84,16 @@ export interface LiveProxy { * healthz body predates the field. */ version?: string; + /** + * Role the live listener reported on `/healthz`, when it reported one: `"client"` for the + * connected-client machine listener, absent for a standalone or hub proxy. + * + * Liveness deliberately still ACCEPTS a client-role listener — see `isOpencodexHealthz`. + * The role is carried so a caller that needs a management plane (src/cli/runtime-api.ts) + * can refuse one, while `stop` and orphan cleanup keep finding the process they must act + * on. Absent for a proxy whose healthz body predates the field. + */ + role?: string; } /** @@ -101,6 +117,13 @@ export function probeHostname(hostname: string | undefined): string { * `service: "opencodex"` marker, plus the legacy `{status, version, uptime}` trio so a * still-running pre-identity proxy (e.g. right after `ocx update`) is not mistaken for a * foreign server and shadow-started over. + * + * The connected-client machine listener answers with the same `service: "opencodex"` marker + * and an extra `role: "client"`, and it is accepted here on purpose. Liveness answers "is one + * of our processes listening on this port", which is exactly what `ocx stop`, orphan cleanup, + * and duplicate-start avoidance need: rejecting the client role would make those paths blind to + * a real opencodex process and let them shadow-start over it. Callers that additionally need a + * management plane discriminate on `LiveProxy.role` instead of narrowing this predicate. */ export function isOpencodexHealthz(body: HealthzIdentity | null): boolean { if (!body) return false; @@ -114,7 +137,7 @@ export async function proxyIdentityAt( port: number, opts: { hostname?: string; expectedPid?: number } = {}, io: LivenessIo = {}, -): Promise<{ pid: number | null; version?: string } | null> { +): Promise<{ pid: number | null; version?: string; role?: string } | null> { const fetchFn = io.fetchFn ?? directLocalHttpFetch; const sleepFn = io.sleepFn ?? ((ms: number) => new Promise(r => setTimeout(r, ms))); const nowFn = io.nowFn ?? Date.now; @@ -139,7 +162,14 @@ export async function proxyIdentityAt( if (opts.expectedPid !== undefined && pid !== null && pid !== opts.expectedPid) return null; // Guarded the same way `pid` is: a non-string version is absent, not coerced. const version = typeof body?.version === "string" ? body.version : undefined; - return version === undefined ? { pid } : { pid, version }; + // Same guard for the role, for the same reason: absent on a standalone/hub proxy and on + // a legacy body, and never coerced from a non-string. + const role = typeof body?.role === "string" ? body.role : undefined; + return { + pid, + ...(version === undefined ? {} : { version }), + ...(role === undefined ? {} : { role }), + }; } catch { // Transport failure (timeout / refused) — retry while budget remains; a proxy that // has only just begun listening can miss a single short probe (#764). @@ -203,6 +233,7 @@ export async function findLiveProxy(io: LivenessIo = {}): Promise []`, r when the value is omitted. Ordering invariants live in [`openai-tiers.md`](providers/openai-tiers.md). +## The client role owns no management plane + +A connected client machine runs `src/client/machine-listener.ts` instead of the standalone server. +It binds the address the standalone proxy would (`port ?? config.port ?? 10100`) and serves +`GET /healthz`, `/readyz`, the packaged GUI/SPA routes, and `/api/machine/*`. Every other `/api/*` +and `/v1/*` path is refused before dispatch with a JSON 404 naming the method and path. There is no +second management port on such a machine: management rides the same listener a standalone or hub +install runs, so a connected client has no `/api/*` management surface at all. + +The discriminator is `role` on `/healthz` and `/readyz`. The machine listener reports +`role: "client"`; the standalone and hub server omit the field. `src/server/proxy-liveness.ts` parses +it into `HealthzIdentity.role` and carries it on `LiveProxy.role`. `isOpencodexHealthz` still accepts +a client-role body: liveness answers "is one of our processes listening here", which is what `ocx stop`, +orphan cleanup, and duplicate-start avoidance need, and narrowing it would make them blind to a real +opencodex process and let them shadow-start over it. Refusing the client role belongs to the caller +that needs a management plane, which is the [CLI management client](config.md#management-backed-cli-commands-need-a-management-plane). + ## Sidebar stop button The dashboard sidebar includes a stop button that calls `POST /api/stop`. The button shows a diff --git a/tests/cli/cli-transport-honesty.test.ts b/tests/cli/cli-transport-honesty.test.ts index a253381f4dc..52aa39f0184 100644 --- a/tests/cli/cli-transport-honesty.test.ts +++ b/tests/cli/cli-transport-honesty.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import { readFileSync } from "node:fs"; import { join } from "node:path"; import { RuntimeApiError, runtimeRequest } from "../../src/cli/runtime-api"; +import { handleModelsRuntimeCommand } from "../../src/cli/models-runtime"; import { apiError, apiJson, proxyUnreachable } from "../../src/cli/account-api"; import { assertNotAdminToken, assertServiceAuthEnvironment } from "../../src/service"; import { dataPlaneCredentialCollisionCheck } from "../../src/cli/doctor"; @@ -297,3 +298,135 @@ describe("#2696 a management token is refused as the data-plane secret", () => { expect(source).toMatch(/networkDown\) return proxyUnreachable\(result\.transportError\)/); }); }); + +/** + * #4662: on a connected client the same CLI lied in a third way. + * + * The machine listener (src/client/machine-listener.ts) binds `config.port ?? 10100`, the same + * address the standalone proxy would, and identifies as opencodex on /healthz — so liveness + * resolves a base URL for it. It serves only /api/machine/*, so every other management request + * came back as `{"error":"not_found","method":"PUT","path":"/api/custom-models/"}`, which + * the CLI printed as the bare token `not_found`. The real handler's unknown-id 404 says + * `not found` — one space apart — so the reporter read a structural "this listener has no + * management API" as "your model id is wrong", and exit code 4 agreed with them. + * + * The role was already on the wire. These tests pin that it is now read: refused up front for + * management, with 503 (exit 1) rather than 404 (exit 4), and that a listener which does not + * route a request says so. + */ +describe("#4662 a client-role listener is refused instead of misreported", () => { + const CLIENT_PROXY = { pid: 4242, port: 10100, source: "config" as const, version: "2.6.17", role: "client" }; + const STANDALONE_PROXY = { pid: 4242, port: 10100, source: "config" as const, version: "2.6.17" }; + + async function refusalFor(live: typeof CLIENT_PROXY): Promise<{ error: RuntimeApiError; sent: number }> { + let sent = 0; + try { + await runtimeRequest("/api/custom-models/2f6f", { method: "PUT" }, { + findLiveProxy: async () => live, + fetchImpl: async () => { sent += 1; return Response.json({ ok: true }); }, + }); + } catch (error) { + if (error instanceof RuntimeApiError) return { error, sent }; + throw error; + } + throw new Error("expected a RuntimeApiError"); + } + + async function captureStderr(run: () => Promise): Promise<{ code: number | null; text: string }> { + const original = console.error; + const lines: string[] = []; + console.error = (...args: unknown[]) => { lines.push(args.map(value => String(value)).join(" ")); }; + try { + return { code: await run(), text: lines.join("\n") }; + } finally { + console.error = original; + } + } + + test("the request is refused before it is sent, and says why and what to do", async () => { + const { error, sent } = await refusalFor(CLIENT_PROXY); + expect(sent).toBe(0); + // 503 (management plane unavailable), never 404: runCliAction maps 404 to exit 4, which + // tells a script "no such custom model" about a machine that has no management API at all. + expect(error.status).toBe(503); + expect(error.message).toContain("port 10100"); + expect(error.message).toContain("client role"); + expect(error.message).toContain("/api/machine/*"); + expect(error.message).toContain("hub"); + expect(error.message).toContain("ocx sync"); + }); + + test("a listener that reports no role is still used: only the client role is refused", async () => { + let sent = 0; + const body = await runtimeRequest("/api/custom-models/2f6f", { method: "PUT" }, { + findLiveProxy: async () => STANDALONE_PROXY, + fetchImpl: async () => { sent += 1; return Response.json({ ok: true }); }, + }); + expect(sent).toBe(1); + expect(body).toEqual({ ok: true }); + }); + + test("a 404 that names a method and path reads as a routing refusal, not a missing record", async () => { + try { + await runtimeRequest("/api/custom-models/2f6f", { method: "PUT" }, { + baseUrl: "http://127.0.0.1:10100", + fetchImpl: async () => Response.json( + { error: "not_found", method: "PUT", path: "/api/custom-models/2f6f" }, + { status: 404 }, + ), + }); + } catch (error) { + const message = (error as RuntimeApiError).message; + expect(message).toContain("PUT /api/custom-models/2f6f"); + expect(message).toContain("does not serve"); + expect(message).not.toBe("not_found"); + return; + } + throw new Error("expected a RuntimeApiError"); + }); + + test("ocx models edit on a connected client exits 1 with the refusal, not 4", async () => { + const { code, text } = await captureStderr(() => handleModelsRuntimeCommand( + "edit", + ["2f6f", "--display-name", "Renamed"], + { + findLiveProxy: async () => CLIENT_PROXY, + fetchImpl: async () => { throw new Error("the refusal must happen before any request"); }, + }, + )); + expect(code).toBe(1); + expect(text).toContain("client role"); + }); + + test("the real handler's unknown-id 404 still names the id and where to find the right one", async () => { + const { code, text } = await captureStderr(() => handleModelsRuntimeCommand( + "edit", + ["2f6f", "--display-name", "Renamed"], + { + baseUrl: "http://127.0.0.1:10100", + fetchImpl: async () => Response.json({ error: "not found" }, { status: 404 }), + }, + )); + // Still exit 4: this one really is "no such custom model". + expect(code).toBe(4); + expect(text).toContain("2f6f"); + expect(text).toContain("ocx models list-custom"); + }); + + test("a not-served-here 404 is not relabelled as a missing custom model", async () => { + const { code, text } = await captureStderr(() => handleModelsRuntimeCommand( + "edit", + ["2f6f", "--display-name", "Renamed"], + { + baseUrl: "http://127.0.0.1:10100", + fetchImpl: async () => Response.json( + { error: "not_found", method: "PUT", path: "/api/custom-models/2f6f" }, + { status: 404 }, + ), + }, + )); + expect(code).toBe(4); + expect(text).toContain("does not serve"); + expect(text).not.toContain("ocx models list-custom"); + }); +}); diff --git a/tests/server/proxy-liveness.test.ts b/tests/server/proxy-liveness.test.ts index ec24c75eaf9..327725fd3d0 100644 --- a/tests/server/proxy-liveness.test.ts +++ b/tests/server/proxy-liveness.test.ts @@ -888,3 +888,61 @@ describe("probeReadiness adversarial contract (never counts ready)", () => { expect(probe).toBeNull(); }); }); + +/** + * #4662: the connected-client machine listener binds `config.port ?? 10100` — the same address + * the standalone proxy would — and answers /healthz as opencodex with an extra `role: "client"`. + * That role was parsed away here, so every management-backed `ocx` subcommand on a connected + * client resolved a base URL pointing at a listener that serves only /api/machine/*, and died on + * its opaque `{"error":"not_found","method":…,"path":…}` 404. + * + * Carrying the role is the whole liveness-side fix. Liveness itself must keep ACCEPTING the + * client role: `ocx stop` and orphan cleanup act on whichever of our processes holds the port, + * and a predicate that rejected the client would make them blind to a real one. + */ +describe("client-role discrimination (#4662)", () => { + const CLIENT = { service: "opencodex", version: "2.6.17", role: "client", uptime: 3, pid: 4242, port: 10100 }; + + test("a client-role body is still live: stop and orphan cleanup must find that process", () => { + expect(isOpencodexHealthz(CLIENT)).toBe(true); + }); + + test("proxyIdentityAt carries the reported role", async () => { + const identity = await proxyIdentityAt(10100, {}, { fetchFn: (async () => healthz(CLIENT)) as typeof fetch }); + expect(identity).toEqual({ pid: 4242, version: "2.6.17", role: "client" }); + }); + + test("a standalone body has no role at all, rather than a coerced one", async () => { + const identity = await proxyIdentityAt(10100, {}, { fetchFn: (async () => healthz(OURS)) as typeof fetch }); + expect(identity).toEqual({ pid: 4242, version: "2.6.17" }); + expect(identity && "role" in identity).toBe(false); + }); + + test("a non-string role is absent, not coerced (same guard as pid and version)", async () => { + const identity = await proxyIdentityAt(10100, {}, { + fetchFn: (async () => healthz({ ...OURS, role: 7 })) as typeof fetch, + }); + expect(identity).toEqual({ pid: 4242, version: "2.6.17" }); + }); + + test("findLiveProxy reports the role on the runtime-record path", async () => { + const live = await findLiveProxy({ + readPidFn: () => 4242, + readRuntimeFn: pid => (pid === 4242 ? { port: 10100 } : null), + configFn: () => ({ port: 10100 }), + fetchFn: (async () => healthz(CLIENT)) as typeof fetch, + }); + expect(live).toEqual({ pid: 4242, port: 10100, source: "runtime", version: "2.6.17", role: "client" }); + }); + + test("findLiveProxy reports the role on the configured-port path", async () => { + const live = await findLiveProxy({ + readPidFn: () => null, + readRuntimeFn: () => null, + configFn: () => ({ port: 10100 }), + verifyPidFn: candidate => candidate, + fetchFn: (async () => healthz(CLIENT)) as typeof fetch, + }); + expect(live).toEqual({ pid: 4242, port: 10100, hostname: undefined, source: "config", version: "2.6.17", role: "client" }); + }); +});