fix(ci): re-run the commitperclip review gate on PR-body edits (BLO-26636)

.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;
 

.github/scripts/tests/run-quality-gates.test.mjs

@@ -1,25 +1,37 @@
 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) => {
     seenPaths.push(path);
     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,14 +49,163 @@ 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);
 
   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]',

.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

.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