enhancement: add an experimental, reversible Codex thread-history migration - #983
Draft
bbednarski9 wants to merge 10 commits into
Draft
Conversation
…ration Codex records the provider that produced each thread in `threads.model_provider` and filters its resume picker by the provider that is currently active. Installing the Relay integration switches Codex to the `nemo-relay-openai` provider, so threads recorded under the built-in `openai` provider stop appearing in the picker. They stay on disk and remain resumable by id; only discovery is affected. Add `nemo-relay install codex --migrate-history`, which rewrites the recorded provider so pre-install history stays visible, and reverse it automatically at `nemo-relay uninstall codex`. Reversal is inferred from a migration journal rather than a repeated flag, and moves every thread still recorded under `nemo-relay-openai` back to `openai`, not only the ids captured at migration time: uninstall removes that provider from config.toml, so a thread left pointing at it would be hidden from the picker in the same way the migration exists to prevent. Pass `--skip-history-migration` to opt out. The migration shells out to `sqlite3` rather than adding a SQLite crate to the workspace, takes the write lock up front so a running Codex fails cleanly instead of writing partial state, and copies the database and its WAL sidecars to a timestamped backup first. This is a stopgap. Codex owns the schema and offers no supported API for changing a thread's provider; see openai/codex#27381. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
The thread database file name carries a Codex schema generation, so the hardcoded `state_5.sqlite` stops resolving the moment Codex moves to `state_6.sqlite` or later. Add `--history-database` to `nemo-relay install codex` and `nemo-relay uninstall codex` so the current database can be named without waiting on a Relay release. A bare file name resolves inside the Codex home, which is the common case for a schema bump; a value with a directory component is used as given. At uninstall time the precedence is explicit flag, then the database the migration journal recorded, then the default, so an ordinary uninstall still reverses the right database without being told. Because the path can now come from a flag, validate the schema before copying or rewriting anything: reject a database with no `threads` table or no `model_provider` column. `--history-database` requires `--migrate-history` at install time rather than silently doing nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
…nce it The Codex history-migration section understated two things a reader needs before running it. `--skip-history-migration` does not merely hide threads from the resume picker: Codex resolves a thread's recorded provider against config.toml on resume, so a thread stranded on a removed provider fails to resume outright. Backups are also never pruned, and each is a full copy of the thread database. The host-neutral installation page was silent about the migration entirely. Its uninstall section enumerates what uninstall removes, so a reader had no way to learn that uninstall may also rewrite the Codex thread database. Cross-reference the Codex page from both the install and uninstall sections rather than duplicating the detail. Also state that Relay does not search for another schema generation when the expected database is missing, matching the explicit-over-discovery behavior of `--history-database`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
…l failed A history-migration failure was returned as `CliError::Install`, which `run_agent_operations` reports as "failed to install one or more integrations". That is wrong and sends the caller to the wrong remedy: the integration is installed and working, and only the opt-in migration failed. The same held for uninstall, where the integration is already removed. Report both as a nonzero exit status with a structured log event and a message naming the actual state and the recovery step, instead of an install error. Also cover the locked-database path with a test. The docs claim a running Codex produces a clean failure rather than partial state; the test holds a competing `BEGIN IMMEDIATE` transaction and asserts the error explains the remedy, the database is unchanged, and no journal is recorded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
…mitation `--migrate-history` shells out to `sqlite3`, which is present by default on macOS and most Linux distributions but not on Windows or in minimal container images. The requirement was mentioned in passing but its practical consequence was not, so a Windows user would only discover it at the point of failure. Call it out where it is decided rather than where it fails: a warning callout in the migration section, a note on the host-neutral installation page, and the `--migrate-history` help text. Also record that uninstall needs `sqlite3` only when a migration was recorded, and that the journal survives a failed reversal so a later uninstall can still complete it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
The install skill told agents never to touch Codex SQLite state as a migration workaround, because no supported path existed. `--migrate-history` is now that path, so the guidance and the generated recovery file were both stale in a way that would send an agent to the wrong answer. Keep the prohibition on hand-editing, which is still correct, and point it at the supported command instead. Add the flag as a third option alongside the two existing exits, and add a section to the recovery asset for keeping threads visible rather than recovering from their disappearance. Present it as experimental in both places, note the `sqlite3` requirement and its Windows limitation, and keep the recovery-file and confirmation steps in place: the flag changes Codex's own thread database. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
github-actions
Bot
added
size:XL
The journal was written after the provider update, so a journal write that failed left every thread moved to the Relay provider with no record of the migration. Uninstall infers reversal from that journal, so it would then report success while silently declining to reverse anything, leaving the threads on a provider that uninstall had just removed from config.toml. Resuming one fails outright. The backup still existed, so nothing was lost, but recovery required restoring it by hand. Write the journal first, so a journal failure aborts before the database is touched. Because the update is a single transaction, a failure after the write changed nothing, so clear the journal in that case rather than leaving one for a migration that never happened; the cleanup result is ignored so it cannot mask the original error. If the process dies between the two steps the journal survives, which reversal already tolerates: it finds no threads under the Relay provider, clears the journal, and makes no changes. Add a regression that a journal whose parent cannot be created leaves the providers untouched, and one that reversal clears a journal describing a migration that never completed. The first fails against the previous ordering. Found by review of the equivalent standalone script. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Reversal cleared the journal whenever the target database was missing, but the target can come from `--history-database`. A typo in that flag therefore deleted the record of a real, still-unreversed migration. That matters more here than in the standalone script: uninstall infers reversal from the journal, so once it is gone `nemo-relay uninstall codex` reports success while silently declining to move anything, leaving the threads on a provider it just removed from config.toml. Resuming one fails outright. The backup still existed, so nothing was lost, but recovery required restoring it by hand -- for a step whose purpose is reversibility, that is the wrong failure mode. Only a missing *recorded* database says the migration went stale, so clear the journal in that case alone. An override that does not resolve is a caller mistake: fail and say the journal was kept, so retrying with the right path is obviously safe. Add a regression that a failed reversal leaves the providers migrated and the journal in place, and that the correct invocation then still reverses the migration. That last step is the point -- it asserts the failure stayed recoverable, not merely that a file exists. It fails against the previous behavior. Found by review of the equivalent standalone script. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Migration never read the journal it was about to write. Migrating a second database with `--history-database` therefore overwrote the first recovery record, and reversal reads that record to find its target -- so uninstall repaired only the second database and cleared the journal, leaving the first stranded on `nemo-relay-openai`. Uninstall also removes that provider from config.toml, so those threads disappear from the picker and resuming one fails: the exact defect this migration exists to fix, inflicted by the migration itself. The backup still existed, so nothing was lost, but recovery required restoring it by hand. One journal describes one outstanding migration, so reject a target that conflicts with the recorded one and name both paths. Migrating the database the journal already records stays allowed: threads created after a migration land on the built-in provider, and re-migrating the same file loses nothing, since reversal moves every Relay-provider thread back rather than only the recorded ids. Paths are compared canonicalized, so a bare `--history-database` name and the absolute path the journal recorded are one database, not a conflict. Reading the journal first also means a journal path whose parent is a regular file now fails on the read rather than the write, so treat that like an absent journal: the write that follows still reports the directory it cannot create, which is the clearer error. That keeps the existing journal-write regression exercising the ordering it was written for. Add coverage that a second database is refused while a migration is outstanding, that the first stays reversible afterwards, and that re-migrating the recorded database still works. The first fails against the previous behavior. Found by review of the equivalent standalone script. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Codex records the provider that produced each thread in
threads.model_providerand filters its resume picker by the provider that is currently active. Installing the Relay integration switches Codex to thenemo-relay-openaiprovider, so threads recorded under the built-inopenaiprovider stop appearing in the picker. The threads stay on disk and remain resumable by id; only discovery is affected.This adds an experimental, reversible migration behind
nemo-relay install codex --migrate-historythat records existing threads under the Relay provider so they stay visible.nemo-relay uninstall codexreverses it automatically, with no flag needed at uninstall.This is a stopgap for the Relay side only. It does not change the upstream situation tracked in openai/codex#27381, where the built-in
openaiprovider cannot be pointed at a local gateway without also opting into a WebSocket transport Relay does not support.Details
New flags
--migrate-historyinstall codexopenaitonemo-relay-openaiafter a successful install--history-database <PATH>install codex,uninstall codexstate_5.sqlite--skip-history-migrationuninstall codex--dry-runworks with all of them and reports counts without touching the database or the journal.Reversibility. A migration writes a journal (owner-only) to the user configuration directory recording the database path, backup path, timestamp, direction, and migrated thread ids. Uninstall reads that journal and reverses the migration without being asked, then clears it.
integrations refreshdoes not re-run the migration; a recorded migration survives reinstalls and upgrades.Reversal moves every thread still recorded under
nemo-relay-openai, not only the ids the migration captured. Uninstall removes that provider fromconfig.toml, and Codex resolves a thread's recorded provider againstconfig.tomlon resume, so a thread left pointing at a removed provider fails to resume withModel provider `nemo-relay-openai` not found— the same defect the migration exists to prevent, mirrored.Safety. Before rewriting anything, Relay checkpoints the WAL and copies the database plus its sidecars to a timestamped
nemo-relay-history-backup-*directory in the Codex home.BEGIN IMMEDIATEtakes the write lock up front, so a running Codex produces a cleandatabase is lockedfailure naming the remedy rather than partial state. Because the database path can come from a flag, the schema is validated before any copy or write: a database with nothreadstable or nomodel_providercolumn is rejected.No new dependency. The migration shells out to
sqlite3rather than adding a SQLite crate to the workspace. That makessqlite3a runtime requirement for this one feature — present by default on macOS and most Linux distributions, but not on Windows or in minimal container images. This is documented in the Codex page, the installation page, and the flag's--help. No other Relay command depends on it.Explicit over discovery.
state_5.sqliteis pinned rather than discovered. When Codex bumps the schema generation, migration stops and names the path it expected instead of guessing at an untested schema;--history-databaseis the stopgap and the installer should be updated in a release.Failure semantics. A migration failure is reported as a nonzero exit status with a structured log event, not as
CliError::Install. Returning an install error maderun_agent_operationsreport "failed to install one or more integrations" when the integration was installed and working, which sent the caller to the wrong remedy.Docs and skills. New "Migrate Codex Thread History" section in the Codex page; cross-references from the host-neutral installation page's install and uninstall sections. The
nemo-relay-installskill previously told agents never to touch Codex SQLite state as a migration workaround, because no supported path existed — that guidance and its generated recovery file are updated to point at the supported command while keeping the prohibition on hand-editing.Where should the reviewer start?
crates/cli/src/agents/codex/history.rs— the whole feature is one module, and its doc comment states the constraint that shapes every decision in it: Codex owns the schema and offers no supported API for changing a thread's provider, so a direct database edit is the only available mechanism.The design decision most worth challenging is the reversal scope, documented on
restore_from_relay: reversal deliberately covers threads created while Relay was installed, not just the migrated ones.restore_also_moves_threads_created_while_relay_was_installedpins that behavior.Tests are in
crates/cli/tests/coverage/agents/codex_history_tests.rs(17 cases: round trip, journal contents, backup contents, both dry runs, the relay-era reversal rule, database override in both bare-name and full-path forms, both schema rejections, and a locked-database case that holds a competingBEGIN IMMEDIATEfrom a second process). They drive a realsqlite3against a temporary Codex home and skip whensqlite3is absent.Validation.
just test-rust(4477 passed),cargo clippy --workspace --all-targets -- -D warningsclean,cargo fmt --all,uv run pre-commit run --all-filespassing,just docsandjust docs-linkcheckclean. Exercised end-to-end against a real 1361-thread Codex database in both directions: migration moved 1361, reversal moved 1376 (including 15 Relay-era threads), the journal was cleared, andconfig.tomlwas left with no reference to the Relay provider.Not yet verified. Windows behavior — Rust CI covers
windows-2022andwindows-11-arm, but GitHub's Windows images do not shipsqlite3, so these tests will skip there and the run will still look green. Windows testing is in progress. Also unverified: whether a resumed migrated thread routes through Relay in a live session, and the stalemodel_providerleft in rollout JSONL session headers, which appears cosmetic because resume reads the provider from the database.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)