diff --git a/.github/workflows/gate-verdict.yml b/.github/workflows/gate-verdict.yml new file mode 100644 index 0000000000..f9b192f2c4 --- /dev/null +++ b/.github/workflows/gate-verdict.yml @@ -0,0 +1,44 @@ +name: Gate verdict + +# Enforces the seat-gate rule in docs/development/external-review.md: a PR that is +# not docs-only or test-only needs a `GATE: PASS` from the gate account at its exact +# head. The logic is scripts/gate-verdict.sh, unit-tested by +# scripts/__tests__/gate-verdict.test.mjs. This file only says when to run it. +# +# The script and its tests are read from the PR checkout, so a PR that edits them +# grades itself. Pin both to main's copy (a follow-up, not in #3051) before this +# check is made required. +# +# Not a required check yet. Making it one is a repo-settings step for the operator. + +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review, edited] + pull_request_review: + types: [submitted, edited, dismissed] + +permissions: + contents: read + pull-requests: read + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + gate-verdict: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + - name: Script unit tests + run: node --test scripts/__tests__/gate-verdict.test.mjs + - name: Docs or tests only, or a seat gate PASS at this head + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: bash scripts/gate-verdict.sh diff --git a/docs/development/external-review.md b/docs/development/external-review.md index 8c910e844b..6059f2180b 100644 --- a/docs/development/external-review.md +++ b/docs/development/external-review.md @@ -93,14 +93,32 @@ can't tell us whether it's wrong or whether we are. as the second check. It reads each changed file in full context, including the callers and readers of whatever changed. For example, a new setting must have something that reads it. The verdict names the head SHA it checked out - and the commands it ran. A PASS without both does not count. Full suites - stay in CI, so run focused tests only. + and the commands it ran. `gate-verdict` enforces the SHA. The commands are not + machine-checked, so the reviewer lists them. Full suites stay in CI, so run + focused tests only. The writer names the tier in the PR body. A reviewer can raise it but never lower it. A seat verdict is a review from the seat account at the exact head - whose last line is `GATE: PASS`, and the press tooling reads only those. A - CodeRabbit review is never a verdict. It only lets a docs-only or test-only - PR skip the seat gate. + whose first line opens with `GATE: PASS` and names the head's first seven characters, + and whose final non-blank line is exactly `GATE: PASS` as stored, with the credit line + above it (review-checklist rules 44 and 51). The press reads that line as stored, so a + footer or bold markup below the sentinel fails the PASS. `gate-verdict` finds a verdict + with footers and markup removed, at either end, so a `GATE: HOLD` at either end blocks a + PASS before it at the same head, and a footer under the HOLD does not hide it. A + CodeRabbit review is never a verdict. It only lets a docs-only or test-only PR skip the + seat gate. + +- **`gate-verdict` checks this rule on every PR head.** `.github/workflows/gate-verdict.yml` + runs `scripts/gate-verdict.sh`. A PR passes as Tier 0 when every changed file is docs + or test code and the file list covers `changed_files`. A rename counts both its old + and its new path. Outside `docs/`, `SKILL.md` files, skill trees, `k8s/helm/commonly/configs/`, + and `CLAUDE.md`, `AGENTS.md` and `REVIEW.md` are behaviour, not docs. Any other PR + needs a verdict from the gate account at its exact head whose first line opens with + `GATE: PASS` and names the head SHA, and whose final non-blank line, as stored, is `GATE: PASS`. The latest + verdict at that head wins. The job does not count the Tier 2 second + gate. It runs the script from the PR head, so a PR that edits the script grades itself + until the script is pinned to main, and it is not a required check until the operator + makes it one. ## Keeping a PR out of automatic review diff --git a/scripts/__tests__/gate-verdict.test.mjs b/scripts/__tests__/gate-verdict.test.mjs new file mode 100644 index 0000000000..2543f1ee07 --- /dev/null +++ b/scripts/__tests__/gate-verdict.test.mjs @@ -0,0 +1,292 @@ +import test, { after } from 'node:test'; +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// Runs scripts/gate-verdict.sh with a fake `gh` on PATH. The fake answers the three REST +// calls from JSON fixtures and applies the script's own --jq filter with jq, so the +// filter the workflow ships is the one under test. +const SCRIPT = join(dirname(fileURLToPath(import.meta.url)), '..', 'gate-verdict.sh'); +const HEAD = 'a'.repeat(40); +const OLD = 'b'.repeat(40); + +const FAKE_GH = `#!/usr/bin/env bash +set -euo pipefail +if [ -n "\${FAKE_GH_FAIL:-}" ]; then echo "HTTP 500" >&2; exit 1; fi +path=""; expr="" +while [ $# -gt 0 ]; do + case "$1" in + api|--paginate) shift ;; + --jq) expr="$2"; shift 2 ;; + *) path="$1"; shift ;; + esac +done +case "$path" in + */files) fixture="$FAKE_FILES" ;; + */reviews) fixture="$FAKE_REVIEWS" ;; + */pulls/[0-9]*) fixture="$FAKE_PR" ;; + *) echo "unexpected path: $path" >&2; exit 2 ;; +esac +jq -r "$expr" "$fixture" +`; + +const bin = mkdtempSync(join(tmpdir(), 'gate-verdict-gh-')); +writeFileSync(join(bin, 'gh'), FAKE_GH); +chmodSync(join(bin, 'gh'), 0o755); +after(() => rmSync(bin, { recursive: true, force: true })); + +let nextId = 1000; +const review = (body, { login = 'lilyshen0722', commit = HEAD, state = 'COMMENTED' } = {}) => + ({ id: nextId++, user: { login }, commit_id: commit, state, submitted_at: '2026-10-31T07:30:00Z', body }); +const PASS = `GATE: PASS @ ${HEAD}\n\nRan the focused tests.\n\nGATE: PASS`; +const FAIL = 'Found a defect in the rollback path.\n\nGATE: FAIL'; +const HOLD = 'The rollback path needs a test.\n\nGATE: HOLD'; + +const CODE_FILES = ['backend/services/byoHandoffService.ts']; +const TIER0_FILES = [ + 'docs/development/external-review.md', + 'backend/__tests__/unit/services/byoHandoffService.test.ts', + 'docs/adr/ADR-011-shell-first-pre-gtm.md', +]; + +// A changed file is a path, or a { filename, previous_filename } pair for a rename. +const entry = (file) => (typeof file === 'string' ? { filename: file } : file); + +function run({ files = [], changedFiles = files.length, reviews = [], env = {} } = {}) { + const fixtures = mkdtempSync(join(tmpdir(), 'gate-verdict-fx-')); + try { + const filesPath = join(fixtures, 'files.json'); + const reviewsPath = join(fixtures, 'reviews.json'); + const prPath = join(fixtures, 'pr.json'); + writeFileSync(filesPath, JSON.stringify(files.map(entry))); + writeFileSync(reviewsPath, JSON.stringify(reviews)); + writeFileSync(prPath, JSON.stringify({ changed_files: changedFiles })); + return spawnSync('bash', [SCRIPT], { + encoding: 'utf8', + env: { + PATH: `${bin}:${process.env.PATH}`, + REPO: 'Team-Commonly/commonly', + PR_NUMBER: '3050', + HEAD_SHA: HEAD, + FAKE_FILES: filesPath, + FAKE_REVIEWS: reviewsPath, + FAKE_PR: prPath, + ...env, + }, + }); + } finally { + rmSync(fixtures, { recursive: true, force: true }); + } +} + +test('docs and test files only are Tier 0: no verdict needed', () => { + const r = run({ files: TIER0_FILES }); + assert.equal(r.status, 0, r.stderr); + assert.match(r.stdout, /Tier 0/); +}); + +test('a dot-directory markdown file is not docs: it needs a gate', () => { + assert.equal(run({ files: ['.github/PULL_REQUEST_TEMPLATE.md', '.claude/skills/x/SKILL.md'] }).status, 1); +}); + +test('a dot-directory under docs is not docs either: it needs a gate', () => { + // Built from parts, as docReferences.test.js builds its fixtures: a literal doc path here would + // be a pointer that the doc-reference scan follows to a file that is not there. + assert.equal(run({ files: [['docs', '.claude', 'skills', 'x', 'SKILL.md'].join('/')] }).status, 1); + assert.equal(run({ files: [['docs', '.hidden', 'notes.md'].join('/')] }).status, 1); +}); + +test('one code file among docs and tests still needs a gate', () => { + const r = run({ files: [...TIER0_FILES, ...CODE_FILES] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /no GATE verdict from lilyshen0722/); +}); + +test('an empty file list is not Tier 0', () => { + assert.equal(run({ files: [] }).status, 1); +}); + +test('a code PR with no verdict is blocked', () => { + const r = run({ files: CODE_FILES }); + assert.equal(r.status, 1); + assert.match(r.stdout, /no GATE verdict/); +}); + +test('a GATE: PASS from the gate account at the head passes', () => { + const r = run({ files: CODE_FILES, reviews: [review(PASS)] }); + assert.equal(r.status, 0, r.stderr); + assert.match(r.stdout, new RegExp(`GATE: PASS at ${HEAD}`)); +}); + +test('the account match ignores case', () => { + assert.equal(run({ files: CODE_FILES, reviews: [review(PASS, { login: 'LilyShen0722' })] }).status, 0); +}); + +test('a PASS at an older head does not count', () => { + assert.equal(run({ files: CODE_FILES, reviews: [review(PASS, { commit: OLD })] }).status, 1); +}); + +test('a PASS from another account does not count, CodeRabbit included', () => { + const reviews = [review(PASS, { login: 'coderabbitai' }), review(PASS, { login: 'sharpen-bot' })]; + assert.equal(run({ files: CODE_FILES, reviews }).status, 1); +}); + +test('GATE: PASS must be the last line of the review', () => { + const body = `GATE: PASS @ ${HEAD}\n\nRan the focused tests.\n\nGATE: PASS\n\nFollow-up: none.`; + assert.equal(run({ files: CODE_FILES, reviews: [review(body)] }).status, 1); +}); + +test('trailing spaces and CRLF on the last line are ignored', () => { + const body = `GATE: PASS @ ${HEAD}\r\n\r\nRan the tests\r\n\r\nGATE: PASS \r\n`; + assert.equal(run({ files: CODE_FILES, reviews: [review(body)] }).status, 0); +}); + +test('a FAIL after a PASS at the same head blocks it', () => { + const r = run({ files: CODE_FILES, reviews: [review(PASS), review(FAIL)] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /last line is 'GATE: FAIL'/); +}); + +test('a PASS after a FAIL at the same head passes', () => { + assert.equal(run({ files: CODE_FILES, reviews: [review(FAIL), review(PASS)] }).status, 0); +}); + +test('a HOLD after a PASS at the same head blocks it', () => { + const r = run({ files: CODE_FILES, reviews: [review(PASS), review(HOLD)] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /last line is 'GATE: HOLD'/); +}); + +test('a PASS after a HOLD at the same head passes', () => { + assert.equal(run({ files: CODE_FILES, reviews: [review(HOLD), review(PASS)] }).status, 0); +}); + +test('markup on the first line is ignored, but a bold last line is not the sentinel', () => { + const body = `**GATE: PASS @ ${HEAD}**\n\nRan the tests.\n\n**GATE: PASS**`; + const r = run({ files: CODE_FILES, reviews: [review(body)] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /last line is '\*\*GATE: PASS\*\*'/); +}); + +test('a credit line below the sentinel fails the PASS; the same line above it passes', () => { + const below = `GATE: PASS @ ${HEAD}\n\nRan the focused tests.\n\nGATE: PASS\n\nWritten by Sable, a Commonly agent`; + const r = run({ files: CODE_FILES, reviews: [review(below)] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /last line is 'Written by Sable, a Commonly agent'/); + const above = `GATE: PASS @ ${HEAD}\n\nRan the focused tests.\n\nWritten by Sable, a Commonly agent\n\nGATE: PASS`; + assert.equal(run({ files: CODE_FILES, reviews: [review(above)] }).status, 0); +}); + +test('a footer under a HOLD does not hide it from the PASS before it', () => { + const pass = review(PASS); + const hold = review('The rollback path needs a test.\n\nGATE: HOLD\n\nPod thread: https://commonly.me/v2/pods/6a692a1be833c668acdb84cf'); + const r = run({ files: CODE_FILES, reviews: [pass, hold] }); + assert.equal(r.status, 1); + assert.match(r.stdout, new RegExp(`review ${hold.id}\\)`)); +}); + +test('only the exact last line GATE: PASS passes, so near-misses block', () => { + for (const last of ['GATE: PASSED', 'GATE:PASS', 'gate: pass', 'GATE: PASS (with notes)']) { + const body = `GATE: PASS @ ${HEAD}\n\nRan the focused tests.\n\n${last}`; + const r = run({ files: CODE_FILES, reviews: [review(body)] }); + assert.equal(r.status, 1, last); + } +}); + +test('a dismissed PASS does not count', () => { + assert.equal(run({ files: CODE_FILES, reviews: [review(PASS, { state: 'DISMISSED' })] }).status, 1); +}); + +test('a bare PASS that names no head SHA does not count', () => { + const r = run({ files: CODE_FILES, reviews: [review('GATE: PASS\n\nRan the focused tests.\n\nGATE: PASS')] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /first line is 'GATE: PASS'/); +}); + +test('a first line that declares HOLD blocks a PASS on the last line', () => { + const body = `GATE: HOLD @ ${HEAD}\n\nThe rollback path needs a test.\n\nGATE: PASS`; + const r = run({ files: CODE_FILES, reviews: [review(body)] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /first line is 'GATE: HOLD @/); +}); + +test('a HOLD whose last line is prose still blocks a PASS before it', () => { + const hold = `GATE: HOLD @ ${HEAD}\n\nThe rollback path needs a test.`; + const r = run({ files: CODE_FILES, reviews: [review(PASS), review(hold)] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /first line is 'GATE: HOLD @/); +}); + +test('a PASS whose last line is prose does not count', () => { + const body = `GATE: PASS @ ${HEAD}\n\nRan the focused tests.\n\nFollow-up: none.`; + const r = run({ files: CODE_FILES, reviews: [review(body)] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /last line is 'Follow-up: none\.'/); +}); + +test('the first line must say GATE: PASS as a word, so near-misses block', () => { + for (const first of ['GATE: PASSED', 'GATE:PASS', 'gate: pass', `Checked out ${HEAD}`]) { + const body = `${first} @ ${HEAD}\n\nRan the focused tests.\n\nGATE: PASS`; + assert.equal(run({ files: CODE_FILES, reviews: [review(body)] }).status, 1, first); + } +}); + +test('a PASS that names only an older SHA does not count', () => { + const r = run({ files: CODE_FILES, reviews: [review(`GATE: PASS @ ${OLD}\n\nRan the tests.\n\nGATE: PASS`)] }); + assert.equal(r.status, 1); +}); + +test('a rename out of a code path into docs is not Tier 0', () => { + const doc = ['docs', 'old-service.md'].join('/'); + const r = run({ files: [{ filename: doc, previous_filename: 'backend/services/foo.ts' }] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /no GATE verdict/); +}); + +test('a rename out of a code path into a test directory is not Tier 0', () => { + const files = [{ filename: 'backend/services/__tests__/foo.ts', previous_filename: 'backend/services/foo.ts' }]; + assert.equal(run({ files }).status, 1); +}); + +test('a rename within docs stays Tier 0', () => { + const [oldDoc, newDoc] = [['docs', 'old.md'], ['docs', 'new.md']].map((parts) => parts.join('/')); + assert.equal(run({ files: [{ filename: newDoc, previous_filename: oldDoc }] }).status, 0); +}); + +test('skill and config markdown is behaviour, not docs', () => { + const behaviour = [ + 'backend/commonly-bundled-skills/github/SKILL.md', + 'cli/skills/commonly/SKILL.md', + 'plugins/commonly-agent-plugin/skills/commonly/SKILL.md', + 'fleet-skills/testing/SKILL.md', + 'k8s/helm/commonly/configs/SKILLS_CATALOG.md', + ]; + for (const file of behaviour) { + assert.equal(run({ files: [file] }).status, 1, file); + } +}); + +test('agent instruction files are behaviour, not docs', () => { + for (const file of ['CLAUDE.md', 'AGENTS.md', 'REVIEW.md', 'backend/CLAUDE.md']) { + assert.equal(run({ files: [file] }).status, 1, file); + } +}); + +test('markdown outside the skill and config trees is still Tier 0, docs/skills included', () => { + const files = ['README.md', 'backend/TESTING.md', 'frontend/design-system/README.md', 'docs/skills/SKILLS_CATALOG.md']; + assert.equal(run({ files }).status, 0); +}); + +test('a file list shorter than changed_files is not Tier 0', () => { + const r = run({ files: TIER0_FILES, changedFiles: 4 }); + assert.equal(r.status, 1); + assert.match(r.stdout, /no GATE verdict/); +}); + +test('an API failure fails closed', () => { + const r = run({ files: CODE_FILES, reviews: [review(PASS)], env: { FAKE_GH_FAIL: '1' } }); + assert.notEqual(r.status, 0); +}); diff --git a/scripts/gate-verdict.sh b/scripts/gate-verdict.sh new file mode 100755 index 0000000000..d96307c5ae --- /dev/null +++ b/scripts/gate-verdict.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +# Gate verdict for one PR head (TASK-595). The policy is docs/development/external-review.md. +# +# Passes when every changed file is docs or test code (Tier 0), or when the latest verdict +# from the gate account at the exact head opens `GATE: PASS` and names the head SHA on its first +# line, and its final line as stored is `GATE: PASS`. Anything else fails, including an API error +# or a file list shorter than the PR's changed_files. +# +# This script runs from the PR checkout, so a PR that edits it grades itself until it is +# pinned to main's copy. Do not make the check required before then. +# +# Env: REPO (owner/name), PR_NUMBER, HEAD_SHA, GH_TOKEN (pull-requests: read). +# Optional: GATE_REVIEWER (default lilyshen0722), the account seat verdicts are posted from. +set -euo pipefail + +: "${REPO:?set REPO}" "${PR_NUMBER:?set PR_NUMBER}" "${HEAD_SHA:?set HEAD_SHA}" +export GATE_REVIEWER="${GATE_REVIEWER:-lilyshen0722}" HEAD_SHA + +# Tier 0 is docs and test files only. Dot-directories never count as docs: .github, +# .claude and .agents hold workflows and agent skills, which change behaviour. +EXEMPT_RE='^(docs/([^./][^/]*/)*[^/]+\.(md|mdx|txt|png|jpe?g|gif|svg|webp)|([^./][^/]*/)*[^/]+\.(md|mdx)|([^./][^/]*/)*__tests__/([^/]+/)*[^/]+|([^./][^/]*/)*[^/]+\.(test|spec)\.[cm]?[jt]sx?)$' +# Markdown that the code or the agents load is behaviour: skill files and skill trees, the +# k8s config directory and the agent instruction files. Under docs/ it stays docs. +BEHAVIOUR_RE='(^|/)SKILL\.md$|(^|/)[^/]*skills/|^k8s/helm/commonly/configs/|(^|/)(CLAUDE|AGENTS|REVIEW)\.md$' + +# One line per changed file: "\t". A rename lists both paths. +rows="$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[] | [.filename, (.previous_filename // "")] | @tsv')" +listed="$(printf '%s\n' "$rows" | grep -c . || true)" +expected="$(gh api "repos/$REPO/pulls/$PR_NUMBER" --jq '.changed_files')" +paths="$(printf '%s\n' "$rows" | tr '\t' '\n' | grep . || true)" + +# The list must be complete: a file list shorter than changed_files is not Tier 0. +if [ "$listed" -gt 0 ] && [ "$listed" -eq "$expected" ] && + [ -z "$(printf '%s\n' "$paths" | grep -Ev "$EXEMPT_RE" || true)" ] && + [ -z "$(printf '%s\n' "$paths" | grep -E "$BEHAVIOUR_RE" | grep -Ev '^docs/' || true)" ]; then + echo "✓ Tier 0: every changed file is docs or test code, so no seat gate is needed" + exit 0 +fi + +# One line per review by the gate account at this head: +# "\t\t\t\t". +# A verdict is found with footers and markup removed, at either end, so a footer under a HOLD cannot +# hide it. The first line of a PASS is read the same way, because rule 51 places the head name there. +# The last line of a PASS is read as stored: the press parses the final non-blank line, so a bold +# sentinel or a credit line below it is not a PASS. Pending and dismissed reviews don't count. +verdicts="$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/reviews" --jq ' + def clean_lines: + split("\n") + | map(sub("\\s+$"; "") | gsub("[*`]"; "") | sub("^\\s+"; "")) + | map(select(length > 0 and (test("^(written[ -]by|pod[ -]thread)"; "i") | not))); + .[] + | select((.user.login | ascii_downcase) == ($ENV.GATE_REVIEWER | ascii_downcase)) + | select(.commit_id == $ENV.HEAD_SHA and .state != "PENDING" and .state != "DISMISSED") + | (.body // "") as $body + | ($body | clean_lines) as $lines + | ($body | split("\n") | map(sub("\\s+$"; "")) | map(select(length > 0)) | .[-1] // "") as $stored_last + | [.id, .submitted_at, ($lines | .[0] // ""), ($lines | .[-1] // ""), $stored_last] + | @tsv')" + +# Review ids rise with creation, so the highest id is the latest verdict. A review is a verdict when +# its first or its last line mentions GATE:, so a HOLD at either end blocks a PASS before it. +latest="$(printf '%s\n' "$verdicts" | awk -F '\t' 'tolower($3) ~ /gate:/ || tolower($4) ~ /gate:/' | sort -t $'\t' -k1,1n | tail -n 1)" +if [ -z "$latest" ]; then + echo "::error::no GATE verdict from $GATE_REVIEWER at $HEAD_SHA. A PR that is not docs-only or test-only needs a seat gate at its exact head, and a new push needs a new gate." + exit 1 +fi + +review_id="$(printf '%s\n' "$latest" | cut -f 1)" +first="$(printf '%s\n' "$latest" | cut -f 3)" +last="$(printf '%s\n' "$latest" | cut -f 5)" +# A PASS opens with the words GATE: PASS and names the head's first seven characters, and its stored +# last line is exactly GATE: PASS, with any credit line above it. Both ends are needed (rules 44 and 51). +opens_pass=no +case "$first" in "GATE: PASS "*"${HEAD_SHA:0:7}"*) opens_pass=yes ;; esac +if [ "$opens_pass" != yes ] || [ "$last" != "GATE: PASS" ]; then + echo "::error::the latest verdict from $GATE_REVIEWER at $HEAD_SHA (review $review_id) is not a PASS at this head. Its first line is '$first' and its last line is '$last'. A PASS needs a first line that opens 'GATE: PASS' and names ${HEAD_SHA:0:7}, and a last line of exactly 'GATE: PASS' as stored, with any credit line above it." + exit 1 +fi +echo "✓ $GATE_REVIEWER: GATE: PASS at $HEAD_SHA (review $review_id)"