feat(review): exact scope and parallel skills for reviewer agents

[peyton-alt]

https://entire.io/gh/entireio/cli/trails/766

One-sentence summary

When entire review launches a reviewer, it now hands that reviewer everything the CLI already knows — exactly which code to review, how long it may run, which tools it may use, and how to start its configured skill — instead of a vague instruction each reviewer had to interpret for itself.

Problems, as observed in real runs

1. Reviewers re-derived — and sometimes got wrong — what code to review. The composed prompt only said "review the commits unique to this branch vs origin/HEAD." Each reviewer then ran its own git commands to work out what that meant. In one live run, codex diffed in the wrong direction on a branch that was behind main and reported main's newer code as regressions the branch had introduced — 2 of the 4 high-severity findings in that run's final verdict were such phantoms, and the judge consolidated them without any way to notice.
2. Reviewers' tool calls were silently denied. Headless claude -p auto-denies any tool call that isn't pre-approved, and the review command passed no permission config. Reviewers bounced off denials for git diff / git log, then either worked around them slowly or skipped verifying their findings.
3. The configured skill never actually ran (claude). The composed prompt started with the skill line (/pr-review-toolkit:review-pr). claude -p treats a leading slash as a slash command and expanded it — resolving to the built-in /review skill with the entire composed prompt stuffed in as its argument (it interpolated the skill name where a GitHub PR number belongs). Every claude review was silently running the wrong workflow.
4. Setup wrote the built-in review brief into your settings file. Guided setup and first-run persisted the default task text into review_profiles.<name>.task as if you had typed it — indistinguishable from user config, and frozen: improving the default wording could never reach existing profiles.

Changes (numbered to match the problems)

1. The exact scope is now in the composed prompt. The review command already computes the commit list, the changed-file list (three-dot, vs merge-base), and the uncommitted files for its banner line — it now passes all of that to every reviewer, marked "authoritative — use as-is, do not re-derive", plus the diff itself when it fits a size budget (otherwise the exact git diff base...HEAD command to run). The judge additionally receives the changed-file list with an instruction to discard findings that point outside it — so even a reviewer that somehow mis-scopes can't pollute the final verdict.
2. Read-only tools are pre-approved. Reviewers launch with --allowedTools: Read/Grep/Glob/Task/Skill plus enumerated read-only git subcommands (diff, log, show, status, blame, rev-parse, merge-base, ls-files). Deliberately not Bash(git:*) — git aliases/hooks execute arbitrary code and push/commit would match the blanket — and never --dangerously-skip-permissions: reviewers process untrusted diffs and must stay unable to modify the repo.
3. The configured skill actually runs. When the composed prompt would start with /, a prose preamble is prepended — "Lines starting with «/» name configured review skills: invoke each with the Skill tool and follow it" — making slash-expansion structurally impossible. Verified in live transcripts: real Skill tool invocations of the configured skill, followed by that skill's actual workflow.
4. The built-in review brief is a runtime default, not saved config. Reviewer behavior is unchanged from main: every reviewer still receives the profile's task, or the built-in default when none is set (Multi-agent review profiles for entire review #1312's deliberate design — a measured A/B without the brief was faster but missed a real finding). The only change is that setup persists an empty task, so saved profiles stay clean and pick up improved default wording. Pinned by TestRunReview_TaskSemantics.

What a reviewer receives now (abridged real capture)

Perform the review below. Lines starting with "/" name configured review skills: invoke each with the Skill tool and follow it.

/review

Review profile: general

Task: Review this change for correctness, regressions, API design, missing tests, … [profile task, or built-in default]
[output-format instructions]

Scope: review the commits unique to this branch vs main, plus any uncommitted changes in the working tree. …

Authoritative scope, computed by entire — use it as-is, do not re-derive it:

Commits under review (oldest first):
3ae0a76 add feature

Files under review (vs merge-base with main):
A	feature.go

Uncommitted working-tree changes:
?? notes.txt

Only the files listed above are in scope. Findings that point anywhere else are out of scope — discard them.

Diff under review:
```diff
diff --git a/feature.go b/feature.go
…
```

Checkpoint context from commits in scope:
- a1b2c3d4e5f6: summary: add feature X handler; wire config loading; open: cover error path
- ... 3 more checkpoints omitted

Use `entire explain <id>` for full checkpoint context, or `entire explain <id> --raw-transcript` for raw transcripts.

In-progress session context (uncommitted):
  95131576 claude-code (touched: 4 files) prompt: add retry handling to the dispatcher

(The checkpoint/session sections come from entire's own history for exactly the commits and working tree under review — the intent behind each commit and what any still-uncommitted agent session was doing. They're empty in fresh repos, which is why the raw capture above was taken with them elided; predates this PR, included here for completeness of what a reviewer receives.)

Plus env (ENTIRE_REVIEW_* session-tagging handshake, unchanged) and the --allowedTools flag from change 2.

Deadline handling is deliberately NOT part of this PR — it landed as #1664: the default reviewer timeout is dropped entirely (reviewers run until done, --timeout stays as an explicit cap). An activity-based stall detector was investigated and ruled out — legitimate review subagents run 12+ minutes with zero parent output, so no liveness signal distinguishes quiet work from a hang.

Measured (same branch: 13 commits, 37 files, ~102KB diff; real claude runs)

	before	after
skill executed	built-in /review (by accident)	the configured skills, via real Skill invocations
phantom findings	2 of 4 highs in one run	none
tool denials	present	none

Testing

• TDD throughout; every behavior change had a failing test first
• mise run test, test:integration (390), test:e2e:canary — green
• Shim smoke: a fake claude on PATH captures argv/env through a real entire review run; asserts prompt contents, allowlist, and flag shape end-to-end
• Multiple real-agent runs on a live branch verifying skill routing and scope accuracy, plus a dogfood run of the command against this PR itself (its findings became commits here)

🤖 Generated with Claude Code

---

Also folded in: skill fan-out (was #1651)

A worker configured with N skills joined them into one child's prompt: the skills ran sequentially (or blended into one muddled pass), so selecting more skills grew the wait linearly. Measured live: a two-skill claude worker took ~9 minutes as a single child. The parallel machinery already existed one level up (worker slots fan out in run_multi); multi-skill selection just never used it.

• explodeSkillWorkers: at plan time, each multi-skill worker becomes one worker per skill (keys like claude-code:review, deduped against existing worker names; agent/model/prompt preserved). Wait becomes max(skills) instead of sum(skills). Two exploded workers also engage the judge, extending crew+judge consolidation to single-agent multi-skill profiles.
• --agent selects a filtered crew: all of that agent's workers run (previously: ambiguity error). Single-agent path only when exactly one worker matches.
• Session matching by skills: same-agent same-model exploded workers defeat the agent+model matcher — AgentRun gains Skills, and the matcher requires skill-set agreement when both sides carry skills (recorded from ENTIRE_REVIEW_SKILLS by the lifecycle hook). Adversarially-ordered test fails without the discriminator.
• TDD throughout; shim smoke: a two-skill profile spawns two one-skill children in parallel (~3s wall) plus one judge invocation.

Was stacked as #1651 (its diff was exactly the last commit here); combined into one PR for a single review/approval/merge.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit ac36d17. Configure here.}

[Copilot]

Pull request overview

This PR improves entire review orchestration by passing parent-computed review scope into spawned reviewer agents and the consolidating judge, adding explicit per-reviewer pacing guidance from the effective timeout, and pre-approving a constrained tool allowlist for headless Claude review children so read-only inspection commands aren’t auto-denied.

Changes:

• Add ScopeContext to RunConfig, build it from git (commits/files/uncommitted + optionally inline diff), and inject it into reviewer prompts.
• Thread authoritative scope into the judge/synthesis prompt so it can discard out-of-scope findings.
• Add reviewer time-budget hint rendering, and pass per-agent timeouts in the multi-agent path.
• Add --allowedTools allowlist to Claude review child invocation.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
cmd/entire/cli/review/types/reviewer.go	Introduces ScopeContext on RunConfig and defines scope payload shape/zero check.
cmd/entire/cli/review/synthesis_sink.go	Threads authoritative scope into synthesis prompt composition.
cmd/entire/cli/review/synthesis_prompt.go	Adds “scope gate” section to judge prompt to discard out-of-scope findings.
cmd/entire/cli/review/synthesis_prompt_test.go	Tests that synthesis prompt includes (or omits) the scope gate correctly.
cmd/entire/cli/review/scope.go	Builds bounded scope context (commits/files/uncommitted + inline diff budget).
cmd/entire/cli/review/scope_test.go	Adds coverage for scope context building, three-dot semantics, and caps.
cmd/entire/cli/review/prompt.go	Renders scope context + time budget into reviewer prompt; optionally inlines diff.
cmd/entire/cli/review/prompt_test.go	Tests scope injection and time budget hint behavior.
cmd/entire/cli/review/export_test.go	Updates/export helpers for new synthesis prompt signature and scoped testing.
cmd/entire/cli/review/cmd.go	Injects scope context into single/multi-agent configs; threads scope to sinks; sets per-agent timeout.
cmd/entire/cli/review/cmd_test.go	Extends tests to validate per-agent timeout threading and scope context injection.
cmd/entire/cli/agent/claudecode/reviewer.go	Adds --allowedTools allowlist to headless Claude review child.
cmd/entire/cli/agent/claudecode/reviewer_test.go	Tests that allowlist is present and does not enable unsafe flags.

[peyton-alt]

Relationship to #1312's task design (for reviewers wondering whether the no-injected-task commits undo something intentional):

The default-task machinery (profileTask, defaultGeneralTask, the Task: prompt section + output-format protocol) all came from #1312. This PR preserves three of its four purposes and reverses exactly one, deliberately:

#1312 intent	here
Judge receives a canonical task for consolidation	kept — still resolved and threaded to the synthesis prompt
Skill-less workers (gemini/pi, whose default prompt says "according to the profile task") need a brief	kept — workerTask falls back to the built-in task for skill-less workers only
User-configured tasks survive configure edits (#1312 follow-ups df29bfb/e0d56ac6d)	kept — guidedProfileTask honors custom + existing tasks; only the built-in text is no longer persisted
Skill-bearing workers get the uniform audit brief + output protocol	reversed — product decision: entire invokes configured skills and adds no prompts the user didn't write. Measured: the injected brief ordered a maximal ~15-minute audit regardless of what the user configured

Default task texts remain in the code as runtime fallbacks (and TestDefaultReviewTasksRejectSlop still validates them).

[peyton-alt]

Update to the #1312-relationship table above: after review discussion, the fourth row is now also kept — the branch no longer changes which workers receive the canonical task. Worker briefing is exactly main's behavior (every worker gets the user task or the built-in default; the A/B without the brief was ~35% faster but missed a real finding, so depth stays). The only task-related change in this PR is that setup persists an empty task and the built-in brief applies at runtime only — saved profiles stay clean and pick up improved default wording. Pinned by TestRunReview_TaskSemantics.