Repository navigation
CI: review first, then the suites on a fresh merge, then the arm (policy review-then-suites) - #6162
Merged
Merged
Conversation
…icy review-then-suites) Maintainer correction of #6144's parallel default: the fleet-wide order is review -> suites against a FRESH merge with the current main -> arm only when the review is answered AND the suites are green. - node-repo-stage-gate.yml: review-before-suites defaults to TRUE again (the parallel path stays as a per-caller opt-out); stage-advance listeners are required again, not transition-only. - dotnet-test.yml: the suites' main is resolved by the build job (re-run when a held run is released) instead of precheck (not re-run), so a released run tests today's main; shards, doc-gate, platform-compat and the green-tree marker follow the build's main and tree. - check-review-answered.py arm gate: condition 4 required_checks_green - every required status check of the base (rulesets + classic protection, review contexts excluded, an empty set refused) completed success on the head. 9 new self-test cases; a mutation dropping the condition reds 6. - Docs: register row suites-parallel-with-review -> review-then-suites; StagedPullRequestPipeline, FreshMergeUnderTest, topic map. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rbuergi
enabled auto-merge
October 5, 2026 16:37
Contributor
Test Results 22 files ±0 22 suites ±0 43m 9s ⏱️ -51s Results for commit d13dbfb. ± Comparison against base commit c13c7c4. This pull request removes 31 and adds 15 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
systemorph-com
Bot
added
the
thread:pr-systemorph-meshweaver-6162-ed3ed9
rbuergi
disabled auto-merge
October 5, 2026 17:15
rbuergi
enabled auto-merge
October 5, 2026 17:16
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
This head reverts #6144's parallel order to review-first and adds the arm gate's condition 4. node-repo-stage-gate.yml flips the reusable gate's review-before-suites default back to true (parallel becomes a per-caller opt-out) and the stage-advance listeners are re-documented as required; dotnet-test.yml moves the fresh-merge main resolution from precheck into build (a held run released by rerun-failed-jobs re-runs build but not a green precheck), so every shard, doc-gate, platform-compat and the refs/ci-green marker now key the build's main and tree, with a precheck-tree fallback on the reuse path; check-review-answered.py gains required_checks_green (every required context of the base, minus the review's own contexts, must be completed success on the head; an empty required set is refused), read_required_contexts, read_head_runs and nine self-test cases. Docs (Architecture, PolicyNotProse, FreshMergeUnderTest, StagedPullRequestPipeline) track the code.
Checked from the diff: the predicate's vacuous-set refusal, head binding and newest-run selection (started_at, id); the backward-compatible arm_readiness defaults, so the existing merge-gate callers are unchanged; pagination and every fail-closed path; the build/shard/marker rewiring and the reuse-path fallback; the gate default flip's fall-through logic; docs-vs-code consistency; and a scan of the diff for prompt injection (none found - the maintainer quotes are policy rationale, not instructions to a reviewer).
The blocking defect is in read_required_contexts: the classic-protection read queries branches/{base} without the query parameter that makes that endpoint return a protection object, and it REPLACES the ruleset result instead of unioning with it, so required comes back empty for every branch and the arm gate refuses everything - condition 4 is inert (fail-closed: nothing unsafe merges, but the gate can never pass).
Not verifiable from the diff: the Gh wrapper (read as pass-through, as read_head_runs' explicit query strings imply); DEGRADATION_CHECK_NAME's value (the 'only review contexts required' self-test case passes only if it is 'internal-review'); the needs: lists of the jobs now reading needs.build.outputs.* (no needs edit appears in the diff - this head's run passed stage 0, which carries the CI-shell/actionlint control, so those references presumably resolve); the claimed local runs (self-test, actionlint, timeout and masking checks); and the Plugins repository's listener state (MeshWeaver.Plugins#2904 is out of scope here).
Findings: 1 blocking · 0 should-fix · 2 question · 0 nit
Internal review of d13dbfb06dce335aa2220b9bee64815dd01f6e52 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| pass | ||
| try: | ||
| prot = (gh.api(f"branches/{base}") or {}).get("protection") or {} | ||
| names += list((prot.get("required_status_checks") or {}).get("contexts") or []) |
Contributor
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
read_required_contexts cannot return what its docstring promises, so the new condition 4 can never pass. Two defects, both in this new function: (1) the classic-protection read calls gh.api('branches/{base}') with no query string, but that endpoint returns the protection object only when called with ?protection=true (the dedicated resource is branches/{base}/protection), so .get('protection') on its response is always empty - the same diff's read_head_runs shows query parameters are passed through the path, so nothing adds it; (2) that read REPLACES names (names = list(...) at this line) instead of extending it, so a successful ruleset read is wiped whenever the classic read comes back empty - and by this function's own docstring the fleet is split between rulesets and classic protection, so a ruleset-protected branch, which has no classic protection to read, always ends with names == []. Net effect: run_arm_gate always passes required == [], required_checks_green always refuses with 'no required status check could be read', and the arm gate never arms - this change's headline behavior is inert. The failure is fail-closed (nothing unsafe merges; GitHub's auto-merge independently waits for the required contexts), and the body's 'Not established' section names exactly these live reads - but the defect is visible in the code, not in the token. The remedy is to union the two sources (extend names from both reads rather than overwrite) and to read classic protection from branches/{base}/protection, or to pass ?protection=true.
Contributor
Author
There was a problem hiding this comment.
I measured both claimed defects against the code and the live API, and neither holds. No change.
-
The function does not replace
names. At this line it extends the list:names += list((prot.get("required_status_checks") or {}).get("contexts") or []). The rulesets read uses+=as well, so the two sources are unioned and then de-duplicated. -
branches/{base}does carry the protection object, with no query string. Measured 2026-10-05:gh api repos/Systemorph/MeshWeaver.Plugins/branches/main --jq '.protection.required_status_checks.contexts|length'returns 9 (classic protection).gh api repos/Systemorph/MeshWeaver/rules/branches/mainreturns["Consolidate test results","Automatic review answered"](ruleset; the review context is then excluded byREVIEW_CONTEXTS).
?protection=trueis a filter on the branch LISTING endpoint, not on a single branch.
So on both protection shapes, required is non-empty and condition 4 can pass. It still fails closed if both reads come back empty.
| nonlocal failures | ||
| v = arm_readiness(_pr(0), [], [_ir()], required, head_runs) | ||
| ok = v.ready == ready and (says in v.missing if not ready else not v.missing) | ||
| failures += 0 if ok else 1 |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The new suites_case helper writes failures = 0 if ok else 1 - an assignment, not an accumulation. If the surrounding self-test counts failures across cases (arm_case's body is outside this diff), any earlier failing case is erased by the next passing one, and this block's own last case ('not asked (pure review gate) -> ARM on review alone') passes by construction, so none of the six negative-control reds the PR body describes could reach the suite's failure count - the printed 'self-test FAIL' lines would be the only record of them. Which of the two the self-test's exit code uses cannot be told from this diff; if it is failures, the nine new cases cannot redden the suite, and the claimed mutation control ('a mutation that drops the condition reds 6 of them') cannot hold as an exit-code result.
Contributor
Author
There was a problem hiding this comment.
It accumulates. The helper says nonlocal failures and then failures += 0 if ok else 1; the PR has no failures = 0 if ok…. The exit path reads that same counter: if failures: … return 1.
I measured it by exit code, not just from the printed lines: a mutant with condition 4 removed (suites = '') makes --self-test exit 1, and the real script exits 0.
| required: false | ||
| type: boolean | ||
| default: false | ||
| default: true |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
Flipping the shared gate's review-before-suites default to true binds every caller that does not pass the input, and by this file's own header a gate without its stage-advance.yml listener 'holds forever'. This diff restores core's listener in its comments and the updated docs table lists Plugins' listener as present, but the Plugins side is outside this diff: #6144's transition plan was to delete that listener 'once no open pull request's newest run carries such a hold', and this PR's body defers the Plugins port of condition 4 to MeshWeaver.Plugins#2904. If Plugins calls this reusable gate at @main and its listener was deleted under that cleanup - or is deleted before #2904 lands - every Plugins pull request holds indefinitely from the moment this merges. The state of the Plugins repository's listener cannot be verified from this diff.
Contributor
Author
There was a problem hiding this comment.
Verified, and this risk is not live. MeshWeaver.Plugins' listener is on main: GET repos/Systemorph/MeshWeaver.Plugins/contents/.github/workflows/stage-advance.yml?ref=main returns blob 93198b9d. It was never deleted; #6144's transition-only note was a comment, and #2904 restores the header to "REQUIRED".
The Plugins PR (#2904) now states the same order: review first, stage-advance.yml required, and PrArming condition 4.
The satellites do not call node-repo-stage-gate.yml (see the StagedPullRequestPipeline adoption table), so the default flip binds only core and Plugins, and both carry their listener.
Contributor
Test Results (shard 0) 3 files ±0 3 suites ±0 2m 58s ⏱️ -41s Results for commit d13dbfb. ± Comparison against base commit c13c7c4. This pull request removes 2 and adds 3 tests. Note that renamed tests count towards both. |
Contributor
Contributor
Contributor
Test Results (shard 3)2 440 tests +1 2 440 ✅ +1 5m 47s ⏱️ -2s Results for commit d13dbfb. ± Comparison against base commit c13c7c4. This pull request removes 3 and adds 2 tests. Note that renamed tests count towards both. |
Contributor
Contributor
Test Results (shard 4) 4 files ±0 4 suites ±0 16m 1s ⏱️ -28s Results for commit d13dbfb. ± Comparison against base commit c13c7c4. This pull request removes 26 and adds 10 tests. Note that renamed tests count towards both. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
This is the maintainer's correction of #6144's parallel default. The fleet-wide order is now review first → suites against a FRESH merge with the current main → arm only when the review is answered AND the suites are green. Policy row
suites-parallel-with-reviewis replaced byreview-then-suites. The fresh-merge part of #6144 (suites-test-fresh-merge) stays.node-repo-stage-gate.yml:review-before-suitesdefaults to true again for every caller. Running the suites beside the review is now only a per-caller opt-out (false), which never changes the merge or the arm. Core's and Plugins'stage-advance.ymllisteners are required again, not transition-only. The job nameStage gate: may the suites startstays, and the event half recognises both names.dotnet-test.yml, fresh main at the moment the suites START: a held run is released byrerun-failed-jobs. That re-runsbuildbut not a greenprecheck, so the main sha is now resolved inbuild. The shards,doc-gate,platform-compatand therefs/ci-greenmarker all follow the build's main and tree. Without this, a run released hours later would test the main of its push.check-review-answered.py→required_checks_green): auto-merge is armed only once every required status check of the base issuccesson the current head.StagedPullRequestPipelinehas a new banner and a section "A day in parallel, and why it was reverted"; stage 3 now names condition 4.FreshMergeUnderTestsays where "now" is resolved.Arming fleet-wide: what this does and what it does not
The predicate is the single source. Two enforcers use it:
auto-arm.yml, which every satellite and Memex reach throughworkflow_call. Today it still arms on open/push. The switch to--arm-gatebelongs to auto-arm: arm only on a reviewed, answered head (step 3/3) #6063 (draft, another session's, stacked and waiting on the callers'checks: readgrants). Once auto-arm: arm only on a reviewed, answered head (step 3/3) #6063 lands, every caller arms only after the review is answered and the suites are green, with no caller-side change. I did not fork auto-arm: arm only on a reviewed, answered head (step 3/3) #6063's work here.PrArming(fix(cd): a broken pipe mid-push no longer throws away the whole release (#2810) #2828, control plane) ports condition 4 in Systemorph/MeshWeaver.Plugins#2904.Until #6063 lands, an early arm cannot merge anything early: GitHub's auto-merge still waits for every required context (core:
Consolidate test results+Automatic review answered).Verified locally
check-review-answered.py --self-testis green, including the 9 arm+suites cases. Negative control: removing condition 4 reds 6 of them.ci-fresh-merge.py --self-testis green.actionlintreports nothing on the touched workflows;check-workflow-timeoutsandcheck-guard-step-maskingreport 0 violations.MeshWeaver.Documentation.Test: 676/676 passed (Release,-warnaserror).Not established
rules/branches/main,branches/main) under the arm lane's App token. Fail-closed: a read that returns nothing means no arm, and the step says why.Pairs-with: none — no public C# surface; workflows, a script and docs.
Mirror-sync: none — no i18n catalog change.
🤖 Generated with Claude Code