diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 66b980ab..9f5e3d55 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,6 +63,13 @@ jobs: # A pull request needs the whole range for `git diff base...head`; a # push or schedule classifies every flag as true without reading git. fetch-depth: ${{ (github.event_name == 'pull_request' || github.event_name == 'merge_group') && 0 || 1 }} + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + # #640: the gate is policy, so its code comes from the base branch and + # never from the pull request it judges. `path: base` keeps it beside + # the checkout that supplies the data under test. + ref: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha || github.event.repository.default_branch }} + path: base - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: 22 @@ -78,11 +85,23 @@ jobs: GROUP_BASE_REF: ${{ github.event.merge_group.base_ref }} run: | set -euo pipefail + # #640: the pull request that introduces `scripts/gate-integrity.mjs` + # runs this job with a base branch that predates the file. The test is + # against the base tree, so a diff cannot decide whether the summary + # runs; without the file there is no summary to write, and the merge + # still needs its T3 PASS. + gate_integrity() { + if [ -f base/scripts/gate-integrity.mjs ]; then + node base/scripts/gate-integrity.mjs "$@" + else + echo "::notice::gate-integrity is not on the base branch yet; this change introduces it" + fi + } ref="" case "$EVENT" in pull_request) base="$PR_BASE"; head="$PR_HEAD"; ref="$PR_BASE_REF" ;; merge_group) base="$GROUP_BASE"; head="$GROUP_HEAD"; ref="$GROUP_BASE_REF" ;; - *) node scripts/ci-changes.mjs --all; exit 0 ;; + *) node base/scripts/ci-changes.mjs --all; gate_integrity --all; exit 0 ;; esac # #752: a branch cut before the latest `main` names a base commit that # this checkout does not carry, so the two-commit diff could not be @@ -91,7 +110,10 @@ jobs: # CodeQL never starts for an ignored path. The script fetches the base # and retries; the conservative every-gate answer is still its last # resort, and it lives there where the suite can drive it. - node scripts/ci-changes.mjs --git-base "$base" --git-head "$head" --base-ref "$ref" + node base/scripts/ci-changes.mjs --repo . --git-base "$base" --git-head "$head" --base-ref "$ref" + # #640: the reviewer must see that the gate is inside the diff this gate + # judges; an unreadable diff fails closed instead of reporting a clean one. + gate_integrity --repo . --git-base "$base" --git-head "$head" --base-ref "$ref" lint: name: lint @@ -435,6 +457,13 @@ jobs: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 with: fetch-depth: 0 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + # #640: the gate is policy, so its code comes from the base branch and + # never from the pull request it judges. `path: base` keeps it beside + # the checkout that supplies the data under test. + ref: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha || github.event.repository.default_branch }} + path: base - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: 22 @@ -443,7 +472,7 @@ jobs: # The choice lives in scripts/lib/risk.mjs (chooseBase) so that # merge_group measures against its base sha instead of HEAD~1, and so # test/riskBase.test.ts covers it. The step fetches what it needs. - run: echo "ref=$(node scripts/risk-gate.mjs --print-base)" >> "$GITHUB_OUTPUT" + run: echo "ref=$(node base/scripts/risk-gate.mjs --print-base)" >> "$GITHUB_OUTPUT" # The branch and the title are the mechanical-release key (#80): only a PR # whose head is release/vX.Y.Z and whose title is exactly "release: vX.Y.Z" # is classified as the mechanical class. A PR that merely touches the same @@ -453,7 +482,7 @@ jobs: PR_BRANCH: ${{ github.event.pull_request.head.ref }} PR_TITLE: ${{ github.event.pull_request.title }} BASE_REF: ${{ steps.base.outputs.ref }} - run: node scripts/risk-gate.mjs --base "$BASE_REF" --branch "$PR_BRANCH" --title "$PR_TITLE" + run: node base/scripts/risk-gate.mjs --repo . --base "$BASE_REF" --branch "$PR_BRANCH" --title "$PR_TITLE" # The drill imports `dist/db.js` and the checked-out `scripts/` only: no file # under `node_modules` is loaded at runtime, so a second `npm ci` and @@ -495,12 +524,19 @@ jobs: contents: read steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + # #640: the gate is policy, so its code comes from the base branch and + # never from the pull request it judges. `path: base` keeps it beside + # the checkout that supplies the data under test. + ref: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha || github.event.repository.default_branch }} + path: base - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: 22 # No `npm ci`: the checker is plain node and reads only the workflow files, # so a floating tag fails before any dependency install can run. - - run: node scripts/action-pins.mjs + - run: node base/scripts/action-pins.mjs --workflows .github/workflows # actionlint reads the YAML, the expressions and the shell syntax. It has no # taint analysis, so `${{ github.event.pull_request.title }}` inside `run:` is @@ -516,13 +552,20 @@ jobs: contents: read steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + # #640: the gate is policy, so its code comes from the base branch and + # never from the pull request it judges. `path: base` keeps it beside + # the checkout that supplies the data under test. + ref: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha || github.event.repository.default_branch }} + path: base - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: 22 # Plain node over the workflow files, so it runs in seconds and before any # dependency install could be the thing that fails. - name: Reject a workflow expression inside a shell script - run: node scripts/workflow-injection.mjs + run: node base/scripts/workflow-injection.mjs --workflows .github/workflows --allowlist base/.github/agent/workflow-injection-allowlist.json - name: Lint the workflows uses: raven-actions/actionlint@3d39aea434753780c3b3d4a1a31c854b4dbf49d7 # v2.2.0 with: @@ -622,6 +665,12 @@ jobs: actions: read steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + # #640: the gate is policy, so its code comes from the base branch and + # never from the pull request it judges. `path: base` keeps it beside + # the checkout that supplies the data under test. + ref: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha || github.event.repository.default_branch }} + path: base - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: 22 @@ -636,7 +685,7 @@ jobs: GITHUB_REPOSITORY: ${{ github.repository }} # No `npm ci`: the script is plain node and reads only the API and the # event payload, so a dependency install cannot be the thing that fails. - run: node scripts/codeql-required.mjs + run: node base/scripts/codeql-required.mjs # #644: a deploy-only diff is not code, so the app gates skip it, but `deploy/` # ships executable and syntax-sensitive files. `changes` sets `deploy` when any @@ -721,9 +770,23 @@ jobs: contents: read steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + # #640: the gate is policy, so its code comes from the base branch and + # never from the pull request it judges. `path: base` keeps it beside + # the checkout that supplies the data under test. + ref: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha || github.event.repository.default_branch }} + path: base - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: 22 + # #640: a reviewer has to be able to see in the log which copy of the gate + # judged the pull request. This names the base tree's blob for the + # evaluator the next step runs, so a tampered copy on the branch cannot be + # mistaken for it — the sha printed here is the one the verdict came from. + - name: Name the evaluator that comes from the base tree + run: | + set -euo pipefail + echo "scripts/ci-ok.mjs from the base tree: blob $(git -C base rev-parse HEAD:scripts/ci-ok.mjs)" # `skipped` is a failure here unless the job is named in SKIPPABLE_ON for # this event, and a required job missing from `needs` fails too. - name: Every required job passed @@ -731,7 +794,7 @@ jobs: NEEDS: ${{ toJSON(needs) }} CHANGES: ${{ toJSON(needs.changes.outputs) }} GITHUB_EVENT_NAME: ${{ github.event_name }} - run: node scripts/ci-ok.mjs + run: node base/scripts/ci-ok.mjs # #305: a red `main` used to be invisible. `ci-ok` computed the commit's # health and stopped, no job held a write scope on issues, and the branch was diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index fe1ba75e..cb0124e2 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -50,10 +50,17 @@ jobs: # analyze exits 0 whether it found a sink or nothing at all, so the # verdict is this step: fail on a high/critical result the recorded # baseline does not already hold. It runs on a push to main too. + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + # #640: the gate is policy, so its code comes from the base branch and + # never from the pull request it judges. `path: base` keeps it beside + # the checkout that supplies the data under test. + ref: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha || github.event.repository.default_branch }} + path: base - name: Fail on a new high or critical CodeQL result env: # #307: the advisory reads this run's review threads, and a step inherits # no token — `pull-requests: read` grants the permission, never the # credential. Scoped to this step, like the other credentials here. GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: node scripts/codeql-gate.mjs --sarif codeql-results --baseline .github/agent/codeql-baseline.json + run: node base/scripts/codeql-gate.mjs --sarif codeql-results --baseline base/.github/agent/codeql-baseline.json diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index 0156d069..10fb6ada 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -341,6 +341,51 @@ CodeQL never starts for an ignored path. The short circuit for a diff that touches only paths `codeql.yml` ignores is separate and is described under [Supply-chain gates](#supply-chain-gates). +### Gate integrity: the evaluator runs from the base branch + +A pull request decides nothing about itself. Every evaluator is checked out from +the base branch into `base/` and executed from there: + +| Job | Runs | +|---|---| +| `changes` | `node base/scripts/ci-changes.mjs --repo . --git-base --git-head ` | +| `risk` | `node base/scripts/risk-gate.mjs --repo . --base ` | +| `action-pins` | `node base/scripts/action-pins.mjs --workflows .github/workflows` | +| `workflow-lint` | `node base/scripts/workflow-injection.mjs --workflows .github/workflows --allowlist base/.github/agent/workflow-injection-allowlist.json` | +| `codeql-required` | `node base/scripts/codeql-required.mjs` | +| `ci-ok` | `node base/scripts/ci-ok.mjs` | +| CodeQL gate | `node base/scripts/codeql-gate.mjs --sarif codeql-results --baseline base/.github/agent/codeql-baseline.json` | + +Only the data under test comes from the head: the diff (`--repo .` names the +checkout that carries the pull request's commits), the workflow files the two +supply-chain checks read, and the SARIF the analysis produced. The CodeQL +baseline and the workflow-injection allowlist are read from `base/`, because a +pull request that could widen either would grade its own work. + +`ci-ok` also prints the blob the base tree holds for the evaluator it is about to +run: `scripts/ci-ok.mjs from the base tree: blob `. That line is how a +reviewer tells the two copies apart in a run log — the sha named there is the +file the next step executes — so a branch that rewrites `scripts/ci-ok.mjs` is +visible as a mismatch instead of an invisible substitution. It is evidence, not +a gate: the verdict still comes from the base file. + +The `changes` job also runs `scripts/gate-integrity.mjs`, which writes every +gate file in the diff into the job summary. It fails closed when the diff cannot +be read: a summary that says "no gate file" over an unreadable diff would be a +blind gate, not a clean one. The step runs it only when the base tree has the +file (`[ -f base/scripts/gate-integrity.mjs ]`), because the pull request that +introduces the summary runs against a base branch that predates it; the test is +on the base path, so a diff cannot decide whether the summary runs. Nothing +about the merge requirement changes — a diff that edits the gate is still T3 and +still needs a PASS at the tip. + +What this does not move: for a `pull_request` event GitHub fetches the workflow +YAML itself from the pull request, so a job's own `run:` lines are, by +construction, the head's text. The protection for that shape is the required +check plus the T3 PASS on a diff the `gate-integrity` summary names, not this +checkout split. Moving the aggregate onto a `workflow_run` workflow, which always +executes the default branch's copy, is a separate slice. + ### A red `main` reports itself `ci-ok` still only computes the commit's health; the job that says so out loud is diff --git a/scripts/action-pins.mjs b/scripts/action-pins.mjs index b6764a34..0e8532da 100644 --- a/scripts/action-pins.mjs +++ b/scripts/action-pins.mjs @@ -3,7 +3,16 @@ // job on any action reference that is not a 40-hex pin with its tag comment. import { scanActorGates, scanWorkflows } from './lib/action-pins.mjs' -const directory = '.github/workflows' +// #640: the checker runs from the base branch, but the workflows it reads are +// the pull request's own — they are the data under test, so the directory is an +// argument. +function argument(name, fallback) { + const index = process.argv.indexOf(name) + if (index === -1) return fallback + return process.argv[index + 1] ?? fallback +} + +const directory = argument('--workflows', '.github/workflows') const report = scanWorkflows(directory) for (const finding of report.findings) { diff --git a/scripts/ci-changes.mjs b/scripts/ci-changes.mjs index dce24a94..81150fb1 100644 --- a/scripts/ci-changes.mjs +++ b/scripts/ci-changes.mjs @@ -31,8 +31,12 @@ function fail(message) { // once by the base branch ref before that conservative answer is allowed — with // its history, because a depth-limited fetch places the commit but grafts it as // a shallow root, so the three-dot diff fails again, now with `no merge base`. +// #640: the classifier runs from the base branch tree, so the checkout that +// holds the pull request's commits is named instead of assumed to be the cwd. +const repo = argument('--repo') + function git(args) { - const result = spawnSync('git', args, { encoding: 'utf8' }) + const result = spawnSync('git', repo === '' ? args : ['-C', repo, ...args], { encoding: 'utf8' }) return { ok: result.status === 0, stdout: result.stdout ?? '', stderr: result.stderr ?? '' } } diff --git a/scripts/ci-ok.mjs b/scripts/ci-ok.mjs index 78095383..4d235cd3 100644 --- a/scripts/ci-ok.mjs +++ b/scripts/ci-ok.mjs @@ -1,160 +1,8 @@ #!/usr/bin/env node -// The aggregate required check. `ci-ok` passes only when every required job is -// present in `needs` and reports `success`. -// -// Why this is a script rather than the inline `node -e` it replaces: that one -// accepted `skipped` as a pass (`result !== "success" && result !== "skipped"`), -// so a required job that GitHub never ran — a false condition, an outage, a -// filtered matrix leg — reported green, and a job that had been deleted from -// `needs` was simply absent from the object and never checked at all. -// -// NEEDS='{"lint":{"result":"success"}}' node scripts/ci-ok.mjs -// node scripts/ci-ok.mjs --needs needs.json -// -// `--needs` exists so a test can feed a fake `needs` object without a runner. - -import { readFileSync } from 'node:fs' -import { pathToFileURL } from 'node:url' - -// Every job the aggregate depends on. `test/ciOk.test.ts` compares this list -// with the `needs:` list in ci.yml, so a job added to one side and not the other -// is caught before it can silently disappear from the gate. -export const REQUIRED_JOBS = [ - // The change classifier is part of the gate: a skipped heavy job is only - // acceptable when this job succeeded and said the category does not apply. - 'changes', - 'lint', - 'typecheck', - 'test', - 'coverage', - 'build', - 'visual', - 'docker', - 'audit', - 'changelog', - 'version', - 'risk', - 'restore-drill', - 'dependency-review', - 'action-pins', - 'workflow-lint', - 'secret-scan', - // #644: deploy/ is not code, but it ships the worker, the alert rules and the - // nginx/systemd samples, so a deploy-only diff gets its own syntax gate - // instead of being waved through on review alone. - 'deploy-check', - // #304: the CodeQL analysis conclusion, read by `scripts/codeql-required.mjs`. - // It is deliberately absent from SKIPPABLE_ON: a gate skipped on the event that - // merges would be vacuous. The change-aware exception is narrower: a docs-only - // or workflow-only pull request cannot add a JS/TS finding, and the CodeQL - // workflow filters those paths itself. On `schedule` the job itself exits 0 - // with its reason. - 'codeql-required', -] - -// A required job that cannot run on every event, with the events on which -// `skipped` is an acceptable result. On every other event it must be `success`; -// a failure, a cancellation or an absence from `needs` fails on all events. -// `dependency-review` reads the pull request's diff, so it has nothing to -// compare on a push to main, a nightly run or a manual run; the pull request run -// is the gate that counts. -export const SKIPPABLE_ON = { - 'dependency-review': ['push', 'merge_group', 'schedule', 'workflow_dispatch'], -} - -// A job that is conditional on a change category. `skipped` is a pass only when -// the category is present and false; a missing category is a failure, so a -// broken or absent classifier cannot turn the gate off. -export const SKIP_WHEN = { - // `test` and `coverage` are deliberately absent: the suite reads `docs/`, - // `public/`, `deploy/` and `.github/`, so it runs on every diff and a skipped - // one is red here whatever the classifier says. - lint: 'source', - typecheck: 'source', - build: 'source', - visual: 'frontend', - docker: 'docker', - audit: 'deps', - 'restore-drill': 'db', - 'action-pins': 'workflows', - 'workflow-lint': 'workflows', - 'codeql-required': 'codeql', - 'deploy-check': 'deploy', -} - -function flagIsFalse(changes, name) { - if (changes === null || typeof changes !== 'object') return false - const value = changes[name] - return value === false || value === 'false' -} - -function resultOf(value) { - if (value === null || typeof value !== 'object') return 'missing' - return typeof value.result === 'string' && value.result !== '' ? value.result : 'missing' -} - -export function evaluate(needs, options = {}) { - const required = options.required ?? REQUIRED_JOBS - const skippable = options.skippable ?? SKIPPABLE_ON - const eventName = options.eventName ?? '' - const changes = options.changes ?? {} - const lines = [] - const problems = [] - for (const [name, value] of Object.entries(needs)) { - const result = resultOf(value) - const requiredHere = required.includes(name) - lines.push(`${name}: ${result}${requiredHere ? '' : ' (not required)'}`) - } - for (const name of required) { - if (!Object.hasOwn(needs, name)) { - problems.push(`required job missing from needs: ${name}`) - continue - } - const result = resultOf(needs[name]) - if (result === 'success') continue - const skippableHere = (skippable[name] ?? []).includes(eventName) - if (result === 'skipped' && skippableHere) continue - const condition = (options.skipWhen ?? SKIP_WHEN)[name] - const conditionalEvent = eventName === 'pull_request' || eventName === 'merge_group' - if (result === 'skipped' && conditionalEvent && condition !== undefined && flagIsFalse(changes, condition)) continue - problems.push(`required job did not succeed: ${name} (${result})`) - } - return { lines, problems } -} - -function readNeeds() { - const index = process.argv.indexOf('--needs') - const file = index === -1 ? undefined : process.argv[index + 1] - const raw = file === undefined ? (process.env.NEEDS ?? '') : readFileSync(file, 'utf8') - if (raw.trim() === '') throw new Error('no needs object: set NEEDS or pass --needs ') - const parsed = JSON.parse(raw) - if (parsed === null || typeof parsed !== 'object' || Array.isArray(parsed)) throw new Error('needs must be a JSON object') - return parsed -} - -export function main() { - let needs - try { - needs = readNeeds() - } catch (error) { - console.error(`ci-ok: cannot read the needs object: ${error.message}`) - process.exit(1) - } - const eventName = process.env.GITHUB_EVENT_NAME ?? '' - let changes = {} - try { - changes = JSON.parse(process.env.CHANGES ?? '{}') - } catch { - changes = {} - } - const { lines, problems } = evaluate(needs, { eventName, changes }) - for (const line of lines) console.log(line) - if (problems.length > 0) { - for (const problem of problems) console.error(problem) - console.error(`ci-ok: ${String(problems.length)} required job(s) did not pass`) - process.exit(1) - } - console.log(`ci-ok: every required job passed (${String(REQUIRED_JOBS.length)} required)`) -} - -if (process.argv[1] !== undefined && import.meta.url === pathToFileURL(process.argv[1]).href) main() +// PROBE (#640): a throwaway mechanism drill, not for merge. +// This copy exits 0 no matter what the other jobs reported, so a pull request +// that read its own copy of the gate would be green here. ci.yml checks the +// base branch out into `base/` and runs `node base/scripts/ci-ok.mjs`, so this +// line must never execute in CI; the run log must name the base blob instead. +console.log('PROBE: the branch copy of scripts/ci-ok.mjs ran — the base-tree guard failed') +process.exit(0) diff --git a/scripts/gate-integrity.mjs b/scripts/gate-integrity.mjs new file mode 100644 index 00000000..d06b3a98 --- /dev/null +++ b/scripts/gate-integrity.mjs @@ -0,0 +1,100 @@ +#!/usr/bin/env node +// #640: name every gate file a pull request changes, in the job summary. +// +// The evaluators (`ci-ok`, `changes`, `risk`, `action-pins`, `workflow-lint`, +// `codeql-required`, `codeql-gate`) run from the base branch, so the pull +// request cannot edit the code that judges it. That is why this summary exists: +// the reviewer must still see that the gate itself is in the diff. +// +// node scripts/gate-integrity.mjs --repo . --git-base --git-head +// node scripts/gate-integrity.mjs --all (push, schedule, manual run) +// +// An unreadable diff fails closed: a summary that says "no gate file" while the +// diff could not be read would be a blind gate, not a clean one. + +import { appendFileSync } from 'node:fs' +import { spawnSync } from 'node:child_process' + +import { changedPathsForPullRequest } from './lib/ciChanges.mjs' +import { classifyGateFiles, gateSummary } from './lib/gate-integrity.mjs' + +function argument(name, fallback = '') { + const index = process.argv.indexOf(name) + if (index === -1) return fallback + return process.argv[index + 1] ?? fallback +} + +const repo = argument('--repo', '.') + +function git(args) { + const result = spawnSync('git', ['-C', repo, ...args], { encoding: 'utf8' }) + return { ok: result.status === 0, stdout: result.stdout ?? '', stderr: result.stderr ?? '' } +} + +function unshallow() { + if (git(['rev-parse', '--is-shallow-repository']).stdout.trim() !== 'true') return false + return git(['fetch', '--no-tags', '--unshallow', 'origin']).ok +} + +function pathsFor(base, head, baseRef) { + return changedPathsForPullRequest({ + base, + head, + baseRef, + git: { + diff: (from, to) => { + const result = git(['diff', '--name-only', '--no-renames', `${from}...${to}`]) + if (!result.ok) return null + return result.stdout + .split('\n') + .map((line) => line.trim()) + .filter((line) => line !== '') + }, + hasCommit: (sha) => git(['cat-file', '-e', `${sha}^{commit}`]).ok, + fetchSha: (sha) => { + if (!git(['fetch', '--no-tags', 'origin', sha]).ok) return false + return git(['cat-file', '-e', `${sha}^{commit}`]).ok + }, + deepen: unshallow, + fetchRef: (ref) => { + if (!git(['fetch', '--no-tags', 'origin', ref]).ok) return false + unshallow() + return true + }, + }, + }) +} + +function writeSummary(markdown) { + const output = process.env.GITHUB_STEP_SUMMARY ?? '' + if (output !== '') appendFileSync(output, markdown) + process.stdout.write(markdown) +} + +if (process.argv.includes('--all')) { + writeSummary(gateSummary([], { changed: 0 })) + console.log('gate-integrity: ok — every path is in scope on this event') + process.exit(0) +} + +const base = argument('--git-base') +const head = argument('--git-head') +const baseRef = argument('--base-ref') +if (base === '' || head === '') { + console.error('gate-integrity: usage: --git-base --git-head [--base-ref ] | --all') + process.exit(2) +} + +const paths = pathsFor(base, head, baseRef) +if (paths === null) { + console.error(`gate-integrity: cannot read the diff ${base}...${head}; failing closed`) + process.exit(2) +} + +const files = classifyGateFiles(paths) +writeSummary(gateSummary(files, { changed: paths.length, base })) +console.log( + files.length === 0 + ? `gate-integrity: ok — no gate file in ${paths.length} changed path(s)` + : `gate-integrity: ok — ${files.length} gate file(s) in ${paths.length} changed path(s): ${files.join(', ')}`, +) diff --git a/scripts/lib/gate-integrity.d.mts b/scripts/lib/gate-integrity.d.mts new file mode 100644 index 00000000..141df452 --- /dev/null +++ b/scripts/lib/gate-integrity.d.mts @@ -0,0 +1,19 @@ +// Types for the gate-integrity classifier, so the TypeScript suite can call the +// real implementation instead of restating the path rules. +export interface GateFileRule { + prefix: string + kind: string +} + +export interface GateSummaryOptions { + changed?: number + base?: string +} + +export declare const GATE_FILE_RULES: GateFileRule[] + +export declare function isGateFile(path: string): boolean + +export declare function classifyGateFiles(paths: readonly string[] | string[] | null | undefined): string[] + +export declare function gateSummary(files: readonly string[] | string[], options?: GateSummaryOptions): string diff --git a/scripts/lib/gate-integrity.mjs b/scripts/lib/gate-integrity.mjs new file mode 100644 index 00000000..e87446d8 --- /dev/null +++ b/scripts/lib/gate-integrity.mjs @@ -0,0 +1,55 @@ +// #640: which files in a diff are the gate itself. +// +// The evaluators run from the base branch, so a pull request cannot change the +// code that judges it. The reviewer still has to see that the gate is in the +// diff — a T3 change to `scripts/**` or `.github/**` is where a self-serving +// edit would hide — so the changed paths are classified here and one summary +// names every gate file the pull request touches. + +export const GATE_FILE_RULES = [ + { prefix: '.github/workflows/', kind: 'workflow' }, + { prefix: '.github/agent/', kind: 'gate policy or baseline' }, + { prefix: '.github/scripts/', kind: 'gate script' }, + { prefix: 'scripts/', kind: 'gate script' }, +] + +export function isGateFile(path) { + const value = String(path ?? '').trim() + if (value === '') return false + return GATE_FILE_RULES.some((rule) => value.startsWith(rule.prefix)) +} + +// Sorted and de-duplicated: the diff of a rename lists the path twice and the +// summary must not read as two different files. +export function classifyGateFiles(paths) { + const seen = new Set() + for (const path of paths ?? []) { + const value = String(path ?? '').trim() + if (isGateFile(value)) seen.add(value) + } + return [...seen].sort() +} + +export function gateSummary(files, { changed = 0, base = '' } = {}) { + const list = Array.isArray(files) ? files : [] + const where = base === '' ? '' : ` against \`${base}\`` + if (list.length === 0) { + return [ + '### gate-integrity', + '', + `No gate file is in this diff${where}${changed > 0 ? ` (${changed} path(s) read)` : ''}.`, + 'The evaluators ran from the base branch tree; the gate is not part of what this pull request changes.', + '', + ].join('\n') + } + return [ + '### gate-integrity', + '', + `This diff changes ${list.length} gate file(s)${where}${changed > 0 ? ` of ${changed} path(s) read` : ''} — the gate that judges this pull request is inside the pull request:`, + '', + ...list.map((file) => `- \`${file}\``), + '', + 'Every evaluator ran from the base branch tree, so these edits cannot loosen their own verdict; a T3 PASS at this tip is still required.', + '', + ].join('\n') +} diff --git a/scripts/risk-gate.mjs b/scripts/risk-gate.mjs index 90de1394..9aefd7fc 100644 --- a/scripts/risk-gate.mjs +++ b/scripts/risk-gate.mjs @@ -53,8 +53,12 @@ const json = process.argv.includes('--json') const branch = arg('--branch', '') const title = arg('--title', '') +// #640: the tier is policy, so the rules come from the base branch tree while +// the diff is read from the checkout named by --repo. +const repo = arg('--repo', '') + function git(args) { - return execFileSync('git', args, { encoding: 'utf8', maxBuffer: 64 * 1024 * 1024 }) + return execFileSync('git', repo === '' ? args : ['-C', repo, ...args], { encoding: 'utf8', maxBuffer: 64 * 1024 * 1024 }) } let files = [] diff --git a/scripts/workflow-injection.mjs b/scripts/workflow-injection.mjs index 685c1adb..264aa33b 100644 --- a/scripts/workflow-injection.mjs +++ b/scripts/workflow-injection.mjs @@ -5,11 +5,21 @@ // be read (fail closed). import { readAllowlist, scanWorkflows } from './lib/workflow-injection.mjs' -const directory = '.github/workflows' +// #640: the checker comes from the base branch, the workflows and the allowlist +// paths are arguments: the workflows are the data under test, and the allowlist +// must be the base copy so a pull request cannot allowlist its own injection. +function argument(name, fallback) { + const index = process.argv.indexOf(name) + if (index === -1) return fallback + return process.argv[index + 1] ?? fallback +} + +const directory = argument('--workflows', '.github/workflows') +const allowlistPath = argument('--allowlist', '') let allowlist try { - allowlist = readAllowlist() + allowlist = allowlistPath === '' ? readAllowlist() : readAllowlist(allowlistPath) } catch (error) { console.error(`workflow-injection: cannot read the allowlist: ${error.message}`) process.exit(1) diff --git a/src/probe-base-tree.ts b/src/probe-base-tree.ts new file mode 100644 index 00000000..55e3f8f7 --- /dev/null +++ b/src/probe-base-tree.ts @@ -0,0 +1,5 @@ +// PROBE (#640): a throwaway mechanism drill, not for merge. This file exists +// only to make `typecheck` red on the demonstration branch: the branch's own +// scripts/ci-ok.mjs exits 0, so a gate that read the branch's copy would report +// `ci-ok` green. The base-tree evaluator must still fail this branch. +export const probeBaseTree: number = 'not a number' diff --git a/test/ciChanges.test.ts b/test/ciChanges.test.ts index f65fac0a..f591c409 100644 --- a/test/ciChanges.test.ts +++ b/test/ciChanges.test.ts @@ -344,8 +344,13 @@ test('a base that is present but grafted is deepened before the diff is called u test('the classify step delegates the diff and keeps the every-gate fallback for other events', () => { const workflow = readFileSync(fileURLToPath(new URL('../.github/workflows/ci.yml', import.meta.url)), 'utf8') const step = workflow.slice(workflow.indexOf('- name: Classify the changed paths'), workflow.indexOf('\n lint:')) - assert.match(step, /node scripts\/ci-changes\.mjs --git-base/, 'the classify step must delegate the diff so the fetch-before-fallback rule is testable') - assert.match(step, /\*\) node scripts\/ci-changes\.mjs --all; exit 0 ;;/, 'a push or schedule still verifies the whole tree') + // #640: the classifier is checked out from the base branch and told which + // checkout holds the pull request's commits; the diff itself is still delegated + // so the fetch-before-fallback rule stays testable. + assert.match(step, /node base\/scripts\/ci-changes\.mjs --repo \. --git-base/, 'the classify step must delegate the diff so the fetch-before-fallback rule is testable') + // #640: the gate-integrity helper tests the base tree for the file, so the pull + // request that introduces it still classifies; a push or schedule runs both. + assert.match(step, /\*\) node base\/scripts\/ci-changes\.mjs --all; gate_integrity --all; exit 0 ;;/, 'a push or schedule still verifies the whole tree') assert.doesNotMatch(step, /git diff/, 'the untestable shell diff is what fell through to --all; it must not come back') }) diff --git a/test/ciOk.test.ts b/test/ciOk.test.ts index ad42e72a..8f2fa5d0 100644 --- a/test/ciOk.test.ts +++ b/test/ciOk.test.ts @@ -189,7 +189,9 @@ test('every conditional job is wired to the change classifier', async () => { test('the workflow runs the script and no longer accepts a skipped job inline', () => { const workflow = readFileSync(workflowPath, 'utf8') - assert.match(workflow, /node scripts\/ci-ok\.mjs/, 'ci-ok must call scripts/ci-ok.mjs') + // #640: the aggregate runs from the base branch tree, never from the pull + // request it judges. + assert.match(workflow, /node base\/scripts\/ci-ok\.mjs/, 'ci-ok must call the base branch copy of scripts/ci-ok.mjs') assert.doesNotMatch(workflow, /value\.result !== "skipped"/, 'the inline predicate that treated skipped as a pass must be gone') }) diff --git a/test/codeqlRequired.test.ts b/test/codeqlRequired.test.ts index a6c21b20..2f6ebc31 100644 --- a/test/codeqlRequired.test.ts +++ b/test/codeqlRequired.test.ts @@ -225,7 +225,9 @@ test('codeql-required is in ci-ok needs, is never skippable, and adds no merge_g const jobStart = ci.indexOf('\n codeql-required:') assert.ok(jobStart !== -1, 'ci.yml must declare the codeql-required job') const job = ci.slice(jobStart, ci.indexOf('\n ci-ok:', jobStart)) - assert.match(job, /run: node scripts\/codeql-required\.mjs/, 'the job must run the decision script') + // #640: the decision script runs from the base branch tree, so a pull request + // cannot edit the code that reads its own analysis. + assert.match(job, /run: node base\/scripts\/codeql-required\.mjs/, 'the job must run the base branch copy of the decision script') assert.match(job, /GITHUB_TOKEN: \$\{\{ secrets\.GITHUB_TOKEN \}\}/, 'a step inherits no token, so the lookup needs one mapped the way review-gate.yml maps it') assert.match(job, /actions: read/, 'reading this repository\'s workflow runs needs actions: read, and a job-level block replaces the workflow set') diff --git a/test/gateBaseTree.test.ts b/test/gateBaseTree.test.ts new file mode 100644 index 00000000..addf53ef --- /dev/null +++ b/test/gateBaseTree.test.ts @@ -0,0 +1,151 @@ +import test from 'node:test' +import assert from 'node:assert/strict' +import { spawnSync } from 'node:child_process' +import { cpSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { fileURLToPath } from 'node:url' + +// #640: the gate that decides whether a pull request may merge must not be +// executed from that pull request's own tree. The evaluator runs from the base +// branch; only the diff under test comes from the head. These tests pin both +// halves: the workflow shape (a base checkout plus a `base/scripts/...` call) +// and the behaviour that makes it matter (a stubbed head copy cannot decide). + +const ci = readFileSync(fileURLToPath(new URL('../.github/workflows/ci.yml', import.meta.url)), 'utf8') +const codeql = readFileSync(fileURLToPath(new URL('../.github/workflows/codeql.yml', import.meta.url)), 'utf8') +const scriptsDir = fileURLToPath(new URL('../scripts', import.meta.url)) + +function jobBlock(text: string, name: string): string { + const lines = text.split('\n') + const start = lines.findIndex((line) => line === ` ${name}:`) + assert.notEqual(start, -1, `ci.yml has no ${name} job`) + let end = lines.length + for (let index = start + 1; index < lines.length; index += 1) { + if (/^ [a-z][a-z0-9-]*:$/.test(lines[index]!)) { + end = index + break + } + } + return lines.slice(start, end).join('\n') +} + +const baseRef = 'github.event.pull_request.base.sha' + +const evaluators: Array<[string, string]> = [ + ['changes', 'base/scripts/ci-changes.mjs'], + ['risk', 'base/scripts/risk-gate.mjs'], + ['action-pins', 'base/scripts/action-pins.mjs'], + ['workflow-lint', 'base/scripts/workflow-injection.mjs'], + ['codeql-required', 'base/scripts/codeql-required.mjs'], + ['ci-ok', 'base/scripts/ci-ok.mjs'], +] + +for (const [job, script] of evaluators) { + test(`${job} evaluates from the base branch tree, not the pull request head`, () => { + const block = jobBlock(ci, job) + assert.ok(block.includes(script), `${job} must run ${script}`) + assert.ok(block.includes('path: base'), `${job} must check the base tree out into base/`) + assert.ok(block.includes(baseRef), `${job}'s base checkout must name the pull request base sha`) + assert.ok(!/node scripts\/(?:ci-ok|ci-changes|risk-gate|action-pins|workflow-injection|codeql-required)\.mjs/.test(block), `${job} must not execute the head copy of its evaluator`) + }) +} + +test('the CodeQL result gate and its baseline come from the base branch', () => { + assert.ok(codeql.includes('base/scripts/codeql-gate.mjs'), 'codeql-gate runs from the base tree') + assert.ok(codeql.includes('--baseline base/.github/agent/codeql-baseline.json'), 'the baseline is the base branch copy, not the pull request copy') +}) + +test('the change classifier reads the head diff while the classifier code comes from base', () => { + const block = jobBlock(ci, 'changes') + assert.ok(block.includes('--repo .'), 'the classifier must be told which checkout holds the head data') + assert.ok(block.includes('base/scripts/ci-changes.mjs'), 'the classifier itself comes from base') +}) + +test('the gate-integrity call is guarded against the base tree, so the pull request that introduces it still runs the job', () => { + const block = jobBlock(ci, 'changes') + assert.ok(block.includes('[ -f base/scripts/gate-integrity.mjs ]'), 'the guard must test the base copy of the evaluator') + assert.ok( + !block.includes('[ -f scripts/gate-integrity.mjs ]'), + 'a guard on the head copy would let the pull request decide whether the summary runs', + ) + for (const call of ['gate_integrity --all', 'gate_integrity --repo . --git-base']) { + assert.ok(block.includes(call), `the changes job must call ${call}`) + } + assert.ok(!block.includes('node base/scripts/gate-integrity.mjs --repo .'), 'the only invocation is the guarded helper') +}) + +function run(script: string, needsPath: string) { + return spawnSync(process.execPath, [script, '--needs', needsPath], { encoding: 'utf8' }) +} + +test('a pull request that stubs scripts/ci-ok.mjs to exit 0 cannot turn ci-ok green, because the base copy decides', () => { + const dir = mkdtempSync(join(tmpdir(), 'gate-base-')) + try { + cpSync(scriptsDir, join(dir, 'base', 'scripts'), { recursive: true }) + cpSync(scriptsDir, join(dir, 'head', 'scripts'), { recursive: true }) + writeFileSync(join(dir, 'head', 'scripts', 'ci-ok.mjs'), 'process.exit(0)\n') + const required = JSON.parse( + spawnSync(process.execPath, ['-e', `import(${JSON.stringify(join(dir, 'base', 'scripts', 'ci-ok.mjs'))}).then((m) => console.log(JSON.stringify(m.REQUIRED_JOBS)))`], { encoding: 'utf8' }).stdout, + ) as string[] + const needs: Record = {} + for (const name of required) needs[name] = { result: 'success' } + needs.lint = { result: 'failure' } + const needsPath = join(dir, 'needs.json') + writeFileSync(needsPath, JSON.stringify(needs)) + + const head = run(join(dir, 'head', 'scripts', 'ci-ok.mjs'), needsPath) + assert.equal(head.status, 0, 'the attack shape: the head copy of the gate exits 0 over a red suite') + const base = run(join(dir, 'base', 'scripts', 'ci-ok.mjs'), needsPath) + assert.equal(base.status, 1, 'the base copy still fails the broken job, so the head cannot decide') + assert.match(base.stderr, /lint/) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +test('the base classifier sees a source diff even when the head copy of the classifier is rewritten to call it docs only', () => { + const dir = mkdtempSync(join(tmpdir(), 'gate-base-')) + try { + const repo = join(dir, 'repo') + mkdirSync(repo, { recursive: true }) + const env = { + ...process.env, + GIT_AUTHOR_NAME: 'a', + GIT_AUTHOR_EMAIL: 'a@example.com', + GIT_COMMITTER_NAME: 'a', + GIT_COMMITTER_EMAIL: 'a@example.com', + } + const git = (args: string[]) => { + const result = spawnSync('git', args, { cwd: repo, encoding: 'utf8', env }) + assert.equal(result.status, 0, `git ${args.join(' ')}: ${result.stderr}`) + } + git(['init', '-q', '-b', 'main']) + mkdirSync(join(repo, 'docs'), { recursive: true }) + writeFileSync(join(repo, 'docs', 'readme.md'), 'base\n') + git(['add', '-A']) + git(['commit', '-qm', 'base']) + mkdirSync(join(repo, 'src'), { recursive: true }) + writeFileSync(join(repo, 'src', 'app.ts'), 'export const a = 1\n') + git(['add', '-A']) + git(['commit', '-qm', 'head']) + + cpSync(scriptsDir, join(dir, 'base', 'scripts'), { recursive: true }) + cpSync(scriptsDir, join(dir, 'head', 'scripts'), { recursive: true }) + writeFileSync( + join(dir, 'head', 'scripts', 'lib', 'ciChanges.mjs'), + 'export function classifyChanges() { return { docsOnly: true, source: false } }\nexport function allGateChanges() { return { docsOnly: true, source: false } }\nexport function changedPathsForPullRequest() { return null }\n', + ) + + const args = ['--repo', repo, '--git-base', 'HEAD~1', '--git-head', 'HEAD'] + const base = spawnSync(process.execPath, [join(dir, 'base', 'scripts', 'ci-changes.mjs'), ...args], { encoding: 'utf8' }) + assert.equal(base.status, 0, base.stderr) + assert.equal(JSON.parse(base.stdout).source, true, 'the base classifier reads the head diff and sees src/') + + const head = spawnSync(process.execPath, [join(dir, 'head', 'scripts', 'ci-changes.mjs'), ...args], { encoding: 'utf8' }) + assert.equal(head.status, 0, head.stderr) + assert.equal(JSON.parse(head.stdout).source, false, 'the attack shape: the rewritten head classifier hides the source diff') + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) diff --git a/test/gateIntegrity.test.ts b/test/gateIntegrity.test.ts new file mode 100644 index 00000000..1ac42f15 --- /dev/null +++ b/test/gateIntegrity.test.ts @@ -0,0 +1,104 @@ +import test from 'node:test' +import assert from 'node:assert/strict' +import { spawnSync } from 'node:child_process' +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { fileURLToPath } from 'node:url' + +import { classifyGateFiles, gateSummary } from '../scripts/lib/gate-integrity.mjs' + +// #640: the gate that decides a merge runs from the base branch, so a pull +// request that edits a gate cannot judge itself. The reviewer still has to see +// that the gate itself is in the diff — that is what this summary is for. + +const script = fileURLToPath(new URL('../scripts/gate-integrity.mjs', import.meta.url)) + +test('a docs or source only diff names no gate file', () => { + assert.deepEqual(classifyGateFiles(['docs/api.md', 'src/app.ts', 'test/ciOk.test.ts']), []) + assert.equal(gateSummary([]).includes('No gate file'), true) +}) + +test('every gate file in the diff is named once, sorted, whatever order it arrives in', () => { + const files = classifyGateFiles([ + 'scripts/ci-ok.mjs', + 'src/app.ts', + '.github/workflows/ci.yml', + 'scripts/lib/ciChanges.mjs', + '.github/agent/codeql-baseline.json', + 'scripts/ci-ok.mjs', + ]) + assert.deepEqual(files, [ + '.github/agent/codeql-baseline.json', + '.github/workflows/ci.yml', + 'scripts/ci-ok.mjs', + 'scripts/lib/ciChanges.mjs', + ]) + const summary = gateSummary(files) + for (const file of files) assert.ok(summary.includes(file), `the summary must name ${file}`) +}) + +function git(cwd: string, args: string[]): void { + const result = spawnSync('git', args, { + cwd, + encoding: 'utf8', + env: { + ...process.env, + GIT_AUTHOR_NAME: 'a', + GIT_AUTHOR_EMAIL: 'a@example.com', + GIT_COMMITTER_NAME: 'a', + GIT_COMMITTER_EMAIL: 'a@example.com', + }, + }) + assert.equal(result.status, 0, `git ${args.join(' ')} failed: ${result.stderr}`) +} + +function repoWithDiff(): string { + const dir = mkdtempSync(join(tmpdir(), 'gate-integrity-')) + git(dir, ['init', '-q', '-b', 'main']) + mkdirSync(join(dir, 'docs'), { recursive: true }) + writeFileSync(join(dir, 'docs', 'readme.md'), 'base\n') + git(dir, ['add', '-A']) + git(dir, ['commit', '-qm', 'base']) + mkdirSync(join(dir, 'src'), { recursive: true }) + mkdirSync(join(dir, 'scripts'), { recursive: true }) + writeFileSync(join(dir, 'src', 'app.ts'), 'export const a = 1\n') + writeFileSync(join(dir, 'scripts', 'ci-ok.mjs'), 'process.exit(0)\n') + git(dir, ['add', '-A']) + git(dir, ['commit', '-qm', 'head']) + return dir +} + +test('the summary names the gate file the pull request changes and stays green', () => { + const dir = repoWithDiff() + try { + const summaryFile = join(dir, 'summary.md') + const result = spawnSync( + process.execPath, + [script, '--repo', dir, '--git-base', 'HEAD~1', '--git-head', 'HEAD'], + { encoding: 'utf8', env: { ...process.env, GITHUB_STEP_SUMMARY: summaryFile } }, + ) + assert.equal(result.status, 0, result.stderr) + assert.match(result.stdout, /gate-integrity: ok — 1 gate file/) + const summary = readFileSync(summaryFile, 'utf8') + assert.ok(summary.includes('scripts/ci-ok.mjs'), 'the job summary must name the gate file in the diff') + assert.ok(summary.includes('2 path(s) read'), 'the summary must say how many paths the diff held') + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +test('an unreadable diff fails closed instead of reporting a clean diff', () => { + const dir = repoWithDiff() + try { + const result = spawnSync( + process.execPath, + [script, '--repo', dir, '--git-base', '0123456789abcdef0123456789abcdef01234567', '--git-head', 'HEAD'], + { encoding: 'utf8', env: { ...process.env, GITHUB_STEP_SUMMARY: '' } }, + ) + assert.notEqual(result.status, 0, 'a diff that cannot be read must not pass') + assert.match(result.stderr, /failing closed/) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) diff --git a/test/supplyChain.test.ts b/test/supplyChain.test.ts index 16fc322a..d97566da 100644 --- a/test/supplyChain.test.ts +++ b/test/supplyChain.test.ts @@ -144,7 +144,9 @@ test('the workflow lint job runs actionlint and the injection checker', () => { const block = jobBlock(ci, 'workflow-lint') assert.match(block, /uses: raven-actions\/actionlint@[0-9a-f]{40} # v\d/) assert.match(block, /version: '1\.\d+\.\d+'/, "the action's default is `latest`, which is an unpinned download behind a pinned action") - assert.match(block, /node scripts\/workflow-injection\.mjs/) + // #640: the checker comes from the base branch; the workflows and the + // allowlist are read from the paths it is given. + assert.match(block, /node base\/scripts\/workflow-injection\.mjs --workflows \.github\/workflows --allowlist base\/\.github\/agent\/workflow-injection-allowlist\.json/) }) test('an expression inside a run block is a finding', () => {