Fix #2243: progress gate before BRC consensus-failure HITL decision

[jwbron]

Summary

• Add a progress gate in _run_concurrent_phase's polling loop that defers the auto consensus-failure HITL decision while BRC bus signals (CONSENSUS_PROPOSE / ACK / NACK) or container heartbeats have fired within brc_consensus_progress_gate_seconds (default 300s).
• New aggregate accessor PeerConsensusTracker.get_latest_progress_timestamp() (and ApprovalMatrix.get_latest_entry_timestamp()) gives the gate a single signal to read.
• Heartbeats are filtered to roles in the current phase's active_executions so cross-phase pollution in the singleton HealthMonitor can't keep the gate deferring forever.
• Default consensus_timeout_minutes stays 30 — the gate is purely additive.

Why

Per the issue post-mortem, decision-15 fired ~3 min before plan-phase consensus completed; decision-17 fired 12 min before coder's first commit landed. "Continue waiting" was the only correct answer in every recorded misfire, so the platform shouldn't have opened the decision in the first place.

The gate covers both documented misfires: BRC-bus signals catch decision-15 (plan phase had recent ACK traffic), and container heartbeats catch decision-17 (coder was alive mid-merge-conflict-resolution before its first CONSENSUS_PROPOSE).

Failures in either collector are logged at WARNING and treated as "no signal from that source" — a crashed collector can't silently keep us off the HITL surface.

This is step 1 of the three-step direction in #2243; per-phase timeouts (step 2) and OVERSEER_ALERT migration (step 3) remain follow-ups.

Test plan

• Unit tests for get_latest_progress_timestamp (no-activity → None; proposal-only; ACK advances past proposal).
• Unit tests for _check_brc_progress_gate: disabled gate, recent proposal defers, stale proposal does not, recent heartbeat defers when bus is silent, stale heartbeat does not, inactive-role heartbeat ignored, no signals → no defer, tracker collector failure logged + no defer, heartbeat collector failure logged + no defer.
• CI: make lint / make test-all on the PR.

[egg-reviewer]

Reviewed the diff end-to-end and traced the gate's interaction with _run_concurrent_phase, the singleton HealthMonitor, and the PeerConsensusTracker. The design is sound: the gate is purely additive (deferring an existing decision, never opening one), the two-source signal (BRC bus + container heartbeats) maps cleanly to the documented decision-15 / decision-17 misfires, and the failure-mode handling (collector crashes never silently keep the gate on) is the right call. No blocking issues.

A handful of non-blocking observations and questions below.

Non-blocking

1. now_mono is wall-clock, not monotonic — routes/pipelines.py:9594

now_mono = time.time()

time.time() is wall-clock. The arithmetic is correct because _last_heartbeat is also populated from time.time() (health_monitor.py:268, 288, 294, 408), but the variable name actively misleads. Rename to now_wallclock or just now_epoch. Don't mix it with now_dt — at minimum a comment that they're deliberately two different clocks.

(Aside: because both ends of the heartbeat comparison are wall-clock, an NTP step on the orchestrator host can make (now - latest_hb) negative or skip the gate window. Real but very narrow; calling out for the per-phase-timeout follow-up.)

2. Empty active_role_names accepts all heartbeats — routes/pipelines.py:9626

active_set = set(active_role_names)
...
if active_set and agent_id not in active_set:
    continue

When active_set is empty, the and short-circuits and the filter is skipped, so every stale heartbeat in the singleton HealthMonitor counts. Today this can't fire because _run_concurrent_phase exits earlier when there are no live containers, but the contract reads inverted from the comment two lines above ("filters out cross-phase pollution"). Either explicitly:

if not active_set:
    return False, None  # nothing to gate on

or drop the if active_set and so an empty set means "match nothing."

3. Same-role cross-phase pollution slips past the filter

The role-name filter handles the different-role case (refiner heartbeat lingering during a coder phase, which is what the test covers). But coder reappears in implement / implement-fix / fix-on-PR phases, and _last_heartbeat["coder"] is only popped on clear_agent_state (i.e. a respawn). If a coder from a prior phase emitted a heartbeat seconds before that phase ended, the next phase's coder inherits a "live" heartbeat it didn't earn.

In practice 30 min of consensus_timeout elapses before this matters, so it's unlikely to bite — but a phase boundary clear (or stamping the heartbeat key with the phase) would close it. Worth a TODO referencing #2243 step 2.

4. Defensive TypeError fallback is dead code — routes/pipelines.py:9608-9611

try:
    tracker = get_peer_consensus_tracker(pipeline_id, slice_id)
except TypeError:
    tracker = get_peer_consensus_tracker(pipeline_id)

peer_consensus.get_peer_consensus_tracker(pipeline_id, slice_id=None) already accepts both shapes (peer_consensus.py:1783-1787). The fallback was carried over from _handle_brc_consensus_timeout (routes/pipelines.py:9676-9679) where the same dead branch lives. Drop the fallback in both places, or at least don't propagate it to new code.

5. Test assertion >= contradicts the test name — test_peer_consensus_integration.py:325

def test_ack_advances_progress_timestamp_past_proposal(self, tracker):
    ...
    assert progress_ts >= proposal_ts

>= allows equality, which would pass even if the ACK didn't advance the timestamp at all (e.g. if get_latest_entry_timestamp ever changed to return the proposal's record timestamp by accident). The name says "advances ... past proposal." Use > so the test would actually catch a regression where ACKs stop advancing progress. datetime.now(UTC) has microsecond resolution, so back-to-back calls are reliably strictly increasing.

6. No integration coverage for the polling-loop wiring

TestBrcProgressGate exercises _check_brc_progress_gate in isolation, but nothing tests the actual splice in _run_concurrent_phase (lines 11283-11320) — the time.sleep(poll_interval); continue branch, the once-per-state-transition logging, the _progress_gate_deferring flag's reset path. _run_concurrent_phase is hard to unit-test, but a small test patching _check_brc_progress_gate to return (True, "x") for N iterations then (False, None) and asserting the loop kept polling instead of calling _handle_brc_consensus_timeout would catch a regression where the gate's continue is removed or misplaced.

7. Operator visibility while the gate is deferring is one INFO log

If the bus/heartbeats stay alive for, say, 2 hours past consensus_timeout, the operator only sees the single "Consensus timeout deferred by progress gate" line that fired at the first defer. There's no surface saying "we're 90 min past nominal timeout, gate still deferring." Not blocking — OVERSEER_ALERT migration is the step-3 follow-up — but consider an egg-orch progress query / status field so an operator polling pipeline state can see the gate is active.

8. Heartbeat dependency is load-bearing for the decision-17 path

The PR's claim is that container heartbeats catch decision-17 (coder mid-merge-conflict before any CONSENSUS_PROPOSE). The gate window is 300s, so the agent must emit something (PROGRESS, HEARTBEAT message, or progress update via routes/progress.py) at least every 5 minutes to keep the gate engaged. If a stuck-but-alive agent stops emitting heartbeats during a long subprocess (e.g. git rebase blocked on a merge driver), the gate falls open and we're back to the pre-fix behavior. Worth explicitly documenting the heartbeat-cadence contract this gate relies on, ideally with a pointer to wherever sandbox heartbeats are scheduled.

---

<!-- has-suggestions -->

— Authored by egg

[james-in-a-box]

Thanks for the careful read. Addressed in 292a5a6:

Fixed

1. now_mono rename — now now_wallclock, with a comment block explaining both clocks are deliberately wall-clock and an NTP step is a known narrow gap to revisit alongside per-phase timeouts.
2. Empty active_role_names — added an explicit if not active_role_names: return False, None early-return before the heartbeat block, and dropped if active_set and so the body's contract matches the comment. Added test_empty_active_roles_does_not_defer_on_heartbeat to lock it in.
3. Same-role cross-phase pollution — added a TODO(#2243 step 2) in the docstring describing the gap (coder reappearing across implement / implement-fix / fix-on-PR) and that per-phase timeouts will likely subsume it.
4. Dead TypeError fallback — dropped in both _check_brc_progress_gate and _handle_brc_consensus_timeout since get_peer_consensus_tracker(pipeline_id, slice_id=None) already accepts both shapes.
5. >= → > — test now uses > with a comment explaining microsecond resolution makes back-to-back calls reliably strictly increasing.
6. Heartbeat-cadence contract — added a docstring section explicitly calling out that the decision-17 path depends on heartbeats firing within gate_seconds, and that a long uninterruptible subprocess (e.g. git rebase blocked on a merge driver) can starve them and re-open the pre-fix gap.

Deferred (non-blocking, tracked)

6. Polling-loop wiring coverage — agreed the splice in _run_concurrent_phase isn't exercised by TestBrcProgressGate. I left this for the per-phase-timeout follow-up since _run_concurrent_phase is hard to unit-test in isolation; the helper-level tests catch the gate logic, and the splice itself is small and reviewable. Happy to add a patched-iteration test if you'd like before merge.
7. Operator visibility while deferring — leaving for the step-3 OVERSEER_ALERT migration as you suggested. The single INFO log on first defer keeps log volume bounded; the step-3 work will surface gate state on the operator dashboard.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 1}

[egg-reviewer]

Re-reviewed the delta f2a682a4..a15ccf34. The two follow-up commits (292a5a6 addressing review feedback, a15ccf3 dropping the now-obsolete test) faithfully address the previous concerns and don't introduce new issues.

Prior feedback verification

1. now_mono rename — addressed. Renamed to now_wallclock (pipelines.py:9627) with the explicit comment block calling out that both clocks are wall-clock and the NTP-step gap is acknowledged for the per-phase-timeout follow-up. Comment is concrete enough to survive code drift.
2. Empty active_role_names short-circuit — addressed. Explicit early return at pipelines.py:9655 (if not active_role_names: return False, None) and the loop body at pipelines.py:9665 is now if agent_id not in active_set: continue (the misleading if active_set and … is gone). New test_empty_active_roles_does_not_defer_on_heartbeat (test_pipelines_routes.py:286-302) locks the contract in.
3. Same-role cross-phase pollution — addressed. TODO at pipelines.py:9613-9620 documents the gap (coder reappearing across implement / implement-fix / fix-on-PR) and points at #2243 step 2. Reasonable to defer; the role-name filter handles the different-role case the PR was scoped to.
4. Dead TypeError fallback — addressed. Dropped in both _check_brc_progress_gate (pipelines.py:9635) and _handle_brc_consensus_timeout (pipelines.py:9704). Verified peer_consensus.get_peer_consensus_tracker(pipeline_id, slice_id=None) accepts both shapes natively — the fallback was indeed unreachable.
5. >= → > test assertion — addressed. test_peer_consensus_integration.py:332 now uses > with a comment justifying it on datetime.now(UTC) microsecond resolution.
6. Heartbeat-cadence contract — addressed. Docstring section at pipelines.py:9594-9602 calls out that decision-17's catchment depends on heartbeats firing within gate_seconds and a long-running uninterruptible subprocess (e.g. git rebase on a merge driver) re-opens the pre-fix gap. Good operator-facing surface.
7. Polling-loop wiring coverage / operator visibility while deferring — explicitly deferred in the response, tracked under the per-phase-timeout / OVERSEER_ALERT follow-ups. Reasonable.

On a15ccf3 (stale test removal)

test_typeerror_falls_back_to_pipeline_scope in test_slice_run_loop_integration.py was correctly removed: it asserted exactly two calls (raise + retry), but with the fallback dropped the production code makes a single call and lets the outer except Exception: at pipelines.py:9716 catch any residual TypeError. Verified the catch path falls through to the standard HITL-escalation branch (elif not _brc_handled: at pipelines.py:9737, which calls _persist_hitl_decision with the standard "Consensus not reached after N minutes" question) — i.e. graceful degradation, not a crash. The remaining tests in TestHandleBrcConsensusTimeoutSliceId still exercise both the slice and pipeline-scope forwarding paths, so coverage of the live behavior is intact.

Non-blocking nits

• Sibling dead-code TypeError fallbacks still present. The same try / except TypeError / retry without slice_id pattern lives at pipelines.py:10814-10821 and pipelines.py:11008-11011 — both inside _run_concurrent_phase, both calling get_peer_consensus_tracker with slice_id. The current signature can't raise TypeError, so they're dead branches in the same way the two now-removed fallbacks were. Not in this PR's diff — flagging as a small follow-up so they don't keep propagating to new code.

No blocking issues. Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

4 previous review(s) hidden.