From 4b9b44275a8aea35be460ddb28ab1f70459fbbe2 Mon Sep 17 00:00:00 2001 From: TzuHsuan <96853116+TzuH-Hsu@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:36:50 +0800 Subject: [PATCH 01/11] ci: fail the PR when its branch or its Closes line breaks the convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six downstream PRs were opened with a bare "Closes #" and a branch named chore/-slug — the PR template's comment dropped, the branch's issue slot empty — and nothing rejected them. scripts/pr-lint.js now checks the head branch is /-, the body links an issue with a GitHub closing keyword (HTML comments stripped first), and the branch's number is among the linked ones; release-please and Dependabot branches are exempt. ci.yml runs it as a step-level-if github-script step on pull_request events, so the required check fails with the fix named (ADR-0008 shape). scripts/check-node-tests.sh replaces check-issue-labeler.sh and runs every scripts/*.test.js. Closes #56 Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 35 +++++- .github/workflows/issue-labeler.yml | 2 +- AGENTS.md | 8 +- CONTRIBUTING.md | 6 +- Makefile | 2 +- ...DR-0008-event-workflow-logic-in-scripts.md | 6 +- docs/template/architecture.md | 7 +- docs/template/design-principles.md | 2 +- docs/template/upgrading.md | 2 +- scripts/check-issue-labeler.sh | 29 ----- scripts/check-node-tests.sh | 53 +++++++++ scripts/issue-labeler.test.js | 2 +- scripts/pr-lint.js | 110 ++++++++++++++++++ scripts/pr-lint.test.js | 99 ++++++++++++++++ skills/branch-and-commit/SKILL.md | 2 + skills/github-actions-hygiene/SKILL.md | 6 +- skills/pr-authoring/SKILL.md | 12 +- 17 files changed, 328 insertions(+), 55 deletions(-) delete mode 100755 scripts/check-issue-labeler.sh create mode 100755 scripts/check-node-tests.sh create mode 100644 scripts/pr-lint.js create mode 100644 scripts/pr-lint.test.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b06eff7..25b110a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,7 +5,8 @@ name: CI # Renaming the job key breaks every merge until the ruleset is updated to match. # # NEVER add a workflow-level `paths:`, `paths-ignore:`, or `branches:` filter to -# `on.pull_request`. A filtered-out workflow does not run at all, so the `ci` +# `on.pull_request`, and never make its `types:` anything less than a superset +# of the defaults. A filtered-out workflow does not run at all, so the `ci` # check is never created, and the PR parks forever on "Expected - waiting for # status to be reported" with no failing job to point at. The repo becomes # unmergeable and the cause is not visible in the Actions tab. @@ -35,9 +36,30 @@ name: CI # executes the PR's own Makefile and scripts. See docs/setup/runners.md. # # See docs/setup/runners.md before setting it. +# +# The `Lint the pull request` step (pull_request events only, step-level `if` +# so the job still reports) fails this required check when the head branch is +# not `/-` or the body links no issue — the logic is +# scripts/pr-lint.js (ADR-0008), tested by `make check`. Bot branches +# (release-please, Dependabot) are exempt. Because this job already executes +# the PR's own Makefile and scripts, it also runs the PR's own copy of +# pr-lint.js; a PR can therefore weaken its own lint, and the reviewer sees +# that in the diff. One difference from the `run:` steps: github-script is a +# JS action, so the PR's script runs with the job's GITHUB_TOKEN (read-only on +# fork PRs, `contents: read` here) and the run-scoped runtime token in its +# environment — nothing this public repository does not already expose, but +# not zero either, which is why the step stays tiny and does not take +# `github`. on: pull_request: + # A SUPERSET of the defaults (opened, synchronize, reopened). `edited` and + # `ready_for_review` are added so a PR that failed the lint on its body or + # title turns green when the author fixes it, without a push — "Re-run + # jobs" replays the ORIGINAL payload and would judge the old body. Never + # narrow this list: a `types:` without the three defaults is the + # "check never created" trap described above. + types: [opened, synchronize, reopened, edited, ready_for_review] push: branches: - main @@ -61,9 +83,18 @@ jobs: # scans every commit, not just the tip. Do not shallow this. fetch-depth: 0 # This job only reads. Keeping the token out of .git/config denies it - # to every later step (and to any tool binary they install). + # to every `run:` step (and to any tool binary they install); the one + # `uses:` step below is the exception the header describes. persist-credentials: false + - name: Lint the pull request + if: github.event_name == 'pull_request' + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const { run } = require(`${process.env.GITHUB_WORKSPACE}/scripts/pr-lint.js`); + await run({ context, core }); + - name: Set up Node.js uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: diff --git a/.github/workflows/issue-labeler.yml b/.github/workflows/issue-labeler.yml index b053984..9fa3fc5 100644 --- a/.github/workflows/issue-labeler.yml +++ b/.github/workflows/issue-labeler.yml @@ -4,7 +4,7 @@ name: Issue labeler # form fields on open/edit. The logic lives in scripts/issue-labeler.js # (ADR-0008); this file only checks out the default branch and calls it, so # it carries no adopter values and can be replaced whole on upgrade. The -# script is tested by `make check` (scripts/check-issue-labeler.sh). +# script is tested by `make check` (scripts/check-node-tests.sh). # # The issue body is untrusted input — it is only ever read inside the # actions/github-script JS sandbox (context.payload.issue.body), never diff --git a/AGENTS.md b/AGENTS.md index 64d54b0..b452cff 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,7 +16,7 @@ Agent-specific entry files (`CLAUDE.md`, `GEMINI.md`, `.github/copilot-instructi ## Build and validation -The Makefile is the only executable contract in this repository. CI calls make targets; customize the Makefile, never the workflows. Two exceptions, neither of which puts adopter values in the YAML: runner selection, which GitHub resolves before any make target exists to be called — set the `RUNNER_LABELS` repository variable instead of editing `runs-on` (see `docs/setup/runners.md`); and event handlers that need the token and payload, whose logic lives in `scripts/*.js` behind a thin `github-script` caller and is tested by `make check` (the issue labeler; ADR-0008). +The Makefile is the only executable contract in this repository. CI calls make targets; customize the Makefile, never the workflows. Two exceptions, neither of which puts adopter values in the YAML: runner selection, which GitHub resolves before any make target exists to be called — set the `RUNNER_LABELS` repository variable instead of editing `runs-on` (see `docs/setup/runners.md`); and event handlers that need the token or payload, whose logic lives in `scripts/*.js` behind a thin `github-script` caller and is tested by `make check` (the issue labeler, the PR lint; ADR-0008). | Level | Name | Command | When required | | --- | --- | --- | --- | @@ -39,15 +39,15 @@ The Makefile is the only executable contract in this repository. CI calls make t | `skills/` | Reusable knowledge modules (one directory per skill, `SKILL.md` inside) | | `docs/adr/` | Architecture Decision Records | | `docs/setup/` | Bootstrap and GitHub configuration guides | -| `scripts/` | Bootstrap, self-consistency checks, and the event-handler logic workflows call (`issue-labeler.js`; ADR-0008) | +| `scripts/` | Bootstrap, self-consistency checks, and the event-handler logic workflows call (`issue-labeler.js`, `pr-lint.js`; ADR-0008) | | `Makefile` | Canonical target contract (validation ladder entry points) | ## Workflow 1. **Issue** — created via issue forms; native type (Bug/Feature/Task) is set by the form; labels for priority/area follow `.github/PROJECT_FIELDS.md`. -2. **Branch** — `/-`; types mirror Conventional Commit types (`feat`, `fix`, `docs`, `chore`, `refactor`, `ci`). +2. **Branch** — `/-`; the type is one of the Conventional Commit types this repository uses: `feat`, `fix`, `docs`, `chore`, `refactor`, `ci`, `test`, `perf` (this line is the one home for that list — `scripts/pr-lint.js` enforces it on the branch and its test asserts equality). 3. **Commits** — Conventional Commits, English, imperative (`feat: add label sync phase to bootstrap`). -4. **PR** — English title in Conventional Commit format; body follows the PR template: summary, linked issue (`Closes #N`), validation ladder checkboxes, `RISK:` lines, rollback notes. +4. **PR** — English title in Conventional Commit format; body follows the PR template: summary, linked issue (`Closes #N`), validation ladder checkboxes, `RISK:` lines, rollback notes. CI fails the PR when the branch name or the `Closes #N` line is missing or malformed (`scripts/pr-lint.js`). 5. **Merge** — squash merge; the PR title becomes the commit message on `main`. ## AI agent conventions diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8b11560..f6f3dac 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -20,7 +20,7 @@ Install the lint tools used by `make lint-docs`: | Tool | Install | | --- | --- | | markdownlint-cli2 | `npm install -g markdownlint-cli2` | -| node (any LTS) | `brew install node` — runs `scripts/issue-labeler.test.js` in `make check` | +| node (any LTS) | `brew install node` — runs `scripts/*.test.js` in `make check` | | yamllint | `brew install yamllint` (or `pip install yamllint`) | | lychee | `brew install lychee` | | actionlint | `brew install actionlint` | @@ -49,8 +49,8 @@ skipped level. ## Branches & commits - Branch name: `/-`, e.g. `feat/42-label-sync`. -- Commits follow [Conventional Commits](https://www.conventionalcommits.org/): - `feat`, `fix`, `docs`, `chore`, `refactor`, `ci`, `test`, `perf`. +- Commits follow [Conventional Commits](https://www.conventionalcommits.org/), + using the types listed in `AGENTS.md` (Workflow, step 2). - Titles are English, imperative mood: `feat: add label sync phase to bootstrap`. - Breaking changes: append `!` after the type (`feat!: ...`) or add a `BREAKING CHANGE:` footer. diff --git a/Makefile b/Makefile index bc21fae..7d5f923 100644 --- a/Makefile +++ b/Makefile @@ -45,7 +45,7 @@ check: ## Run repo self-consistency scripts (skips scripts not yet added) @if [ -x scripts/check-local-md.sh ]; then scripts/check-local-md.sh; else echo "skip: scripts/check-local-md.sh not present yet"; fi @if [ -x scripts/check-license-marker.sh ]; then scripts/check-license-marker.sh; else echo "skip: scripts/check-license-marker.sh not present yet"; fi @if [ -x scripts/check-label-forms.sh ]; then scripts/check-label-forms.sh; else echo "skip: scripts/check-label-forms.sh not present yet"; fi - @if [ -x scripts/check-issue-labeler.sh ]; then scripts/check-issue-labeler.sh; else echo "skip: scripts/check-issue-labeler.sh not present yet"; fi + @if [ -x scripts/check-node-tests.sh ]; then scripts/check-node-tests.sh; else echo "skip: scripts/check-node-tests.sh not present yet"; fi lint: lint-docs lint-actions lint-secrets check ## L0 - aggregate all lint/consistency checks diff --git a/docs/adr/ADR-0008-event-workflow-logic-in-scripts.md b/docs/adr/ADR-0008-event-workflow-logic-in-scripts.md index f5550aa..8751011 100644 --- a/docs/adr/ADR-0008-event-workflow-logic-in-scripts.md +++ b/docs/adr/ADR-0008-event-workflow-logic-in-scripts.md @@ -12,8 +12,10 @@ 1. **The logic moves to `scripts/issue-labeler.js`**, a plain Node module with pure functions (`parseSections`, `matchAllowed`, `parseAllowedAreas`, `computeChanges`) and one I/O entry point (`run({github, context, core})`). The workflow checks out the default branch (`actions/checkout`, pinned, `persist-credentials: false`, `fetch-depth: 1`) and its `github-script` step is two lines: `require` the module, `await run(...)`. 2. **`labels.yml` is read from the checkout, not the API.** On `issues` events `GITHUB_SHA` is the head of the default branch, so the checkout holds exactly the merged declarations — the same trust boundary the API read had, without the extra request. -3. **The script is tested by `make check`** (`scripts/check-issue-labeler.sh` → `node --test scripts/issue-labeler.test.js`, node built-ins only). A missing `node` fails with an install hint, the way every other tool `make` needs does — never a green skip (validation-ladder rule 7). -4. **Rule 1 names the shape.** Event-driven workflows that genuinely need the token and payload keep their logic in `scripts/*.js`, called through `github-script` after a checkout; the workflow stays a thin caller and carries no adopter values. +3. **The script is tested by `make check`** (`scripts/check-node-tests.sh` → `node --test scripts/*.test.js`, node built-ins only). A missing `node` fails with an install hint, the way every other tool `make` needs does — never a green skip (validation-ladder rule 7). +4. **Rule 1 names the shape.** Event-driven workflows that genuinely need the token or the payload keep their logic in `scripts/*.js`, called through `github-script` after a checkout; the workflow stays a thin caller and carries no adopter values. + +The second instance is `scripts/pr-lint.js` (#56): the `Lint the pull request` step in `ci.yml` fails the required check when a PR's branch is not `/-` or its body links no issue. It needs the payload, not the token, and follows the same shape. ## Consequences diff --git a/docs/template/architecture.md b/docs/template/architecture.md index ff87f7a..0e1af9d 100644 --- a/docs/template/architecture.md +++ b/docs/template/architecture.md @@ -15,16 +15,17 @@ Why each piece of this repository exists, and what it costs to keep. A component | `.github/labels.yml` | Labels as code; bootstrap re-run = sync; carries the commented-out coarse-Type fallback (ADR-0006) | Edit alongside label changes; `FORM_MANAGED_TYPES` in `scripts/issue-labeler.js` must match (`scripts/check-label-forms.sh` enforces), and the commented block must stay commented | | `.github/ISSUE_TEMPLATE/` (3 forms) | Set native types; feed the labeler | Area/Priority/Subtype options must match `labels.yml` — `scripts/check-label-forms.sh` fails `make check` until they do | | `.github/PULL_REQUEST_TEMPLATE.md` | Validation ladder + RISK convention at point of use | Near zero | -| `.github/workflows/ci.yml` | L0 gate; installs tools via `make ci-tools`, runs `make ci-pr`. The only required status check — see its header before touching `on:` or `runs-on:` | SHA-pin bumps via Dependabot | +| `.github/workflows/ci.yml` | L0 gate; lints the PR itself (`scripts/pr-lint.js`), installs tools via `make ci-tools`, runs `make ci-pr`. The only required status check — see its header before touching `on:` or `runs-on:` | SHA-pin bumps via Dependabot | | `.github/workflows/issue-labeler.yml` | Thin caller: checkout + `require('scripts/issue-labeler.js')` (ADR-0008) | Two action SHA pins via Dependabot; no adopter values | -| `scripts/issue-labeler.js` (+ `.test.js`) | Form selections → labels (single-home preserving); `area:*` allowlist read from the checked-out `labels.yml` | `ALLOWED_PRIORITIES` / `ALLOWED_SUBTYPES` must match the forms — checked by `scripts/check-label-forms.sh`; behaviour pinned by `scripts/issue-labeler.test.js` via `make check` | +| `scripts/issue-labeler.js` (+ `.test.js`) | Form selections → labels (single-home preserving); `area:*` allowlist read from the checked-out `labels.yml` | `ALLOWED_PRIORITIES` / `ALLOWED_SUBTYPES` must match the forms — checked by `scripts/check-label-forms.sh`; behaviour pinned by its test via `make check` | +| `scripts/pr-lint.js` (+ `.test.js`) | Fails the required `ci` check when a PR's branch is not `/-` or its body links no issue; bot branches exempt | `TYPES` must equal the list in `AGENTS.md`'s Branch step — its test asserts it; behaviour pinned via `make check` | | `.github/workflows/maintenance.yml` | Weekly drift detectors: external link check + CI tool version check | Near zero | | `.github/workflows/release-please.yml` + configs | Human-gated release automation (ADR-0002) | Action SHA bumps; `initial-version` pins adopters' first release and is inert afterwards | | `.github/rulesets/main-branch.json` | Importable branch protection (PR + green `ci` required) | Near zero | | `LICENSE` | The template's own licence (MIT); bootstrap phase 9 replaces it with the adopter's choice and moves upstream attribution to `NOTICE` | Near zero — the holder line is asserted against `bootstrap.sh`'s constants by `scripts/check-license-marker.sh` | | `Makefile` | The only executable contract; adopter customization point | Grows with adopter stack, not with the template | | `scripts/bootstrap.sh` | Applies everything a template can't ship as files; idempotent sync | Highest-cost component — E2E-verified each release (below) | -| `scripts/check-*.sh` | Self-consistency: skills index, local-md hygiene, licence marker, label values repeated in forms and the labeler, the labeler's own tests | Near zero | +| `scripts/check-*.sh` | Self-consistency: skills index, local-md hygiene, licence marker, label values repeated in forms and the labeler, the `scripts/*.test.js` suites | Near zero | | `scripts/install-ci-tools.sh` | Checksum-verified CI tool installs; single home for all five tool version pins, shared by `ci.yml` and `maintenance.yml` via `make ci-tools`; bounds `RUNNER_LABELS` to linux x86_64 | Hand-bump a pin when the drift check flags it | | `scripts/check-tool-versions.sh` | Diffs those pins against upstream weekly and fails on drift — Dependabot cannot see them, so nothing else would | Near zero; add a row when a tool is added | | `docs/adr/` | Decision records; the "why" layer | Grows slowly by trigger criteria | diff --git a/docs/template/design-principles.md b/docs/template/design-principles.md index 5e85535..ac2884b 100644 --- a/docs/template/design-principles.md +++ b/docs/template/design-principles.md @@ -8,7 +8,7 @@ These principles decided every file in this repository. When proposing a change 2. **GitHub-native before third-party.** Native issue types, native sub-issues, native rulesets, Projects v2. A third-party tool must clear a high bar: solve something GitHub genuinely cannot. 3. **Every file justifies its maintenance cost.** If a file stops earning its keep, it gets deleted. The [architecture ledger](architecture.md) records what each piece is for. 4. **One home per fact.** Metadata (ADR-0003), documentation (`skills/docs-hygiene/`), configuration — duplicate homes always drift. -5. **Logic in the Makefile, not workflows.** Adopters customize `make` targets; workflow YAML stays untouched and upgradable. The one shape the Makefile cannot hold — an event handler that needs the token and the payload — lives in `scripts/` behind a two-line caller (ADR-0008), which keeps the YAML just as untouched. +5. **Logic in the Makefile, not workflows.** Adopters customize `make` targets; workflow YAML stays untouched and upgradable. The one shape the Makefile cannot hold — an event handler that needs the token or the payload — lives in `scripts/` behind a two-line caller (ADR-0008), which keeps the YAML just as untouched. 6. **The template obeys its own rules.** Its CI is the CI it ships; its issues use its own forms; its releases use its own flow. Dogfooding is the drift detector. 7. **AI agents are first-class, humans stay in control.** Agents share the same issue, PR, and validation contract as humans and load the same skills; humans decide what agents work on and gate merges, releases, and destructive operations. 8. **Ratchets over cleanup.** Label budgets, workflow-count budgets, handoff caps — prevention mechanisms, because every failure mode this template guards against is cheap to prevent and expensive to undo (see `skills/anti-patterns/`). diff --git a/docs/template/upgrading.md b/docs/template/upgrading.md index 1ab3a25..47d236e 100644 --- a/docs/template/upgrading.md +++ b/docs/template/upgrading.md @@ -15,7 +15,7 @@ Repositories created from a template share no git history with it, so updates ar git diff HEAD template/vX.Y.Z -- .github/ skills/ Makefile scripts/ ``` -3. Cherry-pick what you want by path. Good candidates: `skills/`, `.github/workflows/` (the labeler workflow only calls code that lives elsewhere — take it together with `scripts/issue-labeler.js`, `scripts/issue-labeler.test.js`, `scripts/check-issue-labeler.sh`, `scripts/check-label-forms.sh` and the two `check:` lines in the Makefile; `make check` tells you if the set is incomplete), `scripts/check-*.sh` (rarely customized locally). Careful candidates: `Makefile` (your `test` target lives there), `.github/labels.yml` (your renamed `area:*` labels), `AGENTS.md` (your conventions). +3. Cherry-pick what you want by path. Good candidates: `skills/`, `.github/workflows/` (the labeler workflow only calls code that lives elsewhere — take them together with `scripts/issue-labeler.js`, `scripts/pr-lint.js`, their `*.test.js`, `scripts/check-node-tests.sh`, `scripts/check-label-forms.sh` and the two `check:` lines in the Makefile; `make check` fails if a workflow requires a `scripts/*.js` that is not there), `scripts/check-*.sh` (rarely customized locally). Careful candidates: `Makefile` (your `test` target lives there), `.github/labels.yml` (your renamed `area:*` labels), `AGENTS.md` (your conventions). 4. Apply as a normal PR through your own CI. Never bulk-overwrite customized files. 5. Re-run `scripts/bootstrap.sh` if the update changed `labels.yml` or the ruleset — it syncs GitHub-side state to the files. diff --git a/scripts/check-issue-labeler.sh b/scripts/check-issue-labeler.sh deleted file mode 100755 index e502bd3..0000000 --- a/scripts/check-issue-labeler.sh +++ /dev/null @@ -1,29 +0,0 @@ -#!/usr/bin/env bash -# check-issue-labeler.sh — runs the issue labeler's tests. -# -# The labeler's logic lives in scripts/issue-labeler.js (ADR-0008); the -# workflow only calls it. scripts/issue-labeler.test.js exercises the pure -# functions and the run() entry point with stubbed GitHub clients, under -# plain node with no dependencies. -# -# node is a dependency of `make check` for this one script, like -# markdownlint-cli2 is for lint-docs; a missing node fails with an install -# hint the way the Makefile's other tool checks do — never a green skip. -# -# Exit status: 0 if the tests pass, 1 otherwise. - -set -euo pipefail - -cd "$(dirname "$0")/.." - -command -v node >/dev/null 2>&1 || { echo "install: node (e.g. brew install node) — needed for scripts/issue-labeler.test.js"; exit 1; } - -if node --test --test-reporter=dot scripts/issue-labeler.test.js; then - echo "" - echo "OK: scripts/issue-labeler.test.js passed" - echo "Summary: labeler tests passed" - exit 0 -fi -echo "FAIL: scripts/issue-labeler.test.js failed — rerun with: node --test scripts/issue-labeler.test.js" -echo "Summary: labeler tests FAILED" -exit 1 diff --git a/scripts/check-node-tests.sh b/scripts/check-node-tests.sh new file mode 100755 index 0000000..bf53477 --- /dev/null +++ b/scripts/check-node-tests.sh @@ -0,0 +1,53 @@ +#!/usr/bin/env bash +# check-node-tests.sh — runs every scripts/*.test.js under plain node. +# +# The logic behind the event-driven workflows lives in scripts/*.js +# (ADR-0008: issue-labeler.js, pr-lint.js); the workflows only call it. Each +# module has a sibling *.test.js that exercises its pure functions and its +# run() entry point with stubbed GitHub objects — node built-ins only, no +# dependencies to install. +# +# node is a dependency of `make check` for this one script, like +# markdownlint-cli2 is for lint-docs; a missing node fails with an install +# hint the way the Makefile's other tool checks do — never a green skip. +# +# Exit status: 0 if the tests pass, 1 otherwise. + +set -euo pipefail + +cd "$(dirname "$0")/.." + +command -v node >/dev/null 2>&1 || { echo "install: node (e.g. brew install node) — needed for scripts/*.test.js"; exit 1; } + +# Every `require(... /scripts/.js)` in a workflow must name a file that +# is here — an adopter who takes a thin-caller workflow without its script +# gets a job that fails on every event while `make check` stays green. +missing=0 +for wf in .github/workflows/*.yml; do + [ -f "$wf" ] || continue + while IFS= read -r js; do + [ -n "$js" ] || continue + if [ ! -f "$js" ]; then + echo "FAIL: $wf requires $js, which is missing — take the workflow and the script together (docs/template/upgrading.md)" + missing=$((missing + 1)) + else + echo "OK: $wf requires $js (present)" + fi + done </-`; +// 2. the body links an issue with a GitHub closing keyword (`Closes #N`); +// 3. the branch's issue number is among the linked ones. +// Bot branches (release-please, Dependabot) have no issue and are exempt. +// +// The PR body is untrusted input. It is only ever matched as text here; the +// numbers it yields go into a message, never into a command. + +// The Conventional Commit types AGENTS.md lists (its "Branch" step is the one +// home for this list; pr-lint.test.js asserts the two are equal). The branch +// type mirrors them. +const TYPES = ['feat', 'fix', 'docs', 'chore', 'refactor', 'ci', 'test', 'perf']; +const BRANCH_RE = new RegExp(`^(${TYPES.join('|')})/(\\d+)-[a-z0-9][a-z0-9._-]*$`); +// GitHub's closing keywords: optional colon, then `#N`, `owner/repo#N`, or a +// full issue URL — the three forms GitHub itself links and auto-closes. +const CLOSES_RE = /\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?):?\s+(?:https?:\/\/github\.com\/([\w.-]+\/[\w.-]+)\/issues\/(\d+)|([\w.-]+\/[\w.-]+)?#(\d+))\b/gi; +const EXEMPT_BRANCH_PREFIXES = ['release-please--', 'dependabot/']; + +function stripHtmlComments(text) { + return String(text || '').replace(//g, ''); +} + +// Every issue the body closes: [{ repo: 'owner/name' | null, number }]. A +// reference qualified with this repository's own name counts as local. +function linkedIssues(body, repo) { + const seen = new Map(); + for (const m of stripHtmlComments(body).matchAll(CLOSES_RE)) { + const qualifier = m[1] || m[3] || null; + const number = Number(m[2] || m[4]); + const local = !qualifier || (repo && qualifier.toLowerCase() === String(repo).toLowerCase()); + const key = `${local ? '' : qualifier + '#'}${number}`; + if (!seen.has(key)) seen.set(key, { repo: local ? null : qualifier, number }); + } + return [...seen.values()]; +} + +function describe(ref) { + return ref.repo ? `${ref.repo}#${ref.number}` : `#${ref.number}`; +} + +// Pure. `repo` is 'owner/name' when known (run() passes context.repo), so a +// fully qualified self-reference is treated as local. +// Returns { exempt, problems: [string], branchIssue, linked: [number], cross: [string] }. +function lint({ headRef, body, repo }) { + const ref = String(headRef || ''); + const refs = linkedIssues(body, repo); + const linked = refs.filter((r) => !r.repo).map((r) => r.number); + const cross = refs.filter((r) => r.repo).map(describe); + if (EXEMPT_BRANCH_PREFIXES.some((p) => ref.startsWith(p))) { + return { exempt: true, problems: [], branchIssue: null, linked, cross }; + } + const problems = []; + const branch = ref.match(BRANCH_RE); + const branchIssue = branch ? Number(branch[2]) : null; + if (!branch) { + problems.push( + `branch '${ref}' is not /- (types: ${TYPES.join(', ')}; e.g. fix/42-label-sync) — ` + + 'the usual slip is an empty issue number, which reads as "/-"', + ); + } + if (linked.length === 0) { + const tail = cross.length > 0 + ? ` (it closes ${cross.join(', ')} in another repository; an issue in this one is still required — AGENTS.md rule 1)` + : ' (if this PR only advances an issue, give it a sub-issue it can close — skills/pr-authoring rule 5)'; + problems.push( + 'body links no issue in this repository — the PR template line is `Closes #`; replace the comment ' + + `with the number. Any GitHub closing keyword works: Closes / Fixes / Resolves, optional colon, #N or a full issue URL${tail}`, + ); + } + if (branchIssue !== null && linked.length > 0 && !linked.includes(branchIssue)) { + problems.push( + `branch names issue #${branchIssue} but the body closes #${linked.join(', #')} — one of them is wrong`, + ); + } + return { exempt: false, problems, branchIssue, linked, cross }; +} + +// Entry point for actions/github-script. +async function run({ context, core }) { + const pr = context.payload.pull_request; + if (!pr) { + core.info('not a pull_request event — nothing to lint'); + return { exempt: true, problems: [] }; + } + const repo = context.repo ? `${context.repo.owner}/${context.repo.repo}` : undefined; + const result = lint({ headRef: pr.head && pr.head.ref, body: pr.body, repo }); + if (result.exempt) { + core.info(`branch '${pr.head.ref}' is a bot branch — PR lint skipped`); + return result; + } + if (result.problems.length > 0) { + core.setFailed(`PR lint failed:\n- ${result.problems.join('\n- ')}`); + return result; + } + const also = result.cross.length > 0 ? ` (and ${result.cross.join(', ')} elsewhere)` : ''; + core.info(`PR lint passed: branch issue #${result.branchIssue}, body closes #${result.linked.join(', #')}${also}`); + return result; +} + +module.exports = { run, lint, linkedIssues, describe, stripHtmlComments, TYPES, BRANCH_RE, CLOSES_RE, EXEMPT_BRANCH_PREFIXES }; diff --git a/scripts/pr-lint.test.js b/scripts/pr-lint.test.js new file mode 100644 index 0000000..6ad9842 --- /dev/null +++ b/scripts/pr-lint.test.js @@ -0,0 +1,99 @@ +'use strict'; +// pr-lint.test.js — exercises scripts/pr-lint.js under plain node. +// Run by scripts/check-node-tests.sh from `make check`. +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { lint, linkedIssues, run, TYPES } = require('./pr-lint.js'); +const numbers = (body, repo) => linkedIssues(body, repo).filter((r) => !r.repo).map((r) => r.number); + +const GOOD_BODY = '## Summary\n\nx\n\n## Related issue\n\nCloses #42\n'; + +test('a conforming PR passes', () => { + const r = lint({ headRef: 'fix/42-label-sync', body: GOOD_BODY }); + assert.deepEqual(r, { exempt: false, problems: [], branchIssue: 42, linked: [42], cross: [] }); +}); + +test("TYPES equals the list in AGENTS.md's Branch step (the one home for it)", () => { + const agents = fs.readFileSync(path.join(__dirname, '..', 'AGENTS.md'), 'utf8'); + const line = agents.split('\n').find((l) => l.includes('**Branch**') && l.includes('/-')); + assert.ok(line, 'AGENTS.md Branch step not found'); + const listed = [...line.matchAll(/`([a-z]+)`/g)].map((m) => m[1]); + assert.deepEqual(listed, TYPES); +}); + +test('the six-PR failure shape: empty issue slot in the branch and a bare "Closes #"', () => { + const r = lint({ headRef: 'chore/-initial-version-docs', body: 'Closes #\n' }); + assert.equal(r.problems.length, 2); + assert.match(r.problems[0], /not \/-/); + assert.match(r.problems[1], /links no issue/); +}); + +test('the untouched template line still fails: the number is inside an HTML comment', () => { + const r = lint({ headRef: 'fix/42-x', body: 'Closes #' }); + assert.deepEqual(r.linked, []); + assert.equal(r.problems.length, 1); +}); + +test('a closing keyword hidden inside an HTML comment does not count', () => { + assert.deepEqual(numbers('\nsome text'), []); +}); + +test('all GitHub closing keywords, the colon form and the full URL form are recognised, case-insensitively', () => { + assert.deepEqual(numbers('closes #1 Fixes #2 RESOLVED #3 close #5 resolves #6 Closes: #9 CLOSES: #10'), [1, 2, 3, 5, 6, 9, 10]); + assert.deepEqual(numbers('Closes https://github.com/o/r/issues/77', 'o/r'), [77]); + assert.deepEqual(numbers('Closes o/r#78', 'O/R'), [78]); + assert.deepEqual(numbers('see #7 and refs #8 closes#9 Closes:#10'), []); +}); + +test('cross-repository references are kept apart from local ones and never satisfy the branch number', () => { + const only = lint({ headRef: 'fix/4-x', body: 'Closes other/repo#4', repo: 'o/r' }); + assert.deepEqual(only.linked, []); + assert.deepEqual(only.cross, ['other/repo#4']); + assert.equal(only.problems.length, 1); + assert.match(only.problems[0], /closes other\/repo#4 in another repository/); + const both = lint({ headRef: 'fix/12-x', body: 'Closes #12, closes https://github.com/other/repo/issues/4', repo: 'o/r' }); + assert.deepEqual(both, { exempt: false, problems: [], branchIssue: 12, linked: [12], cross: ['other/repo#4'] }); +}); + +test('a Refs-only body is told to split the work into a sub-issue', () => { + const r = lint({ headRef: 'feat/3-x', body: 'Refs #3' }); + assert.match(r.problems[0], /sub-issue it can close/); +}); + +test('branch issue must be among the linked issues; several linked issues are fine', () => { + assert.deepEqual(lint({ headRef: 'feat/10-x', body: 'Closes #10\nCloses #11' }).problems, []); + const r = lint({ headRef: 'feat/10-x', body: 'Closes #11' }); + assert.equal(r.problems.length, 1); + assert.match(r.problems[0], /branch names issue #10 but the body closes #11/); +}); + +test('branch grammar: every CONTRIBUTING type, lowercase slug with dots/underscores; rejects others', () => { + for (const t of ['feat', 'fix', 'docs', 'chore', 'refactor', 'ci', 'test', 'perf']) { + assert.deepEqual(lint({ headRef: `${t}/7-a.b_c-1`, body: 'Closes #7' }).problems, []); + } + for (const bad of ['Feat/7-x', 'feature/7-x', 'fix/7', 'fix/7-', 'fix/x-7', 'fix-7-x', 'main', 'hotfix/7-x']) { + assert.equal(lint({ headRef: bad, body: 'Closes #7' }).problems.length, 1, bad); + } +}); + +test('bot branches are exempt regardless of body', () => { + assert.equal(lint({ headRef: 'release-please--branches--main', body: ':robot: I have created a release' }).exempt, true); + assert.equal(lint({ headRef: 'dependabot/github_actions/actions-f3c1f23acc', body: '' }).exempt, true); +}); + +test('run: fails the job with every problem listed, passes with an info line, skips non-PR events', async () => { + const out = []; + const core = { info: (m) => out.push(['info', m]), setFailed: (m) => out.push(['failed', m]) }; + await run({ context: { payload: { pull_request: { head: { ref: 'chore/-x' }, body: 'Closes #' } } }, core }); + assert.equal(out.length, 1); + assert.equal(out[0][0], 'failed'); + assert.match(out[0][1], /^PR lint failed:\n- branch .*\n- body links no issue/s); + out.length = 0; + await run({ context: { repo: { owner: 'o', repo: 'r' }, payload: { pull_request: { head: { ref: 'fix/42-x' }, body: GOOD_BODY + 'Closes o/r#42\n' } } }, core }); + assert.deepEqual(out, [['info', 'PR lint passed: branch issue #42, body closes #42']]); + out.length = 0; + await run({ context: { payload: {} }, core }); + assert.equal(out[0][0], 'info'); +}); diff --git a/skills/branch-and-commit/SKILL.md b/skills/branch-and-commit/SKILL.md index 8bd334b..34520a4 100644 --- a/skills/branch-and-commit/SKILL.md +++ b/skills/branch-and-commit/SKILL.md @@ -61,6 +61,8 @@ git branch --show-current # expect /- ## Pitfalls +- A branch with the issue slot empty (`chore/-slug`) — the convention degrades silently; CI fails the PR on it (`scripts/pr-lint.js`). + - Typing `fix:` for what is actually a `feat:` (or vice versa) — this silently miscategorizes the changelog entry; release-please trusts the type literally. - Committing straight to `main` "because it's a one-liner" — there is no exception; diff --git a/skills/github-actions-hygiene/SKILL.md b/skills/github-actions-hygiene/SKILL.md index acae8b7..60eb53f 100644 --- a/skills/github-actions-hygiene/SKILL.md +++ b/skills/github-actions-hygiene/SKILL.md @@ -12,7 +12,7 @@ Workflows are the highest-privilege, least-reviewed code in most repositories ## Rules -1. **Workflows call `make` targets; they never contain logic.** All branching, tool installation flags, and conditionals belong in the `Makefile`. A workflow step should read as `run: make lint`, not a shell script with real decisions in it. Adopters customize the Makefile — never the workflow YAML — to change what runs. Two things the Makefile cannot own: `runs-on`, which GitHub resolves before any make target exists — expose it as a repository variable (`RUNNER_LABELS` here) so *where* a job runs stays configurable without forking the file; and event handlers that need the token and the event payload (the issue labeler) — their logic lives in `scripts/*.js`, tested by `make check`, and the workflow is a two-line `actions/github-script` caller after a pinned checkout (ADR-0008). Neither shape puts adopter values in the YAML. +1. **Workflows call `make` targets; they never contain logic.** All branching, tool installation flags, and conditionals belong in the `Makefile`. A workflow step should read as `run: make lint`, not a shell script with real decisions in it. Adopters customize the Makefile — never the workflow YAML — to change what runs. Two things the Makefile cannot own: `runs-on`, which GitHub resolves before any make target exists — expose it as a repository variable (`RUNNER_LABELS` here) so *where* a job runs stays configurable without forking the file; and event handlers that need the token or the event payload (the issue labeler, the PR lint) — their logic lives in `scripts/*.js`, tested by `make check`, and the workflow step is a two-line `actions/github-script` caller after a pinned checkout (ADR-0008). Neither shape puts adopter values in the YAML. 2. **Least privilege by default.** Set `permissions: contents: read` at the workflow (top) level; escalate only inside the specific job that needs more, and only to the exact scope needed (e.g. `pull-requests: write` on a labeler job, not on the whole workflow). 3. **Pin every third-party action to a full commit SHA**, with the human-readable version as a trailing comment — never a floating tag like `@v4`: @@ -84,8 +84,8 @@ verify_checksum() { ## Related - `` `.github/workflows/ci.yml` `` — reference implementation of pinned actions, top-level `permissions: read`, concurrency group, `timeout-minutes`, and `persist-credentials: false`; also carries the header warning explaining why its `on:` block must never gain a path filter (the `ci` job is the only required status check) -- `Makefile` — where workflow logic lives (`lint`, `test`, `verify`, `ci-pr`, `ci-tools`), except the event handler in `scripts/issue-labeler.js` (rule 1, ADR-0008) -- `` `scripts/issue-labeler.js` `` / `` `scripts/check-issue-labeler.sh` `` — the one event handler and the `make check` step that tests it +- `Makefile` — where workflow logic lives (`lint`, `test`, `verify`, `ci-pr`, `ci-tools`), except the event handlers in `scripts/issue-labeler.js` and `scripts/pr-lint.js` (rule 1, ADR-0008) +- `` `scripts/issue-labeler.js` ``, `` `scripts/pr-lint.js` `` / `` `scripts/check-node-tests.sh` `` — the event handlers and the `make check` step that tests them - `` `scripts/install-ci-tools.sh` `` — checksum-verified tool installs and the single home for all five CI tool version pins, shared by `ci.yml` and `maintenance.yml` - `` `scripts/check-tool-versions.sh` `` — rule 10's watcher: diffs those pins against upstream weekly and fails on drift - `AGENTS.md` — "The Makefile is the only executable contract in this repository" diff --git a/skills/pr-authoring/SKILL.md b/skills/pr-authoring/SKILL.md index 0ab004f..b422cf9 100644 --- a/skills/pr-authoring/SKILL.md +++ b/skills/pr-authoring/SKILL.md @@ -38,7 +38,7 @@ human reviewer and for an agent reviewing another agent's work. No level that applies to the change may simply be absent from both the checkbox list and the RISK lines. -5. Use `Closes #N` to link the issue — one issue per PR. Don't bundle unrelated +5. Use `Closes #N` to link the issue — one issue per PR, and every PR closes one: work that spans several PRs gets a sub-issue per PR, never a `Refs #N` PR that leaves the parent open. CI fails the PR when the body links no issue in this repository or the branch's issue number differs (`scripts/pr-lint.js`). Don't bundle unrelated changes just because they happened to be worked on together. 6. Re-run `make verify` after every revision to the PR, not just before the first push. A review comment that changes code invalidates the previous green run. @@ -46,9 +46,11 @@ human reviewer and for an agent reviewing another agent's work. ## How ```bash -# Open a PR with a Conventional Commit title, linked issue, template intact -gh pr create --title "feat: add label sync phase to bootstrap" \ - --body-file .github/PULL_REQUEST_TEMPLATE.md +# Open a PR with a Conventional Commit title and the issue number filled in — +# the template's `Closes #` submitted as-is links nothing, +# and CI fails the PR on it +sed 's/Closes #/Closes #42/' .github/PULL_REQUEST_TEMPLATE.md > /tmp/pr-body.md +gh pr create --title "feat: add label sync phase to bootstrap" --body-file /tmp/pr-body.md # Re-verify before each push after review feedback make verify && git push @@ -63,6 +65,8 @@ Rollback: revert this PR; no migrations or external state changed ## Pitfalls +- Leaving the template's `Closes #` untouched, or an empty `Closes #` — the link is silently absent; CI fails the PR and names the line. + - Leaving a validation level unchecked with no `RISK:` line — indistinguishable from "forgot to run it," which is exactly the ambiguity the RISK convention exists to remove. From a0161e1b204eb72f36140caa1d0094b97ac1ec20 Mon Sep 17 00:00:00 2001 From: TzuHsuan <96853116+TzuH-Hsu@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:57:08 +0800 Subject: [PATCH 02/11] ci: exempt bot PRs by author or release-please's label, never by branch name Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 6 +++-- docs/template/architecture.md | 2 +- scripts/pr-lint.js | 43 ++++++++++++++++++++++++++--------- scripts/pr-lint.test.js | 20 ++++++++++++---- 4 files changed, 52 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 25b110a..7b586d9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,8 +40,10 @@ name: CI # The `Lint the pull request` step (pull_request events only, step-level `if` # so the job still reports) fails this required check when the head branch is # not `/-` or the body links no issue — the logic is -# scripts/pr-lint.js (ADR-0008), tested by `make check`. Bot branches -# (release-please, Dependabot) are exempt. Because this job already executes +# scripts/pr-lint.js (ADR-0008), tested by `make check`. Bot PRs are exempt +# by author (dependabot[bot], github-actions[bot]) or by release-please's +# `autorelease: pending` label — never by branch name, which a fork author +# chooses. Because this job already executes # the PR's own Makefile and scripts, it also runs the PR's own copy of # pr-lint.js; a PR can therefore weaken its own lint, and the reviewer sees # that in the diff. One difference from the `run:` steps: github-script is a diff --git a/docs/template/architecture.md b/docs/template/architecture.md index 0e1af9d..39cb45e 100644 --- a/docs/template/architecture.md +++ b/docs/template/architecture.md @@ -18,7 +18,7 @@ Why each piece of this repository exists, and what it costs to keep. A component | `.github/workflows/ci.yml` | L0 gate; lints the PR itself (`scripts/pr-lint.js`), installs tools via `make ci-tools`, runs `make ci-pr`. The only required status check — see its header before touching `on:` or `runs-on:` | SHA-pin bumps via Dependabot | | `.github/workflows/issue-labeler.yml` | Thin caller: checkout + `require('scripts/issue-labeler.js')` (ADR-0008) | Two action SHA pins via Dependabot; no adopter values | | `scripts/issue-labeler.js` (+ `.test.js`) | Form selections → labels (single-home preserving); `area:*` allowlist read from the checked-out `labels.yml` | `ALLOWED_PRIORITIES` / `ALLOWED_SUBTYPES` must match the forms — checked by `scripts/check-label-forms.sh`; behaviour pinned by its test via `make check` | -| `scripts/pr-lint.js` (+ `.test.js`) | Fails the required `ci` check when a PR's branch is not `/-` or its body links no issue; bot branches exempt | `TYPES` must equal the list in `AGENTS.md`'s Branch step — its test asserts it; behaviour pinned via `make check` | +| `scripts/pr-lint.js` (+ `.test.js`) | Fails the required `ci` check when a PR's branch is not `/-` or its body links no issue; bot PRs exempt by author or the `autorelease: pending` label | `TYPES` must equal the list in `AGENTS.md`'s Branch step — its test asserts it; behaviour pinned via `make check` | | `.github/workflows/maintenance.yml` | Weekly drift detectors: external link check + CI tool version check | Near zero | | `.github/workflows/release-please.yml` + configs | Human-gated release automation (ADR-0002) | Action SHA bumps; `initial-version` pins adopters' first release and is inert afterwards | | `.github/rulesets/main-branch.json` | Importable branch protection (PR + green `ci` required) | Near zero | diff --git a/scripts/pr-lint.js b/scripts/pr-lint.js index ec50385..2d0e0f2 100644 --- a/scripts/pr-lint.js +++ b/scripts/pr-lint.js @@ -11,7 +11,9 @@ // 1. the head branch is `/-`; // 2. the body links an issue with a GitHub closing keyword (`Closes #N`); // 3. the branch's issue number is among the linked ones. -// Bot branches (release-please, Dependabot) have no issue and are exempt. +// Bot PRs (release-please, Dependabot) have no issue and are exempt — by WHO +// opened them, never by branch name: on a public repository a fork author +// picks the head branch name, so `release-please--…` proves nothing. // // The PR body is untrusted input. It is only ever matched as text here; the // numbers it yields go into a message, never into a command. @@ -24,7 +26,12 @@ const BRANCH_RE = new RegExp(`^(${TYPES.join('|')})/(\\d+)-[a-z0-9][a-z0-9._-]*$ // GitHub's closing keywords: optional colon, then `#N`, `owner/repo#N`, or a // full issue URL — the three forms GitHub itself links and auto-closes. const CLOSES_RE = /\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?):?\s+(?:https?:\/\/github\.com\/([\w.-]+\/[\w.-]+)\/issues\/(\d+)|([\w.-]+\/[\w.-]+)?#(\d+))\b/gi; -const EXEMPT_BRANCH_PREFIXES = ['release-please--', 'dependabot/']; +// Exempt when the PR was opened by one of these accounts (Dependabot; +// release-please run with the default GITHUB_TOKEN), or carries the label +// release-please always applies (covers release-please run with a PAT, where +// the author is a human account). A fork author can choose neither. +const EXEMPT_AUTHORS = ['dependabot[bot]', 'github-actions[bot]']; +const EXEMPT_LABELS = ['autorelease: pending']; function stripHtmlComments(text) { return String(text || '').replace(//g, ''); @@ -48,16 +55,24 @@ function describe(ref) { return ref.repo ? `${ref.repo}#${ref.number}` : `#${ref.number}`; } +function isExempt({ author, labels }) { + if (author && EXEMPT_AUTHORS.includes(author)) return `opened by ${author}`; + const hit = (labels || []).find((l) => EXEMPT_LABELS.includes(l)); + return hit ? `carries the '${hit}' label` : null; +} + // Pure. `repo` is 'owner/name' when known (run() passes context.repo), so a -// fully qualified self-reference is treated as local. -// Returns { exempt, problems: [string], branchIssue, linked: [number], cross: [string] }. -function lint({ headRef, body, repo }) { +// fully qualified self-reference is treated as local. `author` is the PR +// author's login and `labels` the PR's label names. +// Returns { exempt, why, problems: [string], branchIssue, linked: [number], cross: [string] }. +function lint({ headRef, body, repo, author, labels }) { const ref = String(headRef || ''); const refs = linkedIssues(body, repo); const linked = refs.filter((r) => !r.repo).map((r) => r.number); const cross = refs.filter((r) => r.repo).map(describe); - if (EXEMPT_BRANCH_PREFIXES.some((p) => ref.startsWith(p))) { - return { exempt: true, problems: [], branchIssue: null, linked, cross }; + const why = isExempt({ author, labels }); + if (why) { + return { exempt: true, why, problems: [], branchIssue: null, linked, cross }; } const problems = []; const branch = ref.match(BRANCH_RE); @@ -82,7 +97,7 @@ function lint({ headRef, body, repo }) { `branch names issue #${branchIssue} but the body closes #${linked.join(', #')} — one of them is wrong`, ); } - return { exempt: false, problems, branchIssue, linked, cross }; + return { exempt: false, why: null, problems, branchIssue, linked, cross }; } // Entry point for actions/github-script. @@ -93,9 +108,15 @@ async function run({ context, core }) { return { exempt: true, problems: [] }; } const repo = context.repo ? `${context.repo.owner}/${context.repo.repo}` : undefined; - const result = lint({ headRef: pr.head && pr.head.ref, body: pr.body, repo }); + const result = lint({ + headRef: pr.head && pr.head.ref, + body: pr.body, + repo, + author: pr.user && pr.user.login, + labels: (pr.labels || []).map((l) => l.name), + }); if (result.exempt) { - core.info(`branch '${pr.head.ref}' is a bot branch — PR lint skipped`); + core.info(`PR lint skipped: ${result.why}`); return result; } if (result.problems.length > 0) { @@ -107,4 +128,4 @@ async function run({ context, core }) { return result; } -module.exports = { run, lint, linkedIssues, describe, stripHtmlComments, TYPES, BRANCH_RE, CLOSES_RE, EXEMPT_BRANCH_PREFIXES }; +module.exports = { run, lint, isExempt, linkedIssues, describe, stripHtmlComments, TYPES, BRANCH_RE, CLOSES_RE, EXEMPT_AUTHORS, EXEMPT_LABELS }; diff --git a/scripts/pr-lint.test.js b/scripts/pr-lint.test.js index 6ad9842..ba66c8c 100644 --- a/scripts/pr-lint.test.js +++ b/scripts/pr-lint.test.js @@ -12,7 +12,7 @@ const GOOD_BODY = '## Summary\n\nx\n\n## Related issue\n\nCloses #42\n'; test('a conforming PR passes', () => { const r = lint({ headRef: 'fix/42-label-sync', body: GOOD_BODY }); - assert.deepEqual(r, { exempt: false, problems: [], branchIssue: 42, linked: [42], cross: [] }); + assert.deepEqual(r, { exempt: false, why: null, problems: [], branchIssue: 42, linked: [42], cross: [] }); }); test("TYPES equals the list in AGENTS.md's Branch step (the one home for it)", () => { @@ -54,7 +54,7 @@ test('cross-repository references are kept apart from local ones and never satis assert.equal(only.problems.length, 1); assert.match(only.problems[0], /closes other\/repo#4 in another repository/); const both = lint({ headRef: 'fix/12-x', body: 'Closes #12, closes https://github.com/other/repo/issues/4', repo: 'o/r' }); - assert.deepEqual(both, { exempt: false, problems: [], branchIssue: 12, linked: [12], cross: ['other/repo#4'] }); + assert.deepEqual(both, { exempt: false, why: null, problems: [], branchIssue: 12, linked: [12], cross: ['other/repo#4'] }); }); test('a Refs-only body is told to split the work into a sub-issue', () => { @@ -78,9 +78,16 @@ test('branch grammar: every CONTRIBUTING type, lowercase slug with dots/undersco } }); -test('bot branches are exempt regardless of body', () => { - assert.equal(lint({ headRef: 'release-please--branches--main', body: ':robot: I have created a release' }).exempt, true); - assert.equal(lint({ headRef: 'dependabot/github_actions/actions-f3c1f23acc', body: '' }).exempt, true); +test('bot PRs are exempt by author or by the release-please label — never by branch name', () => { + assert.equal(lint({ headRef: 'release-please--branches--main', body: ':robot:', author: 'github-actions[bot]' }).exempt, true); + assert.equal(lint({ headRef: 'dependabot/github_actions/actions-f3c1f23acc', body: '', author: 'dependabot[bot]' }).exempt, true); + // release-please run with a PAT: human author, but the label it applies is proof enough + assert.equal(lint({ headRef: 'release-please--branches--main', body: ':robot:', author: 'someone', labels: ['autorelease: pending'] }).exempt, true); + // a fork author borrowing the bot branch name gets the full lint + const spoof = lint({ headRef: 'release-please--branches--main', body: 'hi', author: 'someone', labels: [] }); + assert.equal(spoof.exempt, false); + assert.equal(spoof.problems.length, 2); + assert.equal(lint({ headRef: 'dependabot/x', body: '', author: 'someone' }).exempt, false); }); test('run: fails the job with every problem listed, passes with an info line, skips non-PR events', async () => { @@ -96,4 +103,7 @@ test('run: fails the job with every problem listed, passes with an info line, sk out.length = 0; await run({ context: { payload: {} }, core }); assert.equal(out[0][0], 'info'); + out.length = 0; + await run({ context: { payload: { pull_request: { head: { ref: 'release-please--branches--main' }, body: '', user: { login: 'github-actions[bot]' }, labels: [{ name: 'autorelease: pending' }] } } }, core }); + assert.deepEqual(out, [['info', 'PR lint skipped: opened by github-actions[bot]']]); }); From b3fc3f8704f8aebfd4c0a6c67dcba496f3995231 Mon Sep 17 00:00:00 2001 From: TzuHsuan <96853116+TzuH-Hsu@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:02:42 +0800 Subject: [PATCH 03/11] ci: re-run on labeled; exactly one closed issue per PR Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 10 ++++++---- scripts/pr-lint.js | 11 ++++++++--- scripts/pr-lint.test.js | 7 +++++-- skills/pr-authoring/SKILL.md | 2 +- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7b586d9..70c2d32 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -58,10 +58,12 @@ on: # A SUPERSET of the defaults (opened, synchronize, reopened). `edited` and # `ready_for_review` are added so a PR that failed the lint on its body or # title turns green when the author fixes it, without a push — "Re-run - # jobs" replays the ORIGINAL payload and would judge the old body. Never - # narrow this list: a `types:` without the three defaults is the - # "check never created" trap described above. - types: [opened, synchronize, reopened, edited, ready_for_review] + # jobs" replays the ORIGINAL payload and would judge the old body. + # `labeled` is added because release-please applies `autorelease: pending` + # AFTER opening the PR; with a PAT-authored release PR that label is what + # exempts it. Never narrow this list: a `types:` without the three + # defaults is the "check never created" trap described above. + types: [opened, synchronize, reopened, edited, ready_for_review, labeled] push: branches: - main diff --git a/scripts/pr-lint.js b/scripts/pr-lint.js index 2d0e0f2..828c21f 100644 --- a/scripts/pr-lint.js +++ b/scripts/pr-lint.js @@ -10,7 +10,7 @@ // carries, which were being broken silently: // 1. the head branch is `/-`; // 2. the body links an issue with a GitHub closing keyword (`Closes #N`); -// 3. the branch's issue number is among the linked ones. +// 3. exactly one issue in this repository is closed, and it is the branch's. // Bot PRs (release-please, Dependabot) have no issue and are exempt — by WHO // opened them, never by branch name: on a public repository a fork author // picks the head branch name, so `release-please--…` proves nothing. @@ -92,9 +92,14 @@ function lint({ headRef, body, repo, author, labels }) { `with the number. Any GitHub closing keyword works: Closes / Fixes / Resolves, optional colon, #N or a full issue URL${tail}`, ); } - if (branchIssue !== null && linked.length > 0 && !linked.includes(branchIssue)) { + if (linked.length > 1) { problems.push( - `branch names issue #${branchIssue} but the body closes #${linked.join(', #')} — one of them is wrong`, + `body closes #${linked.join(', #')} — one issue per PR (skills/pr-authoring rule 5): keep the one this branch is for, ` + + 'and close a genuine duplicate by hand after merge', + ); + } else if (branchIssue !== null && linked.length === 1 && linked[0] !== branchIssue) { + problems.push( + `branch names issue #${branchIssue} but the body closes #${linked[0]} — one of them is wrong`, ); } return { exempt: false, why: null, problems, branchIssue, linked, cross }; diff --git a/scripts/pr-lint.test.js b/scripts/pr-lint.test.js index ba66c8c..0d7485a 100644 --- a/scripts/pr-lint.test.js +++ b/scripts/pr-lint.test.js @@ -62,11 +62,14 @@ test('a Refs-only body is told to split the work into a sub-issue', () => { assert.match(r.problems[0], /sub-issue it can close/); }); -test('branch issue must be among the linked issues; several linked issues are fine', () => { - assert.deepEqual(lint({ headRef: 'feat/10-x', body: 'Closes #10\nCloses #11' }).problems, []); +test('exactly one local issue: a second Closes fails (one issue per PR), a mismatched one fails', () => { + const two = lint({ headRef: 'feat/10-x', body: 'Closes #10\nCloses #11' }); + assert.equal(two.problems.length, 1); + assert.match(two.problems[0], /one issue per PR/); const r = lint({ headRef: 'feat/10-x', body: 'Closes #11' }); assert.equal(r.problems.length, 1); assert.match(r.problems[0], /branch names issue #10 but the body closes #11/); + assert.deepEqual(lint({ headRef: 'feat/10-x', body: 'Closes #10\nCloses #10' }).problems, []); // same issue twice is one issue }); test('branch grammar: every CONTRIBUTING type, lowercase slug with dots/underscores; rejects others', () => { diff --git a/skills/pr-authoring/SKILL.md b/skills/pr-authoring/SKILL.md index b422cf9..a3a7c17 100644 --- a/skills/pr-authoring/SKILL.md +++ b/skills/pr-authoring/SKILL.md @@ -38,7 +38,7 @@ human reviewer and for an agent reviewing another agent's work. No level that applies to the change may simply be absent from both the checkbox list and the RISK lines. -5. Use `Closes #N` to link the issue — one issue per PR, and every PR closes one: work that spans several PRs gets a sub-issue per PR, never a `Refs #N` PR that leaves the parent open. CI fails the PR when the body links no issue in this repository or the branch's issue number differs (`scripts/pr-lint.js`). Don't bundle unrelated +5. Use `Closes #N` to link the issue — one issue per PR, and every PR closes one: work that spans several PRs gets a sub-issue per PR, never a `Refs #N` PR that leaves the parent open. CI fails the PR when the body links no issue in this repository, links more than one, or the branch's issue number differs (`scripts/pr-lint.js`). Don't bundle unrelated changes just because they happened to be worked on together. 6. Re-run `make verify` after every revision to the PR, not just before the first push. A review comment that changes code invalidates the previous green run. From d7fd2941921848dbb1b9cf48523be75305a2488c Mon Sep 17 00:00:00 2001 From: TzuHsuan <96853116+TzuH-Hsu@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:08:38 +0800 Subject: [PATCH 04/11] ci: re-run on unlabeled; the linked issue must exist and be an issue Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 9 +++++---- scripts/pr-lint.js | 33 +++++++++++++++++++++++++++------ scripts/pr-lint.test.js | 14 ++++++++++++-- 3 files changed, 44 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 70c2d32..f4b2f31 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,8 +50,9 @@ name: CI # JS action, so the PR's script runs with the job's GITHUB_TOKEN (read-only on # fork PRs, `contents: read` here) and the run-scoped runtime token in its # environment — nothing this public repository does not already expose, but -# not zero either, which is why the step stays tiny and does not take -# `github`. +# not zero either, which is why the step stays tiny. The script uses the +# client for exactly one read: confirming the linked issue exists and is an +# issue, not a pull request. on: pull_request: @@ -63,7 +64,7 @@ on: # AFTER opening the PR; with a PAT-authored release PR that label is what # exempts it. Never narrow this list: a `types:` without the three # defaults is the "check never created" trap described above. - types: [opened, synchronize, reopened, edited, ready_for_review, labeled] + types: [opened, synchronize, reopened, edited, ready_for_review, labeled, unlabeled] push: branches: - main @@ -97,7 +98,7 @@ jobs: with: script: | const { run } = require(`${process.env.GITHUB_WORKSPACE}/scripts/pr-lint.js`); - await run({ context, core }); + await run({ github, context, core }); - name: Set up Node.js uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 diff --git a/scripts/pr-lint.js b/scripts/pr-lint.js index 828c21f..712c7cb 100644 --- a/scripts/pr-lint.js +++ b/scripts/pr-lint.js @@ -2,15 +2,17 @@ // pr-lint.js — the pull-request checks behind the `Lint the pull request` // step in .github/workflows/ci.yml. Same shape as scripts/issue-labeler.js // (ADR-0008): a pure lint() the tests exercise, and one run() the workflow -// calls through actions/github-script. Nothing here needs the token; it reads -// the pull_request payload and fails the job with a message that names the -// fix. +// calls through actions/github-script. It reads the pull_request payload and +// makes one read call — does the linked issue exist, and is it an issue — then +// fails the job with a message that names the fix. // // What it enforces — the two conventions AGENTS.md states and the PR template // carries, which were being broken silently: // 1. the head branch is `/-`; // 2. the body links an issue with a GitHub closing keyword (`Closes #N`); -// 3. exactly one issue in this repository is closed, and it is the branch's. +// 3. exactly one issue in this repository is closed, and it is the branch's; +// 4. that issue exists and is an issue, not a pull request (a made-up +// number would otherwise satisfy 1-3). // Bot PRs (release-please, Dependabot) have no issue and are exempt — by WHO // opened them, never by branch name: on a public repository a fork author // picks the head branch name, so `release-please--…` proves nothing. @@ -105,8 +107,23 @@ function lint({ headRef, body, repo, author, labels }) { return { exempt: false, why: null, problems, branchIssue, linked, cross }; } +// The one API read: 404 → the number is invented; a pull_request field → +// the number is a PR, which GitHub would not close. Any other failure is a +// failure — a check that cannot check must not pass. +async function verifyIssue(github, owner, repo, number) { + let issue; + try { + ({ data: issue } = await github.rest.issues.get({ owner, repo, issue_number: number })); + } catch (err) { + if (err && err.status === 404) return `issue #${number} does not exist in ${owner}/${repo} — the number is wrong, or the issue was never opened (AGENTS.md rule 1)`; + throw new Error(`could not verify issue #${number}: ${err && err.message ? err.message : err}`); + } + if (issue && issue.pull_request) return `#${number} is a pull request, not an issue — link the issue the work is for`; + return null; +} + // Entry point for actions/github-script. -async function run({ context, core }) { +async function run({ github, context, core }) { const pr = context.payload.pull_request; if (!pr) { core.info('not a pull_request event — nothing to lint'); @@ -124,6 +141,10 @@ async function run({ context, core }) { core.info(`PR lint skipped: ${result.why}`); return result; } + if (result.problems.length === 0 && github && context.repo) { + const problem = await verifyIssue(github, context.repo.owner, context.repo.repo, result.linked[0]); + if (problem) result.problems.push(problem); + } if (result.problems.length > 0) { core.setFailed(`PR lint failed:\n- ${result.problems.join('\n- ')}`); return result; @@ -133,4 +154,4 @@ async function run({ context, core }) { return result; } -module.exports = { run, lint, isExempt, linkedIssues, describe, stripHtmlComments, TYPES, BRANCH_RE, CLOSES_RE, EXEMPT_AUTHORS, EXEMPT_LABELS }; +module.exports = { run, lint, verifyIssue, isExempt, linkedIssues, describe, stripHtmlComments, TYPES, BRANCH_RE, CLOSES_RE, EXEMPT_AUTHORS, EXEMPT_LABELS }; diff --git a/scripts/pr-lint.test.js b/scripts/pr-lint.test.js index 0d7485a..3aadbd4 100644 --- a/scripts/pr-lint.test.js +++ b/scripts/pr-lint.test.js @@ -93,7 +93,7 @@ test('bot PRs are exempt by author or by the release-please label — never by b assert.equal(lint({ headRef: 'dependabot/x', body: '', author: 'someone' }).exempt, false); }); -test('run: fails the job with every problem listed, passes with an info line, skips non-PR events', async () => { +test('run: fails with every problem listed, verifies the issue exists and is not a PR, passes with an info line, skips non-PR events', async () => { const out = []; const core = { info: (m) => out.push(['info', m]), setFailed: (m) => out.push(['failed', m]) }; await run({ context: { payload: { pull_request: { head: { ref: 'chore/-x' }, body: 'Closes #' } } }, core }); @@ -101,9 +101,19 @@ test('run: fails the job with every problem listed, passes with an info line, sk assert.equal(out[0][0], 'failed'); assert.match(out[0][1], /^PR lint failed:\n- branch .*\n- body links no issue/s); out.length = 0; - await run({ context: { repo: { owner: 'o', repo: 'r' }, payload: { pull_request: { head: { ref: 'fix/42-x' }, body: GOOD_BODY + 'Closes o/r#42\n' } } }, core }); + const gh = (issue) => ({ rest: { issues: { get: async () => { if (issue === 404) { const e = new Error('Not Found'); e.status = 404; throw e; } if (issue === 'boom') throw new Error('boom'); return { data: issue }; } } } }); + const ctx = (ref, body) => ({ repo: { owner: 'o', repo: 'r' }, payload: { pull_request: { head: { ref }, body } } }); + await run({ github: gh({ number: 42 }), context: ctx('fix/42-x', GOOD_BODY + 'Closes o/r#42\n'), core }); assert.deepEqual(out, [['info', 'PR lint passed: branch issue #42, body closes #42']]); out.length = 0; + await run({ github: gh(404), context: ctx('fix/999999-x', 'Closes #999999'), core }); + assert.match(out[0][1], /issue #999999 does not exist/); + out.length = 0; + await run({ github: gh({ number: 42, pull_request: { url: 'x' } }), context: ctx('fix/42-x', GOOD_BODY), core }); + assert.match(out[0][1], /#42 is a pull request, not an issue/); + out.length = 0; + await assert.rejects(() => run({ github: gh('boom'), context: ctx('fix/42-x', GOOD_BODY), core }), /could not verify issue #42/); + out.length = 0; await run({ context: { payload: {} }, core }); assert.equal(out[0][0], 'info'); out.length = 0; From 31c13d1ec5ad895fcc312cbc5b6b9b747aa151e2 Mon Sep 17 00:00:00 2001 From: TzuHsuan <96853116+TzuH-Hsu@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:14:04 +0800 Subject: [PATCH 05/11] ci: ignore closing keywords inside code; a cross-repository close is a second issue Co-Authored-By: Claude Opus 5 --- scripts/pr-lint.js | 27 +++++++++++++++++++-------- scripts/pr-lint.test.js | 19 +++++++++++++------ 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/scripts/pr-lint.js b/scripts/pr-lint.js index 712c7cb..3e81ceb 100644 --- a/scripts/pr-lint.js +++ b/scripts/pr-lint.js @@ -35,15 +35,22 @@ const CLOSES_RE = /\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?):?\s+(?:https?:\/\ const EXEMPT_AUTHORS = ['dependabot[bot]', 'github-actions[bot]']; const EXEMPT_LABELS = ['autorelease: pending']; -function stripHtmlComments(text) { - return String(text || '').replace(//g, ''); +// GitHub links closing keywords in prose only: not inside HTML comments, +// fenced code blocks or inline code spans. Strip those before matching so the +// check agrees with what GitHub will actually close. +function stripNonProse(text) { + return String(text || '') + .replace(//g, '') + .replace(/(^|\n)(`{3,}|~{3,})[^\n]*\n[\s\S]*?\n\2[ \t]*(?=\n|$)/g, '$1') + .replace(/`[^`\n]*`/g, ''); } +const stripHtmlComments = stripNonProse; // kept for callers of the old name // Every issue the body closes: [{ repo: 'owner/name' | null, number }]. A // reference qualified with this repository's own name counts as local. function linkedIssues(body, repo) { const seen = new Map(); - for (const m of stripHtmlComments(body).matchAll(CLOSES_RE)) { + for (const m of stripNonProse(body).matchAll(CLOSES_RE)) { const qualifier = m[1] || m[3] || null; const number = Number(m[2] || m[4]); const local = !qualifier || (repo && qualifier.toLowerCase() === String(repo).toLowerCase()); @@ -86,12 +93,16 @@ function lint({ headRef, body, repo, author, labels }) { ); } if (linked.length === 0) { - const tail = cross.length > 0 - ? ` (it closes ${cross.join(', ')} in another repository; an issue in this one is still required — AGENTS.md rule 1)` - : ' (if this PR only advances an issue, give it a sub-issue it can close — skills/pr-authoring rule 5)'; problems.push( 'body links no issue in this repository — the PR template line is `Closes #`; replace the comment ' + - `with the number. Any GitHub closing keyword works: Closes / Fixes / Resolves, optional colon, #N or a full issue URL${tail}`, + 'with the number. Any GitHub closing keyword works: Closes / Fixes / Resolves, optional colon, #N or a full issue URL ' + + '(prose only — not inside code or an HTML comment). If this PR only advances an issue, give it a sub-issue it can close — skills/pr-authoring rule 5', + ); + } + if (cross.length > 0) { + problems.push( + `body closes ${cross.join(', ')} in another repository — one issue per PR, and it lives here (skills/pr-authoring rule 5); ` + + 'mention the other issue with "Refs" instead', ); } if (linked.length > 1) { @@ -154,4 +165,4 @@ async function run({ github, context, core }) { return result; } -module.exports = { run, lint, verifyIssue, isExempt, linkedIssues, describe, stripHtmlComments, TYPES, BRANCH_RE, CLOSES_RE, EXEMPT_AUTHORS, EXEMPT_LABELS }; +module.exports = { run, lint, verifyIssue, isExempt, linkedIssues, describe, stripNonProse, stripHtmlComments, TYPES, BRANCH_RE, CLOSES_RE, EXEMPT_AUTHORS, EXEMPT_LABELS }; diff --git a/scripts/pr-lint.test.js b/scripts/pr-lint.test.js index 3aadbd4..25f78ae 100644 --- a/scripts/pr-lint.test.js +++ b/scripts/pr-lint.test.js @@ -36,8 +36,13 @@ test('the untouched template line still fails: the number is inside an HTML comm assert.equal(r.problems.length, 1); }); -test('a closing keyword hidden inside an HTML comment does not count', () => { +test('a closing keyword inside an HTML comment, a fenced block or inline code does not count', () => { assert.deepEqual(numbers('\nsome text'), []); + assert.deepEqual(numbers('```\nCloses #12\n```\n'), []); + assert.deepEqual(numbers('~~~bash\ngit commit -m "Closes #12"\n~~~'), []); + assert.deepEqual(numbers('use `Closes #12` in the body'), []); + assert.deepEqual(numbers('```\nCloses #12\n```\nCloses #13'), [13]); + assert.deepEqual(numbers('`x` Closes #14 `y`'), [14]); }); test('all GitHub closing keywords, the colon form and the full URL form are recognised, case-insensitively', () => { @@ -47,14 +52,16 @@ test('all GitHub closing keywords, the colon form and the full URL form are reco assert.deepEqual(numbers('see #7 and refs #8 closes#9 Closes:#10'), []); }); -test('cross-repository references are kept apart from local ones and never satisfy the branch number', () => { +test('a cross-repository close is a second issue: rejected alone and alongside a local one', () => { const only = lint({ headRef: 'fix/4-x', body: 'Closes other/repo#4', repo: 'o/r' }); assert.deepEqual(only.linked, []); assert.deepEqual(only.cross, ['other/repo#4']); - assert.equal(only.problems.length, 1); - assert.match(only.problems[0], /closes other\/repo#4 in another repository/); + assert.equal(only.problems.length, 2); // no local issue + a cross-repo close + assert.match(only.problems[1], /closes other\/repo#4 in another repository/); const both = lint({ headRef: 'fix/12-x', body: 'Closes #12, closes https://github.com/other/repo/issues/4', repo: 'o/r' }); - assert.deepEqual(both, { exempt: false, why: null, problems: [], branchIssue: 12, linked: [12], cross: ['other/repo#4'] }); + assert.deepEqual(both.problems.length, 1); + assert.match(both.problems[0], /another repository/); + assert.deepEqual(lint({ headRef: 'fix/12-x', body: 'Closes #12, refs other/repo#4', repo: 'o/r' }).problems, []); }); test('a Refs-only body is told to split the work into a sub-issue', () => { @@ -104,7 +111,7 @@ test('run: fails with every problem listed, verifies the issue exists and is not const gh = (issue) => ({ rest: { issues: { get: async () => { if (issue === 404) { const e = new Error('Not Found'); e.status = 404; throw e; } if (issue === 'boom') throw new Error('boom'); return { data: issue }; } } } }); const ctx = (ref, body) => ({ repo: { owner: 'o', repo: 'r' }, payload: { pull_request: { head: { ref }, body } } }); await run({ github: gh({ number: 42 }), context: ctx('fix/42-x', GOOD_BODY + 'Closes o/r#42\n'), core }); - assert.deepEqual(out, [['info', 'PR lint passed: branch issue #42, body closes #42']]); + assert.deepEqual(out, [['info', 'PR lint passed: branch issue #42, body closes #42']]); // o/r#42 is this repo → local out.length = 0; await run({ github: gh(404), context: ctx('fix/999999-x', 'Closes #999999'), core }); assert.match(out[0][1], /issue #999999 does not exist/); From c6c5895e2e3fca04555ee4d5839ca70d6757f160 Mon Sep 17 00:00:00 2001 From: TzuHsuan <96853116+TzuH-Hsu@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:19:12 +0800 Subject: [PATCH 06/11] ci: the linked issue must be open Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 4 ++-- scripts/pr-lint.js | 5 +++-- scripts/pr-lint.test.js | 5 ++++- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f4b2f31..badb697 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -51,8 +51,8 @@ name: CI # fork PRs, `contents: read` here) and the run-scoped runtime token in its # environment — nothing this public repository does not already expose, but # not zero either, which is why the step stays tiny. The script uses the -# client for exactly one read: confirming the linked issue exists and is an -# issue, not a pull request. +# client for exactly one read: confirming the linked issue exists, is open, +# and is an issue, not a pull request. on: pull_request: diff --git a/scripts/pr-lint.js b/scripts/pr-lint.js index 3e81ceb..6913fd6 100644 --- a/scripts/pr-lint.js +++ b/scripts/pr-lint.js @@ -11,8 +11,8 @@ // 1. the head branch is `/-`; // 2. the body links an issue with a GitHub closing keyword (`Closes #N`); // 3. exactly one issue in this repository is closed, and it is the branch's; -// 4. that issue exists and is an issue, not a pull request (a made-up -// number would otherwise satisfy 1-3). +// 4. that issue exists, is open, and is an issue, not a pull request (a +// made-up or already-closed number would otherwise satisfy 1-3). // Bot PRs (release-please, Dependabot) have no issue and are exempt — by WHO // opened them, never by branch name: on a public repository a fork author // picks the head branch name, so `release-please--…` proves nothing. @@ -130,6 +130,7 @@ async function verifyIssue(github, owner, repo, number) { throw new Error(`could not verify issue #${number}: ${err && err.message ? err.message : err}`); } if (issue && issue.pull_request) return `#${number} is a pull request, not an issue — link the issue the work is for`; + if (issue && issue.state === 'closed') return `issue #${number} is already closed, so merging cannot close it — reopen it, or open a follow-up issue and link that`; return null; } diff --git a/scripts/pr-lint.test.js b/scripts/pr-lint.test.js index 25f78ae..680f75f 100644 --- a/scripts/pr-lint.test.js +++ b/scripts/pr-lint.test.js @@ -110,7 +110,7 @@ test('run: fails with every problem listed, verifies the issue exists and is not out.length = 0; const gh = (issue) => ({ rest: { issues: { get: async () => { if (issue === 404) { const e = new Error('Not Found'); e.status = 404; throw e; } if (issue === 'boom') throw new Error('boom'); return { data: issue }; } } } }); const ctx = (ref, body) => ({ repo: { owner: 'o', repo: 'r' }, payload: { pull_request: { head: { ref }, body } } }); - await run({ github: gh({ number: 42 }), context: ctx('fix/42-x', GOOD_BODY + 'Closes o/r#42\n'), core }); + await run({ github: gh({ number: 42, state: 'open' }), context: ctx('fix/42-x', GOOD_BODY + 'Closes o/r#42\n'), core }); assert.deepEqual(out, [['info', 'PR lint passed: branch issue #42, body closes #42']]); // o/r#42 is this repo → local out.length = 0; await run({ github: gh(404), context: ctx('fix/999999-x', 'Closes #999999'), core }); @@ -119,6 +119,9 @@ test('run: fails with every problem listed, verifies the issue exists and is not await run({ github: gh({ number: 42, pull_request: { url: 'x' } }), context: ctx('fix/42-x', GOOD_BODY), core }); assert.match(out[0][1], /#42 is a pull request, not an issue/); out.length = 0; + await run({ github: gh({ number: 42, state: 'closed' }), context: ctx('fix/42-x', GOOD_BODY), core }); + assert.match(out[0][1], /issue #42 is already closed/); + out.length = 0; await assert.rejects(() => run({ github: gh('boom'), context: ctx('fix/42-x', GOOD_BODY), core }), /could not verify issue #42/); out.length = 0; await run({ context: { payload: {} }, core }); From 7df8355021a153cacfd24c7eb0a2bc49840d8391 Mon Sep 17 00:00:00 2001 From: TzuHsuan <96853116+TzuH-Hsu@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:24:51 +0800 Subject: [PATCH 07/11] ci: strip code spans and fences with linear scanners, matching backtick runs by length Co-Authored-By: Claude Opus 5 --- scripts/pr-lint.js | 61 +++++++++++++++++++++++++++++++++++++---- scripts/pr-lint.test.js | 3 ++ 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/scripts/pr-lint.js b/scripts/pr-lint.js index 6913fd6..57d2852 100644 --- a/scripts/pr-lint.js +++ b/scripts/pr-lint.js @@ -37,12 +37,63 @@ const EXEMPT_LABELS = ['autorelease: pending']; // GitHub links closing keywords in prose only: not inside HTML comments, // fenced code blocks or inline code spans. Strip those before matching so the -// check agrees with what GitHub will actually close. +// check agrees with what GitHub will actually close. Linear scanners, not +// lazy regexes — the body is attacker-controlled and up to 65,536 chars. + +// Fenced blocks (CommonMark): a line starting with 3+ backticks or tildes +// opens; a line with the same character and at least that many closes; an +// unclosed fence runs to the end of the document. +function stripFencedBlocks(text) { + const kept = []; + let fence = null; // { ch, len } + for (const line of text.split('\n')) { + let m = line.match(/^ {0,3}(`{3,}|~{3,})(.*)$/); + // a backtick fence's info string may not contain a backtick — such a line + // is inline code, not a fence + if (m && m[1][0] === '`' && m[2].includes('`')) m = null; + if (fence) { + if (m && m[1][0] === fence.ch && m[1].length >= fence.len && /^ {0,3}(`+|~+)[ \t]*$/.test(line)) fence = null; + continue; + } + if (m) { fence = { ch: m[1][0], len: m[1].length }; continue; } + kept.push(line); + } + return kept.join('\n'); +} + +// Inline code (CommonMark): a run of N backticks is closed by the next run of +// exactly N. A run length that finds no closer never will later either, so +// each length fails at most once — linear in practice, bounded regardless. +function stripCodeSpans(text) { + let out = ''; + let i = 0; + const failed = new Set(); + while (i < text.length) { + if (text[i] !== '`') { out += text[i++]; continue; } + let j = i; + while (j < text.length && text[j] === '`') j++; + const n = j - i; + if (!failed.has(n)) { + let k = j; + let closed = false; + while (k < text.length) { + if (text[k] !== '`') { k++; continue; } + let m = k; + while (m < text.length && text[m] === '`') m++; + if (m - k === n) { closed = true; break; } + k = m; + } + if (closed) { i = k + n; continue; } + failed.add(n); + } + out += text.slice(i, j); + i = j; + } + return out; +} + function stripNonProse(text) { - return String(text || '') - .replace(//g, '') - .replace(/(^|\n)(`{3,}|~{3,})[^\n]*\n[\s\S]*?\n\2[ \t]*(?=\n|$)/g, '$1') - .replace(/`[^`\n]*`/g, ''); + return stripCodeSpans(stripFencedBlocks(String(text || '').replace(//g, ''))); } const stripHtmlComments = stripNonProse; // kept for callers of the old name diff --git a/scripts/pr-lint.test.js b/scripts/pr-lint.test.js index 680f75f..79e1868 100644 --- a/scripts/pr-lint.test.js +++ b/scripts/pr-lint.test.js @@ -43,6 +43,9 @@ test('a closing keyword inside an HTML comment, a fenced block or inline code do assert.deepEqual(numbers('use `Closes #12` in the body'), []); assert.deepEqual(numbers('```\nCloses #12\n```\nCloses #13'), [13]); assert.deepEqual(numbers('`x` Closes #14 `y`'), [14]); + assert.deepEqual(numbers('``Closes #15``'), []); + assert.deepEqual(numbers('`` `Closes #16` `` and Closes #17'), [17]); + assert.deepEqual(numbers('```Closes #18``` Closes #19'), [19]); }); test('all GitHub closing keywords, the colon form and the full URL form are recognised, case-insensitively', () => { From b872a9c7092d6bd518c1de7a8e8f26b1b8bb3bb8 Mon Sep 17 00:00:00 2001 From: TzuHsuan <96853116+TzuH-Hsu@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:30:54 +0800 Subject: [PATCH 08/11] ci: unterminated HTML comments follow CommonMark; all body scanners linear Co-Authored-By: Claude Opus 5 --- scripts/pr-lint.js | 27 ++++++++++++++++++++++++++- scripts/pr-lint.test.js | 4 ++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/scripts/pr-lint.js b/scripts/pr-lint.js index 57d2852..a9dff8f 100644 --- a/scripts/pr-lint.js +++ b/scripts/pr-lint.js @@ -92,8 +92,33 @@ function stripCodeSpans(text) { return out; } +// HTML comments: `` is dropped wherever it is. An unterminated +// `` fails, every later one would too + while (i < text.length) { + const open = text.indexOf('', open + 4); + if (close !== -1) { i = close + 3; continue; } + noCloser = true; + let k = open; + let spaces = 0; + while (k > 0 && text[k - 1] === ' ' && spaces < 4) { k--; spaces++; } + if (spaces <= 3 && (k === 0 || text[k - 1] === '\n')) break; // block comment to EOF + out += '/g, ''))); + return stripCodeSpans(stripFencedBlocks(stripHtmlCommentsLinear(String(text || '')))); } const stripHtmlComments = stripNonProse; // kept for callers of the old name diff --git a/scripts/pr-lint.test.js b/scripts/pr-lint.test.js index 79e1868..97794e9 100644 --- a/scripts/pr-lint.test.js +++ b/scripts/pr-lint.test.js @@ -38,6 +38,10 @@ test('the untouched template line still fails: the number is inside an HTML comm test('a closing keyword inside an HTML comment, a fenced block or inline code does not count', () => { assert.deepEqual(numbers('\nsome text'), []); + assert.deepEqual(numbers(' Closes #45 '), [45]); assert.deepEqual(numbers('```\nCloses #12\n```\n'), []); assert.deepEqual(numbers('~~~bash\ngit commit -m "Closes #12"\n~~~'), []); assert.deepEqual(numbers('use `Closes #12` in the body'), []); From 3f4f902ea0642d690f697227fae5b1d6ca5705d1 Mon Sep 17 00:00:00 2001 From: TzuHsuan <96853116+TzuH-Hsu@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:36:35 +0800 Subject: [PATCH 09/11] ci: resolve block constructs line by line before inline ones (CommonMark precedence) Co-Authored-By: Claude Opus 5 --- scripts/pr-lint.js | 49 +++++++++++++++++++++++++---------------- scripts/pr-lint.test.js | 9 +++++++- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/scripts/pr-lint.js b/scripts/pr-lint.js index a9dff8f..3e29575 100644 --- a/scripts/pr-lint.js +++ b/scripts/pr-lint.js @@ -39,23 +39,39 @@ const EXEMPT_LABELS = ['autorelease: pending']; // fenced code blocks or inline code spans. Strip those before matching so the // check agrees with what GitHub will actually close. Linear scanners, not // lazy regexes — the body is attacker-controlled and up to 65,536 chars. +// +// Order matters (CommonMark): block constructs first, line by line, whichever +// opens first wins — a `` (that whole line included); unclosed, EOF. +function stripBlocks(text) { const kept = []; let fence = null; // { ch, len } + let comment = false; for (const line of text.split('\n')) { - let m = line.match(/^ {0,3}(`{3,}|~{3,})(.*)$/); + if (fence) { + const c = line.match(/^ {0,3}(`+|~+)[ \t]*$/); + if (c && c[1][0] === fence.ch && c[1].length >= fence.len) fence = null; + continue; + } + if (comment) { + if (line.includes('-->')) comment = false; + continue; + } + let f = line.match(/^ {0,3}(`{3,}|~{3,})(.*)$/); // a backtick fence's info string may not contain a backtick — such a line // is inline code, not a fence - if (m && m[1][0] === '`' && m[2].includes('`')) m = null; - if (fence) { - if (m && m[1][0] === fence.ch && m[1].length >= fence.len && /^ {0,3}(`+|~+)[ \t]*$/.test(line)) fence = null; + if (f && f[1][0] === '`' && f[2].includes('`')) f = null; + if (f) { fence = { ch: f[1][0], len: f[1].length }; continue; } + if (/^ {0,3}')) comment = true; continue; } - if (m) { fence = { ch: m[1][0], len: m[1].length }; continue; } kept.push(line); } return kept.join('\n'); @@ -92,11 +108,10 @@ function stripCodeSpans(text) { return out; } -// HTML comments: `` is dropped wherever it is. An unterminated -// `` is dropped, +// across lines if need be; an unterminated `` fails, every later one would too @@ -107,10 +122,6 @@ function stripHtmlCommentsLinear(text) { const close = noCloser ? -1 : text.indexOf('-->', open + 4); if (close !== -1) { i = close + 3; continue; } noCloser = true; - let k = open; - let spaces = 0; - while (k > 0 && text[k - 1] === ' ' && spaces < 4) { k--; spaces++; } - if (spaces <= 3 && (k === 0 || text[k - 1] === '\n')) break; // block comment to EOF out += ' Closes #45 '), [45]); + assert.deepEqual(numbers(' Closes #45 '), []); // line-start + assert.deepEqual(numbers('x Closes #45 '), [45]); // mid-line comments are inline; the prose between them counts + // nesting: whichever block opens first wins + assert.deepEqual(numbers('```\n\nCloses #47'), [47]); // ``` inside a comment block is comment + assert.deepEqual(numbers('`'), [49]); // leftmost construct wins: the backtick makes Closes #51'), [51]); // inline comment across lines assert.deepEqual(numbers('```\nCloses #12\n```\n'), []); assert.deepEqual(numbers('~~~bash\ngit commit -m "Closes #12"\n~~~'), []); assert.deepEqual(numbers('use `Closes #12` in the body'), []); From 18a4509e256bc574112c7b027e7fcc217c2cf883 Mon Sep 17 00:00:00 2001 From: TzuHsuan <96853116+TzuH-Hsu@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:42:17 +0800 Subject: [PATCH 10/11] ci: scan .yaml workflows too for required scripts Co-Authored-By: Claude Opus 5 --- scripts/check-node-tests.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/check-node-tests.sh b/scripts/check-node-tests.sh index bf53477..e378649 100755 --- a/scripts/check-node-tests.sh +++ b/scripts/check-node-tests.sh @@ -23,7 +23,7 @@ command -v node >/dev/null 2>&1 || { echo "install: node (e.g. brew install node # is here — an adopter who takes a thin-caller workflow without its script # gets a job that fails on every event while `make check` stays green. missing=0 -for wf in .github/workflows/*.yml; do +for wf in .github/workflows/*.yml .github/workflows/*.yaml; do [ -f "$wf" ] || continue while IFS= read -r js; do [ -n "$js" ] || continue From 26354b35540cca5fc39393916a7a7adb6b29daa8 Mon Sep 17 00:00:00 2001 From: TzuHsuan <96853116+TzuH-Hsu@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:47:27 +0800 Subject: [PATCH 11/11] ci: every workflow-required script must ship with its test Co-Authored-By: Claude Opus 5 --- scripts/check-node-tests.sh | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/scripts/check-node-tests.sh b/scripts/check-node-tests.sh index e378649..e5dccdf 100755 --- a/scripts/check-node-tests.sh +++ b/scripts/check-node-tests.sh @@ -20,8 +20,9 @@ cd "$(dirname "$0")/.." command -v node >/dev/null 2>&1 || { echo "install: node (e.g. brew install node) — needed for scripts/*.test.js"; exit 1; } # Every `require(... /scripts/.js)` in a workflow must name a file that -# is here — an adopter who takes a thin-caller workflow without its script -# gets a job that fails on every event while `make check` stays green. +# is here, with its sibling test — an adopter who takes a thin-caller workflow +# without its script gets a job that fails on every event while `make check` +# stays green, and a handler without a test is outside ADR-0008. missing=0 for wf in .github/workflows/*.yml .github/workflows/*.yaml; do [ -f "$wf" ] || continue @@ -30,8 +31,12 @@ for wf in .github/workflows/*.yml .github/workflows/*.yaml; do if [ ! -f "$js" ]; then echo "FAIL: $wf requires $js, which is missing — take the workflow and the script together (docs/template/upgrading.md)" missing=$((missing + 1)) + elif [ ! -f "${js%.js}.test.js" ]; then + # ADR-0008: a handler a workflow calls is tested, no exceptions + echo "FAIL: $wf requires $js, which has no ${js%.js}.test.js — every workflow handler ships with its test (ADR-0008)" + missing=$((missing + 1)) else - echo "OK: $wf requires $js (present)" + echo "OK: $wf requires $js (present, tested by ${js%.js}.test.js)" fi done <