Fix #2264: replace consensus-timeout HITL decision with OVERSEER_ALERT

[jwbron]

Summary

• Step 3 of Auto BRC-consensus-failure HITL decision at 30 min misfires on healthy long-running phases #2243: replace the auto-choice HITL decision opened on BRC consensus timeout with a non-blocking OVERSEER_ALERT.
• The platform no longer gates the pipeline on a binary prompt at timeout. The SDLC skill's existing OVERSEER_ALERT flow surfaces the alert (Check agent logs / Acknowledge / Cancel pipeline); operators retain cancel_task / restart_phase / provide_input for active intervention.
• Sequenced after step 1 (progress gate, Fix #2243: progress gate before BRC consensus-failure HITL decision #2254 — already merged) so the false-positive surface was reduced before flipping the protocol.

Why

_handle_brc_consensus_timeout used to open a choice-typed pending_decision with options [Continue waiting, Accept current state, Abort phase]. Three problems:

1. It blocked the pipeline on operator input by default — even after the Fix #2243: progress gate before BRC consensus-failure HITL decision #2254 progress gate, the decision still fires when no progress signals are fresh.
2. Two of the three options ("Accept current state", "Abort phase") silently destroyed real work when picked on a healthy pipeline. "Continue waiting" was the only correct answer in every documented misfire (Auto BRC-consensus-failure HITL decision at 30 min misfires on healthy long-running phases #2243 post-mortem, [#1921 follow-up] Implement phase still force-kills at 90 min — post_timeout_budget is hardcoded 3600s with no per-phase / per-complexity knob #2245 force-kill cycle).
3. OVERSEER_ALERT is the right shape for "the platform thinks something might be wrong, please look at it" — a notification, not a gate.

What changed

orchestrator/routes/pipelines.py

• New _publish_consensus_timeout_alert helper that builds an OVERSEER_ALERT Message and writes it via _get_message_store().
• New _latest_active_role_heartbeat helper that mirrors the heartbeat half of _check_brc_progress_gate so the alert metadata's latest_heartbeat_at reflects active-role heartbeats only (avoids cross-phase pollution from the singleton HealthMonitor).
• _handle_brc_consensus_timeout now publishes an OVERSEER_ALERT on both former decision paths:
  • Critical-blocker escalation → priority=high.
  • No-tracker / tracker-import-error fallback → priority=medium. The existing _emit_event(CONSENSUS_TIMEOUT) on this path is preserved.
  • The advisory-only proceed_with_notification path stays silent (unchanged).
• _handle_brc_consensus_timeout gains an optional active_role_names: list[str] | None parameter; the call site passes [e.role.value for e in active_executions] so the alert can include heartbeat metadata. Tests that don't set it get None-valued metadata.

Alert metadata schema (per AC):

{
  "anomaly_type": "consensus-timeout",
  "phase": "<phase>",
  "blocking_agents": ["..."],
  "latest_proposal_at": "<ISO8601 or null>",
  "latest_heartbeat_at": "<ISO8601 or null>",
  "consensus_timeout_minutes": 30,
  "priority": "high|medium",
  "slice_id": "<when present>"
}

Subject: consensus-timeout: <phase> [<priority>] (matches the existing OVERSEER_ALERT subject format).

Tests

• orchestrator/tests/test_pipelines_routes.py — three tests that asserted on pipeline.decisions[0].question migrate to assert on the OVERSEER_ALERT message and its metadata. The add_decision-failure regression test re-tooled to cover message-store add failures (still logged with exc_info).
• orchestrator/tests/test_consensus_polling.py — test_timeout_creates_hitl_decision renamed to test_timeout_publishes_overseer_alert; assertion migrates to capture and inspect the published alert.

Docs

• docs/guides/concurrent-execution.md — timeout-handling section reflects the new alert flow and metadata.
• docs/guides/sdlc-pipeline.md — troubleshooting note points at the OVERSEER_ALERT notification flow plus the cancel_task / restart_phase / provide_input levers.

SDLC skill — no changes; the existing OVERSEER_ALERT handling at skills/sdlc/SKILL.md already covers this anomaly type (id-based dedup, prompt with Check agent logs / Acknowledge / Cancel pipeline).

Migration note for operators

Any operator-side automation watching pending_decisions for the strings "BRC consensus failure" or "Consensus not reached after N minutes" must migrate to scanning recent_messages for OVERSEER_ALERT with metadata.anomaly_type == "consensus-timeout". The previous decision shape is no longer produced.

Test plan

• pytest orchestrator/tests/test_pipelines_routes.py — 15/15 pass.
• pytest orchestrator/tests/test_consensus_polling.py orchestrator/tests/test_consensus_timeout_recheck.py orchestrator/tests/test_slice_run_loop_integration.py — pass.
• make lint — pass.
• Soak: confirm a real pipeline timeout (synthetic or natural) renders the alert via /sdlc rather than opening a choice decision.

Related

• Parent: Auto BRC-consensus-failure HITL decision at 30 min misfires on healthy long-running phases #2243 (step 1 progress gate landed in Fix #2243: progress gate before BRC consensus-failure HITL decision #2254; step 2 per-phase defaults filed separately).
• Adjacent: [#1921 follow-up] Implement phase still force-kills at 90 min — post_timeout_budget is hardcoded 3600s with no per-phase / per-complexity knob #2245 post-timeout rebaseline runs after this point in the loop, so the alert fires once and the pipeline keeps trying to converge.

[egg-reviewer]

No agent-mode design concerns.

This PR moves toward agent-mode principles by replacing a rigid binary choice HITL gate (Continue waiting / Accept current state / Abort phase) with a non-blocking OVERSEER_ALERT notification. That's a clear "prefer what over how" win — instead of forcing the operator into three pre-defined options (two of which silently destroyed work in every documented misfire), the platform just informs them and lets them use their existing tool surface (cancel_task / restart_phase / provide_input) at their own discretion.

The alert metadata (anomaly_type, phase, blocking_agents, latest_proposal_at, latest_heartbeat_at, consensus_timeout_minutes, priority, slice_id) is lightweight orienting context, not pre-fetching — exactly the kind of "small amounts of structured data that inform the task" the guide calls out as fine. It also has a legitimate downstream consumer (SDLC skill's id-based dedup and notification flow), so the structured shape is justified.

— Authored by egg

[egg-reviewer]

Review

The migration from auto-HITL choice to OVERSEER_ALERT is the right protocol shape and the refactor is mostly clean. No blocking correctness or security issues. A handful of non-blocking issues below — primarily a subject-format mismatch with the SDLC skill's "Check agent logs" parser, plus some minor polish.

Non-blocking

1. Alert subject doesn't match the format the skill parser expects

skills/sdlc/SKILL.md:432 documents the alert subject format as <anomaly_type>: <agent_role> [<priority>] and tells the host to "extract the agent role from the alert subject" so it can call get_container_logs for the affected agent. This PR emits subjects of the shape consensus-timeout: <phase> [<priority>] — the second field is the phase (e.g. implement), not an agent role.

What that buys the operator when they pick "Check agent logs" (orchestrator/mcp_tools.py:2155):

filtered = [c for c in containers if c.get("agent_role") == agent_role]
if filtered:
    containers = filtered

agent_role="implement" matches no container, the filter falls through to the unfiltered list, and the operator gets logs for an arbitrary running container — not the blocking agent. Degraded UX rather than fully broken: the body has Blocking agents: …, and "Acknowledge" / "Cancel pipeline" still work. Worth either:

• putting the first blocking role in the subject (consensus-timeout: <blocking_role> [<priority>]) and moving the phase into metadata/body only, or
• relaxing the SKILL.md convention to admit the consensus-timeout shape and parsing metadata.blocking_agents instead of the subject.

The slice-cascade alert at orchestrator/routes/pipelines.py:10466 (slice-cascade-block: <slice_id>) has the same issue — that's a pre-existing wart, but worth deciding the convention now that two emitters have skipped the role field.

2. Body wording about provide_input is misleading post-#2264

orchestrator/routes/pipelines.py:9768:

"The pipeline continues to poll for convergence. If you want to "
"intervene, use `cancel_task` to stop the pipeline, `restart_phase` "
"to retry, or `provide_input` against any open agent decision."

The whole point of this PR is that consensus timeout does not open a decision anymore. Telling the operator to provide_input "against any open agent decision" implies one was opened by this very alert. Either drop the provide_input clause or qualify it (if any other agent decision happens to be open).

Also worth noting: the post-timeout polling budget is bounded (post_timeout_budget = 3600 at pipelines.py:11538). "Continues to poll for convergence" reads as open-ended; in practice the operator has up to 60 minutes before containers are force-killed. Adding "(up to ~60 min before force-kill)" would set the right expectations.

3. blocking_agents list mismatch on the escalate path

The escalate path means the tracker found critical blockers. But the alert metadata writes through whatever the call site passed (consensus.get("blocking_agents", [])), which is the full unconfirmed-roles set including advisory blockers (peer_consensus.py:1485). The high-priority alert ends up listing advisory roles too, which dilutes the signal. The tracker's handle_timeout() already returns critical_blockers in _brc_timeout_result — _publish_consensus_timeout_alert could prefer that on the escalate branch.

4. _latest_active_role_heartbeat has no direct unit test

orchestrator/routes/pipelines.py:9682-9720 mirrors the heartbeat half of _check_brc_progress_gate but has no equivalent dedicated test. The four migration tests at test_pipelines_routes.py:69-253 all leave active_role_names unset, so _latest_active_role_heartbeat([]) short-circuits at the if not active_role_names: return None guard before exercising the health-monitor lookup, lock snapshot, or active-role filter. The integration test at test_consensus_polling.py:296 does pass through with active_role_names, but it doesn't mock health_monitor and only asserts on priority/blocking_agents — latest_heartbeat_at is invisible. The TestBrcProgressGate fixture at test_pipelines_routes.py:276 already has a _patch_health_monitor helper that would translate directly. Add at least: empty roles → None, recent heartbeat → datetime, only-inactive heartbeats → None, monitor unavailable → None.

5. exc_info=True inconsistency on the warning paths

_publish_consensus_timeout_alert's outer except logs with exc_info=True (pipelines.py:9821), but the proposal-lookup except at pipelines.py:9879 and the heartbeat-lookup except at pipelines.py:9716 only log error=str(e). If those throw, the traceback is lost — diagnosing why metadata is None would mean digging through orchestrator logs without a stack. Three lines that all do the same thing should log the same way.

6. Minor test/code polish

• test_pipelines_routes.py:44 — _capture_alerts(monkeypatch_target=None) parameter is never used. Drop it.
• pipelines.py:9602 (_check_brc_progress_gate docstring) and pipelines.py:11476-11478 still read "auto-HITL decision" / "decision-15 / decision-17 fired"; semantically those are now alerts. Comments are historically correct but a future reader will look for HITL decisions in code that no longer creates them.
• _handle_brc_consensus_timeout keeps the store: StateStore parameter as noqa: ARG001. If the production call site can stop passing it (one-line edit at pipelines.py:11520), the parameter and the _make_store() test fixture both go away cleanly. The PR comment says it's kept "for call-site compatibility" — but the call site is internal, so that's a self-imposed constraint.

Verified working

• _handle_brc_consensus_timeout is called exactly once per _run_concurrent_phase invocation (pipelines.py:11515), and the if escalate / elif not _brc_handled branches are mutually exclusive — at most one alert per timeout. The tracker's is_timeout_handled idempotency keeps duplicate alerts off if the handler is re-entered.
• CONSENSUS_TIMEOUT audit-event preservation on the no-tracker / import-error fallback is correct; the escalate path correctly skips the host emit because PeerConsensusTracker.handle_timeout already emits CONSENSUS_FAILURE internally.
• MessageType.OVERSEER_ALERT == "OVERSEER_ALERT"; to_role="all" and from_role="orchestrator" ride past the SDLC skill filter (which keys only on type, not from_role, despite what SKILL.md:410 documents).
• _get_message_store() failure path (None factory) and add_message failure path are both logged with exc_info and don't crash the polling loop. Test coverage for both is in place.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Lint/Custom Checks": 1}

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
scripts/file-size-allowlist.yaml	Additive (baseline metadata)	Took PR-side baseline for orchestrator/routes/pipelines.py (15702 lines / 684817 bytes — reflects this PR's growth) and main-side baseline for scripts/select_tests.py (1875 lines / 75206 bytes — reflects #2262's bare-name AST resolver). Both values match the actual post-merge file sizes (wc -l -c confirms).

Other files auto-merged cleanly (no conflict markers): Makefile, docs/guides/concurrent-execution.md, docs/guides/sdlc-pipeline.md, plus the new files from main (scripts/check-file-sizes.py, test files).

Verification:

• python3 scripts/check-file-sizes.py → exit 0 (warnings only, no growth violations)
• make lint → exit 0
• YAML re-parses cleanly; both contested baselines match wc -l -c on the merged tree

Please review: Nothing semantic changed — the conflict was purely about baseline byte/line counts in the file-size allowlist where this PR's pipelines.py growth and main's select_tests.py growth (#2262) both needed to be recorded.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns (re-review at fdef6072).

The delta since c259d35 addresses the other reviewer's general code-quality feedback (subject role slot, narrowed blocking_agents on escalate, body wording, exc_info=True on warnings, direct unit tests for _latest_active_role_heartbeat, doc clarifications). None of those changes alter the agent-mode design profile — the alert is still a non-blocking OVERSEER_ALERT with lightweight orienting metadata (anomaly_type, phase, blocking_agents, latest_proposal_at, latest_heartbeat_at, priority, slice_id), no pre-fetched diffs or logs, and the operator continues to use the existing tool surface (cancel_task / restart_phase) at their own discretion.

If anything, the subject-format fix (consensus-timeout: <agent_role> [<priority>] matching skills/sdlc/SKILL.md's convention so "Check agent logs" can extract a real role for get_container_logs) is a small positive — it preserves SDLC-skill routing without baking richer context into the alert itself.

— Authored by egg

[egg-reviewer]

Re-review at fdef6072

The delta since c259d35 cleanly addresses every blocking and non-blocking item from my prior review. Behaviour is correct and the new test coverage is the right shape.

Previous feedback — addressed

1. Subject role slot — _publish_consensus_timeout_alert now derives subject_role = blocking_agents[0] if blocking_agents else phase_value (orchestrator/routes/pipelines.py:9760), so "Check agent logs" extracts a role the host can hand to get_container_logs. The phase-only fallback is documented in the inline comment.
2. Body wording / provide_input — Dropped the misleading "any open agent decision" clause and added the bounded ~60 min force-kill budget so "continues to poll" reads correctly (orchestrator/routes/pipelines.py:9777-9780).
3. Critical-blockers narrowing on escalate — _handle_brc_consensus_timeout now extracts critical_role_names from _brc_timeout_result.get("critical_blockers") and prefers it over the caller-supplied blocking_agents (pipelines.py:9902-9914). Both reviewer_role and producer_role per entry are included with dedupe — that matches the entry shape in peer_consensus.py:1397.
4. _latest_active_role_heartbeat direct tests — TestLatestActiveRoleHeartbeat (test_pipelines_routes.py:486) covers empty roles, recent heartbeat, picking-most-recent, inactive-role filtering, monitor-unavailable, no-heartbeats, and the exc_info=True logging path. The patches go through _patch_health_monitor and route the from health_monitor import … lookup to the mock — tests should pass on the no-imports path too.
5. exc_info=True consistency — Both the proposal-lookup except (pipelines.py:9893) and heartbeat-lookup except (pipelines.py:9719) now log with exc_info=True, matching the publish helper's outer except. A traceback survives if any of them throws.
6. Polish — _capture_alerts(monkeypatch_target=None) parameter dropped; the run-loop comment at pipelines.py:11498-11502 and the _check_brc_progress_gate docstring updates the "auto-HITL decision" wording to "consensus-timeout alert / OVERSEER_ALERT".

Defensive coverage worth calling out

test_brc_escalate_falls_back_to_caller_blocking_agents (test_pipelines_routes.py:170) exercises the case where the tracker's escalate result omits critical_blockers (older return shape, or a matrix that cleared between handle_timeout() calls). The fallback is escalate_blocking = critical_role_names or blocking_agents, so the alert still has roles in the subject. Good defensive choice.

Conflict resolution at the merge

The scripts/file-size-allowlist.yaml resolution in fdef6072 is correct. PR-side baseline for orchestrator/routes/pipelines.py (15702 lines / 684817 bytes — verified by wc -l -c on the merged tree) and main-side for scripts/select_tests.py (1875 / 75206 — from #2262) both match the actual post-merge file sizes.

Non-blocking suggestions

a. Stale "HITL escalation" wording in orchestrator/models.py

models.py:378 still describes consensus_timeout_minutes as "Timeout for consensus before HITL escalation", and models.py:384 references "the consensus-timeout HITL decision". Same flavour of stale wording the previous review flagged in pipelines.py/run_concurrent_phase. Both fields now drive an OVERSEER_ALERT, not a HITL decision. A future reader hunting for "where the HITL decision opens" will land here and find a description that still claims one does. One-line fix on each — non-blocking polish, but the same justification as the previous round's docstring refresh.

b. PR description references the old subject format

The description block under "Alert metadata schema" still states Subject: consensus-timeout: <phase> [<priority>]. After this PR's fix, that's inaccurate — the second slot is <agent_role> (with <phase> only as a fallback when no blocking role is reported). Worth syncing the description so a future cherry-picker doesn't recreate the original wart.

c. _check_brc_progress_gate docstring at pipelines.py:9595

The "Heartbeat-cadence contract: the decision-17 path …" sentence still uses "decision-17" as a name for the coder-mid-merge-conflict path. Strictly speaking that's a historical reference (decision-17 was the original incident's decision id on issue-1557-v2), so it's not wrong — but for consistency with the rest of the comment refresh in this PR, "the alert-17 path" or just "the coder-mid-merge-conflict path" reads cleaner against the post-#2264 protocol. Marginal.

d. Author-chosen non-fix: store: StateStore parameter

The PR keeps _handle_brc_consensus_timeout(... store: StateStore, ...) with # noqa: ARG001 — kept for call-site compatibility (#2264). That's the explicit opt-out the previous review allowed for ("the call site is internal, so that's a self-imposed constraint"). Fine to leave; flagging just so the next reviewer doesn't re-litigate.

— Authored by egg

[james-in-a-box]

Addressed the non-blocking polish from the latest re-review at 42366c6:

(a) Stale "HITL escalation" wording in orchestrator/models.py — Fixed. Both consensus_timeout_minutes (line 378) and brc_consensus_progress_gate_seconds (line 384) now describe the post-#2264 OVERSEER_ALERT flow instead of "HITL escalation" / "consensus-timeout HITL decision".

(c) _check_brc_progress_gate docstring — Reworded to lead with "coder-mid-merge-conflict path" and treat decision-17 as a pre-#2264 historical reference rather than the current name.

(b) PR description references the old subject format — I cannot edit the PR description (gh pr edit rejects with Edit denied: PR #2269 is not owned by james-in-a-box or configured user (author: jwbron)). Flagging here so @jwbron can update the "Alert metadata schema" block:

-Subject: `consensus-timeout: <phase> [<priority>]` (matches the existing `OVERSEER_ALERT` subject format).
+Subject: `consensus-timeout: <agent_role> [<priority>]` — populated from the first `blocking_agents` entry so the SDLC skill's "Check agent logs" can extract a real role for `get_container_logs`. Falls back to `<phase>` when no blocking role is reported (e.g. tracker-import-error path).

(d) store: StateStore parameter — Acknowledged as the explicit opt-out from the previous round; no change.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns (re-review at ea42d12).

The only PR-authored delta since fdef6072 is 42366c678 — a docs/wording refresh that retires "HITL escalation" / "decision-17" phrasing in orchestrator/models.py (two Field(description=...) strings) and the _check_brc_progress_gate docstring in orchestrator/routes/pipelines.py, plus a 1-line/77-byte allowlist bump. No control flow changes. This addresses items (a) and (c) from the prior general-review pass and keeps the documentation consistent with the post-#2264 protocol (consensus-timeout publishes an OVERSEER_ALERT rather than opening a HITL decision).

Nothing here changes the alert's design profile: still a non-blocking broadcast with lightweight orienting metadata, no pre-fetched diffs/logs, operator continues to use the existing tool surface (cancel_task / restart_phase) at their own discretion.

— Authored by egg

[egg-reviewer]

Re-review at ea42d12a (PR-side delta 42366c67)

The single PR-authored commit since my last review (fdef6072) is 42366c67, which addresses two of the three actionable non-blocking items from the previous round. No new code paths, no semantic changes — docstring/Field(description=...) text only. Cleanly merged with main at ea42d12a.

Previous feedback — addressed

1. (a) Stale "HITL escalation" wording in orchestrator/models.py — Fixed.

   • consensus_timeout_minutes (models.py:380): now "Timeout for consensus before publishing an OVERSEER_ALERT (#2264)".
   • brc_consensus_progress_gate_seconds (models.py:410-413): now "Defer the consensus-timeout OVERSEER_ALERT while ..." with the issue tag updated to (#2243, #2264).
     Both fields' descriptions now match the actual post-#2264 behaviour they configure.

2. (c) _check_brc_progress_gate docstring at pipelines.py:9595-9598 — Reworded.
   The "Heartbeat-cadence contract" paragraph now leads with "the coder-mid-merge-conflict path (no CONSENSUS_PROPOSE yet, only container heartbeats — the original incident's decision-17 flavour, pre-#2264)". decision-17 is correctly demoted to a historical-flavour reference rather than a current name.

Previous feedback — explicitly deferred

3. (b) PR description still references the old subject format — Author flagged in their feedback comment that gh pr edit rejects the edit (PR #2269 is not owned by james-in-a-box). This needs @jwbron to update the "Alert metadata schema" block to reflect consensus-timeout: <agent_role> [<priority>] (with <phase> as fallback). Non-blocking, but worth syncing before merge so a future cherry-picker doesn't recreate the original wart.

4. (d) store: StateStore parameter — Acknowledged as the explicit opt-out from the previous round; # noqa: ARG001 — kept for call-site compatibility (#2264) at pipelines.py:9841.

Merge integration

The main-side merge in ea42d12a cleanly integrated:

• scripts/file-size-allowlist.yaml — main's c35517064 dropped per-file lines/bytes baselines in favour of issue only (the merge schema rationalisation). The PR's +1 line / +77 bytes bump in 42366c67 is therefore obsolete post-merge — but harmless: the new schema makes the field unnecessary, and the file is correctly using the simplified format at HEAD.
• orchestrator/models.py added 41 lines from main (new post_consensus_iteration_budget_seconds and post_consensus_max_total_seconds fields). Inserted between the consensus-timeout field this PR rewrote and the progress-gate field, so the description updates survived intact.
• orchestrator/routes/pipelines.py added 83 lines from main (post-timeout polling loop). The consensus-timeout call site at pipelines.py:11562 and the _handle_brc_consensus_timeout / _publish_consensus_timeout_alert helpers are unchanged. Verified by reading the post-merge file: subject-role logic, critical-blockers narrowing, body wording, and exc_info=True consistency are all preserved.

Verified clean post-merge

• orchestrator/models.py:380, 410-413 — descriptions match the actual code path at pipelines.py:9726-9833.
• pipelines.py:9595-9598 docstring — coder-mid-merge-conflict framing reads correctly against the _latest_active_role_heartbeat lookup at pipelines.py:9683 and the heartbeat path at pipelines.py:9696-9719.
• Other references to "HITL decision" / "HITL escalation" in models.py (:5, :79, :440, :720, :889-917) and pipelines.py (:243, :261, :6354, :7863, :7896, :8537, :9393, :9480-9555) are about the generic HITL decision system or other flows (refine, redirect, incomplete-consensus). Not in scope for this PR's docstring refresh — leaving them untouched is correct.

No blocking issues. Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

16 previous review(s) hidden.