feat: add parallel-review subcommand (sharded map-reduce review) - #586
feat: add parallel-review subcommand (sharded map-reduce review)#586QuocHuannn wants to merge 17 commits into
Conversation
Shard a large diff into up to k concurrent background review tasks by directory ownership, supervise them with pid-liveness and log-stall checks (dead workers are cancelled and resumed on their original thread), then run one mandatory low-effort integration pass that verifies every finding and hunts cross-shard defects before emitting a single merged, ranked report. Small diffs fall back to the existing single adversarial review. Enabling changes kept opt-in: task requests accept an explicit resumeThreadId (deterministic recovery when several tasks are in flight) and an outputSchema passthrough (schema-enforced shard output); the cancel core is extracted into cancelJobRecord for reuse; process.mjs gains isPidAlive.
There was a problem hiding this comment.
Pull request overview
Adds a new parallel-review companion subcommand that shards large diffs into multiple concurrent background “shard” reviews, supervises them for silent worker death/stalls, then runs a mandatory cross-shard integration (“reduce”) pass that verifies/dismisses shard findings and looks for seam defects that span shards.
Changes:
- Introduces pure sharding + seam-hint extraction + finding merge/dedupe/rank + report rendering logic (
lib/parallel-review.mjs) with dedicated unit tests. - Extends the companion CLI with a
parallel-revieworchestrator, supervision/recovery loop, and two task-request extensions (resumeThreadId,outputSchema), plus a reusablecancelJobRecord. - Adds new prompts and a schema for the reduce pass (
parallel-reduce-output.schema.json) and exposes the slash command doc (/codex:parallel-review).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/parallel-review.test.mjs | Unit tests for sharding, seam hints, finding merge/dedupe, reduce-join behavior, JSON extraction, rendering, and pid liveness. |
| tests/commands.test.mjs | Updates command-doc enumeration to include parallel-review.md. |
| plugins/codex/scripts/lib/process.mjs | Adds isPidAlive(pid) helper for pid liveness checks. |
| plugins/codex/scripts/lib/parallel-review.mjs | New core library: shard planning, seam hint extraction, diff building, prompt building, finding merge/dedupe/id assignment, reduce outcome application, and report rendering. |
| plugins/codex/scripts/codex-companion.mjs | Adds parallel-review subcommand orchestration, supervision/recovery, resumeThreadId + outputSchema passthrough, and factors cancelJobRecord out of handleCancel. |
| plugins/codex/schemas/parallel-reduce-output.schema.json | New structured-output contract for reduce/integration pass. |
| plugins/codex/prompts/parallel-shard-review.md | New shard-level adversarial review prompt with structured output contract. |
| plugins/codex/prompts/parallel-reduce.md | New reduce/integration prompt focused on cross-shard seams plus per-finding verdicts. |
| plugins/codex/commands/parallel-review.md | New slash-command documentation and execution rules for /codex:parallel-review. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Never exit with children still queued or running, whatever went wrong. | ||
| for (const child of children) { | ||
| if (child.terminal) { | ||
| continue; | ||
| } | ||
| const record = readStoredJob(workspaceRoot, child.jobId); | ||
| if (record && !isTerminalJobStatus(record.status)) { | ||
| try { | ||
| await cancelJobRecord(cwd, workspaceRoot, record, "Cancelled because the parallel-review orchestrator exited."); | ||
| } catch { | ||
| // Best-effort teardown. | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Confirmed. Two escape paths existed: a recovery whose cancelJobRecord failed in the catch block left the previous attempt active while child.jobId already pointed at the replacement, and a child that exhausted its retry budget was marked terminal and returned without cancelling its still-alive worker — which the finally then skipped via the child.terminal check.
Fixed in e552538:
- the teardown now sweeps every attempt in
child.jobIds(guarded by a terminal-status check on the stored record, so completed/cancelled attempts are untouched) and no longer skips terminal children; - the retry-exhausted path in
recoverParallelChildnow best-effort cancels the abandoned attempt immediately, instead of letting a stalled-but-alive worker burn tokens until the orchestrator exits.
| // The job record can say "running" long after the worker died; trust the | ||
| // OS over the record, and treat a silent log as a hung turn. | ||
| const pidDead = record.pid != null && !isPidAlive(record.pid); | ||
| let stalled = false; | ||
| if (!pidDead && record.status === "running" && record.logFile && fs.existsSync(record.logFile)) { | ||
| stalled = Date.now() - fs.statSync(record.logFile).mtimeMs > PARALLEL_STALL_TIMEOUT_MS; | ||
| } |
There was a problem hiding this comment.
Confirmed. queued is normally a sub-second window before the worker flips the record to running in runTrackedJob, but a wedged startup — or a lost status write from the known state race (enqueueBackgroundTask writes the queued record after spawning the worker, and progress updates never patch status) — could sit there with a live pid until the global deadline.
Fixed in e552538: the log-staleness check now applies to queued records as well. This can't misfire on healthy jobs: the log file is created and touched at enqueue time (so a fresh spawn never looks stale), and a healthy worker keeps appending to its log even when the status field itself is stale-clobbered — the silent-for-6-minutes criterion is the same one already trusted for running, and the per-shard retry budget bounds the cost of a recovery.
![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: d63e865ba6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const job = buildTaskJob(workspaceRoot, { title, summary }, false); | ||
| const taskRequest = buildTaskRequest({ cwd, model, effort, prompt, write: false, resumeLast: false, jobId: job.id }); | ||
| taskRequest.outputSchema = shardSchema; | ||
| enqueueBackgroundTask(cwd, job, taskRequest); |
There was a problem hiding this comment.
When a user cancels a running parallel-review orchestrator, the shard jobs spawned here are detached task-worker processes, but their IDs are only kept in the in-memory children array. /codex:cancel <parent> invokes cancelJobRecord on the parent PID and never sees those child IDs, so the shards/reduce turns continue running and consuming tokens after the parent is marked cancelled. Persist the spawned job IDs on the parent record and have cancellation tear them down.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — this was real. cancelJobRecord terminates the orchestrator's process tree, there is no signal handler so the finally teardown never runs, and the detached shard/reduce workers survive with their ids held only in the in-memory children array.
Fixed in e552538:
- the orchestrator persists
childJobIdsonto its own job record before each spawn — initial shards, the reduce job, and every recovery respawn — so cancellation can never race a spawn (a persisted id whose spawn failed has no record and is skipped); cancelJobRecordnow cascades: after terminating the target it re-reads the record (picking up ids persisted after the first read), then best-effort cancels every non-terminal child, with a visited set guarding the recursion. Ordinary jobs have nochildJobIds, so single-job cancellation is unchanged.
Two adjacent limitations called out rather than fixed here: job-file writes are not atomic (pre-existing; a torn read during the cascade degrades to best-effort), and a kill that bypasses /codex:cancel (e.g. a manual kill -9 of the orchestrator) still can't cascade — both bounded by workers being one-shot turns that exit and record a terminal status on their own, and SessionEnd cleanup covers session teardown. Happy to file the atomic-write follow-up alongside #286/#517 if wanted.
| const IMPORT_RESOLVE_SUFFIXES = ["", ".ts", ".tsx", ".js", ".jsx", ".mjs", ".css", "/index.ts", "/index.tsx", "/index.js"]; | ||
|
|
||
| function runGit(cwd, args) { | ||
| return runCommandChecked("git", args, { cwd }).stdout; |
There was a problem hiding this comment.
On Windows, runCommand defaults to invoking commands through a shell, unlike the existing git wrapper in lib/git.mjs; this helper therefore sends branch refs and pathspecs collected from the repository through cmd.exe when parallel-review builds shard diffs. A changed filename or ref containing shell metacharacters can make the review command fail or execute unintended shell syntax, so pass shell: false here as the rest of the git code does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — this regressed the convention #447 established: runCommand defaults to a shell on win32, and every git invocation in this helper carries repository-derived refs and pathspecs. Fixed in e552538: runGit now passes shell: false, with the same comment lib/git.mjs uses. (Kept as a local helper since git.mjs's own wrappers aren't exported.)
- run git without a shell in the shard-diff helper; repository-derived refs and pathspecs must never pass through cmd.exe (matches lib/git.mjs) - apply the log-staleness stall check to queued jobs so a wedged startup is recovered instead of riding to the global deadline - sweep every spawn attempt in the exit teardown and cancel a child's still-alive worker when its retry budget is exhausted - persist child job ids on the orchestrator record before each spawn and cascade cancellation from the parent to its detached children
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5525381c0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| job, | ||
| (progress) => | ||
| executeParallelReviewRun({ | ||
| cwd, |
There was a problem hiding this comment.
When parallel-review is invoked from a subdirectory or with --cwd pointing inside the repo, Git still returns root-relative paths, but the shard runner later passes those paths to git diff -- <path> and fs.readFileSync(path.join(cwd, path)) relative to this subdirectory. For a change like a/f.txt while running from repo/a, the pathspec becomes repo/a/a/f.txt, so shard diffs and seam reads are empty or missing and the review can report success without seeing the actual changes. Pass the resolved repo root/workspace root into the parallel review execution instead of the command cwd.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — reproduced directly: from a subdirectory, git diff --numstat still prints root-relative paths while a root-relative pathspec passed back to git diff -- <path> matches nothing, so shard diffs come back empty and seam reads miss (readFileContent swallows the ENOENT). The single-review path already avoids this by going through collectReviewContext → context.repoRoot, so the parallel path was also inconsistent with the existing convention.
Fixed in ffdecc7: ensureGitRepository already returns the toplevel, so the handler now captures it and anchors the whole run there — target resolution, collectChangedFiles, the sharded execution (which also makes the spawned workers, enqueue, and turn interrupts all run at the repo root), and the single-review fallback. --invariants-file still resolves against the invocation cwd, since a user-typed relative path should resolve where it was typed.
| base: options.base, | ||
| scope: options.scope, | ||
| model, | ||
| focusText, |
There was a problem hiding this comment.
When the diff falls below the parallel gate, an explicit --invariants-file is read above but then dropped because the fallback calls the plain adversarial review with only focusText. For small diffs with user-supplied contracts, the review silently omits the exact invariants the user asked every shard to check, so regressions against those contracts can be missed. Include the invariants in the fallback prompt or avoid the fallback when invariants are provided.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the fallback dropped exactly the contract list the user asked every shard to verify. Fixed in ffdecc7: when --invariants-file is provided, the fallback now folds the invariant list into the review focus (Shared invariants that must hold for this change: …), so the single adversarial review checks the same contracts the shards would have. Kept it in the focus channel rather than adding a new placeholder to adversarial-review.md, to avoid touching the prompt surface of the existing /codex:adversarial-review command.
…he fallback - resolve the git toplevel once and run target resolution, shard diffs, seam reads, spawned workers, and the single-review fallback there, so invoking from a subdirectory cannot produce empty shard diffs - fold the shared invariant list into the fallback review focus so a below-gate run still checks the contracts every shard would have
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffdecc7b16
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (record.status === "running" || record.status === "queued") && | ||
| record.logFile && | ||
| fs.existsSync(record.logFile) | ||
| ) { | ||
| stalled = Date.now() - fs.statSync(record.logFile).mtimeMs > PARALLEL_STALL_TIMEOUT_MS; |
There was a problem hiding this comment.
While a shard or reduce turn is actively reasoning, the worker can emit no progress events for several minutes; the log mtime then stays older than PARALLEL_STALL_TIMEOUT_MS even though the PID is still alive. This branch cancels such live turns and, with only one retry, can mark slow large shards unrecovered and return a review missing that shard. Use an explicit heartbeat or avoid treating a live PID as stalled based only on log mtime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the premise holds: delta notifications are opted out at initialize (optOutNotificationMethods in app-server.mjs), so the log is only touched on item completions, and a live turn in a long reasoning stretch can legitimately sit under the 6-minute threshold.
Fixed in a53fd62 with a two-tier stall policy rather than a worker-side heartbeat:
- Dead pid / vanished record — unchanged, recovered immediately. This is the measured death mode (Jobs killed by host timeouts stay "running" forever (no pid liveness check); concurrent state writers can wipe all job state and silently disable stopReviewGate #517) and the OS signal is unambiguous.
- Live pid,
running, silent log — now recovered only after 15 minutes of silence (PARALLEL_LIVE_STALL_TIMEOUT_MS), with a one-time supervision notice at 6 minutes so the wait is visible. A healthy review turn keeps producing item-completion events (file reads etc.); 15 minutes of zero events with a live pid is hung with high confidence, the global--timeout-mindeadline remains the backstop, and recovery still resumes the original thread, so even a residual false positive keeps its prior analysis. - Live pid,
queued— keeps the 6-minute threshold; no turn is in flight there, so silence isn't ambiguous.
A timer-based worker heartbeat would prove the worker's event loop is alive, not that the turn is — it would neuter exactly the hung-turn detection this supervision exists for. A true turn-liveness heartbeat means re-enabling the opted-out delta notifications, which changes the initialize capabilities of the broker session shared by every consumer — happy to explore that as a follow-up if you'd take it, but it felt too cross-cutting for this PR.
One considered trade-off worth noting: later detection means a hang caught near the --timeout-min deadline may not leave room for its retry, turning what would have been a degraded report into a timeout failure (teardown still cancels everything either way). A small follow-up could skip the retry when the remaining budget is too thin and mark the shard unrecovered immediately so the reduce still runs.
| const reduceJob = buildTaskJob( | ||
| workspaceRoot, | ||
| { title: "Codex Parallel Reduce", summary: `Integration pass over ${findings.length} findings` }, | ||
| false |
There was a problem hiding this comment.
Because the reducer is created with buildTaskJob, it is stored as a normal jobClass: "task" and gets a threadId on completion. task-resume-candidate/task --resume-last pick the newest non-running task job, so after a successful parallel-review the next rescue/continue flow in the same session can resume this integration-review thread instead of the user's last real task. Mark parallel children non-resumable or exclude their kind from the resume candidate query.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — shard/reduce jobs are ordinary jobClass: "task" records with a threadId, in the caller's session, so they win the task-resume-candidate / task --resume-last lookup after a parallel run finishes.
Fixed in a53fd62: internal children are now created with resumable: false on their job records (initial shards, the reduce job, and recovery respawns), and the resume paths skip them. This covers all three pollution modes: findLatestResumableTaskJob no longer picks a shard/reduce thread after the run; the "Task X is still running" guard in resolveLatestTrackedTaskThread no longer lets in-flight children block a user's --resume-last mid-run; and the no-session fallback (findLatestTaskThread, which matches thread names by the TASK_THREAD_PREFIX search) is covered too, because children now name their threads after their job title (Codex Parallel Shard N / Codex Parallel Reduce) via a threadName override instead of the task prefix. Ordinary task jobs carry no resumable field and keep the default thread naming, so their behavior is byte-for-byte unchanged.
…sume - recover a live-pid worker only after 15 minutes of log silence (with a one-time supervision notice at 6): delta notifications are opted out, so an actively reasoning turn can legitimately emit nothing for minutes; dead pids and wedged queued startups keep fast recovery - mark parallel shard/reduce jobs resumable: false and name their threads after the job title so neither the resume-candidate lookup, the active-task guard, nor the thread-name fallback can hand the user an internal thread or block --resume-last mid-run
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a53fd62320
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| seams, | ||
| totals: { wallSec: Math.round((Date.now() - startedAt) / 1000), shardCount: plan.shards.length } | ||
| }; | ||
| const degraded = shardReports.some((report) => report.status !== "completed") || Boolean(reduceReport.error); |
There was a problem hiding this comment.
When a shard job completes but returns non-schema output, the code already records it in unparsed, yet this degraded check ignores that list. The parent job can therefore exit 0 and be stored as completed while an entire shard's findings are absent, which makes CI/status consumers treat an incomplete review as successful; include unparsed.length > 0 in the degraded condition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — a shard that completed but returned non-schema output contributed zero findings while the run still exited 0 and stored completed. Fixed in c909810: unparsed.length > 0 now makes the run degraded (exit 1), same as a failed shard or an unparsable reduce. A clean shard with zero findings still parses (findings: []), so nothing false-degrades, and a degraded run keeps its full result payload — /codex:result still returns whatever findings the other shards produced.
| const model = normalizeRequestedModel(options.model); | ||
| const effort = normalizeReasoningEffort(options.effort); | ||
| const reduceEffort = normalizeReasoningEffort(options["reduce-effort"]) ?? "low"; | ||
| const maxShards = Math.max(2, Number.parseInt(options["max-shards"] ?? `${DEFAULT_MAX_SHARDS}`, 10) || DEFAULT_MAX_SHARDS); |
There was a problem hiding this comment.
When a user passes a large --max-shards value, this can create more child jobs than the 50-job retention cap in lib/state.mjs. Because each child upsertJob prunes old job records while the parent is only refreshed in its job file, high-shard runs can delete the orchestrator or early child job files mid-run, making /codex:status/cancel lose the parent and causing the supervisor to recover shards whose records merely aged out; clamp the shard count or pin active parallel-review jobs outside the retention pruning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — and it's slightly worse than described: saveState runs on every upsertJob and deletes the pruned entries' job files and log files, pruning is ranked purely by updatedAt, the orchestrator only refreshes its state entry on phase changes (children refresh constantly), and --max-shards had no upper clamp. In a workspace whose state already holds ~50 historical entries, an active record could age out and lose its file mid-run.
Fixed in c909810, both ways:
pruneJobsnow ranks active (queued/running) jobs ahead of recency before applying the 50-slot cap. The invariant: an active job can only be pruned if there are more than 50 actives, and within the active tier recency still wins, so the current run's records can never be deleted mid-flight. This also closes the same latent hazard for ordinary long-running background tasks in busy workspaces, and the cap still bounds total size.--max-shardsis clamped to 8 (PARALLEL_MAX_SHARDS_LIMIT); a worst-case run (every shard retried, plus reduce and the parent) now stays under 20 records, well inside the cap. The command doc notes the limit.
One accepted trade-off: a stale running record from a crashed run (nothing left alive to mark it terminal) is now pinned instead of aging out, squeezing terminal history within the cap — visibility-only, not correctness, since newer actives still outrank it. A cheap follow-up would reap those during pruning by demoting non-terminal records whose pid is dead (isPidAlive treats EPERM/pid-reuse as alive, so it can only over-retain, never kill a live record); left out here to keep the saveState hot path untouched.
…ning - a shard that completed with non-schema output contributed no findings; count it toward the degraded exit status instead of reporting success - rank active jobs ahead of recency in retention pruning so an in-flight job's record and log can never be deleted mid-run, and clamp --max-shards to 8 so a worst-case run stays well inside the 50-slot cap
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c909810806
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const degraded = | ||
| shardReports.some((report) => report.status !== "completed") || unparsed.length > 0 || Boolean(reduceReport.error); |
There was a problem hiding this comment.
When the reduce turn ends with status failed but still leaves schema-shaped JSON in rawOutput (for example an interrupted or errored turn after a partial final message), runTrackedJob stores that result and extractJsonPayload makes reduceReport.error false. With all shards completed, this condition returns exit 0 even though the mandatory integration pass did not complete, so CI/users can trust findings that were not fully verified; include the reduce status in the degraded check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — runTrackedJob stores the result payload even for a failed execution, so a reduce turn that errored after emitting schema-shaped partial output parsed cleanly and never set reduceReport.error. Fixed in b1cbeb3: the degraded check now also requires reduceReport.status === "completed", so a failed/cancelled/unrecovered reduce always exits 1. Partial assessments that did parse are still applied and the full payload is kept, so /codex:result shows what the integration pass managed to verify — the exit status just no longer claims it finished.
| scope with `git diff --shortstat <base>...HEAD` (or `git diff --shortstat` | ||
| for working-tree scope) and confirm with the user once if they have not |
There was a problem hiding this comment.
For working-tree parallel reviews where the diff is staged, this instruction tells Claude to inspect only git diff --shortstat; I checked git diff -h, which lists a separate --cached form for staged changes, and a staged-only fixture makes the unstaged shortstat empty. A large staged review can therefore be treated as empty/small and skip the required token-cost confirmation or be misrouted, so the command should check both git diff --shortstat --cached and git diff --shortstat like the other review commands.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the CLI's working-tree target diffs against HEAD (staged and unstaged both included, plus untracked files), so the doc's plain git diff --shortstat pre-check underestimated staged-only changes and could skip the cost confirmation. Fixed in b1cbeb3: the doc now matches the convention review.md / adversarial-review.md already use — inspect both git diff --shortstat --cached and git diff --shortstat, and treat untracked files as reviewable work even when both are empty.
…t check - a reduce turn that failed can still leave parseable partial output; require reduce completion for a clean exit while keeping whatever assessments did parse in the payload - the working-tree target diffs against HEAD, so the command doc now inspects staged changes and untracked files in the pre-launch cost check like the other review commands
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1cbeb36df
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| function diffRangeArgs(target) { | ||
| return target.mode === "branch" ? [`${target.baseRef}...HEAD`] : ["HEAD"]; |
There was a problem hiding this comment.
For working-tree reviews this returns only HEAD, so both collectChangedFiles and buildShardDiff use git diff HEAD. Git's help lists the cached/index form separately (git diff --cached), and an MM path whose unstaged edit reverts the staged edit has a non-empty cached diff but an empty git diff HEAD; in that case parallel-review reports “No changes found” or omits the file from every shard. Collect staged and unstaged path/diff data separately, like collectReviewContext does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — a staged edit whose worktree copy reverts it is invisible to a single git diff HEAD, so the file vanished from planning and shard diffs even though committing would still land it. Fixed in fdaaaa3: working-tree targets now read two legs, git diff --cached (HEAD→index) and git diff (index→worktree), exactly the split collectReviewContext uses. collectChangedFiles unions paths across both legs (staged status wins, churn sums, binary ORs) and buildShardDiff concatenates both legs per file — for an ordinary unstaged edit the cached leg is empty, so shard diffs are byte-identical to before; the MM case now shows both hunks. Covered by a new fixture test: stage an edit, revert the worktree, assert the file is collected and its shard diff contains the staged hunk.
| report.summary = extraction.payload.summary ?? null; | ||
| report.findingCount = extraction.payload.findings.length; | ||
| for (const raw of extraction.payload.findings) { | ||
| rawFindings.push(normalizeShardFinding(raw, child.shard.id)); |
There was a problem hiding this comment.
Each shard is told that findings outside its file list will be discarded, but this loop accepts every raw.file the model returns. When a shard inspects a neighboring seam or dependency and reports the finding on that other file, the out-of-scope result is merged and sent to reduce/final output, creating duplicates or false positives outside the shard's ownership; filter against child.shard.files before pushing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the prompt promised the discard but the merge loop accepted every raw.file. Fixed in fdaaaa3: shard findings are now filtered against the shard's file list (after normalizing a leading ./) before merging; dropped ones are counted as outOfScope on the shard's report entry so the behavior is observable rather than silent, and findingCount now reflects the post-filter set. Cross-file defects remain the reduce pass's job via the seam hints, as designed.
| const degraded = | ||
| shardReports.some((report) => report.status !== "completed") || | ||
| unparsed.length > 0 || | ||
| reduceReport.status !== "completed" || | ||
| Boolean(reduceReport.error); |
There was a problem hiding this comment.
When the reducer emits schema-valid JSON but omits one or more finding IDs, applyReduceOutcome counts only the IDs present and defaults the rest to SUSPECTED, but this success check ignores reduceReport.assessed < findings.length. A truncated/partial integration response can therefore exit 0 even though the reduce prompt requires an assessment for every finding, so CI/users trust an incomplete integration pass; include the assessed-count gap in degraded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — a truncated reduce response that still parsed could leave findings defaulted to SUSPECTED while the run exited 0. Fixed in fdaaaa3: reduceReport.assessed < findings.length now makes the run degraded, so a reduce that skipped ids can't claim a complete integration pass. While wiring this up we also tightened the count itself: assessed now counts only assessments that matched one of our finding ids, so an id the reducer invented can't compensate for one it skipped (unit-tested). The SUSPECTED defaults are still applied and reported — only the exit status stops vouching for them, and zero-finding runs can't false-degrade.
…ssessments - working-tree targets now read HEAD->index and index->worktree as separate legs so a staged edit hidden by a worktree revert is still collected and shown in shard diffs (fixture-tested) - enforce the shard prompt's contract by dropping findings outside the shard's file list, counted as outOfScope on the shard report - degrade the run when the reducer assessed fewer findings than were merged, counting only assessments that matched real finding ids
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdaaaa32ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| outOfScope: 0 | ||
| }; | ||
| shardReports.push(report); | ||
| const extraction = extractJsonPayload(record?.result?.rawOutput ?? "", (parsed) => Array.isArray(parsed.findings)); |
There was a problem hiding this comment.
When a shard completes with a truncated but parseable object such as {"findings":[]}, this predicate treats it as a valid shard report, so it never lands in unparsed and the parent can exit 0 with that shard's review effectively missing. The fresh issue is that the loose shape check bypasses the schema fields that the prompt requires (verdict, summary, next_steps, and valid finding entries); validate the full review schema here and degrade/drop the shard when it does not match.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the loose Array.isArray(parsed.findings) predicate let a truncated {"findings":[]} pass as a clean shard. Fixed in df91a12: the extraction predicate now mirrors the review schema's required contract (verdict, summary, findings, next_steps), so a truncated object lands in unparsed and degrades the run like any other dropped shard. Entry-level validation stays with normalizeShardFinding's coercion plus the shard-scope filter, so a stray malformed finding can't fail an otherwise complete report.
| let seamFindings = []; | ||
| const reduceExtraction = extractJsonPayload( | ||
| reduceRecord?.result?.rawOutput ?? "", | ||
| (parsed) => Array.isArray(parsed.assessments) || Array.isArray(parsed.seam_findings) |
There was a problem hiding this comment.
When the reduce turn completes with all assessments but omits seam_findings (or summary), this || still accepts the payload; applyReduceOutcome then defaults the missing seam list to [], assessed === findings.length, and the run can exit 0 without evidence that the mandatory cross-shard seam hunt was performed. Require the full reduce contract here, especially an explicit seam_findings array, before treating the integration pass as successful.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the || accepted a payload missing seam_findings (or summary), so a run could exit 0 with no evidence the seam hunt happened. Fixed in df91a12: the reduce extraction now requires the full contract — summary string plus explicit assessments and seam_findings arrays — matching the schema's required list; anything less sets reduceReport.error and degrades the run.
| } | ||
| let weight = BINARY_FILE_WEIGHT; | ||
| try { | ||
| weight = fs.readFileSync(path.join(cwd, filePath), "utf8").split("\n").length; |
There was a problem hiding this comment.
For working-tree reviews with a large untracked generated file or binary dump, this reads the entire file as UTF-8 just to estimate shard weight before any gate or size cap is applied; even if the run later falls back or omits the diff, a large artifact can stall or OOM the orchestrator. Use a stat/byte cap and binary detection (or a default weight) before reading untracked content, and apply the same cap when embedding it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — both the weight estimate and the new-file embed read untracked content unboundedly. Fixed in df91a12, mirroring the 24KB bound lib/git.mjs already uses for untracked content: the collector stats before reading, only reads files within the cap, uses isProbablyText to flag binaries (default weight, no read for oversized files), and the shard-diff embed replaces oversized/binary content with a (N bytes, content omitted) placeholder — the reviewer still sees the file exists and has read-only repo access to sample it. The seam-hint reader also gained a 1MB sanity cap so a generated bundle is skipped rather than loaded whole. Fixture-tested for the text, binary, and oversized cases.
…cked reads - require the shard extraction to carry every field the review schema mandates (verdict, summary, findings, next_steps) so a truncated but parseable object degrades the run instead of passing as a clean shard - require the reduce payload to include summary, assessments, and an explicit seam_findings array as evidence the seam hunt ran - stat untracked files before reading, skip binaries and content over 24KB (matching lib/git.mjs), and cap the seam reader so a generated artifact cannot stall or OOM the orchestrator
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df91a12964
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let text = ""; | ||
| for (const argSet of argSets) { | ||
| try { | ||
| text += runGit(cwd, ["diff", "-M", ...argSet, "--", ...pathspec]); |
There was a problem hiding this comment.
In an environment with GIT_EXTERNAL_DIFF or diff.external configured, this plain git diff invokes the external helper; I verified the same command returns helper output unless --no-ext-diff is passed. That means parallel shards can receive custom/empty output instead of the real patch, and the review-only command can run local diff helpers while collecting evidence. Build shard hunks with the same protected diff flags used by collectReviewContext, especially --no-ext-diff.
Useful? React with 👍 / 👎.
| let text = ""; | ||
| for (const argSet of argSets) { | ||
| try { | ||
| text += runGit(cwd, ["diff", "-M", ...argSet, "--", ...pathspec]); |
There was a problem hiding this comment.
For a tracked file with a very large patch, such as a generated bundle or lockfile rewrite, this appends the entire git diff stdout before the MAX_SHARD_DIFF_BYTES budget is checked below. A file that will ultimately be listed as omitted can still stall or OOM the orchestrator while its full patch is buffered. Limit the git stdout size or stream it, then mark the file omitted as soon as it exceeds the shard budget.
Useful? React with 👍 / 👎.
| untracked files as reviewable work even when both are empty. | ||
| - Always run the command in a Claude background task; a full run takes | ||
| several minutes end-to-end: | ||
| `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" parallel-review <arguments>` |
There was a problem hiding this comment.
This launch instruction does not use or quote the raw $ARGUMENTS placeholder, unlike the other command docs. When a user supplies focus text or an invariants path containing shell metacharacters or quotes, Claude can splice it into Bash as syntax instead of one safe argument, so the review runs with corrupted args or unintended shell commands. Use a background Bash example that invokes parallel-review "$ARGUMENTS" or otherwise shell-quotes the raw arguments.
Useful? React with 👍 / 👎.
…es atomic - buildShardDiff bounded each per-file git diff and routed an oversized (>1MB) diff to the omitted list so a large modified/deleted/renamed file can no longer vanish from a shard while the run still exits 0; raise the metadata git buffer above spawnSync's 1MB default for large diffs - write job files and state.json atomically (temp + rename) so a reader polling during a write can never catch a truncated file — the case that under many concurrent workers could crash supervision or delete unrelated job records - surface per-shard out-of-scope finding drops in the report instead of discarding them silently, and document the --max-shards 2..8 clamp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3b218820b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Concurrent job-state writers race; give each enqueue a head start. | ||
| await sleep(PARALLEL_SPAWN_STAGGER_MS); |
There was a problem hiding this comment.
This stagger only separates the initial enqueue writes; once the shards are running, every detached worker still calls the load/mutate/save upsertJob path from progress and completion callbacks at the same time. If two workers save concurrently, a stale state.jobs snapshot can overwrite the other worker's entry and saveState can treat that live job as pruned and delete its job file/log, so /codex:status, cancellation, or the supervisor can lose an active shard during a normal parallel review. Use a workspace-level lock or merge-on-write for job state instead of relying on a spawn delay.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and thank you — the atomic-write change in b3b2188 fixed torn reads but not this read-modify-write race, which is a separate hazard: two workers loadState → mutate → saveState concurrently, and a stale in-memory snapshot at save time makes the delete loop treat a live job (present in the fresh previousJobs, absent from the stale result) as pruned and remove its job file/log.
Fixed in 0774316 with merge-on-write in updateState: after the mutation, any job still queued/running on disk that is missing from the result is merged back before saving. The mutators routed through updateState (upsertJob, setConfig) only add or update, never remove, so a missing active job was necessarily added by a concurrent writer — re-adding it means a stale write can neither delete a live job's files nor lose it from the index. I deliberately scoped this to updateState rather than saveState: the one caller that intends to remove active jobs — session teardown in session-lifecycle-hook, which calls saveState directly — must still be able to, and a unit test now pins both directions (a dropped active job is merged back through updateState; a direct saveState removal still deletes).
Full disclosure on the residual: this closes the data-loss path (no live job's files are deleted) and the common index-loss case, but it does not fully serialize the index — a narrow interleaving can still transiently drop an active job from the index (never its files), which self-heals on the job's next upsertJob (running jobs update within seconds) and affects neither the supervisor (which reads per-job files, not the index) nor /codex:cancel (which uses the parent's persisted childJobIds). A workspace-level lock would close that last window; I kept it out of this PR because a cross-process lock on the shared state store touches every command and warrants its own change — glad to follow up if you'd prefer the full serialization here.
| cwd, | ||
| model: child.model, | ||
| effort: child.effort, | ||
| prompt: threadId ? PARALLEL_RESUME_PROMPT : child.prompt, |
There was a problem hiding this comment.
When a reduce worker is recovered after it has created a thread, this shared recovery path resumes it with PARALLEL_RESUME_PROMPT, whose text tells the model to finish the review of “your shard.” In the reduce-retry scenario that prompt conflicts with the mandatory integration/seam pass the reducer was doing, so it can produce a schema-valid but incomplete integration result after a stall or dead worker. Use a reduce-specific resume prompt, or reuse child.prompt, when child.kind === "reduce".
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the shared recovery path resumed every child with PARALLEL_RESUME_PROMPT ("finish the adversarial review of your shard"), which is wrong for a recovered reduce turn and could yield a schema-valid but incomplete integration result. Fixed in 0774316: recoverParallelChild now picks the resume prompt by child.kind — a reduce-specific PARALLEL_REDUCE_RESUME_PROMPT that tells the model to finish the cross-shard integration pass (assess every finding, hunt seam defects) for a reduce child, and the shard prompt otherwise.
… own prompt - merge active jobs back on write in updateState: concurrent detached workers load/mutate/save the shared index, and a stale snapshot could make saveState delete a live job's files; the updateState mutators only add/update, so a missing active job was added concurrently and is merged back. Direct saveState callers that intend removal (session teardown) are left untouched, pinned by tests in both directions - recover a reduce worker with a reduce-specific resume prompt so a recovered integration turn is told to finish the cross-shard pass instead of reviewing 'your shard'
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0774316020
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const job of loadState(cwd).jobs) { | ||
| if (!resultIds.has(job.id) && (job.status === "queued" || job.status === "running")) { | ||
| state.jobs.push(job); |
There was a problem hiding this comment.
Fresh evidence after the concurrency fix: this merge only restores jobs that are missing and still queued/running, so a stale updater can still overwrite or delete a concurrently completed shard/reduce job. If worker A loaded state while worker B was running, then B completes before A reaches this loop, B is either already present in A's stale snapshot as running or missing but no longer active, so it is not refreshed from disk; the following saveState can write B back as running or prune its completed job file/log, leaving /codex:status/result and parallel-review supervision with a lost or misreported finished child. Merge current on-disk records for concurrent ids, not just missing active jobs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the active-only merge was the wrong shape: a concurrently completed job B was either rewritten back to running (if the stale snapshot still held it) or deleted (if the snapshot never had it), losing a finished child from /codex:status, /codex:result, and the supervisor's view.
Fixed in ebeb618 by rebasing instead of merging-by-status. updateState now snapshots each job's pre-mutation JSON, and saveState (under a reconcile flag it passes) carries forward only the jobs this mutation actually changed, taking every other job fresh from the same authoritative previousJobs read the delete loop uses. Because the updateState mutators (upsertJob, setConfig) only ever touch the single job they target, every concurrent id — added, or advanced to completed — is preserved at its on-disk value and never deleted. Sharing the one read between the rebase and the delete decision also closes the earlier window where saveState's independent re-read could still prune a concurrently-added job. New unit test pins it: a stale updater that still thinks a completed job is running no longer reverts its status or deletes its file, while its own new job is still written; the session-teardown direction (direct saveState) still removes as before.
Residual, stated precisely: with data-loss now closed (no file deletion, no status loss for any job present in the read), the only thing left is the irreducible lock-free window — a concurrent writer committing between this call's single loadState and its atomic rename can lose an index update (never a file, never a completed status for a job already on disk), which self-heals on that job's next write. Fully closing that needs a workspace-level lock serializing the read-modify-write; as noted on the previous thread I've kept that as a follow-up since it touches every command's use of the shared store, and I'm glad to do it here if you'd rather land the full serialization in this PR.
…conciliation updateState now snapshots each job before mutating and saveState rebases only the jobs this mutation changed onto the same authoritative on-disk read the delete loop uses, taking every other job fresh. Because the updateState mutators only touch their own job, a concurrently added or completed job is preserved at its on-disk value and never reverted to a stale status or pruned. Sharing one read between the rebase and the delete also closes the window where saveState's independent re-read could prune a concurrently-added job. Session teardown (direct saveState) still removes as before.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebeb618df7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (fs.statSync(absolute).size > 1_000_000) { | ||
| return null; | ||
| } | ||
| return fs.readFileSync(absolute, "utf8"); |
There was a problem hiding this comment.
When a working-tree review contains staged/index-only content, git diff -h lists --cached as a separate diff form and the shard builder already includes that leg, but this callback reads only the current worktree file. If an import or CSS token change is staged and then the worktree copy is reverted or partially staged, the reducer's seam_hints are computed from stale content and can omit the cross-shard seam the integration pass is meant to check; derive seam inputs from the same Git snapshots/diffs as the shard diff instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the seam reader used the worktree while the shard diffs use the reviewed snapshot(s), so the two could disagree: a branch review reading a dirty worktree, or a working-tree review missing a staged change whose worktree copy was reverted.
Fixed in 371bfd0 with a new readReviewedFileContent(cwd, target, path) that the seam callback now uses instead of a raw worktree read:
- branch review →
git show HEAD:<path>(the reviewed tip, ignoring unrelated uncommitted edits); - working-tree review → the staged blob (
git show :<path>) and the worktree file scanned together, so a seam introduced in either leg — including a staged change the worktree later reverted — is still seen.
Reads are bounded (1MB cap via maxBuffer, missing/oversized snapshots skip to null), and extractSeamHints already limits itself to changed source/style files (skipping deleted and binary), so this adds at most one or two git show calls per changed source file at startup. A git-repo unit test pins both directions (branch reads HEAD not a dirty worktree; working-tree scans staged content after a worktree revert). Scanning both legs is a deliberate over-approximation — seam hints are advisory and the reducer hunts seams independently, so surfacing a token that exists in either reviewed leg is the safe direction versus missing it.
Seam extraction read the worktree file while the shard diffs use the reviewed snapshot, so the two could disagree: a branch review would read unrelated uncommitted edits, and a working-tree review would miss a staged change whose worktree copy was reverted. readReviewedFileContent now reads HEAD for a branch review and scans the staged blob plus the worktree file for a working-tree review, matching the diff's legs. Reads stay bounded (1MB), limited to changed source files, and cover both directions with a git-repo test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 371bfd0305
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const ownedPaths = new Set(child.shard.files.map((file) => file.path)); | ||
| for (const raw of extraction.payload.findings) { | ||
| const finding = normalizeShardFinding(raw, child.shard.id); | ||
| if (!ownedPaths.has(finding.file.replace(/^\.\//, ""))) { |
There was a problem hiding this comment.
When a shard cites the path it sees in the inline git diff header, such as b/src/foo.ts (or a/src/foo.ts for deletions), this ownership check strips only a leading ./, while ownedPaths contains src/foo.ts. That valid in-shard finding is counted as out-of-scope, dropped before the reduce pass, and the run can still exit successfully with the finding missing; normalize the common diff prefixes before applying the shard filter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the filter stripped only ./, so a finding citing the diff-header path (b/src/foo.ts, or a/… on a deletion) missed ownedPaths (src/foo.ts) and a valid in-shard finding was dropped before reduce.
Fixed in b59901d with a resolveOwnedFindingPath(ownedPaths, rawPath) helper the filter now uses: it strips a leading ./ or /, tries the literal path first (so a real top-level a/- or b/-named file is never mis-stripped), then falls back to stripping the a//b/ diff prefix. A resolved finding's file is normalized to the owned path so dedupe and the reduce pass see a consistent path; a genuinely out-of-scope path still resolves to null and is counted in outOfScope. Unit-tested across plain, ./, /, a/, b/, a real a/-named file, and out-of-scope paths.
… check A shard citing a finding by its git diff header path (b/src/foo.ts, or a/... for a deletion) missed ownedPaths (src/foo.ts) and a valid in-shard finding was dropped as out-of-scope. resolveOwnedFindingPath strips a leading ./ or /, matches the literal path first so a real top-level a/- or b/-named file is not mis-stripped, then falls back to stripping the a//b/ diff prefix, and normalizes the finding's path to the owned one for consistent dedupe and reduce input.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b59901d38b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } catch { | ||
| // Unreadable; keep the default weight. | ||
| } | ||
| files.push({ path: filePath, weight, binary, status: "A", oldPath: null }); |
There was a problem hiding this comment.
When a working-tree review has git rm f staged and then recreates f, Git exposes both the cached deletion and the same path as an untracked file. This unconditional append creates two shard entries for one path; buildShardDiff then sees the cached deletion for the A entry and never embeds the recreated worktree content, so the shard reviews the deletion twice and misses the new file. Reconcile same-path untracked replacements or force the worktree content into the added leg.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — a staged git rm f followed by recreating f in the worktree surfaces the path twice (a cached deletion and an untracked entry), and the unconditional append made two shard entries; buildShardDiff then showed the deletion for both and never embedded the recreated content.
Fixed in 41817dd: collectChangedFiles now reconciles a same-path untracked entry into the existing diff entry (flagging it recreated and folding in its churn) instead of duplicating it, and buildShardDiff appends the recreated worktree content (capped, binary-aware, via a shared embedWorktreeFile helper) after the deletion diff, so the shard sees both the staged delete and the new content. Covered by a git-repo test that stages a delete, recreates the file, and asserts one entry whose shard diff contains the recreated content.
| if (assessment) { | ||
| assessed += 1; |
There was a problem hiding this comment.
If a completed reduce turn returns an assessment object for every id but omits or corrupts verdict (for example {id:"f1"}), this counts each object as assessed while line 631 defaults the missing verdict to SUSPECTED; the later degraded check only compares the assessed count, so the run can exit 0 without the required CONFIRMED/SUSPECTED/REJECTED integration verdicts. Count only schema-valid assessments, or reject the reduce payload before applying it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — assessed counted any assessment object present for a finding id, so a reduce that returned {id:"f1"} for every id (no/invalid verdict) reached assessed === findings.length and exited 0 while every finding silently defaulted to SUSPECTED.
Fixed in 41817dd: an assessment now counts toward assessed only when its verdict is one of the schema's valid values (CONFIRMED/SUSPECTED/REJECTED), using the same check that decides whether to apply it. A reduce that omits or corrupts verdicts therefore leaves assessed < findings.length, which the degraded check turns into a non-zero exit. Unit-tested: an assessment missing its verdict is not counted and the finding falls back to SUSPECTED.
…icts - a staged 'git rm' whose path is recreated in the worktree surfaced as two shard entries and buildShardDiff embedded neither the recreated content; collectChangedFiles now reconciles the untracked recreate into the single diff entry and buildShardDiff appends the worktree content after the deletion via a shared embedWorktreeFile helper - applyReduceOutcome now counts an assessment toward completion only when its verdict is a valid CONFIRMED/SUSPECTED/REJECTED value, so a reduce that returns verdict-less objects for every id no longer exits 0 with everything silently defaulted to SUSPECTED
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41817ddfff
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let oversized = false; | ||
| for (const argSet of argSets) { | ||
| try { | ||
| text += runGit(cwd, ["diff", "-M", ...argSet, "--", ...pathspec], { maxBuffer: MAX_SHARD_DIFF_BYTES + 1 }); |
There was a problem hiding this comment.
When a changed file name itself uses Git pathspec magic such as :(glob)*.js (or the oldPath side of a rename), this call passes it after --, but Git still interprets pathspec magic there. I verified git diff -- ':(glob)*.txt' includes every matching .txt file rather than only the literal file, so a shard can receive hunks from files it does not own or miss the intended literal path, leading to false positives or missed defects in the merged review. Prefix per-file paths with :(literal) or run these diffs with literal pathspec handling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — a filename carrying pathspec magic (:(glob)…, a leading :, or a glob character) is still interpreted after --, so a shard could pull in hunks from files it doesn't own or miss its literal path.
Fixed in 315080e by running every git call through --literal-pathspecs (added once in the runGit wrapper), which disables all pathspec magic — every path this code passes is a literal repo path (shard diffs, the rename oldPath side, seam reads), never an intended glob. A git-repo test proves it: a shard owning a file literally named sub/*.txt gets only that file's diff, with a sibling sub/data.txt (which the glob would otherwise match) staying out of the shard.
A changed filename carrying pathspec magic (:(glob)..., a leading :, or a glob character) was still interpreted after --, so a shard could pull in hunks from files it does not own or miss its literal path. runGit now passes --literal-pathspecs on every call; all paths this code hands git are literal repo paths. A test proves a shard owning a file literally named sub/*.txt no longer globs in a sibling sub/data.txt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 315080e12a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (rawPath.includes(" => ")) { | ||
| return rawPath.split(" => ").pop(); |
There was a problem hiding this comment.
In a parallel-eligible diff where a changed file's literal name contains =>, this branch rewrites the path to only the suffix. I checked git diff --numstat -M in a fixture with a modified file named foo => bar.txt, and Git prints 1 1 foo => bar.txt even though it is not a rename; collectChangedFiles then records bar.txt, so buildShardDiff gets an empty diff for a nonexistent path and the shard can miss the actual file. Correlate with --name-status rename records or parse -z output before normalizing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — git diff --numstat -M prints 1\t1\tfoo => bar.txt for a modified file whose name literally contains =>, and normalizeRenamePath then rewrote it to bar.txt, so the shard diffed a nonexistent path and missed the real file.
Fixed in 372f8ca by switching collectChangedFiles to NUL-terminated output, which is the robust fix for the whole class rather than the arrow alone. --name-status -M -z, --numstat -M -z, and ls-files --others -z are now parsed by parseNameStatusZ/parseNumstatZ: real renames are recognized structurally (a rename record carries the old and new paths as separate NUL fields) instead of by scanning for =>, and pathnames come through verbatim — so a filename containing =>, a tab, a quote, or non-ASCII bytes (which the non--z output would C-quote) is handled correctly. normalizeRenamePath and its heuristic are removed. Tests cover the parsers directly (including a real-rename record and a literal => name) plus a git-repo test that modifies a file named weird => name.txt and confirms it's recorded and diffed under its real name.
normalizeRenamePath rewrote any path containing ' => ' to its suffix, so a modified file literally named 'foo => bar.txt' was recorded as 'bar.txt' and the shard diffed a nonexistent path. collectChangedFiles now reads --name-status/--numstat/ls-files with -z and parses the NUL-terminated records (parseNameStatusZ/parseNumstatZ): renames are recognized structurally by their separate old/new fields, and pathnames come through verbatim, so names with ' => ', tabs, quotes, or non-ASCII bytes are handled instead of being mis-parsed or C-quoted. Removes the normalizeRenamePath heuristic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 372f8ca517
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const absolute = path.join(cwd, relativePath); | ||
| const stat = fs.statSync(absolute); | ||
| if (stat.size <= MAX_UNTRACKED_READ_BYTES) { | ||
| const content = fs.readFileSync(absolute); |
There was a problem hiding this comment.
In a large working-tree parallel review that includes an untracked symlink whose target is a small text file, git ls-files --others reports the symlink path but this statSync/readFileSync path follows the link. That means the shard prompt embeds the target file's contents — even if the target is outside the repository — instead of reviewing the symlink itself, which can leak unrelated local data and give the reviewer the wrong artifact. Use lstat/readlink to render the symlink target, or skip symlinks rather than dereferencing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the untracked-file reads used statSync/readFileSync, which dereference a symlink, so an untracked symlink would embed its target's contents into the shard prompt (even a file outside the repo) instead of the symlink itself.
Fixed in 719ad2b: all three worktree read sites now lstatSync first and never follow a link. A symlink is rendered as its target path (--- new file (symlink): link -> target ---), matching what git stores as the symlink's blob, and non-regular entries are skipped; the weight estimate treats a symlink as one line, and seam scanning skips symlinks entirely (a link has no imports/tokens to scan). A git-repo test proves an untracked symlink pointing at a secret file embeds link -> secret.txt and never the target's contents.
The untracked-file read paths used statSync/readFileSync, which follow a symlink, so an untracked symlink embedded its target's contents into the shard prompt — even a file outside the repo. All three read sites now lstat first: a symlink renders as its target path (matching git's blob), non-regular entries are skipped, the weight estimate counts a symlink as one line, and seam scanning skips symlinks. Test proves an untracked symlink embeds 'link -> target', never the target's contents.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 719ad2b366
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const validVerdict = REDUCE_VERDICTS.has(assessment?.verdict); | ||
| if (assessment && validVerdict) { | ||
| assessed += 1; |
There was a problem hiding this comment.
When a completed reducer returns assessment objects with only id and a valid verdict, this counts them as assessed even though the reduce schema requires a note for every assessment. In that partial-output case reduceReport.assessed can equal findings.length, the run exits successfully, and REJECTED findings are hidden without the required explanation; the remaining gap is that the code validates the verdict but not the full assessment item before counting it.
Useful? React with 👍 / 👎.
Implements the proposal in #585: a
parallel-reviewcompanion subcommand that shards a large diff into concurrent background review tasks (map), supervises them against silent worker death, then runs one mandatory cross-shard integration pass (reduce) and returns a single merged, ranked report.Why
A single review of a large multi-workstream diff is single-threaded end-to-end and fragile: on a real 24-file / ~1.8k-changed-line diff, one run took 40+ minutes and its worker died silently mid-run — total loss (#517 / #540 / #509 describe the death modes; #68 asks for parallel reviews directly). The same diff through this design: 9.4 minutes end-to-end (~4.3×), with a SIGKILLed worker auto-recovered mid-run (measured with a behavior-identical prototype orchestrating the current companion CLI from the outside; happy to share the raw run data on the issue).
How it works
lib/parallel-review.mjs, pure + unit-tested). Changed files are grouped by directory, oversized directories are split per-file so no shard becomes the bottleneck, and units are LPT-balanced into ≤--max-shardsshards.task-class background job through the existingenqueueBackgroundTaskpath, with: only its own hunks inline (read-only repo access for anything else), the full shared invariant list (--invariants-file), and the existingreview-outputschema enforced natively via a newrequest.outputSchemapassthrough — shard outputs parse deterministically instead of by convention. Spawns are staggered 3s apart because concurrent state writers race (Same-cwd parallel /codex:* races on jobs.json + broker.json: data loss + orphan brokers #286).isPidAliveinlib/process.mjs), whose log has stalled, or whose record disappeared is cancelled and resumed on its original thread via a newrequest.resumeThreadId— the analysis done before the crash is kept, and recovery does not depend on the session-scoped--resume-lastlookup (which is ambiguous with several tasks in flight). One retry per shard.--reduce-effort lowtask receives the merged findings (stable ids), mechanical seam hints extracted from the diff (imports and CSS tokens crossing shard boundaries), and a fixed seam checklist. It must assess every finding (CONFIRMED / SUSPECTED / REJECTED, schema-enforced viaschemas/parallel-reduce-output.schema.json) and hunt cross-shard defects no single shard could see whole. In the measured run, the highest-value defect was exactly this class: a global CSS token made translucent in one shard silently breakingbg-*/NNconsumers owned by other shards.finallyblock cancels every non-terminal child on any exit path, so the orchestrator can never leave orphaned jobs behind. Children run in the caller's session, so they stay visible in/codex:status, cancellable via/codex:cancel, and covered by the existing SessionEnd cleanup.Changes
plugins/codex/scripts/lib/parallel-review.mjs(new): shard planner, seam-hint extraction, finding merge/dedupe/rank, reduce-verdict join, prompt builders, report renderer. Pure logic with injected I/O — covered bytests/parallel-review.test.mjs.plugins/codex/scripts/codex-companion.mjs:parallel-reviewhandler + orchestrator; two small task-request extensions used by it (resumeThreadId— explicit thread resume wins over the ambiguous latest-thread lookup;outputSchema— passthrough torunAppServerTurn);handleCancel's core extracted into a reusablecancelJobRecord(behavior unchanged) so the supervisor can cancel dead children with a proper interrupt + record update.plugins/codex/scripts/lib/process.mjs:isPidAlive(signal-0 probe, EPERM counts as alive).plugins/codex/prompts/parallel-shard-review.md,parallel-reduce.md(new): shard and integration prompts, same adversarial stance and structured-output conventions asadversarial-review.md.plugins/codex/schemas/parallel-reduce-output.schema.json(new): reduce output contract.plugins/codex/commands/parallel-review.md(new):/codex:parallel-review, review-only, always run as a Claude background task, with an explicit ~k×-token cost confirmation rule.tests/parallel-review.test.mjs(new): gate behavior, directory cohesion + weight balance, oversize splitting, rename-path normalization, cross-shard seam extraction (imports, CSS tokens, global styles), merge/dedupe severity semantics, reduce-verdict join incl. SUSPECTED default, tolerant JSON extraction, renderer sections,isPidAlive.No existing behavior changes:
review,adversarial-review,task,status,result,cancelare untouched except the extracted-but-equivalent cancel core and the two opt-in task-request fields (absent in all existing requests).Testing
npm test: 101/102 locally — every suite green including the newparallel-reviewtests. The one failure (git.test.mjs"skips untracked directories") is a pre-existing environment artifact on my machine: a global gitignore that ignores.claude/makes the fixture's untracked dir invisible to git; it fails identically on a pristinemaincheckout and should be green in CI.Deliberate scope cuts (follow-ups if wanted)
task --resume <thread-id>flag (the plumbing now exists asrequest.resumeThreadId).statusoutput for all jobs (Jobs killed by host timeouts stay "running" forever (no pid liveness check); concurrent state writers can wipe all job state and silently disable stopReviewGate #517).state.jsonwrites (Same-cwd parallel /codex:* races on jobs.json + broker.json: data loss + orphan brokers #286, Jobs killed by host timeouts stay "running" forever (no pid liveness check); concurrent state writers can wipe all job state and silently disable stopReviewGate #517) — the stagger reduces but does not eliminate the race.