Rectify: MCP Idle-Abort Immunity for Long-Blocking Tools - #4657
Merged
Trecek merged 45 commits intoAug 18, 2026
Conversation
Trecek
pushed a commit
that referenced
this pull request
Aug 17, 2026
… builder overloads Replace truthy 'if mcp_tool_timeout_sec:' with explicit 'if mcp_tool_timeout_sec is not None and mcp_tool_timeout_sec > 0:' in build_interactive_cmd (L686), build_resume_cmd (L746), and build_food_truck_cmd (L963). The truthy form accepts NaN and negative seconds, which would serialize to env as 'nan' / '-1' and propagate into the Claude client. The > 0 check at build_skill_session_cmd (L856) was already correct. Addresses PR #4657 review warnings: defense boundary inconsistency finding (4 locations in claude.py). No behavior change for valid inputs.
Trecek
pushed a commit
that referenced
this pull request
Aug 17, 2026
- _doctor_mcp.py: type-guard the user-config timeout field before numeric comparison; a string/non-numeric value in ~/.claude.json now warns instead of raising TypeError. - _doctor_mcp.py: distinguish 'file missing' (skipped) from 'file unparseable' (warning). Previously the latter was silently masked as Severity.OK, defeating the check. - _init_helpers.py: validate mcp_tool_timeout_sec at write site, raising ValueError for non-numeric / non-positive values rather than writing a negative timeout that Claude interprets as 'abort immediately'. - tools_fleet_dispatch.py: validate the tool_timeout before anyio.fail_after() and return a fleet_error on invalid values instead of letting NaN / negative reach anyio. Addresses PR #4657 review warnings: type guard at parse site, swallowed exception masking corruption, late validation at write site, late validation before anyio.fail_after.
Trecek
pushed a commit
that referenced
this pull request
Aug 17, 2026
… signatures - _launch_adapter.py: give mcp_tool_timeout_sec a default of 0.0 to match the SkillSessionConfig convention (one of three optionality styles), eliminating the required-float divergence at the builder boundary. - settings.py: _claude_mcp_timeout_coherence_gate now accepts the same 'tool_timeout=' kwarg as _codex_mcp_timeout_coherence_gate, defaulting to run_skill.mcp_tool_timeout_sec. The two sibling gates no longer have drifted signatures. Addresses PR #4657 review warnings: default-value drift and signature drift between the type/protocol/builder chain and its sibling coherence gates.
Trecek
pushed a commit
that referenced
this pull request
Aug 17, 2026
- _progress_heartbeat.py: 22-line docstring reduced to 8 lines; the BaseExceptionGroup unwrap rationale moves into a short inline comment where the code shows the pattern. - codex.py: three identical 4-line 'Codex has its own timeout mechanism' comments consolidated into one module-level block; the three call sites now reference it with a one-line cross-reference. - _init_helpers.py: _register_mcp_server docstring trims the marketing-style 'Claude Code v2.1.203+' prose. - _type_constants_env.py: 6-line comment for CLAUDE_CODE_MCP_TOOL_IDLE_TIMEOUT reduced to 2 lines. - _doctor_mcp.py: _check_claude_mcp_timeouts docstring drops the lengthy parenthetical scope-split commentary. - settings.py: _claude_mcp_timeout_coherence_gate docstring drops the 4-line parenthetical enumerating cases covered by the doctor check. Addresses PR #4657 review slop findings.
Trecek
pushed a commit
that referenced
this pull request
Aug 17, 2026
- test_run_skill_completion.py: add test_drafts_only branch coverage for pending_info()'s _drafts-only tuple (the first tuple of 'self._drafts or self._delivered' was uncovered). - test_run_skill_completion_middleware.py: trim 88-minute incident narrative from the delivered-only docstring; strengthen the elapsed_seconds assertion to >= 0.02 (was >= 0, trivially satisfied) and apply same to in_flight_only. - test_doctor_claude_mcp.py: write a timeout 1ms above the expected value to close the trivial-pass gap (was 'always-OK' bug blind spot). - test_progress_heartbeat_wiring.py: require await_count >= 2 to prove periodic ticks, not just one-shot (was >= 1, would pass a single one-shot call). - test_type_constants.py: collapse the four T1/T2/T3/T4 divider blocks; the value re-assertions in T3/T4 were tautological with T1 — keep only the import-path check. - test_doctor.py: trim 5-line assertion-design meta-commentary docstring; the test speaks for itself. - test_timeout_coherence.py: drop the '#4620 would have caught' incident-narrative class docstring. - test_subpackage_isolation.py: trim inline incident comment. - test_bundled_mcp_manifest.py: drop the 'see #4620' tag-along. Addresses PR #4657 review warnings (test_drafts_only coverage) and info findings on trivial assertions / incident-narrative slop.
…k blocking spans Both tools block on await inside anyio.fail_after(...) with zero MCP-visible liveness signal, which lets an idle-abort-capable client hard-abort the connection before the server-side timeout fires (#4620). - New server/_progress_heartbeat.py: progress_heartbeat() async context manager emits Context.report_progress() ticks every 30s (safe no-op when the client sent no progressToken). Unwraps the single-item ExceptionGroup anyio.create_task_group() wraps escaping exceptions in, mirroring the existing fleet/_api.py precedent, so callers inspecting type(exception).__name__ keep seeing the real exception type. - Wrap the two known blocking spans: tools_execution.py's tool_ctx.executor.run(...) and tools_fleet_dispatch.py's execute_dispatch(...). - Fix _notify()'s docstring (it sends logging notifications, not progress — it never called report_progress to begin with). Tests: unit coverage for the heartbeat primitive (ticks, cleanup, exception swallowing, ExceptionGroup unwrap), a behavioral regression test each for run_skill and dispatch_food_truck proving report_progress fires during the blocking span, and a _notify() assertion that it calls ctx.info and never ctx.report_progress. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RunSkillConfig.mcp_tool_timeout_sec (the value the server-side anyio.fail_after ceiling in run_skill/dispatch_food_truck actually uses) was already threaded into Codex's MCP timeout coherence gate and doctor check, but had zero consumers on the Claude side — Claude Code had no client-side timeout parity with the server deadline (#4620). - config/settings.py: _claude_mcp_timeout_coherence_gate(), mirroring the Codex gate, warns when mcp_tool_timeout_sec is below max(fleet_total, run_skill.timeout). - cli/doctor/_doctor_mcp.py: _check_claude_mcp_timeouts() checks ~/.claude.json's deployed timeout value against the currently configured one (deployed-vs-configured drift, not session-duration coherence — that's the gate's job). Wired into cli/doctor/__init__.py's check sequence. - New CLAUDE_CODE_MCP_TOOL_IDLE_TIMEOUT_ENV_VAR constant (core/types), the env var Claude Code reads (v2.1.187+; stdio servers exempt before v2.1.203) for its own idle-abort timeout. - Thread mcp_tool_timeout_sec through all Claude command builders (build_interactive_cmd, build_skill_session_cmd, build_food_truck_cmd, build_resume_cmd) via the same SkillSessionConfig/explicit-parameter precedent already used for exit_after_stop_delay_ms/stream_idle_timeout_ms. build_interactive_cmd/build_resume_cmd are shared CodingAgentBackend Protocol methods also implemented by CodexBackend, so the new parameter carries a None default and CodexBackend accepts-and-ignores it (Codex has its own timeout mechanism). - Add a timeout (ms) field to both MCP-server registration surfaces: cli/_init_helpers.py's _register_mcp_server() (~/.claude.json) and the plugin-bundled src/autoskillit/.mcp.json — mirrors the server-side anyio.fail_after ceiling exactly, and per Claude Code v2.1.203+ also floors the client's idle-abort timeout for this server's tool calls. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
When a run_skill result is completed but unacknowledged (the actual shape of the incident behind #4620's 88-minute stranding — a receipt sitting in _delivered while every other tool call is denied 'result awaiting acknowledgement'), the denial gave no indication of what's pending or for how long, making it indistinguishable from a genuine hang. - pipeline/run_skill_completion.py: _Invocation and RunSkillCompletionReceipt now carry started_at (time.monotonic(), set in begin(), copied through by draft()). New pending_info(tool_name) reads whichever of _in_flight/ _drafts/_delivered is authoritative — same priority order admission() checks — and returns {step_name, elapsed_seconds} for the oldest record. Purely additive: admission()'s signature and existing call sites are untouched. - core/types/_type_protocols_execution.py: pending_info() added to the RunSkillCompletionAuthority Protocol. - server/_run_skill_completion.py: the middleware now merges pending_info() plus a fixed guidance string into the run_skill_completion_pending denial envelope, alongside the existing admission() call. - sous-chef/SKILL.md: one sentence on reading elapsed_seconds/guidance from the denial and continuing to poll with backoff rather than concluding the pipeline is wedged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- server/_progress_heartbeat.py: catch BaseExceptionGroup, not the narrower ExceptionGroup — anyio wraps a wrapped-body BaseException (not just Exception subclasses) in a BaseExceptionGroup, which ExceptionGroup does not catch. This let a bare BaseException escape uncaught instead of being unwrapped to its original type (test_run_skill_aborts_completion_when_ base_exception_escapes). Added a regression test for this exact case. - cli/doctor/_doctor_mcp.py: import RunSkillConfig from the autoskillit.config package, not the private _config_dataclasses submodule (architectural guard: no cross-package submodule imports). - Updated architectural-baseline tests to reflect intentional growth: doctor check count (53->54, new claude_mcp_timeouts check), type-constants union size (166->167, new CLAUDE_CODE_MCP_TOOL_IDLE_TIMEOUT_ENV_VAR), SkillSessionConfig's exhaustive field set (+mcp_tool_timeout_sec), server/'s file-count exemption (27->28, new _progress_heartbeat.py), and claude.py/codex.py's line-limit exemptions (+2/+14 lines). - Updated test stubs that construct explicit-kwarg fakes of _run_interactive_session/_launch_cook_session and minimal load_config() doubles to accept/carry the new mcp_tool_timeout_sec parameter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…at the middleware layer Adds test_middleware_denial_payload_when_in_flight_only, exercising admission()'s "run_skill invocation in flight" branch and pending_info()'s corresponding _in_flight-only branch through the middleware — a code path the existing tests didn't reach (both existing middleware tests land in _drafts or _delivered via _finalized(), never a bare begin()). Addresses REQ-010 from the audit-impl finding in remediation_run-skill-idle-abort-immunity_2026-08-17_153315.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… builder overloads Replace truthy 'if mcp_tool_timeout_sec:' with explicit 'if mcp_tool_timeout_sec is not None and mcp_tool_timeout_sec > 0:' in build_interactive_cmd (L686), build_resume_cmd (L746), and build_food_truck_cmd (L963). The truthy form accepts NaN and negative seconds, which would serialize to env as 'nan' / '-1' and propagate into the Claude client. The > 0 check at build_skill_session_cmd (L856) was already correct. Addresses PR #4657 review warnings: defense boundary inconsistency finding (4 locations in claude.py). No behavior change for valid inputs.
- _doctor_mcp.py: type-guard the user-config timeout field before numeric comparison; a string/non-numeric value in ~/.claude.json now warns instead of raising TypeError. - _doctor_mcp.py: distinguish 'file missing' (skipped) from 'file unparseable' (warning). Previously the latter was silently masked as Severity.OK, defeating the check. - _init_helpers.py: validate mcp_tool_timeout_sec at write site, raising ValueError for non-numeric / non-positive values rather than writing a negative timeout that Claude interprets as 'abort immediately'. - tools_fleet_dispatch.py: validate the tool_timeout before anyio.fail_after() and return a fleet_error on invalid values instead of letting NaN / negative reach anyio. Addresses PR #4657 review warnings: type guard at parse site, swallowed exception masking corruption, late validation at write site, late validation before anyio.fail_after.
… signatures - _launch_adapter.py: give mcp_tool_timeout_sec a default of 0.0 to match the SkillSessionConfig convention (one of three optionality styles), eliminating the required-float divergence at the builder boundary. - settings.py: _claude_mcp_timeout_coherence_gate now accepts the same 'tool_timeout=' kwarg as _codex_mcp_timeout_coherence_gate, defaulting to run_skill.mcp_tool_timeout_sec. The two sibling gates no longer have drifted signatures. Addresses PR #4657 review warnings: default-value drift and signature drift between the type/protocol/builder chain and its sibling coherence gates.
- _progress_heartbeat.py: 22-line docstring reduced to 8 lines; the BaseExceptionGroup unwrap rationale moves into a short inline comment where the code shows the pattern. - codex.py: three identical 4-line 'Codex has its own timeout mechanism' comments consolidated into one module-level block; the three call sites now reference it with a one-line cross-reference. - _init_helpers.py: _register_mcp_server docstring trims the marketing-style 'Claude Code v2.1.203+' prose. - _type_constants_env.py: 6-line comment for CLAUDE_CODE_MCP_TOOL_IDLE_TIMEOUT reduced to 2 lines. - _doctor_mcp.py: _check_claude_mcp_timeouts docstring drops the lengthy parenthetical scope-split commentary. - settings.py: _claude_mcp_timeout_coherence_gate docstring drops the 4-line parenthetical enumerating cases covered by the doctor check. Addresses PR #4657 review slop findings.
- test_run_skill_completion.py: add test_drafts_only branch coverage for pending_info()'s _drafts-only tuple (the first tuple of 'self._drafts or self._delivered' was uncovered). - test_run_skill_completion_middleware.py: trim 88-minute incident narrative from the delivered-only docstring; strengthen the elapsed_seconds assertion to >= 0.02 (was >= 0, trivially satisfied) and apply same to in_flight_only. - test_doctor_claude_mcp.py: write a timeout 1ms above the expected value to close the trivial-pass gap (was 'always-OK' bug blind spot). - test_progress_heartbeat_wiring.py: require await_count >= 2 to prove periodic ticks, not just one-shot (was >= 1, would pass a single one-shot call). - test_type_constants.py: collapse the four T1/T2/T3/T4 divider blocks; the value re-assertions in T3/T4 were tautological with T1 — keep only the import-path check. - test_doctor.py: trim 5-line assertion-design meta-commentary docstring; the test speaks for itself. - test_timeout_coherence.py: drop the '#4620 would have caught' incident-narrative class docstring. - test_subpackage_isolation.py: trim inline incident comment. - test_bundled_mcp_manifest.py: drop the 'see #4620' tag-along. Addresses PR #4657 review warnings (test_drafts_only coverage) and info findings on trivial assertions / incident-narrative slop.
Reduce the consolidated Codex-timeout rationale comment to 3 lines and remove the trailing blank line so splitlines() stays under the 2458-line module size limit (was 2459 after pre-commit added the blank separator). Also re-applies the time.sleep(0.02) tweak to the delivered_only test that pre-commit restored.
The original review fix wrote observed = expected + 1, but the check returns OK when observed >= expected — so +1 still returns OK and a buggy always-OK comparison also returns OK. The trivial-pass gap remained open. Replace with a two-assertion companion that exercises BOTH branches: 1. observed == expected → OK (correct code path: not strictly less than) 2. observed == expected - 1 → WARNING (correct code path: strictly less than) A buggy always-OK comparison now fails the second assertion, exposing the bug. Discovered via re-validation pass.
…omment - test_middleware_denies_other_tools_while_receipt_is_pending (test_run_skill_completion_middleware.py:162-186): _finalized() exercises the _drafts-only branch of pending_info(); the elapsed_seconds >= 0 assertion was vacuously satisfied. Add time.sleep(0.02) and tighten the bound to >= 0.02 to match the hardened delivered_only and in_flight_only tests. Now all three pending_info branches are exercised with meaningful elapsed-time assertions. - test_ok_when_timeout_sufficient (test_doctor_claude_mcp.py:46-72): The stale comment said 'Use a value 1ms below expected' but the code writes expected_ms exactly (the +1 was already reverted in 0e6d15a). Rewrite the comment to describe the actual two-assertion contract: (1) observed == expected returns OK (pins the inclusive boundary that the strict-less-than check is the highest-value point to catch), (2) inline companion observed == expected - 1 returns WARNING (catches always-OK mutants).
…gister_mcp_server stub
…default on config load failure
…ailure instead of silent pass
…Group rationale into Raises
…n comparison guards REQ-017 — Add an explicit mcp_tool_timeout_sec parameter to build_headless_cmd() and inject CLAUDE_CODE_MCP_TOOL_IDLE_TIMEOUT when given, mirroring the existing pattern in build_skill_session_cmd and build_food_truck_cmd. Extend tests/execution/test_commands_invariants.py to include build_headless_cmd in both the inject and the omit-when-unset parametrize lists, asserting spec.env['CLAUDE_CODE_MCP_TOOL_IDLE_TIMEOUT'] is '14364.0' on the populated path and absent on the None/0.0 path. Also harden the mcp_tool_timeout_sec boundary check across all four claude builder overloads (build_interactive_cmd, build_resume_cmd, build_skill_session_cmd, build_food_truck_cmd) plus the new build_headless_cmd injection site: replace the bare '> 0' check with 'isinstance(mcp_tool_timeout_sec, (int, float)) and > 0', so that MagicMock values (and any other non-numeric type that sneaks past the config-boundary __post_init__ guards) no longer raise TypeError: '>' not supported between instances of 'MagicMock' and 'int' at the builder. This regression was introduced by ffc7b93 which unified the boundary check from truthy to explicit but did not account for MagicMock-bearing test configs. Fixes the two failures reported by test_check: - tests/cli/test_cook_features.py::TestOrderSubsetGate::test_order_enable_temporarily_sets_env_override - tests/cli/test_cook_order_command.py::TestCLIOrderCommand::test_order_backend_produces_valid_command[claude-code]
…ditions REQ-017 (resolve-failures iteration 1) adds 19 net lines to execution/backends/claude.py: explicit mcp_tool_timeout_sec parameter on build_headless_cmd with env-var injection, plus isinstance hardening at all four boundary checks (build_interactive_cmd, build_resume_cmd, build_skill_session_cmd, build_food_truck_cmd). Bumping the _LINE_LIMIT_EXEMPTIONS cap from 1252 to 1300 keeps the rationale single-source-of-truth in this test file and prevents a module-size regression failure unrelated to the underlying fix.
Trecek
force-pushed
the
impl-run-skill-idle-abort-immunity-20260816-222402
branch
from
August 18, 2026 21:18
f6e21a1 to
6248fef
Compare
Trecek
enabled auto-merge
August 18, 2026 21:22
Trecek
added a commit
that referenced
this pull request
Aug 18, 2026
Resolves conflicts between the execution-authority file decomposition and develop's #4641/#4644 cleanup-evidence, #4575 join-contract, and #4657 MCP-timeout fixes. Develop's fixes are preserved verbatim and relocated into the modules the decomposition moved their code to: - _codex_probes.py receives settle_evidence(), the cleanup_incomplete field, and the _validate_mcp_probe diagnostic warning. - _headless_adjudication.py receives _should_flag_cleanup_incomplete and the _make_terminated_result infra= wiring; _headless_result.py imports it. - process/_termination.py receives the settle_evidence() swap and the int | None return type; _coalesce_returncode stays in process/__init__.py where run_managed_async/run_managed_sync consume it. - Two develop-added probe tests retargeted from codex to _codex_probes so monkeypatching reaches the module that owns the symbol. Line-limit exemption entries keep the narrowed caps (codex.py 1300, _headless_result.py 900) with counts updated for develop's additions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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
run_skillblocks onawait tool_ctx.executor.run(...)(tools_execution.py:2365) insideanyio.fail_after(...)(:2352) with zero MCP-visible liveness signal — confirmed via a zero-hitreport_progressgrep acrosssrc/autoskillit/. Claude Code's client hard-aborts idle calls at 1800s; AutoSkillit computed an equivalent client-side timeout for Codex but never built one for Claude.dispatch_food_truck(tools_fleet_dispatch.py:359/670) has the byte-for-byte identical shape — same config field, same wrapper, same gap — confirming this is a defect class, not a one-off.Three fixes, kept as small as each one can be:
report_progressfrom both blocking call sites via one small reusable primitive.Closes #4620
Implementation Plan
Plan file:
.autoskillit/temp/rectify/rectify_run_skill_idle_abort_immunity_2026-08-16_210544.md🤖 Generated with Claude Code via AutoSkillit