diff --git a/docs-site/src/content/docs/guides/remote-link.md b/docs-site/src/content/docs/guides/remote-link.md index 383f73c8fa9..c1ef54621b6 100644 --- a/docs-site/src/content/docs/guides/remote-link.md +++ b/docs-site/src/content/docs/guides/remote-link.md @@ -195,7 +195,7 @@ The check covers the intent folder and record. It does not protect against repla On the Home, run `ocx link probe child-a --json` to inspect the offered SSH fingerprint, key type and OpenCodex version. Verify the fingerprint through a trusted source. Then run `ocx link add child-a --expect-fingerprint SHA256:… --json`. The command probes again and refuses a mismatch before storing trust or issuing a key. Without the fingerprint flag, it requires an interactive terminal and an explicit `yes` after showing the fingerprint. It never prints the link key. -Home-initiated connections now restart the Child after saving its connection, including capable older Child installations. The Child must already serve standalone OpenCodex on its configured port and expose bound restart support. POSIX Children need curl; Windows uses a bounded loopback HTTP request through .NET. Update or disconnect a Child that fails this preflight. Desktop and system OpenCodex installations take precedence over generic PATH shims; broken candidates are skipped. Windows x64 Children use encoded PowerShell commands without consuming key stdin. The preflight reads `ocx config show --json` under the runner's 4 MiB capture limit, so a Child with a large saved configuration still accepts a link; output past that limit answers `child_preflight_output_limit` and names the command to trim. +Home-initiated connections now restart the Child after saving its connection, including capable older Child installations. The Child must already serve standalone OpenCodex on its configured port and expose bound restart support. POSIX Children need curl; Windows uses a bounded loopback HTTP request through .NET. Update or disconnect a Child that fails this preflight. Desktop and system OpenCodex installations take precedence over generic PATH shims; broken candidates are skipped. Windows x64 Children use encoded PowerShell commands without consuming key stdin. The preflight reads `ocx config show --json` under the runner's 4 MiB capture limit, so a Child with a large saved configuration still accepts a link; output past that limit answers `child_preflight_output_limit` and names the command to trim. When a phase fails, `ocx link add` prints the phase together with the Child's own reason, for example an invalid saved configuration or the failing command's last error line. That reason is shortened and has OpenCodex keys and URL query strings removed. ### Ephemeral Child machines (`ocx-child`) diff --git a/src/cli/link-enrollment.ts b/src/cli/link-enrollment.ts index adee88bfbae..f459c68ba47 100644 --- a/src/cli/link-enrollment.ts +++ b/src/cli/link-enrollment.ts @@ -207,7 +207,10 @@ export async function runLinkEnrollment(command: "probe" | "add", args: string[] if (code === "child_preflight_output_limit") throw new Error("The Child's ocx config show --json output exceeded the Remote Link preflight limit; trim the Child's saved configuration and retry."); if (code === "child_external_provider_conflict") throw new Error("The Child's ~/.codex/config.toml is already managed by another model provider or custom base URL; no Child files were modified."); const phase = typeof code === "string" && Object.hasOwn(PHASES, code) ? PHASES[code] : undefined; - const message = phase ? "Remote Link failed during " + phase : error.message; + // The server's hint is the Child's own bounded, redacted reason (#8357); a phase name + // alone left every failure to SSH reproduction and source reading. + const hint = record(error.body.error) && typeof error.body.error.hint === "string" ? error.body.error.hint : ""; + const message = phase ? "Remote Link failed during " + phase + (hint ? ": " + hint : "") : error.message; const recovery = ephemeralFlow ? RECOVERY_EPHEMERAL : RECOVERY_FULL; if (phase || error.body.residual === true) throw new Error(message + (error.body.residual === true ? ". " + recovery : "")); } diff --git a/src/cli/link.ts b/src/cli/link.ts index c7591093281..159f845ac20 100644 --- a/src/cli/link.ts +++ b/src/cli/link.ts @@ -11,6 +11,7 @@ import type { LinkLocalPeer, LinkPeerStatus } from "../link/compatibility"; import { runLinkEnrollment } from "./link-enrollment"; import { runLinkCleanup, type LinkCleanupDeps } from "./link-cleanup"; import { assertSshAlias } from "../link/ssh-argv"; +import { boundHint } from "../link/ssh-runner"; import { linkStorePath } from "../link/paths"; import { isLinkPort } from "../link/ports"; import { readLinkStore, type LinkDirection, type LinkStore } from "../link/store"; @@ -246,17 +247,22 @@ async function linkRequest(path: string, init: RequestInit, deps: LinkCliDeps } catch (error) { if (error instanceof RuntimeApiError) { // Never echo an API error body: issue responses contain a one-time data key. Only the error - // code survives, because callers branch on it and a code is never secret. + // code, the server's already-bounded hint and the residual flag survive (errorCodeBody). throw new RuntimeApiError(`Link management request failed (${error.status})`, error.status, errorCodeBody(error.body)); } throw error; } } -function errorCodeBody(body: unknown): { error: { code: string }; residual?: true } | null { +function errorCodeBody(body: unknown): { error: { code: string; hint?: string }; residual?: true } | null { if (!isRecord(body) || !isRecord(body.error)) return null; const code = body.error.code; - return typeof code === "string" && /^[a-z_]{1,64}$/.test(code) ? { error: { code }, ...(body.residual === true ? { residual: true as const } : {}) } : null; + if (typeof code !== "string" || !/^[a-z_]{1,64}$/.test(code)) return null; + // The hint is the only other field kept (#8357): the server builds it from a Child's last stderr + // line, already bounded and redacted, and it is bounded and redacted again here. The rest of + // the body is still never echoed. + const hint = typeof body.error.hint === "string" ? boundHint(body.error.hint) : undefined; + return { error: { code, ...(hint ? { hint } : {}) }, ...(body.residual === true ? { residual: true as const } : {}) }; } async function runPort(args: string[], deps: LinkCliDeps): Promise { diff --git a/src/server/management/link-child-preflight.ts b/src/server/management/link-child-preflight.ts index 6bcedc8ce90..d3332f03dee 100644 --- a/src/server/management/link-child-preflight.ts +++ b/src/server/management/link-child-preflight.ts @@ -1,4 +1,4 @@ -import { SSH_MAX_OUTPUT_BYTES } from "../../link/ssh-runner"; +import { SSH_MAX_OUTPUT_BYTES, boundHint, sshFailureHint } from "../../link/ssh-runner"; /** * Home enrollment reads one Child command before it may restart that Child: `ocx config show @@ -19,3 +19,56 @@ export const CHILD_CONFIG_TOO_LARGE_MESSAGE = "The Child configuration is too la /** The operator-facing hint for a Child whose config output exceeded even the raised cap. */ export const CHILD_CONFIG_TOO_LARGE_HINT = `ocx config show --json on the Child exceeded ${CHILD_PREFLIGHT_MAX_OUTPUT_BYTES} bytes; trim the Child's saved configuration and retry`; + +/** + * Why the Child's effective configuration could not be read (#8357): a failed `config show` + * names its own last stderr line (bounded and redacted by `sshFailureHint`, e.g. an invalid + * saved configuration), and a successful one that printed no usable JSON says so. + */ +export function childConfigFailureHint(result: { code: number; stderr: string }): string { + if (result.code !== 0) return sshFailureHint(result.stderr) ?? `ocx config show --json exited ${result.code} on the Child`; + return "ocx config show --json on the Child did not print a configuration with a valid port"; +} + +/** `ocx config validate --json` prints one small object; this bounds a misbehaving Child. */ +export const CHILD_CONFIG_VALIDATE_MAX_OUTPUT_BYTES = 16 * 1024; + +/** + * The validation reason a full `ocx` Child gives for its own saved configuration, or undefined. + * + * `config show` on an invalid configuration prints only a generic warning whose path can push + * the pointer past the 160-code-point hint cap, so it never names the defect (#8357). `config + * validate --json` answers `{ "ok": false, "error": "schema_invalid: : " }`, naming + * fields and expected types, never configured values. Only that `error` string is read from + * stdout, and `boundHint` still redacts and caps it. A Child without the command (`ocx-child`, + * an older `ocx`) prints nothing usable and the caller keeps the `config show` reason. + */ +export function childConfigValidationHint(result: { code: number; stdout: string }): string | undefined { + if (result.code === 0) return undefined; + let parsed: unknown; + try { parsed = JSON.parse(result.stdout); } catch { return undefined; } + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) return undefined; + const { ok, error } = parsed as { ok?: unknown; error?: unknown }; + if (ok !== false || typeof error !== "string") return undefined; + return boundHint(`Child config is invalid: ${error}`); +} + +/** + * The hint for an unreadable Child configuration: the Child's own validation reason when `config + * show` failed and `config validate` can name it, otherwise `childConfigFailureHint`. The + * extra command runs only on this failure path and is read-only on the Child. + */ +export async function childConfigFailureReason( + configured: { code: number; stderr: string }, + validate: () => Promise<{ code: number; stdout: string }>, +): Promise { + if (configured.code !== 0) { + try { + const reason = childConfigValidationHint(await validate()); + if (reason) return reason; + } catch { + // A timeout or transport failure here must not mask the config show reason. + } + } + return childConfigFailureHint(configured); +} diff --git a/src/server/management/link-enrollment.ts b/src/server/management/link-enrollment.ts index c157e964fa9..5afa7eabb96 100644 --- a/src/server/management/link-enrollment.ts +++ b/src/server/management/link-enrollment.ts @@ -14,7 +14,7 @@ import { withConfigMutationLockSync } from "../../config/mutation-lock"; import type { ManagementContext } from "./context"; import { readManagementJsonBodyOr } from "./body"; import { fail, exactBody, port, isRecord, runnerFor, readStoreFor, writeStoreFor, knownHostsFile, putKnownHost, issueKey, compensateNewLink, type LinkRouteState } from "./link-route-state"; -import { CHILD_CONFIG_TOO_LARGE_HINT, CHILD_CONFIG_TOO_LARGE_MESSAGE, CHILD_PREFLIGHT_MAX_OUTPUT_BYTES } from "./link-child-preflight"; +import { CHILD_CONFIG_TOO_LARGE_HINT, CHILD_CONFIG_TOO_LARGE_MESSAGE, CHILD_CONFIG_VALIDATE_MAX_OUTPUT_BYTES, CHILD_PREFLIGHT_MAX_OUTPUT_BYTES, childConfigFailureReason } from "./link-child-preflight"; const PROBE_TTL_MS = 5 * 60_000; const MIN_REMOTE_OCX = [2, 66, 0] as const; @@ -289,7 +289,10 @@ export async function apply(ctx: ManagementContext, state: LinkRouteState): Prom const configured = await run(ocx(["config", "show", "--json"]), budget, CHILD_PREFLIGHT_MAX_OUTPUT_BYTES); let childConfig: unknown = null; try { if (configured.code === 0) childConfig = JSON.parse(configured.stdout); } catch { /* fail closed */ } - if (!isRecord(childConfig) || !port(childConfig.port)) return await failed(fail("child_preflight_failed", "The Child effective configuration could not be read.", 502)); + if (!isRecord(childConfig) || !port(childConfig.port)) { + const hint = await childConfigFailureReason(configured, () => run(ocx(["config", "validate", "--json"]), budget, CHILD_CONFIG_VALIDATE_MAX_OUTPUT_BYTES)); + return await failed(fail("child_preflight_failed", "The Child effective configuration could not be read.", 502, hint)); + } if ((childConfig.runtimeRole ?? "standalone") !== "standalone") return await failed(fail("child_restart_unsupported", "The Child must run standalone OpenCodex; an existing Hub or Child cannot be converted by Add Child.", 409)); const childPort = childConfig.port; if (remotePlatform === "posix" && (await run(REMOTE_CURL_PREFLIGHT)).code !== 0) return await failed(fail("child_curl_missing", "Install curl on the Child before connecting a Remote Link.", 409)); diff --git a/structure/remote-link.md b/structure/remote-link.md index 9d74e557593..a9dd3262a61 100644 --- a/structure/remote-link.md +++ b/structure/remote-link.md @@ -154,7 +154,7 @@ A deadline still rejects late output. `src/server/management/link-enrollment.ts` owns probe, host confirmation and Home apply; `src/server/management/link-route-state.ts` supplies shared authorization and lifecycle compensation. The three routes accept the existing dashboard authority or trusted loopback admin authority; Tailscale identity sessions remain refused. Join remains operator-paired only. CLI `ocx link probe ` requests the optional `includeVersion: true` offer; legacy dashboard offers retain their three fields. Version probing executes against the temporary learned key and persists no trust. Confirm rechecks the pending generation and five-minute expiry after its SSH await, then updates the latest known_hosts atomically under the config lock. Same-alias operations fail fast on overlap; apply pins a private immutable host file and consumes confirmation before issuing a key. -`src/link/remote-probe.ts` separates command budget (15 seconds plus four times measured SSH setup, capped at 60 seconds) from a fresh 15-second relay deadline. Restart adds the existing 60-second drain and 70-second replacement windows. The runner's one deadline covers stdin writes, process exit and output drains; failure cancels readers and kills the local SSH child without an unbounded cleanup wait. Ordinary stdout and stderr cap at 64 KiB. The catalog command and the Child `config show --json` preflight in `src/server/management/link-child-preflight.ts` permit 4 MiB stdout, because no smaller command reports `port`, `runtimeRole` and `ephemeralChild` on every supported Child: full `ocx` has `config get`, the lightweight `ocx-child` binary answers only `config show --json`. A Child exceeding even that cap answers `502 child_preflight_output_limit` instead of a generic preflight failure. Coverage: `tests/clients/link-ssh-runner.test.ts`, `tests/server/link-enrollment.test.ts`. +`src/link/remote-probe.ts` separates command budget (15 seconds plus four times measured SSH setup, capped at 60 seconds) from a fresh 15-second relay deadline. Restart adds the existing 60-second drain and 70-second replacement windows. The runner's one deadline covers stdin writes, process exit and output drains; failure cancels readers and kills the local SSH child without an unbounded cleanup wait. Ordinary stdout and stderr cap at 64 KiB. The catalog command and the Child `config show --json` preflight in `src/server/management/link-child-preflight.ts` permit 4 MiB stdout, because no smaller command reports `port`, `runtimeRole` and `ephemeralChild` on every supported Child: full `ocx` has `config get`, the lightweight `ocx-child` binary answers only `config show --json`. A Child exceeding even that cap answers `502 child_preflight_output_limit` instead of a generic preflight failure. Coverage: `tests/clients/link-ssh-runner.test.ts`, `tests/server/link-enrollment.test.ts`. A `config show` that exits non-zero answers `child_preflight_failed` with the Child's own validation reason as the hint: a full `ocx` Child then runs `config validate --json` (16 KiB cap), and only its `error` string, which names fields and expected types and never configured values, is read through `boundHint`. A Child without that command, or one whose validate fails or times out, keeps the last `config show` stderr line (`sshFailureHint`), or names the exit code when stderr is empty; a `config show` that exits 0 without a usable port names the command. The CLI (`src/cli/link.ts` `errorCodeBody`) keeps exactly that hint, bounded and redacted again with `boundHint`, beside the error code, and still never echoes the rest of an error body (#8357). Before issuance, apply requires curl and effective standalone Child configuration plus a native Child health response on its configured port with `restartCapability: v1`. Already-client, mismatched-port and pre-capability runtimes are refused without enrollment. Thus a capable older standalone Child can be linked by a newer Home. Home calls existing `ocx restart` exactly once after `connect --key-stdin`; no raw stop/start or timeout replay. After client-role health, Home registers a fresh expected-key observer and asks Child's credential-free loopback `/v1/catalog`. Both a complete HTTP 200 JSON catalog and matching authenticated Home callback must occur within the relay window. Enrollment admission cannot satisfy this check. Curl ignores curlrc, proxies and redirects. Phase-specific failures compensate Home state; after possible remote commit, `residual: true` and fixed Child disconnect/restart guidance remain even if compensation fails. This is not remote rollback. diff --git a/tests/cli/cli-link-enrollment.test.ts b/tests/cli/cli-link-enrollment.test.ts index 5cc2cf21dfd..13dd81b9832 100644 --- a/tests/cli/cli-link-enrollment.test.ts +++ b/tests/cli/cli-link-enrollment.test.ts @@ -167,6 +167,33 @@ describe("CLI Remote Link enrollment", () => { } }); + test("a phase failure prints the server's bounded cause, not only the phase name (#8357)", async () => { + let step = 0; + const h = harness(); + try { + const hint = "Error: defaultProvider must exist in providers"; + h.deps.fetchImpl = async () => ++step < 3 ? Response.json(step === 1 ? probe : confirm) : Response.json({ error: { code: "child_preflight_failed", message: "The Child effective configuration could not be read.", hint } }, { status: 502 }); + expect(await runLinkCommand(["add", "child-a", "--expect-fingerprint", fingerprint], h.deps)).not.toBe(0); + const message = h.err.join("\n"); + expect(message).toContain("Remote Link failed during Child preflight"); + expect(message).toContain(hint); + } finally { h.restore(); } + }); + + test("only the bounded, redacted hint survives from an error body (#8357)", async () => { + let step = 0; + const h = harness(); + try { + const secret = "ocx_data_" + "a".repeat(40); + h.deps.fetchImpl = async () => ++step < 3 ? Response.json(step === 1 ? probe : confirm) + : Response.json({ error: { code: "remote_connect_failed", message: "leak " + secret, hint: "connect said " + secret }, key: secret }, { status: 502 }); + expect(await runLinkCommand(["add", "child-a", "--expect-fingerprint", fingerprint], h.deps)).not.toBe(0); + const message = h.err.join("\n"); + expect(message).toContain("Remote Link failed during Child enrollment: connect said ocx_"); + expect(message).not.toContain(secret); + } finally { h.restore(); } + }); + test("a bare host without a local ocx-child build names where Home looks for one", async () => { const missing = { alias: "bare-b", fingerprint, keyType: "ED25519", ocxVersion: null, runtimeKind: "missing", targetTriple: "linux-arm64", plantable: false, plantUnavailableReason: "artifact_not_found" }; const h = harness([missing]); diff --git a/tests/server/link-enrollment.test.ts b/tests/server/link-enrollment.test.ts index acaab8848eb..c7e1365d016 100644 --- a/tests/server/link-enrollment.test.ts +++ b/tests/server/link-enrollment.test.ts @@ -30,7 +30,7 @@ const result = (stdout = "", code = 0): SshRunResult => ({ code, stdout, stderr: const http = (body: unknown, status = 200) => result(`${JSON.stringify(body)}\n${status}`); const health = (role: string, restartCapability: string | null = "v1") => http({ service: "opencodex", pid: 4321, port: 10100, role, ...(restartCapability === null ? {} : { restartCapability }) }); type RunOptions = Parameters[1]; -type Step = "probe" | "uname" | "windows" | "fingerprint" | "version" | "port" | "config" | "curl" | "before" | "connect" | "restart" | "health" | "catalog"; +type Step = "probe" | "uname" | "windows" | "fingerprint" | "version" | "port" | "config" | "validate" | "curl" | "before" | "connect" | "restart" | "health" | "catalog"; interface Call { step: Step; argv: readonly string[]; options: RunOptions; } function deferred() { let resolve!: (value: T) => void; @@ -90,6 +90,7 @@ function harness(remotePlatform: "posix" | "windows" = "posix") { : command === remoteOcx(["--version"]) ? "version" : command === remoteOcx(["link", "port"]) ? "port" : command === remoteOcx(["config", "show", "--json"]) ? "config" + : command === remoteOcx(["config", "validate", "--json"]) ? "validate" : command.includes("ocx-curl-preflight") ? "curl" : (nativeArgs?.[0] === "connect" || command.includes("'connect' '--link' '--key-stdin'")) ? "connect" : command === remoteOcx(["restart"]) ? "restart" @@ -121,6 +122,7 @@ function harness(remotePlatform: "posix" | "windows" = "posix") { case "version": return result("opencodex 2.83.0\n"); case "port": return result('{"port":2200}'); case "config": return result('{"port":10100}\n'); + case "validate": return { code: 1, stdout: "", stderr: "Unknown command: config validate\n" }; case "curl": case "connect": return result(); case "before": return health("standalone"); case "restart": return result(); @@ -330,6 +332,35 @@ describe("Home enrollment of a standalone Child", () => { expect(config.options?.maxOutputBytes).toBe(SSH_MAX_OUTPUT_BYTES); }); + test("a Child preflight failure carries the Child's own reason (#8357)", async () => { + // The line a real `ocx config show --json` prints for an invalid saved configuration: generic, + // and long enough that the 160-code-point cap cuts it before naming any defect. + const showWarning = "Warning: Config at /var/lib/opencodex-child/Library/Application Support/opencodex/config.json is invalid or unreadable; defaults are being shown for invalid settings. Run: ocx config validate. Inspect: ocx config show --source.\n"; + const h = harness(); h.confirmed(); + h.overrides.config = () => ({ code: 1, stdout: "", stderr: showWarning }); + h.overrides.validate = () => ({ code: 1, stdout: '{\n "ok": false,\n "error": "schema_invalid: defaultProvider: defaultProvider must exist in providers"\n}\n', stderr: "" }); + const payload = await denied(await h.call("apply", { alias: "child" }), "child_preflight_failed") as { error: { hint: string } }; + expect(payload.error.hint).toBe("Child config is invalid: schema_invalid: defaultProvider: defaultProvider must exist in providers"); + expect(h.calls.find(call => call.step === "validate")?.options?.maxOutputBytes).toBe(16 * 1024); + noEnrollmentEffects(h); + // A Child without `config validate` (ocx-child, an older ocx) keeps the config show reason. + const legacy = harness(); legacy.confirmed(); + legacy.overrides.config = () => ({ code: 1, stdout: "", stderr: "opencodex: invalid config\nError: defaultProvider must exist in providers\n" }); + const third = await denied(await legacy.call("apply", { alias: "child" }), "child_preflight_failed") as { error: { hint: string } }; + expect(third.error.hint).toBe("Error: defaultProvider must exist in providers"); + // A validate that times out must not mask it either. + const hung = harness(); hung.confirmed(); + hung.overrides.config = () => ({ code: 1, stdout: "", stderr: "Error: defaultProvider must exist in providers\n" }); + hung.overrides.validate = () => { throw new SshRunnerError("timeout", "ssh command timed out"); }; + const fourth = await denied(await hung.call("apply", { alias: "child" }), "child_preflight_failed") as { error: { hint: string } }; + expect(fourth.error.hint).toBe("Error: defaultProvider must exist in providers"); + const unreadable = harness(); unreadable.confirmed(); + unreadable.overrides.config = () => result("not json\n"); + const second = await denied(await unreadable.call("apply", { alias: "child" }), "child_preflight_failed") as { error: { hint: string } }; + expect(second.error.hint).toContain("config show --json"); + expect(unreadable.calls.some(call => call.step === "validate")).toBe(false); + }); + test("an output limit in Child preflight names the cap instead of failing generically", async () => { const h = harness(); h.confirmed(); h.overrides.config = () => { throw new SshRunnerError("output_limit", `ssh output exceeded ${SSH_MAX_OUTPUT_BYTES} bytes`); };