Skip to content

Make blocked batches end with a clear unblock action#259

Open
justin808 wants to merge 5 commits into
mainfrom
claude/blocked-batch-clarity-50a65a
Open

Make blocked batches end with a clear unblock action#259
justin808 wants to merge 5 commits into
mainfrom
claude/blocked-batch-clarity-50a65a

Conversation

@justin808

@justin808 justin808 commented Jul 25, 2026

Copy link
Copy Markdown
Member

Problem

When a batch stopped non-clean, the last thing an operator saw was a single dense line:

Conversation status: Follow-ups remain — PR #123 (open): answer storage-format question; PR #124 (pending): hosted CI `build`.

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 Block section in workflows/pr-processing.md, with every stopping surface routed to it: pr-batch, plan-pr-batch, triage, and post-merge-audit.

Whenever the final status is Follow-ups remain, the closing lines are pinned to exactly this order, with nothing between them:

  1. the optional compact Completed-batch audit: receipt line,
  2. the Unblock: block,
  3. the exact 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:

  • tagged [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];
  • the smallest next step, executable as written: an exact command, a paste-ready prompt, a URL, or the exact question with its allowed answers;
  • followed by a 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 exactly none — <reason>.

An UNKNOWN fact counts as a blocker and names the command that resolves it. A clean batch omits the block, since Conversation 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:

Unblock:
1. [you] (reordered) 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 record it as an accepted-deferral against PR #123 and keep that lane open on its existing PR; the question moves to a follow-up instead of blocking this batch.
2. [external] Hosted CI `build` on PR #124 is queued and clears when the run finishes; no action needed from you
   Help: if it is still queued at the next monitor wake, cancel and retrigger with `gh run cancel --repo OWNER/REPO <run-id>` then `gh run rerun --repo OWNER/REPO <run-id>`; `--failed` does not apply while a run is queued.
Conversation status: Follow-ups remain — PR #124 (pending): hosted CI `build`; PR #123 (open): answer storage-format question.

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:

  • the planning-chat lifecycle receipt rule and its skills/pr-batch/SKILL.md mirror ("immediately before the exact final Conversation status line, emit only:");
  • workflows/post-merge-audit.md and skills/post-merge-audit/SKILL.md ("emits only its verified compact receipt reference plus the final Conversation status line"), plus the pinned constant in post-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 explicit Do not end the batch title with a period; the line ends with the title text. rule to pr-batch, plan-pr-batch, and triage — 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:

  • The block was forbidden by three "emit only" contracts (above) — a coordinator following the more specific rule would have omitted it on non-clean batches.
  • [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].
  • The worked 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; following the canonical example would have produced a contradictory handoff. 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, so the advertised help could not clear the blocker. It now cancels and retriggers.
  • workflows/post-merge-audit.md and docs/pr-batch-skills.md restated the rules with no drift test; both are now covered.
  • triage had 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:

  • the canonical Unblock Block's scope, coverage, owner-tag, smallest-action, help, waiting, and clean-omission rules;
  • the closing order really is receipt → unblock → status (index comparison, not just presence);
  • every stopping surface routes to #unblock-block, including workflows/post-merge-audit.md;
  • both post-merge-audit surfaces carry the wiring with each file's correct relative link;
  • no closing-lines rule says "emit only" without naming the block (mutation-checked);
  • the worked example genuinely reorders against the status line and carries (reordered) (mutation-checked);
  • plan-pr-batch and triage each carry the standalone emission rule;
  • docs/pr-batch-skills.md's summary stays aligned with the canonical rules;
  • no batch title template line ends with a period, and each surface states the rule.

skills/post-merge-audit/bin/post-merge-audit-policy-test.rb — 15 runs, 297 assertions, 0 failures. RuboCop clean.

Note on local flakes: AgentWorkflowsDeliveryStateTest failed intermittently during bin/validate runs with "reason": "Codex plugin state command timed out". It reproduces on origin/main with 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.rb and bin/agent-stack-doctor-test.bash have the same shape. Filed separately.

🤖 Generated with Claude Code

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>
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Changes

Unblock closeout contract

Layer / File(s) Summary
Contract rules and coverage
skills/pr-batch/bin/goal-completion-contract-test.rb
Adds canonical Unblock Block rules and assertions for blocker coverage, ownership tags, next actions, help routes, ordering, omission, and title formatting.
Planning and triage output rules
docs/pr-batch-skills.md, skills/plan-pr-batch/..., skills/triage/SKILL.md
Requires period-free batch titles and an Unblock Block before non-clean final status lines.
Batch handoff and coordinator closeout
skills/pr-batch/SKILL.md, workflows/pr-processing.md
Defines non-clean handoff ordering, blocker entries, closeout restrictions, and archive-readiness conditions.
Post-merge audit closeout
skills/post-merge-audit/..., workflows/post-merge-audit.md, CHANGELOG.md
Aligns audit output with receipt, conditional Unblock Block, and final conversation-status requirements.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

  • shakacode/agent-workflows issue 243 — Both address pr-batch handoff and readiness contract guidance, with this change adding canonical Unblock: output.

Possibly related PRs

Suggested reviewers: ihabadham

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: adding a clear unblock action for blocked batches.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/blocked-batch-clarity-50a65a

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review

This is a doc/prompt-spec change to agent-workflows (no application code) — it adds a canonical ### Unblock Block section to workflows/pr-processing.md and routes every non-clean stopping surface (pr-batch, plan-pr-batch, triage, post-merge-audit) to it, plus drops the trailing period from batch-title templates. I checked out the merge ref and traced the diff end-to-end.

What I verified

  • Anchor links resolve correctly. #unblock-block is a unique heading in workflows/pr-processing.md (no collision), and every relative path to it from docs/, skills/pr-batch/, skills/plan-pr-batch/, skills/triage/, and workflows/post-merge-audit.md is correct for that file's directory depth.
  • Closing order is consistent everywhere it's restated: receipt → Unblock: block → Conversation status: line. The new test test_unblock_block_pins_the_closing_order_before_the_status_line asserts this via index comparison against exact substrings, and those substrings do appear verbatim (with numbering) in the canonical section.
  • Batch-title period removal is applied everywhere the template appearsworkflows/pr-processing.md (goal + continuation templates), skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md, skills/plan-pr-batch/scripts/check_goal_prompt_size.rb's CONTINUATION_BATCH_TITLE_LINE constant, and the explicit "do not end with a period" rule was added to pr-batch, plan-pr-batch, and triage.
  • Duplicated normative text (canonical section vs. per-skill summaries) stays in agreement — each duplicate carries a <!-- Keep ... in sync --> comment and the wording matches on the substantive points (owner tags, smallest-action requirement, Help:/none — <reason> fallback, clean-omission rule).
  • New test coverage (test_blocked_handoffs_end_with_a_canonical_unblock_block, test_every_stopping_surface_routes_to_the_canonical_unblock_block, test_closeout_lane_requires_the_unblock_block_with_the_follow_ups_line, test_batch_titles_do_not_end_with_a_period) reasonably guards against future drift between the canonical section and the four consuming surfaces.

I wasn't able to execute bin/validate / the Ruby test suite directly in this sandbox (command approval was declined), so I relied on static tracing of the constants/assertions against the actual file contents rather than a live test run; the PR description states both pass locally.

Minor observations (non-blocking)

  • skills/pr-batch/bin/goal-completion-contract-test.rb: the new POST_MERGE_AUDIT_SKILL_PATH fixture is read fresh with read_repo_file(...) inline inside test_every_stopping_surface_routes_to_the_canonical_unblock_block rather than cached as a @post_merge_audit_skill instance var in setup like the other three skill fixtures. Harmless since it's only used once, but slightly inconsistent with the surrounding pattern if another test later needs the same fixture.
  • The new UNBLOCK_BLOCK_* rule constants in the test file are fairly long, near-duplicate strings of the markdown prose. That's consistent with this file's existing style (asserting exact/squished prose matches), so not a real issue — just noting the maintenance cost of keeping ~8 new string constants in lockstep with the markdown if the wording is tweaked later.
  • No functional/security concerns — this PR only touches Markdown workflow specs, a doc file, and a Ruby test file; no runtime code paths, secrets, or external I/O are affected.

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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +414 to +416
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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Comment thread workflows/pr-processing.md Outdated

- 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review

This is a well-scoped, self-consistent documentation change to the PR-batch agent workflow instructions. I traced the new ### Unblock Block canonical section in workflows/pr-processing.md against every consumer (skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md, skills/triage/SKILL.md, skills/post-merge-audit/SKILL.md, workflows/post-merge-audit.md, docs/pr-batch-skills.md) and against the new/updated Ruby assertions in goal-completion-contract-test.rb:

  • All cross-references (#unblock-block anchors, relative ../workflows/pr-processing.md / pr-processing.md links) resolve correctly given the repo layout, and there are no heading-text collisions that would produce a disambiguated GitHub anchor (#unblock-block-1).
  • The closing-order rule (receipt → Unblock block → status line) is stated once canonically and every other surface defers to it via <!-- Keep ... in sync --> comments rather than re-deriving it — good, avoids the "three competing claims" problem the PR description calls out.
  • Spot-checked every new UNBLOCK_BLOCK_*_RULE / BATCH_TITLE_NO_TRAILING_PERIOD_RULE Ruby constant character-for-character against the corresponding prose in workflows/pr-processing.md; they match, including the em-dash () in none — <reason>, which is easy to accidentally diverge on (en-dash/hyphen) but is consistent everywhere.
  • test_batch_titles_do_not_end_with_a_period's regex-based offender scan (/^\s*Batch title: <PROJECT>/) correctly catches both the goal-prompt and continuation-title templates in pr-processing.md, and the check_goal_prompt_size.rb CONTINUATION_BATCH_TITLE_LINE constant was updated in lockstep.
  • The second commit's fixture caching (@post_merge_audit_skill read once in setup) is applied consistently with the other cached fixtures.

I wasn't able to execute bin/validate or the Ruby test suite directly in this review sandbox (shell commands required interactive approval that wasn't available), so I verified correctness by manual cross-reference instead of by running the suite the PR description cites.

Minor / non-blocking

  • The new ordering rule — "Order entries so an operator-owned action that would unblock other entries comes first; otherwise keep the status-line order." — isn't covered by any of the new contract tests (only coverage/never-drop is tested, not ordering). Since "would unblock other entries" is a judgment call for the agent, consider either dropping it for simplicity or adding a small worked example specifically exercising a reorder (the current worked example already has [you] before [external], which is also just status-line order, so it doesn't exercise the rule).
  • Nothing else stood out — no correctness, security, or performance concerns; this is a prompt/instruction-only change with no executable application logic beyond the test script, which looks correct.

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>
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review: Make blocked batches end with a clear unblock action

Documentation-only change to this repo's prompt/spec contract (workflows/pr-processing.md and the skills that route to it). Overall this is well-executed: the new ### Unblock Block section is placed correctly in the heading hierarchy (a proper H3 sibling of ### Batch Handoff Format, not nested oddly inside ### Coordinator Closeout Lane), all four stopping surfaces (pr-batch, plan-pr-batch, triage, post-merge-audit) route to it, the "batch title trailing period" cleanup is applied consistently everywhere Batch title: <PROJECT> ... appears (verified via repo-wide grep — no stray trailing periods left behind), and the new Ruby tests (closing-order index comparison, reorder-exercising worked example, cross-surface routing) are meaningful rather than just presence checks.

Suggestion: workflows/post-merge-audit.md update isn't covered by the contract test

workflows/post-merge-audit.md gets the same two-line Unblock Block wiring as skills/post-merge-audit/SKILL.md (correct content, correct relative link pr-processing.md#unblock-block vs. ../../workflows/pr-processing.md#unblock-block), but goal-completion-contract-test.rb's new test_every_stopping_surface_routes_to_the_canonical_unblock_block only reads @post_merge_audit_skill (skills/post-merge-audit/SKILL.md) — it never loads or asserts against workflows/post-merge-audit.md. Given this repo's whole model is strict textual contract-testing to prevent doc drift, this file is the one edited surface that could silently drift out of sync on a future change without CI catching it. Worth adding it to the test's surface map (or a dedicated assertion) for parity with the other four.

Minor: triage/SKILL.md gets thinner Unblock Block wiring than the other skills

pr-batch/SKILL.md and plan-pr-batch/SKILL.md each get both a dedicated instruction ("emit the canonical Unblock Block …") and a Response-order mention. triage/SKILL.md only gets the Response-order bullet (skills/triage/SKILL.md:353) — there's no standalone rule comparable to plan-pr-batch/SKILL.md:490 telling the model explicitly when/how to emit the block. It passes the current test (which only checks for the strings "Unblock Block" and "#unblock-block" anywhere in the file), but is a comparatively weak instruction relative to its siblings for what's meant to be a uniformly-enforced closing-order rule.

Verification

Diff is internally consistent: no leftover trailing-period Batch title: template lines anywhere in the repo, the worked example in ### Unblock Block correctly exercises the "operator-owned entry reorders ahead of the status-line order" rule (PR #123 leads the block while PR #124 leads the status line), and all cross-file relative links (../../workflows/pr-processing.md#unblock-block from skills/*/SKILL.md, pr-processing.md#unblock-block from workflows/post-merge-audit.md) resolve correctly given each file's location.

No security or performance concerns — this PR touches only markdown prompt/spec content and test fixtures.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread skills/pr-batch/SKILL.md
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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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>
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review

This is a well-scoped, docs/prompt-contract-only change to the agent-workflows pack (no runtime application code). It adds a canonical ### Unblock Block section to workflows/pr-processing.md and wires every non-clean stopping surface (pr-batch, plan-pr-batch, triage, post-merge-audit) to it, plus drops the trailing period from batch-title template lines.

What I checked

  • Diffed the PR against main and cross-referenced every new/changed rule string against the Ruby constants added to skills/pr-batch/bin/goal-completion-contract-test.rb (UNBLOCK_BLOCK_*, BATCH_TITLE_NO_TRAILING_PERIOD_RULE, PLAN_PR_BATCH_RESPONSE_ORDER, TRIAGE_RESPONSE_ORDER, etc.) — every squished-string assertion matches the actual doc text verbatim.
  • Verified the closing-order claim (Completed-batch audit: receipt → Unblock: block → Conversation status: line) is stated identically and in the same relative order in workflows/pr-processing.md, workflows/post-merge-audit.md, skills/post-merge-audit/SKILL.md, and skills/pr-batch/SKILL.md.
  • Verified relative markdown links resolve correctly (../../workflows/pr-processing.md#unblock-block from two-levels-deep skill files, pr-processing.md#unblock-block from the same-directory workflows/post-merge-audit.md, and the local #unblock-block summary anchor inside skills/pr-batch/SKILL.md itself).
  • Confirmed the worked example's ordering-rule test (test_unblock_block_example_exercises_the_operator_first_ordering_rule) is genuinely exercised: the Unblock block leads with [you] ... PR #123, while the Conversation status line renders PR #124 first — so the "reorder for operator-first" rule isn't a no-op in the test.
  • Confirmed the trailing-period removal is applied consistently everywhere a Batch title: template line appears, including the standalone check_goal_prompt_size.rb constant.

Overall this looks correct, internally consistent, and appropriately test-guarded. Only minor, non-blocking notes below (inline).

Comment thread docs/pr-batch-skills.md
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`. -->

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread workflows/pr-processing.md Outdated
```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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Comment thread workflows/pr-processing.md Outdated
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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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>
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review

This 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:

  • All new #unblock-block anchors resolve correctly (heading levels and relative paths — ../workflows/... from docs/, ../../workflows/... from skills/*/SKILL.md, bare pr-processing.md#unblock-block from the sibling workflows/post-merge-audit.md — are all correct).
  • The closing-order rule (receipt → Unblock: block → Conversation status: line) is stated once canonically in workflows/pr-processing.md and every other surface (both SKILL.md files, both workflow files) now defers to it rather than restating a competing "immediately before" claim. I grepped for every remaining emit(s) only phrase touching Conversation status/closing lines across the repo and confirmed none omit the Unblock Block.
  • Grepped for every remaining Batch title: template/reference in the repo — none still expect a trailing period, so the two changes (drop period, add explicit no-period rule) are applied everywhere consistently.
  • The worked example in ### Unblock Block correctly exercises the (reordered) rule: it leads with [you] (reordered) referencing #123 while the Conversation status line lists #124 first, so the example genuinely demonstrates reordering rather than trivially matching status-line order.

One minor nit (non-blocking): the PR description's sample Unblock: block uses Help: reply deferto waive it for this batch; that lane then closes asno-pr-evidence..., but the actual worked example landed in workflows/pr-processing.md (and mirrored in the test fixture) uses different wording — Help: reply defer to record it as an accepted-deferral... and keep that lane open on its existing PR. Doesn't affect correctness, just worth updating the PR description (or noting it evolved during review) so it doesn't confuse future readers comparing the description to the diff.

No security, performance, or correctness concerns — this PR only edits Markdown workflow/skill specs and their accompanying Ruby string-matching test fixtures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant