ci: the gate signature is a job of its own, so deleting it takes a context with it (Part of #2124)

.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" }
         ]
       }
     },

.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)

.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/],

.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', () => {

.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: <comment id or url>` 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 ]

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)).

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.

docs/governance.md

@@ -154,18 +154,25 @@ Policy-sign: <comment id or url>
 
 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.
 
 A signature signs one head, not one pull request: every new tip needs a new comment and a new line.
 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