refactor(tools): extract supervised batch execution engine - #207
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTool execution is centralized in a supervised batch engine with ordered results, callbacks, deduplication, bounded cancellation, and late-work cleanup. Core step handling, ChangesTool execution batching
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PythinkerSoul
participant pythinker_core.step
participant PythinkerToolset
participant ToolExecutionEngine
participant _ExecutionBatch
participant Tool
PythinkerSoul->>pythinker_core.step: pass ToolBatchContext
pythinker_core.step->>PythinkerToolset: handle_batch(tool calls)
PythinkerToolset->>ToolExecutionEngine: delegate batch
ToolExecutionEngine->_ExecutionBatch: prepare and supervise
_ExecutionBatch->>Tool: execute calls
Tool-->>_ExecutionBatch: return ToolResult
_ExecutionBatch-->>pythinker_core.step: ordered results and summary
pythinker_core.step-->>PythinkerSoul: StepResult
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@packages/pythinker-core/src/pythinker_core/__init__.py`:
- Around line 94-96: Update the async callback handling around
inspect.isawaitable and async_callback_done so created tasks are registered with
the batch/StepResult supervisor instead of detached via ensure_future. Ensure
tool_results() settlement drains and cancel_tool_execution() cancels these
tasks, preserving synchronous callback behavior and confirming no tasks remain
leaked.
In `@packages/pythinker-core/tests/test_batch_toolset.py`:
- Around line 433-463: Remove the unused started_work field and its trivially
true assertion from ConstructionFailureToolset and
test_batch_construction_failure_is_side_effect_free. Rename the test to reflect
failure propagation and callback suppression, while retaining the RuntimeError
expectation and callbacks == [] assertion.
In `@src/pythinker_code/soul/tool_execution.py`:
- Around line 849-856: Update the BaseException cleanup in PythinkerSoul so
watcher tasks whose corresponding source futures are already complete are not
cancelled; disable callbacks for them, then gather them to let _watch() copy
their results into _completed_results. Continue cancelling watchers tied to
unfinished sources and preserve the existing source-future cancellation and
gathering behavior.
- Around line 876-891: Update _bounded_settlement to check
self._supervisor.done() before invoking asyncio.wait_for, allowing an
already-settled supervisor to complete immediately when timeout is zero without
raising ToolCancellationTimeoutError. Preserve the existing timed wait and
cancellation-timeout handling for unsettled supervisors, and add a regression
test covering timeout=0 with an already-completed supervisor.
In `@tests/core/test_tool_execution_engine.py`:
- Around line 70-80: Decouple the tests from private implementation details: in
tests/core/test_tool_execution_engine.py lines 70-80, exercise handle() through
a real test tool or supported public seam instead of patching _execution.handle;
in tests/core/test_session_logging.py lines 208-208 and 233-233, capture and
assert the emitted error and warning logs rather than patching
tool_execution.logger.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a71a1c18-4f48-428c-a849-95e331bc5cc6
⛔ Files ignored due to path filters (5)
docs/en/customization/architecture.mdis excluded by!docs/**docs/en/release-notes/changelog.mdis excluded by!docs/**docs/superpowers/reports/2026-07-15-tool-execution-after.jsonis excluded by!docs/**docs/superpowers/reports/2026-07-15-tool-execution-before.jsonis excluded by!docs/**docs/superpowers/reports/2026-07-15-tool-execution-engine-characterization.mdis excluded by!docs/**
📒 Files selected for processing (13)
CHANGELOG.mdCONTRIBUTING.mdpackages/pythinker-core/src/pythinker_core/__init__.pypackages/pythinker-core/src/pythinker_core/tooling/__init__.pypackages/pythinker-core/tests/test_batch_toolset.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/tool_execution.pysrc/pythinker_code/soul/toolset.pytests/core/test_pythinkersoul_stuck_loop.pytests/core/test_pythinkersoul_turn_balance.pytests/core/test_session_logging.pytests/core/test_tool_execution_cancellation.pytests/core/test_tool_execution_engine.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/soul/toolset.py (1)
1071-1088: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve caller cancellation until after MCP teardown.
If cancellation lands during
self._execution.cleanup(),CancelledErrorbypasses_close(), leaving MCP session holders and clients open. Capture it alongside the timeout, finish teardown, then re-raise it.Proposed fix
- execution_error: ToolCancellationTimeoutError | None = None + execution_error: BaseException | None = None try: await self._execution.cleanup() - except ToolCancellationTimeoutError as error: + except (asyncio.CancelledError, ToolCancellationTimeoutError) as error: execution_error = error🤖 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 `@src/pythinker_code/soul/toolset.py` around lines 1071 - 1088, Update the cleanup flow in the enclosing teardown method to catch and store asyncio.CancelledError alongside ToolCancellationTimeoutError from self._execution.cleanup(). Always complete the existing MCP _close teardown and gather operations, then re-raise the captured cancellation after teardown, preserving the current timeout-error behavior.
🤖 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.
Outside diff comments:
In `@src/pythinker_code/soul/toolset.py`:
- Around line 1071-1088: Update the cleanup flow in the enclosing teardown
method to catch and store asyncio.CancelledError alongside
ToolCancellationTimeoutError from self._execution.cleanup(). Always complete the
existing MCP _close teardown and gather operations, then re-raise the captured
cancellation after teardown, preserving the current timeout-error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 302d48b8-3168-45ae-99bb-90f5dc472a86
⛔ Files ignored due to path filters (5)
docs/en/customization/agent-architecture.mdis excluded by!docs/**docs/superpowers/plans/2026-07-15-tool-execution-cancellation-state-rollback.mdis excluded by!docs/**docs/superpowers/specs/2026-07-15-tool-execution-cancellation-state-rollback-design.mdis excluded by!docs/**tasks/lessons.mdis excluded by!tasks/**tasks/todo.mdis excluded by!tasks/**
📒 Files selected for processing (7)
packages/pythinker-core/src/pythinker_core/__init__.pypackages/pythinker-core/tests/test_batch_toolset.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/tool_execution.pysrc/pythinker_code/soul/toolset.pytests/core/test_pythinkersoul_turn_balance.pytests/core/test_tool_execution_cancellation.py
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.
Related Issue
No linked issue; implements the approved third phase of the provider/stream/tool-execution design.
Description
BatchToolset/ToolBatchHandleprotocol while preserving third-partyToolset.handle()compatibilityStepResultToolExecutionEnginePythinkerToolsetresponsible for registry, visibility, dependency injection, and MCP lifecyclePythinkerSoulthroughToolBatchContextandToolBatchSummaryinstead of execution internalsFailure and compatibility guarantees
results()stays in model call orderVerification
make check-pythinker-coremake test-pythinker-core— 429 passedmake check-pythinker-codemake test-pythinker-code— 7,063 root passed; 65 e2e passedMeasured local medians increased by 3.6–14.5% for execution fixtures, 19.4–20.7% for dedupe fixtures, and 3.8–4.2% for registry projection. Raw samples and the residual-risk assessment are tracked under
docs/superpowers/reports/2026-07-15-tool-execution-*.Checklist
CHANGELOG.mdand generated the docs changelog withcd docs && npm run sync.Summary by CodeRabbit