diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b06eff7..badb697 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,35 @@ 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 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 +# 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. The script uses the +# client for exactly one read: confirming the linked issue exists, is open, +# and is an issue, not a pull request. 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. + # `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, unlabeled] push: branches: - main @@ -61,9 +88,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({ github, 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..39cb45e 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 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 | | `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..e5dccdf --- /dev/null +++ b/scripts/check-node-tests.sh @@ -0,0 +1,58 @@ +#!/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, 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 + 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)) + 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, tested by ${js%.js}.test.js)" + fi + done </-`; +// 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, 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. +// +// 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; +// 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']; + +// 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. 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')) { + 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 (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; + } + 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; +} + +// Inline HTML comments (block ones are gone by now): `` is dropped, +// across lines if need be; 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; + out += '`; replace the comment ' + + '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) { + problems.push( + `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 }; +} + +// 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`; + 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; +} + +// Entry point for actions/github-script. +async function run({ github, 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, + author: pr.user && pr.user.login, + labels: (pr.labels || []).map((l) => l.name), + }); + if (result.exempt) { + 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; + } + 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, verifyIssue, isExempt, linkedIssues, describe, stripNonProse, stripBlocks, stripCodeSpans, stripInlineComments, stripHtmlComments, TYPES, BRANCH_RE, CLOSES_RE, EXEMPT_AUTHORS, EXEMPT_LABELS }; diff --git a/scripts/pr-lint.test.js b/scripts/pr-lint.test.js new file mode 100644 index 0000000..006aeb2 --- /dev/null +++ b/scripts/pr-lint.test.js @@ -0,0 +1,146 @@ +'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, why: null, 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 inside an HTML comment, a fenced block or inline code does not count', () => { + assert.deepEqual(numbers('\nsome text'), []); + 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'), []); + 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', () => { + 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('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, 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.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', () => { + const r = lint({ headRef: 'feat/3-x', body: 'Refs #3' }); + assert.match(r.problems[0], /sub-issue it can close/); +}); + +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', () => { + 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 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 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 }); + 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; + 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, 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 }); + 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 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 }); + 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]']]); +}); 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..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. 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. @@ -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.