Review commands should persist their Codex threads (write rollouts) like task runs do #529

Activity

1. dinhnguyenminhhoang commented on Jul 26, 2026

   

   dinhnguyenminhhoang

   on Jul 26, 2026

   More actions

   Hi, I’d like to work on this issue.

   My proposed approach is:

   1. Make native review threads non-ephemeral in runAppServerReview.
   2. Pass persistThread: true for adversarial review runs.
   3. Add regression tests covering persistence for both review paths.

   Could you confirm that this approach is acceptable before I start?
2. Jerotheking commented on Sep 24, 2026

   

   Jerotheking

   on Sep 24, 2026

   More actions

   +1, and two findings from patching this locally on 1.0.6 that the proposed approach would still leave broken:

   1. Persisting the thread is necessary but not sufficient for codex resume <id>. With persistThread: true added to the adversarial-review call, the rollout is written to ~/.codex/sessions as expected. But codex exec resume <id> "<follow-up>" then fails with:

   thread/resume failed: thread <id> already has an active writer (code -32600)
   

   because the plugin's shared broker still holds the thread. So the codex resume <session-id> hint that /codex:status prints stays unusable while the broker is alive, even after the persistence fix.

   2. The plugin itself has no way to continue a review. task --resume / --resume-last go through findLatestResumableTaskJob, which only matches jobClass === "task", so the last review is never a candidate. Continuing through the broker avoids the writer conflict above, so this is the natural path. Locally I changed that predicate to also accept adversarial reviews:

   (job.jobClass === "task" || (job.jobClass === "review" && job.kind === "adversarial-review")) &&
   job.threadId &&

   With both changes, node scripts/codex-companion.mjs task --resume "<counter-argument>" resumes the adversarial review with its full context (verified: it answered with its own verdict and the exact line it had flagged). This is the use case that motivated it: sending the reviewer a counter-argument finding by finding and letting it defend, adjust, or withdraw each one.

   Suggestion for the fix: besides persisting review threads, either let task --resume-last pick up review jobs, or add an explicit --resume <threadId> (see #230 / #700), and make /codex:status print a resume command that works while the broker is running (the companion's, not codex resume). Native reviews (runAppServerReview) would need the same treatment; I only patched the adversarial path.
3. Jerotheking commented on Sep 24, 2026

   

   Jerotheking

   on Sep 24, 2026

   More actions

   Correction to my previous comment: widening the findLatestResumableTaskJob predicate is the wrong fix. task-resume-candidate uses the same function, so after an adversarial review /codex:rescue offers to "continue" the review thread, which is read-only (cf. #740) — "apply the top fix" then silently can't write. And each resumed review is recorded as a task job, so the next --resume falls back into the review thread again.

   What works cleanly (tested locally on 1.0.6):

   • keep findLatestResumableTaskJob exactly as upstream (tasks only), so --resume / /codex:rescue never pick up a review;
   • add a separate findLatestResumableReviewJob (jobClass === "review" && kind === "adversarial-review") and an explicit task --resume-review flag that resumes it through the broker, read-only, rejecting --write / --resume / --fresh;
   • record the resumed run as jobClass: "review", kind: "adversarial-review", so repeated --resume-review calls follow the same chain and task-resume-candidate still returns the last real task.

   Verified: after a task T and an adversarial review R, --resume-review resumes R, a following --resume resumes T (not R), and task-resume-candidate returns T.
4. CBEPX

   added a commit that references this issue on Sep 30, 2026

   fix(review): persist review threads under a review name (openai#529)

   ...

   b0e074d