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. 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..8f6500f21a 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -13,7 +13,20 @@ try { /* best-effort */ } } -import { currentExternalCodexModelProvider, restoreNativeCodex, restoreNativeCodexAsync, shouldInjectApiAuthHeader } from "../codex/inject"; +import { + currentExternalCodexModelProvider, + restoreNativeCodex, + restoreNativeCodexAsync, + shouldInjectApiAuthHeader, +} 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 { @@ -119,6 +132,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 +818,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 +834,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 +1384,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 +1533,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..3318a6a28d 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, @@ -163,12 +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. - */ -const HISTORY_RELABEL_STANDS_DOWN = "history_paginated_requires_native_writer"; - class CodexHistoryPreflightRefusal extends Error {} let historyArtifactStageForTests: ((stage: string) => void) | undefined; export function setHistoryArtifactStageForTests(hook: typeof historyArtifactStageForTests): void { diff --git a/src/codex/inject/remove.ts b/src/codex/inject/remove.ts index fb56b71a44..fab8e95fe0 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,52 @@ 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; + historyDisposition?: RemoveCodexConfigHistoryDisposition; +} + +export interface RemoveCodexConfigResult { + success: boolean; + message: string; + /** The exact lines left on disk when the disposition was `stand-down-retain`. */ + retainedProviderTable?: string[]; +} + export function removeCodexConfig( - options: { preserveProfile?: boolean } = {}, -): { success: boolean; message: string } { + options: RemoveCodexConfigOptions = {}, +): RemoveCodexConfigResult { + const historyDisposition = options.historyDisposition ?? "refuse-on-any"; 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 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)) { if (!options.preserveProfile && existsSync(CODEX_PROFILE_PATH)) unlinkSync(CODEX_PROFILE_PATH); @@ -166,13 +291,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 = historyDisposition === "stand-down-retain" + ? 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 +325,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..e5815f0912 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,25 @@ 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}.` }; + } + // 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 +362,51 @@ 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, + // 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) { // 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 +469,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 +480,7 @@ export async function restoreNativeCodexAsync( } async function restoreNativeCodexAsyncImpl( - options: { revalidateDesiredState?: boolean }, + options: { revalidateDesiredState?: boolean; removeProviderTable?: boolean }, ): Promise { const activeProvider = currentExternalCodexModelProvider(); if (activeProvider) { @@ -368,8 +501,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 +557,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 +600,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 +651,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 +677,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 +702,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 +732,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..a386b8ff86 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/restore")).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..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 { 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"; @@ -291,7 +292,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..e5af4692d6 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({ historyDisposition: "stand-down-retain" })]; 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..9f987e60b7 100644 --- a/tests/codex-integration/codex-inject.test.ts +++ b/tests/codex-integration/codex-inject.test.ts @@ -15,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, @@ -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/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"'); 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;