From 240ac38ab9361886a4e1d0159eaa60e5475fa067 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 25 Aug 2026 11:26:44 +0900 Subject: [PATCH 1/2] devlog: operator visibility train roadmap unit (260825) Docs-only roadmap for three operator-visibility defects that share one shape: OpenCodex computes the truth and does not report it. - 010 (#2457): the sidecar pair check collapses a five-member union into a two-arm ternary, so a submitted gemini backend is validated against the stored openai backend and the dashboard save 400s. - 020 (#2411): collectStatus already computes routingKind and ships it in status --json, but the human renderer never prints it, so a healthy proxy reads green while nothing routes through it. - 030 (#2412): a version-manager overwrite leaves a message-less ineligible verdict, and the CLI warns only when a message exists. 002 records the plan audit, including two amendments: WP4 must plumb messages to every reachable silent ineligible return rather than only the one the reporter hit, and WP3 must pin custom-local/unknown as intentionally silent. --- .../000_baseline_and_scope.md | 72 +++++++++ .../001_current_state_inventory.md | 130 ++++++++++++++++ .../002_plan_audit.md | 90 +++++++++++ ...p2_issue2457_sidecar_backend_resolution.md | 129 ++++++++++++++++ ...wp3_issue2411_status_routing_visibility.md | 145 ++++++++++++++++++ .../030_wp4_issue2412_version_manager_shim.md | 142 +++++++++++++++++ 6 files changed, 708 insertions(+) create mode 100644 devlog/_plan/260825_operator_visibility_train/000_baseline_and_scope.md create mode 100644 devlog/_plan/260825_operator_visibility_train/001_current_state_inventory.md create mode 100644 devlog/_plan/260825_operator_visibility_train/002_plan_audit.md create mode 100644 devlog/_plan/260825_operator_visibility_train/010_wp2_issue2457_sidecar_backend_resolution.md create mode 100644 devlog/_plan/260825_operator_visibility_train/020_wp3_issue2411_status_routing_visibility.md create mode 100644 devlog/_plan/260825_operator_visibility_train/030_wp4_issue2412_version_manager_shim.md diff --git a/devlog/_plan/260825_operator_visibility_train/000_baseline_and_scope.md b/devlog/_plan/260825_operator_visibility_train/000_baseline_and_scope.md new file mode 100644 index 00000000000..542f15a1943 --- /dev/null +++ b/devlog/_plan/260825_operator_visibility_train/000_baseline_and_scope.md @@ -0,0 +1,72 @@ +# 000 — Operator visibility train: baseline, scope, and work-phase map + +Unit opened 2026-08-25. Session `01a03688-c5ee-76c2-bb0f-a7a9213345d5`. +Goalplan slug `fix-three-opencodex-operator-visibility-defects`. + +## Baseline + +Verified live at unit open, immediately after the v2.32.1 publish: + +| Ref | SHA | Meaning | +|-----|-----|---------| +| `origin/dev` | `bb89eafbe` | devlog: pin the report to the code SHA its gates describe (#2506) | +| `origin/main` | `71c57ea64` | `release: v2.32.1` | +| `origin/preview` | `f4cb9f800` | `release: v2.32.1-preview.20260825` | + +`git merge-base --is-ancestor origin/dev origin/main` exits 0, so `dev` is an +ancestor of the shipped release and this unit starts from published code. +npm `latest` is `2.32.1`, `preview` is `2.32.1-preview.20260825`. + +## What this unit is + +Three defects that share one shape: **OpenCodex knows the truth and does not +tell the operator.** None of them is a routing or execution bug. In all three +the runtime is already correct and the surface that reports to a human is +wrong, stale, or silent. + +| # | Surface | The lie | +|---|---------|---------| +| #2457 | Management write | The picker offers Gemini, then the save rejects it as an OpenAI model | +| #2411 | `ocx status` | Green proxy while nothing routes through it | +| #2412 | Shim auto-restore | A destroyed shim returns an ineligible verdict with no message | + +That shared shape is why they travel together and why none of them may be +"fixed" by changing behavior. Every fix in this unit is a reporting fix. + +## Work-phase map + +| Phase | Doc | Issue | Deliverable | +|-------|-----|-------|-------------| +| WP1 | this unit | — | Docs-only roadmap at diff-level precision | +| WP2 | `010` | #2457 | Submitted backend is what the pair check validates | +| WP3 | `020` | #2411 | `ocx status` prints routing and warns on unused proxy | +| WP4 | `030` | #2412 | Version-manager shim destruction is detected and reported | + +One work-phase is one full PABCD cycle. WP2, WP3, and WP4 each produce one PR +against `dev`. + +## Scope boundary + +Out of scope, stated once so no later phase reopens it: + +- Merging other contributors' PRs, or another npm release. +- `src/lab/` — the core-lab boundary test exists for a reason. +- The undeclared-tool guard, and any auth, OAuth, credential, workflow, or + release-automation surface. +- Auto-wrapping a version-manager-owned `codex` binary as a new original. + This is the one that is tempting and wrong; see `030`. +- The Codex-side namespaced-model error message in #2411's reproduction. That + is upstream copy, not ours. + +## Evidence rule + +A remembered pass is not evidence. Every completion claim in this unit carries +exact command output, the PR number and head SHA, and the CI run id and +conclusion on that SHA. + +## Prior art consulted + +- `260824_v2_32_1_hotfix_train/` — the freeze/GO discipline this unit inherits. +- `tests/repo-hygiene.test.ts` — no gitlinks, no vendored clones. +- `AGENTS.md` — focused checks during implementation, full suite before a + non-trivial PR goes review-ready. diff --git a/devlog/_plan/260825_operator_visibility_train/001_current_state_inventory.md b/devlog/_plan/260825_operator_visibility_train/001_current_state_inventory.md new file mode 100644 index 00000000000..611edb5af94 --- /dev/null +++ b/devlog/_plan/260825_operator_visibility_train/001_current_state_inventory.md @@ -0,0 +1,130 @@ +# 001 — Current-state inventory + +Read at `bb89eafbe`. Every line anchor below was opened and read, not inferred. + +## #2457 — the pair check discards the union + +The accepted union is complete. `src/server/management/config-routes.ts:591`: + +```ts +const WEB_SEARCH_BACKENDS_UNION = ["openai", "anthropic", "xai", "gemini", "exa"] as const; +``` + +The pair check nineteen lines later throws it away. `config-routes.ts:668`: + +```ts +const effectiveBackend = body.webSearch.backend === "anthropic" + ? "anthropic" + : body.webSearch.backend === "openai" || body.webSearch.backend === null + ? "openai" + : config.webSearchSidecar?.backend ?? "openai"; +``` + +A submitted `"gemini"` is not `"anthropic"`, not `"openai"`, not `null`. +It falls to the final arm and the request is validated against the **stored** +backend. With stored `openai` (or unset), `webSearchModelIsRejected("openai", +"gemini-3.7-flash", candidates)` is true, and the route returns 400 before the +persistence block at `:687` — which does honor the full union — ever runs. + +`src/server/management/agent-settings-routes.ts:1121` carries the same stale +ternary with a different null policy: + +```ts +const effectiveBackend = section.backend === "anthropic" + ? "anthropic" + : section.backend === "openai" + ? "openai" + : section.backend === null + ? config.webSearchSidecar?.backend ?? "openai" + : stored?.backend ?? config.webSearchSidecar?.backend ?? "openai"; +``` + +The comment directly above that block reads: *"Same module as +/api/sidecar-settings — a gate on one route and a stale copy on the other is no +gate at all."* The gate is shared; the backend resolution is not, and it drifted +exactly as the comment feared. + +`xai` and `exa` have the identical hole. They escape notice because +backend-only submissions short-circuit on `effectiveModel` being empty. + +The executor is already correct and must not be touched: +`resolveSidecarBackend("gemini")` returns `"gemini"` +(`src/web-search/index.ts:162`), and `planWebSearch` already defaults Gemini to +`gemini-3.7-flash` (`:285`). Writing the pair directly into `config.json` +works today, which is the reporter's own proof that only the write gate is wrong. + +## #2411 — status has the routing kind and never prints it + +`collectStatus()` already computes it. `src/cli/status.ts:188`: + +```ts +const startup = collectStartupHealth(config, { + service, + shim: codexShim, + routingKind: getCodexRoutingKind(), +}); +``` + +`startup` lands on `json.startup` at `src/cli/status.ts:316`, so +`ocx status --json` **already exposes** `startup.routingKind`. The human +renderer is what drops it. `src/cli/index.ts:845`: + +```ts +if (status.json.proxy.pid || status.json.proxy.health.ok) { + console.log(`✅ Proxy: ${status.proxyLabel}`); +} +``` + +That boolean never consults `startup.routingKind`. A live PID or a good +`/healthz` is sufficient for the green check. + +Worse, the next line reinforces it. `startupHealthSummary` +(`src/codex/autostart-health.ts:143`) renders native routing as *"native Codex +routing (no opencodex restart dependency)"*, and `deriveStartupHealth` marks it +`rebootSafe: true`. That is correct on its own terms — there is genuinely no +restart dependency when nothing routes — but printed under a green proxy it +reads as a second all-clear. + +`ocx doctor` already prints the missing token. `src/cli/doctor.ts:986`: + +```ts +console.log(` routing=${startup.routingKind}, service=${...}, shim=${...}`); +``` + +So the fix is not new computation. It is routing the value that already exists +to the surface people actually run. + +## #2412 — the ineligible verdict carries no message + +`src/codex/shim.ts:2043`: + +```ts +if (!existsSync(file.wrapperPath) || !hasUsableBackingPath(file)) return { status: "ineligible" }; +``` + +No `message` field. That is why the condition is invisible: the CLI warns only +when one exists. `src/cli/codex-shim-autorestore.ts:35`: + +```ts +} else if ((result.status === "deferred" || result.status === "ineligible") && result.message) { + deps.warn(`⚠️ ${result.message}`); +} +``` + +A mise/asdf/volta upgrade rewrites the install tree in place, destroying both +`codex` and its sibling `codex.opencodex-real` (`backupPathFor`, +`src/codex/shim.ts:601`). `hasUsableBackingPath` (`:481`) then returns false, +the silent ineligible fires, and `ocx start` / `ocx ensure` / +`ocx service repair` all proceed to report success. + +`diagnoseCodexShim` (`src/codex/shim.ts:2156`) already produces the exact +diagnostic string the reporter pasted. The information exists; nothing routes it +to the commands that matter. + +## The common root + +In all three, the correct value is computed and then discarded on the way to the +human: a validated union collapsed into a two-arm ternary, a routing kind +carried in JSON but not printed, a diagnosis produced by one command and absent +from three others. None of the three fixes changes what OpenCodex does. They +change what it admits. diff --git a/devlog/_plan/260825_operator_visibility_train/002_plan_audit.md b/devlog/_plan/260825_operator_visibility_train/002_plan_audit.md new file mode 100644 index 00000000000..2e3882fa591 --- /dev/null +++ b/devlog/_plan/260825_operator_visibility_train/002_plan_audit.md @@ -0,0 +1,90 @@ +# 002 — Plan audit (A phase, WP1) + +The dispatched read-only auditor produced nothing across four wait cycles and +was retired under the loop's failed-dispatch rule. The audit below was performed +directly by the main agent against source at `bb89eafbe`. Every anchor cited in +`001`, `010`, `020`, and `030` was re-opened and confirmed. + +## Anchor verification + +| Doc claim | Verified | +|-----------|----------| +| `config-routes.ts:591` union of five backends | yes, exact | +| `config-routes.ts:668` two-arm ternary falling back to stored | yes, exact | +| `config-routes.ts:688` persistence honors the full union | yes | +| `agent-settings-routes.ts:1121` five-arm ternary | yes | +| `cli/status.ts:188` computes `routingKind` | yes | +| `cli/status.ts:316` `startup` lands in JSON | yes | +| `cli/index.ts:845` green check ignores routing | yes | +| `cli/doctor.ts:986` prints `routing=` | yes | +| `shim.ts:481` `hasUsableBackingPath` | yes | +| `shim.ts:1887` `allowFreshInstall` guard | yes | +| `cli/codex-shim-autorestore.ts:35` warns only with a message | yes | +| `autostart-health.ts:143` `startupHealthSummary` | yes | + +One correction: `030` cites the destroyed-shim bail as `shim.ts:2043`. The +actual line is **`2045`**; `2043` is inside the `preserveOnly` branch. The +quoted code is right, the number is off by two. + +## Blocking findings + +**A1 — `030` targets only one of six `ineligible` returns.** +`rg 'status: "ineligible"' src/codex/shim.ts` finds returns at `2028`, `2031`, +`2039`, `2042`, `2045`, `2049`, and `2085`. Only `2028` and `2085` carry a +message today. The plan attaches one to `2045`, but `2042` is the +`preserveOnly` sibling case and `2049` is `isHealthyShimProbe` — both are +reachable in a version-manager overwrite and both would stay silent. + +Correction: WP4 must attach messages to the reachable silent returns, not just +the one the reporter happened to hit. The `preserveOnly` branch at `2042` +deserves its own wording — its condition is a missing backup **or** a resurrected +original, which is a different story from a destroyed wrapper. + +**A2 — `020`'s truth table omits `custom-local` and `unknown`.** +`CodexRoutingKind` (`inject.ts:314`) has five members. The table covers +`opencodex-local`, `native`, and `custom-remote`. The predicate as written +returns `[]` for `custom-local` and `unknown`, which is the correct behavior — +`startupHealthSummary` already renders both as `AT RISK after restart` with a +remedy command (`autostart-health.ts:149-150`), so a second warning would be +noise. But the plan does not say so, and a later reader could "fix" the omission. + +Correction: state the five-member coverage explicitly and record that +`custom-local`/`unknown` are intentionally silent **because** they are already +loud elsewhere. Add both to the helper's test cases so the intent is pinned. + +## Non-blocking findings + +**B1 — `010`'s cast.** `WEB_SEARCH_BACKENDS_UNION.includes(x as ...)` does not +narrow `x` in TypeScript; `includes` returns `boolean`, not a type predicate. +The proposed `submittedBackend as typeof WEB_SEARCH_BACKENDS_UNION[number]` +cast in the true arm is therefore load-bearing, not decorative. It is sound +because `:591` already rejected non-members, but the doc should say that the +cast is doing real work rather than reading as noise. + +**B2 — `webSearchModelIsRejected`'s `backend` parameter type.** If it is typed +as the narrow union, passing the widened value type-checks only because both +resolve to the same union. Confirm at implementation time; if it is narrower, +the signature is the thing to widen, not the call site to cast. + +**B3 — line-number drift.** `030` says `2043`, actual `2045`. Corrected in this +document rather than by rewriting `030`, so the drift stays visible. + +## Verified correct + +- The #2457 mechanism, end to end: union at `:591`, ternary at `:668`, + persistence at `:688`. A submitted `gemini` provably reaches the stored-backend + arm. +- Both null policies genuinely differ between the two routes. `010`'s refusal to + unify them is right. +- `startup.routingKind` is already in `status --json`. `020`'s claim that no + schema change is needed holds. +- `allowFreshInstall: false` at `1887` is the invariant that blocks adoption. + `030`'s refusal to relax it is correct, and it is what makes A1 a + message-plumbing fix rather than a behavior change. + +## Verdict + +**PASS with two required amendments.** A1 and A2 are corrections to WP4 and WP3 +scope respectively; neither invalidates the plan's shape, and both are folded +into this document rather than silently patched into the originals. B1–B3 are +notes for the implementer. diff --git a/devlog/_plan/260825_operator_visibility_train/010_wp2_issue2457_sidecar_backend_resolution.md b/devlog/_plan/260825_operator_visibility_train/010_wp2_issue2457_sidecar_backend_resolution.md new file mode 100644 index 00000000000..dd91d9f3037 --- /dev/null +++ b/devlog/_plan/260825_operator_visibility_train/010_wp2_issue2457_sidecar_backend_resolution.md @@ -0,0 +1,129 @@ +# 010 — WP2: the submitted sidecar backend is what the pair check validates (#2457) + +## The change in one sentence + +Both management write paths must validate the requested model against the +**backend the caller submitted**, not against a two-member subset with the +stored backend as fallback. + +## Hunk 1 — `src/server/management/config-routes.ts` (`PUT /api/sidecar-settings`) + +Before, at `:668`: + +```ts +const effectiveBackend = body.webSearch.backend === "anthropic" + ? "anthropic" + : body.webSearch.backend === "openai" || body.webSearch.backend === null + ? "openai" + : config.webSearchSidecar?.backend ?? "openai"; +``` + +After: + +```ts +const submittedBackend = body.webSearch.backend; +const effectiveBackend = + typeof submittedBackend === "string" + && WEB_SEARCH_BACKENDS_UNION.includes(submittedBackend as typeof WEB_SEARCH_BACKENDS_UNION[number]) + ? submittedBackend as typeof WEB_SEARCH_BACKENDS_UNION[number] + : submittedBackend === null + ? "openai" + : config.webSearchSidecar?.backend ?? "openai"; +``` + +`WEB_SEARCH_BACKENDS_UNION` is already in scope at `:591`; an unknown literal +was already rejected there, so by this point a string is either a union member +or the request is dead. + +## Hunk 2 — `src/server/management/agent-settings-routes.ts` (`PUT /api/claude-code`) + +Before, at `:1121`: the five-arm ternary quoted in `001`. + +After, reusing the local `allowedBackends` built at `:1081`: + +```ts +const submittedBackend = section.backend; +const effectiveBackend = + typeof submittedBackend === "string" && allowedBackends.includes(submittedBackend) + ? submittedBackend as WebSearchBackend + : submittedBackend === null + ? config.webSearchSidecar?.backend ?? "openai" + : stored?.backend ?? config.webSearchSidecar?.backend ?? "openai"; +``` + +## The two null policies are different and both stay + +This is the part a careless fix breaks. They are not the same rule: + +| Route | `backend: null` means | Resolves to | +|-------|------------------------|-------------| +| `/api/sidecar-settings` | unset the global backend | `"openai"` (the resolver's own default for unset) | +| `/api/claude-code` | drop the Claude override | inherit `config.webSearchSidecar?.backend ?? "openai"` | + +Do not unify them. A shared helper that collapses both to one fallback would +silently change what clearing the Claude override means. + +## Shape decision + +Two shapes were considered: + +- **A (chosen):** inline the union membership check in both writers. +- **B:** extract `submittedWebSearchBackend()` into + `web-search-sidecar-options.ts`. + +B reads better as drift protection, which is exactly what failed here. But the +two null policies above cannot live in one helper, so B would extract only the +string arm and leave the divergent part behind — the appearance of unification +without the substance. A is five lines per route with the union named locally. +If a reviewer prefers B, the helper must take the null fallback as a parameter. + +## What must NOT change + +- `webSearchModelIsRejected` / `webSearchModelRejection` + (`src/server/management/web-search-sidecar-options.ts:91`). The helper is + correct; only its `backend` argument was wrong. +- The runtime executor: `src/web-search/index.ts`, `src/web-search/backends.ts`. +- The raw `config.json` escape hatch, which deliberately skips this gate. +- Vision sidecar validation, which has a different three-member union ending in + `"routed"`, not `"exa"`. +- `GET /api/sidecar-settings` and its `webSearchModels` rows. + +## Must still return 400 after the fix + +These are the assertions that prove the gate was not merely widened: + +1. `{ backend: "openai", model: "claude-haiku-4-5" }` — real mismatch. +2. `{ model: "gemini-3.7-flash" }` with backend omitted and stored `openai` — + preserved-backend semantics survive. +3. `{ backend: "gemini", model: "gpt-5.6-luna" }` — inverse mismatch. +4. `{ backend: "zen" }` — still fails the union gate at `:591`. + +## Regression tests + +All in `tests/sidecar-settings-web-search-gate.test.ts`, which already mocks +`getAccountSet` and `listManagementModelRows`. A Gemini pair placed in +`tests/web-search-backend-union.test.ts` would still be rejected after the fix +because that file has no candidate rows — the pair check would correctly find no +matching row. Wrong file, false failure. + +| Test | Setup | Assertion | Fails before? | +|------|-------|-----------|---------------| +| `PUT persists openai/luna -> gemini/gemini-3.7-flash` | stored `{openai, gpt-5.6-luna}`, `google-antigravity` oauth + healthy account set with `projectId`, management row `gemini-3.7-flash` | 200, config holds the Gemini pair | **Yes** — 400 today | +| `each leftover union member persists its own pair` (`test.each(["xai","gemini"])`) | matching candidate per backend | 200 each | **Yes** | +| `omitted backend still validates against the stored backend` | Gemini row live, PUT model only | 400, stored pair unchanged | No — guards the fix | +| `PUT /api/claude-code persists a gemini override` | stored override `{openai, gpt-5.6-luna}` | 200, `claudeCode.webSearchSidecar` is the Gemini pair | **Yes** — 400 today | + +## Existing tests that must stay green + +- `PUT rejects a backend/model mismatch and does not persist it` (`:139`) +- `PUT validates a backend-only update against the preserved effective model` (`:150`) +- `PUT persists the Anthropic auth-slot pair exactly as offered` (`:160`) +- `tests/claude-management-api.test.ts` sidecar round-trip (`:370`) +- `tests/gemini-web-search.test.ts` executor plan test (`:75`) — untouched, and + its continued passing is the proof the executor needed no change. + +## Acceptance + +`bun test tests/sidecar-settings-web-search-gate.test.ts tests/web-search-backend-union.test.ts tests/claude-management-api.test.ts tests/gemini-web-search.test.ts` +green; `bun x tsc --noEmit` exit 0; `bun run privacy:scan` pass; new tests +demonstrated red before the patch. diff --git a/devlog/_plan/260825_operator_visibility_train/020_wp3_issue2411_status_routing_visibility.md b/devlog/_plan/260825_operator_visibility_train/020_wp3_issue2411_status_routing_visibility.md new file mode 100644 index 00000000000..fbf78ea2756 --- /dev/null +++ b/devlog/_plan/260825_operator_visibility_train/020_wp3_issue2411_status_routing_visibility.md @@ -0,0 +1,145 @@ +# 020 — WP3: `ocx status` reports routing and warns on an unused proxy (#2411) + +## The change in one sentence + +`ocx status` prints the routing kind it already computes, and says so plainly +when a healthy proxy is paired with native routing. + +## The design question, settled + +Two shapes: + +- **A (chosen):** keep `✅` on the proxy line, always print `routing=`, and add + a warning only for the healthy-proxy + native-routing combination. +- **B:** flip the first line to `⚠️` for that combination. + +B is tempting because the reporter's complaint is literally "the check is +green." But the proxy line makes a narrow claim — the process is up and +`/healthz` answered — and that claim is **true** in this state. The reporter +proved it himself by curling the proxy directly and getting `ok`. Turning that +line yellow would make the one honest signal lie in order to compensate for a +missing one. It also collides with the existing `❌` path, whose remedy text +("Restart with 'ocx start'") is wrong for this failure: the proxy does not need +restarting, Codex needs re-pointing. + +So: add the missing signal, do not corrupt the present one. + +## Hunk 1 — extract the routing detail so status and doctor cannot drift + +`src/codex/autostart-health.ts`, next to `startupHealthSummary` at `:143`: + +```ts +export function formatStartupRoutingDetail(health: StartupHealth): string { + const service = health.serviceViable + ? "viable" + : health.serviceInstalled ? "installed-but-unhealthy" : "absent"; + const shim = health.shimHealthy + ? "healthy" + : health.shimInstalled ? "stale" : "absent"; + return `routing=${health.routingKind}, service=${service}, shim=${shim}`; +} +``` + +`src/cli/doctor.ts:986` then becomes a call to it, emitting byte-identical +output. This matters: #2457 exists because two routes computed the same thing +separately and drifted. Do not introduce a second copy of doctor's line. + +## Hunk 2 — the warning predicate + +`src/cli/status.ts`, pure and exported for direct testing, in the manner of +`src/cli/status-oauth.ts:55`: + +```ts +export function unusedProxyWarningLines(input: { + proxyUp: boolean; + routingKind: StartupHealth["routingKind"]; +}): string[] { + if (!input.proxyUp || input.routingKind !== "native") return []; + return [ + "⚠️ Codex routing is native — the running proxy is unused.", + " Codex requests go to OpenAI, not this proxy. Re-point with: ocx restore back", + ]; +} +``` + +A pure function is the point: the interesting behavior is a two-input truth +table, and it should be testable without spawning a CLI. + +## Hunk 3 — render + +`src/cli/index.ts`, after the Health line at `:850`: + +```ts +const proxyUp = Boolean(status.json.proxy.pid || status.json.proxy.health.ok); +for (const line of unusedProxyWarningLines({ + proxyUp, + routingKind: status.json.startup.routingKind, +})) { + console.log(` ${line}`); +} +``` + +and after `Restart safety` at `:869`: + +```ts +console.log(` ${formatStartupRoutingDetail(status.json.startup)}`); +``` + +Placing the routing detail directly under restart safety is deliberate. That +summary line is the one that reads as a second all-clear ("no opencodex restart +dependency"); the routing token immediately below it supplies the missing +context for why there is no dependency. + +## Truth table + +| Proxy | Routing | First line | Warning | `routing=` | +|-------|---------|-----------|---------|------------| +| up | `opencodex-local` | ✅ | no | yes | +| up | `native` | ✅ | **yes** | yes | +| up | `custom-remote` | ✅ | no | yes | +| down | `native` | ❌ | no | yes | + +`custom-local` / `custom-remote` are also "this proxy is unused," but they are +a deliberate operator choice and `startupHealthSummary` already names them as a +remote gateway. Warning there would train people to ignore the warning. Native +is the accidental state, and the only one #2411 reports. + +Proxy down plus native routing must not warn: the operator has two problems and +the `❌` line with its restart remedy is the correct lead. + +## JSON + +No schema change, no `schemaVersion` bump. `startup.routingKind` is already in +the payload — the gap was never the data. Adding a derived +`proxyUnusedByCodex` boolean was considered and rejected: consumers can +combine two fields they already have, and `tests/cli-status-json.test.ts:21` +pins `schemaVersion === 1`. + +## What must NOT change + +- `classifyCodexRouting`, `getCodexRoutingKind`, `deriveStartupHealth`, + `startupHealthSummary`. This phase reads them; it does not touch them. +- `rebootSafe: true` for native routing. `tests/autostart-health.test.ts:108` + pins it, and it is correct: there really is no restart dependency. +- The `❌` branch and its `ocx start` / `ocx service repair` guidance. +- Redaction behavior of `status --json`. +- Anything in #2412's shim territory. The two issues are related as cause and + symptom but ship as separate PRs, per the maintainer's own split. + +## Regression tests + +| Test | File | Assertion | Fails before? | +|------|------|-----------|---------------| +| `unusedProxyWarningLines covers the four routing states` | `tests/cli-status-json.test.ts` | the truth table above | **Yes** — helper absent | +| `status prints routing=native without starting the proxy` | `tests/cli-help.test.ts` (extend `:139`) | stdout has `routing=native`, and does **not** have the unused-proxy warning while the proxy is down | **Yes** | +| `status --json exposes startup.routingKind` | `tests/cli-status-json.test.ts` | `parsed.startup.routingKind === "native"` | No — pins existing data against future removal | +| `formatStartupRoutingDetail matches doctor's line` | `tests/autostart-health.test.ts` | `routing=native, service=absent, shim=absent` | **Yes** | + +The CLI tests need a temp `CODEX_HOME` holding a `config.toml` without +`openai_base_url`; `tests/codex-plugins-doctor.test.ts:356` is the pattern. + +## Acceptance + +`bun test tests/cli-status-json.test.ts tests/cli-help.test.ts tests/autostart-health.test.ts tests/codex-plugins-doctor.test.ts` +green; `bun x tsc --noEmit` exit 0; `bun run privacy:scan` pass; doctor's +output byte-identical before and after the extraction. diff --git a/devlog/_plan/260825_operator_visibility_train/030_wp4_issue2412_version_manager_shim.md b/devlog/_plan/260825_operator_visibility_train/030_wp4_issue2412_version_manager_shim.md new file mode 100644 index 00000000000..561197decf8 --- /dev/null +++ b/devlog/_plan/260825_operator_visibility_train/030_wp4_issue2412_version_manager_shim.md @@ -0,0 +1,142 @@ +# 030 — WP4: detect and report version-manager shim destruction (#2412) + +## The change in one sentence + +When a version manager has overwritten the shim and its backup, say so with an +actionable message — and refuse to adopt the new binary as a replacement +original. + +## The temptation, and why it is wrong + +The obvious fix is to make auto-restore work: a backup is missing, so take the +current `codex` binary, rename it to `codex.opencodex-real`, and write a fresh +shim over it. It would make the symptom disappear immediately. + +It is wrong twice over. + +First, it is a lie about provenance. The binary now sitting at that path is the +version manager's newly installed `codex`, not the original OpenCodex wrapped. +Recording it as `.opencodex-real` asserts a history that did not happen. + +Second, it does not survive. The next `mise upgrade codex` rewrites the same +install tree and destroys shim and backup again. The fix would re-arm itself +every upgrade, so the operator gets a repair that silently un-repairs on a +schedule — the worst possible failure shape, because it looks solved. + +The install tree belongs to the version manager. OpenCodex should not be +installing files into it, and the supported route for these users is +`openai_base_url` injection plus `ocx service install`, which is what +`ocx start` already configures. + +So: detect, report, document. Never adopt. + +## Hunk 1 — the ownership heuristic + +`src/codex/shim.ts`, exported for direct unit tests: + +```ts +export function isVersionManagerOwnedCodexPath(path: string): boolean { + const n = path.replace(/\\/g, "/").toLowerCase(); + return n.includes("/mise/installs/") || n.includes("/mise/shims/") + || n.includes("/.asdf/installs/") || n.includes("/.asdf/shims/") + || n.includes("/.volta/"); +} +``` + +Backslash normalization is for Windows, where volta is common. Scope is the +three managers named in #2412; nvm/fnm/npm-prefix are deliberately excluded +until someone reports them, because a false positive here refuses a repair that +would otherwise be correct. + +## Hunk 2 — carry a message, and refuse VM-owned adoption + +`src/codex/shim.ts:2043`, before: + +```ts +if (!existsSync(file.wrapperPath) || !hasUsableBackingPath(file)) return { status: "ineligible" }; +``` + +After: compute `vmOwned` across wrapper/original/backup paths, include it in the +bail condition, and attach a message built from +`diagnoseCodexShim().summary` — the string `ocx codex-shim status` already +prints — plus, when `vmOwned`, this guidance: + +> This Codex binary is owned by a version manager (mise/asdf/volta). OpenCodex +> will not wrap it as a new original, because the next upgrade would overwrite +> the shim and its backup again. Keep routing through Codex `openai_base_url` +> (`ocx start`) and use `ocx service install` for autostart. + +The replacement path at `:2076` needs the same guard. If a stale +`.opencodex-real` happens to survive an upgrade, the existing code would +cheerfully re-wrap the new version-manager binary — the adoption this phase +forbids, arriving through the back door. + +## Hunk 3 — no CLI changes needed for start/ensure/repair + +This is the satisfying part. `src/cli/codex-shim-autorestore.ts:35` already +warns on an ineligible result **if it carries a message**: + +```ts +} else if ((result.status === "deferred" || result.status === "ineligible") && result.message) { + deps.warn(`⚠️ ${result.message}`); +} +``` + +and `src/cli/root.ts:83` runs that preflight before every command except +uninstall and `codex-shim install`. So attaching the message lights up +`ocx start`, `ocx ensure`, `ocx service repair`, and `ocx status` at once. +The mechanism was built correctly; one field was missing. + +## Hunk 4 — docs + +`docs-site/src/content/docs/reference/cli/lifecycle.md`, after the paragraph at +~`:357` promising that a completed Codex update restores the shim. That promise +is false for version-manager installs, and leaving it unqualified is how someone +concludes OpenCodex is broken rather than unsupported here. State plainly: the +install tree is not a supported shim target, upgrades destroy shim and backup, +and the supported configuration is service + `openai_base_url`. + +English is authoritative; translated locales must not keep promising restore for +this case. + +## What must NOT change + +- Healthy shims stay `{ status: "healthy" }` on the zero-overhead path + (`:2058`), including version-manager-owned ones that are currently intact. + Detection gates repair, not operation. +- Non-VM overwrite with a surviving backup still auto-restores and still warns + "automatic repair after Codex update". +- `allowFreshInstall: false`. The never-fresh-install rule at `:1887` is the + invariant this phase reinforces, not one it relaxes. +- `repairService()` semantics. It reports on the background service, and that + report is accurate; the shim warning arrives from the preflight instead. +- The first-line proxy badge. That is #2411's territory. + +## Regression tests + +| Test | File | Assertion | Fails before? | +|------|------|-----------|---------------| +| `version-manager overwrite with missing backup is ineligible and names the paths` | `tests/codex-shim.test.ts` | `ineligible` **with** a message naming wrapper state, missing backup, and the version manager; wrapper bytes unchanged | **Yes** — message is undefined | +| `version-manager-owned replacement is not adopted as a new original` | `tests/codex-shim.test.ts` | backup present but VM-owned path → ineligible; wrapper, backup, and state bytes all unchanged | **Yes** — today this restores | +| `ineligible destroyed shim warns on ordinary commands` | `tests/codex-shim-autorestore.test.ts` | one `⚠️` containing the diagnostic | **Yes** | +| `isVersionManagerOwnedCodexPath classifies known trees` | `tests/codex-shim.test.ts` | mise/asdf/volta true; `/usr/local/bin/codex`, `~/.npm-global/bin/codex` false | **Yes** — helper absent | + +`tests/codex-shim.test.ts:1921` (`missing backup, missing wrapper, corrupt +state, and platform mismatch never fresh-install`) asserts only on `status`, so +adding a message does not break it — and it is the test that would catch an +adoption regression. + +## Acceptance + +`bun test tests/codex-shim.test.ts tests/codex-shim-autorestore.test.ts tests/codex-shim-readiness.test.ts` +green; `bun x tsc --noEmit` exit 0; `bun run privacy:scan` pass; docs build not +required for a Markdown-only change but the page must render in review. + +## Open question for review + +Explicit `ocx codex-shim install` against a version-manager-owned PATH: +warn-and-allow, or refuse outright? Auto-restore must refuse — that is settled +above and is what this issue asks for. An explicit operator command is a +different act. Recommendation: warn, allow, and let the operator own it; a hard +refusal removes a workaround someone may be relying on. This does not block the +phase either way. From 3bf4b95ee0ec78dcfb65b3e14f2b879963f09533 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 25 Aug 2026 11:47:32 +0900 Subject: [PATCH 2/2] fix(cli): report Codex routing in ocx status and name an unused proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ocx status greened on process liveness alone, so a proxy answering /healthz read healthy even when ~/.codex/config.toml had no openai_base_url and every routed request went to OpenAI instead. The next line made it worse: native routing is genuinely rebootSafe, so restart safety printed 'no opencodex restart dependency' — a second all-clear for a broken setup. ocx doctor already knew, and printed routing=native, but status is the command people run first. status now prints the routing token doctor prints, and warns when a live proxy is paired with native routing. The proxy line keeps its check. That claim is narrow and true: the listener is up, which the reporter proved by curling it successfully. Turning it yellow would make the one honest signal lie to compensate for a missing one, and it would collide with the not-running branch whose remedy ('ocx start') is wrong here — the proxy does not need restarting, Codex needs re-pointing. Only native warns. custom-local and unknown are equally unused, but startupHealthSummary already renders both as AT RISK with a remedy, and custom-remote is a deliberate choice; warning on all four would train operators to ignore the line that matters. All five routing kinds are pinned in the test. The routing detail is extracted into formatStartupRoutingDetail so status and doctor cannot drift. Two callers computing the same string separately is exactly how #2457 happened. No routing decision, classifier, or restart-safety verdict changes. Closes #2411 --- src/cli/doctor.ts | 4 ++-- src/cli/index.ts | 11 +++++++++-- src/cli/status.ts | 23 ++++++++++++++++++++++ src/codex/autostart-health.ts | 16 +++++++++++++++ tests/autostart-health.test.ts | 36 +++++++++++++++++++++++++++++++++- tests/cli-help.test.ts | 5 +++++ 6 files changed, 90 insertions(+), 5 deletions(-) diff --git a/src/cli/doctor.ts b/src/cli/doctor.ts index 72a9e6202f9..aaadb343559 100644 --- a/src/cli/doctor.ts +++ b/src/cli/doctor.ts @@ -42,7 +42,7 @@ import { resolveEffectiveUserIdentity, } from "../codex/user-identity"; import { collectProjectCodexConfigWarnings, formatProjectCodexConfigWarningsForDoctor } from "../codex/project-config-warnings"; -import { collectStartupHealth, startupHealthSummary } from "../codex/autostart-health"; +import { collectStartupHealth, formatStartupRoutingDetail, startupHealthSummary } from "../codex/autostart-health"; import { displayCodexRuntimePath, loadLastEffortClamp, @@ -983,7 +983,7 @@ export async function runDoctor(args: string[] = []): Promise { const startup = collectStartupHealth(doctorConfig); console.log("\nCodex restart safety"); console.log(` ${startup.rebootSafe ? "ok " : "!! "} ${startupHealthSummary(startup)}`); - console.log(` routing=${startup.routingKind}, service=${startup.serviceViable ? "viable" : startup.serviceInstalled ? "installed-but-unhealthy" : "absent"}, shim=${startup.shimHealthy ? "healthy" : startup.shimInstalled ? "stale" : "absent"}`); + console.log(` ${formatStartupRoutingDetail(startup)}`); console.log("\nCodex runtime selection"); { diff --git a/src/cli/index.ts b/src/cli/index.ts index 57d5c85c532..85d4b61a94e 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -25,7 +25,7 @@ import { writePid, writeRuntimePort, } from "../config/process-state"; -import { collectStatus } from "./status"; +import { collectStatus, unusedProxyWarningLines } from "./status"; import { discoverStableProxyForRestart, @@ -46,7 +46,7 @@ import { runCli } from "./root"; import { ProxyOwnershipRefusedError, stopProxy } from "../lib/process-control"; import { loadServiceTokenFromFile } from "../lib/service-secrets"; import { diagnoseService, isServiceOwnershipError, serviceCommand, serviceEnvironmentOwnedHere, serviceStartableFromTray, serviceStatusSummary, stopServiceIfInstalled, uninstallServiceIfInstalled } from "../service"; -import { startupHealthSummary } from "../codex/autostart-health"; +import { formatStartupRoutingDetail, startupHealthSummary } from "../codex/autostart-health"; import { drainAndShutdown, isRecyclingForExit, startServer } from "../server"; import { injectSystemEnv, reconcileShellHook, revertSystemEnv, uninstallShellHook } from "../server/system-env"; import { buildDesktop3pRegistry } from "../claude/desktop-3p"; @@ -848,6 +848,12 @@ async function handleStatus() { console.log(`❌ Proxy: ${status.proxyLabel}`); } console.log(` Health: ${status.healthLabel}`); + for (const line of unusedProxyWarningLines({ + proxyUp: Boolean(status.json.proxy.pid || status.json.proxy.health.ok), + routingKind: status.json.startup.routingKind, + })) { + console.log(` ${line}`); + } if (!(status.json.proxy.pid || status.json.proxy.health.ok)) { console.log(" ↳ Not running — Codex/Claude requests will fail with connection errors."); // The service summary a few lines below already tells a registered-but-not-serving @@ -867,6 +873,7 @@ async function handleStatus() { console.log(` Default provider: ${status.json.defaultProvider}`); console.log(` Codex autostart: ${status.json.codexAutostart ? "enabled" : "disabled"}`); console.log(` Restart safety: ${startupHealthSummary(status.json.startup)}`); + console.log(` ${formatStartupRoutingDetail(status.json.startup)}`); console.log(` Service: ${status.json.service.summary}`); console.log(` ${status.json.codexShim.summary}`); console.log(` Codex runtime: ${status.json.codexRuntime.path}`); diff --git a/src/cli/status.ts b/src/cli/status.ts index 9c9f577e0e9..1ba11f96a33 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -118,6 +118,29 @@ export function proxyHealthFailureReason(error: unknown, signal: AbortSignal): " : "unreachable"; } +/** + * `ocx status` greens on process liveness alone, so a proxy that answers + * /healthz reads healthy even when Codex is not pointed at it and every routed + * request goes to OpenAI instead (#2411). The proxy line is not wrong — the + * listener really is up — so it keeps its check, and this supplies the signal + * that was missing rather than corrupting the one that was already honest. + * + * Only `native` warns. `custom-local` and `unknown` are also "this proxy is + * unused", but startupHealthSummary already renders both as AT RISK with a + * remedy command, and `custom-remote` is a deliberate operator choice. Warning + * on all four would teach operators to skip the line that matters. + */ +export function unusedProxyWarningLines(input: { + proxyUp: boolean; + routingKind: StartupHealth["routingKind"]; +}): string[] { + if (!input.proxyUp || input.routingKind !== "native") return []; + return [ + "⚠️ Codex routing is native — the running proxy is unused.", + " Codex requests go to OpenAI, not this proxy. Re-point with: ocx start", + ]; +} + async function checkProxyHealth(target: ListenTarget): Promise { const url = target.healthUrl; const controller = new AbortController(); diff --git a/src/codex/autostart-health.ts b/src/codex/autostart-health.ts index b3496696e17..95df491ac3f 100644 --- a/src/codex/autostart-health.ts +++ b/src/codex/autostart-health.ts @@ -154,3 +154,19 @@ export function startupHealthSummary(health: StartupHealth): string { if (health.serviceInstalled && !health.serviceViable) return `AT RISK after restart (installed service is disabled, stopped, or unhealthy; run '${command}')`; return `AT RISK after restart (no viable background service; run '${command}')`; } + +/** + * The routing/service/shim token `ocx doctor` prints under restart safety. + * Extracted so `ocx status` can show the same string rather than growing a + * second copy that drifts (#2411). Two management routes computing the same + * thing separately is exactly how #2457 happened. + */ +export function formatStartupRoutingDetail(health: StartupHealth): string { + const service = health.serviceViable + ? "viable" + : health.serviceInstalled ? "installed-but-unhealthy" : "absent"; + const shim = health.shimHealthy + ? "healthy" + : health.shimInstalled ? "stale" : "absent"; + return `routing=${health.routingKind}, service=${service}, shim=${shim}`; +} diff --git a/tests/autostart-health.test.ts b/tests/autostart-health.test.ts index 157f1e95abb..c3fe6aa9911 100644 --- a/tests/autostart-health.test.ts +++ b/tests/autostart-health.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; -import { deriveStartupHealth, startupHealthSummary } from "../src/codex/autostart-health"; +import { deriveStartupHealth, formatStartupRoutingDetail, startupHealthSummary } from "../src/codex/autostart-health"; +import { unusedProxyWarningLines } from "../src/cli/status"; import { classifyCodexRouting, hasInjectedCodexRouting } from "../src/codex/inject"; import { handleManagementAPI } from "../src/server/management-api"; import { invalidateStartupHealthCache, markStartupHealthDiagnosticStale } from "../src/server/startup-health-cache"; @@ -231,3 +232,36 @@ describe("Codex startup health", () => { }, 40_000); }); import { ManagementRequest as Request } from "./helpers/management-auth"; + +describe("routing visibility (#2411)", () => { + test("formatStartupRoutingDetail renders the token doctor already prints", () => { + expect(formatStartupRoutingDetail(deriveStartupHealth({ ...base, routingKind: "native" }))) + .toBe("routing=native, service=absent, shim=absent"); + expect(formatStartupRoutingDetail(deriveStartupHealth({ + ...base, + serviceInstalled: true, + serviceViable: true, + shimInstalled: true, + shimHealthy: true, + }))).toBe("routing=opencodex-local, service=viable, shim=healthy"); + expect(formatStartupRoutingDetail(deriveStartupHealth({ ...base, serviceInstalled: true }))) + .toBe("routing=opencodex-local, service=installed-but-unhealthy, shim=absent"); + expect(formatStartupRoutingDetail(deriveStartupHealth({ ...base, shimInstalled: true }))) + .toBe("routing=opencodex-local, service=absent, shim=stale"); + }); + + // A healthy proxy paired with native routing is the state #2411 reports: the + // process answers /healthz truthfully while no Codex request reaches it. + // custom-local and unknown stay silent on purpose — startupHealthSummary + // already renders both as AT RISK with a remedy, so a second warning would + // train operators to ignore this one. + test("unusedProxyWarningLines fires only for a live proxy on native routing", () => { + expect(unusedProxyWarningLines({ proxyUp: true, routingKind: "native" }).length).toBeGreaterThan(0); + expect(unusedProxyWarningLines({ proxyUp: true, routingKind: "native" }).join(" ")).toContain("unused"); + expect(unusedProxyWarningLines({ proxyUp: false, routingKind: "native" })).toEqual([]); + expect(unusedProxyWarningLines({ proxyUp: true, routingKind: "opencodex-local" })).toEqual([]); + expect(unusedProxyWarningLines({ proxyUp: true, routingKind: "custom-remote" })).toEqual([]); + expect(unusedProxyWarningLines({ proxyUp: true, routingKind: "custom-local" })).toEqual([]); + expect(unusedProxyWarningLines({ proxyUp: true, routingKind: "unknown" })).toEqual([]); + }); +}); diff --git a/tests/cli-help.test.ts b/tests/cli-help.test.ts index 4fa55e2da1c..1edfb67a992 100644 --- a/tests/cli-help.test.ts +++ b/tests/cli-help.test.ts @@ -174,6 +174,11 @@ describe("CLI subcommand help", () => { expect(result.stdout).toContain("Service:"); expect(result.stdout).toContain(join(opencodexHome, "service.log")); expect(result.stdout).toContain("Codex autostart shim"); + // #2411: status must name the routing kind it already computes. The + // proxy is down in this fixture, so the unused-proxy warning must stay + // quiet — that warning is for a LIVE proxy nothing routes through. + expect(result.stdout).toContain("routing="); + expect(result.stdout).not.toContain("the running proxy is unused"); } finally { rmSync(opencodexHome, { recursive: true, force: true }); }