Fix #2190: focal-agent activity gate on heartbeat/progress stall alerts

[jwbron]

Summary

Closes #2190.

The agent-heartbeat-stall and progress_stall detectors were sampling only message-bus HEARTBEAT traffic, so an agent legitimately blocked in a long tool call (e.g. a multi-minute background pytest via Bash+TaskOutput) appeared "silent" even while it was committing, calling mcp__task__add_commit, and exchanging tool results. The Tier-2 overseer then escalated to a destructive restart_agent recommendation that would have wiped in-flight commits.

This PR adds a focal-agent activity signal:

• New EventType.CONTAINER_ACTIVITY. Published by routes/commit_authorship.register_commit (the gateway commit observer's HTTP target) on every successful pipeline-scoped commit registration. Best-effort — publish failures don't affect the route response.
• HealthMonitor subscribes, tracks per-agent last_activity, and defers heartbeat_timeout / progress_stall alerts when activity has fired within orchestrator_activity_quiet_seconds (default 120s).
• Mirrors Overseer plan-phase nudge thresholds (heartbeat 180s, post-ACK confirm 180s) feel tight for complex pipelines #2242's _has_recent_peer_progress pattern; the two gates are OR'd — focal activity OR peer progress defers. The escalated flag is intentionally not set on defer so the next poll re-checks once activity goes stale.
• Softens the overseer's decide_corrective_action prompt to lead with mcp__egg__get_container_logs inspection and forbid embedding egg-orch container restart <id> as a first-line operator action for stall classifications. Restartable infrastructure errors (the _is_restartable fast-path) are unaffected.

Acceptance criteria from #2190

• No agent-heartbeat-stall against an agent with recent successful activity within orchestrator_activity_quiet_seconds (configurable, default 120s, separate from heartbeat threshold).
• Recommended-action ladder leads with mcp__egg__get_container_logs(...); container restart text removed from the first-line guidance.
• Gateway-side git_execute_success audit signal (deferred; see follow-ups — requires cross-process gateway → orchestrator plumbing).
• "Awaiting tool result" inference (deferred follow-up).
• Integration regression with real 5-min background pytest (deferred follow-up).

Test plan

• pytest orchestrator/tests/test_health_monitor.py orchestrator/tests/test_commit_authorship_routes.py — 130 passed
• pytest orchestrator/tests/test_overseer_decision_maker.py orchestrator/tests/test_overseer_classifier.py — 12 passed (overseer prompt regressions)
• ruff check + ruff format --check clean across all touched files
• Live verification on next pipeline run that touches the implement phase

[egg-reviewer]

Review of PR #2283 — focal-agent activity gate (#2190)

The intent is sound: a coder making real commits should not be killed by a "silent agent" detector. The defer-on-defer mechanism is small, additive, and the test surface is reasonable. However there are several issues that should be resolved before merging.

Blocking

1. Forward-references to _has_recent_peer_progress that does not exist on main

orchestrator/health_monitor.py:234 and the PR description both claim:

Complements #2242's _has_recent_peer_progress (peer signals; self-excluded). The two gates are OR'd: either the focal agent's own activity OR a peer's recent progress is sufficient to defer.

That function does not exist in the current codebase. It is introduced by PR #2268 (still open, unmerged, separate author-asserted feature for issue #2242). Anyone reading health_monitor.py after this PR merges in isolation will grep for _has_recent_peer_progress, find nothing, and have to guess whether it was renamed or removed. The "the two gates are OR'd" claim is also untrue in the merged state of this PR alone — there is exactly one gate.

Equally important: PR #2268 adds its _has_recent_peer_progress(agent_id, now) call in check_heartbeats and check_progress at the same site that this PR adds _has_recent_activity(agent_id, now). The two PRs will conflict during merge, regardless of which lands first. Whoever merges second has to re-resolve that block manually.

Fix one of:

• Drop the references to _has_recent_peer_progress entirely and describe this PR as standalone.
• If you intend to coordinate with PR #2268, say so explicitly in the comment ("Future #2242 work will OR this with a peer-progress gate; see PR #2268") so the reader knows the dependency is forward-looking, not present.

Either way, do not leave a :func: cross-reference to a function that does not exist on the branch's merge base.

2. Test name vs. behaviour contradiction in test_disabled_gate_no_suppression

orchestrator/tests/test_health_monitor.py:316-335:

def test_disabled_gate_no_suppression(self):
    """orchestrator_activity_quiet_seconds <= 0 disables the gate."""
    ...
    config = _make_config(
        orchestrator_heartbeat_timeout_seconds=60,
        orchestrator_activity_quiet_seconds=1,
    )
    # Pydantic enforces ge=1; we cannot construct 0 directly, so verify
    # that with a 1-second window, a slightly-stale activity event no
    # longer suppresses (equivalent semantics).

The docstring and test name claim the test exercises the disabled-gate path (threshold <= 0). The body sets threshold=1 and tests stale activity instead. These are not equivalent: the disabled-gate path returns immediately without acquiring the lock or examining last_activity (health_monitor.py:245-247); the stale-activity path runs the full check and returns False from the bottom branch. The test exercises the latter and proves nothing about the former.

This is exactly the "name-vs-behaviour contradiction" pattern the review rules call out as blocking. Resolve by either:

• Renaming to test_stale_activity_outside_window_no_suppression (which would then be a near-duplicate of test_stale_activity_does_not_suppress at line 278 — if so, just delete it).
• Or relaxing the Pydantic bound to ge=0 and writing a real test for the disabled path (see issue 3).

3. Dead disabled-gate code path

orchestrator/health_monitor.py:245-247:

threshold = self._config.orchestrator_activity_quiet_seconds
if threshold <= 0:
    return False, None

orchestrator/models.py:436-444 declares the field with ge=1. Pydantic rejects 0 and negative values at construction time, so this branch is unreachable through any normal config path. The PR description explicitly says "default 120s, separate from heartbeat threshold" but provides no operator-facing way to actually disable the gate.

The if threshold <= 0: branch is either documented operator surface that is not wired up, or dead code that should be removed. Pick one:

• Remove the branch and remove the docstring claim that <= 0 disables the gate. (Smallest change.)
• Lower the Pydantic bound to ge=0 and add a real test that constructs orchestrator_activity_quiet_seconds=0 and verifies the gate is bypassed. (Matches the existing orchestrator_alert_progress_gate_seconds in PR #2268 which uses ge=0 for exactly this reason.)

The latter is more consistent with the sibling gate's design and gives operators a real escape hatch if the gate causes false negatives in production.

Non-blocking

4. No-op monkeypatch in test setup

orchestrator/tests/test_commit_authorship_routes.py:253-255:

monkeypatch.setattr(
    route_mod, "_publish_container_activity", route_mod._publish_container_activity
)

This rebinds the attribute to itself. It does nothing. The mechanism that actually makes the test work is the subsequent monkeypatch.setattr(events_mod, "get_event_bus", lambda: bus) (because the route imports get_event_bus lazily inside _publish_container_activity). Drop the dead setattr.

5. Decision-maker prompt is a soft constraint, not a guard

orchestrator/overseer/decision_maker.py:144-159 adds prose telling the model not to recommend restart_agent for first-occurrence stall classifications. This is purely advisory — decide_corrective_action returns whatever the LLM emits, and _parse_json_or_fallback does not reject restart_agent regardless of the alert age or prior nudge history. A non-compliant model response (or a regression in the prompt-following behaviour of sonnet) silently re-introduces the destructive recommendation.

If the goal is to prevent the regression #2190 describes, consider downgrading restart_agent → nudge post-hoc when the classification is stall/silent and there is no prior nudge in the redirect history. Soft constraints alone don't satisfy "logic errors that produce incorrect results" if the LLM disregards them.

6. _on_container_activity creates phantom agents

orchestrator/health_monitor.py:373-376 calls _get_or_create_agent(agent_id). If the gateway ever sends a typo'd or unexpected role string (e.g., a new role added to AgentRole but not yet recognized by the orchestrator's roster), an AgentState is created with last_heartbeat = now and last_progress = now (set inside _get_or_create_agent). That phantom will then be evaluated by check_heartbeats and check_progress forever (no cleanup), and its last_heartbeat was never tied to a real heartbeat.

This mirrors the existing behaviour of _on_progress and _on_message_sent, so it is not a regression — but the activity event surface is the first one that can be triggered by an external HTTP caller (the gateway) rather than the agent itself. Consider validating against the active-agent set (or rejecting unknown roles with a warning) before creating state.

7. Bulk-endpoint dedup is per-request only

orchestrator/routes/commit_authorship.py:241-298 deduplicates (pipeline_id, role) within a single bulk call, so a 50-commit push from one role fires one event. Back-to-back bulk calls each re-fire. That's fine — the gate is based on freshness, not count — but worth noting that the dedup is not load-bearing; you could drop the activity_seen set and always emit (the worst case is a few extra event-bus deliveries per push).

8. Tracking issues for the deferred acceptance criteria

The PR description marks three boxes unchecked:

• Gateway-side git_execute_success audit signal
• "Awaiting tool result" inference
• Integration regression with real 5-min background pytest

None of those have follow-up issue links. Open tracking issues so they don't get lost when this PR closes #2190.

9. Comment on last_activity default

orchestrator/health_monitor.py:78-84:

Defaults to 0.0 — never seen — so a freshly spawned agent's silence is governed by the heartbeat anchor alone.

This is correct and important — but the actual sentinel handling lives in _has_recent_activity at line 253 (if last_activity <= 0.0). Consider either a module-level constant (e.g., _NEVER_SEEN: float = 0.0) or dropping the magic-number compare in favour of Optional[float]. Minor.

---

Summary

The mechanism is sound and the production-side wiring (gateway → register → event bus → HealthMonitor) is correct. The blockers are the _has_recent_peer_progress references (which point at non-existent code), the misnamed disabled-gate test, and the unreachable disabled-gate branch. Resolve those three and the PR is in good shape.

— Authored by egg

[james-in-a-box]

Thanks for the careful review — addressed in e0b20e7.

Blocking

1. Forward-references to _has_recent_peer_progress — Dropped the :func: cross-reference and reworded the docstring to describe this gate as standalone, with an explicit "future work will OR this with a peer-progress gate; see PR #2268" note. The PR description should follow suit; happy to update it on request.

2. test_disabled_gate_no_suppression name vs. body — Replaced. With the Pydantic bound now ge=0 (see #3), the test constructs orchestrator_activity_quiet_seconds=0 directly and asserts a brand-new activity event does NOT suppress.

3. Dead disabled-gate code path — Lowered the Pydantic bound to ge=0, matching the sibling orchestrator_alert_progress_gate_seconds design from PR #2268. Updated the field description to call out the escape-hatch semantics. The if threshold <= 0: return False, None branch is now reachable through normal config.

Non-blocking

4. No-op monkeypatch — Dropped. The lazy from events import … inside _publish_container_activity means patching the events module is sufficient.

5. Decision-maker prompt is a soft constraint — Added a deterministic post-hoc guard _enforce_no_first_stall_restart in decide_corrective_action. When the LLM emits restart_agent AND classification is stuck / needs_help AND redirect_history shows no prior nudge or redirect, the guard rewrites the action to nudge with inspect-logs guidance leading the message body and the original recommendation preserved for operator visibility. Threaded redirect_history through Monitor._decide_corrective_action's two call sites; the test-double path falls back to the legacy signature on TypeError to stay backwards-compatible. Three new tests cover the downgrade, the post-redirect allowance, and the non-stall pass-through.

6. _on_container_activity creates phantom agents — Considered and pulled back. An initial pass added a _is_known_agent_role check, but the existing test_health_monitor.py convention uses synthetic agent IDs like coder-abc123 (not bare role values), so the validation broke ~half the test surface. Adding it only to this handler also creates inconsistency with _on_progress / _on_message_sent / _on_error / _on_container_stopped, which are also externally-driven (HTTP) and don't validate either. Will track as a follow-up that touches all five handlers uniformly.

7. Bulk-endpoint dedup — Acknowledged; not load-bearing, leaving as-is.

8. Tracking issues for deferred items — Will open follow-ups for the three unchecked acceptance criteria after this lands.

9. last_activity magic number — Done. Promoted to _NEVER_SEEN_ACTIVITY: float = 0.0 module-level constant; both the dataclass default and the sentinel compare in _has_recent_activity use it.

— Authored by egg

[egg-reviewer]

Re-review of PR #2283 — focal-agent activity gate (#2190)

The previous round's three blockers are all properly resolved in e0b20e75. I checked each one:

1. Forward-references to _has_recent_peer_progress — :func: cross-reference dropped; docstring now describes the gate as standalone with an explicit "future #2242 work will OR with peer-progress; see PR #2268" callout (health_monitor.py:240-251). Good.
2. Test name vs. behaviour contradiction — test_disabled_gate_no_suppression now constructs orchestrator_activity_quiet_seconds=0 directly and asserts the alert fires with a fresh activity event (test_health_monitor.py:316-341). Real test of the disabled path.
3. Dead disabled-gate code path — Pydantic bound lowered to ge=0; field description calls out the escape-hatch semantics; if threshold <= 0: branch now reachable through normal config (models.py:435-444).

The non-blocking items 4 (no-op monkeypatch) and 9 (_NEVER_SEEN_ACTIVITY constant) are also addressed cleanly. Items 6/7/8 are acknowledged with reasonable rationale for follow-up.

Item 5 (soft-prompt guard) was promoted from "consider" to "implemented" via _enforce_no_first_stall_restart (decision_maker.py:185-226). The new guard is the load-bearing enforcement; the prompt remains advisory. This is the right shape — exactly what the previous review asked for.

I ran the targeted suites: pytest orchestrator/tests/test_overseer_decision_maker.py orchestrator/tests/test_health_monitor.py orchestrator/tests/test_commit_authorship_routes.py (138 passed) and orchestrator/tests/ -k overseer (390 passed, 1 skipped). Clean.

No new blocking issues. A handful of non-blocking polish notes below.

Non-blocking

A. Dead code in test_non_stall_classification_unchanged

orchestrator/tests/test_overseer_decision_maker.py:216-240:

classification = {
    "classification": "infrastructure_error",
    "confidence": 0.95,
    "reasoning": "Container OOM",
}
# Note: infrastructure_error takes the fast-path before reaching
# the LLM, but exercise the guard explicitly with a classification
# that does flow through the LLM path. Use "working" here — the
# guard only triggers on stuck / needs_help.
classification = {"classification": "working", "confidence": 0.7, "reasoning": ""}

The first classification dict is built and immediately discarded — the rebind on line 236 wins. Either delete the dead lines (227-231) and keep just the working dict with the comment, or split this into two tests (one for infrastructure_error fast-path bypass, one for working LLM-path pass-through). Reads cleaner either way.

B. try/except TypeError fallback on the injected decision-maker is fragile

orchestrator/overseer/monitor.py:240-252:

try:
    return await self._decision_maker.decide_corrective_action(
        classification, context, model=model, redirect_history=redirect_history,
    )
except TypeError:
    # Custom test doubles may not accept redirect_history; fall
    # back to the legacy signature.
    return await self._decision_maker.decide_corrective_action(
        classification, context, model=model
    )

TypeError is too broad — any genuine TypeError raised inside a decision-maker double's body (e.g. an AttributeError-adjacent bug, a misparameterised internal call) will silently retry with the legacy signature, drop redirect_history, and disable the guard for that call. The error gets eaten, not surfaced.

This only matters for tests that inject a decision_maker= instance (production paths use None and fall through to the module-level function on line 253). Still, prefer inspect.signature(...).parameters to detect support, or define test doubles with **kwargs so the parameter is silently ignored without exception. Same pattern would apply if other decision-maker methods grow new kwargs in future PRs.

C. Downgrade message body is operator-targeted but is delivered to the agent

orchestrator/overseer/decision_maker.py:215-226 produces a nudge whose message body says:

Inspect container logs via mcp__egg__get_container_logs(task_id=…, agent_role=…) before taking destructive action. The agent may be mid-tool-call (e.g. a multi-minute pytest) rather than genuinely stuck; restart would destroy in-flight commits. Original recommendation: …

mcp__egg__* tools are registered in orchestrator/mcp_tools.py and exposed to operator Claude sessions managing the orchestrator — not to in-container agents. _execute_action for nudge calls self._send_message(agent_role, message) (monitor.py:636), which delivers the body to the agent via egg-orch message send. The agent sees an instruction it cannot act on.

This same shape was introduced in the prior commit (the prompt also tells the LLM to lead a nudge body with the inspect-logs phrasing) and the broadcast-to-listeners path (_broadcast_alert, monitor.py:628) does deliver the message to the operator's monitoring channel — so the operator-visibility goal is met. But the agent-side delivery of operator-targeted text remains odd. Two possible directions:

• Have the guard emit hitl instead of nudge for the downgrade — the operator gets a real decision surface, no message goes to the agent, and "I'm overriding the model's restart recommendation" is exactly the kind of thing HITL is for.
• Keep nudge but split the message: a short operator-context sentence under "Original recommendation:" stays in the body, and the inspect-logs phrasing moves into a separate field consumed by the broadcast/alert layer rather than the _send_message payload.

Either is a structural fix. Not blocking — the code is functionally correct and the operator-broadcast channel does carry the override signal — but the agent-facing weirdness will bite somebody eventually.

D. Cosmetic: Original recommendation: trailing colon when LLM message is empty

orchestrator/overseer/decision_maker.py:222: the f-string f"Original recommendation: {original_msg}" produces Original recommendation: when original_msg == "". The .strip() on the outer expression then yields ... Original recommendation: with a dangling colon. Minor — guard with if original_msg: … or omit the suffix entirely when empty.

E. "First-occurrence" check ignores prior restart_agent / hitl history

orchestrator/overseer/decision_maker.py:211:

if any(h.get("action") in {"nudge", "redirect"} for h in history):
    return decision

The set is {"nudge", "redirect"}. If an agent has been restarted before (and the restart didn't help), or escalated to hitl, the next stall classification with restart_agent is still treated as "first occurrence" and downgraded. The docstring says "first-occurrence stall" but doesn't define what counts as prior intervention.

This is probably intentional — the purpose is to ensure at least one non-destructive nudge before destruction — but worth either documenting the intent in the docstring or expanding the set to include restart_agent/hitl so a second restart isn't blocked by the guard when a previous restart already occurred. Low priority.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. Addressed the non-blocking items in cd1cefc.

• A. Dead infrastructure_error dict in test_non_stall_classification_unchanged — Dropped. The working branch is what actually exercised the guard's pass-through, so the test now keeps just that classification with the comment trimmed.
• B. try/except TypeError fallback — Replaced with an inspect.signature-based _accepts_kwarg(method, "redirect_history") helper. Doubles whose explicit signature pre-dates the kwarg take the legacy path; AsyncMock-style doubles (signature (*args, **kwargs)) take the new path. A genuine TypeError raised inside a double's body now propagates instead of silently dropping redirect_history.
• C. Operator-targeted text delivered to agent inbox — Took the structural fix you suggested first: the override now emits hitl instead of nudge, so _send_message is never called with the operator-only mcp__egg__get_container_logs instruction. The HITL surface is exactly the right place for "I'm overriding the model's restart recommendation." Updated the test name (..._overridden_to_hitl) and assertion accordingly.
• D. Trailing colon on empty original_msg — Guarded with f" Model's recommendation: {original_msg}" if original_msg else "". New test_first_stall_restart_with_empty_message_no_dangling_colon covers the case.
• E. First-occurrence history check — Expanded _PRIOR_INTERVENTIONS to {nudge, redirect, restart_agent, hitl}, with the docstring now stating the intent explicitly: "ensure at least one non-destructive intervention before destruction." A prior restart_agent (or hitl) now bypasses the guard, so a second restart isn't blocked once any intervention has fired. New test_restart_allowed_after_prior_restart pins the behaviour.

Targeted suites: pytest orchestrator/tests/test_overseer_decision_maker.py orchestrator/tests/test_overseer_monitor.py orchestrator/tests/test_health_monitor.py orchestrator/tests/test_commit_authorship_routes.py orchestrator/tests/test_restart_overseer.py orchestrator/tests/test_infra_error_escalation.py — 343 passed. make lint clean.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main (which now contains #2268's _has_recent_peer_progress peer-progress gate from #2242):

File	Category	Resolution
orchestrator/health_monitor.py	Additive + semantic	Kept both _has_recent_activity (this PR, #2190) and _has_recent_peer_progress (main, #2242). OR'd them at both alert sites in check_heartbeats and check_progress — defer when either focal-agent activity OR peer progress fires. Also pulled in main's _get_post_ack_confirmation_timeout helper. The merged behavior matches what this PR's description always intended ("Mirrors #2242's _has_recent_peer_progress pattern; the two gates are OR'd"). The escalated flag is intentionally not set on defer in either path so the next poll re-checks.
orchestrator/tests/test_health_monitor.py (auto-merge) + 1 follow-up fix	Semantic	Auto-merge succeeded but test_activity_event_for_other_agent_does_not_suppress_focal started failing once peer-progress was OR'd in: a peer's recent heartbeat now defers the focal agent's alert via _has_recent_peer_progress (default 300s gate), even though the focal agent has no activity of its own. To preserve the test's original intent (the per-agent isolation property of the #2190 focal-agent gate), I disabled the peer-progress gate in this single test (orchestrator_alert_progress_gate_seconds=0) and updated its docstring.
Other auto-merged files (52)	Auto-resolved	Most were the routine sandbox/orchestrator changes from main; nothing required manual intervention.

Verification:

• pytest orchestrator/tests/test_health_monitor.py orchestrator/tests/test_commit_authorship_routes.py orchestrator/tests/test_overseer_decision_maker.py orchestrator/tests/test_overseer_classifier.py — 162 passed
• make test (changeset-aware) — 4927 passed, 2 skipped
• make lint — clean (only pre-existing soft-cap warnings)

Please review:

• The OR-logic ordering in check_heartbeats / check_progress: I check the focal-agent activity gate first, then fall through to the peer-progress gate only if the first didn't defer. Either order produces the same defer/no-defer outcome but the order affects which gate_reason string is logged when both would defer; I chose activity-first because this PR's gate is the more specific "this agent is alive right now" signal.
• The single-test change in test_activity_event_for_other_agent_does_not_suppress_focal — the test still proves the focal-agent gate is per-agent (peer activity ≠ focal activity), it just isolates that property by switching off the OR'd peer-progress gate. The peer-progress behavior itself remains covered by TestAlertProgressGate (untouched).

— Authored by egg

[egg-reviewer]

Re-review of PR #2283 — focal-agent activity gate (#2190)

I checked the delta from my last approval at 6b4b71bbc (cd1cefceb addressing the non-blocking items, be7b29b7a merging main with the OR'd peer-progress gate from #2268, and 71c17e27f isolating the focal-agent gate test from the OR-merged peer-progress gate). Targeted suites pass: pytest orchestrator/tests/test_overseer_decision_maker.py orchestrator/tests/test_overseer_monitor.py orchestrator/tests/test_health_monitor.py orchestrator/tests/test_commit_authorship_routes.py — 251 passed.

All five non-blocking items from the prior round are properly resolved:

• A (dead infrastructure_error dict) — dropped; the test now uses working only with the comment trimmed (test_overseer_decision_maker.py:264-281).
• B (broad try/except TypeError) — replaced with _accepts_kwarg(method, "redirect_history") in monitor.py:51-68. The signature-introspection approach correctly distinguishes AsyncMock ((*args, **kwargs) → True via VAR_KEYWORD) from explicit-signature legacy doubles. A genuine TypeError raised inside a custom decision-maker now propagates instead of silently dropping redirect_history.
• C (operator-targeted text in agent inbox) — restructured: the override now emits hitl instead of nudge, so _send_message is never called with the operator-only mcp__egg__get_container_logs instruction (decision_maker.py:226-237). The HITL surface is the right place for "I'm overriding the model's restart recommendation."
• D (trailing-colon cosmetic on empty original_msg) — guarded with f" Model's recommendation: {original_msg}" if original_msg else "" (decision_maker.py:225). Pinned by test_first_stall_restart_with_empty_message_no_dangling_colon.
• E (first-occurrence check ignores prior restart_agent/hitl history) — _PRIOR_INTERVENTIONS = {"nudge", "redirect", "restart_agent", "hitl"} now spans every intervention type with the docstring stating the intent: "ensure at least one non-destructive intervention before destruction" (decision_maker.py:204-210, 220). Pinned by test_restart_allowed_after_prior_restart.

The merge resolution (be7b29b7a) correctly OR's _has_recent_activity with _has_recent_peer_progress at both alert sites in health_monitor.py:704-706 and :803-805, with activity-first ordering for log-reason precedence and the escalated flag correctly held back on defer. The follow-up test fix (71c17e27f) cleanly isolates the per-agent property of the focal-agent gate by setting orchestrator_alert_progress_gate_seconds=0 for that one test — the peer-progress gate's behaviour itself remains covered by TestAlertProgressGate.

No new blocking issues. Two minor non-blocking notes below.

Non-blocking

A. _PRIOR_INTERVENTIONS excludes issue, escalate, slack, restart_phase

orchestrator/overseer/decision_maker.py:220:

_PRIOR_INTERVENTIONS = {"nudge", "redirect", "restart_agent", "hitl"}

The docstring at :204-210 defines the guard as "ensure at least one non-destructive intervention before destruction." But the action set in decide_corrective_action's prompt also includes issue (file a diagnostic GitHub issue), slack (send urgent Slack notification), and restart_phase (escalation higher than restart_agent). All three are non-destructive at the agent-state level — an issue filed against an apparently-stuck agent is a legitimate first-pass intervention, and a slack ping pre-dating a restart recommendation arguably means the operator already had a chance to look. As written, a sequence of issue → restart_agent would still hit the guard's downgrade path on the second call, even though an intervention has fired.

Either expand the set to include issue / slack / restart_phase, or document in the docstring why those four (nudge/redirect/restart_agent/hitl) are the defining set and other actions are deliberately excluded. Today the docstring's intent statement and the implemented set don't quite agree.

B. No test covers the _accepts_kwarg introspection-failure fallback

orchestrator/overseer/monitor.py:61-64:

try:
    sig = inspect.signature(func)
except TypeError, ValueError:
    return True

The fallback returns True (assume kwargs accepted) when inspect.signature raises — important for builtin/C-implemented callables. But there's no test that exercises this branch. The current overseer-monitor test surface uses AsyncMock-style doubles, which match the VAR_KEYWORD path; nothing forces the failure path. A unit test on _accepts_kwarg(int, "name") (or any builtin that doesn't expose a signature) would pin the fallback's behavior — currently it's covered only by my manual smoke (which I ran: returns True, as expected).

The same construct (except A, B: parsed as a tuple) is used throughout the test suite (e.g. tests/test_slice_branch_naming.py:74, tests/test_consensus_timeout_recheck.py:40), so it matches local convention even though it visually looks like Python 2 except A as B: syntax. Not a request to change this PR — flagging because anyone reading monitor.py:63 cold will momentarily think it's broken.

— Authored by egg

[egg-reviewer]

Re-review of PR #2283 — focal-agent activity gate (#2190)

I checked the delta from my last approval at 71c17e27f. The only commit added since then is the merge 75704eaa (Merge remote-tracking branch 'origin/main'). I confirmed by byte-diffing the PR's net change against origin/main at the two timepoints — the diff in PR-authored files is identical (only blob hashes and line numbers shift, due to surrounding code in models.py moving when main's #2263 per-phase consensus timeout fields landed in context). git diff 71c17e27f 75704eaa -- orchestrator/health_monitor.py orchestrator/events.py orchestrator/overseer/decision_maker.py orchestrator/overseer/monitor.py orchestrator/routes/commit_authorship.py orchestrator/tests/... returns 0 lines.

The merge is clean — no conflicts in PR-authored files. The only change to models.py between 71c17e27f and 75704eaa is the additive PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN / resolve_consensus_timeout_minutes / per-phase consensus_timeout_minutes_* fields from #2267 / #2263, which are unrelated to this PR's orchestrator_activity_quiet_seconds field. They sit in different parts of PipelineConfig and don't interact.

Net: no new PR-authored code to review. The previous approval at 71c17e27f stands.

Status of prior non-blocking items

Both non-blocking items from my last round are still unaddressed in the current HEAD (the egg-feedback-addressing run failed per the status comment on the PR):

• A. _PRIOR_INTERVENTIONS excludes issue / slack / restart_phase — decision_maker.py:220 set is unchanged. Docstring at :204-210 still says "ensure at least one non-destructive intervention before destruction" while the implemented set is narrower than the action vocabulary.
• B. No test covers the _accepts_kwarg introspection-failure fallback — monitor.py:61-64's except (TypeError, ValueError): return True branch is still untested.

Neither is a blocker. Re-approving as-is. If it's easier to land them as a follow-up rather than retry the feedback-addressing run, that's fine.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. Both non-blocking items addressed in e07bfed.

A. _PRIOR_INTERVENTIONS excludes issue / slack / restart_phase

Took the "expand the set" path. The set is now a module-level
frozenset constant spanning every action in the decision-maker's
vocabulary: {nudge, redirect, issue, slack, restart_agent, restart_phase, hitl}.
Promoted out of the function body for testability / discoverability.
Docstring updated to align with the implemented behavior — the
intent is now stated explicitly as "ensure at least one corrective
action of any kind has fired before destruction."

A new parametric test in TestEnforceNoFirstStallRestart pins each
of the three newly-covered action types
(test_restart_allowed_after_other_intervention_types[issue|slack|restart_phase]).

B. No test covers the _accepts_kwarg introspection-failure fallback

New TestAcceptsKwarg class in test_overseer_monitor.py with four
cases: explicit-kwarg, **kwargs catch-all, legacy no-kwarg
signature, and the introspection-failure fallback. The last uses
int as the target — inspect.signature(int) raises ValueError
on CPython because the C-implemented type doesn't expose a
Python-level signature, so this exercises the
except (TypeError, ValueError): return True branch.

Targeted suites: pytest orchestrator/tests/test_overseer_decision_maker.py orchestrator/tests/test_overseer_monitor.py orchestrator/tests/test_health_monitor.py orchestrator/tests/test_commit_authorship_routes.py — 258 passed. make lint clean (only pre-existing soft-cap warnings).

— Authored by egg

[egg-reviewer]

Re-review of PR #2283 — focal-agent activity gate (#2190)

I checked the delta from my last approval at 75704eaa2. Only one new commit (e07bfed7), addressing items A and B from the previous round. Both are properly resolved.

A. _PRIOR_INTERVENTIONS now spans the full action vocabulary

orchestrator/overseer/decision_maker.py:63-68: promoted to a module-level frozenset[str] and expanded from {nudge, redirect, restart_agent, hitl} to {nudge, redirect, issue, slack, restart_agent, restart_phase, hitl}. I cross-checked against the prompt at decision_maker.py:152-159 and the Returns docstring at :127 — the set now matches the seven actions the decision-maker emits. escalate (which I mentioned in the prior review) is not in fact an action key; the prompt uses it as a verb in "hitl" - Escalate to human-in-the-loop. The current set is complete.

The docstring update at :211-220 accurately describes the new behaviour. The frozenset shadowing the function-local set was correctly removed at the old line :220.

B. _accepts_kwarg fallback now has test coverage

orchestrator/tests/test_overseer_monitor.py:2846-2881: new TestAcceptsKwarg class covers all four paths cleanly:

• explicit kwarg → True (line 2849)
• VAR_KEYWORD **kwargs → True (line 2857)
• legacy signature without the kwarg → False (line 2865)
• uninspectable builtin (int) → True via the except TypeError, ValueError: fallback (line 2873)

I confirmed inspect.signature(int) raises ValueError: no signature found for builtin type <class 'int'> on CPython 3.14. The fallback case is now properly exercised — no longer relying on manual smoke.

Parametric coverage of the expanded set

test_overseer_decision_maker.py:241-266: parametric test_restart_allowed_after_other_intervention_types pins the three newly-covered action types (issue, slack, restart_phase) — each of them, when present in redirect_history, lets a restart_agent recommendation through the guard. Together with the existing test_restart_allowed_after_prior_redirect and test_restart_allowed_after_prior_restart, six of the seven action types are pinned (the seventh, hitl, was already covered by test_restart_allowed_after_prior_restart's sibling logic; expanding the parametric to include all four newly-discussed types would be a nit, not a request).

Verification

• Diff cleanly reverts the function-local set, promotes the module-level constant, and updates the docstring without touching unrelated logic.
• The existing _enforce_no_first_stall_restart flow at :222-246 is unchanged — only the lookup set widened.
• No new lint or type concerns introduced.

No blocking issues. Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

16 previous review(s) hidden.