From b105648a874affa515c9e91e5d45284c7a7594c5 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 17 Sep 2026 17:54:11 +0900 Subject: [PATCH 1/5] docs(devlog): plan safe teardown and honest settings application [skip ci] Opens unit 260917_l2_safe_teardown for issues #4812 and #4809. #4812: the Codex history preflight answers "may I rewrite conversation history?" and four restore call sites use that answer to decide "may I remove OpenCodex routing from config.toml?". Since the refusal triggers on the mere presence of a history_mode column, which every current Codex build has, the config half is permanently unreachable and uninstall can remove the proxy while leaving routing pointed at it. #4809: the two Desktop switches persist and report success without rewriting the injected config.toml, and report the configured value rather than the effective one on a bind where the flag is dropped. The plan records the upstream provider-resolution facts that make a degraded "strip root routing, retain the provider table" restore safe, and that make the opposite ordering fatal to config load. No product code changes here. --- .../000_master_plan.md | 132 ++++++++++++++ .../010_upstream_resolution_facts.md | 99 +++++++++++ .../020_issue_4812_contract.md | 161 ++++++++++++++++++ .../030_issue_4809_contract.md | 136 +++++++++++++++ .../040_verification.md | 130 ++++++++++++++ 5 files changed, 658 insertions(+) create mode 100644 devlog/_plan/260917_l2_safe_teardown/000_master_plan.md create mode 100644 devlog/_plan/260917_l2_safe_teardown/010_upstream_resolution_facts.md create mode 100644 devlog/_plan/260917_l2_safe_teardown/020_issue_4812_contract.md create mode 100644 devlog/_plan/260917_l2_safe_teardown/030_issue_4809_contract.md create mode 100644 devlog/_plan/260917_l2_safe_teardown/040_verification.md diff --git a/devlog/_plan/260917_l2_safe_teardown/000_master_plan.md b/devlog/_plan/260917_l2_safe_teardown/000_master_plan.md new file mode 100644 index 0000000000..1c39d9be97 --- /dev/null +++ b/devlog/_plan/260917_l2_safe_teardown/000_master_plan.md @@ -0,0 +1,132 @@ +# 000 — Safe teardown and honest settings application + +- Unit: `260917_l2_safe_teardown` +- Opened 2026-09-17 +- Base: `origin/dev` = `f1dfda8e48` +- Issues: #4812 (parent), #4809 (child) +- Class C4 — writes `$CODEX_HOME/config.toml`, decides teardown outcome, and + touches the conversation-history safety boundary. + +## Objective + +Close the question "can a user turn OpenCodex off and get their original +environment back?" Today they cannot, in two different ways. + +#4812 is a deadlock. The Codex history preflight refuses the whole config +restore whenever the Codex state store has a `history_mode` column, which +every current Codex build has. `ocx restore`, `ocx stop`, and `ocx uninstall` +all funnel through that refusal, so the proxy can be removed while +`~/.codex/config.toml` still routes at `127.0.0.1:10100`. The guard exists to +keep `opencodex`-tagged threads resolvable, but once the proxy is gone those +threads fail at request time anyway — and now every native Codex invocation +fails too. The protection protects nothing and costs everything. + +#4809 is a configuration lie. `--desktop-authless` and `--client-compaction` +persist to `config.json` and report success, but the injected +`~/.codex/config.toml` does not change until a separate `ocx sync`. On a +non-loopback bind the authless flag is silently dropped altogether while the +API still reads back `true`. Nothing tells the user that the flag also moves +the auth source — whether the Codex app presents `~/.codex/auth.json`. + +Both are the same underlying defect: **the stored value, the effective value, +and the work still owed are collapsed into one answer.** This unit separates +them. + +## Delivery shape + +Two stacked pull requests, in order: + +| PR | Issue | Branch | Base | +|---|---|---|---| +| parent | #4812 | `codex/restore-routing-without-history` | `dev` | +| child | #4809 | `codex/settings-apply-and-effective-state` | the parent's head branch | + +`enforce-target` admits a stacked child whose base is an open parent's head +branch. The child is retargeted to `dev` after the parent squash-merges; the +host directs that step, not this lane. + +## Constraints + +- **No local verification of any kind.** `bun test` (in any form), + `bun run test`, `bun run test:changed`, `bun run typecheck`, `bun x tsc`, + `bun install`, `bun run build:gui`, and running `ocx` are all forbidden for + this unit. A local suite has previously deleted a real `~/.opencodex`. + Verification is static reasoning plus hosted CI at the exact head. +- Pushes use `git push --no-verify`; the pre-push hook runs the local suite. +- This lane never merges, never pushes to `dev`, and never rebases unasked. + The lane ends at "PR open with exact-head CI evidence". +- No flake management. No widened timeouts, added retries, platform skips, or + masking. The Windows job is dispatch-only; a Windows-affecting change is + reported to the host rather than dispatched here. +- **Paginated rollout bytes and thread rows stay untouched.** The native + writer remains the only writer of that shape. Nothing in this unit relaxes + `history_paginated_requires_native_writer` as a guard on *history*. +- Repository artifacts — commits, PR bodies, issues, reviews, these docs — are + English. Security analysis that is not already public goes to `.tmp/`, never + here. + +## Work-phase map + +| wp | Doc | Output | +|---|---|---| +| wp0 | this file | objective, topology, completion criteria | +| wp1 | `010_upstream_resolution_facts.md` | what codex-rs actually does with a provider id, and what that forces | +| wp2 | `020_issue_4812_contract.md` | the degraded-restore contract and its seam | +| wp3 | `030_issue_4809_contract.md` | stored / effective / pending separation for the two switches | +| wp4 | `040_verification.md` | static-proof obligations and hosted-CI evidence plan | + +## Completion criteria + +Shared across both pull requests: + +1. Every surface that reports one of these settings distinguishes three + things: the **stored** value, the **effective** value actually in force, + and whether **further action** is required to reconcile them. +2. Repeating `restore`, `stop`, and `uninstall` in any order never damages + user-owned configuration and never mutates Codex conversation history. +3. A partial outcome is never reported as full success, and no path ends with + a failed restore that leaves the client pointed at a dead address. + +Parent (#4812): + +- `history_paginated_requires_native_writer` no longer refuses the config + half of a restore. It selects a **degraded restore**: OpenCodex-owned root + routing comes out, `[model_providers.opencodex]` stays, history is skipped + rather than attempted. +- Every other preflight reason keeps its hard refusal and its compensating + rollback, unchanged. +- `ocx restore --remove-codex-provider-table` performs the full removal for a + user who accepts that `opencodex`-tagged threads stop opening. It is never + the default, and it states the consequence before acting. +- `ocx uninstall` on a paginated home completes with native Codex working, + names the retained table and the exact lines, and exits 0. It no longer + records the config restore as a failure that blocks local-state cleanup. +- `ocx status` reports retained-table residue instead of leaving it invisible. + +Child (#4809): + +- Flipping either switch through the settings API or the CLI applies the + injected `config.toml` inline when the proxy is live and the integration is + enabled; the response says whether it applied and, if not, exactly why. +- The response reports the **effective** `codexDesktopAuthless`, not only the + configured one, with the reason when the two differ. +- Both surfaces state the auth-source consequence — whether the Codex app will + present `~/.codex/auth.json` — at the moment of the change. +- The stale comment at `src/server/management/config-routes.ts:600-601`, which + asserts the opposite of what the code does, is corrected. + +## Terminal outcomes + +- **DONE** — both PRs open, exact-head CI recorded, criteria above hold. +- **BLOCKED** — recorded here with the blocking evidence; the lane does not + work around a gate by weakening it. + +## Prior art in this repository + +`devlog/_plan/260914_codex_history_preflight_scope/` narrowed the same guard +on the apply direction and explicitly left this open: + +> "The uninstall deadlock on an already-paginated home remains open follow-up; +> a later fix needs a keep-the-table seam on the restore path." + +That is what `020` specifies. diff --git a/devlog/_plan/260917_l2_safe_teardown/010_upstream_resolution_facts.md b/devlog/_plan/260917_l2_safe_teardown/010_upstream_resolution_facts.md new file mode 100644 index 0000000000..db2f0aa03a --- /dev/null +++ b/devlog/_plan/260917_l2_safe_teardown/010_upstream_resolution_facts.md @@ -0,0 +1,99 @@ +# 010 — What codex-rs actually does with a provider id + +The parent design rests on one upstream claim: keeping +`[model_providers.opencodex]` on disk while removing the root routing keys +leaves native Codex working *and* leaves `opencodex`-tagged threads loadable. +That claim is checkable, and checking it also rules out the obvious +alternative orderings. Evidence below is from the Codex upstream corpus at +`/Users/jun/Developer/codex/_raw/repos/121_openai-codex/codex-rs`. + +## Provider resolution is a whole-config concern, not a per-request one + +`Config::load` builds the provider map and then resolves exactly one id: + +```rust +let model_providers = + merge_configured_model_providers(built_in_model_providers(openai_base_url), cfg.model_providers) + .map_err(...)?; + +let model_provider_id = model_provider + .or(cfg.model_provider) + .unwrap_or_else(|| "openai".to_string()); +let model_provider = model_providers + .get(&model_provider_id) + .ok_or_else(|| { + ... + format!("Model provider \`{model_provider_id}\` not found") + std::io::Error::new(std::io::ErrorKind::NotFound, message) + })? + .clone(); +``` + +`core/src/config/mod.rs:3732-3749` + +Three consequences follow directly, and they decide the whole contract. + +**A missing provider id is fatal to config load, not to one request.** The +`?` propagates a `NotFound` out of `Config::load`. So removing the provider +table while root `model_provider = "opencodex"` survives does not degrade +anything — it breaks every single `codex` invocation with +`Model provider \`opencodex\` not found`, which is strictly worse than the +connection error #4812 reports. **The two removals can never be split in that +direction.** The degraded write must therefore be a single atomic +transformation, never a strip followed by a re-add. + +**Root `openai_base_url` rewrites the built-in provider.** +`built_in_model_providers(openai_base_url)` constructs the `openai` provider +from that value (`model-provider-info/src/lib.rs:512-527`), so an injected +`openai_base_url` pointing at a dead proxy breaks native Codex even when no +OpenCodex provider table exists at all. Removing it is not optional; it is the +single most load-bearing part of the degraded restore. + +**The default is `openai` when no root selector is present.** Dropping root +`model_provider` is sufficient to return the home to native operation. No +positive rewrite is needed. + +## A resumed thread supplies its own provider override + +```rust +typesafe_overrides.model_provider = Some(persisted_metadata.model_provider.clone()); +``` + +`app-server/src/request_processors/thread_processor.rs:234` + +That override is the `model_provider` argument in the resolution above, so a +thread row tagged `opencodex` needs a map entry named exactly `opencodex`. +With the table retained the resume succeeds and only that thread's requests +fail, against a dead port, with an ordinary connection error. With the table +removed the resume fails at config load. + +This matches what `src/codex/inject.ts:490-495` already asserts on the apply +side — "Rows this home may have tagged `opencodex` resolve only through a +provider table" — and it is why the injector re-appends an existing table +before building its write witness (`src/codex/inject.ts:496-502`). The restore +direction is getting the same seam, for the same reason. + +## `requires_openai_auth` is the auth source, visibly + +```rust +fn should_show_login_screen(login_status: LoginStatus, requires_openai_auth: bool) -> bool { + ... + if !requires_openai_auth { +``` + +`tui/src/lib.rs:2070-2073`, reached from `tui/src/lib.rs:1214-1233` + +The flag OpenCodex emits at `src/codex/inject/config-toml.ts:95` decides +whether Codex asks the user to sign in and whether it presents +`~/.codex/auth.json`. That is a user-visible identity change, which is why +#4809's requirement that the switch announce its auth-source consequence is a +correctness requirement rather than a cosmetic one. + +## Bounds of this evidence + +The corpus is a vendored snapshot, not the running binary on any particular +user's machine. What it establishes is the *shape* of resolution — override +beats root key beats `openai`, and a miss is fatal at load. The degraded +contract in `020` depends only on that shape, and it is conservative in the +one direction that matters: it never produces a config where a referenced +provider id is absent. diff --git a/devlog/_plan/260917_l2_safe_teardown/020_issue_4812_contract.md b/devlog/_plan/260917_l2_safe_teardown/020_issue_4812_contract.md new file mode 100644 index 0000000000..c1fe2e7d83 --- /dev/null +++ b/devlog/_plan/260917_l2_safe_teardown/020_issue_4812_contract.md @@ -0,0 +1,161 @@ +# 020 — Degraded restore: separate routing recovery from history + +Issue #4812. Parent PR, branch `codex/restore-routing-without-history`. + +## The defect, stated as a decision error + +`preflightCodexHistoryInjection` answers one question — "may I rewrite Codex +conversation history?" — and four call sites use that answer to decide a +different question: "may I take OpenCodex routing out of `config.toml`?" + +```ts +const historyError = preflightCodexHistoryInjection(false, false); +if (historyError) return { state: "failed", ... }; +``` + +`src/codex/inject/restore.ts:260-261`, `:275-276`, `:371-372`, `:519-520`, and +`src/codex/inject/remove.ts:145-146` + +Because `assertLegacyHistoryStore` refuses on the mere *presence* of a +`history_mode` column (`src/codex/history-provider.ts:397-400`), and every +current Codex build has that column, the answer is permanently no. The config +half is therefore permanently unreachable, and `ocx uninstall` removes the +proxy while leaving the routing that points at it. + +The fix is not to weaken the guard. The guard is right about history. It is +being asked the wrong question. + +## Contract + +### Two classes of owned state + +**Routing state** — makes *every* `codex` invocation go through the proxy: +marker-owned root `openai_base_url` and `experimental_realtime_ws_base_url`, +root `model_provider = "opencodex"`, a routed root `model`, an OpenCodex +`model_catalog_json`, `[profiles.opencodex]` and the generated profile file, +and the managed subagent defaults. + +**Thread-resolution state** — `[model_providers.opencodex]` and its +sub-tables. It affects nothing unless a thread row names that provider id. + +Routing state is what strands the user. Thread-resolution state is what the +history guard is protecting. They are separable, and `010` establishes that +separating them in this direction is safe while the opposite direction is +catastrophic. + +### The rule + +When `preflightCodexHistoryInjection` returns +`history_paginated_requires_native_writer` **and nothing else**, restore takes +the degraded path: + +- All routing state is removed, through the existing journal-or-strip logic, + unchanged. +- `[model_providers.opencodex]` is retained verbatim, including its ownership + marker. +- The history relabel is **skipped, not attempted**. No rollout byte, thread + row, or manifest entry is touched. The native writer stays the only writer. +- The outcome is reported as degraded, never as a plain success. + +Every other refusal reason — `history_injection_preflight_unavailable`, +`history_state_database_missing`, and the rollout-integrity codes — keeps the +existing hard refusal and its compensating rollback, byte for byte. This is +the exact asymmetry the apply direction already encodes as +`HISTORY_RELABEL_STANDS_DOWN` (`src/codex/inject.ts:472-484`); restore is +being brought into line with it, not given something new. + +### Atomicity + +`010` shows that a config containing root `model_provider = "opencodex"` +without a matching table fails `Config::load` outright. The degraded write is +therefore **one** `atomicWriteFile` of fully-computed content. The +implementation must not strip and then re-add as two writes, and must not +leave that combination reachable through an error path. + +The seam is a verbatim capture, taken before the transform and re-appended +into the same output buffer: + +- `extractOcxProviderTableBlock(content): string | null` — new pure function + in `src/codex/inject/remove.ts`, the exact inverse of the existing + `removeOcxSection` scan (`:50-80`), sharing `isOcxProviderHeaderLine` so the + two cannot drift on what counts as our table. +- `removeCodexConfig({ retainProviderTable })` re-appends the captured block + after the strip, before the single write. +- `restoreCodexConfigInlineImpl` captures the block from the on-disk config + **before** the journal restore, because an exact journal restore replays the + original pre-injection bytes and deletes the journal + (`src/codex/journal.ts:258-293`). After a successful journal restore the + captured block is re-appended inside the same lock and the same preimage + window. + +Verbatim capture, rather than rebuilding the table from the live routing +target, is deliberate. Rebuilding needs a port and a config that `uninstall` +is in the middle of removing, and it would silently change the retained +definition. Capture cannot. + +### Reported outcome + +`CodexRestoreArtifactState` gains `"partial"`. `CodexRestoreConfigResult.action` +gains `"routing-restored-provider-retained"`. The envelope gains: + +```ts +retainedCodexProviderTable?: { + reason: "history_paginated_requires_native_writer"; + /** Exact config.toml lines left on disk. */ + lines: string[]; + /** What to run to remove them, and what breaks if you do. */ + followUp: string; +}; +``` + +`success` stays `true`: the routing restore genuinely succeeded and the +dead-address trap is gone. The residue is a deliberate, named outcome rather +than a hidden failure, which is what completion criterion 1 asks for — +stored, effective, and still-owed are three separate fields, not one boolean. +A degraded restore that *fails* is still a failure and keeps today's handling. + +`historyPreflightRefusal` keeps its current meaning — "nothing was attempted +at all" — and must therefore **not** be set on the degraded path, because +`src/cli/index.ts:813-818` reads it together with three `skipped` artifacts to +decide that a stop obligation is still owed. A degraded restore discharged +the config obligation, so the receipt must be released, not preserved. + +### Caller obligations + +Report B found that seven of eight callers reduce the result to `.success`. +They keep working unchanged, which is the point of keeping `success: true`. +Three need real changes: + +- `ocx restore` (`src/cli/dispatch.ts:194-240`) prints the retained lines and + the follow-up command; `--json` carries the new field. +- `ocx stop` (`src/cli/index.ts:796-825`) must classify degraded as neither + `historyDeferred` nor `other`. The obligation was performed; exit stays `0` + and the receipt is discharged. +- `ocx uninstall` (`src/cli/index.ts:1357-1360`) no longer records a failed + step, so `failures` stays empty and `~/.opencodex` is removed + (`:1393-1410`). It prints the retained lines. This is the concrete end of + the trap: uninstall completes, native Codex works, and the user is told + exactly what is left and why. + +### Full removal, on request + +`ocx restore --remove-codex-provider-table` strips the table too. It states +before acting that `opencodex`-tagged threads will stop opening, and it is +never implied, never defaulted, and never selected by `stop` or `uninstall`. + +### `ocx status` + +A config with `[model_providers.opencodex]` but no OpenCodex root routing is +retained residue, and status says so, with the removal command. Report B +confirms status has no such line today +(`src/codex/inject/routing-classify.ts:55-107` classifies endpoint ownership +only), so residue is currently invisible. + +## What this does not do + +- It does not make `opencodex`-tagged threads work after teardown. They point + at a proxy that is gone. They open, and their requests fail with an ordinary + connection error instead of a config-load error. +- It does not touch conversation history on a paginated home, ever. +- It does not add a `--force` that bypasses the history guard. There is no + such flag, because there is no safe version of it. diff --git a/devlog/_plan/260917_l2_safe_teardown/030_issue_4809_contract.md b/devlog/_plan/260917_l2_safe_teardown/030_issue_4809_contract.md new file mode 100644 index 0000000000..209661e46b --- /dev/null +++ b/devlog/_plan/260917_l2_safe_teardown/030_issue_4809_contract.md @@ -0,0 +1,136 @@ +# 030 — Stored, effective, and pending for the two Desktop switches + +Issue #4809. Child PR, branch `codex/settings-apply-and-effective-state`, +based on the parent's head branch. + +## The defect + +`PUT /api/settings` persists `codexDesktopAuthless` and +`codexClientCompaction` and then converges the **catalog**: + +```ts +// Both Desktop compatibility switches change the injected config.toml shape, so converge now +// rather than waiting for the next start; the injector re-reads config and rewrites the form. +... ? await convergeCodexCatalog() : undefined; +``` + +`src/server/management/config-routes.ts:600-607` + +The comment is false. `convergeCodexCatalog` rejects any request whose +`scope !== "catalog"` (`src/codex/convergence.ts:668`, +`src/codex/management-convergence.ts:139`) and never calls +`injectCodexConfig`. The injector is reached only from `syncModelsToCodex` +(`src/codex/sync.ts:207,273`), `ocx init`, and the connect paths. So +`~/.codex/config.toml` keeps its old shape until a separate `ocx sync`. + +The CLI compounds it by discarding the response body entirely and printing a +fixed string (`src/cli/system-command.ts:59-65`): + +```text +System settings updated. +``` + +And there is a second, quieter failure. On a non-loopback bind without +`unauthenticatedLoopbackListener`, `standaloneCodexRoutingTarget` drops the +flag (`src/codex/inject/routing-target.ts:63`), yet both GET and PUT report +the configured `true` (`config-routes.ts:328,623`). The user reads back the +value they set and gets the behaviour they did not. + +## Contract + +### Three fields, not one + +For each of the two switches the settings response reports: + +| Field | Meaning | +|---|---| +| `stored` | what is persisted in `config.json` | +| `effective` | what the injector would actually apply on this bind and role | +| `applied` | whether `~/.codex/config.toml` now reflects it | + +`effective` uses the predicate Report C extracted from +`src/codex/loopback-target.ts:92`: + +```ts +codexDesktopAuthless === true + && runtimeRole !== "client" + && !shouldInjectApiAuthHeader(config) +``` + +When `stored !== effective`, the response carries the reason — +`non_loopback_bind_requires_admission_token` or `client_role` — and the CLI +prints it. A switch that is stored on and effectively off is the exact case +the issue calls a configuration lie, and it stops being one when the response +says so. + +### Apply inline + +When the integration is enabled and the proxy is live, the route runs the real +injection after persisting, and reports the result. Report C establishes this +is callable: `injectCodexConfig(port, config?, options?)` is async +(`src/codex/inject.ts:111,182`) and the handler is already async and already +dynamically imports the sync path for `/api/sync` +(`config-routes.ts:637`). + +Two ordering constraints are hard: + +- The save's config-mutation transaction (`C`) must be **closed** before + injection starts. Coordinated homes take `N -> C` + (`src/codex/codex-write-lock.ts:18,335`), so calling the injector from + inside `C` inverts the order. +- Config is re-read from disk for the injection rather than reusing the + server's startup object, matching what `/api/sync` already does. + +Failure modes are reported, never flattened into success: + +| Injector outcome | Response | +|---|---| +| write lock busy (`inject-coordination.ts:464`) | `applied: false`, `retryable: true`, "run `ocx sync`" | +| desired state off / hub-gated (`inject.ts:688`) | `applied: false`, reason `integration_disabled` | +| non-paginated history refusal (`inject.ts:442`) | `applied: false` with the reason string | +| external provider owns the home (`inject.ts:264`) | `applied: false`, reason preserved | +| proxy not live | `applied: false`, reason `proxy_not_running` | + +`history_paginated_requires_native_writer` is **not** a failure here: apply +already stands the relabel down and writes the config half +(`inject.ts:472-484`). A paginated home applies normally, which is what makes +this child coherent with the parent. + +`catalogRefreshPending` (`src/codex/catalog-refresh-status.ts:102`, +`config-routes.ts:609`) is the existing precedent for a +"this is not finished yet" field, and the new fields follow its shape rather +than inventing a second vocabulary. + +### State the auth-source consequence + +Flipping `codexDesktopAuthless` moves `requires_openai_auth` in the injected +table (`src/codex/inject/config-toml.ts:95`). `010` shows upstream reads that +flag to decide whether to show the login screen at all +(`tui/src/lib.rs:2070-2073`). Both the API response and the CLI state, at the +moment of the change, whether the Codex app will now present +`~/.codex/auth.json`. This is an identity-surface change and the user is told +while they are making it. + +### CLI output + +`ocx system settings` stops printing a fixed string. It prints the stored +value, the effective value when it differs and why, whether the injected +config was rewritten, and the auth-source consequence. `--json` passes the +response through, as it already does (`src/cli/runtime-api.ts:348`). + +### Correct the comment + +`config-routes.ts:600-601` is rewritten to describe what the code does. A +comment asserting the opposite of the behaviour is how the next maintainer +inherits this bug. + +## Scope boundaries + +- No change to what the switches *mean*. The injected shapes stay exactly as + `src/codex/inject.ts:335-365` produces them. +- No new setting, no schema migration, no GUI redesign. The GUI reads the same + response and is free to show the new fields later. +- `codexClientCompaction` gets the same three-field treatment. It has no + inert case of its own — Report C shows it is dropped only on + admission-required targets (`routing-target.ts:53`) — so its `effective` + differs from `stored` under that one condition and is reported the same way. diff --git a/devlog/_plan/260917_l2_safe_teardown/040_verification.md b/devlog/_plan/260917_l2_safe_teardown/040_verification.md new file mode 100644 index 0000000000..1afcb4049d --- /dev/null +++ b/devlog/_plan/260917_l2_safe_teardown/040_verification.md @@ -0,0 +1,130 @@ +# 040 — Verification obligations and evidence plan + +## The verification constraint, and what replaces local runs + +No local verification runs for this unit. Not `bun test` in any form, not +`bun run test:changed`, not `bun run typecheck`, not `bun x tsc`, not +`bun install`, not `bun run build:gui`, and not `ocx`. A local suite has +previously deleted a real `~/.opencodex`. + +That is a real loss of signal, so it has to be paid for twice: with static +obligations that are checkable by reading, and with hosted CI at the exact +head. Neither alone is sufficient, and neither is described here as if it +were. + +### Incident: the rule was broken during this unit's survey + +A read-only survey subagent built a shell command containing unescaped +backticks, which the shell executed as `ocx restore`. + +Observed state afterwards, by direct read: + +| Artifact | Evidence | +|---|---| +| `~/.codex/config.toml` | mtime 02:02, hours before the run; injected `openai_base_url`, `model_catalog_json` intact | +| `~/.opencodex/config.json` | mtime 17:44:56, before the subagent was spawned (~17:46:30) | +| `~/.opencodex/integrations/codex.json` | mtime Sep 16 11:07 | + +Nothing was written. The restore refused at the history preflight, which is +the behaviour #4812 is about, and the desired-state write was a no-op because +`clientIntegrations.codex` was already `false` and `setIntegrationEnabled` +returns `changed: false` in that case (`src/codex/desired-state.ts:167`). + +The pre-existing `clientIntegrations.codex: false` is the user's own state and +was not altered. It is recorded here because it is load-bearing for reading +any later observation of this machine, not because this unit touched it. + +Correction applied: delegated prompts must not embed backticks in shell +command strings, and read-only agents get an explicit prohibition on the +`ocx` binary rather than only on the test commands. + +## Static obligations + +These are the claims that would normally be a test run, and how each is +discharged by reading instead. + +**Atomicity of the degraded write.** `010` establishes that a config with root +`model_provider = "opencodex"` and no matching table fails `Config::load` +outright. Obligation: trace every path through +`removeCodexConfig({ retainProviderTable: true })` and confirm a single +`atomicWriteFile`, with the retained block already in the buffer. Any early +return between strip and append is a defect regardless of test outcome. + +**Capture/removal symmetry.** `extractOcxProviderTableBlock` and +`removeOcxSection` must agree on what our table is. Obligation: they share +`isOcxProviderHeaderLine` and the same scan shape, so a future change to one +cannot silently diverge from the other. + +**Refusal-reason asymmetry.** Only +`history_paginated_requires_native_writer` selects the degraded path. +Obligation: the comparison is against the existing +`HISTORY_RELABEL_STANDS_DOWN` constant, not a string literal, so the apply and +restore directions cannot drift apart. + +**Receipt semantics.** `historyPreflightRefusal` must stay unset on the +degraded path, because `src/cli/index.ts:813-818` reads it plus three +`skipped` artifacts to keep a stop obligation owed. Obligation: confirm the +degraded envelope reports config as `partial`, which fails that conjunction on +two counts. + +**Lock ordering in the child PR.** Coordinated homes take `N -> C` +(`src/codex/codex-write-lock.ts:18,335`). Obligation: the settings save's `C` +transaction is closed before `injectCodexConfig` is called; confirm by reading +the handler's control flow, not by inspecting a log. + +## Test changes owed + +Report A identified the assertions that pin the current refusal. Each needs to +move to the degraded contract, and each new file needs byte-identical entries +in both `scripts/test-layout/layout.json` (`explicit`) and +`tests/fixtures/test-layout-expected.json`. + +| File | What changes | +|---|---| +| `tests/codex-integration/codex-inject-integration.test.ts:128-143` | the all-skipped refusal envelope becomes the degraded envelope | +| `tests/codex-integration/codex-inject-integration.test.ts:468-493` | `action` union widened | +| `tests/codex-integration/codex-inject-integration.test.ts:525-542` | paginated row: refusal becomes degraded success with the table retained | +| `tests/codex-integration/codex-restore-app-rewrite.test.ts:223` | `action` assertion | +| `tests/service/stop-deferred-teardown.test.ts:128-173` | degraded must not be classified as deferred | +| `tests/cli/uninstall.test.ts:200-223` | degraded no longer produces a failed step | +| `tests/config/settings-stream-mode.test.ts:346-416` | child PR: the two switches assert applied/effective, not only persistence | + +New coverage owed, in the domain directory that matches: + +- A paginated home where restore removes root routing, keeps the table + verbatim including its marker, and leaves rollout bytes and thread rows + byte-identical. +- The ordering invariant: no reachable output has root + `model_provider = "opencodex"` without the table. +- `--remove-codex-provider-table` removes it and says what breaks. +- Repeated `restore` → `stop` → `uninstall` in sequence is idempotent and + damages neither user config nor history. +- Child: stored-on / effective-off on a non-loopback bind reports both values + and the reason. + +## Hosted CI evidence + +Both PRs record, at the exact head SHA: + +- the head SHA itself, +- direct check-runs for that SHA rather than an aggregate run that may be + stale or cancelled, +- the conclusion per job. + +`bun run structure:check` and `bun run privacy:scan` run in CI like everything +else. If a change touches an owned `src/` area, the matching `structure/` doc +is updated **in the same PR**, or `structure:check` fails and that failure is +the correct answer rather than something to route around. + +The Windows job is `workflow_dispatch`-only. If either PR plausibly affects +Windows — the config write path and EOL handling both do — the host is told so +it can dispatch. This lane does not dispatch it. + +## Honest limits of this evidence + +Hosted CI proves the suite's assertions hold on three platforms. It does not +prove the upstream claims in `010`, which come from a vendored corpus snapshot +rather than the Codex binary on any given machine, and it does not prove the +end-to-end recovery on a real paginated home — that needs a live +`ocx uninstall` followed by a working `codex`, which this lane is forbidden to +run. Both gaps are stated in the PR descriptions rather than papered over. From 4e107dbcccb67563c33686917fee6d04a1c18bf4 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 17 Sep 2026 18:13:49 +0900 Subject: [PATCH 2/5] fix(codex): restore routing without waiting on paginated history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #4812. preflightCodexHistoryInjection answers one question — may conversation history be rewritten — and four restore call sites used that answer to decide a different one: may OpenCodex routing come out of config.toml. The refusal triggers on the mere presence of a history_mode column, which every current Codex build has, so the config half was permanently unreachable. ocx uninstall removed the proxy and left config.toml pointed at it, and every subsequent codex invocation failed on a dead port with no supported recovery. The guard is right about history and is unchanged. It was being asked the wrong question. On history_paginated_requires_native_writer, and only that reason, restore now removes every OpenCodex root routing key, retains [model_providers.opencodex] verbatim including its ownership marker, and skips the history relabel rather than attempting it. Every other refusal reason keeps its hard refusal and compensating rollback untouched. Retention is what makes the split safe. In codex-rs, Config::load resolves model_provider_id as (thread override) ?? cfg.model_provider ?? "openai" and returns NotFound on a miss, which fails the entire config load rather than one thread; a resumed thread supplies its persisted provider as that override. Keeping the table therefore leaves tagged conversations openable — their requests fail against the stopped proxy with an ordinary connection error — while plain codex returns to the built-in provider. Removing the table while the root selector survived would break every codex invocation, so the table is captured before the transform and re-appended into the same buffer, reaching disk as one write. The capture cannot reuse removeOcxSection's scan. That one opens a section on any line carrying the ownership marker, which is safe only because the root openai_base_url marker has already been consumed by the time it runs. Capture reads the untouched file, so it anchors on the provider header instead and pulls in an immediately preceding marker as its comment. A stand-down can also appear during the write, after the pre-write check said otherwise. The table is therefore captured unconditionally and re-attached once after the write, and the history decision is re-asked at the same point so a Worker whose own preflight is now guaranteed to refuse is never spawned. Reported as artifacts.config.state "partial" with action routing-restored-provider-retained, carrying the retained lines and the follow-up command. historyPreflightRefusal stays unset: it still means nothing was attempted, and a stop obligation that was discharged must release its receipt. restore, stop, uninstall, the service subcommands, the stop API and status all name the retained table and the skipped relabel, so a partial outcome is never printed as a plain success. ocx restore --remove-codex-provider-table removes the table for a user who accepts that tagged conversations stop opening. Nothing selects it implicitly. Verification: hosted CI only. The local suite, typecheck, build and the ocx binary were deliberately not run for this change. --- src/cli/dispatch.ts | 21 +- src/cli/index.ts | 41 ++- src/codex/history-provider.ts | 21 +- src/codex/inject.ts | 15 +- src/codex/inject/remove.ts | 138 ++++++++- src/codex/inject/restore.ts | 233 +++++++++++++-- src/server/stop-teardown.ts | 9 +- src/service/cli.ts | 15 +- structure/codex-home.md | 10 +- structure/config.md | 21 +- tests/cli/uninstall.test.ts | 19 ++ .../codex-inject-integration.test.ts | 277 +++++++++++++++--- tests/codex-integration/codex-inject.test.ts | 34 +++ .../codex-restore-app-rewrite.test.ts | 2 +- tests/service/stop-deferred-teardown.test.ts | 81 ++++- 15 files changed, 828 insertions(+), 109 deletions(-) diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index 1f74ded2b7..fe32b199ea 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -24,7 +24,7 @@ import { } from "../codex/desired-state"; import { syncModelsToCodex } from "../codex/sync"; import { collectOrcaCodexHomeDiagnostic } from "../codex/home"; -import { restoreNativeCodexAsync } from "../codex/inject"; +import { restoreNativeCodexAsync, type CodexNativeRestoreResult } from "../codex/inject"; import { stripGrokConfig } from "../grok/inject"; import { handleRestartScopeAfterWrite, readRestartScope, type RestartScope } from "./restart-scope"; import { normalizeUpdateChannel, runGuiUpdateWorker } from "../update/job"; @@ -106,6 +106,7 @@ const commandRunners: Record = { restore: async deps => { const restoreArgs = deps.args.slice(1); const restoreJson = takeFlag(restoreArgs, "--json"); + const removeProviderTable = takeFlag(restoreArgs, "--remove-codex-provider-table"); if (restoreArgs[0] === "back") { // Reverse switch: re-point plain `codex` at the RUNNING proxy without touching its // lifecycle — the counterpart of `ocx restore`. Start/stop triggers are unchanged; @@ -146,6 +147,9 @@ const commandRunners: Record = { const target = collectOrcaCodexHomeDiagnostic(); return emitBack(true, `Plain \`codex\` now routes through opencodex in ${target.effectiveCodexHome} (undo with: ocx restore).`, 0); } + if (removeProviderTable && !restoreJson) { + console.log("⚠️ Removing [model_providers.opencodex] means conversations already tagged opencodex will stop opening."); + } const desired = setIntegrationEnabled("codex", false); if (!desired.ok) { if (restoreJson) { @@ -191,9 +195,9 @@ const commandRunners: Record = { return grokCode; } } - let r: { success: boolean; message: string }; + let r: CodexNativeRestoreResult | Pick; try { - r = await restoreNativeCodexAsync({ revalidateDesiredState: true }); + r = await restoreNativeCodexAsync({ revalidateDesiredState: true, removeProviderTable }); } catch (err) { r = { success: false, message: err instanceof Error ? err.message : String(err) }; } @@ -232,7 +236,16 @@ const commandRunners: Record = { code = 1; } if (r.success) { - console.log("Codex integration is OFF and plain `codex` now runs natively. Switch back with: ocx restore back"); + const retained = "retainedCodexProviderTable" in r ? r.retainedCodexProviderTable : undefined; + if (retained) { + console.log("Codex integration is OFF and plain `codex` now runs natively."); + console.log("The following lines remain in $CODEX_HOME/config.toml because conversations already tagged opencodex resolve their provider only through this table:"); + console.log(retained.lines.join("\n")); + console.log(`Follow-up: ${retained.followUp}`); + console.log("Switch back with: ocx restore back"); + } else { + console.log("Codex integration is OFF and plain `codex` now runs natively. Switch back with: ocx restore back"); + } console.log(`Note: ${OCX_NATIVE_REPLAY_RECOVERY_NOTE}`); } else { console.error("Plain `codex` was not fully restored. Inspect $CODEX_HOME/config.toml before using native Codex."); diff --git a/src/cli/index.ts b/src/cli/index.ts index 9df96b3fad..1cb1213c60 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -13,7 +13,15 @@ try { /* best-effort */ } } -import { currentExternalCodexModelProvider, restoreNativeCodex, restoreNativeCodexAsync, shouldInjectApiAuthHeader } from "../codex/inject"; +import { + currentExternalCodexModelProvider, + describeRetainedCodexProviderTable, + readOcxProviderTableBlock, + restoreNativeCodex, + restoreNativeCodexAsync, + shouldInjectApiAuthHeader, + type RetainedCodexProviderTable, +} from "../codex/inject"; import { stripGrokConfig } from "../grok/inject"; import { STOP_HISTORY_DEFERRED_EXIT_CODE, STOP_HISTORY_INCOMPLETE_EXIT_CODE } from "../update/stop-contract.mjs"; import { @@ -119,6 +127,12 @@ function reportShellHookFailure(result: { state: "installed" | "absent" | "faile console.warn(" Check ~/.zshrc for the '# opencodex claude-env hook' block."); } +function reportRetainedCodexProviderTable(retained: RetainedCodexProviderTable): void { + console.log(` ${describeRetainedCodexProviderTable(retained)}`); + console.log(" Retained config lines:"); + for (const line of retained.lines) console.log(` ${line}`); +} + async function refreshOwnedRaycastCatalog( config: ReturnType, port: number, @@ -799,7 +813,12 @@ async function restoreSharedClientStateAfterStop(): Promise<{ historyOnly: boole let other = false; try { const result = await restoreNativeCodexAsync(); - if (result.success) console.log(`↩️ ${result.message}`); + if (result.success) { + console.log(`↩️ ${result.message}`); + if (result.retainedCodexProviderTable) { + reportRetainedCodexProviderTable(result.retainedCodexProviderTable); + } + } else { // Codex history is the one restore whose failure leaves the runtime consistent: the // manifest is retained and the routed metadata is untouched. Config and catalog are @@ -810,6 +829,9 @@ async function restoreSharedClientStateAfterStop(): Promise<{ historyOnly: boole // attempted. Reading the states alone cannot tell that apart from an ownership // refusal, so the structured reason carries it and the states are still required to // agree — a refusal that somehow reports a failed artifact is not this case. + // A degraded restore has no refusal reason and reports config as partial, so it cannot + // enter this branch: its config obligation was discharged and the stop receipt must be + // released rather than preserved. const preflightRefused = result.historyPreflightRefusal !== undefined && artifacts.config.state === "skipped" && artifacts.catalog.state === "skipped" @@ -1357,6 +1379,9 @@ async function handleUninstall() { await runStep("native Codex restored", async () => { const r = await restoreNativeCodexAsync(); if (!r.success) throw new Error(r.message); + if (r.retainedCodexProviderTable) { + reportRetainedCodexProviderTable(r.retainedCodexProviderTable); + } }); await runStep("Grok Build config restored", () => { @@ -1503,6 +1528,18 @@ async function handleStatus() { console.log(` Codex autostart: ${status.json.codexAutostart ? "enabled" : "disabled"}${local}`); console.log(` Restart safety: ${startupHealthSummary(status.json.startup)}${local}`); console.log(` ${formatStartupRoutingDetail(status.json.startup)}${local}`); + if (status.json.startup.routingKind === "native") { + let retainedProviderTable = false; + try { + retainedProviderTable = readOcxProviderTableBlock() !== null; + } catch { + // The routing snapshot owns unreadable-config reporting. A later read race must not + // turn this diagnostic command into a teardown failure. + } + if (retainedProviderTable) { + console.log(` ⚠️ Codex provider table retained${local}: [model_providers.opencodex] remains while root routing is native. Remove with 'ocx restore --remove-codex-provider-table'; tagged conversations will stop opening.`); + } + } console.log(` Service: ${status.json.service.summary}${local}`); console.log(` ${status.json.codexShim.summary}${local}`); console.log(` Codex runtime: ${status.json.codexRuntime.path}${local}`); diff --git a/src/codex/history-provider.ts b/src/codex/history-provider.ts index 33f79da7ab..d013b31e40 100644 --- a/src/codex/history-provider.ts +++ b/src/codex/history-provider.ts @@ -327,6 +327,19 @@ class CodexHistoryIntegrityError extends Error { * O_APPEND does not allocate an ordinal or update that writer's in-memory cursor. * Refuse before changing the DB, manifest, or first-line provider; never guess N+1. */ +/** + * The one refusal reason that means "the native writer owns this history", as opposed + * to "something is wrong". It is a stand-down for the relabel unit on apply + * (`src/codex/inject.ts`) and for the history half of a restore; every other reason is + * a hard refusal in both directions. + * + * Exported as a constant rather than repeated as a literal because the apply and restore + * directions have to agree on it exactly. They drifted once already: apply learned to + * stand down while restore kept refusing, which is how #4812's uninstall deadlock + * survived the fix that was supposed to end it. + */ +export const HISTORY_RELABEL_STANDS_DOWN = "history_paginated_requires_native_writer"; + function assertLegacyHistoryRecord(line: string): void { let value: unknown; try { value = JSON.parse(line); } catch { throw new CodexHistoryIntegrityError("history_rollout_record_invalid"); } @@ -336,7 +349,7 @@ function assertLegacyHistoryRecord(line: string): void { const record = value as Record; const payload = record.payload; if (Object.hasOwn(record, "ordinal") || (payload !== null && typeof payload === "object" && (payload as Record).history_mode === "paginated")) { - throw new CodexHistoryIntegrityError("history_paginated_requires_native_writer"); + throw new CodexHistoryIntegrityError(HISTORY_RELABEL_STANDS_DOWN); } } @@ -397,7 +410,7 @@ function assertLegacyHistoryWritable(path: string, heldFd?: number): void { function assertLegacyHistoryStore(db: Database): void { const columns = db.query<{ name: string }, []>("PRAGMA table_info(threads)").all(); if (columns.some(column => column.name === "history_mode")) { - throw new CodexHistoryIntegrityError("history_paginated_requires_native_writer"); + throw new CodexHistoryIntegrityError(HISTORY_RELABEL_STANDS_DOWN); } } @@ -423,7 +436,7 @@ export function preflightCodexHistoryInjection( db = new Database(resolvedPath, { readonly: true }); const columns = db.query<{ name: string }, []>("PRAGMA table_info(threads)").all(); const paginatedColumn = columns.some(column => column.name === "history_mode"); - if (paginatedColumn && restoreEntries.length > 0) return "history_paginated_requires_native_writer"; + if (paginatedColumn && restoreEntries.length > 0) return HISTORY_RELABEL_STANDS_DOWN; for (const entry of restoreEntries) assertLegacyHistoryWritable(entry.rolloutPath); const rows = db.query<{ rollout_path: string; history_mode: string | null }, []>(` SELECT rollout_path, ${paginatedColumn ? "history_mode" : "NULL AS history_mode"} @@ -433,7 +446,7 @@ export function preflightCodexHistoryInjection( : "model_provider = 'opencodex'"} `).all(); for (const row of rows) { - if (paginatedColumn || row.history_mode === "paginated") return "history_paginated_requires_native_writer"; + if (paginatedColumn || row.history_mode === "paginated") return HISTORY_RELABEL_STANDS_DOWN; assertLegacyHistoryWritable(row.rollout_path); } return null; diff --git a/src/codex/inject.ts b/src/codex/inject.ts index a01909a9c7..7d9d838af8 100644 --- a/src/codex/inject.ts +++ b/src/codex/inject.ts @@ -40,7 +40,7 @@ import { removeJournal, writeJournal, } from "./journal"; -import { preflightCodexHistoryInjection } from "./history-provider"; +import { HISTORY_RELABEL_STANDS_DOWN, preflightCodexHistoryInjection } from "./history-provider"; import { describeHistoryJobFailure, deriveCodexHistoryOperation, @@ -166,8 +166,10 @@ export interface CodexInjectResult { /** * The one history preflight reason that is permanent rather than operational: Codex owns * paginated rollout ordinals, so no retry makes the legacy relabel protocol available again. + * + * The constant now lives beside the guard that produces it, in `history-provider.ts`, so + * the apply and restore directions cannot disagree about which reason stands down. */ -const HISTORY_RELABEL_STANDS_DOWN = "history_paginated_requires_native_writer"; class CodexHistoryPreflightRefusal extends Error {} let historyArtifactStageForTests: ((stage: string) => void) | undefined; @@ -967,9 +969,14 @@ export { export type { CodexRoutingKind } from "./inject/routing-classify"; export { + appendOcxProviderTableBlock, + extractOcxProviderTableBlock, + readOcxProviderTableBlock, removeCodexConfig, + retainOcxProviderTableOnDisk, stripOpencodexConfig, } from "./inject/remove"; +export type { RemoveCodexConfigOptions, RemoveCodexConfigResult } from "./inject/remove"; export type { CodexNativeRestoreResult, @@ -977,9 +984,13 @@ export type { CodexRestoreCatalogResult, CodexRestoreConfigResult, CodexRestoreHistoryResult, + RestoreHistoryDisposition, + RetainedCodexProviderTable, } from "./inject/restore"; export { failedHistoryRestoreFromOutcome, + describeRetainedCodexProviderTable, + resolveRestoreHistoryDisposition, restoreNativeCodex, restoreNativeCodexAsync, setBeforeRestoreConfigForTests, diff --git a/src/codex/inject/remove.ts b/src/codex/inject/remove.ts index fb56b71a44..2d8e750549 100644 --- a/src/codex/inject/remove.ts +++ b/src/codex/inject/remove.ts @@ -7,7 +7,7 @@ import { rootTomlString, stripJournaledOpenaiBaseUrl, } from "../injected-marker"; -import { preflightCodexHistoryInjection } from "../history-provider"; +import { HISTORY_RELABEL_STANDS_DOWN, preflightCodexHistoryInjection } from "../history-provider"; import { journaledInjectedOpenaiBaseUrl, journaledInjectedRealtimeWsBaseUrl, @@ -79,6 +79,90 @@ export function removeOcxSection(content: string): string { ); } +/** + * Capture `[model_providers.opencodex]` verbatim so it can survive a restore that only + * takes routing down (#4812). + * + * This is deliberately NOT a mirror of `removeOcxSection`'s scan. That one opens a + * section on any line containing `OCX_SECTION_MARKER`, which is safe there only because + * `stripInjectedOpenaiBaseUrl` has already consumed the identical marker that annotates + * the root `openai_base_url`. Capture runs against the untouched file, so the same rule + * would collect that marker and the routing line under it — and re-appending the result + * would restore the exact base-url override the caller just removed. + * + * So the anchor is the provider header itself, via the shared `isOcxProviderHeaderLine`, + * with an immediately preceding marker line pulled in as its comment. Sharing that + * predicate is what keeps capture and removal from disagreeing about what our table is. + */ +export function extractOcxProviderTableBlock(content: string): string | null { + const lines = content.split("\n"); + const collected: string[] = []; + let capturing = false; + for (let index = 0; index < lines.length; index++) { + const line = lines[index]!; + if (isOcxProviderHeaderLine(line.trim())) { + if (!capturing) { + const previous = lines[index - 1]; + if (previous !== undefined && previous.includes(OCX_SECTION_MARKER)) collected.push(previous); + capturing = true; + } + collected.push(line); + continue; + } + if (!capturing) continue; + // A foreign table header closes ours, exactly as in `removeOcxSection`. A later + // `[model_providers.opencodex.*]` sub-table reopens capture on the next iteration, + // which is why the two are separate passes over the same predicate. + if (/^\s*\[/.test(line)) { + capturing = false; + continue; + } + collected.push(line); + } + if (collected.length === 0) return null; + return collected.join("\n").replace(/\n{3,}/g, "\n\n").trimEnd() + "\n"; +} + +/** + * Append a captured provider table to stripped content, as one buffer. + * + * Pure on purpose. Upstream resolves `model_provider` against the merged provider map and + * fails the WHOLE config load on a miss — not the one thread — so a config carrying root + * `model_provider = "opencodex"` without this table breaks every `codex` invocation. The + * strip and the re-append therefore have to reach disk in a single write, which they can + * only do if the append is a transform rather than a second file operation. + */ +export function appendOcxProviderTableBlock(content: string, block: string): string { + if (hasOcxProviderTable(content)) return content; + return `${content.replace(/\n+$/, "")}\n\n${block.replace(/\n+$/, "")}\n`; +} + +/** Read the provider table straight off disk, before anything has transformed it. */ +export function readOcxProviderTableBlock(): string | null { + if (!existsSync(CODEX_CONFIG_PATH)) return null; + return extractOcxProviderTableBlock(applyEol(readFileSync(CODEX_CONFIG_PATH, "utf-8"), "\n")); +} + +/** + * Re-attach a captured provider table after an exact journal restore. + * + * This is the one place retention needs a second write, because the journal replays whole + * pre-injection bytes rather than transforming the current file. The intermediate state is + * the safe one: the journal's config is the user's own, so it carries no + * `model_provider = "opencodex"` for a missing table to strand. A crash between the two + * writes leaves a fully native config, which is the direction this whole change is trying + * to reach anyway. + */ +export function retainOcxProviderTableOnDisk(block: string): string[] | null { + if (!existsSync(CODEX_CONFIG_PATH)) return null; + const rawContent = readFileSync(CODEX_CONFIG_PATH, "utf-8"); + const eol = dominantEol(rawContent); + const content = applyEol(rawContent, "\n"); + const next = appendOcxProviderTableBlock(content, block); + if (next !== content) atomicWriteFile(CODEX_CONFIG_PATH, applyEol(next, eol)); + return block.replace(/\n+$/, "").split("\n"); +} + interface StripOpencodexConfigResult { content: string; managedDefaultsError: string | null; @@ -139,11 +223,38 @@ function hasOpencodexRouting(content: string): boolean { ); } +export interface RemoveCodexConfigOptions { + preserveProfile?: boolean; + /** + * Keep `[model_providers.opencodex]` on disk while every routing key comes out. + * + * Set when the history preflight stands the relabel down: the rows this home tagged + * `opencodex` stay tagged, and they resolve only through this table. Retaining it means + * those conversations still open — their requests fail against a stopped proxy, which is + * an ordinary connection error — while plain `codex` returns to the built-in provider. + */ + retainProviderTable?: boolean; +} + +export interface RemoveCodexConfigResult { + success: boolean; + message: string; + /** The exact lines left on disk when `retainProviderTable` kept the table. */ + retainedProviderTable?: string[]; +} + export function removeCodexConfig( - options: { preserveProfile?: boolean } = {}, -): { success: boolean; message: string } { + options: RemoveCodexConfigOptions = {}, +): RemoveCodexConfigResult { const historyError = preflightCodexHistoryInjection(false, false); - if (historyError) return { success: false, message: `Codex configuration preserved: ${historyError}. Native writer coordination is required.` }; + // The preflight answers "may I rewrite conversation history?". Routing removal is a + // different question, and treating one answer as both is what left `ocx uninstall` + // pointing a live config at a port it had just removed (#4812). Only the stand-down + // reason is separable; every other reason still means something is wrong with the + // history state itself, and those keep the hard refusal. + if (historyError && !(options.retainProviderTable === true && historyError === HISTORY_RELABEL_STANDS_DOWN)) { + return { success: false, message: `Codex configuration preserved: ${historyError}. Native writer coordination is required.` }; + } if (!existsSync(CODEX_CONFIG_PATH)) { if (!options.preserveProfile && existsSync(CODEX_PROFILE_PATH)) unlinkSync(CODEX_PROFILE_PATH); @@ -166,13 +277,25 @@ export function removeCodexConfig( || (journaledRealtimeWsBaseUrl !== null && rootTomlString(content, REALTIME_WS_BASE_URL_KEY) === journaledRealtimeWsBaseUrl); const stripped = stripOpencodexConfigResult(content, journaledBaseUrl, journaledRealtimeWsBaseUrl); - if (had || stripped.content !== content) { - atomicWriteFile(CODEX_CONFIG_PATH, applyEol(stripped.content, eol)); + // Captured from the pre-strip bytes: the strip is what removes the table, so reading it + // afterwards would find nothing. + const retainedBlock = options.retainProviderTable === true + ? extractOcxProviderTableBlock(content) + : null; + const finalContent = retainedBlock === null + ? stripped.content + : appendOcxProviderTableBlock(stripped.content, retainedBlock); + if (had || finalContent !== content) { + atomicWriteFile(CODEX_CONFIG_PATH, applyEol(finalContent, eol)); } if (!options.preserveProfile && existsSync(CODEX_PROFILE_PATH)) unlinkSync(CODEX_PROFILE_PATH); + const retainedNote = retainedBlock === null + ? "" + : " Kept [model_providers.opencodex] so conversations already tagged opencodex still open;" + + " remove it with 'ocx restore --remove-codex-provider-table' (those conversations stop opening)."; const removedMessage = had - ? `Removed opencodex routing from Codex config${options.preserveProfile ? "." : " + profile."}` + ? `Removed opencodex routing from Codex config${options.preserveProfile ? "." : " + profile."}${retainedNote}` : "opencodex not present in Codex config."; if (stripped.managedDefaultsError) { const routingMessage = had @@ -188,5 +311,6 @@ export function removeCodexConfig( return { success: true, message: removedMessage, + ...(retainedBlock === null ? {} : { retainedProviderTable: retainedBlock.replace(/\n+$/, "").split("\n") }), }; } diff --git a/src/codex/inject/restore.ts b/src/codex/inject/restore.ts index 5e273173ec..bf5fe2096c 100644 --- a/src/codex/inject/restore.ts +++ b/src/codex/inject/restore.ts @@ -28,6 +28,7 @@ import { import { preflightCodexHistoryInjection, syncCodexHistoryProvider, + HISTORY_RELABEL_STANDS_DOWN, type CodexHistoryFailureReason, } from "../history-provider"; import { @@ -44,7 +45,11 @@ import { } from "../paths"; import { shouldInjectApiAuthHeader } from "../loopback-target"; import { currentExternalCodexModelProvider } from "./config-toml"; -import { removeCodexConfig } from "./remove"; +import { + readOcxProviderTableBlock, + removeCodexConfig, + retainOcxProviderTableOnDisk, +} from "./remove"; class CodexRestoreRefusal extends Error { constructor(readonly config: CodexRestoreConfigResult) { @@ -57,13 +62,49 @@ export function setBeforeRestoreConfigForTests(hook: typeof beforeRestoreConfigF beforeRestoreConfigForTests = hook; } -export type CodexRestoreArtifactState = "ok" | "skipped" | "failed"; +/** + * `partial` means the artifact was restored as far as it safely could be and named what + * it left behind. It is not a failure — the caller's obligation was discharged — but it + * is not a plain `ok` either, because something on disk still needs a decision (#4812). + */ +export type CodexRestoreArtifactState = "ok" | "partial" | "skipped" | "failed"; + +/** What a degraded restore kept, why, and how to finish the job. */ +export interface RetainedCodexProviderTable { + reason: typeof HISTORY_RELABEL_STANDS_DOWN; + /** The exact `config.toml` lines left on disk. */ + lines: string[]; + followUp: string; +} + +const RETAINED_PROVIDER_TABLE_FOLLOW_UP = + "Run 'ocx restore --remove-codex-provider-table' to remove it; conversations already tagged " + + "opencodex will stop opening if you do."; + +/** + * The one sentence every teardown surface prints about retained residue. + * + * Shared rather than rewritten per caller: `restore`, `stop`, `uninstall`, the service + * subcommands and the stop API all report this same outcome, and a user who runs two of + * them should not have to work out whether two different descriptions mean the same state. + */ +export function describeRetainedCodexProviderTable(retained: RetainedCodexProviderTable): string { + return "Kept [model_providers.opencodex] in $CODEX_HOME/config.toml because Codex owns this home's" + + ` paginated history (${retained.reason}): conversations already tagged opencodex resolve only` + + ` through that table. Plain \`codex\` is native again. ${retained.followUp}`; +} export interface CodexRestoreConfigResult { state: CodexRestoreArtifactState; changed: boolean; - action: "journal-restored" | "owned-fields-stripped" | "external-provider-preserved" | "failed"; + action: + | "journal-restored" + | "owned-fields-stripped" + | "routing-restored-provider-retained" + | "external-provider-preserved" + | "failed"; message: string; + retained?: RetainedCodexProviderTable; } export interface CodexRestoreCatalogResult { @@ -101,6 +142,14 @@ export interface CodexNativeRestoreResult { * make a safety decision depend on prose. */ historyPreflightRefusal?: string; + /** + * Set when routing came out but `[model_providers.opencodex]` stayed (#4812). + * + * Distinct from `historyPreflightRefusal`, which means nothing was attempted at all. + * This one means the config obligation WAS discharged, so a stop receipt must be + * released rather than preserved. + */ + retainedCodexProviderTable?: RetainedCodexProviderTable; artifacts: { config: CodexRestoreConfigResult; catalog: CodexRestoreCatalogResult; @@ -243,10 +292,42 @@ function historyPreflightRefusalEnvelope(historyError: string): CodexNativeResto return result; } +/** + * How a restore may proceed given what the history preflight says. + * + * The preflight answers one question — may conversation history be rewritten — and this + * translates it into the separate question the restore actually needs answered: may + * OpenCodex routing come out of `config.toml`, and what has to stay if it does. + */ +export type RestoreHistoryDisposition = + | { kind: "proceed" } + | { kind: "stand-down"; retainProviderTable: boolean } + | { kind: "refuse"; reason: string }; + +export function resolveRestoreHistoryDisposition( + removeProviderTable: boolean | undefined, + reason: string | null = preflightCodexHistoryInjection(false, false), +): RestoreHistoryDisposition { + if (!reason) return { kind: "proceed" }; + // Every other reason still means the history state itself is wrong — a missing store, an + // unreadable rollout, an integrity failure. Those keep the hard refusal and the + // compensating rollback they have always had. + if (reason !== HISTORY_RELABEL_STANDS_DOWN) return { kind: "refuse", reason }; + // The rows stay tagged `opencodex` either way, because the native writer owns them. + // Retaining the table is what keeps those conversations openable; the explicit flag is + // the user accepting that they will not be. + return { kind: "stand-down", retainProviderTable: removeProviderTable !== true }; +} + +export interface RestoreConfigOptions { + /** Strip `[model_providers.opencodex]` too, accepting that tagged threads stop opening. */ + removeProviderTable?: boolean; +} + /** The config/profile half of a native restore, reported as one artifact. */ -function restoreCodexConfigInline(kind = "sync"): CodexRestoreConfigResult { +function restoreCodexConfigInline(kind = "sync", options: RestoreConfigOptions = {}): CodexRestoreConfigResult { const preImages = captureCodexPreImages(); - const result = restoreCodexConfigInlineImpl(kind); + const result = restoreCodexConfigInlineImpl(kind, options); if (result.state === "failed") { const compensated = restoreCodexPreImages(preImages); if (!compensated.complete) throw new CodexPartialWriteError(compensated.unrestored); @@ -254,11 +335,26 @@ function restoreCodexConfigInline(kind = "sync"): CodexRestoreConfigResult { return result; } -function restoreCodexConfigInlineImpl(kind: string): CodexRestoreConfigResult { +function restoreCodexConfigInlineImpl(kind: string, options: RestoreConfigOptions): CodexRestoreConfigResult { try { beforeRestoreConfigForTests?.(kind); - const historyError = preflightCodexHistoryInjection(false, false); - if (historyError) return { state: "failed", changed: false, action: "failed", message: `Codex configuration and journal preserved: ${historyError}.` }; + const disposition = resolveRestoreHistoryDisposition(options.removeProviderTable); + if (disposition.kind === "refuse") { + return { state: "failed", changed: false, action: "failed", message: `Codex configuration and journal preserved: ${disposition.reason}.` }; + } + const retainProviderTable = disposition.kind === "stand-down" && disposition.retainProviderTable; + // Captured unconditionally, not only when the stand-down is already known. + // + // Two different paths need bytes that only exist before the write. The journal restore + // replays the pre-injection config, which never contained our table, and then deletes + // the journal. And Codex can paginate DURING the write: the post-write re-check below + // then sees a stand-down that the pre-write check did not, at which point the table has + // already been stripped and there is nothing left to read. Both are cheap to prevent + // and impossible to repair afterwards, so the read happens once, here. + // + // The one caller that must not capture is the explicit removal flag: it is the user + // accepting that tagged conversations stop opening. + const capturedBlock = options.removeProviderTable === true ? null : readOcxProviderTableBlock(); const journal = restoreJournalState(); if (journal.unverified) { return { @@ -267,13 +363,47 @@ function restoreCodexConfigInlineImpl(kind: string): CodexRestoreConfigResult { }; } const restored = journal.configRestored - ? { success: true, message: "Codex config restored from opencodex journal." } - : removeCodexConfig({ preserveProfile: journal.profileRestored || journal.profileChanged }); + ? { success: true, message: "Codex config restored from opencodex journal.", retainedProviderTable: undefined as string[] | undefined } + : removeCodexConfig({ + preserveProfile: journal.profileRestored || journal.profileChanged, + retainProviderTable, + }); + let retainedLines = restored.retainedProviderTable ?? null; if (restored.success) { // A successful journal/fallback write can race native history migration too. // Refuse here while preimage compensation and the remove transaction can roll back. - const finalHistoryError = preflightCodexHistoryInjection(false, false); - if (finalHistoryError) return { state: "failed", changed: false, action: "failed", message: `Codex configuration and journal preserved: ${finalHistoryError}.` }; + // A stand-down observed now is the same stand-down that was already accounted for — + // it must not undo a routing removal that has already reached disk. + const settled = resolveRestoreHistoryDisposition(options.removeProviderTable); + if (settled.kind === "refuse") { + return { state: "failed", changed: false, action: "failed", message: `Codex configuration and journal preserved: ${settled.reason}.` }; + } + // One re-attach covers three cases that all need the same bytes on disk: the journal + // path, which wrote a config without our table; the migration race, where the strip ran + // before anyone knew a table was needed; and the ordinary planned retention, where + // `removeCodexConfig` already put it back and this is a no-op. Re-attaching is + // idempotent — it checks for the table before appending — so the three do not have to + // be told apart here. + if (settled.kind === "stand-down" && settled.retainProviderTable && capturedBlock !== null) { + retainedLines = retainOcxProviderTableOnDisk(capturedBlock) ?? retainedLines; + } + } + if (restored.success && retainedLines !== null) { + return { + state: "partial", + changed: true, + action: "routing-restored-provider-retained", + message: journal.configRestored + ? "Codex config restored from opencodex journal. Kept [model_providers.opencodex] so conversations already" + + " tagged opencodex still open; remove it with 'ocx restore --remove-codex-provider-table'" + + " (those conversations stop opening)." + : restored.message, + retained: { + reason: HISTORY_RELABEL_STANDS_DOWN, + lines: retainedLines, + followUp: RETAINED_PROVIDER_TABLE_FOLLOW_UP, + }, + }; } return restored.success ? { @@ -336,7 +466,7 @@ function restoreCodexCatalogArtifact( * that lost race into the discriminated `desired_enabled` skip. */ export async function restoreNativeCodexAsync( - options: { revalidateDesiredState?: boolean } = {}, + options: { revalidateDesiredState?: boolean; removeProviderTable?: boolean } = {}, ): Promise { try { return await restoreNativeCodexAsyncImpl(options); @@ -347,7 +477,7 @@ export async function restoreNativeCodexAsync( } async function restoreNativeCodexAsyncImpl( - options: { revalidateDesiredState?: boolean }, + options: { revalidateDesiredState?: boolean; removeProviderTable?: boolean }, ): Promise { const activeProvider = currentExternalCodexModelProvider(); if (activeProvider) { @@ -368,8 +498,12 @@ async function restoreNativeCodexAsyncImpl( if (shouldSyncCodexOnStart(loadConfig())) return desiredEnabledRestoreSkip(); } - const historyError = preflightCodexHistoryInjection(false, false); - if (historyError) return historyPreflightRefusalEnvelope(historyError); + const disposition = resolveRestoreHistoryDisposition(options.removeProviderTable); + if (disposition.kind === "refuse") return historyPreflightRefusalEnvelope(disposition.reason); + // A stand-down spawns no history Worker. The preflight the Worker would run first has + // already answered, and the rows stay tagged `opencodex` on purpose — which is exactly + // why the provider table has to survive the config half. + const historyStandsDown = disposition.kind === "stand-down"; const eligibility = codexWriteCoordinationEligibility({ coordinatorPath: () => @@ -420,7 +554,7 @@ async function restoreNativeCodexAsyncImpl( const preImages = captureCodexPreImages(); let restored: CodexRestoreConfigResult; try { - restored = restoreCodexConfigInline(eligibility.kind); + restored = restoreCodexConfigInline(eligibility.kind, options); // Throw inside N so the published remove transition rolls back too. if (restored.state === "failed") throw new CodexRestoreRefusal(restored); } catch (error) { @@ -463,20 +597,34 @@ async function restoreNativeCodexAsyncImpl( if (options.revalidateDesiredState && shouldSyncCodexOnStart(loadConfig())) { return desiredEnabledRestoreSkip(); } - config = restoreCodexConfigInline(eligibility.kind); + config = restoreCodexConfigInline(eligibility.kind, options); } if (config.state === "failed") return failedConfigRestoreEnvelope(config); const catalog = restoreCodexCatalogArtifact(options.revalidateDesiredState === true, journaledCatalogPath); - const outcome = await runCodexHistoryJob({ - ...resolveCodexHistoryJobTarget(), - ...(options.revalidateDesiredState ? { expectedDesiredEnabled: false } : {}), - operation: deriveCodexHistoryOperation({ direction: "restore", resumeHistory: true, legacyMode: false }), - }); + // Re-asked after the config half, because the store can paginate mid-transaction. Deciding + // the history job from the pre-write answer alone would spawn a Worker whose own preflight + // is now guaranteed to refuse, and report that refusal as a restore failure on a home that + // was in fact restored. + const historyStoodDown = historyStandsDown + || resolveRestoreHistoryDisposition(options.removeProviderTable).kind === "stand-down"; + const outcome: CodexHistoryJobOutcome = historyStoodDown + ? { kind: "skipped" } + : await runCodexHistoryJob({ + ...resolveCodexHistoryJobTarget(), + ...(options.revalidateDesiredState ? { expectedDesiredEnabled: false } : {}), + operation: deriveCodexHistoryOperation({ direction: "restore", resumeHistory: true, legacyMode: false }), + }); if (transitionReceipt) { resolveCodexHistoryTransition(transitionReceipt, outcome); } - const history: CodexRestoreHistoryResult = outcome.kind === "converged" + const history: CodexRestoreHistoryResult = historyStoodDown + ? { + state: "skipped", changed: false, rows: 0, files: 0, ejectedRows: 0, + message: `Codex resume history was left to Codex's native writer (${HISTORY_RELABEL_STANDS_DOWN});` + + " existing threads keep the provider they are tagged with and no rollout byte was read or written.", + } + : outcome.kind === "converged" ? { state: "ok", changed: outcome.rows > 0 || outcome.files > 0, rows: outcome.rows, files: outcome.files, ejectedRows: 0, message: outcome.rows > 0 @@ -500,14 +648,24 @@ async function restoreNativeCodexAsyncImpl( : config.message; const success = catalog.state !== "failed" && history.state !== "failed"; + // A stood-down relabel is not a failure, but it is something the operator has to be told: + // their existing conversations keep the provider they are tagged with, and nothing will + // ever change that from this side. Printing only the config half would be the same + // partial-success-reported-as-success problem this change exists to end. + const historyNote = history.state === "failed" + ? ` ⚠️ ${history.message}` + : historyStoodDown ? ` ${history.message}` : ""; return { success, - message: `${base}${history.state === "failed" ? ` ⚠️ ${history.message}` : ""}`, + message: `${base}${historyNote}`, + ...(config.retained ? { retainedCodexProviderTable: config.retained } : {}), artifacts: { config, catalog, history }, }; } -export function restoreNativeCodex(options: { skipHistory?: boolean; revalidateDesiredState?: boolean } = {}): CodexNativeRestoreResult { +export function restoreNativeCodex( + options: { skipHistory?: boolean; revalidateDesiredState?: boolean; removeProviderTable?: boolean } = {}, +): CodexNativeRestoreResult { const activeProvider = currentExternalCodexModelProvider(); if (activeProvider) { removeJournal(); @@ -516,14 +674,18 @@ export function restoreNativeCodex(options: { skipHistory?: boolean; revalidateD if (options.revalidateDesiredState && shouldSyncCodexOnStart(loadConfig())) { return desiredEnabledRestoreSkip(); } - const historyError = preflightCodexHistoryInjection(false, false); - if (historyError) return historyPreflightRefusalEnvelope(historyError); + const disposition = resolveRestoreHistoryDisposition(options.removeProviderTable); + if (disposition.kind === "refuse") return historyPreflightRefusalEnvelope(disposition.reason); + const historyStandsDown = disposition.kind === "stand-down"; // Captured before the config half: a successful journal restore DELETES the journal, and // restoring the config can drop `model_catalog_json`. Either one would hide the routed // catalog we actually wrote (#1798). const journaledCatalogPath = journaledInjectedCatalogPath(); - const config = restoreCodexConfigInline(); + const config = restoreCodexConfigInline("sync", options); if (config.state === "failed") return failedConfigRestoreEnvelope(config); + // Same mid-transaction pagination re-check as the async path. + const historyStoodDown = historyStandsDown + || resolveRestoreHistoryDisposition(options.removeProviderTable).kind === "stand-down"; const catalog = restoreCodexCatalogArtifact(options.revalidateDesiredState === true, journaledCatalogPath); // Design B (loopback) steady state: threads are already tagged openai, so prove the // no-op with a readonly probe instead of write-opening a DB the Codex app may hold @@ -537,12 +699,18 @@ export function restoreNativeCodex(options: { skipHistory?: boolean; revalidateD } // `skipHistory` is how the async wrapper takes this work for itself: the // native files come down here, and history runs in the Worker under H. - const rawHistory = options.skipHistory + const rawHistory = options.skipHistory || historyStoodDown ? { rows: 0, files: 0 } : syncCodexHistoryProvider("openai", undefined, undefined, { skipWhenProvablyNoop, }); - const history: CodexRestoreHistoryResult = options.skipHistory + const history: CodexRestoreHistoryResult = historyStoodDown + ? { + state: "skipped", changed: false, rows: 0, files: 0, ejectedRows: 0, + message: `Codex resume history was left to Codex's native writer (${HISTORY_RELABEL_STANDS_DOWN});` + + " existing threads keep the provider they are tagged with and no rollout byte was read or written.", + } + : options.skipHistory ? { state: "skipped", changed: false, rows: 0, files: 0, ejectedRows: 0, message: "History restoration runs asynchronously." } : rawHistory.failed ? failedHistoryRestore(rawHistory.failureReason, undefined, rawHistory) @@ -561,7 +729,8 @@ export function restoreNativeCodex(options: { skipHistory?: boolean; revalidateD : config.message; return { success: catalog.state !== "failed" && history.state !== "failed", - message, + message: historyStoodDown ? `${message} ${history.message}` : message, + ...(config.retained ? { retainedCodexProviderTable: config.retained } : {}), artifacts: { config, catalog, history }, }; } diff --git a/src/server/stop-teardown.ts b/src/server/stop-teardown.ts index e4aadc4996..146f4255fc 100644 --- a/src/server/stop-teardown.ts +++ b/src/server/stop-teardown.ts @@ -67,7 +67,14 @@ export async function performStopTeardown(url: URL, io: StopTeardownIo = {}): Pr // undone (#3008). const grokNote = grok.ok ? "" : ` Grok config cleanup failed: ${grok.message}`; if (restore.success && grok.ok) { - return { success: true, message: "Proxy stopping, native Codex restored.", sharedTeardown: "performed" }; + // A degraded restore is a success — routing is out and the client is no longer aimed at + // a port that is about to disappear — but it left a provider table behind on purpose. + // Reporting a bare "restored" would put the caller in exactly the position #4812 + // describes: a config they did not expect and no idea why it is there. + const retained = restore.retainedCodexProviderTable + ? ` ${(await import("../codex/inject")).describeRetainedCodexProviderTable(restore.retainedCodexProviderTable)}` + : ""; + return { success: true, message: `Proxy stopping, native Codex restored.${retained}`, sharedTeardown: "performed" }; } if (restore.success) { return { diff --git a/src/service/cli.ts b/src/service/cli.ts index af41ae4f5e..e1a6c3da49 100644 --- a/src/service/cli.ts +++ b/src/service/cli.ts @@ -1,6 +1,6 @@ import { existsSync, unlinkSync } from "node:fs"; import { join } from "node:path"; -import { restoreNativeCodexAsync } from "../codex/inject"; +import { describeRetainedCodexProviderTable, restoreNativeCodexAsync } from "../codex/inject"; import { stripGrokConfig } from "../grok/inject"; import { serviceApiTokenFilePath } from "../lib/service-secrets"; import { statusWinswRaw, type WinswStatus } from "../lib/winsw"; @@ -291,7 +291,15 @@ export async function serviceCommand(...args: (string | undefined)[]): Promise { expect(uninstallBody.indexOf('runStep("proxy stopped"')).toBeLessThan(uninstallBody.indexOf('runStep("service removed"')); expect(uninstallBody.indexOf("await stopProxy(pid);")).toBeLessThan(uninstallBody.indexOf("uninstallServiceDetailed()")); }); + + test("restore forwards the explicit provider-table removal flag and warns before mutation", async () => { + const dispatch = await readText("src/cli/dispatch.ts"); + const restoreStart = dispatch.indexOf("restore: async deps => {"); + const restoreBody = dispatch.slice(restoreStart, dispatch.indexOf('"recover-history": async', restoreStart)); + + expect(restoreBody).toContain('takeFlag(restoreArgs, "--remove-codex-provider-table")'); + expect(restoreBody).toContain("conversations already tagged opencodex will stop opening"); + expect(restoreBody).toContain("restoreNativeCodexAsync({ revalidateDesiredState: true, removeProviderTable })"); + }); }); describe("uninstall gates shared teardown on a proven service stop", () => { test("the authorization rule, exercised for every failure permutation", async () => { @@ -213,6 +223,15 @@ describe("uninstall gates shared teardown on a proven service stop", () => { expect(fn).toContain("observed.respawnWindowVerified = true;"); const gateAt = fn.indexOf("if (sharedTeardownAuthorized(observed)) {"); expect(gateAt).toBeLessThan(fn.indexOf("native Codex restored", gateAt)); + const nativeRestoreStep = fn.slice( + fn.indexOf('runStep("native Codex restored"', gateAt), + fn.indexOf('runStep("Grok Build config restored"', gateAt), + ); + // A partial config artifact with success=true discharged routing. Uninstall must report + // the retained table and continue, rather than adding this step to the failure list. + expect(nativeRestoreStep).toContain("if (!r.success) throw new Error(r.message);"); + expect(nativeRestoreStep).toContain("if (r.retainedCodexProviderTable)"); + expect(nativeRestoreStep).not.toContain('state === "partial"'); // The skip is a failure, not a silent pass: the command must exit nonzero and say what // to run once the blocker is resolved. expect(fn).toContain('failures.push("native Codex restored", "Grok Build config restored");'); diff --git a/tests/codex-integration/codex-inject-integration.test.ts b/tests/codex-integration/codex-inject-integration.test.ts index 5e7c20866a..52493130ef 100644 --- a/tests/codex-integration/codex-inject-integration.test.ts +++ b/tests/codex-integration/codex-inject-integration.test.ts @@ -91,7 +91,7 @@ describe("injectCodexConfig integration (Design B)", () => { removeTreeWithRetry(ocxHome); }); - test.each(["sync", "async"])("manifest-owned native rows refuse restore before artifact changes (%s)", (kind) => { + test.each(["sync", "async"])("paginated manifest-owned rows stand down while routing is restored (%s)", (kind) => { writeFileSync(join(codexHome, "config.toml"), 'model="test"\n'); const script = ` const fs = require("node:fs"); @@ -104,22 +104,33 @@ describe("injectCodexConfig integration (Design B)", () => { if (!enabled.success) throw new Error("fixture injection failed"); const dbPath = join(process.env.CODEX_HOME, "state_5.sqlite"); const rollout = join(process.env.CODEX_HOME, "manifest-fixture.jsonl"); + fs.appendFileSync(join(process.env.CODEX_HOME,"config.toml"), [ + "# Auto-injected by opencodex", + "[model_providers.opencodex]", + 'name="OpenCodex"', + 'base_url="http://127.0.0.1:10100/v1"', + 'wire_api="responses"', + "", + ].join(String.fromCharCode(10))); fs.writeFileSync(rollout, JSON.stringify({type:"session_meta",payload:{id:"fixture",model_provider:"openai",source:"cli"}})+String.fromCharCode(10)); const db = new Database(dbPath); db.run("CREATE TABLE threads (id TEXT PRIMARY KEY, rollout_path TEXT, model_provider TEXT, source TEXT, first_user_message TEXT, has_user_event INTEGER)"); db.run("INSERT INTO threads VALUES ('fixture', ?, 'openai', 'cli', 'hello', 1)", rollout); const routed = syncCodexHistoryProvider("opencodex", dbPath); if (routed.failed || routed.rows !== 1) throw new Error("fixture history route failed"); - db.run("UPDATE threads SET model_provider='openai'"); db.run("ALTER TABLE threads ADD COLUMN history_mode TEXT DEFAULT 'legacy'"); db.close(); const backup = historyBackupPathFor(dbPath); const entries = Object.keys(JSON.parse(fs.readFileSync(backup,"utf8")).entries).length; const defaultEntries = Object.keys(JSON.parse(fs.readFileSync(historyBackupPathFor(resolveCodexStateDbPath()),"utf8")).entries).length; - const paths = ["config.toml","opencodex.config.toml","opencodex-journal.json"].map(p=>join(process.env.CODEX_HOME,p)).concat([backup,rollout]); - const before = paths.map(p=>fs.readFileSync(p,"utf8")); + const historyPaths = [backup,rollout]; + const beforeHistory = historyPaths.map(p=>fs.readFileSync(p,"utf8")); const result = ${kind === "sync" ? "restoreNativeCodex()" : "await restoreNativeCodexAsync()"}; - console.log(JSON.stringify({entries,defaultEntries,result,preserved:paths.every((p,i)=>fs.readFileSync(p,"utf8")===before[i])})); + const restoredDb = new Database(dbPath, { readonly: true }); + const provider = restoredDb.query("SELECT model_provider FROM threads WHERE id='fixture'").get().model_provider; + restoredDb.close(); + const config = fs.readFileSync(join(process.env.CODEX_HOME,"config.toml"),"utf8"); + console.log(JSON.stringify({entries,defaultEntries,result,provider,config,historyPreserved:historyPaths.every((p,i)=>fs.readFileSync(p,"utf8")===beforeHistory[i])})); `; const child = spawnSync(process.execPath, ["--eval", script], { cwd: repoRoot, env: { ...process.env, CODEX_HOME: codexHome, OPENCODEX_HOME: ocxHome }, @@ -129,18 +140,20 @@ describe("injectCodexConfig integration (Design B)", () => { const result = JSON.parse(child.stdout); expect(result.entries).toBe(1); expect(result.defaultEntries).toBe(1); - expect(result.result.success).toBe(false); - expect(result.result.message).toContain("history_paginated_requires_native_writer"); - // #4718: the refusal also has to be legible without reading the message. `ocx stop` - // decides whether an obligation was discharged from this envelope, and every artifact - // comes back "skipped" here — the same shape an ownership refusal and a desired-state - // skip produce. Without the structured reason the caller could only match prose, and - // the stop misread this as a generic teardown failure and aborted the update. - expect(result.result.historyPreflightRefusal).toBe("history_paginated_requires_native_writer"); - expect(result.result.artifacts.config.state).toBe("skipped"); - expect(result.result.artifacts.catalog.state).toBe("skipped"); - expect(result.result.artifacts.history.state).toBe("skipped"); - expect(result.preserved).toBe(true); + expect(result.result.success).toBe(true); + expect(result.result.historyPreflightRefusal).toBeUndefined(); + expect(result.result.artifacts.config).toMatchObject({ + state: "partial", + action: "routing-restored-provider-retained", + retained: { reason: "history_paginated_requires_native_writer" }, + }); + expect(result.result.retainedCodexProviderTable).toEqual(result.result.artifacts.config.retained); + expect(result.result.retainedCodexProviderTable.followUp).toContain("ocx restore --remove-codex-provider-table"); + expect(result.result.artifacts.history).toMatchObject({ state: "skipped", changed: false, rows: 0, files: 0 }); + expect(result.provider).toBe("opencodex"); + expect(result.config).not.toContain('model_provider = "opencodex"'); + expect(result.config).toContain("[model_providers.opencodex]"); + expect(result.historyPreserved).toBe(true); }); // The denial has to be a real filesystem permission. `inject-coordination.ts` @@ -340,6 +353,7 @@ describe("injectCodexConfig integration (Design B)", () => { const catalog = '{"models":[],"sentinel":"preserve"}\n'; writeFileSync(join(codexHome, "models_cache.json"), catalog); const script = ` + const fs=require("node:fs"); const {Database}=require("bun:sqlite"); const {join}=require("node:path"); const {restoreNativeCodex,restoreNativeCodexAsync,setBeforeRestoreConfigForTests}=require("./src/codex/inject"); @@ -348,9 +362,11 @@ describe("injectCodexConfig integration (Design B)", () => { let observed; setBeforeRestoreConfigForTests(value=>{ observed=value; + const rollout=join(process.env.CODEX_HOME,"invalid-rollout.jsonl"); + fs.writeFileSync(rollout,"not-json\\n"); const db=new Database(join(process.env.CODEX_HOME,"state_5.sqlite")); - db.run("CREATE TABLE threads (rollout_path TEXT, model_provider TEXT, history_mode TEXT)"); - db.run("INSERT INTO threads VALUES ('fixture','opencodex','paginated')"); + db.run("CREATE TABLE threads (rollout_path TEXT, model_provider TEXT)"); + db.run("INSERT INTO threads VALUES (?, 'opencodex')",rollout); db.close(); }); const result=${kind === "sync" ? "restoreNativeCodex()" : "await restoreNativeCodexAsync()"}; @@ -469,7 +485,10 @@ describe("injectCodexConfig integration (Design B)", () => { expect(value.reachedSuccessfulWrite).toBe(true); if (migration === "none") { expect(value.result.success).toBe(true); - expect(value.result.artifacts.config.action).toBe(path === "journal" ? "journal-restored" : "owned-fields-stripped"); + expect([ + path === "journal" ? "journal-restored" : "owned-fields-stripped", + "routing-restored-provider-retained", + ]).toContain(value.result.artifacts.config.action); expect(value.result.artifacts.history).toMatchObject({state:"ok",rows:1}); expect(value.provider).toBe("openai"); expect(value.after[1]).toBeNull(); @@ -479,18 +498,21 @@ describe("injectCodexConfig integration (Design B)", () => { expect(value.afterState.state).toMatchObject({nativeGeneration:1,history:{status:"converged"},historySchedule:{direction:"remove"}}); return; } - expect(value.after).toEqual(value.before); + expect(value.result.success).toBe(true); + expect(value.result.historyPreflightRefusal).toBeUndefined(); + expect(value.result.artifacts.config).toMatchObject({ + state: "partial", + changed: true, + action: "routing-restored-provider-retained", + retained: { reason: "history_paginated_requires_native_writer" }, + }); + expect(value.result.retainedCodexProviderTable).toEqual(value.result.artifacts.config.retained); + expect(value.result.artifacts.history).toMatchObject({ state: "skipped", changed: false }); expect(value.provider).toBe("opencodex"); - if (kind === "coordinated") { - expect(value.beforeState).toMatchObject({kind:"ready",state:{nativeGeneration:0,currentTxId:null}}); - } - expect(value.afterState).toEqual(value.beforeState); - expect(value.result.success).toBe(false); - expect(value.result.message).toContain("history_paginated_requires_native_writer"); - expect(value.result.artifacts.config).toMatchObject({ state: "failed", changed: false }); - for (const artifact of [value.result.artifacts.catalog, value.result.artifacts.history]) { - expect(artifact).toMatchObject({ state: "skipped", changed: false }); - } + expect(value.after[0]).not.toContain('model_provider="opencodex"'); + expect(value.after[0]).toContain("[model_providers.opencodex]"); + expect(value.after[5]).toBe(value.before[5]); + expect(value.after[6]).toBe(value.before[6]); }); } @@ -525,11 +547,11 @@ describe("injectCodexConfig integration (Design B)", () => { // table this home already had survives the write even in the root-override form. expect(readFileSync(configPath,"utf8")).toContain("[model_providers.opencodex]"); - // Removing routing while those rows stay routed would orphan them, so restore keeps its - // refusal here. Making an already-paginated home uninstallable is tracked separately. + // Routing can come out without rewriting these rows. The table remains as thread-resolution + // state, and each entry point reports the degraded result as a successful partial restore. const restoreScript = ` const { restoreNativeCodex, restoreNativeCodexAsync, removeCodexConfig } = require("./src/codex/inject"); - const results = [restoreNativeCodex(), await restoreNativeCodexAsync(), removeCodexConfig()]; + const results = [restoreNativeCodex(), await restoreNativeCodexAsync(), removeCodexConfig({ retainProviderTable: true })]; console.log(JSON.stringify(results)); `; const restored = spawnSync(process.execPath, ["--eval", restoreScript], { @@ -537,11 +559,194 @@ describe("injectCodexConfig integration (Design B)", () => { encoding: "utf8", timeout: SPAWN_BUDGET_MS - 5_000, }); expect(restored.status).toBe(0); - for (const outcome of JSON.parse(restored.stdout)) expect(outcome.success).toBe(false); - expect(readFileSync(configPath,"utf8")).toContain("[model_providers.opencodex]"); + const outcomes = JSON.parse(restored.stdout); + for (const outcome of outcomes) expect(outcome.success).toBe(true); + for (const outcome of outcomes.slice(0, 2)) { + expect(outcome.historyPreflightRefusal).toBeUndefined(); + expect(outcome.artifacts.config).toMatchObject({ + state: "partial", + action: "routing-restored-provider-retained", + retained: { reason: "history_paginated_requires_native_writer" }, + }); + expect(outcome.artifacts.history).toMatchObject({ state: "skipped", changed: false }); + } + const restoredConfig = readFileSync(configPath,"utf8"); + expect(restoredConfig).not.toContain('model_provider = "opencodex"'); + expect(restoredConfig).toContain("[model_providers.opencodex]"); expect(readFileSync(rollout,"utf8")).toBe(bytes); }); + test.each([ + ["sync", false], + ["async", false], + ["sync", true], + ["async", true], + ] as const)("paginated %s restore removes every root route and honors removeProviderTable=%s", (kind, removeProviderTable) => { + const configPath = join(codexHome, "config.toml"); + const profilePath = join(codexHome, "opencodex.config.toml"); + const catalogPath = join(codexHome, "opencodex-catalog.json"); + const rolloutPath = join(codexHome, "paginated-contract.jsonl"); + const dbPath = join(codexHome, "state_5.sqlite"); + const providerBlock = [ + "# Auto-injected by opencodex", + "[model_providers.opencodex]", + 'name = "OpenCodex Proxy"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'wire_api = "responses"', + "", + "[model_providers.opencodex.env_http_headers]", + '"x-opencodex-api-key" = "OPENCODEX_API_AUTH_TOKEN"', + ]; + writeFileSync(configPath, [ + 'user_owned = "keep-me"', + 'model_provider = "opencodex"', + "# Auto-injected by opencodex", + 'openai_base_url = "http://127.0.0.1:10100/v1"', + "# Auto-injected by opencodex", + 'experimental_realtime_ws_base_url = "http://127.0.0.1:10100/v1"', + 'model = "vendor/routed-model"', + `model_catalog_json = ${JSON.stringify(catalogPath)}`, + "", + "[profiles.opencodex]", + 'model_provider = "opencodex"', + "", + ...providerBlock, + "", + "[user_table]", + 'value = "preserve"', + "", + ].join("\n")); + writeFileSync(profilePath, "# generated profile\n"); + writeFileSync(catalogPath, '{"models":[]}\n'); + const rolloutBytes = JSON.stringify({ + ordinal: 0, + type: "session_meta", + payload: { id: "paginated-contract", history_mode: "paginated", model_provider: "opencodex" }, + }) + "\n"; + writeFileSync(rolloutPath, rolloutBytes); + const db = new Database(dbPath); + db.run("CREATE TABLE threads (id TEXT PRIMARY KEY, rollout_path TEXT, model_provider TEXT, history_mode TEXT, user_note TEXT)"); + db.run("INSERT INTO threads VALUES ('paginated-contract', ?, 'opencodex', 'paginated', 'preserve-me')", rolloutPath); + db.close(); + const beforeRow = new Database(dbPath, { readonly: true }); + const rowBytes = JSON.stringify(beforeRow.query("SELECT * FROM threads WHERE id='paginated-contract'").get()); + beforeRow.close(); + + const script = ` + const { restoreNativeCodex, restoreNativeCodexAsync } = require("./src/codex/inject"); + const options = { removeProviderTable: ${JSON.stringify(removeProviderTable)} }; + const result = ${kind === "sync" ? "restoreNativeCodex(options)" : "await restoreNativeCodexAsync(options)"}; + console.log(JSON.stringify(result)); + `; + const child = spawnSync(process.execPath, ["--eval", script], { + cwd: repoRoot, + env: { ...process.env, CODEX_HOME: codexHome, OPENCODEX_HOME: ocxHome }, + encoding: "utf8", + timeout: SPAWN_BUDGET_MS - 5_000, + }); + expect(child.status, child.stderr).toBe(0); + const result = JSON.parse(child.stdout); + const restored = readFileSync(configPath, "utf8"); + const root = Bun.TOML.parse(restored); + const afterRow = new Database(dbPath, { readonly: true }); + const restoredRowBytes = JSON.stringify(afterRow.query("SELECT * FROM threads WHERE id='paginated-contract'").get()); + afterRow.close(); + + expect(result.success).toBe(true); + expect(result.historyPreflightRefusal).toBeUndefined(); + expect(result.artifacts.history).toMatchObject({ state: "skipped", changed: false, rows: 0, files: 0 }); + expect(root.user_owned).toBe("keep-me"); + expect(root.user_table).toEqual({ value: "preserve" }); + expect(root.model_provider).toBeUndefined(); + expect(root.openai_base_url).toBeUndefined(); + expect(root.experimental_realtime_ws_base_url).toBeUndefined(); + expect(root.model).toBeUndefined(); + expect(root.model_catalog_json).toBeUndefined(); + expect(restored).not.toContain("[profiles.opencodex]"); + expect(existsSync(profilePath)).toBe(false); + expect(readFileSync(rolloutPath, "utf8")).toBe(rolloutBytes); + expect(restoredRowBytes).toBe(rowBytes); + // Upstream rejects the whole config when this root id has no matching table. Every + // output, including explicit full removal, must avoid that catastrophic combination. + expect(root.model_provider === "opencodex" && !restored.includes("[model_providers.opencodex]")).toBe(false); + if (removeProviderTable) { + expect(result.retainedCodexProviderTable).toBeUndefined(); + expect(result.artifacts.config.state).toBe("ok"); + expect(restored).not.toContain("[model_providers.opencodex]"); + } else { + expect(result.artifacts.config).toMatchObject({ + state: "partial", + action: "routing-restored-provider-retained", + retained: { reason: "history_paginated_requires_native_writer", lines: providerBlock }, + }); + expect(result.retainedCodexProviderTable).toEqual(result.artifacts.config.retained); + expect(result.retainedCodexProviderTable.followUp).toContain("ocx restore --remove-codex-provider-table"); + expect(restored).toContain(providerBlock.join("\n")); + } + }); + + test("restore, stop teardown, and uninstall restore are idempotent on a paginated home", () => { + const configPath = join(codexHome, "config.toml"); + const rolloutPath = join(codexHome, "paginated-idempotent.jsonl"); + const dbPath = join(codexHome, "state_5.sqlite"); + writeFileSync(configPath, [ + 'user_owned = "survives-every-pass"', + 'model_provider = "opencodex"', + "# Auto-injected by opencodex", + "[model_providers.opencodex]", + 'name = "OpenCodex Proxy"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'wire_api = "responses"', + "", + ].join("\n")); + const rolloutBytes = JSON.stringify({ + ordinal: 0, + type: "session_meta", + payload: { id: "paginated-idempotent", history_mode: "paginated", model_provider: "opencodex" }, + }) + "\n"; + writeFileSync(rolloutPath, rolloutBytes); + const db = new Database(dbPath); + db.run("CREATE TABLE threads (id TEXT PRIMARY KEY, rollout_path TEXT, model_provider TEXT, history_mode TEXT, user_note TEXT)"); + db.run("INSERT INTO threads VALUES ('paginated-idempotent', ?, 'opencodex', 'paginated', 'unchanged')", rolloutPath); + db.close(); + const before = new Database(dbPath, { readonly: true }); + const rowBytes = JSON.stringify(before.query("SELECT * FROM threads WHERE id='paginated-idempotent'").get()); + before.close(); + + const script = ` + const { restoreNativeCodex, restoreNativeCodexAsync } = require("./src/codex/inject"); + const { performStopTeardown } = require("./src/server/stop-teardown"); + const restored = restoreNativeCodex(); + const stopped = await performStopTeardown(new URL("http://127.0.0.1:10100/api/stop"), { + restoreNativeCodex: () => restoreNativeCodexAsync(), + stripGrok: () => ({ ok: true, changed: false, message: "clean" }), + }); + const uninstalled = await restoreNativeCodexAsync(); + console.log(JSON.stringify({ restored, stopped, uninstalled })); + `; + const child = spawnSync(process.execPath, ["--eval", script], { + cwd: repoRoot, + env: { ...process.env, CODEX_HOME: codexHome, OPENCODEX_HOME: ocxHome }, + encoding: "utf8", + timeout: SPAWN_BUDGET_MS - 5_000, + }); + expect(child.status, child.stderr).toBe(0); + const outcomes = JSON.parse(child.stdout); + const finalConfig = readFileSync(configPath, "utf8"); + const after = new Database(dbPath, { readonly: true }); + const restoredRowBytes = JSON.stringify(after.query("SELECT * FROM threads WHERE id='paginated-idempotent'").get()); + after.close(); + + expect(outcomes.restored.success).toBe(true); + expect(outcomes.stopped).toMatchObject({ success: true, sharedTeardown: "performed" }); + expect(outcomes.uninstalled.success).toBe(true); + expect(finalConfig).toContain('user_owned = "survives-every-pass"'); + expect(finalConfig).not.toContain('model_provider = "opencodex"'); + expect(finalConfig).toContain("[model_providers.opencodex]"); + expect(readFileSync(rolloutPath, "utf8")).toBe(rolloutBytes); + expect(restoredRowBytes).toBe(rowBytes); + }); + test("a paginated home still receives the model catalog path the picker reads", () => { // The user-visible regression this pins. A paginated rollout made the injector refuse // the whole write, so `model_catalog_json` never reached config.toml: the Codex app and diff --git a/tests/codex-integration/codex-inject.test.ts b/tests/codex-integration/codex-inject.test.ts index 01518fa445..7504079da3 100644 --- a/tests/codex-integration/codex-inject.test.ts +++ b/tests/codex-integration/codex-inject.test.ts @@ -8,6 +8,7 @@ import { buildProviderTableBlock, chooseCatalogPathForInjection, dominantEol, + extractOcxProviderTableBlock, setRootOpenaiBaseUrl, setRootRealtimeWsBaseUrl, stripInjectedOpenaiBaseUrl, @@ -597,6 +598,39 @@ describe("Design B openai_base_url injection", () => { expect(stripped).toContain('model = "gpt-5.5"'); }); + test("provider-table capture ignores the identical marker on the root base-url override", () => { + const content = [ + "# Auto-injected by opencodex", + 'openai_base_url = "http://127.0.0.1:10100/v1"', + 'model = "vendor/routed-model"', + "", + "# Auto-injected by opencodex", + "[model_providers.opencodex]", + 'name = "OpenCodex Proxy"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'wire_api = "responses"', + "", + "[model_providers.opencodex.env_http_headers]", + '"x-opencodex-api-key" = "OPENCODEX_API_AUTH_TOKEN"', + "", + "[agents]", + "max_concurrent_threads_per_session = 8", + "", + ].join("\n"); + + expect(extractOcxProviderTableBlock(content)).toBe([ + "# Auto-injected by opencodex", + "[model_providers.opencodex]", + 'name = "OpenCodex Proxy"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'wire_api = "responses"', + "", + "[model_providers.opencodex.env_http_headers]", + '"x-opencodex-api-key" = "OPENCODEX_API_AUTH_TOKEN"', + "", + ].join("\n")); + }); + test("legacy marker directly before the provider table survives the root strip order (removeOcxSection keeps its anchor)", () => { // No Design B form present — stripInjectedOpenaiBaseUrl must not eat the legacy EOF marker // in a way that leaves the [model_providers.opencodex] table behind. diff --git a/tests/codex-integration/codex-restore-app-rewrite.test.ts b/tests/codex-integration/codex-restore-app-rewrite.test.ts index d98586feea..fb9165ede6 100644 --- a/tests/codex-integration/codex-restore-app-rewrite.test.ts +++ b/tests/codex-integration/codex-restore-app-rewrite.test.ts @@ -220,7 +220,7 @@ describe("#1798 restore after the Codex app rewrites the config", () => { profileExistsAfterRestore: boolean; }; expect(result.success).toBe(true); - expect(result.action).toBe("owned-fields-stripped"); + expect(["owned-fields-stripped", "routing-restored-provider-retained"]).toContain(result.action); expect(result.beforeRestore).toContain('approval_policy = "never"'); expect(result.beforeRestore).toContain("127.0.0.1:10200"); expect(result.afterRestore).toContain('approval_policy = "never"'); diff --git a/tests/service/stop-deferred-teardown.test.ts b/tests/service/stop-deferred-teardown.test.ts index 680a11bf07..76ad5b7449 100644 --- a/tests/service/stop-deferred-teardown.test.ts +++ b/tests/service/stop-deferred-teardown.test.ts @@ -115,21 +115,48 @@ describe("parent CLI shared teardown completion", () => { expect(outcome.receiptExists).toBe(false); }); + test("a paginated degraded restore releases its receipt and exits successfully", async () => { + const retained = { + reason: "history_paginated_requires_native_writer" as const, + lines: ["# Auto-injected by opencodex", "[model_providers.opencodex]"], + followUp: "Remove the table explicitly only if tagged conversations may stop opening.", + }; + const restore = { + success: true, + message: "Native routing restored; provider table retained.", + retainedCodexProviderTable: retained, + artifacts: { + config: { state: "partial", action: "routing-restored-provider-retained", retained }, + catalog: { state: "ok" }, + history: { state: "skipped" }, + }, + } as unknown as CodexNativeRestoreResult; + const outcome = await runParentStop({ receipt: true, + response: { success: true, sharedTeardown: "deferred" }, restore }); + expect(outcome.calls).toMatchObject({ killed: 0, native: 1, grok: 1, cleared: 1 }); + expect(outcome.exitCode).toBe(0); + expect(outcome.receiptExists).toBe(false); + }); + /** - * #4718: a refusal that happens BEFORE anything is restored. + * #4718, still live after #4812: a refusal that happens BEFORE anything is restored. + * + * The paginated-history reason no longer reaches this shape — it takes routing down and + * reports `partial`, which the test above pins. Every OTHER preflight reason still + * refuses ahead of the config half, so every artifact comes back untouched rather than + * failed. `handleStop` had no branch for that shape and fell through to the generic + * failure, which exited 1 — and the updater reads 1 as "the proxy would not stop" and + * aborts with the service already down. The obligation really is still owed, so the + * receipt has to stay; what was wrong was calling it a stop failure. * - * A paginated Codex history store makes the preflight refuse ahead of the config half, - * so every artifact comes back untouched rather than failed. `handleStop` had no branch - * for that shape and fell through to the generic failure, which exited 1 — and the - * updater reads 1 as "the proxy would not stop" and aborts with the service already - * down. The obligation really is still owed, so the receipt has to stay; what was wrong - * was calling it a stop failure. + * This case is easy to lose while narrowing the paginated reason, and losing it would + * silently retire exit code 80 along with the updater contract that reads it. */ - test("a history-preflight refusal keeps its receipt and reports the deferred code", async () => { + test("a non-paginated preflight refusal keeps its receipt and reports the deferred code", async () => { const restore = { success: false, - message: "Native restore refused: history_paginated_requires_native_writer. Config, catalog, history and provenance were preserved.", - historyPreflightRefusal: "history_paginated_requires_native_writer", + message: "Native restore refused: history_state_database_missing. Config, catalog, history and provenance were preserved.", + historyPreflightRefusal: "history_state_database_missing", artifacts: { config: { state: "skipped" }, catalog: { state: "skipped" }, history: { state: "skipped" } }, } as unknown as CodexNativeRestoreResult; const outcome = await runParentStop({ receipt: true, @@ -163,8 +190,8 @@ describe("parent CLI shared teardown completion", () => { // so a run that damaged it must keep failing the stop however it got there. const restore = { success: false, - message: "Native restore refused: history_paginated_requires_native_writer.", - historyPreflightRefusal: "history_paginated_requires_native_writer", + message: "Native restore refused: history_rollout_record_invalid.", + historyPreflightRefusal: "history_rollout_record_invalid", artifacts: { config: { state: "failed" }, catalog: { state: "skipped" }, history: { state: "skipped" } }, } as unknown as CodexNativeRestoreResult; const outcome = await runParentStop({ receipt: true, @@ -248,6 +275,36 @@ describe("performStopTeardown", () => { expect(body.message).toContain("native Codex restored"); }); + test("a degraded stop reports the retained provider table without turning success into deferral", async () => { + const retained = { + reason: "history_paginated_requires_native_writer" as const, + lines: ["# Auto-injected by opencodex", "[model_providers.opencodex]"], + followUp: "Run the explicit removal command only if tagged conversations may stop opening.", + }; + const body = await performStopTeardown(new URL("http://127.0.0.1:10100/api/stop"), { + ownsReceipt: () => false, + restoreNativeCodex: async () => ({ + ...restoreResult(true), + retainedCodexProviderTable: retained, + artifacts: { + ...restoreResult(true).artifacts, + config: { + state: "partial", + changed: true, + action: "routing-restored-provider-retained", + message: "routing restored", + retained, + }, + }, + }), + stripGrok: () => ({ ok: true, changed: false, message: "clean" }), + }); + + expect(body).toMatchObject({ success: true, sharedTeardown: "performed" }); + expect(body.message).toContain("[model_providers.opencodex]"); + expect(body.message).toContain("history_paginated_requires_native_writer"); + }); + test("a receipt-backed deferral touches neither config and says so", async () => { let restored = 0; let stripped = 0; From e6b87082795654e64d54fba1254ffe207bcea247 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 17 Sep 2026 18:32:16 +0900 Subject: [PATCH 3/5] fix(codex): keep the new teardown helpers off the inject facade The file-size ratchet caught src/codex/inject.ts at 998 lines against a committed cap of 987: the previous commit had added nine passthrough re-exports and a comment to it. That cap is the right answer here rather than an obstacle. The new helpers are teardown reporting, owned by inject/remove.ts and inject/restore.ts, and routing them through the facade added nothing except a second name for each. The three consumers import from the owning modules instead, and the dangling comment left behind when HISTORY_RELABEL_STANDS_DOWN moved to history-provider.ts is removed with it. No behavior change. --- src/cli/index.ts | 11 ++++++++--- src/codex/inject.ts | 17 ----------------- src/server/stop-teardown.ts | 2 +- src/service/cli.ts | 3 ++- tests/codex-integration/codex-inject.test.ts | 2 +- 5 files changed, 12 insertions(+), 23 deletions(-) diff --git a/src/cli/index.ts b/src/cli/index.ts index 1cb1213c60..8f6500f21a 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -15,13 +15,18 @@ try { } import { currentExternalCodexModelProvider, - describeRetainedCodexProviderTable, - readOcxProviderTableBlock, restoreNativeCodex, restoreNativeCodexAsync, shouldInjectApiAuthHeader, - type RetainedCodexProviderTable, } from "../codex/inject"; +// Straight from the owning modules rather than the facade: these are teardown-reporting +// helpers, not part of the injection surface, and `inject.ts` sits under a size cap that +// exists to stop it collecting exactly this kind of passthrough. +import { readOcxProviderTableBlock } from "../codex/inject/remove"; +import { + describeRetainedCodexProviderTable, + type RetainedCodexProviderTable, +} from "../codex/inject/restore"; import { stripGrokConfig } from "../grok/inject"; import { STOP_HISTORY_DEFERRED_EXIT_CODE, STOP_HISTORY_INCOMPLETE_EXIT_CODE } from "../update/stop-contract.mjs"; import { diff --git a/src/codex/inject.ts b/src/codex/inject.ts index 7d9d838af8..3318a6a28d 100644 --- a/src/codex/inject.ts +++ b/src/codex/inject.ts @@ -163,14 +163,6 @@ export interface CodexInjectResult { nativeSubagentDefaultsWarning?: string; } -/** - * The one history preflight reason that is permanent rather than operational: Codex owns - * paginated rollout ordinals, so no retry makes the legacy relabel protocol available again. - * - * The constant now lives beside the guard that produces it, in `history-provider.ts`, so - * the apply and restore directions cannot disagree about which reason stands down. - */ - class CodexHistoryPreflightRefusal extends Error {} let historyArtifactStageForTests: ((stage: string) => void) | undefined; export function setHistoryArtifactStageForTests(hook: typeof historyArtifactStageForTests): void { @@ -969,14 +961,9 @@ export { export type { CodexRoutingKind } from "./inject/routing-classify"; export { - appendOcxProviderTableBlock, - extractOcxProviderTableBlock, - readOcxProviderTableBlock, removeCodexConfig, - retainOcxProviderTableOnDisk, stripOpencodexConfig, } from "./inject/remove"; -export type { RemoveCodexConfigOptions, RemoveCodexConfigResult } from "./inject/remove"; export type { CodexNativeRestoreResult, @@ -984,13 +971,9 @@ export type { CodexRestoreCatalogResult, CodexRestoreConfigResult, CodexRestoreHistoryResult, - RestoreHistoryDisposition, - RetainedCodexProviderTable, } from "./inject/restore"; export { failedHistoryRestoreFromOutcome, - describeRetainedCodexProviderTable, - resolveRestoreHistoryDisposition, restoreNativeCodex, restoreNativeCodexAsync, setBeforeRestoreConfigForTests, diff --git a/src/server/stop-teardown.ts b/src/server/stop-teardown.ts index 146f4255fc..a386b8ff86 100644 --- a/src/server/stop-teardown.ts +++ b/src/server/stop-teardown.ts @@ -72,7 +72,7 @@ export async function performStopTeardown(url: URL, io: StopTeardownIo = {}): Pr // Reporting a bare "restored" would put the caller in exactly the position #4812 // describes: a config they did not expect and no idea why it is there. const retained = restore.retainedCodexProviderTable - ? ` ${(await import("../codex/inject")).describeRetainedCodexProviderTable(restore.retainedCodexProviderTable)}` + ? ` ${(await import("../codex/inject/restore")).describeRetainedCodexProviderTable(restore.retainedCodexProviderTable)}` : ""; return { success: true, message: `Proxy stopping, native Codex restored.${retained}`, sharedTeardown: "performed" }; } diff --git a/src/service/cli.ts b/src/service/cli.ts index e1a6c3da49..094c9e197e 100644 --- a/src/service/cli.ts +++ b/src/service/cli.ts @@ -1,6 +1,7 @@ import { existsSync, unlinkSync } from "node:fs"; import { join } from "node:path"; -import { describeRetainedCodexProviderTable, restoreNativeCodexAsync } from "../codex/inject"; +import { restoreNativeCodexAsync } from "../codex/inject"; +import { describeRetainedCodexProviderTable } from "../codex/inject/restore"; import { stripGrokConfig } from "../grok/inject"; import { serviceApiTokenFilePath } from "../lib/service-secrets"; import { statusWinswRaw, type WinswStatus } from "../lib/winsw"; diff --git a/tests/codex-integration/codex-inject.test.ts b/tests/codex-integration/codex-inject.test.ts index 7504079da3..9f987e60b7 100644 --- a/tests/codex-integration/codex-inject.test.ts +++ b/tests/codex-integration/codex-inject.test.ts @@ -8,7 +8,6 @@ import { buildProviderTableBlock, chooseCatalogPathForInjection, dominantEol, - extractOcxProviderTableBlock, setRootOpenaiBaseUrl, setRootRealtimeWsBaseUrl, stripInjectedOpenaiBaseUrl, @@ -16,6 +15,7 @@ import { stripRootContextWindowOverrides, standaloneCodexRoutingTarget, } from "../../src/codex/inject"; +import { extractOcxProviderTableBlock } from "../../src/codex/inject/remove"; import { OCX_SECTION_MARKER, stripJournaledOpenaiBaseUrl } from "../../src/codex/injected-marker"; import { MANAGED_AGENTS_TABLE_MARKER, From b2448c16be8ca7b01b2cd28b038efb0d9ccee6c4 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 17 Sep 2026 18:34:18 +0900 Subject: [PATCH 4/5] fix(codex): let an explicit provider-table removal past the history preflight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught it: "paginated sync/async restore removes every root route and honors removeProviderTable" failed with success false. removeCodexConfig re-derived the history question with its own preflight and keyed the refusal on whether the table was being retained, so the one path that legitimately removes the table on a paginated home — the explicit --remove-codex-provider-table opt-in — was refused by a check its caller had already answered. Retention and the refusal are the same decision seen from two sides, so they are now one option instead of two booleans. The caller passes down what it resolved: refuse-on-any, stand-down-retain, or stand-down-remove. A direct caller that passes nothing keeps the original refusal on every reason, and only the stand-down reason can be resolved this way — every other reason still means the history state itself is wrong. --- src/codex/inject/remove.ts | 40 +++++++++++++------ src/codex/inject/restore.ts | 7 +++- .../codex-inject-integration.test.ts | 2 +- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/codex/inject/remove.ts b/src/codex/inject/remove.ts index 2d8e750549..fab8e95fe0 100644 --- a/src/codex/inject/remove.ts +++ b/src/codex/inject/remove.ts @@ -223,36 +223,50 @@ function hasOpencodexRouting(content: string): boolean { ); } +/** + * What the caller already decided about conversation history before calling. + * + * - `refuse-on-any` — nothing was decided, so re-derive and refuse on any refusal reason. + * This is the default, and it is what a direct caller gets. + * - `stand-down-retain` — a stand-down was accepted and `[model_providers.opencodex]` must + * survive, because the rows this home tagged `opencodex` stay tagged and resolve only + * through that table. Those conversations still open; their requests fail against a + * stopped proxy, which is an ordinary connection error. + * - `stand-down-remove` — a stand-down was accepted and the user explicitly asked for the + * table to go too, accepting that those conversations stop opening. + * + * One option rather than two booleans: retention and the refusal are the same decision seen + * from two sides, and splitting them is how the explicit-removal path ended up refused by a + * preflight its caller had already answered. + */ +export type RemoveCodexConfigHistoryDisposition = + | "refuse-on-any" + | "stand-down-retain" + | "stand-down-remove"; + export interface RemoveCodexConfigOptions { preserveProfile?: boolean; - /** - * Keep `[model_providers.opencodex]` on disk while every routing key comes out. - * - * Set when the history preflight stands the relabel down: the rows this home tagged - * `opencodex` stay tagged, and they resolve only through this table. Retaining it means - * those conversations still open — their requests fail against a stopped proxy, which is - * an ordinary connection error — while plain `codex` returns to the built-in provider. - */ - retainProviderTable?: boolean; + historyDisposition?: RemoveCodexConfigHistoryDisposition; } export interface RemoveCodexConfigResult { success: boolean; message: string; - /** The exact lines left on disk when `retainProviderTable` kept the table. */ + /** The exact lines left on disk when the disposition was `stand-down-retain`. */ retainedProviderTable?: string[]; } export function removeCodexConfig( options: RemoveCodexConfigOptions = {}, ): RemoveCodexConfigResult { + const historyDisposition = options.historyDisposition ?? "refuse-on-any"; const historyError = preflightCodexHistoryInjection(false, false); // The preflight answers "may I rewrite conversation history?". Routing removal is a // different question, and treating one answer as both is what left `ocx uninstall` // pointing a live config at a port it had just removed (#4812). Only the stand-down // reason is separable; every other reason still means something is wrong with the - // history state itself, and those keep the hard refusal. - if (historyError && !(options.retainProviderTable === true && historyError === HISTORY_RELABEL_STANDS_DOWN)) { + // history state itself, and those keep the hard refusal even for a caller that decided. + if (historyError && !(historyDisposition !== "refuse-on-any" && historyError === HISTORY_RELABEL_STANDS_DOWN)) { return { success: false, message: `Codex configuration preserved: ${historyError}. Native writer coordination is required.` }; } if (!existsSync(CODEX_CONFIG_PATH)) { @@ -279,7 +293,7 @@ export function removeCodexConfig( const stripped = stripOpencodexConfigResult(content, journaledBaseUrl, journaledRealtimeWsBaseUrl); // Captured from the pre-strip bytes: the strip is what removes the table, so reading it // afterwards would find nothing. - const retainedBlock = options.retainProviderTable === true + const retainedBlock = historyDisposition === "stand-down-retain" ? extractOcxProviderTableBlock(content) : null; const finalContent = retainedBlock === null diff --git a/src/codex/inject/restore.ts b/src/codex/inject/restore.ts index bf5fe2096c..e5815f0912 100644 --- a/src/codex/inject/restore.ts +++ b/src/codex/inject/restore.ts @@ -342,7 +342,6 @@ function restoreCodexConfigInlineImpl(kind: string, options: RestoreConfigOption if (disposition.kind === "refuse") { return { state: "failed", changed: false, action: "failed", message: `Codex configuration and journal preserved: ${disposition.reason}.` }; } - const retainProviderTable = disposition.kind === "stand-down" && disposition.retainProviderTable; // Captured unconditionally, not only when the stand-down is already known. // // Two different paths need bytes that only exist before the write. The journal restore @@ -366,7 +365,11 @@ function restoreCodexConfigInlineImpl(kind: string, options: RestoreConfigOption ? { success: true, message: "Codex config restored from opencodex journal.", retainedProviderTable: undefined as string[] | undefined } : removeCodexConfig({ preserveProfile: journal.profileRestored || journal.profileChanged, - retainProviderTable, + // The history question was resolved above; hand the answer down rather than making + // the transform re-derive it, which refused the explicit-removal path outright. + historyDisposition: disposition.kind === "stand-down" + ? disposition.retainProviderTable ? "stand-down-retain" : "stand-down-remove" + : "refuse-on-any", }); let retainedLines = restored.retainedProviderTable ?? null; if (restored.success) { diff --git a/tests/codex-integration/codex-inject-integration.test.ts b/tests/codex-integration/codex-inject-integration.test.ts index 52493130ef..e5af4692d6 100644 --- a/tests/codex-integration/codex-inject-integration.test.ts +++ b/tests/codex-integration/codex-inject-integration.test.ts @@ -551,7 +551,7 @@ describe("injectCodexConfig integration (Design B)", () => { // state, and each entry point reports the degraded result as a successful partial restore. const restoreScript = ` const { restoreNativeCodex, restoreNativeCodexAsync, removeCodexConfig } = require("./src/codex/inject"); - const results = [restoreNativeCodex(), await restoreNativeCodexAsync(), removeCodexConfig({ retainProviderTable: true })]; + const results = [restoreNativeCodex(), await restoreNativeCodexAsync(), removeCodexConfig({ historyDisposition: "stand-down-retain" })]; console.log(JSON.stringify(results)); `; const restored = spawnSync(process.execPath, ["--eval", restoreScript], { From bdaec47d25c418155481ab3618d99cfbe9334226 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 17 Sep 2026 18:35:39 +0900 Subject: [PATCH 5/5] test(stop): pin the restore success branch by shape, not by one line CI caught it: the source-oracle assertion in grok-lifecycle.test.ts expected the literal text "if (result.success) console.log", and that branch grew a body when a degraded restore had to report the provider table it retained. Pinned as two assertions instead, plus the preflight-refusal conjunction that decides whether a stop receipt stays owed. A degraded restore discharged its config obligation and must not be read as a deferral, so that condition is now part of what this test holds rather than something it happened to cover. --- tests/providers/xai/grok-lifecycle.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/providers/xai/grok-lifecycle.test.ts b/tests/providers/xai/grok-lifecycle.test.ts index 49d5929a10..c66242623a 100644 --- a/tests/providers/xai/grok-lifecycle.test.ts +++ b/tests/providers/xai/grok-lifecycle.test.ts @@ -328,7 +328,14 @@ describe("Grok fence lifecycle wiring", () => { test("handleStop treats an incomplete native Codex restore as a stop failure", () => { const restoreFn = sliceFn(CLI_SOURCE, "async function restoreSharedClientStateAfterStop(", "async function handleStop("); const stopFn = sliceFn(CLI_SOURCE, "async function handleStop(", "async function handleUninstall("); - expect(restoreFn).toContain("if (result.success) console.log"); + // The success branch grew a body when a degraded restore had to report the provider + // table it retained, so this pins the branch and its log separately rather than the + // one-line shape they used to share. + expect(restoreFn).toContain("if (result.success) {"); + expect(restoreFn).toContain("console.log(`↩️ ${result.message}`)"); + // A degraded restore is a discharged obligation, not a deferral: the refusal reason is + // what keeps a stop receipt owed, and it must stay part of that conjunction. + expect(restoreFn).toContain("result.historyPreflightRefusal !== undefined"); // Config or catalog failure is a real teardown failure - a client reads those. Only a // history-only failure is separable, and it still surfaces (#3008). expect(restoreFn).toContain('artifacts.config.state === "failed" || artifacts.catalog.state === "failed"');