docs: document focal-agent activity gate (#2190) [doc-updater] - #2309
Conversation
…uard Co-authored-by: egg <egg@localhost>
There was a problem hiding this comment.
Review
This is a docs-only PR that walks through commit 6cf60ba. I verified each claim against the current code in orchestrator/. One substantive accuracy issue, plus a couple of polish items.
Blocking
1. _PRIOR_INTERVENTIONS list omits restart_phase — docs/guides/pipeline-health-monitoring.md:410
The doc enumerates the bypass set as:
"no prior corrective intervention of any kind (
nudge,redirect,issue,slack,restart_agent,hitl) in the agent's redirect history"
But the actual constant in orchestrator/overseer/decision_maker.py:66-68 is:
_PRIOR_INTERVENTIONS: frozenset[str] = frozenset(
{"nudge", "redirect", "issue", "slack", "restart_agent", "restart_phase", "hitl"}
)restart_phase is missing from the doc's list. This is not a stale-doc miss — the original review feedback that produced this set explicitly added it ("Item A: Expand _PRIOR_INTERVENTIONS to span every action in the decision-maker vocabulary — adds issue, slack, and restart_phase to the existing nudge / redirect / restart_agent / hitl set"). The doc enumerates the set, so it is committing to the contents — an operator reading this would conclude a prior restart_phase does not bypass the guard, when it actually does. Since restart_phase is itself a Step-4 escalation in the very same Corrective Action Ladder table immediately above (docs/guides/pipeline-health-monitoring.md:403), the omission is materially misleading.
Fix: add restart_phase to the parenthetical list.
Non-blocking
2. Subject-verb agreement — docs/guides/pipeline-health-monitoring.md:410
"A deterministic post-hoc check (
_enforce_no_first_stall_restart) downgraderestart_agenttohitlwhen…"
Should be "downgrades".
3. last_activity == 0.0 reads as a magic-value compare — docs/guides/pipeline-health-monitoring.md:226
The actual code uses a named module-level sentinel (_NEVER_SEEN_ACTIVITY: float = 0.0, health_monitor.py:73) and compares with <=, not == (health_monitor.py:278). The 0.0 wasn't a magic number in the implementation — it was deliberately promoted to a named constant in the second review-feedback round on #2283. Suggest either last_activity == _NEVER_SEEN_ACTIVITY or just "no CONTAINER_ACTIVITY event has been recorded for the agent" so the prose tracks the named-constant intent rather than reintroducing the magic value the implementation took pains to remove.
Verified accurate
- Default
orchestrator_activity_quiet_seconds = 120,ge=0: matchesmodels.py:498-507. - Gate ordering (focal-agent first, peer-progress second, OR'd, escalated flag not set on defer): matches
health_monitor.py:704-706and:803-805. - Disabled-gate semantics (
threshold <= 0returns(False, None)): matcheshealth_monitor.py:271-272. CONTAINER_ACTIVITYsource (commit observer'sregister_commitroute): matchesroutes/commit_authorship.py:52-78.- Three-condition trigger for
_enforce_no_first_stall_restart(action ==restart_agent, classification ∈ {stuck,needs_help}, no prior intervention): matchesdecision_maker.py:222-231. - Preservation of original recommendation in the HITL message: matches
decision_maker.py:233-244. - Configuration-table description text: matches the Pydantic field description verbatim.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Add restart_phase to _PRIOR_INTERVENTIONS list (matches decision_maker.py) - Fix subject-verb agreement: downgrade -> downgrades - Replace last_activity == 0.0 magic-value reference with named-constant prose
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review
All three concerns from the previous review are correctly addressed in 9786540a. Verified each fix against the implementation:
Blocking issue resolved
_PRIOR_INTERVENTIONS enumeration — docs/guides/pipeline-health-monitoring.md:410
The parenthetical now reads (nudge, redirect, issue, slack, restart_agent, restart_phase, hitl). This matches _PRIOR_INTERVENTIONS in orchestrator/overseer/decision_maker.py:66-68 exactly, and matches the docstring on _enforce_no_first_stall_restart (decision_maker.py:211-220) which spells out "every action in the decision-maker's vocabulary." No drift.
Non-blocking items resolved
Subject-verb agreement — docs/guides/pipeline-health-monitoring.md:410: downgrade → downgrades.
Magic-value reference — docs/guides/pipeline-health-monitoring.md:226: now reads "no CONTAINER_ACTIVITY event has ever been recorded for the agent (the implementation compares last_activity against the named module-level sentinel _NEVER_SEEN_ACTIVITY)". This tracks the named constant at health_monitor.py:73 and avoids the == vs <= operator pitfall in the previous prose (the actual check at health_monitor.py:278 uses <=).
Other claims rechecked, still accurate
- Gate 1 / Gate 2 ordering and OR semantics:
health_monitor.py:704-706. - Disabled-gate semantics for
orchestrator_activity_quiet_seconds = 0:health_monitor.py:271-272. - Configuration table defaults (
120/300,ge=0):models.py:498-507and:569-…. _enforce_no_first_stall_restartthree-condition trigger and HITL message preservation:decision_maker.py:222-246.
Docs-only delta is +2/-2 lines in a single file. No new claims to verify, nothing else regressed.
— Authored by egg
|
egg review completed. View run logs 3 previous review(s) hidden. |
Update pipeline health monitoring docs to reflect changes from 6cf60ba (Fix #2190: focal-agent activity gate on heartbeat/progress stall alerts, #2283):
heartbeat_timeout/progress_stallalert. Gate 1 is the new focal-agentCONTAINER_ACTIVITYgate (Overseer agent-heartbeat-stall detector escalates to 'restart container' against agents that are running legitimate long-blocking tool calls (e.g. background pytest) #2190) — defers when the agent has committed recently even without a bus-level HEARTBEAT. Gate 2 is the existing peer-progress gate (Overseer plan-phase nudge thresholds (heartbeat 180s, post-ACK confirm 180s) feel tight for complex pipelines #2242). Added escape hatch note fororchestrator_activity_quiet_seconds = 0.orchestrator_activity_quiet_secondsfield (default: 120, added toPipelineConfigin this commit)._enforce_no_first_stall_restart) — documents thatrestart_agenton a first-occurrencestuck/needs_helpclassification with no prior intervention history is deterministically downgraded tohitlto prevent destroying in-flight commits from agents mid-pytest.Triggered by: #2283
Authored-by: egg