mirror of
https://github.com/garrytan/gbrain.git
synced 2026-08-14 00:48:18 +00:00
Garry TanandClaude Fable 5
88731d8cf3
Blind review round 2 rejected the branch. Five findings, all fixed.
BLOCKING 1 — the verdict was forgeable through a PR filename.
renderComment wrote mechanical red-flag details raw, and two of them
interpolate filenames (adds_recipe, deletes_tests). git allows a newline
inside a filename and JS `[^/]` matches one, so RECIPE_RE's anchors were
decorative: a PR adding `src/core/ai/recipes/x\n## PR Gate — ...\ncc
@octocat\n<!-- ...state... -->\nz.ts` put that text verbatim into the bot's
comment — forged heading, live third-party mention, and on the NEUTRAL
render (which writes no state block of its own) parseState() returned the
ATTACKER's block, so the next run hit the spend guard and silently skipped
the verdict with no label and exit 0. Three layers:
(a) flag details go through sanitizeList, exactly like the model's
strings. Audited every other interpolation into the comment; the rest
are literals in the file, Number()-coerced, or already sanitized.
(b) every path regex spells its segment class [^/\n], not [^/] —
RECIPE_RE, SOURCE_EXT_RE and the test-path check.
(c) parseState reads the state block only off line 2 of a marker-leading
comment (where renderComment writes it) and STATE_RE is whole-line
anchored. A block anywhere else is somebody else's text.
BLOCKING 2 — no exemption, so every release PR was close-lane.
Measured: 40 of the last 40 merged PRs would be close-lane on
missing_screenshot, including every /ship release PR. A check that is red
on every release gets switched off within a week, and then it filters
nothing. The #3745 policy exists to filter INCOMING OUTSIDE CONTRIBUTIONS;
release automation cannot take a screenshot of itself. It is now waived for
OWNER/MEMBER/COLLABORATOR, bot authors and drafts — the usefulness verdict,
the title rule and every mechanical red flag still run, and the sticky
comment says the check was skipped. author_association / draft / user.type
are read from the pr.json the workflow already fetches: no new API call, one
source of truth. `draft` is the one author-settable input, so
ready_for_review joins the trigger list and the exemption is folded into the
spend-guard hash — the draft-era verdict cannot be reused after the flip.
Stated as a deliberate decision in both the workflow header and the script.
3 — DOWNGRADE_FLAG_IDS omitted deletes_tests, adds_symlink and
adds_node_modules, so a PR deleting test/e2e/engine-parity.test.ts kept
merge-lane and a green check on the strength of its prose. All three added.
The old test used deletes_tests as its example of a NON-downgrading flag;
rewritten to pin the stronger invariant instead — every id detectRedFlags
can emit is a downgrade trigger (derived from the detector, so a new flag
fails until it is classified on purpose), and the set is still an allowlist
(an unrecognized id changes nothing).
4 — FENCE_RE backtracks superlinearly on a hostile body: 65KB of backticks
(GitHub's max body length) measured 8.2s across the two policy scans on a
pull_request_target runner. stripCodeFences now caps the scan at 16KB —
same input, 0.40s. Tradeoff documented at the constant: the intent
paragraph and the screenshot both sit near the top in practice (the PR
template puts them in the first two sections, and the model payload already
caps the same body at 6KB), so a real contributor is not judged on a
truncated tail.
Verified: test/pr-gate-workflow.test.ts 126 pass / 0 fail (was 95),
typecheck clean, actionlint clean, check-privacy /
check-no-tracked-symlinks / check-progress-to-stdout /
check-bun-test-timeout / check-key-files-current-state all exit 0. Each new
pin was mutation-tested against the pre-fix behavior: all six mutants fail
the suite.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
111 lines
5.5 KiB
YAML
111 lines
5.5 KiB
YAML
name: PR Gate
|
|
|
|
# Strict PR usefulness gate (#3698): classifies every PR to master into
|
|
# merge-lane / close-lane / needs-maintainer BEFORE any human review effort.
|
|
# Verdict + reviewer checklist land in one sticky comment; exactly one
|
|
# gate:* label is applied; close-lane exits 1 (red X = strong signal).
|
|
#
|
|
# SECURITY MODEL (pull_request_target on a 30k-star public repo):
|
|
# - PR code is NEVER checked out or executed. Metadata + diff come from the
|
|
# GitHub API only; the diff is capped at 120KB.
|
|
# - The checkout below is the BASE repo (master) — rubric/script only.
|
|
# NEVER add a `ref:` pointing at the PR head.
|
|
# - Attacker-controlled values (title/body/diff) never touch the shell:
|
|
# every ${{ }} is env-bound; run: scripts use plain env vars.
|
|
# - Only the issues API is used (comments + labels), so issues:write is the
|
|
# single write grant; the checkout drops its credentials.
|
|
# - The mechanical CONTRIBUTING.md #3745 check (intent paragraph + screenshot)
|
|
# runs BEFORE any API dependency, so a PR missing either still lands in
|
|
# close-lane during an Anthropic outage — an outage is not a way through.
|
|
# - If ANTHROPIC_API_KEY is missing or the API is unreachable on an otherwise
|
|
# compliant PR, the script NEUTRAL-skips loudly (sticky comment + warning
|
|
# annotation, exit 0) and CLEARS any stale gate:* label — never a silent
|
|
# green, never a red X for a missing secret, never a stale verdict. A model
|
|
# REFUSAL is not a skip: it routes to needs-maintainer so refusing is not a
|
|
# way to dodge the gate.
|
|
#
|
|
# #3745 EXEMPTION (deliberate — mirrors policyExemption() in
|
|
# scripts/pr-gate.mjs): the intent-paragraph + screenshot requirement filters
|
|
# INCOMING OUTSIDE CONTRIBUTIONS. It is waived for repo owners / members /
|
|
# collaborators, bot authors, and drafts.
|
|
# Release automation cannot take a screenshot of itself, and without the
|
|
# exemption every /ship release PR lands in close-lane
|
|
# (measured: 40 of the last 40 merged PRs) — a check that is red
|
|
# on every release gets switched off within a week, and then it filters
|
|
# nothing. Exempt PRs still get the FULL usefulness verdict, the title rule and
|
|
# every mechanical red flag; only the description requirement is skipped, and
|
|
# the sticky comment says so on its own line.
|
|
# author_association / draft / user.type are read from the pr.json fetched
|
|
# below — GitHub-computed, not author-settable (except `draft`), and already
|
|
# on disk, so nothing new is fetched and there is one source of truth.
|
|
# `ready_for_review` is in the trigger list precisely because `draft` IS
|
|
# author-settable: leaving draft re-runs the gate with the exemption gone, and
|
|
# the exemption is folded into the spend-guard hash so the draft-era verdict
|
|
# cannot be reused.
|
|
# Pinned by test/pr-gate-workflow.test.ts.
|
|
|
|
on:
|
|
pull_request_target:
|
|
types: [opened, edited, synchronize, reopened, ready_for_review]
|
|
branches: [master]
|
|
|
|
# issues:write is the ONLY write grant. Everything the script calls is the
|
|
# issues API (comments, label create, label add/remove on the PR's issue), so
|
|
# pull-requests:write would be a redundant second grant on the same objects.
|
|
permissions:
|
|
contents: read
|
|
issues: write
|
|
|
|
concurrency:
|
|
group: pr-gate-${{ github.event.pull_request.number }}
|
|
cancel-in-progress: true
|
|
|
|
jobs:
|
|
gate:
|
|
runs-on: ubuntu-latest
|
|
timeout-minutes: 10
|
|
steps:
|
|
# Base repo (master) only — provides scripts/pr-gate.mjs.
|
|
- uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
|
|
with:
|
|
# Nothing here needs git auth after the clone; don't leave a token
|
|
# in .git/config for the rest of the job.
|
|
persist-credentials: false
|
|
|
|
- name: Fetch PR metadata + diff (API only — PR code is never checked out)
|
|
env:
|
|
GH_TOKEN: ${{ github.token }}
|
|
REPO: ${{ github.repository }}
|
|
PR_NUMBER: ${{ github.event.pull_request.number }}
|
|
run: |
|
|
set -euo pipefail
|
|
mkdir -p "$RUNNER_TEMP/pr-gate"
|
|
gh api "repos/${REPO}/pulls/${PR_NUMBER}" > "$RUNNER_TEMP/pr-gate/pr.json"
|
|
# First 100 files is enough: red flags key off pr.json's changed_files
|
|
# count, and >40 files already flags.
|
|
gh api "repos/${REPO}/pulls/${PR_NUMBER}/files?per_page=100" \
|
|
> "$RUNNER_TEMP/pr-gate/files.json"
|
|
# Diff via the .diff media type; GitHub can 406 on huge diffs —
|
|
# degrade to a marker instead of failing the gate.
|
|
gh api "repos/${REPO}/pulls/${PR_NUMBER}" \
|
|
-H "Accept: application/vnd.github.diff" \
|
|
> "$RUNNER_TEMP/pr-gate/pr.diff.full" \
|
|
|| printf '[diff unavailable from the GitHub API — too large or unfetchable]\n' \
|
|
> "$RUNNER_TEMP/pr-gate/pr.diff.full"
|
|
MAX=122880 # 120KB cap
|
|
if [ "$(wc -c < "$RUNNER_TEMP/pr-gate/pr.diff.full")" -gt "$MAX" ]; then
|
|
head -c "$MAX" "$RUNNER_TEMP/pr-gate/pr.diff.full" > "$RUNNER_TEMP/pr-gate/pr.diff"
|
|
printf '\n\n[TRUNCATED: diff capped at 120KB]\n' >> "$RUNNER_TEMP/pr-gate/pr.diff"
|
|
else
|
|
mv "$RUNNER_TEMP/pr-gate/pr.diff.full" "$RUNNER_TEMP/pr-gate/pr.diff"
|
|
fi
|
|
rm -f "$RUNNER_TEMP/pr-gate/pr.diff.full"
|
|
|
|
- name: Gate verdict (sticky comment + label; exit 1 only on close-lane)
|
|
env:
|
|
GITHUB_TOKEN: ${{ github.token }}
|
|
ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }}
|
|
GITHUB_REPOSITORY: ${{ github.repository }}
|
|
PR_NUMBER: ${{ github.event.pull_request.number }}
|
|
run: node scripts/pr-gate.mjs "$RUNNER_TEMP/pr-gate"
|