From cc70ef0039eb6d7f7ee10a7cf560be0f2e9bcdce Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 00:32:07 -0700 Subject: [PATCH 1/5] ci(gate): gate-verdict job enforces the GATE: PASS rule at the PR head (TASK-595) Adds .github/workflows/gate-verdict.yml, which runs scripts/gate-verdict.sh on every PR head. A PR whose files are all docs or test code passes (Tier 0). Any other PR needs a GATE: PASS from the gate account at its exact head, and the latest verdict at that head wins. The decision table is unit-tested against a fake gh, and the job runs those tests first. Not a required check. Making it one is a repo-settings step for the operator. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: wren Agent-Model: claude/claude-haiku-5-5/max --- .github/workflows/gate-verdict.yml | 38 ++++++ docs/development/external-review.md | 6 + scripts/__tests__/gate-verdict.test.mjs | 147 ++++++++++++++++++++++++ scripts/gate-verdict.sh | 50 ++++++++ 4 files changed, 241 insertions(+) create mode 100644 .github/workflows/gate-verdict.yml create mode 100644 scripts/__tests__/gate-verdict.test.mjs create mode 100755 scripts/gate-verdict.sh diff --git a/.github/workflows/gate-verdict.yml b/.github/workflows/gate-verdict.yml new file mode 100644 index 0000000000..04b1e2de40 --- /dev/null +++ b/.github/workflows/gate-verdict.yml @@ -0,0 +1,38 @@ +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. +# +# 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 + - 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..f43c2e1f5d 100644 --- a/docs/development/external-review.md +++ b/docs/development/external-review.md @@ -102,6 +102,12 @@ can't tell us whether it's wrong or whether we are. 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 whose files are all docs or test code passes. + Any other PR needs a `GATE: PASS` from the gate account at its exact head, and the + latest verdict at that head wins. The job does not count the Tier 2 second gate, + and it is not a required check until the operator makes it one. + ## Keeping a PR out of automatic review `.coderabbit.yaml` turns off the generated summary in PR descriptions. A PR diff --git a/scripts/__tests__/gate-verdict.test.mjs b/scripts/__tests__/gate-verdict.test.mjs new file mode 100644 index 0000000000..82460c061e --- /dev/null +++ b/scripts/__tests__/gate-verdict.test.mjs @@ -0,0 +1,147 @@ +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 two 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" ;; + *) 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 = 'Checked out HEAD and ran the focused tests.\n\nGATE: PASS'; +const FAIL = 'Found a defect in the rollback path.\n\nGATE: FAIL'; + +const CODE_FILES = ['backend/services/byoHandoffService.ts']; +const TIER0_FILES = [ + 'docs/development/external-review.md', + 'backend/__tests__/unit/services/byoHandoffService.test.ts', + 'CLAUDE.md', +]; + +function run({ files = [], reviews = [], env = {} } = {}) { + const fixtures = mkdtempSync(join(tmpdir(), 'gate-verdict-fx-')); + try { + const filesPath = join(fixtures, 'files.json'); + const reviewsPath = join(fixtures, 'reviews.json'); + writeFileSync(filesPath, JSON.stringify(files.map((filename) => ({ filename })))); + writeFileSync(reviewsPath, JSON.stringify(reviews)); + 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, + ...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('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', () => { + assert.equal(run({ files: CODE_FILES, reviews: [review('GATE: PASS\n\nFollow-up: none.')] }).status, 1); +}); + +test('trailing spaces and CRLF on the last line are ignored', () => { + assert.equal(run({ files: CODE_FILES, reviews: [review('ran the tests\r\n\r\nGATE: PASS \r\n')] }).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, /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 dismissed PASS does not count', () => { + assert.equal(run({ files: CODE_FILES, reviews: [review(PASS, { state: 'DISMISSED' })] }).status, 1); +}); + +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..b1353643cb --- /dev/null +++ b/scripts/gate-verdict.sh @@ -0,0 +1,50 @@ +#!/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 gate account +# has a review at the exact head whose last line is `GATE: PASS` and the latest verdict at +# that head is that PASS. Anything else fails, including an API error. +# +# 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?)$' + +files="$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename')" +if [ -n "$files" ] && [ -z "$(printf '%s\n' "$files" | grep -Ev "$EXEMPT_RE" || 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". +# Pending and dismissed reviews don't count. The text decides the verdict, not the review state. +verdicts="$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/reviews" --jq ' + .[] + | select((.user.login | ascii_downcase) == ($ENV.GATE_REVIEWER | ascii_downcase)) + | select(.commit_id == $ENV.HEAD_SHA and .state != "PENDING" and .state != "DISMISSED") + | [.id, .submitted_at, ((.body // "") | split("\n") | map(sub("\\s+$"; "")) | map(select(length > 0)) | (last // ""))] + | @tsv')" + +# Review ids rise with creation, so the highest id is the latest verdict. A FAIL after a PASS blocks it. +latest="$(printf '%s\n' "$verdicts" | awk -F '\t' '$3 ~ /^GATE: (PASS|FAIL)$/' | sort -t $'\t' -k1,1n | tail -n 1)" +review_id="${latest%%$'\t'*}" +verdict="${latest##*$'\t'}" + +if [ "$verdict" = "GATE: PASS" ]; then + echo "✓ $GATE_REVIEWER: GATE: PASS at $HEAD_SHA (review $review_id)" + exit 0 +fi + +if [ -z "$latest" ]; then + why="no GATE verdict from $GATE_REVIEWER at $HEAD_SHA" +else + why="the latest verdict from $GATE_REVIEWER at $HEAD_SHA (review $review_id) is '$verdict'" +fi +echo "::error::$why. 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 From d4ee025af761f91f0d3834ec110d63473c6a45f5 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 01:17:41 -0700 Subject: [PATCH 2/5] fix(gate): close the three HOLD defects in gate-verdict (TASK-595) - Renames check previous_filename too, so a code file moved into docs/ or a test directory is no longer Tier 0. - Outside docs/, SKILL.md files, skill trees, k8s/helm/commonly/configs/ and CLAUDE.md, AGENTS.md, REVIEW.md are behaviour, not docs. - The file list must match changed_files, or the job fails closed. - The last line that mentions GATE: decides, so a HOLD after a PASS blocks. Only an exact GATE: PASS passes, and the review must name the head SHA. - Footers and markup are removed before the last line is read, as the press reads it. - The checkout does not persist credentials. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: wren Agent-Model: claude/claude-haiku-5-5/max --- .github/workflows/gate-verdict.yml | 6 ++ docs/development/external-review.md | 26 ++++-- scripts/__tests__/gate-verdict.test.mjs | 111 ++++++++++++++++++++++-- scripts/gate-verdict.sh | 68 ++++++++++----- 4 files changed, 174 insertions(+), 37 deletions(-) diff --git a/.github/workflows/gate-verdict.yml b/.github/workflows/gate-verdict.yml index 04b1e2de40..f9b192f2c4 100644 --- a/.github/workflows/gate-verdict.yml +++ b/.github/workflows/gate-verdict.yml @@ -5,6 +5,10 @@ name: Gate verdict # 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: @@ -27,6 +31,8 @@ jobs: 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 diff --git a/docs/development/external-review.md b/docs/development/external-review.md index f43c2e1f5d..47c0758c75 100644 --- a/docs/development/external-review.md +++ b/docs/development/external-review.md @@ -93,20 +93,28 @@ 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 last line is `GATE: PASS`, and the press tooling reads only those. The last + line is taken after `Written by` and `Pod thread:` footers and `*` or backtick + markup are removed. `gate-verdict` also treats any last line that mentions `GATE:` + as a verdict, so a `GATE: HOLD` after a PASS at the same head blocks 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 whose files are all docs or test code passes. - Any other PR needs a `GATE: PASS` from the gate account at its exact head, and the - latest verdict at that head wins. The job does not count the Tier 2 second gate, - and it is not a required check until the operator makes it one. + 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 `GATE: PASS` from the gate account at its exact head that names the head SHA, + and 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 index 82460c061e..6c1bbb42c3 100644 --- a/scripts/__tests__/gate-verdict.test.mjs +++ b/scripts/__tests__/gate-verdict.test.mjs @@ -6,7 +6,7 @@ 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 two REST +// 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'); @@ -27,6 +27,7 @@ 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" @@ -40,23 +41,29 @@ 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 = 'Checked out HEAD and ran the focused tests.\n\nGATE: PASS'; +const PASS = `Checked out ${HEAD} and ran 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', - 'CLAUDE.md', + 'docs/adr/ADR-011-shell-first-pre-gtm.md', ]; -function run({ files = [], reviews = [], env = {} } = {}) { +// 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'); - writeFileSync(filesPath, JSON.stringify(files.map((filename) => ({ filename })))); + 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: { @@ -66,6 +73,7 @@ function run({ files = [], reviews = [], env = {} } = {}) { HEAD_SHA: HEAD, FAKE_FILES: filesPath, FAKE_REVIEWS: reviewsPath, + FAKE_PR: prPath, ...env, }, }); @@ -120,11 +128,12 @@ test('a PASS from another account does not count, CodeRabbit included', () => { }); test('GATE: PASS must be the last line of the review', () => { - assert.equal(run({ files: CODE_FILES, reviews: [review('GATE: PASS\n\nFollow-up: none.')] }).status, 1); + assert.equal(run({ files: CODE_FILES, reviews: [review(`GATE: PASS\n\nFollow-up: none.`)] }).status, 1); }); test('trailing spaces and CRLF on the last line are ignored', () => { - assert.equal(run({ files: CODE_FILES, reviews: [review('ran the tests\r\n\r\nGATE: PASS \r\n')] }).status, 0); + const body = `Checked out ${HEAD} and ran 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', () => { @@ -137,10 +146,98 @@ 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, /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 around the verdict is ignored, as the press reads it', () => { + const body = `Checked out ${HEAD} and ran the tests.\n\n**GATE: PASS**`; + assert.equal(run({ files: CODE_FILES, reviews: [review(body)] }).status, 0); +}); + +test('a footer below the verdict does not hide it', () => { + const pass = `Checked out ${HEAD} and ran the tests.\n\nGATE: PASS\n\nWritten by Sable, a Commonly agent`; + assert.equal(run({ files: CODE_FILES, reviews: [review(pass)] }).status, 0); + // A footer under a HOLD must not let the PASS before it stand. + const hold = '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: [review(PASS), review(hold)] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /is 'GATE: HOLD'/); +}); + +test('only the exact GATE: PASS passes, so near-misses block', () => { + for (const last of ['GATE: PASSED', 'GATE:PASS', 'gate: pass', 'GATE: PASS (with notes)']) { + const r = run({ files: CODE_FILES, reviews: [review(PASS), review(`Checked out ${HEAD}.\n\n${last}`)] }); + 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('Ran the focused tests.\n\nGATE: PASS')] }); + assert.equal(r.status, 1); + assert.match(r.stdout, /does not name the head SHA/); +}); + +test('a PASS that names only an older SHA does not count', () => { + const r = run({ files: CODE_FILES, reviews: [review(`Checked out ${OLD} and ran 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 r = run({ files: [{ filename: 'docs/old-service.md', 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', () => { + assert.equal(run({ files: [{ filename: 'docs/new.md', previous_filename: 'docs/old.md' }] }).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 index b1353643cb..c85d8118fb 100755 --- a/scripts/gate-verdict.sh +++ b/scripts/gate-verdict.sh @@ -1,9 +1,12 @@ #!/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 gate account -# has a review at the exact head whose last line is `GATE: PASS` and the latest verdict at -# that head is that PASS. Anything else fails, including an API error. +# 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 is `GATE: PASS` and names the head SHA. 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. @@ -15,36 +18,59 @@ 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)" -files="$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename')" -if [ -n "$files" ] && [ -z "$(printf '%s\n' "$files" | grep -Ev "$EXEMPT_RE" || true)" ]; then +# 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". +# One line per review by the gate account at this head: "\t\t\t". +# The last line is read the way the press reads it: footers and markup removed, blank lines skipped. # Pending and dismissed reviews don't count. The text decides the verdict, not the review state. verdicts="$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/reviews" --jq ' + def last_line: + split("\n") + | map(sub("\\s+$"; "") | gsub("[*`]"; "") | sub("^\\s+"; "")) + | map(select(test("^(written[ -]by|pod[ -]thread)"; "i") | not)) + | map(select(length > 0)) + | (last // ""); .[] | select((.user.login | ascii_downcase) == ($ENV.GATE_REVIEWER | ascii_downcase)) | select(.commit_id == $ENV.HEAD_SHA and .state != "PENDING" and .state != "DISMISSED") - | [.id, .submitted_at, ((.body // "") | split("\n") | map(sub("\\s+$"; "")) | map(select(length > 0)) | (last // ""))] + | (.body // "") as $body + | [.id, .submitted_at, ($body | last_line), + (if ($body | contains($ENV.HEAD_SHA | .[0:7])) then "yes" else "no" end)] | @tsv')" -# Review ids rise with creation, so the highest id is the latest verdict. A FAIL after a PASS blocks it. -latest="$(printf '%s\n' "$verdicts" | awk -F '\t' '$3 ~ /^GATE: (PASS|FAIL)$/' | sort -t $'\t' -k1,1n | tail -n 1)" -review_id="${latest%%$'\t'*}" -verdict="${latest##*$'\t'}" - -if [ "$verdict" = "GATE: PASS" ]; then - echo "✓ $GATE_REVIEWER: GATE: PASS at $HEAD_SHA (review $review_id)" - exit 0 +# Review ids rise with creation, so the highest id is the latest verdict. Any last line that +# mentions GATE: is a verdict, so a decorated HOLD after a PASS blocks it. +latest="$(printf '%s\n' "$verdicts" | awk -F '\t' 'tolower($3) ~ /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 -if [ -z "$latest" ]; then - why="no GATE verdict from $GATE_REVIEWER at $HEAD_SHA" -else - why="the latest verdict from $GATE_REVIEWER at $HEAD_SHA (review $review_id) is '$verdict'" +review_id="$(printf '%s\n' "$latest" | cut -f 1)" +verdict="$(printf '%s\n' "$latest" | cut -f 3)" +named="$(printf '%s\n' "$latest" | cut -f 4)" +if [ "$verdict" != "GATE: PASS" ]; then + echo "::error::the latest verdict from $GATE_REVIEWER at $HEAD_SHA (review $review_id) is '$verdict', not 'GATE: PASS'. A PASS at this head is needed." + exit 1 +fi +if [ "$named" != yes ]; then + echo "::error::the GATE: PASS from $GATE_REVIEWER (review $review_id) does not name the head SHA ${HEAD_SHA:0:7}. The verdict must say which commit it checked out." + exit 1 fi -echo "::error::$why. 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 +echo "✓ $GATE_REVIEWER: GATE: PASS at $HEAD_SHA (review $review_id)" From 010992f497c8d92c9d08f2bea5ce999c46df3169 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 01:31:12 -0700 Subject: [PATCH 3/5] fix(gate): a dot-directory under docs/ is not Tier 0 (TASK-595) The docs/ arm of EXEMPT_RE accepted any directory name, so docs/.claude/skills/x/SKILL.md was exempt. The PR states that a path through a dot-directory never counts as docs, so the arm now refuses dot-directories. A test pins both cases. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: wren Agent-Model: claude/claude-haiku-5-5/max --- scripts/__tests__/gate-verdict.test.mjs | 5 +++++ scripts/gate-verdict.sh | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/__tests__/gate-verdict.test.mjs b/scripts/__tests__/gate-verdict.test.mjs index 6c1bbb42c3..a2dd35f79a 100644 --- a/scripts/__tests__/gate-verdict.test.mjs +++ b/scripts/__tests__/gate-verdict.test.mjs @@ -92,6 +92,11 @@ 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', () => { + assert.equal(run({ files: ['docs/.claude/skills/x/SKILL.md'] }).status, 1); + assert.equal(run({ files: ['docs/.hidden/notes.md'] }).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); diff --git a/scripts/gate-verdict.sh b/scripts/gate-verdict.sh index c85d8118fb..9b13d4a974 100755 --- a/scripts/gate-verdict.sh +++ b/scripts/gate-verdict.sh @@ -17,7 +17,7 @@ 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?)$' +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$' From a6032aa9632659c4e8b9dec24bb38c9155d8c7c9 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 01:58:22 -0700 Subject: [PATCH 4/5] fix(gate): a PASS opens with GATE: PASS at the head, and a HOLD at either end blocks it (TASK-595) The verdict check read only the last line, and it accepted a PASS that named the head anywhere in the body. A review whose first line declared HOLD and whose last line was GATE: PASS passed. The first line now has to open with GATE: PASS and name the head's first seven characters, and the last line must be exactly GATE: PASS (review-checklist rules 44 and 51). A review that mentions GATE: at either end is a verdict, so a HOLD at either end blocks a PASS before it. Adds four cases, including the first-line HOLD, and updates external-review.md to match. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: wren Agent-Model: claude/claude-haiku-5-5/max --- docs/development/external-review.md | 17 ++++---- scripts/__tests__/gate-verdict.test.mjs | 56 +++++++++++++++++++------ scripts/gate-verdict.sh | 42 +++++++++---------- 3 files changed, 73 insertions(+), 42 deletions(-) diff --git a/docs/development/external-review.md b/docs/development/external-review.md index 47c0758c75..5811acc57a 100644 --- a/docs/development/external-review.md +++ b/docs/development/external-review.md @@ -99,19 +99,22 @@ can't tell us whether it's wrong or whether we are. 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. The last - line is taken after `Written by` and `Pod thread:` footers and `*` or backtick - markup are removed. `gate-verdict` also treats any last line that mentions `GATE:` - as a verdict, so a `GATE: HOLD` after a PASS at the same head blocks it. 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 last line is exactly `GATE: PASS` (review-checklist rules 44 and 51). The + press tooling reads the last line. Both lines are taken after `Written by` and + `Pod thread:` footers and `*` or backtick markup are removed. `gate-verdict` treats a + review whose first or last line mentions `GATE:` as a verdict, so a `GATE: HOLD` at + either end blocks a PASS before it at the same head. 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 `GATE: PASS` from the gate account at its exact head that names the head SHA, - and the latest verdict at that head wins. The job does not count the Tier 2 second + 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 last line 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. diff --git a/scripts/__tests__/gate-verdict.test.mjs b/scripts/__tests__/gate-verdict.test.mjs index a2dd35f79a..c8aefdf5ed 100644 --- a/scripts/__tests__/gate-verdict.test.mjs +++ b/scripts/__tests__/gate-verdict.test.mjs @@ -41,7 +41,7 @@ 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 = `Checked out ${HEAD} and ran the focused tests.\n\nGATE: PASS`; +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'; @@ -133,18 +133,19 @@ test('a PASS from another account does not count, CodeRabbit included', () => { }); test('GATE: PASS must be the last line of the review', () => { - assert.equal(run({ files: CODE_FILES, reviews: [review(`GATE: PASS\n\nFollow-up: none.`)] }).status, 1); + 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 = `Checked out ${HEAD} and ran the tests\r\n\r\nGATE: PASS \r\n`; + 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, /is 'GATE: FAIL'/); + assert.match(r.stdout, /last line is 'GATE: FAIL'/); }); test('a PASS after a FAIL at the same head passes', () => { @@ -154,7 +155,7 @@ test('a PASS after a FAIL at the same head passes', () => { 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, /is 'GATE: HOLD'/); + assert.match(r.stdout, /last line is 'GATE: HOLD'/); }); test('a PASS after a HOLD at the same head passes', () => { @@ -162,23 +163,24 @@ test('a PASS after a HOLD at the same head passes', () => { }); test('markup around the verdict is ignored, as the press reads it', () => { - const body = `Checked out ${HEAD} and ran the tests.\n\n**GATE: PASS**`; + const body = `**GATE: PASS @ ${HEAD}**\n\nRan the tests.\n\n**GATE: PASS**`; assert.equal(run({ files: CODE_FILES, reviews: [review(body)] }).status, 0); }); test('a footer below the verdict does not hide it', () => { - const pass = `Checked out ${HEAD} and ran the tests.\n\nGATE: PASS\n\nWritten by Sable, a Commonly agent`; + const pass = `GATE: PASS @ ${HEAD}\n\nRan the focused tests.\n\nGATE: PASS\n\nWritten by Sable, a Commonly agent`; assert.equal(run({ files: CODE_FILES, reviews: [review(pass)] }).status, 0); // A footer under a HOLD must not let the PASS before it stand. const hold = '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: [review(PASS), review(hold)] }); assert.equal(r.status, 1); - assert.match(r.stdout, /is 'GATE: HOLD'/); + assert.match(r.stdout, /last line is 'GATE: HOLD'/); }); -test('only the exact GATE: PASS passes, so near-misses block', () => { +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 r = run({ files: CODE_FILES, reviews: [review(PASS), review(`Checked out ${HEAD}.\n\n${last}`)] }); + 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); } }); @@ -188,13 +190,41 @@ test('a dismissed PASS does not count', () => { }); test('a bare PASS that names no head SHA does not count', () => { - const r = run({ files: CODE_FILES, reviews: [review('Ran the focused tests.\n\nGATE: PASS')] }); + 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, /does not name the head SHA/); + 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(`Checked out ${OLD} and ran the tests.\n\nGATE: PASS`)] }); + const r = run({ files: CODE_FILES, reviews: [review(`GATE: PASS @ ${OLD}\n\nRan the tests.\n\nGATE: PASS`)] }); assert.equal(r.status, 1); }); diff --git a/scripts/gate-verdict.sh b/scripts/gate-verdict.sh index 9b13d4a974..6e9ca09cc4 100755 --- a/scripts/gate-verdict.sh +++ b/scripts/gate-verdict.sh @@ -2,8 +2,9 @@ # 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 is `GATE: PASS` and names the head SHA. Anything -# else fails, including an API error or a file list shorter than the PR's changed_files. +# from the gate account at the exact head opens `GATE: PASS` and names the head SHA on its first +# line, and its last line 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. @@ -36,41 +37,38 @@ if [ "$listed" -gt 0 ] && [ "$listed" -eq "$expected" ] && exit 0 fi -# One line per review by the gate account at this head: "\t\t\t". -# The last line is read the way the press reads it: footers and markup removed, blank lines skipped. +# One line per review by the gate account at this head: "\t\t\t". +# Both ends are read the way the press reads them: footers and markup removed, blank lines skipped. # Pending and dismissed reviews don't count. The text decides the verdict, not the review state. verdicts="$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/reviews" --jq ' - def last_line: + def clean_lines: split("\n") | map(sub("\\s+$"; "") | gsub("[*`]"; "") | sub("^\\s+"; "")) - | map(select(test("^(written[ -]by|pod[ -]thread)"; "i") | not)) - | map(select(length > 0)) - | (last // ""); + | 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 - | [.id, .submitted_at, ($body | last_line), - (if ($body | contains($ENV.HEAD_SHA | .[0:7])) then "yes" else "no" end)] + | (.body // "" | clean_lines) as $lines + | [.id, .submitted_at, ($lines | .[0] // ""), ($lines | .[-1] // "")] | @tsv')" -# Review ids rise with creation, so the highest id is the latest verdict. Any last line that -# mentions GATE: is a verdict, so a decorated HOLD after a PASS blocks it. -latest="$(printf '%s\n' "$verdicts" | awk -F '\t' 'tolower($3) ~ /gate:/' | sort -t $'\t' -k1,1n | tail -n 1)" +# 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)" -verdict="$(printf '%s\n' "$latest" | cut -f 3)" -named="$(printf '%s\n' "$latest" | cut -f 4)" -if [ "$verdict" != "GATE: PASS" ]; then - echo "::error::the latest verdict from $GATE_REVIEWER at $HEAD_SHA (review $review_id) is '$verdict', not 'GATE: PASS'. A PASS at this head is needed." - exit 1 -fi -if [ "$named" != yes ]; then - echo "::error::the GATE: PASS from $GATE_REVIEWER (review $review_id) does not name the head SHA ${HEAD_SHA:0:7}. The verdict must say which commit it checked out." +first="$(printf '%s\n' "$latest" | cut -f 3)" +last="$(printf '%s\n' "$latest" | cut -f 4)" +# A PASS opens with the words GATE: PASS and names the head's first seven characters, and its last +# line is exactly GATE: PASS. Both ends are needed, as review-checklist rules 44 and 51 require. +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'." exit 1 fi echo "✓ $GATE_REVIEWER: GATE: PASS at $HEAD_SHA (review $review_id)" From dee918a597f97d9162759f9d6e82c36f28a55c46 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 02:17:45 -0700 Subject: [PATCH 5/5] fix(gate): read the stored last line for PASS (TASK-595) Rule 51 has the press parse the final non-blank line of the stored body, so a bold sentinel or a credit line below GATE: PASS is not a pass. The PASS check now reads that raw line. Verdict detection still strips footers and markup at both ends, so a footer under a HOLD cannot hide it. The docs fixtures in the test file are built with join(), as docReferences.test.js expects, so the doc-reference scan does not treat them as pointers to files that are not there. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: wren Agent-Model: claude/claude-haiku-5-5/max --- docs/development/external-review.md | 15 +++++----- scripts/__tests__/gate-verdict.test.mjs | 39 ++++++++++++++++--------- scripts/gate-verdict.sh | 27 ++++++++++------- 3 files changed, 50 insertions(+), 31 deletions(-) diff --git a/docs/development/external-review.md b/docs/development/external-review.md index 5811acc57a..6059f2180b 100644 --- a/docs/development/external-review.md +++ b/docs/development/external-review.md @@ -100,12 +100,13 @@ can't tell us whether it's wrong or whether we are. 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 first line opens with `GATE: PASS` and names the head's first seven characters, - and whose last line is exactly `GATE: PASS` (review-checklist rules 44 and 51). The - press tooling reads the last line. Both lines are taken after `Written by` and - `Pod thread:` footers and `*` or backtick markup are removed. `gate-verdict` treats a - review whose first or last line mentions `GATE:` as a verdict, so a `GATE: HOLD` at - either end blocks a PASS before it at the same head. A CodeRabbit review is never a - verdict. It only lets a docs-only or test-only PR skip the seat gate. + 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 @@ -113,7 +114,7 @@ can't tell us whether it's wrong or whether we are. 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 last line is `GATE: PASS`. The latest + `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 diff --git a/scripts/__tests__/gate-verdict.test.mjs b/scripts/__tests__/gate-verdict.test.mjs index c8aefdf5ed..2543f1ee07 100644 --- a/scripts/__tests__/gate-verdict.test.mjs +++ b/scripts/__tests__/gate-verdict.test.mjs @@ -93,8 +93,10 @@ test('a dot-directory markdown file is not docs: it needs a gate', () => { }); test('a dot-directory under docs is not docs either: it needs a gate', () => { - assert.equal(run({ files: ['docs/.claude/skills/x/SKILL.md'] }).status, 1); - assert.equal(run({ files: ['docs/.hidden/notes.md'] }).status, 1); + // 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', () => { @@ -162,19 +164,28 @@ 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 around the verdict is ignored, as the press reads it', () => { +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**`; - assert.equal(run({ files: CODE_FILES, reviews: [review(body)] }).status, 0); + 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 footer below the verdict does not hide it', () => { - const pass = `GATE: PASS @ ${HEAD}\n\nRan the focused tests.\n\nGATE: PASS\n\nWritten by Sable, a Commonly agent`; - assert.equal(run({ files: CODE_FILES, reviews: [review(pass)] }).status, 0); - // A footer under a HOLD must not let the PASS before it stand. - const hold = '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: [review(PASS), review(hold)] }); +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 'GATE: HOLD'/); + 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', () => { @@ -229,7 +240,8 @@ test('a PASS that names only an older SHA does not count', () => { }); test('a rename out of a code path into docs is not Tier 0', () => { - const r = run({ files: [{ filename: 'docs/old-service.md', previous_filename: 'backend/services/foo.ts' }] }); + 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/); }); @@ -240,7 +252,8 @@ test('a rename out of a code path into a test directory is not Tier 0', () => { }); test('a rename within docs stays Tier 0', () => { - assert.equal(run({ files: [{ filename: 'docs/new.md', previous_filename: 'docs/old.md' }] }).status, 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', () => { diff --git a/scripts/gate-verdict.sh b/scripts/gate-verdict.sh index 6e9ca09cc4..d96307c5ae 100755 --- a/scripts/gate-verdict.sh +++ b/scripts/gate-verdict.sh @@ -3,8 +3,8 @@ # # 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 last line is `GATE: PASS`. Anything else fails, including an API error or a file -# list shorter than the PR's changed_files. +# 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. @@ -37,9 +37,12 @@ if [ "$listed" -gt 0 ] && [ "$listed" -eq "$expected" ] && exit 0 fi -# One line per review by the gate account at this head: "\t\t\t". -# Both ends are read the way the press reads them: footers and markup removed, blank lines skipped. -# Pending and dismissed reviews don't count. The text decides the verdict, not the review state. +# 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") @@ -48,8 +51,10 @@ verdicts="$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/reviews" --jq ' .[] | select((.user.login | ascii_downcase) == ($ENV.GATE_REVIEWER | ascii_downcase)) | select(.commit_id == $ENV.HEAD_SHA and .state != "PENDING" and .state != "DISMISSED") - | (.body // "" | clean_lines) as $lines - | [.id, .submitted_at, ($lines | .[0] // ""), ($lines | .[-1] // "")] + | (.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 @@ -62,13 +67,13 @@ 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 4)" -# A PASS opens with the words GATE: PASS and names the head's first seven characters, and its last -# line is exactly GATE: PASS. Both ends are needed, as review-checklist rules 44 and 51 require. +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'." + 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)"