fix(review): scope reviewer configuration to trusted sources

[peyton-alt]

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

Summary

Automated review runs against code the reviewer did not write — and with --target, against a branch fetched from a remote. Claude's non-interactive mode treats its working directory as trusted, so the reviewer was taking execution-capable configuration from the checkout under review.

The reviewer now loads no Claude settings at all and starts no MCP servers. Entire's own lifecycle hooks are supplied through a settings file the CLI writes outside the worktree, so reviews are still captured; the profile's configured skills are supplied by staging them into a plugin directory the CLI owns, so the user's chosen skills still run.

Flag	Purpose
--setting-sources ""	No settings are read: project and local are branch-controlled, and a user-level hook would run with the reviewed checkout as its working directory
--strict-mcp-config (no --mcp-config)	No MCP servers start; setting sources do not gate these
--permission-mode default	Pinned, so a future default cannot widen the reviewer
--settings <file>	Entire's lifecycle hooks, written outside the worktree, mode 0600
--plugin-dir <dir>	Only the profile's configured skills, copied into a directory Entire owns
--append-system-prompt	Trust-boundary instruction, appended so a profile prompt cannot displace it

Why user settings are excluded too

An earlier revision of this branch kept --setting-sources user to preserve user- and plugin-provided review skills. That reintroduced an execution path: a user-level hook running npm run …, make …, or any checkout-relative script executes code from the branch under review, because that branch is the reviewer's working directory. The trail security monitor flagged it, correctly.

The trusted settings file also no longer carries the user's apiKeyHelper. Claude runs that helper as a shell command with the reviewed checkout as its working directory, so a relative helper executes branch content before the first request; the generation path re-injects it safely only because it runs in os.TempDir(), which review cannot. Reviewers authenticate with ANTHROPIC_API_KEY or a Claude sign-in, and Entire prints a note before launch when a helper is configured and no env key is set.

Skill staging (review_skills.go) resolves the tension without that trade. Each configured skill is copied from the user's own configuration into a temporary plugin directory, loaded with --plugin-dir, and its invocation rewritten to /entire-review:<name>. Only the chosen skill is copied — never a whole plugin, which can define hooks of its own. Curated builtins such as /review need no staging. A skill that cannot be staged fails the review before launch, because a reviewer started without its skill prints Unknown command and reviews nothing while still exiting 0.

Also in here

• One hook inventory. entireHookSpecs() is shared by the project installer and the review launch, so they cannot drift.
• Fail before launch. If the trusted settings or a configured skill cannot be established, the review fails. There is no degraded fallback.
• An architectural test requires every claude launch site in the package to record whether it is isolated.
• PrepareCmd on ReviewerTemplate: a launch precondition that can fail and owns resources, released once the process exits or immediately if Start fails.

Compatibility

• Project-level MCP servers and project-level Claude settings do not apply to reviewers.
• User-level hooks, permission modes and MCP servers do not apply to reviewers either.
• The user's apiKeyHelper is not run for reviews; use ANTHROPIC_API_KEY or a Claude sign-in.
• Review profiles keep working: builtin, user-level and plugin-provided skills all resolve through staging.

This is configuration isolation, not an OS sandbox: the reviewer runs with the invoking account's privileges.

Verification

Full mise run check clean. Differential runs against a real Claude with a loopback mock API and synthetic credentials, each comparing an unpatched build with the fix so the fixture is proven to fire before the fix is credited with blocking it. Five canaries: a committed settings hook, a committed settings.local.json hook, a committed .mcp.json server, a user-level hook that runs a checkout-relative script, and a relative apiKeyHelper with the branch supplying the script — the last two added because the earlier harness only had branch-controlled canaries and could not see either path.

Platform	Claude	Result
Windows	2.1.237, 2.1.267	pass — all four canaries fire unpatched, all four blocked by the fix, capture intact
Linux	2.1.237, 2.1.267	pass — same
macOS	2.1.237, 2.1.268	pass — same; the user-sources revision runs the fourth canary here, reproducing the monitor's finding

Matrix run (private validation repo): https://github.com/peyton-alt/cli-review-windows-validation/actions/runs/34567618641

Covers current-branch and remote-only --target with --cleanup-worktree, multi-worker profiles, builtin / user-level / plugin-provided skills, and session/transcript capture throughout. Not covered: live-service authentication.

Review notes

Separately, and pre-existing: a review whose agent makes zero model requests still reports 1 succeeded and exits 0. It warns that findings were not persisted. Worth filing on its own.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Unresolved critical path-isolation findings and additional moderate correctness gaps block approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This pull request hardens Claude review launches against checkout-controlled configuration while preserving session and transcript capture.

Changes:

• Adds isolated launch flags, trusted temporary settings, and fail-closed preparation.
• Centralizes hook specifications and adds cleanup lifecycle support.
• Adds launch guards, tests, and security documentation.

File summaries

File	Reviewed change and findings
docs/security-and-privacy.md	Documents isolation; nit (2 votes): clarify the apiKeyHelper exception in text-generation settings.
docs/architecture/review-command.md	Documents reviewer isolation and trust boundaries.
cmd/entire/cli/review/types/template.go	Adds launch preparation and cleanup handling.
cmd/entire/cli/review/types/template_test.go	Tests preparation failure and cleanup behavior.
cmd/entire/cli/agent/claudecode/reviewer.go	Integrates trusted settings preparation into reviewer launches.
cmd/entire/cli/agent/claudecode/reviewer_test.go	Updates reviewer argument tests.
cmd/entire/cli/agent/claudecode/review_launch.go	Builds isolated settings; critical findings concern checkout-controlled apiKeyHelper paths (2 votes) and relative TMPDIR placing settings in the worktree (1 vote). A moderate finding (1 vote) concerns PATH-dependent hooks silently allowing unrecorded launches.
cmd/entire/cli/agent/claudecode/review_launch_test.go	Tests isolation flags and trusted settings.
cmd/entire/cli/agent/claudecode/review_launch_guard_test.go	Adds launch-policy guards; moderate findings concern missed GenerateText aliases (1 vote) and incomplete site discovery/function-name collisions (2 votes).
cmd/entire/cli/agent/claudecode/hooks.go	Shares hook specifications; moderate findings concern installer drift (1 vote) and empty hook commands being installed silently (2 votes).

Review details

Suppressed comments (3)

cmd/entire/cli/agent/claudecode/hooks.go:260

• The installer still hard-codes the three non-empty matcher lookups here, while entireHookSpecs() is only used generically for empty-match hooks. Adding or changing a matcher in the advertised single inventory will update the review settings but not entire enable, so the claimed no-drift guarantee is incomplete and can leave reviews captured differently from normal sessions. Derive installation from every spec or validate that every inventory entry is handled.

	preTaskCmd := specCommand(specs, "PreToolUse", subagentToolMatcher)
	postTaskCmd := specCommand(specs, "PostToolUse", subagentToolMatcher)
	postTodoCmd := specCommand(specs, "PostToolUse", taskToolMatcher)

cmd/entire/cli/agent/claudecode/review_launch.go:118

• These commands are validated only for being non-empty, but every WrapProduction... wrapper intentionally exits successfully when entire is not on PATH. A review launched via an absolute binary path (or from a GUI/IDE whose PATH omits the CLI) therefore passes PrepareCmd, runs Claude, and silently records no session or transcript—the exact degraded, unrecorded launch this validation is meant to reject. Build the review hooks with the current executable's absolute path (using a platform-safe wrapper), or verify that the hook command is actually runnable and fail preparation when it is not.

	for _, spec := range entireHookSpecs() {
		hooks[spec.hookType] = addHookToMatcher(hooks[spec.hookType], spec.matcher, spec.command)

cmd/entire/cli/agent/claudecode/review_launch_guard_test.go:91

• This AST scan only recognizes "claude" when it is a direct string literal in a call argument, so it misses the existing GenerateText launch: generate.go:152 assigns claudePath := "claude" and passes that variable to commandRunner at line 175. As a result, GenerateText has no claudeLaunchPolicy entry and a future change could remove its isolation without this guard failing. Resolve the binary aliases (or otherwise enumerate launch sites) and record this site too.

			case *ast.CallExpr:
				for _, arg := range node.Args {
					lit, ok := arg.(*ast.BasicLit)
					if !ok || lit.Kind != token.STRING {
						continue
					}
					// Unquote so `"claude"` matches but `"claude-code"` does not.
					if v, err := strconv.Unquote(lit.Value); err == nil && v == claudeBinaryName && enclosing != "" {
						found[enclosing] = name + ":" + strconv.Itoa(fset.Position(lit.Pos()).Line)

• Files reviewed: 10/10 changed files
• Comments generated: 5
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.