Harden the greenlight reviewer against credential exfiltration - #8481
Merged
Conversation
**Impact:** greenlight PR reviewer (CI only) — the dispatched `greenlight-pr-review.yml` workflow and the `verdict` command **Risk:** low ## What Adds three defense-in-depth controls around the untrusted reviewer model: a read-confinement PreToolUse hook, secret-scrubbing of the verdict message, and an automatic decline for oversized diffs. ## Why The reviewer LLM runs on attacker-influenced input (the PR diff and the checked-out `pytorch/pytorch` tree). Previously it had unrestricted `Read`/`Glob`/`Grep` and its verdict `message` was published verbatim, so a prompt-injection payload could coax it to read a credential (OIDC token in `/proc`, `$GITHUB_ENV`, the scoped checkout token in `./pytorch/.git/config`) and emit it into the ClickHouse row or the public PR comment. These changes narrow that residual gap the previous hardening left open. - **Read confinement** — `restrict-read.py` denies by default, allowing a target only when its `realpath` lands under `./pytorch`, the trusted `.claude/skills`/`.claude/hooks`, or the `/tmp/greenlight-*` scratch, and never through a `.git` component. `persist-credentials: false` keeps the checkout token out of `./pytorch/.git/config`. - **Message scrubbing** — `redact.scrub_secrets` replaces credential-shaped substrings with `[REDACTED]` at a single fan-out point in `verdict.run`, covering both the emitted row and the posted comment. - **Oversized-diff decline** — the workflow gates on diff line count (the model's ~2000-line read window) with a byte backstop, dropping a canned `scope_too_large` NO_LAND rather than reviewing a change it cannot read in full. # Notes - Confinement changes the model's access contract: path-less `Glob`/`Grep` are now denied, so the greenlight-review skill was updated to require an explicit `path`. - Scrubbing is best-effort (precision over recall) — a novel or reshaped secret may slip past; it is not a guarantee. - The size caps are tunable via repo vars `PYTORCH_GREENLIGHT_MAX_DIFF_LINES` (default 2000) and `PYTORCH_GREENLIGHT_MAX_DIFF_BYTES` (default 500000). - The `eval_hash` land-guard was noted in an earlier review comment — this branch does not touch it. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
meta-cla
Bot
added
the
CLA Signed
jeanschmidt
temporarily deployed
to
greenlight-record
August 10, 2026 20:59 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
jeanschmidt
temporarily deployed
to
greenlight-record
August 10, 2026 21:01 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
- restrict-read: rewrite path-less Glob/Grep to search ./pytorch via an exit-0 "allow" decision with updatedInput instead of denying - Preserve all original tool-input fields, setting an absolute path last so an attacker-supplied empty/junk path cannot survive - Keep deny-by-default for Read and explicit paths; still block '..' and absolute patterns/globs on path-less searches - Update greenlight-review SKILL.md to document the default-path behavior - Rework tests: assert the rewrite, field preservation, and that denials still fire for dotdot/absolute patterns and globs Notes: Reviewers hit path-confinement friction because a bare Glob/Grep was denied outright, forcing an explicit ./pytorch path on every call. Defaulting the search root to ./pytorch keeps the deny-by-default guarantees while removing the papercut. The rewrite depends on the reviewer CLI honoring updatedInput; a CLI that ignored it would fall back to a workspace-root search. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
huydhn
approved these changes
Aug 10, 2026
jeanschmidt
temporarily deployed
to
greenlight-record
August 10, 2026 23:35 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
jeanschmidt
temporarily deployed
to
greenlight-record
August 10, 2026 23:36 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
@huydhn - saving time on PR review churn, as they touch similar files, this would make a serial review sequence. But yeah, a ghstack should have done the trick, note taken :) |
atalman
added a commit
that referenced
this pull request
Aug 11, 2026
Two lintrunner failures from this PR: - PYFMT wanted the blank line after imports and the _req signature on one line. Applied exactly the patch it printed. - ACTIONLINT rejects `vars.VLLM_TRIAGE_FILE_ISSUES`: the actionlint pinned in this repo does not know the `vars` context. Dropped the repo-variable escape hatch and gated solely on the file_issues input. Enabling filing for the cron now means flipping that input's default, which is a one-line change rather than a repo setting. Note the same run also reports two ACTIONLINT errors in greenlight-pr-review.yml for the identical `vars` reason. Those are not from this PR -- they arrived with #8481 and lint has been failing on main since. This PR cannot go green until that is fixed separately; the ACTIONLINT adapter lints every workflow, not just changed ones. Test Plan: ``` python3 -m py_compile tools/torchci/vllm_triage_file_issues.py # ok node -e "yaml.load(...)" -> jobs: [triage, root-cause, file-issues] grep -n 'vars\.' .github/workflows/vllm-torch-nightly-triage.yml # none ``` Filing dry run unchanged after the edits: still selects 1 of 4 synthetic causes and prints the same fingerprint.
jeanschmidt
pushed a commit
that referenced
this pull request
Aug 11, 2026
…8490) ## Summary ACTIONLINT has been failing on `main` since #8481, which introduced `vars.PYTORCH_GREENLIGHT_MAX_DIFF_LINES` / `_BYTES` in `greenlight-pr-review.yml`: ``` Error (ACTIONLINT) [expression] undefined variable "vars". available variables are "env", "github", "inputs", "job", "matrix", "needs", "runner", "secrets", "steps", "strategy" 178 | MAX_DIFF_LINES: ${{ vars.PYTORCH_GREENLIGHT_MAX_DIFF_LINES || '2000' }} 179 | MAX_DIFF_BYTES: ${{ vars.PYTORCH_GREENLIGHT_MAX_DIFF_BYTES || '500000' }} ``` `vars` is valid GitHub Actions syntax. The linter is simply stale: `.lintrunner.toml` pins actionlint **1.6.21** via `s3_init_config.json`, and the `vars` context was added in **1.6.24**. ## Why this blocks everything The ACTIONLINT linter's `include_patterns` are `.github/workflows/*.yml` / `*.yaml`, so it lints **every** workflow regardless of what a PR touched. Any PR therefore inherits this failure. Confirmed on `main`: | run | commit | result | |---|---|---| | 31446317087 | `a6fa78f0b9` | failure | | 31444723388 | `0e82c9f495` | failure | | 31443941296 | `982575bb72` ← #8481 | failure | | 31432664642 | `030feb596f` | success | ## The fix Replaces the two lookups with literals carrying the same defaults, plus a comment recording why and how to revert. **Bumping actionlint is the better fix** — `vars` is legitimate and the linter should understand it. I did not do that here because it needs new binaries published to the `oss-clang-format` S3 bucket that `s3_init_config.json` points at, which I can't write to. Worth doing as a follow-up; this unblocks the repo in the meantime. ## Trade-off The runtime override via repo variables is lost until the binary is bumped — tuning the caps becomes a one-line edit rather than a repo setting. Given the alternative is `main` staying red, that seemed the right call, but say the word if you'd rather I wait for an actionlint bump instead. Behaviour is otherwise unchanged: the values were already the defaults, and the step validates them as non-negative integers before use, so nothing downstream is affected. ## Test plan ``` $ grep -rn 'vars\.' .github/workflows/ | grep -v 'env/vars.sh' (only the explanatory comment remains) $ node -e "yaml.load(...)" YAML OK | jobs: [ announce_start, review, record ] sizecheck env: {"MAX_DIFF_LINES":"2000","MAX_DIFF_BYTES":"500000"} ``` actionlint itself was not run locally — the binary is fetched by `lintrunner init` from S3 — so CI is the real check. If `lintrunner` goes green here, that confirms these two lines were the only remaining ACTIONLINT failures. cc @atalman --- 🤖 Authored with assistance from Claude Code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Impact: greenlight PR reviewer
Risk: low
What
Adds three defense-in-depth controls around the untrusted reviewer model: a read-confinement PreToolUse hook, secret-scrubbing of the verdict message, and an automatic decline for oversized diffs.
Why
The reviewer LLM runs on attacker-influenced input (the PR diff and the checked-out
pytorch/pytorchtree). Previously it had unrestrictedRead/Glob/Grepand its verdictmessagewas published verbatim, so a prompt-injection payload could coax it to read a credential (OIDC token in/proc,$GITHUB_ENV, the scoped checkout token in./pytorch/.git/config) and emit it into the ClickHouse row or the public PR comment. These changes narrow that residual gap the previous hardening left open.restrict-read.pydenies by default, allowing a target only when itsrealpathlands under./pytorch, the trusted.claude/skills/.claude/hooks, or the/tmp/greenlight-*scratch, and never through a.gitcomponent.persist-credentials: falsekeeps the checkout token out of./pytorch/.git/config.redact.scrub_secretsreplaces credential-shaped substrings with[REDACTED]at a single fan-out point inverdict.run, covering both the emitted row and the posted comment.scope_too_largeNO_LAND rather than reviewing a change it cannot read in full.Notes
Glob/Grepare now denied, so the greenlight-review skill was updated to require an explicitpath.PYTORCH_GREENLIGHT_MAX_DIFF_LINES(default 2000) andPYTORCH_GREENLIGHT_MAX_DIFF_BYTES(default 500000).eval_hashland-guard was noted in an earlier review comment — this branch does not touch it.