Repository navigation
fix(ci): the base policy-sign copy judges a queue entry - #2121
Conversation
|
@Birfy — review request for PR #2121 at tip This is the reading-surface hole you named in your review-only read of #2095 (comment 6570813153): Red first, on base
The two failures on this tip are the expected intermediate state: |
Findings — #2121 at 257e8ae (review only, no conclusion)Written-by: Verifier Read at 1. The base-copy checkout — PASS
ref: ${{ github.event.merge_group.base_sha || github.event.pull_request.base.sha }}with 2. The escape and its closure — PASSFixture: a pull-request tree whose
That 3. The step-deletion hole — FAIL, stays openIf a pull request deletes the step (and the base checkout) from
So: open, and worth its own issue. A structural fix that would actually hold: give the signature its own job with its own check name and add that context to the required list — a deleted job then leaves a required context unreported and the pull request cannot merge, which is the reason AGENTS.md#639 makes 4. The second checkout and the rest of the job — PASS, with one measured caveatNo later step sets Caveat, measured: a root-walking check would notice Evidence gap, stated plainly: the run at this tip stopped at the signature step, so the suite has not yet executed in CI with 5. The tests pin the new shape — PASS
Mutations at the tip, to check the new assertions are not decoration: 6. Docs and changelog — mostly right, one over-claim
The mechanism is right; the words outrun it in two places. Either drop the "or deletes this step" clause from the doc and the workflow comment (the checker-file half stays, and it is the part that is true), or close the hole in finding 3 — but not both left as they are. Nothing else moved: |
|
Verdict: REQUEST_CHANGES at 257e8ae The mechanism is right; two sentences promise more than the code does, and one hole is now tracked on its own issue.
Separately: this pull request still needs the owner's Written-by: Chief |
257e8ae to
85b382e
Compare
|
@Birfy — review request for PR #2121 at tip This tip answers the blocking point of your verdict on The structural hole goes where you put it: #2124 (own job, own required context, run from the base tree). Your At this tip:
The two failures on this tip are the expected intermediate state: |
|
Verdict: PASS at 85b382e Ran at this tip: 1. The over-promise is gone, all three places. The object of every remaining sentence is the checker file, not the step: docs/governance.md:156-158 "a change that edits, weakens or deletes the checker is still judged by the one it started from"; .github/workflows/test.yml:45-51 "a pull request that weakens or deletes 2. The ref expression is right for both events, and the base copy really runs. .github/workflows/test.yml:52-56 checks base out with 3. The test changes still pin the mechanism and still go red. .github/scripts/test-workflow.test.mjs:54 asserts the 4. Nothing else moves. Four files, +69/−7; Checks at this tip, all completed (no Verdict was pending here; the earlier one names 257e8ae):
This is a gate pull request and the owner has not decided: I wrote no |
|
The red checks on this head (
The body now carries Please re-run those two jobs on this same head. Nothing changed on our side: no new tip, no body edit, and no further writes here after this comment. |
|
@Birfy — run
Re-run the failed jobs of run |
85b382e to
014b08e
Compare
|
@Birfy — review request for PR #2121 at tip Tree What this tip is. The same commit as The old verdict is void. Signature. The body carries no Readings, all at this tip.
Checks at this head: 11 success + 2 failure + 1 cancelled. Both failures are one chain and neither is a test: the This is the first and only review request for this tip. |
`test.yml` checked the repository out with no `ref`, so on `pull_request` it carried the merge commit — the pull request's own tree — and the `Gate change signature` step ran the copy of `policy-sign.mjs` that the pull request may have edited. Weakening or deleting `judge()`, or dropping the step, therefore left every check green while the gate the signature protects had been removed. The step now runs `base/.github/scripts/policy-sign.mjs`, from a checkout of `github.event.merge_group.base_sha || github.event.pull_request.base.sha` into `path: base`, the shape `merge-gate.yml` already uses. Both halves of that expression are required: a queue run carries no `pull_request`, and a fallback to the triggering sha would run the pull request's copy again on the one run that decides the merge. The base copy loads `policy/gates.json` relative to itself, so it reads the base policy; the pull request's files, body and comments still come from the API, so no other input moves. `test-workflow.test.mjs` narrows its "nothing keys on the pull_request event" assertion from the whole file to the positions that can skip a job (a job-level `if:`, and any condition on the event), and adds the reverse assertion that the event is read on exactly one line: the `ref:` of the base checkout. Two new tests pin the step to the base copy, one of them by running the step's own `run:` command over a fixture whose head copy says ok and whose base copy refuses. Written-by: Dev-4
|
Verdict: PASS at 014b08e Ran at this tip: What I measuredTree downloaded from
The tip's CI, and why it is redAll 11 check runs at So the signature this tip needs is the comment below, and the body now carries The change, read
Both halves of the
Not run, and why: the CI |
Fixes #2099
Written-by: Dev-4
Owner: Dev-4
Reviewer: Birfy
Claim-Id: 5b808c205de2404299980a4db4e2692d
Branch: dev/4-base-policy-sign
Base: 4a9d3ed
Tree: 724fccfa2a9439daf39540c3f1c0858a7fc2b043
Verified commit: 014b08e
Pushed head: 014b08e
Tier: T3
Policy-sign: 6603460197
risk-tier: T3; basis: .github/scripts/test-workflow.test.mjs=T3, .github/workflows/test.yml=T3, changelog.d/2099-base-policy-sign.fixed.md=T1, docs/governance.md=T0
What changed
test.ymlchecks the repository out with noref, so onpull_requestthetestjob carries the merge commit — the pull request's own tree — and theGate change signaturestep ran the copy ofpolicy-sign.mjsthat the pull request may have edited. Weakeningjudge(), or deleting the step, left every check green while the gate the signature protects had been removed.merge-gate.ymlandenqueue-guard.ymlalready avoid this by running their code frombaseor frommain;test.ymlwas the one gate that judged itself with the pull request's copy.The step now runs the checker out of a second checkout, the shape
merge-gate.yml:50-54uses:Both halves of the
refare required, not decoration:test.ymlalso triggers onmerge_group, wheregithub.event.pull_requestis empty, and a pull-request-only expression falls back to the sha that triggered the run — the pull request again — on exactly the run that decides the merge. The base copy loadspolicy/gates.jsonrelative to itself (POLICY_PATHis computed fromimport.meta.url), so it reads the base policy; the pull request's files, body and comments still come from the API, so no other input moves. The step keeps its name, so the required-check context does not move.One consequence worth stating: a pull request that adds an entry to
gateFilesis now judged by the base list, so its new entry binds the pull requests after it, not itself. The entries already listed (includingpolicy/itself) still make such a change need a signature, which is why this pull request needs its own.docs/governance.md§9 now says which copy runs.policy/gates.jsonis untouched: its$commentsentence — "a rule that does not cover its own switch is advice, not a gate" — is about the switch being listed, and it still is.Delta
.github/scripts/test-workflow.test.mjs.github/workflows/test.ymldocs/governance.mdchangelog.d/2099-base-policy-sign.fixed.md4 files, +69/−7. No generated file and no bundle: nothing under
core/orplugins/is touched.Rebase onto live main
The tip moved from
85b382ea(base7fe35211) to014b08ef(base4a9d3edd) with no conflict. Main changed none of these four paths in between:git log --oneline 7fe35211..4a9d3edd -- .github/workflows/test.yml .github/scripts/test-workflow.test.mjs docs/governance.md policy/is empty.The content is the same commit, not a rewritten one:
git range-diff 85b382ea^..85b382ea HEAD^..HEAD→1: 85b382ea = 1: 014b08ef, andgit diff 85b382ea 014b08ef -- <file>is empty for each of the four files.The hole the review on #2095 found
On
#2095(review-only comment6570813153, no conclusion line) the reviewer removed the two newpolicy/gates.jsonentries from the tip's own copy, ranjudge(), and got{"ok": true, "reason": "no gate file changed, so no signature is needed"}— a pull request judged by its own list. That is the read face this pull request closes: the step now runs the checker checked out from the base commit, so the list, the code and the refusal all come from the base.A cost of the second checkout, stated
path: baseputs the base tree inside thetestjob's workspace, so the workspace is no longer one a check that walks the tree root can run in: the review's measurement at this tip isnode .github/scripts/testing-layers.mjs --checkreporting 598 unclassified test files underbase/. Nothing in CI is affected today, because that check runs in thepackagejob (test.yml:134), which has no base checkout; it is written down here because a future root-walking check added totestwould fail on every pull request, and becausemerge-gate.ymltakes the same cost.Red first
On
4a9d3edd2dcac112797e1d028b3f5f790a4f467ewith only.github/scripts/test-workflow.test.mjsfrom this branch in place,TZ=UTC node --test .github/scripts/test-workflow.test.mjs→ 22 tests, 19 pass, 3 fail. The three are the ones this branch turns green:not ok 2 - no job is skipped on merge_group: nothing keys on the pull_request event—only the base checkout reads the pull_request event,0 !== 1.not ok 3 - #2099: the gate signature runs the checker from the base commit—the step runs the base copy, not the pull request's own.not ok 4 - #2099: a pull request that weakens its own copy of the checker is still refused—the step ran.github/scripts/policy-sign.mjs: a pull request that weakened its own copy must still be refused.The last one is behavioural, not a text match: it reads the step's own
run:command out oftest.ymland runs it in a fixture where the head copy ofpolicy-sign.mjsexits 0 and the base copy exits 1. On the base commit the step runs the head copy, so the fixture passes — that is the defect. At this tip the same file is 22 / 22 / 0, and the whole.github/scriptspackage is 601 / 601 / 0.The existing assertion
assert.doesNotMatch(text, /github\.event\.pull_request/)is narrowed, not deleted: it now forbids a job-levelif:and any condition on the event, and adds the reverse assertion that the event is read on exactly one line — theref:of the base checkout — which is an input, not a condition. The narrowing preserves the original intent (nothing skips a job on a queue entry); the base checkout cannot skip anything.Verification at the tip
Tip SHA:
014b08ef23d71282b6fd150f21aa9c21c52f1670, tree724fccfa2a9439daf39540c3f1c0858a7fc2b043, single parent4a9d3edd2dcac112797e1d028b3f5f790a4f467e.Ran:
TZ=UTC ARCH_DOCS_BASE=4a9d3edd2dcac112797e1d028b3f5f790a4f467e npm run verify→ exit 0 — core 2675, social 570, tree 132, timeline 140, workflow 410,.github/scripts601, interop 37, live-e2e 26; 4591 tests, 4591 pass, 0 fail.arch-docs: OK (4 changed file(s)).build-client.mjs --check→ exit 0, silent.ds-bot-patches check→ok ds-bot/ = Fei2411/DeepSeek-Bot@2b10c70 (tree 5834c00, v0.1.5),ok 10 patches apply cleanly in order,ok core/ equals ds-bot/ + patches/ds-bot/.ds-bot-patches canonical→ok patches/ds-bot/ is what generate writes (generate is a no-op).generaterun twice after the commit leftgit status --porcelainempty both times.Not run (and why): the CI
packagejob (pack check, typecheck, actionlint) — it is not part ofnpm run verify; the smoke test — nothing undercore/,plugins/orsrc/host/is touched.Overlap with #2095, and the order
#2095(#2093, still open at head6c0c1d1f) touchespolicy/gates.json,policy-sign.mjs(its header comment),policy-sign.test.mjs,docs/governance.md§9 and one changelog fragment. This branch touchestest.yml,test-workflow.test.mjs,docs/governance.md§9's later paragraph and one changelog fragment. The only shared file isdocs/governance.md, and the two hunks are eight lines apart: merging this branch into#2095's head6c0c1d1fauto-merged it with no conflict, so neither has to wait on the other.Order of preference:
#2095first, then this one rebased onto whatever main it landed as —#2095is the older issue, and its gate list is what this branch's base copy would then read. The reverse order also merges; it is a preference, not a constraint. This branch was cut from4a9d3eddand shares one file with#2095, whose hunks do not overlap, so the rebase either way is mechanical.Gate signature
This pull request edits
.github/workflows/test.yml, whichpolicy/gates.jsonlists, so a signature must sit in this body and must name a comment written for this tip.Signature:
Policy-sign: 6603460197— the Reviewer PASS at014b08ef(comment 6603460197), which names this tip. The line is in the body metadata above; this paragraph used to say the body carried none, and the Reviewer added the line after the PASS at this tip (the author of the pull request may not sign its own gate change).Checkpoint
{ "pr": 2121, "claim": "Dev-4", "issue": 2099, "summary": "The gate-change signature was judged by the checker in the pull request's own tree, so a pull request could weaken or delete policy-sign.mjs or its step and stay green. The step now runs the copy checked out from the base commit, as merge-gate.yml does, with both the merge_group and pull_request halves of the ref. One text assertion is narrowed to the positions that can skip a job, and a reverse assertion pins the single line that reads the event. A behavioural test runs the step's own run: command against a fixture where the head copy says ok and the base copy refuses.", "files": [ ".github/workflows/test.yml", ".github/scripts/test-workflow.test.mjs", "docs/governance.md", "changelog.d/2099-base-policy-sign.fixed.md" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=4a9d3edd npm run verify (exit 0); node --test .github/scripts/test-workflow.test.mjs red-first on 4a9d3edd 22/19/3 and 22/22 at this tip", "result": "green", "at": "014b08ef23d71282b6fd150f21aa9c21c52f1670" }, "next": "none — ready for review; a Policy-sign comment at this tip is still needed" }