Skip to content

Commit 57140d6

Browse files
authored
Merge pull request #1265 from lidge-jun/hotfix/workflow-comment-spam-hardening-main
fix(ci): promote workflow comment-spam hardening to main - Hotfix
2 parents 726e1c7 + ac2c446 commit 57140d6

10 files changed

Lines changed: 618 additions & 261 deletions

File tree

.github/scripts/enforce-pr-target.test.cjs

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -49,17 +49,17 @@ describe("enforce-pr-target workflow", () => {
4949
assert.match(workflow, /synchronize/);
5050
});
5151

52-
it("re-runs on issue_comment so a maintainer GUI waiver takes effect", () => {
53-
// The GUI-screenshot gate is waived by a maintainer issue comment
54-
// ("not touching gui"). `pull_request_target` types do not include issue
55-
// comments, so without this trigger the waiver sits unread until a PR
56-
// edit or push re-runs the gate.
57-
assert.match(workflow, /^ issue_comment:/m);
58-
assert.match(workflow, /- created/);
59-
assert.match(workflow, /- edited/);
60-
// The script resolves the PR number from the issue payload, which is what
61-
// an issue_comment event delivers instead of a pull_request object.
62-
assert.match(workflow, /context\.payload\.issue\?\.number/);
52+
it("uses label events for GUI waivers and a trusted CodeRabbit status signal", () => {
53+
assert.doesNotMatch(workflow, /^ issue_comment:/m);
54+
assert.match(workflow, /- labeled/);
55+
assert.match(workflow, /- unlabeled/);
56+
assert.match(workflow, /^ status:/m);
57+
assert.match(workflow, /github\.event\.context == 'CodeRabbit'/);
58+
assert.match(workflow, /github\.event\.state == 'success'/);
59+
assert.match(workflow, /github\.event\.label\.name == 'gui-screenshot-waived'/);
60+
assert.match(workflow, /listPullRequestsAssociatedWithCommit/);
61+
assert.match(workflow, /candidate\.head\?\.sha === statusSha/);
62+
assert.match(workflow, /candidates\.length !== 1/);
6363
});
6464

6565
it("does not add review events that would break the trusted-base model", () => {
@@ -141,13 +141,12 @@ describe("enforce-pr-target workflow", () => {
141141
.split("- name: Checkout trusted PR-quality scripts")[1]
142142
.split(/\n {6}- name:/)[0];
143143
assert.match(checkoutStep, /actions\/checkout@[0-9a-f]{40}/);
144-
// `pull_request_target` pins the PR base SHA. Privileged `issue_comment`
145-
// runs must source scripts from the repository default branch, matching
146-
// the branch that supplied the workflow itself; unpromoted `dev` scripts
147-
// must never execute under the write-capable token.
144+
// `pull_request_target` pins the PR base SHA. Trusted `status`
145+
// revalidation has no pull_request payload, so it sources scripts from the
146+
// repository default branch that supplied the privileged workflow itself.
148147
assert.match(
149148
checkoutStep,
150-
/ref:\s*\$\{\{\s*github\.event_name\s*==\s*'issue_comment'\s*&&\s*github\.event\.repository\.default_branch\s*\|\|\s*github\.event\.pull_request\.base\.sha\s*\}\}/,
149+
/ref:\s*\$\{\{\s*github\.event_name\s*==\s*'status'\s*&&\s*github\.event\.repository\.default_branch\s*\|\|\s*github\.event\.pull_request\.base\.sha\s*\}\}/,
151150
);
152151
assert.doesNotMatch(checkoutStep, /\|\|\s*'dev'/);
153152
// The readiness ping reads MAINTAINERS.md from the same trusted checkout.

.github/workflows/enforce-issue-quality.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -443,7 +443,7 @@ jobs:
443443
444444
translate-comment:
445445
name: Translate non-English issue comments
446-
if: github.event_name == 'issue_comment'
446+
if: github.event_name == 'issue_comment' && github.event.issue.pull_request == null && github.event.comment.user.type != 'Bot'
447447
runs-on: ubuntu-latest
448448
concurrency:
449449
# Shares the per-issue queue with the `translate` job: both jobs RMW the

.github/workflows/enforce-pr-target.yml

Lines changed: 178 additions & 86 deletions
Large diffs are not rendered by default.

.github/workflows/pr-hygiene.yml

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,9 @@ permissions: {}
1111

1212
concurrency:
1313
# Shared with the enforce-target gate: both workflows read-modify-write the
14-
# same consolidated gate comment, so one per-PR group serializes them.
15-
# `cancel-in-progress` stays false (the enforce-target gate also omits it):
16-
# a newer run must queue behind the in-flight one, never cancel it mid
17-
# comment mutation, or the cancelled run's read-modify-write is lost.
14+
# same consolidated gate comment, so one stable PR-number group serializes
15+
# old-head and new-head runs as well as hygiene and gate writes.
16+
# A newer run queues behind the in-flight one instead of cancelling it.
1817
group: pr-gate-comment-${{ github.event.pull_request.number }}
1918
cancel-in-progress: false
2019

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
# Workflow comment-spam hardening implementation plan
2+
3+
> **For agentic workers:** execute this plan test-first. Do not broaden workflow permissions or execute pull-request head code with a write-capable token.
4+
5+
**Goal:** Reduce GitHub Actions noise and runner consumption caused by `issue_comment` while preserving issue-comment translation and the PR readiness gate's ability to invalidate a ready PR when CodeRabbit reports new findings.
6+
7+
**Architecture:** Keep `issue_comment` only where GitHub offers no narrower native trigger: real-time issue-comment translation. Revalidate CodeRabbit readiness from its `CodeRabbit` commit status using the default-branch-only `status` event, then resolve the status SHA to exactly one open PR before the privileged gate writes anything. Make `gui-screenshot-waived` the immediate maintainer-controlled waiver trigger while preserving legacy maintainer-comment recognition on later PR events for compatibility.
8+
9+
**Tech stack:** GitHub Actions YAML, `actions/github-script`, Bun tests, existing PR-gate scripts.
10+
11+
## Global constraints
12+
13+
- PR targets `dev`.
14+
- Workflow changes become live only after promotion to default branch `main`.
15+
- Never checkout or execute PR-head code in a workflow with write permissions.
16+
- Preserve real-time non-English issue-comment translation.
17+
- Preserve CodeRabbit/Codex review-thread verification as the source of truth; review/comment bodies are trigger signals only, never trusted gate evidence.
18+
- Do not claim that a job-level `if` removes an `issue_comment` workflow-run entry: it only prevents runner allocation for filtered comments.
19+
20+
## Task 1: Stop PR and bot comments from allocating issue-quality runners
21+
22+
**Files:**
23+
- Modify: `.github/workflows/enforce-issue-quality.yml`
24+
- Modify: `tests/ci-workflows.test.ts`
25+
26+
- [ ] Add regression assertions requiring the `translate-comment` job to run only for `issue_comment` events on real issues and non-bot authors.
27+
- [ ] Run the focused workflow test and confirm it fails against the current workflow.
28+
- [ ] Add the minimal job-level guard: exclude `github.event.issue.pull_request != null` and bot-authored comments before checkout/setup/AI steps.
29+
- [ ] Re-run the focused workflow test and confirm it passes.
30+
31+
## Task 2: Replace CodeRabbit status-comment gate triggers with a trusted commit-status signal
32+
33+
**Files:**
34+
- Modify: `.github/workflows/enforce-pr-target.yml`
35+
- Replace: `tests/zz-pr-coderabbit-readiness-revalidation.test.ts`
36+
37+
- [ ] Require no `issue_comment`, `pull_request_review`, or PR-controlled signal workflow for CodeRabbit revalidation.
38+
- [ ] Consume CodeRabbit's successful `CodeRabbit` commit status through the default-branch-only `status` event.
39+
- [ ] Resolve the status SHA with `listPullRequestsAssociatedWithCommit` and continue only when exactly one open PR has that SHA as its current head.
40+
- [ ] Treat status-triggered runs as signal-only head evidence and re-read live review threads/bodies before any write.
41+
- [ ] Keep the write-capable checkout pinned to the trusted default branch for status events.
42+
43+
## Task 3: Move GUI screenshot waiver from maintainer comments to a label
44+
45+
**Files:**
46+
- Modify: `.github/workflows/enforce-pr-target.yml`
47+
- Modify: `tests/ci-workflows.test.ts`
48+
- Modify: `docs-site/src/content/docs/contributing/pr-quality.md`
49+
50+
- [ ] Add regression assertions for `labeled` / `unlabeled` PR-target events and `gui-screenshot-waived` semantics.
51+
- [ ] Confirm the new assertions fail against current behavior.
52+
- [ ] Use `gui-screenshot-waived` as the only immediate GUI-waiver trigger, while preserving legacy maintainer-comment recognition on later PR events for compatibility.
53+
- [ ] Document that the label is maintainer-controlled and that adding/removing it immediately re-evaluates the gate.
54+
- [ ] Re-run focused workflow tests.
55+
56+
## Task 4: Verification and PR
57+
58+
- [ ] Run `bun test tests/zz-pr-coderabbit-readiness-revalidation.test.ts tests/ci-workflows.test.ts`.
59+
- [ ] Run `node --test .github/scripts/*.test.cjs` because the gate still consumes those helpers.
60+
- [ ] Run `bun run typecheck`.
61+
- [ ] Run `git diff --check`.
62+
- [ ] Verify the final diff contains no temporary implementation workflow or helper.
63+
- [ ] Open a draft PR against `dev` with deployment note: event-driven workflow changes take effect only after promotion to `main`.
64+
65+
## Expected effect
66+
67+
- CodeRabbit PR status-comment edits no longer invoke `enforce-pr-target`.
68+
- Ordinary maintainer PR comments no longer invoke `enforce-pr-target` merely to carry a GUI waiver.
69+
- PR and bot comments still create an `Enforce issue quality` workflow-run record because GitHub cannot filter `issue_comment` by PR-vs-issue at trigger time, but the translation job is skipped before runner allocation.
70+
- Real issue comments from humans continue to translate in real time.
71+
- New CodeRabbit reviews can still invalidate a previously completed findings claim through CodeRabbit's commit status and a default-branch, write-capable gate without executing untrusted PR code or trusting an ambiguous SHA-to-PR association.

docs-site/src/content/docs/contributing/pr-quality.md

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,12 @@ tells you exactly what to change:
4040
plan** (or equivalent substance). When the title or description mentions
4141
`gui`, the description must include a screenshot of the UI change; the check
4242
keeps the PR a draft and comments until the screenshot is present. A
43-
maintainer (OWNER / COLLABORATOR / MEMBER — repository owners,
44-
collaborators, and members) can waive the screenshot
45-
requirement with an issue comment saying the change does not touch the GUI
46-
(for example "no gui changes"); a contributor PR author cannot self-waive
47-
(a maintainer who authors the PR can waive, but they already hold push
48-
permission and are not gated by the contributor checklist).
43+
maintainer can waive a false-positive GUI cue by adding the
44+
`gui-screenshot-waived` label; adding or removing that label immediately
45+
re-evaluates the gate. Legacy maintainer comments such as "no gui changes"
46+
are still recognised on the next PR event for compatibility, but comments
47+
themselves no longer trigger the privileged PR gate. A contributor cannot
48+
self-waive the screenshot requirement.
4949
Contributor PRs (authors without repository push permission) open in draft
5050
and stay there until a four-box review-readiness checklist in the
5151
description is complete: local CI green, the branch on the latest `dev`
@@ -71,6 +71,13 @@ tells you exactly what to change:
7171
unticks the matching box and keeps the PR a draft. When the checklist is
7272
complete and every gate is green, the gate adds a `review-ready` label as a
7373
visible status marker at the ready moment.
74+
CodeRabbit status-comment edits do not trigger the PR gate. CodeRabbit's
75+
successful `CodeRabbit` commit status wakes the trusted default-branch gate
76+
through the `status` event. The gate maps that status SHA to exactly one open
77+
PR whose current head still matches, then re-reads live review threads and
78+
review bodies before changing checklist, label, comment, or draft state. An
79+
ambiguous or stale SHA association is ignored, and no PR-head code is
80+
executed with the gate's write-capable token.
7481

7582
- **Hygiene.** Behavior changes need a test; new lint or type suppressions,
7683
focused or skipped tests, empty catch blocks, edited generated output, and a
@@ -98,11 +105,12 @@ right; say why when it is wrong. It does not block a merge.
98105

99106
### When a workflow change takes effect
100107

101-
`enforce-target` and `label` run on `pull_request_target`, which GitHub always
102-
loads from the repository **default branch**. A change to either takes effect
103-
only after it is promoted to `main` — merging it to `dev` does not change live
104-
behavior. The cross-platform CI workflow runs on `pull_request` and takes effect
105-
as soon as it is on the branch being targeted.
108+
`enforce-target` and `label` use trusted default-branch automation. The PR gate
109+
runs on `pull_request_target` and on CodeRabbit `status` events, both loaded
110+
from the repository default branch; the write-capable behavior therefore
111+
changes only after the gate revision is promoted to `main`. The cross-platform
112+
CI workflow runs on `pull_request` and takes effect as soon as it is on the
113+
branch being targeted.
106114

107115
## Sponsored surfaces
108116

structure/06_docs-and-release.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ bun run build
4444
| `.github/workflows/release.yml` | Manual dispatch only | npm publish/dry-run workflow. It requires the exact `GITHUB_SHA` to have a successful Cross-platform CI run before publish or dry-run. |
4545
| `.github/workflows/deploy-docs.yml` | `push` to `main` touching `docs-site/**` or the workflow, or manual dispatch | Build and publish the Astro/Starlight docs site to GitHub Pages. |
4646
| `.github/workflows/service-lifecycle.yml` | `pull_request` to `main`/`dev` and `push`, both filtered on the service path set (`src/service.ts`, `src/cli.ts`, `src/cli/index.ts`, `src/lib/bun-runtime.ts`, `package.json`, `bun.lock`, the workflow), or manual dispatch | Service-lifecycle smoke on three platforms: Linux systemd, macOS launchd, and Windows Scheduled Tasks. Each installs, verifies, stops via `ocx stop`, and uninstalls. The path list is kept in sync with the `release.yml` service-gate regex. |
47-
| `.github/workflows/enforce-pr-target.yml` | `pull_request_target` (opened, reopened, edited, ready_for_review, synchronize) | The `enforce-target` gate: rejects pull requests whose head ancestry sits on the `main` tip while far behind `dev`, rejects empty or malformed descriptions, requires a GUI screenshot when the title/body mentions `gui` (waivable by a maintainer comment), keeps contributor PRs in draft until a four-box readiness checklist is complete, verifies the CI / latest-dev / Codex+CodeRabbit-findings claims (review threads plus current-head CodeRabbit review-body findings outside the diff range), and adds a `review-ready` status label at the ready moment. Stacked child PRs targeting another open PR's head skip the wrong-base gate. |
47+
| `.github/workflows/enforce-pr-target.yml` | `pull_request_target` (opened, reopened, edited, labeled, unlabeled, ready_for_review, synchronize) plus default-branch `status` events filtered to successful `CodeRabbit` statuses | The `enforce-target` gate: rejects pull requests whose head ancestry sits on the `main` tip while far behind `dev`, rejects empty or malformed descriptions, requires a GUI screenshot when the title/body mentions `gui` (immediately waivable with the maintainer-controlled `gui-screenshot-waived` label; legacy maintainer comments remain compatibility evidence on later PR events), keeps contributor PRs in draft until a four-box readiness checklist is complete, verifies the CI / latest-dev / Codex+CodeRabbit-findings claims (review threads plus current-head CodeRabbit review-body findings outside the diff range), and adds a `review-ready` status label at the ready moment. CodeRabbit status SHAs must resolve to exactly one open current-head PR before writes. Stacked child PRs targeting another open PR's head skip the wrong-base gate. |
4848
| `.github/workflows/enforce-issue-quality.yml` | `issues` (opened, edited, reopened), `issue_comment` (created, edited), or manual dispatch with an issue number | Issue-template compliance gate. |
4949
| `.github/workflows/issue-quality-tests.yml` | `pull_request` and `push` filtered on the issue/PR automation scripts, templates, and their workflows | Tests the issue and PR automation scripts themselves, so the gates cannot rot silently. |
5050
| `.github/workflows/issue-triage.yml` | `issues` (opened) | Duplicate detection and triage labeling for new issues. |

0 commit comments

Comments
 (0)