Skip to content

fix(review): exclude project settings from branch-review worktrees - #118

Open
seungpyoson wants to merge 6 commits into
sendbird:mainfrom
seungpyoson:fix/117-review-children-disable-hooks
Open

seungpyoson wants to merge 6 commits into
sendbird:mainfrom
seungpyoson:fix/117-review-children-disable-hooks

Conversation

@seungpyoson

@seungpyoson seungpyoson commented Sep 28, 2026 •

Copy link
Copy Markdown

Fixes #117 for branch reviews. The protection has limits, listed under "What this does not cover".

Problem

For a branch review, meaning --base, --scope branch, or auto scope on a clean working tree, review and adversarial review run claude -p inside a new worktree the plugin creates (scripts/lib/review-worktree.mjs). The worktree checks out the commit under review, and a print-mode child doesn't ask for trust. Nothing limited setting sources, so the checkout's project settings loaded. Tested at 19e5651 with the real CLI:

  • .claude/settings.json hooks run at startup;
  • its env reaches the processes the child starts. The review's git MCP server is node, so env: {"NODE_OPTIONS": "--require ./.claude/probe.cjs"} runs a file committed in the branch;
  • CLAUDE.md and .claude/rules reach the model. So does AGENTS.md, which Claude Code 2.1.277 and later reads when a project has no CLAUDE.md.

Why --setting-sources, not disableAllHooks

#117 first suggested disableAllHooks. That stops hooks but not the other two paths: with hooks disabled, the NODE_OPTIONS file above still runs, and CLAUDE.md still loads. --setting-sources user stops project and local settings from loading at all, which covers all three.

An external review also found that on Claude Code 2.0.64 and 2.1.150, a checkout's permissions.allow: ["Bash"] let a worktree review run Bash under dontAsk, and that --setting-sources user blocked it on every version tested. Those versions are now refused (see below).

Change

  • createReviewIsolation returns settingSources: "user" for a review worktree. Review and adversarial review pass it through.

  • buildArgs gains an optional settingSources, emitted as --setting-sources <value>.

  • In the worktree, user settings, managed settings and the --settings file still load; project and local settings do not.

  • Claude Code 2.1.281 or later is required whenever settingSources is set. Older versions accept the flag without applying it everywhere. The Claude Code changelog records three fixes:

    • 2.1.211: nested .claude/rules files no longer load (before it, one loaded after Claude read a file in its directory);
    • 2.1.246: the command sandbox's filesystem configuration respects the flag;
    • 2.1.281: spawned sessions (teammates, /bg, claude agents sessions) inherit it.

    2.1.281 is the latest release that fixed the flag letting excluded settings through. runClaudeTurn reads claude --version and throws before starting Claude if the output doesn't start with a plain major.minor.patch at or above 2.1.281. A prerelease suffix counts as unreadable. An older release gets an error telling the user to update; output that can't be read as a release version gets an error saying so. The README prerequisites name the version.

  • 2.1.281 is above npm's stable tag (2.1.274 today; latest is 2.1.283). Users on the stable channel can't run branch reviews until stable reaches 2.1.281, and "update Claude Code" won't help them unless they switch channels. If that's too strict, 2.1.246 is the alternative. The only gap between the two is spawned sessions. A review child can reach those only if the tools that start them are allowed. The review allowlist doesn't include them, so user or managed settings would have to allow them. This comes from reading the allowlist; spawned sessions weren't tested.

  • Working-tree reviews, the stop-review gate, task and rescue are unchanged. They don't pass the flag, so the version check doesn't apply to them.

What this does not cover (for maintainers to decide)

  • Working-tree reviews load the user's repository's project settings without a trust prompt, as they do today. Auto scope picks a working-tree review whenever the tree has any change, including an untracked file. With an untrusted branch checked out and any local change, a review runs that branch's hooks and env (including code through NODE_OPTIONS) and loads its CLAUDE.md.
  • The stop-review gate has the same exposure and doesn't need a local change. Once enabled, it runs whenever a turn changes the working-tree fingerprint, and the fingerprint includes the index (git write-tree, scripts/lib/git.mjs:115; hooks/stop-review-gate-hook.mjs:309-313). A turn that only checks out a branch with different content changes the index, so asking Codex to check out a PR is enough to run that PR's settings, unattended.
  • Passing settingSources: "user" to the gate or to working-tree reviews would close this, but those runs would then lose the project's own CLAUDE.md and deny rules. Whether to do that is a product decision, so this PR leaves it alone. The gate is the stronger candidate because it runs without being asked.
  • Rescue loads workspace project settings, as today: hooks, env, .mcp.json servers (it doesn't pass --strict-mcp-config), and CLAUDE.md. --write rescue behaves the same.

Behaviour change

  • Branch reviews no longer load the checkout's CLAUDE.md, AGENTS.md, .claude/rules or project settings. Repositories used with Codex often have only AGENTS.md; their branch reviews lose those project instructions. That includes all project deny rules: rules on paths outside the repository and on tools such as WebFetch stop applying as well, and a worktree does not confine reads.
  • Deny rules, allow rules and instructions in user settings still apply. An external review confirmed that a user permissions.allow rule for Bash still lets a review run Bash under dontAsk. Users who rely on project deny rules for reviews should move them to user settings.
  • Branch reviews fail on Claude Code older than 2.1.281, with a message to update.
  • This needs a CHANGELOG line at release.

Verification

The full companion (node scripts/claude-companion.mjs review --cwd <repo> ...) was run with real Claude CLIs against a local mock API, with a fresh CLAUDE_CONFIG_DIR and CODEX_HOME. The test repo commits .claude/settings.json, .claude/probe.cjs and CLAUDE.md on main, then a code change on feature, so none of them is in the reviewed diff. .claude/settings.json contains a SessionStart hook and the NODE_OPTIONS env above.

Ref, CLI, target Result Hook ran NODE_OPTIONS code ran CLAUDE.md reached model
main, 2.1.283, --base main completed yes yes yes
main, 2.1.283, --base main, user disableAllHooks completed no yes yes
this branch, 2.1.283, --base main completed no no no
this branch, 2.1.283, working tree completed yes yes yes (unchanged)
this branch, 2.1.281, --base main completed no no no
this branch, 2.1.211, --base main refused: update to 2.1.281 no no no
this branch, 2.1.210, --base main refused: update to 2.1.281 no no no
this branch, 2.1.210, working tree completed yes yes yes (unchanged)

The same run with AGENTS.md in place of CLAUDE.md on 2.1.283: AGENTS.md reached the model on main with --base main and in this branch's working-tree review, but not in this branch's --base main review.

Checks at head b937125:

  • check:version-sync, check:changelog, lint, typecheck pass.
  • test 518/518, test:integration 47/47, test:e2e 23/23.
  • On a heavily loaded machine, some integration and e2e runs failed, including this PR's refusal test in an external review: the fake CLI's auth status exceeded the 10-second preflight timeout. Reruns pass.

New tests:

  • buildArgs emits the flag only when set.
  • assertSettingSourcesSupported:
    • accepts 2.1.281 and later;
    • rejects 2.1.280, 2.1.246, 2.1.210 and older;
    • rejects output that doesn't start with a plain release version, such as 2.1.283-rc.1, 2.1.283.0 or v2.1.283.
  • createReviewIsolation returns "user" for a worktree and nothing for the working tree.
  • The stop-gate argv carries no flag.
  • Review and adversarial review, across auto scope on a clean tree, --scope branch and --base main, pass user. Across auto scope on a dirty tree and --scope working-tree, they pass no flag. Task passes no flag.
  • On a fake 2.1.280 CLI, both review commands fail with the update message before starting Claude and leave no worktree behind. A working-tree review still runs on that CLI.

Pre-existing, noticed while testing

  • createSandboxSettings returns null for an unknown mode, and buildArgs then omits --settings.
  • The read-only preset comment says there is "no shell surface", which doesn't hold for rescue's Bash(git …:*) entries.
  • getClaudeAvailability reports any claude --version failure, including its 10-second timeout, as "claude CLI not found in PATH". The version check uses it, so a slow CLI on a loaded machine fails a branch review with that message.
  • $cc:setup shows the Claude Code version but doesn't flag one below 2.1.281, so users learn about the requirement from the README or from a refused branch review.
  • A CC_PLUGIN_CODEX_CLAUDE_BIN wrapper whose --version output doesn't start with the plain version now blocks branch reviews that worked before. Real claude --version output from 2.1.281 to 2.1.283 passes.

The branch has six commits. The first two tried disableAllHooks, then --setting-sources on every review. A squash merge gives the net diff.

seungpyoson and others added 4 commits September 28, 2026 14:55
Review and adversarial review start Claude with -p in a worktree of the
commit under review. Nothing limited setting sources, so that commit's
.claude/settings.json hooks ran at startup: shell commands outside the
Bash-free review allowlist, with no workspace trust prompt under -p.

Set disableAllHooks in the read-only preset, which review, adversarial
review, the stop-review gate and read-only rescue share. It applies only
to the spawned child and keeps project CLAUDE.md and permissions.

Fixes sendbird#117

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ZnqeTJxgWVEKtH4XefpUV
disableAllHooks closed one path from the code under review into its
reviewer and left the rest: with project settings loaded, the checkout's
`env` reached the processes the child starts (the git MCP server) and its
CLAUDE.md and rules reached the model, and a print-mode child never asks
whether to trust them. It also switched off the user's own hooks.

runClaudeReview now always passes --setting-sources user, after caller
options, so standard review, adversarial review and the stop-review gate
load no project or local settings. --settings and managed settings still
apply. Rescue keeps project settings. The read-only preset is unchanged
from main.

Fixes sendbird#117

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ZnqeTJxgWVEKtH4XefpUV
Forcing --setting-sources user on every review also dropped the user's
own project settings where reviews run in their repository: working-tree
reviews and the stop-review gate lost project deny rules (for example
Read(./.env), with Read in the review allowlist) and the repository's
CLAUDE.md.

The exposure in sendbird#117 is the review worktree: a checkout of the commit
under review that the user never trusted, in which a print-mode child
loads its settings without asking. createReviewIsolation now returns the
setting sources for the directory it chooses: "user" for a worktree, none
for the user's repository, where the child loads settings as any Claude
session there does. Review and adversarial review pass it through;
runClaudeReview no longer forces a value.

Fixes sendbird#117

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ZnqeTJxgWVEKtH4XefpUV
The comment called the review worktree a checkout the user never trusted.
It is a new directory, usually at the user's own HEAD, that Claude has
never been asked to trust. It also said a worktree run "loads user
settings only", although managed settings and the --settings file still
load, and it described the working-tree case as ordinary trust, although
that run skips the trust prompt because the plugin treats the user's
repository as trusted. The return-value description now says
settingSources is set only for a worktree.

Refs sendbird#117

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ZnqeTJxgWVEKtH4XefpUV
Claude Code before 2.1.211 accepts --setting-sources user but still loads
a nested .claude/rules file from the checkout once Claude reads a file in
that directory (Claude Code changelog, 2.1.211). A branch review on such
a CLI completed with the reviewed code's instructions in the model
context. runClaudeTurn now reads `claude --version` whenever
settingSources is set and throws before starting Claude if the version is
older than 2.1.211 or cannot be read. Working-tree reviews, the stop gate,
task and rescue do not pass the flag and are not checked.

The integration test now covers auto scope on a clean tree, auto scope on
a dirty tree and --scope branch as well as --scope working-tree and
--base, and a new test runs both review commands against a fake 2.1.210
CLI: they fail with the update message, Claude is never started and no
review worktree is left behind. The fake CLI reports 2.1.283 unless a
test sets CLAUDE_VERSION_OUTPUT.

The createReviewIsolation comment no longer says Claude was never asked
to trust the worktree, and no longer calls the Bash-free allowlist
containment for working-tree reviews: it limits tools, not what the
repository's project settings do.

Refs sendbird#117

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ZnqeTJxgWVEKtH4XefpUV
@seungpyoson seungpyoson changed the title fix(review): load user settings only in the review worktree fix(review): exclude project settings from branch-review worktrees Sep 28, 2026
@seungpyoson
seungpyoson marked this pull request as draft September 28, 2026 09:56
…rktree reviews

The 2.1.211 floor covered only the nested .claude/rules fix. The Claude
Code changelog records two later --setting-sources fixes: 2.1.246 made
the command sandbox's filesystem configuration respect the flag, and
2.1.281 made spawned sessions (teammates, /bg, claude agents sessions)
inherit it. The floor is now 2.1.281, the latest release that fixed the
flag letting excluded settings through.

The version check matched only a numeric prefix, so 2.1.283-rc.1 or
2.1.283.0 passed as 2.1.283. It now requires plain major.minor.patch
followed by whitespace or the end of the output, and rejects anything
else as unreadable.

README prerequisites now name the version branch reviews need.

Refs sendbird#117

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ZnqeTJxgWVEKtH4XefpUV
@seungpyoson
seungpyoson marked this pull request as ready for review September 28, 2026 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Branch reviews load the reviewed checkout's project settings: hooks, env and CLAUDE.md

1 participant