From 4303a8656aaea79b38a0f7a10aea69f8bd0fdd37 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 17 Sep 2026 14:18:59 +0900 Subject: [PATCH] perf(cli): stop config show from importing the connect graph to read one flag `ocx config show` timed out at its 40-second bound on Windows shard 6/9 of run 35182806140. The same case took 879ms and 1673ms in the two preceding dispatches, so this was a 25-45x outlier rather than a chronic cost, and a bigger budget would have been the wrong answer twice over. The obvious suspect was Windows ACL hardening, and it is not that: `config show` reads through readConfigDiagnostics and never calls loadConfig, the failing job carries no ACL diagnostic line, and the ACL-free `config get` case beside it also took 16.5 seconds. What both share is cold module loading. The command was importing the whole connect, lifecycle and catalog graph for one thing: deciding whether the `_remoteHub` annotation should say connected. On a cold Windows process that import is most of the command's cost, and it can drag the lifecycle recovery path in behind it. It now derives that annotation from the validated client record and the bounded service-token reader, without importing ./connect, without catalog readiness work and without entering lifecycle recovery. Nothing about the security boundary changes: no writer moved, and no `required: true` ACL call was touched. If the read-path reasoning is wrong the worst case is an inaccurate `_remoteHub.connected` display during an unusual recovery state - secret permissions and persisted bytes are unaffected. No local suite, focused test, typecheck, build, or install was run. --- src/cli/config-command.ts | 53 ++++++++++++++++-------- structure/clients/claude-desktop.md | 4 ++ structure/config.md | 5 +++ structure/ops/docs-and-release.md | 4 ++ structure/runtime.md | 5 +++ tests/cli/cli-config-show-client.test.ts | 29 +++++++++++-- 6 files changed, 78 insertions(+), 22 deletions(-) diff --git a/src/cli/config-command.ts b/src/cli/config-command.ts index a063602b59..9a970299fe 100644 --- a/src/cli/config-command.ts +++ b/src/cli/config-command.ts @@ -4,7 +4,7 @@ import { getConfigPath, mutatePersistedConfig, readConfigDiagnostics, sanitizeMo import { VISION_REASONING_EFFORTS, isVisionReasoningEffort } from "../reasoning-effort"; import type { OcxConfig } from "../types"; import { normalizeVisionReasoningForModel } from "../vision/reasoning"; -import type { ClientConnectionStatus } from "./connect"; +import type { ServiceApiTokenState } from "../lib/service-secrets"; import { CliUsageError, printData, rejectArgs, runCliAction, takeFlag } from "./runtime-api"; const USAGE = `Usage: @@ -38,9 +38,8 @@ const BLOCKED_SEGMENTS = new Set(["__proto__", "prototype", "constructor"]); * `client` block, which is the same defect in miniature: the presence of configuration is not * evidence that the connection works, and a machine whose data-plane token was revoked, rotated * away or deleted would have been labelled `connected: true` while it could not reach the hub at - * all. `collectClientConnectionStatus` is the one reader that knows — it compares the token file's - * fingerprint against the connection record — so the caller passes its answer in and this stays - * pure and testable. + * all. The read-only projection compares the bounded token file's fingerprint against the + * connection record, and passes that answer in so this formatter stays pure and testable. * * Synthetic and NOT persisted, for two reasons. `clientConnectionSchema` is `.strict()`, so a * `client.note` field would not validate; and persisted prose drifts from the behaviour it @@ -49,7 +48,7 @@ const BLOCKED_SEGMENTS = new Set(["__proto__", "prototype", "constructor"]); */ export function remoteHubConfigNote( config: OcxConfig, - readConnection: () => Pick, + readConnection: () => RemoteHubConnectionObservation, ): { connected: boolean; origin: string; note: string } | null { if (config.runtimeRole !== "client" || !config.client) return null; // A thunk, so a standalone or hub install pays nothing: the guard above returns first and the @@ -67,6 +66,36 @@ export function remoteHubConfigNote( return { connected, origin: config.client.serverUrl, note }; } +export type RemoteHubConnectionObservation = { + state: "disconnected" | "connected" | "invalid" | "mismatched"; + reason?: string; + token: "owned" | "missing" | "changed" | "unsafe"; +}; + +export function remoteHubConnectionFromTokenState( + config: Pick, + tokenState: ServiceApiTokenState, +): RemoteHubConnectionObservation { + const token = tokenState.kind === "absent" + ? "missing" + : tokenState.kind === "unsafe" + ? "unsafe" + : tokenState.fingerprint === config.client?.tokenFingerprint ? "owned" : "changed"; + return { state: "connected", token }; +} + +async function readRemoteHubConfigNote(config: OcxConfig): Promise> { + if (config.runtimeRole !== "client" || !config.client) return null; + // This display command needs only connection ownership, not lifecycle recovery, catalog + // readiness, or any write-capable connect machinery. Keep the read on the bounded token + // observer so a cold `config show` never imports the full connect command graph. + const { readServiceApiTokenState } = await import("../lib/service-secrets"); + return remoteHubConfigNote( + config, + () => remoteHubConnectionFromTokenState(config, readServiceApiTokenState()), + ); +} + function redact(value: unknown, key = ""): unknown { if (SECRET_KEYS.test(key) && typeof value === "string") return value ? "********" : value; // `client.priorCatalog` is the base64 catalog snapshot connect took before overwriting the @@ -168,19 +197,7 @@ export async function handleConfigCommand(argv: string[]): Promise { rejectArgs(args, USAGE); const diagnostics = readConfigDiagnostics(); const redacted = redact(diagnostics.config); - // Imported here rather than at module scope: `./connect` pulls the whole client lifecycle - // in, and `ocx config get/set` has no use for it. - const { collectClientConnectionStatus } = await import("./connect"); - // The readiness probe is declined explicitly. `collectClientConnectionStatus` observes the - // local Codex ladder for a connected client, and observing it spawns `codex debug models` - // under a 45s budget. `ocx config show` reads only `state`, `reason` and `token` from the - // result, so paying for a subprocess here would buy nothing and would quietly turn a - // read-only config dump into a runtime probe. Returning no ladder resolves readiness to - // `unverified`, which is the honest answer for a caller that never asked. - const note = remoteHubConfigNote( - diagnostics.config, - () => collectClientConnectionStatus(undefined, undefined, { supportedEfforts: () => null }), - ); + const note = await readRemoteHubConfigNote(diagnostics.config); // First key, not last: it has to be read before the empty `providers` map that misled a // reader into concluding nothing was configured anywhere. const config = note && redacted && typeof redacted === "object" && !Array.isArray(redacted) diff --git a/structure/clients/claude-desktop.md b/structure/clients/claude-desktop.md index 3c7ec11ea9..305c123854 100644 --- a/structure/clients/claude-desktop.md +++ b/structure/clients/claude-desktop.md @@ -97,6 +97,10 @@ The shared Responses path follows the [bounded multipart recovery contract](../s Connected `ocx status` diagnostics follow the shared [status credential binding](../runtime.md#remote-hub-status-credential-binding). +The smaller `_remoteHub` annotation from `src/cli/config-command.ts` is intentionally independent +of Desktop recovery and catalog readiness. It observes only the validated client record and local +data-token ownership, so displaying configuration cannot enter Desktop or client lifecycle work. + ## Claude Desktop config-library resolution The Desktop profile writer and the management status probe share diff --git a/structure/config.md b/structure/config.md index f5e680666c..2c066e4fbb 100644 --- a/structure/config.md +++ b/structure/config.md @@ -79,6 +79,11 @@ the secret itself. Malformed optional data-loopback and nested hub-management listener blocks are disabled in memory and reported by load-time warnings and read-only config diagnostics. Ingress warnings validate the raw ingress independently, so an invalid hub sibling does not falsely blame a valid ingress. The warning names only the field; unrelated providers and keys survive. Explicit writes remain strictly validated. +The `ocx config show` reader in `src/cli/config-command.ts` uses those diagnostics directly. Its +client annotation compares only the bounded service-token fingerprint with the validated client +record; it does not call `loadConfig`, mutate permissions, or import the write-capable connect flow. +All config publication continues through the existing required ACL-hardened writers above. + `claudeCode.desktopProfile` follows the same preserve-the-rest rule. JSON `null` (or any non-string) `appliedFingerprint` / `appliedAt` is treated as unset. A profile that is still invalid after that is dropped as a whole — `src/config/salvage.ts` already does this for independent `routingProfiles` / `combos` entries — so one bad Desktop marker cannot replace the operator's providers with `getDefaultConfig()`. A `claudeCode` value that is not an object still fails the document, because there is no safe subtree to keep. The former `showCodexSparkQuota` key is inert passthrough data when loading an old config. diff --git a/structure/ops/docs-and-release.md b/structure/ops/docs-and-release.md index 2f37944405..1bb34e4dc4 100644 --- a/structure/ops/docs-and-release.md +++ b/structure/ops/docs-and-release.md @@ -11,6 +11,10 @@ Shared parsing and streaming follow the [request-copy](../transports/byte-accoun Human-readable connect and sync-refresh diagnostics follow the [terminal rendering contract](../runtime.md#cli-readiness-diagnostics), with regression coverage for both paths in `tests/cli/cli-connect-readiness.test.ts`. +`tests/cli/cli-config-show-client.test.ts` covers the separate read-only config annotation path: +`src/cli/config-command.ts` derives token ownership without importing the connect command or +triggering catalog, lifecycle, or ACL-hardening work. + The CLI default dashboard address follows the [management ingress bind](../runtime.md#hub-management-dashboard-address), covered by `tests/cli/cli-dispatch.test.ts`. Native main reauthentication follows the [CLI JSON output contract](../runtime.md#native-main-reauth-json-output). diff --git a/structure/runtime.md b/structure/runtime.md index 980ff94208..17a3ccbf95 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -355,6 +355,11 @@ readiness, then reads that runtime's effort ladder without persisting its select preferred candidates still fall back in priority order. General `ocx status` retains full runtime discovery and passes its resolved command into readiness, avoiding a second version probe without adding cache state. +`ocx config show` stays outside that lifecycle path. `src/cli/config-command.ts` reads the validated +config snapshot and the bounded service-token observation needed for its `_remoteHub` annotation; +it does not import the connect command, inspect catalog readiness, acquire lifecycle locks, or run +config/secret ACL hardening. + `src/remote/protocol.ts` owns pure interval/feature negotiation. `src/remote/hub-state.ts` owns the `GET|HEAD /v1/hub-state` contract, its caps, and the parser both sides share. `src/client/hub-client.ts` owns bounded, schema-validated remote catalog consumption, hub-state reads, and key-id probes; `src/client/hub-state.ts` owns the resolution and the owner-stamped 0600 cache, and a failed read reports "unavailable" rather than degrading to the client's own local provider and login state. `src/client/hub-relay.ts` is a fixed-authority management relay with URL, header, body, redirect, and stream bounds. The public data listener remains the direct client→hub path; the loopback management ingress never serves data-plane routes. ### Remote Hub status credential binding diff --git a/tests/cli/cli-config-show-client.test.ts b/tests/cli/cli-config-show-client.test.ts index 336c869141..575f9522bc 100644 --- a/tests/cli/cli-config-show-client.test.ts +++ b/tests/cli/cli-config-show-client.test.ts @@ -19,7 +19,10 @@ import { createHash } from "node:crypto"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; -import { remoteHubConfigNote } from "../../src/cli/config-command"; +import { + remoteHubConfigNote, + remoteHubConnectionFromTokenState, +} from "../../src/cli/config-command"; import type { OcxConfig } from "../../src/types"; import { removeTreeWithRetry } from "../helpers/remove-tree"; import { SPAWN_BUDGET_MS } from "../helpers/test-budget"; @@ -44,6 +47,7 @@ const PRIOR_CATALOG = "A".repeat(12_288); /** The data-plane token this fixture's `tokenFingerprint` is computed from. */ const FIXTURE_TOKEN = "fixture-token"; +const FIXTURE_TOKEN_FINGERPRINT = createHash("sha256").update(FIXTURE_TOKEN).digest("hex"); function clientHome(options: { token?: string | null } = {}): string { const home = mkdtempSync(join(tmpdir(), "ocx-config-client-")); @@ -60,7 +64,7 @@ function clientHome(options: { token?: string | null } = {}): string { selectedClients: ["codex", "claude"], tokenEnv: "OPENCODEX_API_AUTH_TOKEN", apiKeyId: "client-one", - tokenFingerprint: createHash("sha256").update(FIXTURE_TOKEN).digest("hex"), + tokenFingerprint: FIXTURE_TOKEN_FINGERPRINT, protocolVersion: 1, connectedAt: "2026-09-01T00:00:00.000Z", priorCatalog: PRIOR_CATALOG, @@ -75,7 +79,7 @@ function standaloneHome(): string { return home; } -/** The shape `collectClientConnectionStatus()` returns, narrowed to what the note reads. */ +/** The narrow connection observation consumed by the display-only note. */ type NoteConnection = Parameters[1] extends () => infer T ? T : never; function connection(overrides: Partial = {}): NoteConnection { @@ -84,7 +88,10 @@ function connection(overrides: Partial = {}): NoteConnection { const CLIENT_CONFIG = { runtimeRole: "client", - client: { serverUrl: "https://hub.example.test:8443" }, + client: { + serverUrl: "https://hub.example.test:8443", + tokenFingerprint: FIXTURE_TOKEN_FINGERPRINT, + }, } as OcxConfig; describe("remoteHubConfigNote", () => { @@ -133,6 +140,20 @@ describe("remoteHubConfigNote", () => { expect(disconnected?.connected).toBe(false); expect(disconnected?.note).toContain("its connection is disconnected"); }); + + test("the read-only note derives ownership from the bounded token observation alone", () => { + expect(remoteHubConnectionFromTokenState(CLIENT_CONFIG, { + kind: "present", + token: FIXTURE_TOKEN, + fingerprint: FIXTURE_TOKEN_FINGERPRINT, + })).toEqual({ state: "connected", token: "owned" }); + expect(remoteHubConnectionFromTokenState(CLIENT_CONFIG, { kind: "absent" })) + .toEqual({ state: "connected", token: "missing" }); + expect(remoteHubConnectionFromTokenState(CLIENT_CONFIG, { + kind: "unsafe", + reason: "not a bounded regular file", + })).toEqual({ state: "connected", token: "unsafe" }); + }); }); describe("ocx config show on a client", () => {