diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 2f363571a..4934d7f2f 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -68,6 +68,7 @@ CodeQL (`codeql.yml`, which `ci.yml` calls) scans the workflows, the TypeScript - Keep each pull request to one concern. - Fill in the template. +- Link the issue it's for, as in [Issues and planning](#issues-and-planning), or add the `no-issue` label. The PR issue check fails otherwise. - For UI changes, add before and after screenshots at phone and wide widths. - Pull requests merge with a merge commit. Squash and rebase merges are off. diff --git a/.github/NEW_PROJECT.md b/.github/NEW_PROJECT.md index 2fbf7e288..da175134f 100644 --- a/.github/NEW_PROJECT.md +++ b/.github/NEW_PROJECT.md @@ -4,7 +4,7 @@ This covers what the repository runs on and every place a new project has to be Most of the steps are checked, so a missed one fails CI with a message that says what to add: -- `tools/policy.sh` (the Repo checks workflow) runs Rego policies in `tools/policy/` over the config files with [conftest](https://www.conftest.dev). They check each TypeScript app's scripts, `package.json` fields and git hooks; that every workflow on pull requests (but dependency review) has the `changes`, work and `passed` jobs and no `paths` in its trigger, that the workflows it calls have no concurrency group or push or pull request trigger and have report jobs that can't fail the call, that one of them runs when each app, package, crate or plugin changes, and that the `locallibrary` filter has every file the Rust code embeds from outside its crate; that `.github/rulesets/protect-main.json` requires every `passed` check; that the labels, labeler and issue forms know every app and every file gets a label; that Rust packages share the workspace's settings and the dev profile keeps its entries for the crates in `Cargo.lock`; that lexicons are filed by NSID; that Dependabot updates every lockfile; and that an app with a Dockerfile has a Railway service in `.railway/railway.ts`, which has to type-check against the railway SDK. It needs Docker, and Node with the root's dev dependencies locally. The policies' own tests have to cover every line. +- `tools/policy.sh` (the Repo checks workflow) runs Rego policies in `tools/policy/` over the config files with [conftest](https://www.conftest.dev). They check each TypeScript app's scripts, `package.json` fields and git hooks; that every workflow on pull requests (but dependency review) has the `changes`, work and `passed` jobs (`pr-issue.yml` reads no files, so it has no `changes` job) and no `paths` in its trigger, that the workflows it calls have no concurrency group or push or pull request trigger and have report jobs that can't fail the call, that one of them runs when each app, package, crate or plugin changes, and that the `locallibrary` filter has every file the Rust code embeds from outside its crate; that `.github/rulesets/protect-main.json` requires every `passed` check; that the labels, labeler and issue forms know every app and every file gets a label; that Rust packages share the workspace's settings and the dev profile keeps its entries for the crates in `Cargo.lock`; that lexicons are filed by NSID; that Dependabot updates every lockfile; and that an app with a Dockerfile has a Railway service in `.railway/railway.ts`, which has to type-check against the railway SDK. It needs Docker, and Node with the root's dev dependencies locally. The policies' own tests have to cover every line. - The website's tests check that every folder in `apps/` has a tool page and is named in the root README, CONTRIBUTING, SECURITY, the website's README and the `web: tools` label (`apps/web/src/lib/tools/docs.test.ts`). - locallibrary's build fails while a folder in `plugins/` isn't one of its dependencies, or a folder in `plugins/accounts/` has no manifest (`apps/locallibrary/build.rs`), and its tests check that every one ships and is in its docs (`apps/locallibrary/tests/it/plugins.rs`). - clippy's `cargo_common_metadata` wants full metadata on every crate that isn't `publish = false`, and the release workflow's plan fails while it would release anything but locallibrary. @@ -71,7 +71,7 @@ Jobs that only report have `continue-on-error: true`, so they never fail their c Pull requests run on Linux. The plan's 5 macOS runners can't keep up with every push, so locallibrary's macOS and Windows job (`other-os`) runs once for each merge queue group, which runs nothing else but CodeQL, or on a pull request with the `all-os` label. `locallibrary-cache.yml` runs it nightly on `main`, which is where its caches are saved, when what they're keyed on changed or they were evicted. A push to `main`, whose commit is the merge queue group's, runs only what pull requests use from it: the website's and localtoolbox's coverage and sizes, which the reports compare with, the Rust and Docker caches when what they're keyed on changed, CodeQL, and the ruleset check when the ruleset file changed. -The "Protect Main" ruleset requires "CI passed", which passes when it's skipped, so a tool's checks never block a pull request that doesn't touch it. The ruleset is `.github/rulesets/protect-main.json`: an admin applies it with `tools/ruleset.sh apply` once the pull request that changes it merges, and the Repo checks workflow fails on `main` while GitHub's copy differs. A pull request that renames or drops a required check is the exception: GitHub won't merge it while the old name is required, so the admin applies the file from its branch first, and open pull requests then merge `main` to report the new name. It merges through a merge queue, which builds at most 2 groups of up to 5 pull requests at once, and requires CI passed and code scanning, which is `codeql.yml`'s CodeQL analysis of the workflows, the TypeScript and the Rust, leaving out the Rust tests (`.github/codeql/codeql-config.yml`). `ci.yml` calls it with every language on every pull request, group and push, whatever changed: the rule holds a commit until it has a result in each category `main` has. A language whose code, CodeQL config and `codeql.yml` didn't change uploads `main`'s analysis of the commit it was diffed against for this commit, where `main` has one, rather than analysing again. +The "Protect Main" ruleset requires "CI passed", which passes when it's skipped, so a tool's checks never block a pull request that doesn't touch it. It also requires "PR issue passed" from `pr-issue.yml`, which runs on its own so that editing a description or a label doesn't run CI again; on a merge queue group it passes at once, since each pull request in it already passed. The ruleset is `.github/rulesets/protect-main.json`: an admin applies it with `tools/ruleset.sh apply` once the pull request that changes it merges, and the Repo checks workflow fails on `main` while GitHub's copy differs. A pull request that renames or drops a required check is the exception: GitHub won't merge it while the old name is required, so the admin applies the file from its branch first, and open pull requests then merge `main` to report the new name. It merges through a merge queue, which builds at most 2 groups of up to 5 pull requests at once, and requires CI passed and code scanning, which is `codeql.yml`'s CodeQL analysis of the workflows, the TypeScript and the Rust, leaving out the Rust tests (`.github/codeql/codeql-config.yml`). `ci.yml` calls it with every language on every pull request, group and push, whatever changed: the rule holds a commit until it has a result in each category `main` has. A language whose code, CodeQL config and `codeql.yml` didn't change uploads `main`'s analysis of the commit it was diffed against for this commit, where `main` has one, rather than analysing again. Changes reach `main` only through a pull request, merged as a merge commit. Each one needs an approval from a code owner (`.github/CODEOWNERS`) given after its last push, and a new push dismisses earlier approvals, so every pull request waits for a code owner to approve its latest push. @@ -88,7 +88,7 @@ Repository-wide checks: `npm-advisories.yml` runs `pnpm audit --prod` over the l Labels: `ci.yml`'s changes job labels pull requests from `labeler.yml` (`pr-labels.yml` labels those from forks, whose token can't), and `labels.yml` (the workflow) syncs `.github/labels.yml` to GitHub. Each tool has a label, and its areas are `": "` labels in a lighter shade of its colour. -Issues are planned in Linear, in the `localnetwork` workspace's Localnetwork team (`LOC`). Linear's GitHub integration syncs this repository's issues both ways with that team and links pull requests to them, so nothing in the repository runs for it. Its settings live in Linear, so they're listed here: +Issues are planned in Linear, in the `localnetwork` workspace's Localnetwork team (`LOC`). Linear's GitHub integration syncs this repository's issues both ways with that team and links pull requests to them, and `pr-issue.yml` fails a pull request that references no issue, unless it has the `no-issue` label. The integration's settings live in Linear, so they're listed here: - Settings > Integrations > GitHub: the Linear app installed on `cutenode` with access to `cutenode/localnetwork`, and two-way Issues Sync from `cutenode/localnetwork` to Localnetwork. The issues from before the sync came in through the GitHub importer (Settings > Import/Export), which keeps them synced the same way. - Settings > Workspace > Integrations > GitHub > Branch format: `bnb/{issueIdentifier}-{issueTitle}`, so "Copy git branch name" gives a `bnb/loc-123-…` branch. diff --git a/.github/labels.yml b/.github/labels.yml index 95bfaed82..11aa864f4 100644 --- a/.github/labels.yml +++ b/.github/labels.yml @@ -132,6 +132,9 @@ - name: good first issue color: "7057ff" description: Good for newcomers +- name: no-issue + color: "cfd3d7" + description: No issue to link, so the PR issue check passes - name: help wanted color: "008672" description: Extra attention is needed diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index dc3787d1a..cd94cfa75 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -2,7 +2,7 @@ ## What and why - + ## How it was tested diff --git a/.github/rulesets/protect-main.json b/.github/rulesets/protect-main.json index 486fd02fa..84d610e7f 100644 --- a/.github/rulesets/protect-main.json +++ b/.github/rulesets/protect-main.json @@ -64,6 +64,9 @@ "required_status_checks": [ { "context": "CI passed" + }, + { + "context": "PR issue passed" } ] } diff --git a/.github/workflows/pr-issue.yml b/.github/workflows/pr-issue.yml new file mode 100644 index 000000000..80559e2e0 --- /dev/null +++ b/.github/workflows/pr-issue.yml @@ -0,0 +1,59 @@ +name: PR issue + +# Fails a pull request that doesn't say which issue it's for, so Linear links the work to it +# (.github/CONTRIBUTING.md, "Issues and planning"). It passes on `LOC-123`, `#123` or the issue's +# URL in the description, a `bnb/loc-123-...` branch, or the no-issue label. It reads only the +# event, never the code, so it has no files to filter and no changes job, and it runs again on each +# edit and label change, which is why it isn't in ci.yml. The ruleset requires its passed check, so +# it runs on merge queue groups too, where it passes at once: each pull request in a group passed. +on: + pull_request: + types: [opened, edited, reopened, synchronize, labeled, unlabeled] + merge_group: + +permissions: {} + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + # Bots' pull requests, such as Dependabot's, have no issue, and the no-issue label marks others + # that don't. + issue: + if: >- + github.event_name == 'pull_request' && github.event.pull_request.user.type != 'Bot' + && !contains(github.event.pull_request.labels.*.name, 'no-issue') + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - env: + BODY: ${{ github.event.pull_request.body }} + BRANCH: ${{ github.event.pull_request.head.ref }} + REPO: ${{ github.repository }} + run: | + shopt -s nocasematch + # The template's hints are HTML comments, and their example references would count. + body=$(perl -0pe 's///gs' <<< "$BODY") + reference="(^|[^[:alnum:]_])(loc-[0-9]+|#[0-9]+)|https://github\.com/$REPO/issues/[0-9]+" + if [[ $body =~ $reference || $BRANCH =~ ^bnb/loc-[0-9]+- ]]; then + exit 0 + fi + echo "::error::Reference the issue in the description (Fixes LOC-123, Part of LOC-123 or Fixes #123), name the branch bnb/loc-123-..., or add the no-issue label." + exit 1 + + # The check the "Protect Main" ruleset requires. It runs, and fails, only when the issue job failed + # or was skipped for anything but a merge queue group, a bot or the no-issue label; a skipped + # passed job counts as passing. + passed: + name: PR issue passed + if: >- + always() && (contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') + || (needs.issue.result == 'skipped' && github.event_name == 'pull_request' + && github.event.pull_request.user.type != 'Bot' + && !contains(github.event.pull_request.labels.*.name, 'no-issue'))) + needs: issue + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - run: exit 1 diff --git a/package.json b/package.json index 027a7a707..22a679282 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,8 @@ "oxlint": "1.86.0", "railway": "^3.16.0", "typescript": "^6.0.3", - "vitest": "^5.0.5" + "vitest": "^5.0.5", + "yaml": "^2.9.1" }, "engines": { "node": ">=24.12" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ce4747033..100c80733 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -253,6 +253,9 @@ importers: vitest: specifier: ^5.0.5 version: 5.0.5(@opentelemetry/api@1.9.1)(@types/node@24.19.2)(@vitest/browser-playwright@5.0.5)(@vitest/coverage-v8@5.0.5)(@vitest/ui@5.0.5)(jsdom@30.1.3(@noble/hashes@1.8.0))(vite@8.3.4(@types/node@24.19.2)(esbuild@0.28.2)(jiti@2.7.0)(tsx@4.23.15)(yaml@2.9.1)) + yaml: + specifier: ^2.9.1 + version: 2.9.1 apps/localtoolbox: dependencies: diff --git a/tools/policy/pr-issue.test.ts b/tools/policy/pr-issue.test.ts new file mode 100644 index 000000000..028fae797 --- /dev/null +++ b/tools/policy/pr-issue.test.ts @@ -0,0 +1,52 @@ +import { spawnSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { parse } from 'yaml'; + +const repo = resolve(import.meta.dirname, '../..'); + +const workflow = parse(readFileSync(resolve(repo, '.github/workflows/pr-issue.yml'), 'utf8')); + +const template = readFileSync(resolve(repo, '.github/pull_request_template.md'), 'utf8'); + +const script: string = workflow.jobs.issue.steps[0].run; + +// Runs the workflow's own step as the runner does, for a pull request with this description and branch. +const passes = (BODY: string, BRANCH = 'bnb/some-feature'): boolean => + spawnSync('bash', ['-e', '-c', script], { + env: { PATH: process.env.PATH, BODY, BRANCH, REPO: 'cutenode/localnetwork' }, + stdio: 'ignore' + }).status === 0; + +describe('the PR issue check', () => { + it.each([ + 'Fixes LOC-12', + 'part of loc-9', + 'Fixes #301', + '(#301)', + 'See https://github.com/cutenode/localnetwork/issues/301' + ])('passes on %s', (body) => { + expect(passes(body)).toBe(true); + }); + + it('passes on a bnb/loc-123 branch with an empty description', () => { + expect(passes('', 'bnb/loc-44-foo')).toBe(true); + }); + + it.each([ + ['an empty description', ''], + ['the unedited template', template], + ['a reference only in a comment', 'Nothing yet. '], + ['a comment across lines', ''], + ['the end of a longer word', 'ALLOC-12 and issue#12'], + ['an issue in another repository', 'https://github.com/cutenode/other/issues/301'], + ['a pull request URL', 'https://github.com/cutenode/localnetwork/pull/301'] + ])('fails on %s', (_, body) => { + expect(passes(body)).toBe(false); + }); + + it.each(['bnb/feature-loc-44', 'loc-44-foo', 'bnb/loc-foo'])('fails on the branch %s', (branch) => { + expect(passes('', branch)).toBe(false); + }); +}); diff --git a/tools/policy/workflows.rego b/tools/policy/workflows.rego index 7db3ef4d4..974f95c71 100644 --- a/tools/policy/workflows.rego +++ b/tools/policy/workflows.rego @@ -8,6 +8,10 @@ import data.lib # pull request adds, which isn't a required check. ungated := {".github/workflows/dependency-review.yml"} +# Gated workflows that check the pull request itself rather than its files, so they have nothing +# to filter and no changes job: pr-issue.yml reads the description, branch and labels. +unfiltered := {".github/workflows/pr-issue.yml"} + # Jobs that never hold up the passed check: `report` posts coverage on the pull request, so a # comment that can't post never holds up a merge, and `semver` only reports breaking changes in the # loclib-* crates, which aren't published yet. In a gated workflow they stay out of passed's needs; @@ -45,6 +49,7 @@ needs(job) := lib.strings(object.get(job, "needs", [])) deny contains msg if { some path, wf in gated + not path in unfiltered not wf.jobs.changes msg := sprintf("%s: add a `changes` job that runs dorny/paths-filter, which the other jobs need (see localtoolbox-ci.yml)", [path]) } diff --git a/tools/policy/workflows_test.rego b/tools/policy/workflows_test.rego index 364ff2252..d0ce5ff91 100644 --- a/tools/policy/workflows_test.rego +++ b/tools/policy/workflows_test.rego @@ -27,6 +27,16 @@ test_no_changes_job if { ".github/workflows/site-ci.yml: add a `changes` job that runs dorny/paths-filter, which the other jobs need (see localtoolbox-ci.yml)" in workflows.deny with input as with_ci(broken) } +test_unfiltered_workflow_needs_no_changes_job if { + unfiltered := json.patch(ci, [ + {"op": "remove", "path": "/jobs/changes"}, + {"op": "remove", "path": "/jobs/e2e"}, + {"op": "remove", "path": "/jobs/verify/needs"}, + {"op": "replace", "path": "/jobs/passed/needs", "value": ["verify"]}, + ]) + count(workflows.deny) == 0 with input as fixtures.with_file(".github/workflows/pr-issue.yml", unfiltered) +} + test_trigger_paths if { broken := json.patch(ci, [{"op": "replace", "path": "/true/pull_request", "value": {"paths-ignore": ["docs/**"]}}]) ".github/workflows/site-ci.yml: move `on.pull_request.paths-ignore` into the changes job's filter; the passed check has to report on every pull request" in workflows.deny with input as with_ci(broken)