Branch-reconcile finishes a review branch whose run died before opening a PR

server/services/README.md

@@ -23,7 +23,7 @@ image-generation paths below are useful entry points when tracing render jobs.
 | `cosClaimOwnershipState.js` | Shared ownership lock, per-checkout retirement fence with `retireWithUnlockedArchive` (archives run between two lock sections), and fail-closed live-agent reader used by claim binding and worktree cleanup. Re-exported by `cosClaimOwnership.js` for existing callers; does not import reconciliation. |
 | `systemActivityNotify.js` | Coalesced activity invalidation signal and LLM lifecycle adapters; producers do not import media execution. |
 | `systemActivityWatchers.js` | Socket initialization arms media and CoS activity subscriptions here, sharing the notification signal without pulling boot observers into producers. |
-| `branchCompletionPolicy.js` | Retains explicit registered PR completion policy through agent archives. `readBranchCompletionHold` gates reconciliation by exact repository/branch/PR identity; human handoffs and configured review stay held, explicit releases require a fresh matching forge head, and verified merged retirement is a separate action. |
+| `branchCompletionPolicy.js` | Retains explicit registered PR completion policy through agent archives. `readBranchCompletionHold` gates reconciliation by exact repository/branch/PR identity (`readBranchCompletionHoldDetail` also flags a review obligation whose run never opened a PR, so branch-reconcile still dispatches that unshipped work); human handoffs and configured review stay held, explicit releases require a fresh matching forge head, and verified merged retirement is a separate action. |
 | `databaseCutoverHandshake.js` | Narrow fenced-boot path for the ordinary server: at `verifying`/`verified` prove this process's own pool reaches the recorded target, publish a per-pid proof, and wait for admission release before the app graph loads. |
 | `databaseMaintenanceCutover.js` | Internal cutover worker body (transfer → forward-only `.env` mode commit → server restart with target proof → release → CoS restart) and `recoverDatabaseCutover(id)` same-operation recovery for the CLI/API. |
 | `databaseMaintenanceProducers.js` | Internal one-use coordinator stage: retain producer identity, stop this install's PM2 CoS/server, and verify fresh readback while admission remains fenced. |

server/services/branchCompletionPolicy.js

@@ -686,6 +686,14 @@ export function mergedReceiptRetirementPolicy(input) {
 }
 
 export async function readBranchCompletionHold(input, deps = {}) {
+  return (await readBranchCompletionHoldDetail(input, deps)).reason;
+}
+
+/** `readBranchCompletionHold` plus `unshipped`: a review-then-merge obligation
+ * whose run ended before any PR was recorded for it (no open PR either). The
+ * obligation gates the eventual merge, not the work: branch-reconcile may hand
+ * such a branch to a coordinator to open its PR through the review workflow. */
+export async function readBranchCompletionHoldDetail(input, deps = {}) {
   const readPolicies = deps.readPolicies || readBranchCompletionPolicies;
   const agents = input.agents ?? await readPolicies().catch(() => null);
   const { execGit } = await import('../lib/execGit.js');
@@ -696,12 +704,17 @@ export async function readBranchCompletionHold(input, deps = {}) {
   // fetched. Only a potential explicit release needs a fresh forge read; offline
   // cleanup preserves the handoff when that read cannot establish the same head.
   if (input.retirement === true && decision.obligation?.prUrl
-    && await verifiedPolicyTip(input.repoPath, input.branch, decision.obligation.prUrl, { requireMerged: true, expectedTip: input.tip }).catch(() => null) === tip) return null;
-  if (decision.release && await verifiedPolicyTip(input.repoPath, input.branch, decision.release.prUrl).catch(() => null) === tip) return null;
-  if (input.retirement === true && await isVacuousReviewObligation(input, decision, tip)) return null;
-  return decision.reason;
+    && await verifiedPolicyTip(input.repoPath, input.branch, decision.obligation.prUrl, { requireMerged: true, expectedTip: input.tip }).catch(() => null) === tip) return { reason: null };
+  if (decision.release && await verifiedPolicyTip(input.repoPath, input.branch, decision.release.prUrl).catch(() => null) === tip) return { reason: null };
+  if (input.retirement === true && await isVacuousReviewObligation(input, decision, tip)) return { reason: null };
+  return { reason: decision.reason ?? null, unshipped: isUnshippedReviewObligation(input, decision) };
 }
 
+// No PR was ever opened for this review-then-merge obligation: nothing to
+// review yet, and no leave-open handoff a human must answer.
+const isUnshippedReviewObligation = (input, decision) => decision.reason === 'pr-completion-review-required' && !input.prUrl
+  && !decision.policies.some(policy => policy.completion === PR_COMPLETIONS.LEAVE_OPEN || retirementPr(policy));
+
 /** A review-then-merge run that never opened a PR and left no commit the default
  * branch lacks has nothing to review or merge, and no later receipt can name a
  * PR to release it. Strict ancestry, not patch equivalence: a squash- or
@@ -710,9 +723,7 @@ export async function readBranchCompletionHold(input, deps = {}) {
  * a recorded PR, unmerged commits, an unresolvable default branch) is kept.
  */
 async function isVacuousReviewObligation(input, decision, tip) {
-  if (decision.reason !== 'pr-completion-review-required' || input.prUrl
-    || !/^[a-f0-9]{40,64}$/i.test(tip || '')
-    || decision.policies.some(policy => policy.completion === PR_COMPLETIONS.LEAVE_OPEN || retirementPr(policy))) return false;
+  if (!isUnshippedReviewObligation(input, decision) || !/^[a-f0-9]{40,64}$/i.test(tip || '')) return false;
   const { getDefaultBranch } = await import('./git.js');
   const base = await getDefaultBranch(input.repoPath, { strict: true }).catch(() => null);
   if (!base) return false;

server/services/branchCompletionPolicy.test.js

@@ -179,6 +179,18 @@ describe('completion policy across lifecycle and reconciliation boundaries', ()
       expect(await retire()).toBe('pr-completion-review-required');
     });
 
+    // A claim run killed after committing but before pushing left a review
+    // obligation with no PR. The hold must not hide the unshipped work from the
+    // coordinator; review still gates the merge once the PR exists.
+    it('dispatches an unshipped review branch as NEEDS_PR, and holds it again once a PR is recorded', async () => {
+      register('agent-claim', 'review-then-merge');
+      runtime.prState = 'CLOSED';
+      const gathered = async () => classifyBranches(await gatherBranchState(repoPath, { defaultBranch: 'main', claimOwners: { agents: [] } }))[0];
+      expect(await gathered()).toMatchObject({ state: 'NEEDS_PR', completionPolicyReason: 'pr-completion-review-required', completionReviewUnshipped: true });
+      runtime.agents['agent-claim'].metadata.branchCompletionPolicy = { branch, prUrl, completion: 'review-then-merge', tip: null };
+      expect(await gathered()).toMatchObject({ state: 'WIP', rule: 'completion-policy', completionReviewUnshipped: false });
+    });
+
     it('keeps a leave-open handoff and a recorded PR held even when the tip is merged', async () => {
       runtime.merged = true;
       const handoff = register('agent-human', 'leave-open');

server/services/branchReconcile.js

@@ -156,6 +156,10 @@ const PR_LIST_LIMIT = 200;
  *   completion-policy  WIP            policy hold. Sits BEFORE merged so a merged pointer is not
  *                                     deleted by cleanupMerged without the review proof that
  *                                     prepareReconcileRetirements requires (it scans these rows).
+ *                                     Except an UNSHIPPED review obligation (no PR ever recorded,
+ *                                     none open, commits not on the default branch): the run died
+ *                                     before delivery, so the hold falls through to the rules below
+ *                                     and normally lands NEEDS_PR. Review still gates the merge.
  *   abandoned-wip      ABANDONED_WIP  dirty tree of an INACTIVE run, no live owner → agent finishes it.
  *                                     Before merged: a dead agent that never committed leaves its
  *                                     pointer on an old default-branch commit, which reads as MERGED
@@ -181,7 +185,8 @@ const PR_LIST_LIMIT = 200;
  */
 const CLASSIFY_RULES = [
   { id: 'draft', state: 'WIP', when: ({ openPr }) => Boolean(openPr?.isDraft) },
-  { id: 'completion-policy', state: 'WIP', when: ({ completionPolicyReason }) => Boolean(completionPolicyReason) },
+  { id: 'completion-policy', state: 'WIP', when: ({ completionPolicyReason, completionReviewUnshipped, isMerged, openPr }) => Boolean(completionPolicyReason)
+    && !(completionReviewUnshipped && !isMerged && !openPr) },
   { id: 'abandoned-wip', state: 'ABANDONED_WIP', when: ({ worktreeDirty, liveOwnerReason, abandonedAgentWorktree, abandonedClaimWorktree }) => Boolean(worktreeDirty && !liveOwnerReason && (abandonedAgentWorktree || abandonedClaimWorktree)) },
   { id: 'merged', state: 'MERGED', when: ({ isMerged }) => Boolean(isMerged) },
   { id: 'live-owner', state: 'WIP', when: ({ liveOwnerReason }) => Boolean(liveOwnerReason) },
@@ -205,7 +210,7 @@ const CLASSIFY_RULES = [
  *                dirty tree, unreadable forge, or bare pointer. It is NOT "never touch":
  *                completion-policy rows are still scanned for review-gated retirement.
  *
- * @param {{ hasUpstream:boolean, ahead?:number|null, hasOrigin?:boolean, isMerged:boolean, worktreeDirty:boolean, abandonedAgentWorktree?:boolean, abandonedClaimWorktree?:boolean, liveOwnerReason?:string|null, completionPolicyReason?:string|null, openPr:({mergeable?:string, isDraft?:boolean}|null), prStateUnavailable?:boolean }} input
+ * @param {{ hasUpstream:boolean, ahead?:number|null, hasOrigin?:boolean, isMerged:boolean, worktreeDirty:boolean, abandonedAgentWorktree?:boolean, abandonedClaimWorktree?:boolean, liveOwnerReason?:string|null, completionPolicyReason?:string|null, completionReviewUnshipped?:boolean, openPr:({mergeable?:string, isDraft?:boolean}|null), prStateUnavailable?:boolean }} input
  *   `ahead` is the branch's own commit count over the default branch (null =
  *   unreadable), and `hasOrigin` says the repo has a remote to push that work to.
  *   `hasOrigin` defaults to FALSE (fail-closed): a caller that cannot vouch for a
@@ -956,7 +961,7 @@ export async function gatherBranchState(repoPath, { defaultBranch, activeAgentId
     (b) => !b.isDefault && !b.current && !protectedSet.has(b.name)
   );
 
-  const { readBranchCompletionPolicies, readBranchCompletionHold } = await import('./branchCompletionPolicy.js');
+  const { readBranchCompletionPolicies, readBranchCompletionHoldDetail } = await import('./branchCompletionPolicy.js');
   const completionPolicies = candidates.length ? await readBranchCompletionPolicies().catch(() => null) : [];
 
   const inputs = [];
@@ -987,11 +992,13 @@ export async function gatherBranchState(repoPath, { defaultBranch, activeAgentId
     // -circuit when the cheap check already proved it merged.
     const hasMergeEvidence = b.merged || await hasBranchMergeEvidence(repoPath, b.name, defaultBranch);
     const claimOwnerReason = resolveClaimOwnerReason({ branch: b.name, path: worktreePath, claimOwners, sourceWorkspace: repoPath });
-    const completionPolicyReason = await readBranchCompletionHold({ repoPath, branch: b.name, prUrl: prsByHead.get(b.name)?.url, tip, agents: completionPolicies, retirement: hasMergeEvidence });
+    const completionHold = await readBranchCompletionHoldDetail({ repoPath, branch: b.name, prUrl: prsByHead.get(b.name)?.url, tip, agents: completionPolicies, retirement: hasMergeEvidence });
+    const completionPolicyReason = completionHold.reason;
     inputs.push({
       branch: b.name,
       tip,
       completionPolicyReason,
+      completionReviewUnshipped: completionHold.unshipped === true,
       ...(completionPolicyReason ? { completionPolicyWorkflow: completionPolicyReason === 'pr-completion-review-required'
         ? 'Resume the configured review workflow; delivery success does not prove review passed.'
         : completionPolicyReason === 'pr-completion-human-handoff'

server/services/branchReconcile.test.js

@@ -323,6 +323,13 @@ describe('classifyBranches rule ids and wipParkCause', () => {
     const rule = (over) => { const { state, rule: id } = classifyBranches([{ ...base, ...over }])[0]; return [state, id]; };
     expect(rule({ openPr: { isDraft: true }, completionPolicyReason: 'pr-completion-review-required' })).toEqual(['WIP', 'draft']);
     expect(rule({ completionPolicyReason: 'pr-completion-review-required', isMerged: true })).toEqual(['WIP', 'completion-policy']);
+    // An unshipped review obligation falls through to the ordinary rules, but a
+    // merged pointer, an open PR and a live owner keep their holds.
+    const unshipped = { completionPolicyReason: 'pr-completion-review-required', completionReviewUnshipped: true, hasUpstream: false, hasOrigin: true, ahead: 2 };
+    expect(rule(unshipped)).toEqual(['NEEDS_PR', 'needs-pr']);
+    expect(rule({ ...unshipped, isMerged: true })).toEqual(['WIP', 'completion-policy']);
+    expect(rule({ ...unshipped, openPr: { mergeable: 'MERGEABLE' } })).toEqual(['WIP', 'completion-policy']);
+    expect(rule({ ...unshipped, liveOwnerReason: 'active-agent' })).toEqual(['WIP', 'live-owner']);
     expect(rule({ prStateUnavailable: true, hasUpstream: false })).toEqual(['WIP', 'pr-state-unavailable']);
     expect(rule({ hasUpstream: false })).toEqual(['WIP', 'bare-pointer']);
     expect(rule({ isMerged: true })).toEqual(['MERGED', 'merged']);