feat(codex): import existing sessions - #5146
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| ); | ||
| } | ||
|
|
||
| function importedMessageId(input: { |
There was a problem hiding this comment.
🟠 High Layers/CodexSessionImport.ts:75
importedMessageId omits providerInstanceId, unlike importedThreadId and importCommandId which both include it. Importing the same native Codex thread through two different Codex provider instances produces identical messageId values for two separate T3 threads. Since the projection repository upserts messages globally by messageId, the second import overwrites the first import's projected messages, corrupting the first T3 thread's conversation history. Include providerInstanceId in the importedMessageId key so message IDs are unique per provider instance.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/CodexSessionImport.ts around line 75:
`importedMessageId` omits `providerInstanceId`, unlike `importedThreadId` and `importCommandId` which both include it. Importing the same native Codex thread through two different Codex provider instances produces identical `messageId` values for two separate T3 threads. Since the projection repository upserts messages globally by `messageId`, the second import overwrites the first import's projected messages, corrupting the first T3 thread's conversation history. Include `providerInstanceId` in the `importedMessageId` key so message IDs are unique per provider instance.
| if (truncated) break; | ||
| } | ||
| threads.sort((left, right) => right.updatedAt.localeCompare(left.updatedAt)); |
There was a problem hiding this comment.
🟡 Medium Layers/CodexThreadHistory.ts:264
listThreads stops scanning archived sessions entirely once the unarchived (archived: false) scan exceeds 500 candidates. When there are more than 500 unarchived sessions, no archived sessions are ever queried, even if some archived sessions are newer than unarchived entries that make the final cut. The later global updatedAt sort cannot recover records that were never fetched, so discovery does not return the 500 most recent eligible sessions across both archive states. This happens because the if (truncated) break; at line 264 exits the outer archived loop as soon as truncated is set in the first iteration, skipping the archived: true scan entirely. Consider scanning both archive states and applying the scan limit only across the combined result set, or performing the updatedAt sort and truncation after both scans complete.
} while (cursor);
}
+ if (threads.length > CODEX_THREAD_DISCOVERY_MAX_RESULTS) {
+ truncated = true;
+ }
threads.sort((left, right) => right.updatedAt.localeCompare(left.updatedAt));🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/CodexThreadHistory.ts around lines 264-266:
`listThreads` stops scanning archived sessions entirely once the unarchived (`archived: false`) scan exceeds 500 candidates. When there are more than 500 unarchived sessions, no archived sessions are ever queried, even if some archived sessions are newer than unarchived entries that make the final cut. The later global `updatedAt` sort cannot recover records that were never fetched, so discovery does not return the 500 most recent eligible sessions across both archive states. This happens because the `if (truncated) break;` at line 264 exits the outer `archived` loop as soon as `truncated` is set in the first iteration, skipping the `archived: true` scan entirely. Consider scanning both archive states and applying the scan limit only across the combined result set, or performing the `updatedAt` sort and truncation after both scans complete.
There was a problem hiding this comment.
🟠 High
When an imported Codex session with requireExistingThread: true starts successfully, start overwrites the session's resumeCursor with { threadId: providerThreadId }, dropping the requireExistingThread flag. On the next restart, allowResumeFallback evaluates to true (the default), so if the original native thread no longer exists, openCodexThread silently falls back to a fresh thread/start instead of failing — exactly the behavior requireExistingThread was added to prevent. The same drop happens in the thread/started handler. Preserve requireExistingThread when rewriting the resume cursor.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/CodexSessionRuntime.ts around line 920:
When an imported Codex session with `requireExistingThread: true` starts successfully, `start` overwrites the session's `resumeCursor` with `{ threadId: providerThreadId }`, dropping the `requireExistingThread` flag. On the next restart, `allowResumeFallback` evaluates to `true` (the default), so if the original native thread no longer exists, `openCodexThread` silently falls back to a fresh `thread/start` instead of failing — exactly the behavior `requireExistingThread` was added to prevent. The same drop happens in the `thread/started` handler. Preserve `requireExistingThread` when rewriting the resume cursor.
There was a problem hiding this comment.
Reviewed the new Codex session import service and its call sites against the Effect service conventions. Six findings, all in the new/changed Effect service code: a standalone ...Shape interface plus split service/layer modules and a ...Live layer name for a brand-new service, service tags imported as named imports at a service boundary, an error-construction helper, and two error models that carry free-text or constant message/detail instead of structural attributes.
Posted via Macroscope — Effect Service Conventions
| function importError(operation: string, message: string, cause?: unknown): CodexSessionImportError { | ||
| return new CodexSessionImportError({ | ||
| operation, | ||
| message, | ||
| ...(cause === undefined ? {} : { cause }), | ||
| }); | ||
| } |
There was a problem hiding this comment.
importError is a helper whose only behavior is new CodexSessionImportError({ ... }), which the conventions ask to avoid so error attributes and cause stay visible at the failure boundary. Consider constructing CodexSessionImportError inline at each failure/mapError site, or replacing this with a static factory on the error class if it starts performing real normalization or pass-through of existing domain errors.
Posted via Macroscope — Effect Service Conventions
| export class CodexSessionImportError extends Schema.TaggedErrorClass<CodexSessionImportError>()( | ||
| "CodexSessionImportError", | ||
| { | ||
| operation: TrimmedNonEmptyString, | ||
| message: TrimmedNonEmptyString, | ||
| cause: Schema.optional(Schema.Defect()), | ||
| }, | ||
| ) {} |
There was a problem hiding this comment.
This error stores an unstructured, caller-supplied message as its only real payload and an unconstrained operation string, so message is not derived from structural attributes. Consider constraining operation to Schema.Literals([...]) for the actual stages (resolve-project, resolve-provider, list-sessions, validate-sessions, read-sessions, create-thread, save-binding, ...) and deriving message from those attributes — or splitting the genuinely distinct user-facing failures into separate tagged error classes, since each one currently carries its own caller-visible sentence.
Posted via Macroscope — Effect Service Conventions
| import { ProviderInstanceRegistry } from "../Services/ProviderInstanceRegistry.ts"; | ||
| import { CodexSessionImport } from "../Services/CodexSessionImport.ts"; | ||
| import { ProviderSessionDirectory } from "../Services/ProviderSessionDirectory.ts"; |
There was a problem hiding this comment.
At a service boundary these local service modules should be imported as namespaces (as already done for OrchestrationEngine and ProjectionSnapshotQuery above) so the module shape stays visible. Consider import * as ProviderInstanceRegistry from "../Services/ProviderInstanceRegistry.ts" (and likewise for CodexSessionImport and ProviderSessionDirectory), updating the three usage sites to ProviderInstanceRegistry.ProviderInstanceRegistry, ProviderSessionDirectory.ProviderSessionDirectory, and CodexSessionImport.CodexSessionImport.
Posted via Macroscope — Effect Service Conventions
| export interface CodexSessionImportShape { | ||
| /** Discover importable sessions whose native workspace matches a T3 project. */ | ||
| readonly list: ( | ||
| input: CodexSessionListInput, | ||
| ) => Effect.Effect<CodexSessionListResult, CodexSessionImportError>; | ||
| /** Adopt selected native sessions into the project's T3 thread list. */ | ||
| readonly import: ( | ||
| input: CodexSessionImportInput, | ||
| ) => Effect.Effect<CodexSessionImportResult, CodexSessionImportError>; | ||
| } | ||
|
|
||
| export class CodexSessionImport extends Context.Service< | ||
| CodexSessionImport, | ||
| CodexSessionImportShape | ||
| >()("t3/provider/Services/CodexSessionImport") {} |
There was a problem hiding this comment.
The service interface should be declared inline in the Context.Service declaration rather than retained as a standalone ...Shape type. Consumers already refer to CodexSessionImport["Service"], so inlining is self-contained.
| export interface CodexSessionImportShape { | |
| /** Discover importable sessions whose native workspace matches a T3 project. */ | |
| readonly list: ( | |
| input: CodexSessionListInput, | |
| ) => Effect.Effect<CodexSessionListResult, CodexSessionImportError>; | |
| /** Adopt selected native sessions into the project's T3 thread list. */ | |
| readonly import: ( | |
| input: CodexSessionImportInput, | |
| ) => Effect.Effect<CodexSessionImportResult, CodexSessionImportError>; | |
| } | |
| export class CodexSessionImport extends Context.Service< | |
| CodexSessionImport, | |
| CodexSessionImportShape | |
| >()("t3/provider/Services/CodexSessionImport") {} | |
| export class CodexSessionImport extends Context.Service< | |
| CodexSessionImport, | |
| { | |
| /** Discover importable sessions whose native workspace matches a T3 project. */ | |
| readonly list: ( | |
| input: CodexSessionListInput, | |
| ) => Effect.Effect<CodexSessionListResult, CodexSessionImportError>; | |
| /** Adopt selected native sessions into the project's T3 thread list. */ | |
| readonly import: ( | |
| input: CodexSessionImportInput, | |
| ) => Effect.Effect<CodexSessionImportResult, CodexSessionImportError>; | |
| } | |
| >()("t3/provider/Services/CodexSessionImport") {} |
Posted via Macroscope — Effect Service Conventions
| return { list, import: importSessions }; | ||
| }); | ||
|
|
||
| export const CodexSessionImportLive = Layer.effect(CodexSessionImport, makeCodexSessionImport); |
There was a problem hiding this comment.
For a newly introduced service, the tag, make, and the layer should live in one canonical module, and the layer should be exported as layer. Consider hoisting Services/CodexSessionImport.ts + Layers/CodexSessionImport.ts into apps/server/src/provider/CodexSessionImport.ts with export const make = ... / export const layer = Layer.effect(CodexSessionImport, make), and updating server.ts to use CodexSessionImport.layer.
Posted via Macroscope — Effect Service Conventions
| function toThreadHistoryError(operation: "list" | "read") { | ||
| return (cause: CodexErrors.CodexAppServerError) => | ||
| new ProviderThreadHistoryError({ | ||
| provider: "codex", | ||
| operation, | ||
| detail: "Codex app-server could not read persisted thread history.", | ||
| cause, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This is a curried alias whose only behavior is constructing ProviderThreadHistoryError, and the detail it passes is a constant that just restates the tag while dropping the context that is available at the failure site (cwd for list, externalThreadId for read). Consider constructing the error where the failure occurs and capturing that stable context (e.g. cwd / externalThreadId) instead of a fixed detail string.
Posted via Macroscope — Effect Service Conventions
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0ac6907. Configure here.
| requestedModel, | ||
| serviceTier: options.serviceTier, | ||
| resumeThreadId: readResumeCursorThreadId(options.resumeCursor), | ||
| allowResumeFallback: options.resumeCursor?.requireExistingThread !== true, |
There was a problem hiding this comment.
Strict import flag lost on resume
High Severity
Imported Codex sessions lose their requireExistingThread: true flag from the resumeCursor after the first successful start. This causes subsequent resumes to incorrectly allow fallback to a new thread and impacts their identification as imported.
Reviewed by Cursor Bugbot for commit 0ac6907. Configure here.
ApprovabilityVerdict: Needs human review 3 blocking correctness issues found. This PR introduces a new feature (Codex session import) with new services, RPC endpoints, orchestration commands, and UI. Multiple high-severity review comments remain unresolved, including potential message ID collisions that could corrupt imported thread history. You can customize Macroscope's approvability policy. Learn more. |
1 participant
What changed
Adds an opt-in Import Codex sessions flow from a project's settings.
Why
This addresses #207 with a safe migration path for people moving work between Codex and T3 Code. It preserves access to the original Codex conversation while making the imported context visible as a T3 thread.
Implementation notes
thread/list,thread/read, andthread/resumecapabilities rather than parsing or moving Codex session files.UI verification
Verified in an isolated local T3 environment:
Validation
corepack pnpm exec vp test run apps/server/src/provider/Layers/CodexSessionImport.test.ts apps/server/src/provider/Layers/CodexSessionRuntime.test.ts apps/server/src/orchestration/decider.historyImport.test.ts apps/server/src/environment/ServerEnvironment.test.ts apps/server/src/server.test.tscorepack pnpm --filter t3 typecheckcorepack pnpm --filter @t3tools/contracts typecheckcorepack pnpm --filter @t3tools/client-runtime typecheckcorepack pnpm --filter @t3tools/web typecheckcorepack pnpm exec oxfmt --checkon the changed filesgit diff --checkChecklist
mainImplemented with Codex desktop (GPT-5.6 Terra) through T3 Code.
Note
Medium Risk
Touches orchestration event projection, provider session bindings, and Codex resume semantics; mistakes could create duplicate threads or break continuation on imported conversations, though idempotent command ids and strict resume cursors limit blast radius.
Overview
Adds an Import Codex sessions flow so users can adopt existing native Codex conversations into a T3 project from project settings.
The server exposes
codex.listSessionsandcodex.importSessionsWebSocket RPCs (read vs operate scopes), advertisescodexSessionImporton the environment descriptor, and wires aCodexSessionImportservice that lists sessions scoped to the project workspace, reads history through Codex app-server (thread/list,thread/read), and imports via a new internalthread.history.importorchestration command that creates a stopped thread with a text-only message snapshot in one transaction.Imported threads get deterministic ids and strict resume cursors (
requireExistingThread: true) soopenCodexThreaddoes not fall back to a fresh native thread if the original was deleted; ordinary Codex continuations keep the existing resume fallback.CodexThreadHistoryon the Codex driver filters ephemeral/sub-agent/exec sessions and caps discovery at 500 results.The web app adds
CodexSessionImportDialog(provider picker, refresh, batch limit of 50, already-imported state) and a sidebar project-settings entry gated on the capability flag. Client-runtime adds environment-scoped list/import atoms; contracts and user docs describe the deliberate non-mirror boundary.Reviewed by Cursor Bugbot for commit 0ac6907. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add Codex session import feature to discover and import native Codex conversations into T3 threads
CodexSessionImportservice (CodexSessionImport.ts) that lists importable Codex sessions and imports selected ones by creating T3 threads, dispatchingthread.history.importcommands, and persisting strict resume bindings (requireExistingThread: true).CodexThreadHistorysource (CodexThreadHistory.ts) backed by the Codex app-server child process to list and read native Codex conversations without modifying Codex storage.thread.history.importcommand, emittingthread.createdfollowed by orderedthread.message-sentevents withstreaming: false.codexListSessionsandcodexImportSessionsWebSocket RPCs with authorization scopes wired through the server runtime.CodexSessionImportDialogReact component (CodexSessionImportDialog.tsx) in the sidebar project actions panel, allowing users to select and import up to 50 sessions with provider selection, pagination, refresh, and toast feedback.CodexSessionRuntimeso sessions with strict bindings do not fall back to starting a new native thread if resume fails — the error is propagated instead.📊 Macroscope summarized 0ac6907. 20 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.