dead docker containers during an M3 run get silently miscounted as task failures#120
Open
haroldship wants to merge 14 commits into
Open
dead docker containers during an M3 run get silently miscounted as task failures#120haroldship wants to merge 14 commits into
haroldship wants to merge 14 commits into
Conversation
Add _env_float() and _env_int() helpers that parse env vars with graceful fallback to defaults + warning logs. Prevents ValueError exceptions when M3_ENV_HEALTH_TIMEOUT or M3_ENV_FAIL_STREAK are malformed (e.g., "three" or "abc" instead of numbers).
…ries A single domain can hold 100+ samples and run for hours; detection that only re-checks at domain boundaries (run_config_mode's sequential loop) would grind through every remaining sample against a dead container before anything noticed. Add a per-sample environment- failure streak inside M3Evaluator.evaluate_all's sample loop, fresh per domain, so it trips after a few consecutive failures instead of waiting for the whole domain to finish. Also fixes a second swallowing bug: evaluate_single_task's per-domain except clause had no passthrough for EnvironmentFailureError, so even a correctly-raised abort from inside the domain's evaluation would have been silently logged and skipped to the next domain instead of propagating to the existing abort path.
…st raised exceptions A live run surfaced a failure mode neither detector caught: the registry stays reachable (find_tools still works) but the MCP transport to the dead container itself is closed. The tool call doesn't raise — it returns "Connection closed" as its own value — the agent retries a few times, then produces a normal-looking "I couldn't get the data" final answer. result["error"] stays unset throughout, so both the domain-level and sample-level streak checks (which only looked at result["error"]) never saw it. Add is_environment_shaped_result(), which checks both the top-level error field and every tool call's own return value (multi-turn and single-turn shapes), and wire EnvironmentFailureStreakTracker.record() to use it instead of the narrower error-only check. Also add "Connection closed" (and variants) to the marker list.
…of proceeding anyway start_registry_server polled for up to 5 minutes waiting for the registry to report at least one registered MCP application, but on timeout it only logged a warning and proceeded — returning a "healthy" registry handle with nothing behind it. That's the same failure class this whole feature targets (a broken docker environment), just at startup time rather than mid-run, and it was the one path our other detectors couldn't see. Zero apps registered after the full warmup window is unambiguous (the loop's own success condition is len(apps) > 0, so "not all_ready" after 300s means literally none ever did) — raise EnvironmentFailureError instead. This reuses the function's existing `except RuntimeError: raise` passthrough (added earlier for a stale- registry mismatch) for free, since EnvironmentFailureError is a RuntimeError subclass.
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughM3 evaluation now probes Docker containers, detects consecutive environment-shaped failures, aborts broken runs with banners and resume hints, maps environment failures to exit code 3, and adds regression tests for detection, wiring, and shell reporting. ChangesM3 environment health
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EvalShell
participant M3Evaluation
participant DockerContainer
participant FailureTracker
EvalShell->>M3Evaluation: start evaluation
M3Evaluation->>DockerContainer: inspect and exec health probes
DockerContainer-->>M3Evaluation: health result
M3Evaluation->>FailureTracker: record task results
FailureTracker-->>M3Evaluation: threshold status
M3Evaluation-->>EvalShell: exit code 3 on environment failure
EvalShell-->>EvalShell: print resume instructions
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
9 tasks
haroldship
added a commit
that referenced
this pull request
Jul 14, 2026
…ures (#122) * fix(deps): bump pillow and json-repair to clear CI security-scan failures CI's pip-audit step was failing (blocking PR #120 and #121) because two dependencies had known security bugs with fixes already published. - pillow 12.2.0 -> 12.3.0, json-repair 0.59.10 -> 0.61.4 (both just needed a lockfile bump, no code changes required) - setuptools also has a known bug (fixed in 83.0.0), but torch pins it to "<82" so we can't move it ourselves; added it to the security recipe's ignore list with a comment, matching the existing docling/torch entries * fix(ci): ignore setuptools CVE in the CI workflow's pip-audit too CodeRabbit caught that the CI workflow runs its own copy of the pip-audit command instead of `just security`, so the setuptools ignore added in the justfile wasn't actually reaching CI — the Security check was still red on this PR. Mirrored the same --ignore-vuln flag and explanation into ci.yml.
haroldship
changed the title
haroldship
marked this pull request as ready for review
July 14, 2026 13:59
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarks/m3/eval_m3.py (1)
1013-1123: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMirror the environment-failure streak check in the single-turn loop. Single-turn evaluation is still a live path, and this loop never calls
self._env_fail_streak.record([result]), so a dead container can keep burning through the remaining tasks instead of aborting early.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/m3/eval_m3.py` around lines 1013 - 1123, Add the same environment-failure streak handling from the multiturn loop to the single-turn loop after appending each result from evaluate_task. Call self._env_fail_streak.record([result]), build the threshold/domain/identity failure reason, print the failure banner with resume_hint_for(self.bundle_dir), and raise EnvironmentFailureError when the threshold is reached.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/m3/container_health.py`:
- Around line 90-129: Update check_container_health to catch OSError alongside
subprocess.TimeoutExpired around both subprocess.run calls for inspect and exec.
Return an unhealthy result with a descriptive environment/runtime failure reason
instead of allowing missing or broken container runtimes to propagate an
exception; preserve the existing timeout and successful health-check behavior.
- Around line 131-155: Validate threshold in
EnvironmentFailureStreakTracker.__init__ and reject any value less than or equal
to zero before storing it, so record() cannot immediately satisfy the streak
condition for invalid configuration. Preserve the existing positive-threshold
streak behavior and use the project’s established validation/error convention if
one is available.
In `@benchmarks/m3/tests/test_eval_m3_env_health_wiring.py`:
- Around line 70-78: Update
test_environment_failure_error_is_not_swallowed_by_the_generic_handler and the
related run_config_mode ordering test to locate run_config_mode’s own except
EnvironmentFailureError clause, searching after the first occurrence or within
the function’s extracted source. Compare that occurrence against
run_config_mode’s generic logger.error handler so the assertion fails if the
local passthrough clause is moved below the generic handler.
---
Outside diff comments:
In `@benchmarks/m3/eval_m3.py`:
- Around line 1013-1123: Add the same environment-failure streak handling from
the multiturn loop to the single-turn loop after appending each result from
evaluate_task. Call self._env_fail_streak.record([result]), build the
threshold/domain/identity failure reason, print the failure banner with
resume_hint_for(self.bundle_dir), and raise EnvironmentFailureError when the
threshold is reached.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d40deac-0395-44d1-bae4-8306792be008
📒 Files selected for processing (6)
benchmarks/m3/container_health.pybenchmarks/m3/eval.shbenchmarks/m3/eval_m3.pybenchmarks/m3/tests/test_container_health.pybenchmarks/m3/tests/test_eval_m3_env_health_wiring.pybenchmarks/m3/tests/test_eval_sh_env_health.py
- catch OSError (missing/broken container runtime) in check_container_health - validate EnvironmentFailureStreakTracker threshold >= 1 - fix except-clause ordering test to anchor on run_config_mode's own clause
Collaborator
Author
Fixes Applied SuccessfullyFixed 2 file(s) based on 3 CodeRabbit feedback item(s). Files modified:
Commit: The latest autofix changes are on the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
M3 runs its tests against a bunch of docker containers that live outside this repo (one per "capability"). Sometimes one of those containers crashes or freezes partway through a run. Before this PR, M3 didn't notice — it just kept going, and every failure caused by the dead container got written down as if the agent itself had failed the task. That silently pollutes results with noise that's indistinguishable from a real bug after the fact.
This PR adds three tripwires that all lead to the same outcome when triggered: print a big, hard-to-miss banner, stop the run with exit code 3, and let
--resume-experimentpick the run back up once the environment is fixed.docker inspect+ a realdocker exec) before wasting a restart on one that's already dead.This only covers the default sequential mode (
batch_size < 2); batched/parallel mode is untouched.Config knobs (env vars, all optional)
M3_ENV_HEALTH_CHECK(defaulttrue) — turn off the active per-domain checkM3_ENV_HEALTH_TIMEOUT(default5) — seconds before an inspect/exec counts as timed outM3_ENV_FAIL_STREAK(default3) — consecutive environment-shaped failures before abortingBad values for the numeric knobs fall back to defaults with a warning instead of crashing.
Design spec:
docs/superpowers/specs/2026-07-13-m3-docker-env-health-check-design.mdImplementation plan:
docs/superpowers/plans/2026-07-13-m3-docker-env-health-check.mdCloses #119.
Tests
benchmarks/m3/tests/, all passing (2 pre-existing skips, unrelated — Vakra vendor not installed).capability_2_dashboard_apis), using realdocker compose stop/down:--resume-experiment <id>hint.Test plan for reviewers
uv run --no-sync pytest benchmarks/m3/tests/ -m sanitySummary by CodeRabbit
New Features
Tests