feat: add task --thread <id> to resume a specific Codex thread - #719
feat: add task --thread <id> to resume a specific Codex thread#719y-cruce wants to merge 5 commits into
task --thread <id> to resume a specific Codex thread#719Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
--thread accepts any known thread ID without checking the thread’s repository. That can resume context from repo A while executing with repo B’s current cwd, which risks cross-repo context leakage and wrong-repo edits. Please verify the saved thread cwd/repository before resuming, with an explicit override only if cross-repo resume is intended.
…tory unless --allow-other-repo
|
Good catch, thanks. Added in the latest commit: before resuming, the thread id must appear in the current workspace's tracked task jobs (any Claude session). Otherwise the run fails before any resume request is sent, with an error pointing to Threads created outside the plugin (plain Tests cover: tracked thread from another Claude session resumes; untracked thread rejected in foreground and background; |
![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: 8ecb158f7b
ℹ️ 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".
| if (listJobs(workspaceRoot).some((job) => job.threadId === threadId)) { | ||
| return; |
There was a problem hiding this comment.
When a thread from repo A is first resumed in repo B with --allow-other-repo, the successful run records that thread ID in repo B's job state. On a later invocation this predicate therefore accepts task --thread <id> without the required override, even though the thread still contains repo A's history. Track whether a thread was admitted through the override, or verify its original workspace, rather than treating every local job record as proof of repository ownership.
Useful? React with 👍 / 👎.
| if (request.resumeThreadId && !request.allowOtherRepo) { | ||
| requireTrackedThreadForWorkspace(workspaceRoot, request.resumeThreadId); |
There was a problem hiding this comment.
When the state already contains the maximum 50 jobs and the requested thread appears only in the oldest record, runTrackedJob (or the background enqueue path) inserts the new job before this validation runs. saveState then prunes that oldest record via pruneJobs(...).slice(0, MAX_JOBS), so this check rejects a thread that was tracked when the command started and incorrectly demands --allow-other-repo. Validate against the pre-insertion state or preserve thread ownership separately from the bounded job history.
Useful? React with 👍 / 👎.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Rechecked the current 9b63301 head against the repository-scope guard. The original cross-repo concern is improved, but two ownership edges remain:
- A foreign thread resumed once with
--allow-other-repois then written into this repository's bounded job history. A latertask --thread <id>sees that local job record and accepts the thread without the override, even though its original provenance is still another repository. The override should not permanently convert foreign provenance into local ownership. handleTask()creates/persists the new job beforeexecuteTaskRun()callsrequireTrackedThreadForWorkspace(). If the requested thread exists only in the oldest record at the 50-job cap, creating the new job can prune the very evidence the validation needs and incorrectly reject a thread that was tracked at command start.
Could thread ownership/provenance be stored separately from bounded job history, or validated from a pre-insertion snapshot, with foreign-thread provenance retained across overridden resumes?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b6330164a
ℹ️ 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".
| const state = this.state(p.threadId); | ||
| if (state.turnId !== p.turnId) return false; | ||
| const question = { requestId: message.id, ...p, expiresAt: Date.now() + this.inputTimeoutMs }; | ||
| state.questions.push(question); |
There was a problem hiding this comment.
When a task's collaboration subagent calls request_user_input, this stores the question under the child thread ID. However, liveStatus and sendLiveCommand query only the parent job's thread, so /codex:status cannot display the question and /codex:answer cannot find it; after ten minutes the timer interrupts the child turn. Associate child-thread questions with the owning parent job or search the active thread tree when reporting and answering questions.
Useful? React with 👍 / 👎.
| if (job.jobClass !== "task" || job.status !== "running" || !job.threadId) { | ||
| throw new Error("This command requires a running task with a known thread and turn."); | ||
| } |
There was a problem hiding this comment.
When an adversarial-review turn invokes request_user_input, the shared broker accepts and exposes that request because the feature and handler apply globally, but this guard rejects /codex:answer for every review job. The displayed question therefore cannot be answered and remains blocked until the ten-minute timeout interrupts the review. Allow answer for any running job with a matching pending broker request while retaining the task-only restriction for live messages, or prevent these review turns from issuing input requests.
Useful? React with 👍 / 👎.
- Task threads declare a `notify_director` dynamic tool (experimentalApi enabled at initialize). The broker answers `item/tool/call` at once, stores the note on the thread's live state, and `status --wait` returns early with `hasNotifications`, acknowledging the notes it returned. Notes are capped at 400 characters and logged as `notified` progress. - `events --cwd <repo> [--poll-ms]` streams one line per job event (DONE, FAILED, QUESTION, NOTIFIED, STALLED) for a long-running monitor, acknowledging notes it delivers. - `task --sandbox <read-only|workspace-write|danger-full-access>` and `--network` override the sandbox derived from `--write`; the mode is persisted and rendered.
…solation - The shared broker tracks stream ownership per thread instead of one active stream, so several tasks run concurrently in one repository; notifications are routed to the owning socket, non-streaming requests are serialized, and busy is returned only when another socket owns the target thread. - The broker exits after 10 minutes without connections or streams (`--idle-timeout-ms`); `ensureBrokerSession` recovers from a dead broker and serializes cold starts. Job store writes are atomic and locked. - `events` and `status --wait` detect a dead owner process or an unreachable broker and mark the job failed; `--stall-ms` (default 15 minutes) reports STALLED. - Tests use isolated plugin data and broker endpoints and shut down only the brokers they created; previously a test run could send shutdown to a live session's broker.
0955bb3 to
251ef06
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 251ef06f89
ℹ️ 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".
| const reason = (options.ownerAlive ?? ownerProcessAlive)(job.pid) === false | ||
| ? "owner process exited" | ||
| : failures > 2 ? "broker unreachable" : null; |
There was a problem hiding this comment.
When a native or adversarial review falls back to a direct app-server in withAppServer (for example, because the configured broker endpoint is unavailable), its owner process and Codex turn can remain healthy while liveStatus necessarily reports the broker unavailable. After three polls, this branch marks that running review failed, so status --wait can return a false failure and events can permanently emit FAILED before the direct review completes. Persist the transport used by the job, or only use broker reachability as a liveness signal for jobs that require the broker.
Useful? React with 👍 / 👎.
| const notification = { id: crypto.randomBytes(6).toString("hex"), message: p.arguments.message, | ||
| turnId: p.turnId, receivedAt: new Date().toISOString() }; | ||
| state.notifications.push(notification); | ||
| this.notify({ method: "companion/notification", params: { threadId: p.threadId, ...notification } }); |
There was a problem hiding this comment.
When a collaboration child invokes the newly added notify_director tool, state is the child thread's state and the notification is stored there. /codex:status, events, and notification acknowledgement query only the parent job's thread ID, so the tool reports successful delivery but the note never appears through those user-facing paths and remains unacknowledged. Associate the notification with the root/owning task state or aggregate child-thread notifications when taking the parent snapshot.
Useful? React with 👍 / 👎.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Rechecked current head 251ef06. The two thread-ownership issues from my previous review are still present:
requireTrackedThreadForWorkspace()still treats any local job record containing the thread ID as repository ownership. A foreign thread resumed once with--allow-other-repois persisted into this repositorys
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Rechecked current head 251ef06. The two thread-ownership issues from my previous review are still present:
requireTrackedThreadForWorkspace()still treats any local job record containing the thread ID as repository ownership. A foreign thread resumed once with--allow-other-repois persisted into this repository's job history, so a latertask --thread <id>can resume it without the override. The override still effectively converts foreign provenance into local ownership.- Validation still happens after job-state mutation. Foreground
runTrackedJob()writes/upserts the new running record beforeexecuteTaskRun()callsrequireTrackedThreadForWorkspace(), and backgroundenqueueBackgroundTask()does the same before the worker validates. BecauseupsertJob()prunes the bounded history, the 50-job edge can still discard the oldest record that proves the requested thread was tracked at command start.
The new broker/liveness changes do not address either ownership boundary. I would keep thread provenance separate from bounded job history, or validate from an immutable pre-insertion snapshot and retain foreign provenance explicitly across overridden resumes.
2 participants
Summary
task --resume-lastcan only continue the newest finished task thread of the current Claude session in the repo. When a caller drives several Codex threads in one session (for example one thread per problem, or a review that ran as a task in between), the older threads become unreachable even though the app-server can resume them by id.This adds
task --thread <id>to resume a specific thread through the same path--resume-lastuses.Changes
plugins/codex/scripts/codex-companion.mjs: parse--thread, validate it (non-empty, not flag-like, mutually exclusive with--resume/--resume-last/--fresh), passresumeThreadIdthroughbuildTaskRequest/executeTaskRunin both foreground and--backgroundmodes. The thread id is not pre-written into the job record; it is stored by the existingThread readyprogress handling after a successful resume, so a failed resume of an unknown id does not pollute the next--resume-last.tests/runtime.test.mjs: resume by id (foreground and background), flag not leaked into the prompt, conflicting and invalid values rejected, failed resume does not pre-write the id.tests/commands.test.mjs,README.md,plugins/codex/commands/rescue.md,plugins/codex/agents/codex-rescue.md,plugins/codex/skills/codex-cli-runtime/SKILL.md: document the flag and its routing.No changes under
lib/;--resume-last,--resume, and--freshbehave as before. Version not bumped.Verification
npm test: the new tests pass; no new failures relative to the clean tree on the same machine (the remaining failures there are environment-relatedsetup/status/result/resolveStateDircases that fail identically without this change).task-resume-candidate,task --thread <older-id> "..."resumed the older thread (Thread ready (<older-id>)) and Codex answered from that thread's context.Notes
--threadintentionally bypasses the current-session filter used by--resume-last. It does not check repository ownership of the thread; the resume request carries the currentcwd.