diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 73b05acb8..169c1f44f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -137,7 +137,7 @@ jobs: - name: The self-test catalog page matches docs/self-test.json (#782) run: node scripts/self-test-catalog.mjs --check - name: Tests of the repo scripts (risk tier, labels) - run: node --test ".github/scripts/*.test.mjs" + run: node --test ".github/scripts/*.test.mjs" "policy/*.test.mjs" - name: Offline cross-runtime harness (no network or model) run: npm run test:interop - name: Changelog has an Unreleased section and well-formed fragments (changelog.d/) diff --git a/changelog.d/2303-gate-list-derived.changed.md b/changelog.d/2303-gate-list-derived.changed.md new file mode 100644 index 000000000..bb9c8f2fc --- /dev/null +++ b/changelog.d/2303-gate-list-derived.changed.md @@ -0,0 +1 @@ +The gate list is derived from `docs/testing-layers.json` now instead of being retyped: a workflow the manifest marks `gate: true` is a `Policy-sign:` gate file by construction, so `merge-gate.yml`, `enqueue-guard.yml` and the container-image build `docker.yml` joined the list, together with `gate-check.mjs`, the script that writes the `enqueue-guard` check run — `policy/gates.test.mjs` pins each gate workflow to the scripts it runs in both directions, so a pull request can no longer weaken a gate's switch or move a rule under another workflow without a second person's signature ([#2303](https://github.com/ThinkFlowLab/FlyBot/issues/2303)). diff --git a/docs/testing-layers.json b/docs/testing-layers.json index 330967c74..38008d238 100644 --- a/docs/testing-layers.json +++ b/docs/testing-layers.json @@ -8,7 +8,7 @@ "guarantee": "The cheapest layer that can catch a defect: it must be deterministic and hermetic — no network, no model, no wall-clock or calendar dependence, no ordering between files.", "mustNot": "Reach the network, call a model, read the clock, or depend on a machine's home directory.", "budget": "The whole layer under 10 minutes on a cold runner. A suite that cannot fit is either split or moved up, not added.", - "command": "node .github/scripts/testing-layers.mjs --check && npm --prefix core test && npm --prefix plugins/social test && npm --prefix plugins/tree test && npm --prefix plugins/timeline test && npm --prefix plugins/workflow test && node --test \".github/scripts/*.test.mjs\" && npm run test:interop" + "command": "node .github/scripts/testing-layers.mjs --check && npm --prefix core test && npm --prefix plugins/social test && npm --prefix plugins/tree test && npm --prefix plugins/timeline test && npm --prefix plugins/workflow test && node --test \".github/scripts/*.test.mjs\" \"policy/*.test.mjs\" && npm run test:interop" }, "L2": { "name": "merge", @@ -77,9 +77,10 @@ "id": "scripts", "layer": "L1", "paths": [ - ".github/scripts/*.test.mjs" + ".github/scripts/*.test.mjs", + "policy/*.test.mjs" ], - "command": "node --test \".github/scripts/*.test.mjs\"", + "command": "node --test \".github/scripts/*.test.mjs\" \"policy/*.test.mjs\"", "note": "The repository's own gates: risk tier, review gate, merge gate, enqueue guard, changelog, labels, and this manifest's check." }, { diff --git a/docs/testing-layers.md b/docs/testing-layers.md index ba5b18ac2..3fe6f493b 100644 --- a/docs/testing-layers.md +++ b/docs/testing-layers.md @@ -82,7 +82,7 @@ The tables below are generated: edit `docs/testing-layers.json`, then run | `plugins/tree` | L1 | `plugins/tree/test/**/*.test.js` | `npm --prefix plugins/tree test` | | `plugins/timeline` | L1 | `plugins/timeline/test/**/*.test.js` | `npm --prefix plugins/timeline test` | | `plugins/workflow` | L1 | `plugins/workflow/test/**/*.test.js` | `npm --prefix plugins/workflow test` | -| `scripts` | L1 | `.github/scripts/*.test.mjs` | `node --test ".github/scripts/*.test.mjs"` | +| `scripts` | L1 | `.github/scripts/*.test.mjs`
`policy/*.test.mjs` | `node --test ".github/scripts/*.test.mjs" "policy/*.test.mjs"` | | `interop` | L1 | `dev/interop/**/*.test.mjs` | `npm run test:interop` | | `patch-payload` | none (see reason) | `patches/ds-bot/files/**` | — Generated copies of the files the patches carry. They are compared against `core/` by `node scripts/ds-bot-patches.mjs check` (L2) and never run on their own; the code under test is the copy in `core/`. | | `upstream` | none (see reason) | `ds-bot/test/**` | — The pristine upstream checkout at the pinned version. FlyBot never edits or runs it directly — `core/` is `ds-bot/` plus the patches, and the `core` suite runs the patched copies. | diff --git a/package.json b/package.json index e49d35720..7b80f69b7 100644 --- a/package.json +++ b/package.json @@ -6,7 +6,7 @@ "node": ">=22" }, "scripts": { - "verify": "npm --prefix core run lint && node scripts/lint-plugins.mjs && node core/scripts/build-client.mjs --check && node scripts/ds-bot-patches.mjs check && node scripts/ds-bot-patches.mjs canonical && npm --prefix core test && npm --prefix plugins/social test && npm --prefix plugins/tree test && npm --prefix plugins/timeline test && npm --prefix plugins/workflow test && node --test \".github/scripts/*.test.mjs\" && npm run lint:interop && npm run test:interop && npm run lint:live-e2e && npm run test:live-e2e", + "verify": "npm --prefix core run lint && node scripts/lint-plugins.mjs && node core/scripts/build-client.mjs --check && node scripts/ds-bot-patches.mjs check && node scripts/ds-bot-patches.mjs canonical && npm --prefix core test && npm --prefix plugins/social test && npm --prefix plugins/tree test && npm --prefix plugins/timeline test && npm --prefix plugins/workflow test && node --test \".github/scripts/*.test.mjs\" \"policy/*.test.mjs\" && npm run lint:interop && npm run test:interop && npm run lint:live-e2e && npm run test:live-e2e", "lint:interop": "node scripts/lint-interop.mjs", "test:interop": "node --import ./dev/interop/no-network.mjs --test \"dev/interop/test/*.test.mjs\"", "demo:interop": "node --import ./dev/interop/no-network.mjs dev/interop/demo.mjs", diff --git a/policy/gates.json b/policy/gates.json index 2cd3933a9..bd805bb4b 100644 --- a/policy/gates.json +++ b/policy/gates.json @@ -11,7 +11,11 @@ ".github/scripts/policy-sign.mjs", ".github/scripts/enqueue-guard.mjs", ".github/scripts/enqueue-rules.mjs", + ".github/scripts/gate-check.mjs", ".github/workflows/test.yml", + ".github/workflows/merge-gate.yml", + ".github/workflows/enqueue-guard.yml", + ".github/workflows/docker.yml", "policy/" ] } diff --git a/policy/gates.test.mjs b/policy/gates.test.mjs new file mode 100644 index 000000000..43435a90e --- /dev/null +++ b/policy/gates.test.mjs @@ -0,0 +1,144 @@ +// The gate list is derived from the test-layer manifest (#2303). +// +// `policy/gates.json` decides which files need another person's signature, and it was handwritten. +// The repository already carries the machine-readable answer: `docs/testing-layers.json` marks a +// workflow `gate: true` when a merge cannot land without it, and `policy/gates.json`'s own `$comment` +// says "a rule that does not cover its own switch is advice, not a gate". The two drifted — +// `merge-gate.yml` and `enqueue-guard.yml` are gates by the manifest and were not gate files, and +// `gate-check.mjs`, which writes the check run the fork's ruleset requires, was outside too. +// +// These tests are the derivation: a `gate: true` workflow is a gate file by construction, and each +// gate workflow is pinned to the scripts it runs, in both directions — the workflow runs the script +// itself or imports it, and no other workflow runs it. A new gate workflow, or a rule moving under +// another workflow, fails here instead of shipping advice. +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { readFileSync, readdirSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import { dirname, join, relative, resolve } from 'node:path' + +const ROOT = fileURLToPath(new URL('..', import.meta.url)) +const read = (path) => readFileSync(join(ROOT, path), 'utf8') + +const manifest = JSON.parse(read('docs/testing-layers.json')) +const policy = JSON.parse(read('policy/gates.json')) +const gateFiles = new Set(policy.gateFiles) +const gateWorkflows = manifest.workflows.filter((row) => row.gate === true).map((row) => row.workflow) + +const workflowFiles = readdirSync(join(ROOT, '.github/workflows')) + .filter((file) => /\.ya?ml$/.test(file)) + .sort() + .map((file) => `.github/workflows/${file}`) + +// The scripts a workflow starts, read from its `node ` steps. `base/` is stripped because the +// judging workflows check the base commit out under that prefix and run the script from there. +function runsOf(workflow) { + const out = new Set() + for (const match of read(workflow).matchAll(/\bnode\s+([\w./-]+\.mjs)\b/g)) { + out.add(match[1].replace(/^base\//, '')) + } + return out +} + +// The repository-relative scripts a script imports, following only relative specifiers. A step that +// runs `node scripts/x.mjs` from another package (`core/`, a plugin) is not this repository's script +// at that path, so a missing file is simply not one of them. +function importsOf(script) { + const out = new Set() + let text + try { + text = read(script) + } catch { + return out + } + for (const match of text.matchAll(/\bfrom\s+['"](\.[^'"]+)['"]/g)) { + out.add(relative(ROOT, resolve(ROOT, dirname(script), match[1]))) + } + return out +} + +// A script and everything it imports: `gate-check.mjs` is the run step, and the rules it judges by +// are one and two imports down (`enqueue-guard.mjs`, `enqueue-rules.mjs`). +function reachableFrom(seeds) { + const seen = new Set() + const queue = [...seeds] + while (queue.length > 0) { + const script = queue.shift() + if (seen.has(script) || !script.endsWith('.mjs')) continue + seen.add(script) + queue.push(...importsOf(script)) + } + return seen +} + +test('#2303: every workflow the manifest marks as a gate is a gate file', () => { + const missing = gateWorkflows.filter((workflow) => !gateFiles.has(workflow)) + assert.deepEqual( + missing, + [], + `a workflow a merge cannot land without is a gate by construction; add it to \`gateFiles\` in policy/gates.json: ${missing.join(', ')}`, + ) + + // The other direction: a workflow quoted in `gateFiles` that the manifest does not call a gate is + // a stale entry, and it would ask for a signature the repository no longer requires. + const stale = [...gateFiles].filter((file) => file.startsWith('.github/workflows/') && !gateWorkflows.includes(file)) + assert.deepEqual(stale, [], `\`gateFiles\` names a workflow the manifest does not mark \`gate: true\`: ${stale.join(', ')}`) +}) + +test('#2303: each gate workflow is pinned to the scripts it runs, in both directions', () => { + // The explicit table, rather than an inference: the manifest says which workflows are gates, and + // this says which scripts each one judges by. A rule that moves to another workflow fails here. + const PINS = [ + { + workflow: '.github/workflows/merge-gate.yml', + scripts: ['.github/scripts/merge-gate.mjs'], + }, + { + workflow: '.github/workflows/enqueue-guard.yml', + scripts: [ + '.github/scripts/gate-check.mjs', + '.github/scripts/enqueue-guard.mjs', + '.github/scripts/enqueue-rules.mjs', + ], + }, + { + workflow: '.github/workflows/test.yml', + scripts: ['.github/scripts/policy-sign.mjs'], + }, + ] + + const runs = new Map(workflowFiles.map((workflow) => [workflow, runsOf(workflow)])) + const reached = new Map(workflowFiles.map((workflow) => [workflow, reachableFrom(runs.get(workflow))])) + + for (const pin of PINS) { + for (const script of pin.scripts) { + assert.ok(gateFiles.has(script), `${script} decides a gate workflow, so it is a gate file too; add it to policy/gates.json`) + assert.ok(reached.get(pin.workflow).has(script), `${pin.workflow} does not run ${script} (directly or through its imports)`) + for (const other of workflowFiles) { + if (other === pin.workflow) continue + assert.equal(runs.get(other).has(script), false, `${other} runs ${script} too; a gate script belongs to one workflow, and the pin above is stale`) + } + } + } +}) + +// The image build is `gate: true` in the manifest — a packaging change cannot land without the image +// proving it starts — so it is a gate file as well. Product asked the implementer to include it in +// this change unless the image build is not a gate; the row answers that it is. The decision is +// written down here instead of inferred, so a manifest that stops calling it a gate has to move this +// line in the same pull request. +const DOCKER_WORKFLOW = '.github/workflows/docker.yml' + +test('#2303: the image build is a gate file while the manifest calls it a gate', () => { + const row = manifest.workflows.find((entry) => entry.workflow === DOCKER_WORKFLOW) + assert.ok(row !== undefined, `${DOCKER_WORKFLOW} has no row in docs/testing-layers.json`) + assert.equal( + row.gate, + true, + 'the manifest no longer marks the image build a gate; then take it out of `gateFiles` in the same change', + ) + assert.ok( + gateFiles.has(DOCKER_WORKFLOW), + 'docker.yml builds the image a packaging change must start before it merges, so it is a gate file', + ) +})