diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index bcda24f68d..5fc9455a7c 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 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*'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/); assert.match(checkoutStep, /persist-credentials:\s*false/); diff --git a/.github/scripts/pr-quality-messages.cjs b/.github/scripts/pr-quality-messages.cjs index 0f6c6619a9..aec05331b8 100644 --- a/.github/scripts/pr-quality-messages.cjs +++ b/.github/scripts/pr-quality-messages.cjs @@ -16,6 +16,20 @@ 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 = ""; +/** + * 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); @@ -57,7 +71,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 +99,64 @@ 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(HYGIENE_BLOCK_RE); + 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 (HYGIENE_BLOCK_RE.test(base)) { + return base.replace(HYGIENE_BLOCK_RE, 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 +323,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..2cd474df7c 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,167 @@ 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)); + }); + + 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); + }); + + 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.", + ]); + // 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: extractedHygiene.split("\n"), + }, + ).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/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 cc85e946d2..910baacd62 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,10 +28,24 @@ 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. 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: + # `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: @@ -33,8 +56,13 @@ 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 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 @@ -81,8 +109,12 @@ jobs: const { GATE_MARKER, READINESS_MARKER, + HYGIENE_MARKER, + HYGIENE_BLOCK_START, + HYGIENE_BLOCK_END, inlineCode, buildGateCommentBody, + extractHygieneSection, buildFailureSections, failureSummary, buildStaleNotice, @@ -115,7 +147,44 @@ 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; + + // 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; + // 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()) + ); + 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 canonical maintainer on a PR; skipping the gate." + ); + return; + } + } const { data: pr } = await github.rest.pulls.get({ owner, @@ -231,7 +300,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, @@ -474,8 +552,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/.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..7d42712cb6 100644 --- a/.github/workflows/pr-hygiene.yml +++ b/.github/workflows/pr-hygiene.yml @@ -10,8 +10,13 @@ on: permissions: {} concurrency: - group: pr-hygiene-${{ github.event.pull_request.number }} - cancel-in-progress: true + # 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: false jobs: hygiene: @@ -41,10 +46,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 +135,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/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..c419de5968 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,22 +874,49 @@ 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: + "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 }}", + ); + // 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 // 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. @@ -895,7 +936,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 || '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. @@ -946,6 +987,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 +1021,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 +2657,208 @@ 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("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" }, + readinessComment({ + version: 2, + autoDraftedByBot: false, + maintainersPinged: true, + completedAtHeadSha: null, + }), + ], + maintainersFile: MAINTAINERS_FIXTURE, + }); + + // 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 + // 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("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 + // 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: { @@ -4339,8 +4595,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 +4698,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..9f4710a217 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -82,6 +82,30 @@ 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; + /** + * `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; + /** + * 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. + */ + 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 @@ -822,9 +846,31 @@ 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, + ...(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: options.commentAuthorLogin ?? "wibias" }, + author_association: options.commentAuthorAssociation ?? "COLLABORATOR", + }, + } + : { pull_request: eventPr }), repository: { id: 987654321, name: "opencodex", @@ -838,7 +884,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";