docs: add PR Review Fleet workflow README and design notes#27373
docs: add PR Review Fleet workflow README and design notes#27373tylerbutler wants to merge 12 commits into
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (158 lines, 2 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
Pull request overview
Adds documentation alongside the PR Review Fleet GitHub Actions workflows to explain how the fleet system is triggered, routed, and secured (including the pull_request vs pull_request_target rationale and the credential/prompt-isolation invariants).
Changes:
- Add a workflow reference README describing the four fleet workflows, their triggers/permissions, sizing thresholds, end-to-end flows, and security invariants.
- Add design notes explaining the security tradeoffs of
pull_requestvspull_request_targetas applied to this pipeline.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .github/workflows/pr-review-fleet.README.md | Reference documentation for the fleet review workflow system (triggers, permissions, flow, security invariants). |
| .github/workflows/pr-review-fleet.design-notes.md | Design/security notes for choosing pull_request vs pull_request_target in this pipeline. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
|
||
| - **Trigger:** `pull_request_target` on `opened` / `reopened` / `synchronize` (base branches `main`, `next`, `release/**/*`). | ||
| - **Permissions:** `contents: read`, `pull-requests: write`. | ||
| - **Context:** Runs from the **base branch** with a write token + secrets. Critical security invariant: never checks out the PR head; only diffs against it for stats (lines / files / packages). This is what makes it safe to use `pull_request_target` for fork PRs. |
There was a problem hiding this comment.
What is dangerous about checking out the PR head? We should note here that the PR head could have attack instructions which would be sent to an LLM. Even so - what is the level of danger? Is the LLM allowed to execute commands, or are all shell commands run deterministically and only their output is given to the LLM? Isn't the output of this step simply a 1/3/5? Is that constrained deterministically? What's the worst that could happen? The Jobs section and security section below go into a some more detail but are still insufficient to fully describe the risk IMO.
|
|
||
| ## 1. `pr-review-auto-route.yml` — Sizer + Proposal | ||
|
|
||
| - **Trigger:** `pull_request_target` on `opened` / `reopened` / `synchronize` (base branches `main`, `next`, `release/**/*`). |
There was a problem hiding this comment.
Do you think we should run review on release branches?
There was a problem hiding this comment.
THat's a good point. Most release branch PRs are either release-related/semi-automated or are basically cherry-picks from main. However, since the reviews are opt-in, I think it's valuable to leave them on. Occasionally we do have PRs that aren't straight ports, and the reviews might be useful there.
| - **Trust check:** Uses `actions-cool/check-user-permission` to require `write` on the *acting user* (the comment author is the bot, so `author_association` is meaningless here). | ||
| - **What it does:** Fetches the PR's current `head_sha` / `base_ref` from the API (not from any pinned artifact — handles force-pushes between proposal and toggle), parses selected reviewer checkboxes, calls `gh workflow run pr-review-fleet.yml` with explicit `pr_number`, `reviewers`, `head_sha`, `base_ref` inputs. Then resets the "Start review" checkbox so a single tick re-runs it next time. | ||
|
|
||
| ## 3. `pr-review-dispatch.yml` — Label Path |
There was a problem hiding this comment.
We note in the auto-route section that auto-route get skipped if pr-review-fleet label is present. What happens if we add the label when auto-route (or a review kicked off by auto-route) is already running?
| └─▶ pr-review-fleet via workflow_run (downloads artifact, default 3 reviewers) | ||
| ``` | ||
|
|
||
| ## Security Notes |
There was a problem hiding this comment.
As mentioned above, I think each possible attack should be described in a little bit more detail. E.g. "We give access to A, but not B. If an attacker has A, it doesn't help them because X. But if an attacker had B, they would be able to do Y."
Probably, do all the detail here, and then have the sections that mention security in auto-route and review-fleet can simply link/reference this section. E.g. "We load from the base branch, not the fork branch - see below for the security rationale."
|
|
||
| > Adapted from a transcript discussion, corrected against the actual Fleet Review workflows in this directory. See [`pr-review-fleet.README.md`](./pr-review-fleet.README.md) for the workflow reference. | ||
|
|
||
| Understanding the difference between `pull_request` and `pull_request_target` is important for designing secure GitHub Actions workflows. |
There was a problem hiding this comment.
They're events that can be used as triggers. I'll clarify.
|
|
||
| The pattern we use is to explicitly treat `pull_request_target` jobs as handling untrusted input, and to aggressively constrain what comes out of them. Instead of passing raw data downstream, we convert things into well-defined, controlled outputs — typically small blobs of JSON that we generate ourselves. The idea is that no matter what the PR contains, the only thing leaving the job is something we've sanitized and shaped. | ||
|
|
||
| A concrete example is the review routing logic in `pr-review-auto-route.yml`. The workflow runs on `pull_request_target` from the base branch and **never checks out the PR head** — it only `git fetch`es the head commit and reads `--numstat` from it. From those numbers it derives `lines` / `files` / `packages` counts and picks a tier. The only thing that leaves the job is a sticky proposal comment that the bot constructs itself. That base-branch-only checkout is the foundational invariant that makes `pull_request_target` safe here; the JSON-shaped outputs are a second layer on top. |
There was a problem hiding this comment.
This paragraph is mixing two points together.
- Sanitizing input
- Sanitizing output
The previous paragraph is only talking about 2, and this paragraph leads by saying it's a concrete example of that. But then it emphasizes (in bold) 1. Then it goes back to 2. Then in concludes with 1, saying that 2 is secondary.
Nothing incorrect, just a little pedagogically disorganized and confusing.
There was a problem hiding this comment.
Yeah, classic stream of consciousness that didn't get cleaned up enough. I'll re-work it.
|
|
||
| 1. The reviewer agent has **no shell or exec tools** — only `--allow-tool=read --allow-tool=write`. Even though `.git` is on disk, the agent can't invoke `git`. | ||
| 2. Checkout uses `persist-credentials: false`, so the auth token isn't stashed in the local Git config for later steps to reuse. | ||
| 3. All git-derived context (the diff, the changed-files list, the reviewer prompt) is materialized **before** `COPILOT_GITHUB_TOKEN` enters the step's environment. |
|
|
||
| ## Output is untrusted too | ||
|
|
||
| Model output is also untrusted. The consolidator regex-scrubs GitHub token patterns (`gh[pousr]_…`, `github_pat_…`) from the report before it's posted as a PR comment. It's defense-in-depth, but worth doing — the agent is denied shell tools, but its prose output still flows to a public-facing comment. |
There was a problem hiding this comment.
A clever model could easily just switch the prefix and output the token anyway, though. That's exactly the kind of thing I would expect an exfiltration attempt to do as a baseline. Is there any way we can robustly prevent this? Seems like we would need one-time-use tokens generated per action (maybe that's already the case? Or is it one special token shared by all?).
There was a problem hiding this comment.
We currently have a single token that is used for the CLI's auth. Ideally we would have something like GitHub Apps where you can get a short-lived token on-demand, but that doesn't work because of how Copilot is licensed - it's per seat, and requires a billable account to tie the acticity to. So the PAT is as good as we get today - but it's not a long term solution IMO because PAT lifetimes are continually being reduced as part of SFI. I think we're ultimately going to need something like AgencyHub or what Justin's been working on, because this security posture isn't sustainable.
| 2. Checkout uses `persist-credentials: false`, so the auth token isn't stashed in the local Git config for later steps to reuse. | ||
| 3. All git-derived context (the diff, the changed-files list, the reviewer prompt) is materialized **before** `COPILOT_GITHUB_TOKEN` enters the step's environment. | ||
|
|
||
| This was the shape of a previous MSRC issue: earlier versions allowed arbitrary Git operations with persisted credentials, which could have been abused for exfiltration. Removing Git capabilities from the credentialed step closed that hole. The mitigation is "the agent can't reach git," not "git isn't used at all." |
There was a problem hiding this comment.
"The mitigation is "the agent can't reach git," not "git isn't used at all.""
FWIW this wasn't useful for my understanding - I wasn't already thinking "git isn't used at all" and it's also pretty vague what "reaching git" means.
There was a problem hiding this comment.
Good point. This was what helped me understand the original mitigations, because my initial understanding was that we couldn't safely invoke git operations anywhere in the pipeline, but the problem was thankfully more subtle than that. I'll either wipe this or reword it.
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Adds two docs alongside the fleet review workflows in
.github/workflows/:pr-review-fleet.README.md— reference for the four workflows that compose the fleet review system (pr-review-auto-route,pr-review-confirm,pr-review-dispatch,pr-review-fleet). Covers triggers, permissions, sizing thresholds, end-to-end flows for both the auto-route and label paths, and per-workflow security invariants.pr-review-fleet.design-notes.md— design discussion ofpull_requestvspull_request_targetas applied to this pipeline. Covers why the auto-router can safely run onpull_request_target(never checks out PR head), how the fleet workflow isolates the credentialed Copilot agent from git (no shell tools +persist-credentials: false+ prompt loaded from base branch), output scrubbing as defense-in-depth, theactions: writerequirement on the confirm workflow, and existing actor-permission checks.Docs-only. No workflow behavior changes.