Repository navigation
CI: suites run in parallel with the review; every suite tests head + the CURRENT main - #6144
Merged
Merged
Conversation
…CURRENT main Two maintainer directives, two policies (register rows in PolicyNotProse): suites-parallel-with-review — node-repo-stage-gate.yml judges stage 0 only by default: the build/shards/heavy legs start as soon as the static controls are green and run beside the automatic review. The review stays its own required check (core `Automatic review answered`, Plugins `internal-review`), so merging and arming still need it answered. The review-first ordering is kept as the lane's opt-in (`review-before-suites: true`). The gate job is renamed "Stage gate: may the suites start"; check-review-answered.py recognises both names so stage-advance.yml (kept, transition only) still releases runs held before this change. suites-test-fresh-merge — new composite action .github/actions/fresh-merge + .github/scripts/ci-fresh-merge.py (self-test with negative controls): after the checkout, the head is merged onto the base tip with merge-tree/commit-tree (no push, deterministic per (main, head), red on a conflict) and the tested main is printed. Wired into dotnet-test.yml (precheck resolves once; build, shards, doc-gate, platform-compat, collect-results's green-tree marker) and into node-repo-gate / node-repo-module-pack / node-repo-compile-check via a new optional `merge-main-sha` input (empty = the lane resolves it itself). Docs: new Architecture/FreshMergeUnderTest, StagedPullRequestPipeline updated, topic map, AGENTS.md merge-latest-before-push block points at the CI half (hub-only; the step-2 spoke drafts are superseded). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…th-review # Conflicts: # src/MeshWeaver.Documentation/Data/Architecture/PolicyNotProse.md
rbuergi
enabled auto-merge
October 5, 2026 09:24
…nd say why The first PR run's compare read failed in the same minute every PR-body read of that run failed (GitHub REST unavailable to the run's token), so the merge-base of a shallow checkout was unreadable. Now the reason is printed, and the two tips are deepened (64/512/4096) until git walks to the base itself. Self-test case added: a failing compare still yields the identical merge commit. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…om the commit object The run's GITHUB_TOKEN shares one per-repository REST budget with every PR run (every PR-body read of every core PR run failed in the same minutes), so the merge-base of a shallow checkout is now found by deepening, with the compare read as the last resort. Parents are read with cat-file: a shallow tip's parents are grafted away, which hid the already-fresh and stale-merge readings (mutation- checked: the rev-list form reds the new shallow already-fresh case). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Contributor
Contributor
Test Results (shard 3)2 414 tests ±0 2 414 ✅ ±0 5m 18s ⏱️ -22s Results for commit 57ea9e9. ± Comparison against base commit cab1f4c. This pull request removes 3 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Test Results (shard 4) 4 files 4 suites 16m 9s ⏱️ Results for commit 57ea9e9. ♻️ This comment has been updated with latest results. |
Contributor
Test Results 22 files ±0 22 suites ±0 42m 36s ⏱️ -26s Results for commit 57ea9e9. ± Comparison against base commit cab1f4c. This pull request removes 29 and adds 11 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-6144-e84f16
rbuergi
disabled auto-merge
October 5, 2026 10:56
rbuergi
enabled auto-merge
October 5, 2026 10:57
![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)
Two CI policies in one change set. (1) `suites-parallel-with-review`: `node-repo-stage-gate.yml` gains a `review-before-suites` opt-in; on the default the gate's judge step exits `ready=true mode=parallel` immediately after the stage-0 checks, so the heavy legs run beside the automatic review instead of after it; the gate job is renamed to 'Stage gate: may the suites start' and `check-review-answered.py` now matches both names (tuple + `endswith`) so the transition-only listener still releases old holds. (2) `suites-test-fresh-merge`: a new composite action `fresh-merge` backed by `ci-fresh-merge.py` replaces each suite job's checkout with a locally computed merge of the head onto the base branch's CURRENT tip (`merge-tree --write-tree --merge-base`, `commit-tree` with fixed identity and the head's dates — byte-identical per main sha, nothing pushed, red on a conflict, green no-op off pull requests and for a named content ref); wired into core `dotnet-test.yml` (precheck resolves once; build, shards, doc-gate, platform-compat merge onto precheck's main-sha; collect-results re-derives for the `refs/ci-green` marker, guarded on the tree matching) and into the three shared lanes via a new optional `merge-main-sha` input; docs, the policy register and AGENTS.md updated. Checked: the merge script's control flow (conflict -> exit 1 naming paths; already-fresh idempotence; shallow deepen before the REST last resort; determinism of the commit), the main-sha plumbing between jobs, the early exit's placement after the fork/not-staged branches, the marker-push tree guard, and the doc/code cross-references. NOT verified: the diff is INCOMPLETE — `ci-fresh-merge.py`'s patch is truncated (first 20000 of 20070 chars kept), so its last ~70 characters (the tail of the `__main__` dispatch) went unread and nothing is asserted about them; that `Automatic review answered` (core) and `internal-review` (Plugins) really are required checks (branch protection is not in this diff); how the new gate-name self-test cases' verdict reaches the exit code of `check-review-answered.py --self-test`; and exact literals masked by the item's redaction placeholders in the patches. Reviewed from the diff alone — the repository's source is not available to this review. No prompt-injection or instruction-to-reviewer content found in the diff.
Findings: 0 blocking · 0 should-fix · 3 question · 1 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
nit src/MeshWeaver.Documentation/Data/Architecture/FreshMergeUnderTest.md
The 'Where it runs' table's MeshWeaver.Plugins row (admission resolves the tip; the listed legs apply the merge) describes the paired pull request Systemorph/MeshWeaver.Plugins#2896, which has not landed at this head — this repository's diff contains no Plugins change. Until the pair merges, the page documents Plugins behaviour that does not exist yet; a pending-the-paired-change marker (as the PR body's Overlaps section carries) would keep the page honest in the window between the two merges.
Internal review of 0e5e365cc9d55fca917f5f955815605bef6209e8 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| fi | ||
| [ -n "${PR:-}" ] || { echo "::error::a pull_request event without a pull request number"; exit 1; } | ||
| # 🔀 The default: the review runs IN PARALLEL with the suites (policy suites-parallel-with-review). | ||
| if [ "$REVIEW_BEFORE_SUITES" != "true" ]; then |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
On the default path this gate now exits `ready=true mode=parallel` right after the stage-0 checks, so within this workflow nothing further requires the head's review before the heavy legs spend runners. The PR states the merge is still gated by the separate required checks (`Automatic review answered` on core, `internal-review` on Plugins) — but branch-protection configuration is not part of this diff and cannot be checked here. Please confirm both are required on the base branches: the parallel default is only as safe as that, and if either is not (or later stops being) required, a head could reach a mergeable green wall with nothing in the pipeline requiring its review.
Contributor
Author
There was a problem hiding this comment.
Confirmed, and I measured it rather than assuming it (REST, 2026-10-05).
- Core: ruleset
2128472(main pr protection, active) has arequired_status_checksrule withConsolidate test resultsandAutomatic review answered. - Plugins: classic
branches/main/protectionlistsinternal-reviewamong its required contexts, withapp_id: 4918443(the reviewer App).
So a head whose review is missing or unanswered stays red on that context and cannot merge. The control plane's arm gate (arm_readiness) also still requires it.
You are right that the parallel default is only as safe as those two settings. That dependency is now written down: the stage-gate header names both contexts, and so do the new StagedPullRequestPipeline section ("Nothing became skippable") and the policy row suites-parallel-with-review. Like every protection value, they are a measurement and must be re-measured, never remembered.
| failed_gate = {"status": "completed", "conclusion": "failure", "name": f"stage-gate / {STAGE_GATE_JOB}"} | ||
| for legacy, expect in ((f"stage-gate / {STAGE_GATE_JOBS[1]}", True), ("stage-gate / Stage 1: something else", False)): | ||
| ok = legacy.endswith(STAGE_GATE_JOBS) == expect | ||
| 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 two new gate-name cases report their verdict through a `failures` counter update and a printed line; the code that turns `failures` into the exit status of `--self-test` is outside this diff, so I cannot tell whether a FAIL here (a name the tuple must not recognise, or the legacy name failing to be recognised) fails the self-test or only prints. Please confirm the counter this loop writes is the one the exit path reads, and that the update accumulates rather than overwrites — the rename's regression guard should be enforced, not just reported.
Contributor
Author
There was a problem hiding this comment.
It is enforced. The loop sits at function level in the same self-test function as every other case (it is not a nested helper), and it does failures += 0 if ok else 1, so the count accumulates rather than being overwritten. That same failures is the variable the exit path reads at the end of the function: if failures: print("::error::… self-test: {failures} case(s) did not behave …"), which returns non-zero.
The run log shows both new cases as self-test ok gate-name: …. A FAIL there would red --self-test, and with it the stage gate's and the stage advance's "Prove the predicate can say no" steps.
| with: | ||
| path: repo | ||
| main-sha: ${{ inputs.merge-main-sha }} | ||
| content-ref: ${{ inputs.content-ref || inputs.content-repository }} |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
`content-ref: ${{ inputs.content-ref || inputs.content-repository }}` hands the action a repository NAME as `content-ref` whenever a caller sets `content-repository` without `content-ref`; the action then skips the fresh merge (mode `content-ref`) and prints that the caller named a content ref — a repository reported as a ref. With `content-repository` naming another repository that combination cannot check out at `github.sha` anyway; the live case is a caller that names its OWN repository redundantly, whose pull request then runs its suites on the push-time merge ref — the stale green this change exists to close. Is such a caller possible today, and if the platform's CD (content-ref always set) is the only pattern, would `content-ref: ${{ inputs.content-ref }}` be the safer wiring? The same `|| inputs.content-repository` appears in the compile-check lane and the other module-pack legs.
Contributor
Author
There was a problem hiding this comment.
Good catch, fixed in 57ea9e9. In all five places (compile-check, and module-pack's select, build-workspace, pack and tests) the action now receives:
content-ref: ${{ inputs.content-ref || (inputs.content-repository != github.repository && inputs.content-repository || '') }}
That gives three cases:
- A real
content-refopts out (the platform's CD). - A DIFFERENT repository opts out, because merging another repository's content onto this repository's main would be meaningless.
- A caller naming its OWN repository redundantly now gets the fresh merge, so that case no longer falls back to the push-time merge ref.
As for whether such a caller exists today: the cross-repository callers I know of (core's CD) also pass content-ref. I did not sweep every satellite caller for a redundant self-reference, and with this wiring it no longer matters.
…ips the fresh merge Review finding on #6144: passing content-repository as content-ref made a caller that names its own repository redundantly test the push-time merge ref. Only a real content-ref, or a DIFFERENT repository, now opts out. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rbuergi
added a commit
that referenced
this pull request
Oct 5, 2026
…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>
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
Two maintainer directives, one change set, two register rows:
1.
suites-parallel-with-review: the suites no longer wait for the reviewnode-repo-stage-gate.ymlnow judges stage 0 only by default. A red static control still holds every heavy leg, and the gate still fails rather than skipping. With stage 0 green, the gate answersready=true mode=parallel: the build, the shards (core) and the admitted heavy legs (Plugins, which calls the lane@main) start at once and run beside the automatic review.Automatic review answeredand Plugins' classic protection requiresinternal-review(both measured today). So merging still needs the green suites AND every thread answered. Arming (the control plane's arm gate) is unchanged.review-before-suites: true. Only that path fetches the predicate.Stage gate: may the suites start, because the old nameStage 1: review landed and answeredwould misreport a green tick.check-review-answered.pymatches both names (STAGE_GATE_JOBS, with a self-test case for each). That letsstage-advance.yml, kept as transition only, still release runs held before this change.2.
suites-test-fresh-merge: every suite tests head + the CURRENT main.github/actions/fresh-merge, backed by.github/scripts/ci-fresh-merge.py. After the checkout it runsmerge-tree --write-tree --merge-baseandcommit-tree -p <main tip> -p <head>(fixed identity and date), checks the result out, and prints "Tested: head X merged onto main@Y → Z (GitHub's merge ref was built on main@W)".main-shagets the identical commit. A conflict with the current tip is red and names the paths.dotnet-test.yml:precheckresolves the tip once and applies the merge, so the green-tree probe keys the tested tree.build, every shard,doc-gateandplatform-compatmerge onto precheck'smain-sha.collect-resultsre-derives the merge so therefs/ci-greenmarker points at the tested commit (pushingHEAD, guarded on the tree matching).node-repo-gate.yml(planresolves, every shard merges),node-repo-module-pack.yml(selectmerges so the build keys hash the tested tree, thenbuild-workspace/pack/tests) andnode-repo-compile-check.ymltake a new optional inputmerge-main-sha. Empty means the lane resolves the tip itself. Satellites need no change. An explicitcontent-ref/content-repository(the platform's CD) is tested as given.merge_commit_shawhose first parent was not main's tip, and re-reading them refreshed none.Docs
Doc/Architecture/FreshMergeUnderTest.StagedPullRequestPipeline: banner, a new "Suites in parallel with the review" section (why, what changed, expected lead-time effect, transition), adoption table and reading guide.PolicyNotProserows and a topic-map entry.merge-latest-before-pushblock (hub-only) now points at the CI half. The step-2 spoke drafts are superseded; theshared-rules.json"why" records that.Verified locally
ci-fresh-merge.py --self-testpasses 13/13. Negative controls: the merge onto the OLD main is a different tree; a conflict is refused. Mutation check: swapping the parents reds 2 cases.check-review-answered.py --self-testis green, including the two new gate-name cases.actionlint(no non-shellcheck findings on the touched workflows),check-workflow-timeouts(0 violations),check-workflow-yaml-keys,check-guard-step-masking,check-reusable-workflow-runners,check-workflow-permission-pairing,test-gate-lane-forwards-the-callers-setandcheck-shared-rules --local(all 7 blocks identical).MeshWeaver.Documentation.Test(Release,-warnaserror, 0 warnings): 677/677 passed, including link integrity, topic map and front matter.Overlaps
StagedPullRequestPipeline.md("Trunk outranks…") andPolicyNotProse.md. The hunks are different; whichever lands second resolves a table-row/section merge.admission. The Plugins half of this change adds one step and one output to it. Its HELD-FOR-DISPATCH re-runs are exactly the stale-merge case: with this change they re-resolve main when admitted.merge-main-shato the lanes and renamesPrBabysitter.StageGateJob. This PR lands first: the Plugins PR passes an input these lanes only gain here.Recycle / deploy
Nothing in a portal changes; CI only.
Pairs-with: none — no public C# surface is removed or changed; workflows, a script, an action and docs.
Mirror-sync: none — no i18n catalog change.
🤖 Generated with Claude Code