docs: update BRC confirmation timeout docs for direct nudge [doc-updater] - #2092
Conversation
There was a problem hiding this comment.
Approve. Documentation-only PR. Verified the changes against the actual implementation merged in #2086 (129270a52):
_send_brc_confirmation_nudgeinorchestrator/routes/pipelines.py:510-606— registers as aHealthMonitor.on_escalationcallback, filters onalert_type == "brc_confirmation_timeout", posts anOVERSEER_ALERTdirectly viamsg_store.add_message(bypassing the POST/messages/sendroute to avoidMESSAGE_SENTrate-limit + heartbeat tracking), and the body explicitly tells the producer toCall mcp__brc__confirm nowwith guidance forstatus='pending_acks'. ✓check_brc_progressinorchestrator/health_monitor.py:822-870— emits the escalation dict withalert_type: "brc_confirmation_timeout"andelapsed_seconds, also registers an entry in_active_alerts, and fires registered callbacks. The escalationtypeis"overseer"or"hitl"based onoverseer_enabled, so the standard escalation path still runs. ✓- The "post-ACK confirmation timeout" framing, the
_fully_acked_first_seentracking, thebrc_progress_escalateddedup flag, and the cleanup-on-confirm behavior all match the code. ✓
Both edited paragraphs accurately describe the behavior and the rationale (why OVERSEER_ALERT was chosen — because it appears in both the pre-confirm and post-confirm wait_loop filters).
Non-blocking nits:
-
docs/guides/concurrent-execution.md:959— the phrasingsends a direct OVERSEER_ALERT ... then escalates to overseer/HITLuses "then" which implies sequencing, but in the code both actions originate from the same escalation event and run concurrently (the callback fires the nudge while_active_alertsis consumed asynchronously by the overseer agent). The sister sentence inpipeline-health-monitoring.md:203usesThe alert also triggers the standard overseer/HITL escalation pathwhich reads more accurately. Consider aligning the wording — e.g., replace "then escalates" with "and also escalates". -
docs/guides/pipeline-health-monitoring.md:210—bypassing MESSAGE_SENT tracking to avoid rate-limit side-effectsis slightly understated. The code comment in_send_brc_confirmation_nudgenotes the bypass avoids both rate-limit and HEARTBEAT tracking side-effects (MESSAGE_SENT handler (rate-limit + HEARTBEAT tracking)). Suggest:bypassing MESSAGE_SENT tracking to avoid rate-limit and heartbeat-tracking side-effects. Minor — the current wording is not wrong, just incomplete.
Neither nit blocks merge.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address non-blocking review nits on #2092: - concurrent-execution.md: 'then escalates' implied sequencing; the callback nudge and overseer/HITL escalation originate from the same event and run concurrently. Use 'and also escalates'. - pipeline-health-monitoring.md: bypass also avoids HEARTBEAT tracking side-effects, not just rate-limit. Mention both.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Approve. Both non-blocking nits from the previous review have been properly addressed in af2d488:
concurrent-execution.md:959— "then escalates" → "and also escalates" ✓ (no longer implies sequencing)pipeline-health-monitoring.md:210— now reads "bypassingMESSAGE_SENTtracking to avoid rate-limit and heartbeat-tracking side-effects" ✓ (matches the code comment in_send_brc_confirmation_nudge:MESSAGE_SENT handler (rate-limit + HEARTBEAT tracking))
Bonus: the table row in pipeline-health-monitoring.md:108 was also expanded from the bare "Escalate to overseer/HITL" to spell out the direct OVERSEER_ALERT + mcp__brc__confirm instruction. Reads more accurately now.
Re-verified the prose against the implementation:
_send_brc_confirmation_nudgeatorchestrator/routes/pipelines.py:510still filters onalert_type == "brc_confirmation_timeout", posts directly viamsg_store.add_message(bypassing the route +MESSAGE_SENT), and tells the producer toCall mcp__brc__confirm now. ✓check_brc_progressatorchestrator/health_monitor.pystill emits thebrc_confirmation_timeoutescalation dict and fires registered callbacks; standard overseer/HITL path also runs (the escalationtypeis"overseer"or"hitl"based onoverseer_enabled). ✓- "How it works" steps 4–6 accurately describe the new callback split.
Doc-only change, no functional impact, no blocking issues.
— Authored by egg
|
egg review completed. View run logs 3 previous review(s) hidden. |
Update documentation to reflect changes from 129270a (#2086):
brc_confirmation_timeoutescalation now directly sends anOVERSEER_ALERTto the stuck producer via_send_brc_confirmation_nudge, bypassing the overseer agent's decision loop for this deterministic failure mode.OVERSEER_ALERTis used (it appears in both the pre-confirm and post-confirm producer wait_loop filters).Files changed:
docs/guides/pipeline-health-monitoring.md: Updated tripwire table row and the "Post-ACK Confirmation Timeout" section to describe the escalation callback and_send_brc_confirmation_nudgebehavior.docs/guides/concurrent-execution.md: Updated the Post-ACK confirmation timeout paragraph to mention the direct nudge.Triggered by: #2086
Authored-by: egg