fix(codex): auto-unarchive archived sessions before retry - #1088
Conversation
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. I will use the review rule guidelines. I will post one final review after the parallel checks finish. |
| commandType: "thread/unarchive", | ||
| ...recovery, | ||
| }); | ||
| return sendJsonRpcRequest(request); |
There was a problem hiding this comment.
🚨 slopcop/review — The retry can use a terminated provider process.
This request stores the original child process and request table. The recovery can start a replacement process.
The retry still sends to the original child. A closed input drops the write, so the request waits for the timeout.
Resolve the current process after recovery. Otherwise, fail quickly when the original process changed. Add a process-exit test.
| message: SendJsonRpcRequestArgs<TResult>["message"]; | ||
| resultSchema: SendJsonRpcRequestArgs<TResult>["resultSchema"]; | ||
| timeoutMs?: number; | ||
| recovery?: CodexArchivedSessionRecoveryArgs; |
There was a problem hiding this comment.
🚨 slopcop/review — The fork path does not use this recovery.
The startThread path also sends thread/fork. Its call at line 1149 does not supply recovery data.
Thus, an archived source can still make a fork or side chat fail. Add fork recovery and a focused test.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5
BB opens an archived Codex conversation and retries the action. Two paths can still fail during that recovery.
Findings
- P2: A provider exit can make the retry use the terminated process. The request can then wait 30 seconds and fail.
- P2: The
thread/forkpath does not pass recovery data. A fork from an archived source can still fail. - P3: The fork adapter still sends
persistExtendedHistory. Start and resume remove this stale field, and generated contracts omit it.
I would fix both P2 findings before merge. I would also align the fork persistence contract with the other creation paths.
Security and performance
I found no confirmed security issue. The provider has full local trust, and the error text does not select the target identifier.
Normal recovery adds one unarchive request and one retry. This work has constant cost.
Validation
git diff --checkpassed.- GitHub reports no CI checks for this head.
- I did not run code from the untrusted fork.
- A browser test does not apply because this runtime change has no browser flow.
- Add process tests for provider exit, fork, resume, and steer.
This is a comment-only review. I did not approve the pull request or request changes.
Address review feedback on the archived-session recovery. - Recover `thread/fork` too. A fork reads its source session, so an archived source failed the same way a resume did. - Resolve the provider process again before the retry. Unarchiving can replace a dead process, and the retry wrote to the old child's stdin. - Keep the archived-session error when unarchiving fails. That message names the session and the CLI command that fixes it. - Drop the stale `persistExtendedHistory` field from `thread/fork`. Codex does not know the field, and start and resume already dropped it. - Correct the `ephemeral` comment. Codex already defaults to non-ephemeral, so the field pins the value; it does not change it. Add runtime tests for the resume, fork, and failed-unarchive paths, and assert the fork adapter sends no `persistExtendedHistory`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🚨 I will post the combined findings after the parallel reviews finish. |
|
|
||
| // Unarchiving can replace a dead provider process, so resolve the | ||
| // process again instead of writing to the captured child's stdin. | ||
| const retryProc = requireProviderProcess({ |
There was a problem hiding this comment.
🚨 slopcop/review — The recovery stops a replacement process before the retry.
If the original Codex process exits, the exit handler clears its thread identity and configuration. Recovery then starts a replacement with no registered thread.
The idle cleanup at line 789 stops that replacement. This lookup then throws Provider "codex" is not running.
Keep the recovery process alive and attach the thread before the retry. Add a test where the fake provider reports the archive error and exits.
There was a problem hiding this comment.
Fixed the part I can demonstrate, and dropped the part I cannot.
Fixed. The recovery now reports the archived-session error whenever the recovery itself cannot run, with the underlying failure attached as cause. That message names the session and the codex unarchive command that fixes it. Added a test where the fake provider reports the archived error and then exits, per your suggestion.
Not fixed, deliberately. I did not add a guard to keep the replacement process alive. I could not write a test that distinguishes it, for two reasons:
- In the interleaving the test actually produces, the exit handler has not run yet when recovery starts.
ensureProvidertherefore finds the old process still in its map and returns it, so thethread/unarchivegoes to a dead stdin and is rejected by the exit handler. The idle cleanup is never reached. - In the interleaving you describe, where the exit handler has run first, the guard would keep the replacement alive — but I cannot schedule that interleaving from a test, and a
turn/startretry against a fresh process would fail anyway because the thread is not loaded there.
The user-visible guarantee holds in both cases now: the caller gets the archived-session error, never a bare Provider "codex" has exited. I preferred that over shipping a defensive branch with no test behind it.
The deeper issue — ensureProvider handing back a process whose child has exited but whose exit event has not fired — is pre-existing and affects every in-flight command, not just this recovery path. It belongs in its own change.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5
BB reopens an archived Codex session and retries the action. A provider restart can make BB close the new helper before the retry.
Finding
- P2: Recovery can stop the replacement process before the retry uses it.
A provider exit clears the old thread identity and configuration. Recovery starts a replacement process with no registered thread.
The idle cleanup stops that replacement. The next process lookup then throws Provider "codex" is not running.
Keep the replacement alive and attach the thread before the retry. Add a test where the fake provider reports the archive error and exits.
Other checks
I found no security issue. The provider error text does not control a command or select a different session.
The normal recovery path has constant cost. I found no other performance or concurrency issue.
The central recovery helper fits the current architecture. I found no material duplication or stale production use of persistExtendedHistory.
I did not start a browser test because this runtime change has no browser route.
Validation
- The
@bb/agent-runtimesuite passed all 814 tests. - The Turbo type check passed.
git diff --checkpassed.- All GitHub checks passed on the exact head.
- The final GPT-5.6 review confirmed the P2 finding.
This review only adds comments. I did not approve the pull request or request changes.
Address the SlopCop finding about a provider exit during recovery. A provider that exits while bb unarchives cannot be unarchived or retried. Report the archived-session error in that case, and attach the recovery failure as the cause. That message names the session and the CLI command that fixes it. A process-level error such as `Provider "codex" has exited` tells the user nothing actionable. Add a test where the fake provider reports the archived error and then exits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Why this is needed
bb reaps idle thread-scoped Codex processes after 30 minutes and later resumes them by provider thread ID. The persisted Codex session can also be archived independently, for example when its thread is archived in Codex Desktop. If bb later sends
thread/resume,turn/start, orturn/steerfor that provider thread, Codex rejects the request, and bb surfaces the failure, leaving the user to unarchive the session manually before trying again.What changed
The runtime now recognizes Codex's archived-session error, sends
thread/unarchivefor the recorded provider thread, and retries the original request once. Recovery is limited to Codex and to that specific error, so other providers and other failures keep their existing behavior.The recovery uses the existing command path for resume, turn start, and steer, including resume during thread reconfiguration. Turn-start bookkeeping still rolls back correctly if either the initial request or the retry fails.
Persisted Codex threads now start with
ephemeral: false. This lets Codex materialize the rollout on disk so bb can resume it after reaping the provider process. The obsoletepersistExtendedHistoryfield is no longer sent on start or resume.Validation
The regression test reuses the existing fake provider and makes it return the archived-session error until bb sends
thread/unarchive. This covers the recovery path without adding a separate test process or harness.pnpm exec turbo run test --filter=@bb/agent-runtime— 811 tests passedpnpm exec turbo run test --filter=@bb/host-daemon— 479 tests passedpnpm exec turbo run typecheck --filter=@bb/agent-runtime --filter=@bb/host-daemon— passedgit diff --check— passedLive verification
I also ran the source dev app from this branch with the real Codex CLI. I created a disposable thread, confirmed the initial Codex response, and archived only its underlying provider session with
codex archive. I then restarted the dev host daemon to recreate the provider process and sent a normal follow-up throughpnpm bb:dev thread tell --mode auto.The follow-up completed successfully and returned
BB_LIVE_RESTARTED_RECOVERY_OK. The Codex rollout record moved from its archived-session store back into the active session store, confirming that bb unarchived the provider session before retrying.This remains a draft for maintainer review.