From 47d2eca74b0e992791da7da77ac68b21bd8059c3 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Mon, 17 Aug 2026 10:25:54 -0400 Subject: [PATCH 1/2] ci(deep-review): restore fork PR reviews after checkout v6.1.0 change actions/checkout v6.1.0 (2026-07-20) backported `allow-unsafe-pr-checkout` as an explicitly breaking change defaulting to false, which refuses to check out fork code from a `pull_request_target` workflow. This workflow tracks the moving `v6` tag, so the guardrail arrived with no change on our side and every fork-PR review has failed at "Checkout PR head" since -- 18 runs, 0 successes, across 6 fork PRs. Bumping to v7 is not an escape: v7.0.1 ships the same input with the same default. Set the flag on the PR-head checkout to restore reviews, and tighten the reviewer's credential surface while opting in: - `persist-credentials: false` keeps the base repo's token out of `.git/config` in the fork worktree. Nothing downstream needs git credentials: the base fetch uses a public HTTPS URL, and every `gh` call reads its token from the environment. - Drop `Bash(gh api:*)` from the reviewer allowlist. `--allowedTools` matches by command prefix and cannot constrain an HTTP method, so the grant was far broader than a read-only review needs. Only ce-previous-comments-reviewer reached for it, and that agent already prefers `gh pr view --json reviews,comments`; the sole loss is inline-comment path/line metadata. The plugin checkout and all same-repo PRs were never affected -- the guard only fires when the checkout target is the PR head repo. --- .github/workflows/deep-review.yml | 38 ++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/.github/workflows/deep-review.yml b/.github/workflows/deep-review.yml index 2185f3caa4..da730c9943 100644 --- a/.github/workflows/deep-review.yml +++ b/.github/workflows/deep-review.yml @@ -123,12 +123,37 @@ jobs: # Check out the PR head so reviewer sub-agents can read the actual code. # Works for both same-repo and forked PRs. + # + # `allow-unsafe-pr-checkout` is REQUIRED for the fork case. checkout + # v6.1.0 (2026-07-20) backported it as an explicitly BREAKING change + # defaulting to false, so checking out fork code from a + # `pull_request_target` workflow now hard-fails. Because we track the + # moving `v6` tag, that arrived with no change on our side -- and every + # fork-PR review failed at this step until this flag was set. Bumping to + # v7 is not an escape: v7.0.1 ships the same input with the same default. + # https://github.blog/changelog/2026-06-18-safer-pull_request_target-defaults-for-github-actions-checkout/ + # + # The flag's warning is about executing fork code in a credentialed + # context. This job never executes it: no `yarn install`, no build, no + # Edit/Write tools, `contents: read`, and `--setting-sources user` below + # so the fork's `.claude/settings.json` cannot widen permissions. Note + # the checkout widens the prompt-injection surface from "the diff" to + # "all files" but does not create it -- `Bash(gh pr diff:*)` already + # feeds untrusted fork text to the model with no checkout at all. - name: Checkout PR head uses: actions/checkout@v6 with: repository: ${{ steps.pr.outputs.head_repo }} ref: ${{ steps.pr.outputs.head_ref }} fetch-depth: 0 + allow-unsafe-pr-checkout: true + # Keep the base repo's GITHUB_TOKEN out of `.git/config` inside an + # untrusted worktree, where the reviewer's `Bash(git:*)` grant could + # read it back via `git config --get http..extraheader`. + # Nothing downstream needs git credentials: the base fetch below + # uses a public HTTPS URL, and every `gh` call takes its token from + # the environment rather than from git config. + persist-credentials: false # Make the PR base SHA reachable AND fetch enough history that # `git merge-base HEAD ` succeeds. The ce-code-review skill @@ -489,9 +514,20 @@ jobs: 3. Do NOT post the review yourself with `gh` or any comment tool -- the workflow posts the structured output as a sticky comment. + # Deliberately no `Bash(gh api:*)` in the allowlist below. + # --allowedTools matches by command prefix and cannot constrain an + # HTTP method, so granting `gh api` grants POST/PATCH/DELETE against a + # token holding `pull-requests: write` + `issues: write` -- a + # repo-write channel driven by a model reading untrusted fork code. + # In the pinned plugin only ce-previous-comments-reviewer reaches for + # it, and that agent already prefers + # `gh pr view --json reviews,comments` (still allowed via + # `Bash(gh pr view:*)`); the sole loss is inline-comment path/line + # metadata. Comments must stay OUT of the block scalar below -- every + # line in it is passed to the CLI verbatim as an argument. claude_args: | --setting-sources user - --allowedTools "Bash(git:*),Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr list:*),Bash(gh issue view:*),Bash(gh issue list:*),Bash(gh search:*),Bash(gh api:*)" + --allowedTools "Bash(git:*),Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr list:*),Bash(gh issue view:*),Bash(gh issue list:*),Bash(gh search:*)" --json-schema '{"type":"object","properties":{"review":{"type":"string","description":"Complete markdown review starting with on the first line and ## Deep Review on the second line"}},"required":["review"]}' # --- Sandbox smoke check ---------------------------------------------- From f4a146ef0f70ae04d83d0bc60a063970dc3159c1 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Mon, 17 Aug 2026 12:44:58 -0400 Subject: [PATCH 2/2] ci(deep-review): keep the reviewer's git grant read-only Follow-up hardening on the fork-checkout change in this PR. The `persist-credentials: false` on the PR-head checkout was only half the job: the plugin checkout still persisted the base repo's GITHUB_TOKEN into `ce-plugin/.git/config`, a subdirectory of the very worktree that now holds fork content, where the reviewer's `Bash(git:*)` grant could read it back. Nothing there needs credentials -- the plugin is consumed only as a local marketplace path and the repo is public -- so set the flag there too. Two further steps so the same shape cannot come back: - Assert, immediately before the reviewer runs, that no `.git/config` under the workspace retains an auth header. A checkout added later without the flag now fails loud instead of leaking quietly. - Grant git per read-only subcommand instead of `Bash(git:*)`. The wildcard was both a credential-read path (`git config --file ...`) and a network egress path (`git fetch` / `ls-remote` / `archive --remote`) available to a model reading untrusted fork code. Verified against the pinned plugin ref that this costs nothing: 20 of the 21 catalog personas invoke git zero times (the orchestrator hands them the resolved diff), and the `base:` fast path this workflow drives needs only merge-base/diff/ls-files/rev-parse/log. `--disallowedTools` backs the allowlist up, since `--allowedTools` only pre-approves. --- .github/workflows/deep-review.yml | 75 +++++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 3 deletions(-) diff --git a/.github/workflows/deep-review.yml b/.github/workflows/deep-review.yml index da730c9943..c75dbab01b 100644 --- a/.github/workflows/deep-review.yml +++ b/.github/workflows/deep-review.yml @@ -285,6 +285,15 @@ jobs: repository: EveryInc/compound-engineering-plugin ref: ${{ env.PLUGIN_REF }} path: ce-plugin + # Same reason as the PR-head checkout above, and it matters even + # though this repo is not the untrusted one: `ce-plugin` is a + # subdirectory of the very worktree holding fork code, so a + # credential left in `ce-plugin/.git/config` is reachable from the + # same reviewer that reads that code. Nothing here needs it -- the + # plugin is consumed only as a local marketplace path + # (`plugin_marketplaces: ./ce-plugin`) and the repo is public, so + # even a re-fetch would not need auth. + persist-credentials: false # --- CLI pin (TEMPORARY) ---------------------------------------------- # claude-code-action hardcodes the Claude Code CLI version it installs @@ -344,6 +353,28 @@ jobs: fi echo "PINNED_CLAUDE=$CLAUDE_BIN" >> "$GITHUB_ENV" + # Backstop for the two `persist-credentials: false` settings above, and + # the last step before the reviewer runs -- so it covers every worktree + # any earlier step created, not just the two checkouts we know about. + # The reviewer's git grant reads from an untrusted worktree, so a + # checkout added later without the flag (or an upstream default that + # flips back) must fail loud here rather than leak quietly. + - name: Assert no persisted git credentials in the workspace + if: steps.gate.outputs.should_review == 'true' + run: | + set -euo pipefail + # `-name config -path '*/.git/*'` also catches submodule configs at + # `.git/modules//config`. `|| true` is required: `grep -l` + # exits 1 on no match, which pipefail would turn into a job failure + # on the healthy path. + LEAKED=$(find "$GITHUB_WORKSPACE" -type f -name config -path '*/.git/*' -print0 \ + | xargs -0 -r grep -lEi 'extraheader|sshCommand' || true) + if [ -n "$LEAKED" ]; then + echo "::error::Persisted git credentials found in: $(echo "$LEAKED" | tr '\n' ' '). Add 'persist-credentials: false' to the checkout that created it -- the reviewer's git grant can read it back out of an untrusted worktree." + exit 1 + fi + echo "No persisted git credentials under $GITHUB_WORKSPACE." + - name: Run deep review id: review if: steps.gate.outputs.should_review == 'true' @@ -523,11 +554,49 @@ jobs: # it, and that agent already prefers # `gh pr view --json reviews,comments` (still allowed via # `Bash(gh pr view:*)`); the sole loss is inline-comment path/line - # metadata. Comments must stay OUT of the block scalar below -- every - # line in it is passed to the CLI verbatim as an argument. + # metadata. + # + # git is granted per read-only subcommand rather than as `Bash(git:*)` + # for the same reason: a wildcard git grant is a credential-read + # channel (`git config --file /.git/config --list`) and a network + # egress channel (`git fetch`/`ls-remote`/`archive --remote` to an + # attacker URL), both driven by a model reading untrusted fork code. + # The allowlist is the primary control: a denylist cannot enumerate + # every subcommand that reaches the network or a credential + # (`git archive --remote`, `git credential fill`, `git submodule + # update --remote`), whereas an allowlist has no such tail. The + # `--disallowedTools` line is belt-and-suspenders, because + # --allowedTools only PRE-APPROVES -- an un-approved call fails here + # only because a `-p` run cannot answer the prompt -- while + # --disallowedTools is a real deny. + # + # Verified against the pinned plugin ref. Of the 21 personas in + # ce-code-review/references/persona-catalog.md, 20 invoke git zero + # times; references/diff-scope.md says why -- "The scope step in the + # SKILL.md handles discovery and passes you the resolved diff." The + # exception, ce-schema-drift-detector, is Rails-only (spawned solely + # for `db/migrate/*.rb` / `db/schema.rb`, which do not exist in this + # repo) and its reads are plain `git diff`; its one write, + # `git checkout -- db/schema.rb`, is a working-tree mutation + # `mode:report-only` forbids regardless. + # + # The orchestrator's own surface is the `base:` fast path + # (SKILL.md "Skip all base-branch detection, remote resolution, and + # merge-base computation"), which needs only + # merge-base/diff/ls-files/rev-parse/log. The skill's `git fetch` + # fallbacks live in scripts/resolve-base.sh, which needs + # `Bash(bash:*)` -- never granted -- and which `base:` skips anyway; + # `git checkout` / `gh pr checkout` are already blocked by + # `mode:report-only`. `git -c diff` cannot slip past the + # allowlist either: `-c` precedes the subcommand, so the command + # string starts `git -c` and matches no prefix. + # + # Comments must stay OUT of the block scalar below -- every line in it + # is passed to the CLI verbatim as an argument. claude_args: | --setting-sources user - --allowedTools "Bash(git:*),Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr list:*),Bash(gh issue view:*),Bash(gh issue list:*),Bash(gh search:*)" + --allowedTools "Bash(git diff:*),Bash(git log:*),Bash(git show:*),Bash(git blame:*),Bash(git merge-base:*),Bash(git rev-parse:*),Bash(git ls-files:*),Bash(git cat-file:*),Bash(git status:*),Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr list:*),Bash(gh issue view:*),Bash(gh issue list:*),Bash(gh search:*)" + --disallowedTools "Bash(git config:*),Bash(git credential:*),Bash(git fetch:*),Bash(git pull:*),Bash(git clone:*),Bash(git ls-remote:*),Bash(git remote:*),Bash(git push:*),Bash(git archive:*),Bash(git submodule:*)" --json-schema '{"type":"object","properties":{"review":{"type":"string","description":"Complete markdown review starting with on the first line and ## Deep Review on the second line"}},"required":["review"]}' # --- Sandbox smoke check ----------------------------------------------