docs: update consensus-timeout HITL → OVERSEER_ALERT [doc-updater] - #2275
docs: update consensus-timeout HITL → OVERSEER_ALERT [doc-updater]#2275james-in-a-box[bot] wants to merge 5 commits into
Conversation
Update docs/guides/concurrent-execution.md and docs/guides/sdlc-pipeline.md to reflect #2264: consensus timeout now publishes a non-blocking OVERSEER_ALERT instead of opening a choice-typed HITL decision.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
No agent-mode design concerns — documentation-only update aligning consensus-timeout terminology with #2269.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review
The PR catches the right stale doc strings in two of three places, but misses two more in the same file (docs/guides/concurrent-execution.md) that contradict the corrections this PR is making elsewhere in that very file. PR #2275's stated goal is to bring docs in line with #2269's protocol change (consensus-timeout HITL decision → OVERSEER_ALERT). The two misses below leave the doc internally self-contradictory — the timeout-handling section (lines 730–738) correctly describes the alert flow, while the lead-in sentence (line 725) and the escalation-path table (line 856) still describe the removed HITL decision.
Per the review rules, "if a PR modifies code that has existing broken or inconsistent behavior, request changes to fix it — do not dismiss it as 'not a regression.' The PR is already in the area, making it the natural place to fix the issue." This PR is the right place.
Blocking
1. docs/guides/concurrent-execution.md:725 — still says "before opening a HITL decision"
If consensus is not reached within `consensus_timeout_minutes`, the orchestrator first checks the **BRC progress gate** before opening a HITL decision. While any of the following have fired within `brc_consensus_progress_gate_seconds` (default 300 s), the orchestrator continues polling rather than escalating immediately:
This is wrong post-#2269. The orchestrator no longer opens a HITL decision on consensus timeout — _handle_brc_consensus_timeout (orchestrator/routes/pipelines.py:9845) now calls _publish_consensus_timeout_alert on both former decision paths. The progress gate now defers an OVERSEER_ALERT, not a HITL decision. Compare with line 738 in the same section which the PR (correctly, via #2269) has already updated.
Suggested fix: replace "before opening a HITL decision" with "before publishing the OVERSEER_ALERT (#2264)" or "before publishing the consensus-timeout OVERSEER_ALERT".
2. docs/guides/concurrent-execution.md:856 — HITL Escalation Paths table still lists the removed options
| Consensus timeout (critical blockers) | Continue waiting, Accept current state, Abort phase |
These three option strings are exactly the ones #2269's PR description called out as removed ([Continue waiting, Accept current state, Abort phase]), and #2264's problem statement names them as the wrong protocol shape. There is no longer any HITL decision for consensus timeout (critical blockers); the row should either be removed or rewritten to point at the alert flow — the same way line 857 already handles the advisory-only case (*(no HITL — proceeds automatically)*).
Suggested fix: change the row to something like
| Consensus timeout (critical blockers) | *(no HITL — publishes high-priority `OVERSEER_ALERT`, see Timeout Handling)* |
This also keeps the table consistent with the surrounding rows (which use the same italic-aside convention for "no HITL" cases).
Non-blocking
sdlc-pipeline.md:1321cites(#2264)at the end of the bullet, which is helpful for backlinks. The correspondingconcurrent-execution.md:37description does not — minor consistency nit, fine to leave.- The phrase "Defer the consensus-timeout OVERSEER_ALERT" in both updated rows is grammatically a bit clipped (
OVERSEER_ALERTreads as a noun without an article); the existing prose in the timeout-handling section uses "publishes anOVERSEER_ALERT" / "the alert", which scans better. Not blocking — the meaning is clear.
What's good
- The three fixes the PR does make are accurate against the merged code:
_handle_brc_consensus_timeoutcalls_publish_consensus_timeout_alert, the gate function's docstring (orchestrator/routes/pipelines.py:9580) was already updated to referenceOVERSEER_ALERTin #2269, and the mergedmodels.pydescription matches the new doc text insdlc-pipeline.md:1252. - Subject format
consensus-timeout: <agent_role> [<priority>]in the new BRC-flow step 5 matches the actual format produced atorchestrator/routes/pipelines.py:9771. - The "intervene with
cancel_taskorrestart_phase" guidance matches the alert body's own copy atorchestrator/routes/pipelines.py:9787-9790.
Please fold the two missed strings into this PR rather than spinning a follow-up — they're in the same file and both directly contradict text this PR is editing.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address review feedback on PR #2275: - Line 725 (Timeout Handling lead-in): replace 'before opening a HITL decision' with 'before publishing the consensus-timeout OVERSEER_ALERT' so it matches the per-bullet description that follows. - Line 856 (HITL Escalation Paths table): remove the obsolete [Continue waiting, Accept current state, Abort phase] options for consensus timeout (critical blockers) and replace with the italic-aside convention used by the surrounding 'no HITL' rows, pointing at the Timeout Handling section. Both strings contradicted the corrections this PR was making elsewhere in the same file; #2269 removed the consensus-timeout HITL decision entirely in favor of OVERSEER_ALERT. Authored-by: egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…2277) * Fix #2275: docs follow-up — consensus-timeout HITL → OVERSEER_ALERT Docs leftovers from #2269 (which replaced the consensus-timeout HITL choice with an OVERSEER_ALERT but didn't reach every reference). - concurrent-execution.md: brc_consensus_progress_gate_seconds row references OVERSEER_ALERT; Timeout Handling lead sentence updated to match (it contradicted the bullets that already described the alert flow). - sdlc-pipeline.md: consensus_timeout_minutes / brc_consensus_progress_gate_seconds rows reference OVERSEER_ALERT; BRC consensus-protocol step 5 rewritten as a non-blocking alert (post-timeout polling continues; cancel_task / restart_phase / provide_input replace the removed three-option HITL). * Update concurrent-execution.md HITL/consensus-timeout rows to OVERSEER_ALERT Address PR #2277 review feedback: two stale rows in concurrent-execution.md contradicted the OVERSEER_ALERT flow established by #2269. - L36 (PipelineConfig table): consensus_timeout_minutes description now matches the parallel sdlc-pipeline.md row. - L856 (HITL Escalation Paths table): critical-blocker consensus timeout no longer lists the removed three-option HITL choice; matches the shape of the advisory-only row directly below it. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review
The two issues flagged in my previous review on f5c476c are addressed in dd0d6d9 ("docs: align two more consensus-timeout references with OVERSEER_ALERT"). Both fixes match the suggested replacements:
docs/guides/concurrent-execution.md:725— "before opening a HITL decision" → "before publishing the consensus-timeoutOVERSEER_ALERT(issue #2264)". Now consistent with line 732 in the same section.docs/guides/concurrent-execution.md:856— Removed the obsolete "Continue waiting, Accept current state, Abort phase" options and replaced with*(no HITL — publishes high-priority OVERSEER_ALERT, see Timeout Handling)*, matching the italic-aside convention used by surrounding rows (857, 858, 860).
Re-verified the rest
- The full PR diff is now constrained to two files (
concurrent-execution.md,sdlc-pipeline.md), 6 insertions / 8 deletions — pure terminology alignment. - Spot-checked all remaining "HITL" mentions in
concurrent-execution.md(lines 529, 535, 721, 761, 792–797, 803): each one is for a different escalation surface (objections, reviewer crashes, dual-role stall demotion, incomplete-consensus stall) — none are the consensus-timeout HITL that #2269 removed. Nothing else stale in this file. sdlc-pipeline.md:1252,1253, and1321all match the merged orchestrator behavior in_handle_brc_consensus_timeout/_publish_consensus_timeout_alert.- The two merge commits since the prior review (
2f1c1e1ruff sync,ca4191dtester scaffold doc) come frommainand don't affect the PR-authored delta.
What's good
- Backlinking issue #2264 inline at line 725 makes the protocol-change provenance discoverable from the doc itself.
- The italic-aside row in the escalation-paths table now reads identically to the surrounding "no HITL" rows, so the table is self-consistent.
No blocking issues. Approving.
— Authored by egg
|
egg review completed. View run logs 11 previous review(s) hidden. |
1 participant
Update documentation to reflect changes from #2269 (fixes #2264):
concurrent-execution.md:brc_consensus_progress_gate_secondsdescription updated from "Defer the consensus-timeout HITL decision" to "Defer the consensus-timeout OVERSEER_ALERT"sdlc-pipeline.md:consensus_timeout_minutesandbrc_consensus_progress_gate_secondstable entries updated to reference OVERSEER_ALERT; BRC flow step 5 updated to describe the non-blocking alert instead of the removed HITL choice promptTriggered by: #2269
Authored-by: egg