diff --git a/.github/scripts/run-quality-gates.mjs b/.github/scripts/run-quality-gates.mjs index 555092893c56..dbf2a2d499ea 100644 --- a/.github/scripts/run-quality-gates.mjs +++ b/.github/scripts/run-quality-gates.mjs @@ -31,7 +31,7 @@ export function isGraphifyReindexArtifactOnlyPr({ author, branch, files }) { ); } -function buildComment(author, failures, informational) { +export function buildComment(author, failures, informational) { if (failures.length === 0 && informational.length === 0) { return `✅ All checks passing — ready for Greptile review and maintainer approval.\n\n${COMMENT_SIGNATURE}`; } @@ -52,7 +52,11 @@ function buildComment(author, failures, informational) { } lines.push( - '\nOnce updated, push a new commit and these checks will re-run automatically.\n', + // Most of what this gate fails on lives in the PR description or title, + // and `pull_request_target: edited` re-fires the check for those — so do + // not send the author to push a no-op commit to re-trigger a body check + // (BLO-26636). + '\nOnce updated, these checks re-run automatically: editing the PR description or title re-triggers them, as does pushing a new commit.\n', COMMENT_SIGNATURE ); @@ -66,9 +70,26 @@ export async function findExistingComment(fetchFromGitHub, token, repo, prNumber token ); + // Match on the signature this script itself writes plus `type === 'Bot'`, + // NOT on a hard-coded App login. get-bot-token.mjs resolves the App from + // COMMITPERCLIP_APP_ID by design, so the author is `commitperclip[bot]` + // upstream and `allyblockcast[bot]` on Blockcast — the old allowlist + // hard-coded the upstream login, so on Blockcast `existing` was + // permanently null: failing runs POSTed a duplicate every time, and + // passing runs fell through the `|| existing` guard in main() and left the + // stale failure comment standing forever (BLO-26636). `type === 'Bot'` is + // what keeps a human comment quoting the signature out of the PATCH path. + // + // `endsWith`, not `includes`: on Blockcast every agent posts as + // `allyblockcast[bot]`, so `type === 'Bot'` discriminates nothing against + // an agent comment that merely *quotes* `— commitperclip` (they routinely + // do). Since `.find` returns the first match in ascending id order, such a + // comment posted before ours would take the PATCH — destroying content + // that is not ours and leaving our stale comment standing. buildComment + // puts the signature last in both branches, so anchoring on that is the + // property we actually own. const existing = comments.find( - c => (c.user.login === 'commitperclip[bot]' || c.user.login === 'commitperclip') && - c.body.includes(COMMENT_SIGNATURE) + c => c.user?.type === 'Bot' && (c.body ?? '').trimEnd().endsWith(COMMENT_SIGNATURE) ); if (existing) return existing; diff --git a/.github/scripts/tests/run-quality-gates.test.mjs b/.github/scripts/tests/run-quality-gates.test.mjs index 58e141dc41e4..45fe3a9796a0 100644 --- a/.github/scripts/tests/run-quality-gates.test.mjs +++ b/.github/scripts/tests/run-quality-gates.test.mjs @@ -1,10 +1,22 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; import { + buildComment, findExistingComment, isGraphifyReindexArtifactOnlyPr, } from '../run-quality-gates.mjs'; +const workflow = readFileSync( + path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '../../workflows/commitperclip-review.yml', + ), + 'utf8', +); + test('findExistingComment: paginates until it finds the commitperclip comment', async () => { const seenPaths = []; const comment = await findExistingComment(async (path) => { @@ -12,14 +24,14 @@ test('findExistingComment: paginates until it finds the commitperclip comment', if (path.endsWith('page=1')) { return Array.from({ length: 100 }, (_, index) => ({ id: index + 1, - user: { login: 'someone-else' }, + user: { login: 'someone-else', type: 'User' }, body: 'unrelated', })); } if (path.endsWith('page=2')) { return [{ id: 200, - user: { login: 'commitperclip[bot]' }, + user: { login: 'commitperclip[bot]', type: 'Bot' }, body: 'Looks good.\n\n— commitperclip', }]; } @@ -37,7 +49,7 @@ test('findExistingComment: returns null when no signed comment exists', async () const comment = await findExistingComment(async () => ([ { id: 1, - user: { login: 'commitperclip[bot]' }, + user: { login: 'commitperclip[bot]', type: 'Bot' }, body: 'Unsigned status update', }, ]), 'token', 'paperclipai/paperclip', 6469); @@ -45,6 +57,155 @@ test('findExistingComment: returns null when no signed comment exists', async () assert.equal(comment, null); }); +// BLO-26636. The old predicate allowlisted the literal login +// `commitperclip[bot]`, but get-bot-token.mjs resolves whichever App +// COMMITPERCLIP_APP_ID points at — `allyblockcast[bot]` on Blockcast. So +// `existing` was permanently null there: every failing run POSTed a duplicate, +// and a passing run skipped the write entirely, stranding the failure comment. +// This fixture is the one the old code returns undefined for. +test('findExistingComment: matches whichever App posted it, not a hard-coded login', async () => { + const comment = await findExistingComment(async () => ([ + { + id: 5654012482, + user: { login: 'allyblockcast[bot]', type: 'Bot' }, + body: 'Hey @someone! Before this PR can be reviewed…\n\n— commitperclip', + }, + ]), 'token', 'Blockcast/paperclip', 1828); + + assert.equal(comment.id, 5654012482); +}); + +// `type === 'Bot'` is the whole guard against the PATCH path overwriting a +// human's comment — repo-write identities can edit others' comments via the +// API, so a human quoting the signature must not be treated as ours. +test('findExistingComment: ignores a human comment carrying the signature', async () => { + const comment = await findExistingComment(async () => ([ + { + id: 9, + user: { login: 'kkroo', type: 'User' }, + body: 'Quoting the bot here:\n\n— commitperclip', + }, + ]), 'token', 'Blockcast/paperclip', 1828); + + assert.equal(comment, null); +}); + +// On Blockcast every agent posts as `allyblockcast[bot]`, so `type === 'Bot'` +// cannot tell our gate comment from an agent comment that quotes the +// signature — and `.find` takes the first match in id order, so a quoting +// comment posted first would get PATCHed over. The signature is last in both +// buildComment branches; anchor there. +test('findExistingComment: ignores a bot comment that merely quotes the signature', async () => { + const comment = await findExistingComment(async () => ([ + { + id: 11, + user: { login: 'allyblockcast[bot]', type: 'Bot' }, + body: 'The old code matched `— commitperclip` by login.\n\nSee the diff above.', + }, + { + id: 12, + user: { login: 'allyblockcast[bot]', type: 'Bot' }, + body: 'Hey @someone! Before this PR can be reviewed…\n\n— commitperclip', + }, + ]), 'token', 'Blockcast/paperclip', 1889); + + assert.equal(comment.id, 12); +}); + +// Pins `.find` over `findLast` (BLO-26636). Both pass every other fixture, so +// nothing else in this suite would notice the swap. Our comment is posted by +// the first failing run, i.e. near PR open, and a bot comment quoting it can +// only exist after it does — so oldest-match is the genuine one and +// newest-match is the paste. Adopting the paste PATCHes over another agent's +// comment, which recurs; picking the older of two genuine gate comments only +// mattered for pre-fix duplicates. Those were deleted from the affected open +// PRs by a manual sweep, but the producing bug runs on `master` until this +// merges and keeps minting more (#1933 took two inside eleven minutes) — so +// sweep once more at merge time rather than treating the earlier sweep as +// durable. After merge the class cannot recur. +test('findExistingComment: adopts the oldest gate comment, not the newest', async () => { + const comment = await findExistingComment(async () => ([ + { + id: 20, + user: { login: 'allyblockcast[bot]', type: 'Bot' }, + body: 'Hey @someone! Before this PR can be reviewed…\n\n— commitperclip', + }, + { + id: 21, + user: { login: 'allyblockcast[bot]', type: 'Bot' }, + body: 'Quoting the gate verbatim:\n\n> Hey @someone!\n\n— commitperclip', + }, + ]), 'token', 'Blockcast/paperclip', 1889); + + assert.equal(comment.id, 20); +}); + +// Every other fixture here is a hand-written literal, so none of them pin the +// cross-function coupling the whole fix rests on: findExistingComment anchors +// on `endsWith(COMMENT_SIGNATURE)`, which is only correct while buildComment +// keeps the signature last in *both* branches. Append a footer after the +// signature — the edit anyone adding a run link would make — and `existing` +// goes permanently null again: failing runs POST a duplicate and passing runs +// fall through the `|| existing` guard, leaving the stale red comment up. That +// is the BLO-26636 defect restored, with the rest of this suite fully green. +test('findExistingComment: matches what buildComment actually produces', async () => { + for (const body of [ + buildComment('someone', ['Missing section: **## Risks**'], []), + buildComment('someone', [], []), + ]) { + const comment = await findExistingComment(async () => ([ + { id: 1, user: { login: 'allyblockcast[bot]', type: 'Bot' }, body }, + ]), 'token', 'Blockcast/paperclip', 1889); + + assert.equal(comment?.id, 1); + } +}); + +test('findExistingComment: tolerates a comment with no body', async () => { + const comment = await findExistingComment(async () => ([ + { id: 13, user: { login: 'allyblockcast[bot]', type: 'Bot' }, body: null }, + ]), 'token', 'Blockcast/paperclip', 1889); + + assert.equal(comment, null); +}); + +// BLO-26636. The failure the gate reports most often is a body/title +// violation, and the old text sent the author to push a commit — advice that +// re-runs nothing for a body edit and burns a CI matrix when followed. +test('buildComment: names editing the description, not just pushing a commit', () => { + const body = buildComment('someone', ['Missing section: **## Risks**'], []); + + assert.match(body, /editing the PR description or title/); + assert.doesNotMatch(body, /push a new commit and these checks will re-run/); +}); + +// The remedy above is only true because the workflow listens for `edited`. +// These assertions have to move together or the comment starts lying. +test('commitperclip-review: listens for edited without displacing the original triggers', () => { + for (const type of ['opened', 'synchronize', 'reopened', 'edited']) { + assert.match(workflow, new RegExp(`types:\\s*\\[[^\\]]*\\b${type}\\b`)); + } + assert.match(workflow, /merge_group:\s*\n\s*types:\s*\[checks_requested\]/); +}); + +// pull_request_target hands secrets to a job triggered by an untrusted fork +// PR, so the base-branch checkout is what makes adding a trigger type safe at +// all. If this ever flips to the PR head, `edited` stops being a one-line +// change and becomes an arbitrary-code-execution path. The count is what bounds +// the blacklist problem: enumerating `ref:` spellings can only ever chase an +// unbounded set (`github.head_ref`, `refs/pull/`, `env.PR_HEAD_SHA`, +// `merge_commit_sha`, …). Asserting there is exactly one checkout, and that it +// is `master`, rejects every second *checkout step* regardless of how its +// `ref:` is written. Ceiling: a `run:` step fetching PR code itself (`git fetch +// origin pull/N/head`), or a third-party action taking its own `ref:` input, is +// outside any guard that reads this workflow as text. Neither exists today — +// the only `uses:` steps are checkout, dependency-review-action and setup-node +// — so if you add one, this test will not stop you. +test('commitperclip-review: still checks out master, never PR code', () => { + assert.match(workflow, /uses:\s*actions\/checkout@[^\n]*\n\s*with:\s*\n\s*ref:\s*master/); + assert.equal((workflow.match(/uses:\s*actions\/checkout@/g) ?? []).length, 1); +}); + test('isGraphifyReindexArtifactOnlyPr: permits generated graphify reindex PRs', () => { const result = isGraphifyReindexArtifactOnlyPr({ author: 'allyblockcast[bot]', diff --git a/.github/workflows/commitperclip-review.yml b/.github/workflows/commitperclip-review.yml index e780ddf7654b..a165b8caf5f0 100644 --- a/.github/workflows/commitperclip-review.yml +++ b/.github/workflows/commitperclip-review.yml @@ -2,7 +2,29 @@ name: commitperclip PR Review on: pull_request_target: - types: [opened, synchronize, reopened] + # `edited` is load-bearing, not optional (BLO-26636). Three of the gates + # below read only the PR body and title (checkTemplate, checkLinkedIssue, + # checkDedupSearch), and the failure comment asks the author to fix exactly + # those. Without `edited` that fix fires no event, so the check stays red + # until somebody hand-re-runs the job; measured instances sat red 7h, 10h, + # ~21h and ~7h across #1328, #1359, #1696 and #1873. + # + # Deliberately NOT filtered down to `github.event.changes.body != null`. + # `edited` also fires on a base-branch retarget, and a job-level `if` that + # skips those publishes a check-run with conclusion `skipped` for the same + # head — which every downstream reader scores as non-failing, so a retarget + # would silently clear a genuinely red gate. The run is cheap enough that + # the filter is not worth that: 20 recent pull_request_target runs measured + # 25-133s (median ~60s) on arc-light. + # + # Known residual: `edited` makes same-head re-runs routine, and GitHub adds + # a check-run rather than replacing the old one, so a fixed body leaves a + # stale `review: failure` row beside the new success at that SHA. Branch + # protection collapses to latest-per-name and is unaffected, but a reader + # that enumerates check-runs keyed on (name, run id) still scores the dead + # row as a stop. The fix belongs reader-side — collapse same-name re-runs + # to the latest run id — not here. Tracked on BLO-26636. + types: [opened, synchronize, reopened, edited] merge_group: types: [checks_requested] @@ -47,10 +69,12 @@ jobs: # without letting a genuinely hung job drain a runner. timeout-minutes: 10 env: - # Blockcast does not provision the upstream commitperclip bot key. Skip - # the bot steps (token + quality/security gates) when it is absent so - # Dependency Review still gates every PR instead of the whole job - # failing at token generation. + # Skip the bot steps (token + quality/security gates) when no App key is + # provisioned, so Dependency Review still gates every PR instead of the + # whole job failing at token generation. Blockcast DOES provision one — + # it points COMMITPERCLIP_APP_ID at the `allyblockcast` App rather than + # upstream's `commitperclip` (see get-bot-token.mjs), so these steps run + # here and the gate comment is authored by `allyblockcast[bot]`. COMMITPERCLIP_ENABLED: ${{ secrets.COMMITPERCLIP_KEY != '' }} steps: - name: Checkout base branch (never PR or queue-candidate code) @@ -90,13 +114,12 @@ jobs: base-ref: ${{ env.PR_BASE_SHA }} head-ref: ${{ env.PR_HEAD_SHA }} - # Only the commitperclip bot steps below use Node. On Blockcast the bot - # key is absent (COMMITPERCLIP_ENABLED=false), so setting up Node here - # would download the toolcache purely to leave it unused — the main cost - # that pushes this job past its cap. Gate it behind the same flag as its - # consumers: upstream (key present) still installs Node, Blockcast skips. - # Dependency Review runs as a JS action on the runner's own node20, so it - # does not depend on this step. + # Only the commitperclip bot steps below use Node. Where no App key is + # provisioned (COMMITPERCLIP_ENABLED=false) those steps are skipped, so + # setting up Node would download the toolcache purely to leave it unused + # — the main cost that pushes this job past its cap. Gate it behind the + # same flag as its consumers. Dependency Review runs as a JS action on + # the runner's own node20, so it does not depend on this step. - name: Set up Node if: env.COMMITPERCLIP_ENABLED == 'true' uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 57c65906667a..ab2de634271b 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -376,6 +376,18 @@ jobs: run: node --test ./.github/scripts/tests/check-pr-test-coverage.test.mjs timeout-minutes: 1 + # BLO-26636. This suite existed but was wired into no workflow, so it had + # never run — which is exactly why the two defects it now pins survived: + # a login allowlist that matched neither deployment's App (so the gate + # could never update its own comment), and a `pull_request_target` trigger + # list missing `edited` (so the fix the gate asks for re-ran nothing). + # Both are properties of the commitperclip gate that only fail on a live + # PR, days later, as a stalled review. + - name: Test commitperclip quality-gate orchestrator (BLO-26636) + if: ${{ !cancelled() }} + run: node --test ./.github/scripts/tests/run-quality-gates.test.mjs + timeout-minutes: 1 + # Guards the one property of the BLO-28888 soak workflow that cannot be # re-checked after the fact: that it stays dispatch-only. Adding its # ~50-90 minutes to a per-PR trigger would be invisible until it was