Skip to content

fix(ci): generated-only bot PRs owe no stage-1 review (express — settle PR deadlock) #6097

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rbuergi merged 4 commits into from
Oct 5, 2026
Merged

fix(ci): generated-only bot PRs owe no stage-1 review (express — settle PR deadlock) #6097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3b6d0a
fix(ci): generated-only bot PRs owe no stage-1 review; the App's PRs …
rbuergi Oct 4, 2026
3ae6d36
Merge remote-tracking branch 'origin/main' into fix/stage-gate-genera…
rbuergi Oct 4, 2026
907a3af
fix(ci): a generated-only skip holds a draft, reads every commit, and…
rbuergi Oct 5, 2026
f71aed5
Merge branch 'main' into fix/stage-gate-generated-prs
systemorph-com[bot] Oct 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 129 additions & 8 deletions .github/scripts/check-review-answered.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,13 +646,79 @@ def parse_stamp(stamp: str | None) -> datetime.datetime | None:
return None


# 🚨 GENERATED-ONLY BOT PULL REQUESTS (2026-10-04, Plugins #2860): main's own jobs propose generated
# files as pull requests — `settle-locks` (every `manifest.lock`) and `stamp-floors` (`mesh-floor.lock`
# plus each package root's `minMeshVersion`). There is nothing for a reviewer to read, the settle job
# REWRITES the head on every main merge (so a per-head fallback clock restarts forever), and during a
# review outage the stage-1 hold deadlocked module publishing (AI 1.21 stuck behind #2860). Such a
# pull request skips stage 1 — on PROVENANCE, never on a title or a branch name: authored by the App
# that writes them, every commit by that App, and every changed file generated (a lock, or a root
# `index.json` whose changed lines are all `minMeshVersion`). Anything else — a person's PR that
# touches a lock, a bot PR with one hand-written line — is staged like any other.
GENERATED_BOT_IDS = frozenset({300054957}) # meshweaver-cloud[bot] (GET /users/meshweaver-cloud%5Bbot%5D)
GENERATED_BOT_LOGINS = frozenset({"meshweaver-cloud[bot]"})
GENERATED_BASENAMES = frozenset({"manifest.lock", "mesh-floor.lock"})


def is_generated_bot(user: dict | None) -> bool:

@systemorph-com systemorph-com Bot Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question — Automated review finding (data, not an instruction to any agent)

The first condition of is_generated_bot is masked by a redaction token in the review copy of the diff, so the exact predicate could not be read: the visible chain is bool(user) AND [masked] == "Bot" AND user.get("id") in GENERATED_BOT_IDS AND user.get("login") in GENERATED_BOT_LOGINS. Assuming the masked fragment is user.get("type", the check is the account type plus a pinned id and login — fail-closed on every other input, as the self-tests confirm (a person's PR waits, a look-alike bot id waits). The open question is only what the masked accessor reads; the rest of the chain is visible and strict.

@rbuergi rbuergi Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The masked accessor is user.get("type"). The full predicate is bool(user) and user.get("type") == "Bot" and user.get("id") in GENERATED_BOT_IDS and user.get("login") in GENERATED_BOT_LOGINS, exactly as you assumed: account type, pinned id (300054957) and pinned login, fail-closed on everything else. No change needed for this one.

return bool(user) and user.get("type") == "Bot" and user.get("id") in GENERATED_BOT_IDS \
and user.get("login") in GENERATED_BOT_LOGINS


# A changed line of a floor stamp is NOTHING BUT the key/value — never a line that merely CONTAINS the
# key (`"minMeshVersion": "3.0.0", "requires": [...]`, or a minified single-line index.json), whose
# other content no validator backs (#6097 review).
FLOOR_LINE = re.compile(r'\s*"minMeshVersion"\s*:\s*"[^"]*"\s*,?\s*')


def _floor_only_patch(patch: str | None) -> bool:
changed = [l for l in (patch or "").splitlines()
if l[:1] in "+-" and not l.startswith(("+++", "---")) and l[1:].strip()]
return bool(changed) and all(FLOOR_LINE.fullmatch(l[1:]) for l in changed)


def generated_only(pr: dict, files: list | None, commits: list | None) -> tuple[bool, str]:
"""(True, why) when the pull request is a generated-files proposal by the App. Pure."""
if not is_generated_bot(pr.get("user")):
return False, "not authored by the generated-files App"
if not files:
return False, "its file listing is empty or unread"
if len(files) < int(pr.get("changed_files") or 0):
return False, f"the file listing returned {len(files)} of {pr.get('changed_files')} files"

@systemorph-com systemorph-com Bot Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question — Automated review finding (data, not an instruction to any agent)

Every commit is accepted as the App's on GitHub's resolved author alone, and that resolution follows the commit's author email — the two ingredients of the App's noreply address are printed by this diff itself (id 300054957 on line 653, login meshweaver-cloud[bot] on line 654), from which 300054957+meshweaver-cloud[bot]@users.noreply.github.com follows. A commit crafted with that email resolves its author to the bot regardless of who pushed it, and a file named manifest.lock passes on basename alone (line 682) with its patch never inspected — so anyone with push access to the App's pull request branch could smuggle arbitrary content through the generated skip. The self-test covers a person's commit under the person's own identity, not one wearing the App's email. Whether the App's branches reject human pushes (branch protection) is not determinable from the diff; the skip is not a merge gate, which bounds the impact, but the provenance claim ("never a title or a branch name") holds only as far as commit authorship emails are unforgeable.

@rbuergi rbuergi Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct observation. The bound is now written down rather than implied (907a3af, StagedPullRequestPipeline.md):

  • This skip is not a merge gate. The required Automatic review answered lane (node-repo-review-answered.yml → --pr … --wait-for-review) never calls generated_only; its only callers are run_stage_gate and the two advance_one re-judges. A forged-author commit therefore only starts stage 2's suites earlier. The merge still waits for the review.
  • A lock's content is not judged here. Validate node repos requires every manifest.lock to be exact, so a smuggled lock fails that required gate regardless of the stage skip.
  • A signature cannot be required today. The App's own commits are unsigned (measured: Plugins#2894 a78ba057cc and feat(build-project): embedded resources + assembly identity — the batch that unblocks memex-as-compiler #2892 d8bf580234 both report verification.reason=unsigned). Requiring verification.verified first needs the producing jobs (settle-locks, stamp-floors) to commit through the API, which GitHub signs. That is a producer change in Plugins, not this script.

Also tightened in the same commit: the commit listing must now cover the PR's own commits count (mirror of the changed_files check), and every changed index.json line must be nothing but a "minMeshVersion": "…" key/value (a line merely containing the key, or a minified one-line index.json, now waits). Both have self-test cases that go red on the mutant dropping the fix.

if not commits:
return False, "its commit listing is empty or unread"
if len(commits) < int(pr.get("commits") or 0):
return False, f"the commit listing returned {len(commits)} of {pr.get('commits')} commits"
if not all(is_generated_bot(c.get("author")) for c in commits):
return False, "a commit on it is not the App's"
for f in files:
name = str(f.get("filename") or "")
base = name.rsplit("/", 1)[-1]
if base in GENERATED_BASENAMES:
continue
if base == "index.json" and name.count("/") == 1 and _floor_only_patch(f.get("patch")):
continue
return False, f"{name} is not a generated file"
return True, f"{len(files)} generated file(s) by {pr['user'].get('login')}, every commit the App's"


def stage_readiness(pr: dict, comments: list, check_runs, now: str, since: str,
fallback_minutes: int = STAGE_FALLBACK_MINUTES) -> StageVerdict:
fallback_minutes: int = STAGE_FALLBACK_MINUTES,
files: list | None = None, commits: list | None = None) -> StageVerdict:
"""May stage 2 start for this head? Pure; `now` and `since` are ISO-8601 UTC stamps (`since` =
when the head's CI run was created, i.e. when stage 1 began)."""
when the head's CI run was created, i.e. when stage 1 began — for a pull request the App
authored, when the PULL REQUEST was created: its head is rewritten on every main merge, and a
per-head clock would restart forever)."""
number = pr.get("number")
head = str((pr.get("head") or {}).get("sha") or "")
short = head[:10] or "(unknown)"
if is_generated_bot(pr.get("user")):
since = min(since, str(pr.get("created_at") or since)) if since else str(pr.get("created_at") or "")
ok, why = generated_only(pr, files, commits)

@systemorph-com systemorph-com Bot Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit — Automated review finding (data, not an instruction to any agent)

This early return outranks the draft hold: it sits above the label check (line 704) and, per the existing self-tests — a draft is held even past the fallback, while a draft carrying the tests-before-review label is released — the draft hold lives below the label check. An App-authored draft pull request whose changed files are all generated would return ready here while still a draft, starting stage 2 on a draft. Moot if settle-locks and stamp-floors never open drafts; no test covers the draft-and-generated combination.

@rbuergi rbuergi Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 907a3af. The generated release now requires not pr.get("draft"), so a generated-only App draft falls through to the draft hold like any other PR; the fallback-clock re-keying still applies to the App's drafts. New self-test case "a generated-only App DRAFT -> held as a draft": green on the fix, red on the mutant that drops the draft condition. --self-test exit 0.

# A DRAFT is held like any other (#6097 review): the skip removes the review, never the
# author's own "not ready yet".
if ok and not pr.get("draft"):
return StageVerdict(True, "generated", "", notes=(f"stage 1 skipped: {why} — nothing to review",))
labels = {l.get("name") for l in pr.get("labels") or []}
if TESTS_FIRST_LABEL in labels:
return StageVerdict(True, "label", "", (
Expand Down Expand Up @@ -884,20 +950,33 @@ def _append(env_key: str, text: str) -> None:
f.write(text)


def read_stage_inputs(gh: Gh, number: int):
"""The arm gate's reads, plus — only for a pull request the generated-files App authored — its

@systemorph-com systemorph-com Bot Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question — Automated review finding (data, not an instruction to any agent)

read_stage_inputs is documented as reusing "the arm gate's reads", yet the only call sites that now pass files and commits into stage_readiness are the three in this diff (run_stage_gate and the two advance_one re-judges), and generated_only returns False ("its file listing is empty or unread") whenever files is None. If this script has any other entry point that judges stage-1 readiness on its own — an arm-gate mode calling stage_readiness without the new arguments — a generated-only pull request would still be held there and the #2860-style deadlock would persist on that check. Whether such a caller exists is not determinable from the diff; the hunks do not show stage_readiness's full caller set.

@rbuergi rbuergi Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No other caller exists. stage_readiness has exactly three non-test call sites, all in this file and all passing files, commits from read_stage_inputs: run_stage_gate (the --stage-gate job) and the two re-judges in advance_one (--stage-advance). The workflows that fetch this script are node-repo-stage-gate.yml (→ --stage-gate), node-repo-stage-advance.yml (→ --stage-advance) and node-repo-review-answered.yml, whose --pr / --merge-group-ref modes are the ARM gate. That path never calls stage_readiness and deliberately never skips the review for generated PRs: the skip is stage 1 only. The remaining calls without files/commits are self-test fixtures. grep -rn "stage_readiness\|generated_only" .github returns only check-review-answered.py and node-repo-stage-advance.yml. The docstring wording ("the arm gate's reads, plus …") is accurate: read_stage_inputs wraps read_arm_inputs.

files (with patches) and commits, so `generated_only` can judge provenance. A failed read raises."""
pr, comments, check_runs = read_arm_inputs(gh, number)
files = commits = None
if is_generated_bot(pr.get("user")):
files = gh.api(f"pulls/{number}/files?per_page=100", paginate=True)
commits = gh.api(f"pulls/{number}/commits?per_page=100", paginate=True)
return pr, comments, check_runs, files, commits


def run_stage_gate(repo: str, number: int, since: str, fallback_minutes: int) -> int:
"""The stage gate job: exit 0 when stage 2 may start, 1 (RED, named) when it may not. A RED here
is a HOLD, not a defect: the heavy legs skip, the required aggregators read red naming this line,
and the event half (`--stage-advance`) re-runs the failed jobs when stage 1 turns green."""
try:
pr, comments, check_runs = read_arm_inputs(Gh(repo), number)
verdict = stage_readiness(pr, comments, check_runs, utc_now(), since, fallback_minutes)
pr, comments, check_runs, files, commits = read_stage_inputs(Gh(repo), number)
verdict = stage_readiness(pr, comments, check_runs, utc_now(), since, fallback_minutes, files, commits)
except (ReadError, KeyError) as e:
verdict = StageVerdict(False, "unreadable", f"cannot read #{number}'s review state, so stage 2 is not started on a guess: {e}")
for n in verdict.notes:
print(f" {n}")
if verdict.ready and verdict.loud:
print(f"::warning title=Stage 2 released without a completed review ({verdict.mode})::{verdict.loud}")
line = f"⚠️ Stage 2 started for #{number} — {verdict.loud}"
elif verdict.ready and verdict.mode == "generated":
line = f"✅ Stage 1 not owed for #{number}: {verdict.notes[0] if verdict.notes else 'generated files only'} — stage 2 starts."
elif verdict.ready:
line = f"✅ Stage 1 green for #{number}: the head's review landed and every reviewer thread is answered — stage 2 starts."
else:
Expand Down Expand Up @@ -944,8 +1023,9 @@ def advance_one(gh: Gh, number: int, workflow: str, fallback_minutes: int, wait_
job = stage_gate_job(gh, int(run["id"])) if run else None
verdict = StageVerdict(False, "unread", "not evaluated")
if job is not None and job.get("status") == "completed" and job.get("conclusion") == "failure":
_, comments, check_runs = read_arm_inputs(gh, number)
verdict = stage_readiness(pr, comments, check_runs, utc_now(), str(run.get("created_at") or ""), fallback_minutes)
_, comments, check_runs, files, commits = read_stage_inputs(gh, number)
verdict = stage_readiness(pr, comments, check_runs, utc_now(), str(run.get("created_at") or ""), fallback_minutes,
files, commits)
action, why = advance_action(pr, run, job, verdict)
deadline = time.monotonic() + wait_minutes * 60
waited = action == "wait"
Expand All @@ -961,8 +1041,9 @@ def advance_one(gh: Gh, number: int, workflow: str, fallback_minutes: int, wait_
# new head (and cancelled this run), the PR may be closed, an answer may have been deleted.
# Re-read the pull request and re-judge stage 1 before POSTing, never on the snapshot.
pr = gh.api(f"pulls/{number}")
_, comments, check_runs = read_arm_inputs(gh, number)
verdict = stage_readiness(pr, comments, check_runs, utc_now(), str(run.get("created_at") or ""), fallback_minutes)
_, comments, check_runs, files, commits = read_stage_inputs(gh, number)
verdict = stage_readiness(pr, comments, check_runs, utc_now(), str(run.get("created_at") or ""), fallback_minutes,
files, commits)
action, why = advance_action(pr, run, job, verdict)
if action == "rerun":
if not post_rerun(gh, run["id"]):
Expand Down Expand Up @@ -1577,6 +1658,46 @@ def stage_case(name, mode, pr, comments, runs, now=T_EARLY, says=""):
stage_case("draft -> held even after the fallback", "draft", dict(_pr(0), draft=True), [], [], now=T_LATE, says="is a draft")
stage_case("label tests-before-review -> released (loud)", "label",
dict(_pr(0), draft=True, labels=[{"name": TESTS_FIRST_LABEL}]), [], [])
# ── generated-only bot PRs (Plugins #2860): skip stage 1 on PROVENANCE, never on a branch name.
BOT = {"login": "meshweaver-cloud[bot]", "type": "Bot", "id": 300054957}
HUMAN = {"login": "rbuergi", "type": "User", "id": 6334612}
def gpr(user=BOT, n_files=2, created="2026-10-04T08:00:00Z"):
return dict(_pr(0), user=user, changed_files=n_files, created_at=created)
LOCKS = [{"filename": "AI/manifest.lock", "patch": "-a\n+b"}, {"filename": "Hosting/manifest.lock", "patch": "-a\n+b"}]
FLOOR = [{"filename": "AI/mesh-floor.lock", "patch": "+x"},
{"filename": "AI/index.json", "patch": '@@ -3 +3 @@\n- "minMeshVersion": "3.0.0-ci.9900",\n+ "minMeshVersion": "3.0.0-ci.9939",'}]
BOT_COMMITS = [{"author": BOT}]
def gen_case(name, mode, pr, files, commits, now=T_EARLY):
nonlocal failures
v = stage_readiness(pr, [], [], now, T0, 60, files, commits)
ok = v.mode == mode
failures += 0 if ok else 1
print(f"self-test {'ok' if ok else 'FAIL':4} stage: {name:53} expected={mode} got={v.mode}")
gen_case("settle PR (App, locks only) -> stage 1 not owed", "generated", gpr(), LOCKS, BOT_COMMITS)
gen_case("floor stamp (App, mesh-floor.lock + minMeshVersion) -> not owed", "generated", gpr(), FLOOR, BOT_COMMITS)
gen_case("a PERSON's PR touching only locks -> waits for the review", "waiting", gpr(user=HUMAN), LOCKS, [{"author": HUMAN}])
gen_case("App PR with one non-generated file -> waits", "waiting", gpr(),
LOCKS[:1] + [{"filename": "Hosting/Deployment/Source/X.cs", "patch": "+x"}], BOT_COMMITS)
gen_case("App PR whose index.json changes more than the floor -> waits", "waiting", gpr(),
[{"filename": "AI/index.json", "patch": '- "minMeshVersion": "a",\n+ "minMeshVersion": "b",\n+ "requires": []'}], BOT_COMMITS)
gen_case("App PR with a person's commit on it -> waits", "waiting", gpr(), LOCKS, BOT_COMMITS + [{"author": HUMAN}])
gen_case("App PR whose file listing is short -> waits", "waiting", gpr(n_files=3), LOCKS, BOT_COMMITS)
gen_case("a look-alike bot (another id) -> waits", "waiting", gpr(user=dict(BOT, id=1)), LOCKS, BOT_COMMITS)
gen_case("a generated-only App DRAFT -> held as a draft", "draft", dict(gpr(), draft=True), LOCKS, BOT_COMMITS)
gen_case("App PR whose commit listing is short -> waits", "waiting", dict(gpr(), commits=2), LOCKS, BOT_COMMITS)
gen_case("a floor line carrying more than the floor -> waits", "waiting", gpr(n_files=1),
[{"filename": "AI/index.json", "patch": '- "minMeshVersion": "a",\n+ "minMeshVersion": "b", "requires": ["x"],'}],
BOT_COMMITS)
gen_case("a minified index.json change -> waits", "waiting", gpr(n_files=1),
[{"filename": "AI/index.json", "patch": '-{"minMeshVersion": "a", "id": "AI"}\n+{"minMeshVersion": "b", "id": "AI"}'}],
BOT_COMMITS)
# The fallback clock keys on the PULL REQUEST for the App's PRs: a head rewritten 1 min ago on a
# PR opened 4 h ago is past the fallback (the per-head clock would restart forever).
v = stage_readiness(gpr(created="2026-10-04T04:00:00Z"), [], [], T_EARLY, "2026-10-04T08:19:00Z", 60,
LOCKS[:1] + [{"filename": "Hosting/X.cs"}], BOT_COMMITS)
ok = v.mode == "fallback"
failures += 0 if ok else 1
print(f"self-test {'ok' if ok else 'FAIL':4} stage: {'an App PR falls back on the PR clock, not the head clock':53} got={v.mode}")
v = stage_readiness(_pr(0), [], [], "garbage", T0, 60)
ok = (not v.ready) and v.mode == "unreadable"
failures += 0 if ok else 1
Expand Down
19 changes: 19 additions & 0 deletions src/MeshWeaver.Documentation/Data/Architecture/StagedPullRequestPipeline.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,25 @@ quarter of all Plugins heads into stage 2 before their review — exactly the sp
for the review anyway: the fallback buys an early test READING, never an earlier merge. A caller may
pass `fallback-minutes` to both lanes (they must agree), and `tests-before-review` is the per-PR override.

**Generated-only pull requests owe no review.** main's own jobs propose generated files as pull
requests — `settle-locks` (every `manifest.lock`) and `stamp-floors` (`mesh-floor.lock` plus each
root's `minMeshVersion`). There is nothing to review, and the settle job rewrites the head on every
main merge, so a per-head fallback clock restarted forever: during the 2026-10-04 review outage the
held settle PR (Plugins #2860) stopped module publishing. Such a pull request skips stage 1 on
PROVENANCE (`generated_only`): authored by the App that writes them (`meshweaver-cloud[bot]`, by id),
every commit that App's (the commit listing must cover the pull request's own commit count), every
changed file a lock or a root `index.json` whose changed lines are each NOTHING BUT a
`"minMeshVersion": "…"` key/value. A person's pull request that touches a lock is staged like any
other, a generated-only DRAFT is held as a draft, and for any pull request the App authored the
fallback clock keys on the PULL REQUEST's creation, not the head.

What this skip is NOT: a merge gate. The required `Automatic review answered` lane never consults
`generated_only`, so the skip only starts stage 2's suites earlier. Its provenance reads GitHub's
resolved commit author, which follows the author email — so it is as strong as the App's branch is
closed to other pushers (the App's commits are unsigned today, so a signature cannot be required
until the producing jobs commit through the API), and a lock's CONTENT is held exact by
`Validate node repos`, not by this check.

**Stage-0 blockers keep their own rules.** A control whose INPUT is absent (the confidential-terms
denylist secret on a fork or a Dependabot run) already *skips with a notice* by its own design and so
does not hold stage 2. A control that goes RED on infrastructure (an unreadable API, a lost runner) is
Expand Down