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.