From d6c8a54d625d384f153bdeab65ada73dc586595c Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Wed, 16 Sep 2026 16:10:41 +0000 Subject: [PATCH 1/8] fix(ci): re-run the commitperclip review gate on PR-body edits (BLO-26636) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `review` gate fails PRs for missing template sections, then tells the author to fix the PR description — but `pull_request_target` did not list `edited`, so that fix fired no event and the check stayed red until somebody hand-ran `gh run rerun`. Measured instances sat red 7h (#1328), 10h (#1359), ~21h (#1696) and ~7h (#1873). `review` is not a required check, so there is no queue-side retry either. Adding `edited` is safe: the job already checks out `master` and never executes PR code. Deliberately not filtered to `changes.body != null` — a job-level `if` would publish a `skipped` check-run on a base retarget, which every downstream reader scores as non-failing, silently clearing a red gate. 20 recent runs measured 25-133s, so the filter is not worth that risk. Also fixes the compounding defect that survives the trigger fix: `findExistingComment` allowlisted the literal login `commitperclip[bot]`, but get-bot-token.mjs resolves whichever App COMMITPERCLIP_APP_ID names — here `allyblockcast[bot]`. `existing` was therefore permanently null, so failing runs POSTed a duplicate each time and passing runs skipped the write entirely, stranding a "not ready for review" comment beside a green check. Matching on the script's own signature + `user.type === 'Bot'` cannot rot when the App slug changes. Verified against live payloads for #1869/#1828/#1873/#1696/#1359: old predicate null on all five, new predicate resolves each comment id. The test suite that pins this existed but was wired into no workflow, so it had never run — that is why both defects survived. Wired into pr.yml. Co-Authored-By: Paperclip --- .github/scripts/run-quality-gates.mjs | 20 ++++- .../scripts/tests/run-quality-gates.test.mjs | 82 ++++++++++++++++++- .github/workflows/commitperclip-review.yml | 39 ++++++--- .github/workflows/pr.yml | 12 +++ 4 files changed, 134 insertions(+), 19 deletions(-) diff --git a/.github/scripts/run-quality-gates.mjs b/.github/scripts/run-quality-gates.mjs index 555092893c56..bfb535e3e991 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,17 @@ 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. 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.includes(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..d3026bbf7fb4 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,70 @@ 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); +}); + +// 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 two assertions have to move together or the comment starts lying. +test('commitperclip-review: pull_request_target listens for edited', () => { + assert.match(workflow, /types:\s*\[opened,\s*synchronize,\s*reopened,\s*edited\]/); +}); + +test('commitperclip-review: edited does not displace the original triggers', () => { + for (const type of ['opened', 'synchronize', 'reopened']) { + 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. +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/); +}); + 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..4ce396ecfa79 100644 --- a/.github/workflows/commitperclip-review.yml +++ b/.github/workflows/commitperclip-review.yml @@ -2,7 +2,21 @@ 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. + types: [opened, synchronize, reopened, edited] merge_group: types: [checks_requested] @@ -47,10 +61,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 +106,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 From 215e9cbb1ac8631ee5d84e0c45a156ced2e6591c Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Thu, 17 Sep 2026 00:47:28 +0000 Subject: [PATCH 2/8] fix(ci): anchor the gate-comment match on the trailing signature (BLO-26636) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding on #1889: `user.type === 'Bot'` discriminates nothing on this deployment. The gate comment is authored by `allyblockcast[bot]`, the same shared identity every Paperclip agent posts under, so an agent comment that merely quotes `— commitperclip` — which agents discussing this gate routinely do, including this PR's own description — also matches. `.find` returns the first match in ascending id order, so a quoting comment posted before ours takes the PATCH: it destroys content that is not ours and leaves our stale comment standing. Strictly worse than the duplicate-POST bug being fixed. buildComment puts the signature last in both branches, so `trimEnd().endsWith` anchors on a property the script itself guarantees and stays App-agnostic. `?? ''` also stops a null body throwing. Both existing gate-comment fixtures still resolve and the human `kkroo` fixture still returns null; two new fixtures pin the quoting-bot and null-body cases. 15/15 pass. Also records the second finding as a known residual in the workflow: `edited` makes same-head re-runs routine, and GitHub adds a check-run rather than replacing one, so a fixed body leaves a stale `review: failure` row beside the new success. Branch protection collapses to latest-per-name and is unaffected; a reader keyed on (name, run id) is not. That fix is reader-side, not here. Co-Authored-By: Paperclip --- .github/scripts/run-quality-gates.mjs | 11 ++++++- .../scripts/tests/run-quality-gates.test.mjs | 31 ++++++++++++++++++- .github/workflows/commitperclip-review.yml | 8 +++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/.github/scripts/run-quality-gates.mjs b/.github/scripts/run-quality-gates.mjs index bfb535e3e991..dbf2a2d499ea 100644 --- a/.github/scripts/run-quality-gates.mjs +++ b/.github/scripts/run-quality-gates.mjs @@ -79,8 +79,17 @@ export async function findExistingComment(fetchFromGitHub, token, repo, prNumber // 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?.type === 'Bot' && 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 d3026bbf7fb4..7186605af33e 100644 --- a/.github/scripts/tests/run-quality-gates.test.mjs +++ b/.github/scripts/tests/run-quality-gates.test.mjs @@ -90,7 +90,36 @@ test('findExistingComment: ignores a human comment carrying the signature', asyn assert.equal(comment, null); }); -// BLO-26636. The failure the gate reports most often is a body/title +// 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); +}); + +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); +}); + // 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', () => { diff --git a/.github/workflows/commitperclip-review.yml b/.github/workflows/commitperclip-review.yml index 4ce396ecfa79..a165b8caf5f0 100644 --- a/.github/workflows/commitperclip-review.yml +++ b/.github/workflows/commitperclip-review.yml @@ -16,6 +16,14 @@ on: # 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] From 55aa25ad1eba00f447e8258b75a607f078daedcc Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Thu, 17 Sep 2026 00:48:17 +0000 Subject: [PATCH 3/8] test(ci): drop the brittle exact-order trigger assertion (BLO-26636) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review suggestions 2 and 3 on #1889, both net simplifications. The exact-order regex `[opened, synchronize, reopened, edited]` duplicated the per-type membership loop beside it and was strictly more brittle — a valid reorder or a block-style conversion failed it while behaviour was unchanged. Folded `edited` into the loop and deleted the regex; two tests become one. The checkout guard pinned "the first actions/checkout has ref: master", which stays green if a *second* step checking out PR code is added. Added the negative form, which no added step can bypass. Negative-controlled: the regex matches `ref: ${{ github.event.pull_request.head.sha }}`, and passes against the current file. 14/14 pass. Co-Authored-By: Paperclip --- .github/scripts/tests/run-quality-gates.test.mjs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/.github/scripts/tests/run-quality-gates.test.mjs b/.github/scripts/tests/run-quality-gates.test.mjs index 7186605af33e..f69fec403426 100644 --- a/.github/scripts/tests/run-quality-gates.test.mjs +++ b/.github/scripts/tests/run-quality-gates.test.mjs @@ -130,13 +130,9 @@ test('buildComment: names editing the description, not just pushing a commit', ( }); // The remedy above is only true because the workflow listens for `edited`. -// These two assertions have to move together or the comment starts lying. -test('commitperclip-review: pull_request_target listens for edited', () => { - assert.match(workflow, /types:\s*\[opened,\s*synchronize,\s*reopened,\s*edited\]/); -}); - -test('commitperclip-review: edited does not displace the original triggers', () => { - for (const type of ['opened', 'synchronize', 'reopened']) { +// 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\]/); @@ -145,9 +141,12 @@ test('commitperclip-review: edited does not displace the original triggers', () // 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. +// change and becomes an arbitrary-code-execution path. The negative form is +// the load-bearing one: the positive match only pins the first checkout, so +// adding a *second* step that checks out PR code would slip past it. 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.doesNotMatch(workflow, /ref:\s*\$\{\{[^}]*\bhead\b/); }); test('isGraphifyReindexArtifactOnlyPr: permits generated graphify reindex PRs', () => { From 3b862e5572182c6d38b1329d7a0b2e0ea6d44383 Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Thu, 17 Sep 2026 07:19:07 +0000 Subject: [PATCH 4/8] test(ci): catch head_ref and refs/pull in the ACE guard (BLO-26636) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard's negative assertion ended `\bhead\b`, but `head_ref` continues with `_` — a word character — so `ref: ${{ github.head_ref }}`, the canonical pull_request_target footgun and the shortest edit anyone would reach for, slipped straight through. `refs/pull//head` was missed too. Drop the trailing `\b` and add a second assertion for the refs/pull form. Mutation-tested by appending a second checkout step to the workflow, one attack form per run: the new guard CAUGHT all four (head_ref, head.sha, head.ref, refs/pull); reverted to the old regex, two MISSED. No false positive on `ref: master`, `head-ref:` or `base-ref:`. No runtime change — the shipped workflow checks out master and was correct throughout. The defect was a guard that would stay green through the vulnerability it advertised covering. Also restores the first line of the buildComment test comment, dropped in ddb2320, which left the block starting mid-sentence at "violation, and". Co-Authored-By: Claude --- .github/scripts/tests/run-quality-gates.test.mjs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/.github/scripts/tests/run-quality-gates.test.mjs b/.github/scripts/tests/run-quality-gates.test.mjs index f69fec403426..bf78a546faa6 100644 --- a/.github/scripts/tests/run-quality-gates.test.mjs +++ b/.github/scripts/tests/run-quality-gates.test.mjs @@ -120,6 +120,7 @@ test('findExistingComment: tolerates a comment with no body', async () => { 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', () => { @@ -141,12 +142,15 @@ test('commitperclip-review: listens for edited without displacing the original t // 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 negative form is -// the load-bearing one: the positive match only pins the first checkout, so -// adding a *second* step that checks out PR code would slip past it. +// change and becomes an arbitrary-code-execution path. The negative forms are +// the load-bearing ones: the positive match only pins the first checkout, so +// adding a *second* step that checks out PR code would slip past it. No `\b` +// after `head` — `head_ref` continues with a word character, so anchoring the +// far end would miss `${{ github.head_ref }}`, the canonical footgun. 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.doesNotMatch(workflow, /ref:\s*\$\{\{[^}]*\bhead\b/); + assert.doesNotMatch(workflow, /ref:\s*\$\{\{[^}]*\bhead/); + assert.doesNotMatch(workflow, /ref:\s*[^\n]*refs\/pull\//); }); test('isGraphifyReindexArtifactOnlyPr: permits generated graphify reindex PRs', () => { From 5d1baadb8a4682cbd2531dcab6777bee6250ffa7 Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Thu, 17 Sep 2026 15:23:17 +0000 Subject: [PATCH 5/8] test(ci): bound the checkout guard by count instead of ref blacklist (BLO-26636) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two `doesNotMatch` forms chased an unbounded set of `ref:` spellings and had already been widened twice. They missed `${{ env.PR_HEAD_SHA }}` — the variable this workflow defines itself at commitperclip-review.yml:61 and the shortest thing an editor would reach for — and `merge_commit_sha`. Assert instead that exactly one `actions/checkout` exists. With the positive assertion pinning that one to `ref: master`, every second-checkout spelling fails regardless of how its `ref:` is written. Net deletion of one line. Mutation-tested, one revert per run: splicing a second checkout at `ref: ${{ env.PR_HEAD_SHA }}` is missed by both old regexes (false/false) and caught by the count; flipping the single checkout off master fails the positive assertion. 14/14 green at head. Co-Authored-By: Claude --- .github/scripts/tests/run-quality-gates.test.mjs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/.github/scripts/tests/run-quality-gates.test.mjs b/.github/scripts/tests/run-quality-gates.test.mjs index bf78a546faa6..1bc149e5f7f5 100644 --- a/.github/scripts/tests/run-quality-gates.test.mjs +++ b/.github/scripts/tests/run-quality-gates.test.mjs @@ -142,15 +142,14 @@ test('commitperclip-review: listens for edited without displacing the original t // 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 negative forms are -// the load-bearing ones: the positive match only pins the first checkout, so -// adding a *second* step that checks out PR code would slip past it. No `\b` -// after `head` — `head_ref` continues with a word character, so anchoring the -// far end would miss `${{ github.head_ref }}`, the canonical footgun. +// change and becomes an arbitrary-code-execution path. The count is what makes +// this airtight: blacklisting `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 spelling regardless of how its `ref:` is written. 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.doesNotMatch(workflow, /ref:\s*\$\{\{[^}]*\bhead/); - assert.doesNotMatch(workflow, /ref:\s*[^\n]*refs\/pull\//); + assert.equal((workflow.match(/uses:\s*actions\/checkout@/g) ?? []).length, 1); }); test('isGraphifyReindexArtifactOnlyPr: permits generated graphify reindex PRs', () => { From 8096fca50a3021959463c7e5a6844d7a8d5809a4 Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Fri, 18 Sep 2026 16:35:28 +0000 Subject: [PATCH 6/8] docs(ci): state the checkout guard's real bound instead of "airtight" (BLO-26636) The count assertion bounds actions/checkout steps. It does not bound a run: step fetching PR code itself, nor a third-party action taking its own ref: input -- neither is visible to anything reading the workflow as text. "Airtight" is the word that stops the next reader noticing that, so name the ceiling and the fact that nothing exercises it today. Comment-only; the assertion is unchanged and mutation-proven in 1397d18. --- .github/scripts/tests/run-quality-gates.test.mjs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/.github/scripts/tests/run-quality-gates.test.mjs b/.github/scripts/tests/run-quality-gates.test.mjs index 1bc149e5f7f5..38b76c017f97 100644 --- a/.github/scripts/tests/run-quality-gates.test.mjs +++ b/.github/scripts/tests/run-quality-gates.test.mjs @@ -142,11 +142,16 @@ test('commitperclip-review: listens for edited without displacing the original t // 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 makes -// this airtight: blacklisting `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 spelling regardless of how its `ref:` is written. +// 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); From 683850bfc688e36902576dc3e8e9a8d29a5dd3ea Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Sat, 19 Sep 2026 02:52:27 +0000 Subject: [PATCH 7/8] test(ci): pin oldest-match in findExistingComment (BLO-26636) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review raised that `.find` adopts the oldest duplicate gate comment. Both `.find` and `findLast` passed the whole suite, so nothing pinned the choice. Keeping `.find`: our comment is posted by the first failing run, so a bot comment quoting it can only exist after it does. Oldest-match is the genuine one; newest-match is the paste, and PATCHing a paste destroys another agent's comment. That hazard recurs. The duplicate-gate-comment hazard does not — this PR stops it being created, and the existing litter was deleted. Mutation-tested: reverting to findLast turns this test red, 14/15. Co-Authored-By: Claude --- .../scripts/tests/run-quality-gates.test.mjs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/.github/scripts/tests/run-quality-gates.test.mjs b/.github/scripts/tests/run-quality-gates.test.mjs index 38b76c017f97..68b266b7340c 100644 --- a/.github/scripts/tests/run-quality-gates.test.mjs +++ b/.github/scripts/tests/run-quality-gates.test.mjs @@ -112,6 +112,31 @@ test('findExistingComment: ignores a bot comment that merely quotes the signatur 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, which this fix stops and which were deleted +// from all 20 affected open PRs when it landed. +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); +}); + test('findExistingComment: tolerates a comment with no body', async () => { const comment = await findExistingComment(async () => ([ { id: 13, user: { login: 'allyblockcast[bot]', type: 'Bot' }, body: null }, From 2799dc2ee03b1af5ee221772a25be403e2ff97ae Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Sun, 20 Sep 2026 09:10:55 +0000 Subject: [PATCH 8/8] test(ci): pin findExistingComment to buildComment output (BLO-26636) findExistingComment anchors on endsWith(COMMENT_SIGNATURE), which is only correct while buildComment keeps the signature last in both branches. Every existing fixture is a hand-written literal, so nothing tested that coupling. Mutation-tested: appending a footer after the signature turns `existing` permanently null again -- the BLO-26636 defect restored -- and the suite was fully green. The new test is the only one that fails under that mutation. Also corrects the oldest-match comment, which claimed the duplicate sweep happened "when it landed". It has not landed; the sweep was manual and the producing bug still runs on master (#1933 took two comments in eleven minutes), so the comment now says to sweep again at merge time. Co-Authored-By: Claude --- .../scripts/tests/run-quality-gates.test.mjs | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/.github/scripts/tests/run-quality-gates.test.mjs b/.github/scripts/tests/run-quality-gates.test.mjs index 68b266b7340c..45fe3a9796a0 100644 --- a/.github/scripts/tests/run-quality-gates.test.mjs +++ b/.github/scripts/tests/run-quality-gates.test.mjs @@ -118,8 +118,11 @@ test('findExistingComment: ignores a bot comment that merely quotes the signatur // 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, which this fix stops and which were deleted -// from all 20 affected open PRs when it landed. +// 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 () => ([ { @@ -137,6 +140,27 @@ test('findExistingComment: adopts the oldest gate comment, not the newest', asyn 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 },