[codex] Preserve reviewer when resuming threads#30278
Conversation
Co-authored-by: Codex noreply@openai.com
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9cdee51c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| typesafe_overrides.approvals_reviewer = resumed_history.history.iter().rev().find_map(|item| { | ||
| let RolloutItem::TurnContext(turn_context) = item else { | ||
| return None; | ||
| }; | ||
| turn_context.approvals_reviewer |
There was a problem hiding this comment.
When resuming an existing rollout created before this new field was added, or one where the latest change was thread/settings/update before the next user turn, the latest reviewer is persisted on SessionConfigured/ThreadSettingsApplied events rather than on a TurnContext item. Because this scan only accepts TurnContext, it returns None and the later config load falls back to the current config, so auto-review threads can still silently resume as user-review in those cases; please fall back to those existing events when no TurnContext reviewer is present.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| if typesafe_overrides.approvals_reviewer.is_some() | ||
| || request_overrides.is_some_and(|overrides| overrides.contains_key("approvals_reviewer")) |
There was a problem hiding this comment.
This guard assumes Some means the resume request explicitly overrode the reviewer, but the codex exec resume path always fills ThreadResumeParams.approvals_reviewer from the resolved config (codex-rs/exec/src/lib.rs:1094, via a helper that always returns Some at 1144-1147). As a result, resuming an auto-review thread through exec with the current config/default set to user still exits here before reading the persisted rollout reviewer, preserving the exact silent switch this change is meant to prevent for that resume surface.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
2 participants
Why
A thread resumed without an explicit reviewer could pick up the reviewer from the current config instead of preserving the reviewer already in use by the thread. After an app restart, this meant a thread running with auto review could silently switch back to user review, and the next turn could continue under the wrong reviewer.
What changed
Persist the effective reviewer with each turn and restore the latest persisted value when the thread resumes. If the resume request explicitly provides a reviewer, that value still takes precedence.
Test plan
just test -p codex-protocoljust test -p codex-statejust test -p codex-rolloutjust test -p codex-app-server thread_resume_preserves_persisted_approvals_reviewer