From 8b39f1a38e3deae4d37b16c25f63f5be6ff0db78 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 12:47:10 +0200 Subject: [PATCH 1/8] fix(ci): re-run PR gate on issue_comment and gate prepush lint on gui changes The maintainer GUI-waiver comment ("not touching gui") never re-ran the enforce-target gate because pull_request_target types do not include issue comments. Add an issue_comment trigger so the waiver takes effect when posted, resolving the PR number from the issue payload and falling the checkout back to the default branch. Also stop running lint:gui unconditionally in the local prepush hook: it now runs only when the push touches gui/, mirroring doctor:gui:if-changed. CI already gated GUI lint behind the changes filter; the local hook now matches. --- .github/workflows/enforce-pr-target.yml | 25 ++++- CONTRIBUTING.md | 8 +- package.json | 3 +- scripts/fixtures/lint-findings-exit.ts | 3 + scripts/lint-gui-if-changed.ts | 99 ++++++++++++++++++ scripts/setup-hooks.ts | 7 +- tests/ci-workflows.test.ts | 116 +++++++++++++++++++-- tests/helpers/enforce-pr-target-harness.ts | 32 +++++- 8 files changed, 272 insertions(+), 21 deletions(-) create mode 100644 scripts/fixtures/lint-findings-exit.ts create mode 100644 scripts/lint-gui-if-changed.ts diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index cc85e946d2..34e556eb09 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -8,6 +8,15 @@ on: - edited - ready_for_review - synchronize + # A maintainer issue comment ("not touching gui") waives the GUI-screenshot + # gate. `pull_request_target` types do not include issue comments, so a + # separate `issue_comment` trigger re-runs the gate the moment the waiver is + # posted. The gate is idempotent — it re-reads the live PR and updates the + # single consolidated comment — so a comment cannot race or double-mutate. + issue_comment: + types: + - created + - edited # pull-requests:write covers title/comment/label updates. # contents:write is required for convertPullRequestToDraft / @@ -19,7 +28,8 @@ permissions: pull-requests: write concurrency: - group: enforce-pr-target-${{ github.event.pull_request.number }} + # `issue_comment` events carry the issue number, not the PR number. + group: enforce-pr-target-${{ github.event.pull_request.number || github.event.issue.number }} jobs: enforce-target: @@ -33,8 +43,10 @@ jobs: # runs this workflow from the base revision, and the scripts must come # from the same revision or a merged gate would run against the # pre-promotion scripts on `main`. The immutable SHA pins the checkout - # to the exact base commit the event was built against. - ref: ${{ github.event.pull_request.base.sha }} + # to the exact base commit the event was built against. On an + # `issue_comment` event there is no PR payload, so the checkout falls + # back to the repository default branch — the trusted gate source. + ref: ${{ github.event.pull_request.base.sha || github.event.repository.default_branch }} persist-credentials: false sparse-checkout: | .github/scripts @@ -115,7 +127,12 @@ jobs: const MAINTAINERS_FILE = "MAINTAINERS.md"; const { owner, repo } = context.repo; - const pull_number = context.payload.pull_request.number; + // `issue_comment` events carry the PR's issue object, not a + // `pull_request` object. The issue number is the PR number either + // way, so resolve it from whichever payload the event delivered. + const pull_number = + context.payload.pull_request?.number ?? + context.payload.issue?.number; const { data: pr } = await github.rest.pulls.get({ owner, diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e8d288993c..e28f00f8eb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -58,7 +58,7 @@ A ready-for-review PR is the author's claim that the change is complete, underst ## Pre-push hook After cloning, run once to install a local pre-push hook that runs the typecheck, -GUI eslint, unit-test, privacy-scan, and (when `gui/` changed) React Doctor +unit-test, privacy-scan, and (when `gui/` changed) GUI eslint and React Doctor portions of the CI gate: ```sh @@ -66,8 +66,10 @@ bun run setup:hooks ``` This installs a `pre-push` hook (into the hooks dir git reports, so worktrees and -`core.hooksPath` work) that runs `bun run prepush` — `typecheck`, `lint:gui`, -`test`, `privacy:scan`, and `doctor:gui:if-changed` — before every `git push`. +`core.hooksPath` work) that runs `bun run prepush` — `typecheck`, +`lint:gui:if-changed`, `test`, `privacy:scan`, and `doctor:gui:if-changed` — +before every `git push`. Both `lint:gui:if-changed` and `doctor:gui:if-changed` +run their check only when the push touches `gui/`. The same checks run on ubuntu-latest, macos-latest, and windows-latest in CI (CI additionally builds the GUI and smoke-tests the CLI). Skip in an emergency with `git push --no-verify`. diff --git a/package.json b/package.json index d254e0e94d..aa51127adf 100644 --- a/package.json +++ b/package.json @@ -49,8 +49,9 @@ "prepublishOnly": "bun run audit:high && bun run typecheck && bun run build:gui", "release": "bun scripts/release.ts", "release:watch": "bun scripts/release.ts watch", - "prepush": "bun run typecheck && bun run lint:gui && bun run test && bun run privacy:scan && bun run doctor:gui:if-changed", + "prepush": "bun run typecheck && bun run lint:gui:if-changed && bun run test && bun run privacy:scan && bun run doctor:gui:if-changed", "lint:gui": "cd gui && bun run lint", + "lint:gui:if-changed": "bun scripts/lint-gui-if-changed.ts", "doctor:gui": "cd gui && bun run doctor", "doctor:gui:full": "cd gui && bun run doctor:full", "doctor:gui:if-changed": "bun scripts/doctor-gui-if-changed.ts", diff --git a/scripts/fixtures/lint-findings-exit.ts b/scripts/fixtures/lint-findings-exit.ts new file mode 100644 index 0000000000..1d0c08cb84 --- /dev/null +++ b/scripts/fixtures/lint-findings-exit.ts @@ -0,0 +1,3 @@ +// Simulate eslint finding violations: non-zero exit with finding text. +process.stdout.write("2 problems (2 errors, 0 warnings)\n"); +process.exit(1); diff --git a/scripts/lint-gui-if-changed.ts b/scripts/lint-gui-if-changed.ts new file mode 100644 index 0000000000..0e5cb0dd13 --- /dev/null +++ b/scripts/lint-gui-if-changed.ts @@ -0,0 +1,99 @@ +/** + * Run GUI eslint when this push includes gui/ changes. + * Used by `bun run prepush`. Skip with: git push --no-verify + * + * Mirrors `scripts/doctor-gui-if-changed.ts` so the local pre-push gate and + * the CI `gates` job agree: GUI lint runs only when the push actually touches + * `gui/`. Unlike doctor there is no engine to fetch, so lint findings always + * fail the push — there is no infra-degradation path to soft-skip on. + * + * Test hooks: LINT_DRY_RUN=1 prints the run/skip decision without spawning; + * LINT_FILES (newline-separated) overrides git-derived changed files; + * LINT_CMD overrides the spawned command. + */ +import { spawnSync } from "node:child_process"; +import { join, resolve } from "node:path"; + +/** True when any changed path is the gui directory or inside it (slash-guarded). */ +function guiPathsChanged(files: string[]): boolean { + return files.some(f => f === "gui" || f.startsWith("gui/")); +} + +if (import.meta.main) { + const repoRoot = resolve(import.meta.dirname, ".."); + const guiDir = join(repoRoot, "gui"); + + const hasRef = (ref: string): boolean => { + try { + const probe = spawnSync("git", ["rev-parse", "--verify", ref], { + cwd: repoRoot, + stdio: "ignore", + }); + return probe.status === 0; + } catch { + return false; + } + }; + + const diffNames = (range: string): string[] => { + try { + const diff = spawnSync("git", ["diff", "--name-only", range], { + cwd: repoRoot, + encoding: "utf8", + }); + if (diff.status !== 0) return []; + return (diff.stdout ?? "") + .split(/\r?\n/) + .map(line => line.trim()) + .filter(Boolean); + } catch { + return []; + } + }; + + let files: string[]; + let hadBase = true; + if (process.env.LINT_FILES !== undefined) { + files = process.env.LINT_FILES.split(/\r?\n/).map(f => f.trim()).filter(Boolean); + } else { + let range: string | null = null; + if (hasRef("@{u}")) range = "@{u}...HEAD"; + else if (hasRef("origin/main")) range = "origin/main...HEAD"; + else if (hasRef("main")) range = "main...HEAD"; + hadBase = range !== null; + files = range ? diffNames(range) : []; + } + + // No usable base — run lint so GUI pushes still get a check. + const shouldRun = hadBase ? guiPathsChanged(files) : true; + + if (process.env.LINT_DRY_RUN === "1") { + console.log(shouldRun ? "lint:run" : "lint:skip"); + process.exit(0); + } + + if (!shouldRun) { + console.log("lint:gui: skip (no gui/ changes in push range)"); + process.exit(0); + } + + console.log("lint:gui: gui/ changed — running eslint (scope=changed)"); + const [cmd, ...args] = process.env.LINT_CMD + ? process.env.LINT_CMD.split(" ") + : ["bun", "run", "lint"]; + + const result = spawnSync(cmd!, args, { + cwd: guiDir, + encoding: "utf8", + stdio: "inherit", + }); + + // Lint is local and deterministic: findings fail the push, and a failed + // spawn is a real error, not an infrastructure soft-skip. + if (result.error) { + console.error(`lint:gui: could not run eslint: ${result.error.message}`); + process.exit(1); + } + + process.exit(result.status === null ? 1 : result.status); +} diff --git a/scripts/setup-hooks.ts b/scripts/setup-hooks.ts index db2fc8f8a3..ecd8009331 100644 --- a/scripts/setup-hooks.ts +++ b/scripts/setup-hooks.ts @@ -2,8 +2,9 @@ * Sets up the git pre-push hook for local development. * Run once after cloning: bun run setup:hooks * - * The hook runs `bun run prepush` (typecheck + gui eslint + tests + privacy scan + - * React Doctor when `gui/` changed) before every push — the local portion of the CI gate. + * The hook runs `bun run prepush` (typecheck + tests + privacy scan + GUI + * eslint and React Doctor when `gui/` changed) before every push — the local + * portion of the CI gate. * * To skip in an emergency: git push --no-verify */ @@ -58,5 +59,5 @@ try { // Windows: Git for Windows calls sh.exe directly, executable bit not required. } -console.log(`pre-push hook installed at ${dest}. Runs typecheck + gui eslint + tests + privacy scan (+ React Doctor when gui/ changed) before every push.`); +console.log(`pre-push hook installed at ${dest}. Runs typecheck + tests + privacy scan (+ GUI eslint and React Doctor when gui/ changed) before every push.`); console.log("Skip in an emergency with: git push --no-verify"); diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 35e39beab7..cbee2cc634 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -29,6 +29,7 @@ const lastEnforcerCommentBody = lastGateCommentBody; const root = new URL("../", import.meta.url); const doctorGuiIfChangedScript = fileURLToPath(new URL("../scripts/doctor-gui-if-changed.ts", import.meta.url)); +const lintGuiIfChangedScript = fileURLToPath(new URL("../scripts/lint-gui-if-changed.ts", import.meta.url)); async function readText(path: string): Promise { return await Bun.file(new URL(path, root)).text(); @@ -675,7 +676,10 @@ describe("GitHub Actions hardening", () => { }; type WorkflowJob = Record & { "runs-on"?: unknown; steps?: WorkflowStep[] }; type WorkflowShape = Record & { - on?: { pull_request_target?: { types?: string[] } }; + on?: { + pull_request_target?: { types?: string[] }; + issue_comment?: { types?: string[] }; + }; permissions?: Record | string; concurrency?: Record & { group?: string }; jobs?: Record; @@ -834,7 +838,16 @@ describe("GitHub Actions hardening", () => { // head branch (like `pull_request`), which would run head-controlled // workflow YAML under a write token against base-pinned scripts — a // mismatch that crashes the gate and breaks the trusted-base model. - expect(Object.keys(workflow.on ?? {})).toEqual(["pull_request_target"]); + // + // `issue_comment` is the one extra trigger: a maintainer's GUI-waiver + // comment ("not touching gui") must re-run the gate, and issue comments + // are not a `pull_request_target` activity type. It never touches PR head + // code — the checkout stays on the trusted base/default branch — so it + // does not open the escalation path review events would. + expect(Object.keys(workflow.on ?? {}).sort()).toEqual([ + "issue_comment", + "pull_request_target", + ]); // And the trigger is exactly a `types:` list — nothing else. // @@ -846,6 +859,7 @@ describe("GitHub Actions hardening", () => { // additive, both look like ordinary scoping in a diff, and neither failed a // single assertion. expect(Object.keys(workflow.on?.pull_request_target ?? {})).toEqual(["types"]); + expect(Object.keys(workflow.on?.issue_comment ?? {})).toEqual(["types"]); // Exactly the scopes this gate needs. `pull-requests: write` covers title // and comment updates. `contents: write` is required for the draft GraphQL @@ -860,8 +874,11 @@ describe("GitHub Actions hardening", () => { // One run per PR, so two rapid events cannot race on the title/draft state, // and no `cancel-in-progress` — cancelling the in-flight run mid-mutation is // how the bot ends up having prefixed the title but not recorded that it did. + // `issue_comment` events carry the PR's number under `issue`, not + // `pull_request`, so the group resolves from whichever payload exists. expect(workflow.concurrency).toEqual({ - group: "enforce-pr-target-${{ github.event.pull_request.number }}", + group: + "enforce-pr-target-${{ github.event.pull_request.number || github.event.issue.number }}", }); // One job, and it is this one. An audit round added a `sidecar:` job that @@ -895,7 +912,7 @@ describe("GitHub Actions hardening", () => { // runs this workflow from the base revision, and the scripts must match // it — a merged gate would otherwise run against pre-promotion `main` // scripts. The immutable SHA pins the checkout to the event's base commit. - ref: "${{ github.event.pull_request.base.sha }}", + ref: "${{ github.event.pull_request.base.sha || github.event.repository.default_branch }}", "persist-credentials": false, // MAINTAINERS.md rides along so the completion ping reads the canonical // maintainer list from the same trusted base revision as the scripts. @@ -946,6 +963,16 @@ describe("GitHub Actions hardening", () => { "reopened", "synchronize", ]); + + // A maintainer's GUI-waiver comment must re-run the gate. Issue comments + // are delivered as the `issue_comment` event, which is the only way the + // waiver can take effect without a PR edit or push. + expect(workflow.on?.issue_comment?.types).toBeDefined(); + expect([...(workflow.on?.issue_comment?.types ?? [])].sort()).toEqual([ + "created", + "edited", + ]); + // Review events must NOT be added: they load the workflow from the PR // head branch, breaking the base-pinned checkout (`pull_request_review` // runs head YAML + base scripts → `parseGateState is not a function`). @@ -970,8 +997,11 @@ describe("GitHub Actions hardening", () => { // `Number(context.payload.pull_request.title)` — a value the PR author // controls, which turns the bot into a write primitive against any PR // number the author can name. Bind it to the immutable event field. + // `issue_comment` events carry the number under `issue`, so the resolution + // falls back from the PR object to the issue object — both are immutable + // event fields, never author-controlled title text. expect(script).toMatch( - /const pull_number = context\.payload\.pull_request\.number;/, + /const pull_number =\s*context\.payload\.pull_request\?\.number \?\?\s*context\.payload\.issue\?\.number;/, ); expect(script.match(/pull_number\s*=/g) ?? []).toHaveLength(1); @@ -2603,6 +2633,36 @@ describe("GitHub Actions hardening", () => { expect(lastEnforcerCommentBody(result)).toContain("UI screenshot waived by a maintainer comment"); }); + test("an issue_comment event re-runs the gate and the waiver takes effect", async () => { + // This is the scenario that PR #1119 hit: a maintainer posts the waiver + // as an issue comment, and the gate must re-evaluate on that event — + // `pull_request_target` types do not include issue comments, so the + // separate `issue_comment` trigger carries it. The payload has no + // `pull_request` object; the PR number comes from `issue.number`. + const result = await run({ + pr: { + base: { ref: "dev" }, + title: "GUI: fix provider list spacing", + body: [ + "## Summary", + "This change fixes the provider list spacing in the dashboard.", + "", + "## Test plan", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"), + }, + eventName: "issue_comment", + eventAction: "created", + comments: [ + { id: 1, user: { login: "wibias" }, author_association: "COLLABORATOR", body: "not touching gui" }, + ], + }); + + expect(result.warnings.some((w) => w.startsWith("setFailed:") && w.includes("screenshot"))).toBe(false); + expect(lastEnforcerCommentBody(result)).not.toContain("UI screenshot required"); + expect(lastEnforcerCommentBody(result)).toContain("UI screenshot waived by a maintainer comment"); + }); + test("the PR author cannot waive their own screenshot requirement", async () => { const result = await run({ pr: { @@ -4339,8 +4399,9 @@ describe("GitHub Actions hardening", () => { expect(doctorConfig).toContain('"blocking": "warning"'); expect(rootPkg).toContain('"doctor:gui:if-changed": "bun scripts/doctor-gui-if-changed.ts"'); expect(rootPkg).toContain('"lint:gui": "cd gui && bun run lint"'); - // Gating steps include React Doctor after privacy scan on gui/ pushes. - expect(rootPkg).toContain("bun run typecheck && bun run lint:gui && bun run test"); + expect(rootPkg).toContain('"lint:gui:if-changed": "bun scripts/lint-gui-if-changed.ts"'); + // Gating steps include lint and React Doctor only on gui/ pushes. + expect(rootPkg).toContain("bun run typecheck && bun run lint:gui:if-changed && bun run test"); expect(rootPkg).toContain("bun run privacy:scan && bun run doctor:gui:if-changed"); }); }); @@ -4441,3 +4502,44 @@ describe("doctor-gui-if-changed", () => { expect(run.stderr.toString()).toContain("exceeded buffer"); }); }); + +describe("lint-gui-if-changed", () => { + test("DRY_RUN prints the run/skip decision without spawning lint", () => { + const run = Bun.spawnSync(["bun", lintGuiIfChangedScript], { + env: { ...process.env, LINT_DRY_RUN: "1", LINT_FILES: "gui/src/App.tsx\nscripts/x.ts" }, + }); + expect(run.exitCode).toBe(0); + expect(run.stdout.toString()).toContain("lint:run"); + + const skip = Bun.spawnSync(["bun", lintGuiIfChangedScript], { + env: { ...process.env, LINT_DRY_RUN: "1", LINT_FILES: "scripts/x.ts\nREADME.md" }, + }); + expect(skip.exitCode).toBe(0); + expect(skip.stdout.toString()).toContain("lint:skip"); + }); + + test("runs eslint when gui/ changed and fails the push on findings", () => { + // `bun run lint` in gui/ exits non-zero on findings; a fake command makes + // the spawn deterministic without depending on the real eslint output. + const run = Bun.spawnSync(["bun", lintGuiIfChangedScript], { + env: { + ...process.env, + LINT_FILES: "gui/src/App.tsx", + LINT_CMD: "bun ../scripts/fixtures/lint-findings-exit.ts", + }, + }); + expect(run.exitCode).not.toBe(0); + }); + + test("skips eslint when gui/ did not change", () => { + const run = Bun.spawnSync(["bun", lintGuiIfChangedScript], { + env: { + ...process.env, + LINT_FILES: "scripts/x.ts\nREADME.md", + LINT_CMD: "bun ../scripts/fixtures/lint-findings-exit.ts", + }, + }); + expect(run.exitCode).toBe(0); + expect(run.stdout.toString()).toContain("lint:gui: skip"); + }); +}); diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index 4794913c61..0eaea9bdff 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -82,6 +82,13 @@ export type RunOptions = { * completion provenance rules. */ eventAction?: string; + /** + * Webhook event name. Defaults to `"pull_request_target"`. Pass + * `"issue_comment"` to exercise the GUI-waiver re-run path: the payload then + * carries `issue` and `comment` (never `pull_request`), exactly as GitHub + * delivers an issue comment on a PR. + */ + eventName?: string; /** * Comments as `listComments` returns them, PAGE BY PAGE. Pass more than one * page to prove the script paginates: an audit round replaced `paginate` with @@ -822,9 +829,28 @@ export async function runEnforcePrTarget( * runner and absent here is another `if (payload.x) return;`. */ payload = { - action: options.eventAction ?? "opened", + action: options.eventAction ?? (options.eventName === "issue_comment" ? "created" : "opened"), number: eventPr.number, - pull_request: eventPr, + // An issue comment on a PR is delivered with `issue` + `comment`, never + // `pull_request`. The gate resolves the PR number from whichever object + // the event carried. + ...(options.eventName === "issue_comment" + ? { + issue: { + number: eventPr.number, + node_id: eventPr.node_id, + title: eventPr.title, + body: eventPr.body, + user: eventPr.user, + pull_request: { url: "https://api.github.com/repos/lidge-jun/opencodex/pulls/42" }, + }, + comment: { + id: 424242, + body: "not touching gui", + user: { login: "wibias" }, + }, + } + : { pull_request: eventPr }), repository: { id: 987654321, name: "opencodex", @@ -838,7 +864,7 @@ export async function runEnforcePrTarget( organization: undefined, installation: undefined, }; - eventName = "pull_request_target"; + eventName = options.eventName ?? "pull_request_target"; sha = "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"; ref = "refs/pull/42/merge"; workflow = "Enforce PR target branch"; From ac8505d76c41951faad89367970978cdb359abef Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 12:49:14 +0200 Subject: [PATCH 2/8] fix(ci): pin issue_comment trigger and fallback checkout ref in gate tests The CJS validator test asserted the exact base-SHA checkout ref and did not cover the new issue_comment trigger. Update the ref assertion to the fallback form and add a test pinning that a maintainer GUI-waiver comment re-runs the gate via the issue_comment event. --- .github/scripts/enforce-pr-target.test.cjs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index bcda24f68d..236ae4c909 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -48,6 +48,19 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /synchronize/); }); + it("re-runs on issue_comment so a maintainer GUI waiver takes effect", () => { + // The GUI-screenshot gate is waived by a maintainer issue comment + // ("not touching gui"). `pull_request_target` types do not include issue + // comments, so without this trigger the waiver sits unread until a PR + // edit or push re-runs the gate. + assert.match(workflow, /^ issue_comment:/m); + assert.match(workflow, /- created/); + assert.match(workflow, /- edited/); + // The script resolves the PR number from the issue payload, which is what + // an issue_comment event delivers instead of a pull_request object. + assert.match(workflow, /context\.payload\.issue\?\.number/); + }); + it("does not add review events that would break the trusted-base model", () => { // `pull_request_review` / `pull_request_review_comment` load the workflow // from the PR head branch (like `pull_request`), while this workflow's @@ -127,7 +140,14 @@ describe("enforce-pr-target workflow", () => { .split("- name: Checkout trusted PR-quality scripts")[1] .split(/\n {6}- name:/)[0]; assert.match(checkoutStep, /actions\/checkout@[0-9a-f]{40}/); - assert.match(checkoutStep, /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}/); + // `pull_request_target` pins the PR's base SHA so the scripts match the + // event's base revision. An `issue_comment` event has no PR payload, so + // the ref falls back to the repository default branch — still trusted, and + // never the PR head. + assert.match( + checkoutStep, + /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\|\|\s*github\.event\.repository\.default_branch\s*\}\}/, + ); // The readiness ping reads MAINTAINERS.md from the same trusted checkout. assert.match(checkoutStep, /sparse-checkout:\s*\|\s*\n\s*\.github\/scripts\n\s*MAINTAINERS\.md/); assert.match(checkoutStep, /persist-credentials:\s*false/); From 732f296e2b6401c19c27d83f3397c2d8e905ad25 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:00:22 +0200 Subject: [PATCH 3/8] fix(ci): restrict issue_comment re-run to maintainers on PRs and consolidate bot comments The issue_comment trigger fired for any comment on any issue from any user. Guard the enforce-target job so only maintainer (OWNER/COLLABORATOR/MEMBER) comments on actual PRs re-run the write-capable gate; a comment on a plain issue or from a contributor is skipped, with a defensive in-script re-check matching the job-level if. Also consolidate the PR gate and PR hygiene bot messages into the single opencodex-pr-gate comment. The hygiene workflow now writes its status block into the gate comment (preserving the gate section) instead of posting a second standalone message, and the gate rebuild preserves an existing hygiene block so neither workflow clobbers the other. --- .github/scripts/pr-quality-messages.cjs | 72 ++++++++++- .github/scripts/pr-quality-messages.test.cjs | 58 +++++++++ .github/workflows/enforce-pr-target.yml | 44 ++++++- .github/workflows/issue-quality-tests.yml | 4 + .github/workflows/pr-hygiene.yml | 34 +++++- tests/ci-workflows.test.ts | 120 ++++++++++++++++++- tests/helpers/enforce-pr-target-harness.ts | 16 ++- 7 files changed, 336 insertions(+), 12 deletions(-) diff --git a/.github/scripts/pr-quality-messages.cjs b/.github/scripts/pr-quality-messages.cjs index 0f6c6619a9..e7855aaea5 100644 --- a/.github/scripts/pr-quality-messages.cjs +++ b/.github/scripts/pr-quality-messages.cjs @@ -16,6 +16,11 @@ const { const READINESS_MARKER = ""; /** Marks the bot's consolidated PR gate message. */ const GATE_MARKER = ""; +/** Marks the hygiene status block inside the consolidated gate comment. */ +const HYGIENE_MARKER = ""; +/** HTML comment wrapping the hygiene block so it survives gate rebuilds. */ +const HYGIENE_BLOCK_START = ""; +const HYGIENE_BLOCK_END = ""; function inlineCode(value) { const text = String(value); @@ -57,7 +62,8 @@ function buildGateCommentBody(state, opts) { actions = [], readiness, checklistRequired = true, - notices = [] + notices = [], + hygiene } = opts; const complete = readiness?.present && readiness?.complete; const statusEmoji = status === "READY" ? "✅" : "⏳"; @@ -84,10 +90,69 @@ function buildGateCommentBody(state, opts) { "" ] : []), + ...(hygiene && hygiene.length > 0 + ? [ + "## Hygiene", + "", + HYGIENE_BLOCK_START, + HYGIENE_MARKER, + "", + ...hygiene, + "", + HYGIENE_BLOCK_END, + "" + ] + : []), ...notices ].filter(line => line !== null && line !== undefined); } +/** + * The hygiene status block as stored inside the consolidated gate comment, or + * `null` when the comment has none. The gate rebuilds its body from scratch + * every run, so without this round-trip a hygiene update from the separate + * hygiene workflow would be silently dropped on the next gate write. + */ +function extractHygieneSection(body) { + if (typeof body !== "string") return null; + const match = body.match( + new RegExp(`${HYGIENE_BLOCK_START}([\\s\\S]*?)${HYGIENE_BLOCK_END}`) + ); + if (!match) return null; + return match[1] + .split("\n") + .map(line => line.trim()) + .filter(line => line !== "" && line !== HYGIENE_MARKER) + .join("\n"); +} + +/** + * Insert (or replace) a hygiene block in a gate-comment body. Used by the + * hygiene workflow to write its status into the single consolidated comment + * instead of posting a second bot message. + */ +function withHygieneSection(body, hygieneLines) { + const base = typeof body === "string" ? body : ""; + const block = [ + HYGIENE_BLOCK_START, + HYGIENE_MARKER, + "", + ...hygieneLines, + "", + HYGIENE_BLOCK_END + ].join("\n"); + + if (base.includes(HYGIENE_BLOCK_START) && base.includes(HYGIENE_BLOCK_END)) { + return base.replace( + new RegExp(`${HYGIENE_BLOCK_START}[\\s\\S]*?${HYGIENE_BLOCK_END}`), + block + ); + } + + // No existing block: append one at the end. + return `${base.replace(/\s+$/, "")}\n\n## Hygiene\n\n${block}\n`; +} + function descriptionFailureLines(reason) { switch (reason) { case "empty": @@ -254,9 +319,14 @@ function buildStaleNotice({ completionHeadSha, liveHeadSha, eventAction }) { module.exports = { READINESS_MARKER, GATE_MARKER, + HYGIENE_MARKER, + HYGIENE_BLOCK_START, + HYGIENE_BLOCK_END, inlineCode, readinessChecklistLines, buildGateCommentBody, + extractHygieneSection, + withHygieneSection, descriptionFailureLines, buildFailureSections, failureSummary, diff --git a/.github/scripts/pr-quality-messages.test.cjs b/.github/scripts/pr-quality-messages.test.cjs index 0866a952d5..71d889f05f 100644 --- a/.github/scripts/pr-quality-messages.test.cjs +++ b/.github/scripts/pr-quality-messages.test.cjs @@ -7,9 +7,14 @@ const { } = require("./pr-quality.cjs"); const { GATE_MARKER, + HYGIENE_MARKER, + HYGIENE_BLOCK_START, + HYGIENE_BLOCK_END, inlineCode, readinessChecklistLines, buildGateCommentBody, + extractHygieneSection, + withHygieneSection, descriptionFailureLines, buildFailureSections, failureSummary, @@ -254,3 +259,56 @@ describe("buildFindingsClaimNotice", () => { assert.match(notice[1], /Resolve every open review conversation/); }); }); + +describe("hygiene section round-trip", () => { + const GATE = [ + GATE_MARKER, + '', + "", + "## ✅ READY", + "- all PR quality gates passed.", + ].join("\n"); + + it("renders a hygiene block in the gate comment when requested", () => { + const body = buildGateCommentBody( + { version: 1, active: false }, + { + status: "READY", + statusReason: "all PR quality gates passed.", + checklistRequired: false, + hygiene: ["✅ **Deterministic PR hygiene checks passed.**"], + }, + ).join("\n"); + assert.ok(body.includes(HYGIENE_BLOCK_START)); + assert.ok(body.includes(HYGIENE_BLOCK_END)); + assert.ok(body.includes(HYGIENE_MARKER)); + assert.ok(body.includes("✅ **Deterministic PR hygiene checks passed.**")); + }); + + it("extracts the hygiene content from a gate comment", () => { + const withBlock = `${GATE}\n\n## Hygiene\n\n${HYGIENE_BLOCK_START}\n${HYGIENE_MARKER}\n\n✅ **Deterministic PR hygiene checks passed.**\n\n${HYGIENE_BLOCK_END}\n`; + const extracted = extractHygieneSection(withBlock); + assert.equal(extracted, "✅ **Deterministic PR hygiene checks passed.**"); + assert.equal(extractHygieneSection(GATE), null); + }); + + it("replaces an existing hygiene block without duplicating it", () => { + const withBlock = `${GATE}\n\n## Hygiene\n\n${HYGIENE_BLOCK_START}\n${HYGIENE_MARKER}\n\n✅ **Deterministic PR hygiene checks passed.**\n\n${HYGIENE_BLOCK_END}\n`; + const updated = withHygieneSection(withBlock, [ + "⚠️ **Deterministic hygiene checks failed.**", + "- `missing_regression_test` — Behavior changed under `src/` without a test change.", + ]); + assert.ok(updated.includes("⚠️ **Deterministic hygiene checks failed.**")); + assert.ok(!updated.includes("✅ **Deterministic PR hygiene checks passed.**")); + assert.equal(updated.split(HYGIENE_BLOCK_START).length - 1, 1); + }); + + it("appends a hygiene block when the gate comment has none", () => { + const updated = withHygieneSection(GATE, [ + "✅ **Deterministic PR hygiene checks passed.**", + ]); + assert.ok(updated.includes(HYGIENE_BLOCK_START)); + assert.ok(updated.includes("✅ **Deterministic PR hygiene checks passed.**")); + assert.ok(updated.includes(GATE_MARKER)); + }); +}); diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 34e556eb09..c613d30910 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -33,6 +33,16 @@ concurrency: jobs: enforce-target: + # `issue_comment` fires for comments on ANY issue, PR or not, from ANY + # user. This gate is PR-only and write-capable, so a comment on a plain + # issue — or from a non-maintainer — must not start it. Only a maintainer + # comment on a PR (the GUI-waiver case) may re-run the gate. + if: >- + github.event_name != 'issue_comment' || + (github.event.issue.pull_request != null && + (github.event.comment.author_association == 'OWNER' || + github.event.comment.author_association == 'COLLABORATOR' || + github.event.comment.author_association == 'MEMBER')) runs-on: ubuntu-latest steps: @@ -93,8 +103,12 @@ jobs: const { GATE_MARKER, READINESS_MARKER, + HYGIENE_MARKER, + HYGIENE_BLOCK_START, + HYGIENE_BLOCK_END, inlineCode, buildGateCommentBody, + extractHygieneSection, buildFailureSections, failureSummary, buildStaleNotice, @@ -134,6 +148,25 @@ jobs: context.payload.pull_request?.number ?? context.payload.issue?.number; + // Defensive re-check of the job-level guard. `issue_comment` events + // carry a `comment` object with the author's association; a comment + // on a plain issue has no `issue.pull_request`, and a comment from + // anyone but a maintainer must not re-run this write-capable gate. + if (context.eventName === "issue_comment") { + const isPrComment = + context.payload.issue?.pull_request != null; + const association = context.payload.comment?.author_association; + const isMaintainer = ["OWNER", "COLLABORATOR", "MEMBER"].includes( + association + ); + if (!isPrComment || !isMaintainer) { + core.info( + "issue_comment not from a maintainer on a PR; skipping the gate." + ); + return; + } + } + const { data: pr } = await github.rest.pulls.get({ owner, repo, @@ -248,7 +281,16 @@ jobs: * readiness section or a stale intermediate checkpoint body. */ async function upsertGateComment(state, opts) { - const body = buildGateCommentBody(state, opts).join("\n"); + let body = buildGateCommentBody(state, opts).join("\n"); + // The hygiene workflow writes its status into this same comment. + // Preserve whatever it left so a gate rebuild does not drop it. + const existingHygiene = extractHygieneSection(gateComment?.body); + if (existingHygiene && !body.includes("pr-hygiene-block")) { + body = body.replace( + /\s+$/, + `\n\n## Hygiene\n\n${HYGIENE_BLOCK_START}\n${HYGIENE_MARKER}\n\n${existingHygiene}\n\n${HYGIENE_BLOCK_END}` + ); + } if (gateCommentId) { await github.rest.issues.updateComment({ owner, diff --git a/.github/workflows/issue-quality-tests.yml b/.github/workflows/issue-quality-tests.yml index 78b96f4672..e5f95e6c7d 100644 --- a/.github/workflows/issue-quality-tests.yml +++ b/.github/workflows/issue-quality-tests.yml @@ -8,6 +8,8 @@ on: - ".github/scripts/issue-quality.test.cjs" - ".github/scripts/pr-quality.cjs" - ".github/scripts/pr-quality.test.cjs" + - ".github/scripts/pr-quality-messages.cjs" + - ".github/scripts/pr-quality-messages.test.cjs" - ".github/scripts/pr-labeler.cjs" - ".github/scripts/pr-labeler.test.cjs" - ".github/scripts/enforce-pr-target.test.cjs" @@ -35,6 +37,8 @@ on: - ".github/scripts/issue-quality.test.cjs" - ".github/scripts/pr-quality.cjs" - ".github/scripts/pr-quality.test.cjs" + - ".github/scripts/pr-quality-messages.cjs" + - ".github/scripts/pr-quality-messages.test.cjs" - ".github/scripts/pr-labeler.cjs" - ".github/scripts/pr-labeler.test.cjs" - ".github/scripts/enforce-pr-target.test.cjs" diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml index 4ead21bdde..53e1d6044e 100644 --- a/.github/workflows/pr-hygiene.yml +++ b/.github/workflows/pr-hygiene.yml @@ -41,10 +41,17 @@ jobs: const { assessSponsoredSurface } = require( path.join(process.cwd(), ".github", "scripts", "pr-sponsored-surface.cjs"), ); + const { + GATE_MARKER, + HYGIENE_MARKER, + withHygieneSection + } = require( + path.join(process.cwd(), ".github", "scripts", "pr-quality-messages.cjs"), + ); const { owner, repo } = context.repo; const pull_number = context.payload.pull_request.number; - const marker = ""; + const marker = HYGIENE_MARKER; const blockedLabel = "intake: hygiene-blocked"; const labelDefinitions = { [blockedLabel]: ["b60205", "Deterministic PR hygiene checks failed"], @@ -123,11 +130,30 @@ jobs: const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: pull_number, per_page: 100, }); - const existing = comments.find( + // The single consolidated bot comment is the one the PR gate + // owns (GATE_MARKER). Write the hygiene status into that same + // comment so there is one editable message, not two. Fall back + // to a standalone hygiene comment only when the gate has not + // posted yet (the next gate run absorbs it). + const gateComment = comments.find( + (comment) => comment.user?.login === "github-actions[bot]" && comment.body?.includes(GATE_MARKER), + ); + if (gateComment) { + // The hygiene block already carries the marker; strip the + // standalone body's own marker line before merging. + const hygieneLines = body + .split("\n") + .map(line => line.trim()) + .filter(line => line !== "" && line !== marker); + const merged = withHygieneSection(gateComment.body, hygieneLines); + await github.rest.issues.updateComment({ owner, repo, comment_id: gateComment.id, body: merged }); + return; + } + const existingHygiene = comments.find( (comment) => comment.user?.login === "github-actions[bot]" && comment.body?.includes(marker), ); - if (existing) { - await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); + if (existingHygiene) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existingHygiene.id, body }); } else { await github.rest.issues.createComment({ owner, repo, issue_number: pull_number, body }); } diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index cbee2cc634..e507489c8b 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -886,13 +886,22 @@ describe("GitHub Actions hardening", () => { // still passed, because they only ever looked at `enforce-target`. expect(jobs.map(([name]) => name)).toEqual(["enforce-target"]); - // The job is exactly a runner plus steps. No `if:` (which silently disables - // the whole gate), no `permissions:` (a job-level block overrides the narrow - // workflow-level one), no `container:`/`strategy:`/`outputs:`/`env:`/ - // `defaults:`, and no `<<:` merge key to reintroduce any of them sideways. + // The job is a runner plus steps, with one deliberate `if:` guard. The + // guard restricts the `issue_comment` trigger to maintainer comments on + // PRs — a comment on a plain issue, or from a non-maintainer, must not + // start this write-capable gate. On `pull_request_target` events the guard + // is always true, so it never disables the gate. + // No `permissions:` (a job-level block overrides the narrow workflow-level + // one), no `container:`/`strategy:`/`outputs:`/`env:`/`defaults:`, and no + // `<<:` merge key to reintroduce any of them sideways. const [, job] = jobs[0]!; - expect(Object.keys(job).sort()).toEqual(["runs-on", "steps"]); + expect(Object.keys(job).sort()).toEqual(["if", "runs-on", "steps"]); expect(job["runs-on"]).toBe("ubuntu-latest"); + expect(job["if"]).toContain("github.event_name != 'issue_comment'"); + expect(job["if"]).toContain("github.event.issue.pull_request != null"); + expect(job["if"]).toContain("'OWNER'"); + expect(job["if"]).toContain("'COLLABORATOR'"); + expect(job["if"]).toContain("'MEMBER'"); // Checkout trusted scripts, then run the gate. Anything more is an extra // privileged action nobody reviewed. @@ -2663,6 +2672,107 @@ describe("GitHub Actions hardening", () => { expect(lastEnforcerCommentBody(result)).toContain("UI screenshot waived by a maintainer comment"); }); + test("a non-maintainer issue_comment does not re-run the gate", async () => { + // The `issue_comment` trigger must only re-run for maintainer comments + // (OWNER / COLLABORATOR / MEMBER). A random comment from a contributor + // must not start the write-capable gate or re-draft the PR. + const result = await run({ + pr: { + base: { ref: "dev" }, + title: "GUI: fix provider list spacing", + body: [ + "## Summary", + "This change fixes the provider list spacing in the dashboard.", + "", + "## Test plan", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"), + }, + eventName: "issue_comment", + eventAction: "created", + commentAuthorAssociation: "CONTRIBUTOR", + comments: [ + { id: 1, user: { login: "someone" }, author_association: "CONTRIBUTOR", body: "looks good to me" }, + ], + }); + + // The gate never runs: no screenshot failure, no waiver notice, no draft + // mutation, no comment write. + expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(false); + expect(methodsOf(result)).not.toContain("issues.createComment"); + expect(methodsOf(result)).not.toContain("issues.updateComment"); + expect(methodsOf(result)).not.toContain("graphql"); + }); + + test("an issue_comment on a plain issue does not re-run the gate", async () => { + // `issue_comment` fires for comments on ANY issue. A comment on a plain + // issue (no `issue.pull_request`) is not a PR comment and must not start + // this PR-only gate. + const result = await run({ + pr: { + base: { ref: "dev" }, + title: "GUI: fix provider list spacing", + body: [ + "## Summary", + "This change fixes the provider list spacing in the dashboard.", + "", + "## Test plan", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"), + }, + eventName: "issue_comment", + eventAction: "created", + issueIsPullRequest: false, + comments: [ + { id: 1, user: { login: "wibias" }, author_association: "COLLABORATOR", body: "not touching gui" }, + ], + }); + + expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(false); + expect(methodsOf(result)).not.toContain("issues.createComment"); + expect(methodsOf(result)).not.toContain("issues.updateComment"); + expect(methodsOf(result)).not.toContain("graphql"); + }); + + test("the gate comment preserves an existing hygiene section across rebuilds", async () => { + // The hygiene workflow writes its status into the same consolidated gate + // comment. When the gate rebuilds that comment, it must carry the + // hygiene block forward instead of dropping it. + const HYGIENE_BLOCK_START = ""; + const HYGIENE_BLOCK_END = ""; + const existingGateBody = [ + GATE_MARKER, + '', + "", + "## ⏳ DRAFT", + "- PR is kept in draft.", + "", + "## Hygiene", + "", + HYGIENE_BLOCK_START, + "", + "", + "✅ **Deterministic PR hygiene checks passed.**", + "", + HYGIENE_BLOCK_END, + ].join("\n"); + + const result = await run({ + pr: { base: { ref: "dev" }, draft: false }, + authorPermission: "write", + comments: [ + { id: 7, user: { login: "github-actions[bot]" }, body: existingGateBody }, + ], + }); + + const updated = callsTo(result, "issues.updateComment") as [{ body: string }]; + expect(updated.length).toBeGreaterThan(0); + const gateUpdate = updated.find(call => call.body.includes(GATE_MARKER))!; + expect(gateUpdate.body).toContain(HYGIENE_BLOCK_START); + expect(gateUpdate.body).toContain(HYGIENE_BLOCK_END); + expect(gateUpdate.body).toContain("✅ **Deterministic PR hygiene checks passed.**"); + }); + test("the PR author cannot waive their own screenshot requirement", async () => { const result = await run({ pr: { diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index 0eaea9bdff..abc8c6f8c5 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -89,6 +89,17 @@ export type RunOptions = { * delivers an issue comment on a PR. */ eventName?: string; + /** + * `author_association` of the commenter on an `issue_comment` event. + * Defaults to `"COLLABORATOR"`. The gate only re-runs for maintainer + * associations (OWNER / COLLABORATOR / MEMBER). + */ + commentAuthorAssociation?: string; + /** + * Whether the commented-on issue is a pull request. Defaults to `true`. + * An `issue_comment` on a plain issue must not start this PR-only gate. + */ + issueIsPullRequest?: boolean; /** * Comments as `listComments` returns them, PAGE BY PAGE. Pass more than one * page to prove the script paginates: an audit round replaced `paginate` with @@ -842,12 +853,15 @@ export async function runEnforcePrTarget( title: eventPr.title, body: eventPr.body, user: eventPr.user, - pull_request: { url: "https://api.github.com/repos/lidge-jun/opencodex/pulls/42" }, + ...(options.issueIsPullRequest === false + ? {} + : { pull_request: { url: "https://api.github.com/repos/lidge-jun/opencodex/pulls/42" } }), }, comment: { id: 424242, body: "not touching gui", user: { login: "wibias" }, + author_association: options.commentAuthorAssociation ?? "COLLABORATOR", }, } : { pull_request: eventPr }), From 36c87afe06fb073c57972af4a3e16c21ce858033 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:06:31 +0200 Subject: [PATCH 4/8] fix(ci): anchor hygiene delimiters to complete lines and share the gate comment concurrency Two CodeRabbit findings on the consolidated gate comment: - The hygiene block delimiters matched anywhere in the comment body. A contributor-controlled changed filename could embed delimiter text mid-line and corrupt the block boundary on the next rewrite. Anchor both delimiters to complete lines via a shared regex used by the existence check and the replacement, with a regression test for embedded delimiter text. - The gate and hygiene workflows each had their own per-PR concurrency group while both read-modify-write the same consolidated comment. A concurrent gate rebuild and hygiene update could run from stale snapshots and the last write would drop the other's section. Share one per-PR concurrency group between the two workflows and pin it in the workflow tests. --- .github/scripts/pr-quality-messages.cjs | 20 ++++++---- .github/scripts/pr-quality-messages.test.cjs | 40 ++++++++++++++++++++ .github/workflows/enforce-pr-target.yml | 7 +++- .github/workflows/pr-hygiene.yml | 4 +- tests/ci-workflows.test.ts | 13 ++++++- 5 files changed, 72 insertions(+), 12 deletions(-) diff --git a/.github/scripts/pr-quality-messages.cjs b/.github/scripts/pr-quality-messages.cjs index e7855aaea5..aec05331b8 100644 --- a/.github/scripts/pr-quality-messages.cjs +++ b/.github/scripts/pr-quality-messages.cjs @@ -21,6 +21,15 @@ const HYGIENE_MARKER = ""; /** HTML comment wrapping the hygiene block so it survives gate rebuilds. */ const HYGIENE_BLOCK_START = ""; const HYGIENE_BLOCK_END = ""; +/** + * Both delimiters must occupy a complete line. A contributor-controlled + * hygiene line (for example a changed filename) can otherwise embed delimiter + * text mid-line and corrupt the block boundary on the next rewrite. + */ +const HYGIENE_BLOCK_RE = new RegExp( + `^[ \\t]*${HYGIENE_BLOCK_START}[ \\t]*\\n([\\s\\S]*?)\\n[ \\t]*${HYGIENE_BLOCK_END}[ \\t]*$`, + "m" +); function inlineCode(value) { const text = String(value); @@ -115,9 +124,7 @@ function buildGateCommentBody(state, opts) { */ function extractHygieneSection(body) { if (typeof body !== "string") return null; - const match = body.match( - new RegExp(`${HYGIENE_BLOCK_START}([\\s\\S]*?)${HYGIENE_BLOCK_END}`) - ); + const match = body.match(HYGIENE_BLOCK_RE); if (!match) return null; return match[1] .split("\n") @@ -142,11 +149,8 @@ function withHygieneSection(body, hygieneLines) { HYGIENE_BLOCK_END ].join("\n"); - if (base.includes(HYGIENE_BLOCK_START) && base.includes(HYGIENE_BLOCK_END)) { - return base.replace( - new RegExp(`${HYGIENE_BLOCK_START}[\\s\\S]*?${HYGIENE_BLOCK_END}`), - block - ); + if (HYGIENE_BLOCK_RE.test(base)) { + return base.replace(HYGIENE_BLOCK_RE, block); } // No existing block: append one at the end. diff --git a/.github/scripts/pr-quality-messages.test.cjs b/.github/scripts/pr-quality-messages.test.cjs index 71d889f05f..e24f3addda 100644 --- a/.github/scripts/pr-quality-messages.test.cjs +++ b/.github/scripts/pr-quality-messages.test.cjs @@ -311,4 +311,44 @@ describe("hygiene section round-trip", () => { assert.ok(updated.includes("✅ **Deterministic PR hygiene checks passed.**")); assert.ok(updated.includes(GATE_MARKER)); }); + + it("ignores delimiter text embedded inside a hygiene content line", () => { + // A contributor-controlled changed filename can contain delimiter text + // mid-line (e.g. `src//x.ts`). The block + // regex must anchor delimiters to complete lines so such a line neither + // ends the block early nor corrupts the next rewrite. + const malicious = [ + GATE_MARKER, + '', + "", + "## ✅ READY", + "- all PR quality gates passed.", + "", + "## Hygiene", + "", + HYGIENE_BLOCK_START, + "", + "", + "✅ **Deterministic PR hygiene checks passed.**", + `- Paths: \`src/${HYGIENE_BLOCK_END}/x.ts\`.`, + "", + HYGIENE_BLOCK_END, + ].join("\n"); + + const extracted = extractHygieneSection(malicious); + assert.ok(extracted); + assert.ok(extracted.includes("✅ **Deterministic PR hygiene checks passed.**")); + assert.ok(extracted.includes("Paths")); + + // Replacing must preserve the malicious line inside the block, not split + // the block at the embedded delimiter. + const updated = withHygieneSection(malicious, [ + "⚠️ **Deterministic hygiene checks failed.**", + ]); + assert.ok(updated.includes(HYGIENE_BLOCK_START)); + assert.ok(updated.includes(HYGIENE_BLOCK_END)); + assert.ok(updated.includes("⚠️ **Deterministic hygiene checks failed.**")); + assert.equal(updated.split(HYGIENE_BLOCK_START).length - 1, 1); + assert.equal(updated.split(HYGIENE_BLOCK_END).length - 1, 1); + }); }); diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index c613d30910..fa9a8743aa 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -28,8 +28,11 @@ permissions: pull-requests: write concurrency: - # `issue_comment` events carry the issue number, not the PR number. - group: enforce-pr-target-${{ github.event.pull_request.number || github.event.issue.number }} + # `issue_comment` events carry the issue number, not the PR number. The + # group is shared with the hygiene workflow: both read-modify-write the same + # consolidated gate comment, so serializing them under one key prevents a + # concurrent update from clobbering the other's section. + group: pr-gate-comment-${{ github.event.pull_request.number || github.event.issue.number }} jobs: enforce-target: diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml index 53e1d6044e..e99a765653 100644 --- a/.github/workflows/pr-hygiene.yml +++ b/.github/workflows/pr-hygiene.yml @@ -10,7 +10,9 @@ on: permissions: {} concurrency: - group: pr-hygiene-${{ github.event.pull_request.number }} + # Shared with the enforce-target gate: both workflows read-modify-write the + # same consolidated gate comment, so one per-PR group serializes them. + group: pr-gate-comment-${{ github.event.pull_request.number }} cancel-in-progress: true jobs: diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index e507489c8b..d53939e671 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -878,9 +878,20 @@ describe("GitHub Actions hardening", () => { // `pull_request`, so the group resolves from whichever payload exists. expect(workflow.concurrency).toEqual({ group: - "enforce-pr-target-${{ github.event.pull_request.number || github.event.issue.number }}", + "pr-gate-comment-${{ github.event.pull_request.number || github.event.issue.number }}", }); + // The hygiene workflow reads and rewrites the same consolidated gate + // comment, so it must share the gate's per-PR concurrency group. Separate + // groups would let a gate rebuild and a hygiene update run concurrently + // from stale snapshots, and the last write would drop the other's section. + const hygieneWorkflow = Bun.YAML.parse( + await readText(".github/workflows/pr-hygiene.yml"), + ) as { concurrency?: { group?: string; "cancel-in-progress"?: boolean } }; + expect(hygieneWorkflow.concurrency?.group).toBe( + "pr-gate-comment-${{ github.event.pull_request.number }}", + ); + // One job, and it is this one. An audit round added a `sidecar:` job that // inherited the PR-write token and un-drafted the PR — every assertion below // still passed, because they only ever looked at `enforce-target`. From 4d13067c946f0eb29ae1bc6746c9e5bd1664a1db Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:12:57 +0200 Subject: [PATCH 5/8] fix(ci): checkout dev on issue_comment reruns and fail closed on unverifiable checklists Codex-bot review findings on the issue_comment trigger: - The checkout fell back to the repository default branch (main) on issue_comment, which can lag the integration branch the gate enforces. Fall back to dev (the gate's only allowed base) so comment-triggered runs evaluate with the gate's own current scripts. - issue_comment events carry no pull_request.head.sha, so eventHeadSha fell back to the live head and a completed checklist with no recorded completion head was accepted as attesting the current head. Pass an empty eventHeadSha on issue_comment so completionIsStale resets the checklist (fail closed) instead of promoting readiness from an unverifiable attestation. Regression tests cover both. --- .github/scripts/enforce-pr-target.test.cjs | 6 ++-- .github/scripts/pr-quality-state.test.cjs | 17 ++++++++++ .github/workflows/enforce-pr-target.yml | 17 ++++++++-- tests/ci-workflows.test.ts | 37 +++++++++++++++++++++- 4 files changed, 70 insertions(+), 7 deletions(-) diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 236ae4c909..5fc9455a7c 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -142,11 +142,11 @@ describe("enforce-pr-target workflow", () => { assert.match(checkoutStep, /actions\/checkout@[0-9a-f]{40}/); // `pull_request_target` pins the PR's base SHA so the scripts match the // event's base revision. An `issue_comment` event has no PR payload, so - // the ref falls back to the repository default branch — still trusted, and - // never the PR head. + // the ref falls back to the integration branch `dev` (the gate's only + // allowed base) — still trusted, and never the PR head. assert.match( checkoutStep, - /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\|\|\s*github\.event\.repository\.default_branch\s*\}\}/, + /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\|\|\s*'dev'\s*\}\}/, ); // The readiness ping reads MAINTAINERS.md from the same trusted checkout. assert.match(checkoutStep, /sparse-checkout:\s*\|\s*\n\s*\.github\/scripts\n\s*MAINTAINERS\.md/); diff --git a/.github/scripts/pr-quality-state.test.cjs b/.github/scripts/pr-quality-state.test.cjs index 1b0a1a2c82..f005ff5784 100644 --- a/.github/scripts/pr-quality-state.test.cjs +++ b/.github/scripts/pr-quality-state.test.cjs @@ -186,6 +186,23 @@ describe("completionIsStale", () => { ); }); + it("is stale when the event delivered no head SHA at all (issue_comment rerun)", () => { + // `issue_comment` events carry no `pull_request.head.sha`. The gate passes + // an empty eventHeadSha so a completed checklist with no recorded head + // cannot be accepted as attesting the live head on a comment-triggered + // rerun — the contributor could have pushed since ticking the boxes. + assert.equal( + completionIsStale({ + ...base, + checklistComplete: true, + completionHeadSha: null, + eventHeadSha: "", + eventAction: "created" + }), + true, + ); + }); + it("is not stale for maintainers or absent checklists", () => { assert.equal( diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index fa9a8743aa..018872eedd 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -58,8 +58,11 @@ jobs: # pre-promotion scripts on `main`. The immutable SHA pins the checkout # to the exact base commit the event was built against. On an # `issue_comment` event there is no PR payload, so the checkout falls - # back to the repository default branch — the trusted gate source. - ref: ${{ github.event.pull_request.base.sha || github.event.repository.default_branch }} + # back to the integration branch `dev` — the branch this gate enforces + # and the source of the gate's own scripts. The repository default + # (`main`) can lag `dev`, which would make a comment-triggered run + # evaluate with stale helpers. + ref: ${{ github.event.pull_request.base.sha || 'dev' }} persist-credentials: false sparse-checkout: | .github/scripts @@ -536,8 +539,16 @@ jobs: // (see `completionIsStale`). When it is stale the gate resets the // boxes and the notification state, re-drafts, and tells the // author to re-test and re-tick against the latest code. + // `issue_comment` events carry no `pull_request.head.sha`, so the + // fallback to the live head would let a completed checklist with + // no recorded completion head pass as if it attested the current + // head. A comment-triggered run must not promote readiness: pass + // the live head only when the event actually delivered it. const eventHeadSha = - context.payload.pull_request?.head?.sha ?? pr.head.sha; + context.payload.pull_request?.head?.sha ?? + (context.eventName === "issue_comment" + ? "" + : pr.head.sha); const completionHeadSha = gateState.completedAtHeadSha ?? null; const headDrifted = completionIsStale({ diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index d53939e671..5e2aa42caf 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -932,7 +932,7 @@ describe("GitHub Actions hardening", () => { // runs this workflow from the base revision, and the scripts must match // it — a merged gate would otherwise run against pre-promotion `main` // scripts. The immutable SHA pins the checkout to the event's base commit. - ref: "${{ github.event.pull_request.base.sha || github.event.repository.default_branch }}", + ref: "${{ github.event.pull_request.base.sha || 'dev' }}", "persist-credentials": false, // MAINTAINERS.md rides along so the completion ping reads the canonical // maintainer list from the same trusted base revision as the scripts. @@ -2683,6 +2683,41 @@ describe("GitHub Actions hardening", () => { expect(lastEnforcerCommentBody(result)).toContain("UI screenshot waived by a maintainer comment"); }); + test("an issue_comment rerun does not accept a checklist with no recorded head", async () => { + // `issue_comment` events carry no `pull_request.head.sha`. A contributor + // who ticked the readiness checklist, then pushed, must not have that + // stale attestation accepted by a maintainer-waiver comment rerun — the + // gate must reset the boxes and re-draft. + const result = await run({ + pr: { + base: { ref: "dev" }, + title: "GUI: fix provider list spacing", + body: readinessChecklistBody(4), + }, + eventName: "issue_comment", + eventAction: "created", + comments: [ + { id: 1, user: { login: "wibias" }, author_association: "COLLABORATOR", body: "not touching gui" }, + ], + maintainersFile: MAINTAINERS_FIXTURE, + comments: [readinessComment({ + version: 2, + autoDraftedByBot: false, + maintainersPinged: true, + completedAtHeadSha: null, + })], + }); + + // The comment-triggered rerun delivers no head SHA, so the completed + // checklist cannot be attributed to the live head: the gate resets the + // boxes and keeps the PR in draft. + const resetBody = callsTo(result, "pulls.update") as [{ body: string }]; + expect(resetBody[0]!.body).toContain(CHECKLIST_START); + expect(resetBody[0]!.body).not.toContain("- [x]"); + expect(resetBody[0]!.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(resetBody[0]!.body).toContain("- [ ] My PR is ready for review."); + }); + test("a non-maintainer issue_comment does not re-run the gate", async () => { // The `issue_comment` trigger must only re-run for maintainer comments // (OWNER / COLLABORATOR / MEMBER). A random comment from a contributor From a0740f7f1950960fb52ae1021153a5252639c5a8 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:16:54 +0200 Subject: [PATCH 6/8] fix(ci): queue (not cancel) shared gate-comment writes and prove interleaving Maintainer review blocker: both workflows read-modify-write the same consolidated gate comment, but pr-hygiene used cancel-in-progress: true. With the shared per-PR concurrency group, a newer hygiene run could cancel an in-flight gate mutation, losing that read-modify-write. Set cancel-in-progress: false so runs in the shared group queue, matching the enforce-target workflow. Add an interleaving regression test: a gate rebuild followed by a hygiene update (and the reverse order) preserves both the gate status and the hygiene block, with exactly one block each way. Pin the non-cancelling shared group in the workflow structure test. --- .github/scripts/pr-quality-messages.test.cjs | 66 ++++++++++++++++++++ .github/workflows/pr-hygiene.yml | 5 +- tests/ci-workflows.test.ts | 4 ++ 3 files changed, 74 insertions(+), 1 deletion(-) diff --git a/.github/scripts/pr-quality-messages.test.cjs b/.github/scripts/pr-quality-messages.test.cjs index e24f3addda..469ed8ccc4 100644 --- a/.github/scripts/pr-quality-messages.test.cjs +++ b/.github/scripts/pr-quality-messages.test.cjs @@ -351,4 +351,70 @@ describe("hygiene section round-trip", () => { assert.equal(updated.split(HYGIENE_BLOCK_START).length - 1, 1); assert.equal(updated.split(HYGIENE_BLOCK_END).length - 1, 1); }); + + it("preserves both sections across an interleaved gate rebuild and hygiene update", () => { + // The gate and hygiene workflows share one concurrency group, but the + // merge helpers must also be order-independent: whichever write lands + // second must preserve the other's section. Start with a gate comment + // carrying a hygiene block, apply a gate rebuild, then a hygiene update, + // and assert both the gate status and the hygiene status survive. + const withBlock = [ + GATE_MARKER, + '', + "", + "## ✅ READY", + "- all PR quality gates passed.", + "", + "## Hygiene", + "", + HYGIENE_BLOCK_START, + "", + "", + "✅ **Deterministic PR hygiene checks passed.**", + "", + HYGIENE_BLOCK_END, + ].join("\n"); + + // Gate rebuild (the gate rewrites its own section, preserving hygiene). + const afterGate = buildGateCommentBody( + { version: 1, active: false }, + { + status: "READY", + statusReason: "all PR quality gates passed.", + checklistRequired: false, + hygiene: ["✅ **Deterministic PR hygiene checks passed.**"], + }, + ).join("\n"); + + // Hygiene update (the hygiene workflow rewrites its block, preserving gate). + const afterHygiene = withHygieneSection(afterGate, [ + "✅ **Deterministic PR hygiene checks passed.**", + ]); + + assert.ok(afterHygiene.includes(GATE_MARKER)); + assert.ok(afterHygiene.includes("## ✅ READY")); + assert.ok(afterHygiene.includes("✅ **Deterministic PR hygiene checks passed.**")); + assert.equal(afterHygiene.split(HYGIENE_BLOCK_START).length - 1, 1); + assert.equal(afterHygiene.split(HYGIENE_BLOCK_END).length - 1, 1); + + // Reverse order: hygiene first, then gate rebuild — same invariant. + const afterHygieneFirst = withHygieneSection(withBlock, [ + "⚠️ **Deterministic hygiene checks failed.**", + "- `missing_regression_test` — Behavior changed under `src/` without a test change.", + ]); + const afterGateSecond = buildGateCommentBody( + { version: 1, active: false }, + { + status: "READY", + statusReason: "all PR quality gates passed.", + checklistRequired: false, + hygiene: ["⚠️ **Deterministic hygiene checks failed.**", "- `missing_regression_test` — Behavior changed under `src/` without a test change."], + }, + ).join("\n"); + assert.ok(afterGateSecond.includes(GATE_MARKER)); + assert.ok(afterGateSecond.includes("## ✅ READY")); + assert.ok(afterGateSecond.includes("⚠️ **Deterministic hygiene checks failed.**")); + assert.equal(afterGateSecond.split(HYGIENE_BLOCK_START).length - 1, 1); + assert.equal(afterGateSecond.split(HYGIENE_BLOCK_END).length - 1, 1); + }); }); diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml index e99a765653..7d42712cb6 100644 --- a/.github/workflows/pr-hygiene.yml +++ b/.github/workflows/pr-hygiene.yml @@ -12,8 +12,11 @@ permissions: {} concurrency: # Shared with the enforce-target gate: both workflows read-modify-write the # same consolidated gate comment, so one per-PR group serializes them. + # `cancel-in-progress` stays false (the enforce-target gate also omits it): + # a newer run must queue behind the in-flight one, never cancel it mid + # comment mutation, or the cancelled run's read-modify-write is lost. group: pr-gate-comment-${{ github.event.pull_request.number }} - cancel-in-progress: true + cancel-in-progress: false jobs: hygiene: diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 5e2aa42caf..b6ff270399 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -891,6 +891,10 @@ describe("GitHub Actions hardening", () => { expect(hygieneWorkflow.concurrency?.group).toBe( "pr-gate-comment-${{ github.event.pull_request.number }}", ); + // Both comment-writing workflows share the group and neither cancels: + // `cancel-in-progress: true` would kill an in-flight gate mutation when a + // newer hygiene run starts, losing that read-modify-write. + expect(hygieneWorkflow.concurrency?.["cancel-in-progress"]).toBe(false); // One job, and it is this one. An audit round added a `sidecar:` job that // inherited the PR-write token and un-drafted the PR — every assertion below From 8c4ad03f27959f17f22c23d86bb101fd09b3f4ec Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:21:13 +0200 Subject: [PATCH 7/8] fix(test): merge duplicate comments fixture and consume hygiene in reverse-order test Two CodeRabbit findings on the interleaving and issue_comment tests: - The issue_comment checklist-provenance test passed two `comments` keys; the second overwrote the maintainer waiver comment. Merge both fixtures into one array so the waiver is actually delivered to the harness. - The reverse-order interleaving test built afterHygieneFirst but never consumed it; afterGateSecond used a hard-coded hygiene array, so the test passed even if the gate rebuild discarded the prior hygiene update. Extract the hygiene content from afterHygieneFirst via extractHygieneSection and feed it into the gate rebuild. --- .github/scripts/pr-quality-messages.test.cjs | 7 ++++++- tests/ci-workflows.test.ts | 12 ++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/.github/scripts/pr-quality-messages.test.cjs b/.github/scripts/pr-quality-messages.test.cjs index 469ed8ccc4..2cd474df7c 100644 --- a/.github/scripts/pr-quality-messages.test.cjs +++ b/.github/scripts/pr-quality-messages.test.cjs @@ -402,13 +402,18 @@ describe("hygiene section round-trip", () => { "⚠️ **Deterministic hygiene checks failed.**", "- `missing_regression_test` — Behavior changed under `src/` without a test change.", ]); + // The gate rebuild must consume the hygiene content the hygiene update + // wrote, not a hard-coded copy — otherwise the test passes even if the + // rebuild discards the prior update. + const extractedHygiene = extractHygieneSection(afterHygieneFirst); + assert.ok(extractedHygiene, "hygiene block must survive the hygiene update"); const afterGateSecond = buildGateCommentBody( { version: 1, active: false }, { status: "READY", statusReason: "all PR quality gates passed.", checklistRequired: false, - hygiene: ["⚠️ **Deterministic hygiene checks failed.**", "- `missing_regression_test` — Behavior changed under `src/` without a test change."], + hygiene: extractedHygiene.split("\n"), }, ).join("\n"); assert.ok(afterGateSecond.includes(GATE_MARKER)); diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index b6ff270399..eb91b07948 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -2702,14 +2702,14 @@ describe("GitHub Actions hardening", () => { eventAction: "created", comments: [ { id: 1, user: { login: "wibias" }, author_association: "COLLABORATOR", body: "not touching gui" }, + readinessComment({ + version: 2, + autoDraftedByBot: false, + maintainersPinged: true, + completedAtHeadSha: null, + }), ], maintainersFile: MAINTAINERS_FIXTURE, - comments: [readinessComment({ - version: 2, - autoDraftedByBot: false, - maintainersPinged: true, - completedAtHeadSha: null, - })], }); // The comment-triggered rerun delivers no head SHA, so the completed From 22cd62cb53c35731e415b7ec2e2090d7d12ed7f8 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:28:01 +0200 Subject: [PATCH 8/8] fix(ci): require canonical maintainer for issue_comment gate reruns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer security blocker: OWNER/COLLABORATOR/MEMBER association is broader than the repository's canonical maintainer list, so a non-maintainer collaborator or member could start the write-capable gate and cause PR comment, label, title, and draft-state mutations. Before pulls.get or any mutation, the issue_comment guard now loads the trusted MAINTAINERS.md list (case-insensitively) and requires the commenter's login to be in it. The association check remains as the cheap job-level prefilter; the canonical list is the authorization. Regression: a COLLABORATOR who is not in MAINTAINERS.md cannot re-run the gate — no PR lookup, no writes, no GraphQL mutation. --- .github/workflows/enforce-pr-target.yml | 21 ++++++++++--- tests/ci-workflows.test.ts | 36 ++++++++++++++++++++++ tests/helpers/enforce-pr-target-harness.ts | 8 ++++- 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 018872eedd..910baacd62 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -162,12 +162,25 @@ jobs: const isPrComment = context.payload.issue?.pull_request != null; const association = context.payload.comment?.author_association; - const isMaintainer = ["OWNER", "COLLABORATOR", "MEMBER"].includes( - association + // The association is a cheap prefilter, but OWNER/COLLABORATOR/ + // MEMBER is broader than this repository's canonical maintainer + // list. A collaborator or member who is not a maintainer must not + // start this write-capable gate, so require the commenter's + // login to be in the trusted MAINTAINERS.md list too. + const maintainerLogins = new Set( + readMaintainerLogins().map(login => login.toLowerCase()) ); - if (!isPrComment || !isMaintainer) { + const commenter = context.payload.comment?.user?.login; + const isCanonicalMaintainer = + typeof commenter === "string" && + maintainerLogins.has(commenter.toLowerCase()); + if ( + !isPrComment || + !["OWNER", "COLLABORATOR", "MEMBER"].includes(association) || + !isCanonicalMaintainer + ) { core.info( - "issue_comment not from a maintainer on a PR; skipping the gate." + "issue_comment not from a canonical maintainer on a PR; skipping the gate." ); return; } diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index eb91b07948..c419de5968 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -2784,6 +2784,42 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).not.toContain("graphql"); }); + test("a COLLABORATOR who is not in MAINTAINERS.md cannot re-run the gate", async () => { + // OWNER/COLLABORATOR/MEMBER association is broader than the canonical + // maintainer list. A collaborator or member who is absent from + // MAINTAINERS.md must not start the write-capable gate — no PR lookup, + // no comment/label/title/draft mutations. + const result = await run({ + pr: { + base: { ref: "dev" }, + title: "GUI: fix provider list spacing", + body: [ + "## Summary", + "This change fixes the provider list spacing in the dashboard.", + "", + "## Test plan", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"), + }, + eventName: "issue_comment", + eventAction: "created", + commentAuthorAssociation: "COLLABORATOR", + commentAuthorLogin: "someone-else", + maintainersFile: MAINTAINERS_FIXTURE, + comments: [ + { id: 1, user: { login: "someone-else" }, author_association: "COLLABORATOR", body: "not touching gui" }, + ], + }); + + // The in-script guard reads MAINTAINERS.md and skips before pulls.get: + // no PR lookup, no writes, no GraphQL mutation. + expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(false); + expect(methodsOf(result)).not.toContain("pulls.get"); + expect(methodsOf(result)).not.toContain("issues.createComment"); + expect(methodsOf(result)).not.toContain("issues.updateComment"); + expect(methodsOf(result)).not.toContain("graphql"); + }); + test("the gate comment preserves an existing hygiene section across rebuilds", async () => { // The hygiene workflow writes its status into the same consolidated gate // comment. When the gate rebuilds that comment, it must carry the diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index abc8c6f8c5..9f4710a217 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -95,6 +95,12 @@ export type RunOptions = { * associations (OWNER / COLLABORATOR / MEMBER). */ commentAuthorAssociation?: string; + /** + * Login of the commenter on an `issue_comment` event. Defaults to + * `"wibias"`. The gate requires the commenter to be in the trusted + * MAINTAINERS.md list, so tests can set a non-maintainer login here. + */ + commentAuthorLogin?: string; /** * Whether the commented-on issue is a pull request. Defaults to `true`. * An `issue_comment` on a plain issue must not start this PR-only gate. @@ -860,7 +866,7 @@ export async function runEnforcePrTarget( comment: { id: 424242, body: "not touching gui", - user: { login: "wibias" }, + user: { login: options.commentAuthorLogin ?? "wibias" }, author_association: options.commentAuthorAssociation ?? "COLLABORATOR", }, }