fix: pass review findings to rescue automatically#129
fix: pass review findings to rescue automatically#129Co-Messi wants to merge 5 commits intoopenai:mainfrom
Conversation
![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: 155bdbef6a
ℹ️ 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".
plugins/codex/commands/rescue.md
Outdated
| - If the user did not supply a request, ask what Codex should investigate or fix. | ||
| - If the user did not supply a request, check for a saved review from `/codex:review` or `/codex:adversarial-review`: | ||
| ```bash | ||
| [ -f ~/.codex/last-review.md ] && echo "LAST_REVIEW_AVAILABLE" || echo "NO_LAST_REVIEW" |
There was a problem hiding this comment.
This path cannot execute as written because rescue.md only permits Bash(node:*), but the new check uses [ -f ~/.codex/last-review.md ], which is not node-prefixed. When users run /codex:rescue without arguments, the review-detection step will be blocked by tool restrictions, so the new “fix issues from last review” prompt never appears.
Useful? React with 👍 / 👎.
plugins/codex/commands/review.md
Outdated
| - Do not fix any issues mentioned in the review output. | ||
| - After returning the output to the user, save it for use by `/codex:rescue`: | ||
| ```bash | ||
| mkdir -p ~/.codex |
There was a problem hiding this comment.
The foreground save instructions use mkdir -p ~/.codex, but this command file only allows Bash(node:*) and Bash(git:*). Because mkdir is outside that allowlist, the new persistence step can fail under command-policy enforcement, leaving ~/.codex/last-review.md unsaved and breaking the intended handoff into /codex:rescue.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b739a7b9b9
ℹ️ 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".
plugins/codex/commands/review.md
Outdated
| Background flow: | ||
| - Determine the repo-scoped save path: | ||
| ```bash | ||
| _REPO_HASH=$(git rev-parse --show-toplevel 2>/dev/null | md5 | awk '{print $1}' || echo "global") |
There was a problem hiding this comment.
The _REPO_HASH computation is not compatible with the hash format used everywhere else (crypto.createHash('md5').digest('hex')): md5 | awk '{print $1}' can produce a non-hex token (e.g., MD5) or an empty value when md5 is unavailable, so the background run writes to a different filename than /codex:rescue reads. In that case, users who run /codex:review --background will not get the promised “fix issues from last review” handoff because the saved file is looked up under a different path.
Useful? React with 👍 / 👎.
plugins/codex/commands/review.md
Outdated
| ```typescript | ||
| Bash({ | ||
| command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" review "$ARGUMENTS"`, | ||
| command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" review "$ARGUMENTS" | tee ~/.codex/last-review-${_REPO_HASH}.md`, |
There was a problem hiding this comment.
The background command writes to ~/.codex/last-review-...md but this flow never creates ~/.codex. On a first-time run (or any environment without that directory), tee fails to open the file, so the review output is not persisted and the job can be marked failed even if the reviewer itself succeeded. That breaks the new rescue handoff for background reviews.
Useful? React with 👍 / 👎.
When /codex:review or /codex:adversarial-review completes in the foreground, pipe output through `tee` to a repo-scoped file at ~/.codex/last-review-<md5-of-repo-root>.md so findings survive across Codex cold-starts. When /codex:rescue is invoked with no task, check for that saved review and offer to fix those issues directly — no copy-pasting required. Using a per-repo hash instead of a global path prevents stale findings from one project bleeding into a rescue session in a different repo. All shell operations stay within each file's allowed-tools constraints: review.md and adversarial-review.md use Bash(git:*) for repo root and Bash(node:*) for dir creation and hashing; rescue.md (node-only) calls git via child_process.execSync inside a node one-liner.
Missed in the previous commit — adversarial-review.md needs the same per-repo tee flow so its output is also available for /codex:rescue.
Co-Messi
force-pushed
the
fix/review-rescue-context-handoff
branch
from
b739a7b to
7dee7f0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dee7f0d21
ℹ️ 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".
plugins/codex/commands/review.md
Outdated
| ```typescript | ||
| Bash({ | ||
| command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" review "$ARGUMENTS"`, | ||
| command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" review "$ARGUMENTS" | tee ~/.codex/last-review-${_REPO_HASH}.md`, |
There was a problem hiding this comment.
The new background command pipes codex-companion through tee, but there is no pipefail guard, so Bash returns tee’s status (usually 0) instead of the review command’s status. When the companion exits non-zero (for example, auth/setup/runtime failures), the background job can be recorded as successful even though the review failed, which breaks status/result reliability and can persist invalid handoff data.
Useful? React with 👍 / 👎.
| - Leave `--resume` and `--fresh` in the forwarded request. The subagent handles that routing when it builds the `task` command. | ||
| - If the helper reports that Codex is missing or unauthenticated, stop and tell the user to run `/codex:setup`. | ||
| - If the user did not supply a request, ask what Codex should investigate or fix. | ||
| - If the user did not supply a request, check for a saved review from `/codex:review` or `/codex:adversarial-review`: |
There was a problem hiding this comment.
This no-argument rescue path now promises saved review detection for both /codex:review and /codex:adversarial-review, but the adversarial background flow still does not write ~/.codex/last-review-*.md (it only launches the command). After /codex:adversarial-review --background, rescue may not find the latest findings (or may use stale ones), so the new handoff behavior is inconsistent with the documented flow.
Useful? React with 👍 / 👎.
The background flow had four bugs vs the foreground implementation:
1. Hash mismatch: `git ... | md5 | awk '{print $1}'` produces a
different format than crypto.createHash('md5').digest('hex') used
in foreground and rescue — background-saved files were never found.
2. No mkdir: ~/.codex was not created before tee, causing tee to fail
on first run and silently drop the review output.
3. Exit code swallowed: piping through tee masks a non-zero companion
exit, so failed reviews could be recorded as successful background jobs.
4. Adversarial background never saved: the adversarial-review background
flow launched without any tee, so /codex:rescue could never find it.
Fix: replace the background command in both review.md and
adversarial-review.md with a single node wrapper that computes the hash
via crypto (same as foreground/rescue), calls mkdirSync, runs the
companion via spawnSync, writes the file only on exit 0, and exits
with the companion's status code.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 647cd1bc80
ℹ️ 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".
plugins/codex/commands/review.md
Outdated
| ``` | ||
| - Run, piping output to both the terminal and the save path. Substitute `<SAVE_PATH>` with the node output above: | ||
| ```bash | ||
| node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" review "$ARGUMENTS" | tee "<SAVE_PATH>" |
There was a problem hiding this comment.
This foreground command pipes codex-companion into tee without pipefail, so Bash reports tee's exit status instead of the review command’s. If codex-companion fails (for example due to auth/runtime errors), the slash command can still look successful and continue with invalid or empty saved output. The same pattern appears in adversarial-review.md, so both foreground review modes are affected.
Useful? React with 👍 / 👎.
plugins/codex/commands/review.md
Outdated
| ```typescript | ||
| Bash({ | ||
| command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" review "$ARGUMENTS"`, | ||
| command: `node -e "const {spawnSync:sp}=require('child_process'),os=require('os'),fs=require('fs'),c=require('crypto'),path=require('path');let t;try{t=sp('git',['rev-parse','--show-toplevel'],{encoding:'utf8'}).stdout.trim()}catch(e){t=''};const h=t?c.createHash('md5').update(t).digest('hex'):'global';const d=os.homedir()+'/.codex';fs.mkdirSync(d,{recursive:true});const args=process.argv.slice(2).filter(Boolean);const r=sp(process.execPath,[path.resolve(process.env.CLAUDE_PLUGIN_ROOT,'scripts/codex-companion.mjs'),'review',...args],{stdio:['inherit','pipe','inherit'],env:process.env});const out=r.stdout?r.stdout.toString():'';process.stdout.write(out);if(r.status===0)fs.writeFileSync(path.join(d,'last-review-'+h+'.md'),out);process.exit(r.status??1);" -- "$ARGUMENTS"`, |
There was a problem hiding this comment.
The background wrapper captures stdout via spawnSync(..., stdio: ['inherit','pipe','inherit']) and only writes it after completion, which forces the entire review output into an in-memory buffer with spawnSync's max-buffer limit. Larger outputs can fail with ENOBUFS (r.status becomes null), causing background review jobs to fail and skip writing last-review-*.md. This is a regression from the prior streamed execution and the same wrapper is duplicated in adversarial-review.md.
Useful? React with 👍 / 👎.
- Foreground: replace 3-step git+node+tee pipeline with a single streaming node wrapper — fixes pipefail gap where companion failure was masked by tee's exit 0 - Background: replace spawnSync (buffers entire output) with spawn (streams chunks in real-time) — fixes memory pressure on large reviews and ensures output appears progressively in /codex:result - Both flows now share the same wrapper pattern: streams stdout live, saves to ~/.codex/last-review-<hash>.md only on exit 0, propagates the companion exit code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74da0b248f
ℹ️ 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".
plugins/codex/commands/review.md
Outdated
| - Run the review. The node wrapper streams output in real-time, creates `~/.codex` if needed, saves output only on success, and propagates the exit code: | ||
| ```bash | ||
| node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" review "$ARGUMENTS" | ||
| node -e "const {spawn,spawnSync:ss}=require('child_process'),os=require('os'),fs=require('fs'),c=require('crypto'),path=require('path');let t;try{t=ss('git',['rev-parse','--show-toplevel'],{encoding:'utf8'}).stdout.trim()}catch(e){t=''};const h=t?c.createHash('md5').update(t).digest('hex'):'global';const d=os.homedir()+'/.codex';fs.mkdirSync(d,{recursive:true});const args=process.argv.slice(2).filter(Boolean);const child=spawn(process.execPath,[path.resolve(process.env.CLAUDE_PLUGIN_ROOT,'scripts/codex-companion.mjs'),'review',...args],{stdio:['inherit','pipe','inherit'],env:process.env});const chunks=[];child.stdout.on('data',chunk=>{process.stdout.write(chunk);chunks.push(chunk)});child.on('close',code=>{if(code===0)fs.writeFileSync(path.join(d,'last-review-'+h+'.md'),Buffer.concat(chunks));process.exit(code??1)});" -- "$ARGUMENTS" |
There was a problem hiding this comment.
This wrapper performs fs.mkdirSync(d,{recursive:true}) before launching the actual review and does not catch filesystem errors, so /codex:review now fails outright when ~/.codex is not writable (common in locked-down/containerized environments). Saving last-review-*.md should be optional metadata, but this change makes it a hard prerequisite for running the review at all; the same inline wrapper is also used in adversarial-review.md.
Useful? React with 👍 / 👎.
|
🦀 Assigned! I will submit a PR for this issue. Looking at the fix now. |
mkdirSync and writeFileSync are now wrapped in try/catch so that unwritable ~/.codex (containers, locked-down envs) does not abort the review — saving last-review-*.md is optional metadata, not a hard prerequisite.
|
Hi @D2758695161 — looks like this PR already covers the fix you're looking at. We've iterated quite a bit based on the Codex bot's P1/P2 reviews — all issues resolved as of the latest commit. Happy to rebase or adjust anything if there's a style preference before you merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d5aa4ae92
ℹ️ 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".
| - Run the review. The node wrapper streams output in real-time, creates `~/.codex` if needed, saves output only on success, and propagates the exit code: | ||
| ```bash | ||
| node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" review "$ARGUMENTS" | ||
| node -e "const {spawn,spawnSync:ss}=require('child_process'),os=require('os'),fs=require('fs'),c=require('crypto'),path=require('path');let t;try{t=ss('git',['rev-parse','--show-toplevel'],{encoding:'utf8'}).stdout.trim()}catch(e){t=''};const h=t?c.createHash('md5').update(t).digest('hex'):'global';const d=os.homedir()+'/.codex';try{fs.mkdirSync(d,{recursive:true})}catch(e){};const args=process.argv.slice(2).filter(Boolean);const child=spawn(process.execPath,[path.resolve(process.env.CLAUDE_PLUGIN_ROOT,'scripts/codex-companion.mjs'),'review',...args],{stdio:['inherit','pipe','inherit'],env:process.env});const chunks=[];child.stdout.on('data',chunk=>{process.stdout.write(chunk);chunks.push(chunk)});child.on('close',code=>{if(code===0)try{fs.writeFileSync(path.join(d,'last-review-'+h+'.md'),Buffer.concat(chunks))}catch(e){};process.exit(code??1)});" -- "$ARGUMENTS" |
There was a problem hiding this comment.
The new wrapper accumulates every stdout chunk in memory (chunks.push(...) then Buffer.concat(chunks)) before writing last-review-*.md, which means large review outputs can grow RAM usage unbounded and potentially crash or be OOM-killed on big repos. This is a regression from direct streaming behavior and it affects both foreground and background runs (the same pattern is duplicated in adversarial-review.md).
Useful? React with 👍 / 👎.
| - If the user did not supply a request, ask what Codex should investigate or fix. | ||
| - If the user did not supply a request, check for a saved review from `/codex:review` or `/codex:adversarial-review`: | ||
| ```bash | ||
| node -e "const {execSync:x}=require('child_process'),os=require('os'),fs=require('fs'),c=require('crypto');let t;try{t=x('git rev-parse --show-toplevel',{encoding:'utf8'}).trim()}catch(e){t=null};const h=t?c.createHash('md5').update(t).digest('hex'):'global';const p=os.homedir()+'/.codex/last-review-'+h+'.md';console.log(fs.existsSync(p)?'LAST_REVIEW_AVAILABLE':'NO_LAST_REVIEW');" |
There was a problem hiding this comment.
The rescue handoff only checks whether the cache file exists, but review/adversarial wrappers only overwrite that file on exit code 0, so a failed review leaves the previous successful output in place. In that scenario, /codex:rescue with no task can present and reuse outdated findings against newer code, which can send users down the wrong fix path after an auth/runtime failure.
Useful? React with 👍 / 👎.
After a successful /opencode:review or /opencode:adversarial-review, save the rendered output to ~/.opencode-companion/last-review-<hash>.md (per repo, SHA-256 of workspace path). Add a new 'last-review' subcommand that reports availability or streams the content. rescue.md now checks for a saved review when invoked without task text and asks via AskUserQuestion whether to fix the prior findings or describe a new task. The save is best-effort — a failed persistence never fails the review itself. Closes #44. Port of openai/codex-plugin-cc#129 (simplified: logic lives in the companion script rather than an inline node -e one-liner).
* fix: quote $ARGUMENTS in cancel/result/status commands Unquoted $ARGUMENTS allows shell splitting on user-supplied job IDs containing metacharacters. Wrap in double quotes to match review.md and adversarial-review.md. Closes #38. Port of openai/codex-plugin-cc#168. * fix: declare model: sonnet in opencode-rescue agent frontmatter Without a model declaration the agent tier was unpredictable. The rescue subagent is a thin forwarder that invokes the companion via a single Bash call and applies trivial routing logic — sonnet is sufficient and gives users a cost guarantee. Closes #39. Port of openai/codex-plugin-cc#169. * fix: scope /opencode:cancel default to current Claude session Without ref, resolveCancelableJob now filters running jobs by sessionId so a cancel in session A cannot kill jobs in session B. Explicit ref still searches all sessions — naming a job counts as intent. Closes #45. Port of openai/codex-plugin-cc#84. * fix: enforce hard wall-clock timeout on runTrackedJob Wrap the runner with Promise.race against a 30-minute default timeout. On expiry the job transitions to failed/phase:failed so zombie 'running' rows can't accumulate when a runner hangs. OPENCODE_COMPANION_JOB_TIMEOUT_MS overrides the default. Closes #41. Port of openai/codex-plugin-cc#184. * fix: reconcile dead-PID jobs on every status read Adds isProcessAlive helper and reconcileIfDead / reconcileAllJobs / markDeadPidJobFailed in job-control. buildStatusSnapshot and the handleResult/handleCancel paths now probe kill(pid, 0) on any active-state job and rewrite dead ones to failed before consuming the list. A single /opencode:status / result / cancel surfaces stuck workers without waiting for SessionEnd. markDeadPidJobFailed is race-safe: it re-reads state and refuses to downgrade terminal states or rewrite when the pid has changed. Closes #42. Port of openai/codex-plugin-cc#176 + dead-PID parts of #184. * fix: avoid embedding large diffs in review prompts Classify review scope before building the prompt. When the diff exceeds ~5 files or ~256 KB, fall back to a lightweight context (status, changed-files, diff_stat) and tell OpenCode to inspect the diff itself via read-only git commands. Prevents HTTP 400 / shallow findings on moderate-to-large changesets. Adversarial template grows a {{REVIEW_COLLECTION_GUIDANCE}} slot. Thresholds overridable via opts.maxInlineDiffFiles/Bytes. Closes #40. Port of openai/codex-plugin-cc#179. * fix: respect \$SHELL on Windows when spawning child processes Add platformShellOption() helper that picks false on POSIX, and \$SHELL || true on win32 so Git Bash users get their shell while cmd fallback still resolves .cmd/.bat shims. Apply to runCommand, spawnDetached, resolveOpencodeBinary, getOpencodeVersion, and the ensureServer spawn of 'opencode serve'. Uses 'where' instead of 'which' on win32, and parses the first line of its CRLF-separated output. Closes #46. Port of openai/codex-plugin-cc#178. * fix: migrate tmpdir state to CLAUDE_PLUGIN_DATA + fix /tmp literal The fallback path was hard-coded to '/tmp' — broken on Windows. Use os.tmpdir() so Windows and other platforms get a real tmp path. Additionally: when CLAUDE_PLUGIN_DATA is set on a later call but state was previously written to the tmpdir fallback, copy it into the plugin-data dir and rewrite absolute path references inside state.json and jobs/*.json so logFile pointers don't dangle. Prevents job history from being silently dropped when commands run under different env contexts within one Claude session. Closes #47. Port of openai/codex-plugin-cc#125. * feat: pass last review findings to rescue automatically After a successful /opencode:review or /opencode:adversarial-review, save the rendered output to ~/.opencode-companion/last-review-<hash>.md (per repo, SHA-256 of workspace path). Add a new 'last-review' subcommand that reports availability or streams the content. rescue.md now checks for a saved review when invoked without task text and asks via AskUserQuestion whether to fix the prior findings or describe a new task. The save is best-effort — a failed persistence never fails the review itself. Closes #44. Port of openai/codex-plugin-cc#129 (simplified: logic lives in the companion script rather than an inline node -e one-liner). * feat: throttle controls for stop-time review gate Add --review-gate-max and --review-gate-cooldown flags to /opencode:setup so users can bound the review gate's spend: /opencode:setup --review-gate-max 5 /opencode:setup --review-gate-cooldown 10 /opencode:setup --review-gate-max off The stop hook now loads state before touching stdin, checks reviewGateMaxPerSession and reviewGateCooldownMinutes against the current session's reviewGateUsage entry, and allows the stop without running OpenCode when a limit would be exceeded. Usage entries older than 7 days are pruned on each successful run so state.json doesn't grow unbounded. renderSetup surfaces the configured limits. Closes #48. Port of openai/codex-plugin-cc#20. * feat: --worktree flag for isolated write-capable rescue tasks Add a disposable-git-worktree mode so /opencode:rescue --write --worktree runs OpenCode inside .worktrees/opencode-<ts> on a fresh opencode/<ts> branch instead of editing the working tree in place. Useful for exploratory runs, parallel rescues, and running against a dirty tree. Pieces: - lib/git.mjs: createWorktree / removeWorktree / deleteWorktreeBranch / getWorktreeDiff / applyWorktreePatch. Adds .worktrees/ to .git/info/exclude on first use so the dir never shows in status. - lib/worktree.mjs: session wrapper — createWorktreeSession, diffWorktreeSession, cleanupWorktreeSession (keep applies patch back, discard just removes). - opencode-companion.mjs: handleTask threads --worktree + swaps cwd + stores session data on the job record + renders a keep/discard footer. New worktree-cleanup subcommand reads the stored session and runs the keep or discard path. - agents/opencode-rescue.md, commands/rescue.md, skills/opencode-runtime: propagate --worktree through the forwarding layer. - tests/worktree.test.mjs: create, diff, keep-applies, discard, no-change no-op. Closes #43. Port of openai/codex-plugin-cc#137. * fix: address pr51 review findings * fix: keep tracked job timeout referenced * fix: address pr51 review conversations * fix: add exclusive file lock to updateState for concurrency safety updateState's read-modify-write cycle was not protected against concurrent companion processes (background worker + status/cancel handler), which could silently lose each other's writes. Acquire an exclusive lock file (state.json.lock via O_EXCL) before reading, hold it through mutation and write, release in finally. Stale locks older than 30s are evicted. Blocks up to 5s with retry. Closes the pre-existing concurrency race amplified by PR #51's dead-PID reconciliation (which adds upsertJob calls on every status read). * fix: address brownfield discovery bugs Critical/high fixes: - BUG-1: saveLastReview use copyFileSync+unlinkSync instead of renameSync (fixes Windows compatibility issue where rename fails if target exists) - BUG-2: handleTask worktree leak - wrap in try/finally to guarantee cleanup - BUG-3: State migration race - add fallback directory lock during migration - BUG-4/13: handleTaskWorker missing signal handlers for graceful shutdown Medium fixes: - BUG-5: releaseStateLock now fsyncs directory after lock removal - BUG-11: Error from getConfiguredProviders now logged instead of swallowed Low fixes: - BUG-6: PR number validation now rejects negative values - BUG-7: getBundledConfigDir checks directory exists before returning - BUG-8: tailLines now properly filters empty lines after split - BUG-9: resolveReviewAgent always returns tools property (undefined if not used) - BUG-10: Diff retrieval failure now logs warning instead of silent swallow - BUG-12: resolveOpencodeBinary now handles spawn errors properly Additional pre-existing work included: - safe-command.mjs wrapper for secure command execution - Command documentation updates - Test improvements * fix: polish pr51 follow-up fixes * fix: address Copilot PR#51 review comments Four findings, all valid: C1 (prompts.mjs, git.mjs, process.mjs) — buildReviewPrompt materialized the full diff string before checking thresholds. For huge diffs the git/gh subprocess could OOM the companion before the size check ran. Fix: runCommand gains maxOutputBytes, killing the child and reporting overflowed once the cap is exceeded. getDiff and getPrDiff thread maxBytes through. buildReviewPrompt now bounds the read at maxBytes+1 and treats overflow as over-byte-limit without ever materializing the rest. C2 (git.mjs) — getDiffByteSize had a docstring claiming it avoided streaming the full contents, but the implementation did exactly that. It was also dead code (zero callers). Removed. C3 (tests/state-lock.test.mjs) — the test injected path.resolve(...) into a generated ESM import specifier. On Windows that path contains backslashes and a drive letter, producing an invalid module specifier. Fix: pathToFileURL(...).href for the injected specifier. C4 (tests/dead-pid-reconcile.test.mjs) — beforeEach mutated the object returned by loadState() without saving it back, leaving on-disk state from earlier tests intact. Fix: saveState(workspace, { config:{}, jobs:[] }). Adds coverage: - tests/process.test.mjs: runCommand overflow path and non-overflow path. - tests/review-prompt-size.test.mjs: bounds-huge-diff end-to-end test that writes a 50k-byte file and asserts fewer than 10k 'x' chars land in the prompt. All 167 tests pass. * fix: keep reading fallback state while migrate lock is held Addresses a Codex P1 on PR#51: when another migrator holds \`primaryDir.migrate.lock\`, migrateTmpdirStateIfNeeded waits up to 2s and then returns without copying anything. Before this fix, stateRoot still returned primaryDir — but primary/state.json didn't exist yet, so loadState returned an empty state and the next upsertJob created primary/state.json with only the new entry, orphaning every seeded fallback job. Fix: after a migration attempt, if primary/state.json is absent and fallback/state.json is present, stateRoot returns the fallback dir. Reads see real data, writes land in fallback, and a later migration retry can pick them up cleanly. Adds a regression test that pre-creates the migrate lock, seeds fallback with a job, switches to CLAUDE_PLUGIN_DATA, and verifies that stateRoot falls back, loadState sees the seeded job, and a subsequent write preserves both the seeded and the in-flight rows. The symlink-refusal test had to be updated because it was reusing stateRoot to name the "primary" dir — with the new fallback guard, that call now returns fallbackDir on failed migration. The test now computes the expected primary path directly.
2 participants
Problem
`/codex:review` and `/codex:rescue` don't share context. After running a review, users have to manually copy-paste the findings into the rescue prompt — or describe the issues from memory. This is the most common workflow and it's broken by default:
```
/codex:review ← finds issues
/codex:rescue ← has no idea what was found
```
Fix
`review.md` — after returning output, save it to `~/.codex/last-review.md`. Works for both foreground and background flows.
`rescue.md` — when invoked with no task argument, check for a saved review. If found, offer via `AskUserQuestion`:
Result
```
/codex:review ← finds issues, saves to ~/.codex/last-review.md
/codex:rescue ← detects saved review, offers to fix it in one click
```
No copy-pasting. Existing rescue workflows with explicit task text are unaffected.