Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 15 additions & 16 deletions .github/scripts/enforce-pr-target.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,17 @@ 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("uses label events for GUI waivers and a trusted CodeRabbit status signal", () => {
assert.doesNotMatch(workflow, /^ issue_comment:/m);
assert.match(workflow, /- labeled/);
assert.match(workflow, /- unlabeled/);
assert.match(workflow, /^ status:/m);
assert.match(workflow, /github\.event\.context == 'CodeRabbit'/);
assert.match(workflow, /github\.event\.state == 'success'/);
assert.match(workflow, /github\.event\.label\.name == 'gui-screenshot-waived'/);
assert.match(workflow, /listPullRequestsAssociatedWithCommit/);
assert.match(workflow, /candidate\.head\?\.sha === statusSha/);
assert.match(workflow, /candidates\.length !== 1/);
});

it("does not add review events that would break the trusted-base model", () => {
Expand Down Expand Up @@ -141,13 +141,12 @@ 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}/);
// `pull_request_target` pins the PR base SHA. Privileged `issue_comment`
// runs must source scripts from the repository default branch, matching
// the branch that supplied the workflow itself; unpromoted `dev` scripts
// must never execute under the write-capable token.
// `pull_request_target` pins the PR base SHA. Trusted `status`
// revalidation has no pull_request payload, so it sources scripts from the
// repository default branch that supplied the privileged workflow itself.
assert.match(
checkoutStep,
/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*\}\}/,
/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*\}\}/,
);
assert.doesNotMatch(checkoutStep, /\|\|\s*'dev'/);
// The readiness ping reads MAINTAINERS.md from the same trusted checkout.
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/enforce-issue-quality.yml
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ jobs:

translate-comment:
name: Translate non-English issue comments
if: github.event_name == 'issue_comment'
if: github.event_name == 'issue_comment' && github.event.issue.pull_request == null && github.event.comment.user.type != 'Bot'
runs-on: ubuntu-latest
concurrency:
# Shares the per-issue queue with the `translate` job: both jobs RMW the
Expand Down
264 changes: 178 additions & 86 deletions .github/workflows/enforce-pr-target.yml

Large diffs are not rendered by default.

7 changes: 3 additions & 4 deletions .github/workflows/pr-hygiene.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,9 @@ 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.
# same consolidated gate comment, so one stable PR-number group serializes
# old-head and new-head runs as well as hygiene and gate writes.
# A newer run queues behind the in-flight one instead of cancelling it.
group: pr-gate-comment-${{ github.event.pull_request.number }}
cancel-in-progress: false

Expand Down
71 changes: 71 additions & 0 deletions devlog/_plan/260808_workflow_comment_spam_hardening/000_plan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# Workflow comment-spam hardening implementation plan

> **For agentic workers:** execute this plan test-first. Do not broaden workflow permissions or execute pull-request head code with a write-capable token.

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

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

**Tech stack:** GitHub Actions YAML, `actions/github-script`, Bun tests, existing PR-gate scripts.

## Global constraints

- PR targets `dev`.
- Workflow changes become live only after promotion to default branch `main`.
- Never checkout or execute PR-head code in a workflow with write permissions.
- Preserve real-time non-English issue-comment translation.
- Preserve CodeRabbit/Codex review-thread verification as the source of truth; review/comment bodies are trigger signals only, never trusted gate evidence.
- Do not claim that a job-level `if` removes an `issue_comment` workflow-run entry: it only prevents runner allocation for filtered comments.

## Task 1: Stop PR and bot comments from allocating issue-quality runners

**Files:**
- Modify: `.github/workflows/enforce-issue-quality.yml`
- Modify: `tests/ci-workflows.test.ts`

- [ ] Add regression assertions requiring the `translate-comment` job to run only for `issue_comment` events on real issues and non-bot authors.
- [ ] Run the focused workflow test and confirm it fails against the current workflow.
- [ ] Add the minimal job-level guard: exclude `github.event.issue.pull_request != null` and bot-authored comments before checkout/setup/AI steps.
- [ ] Re-run the focused workflow test and confirm it passes.

## Task 2: Replace CodeRabbit status-comment gate triggers with a trusted commit-status signal

**Files:**
- Modify: `.github/workflows/enforce-pr-target.yml`
- Replace: `tests/zz-pr-coderabbit-readiness-revalidation.test.ts`

- [ ] Require no `issue_comment`, `pull_request_review`, or PR-controlled signal workflow for CodeRabbit revalidation.
- [ ] Consume CodeRabbit's successful `CodeRabbit` commit status through the default-branch-only `status` event.
- [ ] Resolve the status SHA with `listPullRequestsAssociatedWithCommit` and continue only when exactly one open PR has that SHA as its current head.
- [ ] Treat status-triggered runs as signal-only head evidence and re-read live review threads/bodies before any write.
- [ ] Keep the write-capable checkout pinned to the trusted default branch for status events.

## Task 3: Move GUI screenshot waiver from maintainer comments to a label

**Files:**
- Modify: `.github/workflows/enforce-pr-target.yml`
- Modify: `tests/ci-workflows.test.ts`
- Modify: `docs-site/src/content/docs/contributing/pr-quality.md`

- [ ] Add regression assertions for `labeled` / `unlabeled` PR-target events and `gui-screenshot-waived` semantics.
- [ ] Confirm the new assertions fail against current behavior.
- [ ] Use `gui-screenshot-waived` as the only immediate GUI-waiver trigger, while preserving legacy maintainer-comment recognition on later PR events for compatibility.
- [ ] Document that the label is maintainer-controlled and that adding/removing it immediately re-evaluates the gate.
- [ ] Re-run focused workflow tests.

## Task 4: Verification and PR

- [ ] Run `bun test tests/zz-pr-coderabbit-readiness-revalidation.test.ts tests/ci-workflows.test.ts`.
- [ ] Run `node --test .github/scripts/*.test.cjs` because the gate still consumes those helpers.
- [ ] Run `bun run typecheck`.
- [ ] Run `git diff --check`.
- [ ] Verify the final diff contains no temporary implementation workflow or helper.
- [ ] Open a draft PR against `dev` with deployment note: event-driven workflow changes take effect only after promotion to `main`.

## Expected effect

- CodeRabbit PR status-comment edits no longer invoke `enforce-pr-target`.
- Ordinary maintainer PR comments no longer invoke `enforce-pr-target` merely to carry a GUI waiver.
- 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.
- Real issue comments from humans continue to translate in real time.
- 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.
30 changes: 19 additions & 11 deletions docs-site/src/content/docs/contributing/pr-quality.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,12 @@ tells you exactly what to change:
plan** (or equivalent substance). When the title or description mentions
`gui`, the description must include a screenshot of the UI change; the check
keeps the PR a draft and comments until the screenshot is present. A
maintainer (OWNER / COLLABORATOR / MEMBER — repository owners,
collaborators, and members) can waive the screenshot
requirement with an issue comment saying the change does not touch the GUI
(for example "no gui changes"); a contributor PR author cannot self-waive
(a maintainer who authors the PR can waive, but they already hold push
permission and are not gated by the contributor checklist).
maintainer can waive a false-positive GUI cue by adding the
`gui-screenshot-waived` label; adding or removing that label immediately
re-evaluates the gate. Legacy maintainer comments such as "no gui changes"
are still recognised on the next PR event for compatibility, but comments
themselves no longer trigger the privileged PR gate. A contributor cannot
self-waive the screenshot requirement.
Contributor PRs (authors without repository push permission) open in draft
and stay there until a four-box review-readiness checklist in the
description is complete: local CI green, the branch on the latest `dev`
Expand All @@ -71,6 +71,13 @@ tells you exactly what to change:
unticks the matching box and keeps the PR a draft. When the checklist is
complete and every gate is green, the gate adds a `review-ready` label as a
visible status marker at the ready moment.
CodeRabbit status-comment edits do not trigger the PR gate. CodeRabbit's
successful `CodeRabbit` commit status wakes the trusted default-branch gate
through the `status` event. The gate maps that status SHA to exactly one open
PR whose current head still matches, then re-reads live review threads and
review bodies before changing checklist, label, comment, or draft state. An
ambiguous or stale SHA association is ignored, and no PR-head code is
executed with the gate's write-capable token.

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

### When a workflow change takes effect

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

## Sponsored surfaces

Expand Down
2 changes: 1 addition & 1 deletion structure/06_docs-and-release.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ bun run build
| `.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. |
| `.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. |
| `.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. |
| `.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. |
| `.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. |
| `.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. |
| `.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. |
| `.github/workflows/issue-triage.yml` | `issues` (opened) | Duplicate detection and triage labeling for new issues. |
Expand Down
Loading
Loading