feat(space): enforce PR merge before completion via pr_merged hook

[lsm]

Adds a pr_merged built-in validator on the post-approval reviewer's mark_complete (approved → done), the symmetric counterpart to the existing pr_ready coder→reviewer handoff hook. It blocks task completion unless the PR is actually merged, so a task can never be closed for an unmerged PR. On a merge conflict it blocks with remediation routing the conflict back to the coder instead of completing or escalating routine coder-owned work to a human; on unknown mergeability or a GitHub rate limit it returns a retryable block.

This converts merge-conflict routing after approval from a prompt-only instruction into an enforced workflow transition. Wired into the Coding, Research, and Coding-with-QA workflows. Shared GitHub-lookup primitives (gh execution, rate-limit probing, restricted env, artifact PR resolution) are extracted from pr-ready-validator into a new gh-lookup-helpers module so both validators share one implementation; pr_ready behavior is unchanged.

Covered by 15 new pr-merged-validator unit tests plus hook-declaration assertions; all daemon shards and quality gates pass.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f3b0b3252a

ℹ️ 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]

🤖 Review by glm-5.1 (Anthropic)

Model: glm-5.1 | Client: NeoKai | Provider: Anthropic

Recommendation: REQUEST_CHANGES (2× P2, 1× P3 — see line comments)

What I verified (integration chain, end-to-end)

• pr_merged is registered in WorkflowHookValidatorId, the web editor validator list, VALID_BUILT_IN_VALIDATORS, and the pr_ready/pr_merged github-lookup permit set in buildExecutorContext.
• Wired as a mark_complete hook (sourceNode: 'Review', no targetNode) on the Coding, Research, and Fullstack-QA workflows. Hook matching correctly skips the targetNode check for non-send_message methods.
• The spawned post-approval reviewer resolves to the Review node (spawnPostApprovalSubSession picks the first node declaring the reviewer slot), and buildNodeAgentMcpServerForSession constructs a hookEngine whenever the workflow has hooks and threads it into mark_complete wrapping — so the hook fires on the correct node identity for all three workflows, including Fullstack-QA (whose postApproval lives on the QA end node but spawns a Review-node reviewer).
• Existing persisted workflows pick up the hook via the template-hash re-stamp path: hooks are in the fingerprint, drift triggers mergeHooksFromTemplate, which appends new template hooks while preserving custom ones.
• The gh-lookup-helpers extraction is clean — only createPrReadyValidator is imported externally; the moved primitives are behaviorally identical; pr_ready tests still pass.
• Block surfaces to the agent as a tool error with remediation; retryable_block surfaces for UNKNOWN/rate-limit (no auto-queue for mark_complete, correct for a terminal action). Fail-closed when no PR resolves.
• 15 new pr_merged tests + hook-declaration tests pass; typecheck + lint clean.

Findings

P2 — CLOSED PR with transient UNKNOWN mergeability loops instead of hard-blocking (pr-merged-validator.ts:89). The UNKNOWN branch precedes the CLOSED branch, so a closed PR returns retryable_block and retries for ~30s+ instead of the definitive "CLOSED without being merged — investigate" block. A closed PR is terminal; move the state === 'CLOSED' check above the mergeable === 'UNKNOWN' check (MERGED → CLOSED → UNKNOWN → conflict → open).

P2 — Shared completion instruction now mentions pr_merged/merge unconditionally (post-approval-router.ts:200). appendPostApprovalCompletionInstructions appends POST_APPROVAL_COMPLETION_INSTRUCTIONS to every post-approval spawn route, including non-PR custom routes (e.g. deployer). After this change those routes are told "A mark_complete hook (pr_merged) verifies the PR is actually merged… only call mark_complete after a successful merge," which is wrong for a non-PR route (no such hook is declared, and there is nothing to merge). Scope the PR-specific guidance to PR-based routes, or gate the pr_merged sentence on the route actually being PR-based.

P3 — Artifact PR resolution can in principle diverge from the dispatched/merged PR (pr-merged-validator.ts:153). pr_merged scans artifacts most-recent-first for prUrl/pr_url, the same strategy dispatchPostApproval uses to interpolate {{pr_url}}, and the step-6 completion artifact writes merged_pr_url (not matched) — so the standard single-PR path is consistent. A run that carries multiple different PR URLs could still validate a different PR than the one merged. Low likelihood; consider stamping/reading the exact dispatched pr_url, or document the single-PR assumption.

The core design (backstop hook on mark_complete, symmetric to pr_ready, conflict block routes back to coder) is sound and well-tested — these are localized fixes.

[lsm]

🤖 Review by glm-5.1 (Anthropic)

Model: glm-5.1 | Client: NeoKai | Provider: Anthropic

Recommendation: APPROVE — all three findings resolved, no new issues.

Verified each fix in commit 54b7a28 (+ doc 20803ce):

• P2 CLOSED-before-UNKNOWN — pr-merged-validator.ts now checks state === 'CLOSED' before mergeable === 'UNKNOWN'. Order is MERGED → CLOSED → UNKNOWN → conflict → open. A closed PR now hard-blocks regardless of transient mergeability. Covered by the new "closed PR with transient UNKNOWN mergeability → block" test.
• P2 scoped merge guidance — post-approval-router.ts splits the instruction into HEAD (generic) / conditional PR_MERGE / TAIL (generic); appendPostApprovalCompletionInstructions takes hasPrMergedHook and workflowHasPrMergedHook(workflow) detects an enabled mark_complete + pr_merged built-in hook. Non-PR routes no longer see the merge guidance; PR-merge routes still do. Router tests assert both presence (PR route) and absence (deployer route). The only production caller is the router, so no other call site regressed.
• P3 single-PR assumption — documented precisely in resolvePrUrl, noting it matches dispatchPostApproval's scan and that the step-6 audit artifact uses merged_pr_url (skipped).

Re-ran the full affected suite (pr-merged/pr-ready validators, post-approval-router, workflow-hook-engine, built-in-workflows): 437 pass / 0 fail. Typecheck + lint clean. All 6 review threads resolved. The integration chain I verified last round (validator registration, Review-node wiring across all 3 workflows, post-approval session identity, template-hash re-stamp migration, clean helper extraction) stands.

mergeStateStatus: BLOCKED is just the Codex bot not yet re-approving the new head — unrelated to code quality.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 20803ceabd

ℹ️ 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".

[chatgpt-codex-connector]

Scope merge guidance to the selected post-approval route

In a custom workflow with multiple post-approval routes, a pr_merged hook for one node makes workflowHasPrMergedHook return true for every route, so an unrelated deployer or release agent is again told that completion requires merging a PR even though its own mark_complete call will not match that hook. Fresh evidence in the revised code is that the new .some(...) scans workflow-wide and never compares the hook's sourceNode or authorized slot with the route selected by resolvePostApprovalRoute; determine this flag from the selected source node and target agent instead.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve uncapped retries when serializing pr_merged hooks

When a workflow containing this newly supported validator is opened and saved in the visual editor, serializeHook only exempts pr_ready from retry defaults, so it silently adds { maxAttempts: 3, ... } to pr_merged. After three transient UNKNOWN or rate-limit results, the hook engine converts subsequent mark_complete calls into a permanent hard block without rerunning GitHub validation, leaving the task stuck even after the PR is merged. Treat pr_merged like pr_ready in the editor's retry UI and serialization.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Gate mark_complete for every node that can invoke it

In all three updated workflows, this hook only matches the Review node, but TaskAgentManager mirrors mark_complete onto every spawned node-agent and the handler validates only that the task is currently approved. Consequently, an existing Coding/Research session—or the QA session in the Fullstack workflow—can call mark_complete after approval without matching this hook and transition the task to done while the PR remains unmerged; this is especially reachable when the post-approval reviewer reactivates the implementation node for a merge-conflict fix. Enforce the merge check for every node that exposes mark_complete, or authorize completion by the actual post-approval session rather than only by node name.

Useful? React with 👍 / 👎.