Uh oh!
There was an error while loading. Please reload this page.
ci(deep-review): restore fork PR reviews and tighten reviewer confinement - #2955
Conversation
actions/checkout@v6 now refuses to check out fork PR code from a pull_request_target workflow, so every fork PR has failed at the checkout step since 18 Aug and external contributors get no deep review. Opt in with allow-unsafe-pr-checkout, which is defensible only because nothing in this job executes the fork's code: no install, no build, no tests. Close the gaps that guard is warning about: - persist-credentials: false on both checkouts. The default writes a pull-requests: write token into .git/config, and the action's env scrub covers subprocess environment, not files on disk. - --strict-mcp-config. .mcp.json, .claude/agents and .claude/skills are all tracked, so a PR can ship config for the reviewer itself. --setting-sources user already drops project skills and agents (verified against CLI 2.1.215 with a canary config); .mcp.json is not covered by it. - Drop Bash(gh api:*), which accepts --method POST and cannot be narrowed to GET by a prefix pattern. Prior review threads are fetched in trusted shell to .deep-review/prior-comments.md instead, so ce-previous-comments-reviewer does not silently leave the roster. - Enumerate git subcommands rather than Bash(git:*), which permits git config diff.external plus git diff as arbitrary execution. Neutralize by configuration rather than by removing the files: the skill computes the reviewed diff with git diff $BASE against the working tree, so touching the tree would forge deletions into the diff the reviewers see, and hide real changes to those paths.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
🟢 Tier 1 — TrivialDocs, images, lock files, a dependency bump, or an automated release. No functional code changes detected. Why this tier:
Review process: Auto-merge once CI passes. No human review required. Stats
|
Greptile SummaryThis PR restores fork deep reviews by opting into fork-head checkout while removing persisted checkout credentials and tightening reviewer configuration and command access.
Confidence Score: 4/5The prior-comment handoff should be fixed before merging because the confined reviewer cannot read the file that replaces its removed GitHub API access. The workflow writes prior feedback to an ignored untracked file while granting only enumerated Git and GitHub CLI commands, causing the previous-comments reviewer to lose the context this change is intended to preserve; the new mutable action reference also leaves a non-blocking supply-chain concern. Files Needing Attention: .github/workflows/deep-review.yml
|
| Filename | Overview |
|---|---|
| .github/workflows/deep-review.yml | Restores fork checkout and tightens reviewer confinement, but the prior-comments file is inaccessible to the confined reviewer and the new privileged action uses a mutable tag. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Event[pull_request_target] --> Checkout[Checkout fork PR head]
Checkout --> Gate{Review required?}
Gate -->|Yes| Fetch[Materialize prior comments]
Fetch --> File[.deep-review/prior-comments.md]
File -. blocked by tool allowlist .-> Reviewer[Previous-comments reviewer]
Reviewer --> Review[Structured deep review]
Reviews (1): Last reviewed commit: "ci(deep-review): restore fork PR reviews..." | Re-trigger Greptile
| --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:*)" | ||
| --strict-mcp-config | ||
| --allowedTools "Bash(git diff:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*),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:*)" |
There was a problem hiding this comment.
When a PR has prior feedback, the workflow writes it to an ignored untracked file but grants the reviewer only the enumerated Git and GitHub CLI commands. Neither Git nor an allowed file-reading command can read that file, causing the previous-comments reviewer to omit the materialized feedback.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| - name: Materialize prior review comments | ||
| id: prior | ||
| if: steps.gate.outputs.should_review == 'true' | ||
| uses: actions/github-script@v9 |
There was a problem hiding this comment.
The new actions/github-script@v9 invocation runs in a pull_request_target job with pull-request and issue write permissions, so moving or compromising the mutable tag would allow unauthorized mutation of reviews, comments, or issues. Pinning this invocation to a full commit SHA would make changes to the privileged action auditable. How this was verified: The new action receives GITHUB_TOKEN in a job that explicitly grants pull-requests: write and issues: write.
E2E Test Results✅ All tests passed • 307 passed • 1 skipped • 1149s
Tests ran across 4 shards in parallel. |
Deep ReviewA single-file change to 🔴 P0/P1 -- must fix
🟡 P2 -- recommended
🔵 P3 nitpicks (5)
Reviewers (6): correctness, security, reliability, adversarial, maintainability, project-standards. Testing gaps:
|
Uh oh!
There was an error while loading. Please reload this page.
Summary
Fork PRs have had no deep review since 18 Aug.
actions/checkout@v6added a guard that refuses to check out fork PR code from apull_request_targetworkflow, so the job fails atCheckout PR headon every external contribution. We track the floating@v6tag, so this arrived with no change on our side. Same-repo PRs were unaffected, and the job fails loudly rather than skipping quietly.This opts in with
allow-unsafe-pr-checkout, which is defensible here only because the job never executes the fork's code: there is no install, no build and no test run, so the reviewers only read the tree. The rest of the change tightens what the reviewer can reach, so the opt-in is a considered trade rather than a muted warning.What changed
allow-unsafe-pr-checkout: truepersist-credentials: falseon both checkouts.git/config. The action's env scrub covers subprocess environment, not files on disk. Nothing in this job pushes.--strict-mcp-config.mcp.json,.claude/agentsand.claude/skillsare all tracked, so a PR can ship config for the reviewer itself.--setting-sources useralready covers project skills and agents (verified against CLI 2.1.215 with a canary config);.mcp.jsonis not covered by it.gh api, which takes a write method and cannot be constrained by a prefix pattern, and enumeratesgitby subcommand instead ofgit:*.Materialize prior review commentsstep.deep-review/prior-comments.md, so droppinggh apidoes not silently cost us the previous-comments reviewer.Reviewer config is neutralized with flags rather than by removing files. The review skill computes its diff with
git diff $BASEagainst the working tree, so editing the tree first would forge deletions into the diff reviewers see, and would hide a real change if a PR legitimately touches one of those paths.Not fixed here
compound-engineering-v3.6.1) while the CLI beside it is pinned by SHA-512.actions/checkout@v6is a floating tag too, which is what produced this outage.permission_denials_count: 19, and the action hides tool output, so I could not confirm which calls were already being denied. This change narrows the allowlist further. Worth comparing theReviewers (N):footer against a pre-change run on a comparable diff before relying on it for a fork PR.How to test on Vercel preview
N/A — non-UI change.
References