Repository navigation
fix(ci): generated-only bot PRs owe no stage-1 review (express — settle PR deadlock) - #6097
Conversation
…fall back on the PR clock Express (2026-10-04): the settle PR (Plugins #2860, ci/settle-manifest-locks) sat at stage 1 waiting for a review during the review outage, and because the settle job rewrites its head on every main merge the 120-min per-head fallback restarted each time and never fired — module publishing stopped (AI 1.21 stuck). - check-review-answered.py: `generated_only` — authored by meshweaver-cloud[bot] (by id), every commit the App's, every file a manifest.lock / mesh-floor.lock or a root index.json whose changed lines are all `minMeshVersion` → stage 1 is not owed (mode `generated`). For any App-authored PR the fallback clock keys on the PR's creation, not the head. The files/commits are read only for App PRs. Self-test: settle and floor PRs pass; a person's lock-only PR, an App PR with a hand-written file, a wider index.json change, a person's commit, a short listing and a look-alike bot all wait; an App PR falls back on the PR clock. A mutant without the commit check reds. - Doc: StagedPullRequestPipeline, "Generated-only pull requests owe no review". Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results (shard 0) 3 files 3 suites 3m 12s ⏱️ Results for commit f71aed5. ♻️ This comment has been updated with latest results. |
Test Results (shard 2) 5 files 5 suites 4m 4s ⏱️ Results for commit f71aed5. ♻️ This comment has been updated with latest results. |
Test Results (shard 1)1 753 tests 1 751 ✅ 5m 50s ⏱️ Results for commit f71aed5. ♻️ This comment has been updated with latest results. |
Test Results (shard 3)2 414 tests 2 414 ✅ 5m 16s ⏱️ Results for commit f71aed5. ♻️ This comment has been updated with latest results. |
Test Results (shard 5) 2 files 2 suites 8m 46s ⏱️ Results for commit f71aed5. ♻️ This comment has been updated with latest results. |
Test Results (shard 4) 4 files 4 suites 15m 34s ⏱️ Results for commit f71aed5. ♻️ This comment has been updated with latest results. |
Test Results 22 files 22 suites 42m 44s ⏱️ Results for commit f71aed5. ♻️ This comment has been updated with latest results. |
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Adds a 'generated' stage-1 exemption to the review-answered gate: a pull request authored by the meshweaver-cloud App (pinned id 300054957, type Bot, pinned login), every commit resolved to that App, and every changed file a manifest.lock / mesh-floor.lock at any depth or a package-root index.json whose changed lines all contain minMeshVersion, skips stage 1; for any App-authored pull request the fallback clock is re-keyed to the pull request's created_at so head rewrites can no longer restart it. The new files+commits reads (paginated, bot PRs only) are wired through run_stage_gate and both advance_one re-judges, the gate prints a dedicated success line for the new mode, an executing self-test covers the pass and the wait mutants (a person's PR, a hand-written file, a wider index.json, a person's commit, a short listing, a look-alike bot id, the PR-clock fallback), and the pipeline doc gains a section that matches the code. Checked: the gate fails closed on every unprovable input (unknown bot id or login, empty or truncated file listing vs changed_files, a non-generated file, a wider index.json, a non-App commit, files=None callers — all fall through to normal staging); _floor_only_patch treats a missing patch as not floor-only; the since re-key uses min() so it can only release sooner, never extend a hold. The diff touches only which pull requests start stage 2 (the heavy legs) without a landed review; no merge or approval behavior is in it. Could not verify from the diff: the item's stored patches contain redaction tokens ([PERSON_NAME]/[ADDRESS]) in place of several short code fragments — notably the first condition of is_generated_bot (line 659) and the since literals in the self-tests — so those expressions were reviewed structurally, not verbatim; everything outside the hunks (advance_action's handling of the new mode, the draft hold's position, STAGE_FALLBACK_MINUTES, T_EARLY, stage_readiness's full caller set); and that the claimed green self-test and Documentation.Test actually ran.
Findings: 0 blocking · 0 should-fix · 3 question · 1 nit
Internal review of 3ae6d369fdc494661a8021842aa133b6c33561f0 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| GENERATED_BASENAMES = frozenset({"manifest.lock", "mesh-floor.lock"}) | ||
|
|
||
|
|
||
| def is_generated_bot(user: dict | None) -> bool: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 answeredlane (node-repo-review-answered.yml→--pr … --wait-for-review) never callsgenerated_only; its only callers arerun_stage_gateand the twoadvance_onere-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 reposrequires everymanifest.lockto 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). Requiringverification.verifiedfirst 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.
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
|
|
||
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
systemorph-com
Bot
added
the
thread:pr-systemorph-meshweaver-6097-caafb1
… takes only a bare floor line Review of #6097: the generated release returned ready above the draft hold; the commit listing was never compared to the PR's commit count; and a changed index.json line passed whenever it merely contained "minMeshVersion". Self-test gains four cases, each red on the mutant that drops its fix. Doc states what the skip is not (a merge gate) and how far its provenance holds. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
🚰 PR babysitter (build instance) is merging the base into this branch on head Why: inherited from its base 'main': 9 pull requests of Systemorph/MeshWeaver fail identically — 'lane / Automatic review answered' concluded failure: Process completed with exit code 1. — the base 'main' moved from 2890fa1 (what the red run tested) to b0f38f7. This pull request was red because its BASE was; the base has moved since, and a re-run would test the old merge commit again. Validated: 'lane / Automatic review answered' is red on run 37286570154, a head that merged main at 2890fa1; main has since moved to b0f38f7 and its newest run is green on that check — the red is a gate the base has fixed since, not this diff's. Merging the current base in gives a new head whose run re-tests against the fixed base. It does not merge the pull request, push anything else or dequeue. A red after this is left for the owner (rbuergi). |
1 participant
Express fix. During today's review outage the settle PR (Systemorph/MeshWeaver.Plugins#2860) was held at stage 1, and because
settle-locksrewrites its head on every main merge the per-head 120-min fallback restarted each time and never fired — module publishing stopped (AI 1.21 stuck → Hosting/TriageItem broken on the public instance).check-review-answered.pygenerated_only: authored bymeshweaver-cloud[bot](id 300054957), every commit the App's, every changed file amanifest.lock/mesh-floor.lockor a rootindex.jsonwhose changed lines are allminMeshVersion→ stage 1 not owed (modegenerated). Provenance, never a branch name.index.jsonchange, a person's commit on the App's PR, a short file listing and a look-alike bot all wait; an App PR falls back on the PR clock. A mutant dropping the commit check reds.@main, so this reaches Plugins on merge; the held feat(build-project): embedded resources, with the SDK's manifest names #2860 run is then released bystage-advance.yml's sweep (or its next head).Verified:
--self-testgreen; MeshWeaver.Documentation.Test green.🤖 Generated with Claude Code