Improve merger post-approval instructions — exhaustive GitHub merge-blocker checklist - #2382
Conversation
…al [#856] When `gh pr merge` fails for a non-conflict reason, the merger now runs an exhaustive Category A-F diagnostic checklist (branch-protection/ruleset, PR state, reviews, CI/checks, permission/access, GitHub mechanics) in order before escalating to space-agent. mergeStateStatus from step 1 narrows the category, then each diagnosed blocker routes to its actor: self-fix + retry (rerun CI, switch method, use --queue), message the coder, or step h for reviewer/operator needs. The merger never assumes a "GitHub bug" or gives up silently. Step h keeps the blocked-merge escalation as the final fallback for cycle-cap exhaustion, routed human/review blockers, or undeterminable failures.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Zhipu GLM)
Model: glm-5.1 | Client: HyperNeo | Provider: Zhipu GLM
Recommendation: REQUEST_CHANGES
The checklist structure, A→F ordering, diagnose-before-escalate flow, reinforced hard rules, and tests are all solid, and the conflict loop / step-3h escalation guarantees are cleanly preserved. But three of the concrete diagnostic commands hand the merger commands that error at the shell — exactly the "looks like a GitHub bug" failure shape this checklist exists to prevent. All verified against the installed gh v2.97.0.
P1 — three diagnostic commands are invalid:
-
F1
gh pr merge {{pr_url}} --queue—gh pr mergehas no--queueflag (only--squash | --rebase | --auto | --disable-auto | --admin). For a merge-queue-required branch, the plaingh pr merge {{pr_url}} --squashalready enqueues / enables auto-merge per gh's own help;--queueerrors.⚠️ The new test also locks in this broken command verbatim (expect(text).toContain('gh pr merge {{pr_url}} --queue')) — update it to match the fix. -
B3
gh pr view {{pr_url}} --json headRef—headRefis not a validgh pr viewJSON field (Unknown JSON field: "headRef"). UseheadRefName(returns an empty string when the branch is deleted). Verified empirically against this PR. -
B4
gh pr view {{pr_url}} --json baseRef— same problem:baseRefis invalid. UsebaseRefName.
P3 — minor (not blocking, happy to defer):
- The
mergeStateStatustable lists BLOCKED/BEHIND/UNSTABLE/CLEAN/DIRTY but omits real enum valuesOUT_OF_DATE,HAS_HOOKS,UNKNOWN. An unlisted status just falls through to the full A→F sweep (still correct, just less direct); a catch-all row ("any other status → run the full checklist in order") would close the gap. NoteMergeStateStatus.DRAFTis deprecated — drafts surface asBLOCKED, and B2'sisDraftcheck is the authoritative detector (correct as-is).
Once the three commands are corrected (and the F1 test assertion updated), this is good to go. The surrounding design — mergeStateStatus narrowing, category ordering matching the task's hard rules, hard-rules reinforcement, and diagnose-before-escalate — is exactly right.
Address PR #2382 review feedback — three diagnostic commands in the checklist were invalid and would error at the shell (verified on gh v2.97.0): - B3: `--json headRef` -> `headRefName` (Unknown JSON field otherwise) - B4: `--json baseRef` -> `baseRefName` - F1: there is no `--queue` flag on the merge command; a merge-queue-required base branch is enqueued by the plain `--squash` attempt (or `--auto`) Also add a mergeStateStatus catch-all row (OUT_OF_DATE / HAS_HOOKS / UNKNOWN) and split Category E (escalate) from Category F (F1 self-fix, F2 escalate). Tests now assert the valid fields/flags and the absence of the invalid ones.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 704e9978d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Zhipu GLM)
Model: glm-5.1 | Client: HyperNeo | Provider: Zhipu GLM
Recommendation: APPROVE
All three P1s are fixed and re-verified against gh v2.97.0:
- F1
--queueremoved → plaingh pr merge {{pr_url}} --squashenqueues a merge-queue-required base branch (or--auto), with an explicit "there is NO--queueflag" note. Categories E/F cleanly split (E escalate, F1 self-fix, F2 escalate). - B3 →
--json headRefName... "is empty". - B4 →
--json baseRefName.
P3 catch-all row added (OUT_OF_DATE / HAS_HOOKS / UNKNOWN → run the per-category checks, with an OUT_OF_DATE/A10 hint).
Tests now carry both positive assertions (valid fields/flags present) and negative guards (invalid forms absent); the not.toContain('--json headRef\')guard correctly distinguishes the broken form fromheadRefName`. Changed test file 59/0 pass, storage shard 1884/1884 pass locally, lint clean, all 3 threads resolved.
CI note (verified, not just trusted): in run 31119419888 four jobs (1-core, 4-space-storage, 5-space-runtime-a, 5-space-runtime-b) failed at the GitHub-internal "Set up job" step ("Failed to resolve action download info") — a runner infra failure; no test code ran. Confirmed locally those shards are green. A fresh run (31120465435) is in progress; required-status branch protection will block the merge until CI is green.
LGTM — zero outstanding findings.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Zhipu GLM)
Model: glm-5.1 | Client: HyperNeo | Provider: Zhipu GLM
Recommendation: REQUEST_CHANGES (revising my prior APPROVE)
A third-party bot reviewer posted 9 comments. I validated each independently rather than rubber-stamping, and five are real correctness/robustness bugs — two of them contradict this repo's own code patterns. The prior round's fixes (valid gh fields/flags) stand; these are deeper issues in the diagnostic logic itself, so I'm revising my recommendation.
Should-fix (each verified by me):
-
B1
--json mergeable"false" (line 240) —mergeableis a string enum (MERGEABLE/CONFLICTING/UNKNOWN), not a boolean. I confirmed it returns"MERGEABLE", and this repo's ownpresets.ts/pr-ready-validator.tscompare it as strings (eq: ['mergeable','MERGEABLE']). "false" never matches. Fix: compare toCONFLICTING. -
A9
git log --format=%G?(line 229) — no commit range, so it scans all local history. The template's own steps a/e are meticulous about fetching the PR head and scoping tobase..head; A9 breaks that rule. In a restarted/cache-missed session (wrong branch) or with any unsigned base-history commit, this false-positives and triggers a needless force-push. Fix:git log --format=%G? origin/$BASE..$HEAD_OID(fetch both refs first, per step a). -
C1 changes-requested (lines 252-255) — the reviews API returns history; a reviewer who requested-changes-then-approved leaves a stale
CHANGES_REQUESTED. Matching any historical entry misroutes the coder on a common scenario. A2 already uses the betterreviewDecision. Fix: use the effective latest review per author, or gate onreviewDecision. -
A1 rerun on in-progress checks (lines 212-215) — the trigger is "fail or in_progress," but
gh run rerun --failed(confirmed: "Rerun only failed jobs") reruns nothing/errors on a still-running run. Fix: for in_progress/pending, wait (or use--auto); only--failed-rerun completed failures. -
Step h missing the "undeterminable" branch (line 282) — the file-level comment (lines 59-61) says step h fires on "a genuinely undeterminable failure," but the step-h text only lists human/operator blockers + cycle cap. If step g's APIs fail or no category matches, the agent has no terminating action. Fix: add "OR step g could not determine a category" to step h.
Defer (P3 — valid but lower-value):
CLEANrow (line 206): "retry the merge" can loop on a CLEAN-but-permission/method error — add "retry once, then continue to E/F if it fails the same way."- C1/D2
gh api(lines 253, 262): omit--hostname <host>— breaks GitHub Enterprise; the template extracts<host>elsewhere. - A4 code-owner (line 222): a CODEOWNERS-path change alone doesn't confirm the blocker — verify the branch requires it and an owner hasn't approved before routing.
- A14 push restrictions (line 236): unlike A12 (force-push)/A13 (deletion), base-branch push restrictions DO gate who can merge (a merge writes the branch) — don't lump them together.
The core design remains sound; these tighten the diagnostic commands to match the template's own established patterns (scoped fetch, string-enum mergeable, effective reviewDecision). Once the five should-fix items land, this is good to go.
Address chatgpt-codex-connector bot review on PR #2382 (1 P1 + 8 P2s). Every diagnostic command the checklist hands the merger must actually work at the shell and route correctly: - P1 CLEAN: mergeable (rules out A-D) is NOT "transient, just retry" — the merge already failed, so read the failure output and check Categories E/F (permissions, merge method, merge queue) before retrying. - B1 `mergeable` is the enum MERGEABLE/CONFLICTING/UNKNOWN, not a boolean (matches the repo's own pr-ready-validator.ts). - A1: pending/in_progress checks are WAITED on (or --auto), not rerun — `gh run rerun` errors on an unfinished run. - A9: signature check scoped to the base..head range, not the whole repo history (which would blame the coder for unsigned base commits). - C1: use the latest review / aggregate reviewDecision, not a stale historical CHANGES_REQUESTED entry a later APPROVED supersedes. - A4: a CODEOWNERS path match routes only if owner review is required AND unsatisfied. - A14: push restrictions CAN block a merge (merging writes to the base branch); only A12/A13 are non-blocking. - D2: pass `--hostname <host>` so GitHub Enterprise queries the right host. - Step h: undeterminable failures (no category match, or a diagnostic API error) fall through to step h instead of looping forever. Updates existing assertions and adds a locking test; 72 workflow tests pass.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Zhipu GLM)
Model: glm-5.1 | Client: HyperNeo | Provider: Zhipu GLM
Recommendation: APPROVE
Commit a54a836a resolves all five should-fix items from my prior round, verified independently — and also picks up the four P3s:
- B1
mergeablenow compared as the enumCONFLICTING("never a boolean") — consistent with this repo's ownpr-ready-validator.ts. - A9 signature check scoped to "ONLY the base..head commits" — no more whole-repo scan / false force-push.
- C1 rewritten to use
reviewDecision+ "LATEST review per reviewer" — stale supersededCHANGES_REQUESTEDno longer misroutes. - A1 now waits on in_progress/pending checks and only
--failed-reruns completed failures ("gh run rerunerrors on an unfinished run"). - Step h now explicitly covers "diagnostic APIs themselves error / NO Category A-F item matches," with a matching "go straight to step h" fallback in the step-g hard-rules paragraph.
P3s landed too: CLEAN no longer blindly retries (checks E/F first); D2 passes --hostname; A4 requires owner review to be required AND unsatisfied; A14 correctly treated as a merge blocker (merging writes the base branch) while A12/A13 stay non-blocking.
A new "review round 2" test locks these in with positive + negative assertions; changed file 60/0 pass, lint clean, all 12 review conversations resolved.
CI: the check_failed notifications on run 31120465435 were all conclusion: cancelled — the new push superseded that run and GitHub cancelled its in-progress jobs (not real failures). A fresh run on a54a836a is in progress; required-status branch protection gates the merge until green.
Zero outstanding findings. LGTM.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a54a836a88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Zhipu GLM)
Model: glm-5.1 | Client: HyperNeo | Provider: Zhipu GLM
Recommendation: REQUEST_CHANGES (revising my prior APPROVE — the bot's second pass found real issues in the round-3 fixes themselves, one of which I introduced by endorsing the scoped-%G? approach)
P1 — --auto returns without merging (destructive potential). A1's new "gh pr merge --auto" option (line 219) only enables auto-merge — gh's own help says "If required checks have not yet passed, auto-merge will be enabled," and it returns exit 0 without merging. Steps 4–6 assume a real merge, so an agent could delete the head branch, save a "merged" result artifact, and complete the task while the PR is still open (and a premature git push --delete could even break the pending auto-merge). Fix: drop --auto and say "wait for the pending check to complete, then retry step 3 (--squash)"; OR keep it but add an explicit gate: "--auto only enables auto-merge — do NOT proceed to steps 4–6 until gh pr view --json state shows MERGED." (Simplest/safest is to remove it.)
P2 — signature check uses the wrong verifier (destructive false-positive). A9's scoped git log --format=%G? is local verification — it depends on the merger machine's gpg.ssh.allowedSignersFile/keyring, not GitHub's record. An SSH-signed commit GitHub verifies can show E locally → the template tells the agent to message the coder for a needless force-push. Branch protection uses GitHub's verification. Fix: query GitHub's authoritative result, e.g. gh api repos/<owner>/<repo>/commits/<SHA> --jq '.commit.verification.verified' per PR commit (base..head), instead of local %G?. (Credit to the bot — my round-3 "scope it" fix was insufficient; local verification is the wrong tool.)
P2 — C1's condition likely never matches. C1 gates on "reviewDecision is REVIEW_REQUIRED and latest review is CHANGES_REQUESTED," but per GitHub's reviewDecision enum an active change request reports CHANGES_REQUESTED, not REVIEW_REQUIRED — the compound never matches, so an active change request falls through to undeterminable/step-h instead of routing to the coder. Fix: gate on the aggregate reviewDecision == CHANGES_REQUESTED (it already accounts for superseded reviews) — drop the REVIEW_REQUIRED clause.
P3 — A1 should restrict to required checks. Plain gh pr checks includes optional checks; an optional red check would trigger a needless rerun + coder misroute. gh pr checks --required exists ("Only show checks that are required") — use it in A1.
These are the last loose ends I'd want closed before merge; the P1 and the signature P2 have real destructive potential (premature complete / needless force-push). After these, the runbook should be in solid shape. The other categories (A2/A3/A4/A8/A10/A11/A14/B/D2/E/F1/step-h) look correct.
…#856] Round-4 review (bot + reviewer) on PR #2382 — fixes for issues found in the prior round's changes: - P1 A1 --auto: enabling auto-merge only ENABLES it and returns before the merge happens (confirmed via gh pr merge --help). A1 now says "do NOT enable auto-merge — wait for the check, then retry the step-3 merge" and gates steps 4-6 on `state` == MERGED. - P3 A1 --required: use `gh pr checks --required` so an optional red check does not trigger a needless rerun + coder misroute. - P2 C1: gate on aggregate `reviewDecision == CHANGES_REQUESTED` (not REVIEW_REQUIRED — mutually exclusive enum values), superseded only by a later APPROVED. - P2 A9: signature check now uses GitHub verification (`gh api .../commits/<SHA> --jq .commit.verification.verified`), not local `git log --format=%G?` which depends on the machine GPG/SSH allowed-signers config and can disagree with GitHub. 72 workflow tests pass.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Zhipu GLM)
Model: glm-5.1 | Client: HyperNeo | Provider: Zhipu GLM
Recommendation: APPROVE
Commit c853064e resolves all four round-4 items, verified independently — and the fixes are cleaner than the round-3 changes that triggered them:
- P1
--auto: A1 now explicitly "do NOT enable auto-merge — wait for the check, then retry the step-3 merge," plus a hard gate "Only proceed to steps 4-6 oncestateis MERGED." The premature-merge footgun is gone. - P2 A9 signatures: switched to GitHub's authoritative
gh api --hostname <host> repos/<owner>/<repo>/commits/<SHA> --jq .commit.verification.verifiedper PR commit — explicitly NOT localgit log %G?(which depends on the machine's GPG/SSH allowed-signers). Bonus: carries--hostnamefor GHES. - P2 C1: gates on the aggregate
reviewDecision == CHANGES_REQUESTED(not the mutually-exclusiveREVIEW_REQUIRED), with the superseded-by-later-APPROVED note. - P3 A1:
gh pr checks {{pr_url}} --required(excludes optional checks).
Tests updated (round-3 locking test rewritten with the new assertions); changed file 60/0 pass, lint clean, 16/16 review conversations resolved.
My independent read: these are correct and introduce no new attackable surface (the --auto option is removed rather than refined, A9 uses GitHub's own record, C1 is simplified). One minor completeness nit, not blocking: A9 doesn't spell out how to enumerate the base..head commit SHAs first (e.g. gh api repos/.../pulls/<n>/commits) — a competent agent will derive it, and the verification approach itself is right.
CI: the check_failed flood on run 31121202624 was all conclusion: cancelled (the c853064e push superseded a54a836a's run). A fresh run is pending on c853064e; required-status branch protection gates the merge.
Zero outstanding findings. LGTM.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c853064ecc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Round-5 bot review on PR #2382 — four consistency/correctness fixes: - A1: gh run rerun is Actions-only; an external-provider required check has no RUN_ID, so retrigger it via the provider (re-push or its own rerun command). - step 3g: re-fetch fresh mergeStateStatus (state + check rollup) before narrowing — the step-1 snapshot can be stale once the merge fails (base advanced / check changed), and a stale CLEAN would wrongly rule out A-D. - C2: a pending review only blocks if the required approvals are not yet met (reviewDecision != APPROVED) and that reviewer is required. - Hard rules: exempt genuinely in-flight pending checks/reviews from the "never sit and poll" rule (waiting for them is correct), with a re-check method (gh pr checks --required --watch). 72 workflow tests pass.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (HyperNeo)
Model: glm-5.1 | Client: HyperNeo | Provider: HyperNeo
Recommendation: REQUEST_CHANGES — one focused P2 fix before merge.
Follow-up to my round-5 APPROVE. The bot posted 4 new P2 comments on c853064e; I evaluated each independently against installed gh (v2.97.0) and this repo's actual CI setup. Verdict: 1 actionable P2, 1 minor P3, 2 dismissed.
P2 — Re-query mergeStateStatus after a failed merge, before narrowing (bot discussion_r3730716109) — ACTIONABLE
The step-1 mergeStateStatus is a point-in-time snapshot. Between step 1 and the step-3 merge attempt the base can advance (CLEAN→BEHIND) or a check can turn red (CLEAN→UNSTABLE). Step g then narrows categories from the STALE value. The CLEAN branch (L206-209) explicitly says it "rules out Categories A-D" and steers to "check Categories E/F" — so a stale CLEAN skips A1/D1/D2 (the very gh pr checks --required / commit-status calls that would catch a newly-failed check) and the merger escalates as "unmatched" (step h). That is a false-negative escalation that defeats the checklist, and the "read the failure output" clause scopes itself to E/F so it does not recover.
Fix is cheap and local: at the top of step g, before applying the narrowing, re-query gh pr view {{pr_url}} --json mergeStateStatus,mergeable,state and use the fresh value. The loop already re-queries after a blocker is resolved (L301-303) — mirror that before the initial narrowing. Closes the asymmetry.
P3 — Exempt in-flight checks from "never sit and poll" (bot discussion_r3730716101) — minor, optional
A1 says "wait for the check to finish, then retry"; the hard rule at L300 says "never sit and poll." They do not truly conflict (a pending in-flight check is not an actionable blocker), but one clause ("never sit and poll on an actionable blocker — an in-flight check per A1 is not actionable") removes the ambiguity. Optional; the P2 re-query gives a natural recheck point.
Dismissed — External non-Actions required checks (bot discussion_r3730716099)
Premise is technically right (gh run rerun takes an Actions run-id; an external CI check has none; gh pr checks --json exposes link/workflow but no run-id). But (a) this repo's CI is GitHub Actions only (CLAUDE.md: lint/typecheck/unit/integration are all Actions workflows), so every required check here has an extractable run-id in its link URL; and (b) the existing A1 fallback ("if still failing after 2 reruns, message the coder") is ALREADY the correct action for an external check — the merger cannot rerun external CI, only the coder/provider can. Adding provider-type branching handles a scenario that does not occur here, against the repo's "no error handling for impossible scenarios" rule.
Dismissed — C2 pending-review misroute (bot discussion_r3730716105)
A2 gates on the aggregate reviewDecision; if approvals are satisfied, reviewDecision=APPROVED and A2 does not route to step h. The bot's edge case (approvals-met + an extra unsubmitted pending review) is therefore not reachable as a Category-C blocker, and the checklist's "run the check, then take its action" framing already requires confirming an item is the actual blocker before acting. GitHub does not let an unsubmitted pending review block a merge.
Net
One real P2 (stale mergeStateStatus narrowing) with a one-line re-query fix, directly in the feature being delivered. The rest are a minor clarity nit or over-engineering for a scenario this repo does not hit. After the P2 lands I will re-verify tests and re-approve.
#856] Per reviewer Round-6 adjudication on PR #2382, revert two over-engineered items the bot suggested but the reviewer dismissed: - A1 external-provider branching: this repo CI is GitHub Actions only, so every required check has an extractable run-id; the existing "message coder" fallback is already the correct action for any other check. - C2 conditional pending-review routing: A2 already gates on the aggregate reviewDecision (approvals-met -> APPROVED -> A2 does not route), and GitHub does not let an unsubmitted pending review block a merge, so the edge case is not reachable. Retained (reviewer-endorsed): the fresh mergeStateStatus re-query at the top of step g (71a9935) and the exemption of in-flight pending checks from the "never sit and poll" hard rule (71a9935). 72 workflow tests pass.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (HyperNeo)
Model: glm-5.1 | Client: HyperNeo | Provider: HyperNeo
Recommendation: APPROVE.
Round-7 fresh review of head 1a044bb9 (verified independently, not relayed from claims).
Round-6 items — all correctly resolved:
- P2 (stale mergeStateStatus) — FIXED. Step g now re-queries
gh pr view {{pr_url}} --json state,mergeStateStatus,statusCheckRollupat the top and narrows on the fresh value, closing the stale-CLEAN false-negative that would have skipped the A1/D CI checks. ConfirmedstatusCheckRollupis a valid field (live query on this PR returned valid JSON, exit 0). - P3 (in-flight-check exemption) — FIXED. Hard-rules paragraph now has an explicit EXCEPTION clause: an IN_PROGRESS/pending check or review is not an actionable blocker — wait via
gh pr checks --required --watchor a periodic re-query.--watchis a valid flag. This resolves the A1-vs-"never sit and poll" tension. - Dismissed items correctly reverted. The external non-Actions-check branching (discussion_r3730716099) and the C2 conditional routing (discussion_r3730716105) were reverted in
1a044bb9. Net diff vs my round-5 APPROVE base shows ZERO change to A1/C2, confirming the simple forms are restored — the right call on the simplicity tradeoff for this Actions-only repo.
Verification at stable head 1a044bb9:
- Tests: 60 pass / 0 fail (257 expect() calls). (An earlier local failure was a transitional worktree artifact — HEAD advanced 71a9935→1a044bb9 mid-read — not a real failure at any commit.)
- lint, typecheck, knip, session-guards, db-schema-parity, space-task-handler-tests: all pass.
- Net PR scope: 2 files (template +132/-1, test +151) — surgical.
- Review conversations: 21 threads, 0 unresolved.
- PR: OPEN, mergeable=MERGEABLE (no conflict), targets
dev.
One out-of-scope note (NOT a blocker): bun run check exits 1 at check:test-quality, flagging provider-registry.test.ts:748 (describe('inferPersistableProviderForModel')). This is pre-existing — that file is untouched by this PR and the flagged test is present at the merge-base with dev (a37544140). It belongs in a separate PR; fixing it here would be scope creep into an unrelated test.
Clean to merge (squash, no --delete-branch). Returning to APPROVE.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a044bb977
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…klist [#856] Two clear internal-consistency bugs from the bot round-7 pass: - A2 ordering: A2 (review count) fired before C1, so a PR whose aggregate reviewDecision is CHANGES_REQUESTED routed to step h (reviewer) instead of C1 (coder work). A2 now restricts to REVIEW_REQUIRED and defers CHANGES_REQUESTED to C1. - F1: recommended --auto, contradicting A1 explicit prohibition (same enables-not-merges hazard). F1 now says do NOT add --auto; wait for state == MERGED. The other round-7 bot items (changed-head re-review P1, external-CI re-raise, C2/exemption, headRefName, D2 status filter) are deferred to the reviewer adjudication — several re-raise Round-6-dismissed items or are conditional edge cases, and the bot contradicted its own round-2 headRefName advice. 72 workflow tests pass.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (HyperNeo)
Model: glm-5.1 | Client: HyperNeo | Provider: HyperNeo
Recommendation: REQUEST_CHANGES — withdrawing my round-7 APPROVE.
I submitted this for approval at round 7, but the bot's new P1 (plus several P2s) on head 1a044bb9 are real and I missed them. Independent verification below. The P1 is a merge-integrity gap, so the approval must not proceed until it is fixed.
P1 — Re-review a changed head before retrying the merge (bot r3730801484) — BLOCKING
Confirmed asymmetry in the current head:
- Conflict path (step e, L160-190): after a coder force-push the merger re-runs step 1/2, diffs
approved_head_oid..current_head, inspects the FULL delta, and posts a fresh APPROVED/COMMENT before re-merge — "the approval no longer covers the new commits ... re-approve on every retry." - Non-conflict path (hard rules, L308-310): "After a routed blocker is resolved (coder pushes...), re-run the step 1 and step 2 checks, then retry the merge." — NO head-delta inspection, NO re-approval.
The non-conflict blockers A9 (sign + force-push), A10 (rebase + force-push), B3 (recreate branch), D1 (fix workflow + push), D4 (push to retrigger) are ALL resolved by a coder push that changes the head OID. A10 (rebase) is a very common path. In a repo without "dismiss stale approvals on push" — and this template explicitly targets arbitrary repos — the merger merges rebased/force-pushed code the reviewer never saw, covered only by the stale approval. That is exactly the failure step e exists to prevent.
Fix: make the hard-rules resolution path mirror step e for ANY coder push that changes the head — capture the approved head OID; after the coder pushes, compare OIDs; if changed, inspect the full delta and post a fresh review before retrying the merge. Conflict fixes already do this; non-conflict fixes must too.
P2 — F1 --auto contradicts A1 (bot r3730801488)
F1 (L297-299) recommends "add --auto for auto-merge," but A1 (L220-224) explicitly prohibits it: "--auto only ENABLES auto-merge and returns before the merge actually happens ... proceed only once state is MERGED." Verified gh pr merge --help: --auto = "Automatically merge only after necessary requirements are met" — i.e. it returns before the merge and the PR can stay open (async merge outside session control). --auto was deliberately dropped from A1 in round 4 for this footgun; F1 reintroduces it. Drop --auto from F1; for a merge queue, retry the plain merge and verify state == MERGED (same as A1).
P2 — A2 misroutes CHANGES_REQUESTED past C1 (bot r3730801510)
A2 (L229-231) fires on "required approving-review count not met" → step h. But reviewDecision=CHANGES_REQUESTED also satisfies "count not met," so the ordered checklist hits A2 → step h (engage reviewer) BEFORE reaching C1, which correctly routes CHANGES_REQUESTED to the CODER. Engaging the reviewer first is wrong — they already reviewed (changes requested); the coder must act first. Restrict A2 to reviewDecision=REVIEW_REQUIRED and let CHANGES_REQUESTED fall through to C1.
P2 — C2 contradicts the EXCEPTION clause (bot r3730801494) — partly my round-7 miss
C2 (L281-282) routes every pending review to step h, but the round-7 EXCEPTION (L305-308) says a pending review is NOT actionable — "wait ... instead of messaging or escalating." Direct contradiction. I introduced this tension by adding the EXCEPTION; the revert of the C2 conditional in 1a044bb9 left them at odds. Resolve one of two ways: scope the EXCEPTION to CHECKS only (a pending review that is required to meet the approval count IS actionable → C2 stands for reviews), or gate C2 on "approvals not met AND that reviewer required." I lean toward scoping the EXCEPTION to checks.
P3 — B3 tests the ref NAME, not existence (bot r3730801501)
headRefName is the recorded branch name (string metadata), not a live-ref check — it stays populated after the branch/fork is deleted, so B3's "headRefName is empty" test may never match. Verify the head ref actually resolves (the headRef object goes null on deletion, or query the head repo's git ref). Rare path, safe degradation (escalate), but the diagnostic is unreliable. Minor.
P3 — D2 should filter to required status contexts (bot r3730801508)
/commits/<SHA>/status aggregates ALL legacy status contexts; an OPTIONAL failing legacy status flips the aggregate to failure and D2 misroutes to the coder. This is the same class of issue as the A1 --required filter added in round 4 — compare individual statuses against the contexts required by branch protection before acting. Minor (legacy statuses are rare in Actions-only repos, but the template targets arbitrary repos).
Dismissed — External non-Actions checks (bot r3730801492, re-raise)
Same item dismissed in round 6. The "template targets arbitrary repos" point is fair, but the existing A1 fallback ("if still failing after 2 attempts, message the coder") is ALREADY the correct terminal action for an external check — the merger cannot rerun external CI, only the coder/provider can. The worst case is 1-2 wasted gh run rerun attempts before that fallback fires; no incorrect merge. Adding provider-type branching for a scenario that degrades safely is over-engineering against the repo's simplicity rule. Not blocking.
Net
One blocking P1 (head re-review on non-conflict coder pushes — merge integrity) and three real P2s (F1 --auto, A2/C1 ordering, C2/EXCEPTION contradiction) to fix before merge; two minor P3s; one re-raise dismissed. Withdrawing the round-7 approval request until at least the P1 and the P2s land.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (HyperNeo)
Model: glm-5.1 | Client: HyperNeo | Provider: HyperNeo
Recommendation: REQUEST_CHANGES — round-9. Two items remain (1 P1 + 1 P2); then I approve.
Verified at head 22e6df0be: tests 60/0 (259 expect), PR OPEN + MERGEABLE.
Confirmed fixed in 22e6df0be (verified):
- F1
--auto: removed; now "Do NOT add--auto... wait forstate== MERGED." ✓ - A2/C1 ordering: A2 gates on
reviewDecision == REVIEW_REQUIREDand routesCHANGES_REQUESTEDto C1 ("skip to C1"). ✓
Still open — P1 (head re-review). My decision: IMPLEMENT.
The coder deferred this to me; my call is to implement it. The template targets arbitrary repos (cannot assume "dismiss stale approvals on push," which is opt-in — and the safety argument also requires required reviews to be enabled; without either, the gap is fully exposed). A10 rebase is a common head-changing path. And the conflict path (step e) already enforces head-delta + fresh review for head-changing pushes — the non-conflict path must be consistent. Fix (low cost): in the hard-rules resolution clause, after a coder push that changes the head OID, apply step-e's head-delta inspection and post a fresh review before retrying — the approval only covers the head it was given. (Decision also posted on the thread.)
Still open — P2 (C2/EXCEPTION contradiction). Must fix.
Not addressed in 22e6df0be. C2 (L281-282) routes pending reviews to step h, but the round-7 EXCEPTION (L305-308) says a pending review is not actionable (wait, don't escalate) — a direct contradiction I introduced. Minimal correct fix: scope the EXCEPTION to CHECKS only ("a check," not "a check or review"). A pending check is never actionable (wait); a pending review needed to satisfy the approval count IS actionable (C2 → step h). The two clauses then no longer overlap. One-word change.
Optional (will NOT block): B3 (verify the head ref resolves, not just headRefName); D2 (filter legacy /commits/<SHA>/status to required contexts, parallel to A1 --required). Nice-to-haves for a later hardening pass.
Dismissed: external non-Actions checks (re-raise; the "message the coder" fallback is already the correct terminal action).
Convergence: once the P1 and the C2/EXCEPTION fix land, I will re-verify and APPROVE. I will not block on the two P3s.
… R8/R9) [#856] Address the reviewer Round-8/9 blocking items on PR #2382: - P1: the non-conflict resolution path now mirrors conflict-loop step e for any coder push that changes the head. After a routed blocker is resolved, if the current headRefOid differs from the one step 1 captured, the merger inspects the full delta vs the approved head and posts a FRESH review before retrying (no merging unreviewed rebases/signature-fixes in repos without auto-dismiss-stale-approvals). - P2 C2/EXCEPTION: scoped the "never sit and poll" EXCEPTION to CHECKS only ("a CHECK ... IN_PROGRESS / pending"); a pending review remains actionable via C2. Removes the contradiction. - P3 B3: tests the head ref itself (git/refs/heads/<headRefName> 404s on deletion) instead of the headRefName string, which stays populated. - P3 D2: notes the /commits/<SHA>/status aggregate includes optional contexts; confirm the failing status is required before routing. (F1 --auto and A2/C1 ordering were fixed earlier at 22e6df0; external non-Actions checks remain dismissed.) 72 workflow tests pass.
…ws [#856] Per the reviewer refined preference on the C2 thread (R9): dropping "pending review in progress" from C2 escalation is strictly better than scoping the EXCEPTION to checks (option a left the misroute of escalating an extra pending review when approvals are already met). Now: - C2 pending review in progress is NOT escalated — the EXCEPTION wait covers it (an extra pending review beyond a satisfied requirement does not block). C3 (required team review) / C4 (dismissed request) escalate. - The EXCEPTION again covers both checks and reviews ("a check or review"). P1 (changed-head re-review) was implemented at 0c3a852. 72 tests pass.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (HyperNeo)
Model: glm-5.1 | Client: HyperNeo | Provider: z.ai
Round-10 re-verify at f6e69ed67. C2/EXCEPTION is resolved exactly as agreed — the EXCEPTION now covers checks and reviews (wait, don't escalate), C2 defers a pending review to it (no longer escalates an extra pending review when approvals are met), and C3/C4 still escalate. A2/C1 ordering, F1 --auto, B3 ref-existence, and the D2 required-context caveat all landed cleanly. 60/0 tests (264 expect), 0 unresolved threads.
One item remains — P1, the changed-head guard has no baseline (can't fire).
The guard (L321-325) says: if the coder PUSHED … "the current headRefOid differs from the one step 1 captured" … "inspect the FULL delta vs the approved head." But:
- Step 1 (L76) captures only
state,mergeStateStatus,statusCheckRollup+baseRefName— it does not captureheadRefOid. The onlyheadRefOidcaptures (L113, L170) live inside the conflict path (steps a/e), which this non-conflict path never runs. So "the one step 1 captured" points at a capture that doesn't exist. - A transient capture wouldn't persist anyway: the template itself documents (L82-85) that each Bash call is a fresh shell where
$BASEmust be re-derived every call. The conflict path works only because step b persistsapproved_head_oidin an artifact — which the non-conflict path never creates.
Net: the guard references a baseline that doesn't exist, so it cannot detect a changed head, so it cannot prevent merging on a stale approval — which is the entire point of the P1 fix.
Fix — one clause, reusing an existing fetch. Establish the approved head from the latest APPROVED/COMMENTED review's commit_id (the head at approval time, recoverable anytime). The template already fetches reviews in step c (gh api --hostname <host> repos/<owner>/<repo>/pulls/<number>/reviews); each review carries commit_id. Verified live on this PR: reviews carry commit_id (e.g. f53a54413…), distinct from the current head f6e69ed67… — so current headRefOid != latest review commit_id correctly flags a coder push, across fresh shells, no transient capture needed.
Suggested wording for L321-325:
BUT if the coder PUSHED to resolve it (A9/A10/B3/D1/D4), compare the current
headRefOidto the APPROVED HEAD — thecommit_idof the latest APPROVED or COMMENTED review (fetch via the step-c review API; it is the head at approval time and survives across fresh shells, unlike a step-1 shell variable). If they differ, do NOT merge on the stale approval … mirror conflict-loop step e —git diff "<approved commit_id>".."$CUR_HEAD"— and post a FRESH review before retrying.
This is the last item. Once it lands I re-verify and APPROVE — I'm not blocking on anything else (B3/D2 already done; external-CI dismissed).
Recommendation: REQUEST_CHANGES — round-10. One P1 remains (changed-head guard baseline); C2/EXCEPTION and every other item are resolved.
Bot P1: the changed-head guard compared the current headRefOid to "the one step 1 captured", but step 1 captures no headRefOid (only state/ mergeStateStatus/statusCheckRollup/baseRefName) and a transient capture would not survive the fresh shell of a later Bash call, so the baseline was undefined and the guard could not fire. The guard now recovers the APPROVED head = the commit_id of the latest APPROVED/COMMENTED review (via the paginated reviews API) — the head at approval time, durable across fresh shells. 72 workflow tests pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdc8b8d1b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f63a8e8583
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…icate merger) [#856] The post-approval merger sub-session (created by spawnPostApprovalSubSession) has no node_execution row, so it was invisible to ALL message-delivery paths (AgentMessageRouter peers, ChannelRouter.activateNode). A Reviewer reply over Review→Post-Approval would lazy-activate a DUPLICATE merger session. Fix: AgentMessageRouter now accepts a findPostApprovalSessionId callback. When building the peers list, if the task has a live postApprovalSessionId and an agent declared in nodeGroups that has no node_execution (the merger), the live session is added to peers — so messages route to the existing session instead of activating a new one. Wired in TaskAgentManager.buildNodeAgentMcpServerForSession via taskRepo lookup.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03cdc06e8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…channels + QA prompt [#856]
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 273704b884
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5bfc21a0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…856] Addresses the post-approval redesign review round (3 inline comments) and completes the duplicate-merger-session fix. 1. Approval authority is now derived, not hard-coded. The merger template addressed blockers to "Review" by default, but in the Fullstack workflow QA is the approval authority and BOTH Review and QA are reachable — so a blocker could misroute to Review. dispatchPostApproval now derives {{approval_authority}} from the approving/submitting node (Review for Coding/Research, QA for Fullstack) and the merge template addresses it directly. The blocker report message is also self-instructing (it tells the authority to re-approve the current head and reply), so it works even for a Space whose authority prompt predates the redesign paragraph. 2. Backfill the post-approval re-approval paragraph for existing Spaces. Existing template-linked workflows retain the pre-redesign QA/Reviewer prompt (no paragraph). The appended paragraph is now a retired-prompt variant so mergeNodeStructuralFieldsFromTemplate restores it on re-stamp. 3. Liveness-gate the merger session in delivery paths. AgentMessageRouter.findPostApprovalSessionId returns the id only when isSessionAlive confirms it, so a merger that died mid-wait is not treated as a live peer (which would inject the continue message into a dead session). 4. Complete the duplicate-merger fix (#11). ChannelRouter.deliverMessage knew about the live merger only via AgentMessageRouter's peers list, but its own lazy-activateNode step still created a pending node_execution for the Post-Approval node, so the tick loop spawned a DUPLICATE merger. ChannelRouter now skips activateNode when the target node declares a post-approval route and a live merger session exists.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14cb60c86c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…uting [#856] Second review round on the post-approval redesign. - Scope the live merger session to its target agent only. The peers fix mapped the merger session to every declared-but-unactivated agent, so a message to an unrelated inactive node (e.g. an unused branch) would be injected into the merger. AgentMessageRouter now maps it only under the post-approval route's targetAgent (resolved via findPostApprovalTargetAgentName); covered by a new over-matching test. - Bind the INITIAL merge to the approved head. The first attempt did not verify the approval covered the current head or pass --match-head-commit, so a push that changed the head after approval could merge an unreviewed head. Step 3 now captures headRefOid, confirms the latest review covers it, and binds the merge with --match-head-commit (matching the retry path). - Require "Recommendation: APPROVE" in the COMMENTED own-PR fallback. GitHub stores both APPROVE and REQUEST_CHANGES from the PR owner as COMMENTED; without the marker a changes-requested review or plain comment could be accepted as approval and a rejected head merged. Both step 3 and 3d now require the marker. - Unresolvable-blocker reply. The reviewer/QA prompts now instruct replying with reason "unresolvable" for administrative blockers (merge permission, ruleset), and the merger's step 3c escalates to space-agent on that signal instead of waiting indefinitely. - QA own-PR COMMENTED fallback. QA has no post_review tool, so the QA prompt now prescribes the accepted COMMENTED fallback via a direct reviews API call.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd23cb661b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Third review round.
- Register post_review for QA (not just the Reviewer). QA is now a review-capable
approval authority, so it re-approves via the same centralized tool the Reviewer
uses, which posts the marked review (own-PR COMMENTED fallback with the
"Recommendation: APPROVE" body marker, and the PR URL host). This replaces the
QA prompt's hand-rolled reviews-API fallback, which (a) omitted the
"Recommendation: APPROVE" marker the merger requires and would have been
rejected, and (b) did not pass the hostname, breaking GitHub Enterprise PRs.
- Non-lazy (in-memory) liveness for the post-approval merger session in both
delivery paths. The lazy isSessionAlive can report a persisted-but-unrehydrated
merger as alive after a daemon restart (the merger has no NodeExecution to
rehydrate from); treating it as live would skip activation and crash
injectSubSessionMessage ("Sub-session not found"). Both the AgentMessageRouter
callback and ChannelRouter.resolveLivePostApprovalSession now prefer an
in-memory probe that returns true only when the session is actually registered.
The dead-merger channel-router test now proves the in-memory probe overrides
the lazy false-positive.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e3f569772
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…iew guidance [#856] Fourth review round. - PR_MERGER_SLOT_PROMPT is behavioural-only: it no longer hard-codes "Reviewer" as the blocker authority. The merge procedure (first user turn) carries the {{approval_authority}} token; a slot prompt naming "Reviewer" is higher priority and would override it, so in the Fullstack workflow the Merger could route a blocker to Review instead of QA. The slot prompt now says "the approval authority" and defers to the Runtime Execution Contract. - Move the QA post-approval paragraph to AFTER the QA slot procedure (it was embedded in FULLSTACK_QA_PROMPT, before the steps that end with "call approve_task()"). On a blocker resume the green-path "call approve_task as your final action" step could otherwise lead QA to re-approve the already-approved task and stop without signalling the Merger. Appended after the steps, the paragraph is the final applicable behaviour and explicitly overrides that step for blocker cycles. - Correct the own-PR re-approval guidance in the QA and Reviewer paragraphs: request APPROVE via post_review (the tool auto-retries as a marked COMMENT review carrying "Recommendation: APPROVE" on an own-PR). The prior wording told the agent to request COMMENT directly, which lands an UNMARKED comment the Merger rejects. New regression tests: merger slot prompt is behavioural-only (no "Reviewer"); QA paragraph placement after the approve_task step; QA requests APPROVE not COMMENT.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07ac26fe74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…de routes) [#856] Fifth review round. The duplicate-merger skip guard in ChannelRouter.deliverMessage checked targetNode.postApproval — whether the TARGET node declares a post-approval route. But post-approval-validator allows a route declared on one node to target an agent in ANOTHER node. For such cross-node routes the guard failed both ways: the real target node (no declaration) was activated, letting the tick loop spawn a duplicate merger; and a message to the declaring node (not the target) would incorrectly skip activation. The guard now keys on the dispatched route's target agent: it collects every post-approval targetAgent (node-level routes plus the legacy workflow-level route) and skips activation only when the target node CONTAINS one of those agents and a live merger session exists. The AgentMessageRouter findPostApprovalTargetAgentName callback now also falls back to the legacy workflow-level route. New cross-node test: route declared on the Review node, target agent in the Post-Approval node, live session present — activation is correctly skipped for the Post-Approval node (which does not itself declare the route).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aefd242fe4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sk-agent route [#856] Sixth review round (2 items). - Prefer the recorded live merger over stale executions. The over-matching guard refused to map the live postApprovalSessionId if ANY merger node_execution existed, so a stale execution (a terminal row with an old session, or a pending row left by the pre-fix duplicate-activation path) shadowed the live merger — the approval authority's continuation reached the stale execution instead of the waiting merger and the loop stalled. AgentMessageRouter now keeps the live merger as the delivery target for its agent unless an execution already represents that exact session, dropping same-agent peers pointing at a different session. New test: stale merger execution + live session → message reaches the live session. - Resolve the same dispatched route. findPostApprovalTargetAgentName and ChannelRouter.getPostApprovalTargetAgents returned the first declaration unconditionally, but PostApprovalRouter skips the legacy 'task-agent' target and dispatches the first VALID route. For a workflow with a task-agent route before a merger route, the live session was registered under 'task-agent' instead of the dispatched merger agent. Both now mirror the router's selection (skip 'task-agent', nodes-then-legacy order, first valid). New channel-router test: a task-agent-only route does not skip activation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcd399bd04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ep [#856] Seventh review round (1 item). Step 3e (cycle cap / escalation) hard-coded "Post-Approval ↔ Review loop". In the Fullstack workflow the blocker loop is Post-Approval ↔ QA while a separate Review channel also exists; if they have different maxCycles, the Merger would inspect the wrong budget and wait or escalate on an unrelated route. Use {{approval_authority}} (as the blocker handoff in 3b already does) and tell the Merger to read the Post-Approval ↔ {{approval_authority}} budget specifically.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6b08fe488
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…review for authority slots [#856] Eighth review round (2 items). - Migration 171 now remaps backfilled channel endpoints to the persisted node names via their stable agent slots ('merger' -> Post-Approval, 'reviewer' -> Review, 'qa' -> QA), querying space_workflow_nodes. A template-linked built-in with renamed nodes previously got unusable channels (canonical literals naming no node); now they resolve to the renamed names, matching mergeChannelsFromTemplate. New test: renamed nodes -> channels use the renamed endpoints. - post_review is now granted based on the slot being the workflow's approval authority (the end node), not just the agent's reviewer/qa preset. A customized agent assigned to the Fullstack qa slot (different handle) still gets the tool its slot prompt tells it to use; the Post-Approval/merger node never qualifies.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d903428c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…al schema) [#856] Ninth review round. Migration 171 eagerly prepared its space_workflow_nodes lookup statement, so on a partial schema that has the full space_workflows columns but lacks space_workflow_nodes, SQLite threw "no such table" and aborted the migration (and daemon startup) instead of taking the documented fallback path. The statement is now prepared only when the table exists; the canonical-name fallback applies otherwise. New test drops space_workflow_nodes and asserts the migration is a no-op-with-fallback rather than throwing. (The CI migrations-b timeouts on the prior commit were unrelated — the shard passes locally in ~40s; they were CI-load flakes.)
…2) [#851] Adds a nullable space_tasks.post_approval_source_node_id column (migration 172 — 171 was taken by the Post-Approval ↔ Review channel backfill in #2382) plus type / repository mapping and the test-DB schema entry. Migration 172 also backfills the column from pending_completion_submitted_by_node_id for in-flight review / crash-stranded approved rows, so an upgrading database does not lose the source node when the pending field is later cleared on approval.
1 participant
Closes #856.
Redesigns the post-approval merge flow: the Merger's only job is to merge. On any merge failure it reports the blockers to the Reviewer (the re-approval authority) and waits; the Reviewer re-checks, coordinates the coder, re-approves the (possibly changed) head on GitHub, and signals the Merger to continue; the Merger re-verifies and retries. This replaces the earlier Category A–F self-diagnosis + conflict loop (the merger never self-diagnoses or self-approves), resolving the never-approve-vs-never-merge-unreviewed tension by making the Reviewer the re-approval authority.
Adds Post-Approval ↔ Review channels to the Coding/Research/Coding-with-QA workflows (+ migration 171 to backfill existing Spaces), extends the reviewer prompts with post-approval blocker handling, and rewrites the merge template (~424 → ~150 lines). The Fullstack workflow's approval authority is QA, so its post-approval re-approval routing is left for a follow-up.