Make blocked batches end with a clear unblock action#259
Conversation
A batch that stopped non-clean ended on a single dense `Conversation status: Follow-ups remain — ...` line. That line names the blockers but not the smallest next step, who owns it, or any alternative way to clear it, so an operator had to reread the whole handoff to work out what to do. Add a canonical Unblock Block to workflows/pr-processing.md and route every stopping surface to it: pr-batch, plan-pr-batch, triage, and post-merge-audit. Whenever the final status is `Follow-ups remain`, the closing lines are now the optional compact completed-batch-audit receipt, then the `Unblock:` block, then the exact status line. Each block carries one numbered entry per blocker in the same normalized union the status line renders — never dropped, added, or merged — tagged `[you]`, `[agent]`, or `[external]` so the operator can tell at a glance whether anything is owed from them, naming the smallest next action as an exact command, paste-ready prompt, URL, or question, and followed by a `Help:` line offering a genuinely different route to clearing that same blocker (waive, rerun, reassign, cancel, escalate) or exactly `none — <reason>`. A clean batch omits the block; `Ready for archiving.` already says the only remaining action is archiving. Also drop the trailing period from batch title lines in the goal prompt and continuation prompt templates, and state the no-trailing-period rule in pr-batch, plan-pr-batch, and triage so a generated title does not reintroduce it. Verified with bin/validate (full suite green; the drift-test 0.1s timeout case is the known flaky one and passes on rerun). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
WalkthroughChangesUnblock closeout contract
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ReviewThis is a doc/prompt-spec change to What I verified
I wasn't able to execute Minor observations (non-blocking)
Overall this looks well-scoped and internally consistent. No blocking issues found. |
Matches how the other skill fixtures are read, per PR review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a65681f107
ℹ️ 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".
| In final chat, this compact receipt line opens the closing lines: it is followed | ||
| by the [Unblock Block](../../workflows/pr-processing.md#unblock-block) whenever | ||
| the status is not clean, and then by the exact `Conversation status` final line; |
There was a problem hiding this comment.
Include the Unblock block in the earlier output contract
In completed-batch mode, the instruction at line 392 still says the coordinator emits only the compact receipt and final status; the mirrored workflow at workflows/post-merge-audit.md:66 and its pinned policy-test constant repeat that contract. This directly forbids the Unblock block required here, so a coordinator following the earlier explicit instruction will omit the new block precisely for non-clean audits. Update the earlier contract, mirrored copy, and test constant to describe the new three-part closing sequence.
Useful? React with 👍 / 👎.
|
|
||
| - One numbered entry per exact blocker in the same normalized blocker union rendered in the `Conversation status` line. Never drop a blocker, never add one that is missing from that union, and never merge two blockers into one entry. | ||
| - Order entries so an operator-owned action that would unblock other entries comes first; otherwise keep the status-line order. | ||
| - `[you]` means the operator must act before anything else moves. `[agent]` means this thread resumes on its own — name the exact trigger, such as the 15-minute monitor wake, the watch window, or the exact resume prompt when a restart is needed. `[external]` means a check, bot, or third party is being waited on — name it, name the condition that clears it, and say plainly that no operator action is required. |
There was a problem hiding this comment.
Classify manual resume prompts as operator actions
When recurring same-thread wakeups are unavailable and a restart is required, the exact resume prompt must be pasted by the operator—the Goal Mode contract below explicitly calls these “manual resume instructions.” Listing that prompt under [agent], which is defined here as resuming on its own, can tell the operator nothing is owed while the batch actually cannot continue without them. Reserve [agent] for a real monitor/watch trigger and tag manual restart prompts [you].
Useful? React with 👍 / 👎.
ReviewThis is a well-scoped, self-consistent documentation change to the PR-batch agent workflow instructions. I traced the new
I wasn't able to execute Minor / non-blocking
|
The example put [you] before [external], which was also status-line order, so it never demonstrated the reorder the rule asks for. Swap the status-line order and explain why the operator-owned entry leads, and guard it with a test that fails if the example is flattened back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review: Make blocked batches end with a clear unblock actionDocumentation-only change to this repo's prompt/spec contract ( Suggestion:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52cea8d510
ℹ️ 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".
| split final states from `.agents/workflows/pr-processing.md`. | ||
|
|
||
| End the final user-visible message carrying the batch handoff with the exact archive-readiness status line, either `Conversation status: Ready for archiving.` or `Conversation status: Follow-ups remain — <each exact action or blocker>.`, selected by the [Coordinator Closeout Lane](#coordinator-closeout-lane) rules rather than by any criteria restated here. A final handoff without one of those two exact lines is incomplete, because the operator cannot tell whether the conversation is safe to archive. | ||
| End the final user-visible message carrying the batch handoff with the [Unblock Block](#unblock-block), required whenever the status is not clean, followed by the exact archive-readiness status line, either `Conversation status: Ready for archiving.` or `Conversation status: Follow-ups remain — <each exact action or blocker>.`, selected by the [Coordinator Closeout Lane](#coordinator-closeout-lane) rules rather than by any criteria restated here. A final handoff without one of those two exact lines is incomplete, because the operator cannot tell whether the conversation is safe to archive; a `Follow-ups remain` handoff without the Unblock Block is incomplete for the same reason, because the operator cannot tell which small action to take next. |
There was a problem hiding this comment.
Reconcile the later compact-receipt closeout contract
For a non-clean completed-batch handoff, this new requirement is still contradicted by the same skill's closeout instructions at lines 717–719, which say that immediately before the final status the coordinator must emit only the compact audit receipt. A coordinator following that later, more specific instruction will omit the required Unblock: block. The prior review comment covered the post-merge-audit copies; the fresh evidence is this additional conflicting contract in skills/pr-batch/SKILL.md, which must be updated as well.
Useful? React with 👍 / 👎.
workflows/post-merge-audit.md carried the same Unblock Block wiring as its skill but no contract test read it, so it was the one edited surface that could drift silently. Add it to the routing surface map plus a test asserting both wiring sentences with each file"\s correct relative link. triage had only the response-order mention while pr-batch and plan-pr-batch each had a standalone emission rule too; give it the same rule and assert the shared wording on both planning surfaces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ReviewThis is a well-scoped, docs/prompt-contract-only change to the What I checked
Overall this looks correct, internally consistent, and appropriately test-guarded. Only minor, non-blocking notes below (inline). |
| Silence is not an accepted value; a batch that wrote nothing to the coordination | ||
| backend must say so in the declaration. | ||
|
|
||
| <!-- Keep this rule in sync with `../workflows/pr-processing.md` -> `### Unblock Block`. --> |
There was a problem hiding this comment.
Nit / maintainability gap: this summary (and the ### Batch Handoff Format one above it) relies on an HTML-comment reminder to stay in sync with workflows/pr-processing.md, but unlike the SKILL.md files, nothing in skills/pr-batch/bin/goal-completion-contract-test.rb asserts this doc's text against the canonical ### Unblock Block rules. If the canonical section is edited later (e.g. the owner-tag wording or the Help: requirement), this paraphrase can silently drift with no test failure to catch it. Consider either a lightweight contract-test check on this file too, or trimming this to a one-line pointer + link instead of restating the substance.
| - Carry only blockers. Decisions, evidence, and FYI items stay in the handoff body above. | ||
| - When every entry is `[agent]` or `[external]`, still emit the block and say that waiting is the correct action, so the operator can tell that nothing is owed from them. | ||
|
|
||
| Worked example. The status line renders PR #124 first, but answering PR #123 with |
There was a problem hiding this comment.
Good that this worked example actually exercises the "operator-first" reordering rule (the test checks the leading Unblock: entry ref differs from the leading Conversation status ref), but worth flagging for a human reader: the Unblock: block lists PR #123 then PR #124, while the Conversation status line right below it lists PR #124 then PR #123. An operator skimming both lines quickly could read that as an inconsistency/typo rather than intentional reordering. The prose above the example does explain it, but since these two lines sit back-to-back at the very end of the message (the point of this feature is skimmability), consider making the divergence more visually obvious — e.g. a short inline note like (reordered — see rule above) next to the block, or picking an example where the reorder is less likely to be misread as a mismatch.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6aaa35f04c
ℹ️ 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".
| ```text | ||
| Unblock: | ||
| 1. [you] Answer the storage-format question on PR #123 — https://github.com/OWNER/REPO/pull/123#discussion_r1 — reply `keep` or `migrate` | ||
| Help: reply `defer` to waive it for this batch; that lane then closes as `no-pr-evidence` with your rationale recorded. |
There was a problem hiding this comment.
Keep deferred PR lanes out of no-pr-evidence
When the operator replies defer in this example, PR #123 already exists, but this help text tells the coordinator to close its lane as no-pr-evidence; the readiness definition at line 1156 reserves that state for cases where no PR was created. Following the canonical example can therefore produce a contradictory handoff and cause later audit/reconciliation to treat an existing PR as absent; record the question as an accepted waiver/deferral while preserving the PR lane instead.
Useful? React with 👍 / 👎.
| 1. [you] Answer the storage-format question on PR #123 — https://github.com/OWNER/REPO/pull/123#discussion_r1 — reply `keep` or `migrate` | ||
| Help: reply `defer` to waive it for this batch; that lane then closes as `no-pr-evidence` with your rationale recorded. | ||
| 2. [external] Hosted CI `build` on PR #124 is queued and clears when the run finishes; no action needed from you | ||
| Help: force a rerun with `gh run rerun --failed --repo OWNER/REPO <run-id>` if it is still queued at the next monitor wake. |
There was a problem hiding this comment.
Replace the failed-only rerun for queued runs
For the exact scenario stated here—this workflow run is still queued—gh run rerun --failed is not an executable alternative because --failed reruns only failed jobs, while a queued run has no failed jobs and is not yet eligible for that rerun operation (GitHub CLI manual). The advertised Help: action therefore cannot clear the blocker; provide a valid cancel-and-retrigger route or use none — <reason> while it remains queued.
Useful? React with 👍 / 👎.
Codex review found the Unblock Block was forbidden by three pre-existing "emit only" contracts that are more specific than the new requirement, so a coordinator following them would omit the block exactly when a batch is non-clean: - skills/pr-batch/SKILL.md mirrored the pr-processing.md sentence I had already fixed upstream but was missed in the mirror. - workflows/post-merge-audit.md and its skill both said the coordinator emits "only" the receipt plus the status line; the pinned policy-test constant repeated it. All now name the Unblock Block, and a regex guard fails on any future closing-lines rule that says "emit only" without it. Also fix three defects in the rule and its example: - [agent] was defined to cover "the exact resume prompt when a restart is needed", but a prompt the operator must paste is operator work. Manual resume instructions are now always [you], so the block cannot tell an operator nothing is owed while the batch cannot continue. - The example's `defer` help closed an existing PR lane as `no-pr-evidence`, which the readiness vocabulary reserves for targets where no PR was created. It now records an accepted-deferral and keeps the lane on its PR. - The example offered `gh run rerun --failed` for a queued run, which has no failed jobs to rerun. It now cancels and retriggers. Add a `(reordered)` marker so an entry that moves ahead of its status-line position does not read as a mismatch, and cover the docs/pr-batch-skills.md paraphrase, which was the last restatement with no drift test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ReviewThis is a well-scoped, internally consistent documentation change to the agent-workflow spec (no application code touched). I traced every cross-reference this PR adds:
One minor nit (non-blocking): the PR description's sample No security, performance, or correctness concerns — this PR only edits Markdown workflow/skill specs and their accompanying Ruby string-matching test fixtures. |
Problem
When a batch stopped non-clean, the last thing an operator saw was a single dense line:
That names the blockers, but not the smallest next step, not who owns it, and not any alternative way to clear it. Working out "what do I actually do now?" meant rereading the whole handoff above it.
Batch title lines also carried a trailing period, which reads oddly in a title.
Change
Unblock Block
New canonical
### Unblock Blocksection inworkflows/pr-processing.md, with every stopping surface routed to it:pr-batch,plan-pr-batch,triage, andpost-merge-audit.Whenever the final status is
Follow-ups remain, the closing lines are pinned to exactly this order, with nothing between them:Completed-batch audit:receipt line,Unblock:block,Conversation status:line.The block carries one numbered entry per blocker in the same normalized union the status line renders — never dropped, added, or merged. Each entry is:
[you],[agent], or[external], so the operator can tell at a glance whether anything is owed from them — a manual resume prompt they must paste is always[you], never[agent];Help:line offering a genuinely different route to clearing that same blocker (waive, rerun, reassign, cancel, escalate, or the exact skill/section that does it) or exactlynone — <reason>.An
UNKNOWNfact counts as a blocker and names the command that resolves it. A clean batch omits the block, sinceConversation status: Ready for archiving.already says the only remaining action is archiving. When every entry is[agent]or[external], the block is still emitted and says waiting is the correct action — silence should not be confused with "nothing is happening".What that looks like in practice. An entry that moves ahead of its status-line position carries
(reordered), so the divergence between the two blocks does not read as a mismatch:Several pre-existing rules constrained the closing lines in ways that contradicted the new block, and each was more specific than the general requirement — so a coordinator following them would have dropped the block exactly when a batch is non-clean. All now defer to the single closing order above:
skills/pr-batch/SKILL.mdmirror ("immediately before the exact finalConversation statusline, emit only:");workflows/post-merge-audit.mdandskills/post-merge-audit/SKILL.md("emits only its verified compact receipt reference plus the finalConversation statusline"), plus the pinned constant inpost-merge-audit-policy-test.rb.A regex guard now fails any future closing-lines rule that says "emit only" without naming the Unblock Block.
Batch title periods
Dropped the trailing period from the batch title line in the goal-prompt and continuation-prompt templates (
pr-processing.md,pr-batch,plan-pr-batch), and added an explicitDo not end the batch title with a period; the line ends with the title text.rule topr-batch,plan-pr-batch, andtriage— a template alone would not stop a generated title from reintroducing it.Fixed during review
Review rounds caught real defects in the first cut, all fixed here:
[agent]was defined to cover "the exact resume prompt when a restart is needed" — but a prompt the operator must paste is operator work. Tagging it[agent]would tell the operator nothing was owed while the batch could not continue. Manual resume instructions are now always[you].deferhelp closed an existing PR lane asno-pr-evidence, which the readiness vocabulary reserves for targets where no PR was created; following the canonical example would have produced a contradictory handoff. It now records an accepted-deferral and keeps the lane on its PR.gh run rerun --failedfor a queued run, which has no failed jobs to rerun, so the advertised help could not clear the blocker. It now cancels and retriggers.workflows/post-merge-audit.mdanddocs/pr-batch-skills.mdrestated the rules with no drift test; both are now covered.triagehad only a response-order mention while its siblings also carried a standalone emission rule; it now has the same rule.Verification
bin/validate— full suite green (93 files inspected, no offenses;PASS agent-workflows validation).skills/pr-batch/bin/goal-completion-contract-test.rb— 84 runs, 1139 assertions, 0 failures. New tests cover:#unblock-block, includingworkflows/post-merge-audit.md;(reordered)(mutation-checked);plan-pr-batchandtriageeach carry the standalone emission rule;docs/pr-batch-skills.md's summary stays aligned with the canonical rules;skills/post-merge-audit/bin/post-merge-audit-policy-test.rb— 15 runs, 297 assertions, 0 failures. RuboCop clean.Note on local flakes:
AgentWorkflowsDeliveryStateTestfailed intermittently duringbin/validateruns with"reason": "Codex plugin state command timed out". It reproduces onorigin/mainwith none of these changes (1 failure in 3 runs there, 0 in 3 on this branch), so it is a pre-existing load-sensitive timeout, not a regression from this PR.bin/check-agent-workflow-drift-test.rbandbin/agent-stack-doctor-test.bashhave the same shape. Filed separately.🤖 Generated with Claude Code