From 4ee7bda3c2202ee9348f9e6574213f6a04df01c7 Mon Sep 17 00:00:00 2001 From: Patrick Yang <266918795+patriyang@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:54:00 -0400 Subject: [PATCH 1/2] Shell-escape every dynamic value in the companion wait commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bounded-wait contract added in #94 built its command by interpolating `workspaceRoot` (or `WORKTREE_ROOT`) straight into double quotes and `jobId` bare. Double quotes do not neutralize an embedded `"`, a backtick, or `$(...)`: a checkout at `/tmp/repo$(id -un)` sends the companion a substituted path, and one containing `"` fails to parse at all. These markdown files are executable instructions, so that was the recipe the controller followed. Apply implement.md's existing shellEscape doctrine — one dynamic value per argument, `--` before the first positional — to the wait, persisted-result, and recovery commands in all five commands that carry the contract. implement.md defined the doctrine but had never applied it to its own companion or git invocations; it now derives `rootArg` in Pre-flight and uses it throughout. `status` and `result` accept a bare `--` (neither sets stopAtFirstPositional), so the job id still lands in positionals[0] with the flags ahead of it. `"${CLAUDE_PLUGIN_ROOT}"` stays double quoted: the shell expands it rather than the model interpolating a value into it. The shared cross-command tests now pin the escaped recipe rather than the raw one, and a new test rejects any double-quoted interpolation other than CLAUDE_PLUGIN_ROOT, plus any raw root/job/thread value on a companion or git command line. Closes #107 Co-Authored-By: Claude Opus 5 (1M context) --- plugins/codex/commands/adversarial-review.md | 16 ++-- plugins/codex/commands/deep-review.md | 16 ++-- plugins/codex/commands/implement.md | 57 +++++++------- plugins/codex/commands/rescue.md | 16 ++-- plugins/codex/commands/review.md | 16 ++-- tests/commands.test.mjs | 79 ++++++++++++++++---- 6 files changed, 138 insertions(+), 62 deletions(-) diff --git a/plugins/codex/commands/adversarial-review.md b/plugins/codex/commands/adversarial-review.md index 3fd630d54..58c8f4638 100644 --- a/plugins/codex/commands/adversarial-review.md +++ b/plugins/codex/commands/adversarial-review.md @@ -57,19 +57,25 @@ Background flow: ```bash node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" adversarial-review "--background --json $ARGUMENTS" ``` -- Extract `jobId` and `workspaceRoot` from that JSON and wait for that job in a foreground `Bash` call whose tool timeout is comfortably larger than the companion timeout: +- Extract `jobId` and `workspaceRoot` from that JSON and wait for that job in a foreground `Bash` call whose tool timeout is comfortably larger than the companion timeout. Both are dynamic values: shell-escape each exactly once before building the command. `shellEscape(value)` means robust shell argument escaping (for example, Bash `printf '%q' "$value"`). Keep `--` before the job ID, and keep `"${CLAUDE_PLUGIN_ROOT}"` double quoted because the shell expands it: ```typescript +const rootArg = shellEscape(workspaceRoot) +const jobArg = shellEscape(jobId) + Bash({ - command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C "${workspaceRoot}" ${jobId} --wait --timeout-ms 240000 --json`, + command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C ${rootArg} --wait --timeout-ms 240000 --json -- ${jobArg}`, description: "Wait for Codex adversarial review", timeout: 300000 }) ``` - On `waitTimedOut: true`, apply the PID-aware timeout branch in `status.md`; repeat the bounded wait only for a healthy job. After a terminal payload, load the stored result: -```bash -node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" result -C "${workspaceRoot}" --json +```typescript +Bash({ + command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" result -C ${rootArg} --json -- ${jobArg}`, + description: "Read persisted Codex adversarial review result" +}) ``` - Read `.storedJob.rendered` from the result JSON and return it verbatim in the same turn the terminal wait returns. Do not wait for "is it done?" or "continue". - If the enqueue or read fails, the job ends as `failed` or `cancelled`, or any JSON or review output is empty or malformed, report that and include the most actionable failure lines. A failed review must not vanish silently. - Never re-dispatch a second adversarial review over the same diff, and never substitute a "check `/codex:status`" note for its findings. -- If a dispatched adversarial review is unread when a later turn begins, recover its record with `status -C "${workspaceRoot}" --json`; continue the bounded wait if active, then use `result -C "${workspaceRoot}" --json` once terminal. Do not launch a duplicate or call the existing run stuck. +- If a dispatched adversarial review is unread when a later turn begins, recover its record with `status -C ${rootArg} --json -- ${jobArg}`; continue the bounded wait if active, then use `result -C ${rootArg} --json -- ${jobArg}` once terminal. Do not launch a duplicate or call the existing run stuck. diff --git a/plugins/codex/commands/deep-review.md b/plugins/codex/commands/deep-review.md index db41b8e90..734e08b75 100644 --- a/plugins/codex/commands/deep-review.md +++ b/plugins/codex/commands/deep-review.md @@ -56,19 +56,25 @@ Background flow: ```bash node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" deep-review "--background --json $ARGUMENTS" ``` -- Parse `jobId` and `workspaceRoot` from the enqueue response. Await it through foreground `Bash`, giving the tool a timeout comfortably above the bounded status wait: +- Parse `jobId` and `workspaceRoot` from the enqueue response. Await it through foreground `Bash`, giving the tool a timeout comfortably above the bounded status wait. Both are dynamic values: shell-escape each exactly once before building the command. `shellEscape(value)` means robust shell argument escaping (for example, Bash `printf '%q' "$value"`). Keep `--` before the job ID, and keep `"${CLAUDE_PLUGIN_ROOT}"` double quoted because the shell expands it: ```typescript +const rootArg = shellEscape(workspaceRoot) +const jobArg = shellEscape(jobId) + Bash({ - command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C "${workspaceRoot}" ${jobId} --wait --timeout-ms 240000 --json`, + command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C ${rootArg} --wait --timeout-ms 240000 --json -- ${jobArg}`, description: "Wait for Codex deep review", timeout: 300000 }) ``` - When the JSON says `waitTimedOut: true`, use the PID-aware timeout branch in `status.md` and re-arm only a healthy job. When it does not, retrieve the persisted deep-review result: -```bash -node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" result -C "${workspaceRoot}" --json +```typescript +Bash({ + command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" result -C ${rootArg} --json -- ${jobArg}`, + description: "Read persisted Codex deep-review result" +}) ``` - Read `.storedJob.rendered` from the result JSON and present it verbatim in the same turn the terminal wait returns; never wait for the user to ask "is it done?" or "continue". - If enqueueing or result retrieval exits non-zero, the job becomes `failed` or `cancelled`, or the JSON or review output is empty or malformed, report the failure with the most actionable lines. A failed review must not vanish silently. - Never re-dispatch another deep review over the same diff, and never replace the findings with a "check `/codex:status`" note. -- If a later turn inherits a dispatched-but-unread deep review, use `status -C "${workspaceRoot}" --json` to recover it from disk, resume the bounded wait while active, and call `result -C "${workspaceRoot}" --json` after it is terminal. Do not re-dispatch or describe the job as stuck. +- If a later turn inherits a dispatched-but-unread deep review, use `status -C ${rootArg} --json -- ${jobArg}` to recover it from disk, resume the bounded wait while active, and call `result -C ${rootArg} --json -- ${jobArg}` after it is terminal. Do not re-dispatch or describe the job as stuck. diff --git a/plugins/codex/commands/implement.md b/plugins/codex/commands/implement.md index a41c01b18..2a89edc4d 100644 --- a/plugins/codex/commands/implement.md +++ b/plugins/codex/commands/implement.md @@ -13,7 +13,7 @@ Raw slash-command arguments: ## Git metadata writes -The controller owns Git metadata writes; implementers edit and test only. This includes linked-worktree metadata under `.git/worktrees/` and the representative operations below; apply the same rule to any other Git command that writes metadata. Shell escaping and Git option termination are separate defenses: shell escaping keeps each dynamic value as one argument, while Git's `--` must appear before the first dynamic positional operand so a value beginning with `-` cannot be interpreted as a Git option. Shell-escape every dynamic value as exactly one shell argument before constructing an escalated command. In the examples, `shellEscape(value)` means a robust shell-argument escaping step (for example, Bash `printf '%q' "$value"`); compute each argument separately before interpolation. Values containing whitespace or shell metacharacters must remain one argument. Never interpolate raw values. `WORKTREE_ROOT`, `WORKTREE_PATH`, `REF`, `REMOTE`, and `REFSPEC` are dynamic values below; the fixed commit subject stays literal. +The controller owns Git metadata writes; implementers edit and test only. This includes linked-worktree metadata under `.git/worktrees/` and the representative operations below; apply the same rule to any other Git command that writes metadata. Shell escaping and Git option termination are separate defenses: shell escaping keeps each dynamic value as one argument, while Git's `--` must appear before the first dynamic positional operand so a value beginning with `-` cannot be interpreted as a Git option. Shell-escape every dynamic value as exactly one shell argument before constructing an escalated command. The same escaped arguments apply to all companion and Git invocations, not merely escalated metadata writes. In the examples, `shellEscape(value)` means a robust shell-argument escaping step (for example, Bash `printf '%q' "$value"`); compute each argument separately before interpolation. Values containing whitespace or shell metacharacters must remain one argument. Never interpolate raw values. `WORKTREE_ROOT`, `WORKTREE_PATH`, `REF`, `REMOTE`, and `REFSPEC` are dynamic values below; the fixed commit subject stays literal. ```typescript const rootArg = shellEscape(WORKTREE_ROOT) @@ -87,9 +87,9 @@ If no plan-like content can be found anywhere, ask the user once what plan to im ## Pre-flight Checks Before extracting tasks: -1. Establish `WORKTREE_ROOT`: run `git rev-parse --show-toplevel` from the controller's working directory (or use the explicit worktree path if the controller already created a dedicated worktree for this task) and record the absolute path as `WORKTREE_ROOT`. This matters because `codex-companion.mjs` resolves its workspace from its own process cwd, which defaults to the harness's main checkout, not the task's worktree — every Codex invocation below passes `-C "${WORKTREE_ROOT}"` so implementers and reviewers target the same tree the controller commits to. -2. Confirm Codex is ready by running `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C "${WORKTREE_ROOT}" --json`. If the helper reports Codex is missing or unauthenticated, stop and tell the user to run `/codex:setup`. -3. Confirm git is in a sane state: `git -C "${WORKTREE_ROOT}" status --short`. If the working tree is dirty with unrelated changes, tell the user and ask whether to proceed. +1. Establish `WORKTREE_ROOT`: run `git rev-parse --show-toplevel` from the controller's working directory (or use the explicit worktree path if the controller already created a dedicated worktree for this task) and record the absolute path as `WORKTREE_ROOT`, then derive `const rootArg = shellEscape(WORKTREE_ROOT)` as above. This matters because `codex-companion.mjs` resolves its workspace from its own process cwd, which defaults to the harness's main checkout, not the task's worktree — every Codex invocation below passes `-C ${rootArg}` so implementers and reviewers target the same tree the controller commits to. +2. Confirm Codex is ready by running `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C ${rootArg} --json`. If the helper reports Codex is missing or unauthenticated, stop and tell the user to run `/codex:setup`. +3. Confirm git is in a sane state: `git -C ${rootArg} status --short`. If the working tree is dirty with unrelated changes, tell the user and ask whether to proceed. 4. Confirm we are NOT on `main` / `master`. If we are, tell the user and ask before proceeding — you (the controller) will be committing each task. All `git` commands in this loop, and all `codex-companion.mjs` invocations, run against `WORKTREE_ROOT` (via `git -C` / `-C`) rather than the controller's ambient cwd — this keeps the tree Codex edits and the tree the controller commits to in sync. @@ -99,25 +99,30 @@ All `git` commands in this loop, and all `codex-companion.mjs` invocations, run This loop is sequential: the controller cannot take the next step until the current job finishes. Implementer and reviewer runs at `xhigh` routinely outlast a single foreground `Bash` window, so enqueue the Codex work detached and use bounded foreground waits against its persisted job record. - Enqueue every implementer, spec-reviewer, and code-quality-reviewer step with `task --background --json`. The enqueue call returns immediately; read its `jobId` from the single JSON blob. The Final Review uses `review --background --json` under the same contract. -- Block on that ID in a foreground `Bash` call, setting the tool timeout comfortably above the companion wait timeout: +- Block on that ID in a foreground `Bash` call, setting the tool timeout comfortably above the companion wait timeout. `jobId` and `WORKTREE_ROOT` are dynamic values: shell-escape each exactly once before building the command. Keep `--` before the job ID, and keep `"${CLAUDE_PLUGIN_ROOT}"` double quoted because the shell expands it: ```typescript +const jobArg = shellEscape(jobId) + Bash({ - command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C "${WORKTREE_ROOT}" ${jobId} --wait --timeout-ms 240000 --json`, + command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C ${rootArg} --wait --timeout-ms 240000 --json -- ${jobArg}`, description: "Wait for Codex job", timeout: 300000 }) ``` - If the status JSON has `waitTimedOut: true`, follow the PID-aware timeout branch in `status.md` and re-arm only when the job is healthy. Otherwise retrieve the persisted result: -```bash -node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" result -C "${WORKTREE_ROOT}" --json +```typescript +Bash({ + command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" result -C ${rootArg} --json -- ${jobArg}`, + description: "Read persisted Codex result" +}) ``` - `--json` keeps stdout as a single JSON blob throughout. In the result payload, the task report is `.storedJob.result.rawOutput`, its partial failure text and reason are `.storedJob.result.partialOutput` and `.storedJob.result.failureMessage`, and the resumable thread ID is `.storedJob.threadId`. - **"Dispatched" is never a stopping point.** Do not end a turn with an unread Codex run and a note that you will check back, and do not wait to be told "continue" or "keep going". The loop advances only when you advance it. - Never abandon a still-running dispatch and re-dispatch on top of it. Two live Codex threads mutating `WORKTREE_ROOT` will corrupt each other's work. - If enqueueing or result retrieval exits non-zero, the job ends as `failed` or `cancelled`, or any returned JSON or report is empty or malformed, treat the step as `BLOCKED` (step 3) rather than assuming it succeeded. Recover according to which role failed: - - **A failed implementer dispatch may already have applied edits** even though its report never arrived — `.storedJob.result.rawOutput` is empty in that case, with the aborted turn's partial text under `.storedJob.result.partialOutput` and the reason under `.storedJob.result.failureMessage`. Inspect `.storedJob.result.touchedFiles` and `git status` / `git diff` in `WORKTREE_ROOT` before doing anything, then resume that same thread with `--resume-id "${IMPLEMENTER_THREAD_ID}"`. Never dispatch a fresh implementer on top of applied work. + - **A failed implementer dispatch may already have applied edits** even though its report never arrived — `.storedJob.result.rawOutput` is empty in that case, with the aborted turn's partial text under `.storedJob.result.partialOutput` and the reason under `.storedJob.result.failureMessage`. Inspect `.storedJob.result.touchedFiles` and `git status` / `git diff` in `WORKTREE_ROOT` before doing anything, then set `const threadIdArg = shellEscape(IMPLEMENTER_THREAD_ID)` and resume that same thread with `--resume-id ${threadIdArg}`. Never dispatch a fresh implementer on top of applied work. - **A failed spec or code-quality reviewer changed nothing on disk** (they run without `--write`). Dispatch a fresh reviewer of the same role. Never resume `${IMPLEMENTER_THREAD_ID}` to recover a review — that abandons the review and hands control back to the write-capable implementer. -- If a bounded foreground wait is interrupted, the detached Codex job continues; recover it with `status --json` and re-arm the wait instead of re-dispatching. If the `task` worker itself was killed, its edits are already on disk in `WORKTREE_ROOT`; only the report is lost. Recover `.storedJob.threadId` with `result --json`, then resume that same thread with `--resume-id "${threadId}"`; do not dispatch fresh over the killed job's edits. +- If a bounded foreground wait is interrupted, the detached Codex job continues; recover it with `status -C ${rootArg} --json -- ${jobArg}` and re-arm the wait instead of re-dispatching. If the `task` worker itself was killed, its edits are already on disk in `WORKTREE_ROOT`; only the report is lost. Recover `.storedJob.threadId` with `result -C ${rootArg} --json -- ${jobArg}`, then set `const threadIdArg = shellEscape(threadId)` and resume that same thread with `--resume-id ${threadIdArg}`; do not dispatch fresh over the killed job's edits. - That resume works only for `task` jobs, which run on persistent threads. A killed `review` (including the Final Review) has no thread to resume — review threads are ephemeral, and reviews change nothing on disk, so dispatch a fresh review instead. ## Task Extraction @@ -138,7 +143,7 @@ For each task in order: ### 1. Snapshot base SHA ```bash -git -C "${WORKTREE_ROOT}" rev-parse HEAD +git -C ${rootArg} rev-parse HEAD ``` Record as `BASE_SHA` for this task. @@ -161,12 +166,12 @@ Substitute placeholders: Enqueue Codex with `--background --json` so the controller gets a job ID and can later read the structured result: ```bash -node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task -C "${WORKTREE_ROOT}" --write --fresh --background --json [--model ] [--effort ] "" +node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task -C ${rootArg} --write --fresh --background --json [--model ] [--effort ] "" ``` - The controller must have the implementer's report before it can act, so this step blocks the loop. Run it per Dispatch and Follow-Through above rather than as a plain foreground call. - Use `--fresh` so the implementer gets a clean Codex thread. -- After the enqueue-and-wait contract returns the result JSON, read `.storedJob.result.rawOutput` for the report body (the `## Status` section step 3 inspects) and record `.storedJob.threadId` as `IMPLEMENTER_THREAD_ID` for this task — it stays fixed for the whole task's fix loop. +- After the enqueue-and-wait contract returns the result JSON, read `.storedJob.result.rawOutput` for the report body (the `## Status` section step 3 inspects), record `.storedJob.threadId` as `IMPLEMENTER_THREAD_ID` for this task, and set `const threadIdArg = shellEscape(IMPLEMENTER_THREAD_ID)` for subsequent resume instructions — it stays fixed for the whole task's fix loop. - For `--model`, use the user's value if they passed one; otherwise pass `--model gpt-5.6-luna` explicitly. `/codex:implement` defaults to `gpt-5.6-luna` rather than the runtime default of `gpt-5.5`. - For `--effort`, use the user's value if they passed one; otherwise pass `--effort xhigh` explicitly. `/codex:implement` defaults to `xhigh` rather than the runtime default of `high`. - The prompt is the substituted template text. Pass it as a single positional argument (heredoc/quoting as needed). @@ -175,7 +180,7 @@ node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task -C "${WORKTREE_ROO The report body is the `.storedJob.result.rawOutput` field of the result JSON from step 2. Locate the `## Status` heading within it. Branch on value: -- **NEEDS_CONTEXT** → The operator can unblock with a reply. If Codex listed discrete options, present them via `AskUserQuestion`; otherwise show the questions inline and collect answers. Re-dispatch step 2 with `{{TASK_CONTEXT}}` augmented (or with the operator's decision appended) and `--resume-id "${IMPLEMENTER_THREAD_ID}"` so the implementer keeps its working context. +- **NEEDS_CONTEXT** → The operator can unblock with a reply. If Codex listed discrete options, present them via `AskUserQuestion`; otherwise show the questions inline and collect answers. Re-dispatch step 2 with `{{TASK_CONTEXT}}` augmented (or with the operator's decision appended) and `--resume-id ${threadIdArg}` so the implementer keeps its working context. - **BLOCKED** → The operator alone cannot unblock. Diagnose the specific reason Codex gave: - Model/capacity issue → re-dispatch one effort step above the run's current effort when the run's model supports the next level; otherwise escalate to a stronger model. The `gpt-5.6-luna` default supports one step above `xhigh` (`max`) but not `ultra`. The run now warns when the model does not advertise the requested level; treat that warning as the escalation not taking effect rather than assuming it did. - Codex sandbox or permission denial → check the error, decide whether to grant access or re-scope. Surface to user if unsure. @@ -192,7 +197,7 @@ The Codex implementer leaves its changes in the working tree; it does **not** st Check for changes: ```bash -git -C "${WORKTREE_ROOT}" status --porcelain +git -C ${rootArg} status --porcelain ``` - If **empty** (the implementer produced no file changes) yet it reported `DONE` → treat as `BLOCKED`: the implementer did nothing. Re-dispatch step 2 with an explicit instruction to actually make the change. @@ -215,7 +220,7 @@ If either already-escalated request returns a nonzero result, stop immediately a Then, in a normal sandboxed Bash request, read the resulting commit: ```bash -git -C "${WORKTREE_ROOT}" rev-parse HEAD +git -C ${rootArg} rev-parse HEAD ``` Record the new commit as `HEAD_SHA`. Set `COMMITS_RANGE = ${BASE_SHA}..${HEAD_SHA}` — this is what the reviewers examine. @@ -230,7 +235,7 @@ Load `${CLAUDE_PLUGIN_ROOT}/prompts/sdd-spec-reviewer.md`. Substitute: Invoke Codex read-only (same `--model`/`--effort` resolution as step 2 — default `--model gpt-5.6-luna`, `--effort xhigh`): ```bash -node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task -C "${WORKTREE_ROOT}" --fresh --background --json [--model ] [--effort ] "" +node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task -C ${rootArg} --fresh --background --json [--model ] [--effort ] "" ``` (No `--write`. Spec reviewer must not edit code.) @@ -239,7 +244,7 @@ node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task -C "${WORKTREE_ROO Locate `## Verdict` heading: - **SPEC_COMPLIANT** → proceed to step 7. -- **ISSUES_FOUND** → Build a fix brief listing the issues. Re-dispatch implementer (step 2 again) with `{{REVIEWER_FEEDBACK}}` populated and `--resume-id "${IMPLEMENTER_THREAD_ID}"` so the implementer keeps its working context — naming the thread explicitly is what actually preserves it, since `--resume-last` would resolve to the reviewer's thread (the most recently dispatched `task`-class job) instead. After it returns, commit the fix yourself (step 4 — the implementer still does not commit) and update `HEAD_SHA` / `COMMITS_RANGE`. Then re-dispatch spec reviewer (step 5) — fresh thread each time so it does not anchor on prior judgments. Loop until SPEC_COMPLIANT or until the same issue recurs 3 times (then escalate to user). +- **ISSUES_FOUND** → Build a fix brief listing the issues. Re-dispatch implementer (step 2 again) with `{{REVIEWER_FEEDBACK}}` populated and `--resume-id ${threadIdArg}` so the implementer keeps its working context — naming the thread explicitly is what actually preserves it, since `--resume-last` would resolve to the reviewer's thread (the most recently dispatched `task`-class job) instead. After it returns, commit the fix yourself (step 4 — the implementer still does not commit) and update `HEAD_SHA` / `COMMITS_RANGE`. Then re-dispatch spec reviewer (step 5) — fresh thread each time so it does not anchor on prior judgments. Loop until SPEC_COMPLIANT or until the same issue recurs 3 times (then escalate to user). ### 7. Dispatch code quality reviewer (fresh Codex thread) @@ -251,14 +256,14 @@ Load `${CLAUDE_PLUGIN_ROOT}/prompts/sdd-code-quality-reviewer.md`. Substitute: Invoke read-only (same `--model`/`--effort` resolution as step 2 — default `--model gpt-5.6-luna`, `--effort xhigh`): ```bash -node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task -C "${WORKTREE_ROOT}" --fresh --background --json [--model ] [--effort ] "" +node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task -C ${rootArg} --fresh --background --json [--model ] [--effort ] "" ``` ### 8. Parse code quality verdict Locate `## Verdict` heading: - **APPROVED** → mark task complete in TodoWrite, move to next task. -- **CHANGES_REQUESTED** → Build a fix brief from `Issues — Critical` and `Issues — Important` (skip `Minor` unless they're easy). Re-dispatch implementer with `--resume-id "${IMPLEMENTER_THREAD_ID}"` so it resumes its own thread rather than the reviewer's. After it returns, commit the fix yourself (step 4) and update `HEAD_SHA` / `COMMITS_RANGE`. Then re-dispatch code quality reviewer fresh. Loop until APPROVED or same issue recurs 3 times. +- **CHANGES_REQUESTED** → Build a fix brief from `Issues — Critical` and `Issues — Important` (skip `Minor` unless they're easy). Re-dispatch implementer with `--resume-id ${threadIdArg}` so it resumes its own thread rather than the reviewer's. After it returns, commit the fix yourself (step 4) and update `HEAD_SHA` / `COMMITS_RANGE`. Then re-dispatch code quality reviewer fresh. Loop until APPROVED or same issue recurs 3 times. Update TodoWrite as you go. @@ -279,13 +284,13 @@ Waiting on a dispatched Codex run is not one of those reasons. If a step is stil After every task is complete: ```bash -git -C "${WORKTREE_ROOT}" log --oneline ${ORIGINAL_BASE_SHA}..HEAD +git -C ${rootArg} log --oneline ${ORIGINAL_BASE_SHA}..HEAD ``` Dispatch one final Codex code review across the entire branch: ```bash -node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" review -C "${WORKTREE_ROOT}" --background --json --base +node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" review -C ${rootArg} --background --json --base ``` Use the enqueue-and-bounded-wait contract in **Dispatch and Follow-Through** for this branch-wide review, then load its persisted result by job ID. @@ -323,7 +328,7 @@ If the user passed `--single-shot`, skip task extraction and the per-task loop. Use the enqueue-and-bounded-wait contract in **Dispatch and Follow-Through** for this single-shot task; one Codex agent handling the entire plan is the longest possible call. ```bash -node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task -C "${WORKTREE_ROOT}" --write --fresh --background --json [--model ] [--effort ] "" +node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task -C ${rootArg} --write --fresh --background --json [--model ] [--effort ] "" ``` (Same `--model`/`--effort` resolution as sequential mode — default `--model gpt-5.6-luna`, `--effort xhigh` unless the user passed one.) @@ -331,7 +336,7 @@ node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task -C "${WORKTREE_ROO The single-shot Codex agent leaves its changes unstaged and uncommitted too (same sandbox limitation). After it returns, stage and commit the working-tree changes yourself as two separate controller requests: ```bash -git -C "${WORKTREE_ROOT}" status --porcelain # if empty, Codex made no changes — report that instead of committing +git -C ${rootArg} status --porcelain # if empty, Codex made no changes — report that instead of committing ``` ```typescript @@ -356,8 +361,8 @@ Show the report. Propose next steps. - `--sequential` → explicit SDD mode (also the default). - User-supplied `--background` / `--wait` → Claude-side execution control only. Do not forward either raw flag to `task`; independently add `task --background --json` to every Codex step so SDD can use the tracked enqueue-and-wait contract. `task --wait` remains an explicit no-op and is never needed here. - `--model ` / `--effort ` → applied to every Codex invocation in this run. If omitted, `--model` defaults to `gpt-5.6-luna` and `--effort` defaults to `xhigh` (both passed explicitly by this command, overriding the runtime defaults of `gpt-5.5` / `high`). -- `-C "${WORKTREE_ROOT}"` → applied to every Codex invocation in this run (established in Pre-flight Checks). Pins the implementer/reviewer workspace to the task's worktree instead of `codex-companion.mjs`'s default of the controller's own process cwd. -- `--resume` / `--fresh` → ignored in SDD mode (the orchestrator picks per-step). SDD resumes the implementer by explicit thread id via `--resume-id "${IMPLEMENTER_THREAD_ID}"` (not `--resume-last`, which would resolve to whichever `task`-class thread was dispatched most recently — often a reviewer, not the implementer). +- `-C ${rootArg}` → applied to every Codex invocation in this run (established in Pre-flight Checks). Pins the implementer/reviewer workspace to the task's worktree instead of `codex-companion.mjs`'s default of the controller's own process cwd. +- `--resume` / `--fresh` → ignored in SDD mode (the orchestrator picks per-step). SDD resumes the implementer by explicit thread id via `--resume-id ${threadIdArg}` (not `--resume-last`, which would resolve to whichever `task`-class thread was dispatched most recently — often a reviewer, not the implementer). ## Failure Modes diff --git a/plugins/codex/commands/rescue.md b/plugins/codex/commands/rescue.md index 42ed52a94..77820bf6f 100644 --- a/plugins/codex/commands/rescue.md +++ b/plugins/codex/commands/rescue.md @@ -61,19 +61,25 @@ Foreground flow: Background flow: - Invoke `codex:codex-rescue` through the foreground `Agent` call. Tell the subagent to use exactly one `Bash` call for `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task --background --json ...`, return the enqueue payload, and do no waiting, polling, or result fetching. -- Treat the subagent stdout as dispatch JSON. Read `jobId` and `workspaceRoot` from it, then block with a bounded foreground wait. The `Bash` tool timeout must be comfortably larger than `--timeout-ms`: +- Treat the subagent stdout as dispatch JSON. Read `jobId` and `workspaceRoot` from it, then block with a bounded foreground wait. Both are dynamic values: shell-escape each exactly once before building the command. `shellEscape(value)` means robust shell argument escaping (for example, Bash `printf '%q' "$value"`). Keep `--` before the job ID, and keep `"${CLAUDE_PLUGIN_ROOT}"` double quoted because the shell expands it. The `Bash` tool timeout must be comfortably larger than `--timeout-ms`: ```typescript +const rootArg = shellEscape(workspaceRoot) +const jobArg = shellEscape(jobId) + Bash({ - command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C "${workspaceRoot}" ${jobId} --wait --timeout-ms 240000 --json`, + command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C ${rootArg} --wait --timeout-ms 240000 --json -- ${jobArg}`, description: "Wait for Codex rescue", timeout: 300000 }) ``` - If the wait payload has `waitTimedOut: true`, follow the PID-aware timeout branch in `status.md` and re-arm only when it classifies the job as healthy. Otherwise, read the persisted result: -```bash -node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" result -C "${workspaceRoot}" --json +```typescript +Bash({ + command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" result -C ${rootArg} --json -- ${jobArg}`, + description: "Read persisted Codex rescue result" +}) ``` - Read `.storedJob.rendered` from the result JSON and present it verbatim in the same turn the terminal wait returns. Do not replace the rescue result with a status note or wait to be asked "is it done?" or "continue". - If enqueueing or reading the job exits non-zero, the job reaches `failed` or `cancelled`, or the dispatch JSON, result JSON, or rescue output is empty or malformed, report the failure and surface the most actionable failure lines. Empty or malformed Agent/Bash output, or a reported nonzero Agent invocation, must surface the available tool and stderr failure lines; never silently treat it as a successful rescue. A failed rescue must not vanish silently. - Never re-dispatch a second rescue for the same request. Do not close out a turn with a dispatched-but-unread rescue or a "check `/codex:status`" note in place of its result. -- If a later turn inherits a dispatched-but-unread rescue, recover it from disk with `status -C "${workspaceRoot}" --json`; if it is still active, resume the bounded wait, and when it is terminal use `result -C "${workspaceRoot}" --json`. Do not re-dispatch it or report it as stuck. +- If a later turn inherits a dispatched-but-unread rescue, recover it from disk with `status -C ${rootArg} --json -- ${jobArg}`; if it is still active, resume the bounded wait, and when it is terminal use `result -C ${rootArg} --json -- ${jobArg}`. Do not re-dispatch it or report it as stuck. diff --git a/plugins/codex/commands/review.md b/plugins/codex/commands/review.md index f8fd04de3..0243bdbc2 100644 --- a/plugins/codex/commands/review.md +++ b/plugins/codex/commands/review.md @@ -51,19 +51,25 @@ Background flow: ```bash node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" review "--background --json $ARGUMENTS" ``` -- Read `jobId` and `workspaceRoot` from the returned JSON, then block with a bounded foreground wait. The `Bash` tool timeout must be comfortably larger than `--timeout-ms`: +- Read `jobId` and `workspaceRoot` from the returned JSON, then block with a bounded foreground wait. Both are dynamic values: shell-escape each exactly once before building the command. `shellEscape(value)` means robust shell argument escaping (for example, Bash `printf '%q' "$value"`). Keep `--` before the job ID, and keep `"${CLAUDE_PLUGIN_ROOT}"` double quoted because the shell expands it. The `Bash` tool timeout must be comfortably larger than `--timeout-ms`: ```typescript +const rootArg = shellEscape(workspaceRoot) +const jobArg = shellEscape(jobId) + Bash({ - command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C "${workspaceRoot}" ${jobId} --wait --timeout-ms 240000 --json`, + command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C ${rootArg} --wait --timeout-ms 240000 --json -- ${jobArg}`, description: "Wait for Codex review", timeout: 300000 }) ``` - If the wait payload has `waitTimedOut: true`, follow the PID-aware timeout branch in `status.md` and re-arm only when it classifies the job as healthy. Otherwise, read the persisted result: -```bash -node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" result -C "${workspaceRoot}" --json +```typescript +Bash({ + command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" result -C ${rootArg} --json -- ${jobArg}`, + description: "Read persisted Codex review result" +}) ``` - Read `.storedJob.rendered` from the result JSON and present it verbatim in the same turn the terminal wait returns. Do not wait to be asked "is it done?" or "continue". - If enqueueing or reading the job exits non-zero, the job reaches `failed` or `cancelled`, or the JSON or review output is empty or malformed, say so and surface the most actionable failure lines. A failed review must not vanish silently. - Never re-dispatch a second review over the same diff, and never hand the user a "check `/codex:status`" note in place of the findings. -- If you ever enter a later turn holding a dispatched-but-unread review, recover it from disk with `status -C "${workspaceRoot}" --json`; if it is still active, resume the bounded wait, and when it is terminal use `result -C "${workspaceRoot}" --json`. Do not re-dispatch it or report it as stuck. +- If you ever enter a later turn holding a dispatched-but-unread review, recover it from disk with `status -C ${rootArg} --json -- ${jobArg}`; if it is still active, resume the bounded wait, and when it is terminal use `result -C ${rootArg} --json -- ${jobArg}`. Do not re-dispatch it or report it as stuck. diff --git a/tests/commands.test.mjs b/tests/commands.test.mjs index 512388cde..a64c880a6 100644 --- a/tests/commands.test.mjs +++ b/tests/commands.test.mjs @@ -25,9 +25,9 @@ test("review command auto-decides execution mode and uses a tracked background j assert.match(source, /\[--scope auto\|working-tree\|branch\]/); assert.doesNotMatch(source, /run_in_background/); assert.match(source, /review "--background --json \$ARGUMENTS"/); - assert.match(source, /status -C "\$\{workspaceRoot\}" \$\{jobId\} --wait --timeout-ms 240000 --json/); + assert.match(source, /status -C \$\{rootArg\} --wait --timeout-ms 240000 --json -- \$\{jobArg\}/); assert.match(source, /description:\s*"Wait for Codex review"/); - assert.match(source, /result -C "\$\{workspaceRoot\}" --json/); + assert.match(source, /result -C \$\{rootArg\} --json -- \$\{jobArg\}/); assert.match(source, /waitTimedOut: true/); assert.match(source, /Return the command stdout verbatim, exactly as-is/i); assert.match(source, /git status --short --untracked-files=all/); @@ -63,9 +63,9 @@ test("adversarial review command auto-decides execution mode and uses a tracked assert.match(source, /\[--scope auto\|working-tree\|branch\] \[--model \] \[--effort \] \[focus \.\.\.\]/); assert.doesNotMatch(source, /run_in_background/); assert.match(source, /adversarial-review "--background --json \$ARGUMENTS"/); - assert.match(source, /status -C "\$\{workspaceRoot\}" \$\{jobId\} --wait --timeout-ms 240000 --json/); + assert.match(source, /status -C \$\{rootArg\} --wait --timeout-ms 240000 --json -- \$\{jobArg\}/); assert.match(source, /description:\s*"Wait for Codex adversarial review"/); - assert.match(source, /result -C "\$\{workspaceRoot\}" --json/); + assert.match(source, /result -C \$\{rootArg\} --json -- \$\{jobArg\}/); assert.match(source, /waitTimedOut: true/); assert.match(source, /Return the command stdout verbatim, exactly as-is/i); assert.match(source, /git status --short --untracked-files=all/); @@ -98,9 +98,9 @@ test("deep review command auto-decides execution mode and uses a tracked backgro assert.match(source, /\[--scope auto\|working-tree\|branch\] \[--model \] \[--effort \] \[focus \.\.\.\]/); assert.doesNotMatch(source, /run_in_background/); assert.match(source, /deep-review "--background --json \$ARGUMENTS"/); - assert.match(source, /status -C "\$\{workspaceRoot\}" \$\{jobId\} --wait --timeout-ms 240000 --json/); + assert.match(source, /status -C \$\{rootArg\} --wait --timeout-ms 240000 --json -- \$\{jobArg\}/); assert.match(source, /description:\s*"Wait for Codex deep review"/); - assert.match(source, /result -C "\$\{workspaceRoot\}" --json/); + assert.match(source, /result -C \$\{rootArg\} --json -- \$\{jobArg\}/); assert.match(source, /waitTimedOut: true/); assert.match(source, /Return the command stdout verbatim, exactly as-is/i); assert.match(source, /git status --short --untracked-files=all/); @@ -210,9 +210,9 @@ test("implement routes Git metadata writes through scoped controller escalation" } for (const snippet of [ - /git -C \"\$\{WORKTREE_ROOT\}\" status\b/, - /git -C \"\$\{WORKTREE_ROOT\}\" rev-parse\b/, - /git -C \"\$\{WORKTREE_ROOT\}\" log\b/ + /git -C \$\{rootArg\} status\b/, + /git -C \$\{rootArg\} rev-parse\b/, + /git -C \$\{rootArg\} log\b/ ]) { assert.match(source, snippet, `implement includes read-only Git snippet ${snippet}`); assert.doesNotMatch(allEscalatedCommands.join("\n"), snippet, `read-only Git command is not escalated: ${snippet}`); @@ -620,8 +620,16 @@ test("the foreground wait mechanism is provisioned on every command that require const allowed = /^allowed-tools:(.*)$/m.exec(frontmatter); assert.ok(allowed, `${file} declares allowed-tools`); assert.match(allowed[1], /\bBash\(node:\*\)/, `${file} may invoke the companion script`); - assert.match(source, /status .*--wait --timeout-ms 240000 --json/, `${file} documents the bounded wait`); - assert.match(source, /result .*.*--json/, `${file} documents persisted result retrieval`); + assert.match( + source, + /status -C \$\{rootArg\} --wait --timeout-ms 240000 --json -- \$\{jobArg\}/, + `${file} documents the bounded wait` + ); + assert.match( + source, + /result -C \$\{rootArg\} --json -- \$\{jobArg\}/, + `${file} documents persisted result retrieval` + ); const companionTimeout = /--timeout-ms\s+(\d+)/.exec(source); const bashTimeout = /\btimeout:\s*(\d+)/.exec(source); assert.ok(companionTimeout, `${file} declares the companion wait timeout`); @@ -635,6 +643,45 @@ test("the foreground wait mechanism is provisioned on every command that require } }); +test("every companion command is built from shell-escaped dynamic values", () => { + // Regression for #107: wrapping an interpolated value in double quotes is not + // escaping. A checkout path containing `"` breaks the command outright, and + // one containing a backtick or $(...) is evaluated by the shell before the + // companion script ever runs. `${CLAUDE_PLUGIN_ROOT}` is the one legitimate + // double-quoted interpolation: the shell expands it, the model does not. + for (const file of [ + "commands/review.md", + "commands/adversarial-review.md", + "commands/deep-review.md", + "commands/implement.md", + "commands/rescue.md" + ]) { + const source = read(file); + assert.match(source, /shellEscape\(/, `${file} names the escaping step`); + assert.match(source, /const rootArg = shellEscape\(/, `${file} escapes the workspace root`); + assert.match(source, /const jobArg = shellEscape\(jobId\)/, `${file} escapes the job id`); + + const doubleQuoted = new Set( + [...source.matchAll(/"\$\{([A-Za-z_][A-Za-z0-9_]*)\}"/g)].map((match) => match[1]) + ); + doubleQuoted.delete("CLAUDE_PLUGIN_ROOT"); + assert.deepEqual( + [...doubleQuoted], + [], + `${file} must not interpolate a dynamic value inside double quotes` + ); + + for (const line of source.split("\n")) { + if (!/codex-companion\.mjs|^git -C|`git -C/.test(line)) continue; + assert.doesNotMatch( + line, + /\$\{(workspaceRoot|WORKTREE_ROOT|jobId|threadId|IMPLEMENTER_THREAD_ID)\}/, + `${file} must pass a pre-escaped argument, not a raw value: ${line.trim()}` + ); + } + } +}); + test("no command tells the model to wait via a command it cannot invoke", () => { // status.md is disable-model-invocation: true, so "/codex:status" is advice // for a human. A flow that names it as *its own* wait mechanism dead-ends. @@ -673,8 +720,8 @@ test("dispatch is never a stopping point across every async surface", () => { assert.match(source, /same turn the terminal wait returns/i); assert.match(source, /never re-dispatch (a second|another)/i); assert.match(source, /dispatched-but-unread|dispatched .* unread/i); - assert.match(recovery, /status -C "\$\{workspaceRoot\}" --json/); - assert.match(recovery, /result -C "\$\{workspaceRoot\}" --json/); + assert.match(recovery, /status -C \$\{rootArg\} --json -- \$\{jobArg\}/); + assert.match(recovery, /result -C \$\{rootArg\} --json -- \$\{jobArg\}/); assert.match(source, /\.storedJob\.rendered/); for (const condition of ["failed", "cancelled", "empty", "malformed"]) { assert.match(background, new RegExp(`\\b${condition}\\b`, "i"), `${file} handles ${condition} review results`); @@ -798,7 +845,7 @@ test("implement enqueues and awaits every long Codex job, including final review assert.match(dispatch, /review --background --json/); assert.match(dispatch, /status .*--wait --timeout-ms 240000 --json/); assert.match(dispatch, /waitTimedOut: true/); - assert.match(dispatch, /result .*.*--json/); + assert.match(dispatch, /result -C \$\{rootArg\} --json -- \$\{jobArg\}/); assert.match(dispatch, /\.storedJob\.result\.rawOutput/); assert.match(dispatch, /\.storedJob\.threadId/); assert.doesNotMatch(dispatch, /run_in_background|BashOutput/); @@ -896,8 +943,8 @@ test("rescue never depends on a background-subagent re-invocation to present its assert.doesNotMatch(rescue, /run the `codex:codex-rescue` subagent in the background/i); assert.match(rescue, /task --background --json/); - assert.match(rescue, /status -C "\$\{workspaceRoot\}" \$\{jobId\} --wait --timeout-ms 240000 --json/); - assert.match(rescue, /result -C "\$\{workspaceRoot\}" --json/); + assert.match(rescue, /status -C \$\{rootArg\} --wait --timeout-ms 240000 --json -- \$\{jobArg\}/); + assert.match(rescue, /result -C \$\{rootArg\} --json -- \$\{jobArg\}/); assert.match(rescue, /\.storedJob\.rendered/); // The forwarding contract has to agree with the command doc. From d6dc8c7acf472acfaf1a780791fc0ac68d6543dd Mon Sep 17 00:00:00 2001 From: Patrick Yang <266918795+patriyang@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:54:29 -0400 Subject: [PATCH 2/2] Bump plugin version to 1.0.44 Co-Authored-By: Claude Opus 5 (1M context) --- .claude-plugin/marketplace.json | 4 ++-- package-lock.json | 4 ++-- package.json | 2 +- plugins/codex/.claude-plugin/plugin.json | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 8ea345e32..227b9f54f 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -5,13 +5,13 @@ }, "metadata": { "description": "Codex plugins to use in Claude Code for delegation and code review.", - "version": "1.0.43" + "version": "1.0.44" }, "plugins": [ { "name": "codex", "description": "Use Codex from Claude Code to review code or delegate tasks.", - "version": "1.0.43", + "version": "1.0.44", "author": { "name": "OpenAI" }, diff --git a/package-lock.json b/package-lock.json index d6bc2c58b..6ae452c42 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@openai/codex-plugin-cc", - "version": "1.0.43", + "version": "1.0.44", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@openai/codex-plugin-cc", - "version": "1.0.43", + "version": "1.0.44", "license": "Apache-2.0", "devDependencies": { "@types/node": "^25.5.0", diff --git a/package.json b/package.json index f9cc617de..2d0a9d2e1 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@openai/codex-plugin-cc", - "version": "1.0.43", + "version": "1.0.44", "private": true, "type": "module", "description": "Use Codex from Claude Code to review code or delegate tasks.", diff --git a/plugins/codex/.claude-plugin/plugin.json b/plugins/codex/.claude-plugin/plugin.json index e361bcc33..19c02acba 100644 --- a/plugins/codex/.claude-plugin/plugin.json +++ b/plugins/codex/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "codex", - "version": "1.0.43", + "version": "1.0.44", "description": "Use Codex from Claude Code to review code or delegate tasks.", "author": { "name": "OpenAI"