From 8d6627489c3cf242a8367f5529a30c121c1efee2 Mon Sep 17 00:00:00 2001 From: Dev-4 Date: Tue, 3 Nov 2026 14:42:41 +0000 Subject: [PATCH] fix(ci): give the gate signature its own job so deleting it leaves a context missing (#2124) --- .github/agent/merge-gate-ruleset.json | 3 +- .github/scripts/gate-ruleset.mjs | 4 +- .github/scripts/gate-ruleset.test.mjs | 9 ++++ .github/scripts/test-workflow.test.mjs | 61 +++++++++++++++++++++-- .github/workflows/test.yml | 47 ++++++++++------- changelog.d/2124-policy-sign-job.fixed.md | 1 + docs/autonomous-dev.md | 7 +-- docs/governance.md | 17 +++++-- 8 files changed, 116 insertions(+), 33 deletions(-) create mode 100644 changelog.d/2124-policy-sign-job.fixed.md diff --git a/.github/agent/merge-gate-ruleset.json b/.github/agent/merge-gate-ruleset.json index d9befc3f0..bd0acb78f 100644 --- a/.github/agent/merge-gate-ruleset.json +++ b/.github/agent/merge-gate-ruleset.json @@ -18,7 +18,8 @@ "strict_required_status_checks_policy": false, "required_status_checks": [ { "context": "tests-ok" }, - { "context": "enqueue-guard" } + { "context": "enqueue-guard" }, + { "context": "policy-sign" } ] } }, diff --git a/.github/scripts/gate-ruleset.mjs b/.github/scripts/gate-ruleset.mjs index 8b9031107..1d245beee 100644 --- a/.github/scripts/gate-ruleset.mjs +++ b/.github/scripts/gate-ruleset.mjs @@ -3,7 +3,7 @@ // The check run is only half of #812: `enqueue-guard.yml` writes it, and a ruleset on `main` has to // require it, or the refusal is a red badge a merge endpoint never reads. `.github/agent/ // merge-gate-ruleset.json` is that ruleset verbatim — branch ruleset, `enforcement: active`, -// `bypass_actors: []`, `refs/heads/main`, `tests-ok` and `enqueue-guard` required, +// `bypass_actors: []`, `refs/heads/main`, `tests-ok`, `enqueue-guard` and `policy-sign` required, // `strict_required_status_checks_policy: false` (main moves on every merge, and a strict rule would make // every open pull request rebase before it could merge). // @@ -27,7 +27,7 @@ import { fileURLToPath } from 'node:url' export const BODY_PATH = fileURLToPath(new URL('../agent/merge-gate-ruleset.json', import.meta.url)) export const RULESET_NAME = 'main merge gate' export const BASE_REF = 'refs/heads/main' -export const REQUIRED_CONTEXTS = ['tests-ok', 'enqueue-guard'] +export const REQUIRED_CONTEXTS = ['tests-ok', 'enqueue-guard', 'policy-sign'] function arg(name) { const i = process.argv.indexOf(name) diff --git a/.github/scripts/gate-ruleset.test.mjs b/.github/scripts/gate-ruleset.test.mjs index 26de36d1b..306da49ba 100644 --- a/.github/scripts/gate-ruleset.test.mjs +++ b/.github/scripts/gate-ruleset.test.mjs @@ -88,6 +88,15 @@ test('#812: a correctly built ruleset has no problems', () => { assert.deepEqual(s.bypass, []) }) +test('#2124: the gate requires the signature job as a context of its own', () => { + // The signature used to be a step inside `test`, and a required context is a job name: deleting + // the step left the suite green and nothing was missing. Here it is a job, so it enters the + // ruleset the way the other two contexts do — and `--check` reads the live ruleset against it. + const s = summarize(goodRow()) + assert.ok(s.contexts.includes('policy-sign'), `the required checks are ${s.contexts.join(', ')}`) + assert.ok(REQUIRED_CONTEXTS.includes('policy-sign'), `gate-ruleset calls ${REQUIRED_CONTEXTS.join(', ')} required`) +}) + test('#812: every way the gate could be bypassed is a separate problem', () => { const cases = [ ['a bypass actor', (row) => { row.bypass_actors = [{ actor_id: 1, actor_type: 'RepositoryRole', bypass_mode: 'always' }] }, /bypass actors/], diff --git a/.github/scripts/test-workflow.test.mjs b/.github/scripts/test-workflow.test.mjs index b4c62cd79..84bfb22f8 100644 --- a/.github/scripts/test-workflow.test.mjs +++ b/.github/scripts/test-workflow.test.mjs @@ -22,6 +22,23 @@ const job = name => { return text.slice(start, next < 0 ? undefined : start + 1 + next) } +// #2124: a required context is a job id or a job name, never a step, so the signature has to be a +// job. These two read a workflow's job graph the way GitHub does: the ids at two-space indent, and +// the `needs:` of one job from either the flow form or the block form. +const jobIds = (source = text) => [...source.matchAll(/^ {2}([a-z][\w-]*):$/gm)].map((m) => m[1]) +const needsOf = (source, name) => { + const at = source.indexOf(`\n ${name}:\n`) + assert.ok(at >= 0, `no job ${name}`) + const end = source.slice(at + 1).search(/\n {2}[a-z][\w-]*:\n/) + const block = source.slice(at, end < 0 ? undefined : at + 1 + end) + const flow = /^ {4}needs: \[([^\]]*)\]$/m.exec(block) + if (flow) return flow[1].split(',').map((value) => value.trim()) + const one = /^ {4}needs: ([a-z][\w-]*)$/m.exec(block) + if (one) return [one[1]] + const list = /^ {4}needs:\n((?: {6}- \S+\n?)+)/m.exec(block) + return list ? [...list[1].matchAll(/- (\S+)/g)].map((m) => m[1]) : [] +} + test('triggers: pull_request, merge_group (no filters), push only on main, dispatch', () => { assert.match(triggers, /^ {2}pull_request:\s*$/m) assert.match(triggers, /^ {2}merge_group:\s*$/m, 'a merge queue needs merge_group or required checks never report on queue entries') @@ -77,12 +94,50 @@ test('#2099: a pull request that weakens its own copy of the checker is still re assert.notEqual(run.status, 0, `the step ran \`${script}\`: a pull request that weakened its own copy must still be refused`) }) -test('tests-ok: always runs and is green only when test, plugins and package are', () => { +// #2124: the required contexts are job names, so a step that is deleted leaves its job green and +// nothing notices. The signature is a job of its own, named `policy-sign`, and `test` needs it: a +// pull request that deletes the job leaves a required context unreported instead of passing. +test('#2124: the signature is its own job, named for the required context', () => { + const signature = job('policy-sign') + assert.match(signature, /^ {4}name: policy-sign$/m, 'the check name is the job name a required-context list can name') + assert.match(signature, /^\s+run: node base\/\.github\/scripts\/policy-sign\.mjs$/m, 'the job runs the base copy, not the pull request\'s own') + assert.match(signature, /^ {10}ref: \$\{\{ github\.event\.merge_group\.base_sha \|\| github\.event\.pull_request\.base\.sha \}\}$/m) + assert.match(signature, /^ {10}path: base$/m) + assert.match(signature, /^ {10}persist-credentials: false$/m) + + const runner = job('test') + assert.deepEqual(needsOf(text, 'test'), ['policy-sign'], 'the suite waits for the signature, so an unsigned change spends no suite time') + assert.ok(!runner.includes('Gate change signature'), 'the signature is not a step of `test`: a step carries no context of its own') + assert.ok(!runner.includes('path: base'), 'the base checkout belongs to the signature job, not to the suite') + for (const name of ['test', 'plugins', 'package']) assert.ok(jobIds().includes(name), `${name} still exists`) + + const ok = job('tests-ok') + assert.match(ok, /needs: \[policy-sign, test, plugins, package\]/, 'the required context also names the signature, so a failure is red there too') + assert.match(ok, /\[ "\$SIGN" = success \]/) +}) + +// The defect (#2124): delete the job and the context is missing, not green. The same edit that used +// to leave `test` reporting success now leaves `test` unable to run at all. +test('#2124: deleting the signature job leaves the required context unreported, not green', () => { + const ids = jobIds() + for (const id of ids) for (const need of needsOf(text, id)) assert.ok(ids.includes(need), `${id} needs ${need}, which exists`) + assert.ok(needsOf(text, 'tests-ok').includes('policy-sign')) + assert.ok(needsOf(text, 'test').includes('policy-sign')) + + const block = job('policy-sign') + const without = text.replace(block, '').replace(/\njobs:\n/, '\njobs:\n') + const left = jobIds(without) + assert.ok(!left.includes('policy-sign'), 'the job is gone from the doctored workflow') + const dangling = [...needsOf(without, 'test'), ...needsOf(without, 'tests-ok')].filter((need) => !left.includes(need)) + assert.deepEqual([...new Set(dangling)].sort(), ['policy-sign'], 'both the suite and the required context now name a job that no longer exists — the check cannot report success without it') +}) + +test('tests-ok: always runs and is green only when policy-sign, test, plugins and package are', () => { const ok = job('tests-ok') assert.match(ok, /^ {4}name: tests-ok$/m) assert.match(ok, /^ {4}if: always\(\)$/m) - assert.match(ok, /needs: \[test, plugins, package\]/) - assert.match(ok, /\[ "\$TEST" = success \] && \[ "\$PLUGINS" = success \] && \[ "\$PACKAGE" = success \]/) + assert.match(ok, /needs: \[policy-sign, test, plugins, package\]/) + assert.match(ok, /\[ "\$SIGN" = success \] && \[ "\$TEST" = success \] && \[ "\$PLUGINS" = success \] && \[ "\$PACKAGE" = success \]/) }) test('main keeps its run (no cancel), every other ref, queue branches included, cancels', () => { diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 169c1f44f..47f4dd80f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -25,39 +25,47 @@ permissions: contents: read jobs: - # The PR gate: one job (see #31). Its name is the required check on main: keep it exactly. - # Windows, Node 22 and the next DSH run in the Compatibility workflow (nightly.yml). - test: - name: ubuntu-latest · Node 24 · DSH 0.2.0-rc.2 + # #2124: the gate-change signature is a job of its own. Required contexts in this repository are + # job names (.github/agent/merge-gate-ruleset.json), so while it lived as a step of `test` a pull + # request that deleted the step left `test` green and nothing was missing. Deleting this job + # instead removes a context the merge path needs, and `test` cannot start because it needs it. + policy-sign: + name: policy-sign runs-on: ubuntu-latest - timeout-minutes: 25 - # `pull-requests: read` is for the gate-signature step (#2054): it reads the pull request's - # changed files, body and comments. The workflow-level `contents: read` alone would make that - # call a 403. + timeout-minutes: 5 permissions: contents: read pull-requests: read steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: + ref: ${{ github.event.merge_group.base_sha || github.event.pull_request.base.sha }} + path: base persist-credentials: false # policy/gates.json (#2054): a pull request that edits an acceptance gate carries # `Policy-sign: ` in its body, written by someone other than its author. # It runs before the suite: a gate change nobody else signed is not worth 20 minutes of tests. # #2099: the checker comes from the base commit, not from the pull request's own tree, so a # pull request that weakens or deletes `policy-sign.mjs` is still judged by the copy it started - # from. `merge-gate.yml` checks base out the same way. The `merge_group` half - # is required, not decoration: a queue run carries no `pull_request`, and an expression with - # only that half falls back to the sha that triggered the run, which is the pull request again. - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - ref: ${{ github.event.merge_group.base_sha || github.event.pull_request.base.sha }} - path: base - persist-credentials: false + # from. The `merge_group` half of `ref` is required, not decoration: a queue run carries no + # `pull_request`, and an expression with only that half falls back to the sha that triggered + # the run, which is the pull request again. `merge-gate.yml` checks base out the same way. - name: Gate change signature env: GH_TOKEN: ${{ github.token }} run: node base/.github/scripts/policy-sign.mjs + + # The PR gate: one job (see #31). Its name is the required check on main: keep it exactly. + # Windows, Node 22 and the next DSH run in the Compatibility workflow (nightly.yml). + test: + name: ubuntu-latest · Node 24 · DSH 0.2.0-rc.2 + needs: policy-sign + runs-on: ubuntu-latest + timeout-minutes: 25 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: ./.github/actions/flybot-tests with: node: '24' @@ -153,15 +161,16 @@ jobs: tests-ok: name: tests-ok if: always() - needs: [test, plugins, package] + needs: [policy-sign, test, plugins, package] runs-on: ubuntu-latest timeout-minutes: 5 steps: - name: Every job passed env: + SIGN: ${{ needs.policy-sign.result }} TEST: ${{ needs.test.result }} PLUGINS: ${{ needs.plugins.result }} PACKAGE: ${{ needs.package.result }} run: | - echo "test: $TEST, plugins: $PLUGINS, package: $PACKAGE" - [ "$TEST" = success ] && [ "$PLUGINS" = success ] && [ "$PACKAGE" = success ] + echo "policy-sign: $SIGN, test: $TEST, plugins: $PLUGINS, package: $PACKAGE" + [ "$SIGN" = success ] && [ "$TEST" = success ] && [ "$PLUGINS" = success ] && [ "$PACKAGE" = success ] diff --git a/changelog.d/2124-policy-sign-job.fixed.md b/changelog.d/2124-policy-sign-job.fixed.md new file mode 100644 index 000000000..61575a441 --- /dev/null +++ b/changelog.d/2124-policy-sign-job.fixed.md @@ -0,0 +1 @@ +The gate-change signature is a job of its own (`policy-sign`) instead of a step inside the `Test` suite, and `main`'s ruleset file names it next to `tests-ok` and `enqueue-guard`: required contexts are job names, so deleting the step used to leave the suite green, while the suite now needs the signature job and cannot run without it — a pull request that removes it loses a required context instead of passing. The live ruleset still has to be given the same context, which is the owner's edit: until then the job reports without being required ([#2124](https://github.com/ThinkFlowLab/FlyBot/issues/2124)). diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index d5c05d854..075d43e24 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -390,9 +390,10 @@ the PR onto the current `main`, plus the entries ahead of it) and merges only wh commit. Required checks are matched by name on that commit, so `Test` must run on `merge_group`; otherwise `tests-ok` and the `test` job never report there and the entry waits until the queue's timeout. With the trigger: -- `test`, `plugins (tree)`, `plugins (social)`, `package` and `tests-ok` run unchanged. Nothing in `test.yml` reads the - `pull_request` event, no job has an `if:` that would skip it, and `tests-ok` keeps `if: always()` and needs all three, - so a skipped or failed job still makes it red (`.github/scripts/test-workflow.test.mjs` checks all of this). +- `policy-sign`, `test`, `plugins (tree)`, `plugins (social)`, `package` and `tests-ok` run unchanged. Nothing in + `test.yml` reads the `pull_request` event, no job has an `if:` that would skip it, and `tests-ok` keeps + `if: always()` and needs all of them, so a skipped or failed job still makes it red + (`.github/scripts/test-workflow.test.mjs` checks all of this). - `merge_group` takes no path filters, so a docs-only entry runs in full. That is also why `paths-ignore` stays on `push` only. - `push` stays limited to `main`, so a queue branch does not run twice. The concurrency group is per ref, so entries do not cancel each other, and every non-`main` ref (queue branches included) cancels its own earlier run. diff --git a/docs/governance.md b/docs/governance.md index 17982bc2d..c7b88cea8 100644 --- a/docs/governance.md +++ b/docs/governance.md @@ -154,11 +154,12 @@ Policy-sign: The comment it names must be on that pull request, must have been written by someone other than the pull request's author, and must name the tip it signs — the head sha, in full or abbreviated to at -least twelve characters. A GitHub bot account cannot sign. `Test`'s `Gate change signature` step runs, -before the suite, the copy of the checker checked out from the pull request's **base** commit — not the -copy in the pull request's own tree — so a change that edits, weakens or deletes the checker is still -judged by the one it started from. A missing line, a comment that is not on the pull request, a -self-signature, or a comment written for another tip fails the pull request. +least twelve characters. A GitHub bot account cannot sign. `Test`'s `policy-sign` job runs, before the +suite, the copy of the checker checked out from the pull request's **base** commit — not the copy in +the pull request's own tree — so a change that edits, weakens or deletes the checker is still judged by +the one it started from. It is a job because a required context is a job name: the suite needs it, so +deleting the job leaves `Test` unable to run rather than green. A missing line, a comment that is not +on the pull request, a self-signature, or a comment written for another tip fails the pull request. Anyone else with comment access can sign, and editing the body is the whole ceremony: no new workflow, no threshold changes, no merge path. @@ -166,6 +167,12 @@ A signature signs one head, not one pull request: every new tip needs a new comm That is the price of binding, and it is the point — a signature that survived a push would let an author sign a reviewable change set and then push another one under it. +The job is a gate only where a ruleset requires it: `.github/agent/merge-gate-ruleset.json` now lists +`policy-sign` next to `tests-ok` and `enqueue-guard`, and `node .github/scripts/gate-ruleset.mjs +--check` reads the live ruleset against that file. The live ruleset is the owner's edit, and until it +is made the job only reports: it runs, `tests-ok` needs it, and the merge path still accepts a pull +request where it never reported. That half is #2124's own, not this pull request's. + The cost is accepted and is real: a gate change — including our own Bots' pull requests — waits for a second person to comment before it can merge, so the Lead of the round that owns the pull request asks for the signature and adds the line. A line written into the body after the check went green starts no