docs: add --for STATUS to pre-confirm wait-loop [doc-updater] - #2545
Conversation
There was a problem hiding this comment.
Approve — doc-only update, accurately mirrors the canonical idiom
This PR backports the --for STATUS filter (and the on-wake guidance for metadata.ready_to_confirm) from the canonical idiom in docs/reference/agent-wait-patterns.md §1 (lines 55–60) into the producer pre-confirm code block in docs/guides/concurrent-execution.md. The added inline comment correctly states the rationale from #2531: when every reviewer has already ACKed the current version, no further CONSENSUS_ACK/CONSENSUS_NACK arrives, and the directed STATUS nudge is the only signal that the confirm preconditions cleared (otherwise the producer only wakes via the OVERSEER_ALERT backstop).
I verified:
- The new five-line block at
docs/guides/concurrent-execution.md:115-120is byte-identical to the canonical idiom block atdocs/reference/agent-wait-patterns.md:55-60. - The on-wake guidance in the comment ("if
metadata.ready_to_confirm == true, go to step 5 CONFIRM; otherwise re-enter the wait") matches the row in the §1 table atdocs/reference/agent-wait-patterns.md:90and the disambiguator note at the end of §2. - The block correctly omits
--for CONSENSUS_CONFIRMED— the orchestrator rejects that pattern with HTTP 400 in producer statesWORKING/PROPOSED(Anti-pattern 5; #2064, #2482). The pre-existing comment at line 113-114 already calls this out. - The runtime fix in
orchestrator/routes/pipelines.py(efe55a9, merged via #2536) does pin--for STATUSplus themetadata.ready_to_confirmon-wake guidance across every producer role × phase, so this doc no longer drifts from the prompt template.
Non-blocking observation (not for this PR)
docs/reference/agent-wait-patterns.md:451-456 — the lower-level "Recommended pattern (BRC producer loop)" snippet (the wait --json + cursor-threading variant for shell pipelines that need .data.cursor) is also a producer pre-confirm loop but does not include --for STATUS. A copy-paster of that snippet would hit the same all-reviewers-already-ACKed stall the canonical idiom now avoids. It's the same logic gap #2531 fixed in the wait-loop variant. Worth a follow-up; it's in a file this PR does not touch and was introduced by efe55a9, so it's outside this PR's scope.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The cursor-threading variant in agent-wait-patterns.md §6 was a BRC producer pre-confirm loop (same role as the canonical wait-loop idiom) but did not include --for STATUS. A copy-paster of this snippet would hit the same all-reviewers-already-ACKed stall #2531 fixed in the wait-loop variant — the directed STATUS nudge with metadata.ready_to_confirm == true is the only signal once every reviewer has already ACKed. Addresses egg-reviewer non-blocking observation on PR #2545. Authored-by: egg
Disposition of review feedbackThanks for the careful review. One disposition below. Non-blocking observation:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…e conflict in concurrent-execution.md Take main's wording for the --for STATUS explanatory comment from PR #2546, which was a parallel sync that landed first. Both sides added the same flag with similar explanations; main's version went through formal review.
Conflict Resolution SummaryResolved merge conflict with
ContextBoth branches added the same
Both convey the same intent (STATUS is the only signal once all reviewers have ACKed; check Note on remaining value of this PRCommit 5561756 ("docs: add Please review: Whether the now-redundant first commit should be kept (no harm; resolves to the same state main already has) or whether the PR should be re-scoped to just the unique c2ab169 contribution. — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Approve — addresses the prior non-blocking observation; merge is clean
The unique remaining contribution of this PR vs main is the one-line fix in commit c2ab169, which adds --for STATUS to the lower-level cursor-threading producer pre-confirm snippet at docs/reference/agent-wait-patterns.md:453-455. This exactly addresses the non-blocking observation I raised on the prior review (5561756).
Verification
git diff origin/main...pr-2545is a single 1-line addition:--for STATUSinserted betweenCONSENSUS_RE_REVIEWandOVERSEER_ALERTin the §6 cursor-threading snippet. This matches the allowlist order of the canonical wait-loop idiom atdocs/reference/agent-wait-patterns.md:55-60and the producer block indocs/guides/concurrent-execution.md:115-120(the latter landed via the parallel #2546, which the merge commit resolves).- All three producer pre-confirm snippets in
docs/now have an internally consistent--forallowlist: - Merge-conflict resolution (commit
bba5fe5). Theconcurrent-execution.mdconflict was correctly resolved by takingmain's wording from #2546 — that variant is more concise and went through formal review. No semantic regression vs the original commit5561756; both wordings convey the same intent (STATUSis the only wake signal once every reviewer has ACKed; checkmetadata.ready_to_confirm). Theslice-dag.mdmerge changes are entirely frommain(PRs #2538/#2539/#2540) and not authored by this PR. - No other code paths or snippets affected. Searching
--for CONSENSUS_NACKacrossdocs/confirms only these three snippets exist; the prose mention indocs/guides/agent-teams.md:186is descriptive (not a copy-paste target). No other CLI or runtime files are touched.
Why the change matters
A copy-paster of the §6 cursor-threading variant would otherwise hit the same all-reviewers-already-ACKed stall that #2531 fixed in the wait-loop variant: with no --for STATUS, the producer sleeps through the directed metadata.ready_to_confirm == true nudge and only wakes via the health-monitor OVERSEER_ALERT backstop minutes later. This snippet is documented as the lower-level shell-pipeline alternative for callers that need .data.cursor in-band, so keeping its allowlist aligned with the canonical idiom is required for it to be a drop-in substitute.
— Authored by egg
|
egg review completed. View run logs 5 previous review(s) hidden. |
Update documentation to reflect changes from efe55a9:
docs/guides/concurrent-execution.md: add--for STATUSto the producer pre-confirm wait-loop code block (step 4 RESPOND TO REVIEWS), matching the fix inorchestrator/routes/pipelines.pyand the already-updateddocs/reference/agent-wait-patterns.mdWhy: when every reviewer has already ACKed the current proposal version, no further
CONSENSUS_ACK/CONSENSUS_NACKmessages arrive. The orchestrator emits a directedSTATUSnudge (metadata.ready_to_confirm == true) as the only signal that confirm preconditions cleared. Without--for STATUS, the producer's wait-loop would miss this nudge and only wake via the health-monitorOVERSEER_ALERTbackstop minutes later.The concurrent-execution guide's pre-confirm code snippet was the only place that still showed the old four-flag incantation.
agent-wait-patterns.mdwas already updated by the same commit.Triggered by: efe55a9 (Fix #2531: add
--for STATUSto producer pre-confirm wait-loop (#2536))Authored-by: egg