ch10 phase 5: TaskStop typed dispatch via stop_task() helper - #24
Merged
agentforce314 merged 1 commit intoMay 8, 2026
Merged
Conversation
Phase 5 of the chapter-10 orchestration port. SRP cleanup: the TaskStop
tool layer becomes a thin formatter; dispatch logic moves to a typed
helper.
What lands:
- src/tasks/stop_task.py: stop_task(task_id, context, *, reason="")
async helper. Three dispatch branches in priority order:
1. Typed runtime_tasks lookup → get_task_by_type(state.type).kill()
bounded by asyncio.wait_for(timeout=5.0).
2. Legacy task_manager ManagedTask fallback (carries forward the
Phase-0 sub-fix; documented as "removed when ManagedTask is fully
migrated to the typed registry — Phase-11 follow-up backlog").
3. Not-found.
StopTaskResult and StopTaskError are frozen dataclasses with
is_error as a derived property. Return-value path (not raised
exceptions), matching TS stopTask.ts.
StopTaskErrorCode literal includes the chapter's three canonical
codes (`not_found` / `not_running` / `unsupported_type`) plus a
Python-specific extension `kill_timeout` for the async-kill timeout
case (TS kills are sync and never time out, so the code didn't exist
there). Without this, a hung kill would either claim success silently
(the WI-0.1 footgun returns) or get mapped to a chapter code that
lies about the failure mode.
- src/tool_system/tools/task_stop.py: _task_stop_call shrinks from
~135 lines (Phase 0) to ~30 lines, of which one is the dispatch call:
result = await stop_task(task_id, context, reason=reason).
Remaining lines are schema validation + output formatting. SRP:
the tool formats; the helper dispatches.
The Phase-0 sub-fix's task_manager dispatch branch is folded into
stop_task() proper; legacy tests/test_tool_system_tools.py::
TestTaskStopTool::test_task_stop now passes for the right reason
via typed dispatch.
The legacy background_bash_tasks dispatch branch is dropped —
production bash tasks are reachable via runtime_tasks (the lockstep
mirror from Phase 1's WI-1.4); only test fixtures that bypassed the
spawn pipeline needed migration.
- tests/test_tool_system_tools.py: TestTaskStopTool::test_task_stop
and TestNewParityTools::test_task_tools_roundtrip updated to drive
async tools through asyncio.run.
Tests (14 in tests/tasks/test_stop_task.py): dataclass shapes (2),
not_found, 4 not_running cases (parametrized over completed/failed/
killed), unsupported_type, kill_timeout (M1 regression preserved
through the WI-5.1 hoist), happy-path typed dispatch, local_agent
kill (status flips, abort_event set), task_manager fallback,
race-vs-natural-completion (natural completion wins → not_running),
tool-layer formatting (StopTaskError.code → output.error_code).
Phase 5 acceptance gates: _task_stop_call body is one dispatch line +
~30 lines formatting; all three TS-canonical error codes round-trip;
Chunk-A task_manager legacy branch folded into stop_task(), legacy
test still passes via typed dispatch; M1 timeout regression preserved
verbatim through the move; all Phase 0+1+2+3+4 tests still green.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
agentforce314
approved these changes
May 8, 2026
agentforce314
left a comment
Owner
There was a problem hiding this comment.
Approving — clean SRP-cleanup refactor.
What I checked
_task_stop_callshrinking from ~135 lines to ~30 lines (one of which is the dispatch call) is the textbook outcome of hoisting business logic out of a tool layer. The remaining ~30 lines really should be schema validation + output formatting only.- The three-branch dispatch order (typed
runtime_tasks→ legacytask_managerfallback → not-found) is the right priority. Documenting the legacy branch for Phase-11 backlog removal — rather than hiding it — is the right call. StopTaskResult/StopTaskErroras frozen dataclasses withis_erroras a derived property avoids the "two fields that can disagree" failure mode.
The kill_timeout divergence is well-justified
- TS kills are sync and never time out, so the code didn't exist there. Mapping the timeout to
not_found/not_running/unsupported_typewould lie about the failure mode — losing the ability for callers to distinguish "agent didn't respond" from "agent doesn't exist." Adding a fourth literal that doesn't exist in TS is the right call here. - The M1 regression test (
kill_timeoutreturnsis_error=Truerather than silently claiming success) is exactly the test that needs to exist for this divergence.
Nit (non-blocking)
- The
asyncio.wait_for(timeout=5.0)constant should probably be a module-level named constant (e.g._KILL_TIMEOUT_SECONDS = 5.0) — easier to find / tune later.
2 tasks
3 tasks
ericleepi314
pushed a commit
that referenced
this pull request
Jul 7, 2026
ch10 phase 5: TaskStop typed dispatch via stop_task() helper
singlaamitesh
pushed a commit
to singlaamitesh/clawcodex
that referenced
this pull request
Jul 7, 2026
Major issues raised in the post-blocker Critic review: - WI-8.5 binary persistence: wire persist_binary_content into the tool_wrapper flatten path. Image blocks + binary resource blocks now persist to a tempfile and surface a path reference instead of '[image content]' / raw base64. Resource blocks with .text fields inline as text. - filter_mcp_servers_by_policy: read disable_all_mcp / allow_managed_only_mcp from SettingsSchema.extra (and accept the camelCase alias) since these fields are not declared on the schema. The policy gate was previously inert. - claudeai loader: get_all_mcp_configs now picks up the cached claudeai snapshot (warmed by an async prefetch) via get_cached_claudeai_mcp_configs, and runs the gap-agentforce314#24 dedup against manual entries. Adds get_cached_claudeai_mcp_configs export. - Session-expiry regex (_SESSION_TERMINATED_RE): tighten to require a recognized session-expiry code (-32001 or 32600) alongside the 'Session terminated' literal. Earlier permissive form would misclassify e.g. -32602 (Invalid Params) errors that happened to use that message text, triggering spurious reconnects. Test for the old behavior renamed and inverted to a regression guard. - auth_provider: drop scopes_supported fallback. Requesting every scope an AS advertises is an overreach; default to empty list and let the AS pick its minimum. - auth_discovery timeout 10s → 30s to match TS AUTH_REQUEST_TIMEOUT_MS = 30000. Cold AS discoveries can legitimately take 10-20s. - auth_provider._inflight_locks: replace setdefault (which builds a discarded Lock on every call) with a get/insert pattern. - connection_manager.toggle_mcp_server: run flip + reconnect in one lock acquisition (inline the reconnect path) to close the race where two concurrent toggles could observe inconsistent state. Test coverage added (tests/test_mcp_critic_majors.py, 36 tests): - XAA two-step token exchange (happy path + 5 error paths) - xaa_idp_login JWT exp parsing + eligibility gate - output_validation truncation + tiktoken fast path - tool_wrapper input validation (jsonschema) - connection_manager reconnect / toggle / trigger_oauth - InProcessTransport round-trip + close cascade + close-unblocks-peer - WI-8.5 binary content persistence (image + blob resource) - Policy filter reads extra dict (snake + camelCase) + dedup - Session-expiry regex tightening regression Suite: 343 passed (was 292 baseline). Co-Authored-By: Claude Opus 4.7 <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
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
Phase 5 — SRP cleanup hoist. The TaskStop tool layer becomes a thin formatter; dispatch logic moves to a typed helper.
src/tasks/stop_task.py—stop_task(task_id, context, *, reason="")async helper. Three dispatch branches in priority order:runtime_taskslookup →get_task_by_type(state.type).kill()bounded byasyncio.wait_for(timeout=5.0).task_managerManagedTaskfallback (carries forward the Phase-0 sub-fix; documented for Phase-11 backlog removal).StopTaskResultandStopTaskErrorare frozen dataclasses;is_erroris a derived property. Return-value path (not raised exceptions), matching TS.StopTaskErrorCodeliteral includes the chapter's three codes (not_found/not_running/unsupported_type) pluskill_timeout— a Python-specific extension covering the async-kill timeout case (TS kills are sync and never time out, so the code didn't exist there). Mapping the timeout to any of the chapter's three codes would lie about the failure mode._task_stop_callshrinks from ~135 lines to ~30 lines, of which one is the dispatch call:result = await stop_task(...). The rest is schema validation + output formatting.task_managerlegacy branch is folded intostop_task(); legacyTestTaskStopTool::test_task_stopnow passes for the right reason via typed dispatch.background_bash_tasksdirect-read fallback is dropped — production bash tasks are reachable via the lockstep mirror; only test fixtures needed migration.14 new tests in
tests/tasks/test_stop_task.py, including an M1 regression test (kill_timeout) that asserts the timeout path returnsis_error=Trueinstead of silently claiming success.Stacked PR. Base:
feat/ch10-coordination-phase3-4. Merge after the Phase 3+4 PR.Test plan
_task_stop_callbody is one dispatch line + ~30 lines formattingtask_managerlegacy branch folded intostop_task(); legacy test still passes via typed dispatch