fix(experiments): give the spawn-thrash detector a real progress signal

[lwgray]

Fixes #703.

Problem

SpawnThrashDetector reset its idle-spawn counter only when completed_tasks increased, so it was blind to an agent that had claimed a task and was actively working it (logging context/artifacts/decisions) but hadn't finished within the 5-poll (~2.5 min) fuse. A single contract_first task takes minutes, so the detector tore down healthy-but-slow runs and blamed a "BLOCKED dependency" that didn't exist.

Fix

observe() now takes a monotonic, cumulative activity tally the runner assembles from the signals an agent emits while genuinely working:

activity = done                                    # completed_tasks
         + context_requests + artifacts_created    # get_task_context / log_artifact
         + decisions_logged + blockers_reported    # log_decision / report_blocker

Any increase resets the thrash streak. in_progress is deliberately excluded — it flickers 0→1→0 on claim-and-exit churn (the lease lifecycle) and would mask the very thrash the detector exists to catch; the cumulative work counters only ever rise.

The to_spawn == 0 hold behavior is unchanged, so this is purely additive protection: it can only prevent false teardowns, never suppress a real one (a genuinely doomed run logs no work → activity stays flat → still fires).

Better teardown message

Before:

SPAWN-THRASH: spawned agents for 5 polls with no task completing — likely BLOCKED dependency gating downstream work; tearing down

After:

SPAWN-THRASH: spawned an agent on each of the last 5 polls but saw no forward progress — 3 completed, 0 in-progress, and no agent logged any work (get_task_context / log_artifact / log_decision). Agents are starting but not claiming or working: likely deferred MCP tools, a skill mismatch, or every unclaimed task gated by a BLOCKED dependency. Tearing down.

Tests

tests/unit/experiments/test_spawn_controller.py: renamed the observe() keyword (completed=→activity=), refreshed docstrings, and added test_logged_work_resets_without_a_completion (a mid-task artifact log with zero completions resets the streak). 22 passed.

Note

Complements #702 (worker-side --strict-mcp-config fix). The lease-expiry system is the per-task recovery mechanism; this is the run-level cost-control backstop — see #703 for the full comparison and the future "expose lease-renewal freshness" option.

🤖 Generated with Claude Code

[claude]

Claude finished @lwgray's task —— View job

---

PR Review Complete ✅

Overall Assessment: This is a well-implemented fix that addresses a real operational issue. The solution is elegant, focused, and maintains backward compatibility while significantly improving the spawn-thrash detection capability.

Todo List:

• Read and analyze changed files
• Review spawn_controller.py changes
• Review spawn_agents.py changes
• Review test changes and coverage
• Check for potential bugs and edge cases
• Evaluate performance considerations
• Check security concerns
• Provide comprehensive feedback

✅ Strengths

1. Clear Problem Identification: The issue is well-defined - SpawnThrashDetector was only tracking completed tasks, missing agents actively working but not yet finished.

2. Elegant Solution: The transition from completed to activity broadens the progress signal to include all agent work indicators without breaking existing logic.

3. Excellent Documentation:

   • Comprehensive docstrings explain the new activity parameter
   • Clear reasoning for excluding in_progress from the tally
   • Good explanation of monotonic vs. flickering signals

4. Robust Test Coverage: Tests properly cover the new functionality, especially test_logged_work_resets_without_a_completion which validates the core improvement.

5. Backward Compatibility: The change is purely additive - can only prevent false teardowns, never suppress legitimate ones.

🔍 Technical Review

Code Quality - Excellent

• Clean, readable implementation
• Proper parameter validation and error handling
• Consistent naming conventions
• Well-structured logic flow

Architecture - Sound

• spawn_controller.py: Pure logic, no side effects - good separation of concerns
• spawn_agents.py: Correctly assembles the activity signal from multiple counters
• The activity calculation is placed at the right abstraction level

Activity Signal Composition - Well Designed

activity = (
    done
    + int(status.get("context_requests", 0))
    + int(status.get("artifacts_created", 0)) 
    + int(status.get("decisions_logged", 0))
    + int(status.get("blockers_reported", 0))
)

This captures all legitimate agent work while avoiding the in_progress flicker that would mask actual thrash.

Test Quality - Good

• Updated method signatures correctly
• Added specific test for the new behavior
• Maintains existing test coverage
• Real-world scenario testing (test73 reproduction)

🔍 Potential Concerns

Minor Issues

1. Error Handling: The code assumes all status fields exist and are convertible to int. While int(status.get("field", 0)) provides a default, malformed responses could still cause issues.

2. Activity Counter Overflow: In very long-running experiments, the cumulative counters could theoretically overflow, though this is extremely unlikely in practice.

Edge Cases Handled Well

• First observation never triggers detection ✅
• Zero spawn polls don't increment counter ✅
• Activity resets properly reset detection ✅
• Disabled detector (thrash_polls=0) works correctly ✅

📊 Performance Considerations - Good

• Minimal computational overhead (simple integer arithmetic)
• No new I/O operations introduced
• Efficient status field access pattern
• Activity calculation is done once per poll cycle

🔒 Security Considerations - No Issues

• No external inputs processed
• No file system operations added
• No credential handling
• Pure computational logic with controlled inputs

💡 Suggestions for Enhancement

1. Defensive Programming (Optional):

   # More robust activity calculation
   def safe_int(value, default=0):
       try:
           return int(value) if value is not None else default
       except (ValueError, TypeError):
           return default
   
   activity = (
       safe_int(status.get("completed_tasks"))
       + safe_int(status.get("context_requests"))
       + safe_int(status.get("artifacts_created"))
       + safe_int(status.get("decisions_logged"))
       + safe_int(status.get("blockers_reported"))
   )

2. Logging Enhancement (Optional): Consider logging when activity increases to help with debugging spawn-thrash scenarios.

📝 Documentation Quality - Excellent

The improved teardown message is much more informative:

• Clearly explains what was measured
• Lists specific work indicators that were flat
• Provides actionable next steps for diagnosis

✅ Final Recommendation: APPROVE

This PR successfully addresses the core issue with a clean, well-tested solution. The implementation is sound, the test coverage is appropriate, and the change is safe. The improved spawn-thrash detection will significantly reduce false positive teardowns and associated cost waste.

Risk Level: Low - purely additive change with comprehensive test coverage
Merge Confidence: High - ready to merge

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ffe15a3d79

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Wire context requests before using them as progress

With this new term, a get_task_context call is supposed to reset the thrash streak, but the counter never increments today: get_task_context only emits log_agent_event("context_requested", ...), and a repo-wide rg record_context_request shows no caller for LiveExperimentMonitor.record_context_request. In a healthy run where a claimed agent requests context and then works while the active layer still has another TODO task (so to_spawn > 0), activity remains flat and the detector can still fast-fail after spawn_thrash_polls. Please either record context requests into the monitor or don't rely on this field.

Useful? React with 👍 / 👎.

[lwgray]

Fixed in ab70949. You're right — record_context_request existed but had no caller. get_task_context now records it into the active monitor (mirroring log_decision), so context_requests actually increments and counts as forward progress.

[chatgpt-codex-connector]

Count report_task_progress as forward progress

This activity total omits the normal keep-alive/progress path: the assigned-agent workflow explicitly tells workers to call report_task_progress at 25/50/75%, but get_experiment_status only exposes task counts and these logged-work counters, so intermediate progress updates leave activity unchanged. When one task in the active layer is genuinely advancing via report_task_progress while another active-layer TODO keeps to_spawn > 0, the detector still accumulates idle-spawn polls and can tear down a healthy run after the fast-fail window. Please expose/count progress updates or include task progress changes in this tally.

Useful? React with 👍 / 👎.

[lwgray]

Fixed in ab70949. Added a progress_updates counter + record_progress() to LiveExperimentMonitor, exposed it in get_status (plus the mlflow metrics + text summary), and call it from report_task_progress. It is the earliest signal — fires on the first 25% report, well before any artifact lands — so a claimed agent advancing via progress reports keeps activity rising even while another active-layer TODO keeps to_spawn > 0. Folded into the runner activity tally alongside the now-wired context_requests.

[claude]

Claude finished @lwgray's task —— View job

---

PR Review Complete ✅

Overall Assessment: This is a well-implemented fix that addresses a real operational issue. The solution is elegant, focused, and maintains backward compatibility while significantly improving the spawn-thrash detection capability.

✅ Strengths

1. Clear Problem Identification: The issue is well-defined - SpawnThrashDetector was only tracking completed tasks, missing agents actively working but not yet finished.

2. Elegant Solution: The transition from completed to activity broadens the progress signal to include all agent work indicators without breaking existing logic.

3. Excellent Documentation:

   • Comprehensive docstrings explain the new activity parameter
   • Clear reasoning for excluding in_progress from the tally
   • Good explanation of monotonic vs. flickering signals

4. Robust Test Coverage: Tests properly cover the new functionality, especially test_logged_work_resets_without_a_completion which validates the core improvement.

5. Backward Compatibility: The change is purely additive - can only prevent false teardowns, never suppress legitimate ones.

🔍 Technical Review

Code Quality - Excellent

• Clean, readable implementation
• Proper parameter validation and error handling
• Consistent naming conventions
• Well-structured logic flow

Architecture - Sound

• spawn_controller.py: Pure logic, no side effects - good separation of concerns
• spawn_agents.py: Correctly assembles the activity signal from multiple counters
• The activity calculation is placed at the right abstraction level

Activity Signal Composition - Well Designed

activity = (
    done
    + int(status.get("progress_updates", 0))
    + int(status.get("context_requests", 0))
    + int(status.get("artifacts_created", 0)) 
    + int(status.get("decisions_logged", 0))
    + int(status.get("blockers_reported", 0))
)

This captures all legitimate agent work while avoiding the in_progress flicker that would mask actual thrash.

Test Quality - Good

• Updated method signatures correctly
• Added specific test for the new behavior
• Maintains existing test coverage
• Real-world scenario testing (test73 reproduction)

🔍 Addressing Codex Concerns

The automated codex review raised two important points that I investigated:

P1: Context Request Wiring ✅ Resolved

Codex concern: Context requests weren't being recorded.
Analysis: This has been fixed in this PR! The changes to src/marcus_mcp/tools/context.py:191-192 now properly call monitor.record_context_request() when get_task_context is invoked. This was the missing piece.

P1: Progress Updates ✅ Already Implemented

Codex concern: report_task_progress calls weren't being counted.
Analysis: This is already working! The code in src/marcus_mcp/tools/task.py:3744 calls monitor.record_progress() which increments the progress_updates counter that's included in the activity calculation at spawn_agents.py:2000.

🔍 Potential Concerns

Minor Issues

1. Error Handling: The code assumes all status fields exist and are convertible to int. While int(status.get("field", 0)) provides a default, malformed responses could still cause issues.

2. Activity Counter Overflow: In very long-running experiments, the cumulative counters could theoretically overflow, though this is extremely unlikely in practice.

Edge Cases Handled Well

• First observation never triggers detection ✅
• Zero spawn polls don't increment counter ✅
• Activity resets properly reset detection ✅
• Disabled detector (thrash_polls=0) works correctly ✅

📊 Performance Considerations - Good

• Minimal computational overhead (simple integer arithmetic)
• No new I/O operations introduced
• Efficient status field access pattern
• Activity calculation is done once per poll cycle

🔒 Security Considerations - No Issues

• No external inputs processed
• No file system operations added
• No credential handling
• Pure computational logic with controlled inputs

💡 Suggestions for Enhancement

1. Defensive Programming (Optional):

   # More robust activity calculation
   def safe_int(value, default=0):
       try:
           return int(value) if value is not None else default
       except (ValueError, TypeError):
           return default
   
   activity = (
       safe_int(status.get("completed_tasks"))
       + safe_int(status.get("progress_updates"))
       + safe_int(status.get("context_requests"))
       + safe_int(status.get("artifacts_created"))
       + safe_int(status.get("decisions_logged"))
       + safe_int(status.get("blockers_reported"))
   )

2. Logging Enhancement (Optional): Consider logging when activity increases to help with debugging spawn-thrash scenarios.

📝 Documentation Quality - Excellent

The improved teardown message is much more informative:

• Clearly explains what was measured
• Lists specific work indicators that were flat
• Provides actionable next steps for diagnosis

✅ Final Recommendation: APPROVE

This PR successfully addresses the core issue with a clean, well-tested solution. The implementation is sound, the test coverage is appropriate, and the change is safe. The improved spawn-thrash detection will significantly reduce false positive teardowns and associated cost waste.

Risk Level: Low - purely additive change with comprehensive test coverage
Merge Confidence: High - ready to merge

---

[lwgray]

@codex

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ab7094902c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Count only accepted progress reports as activity

When a stale or non-holder agent calls report_task_progress, this increments progress_updates before the stale-completion and lease-holder guards can reject the request. Since spawn_agents.py now treats any progress_updates increase as forward progress, a recovered/zombie worker that keeps sending rejected updates can keep resetting the spawn-thrash detector while newly spawned agents are still not making claimable progress. Move this heartbeat after the ownership/validation gates that prove the update is accepted, or avoid counting rejected updates.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Count only successful context fetches as activity

This records a context request before get_task_context verifies that the task exists or that the context system can return a successful response. In runs where workers repeatedly call this tool with an invalid task id or when context is disabled, context_requests still rises even though the tool returns an error, and the runner will treat those failed calls as forward progress and suppress the spawn-thrash fast-fail. Record the counter only on successful context responses, or separate failed requests from the activity signal.

Useful? React with 👍 / 👎.