fix(subagent): inherit parent tools and abort on Ctrl+C - #350
Merged
F16shen merged 2 commits intoJul 13, 2026
Conversation
Spawn filtered parent Tool Arcs instead of re-registering factories, and propagate raw-mode Ctrl+C (0x03) into session cancel so explore stops instead of inventing a timeout narrative.
Contributor
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
Contributor
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSub-agent spawning now inherits filtered parent tools through shared handles, with optional child-specific tool instances. Cancellation state distinguishes user interrupts, propagates through PTY and Bash execution, stops further LLM turns, and produces localized interruption output. ChangesSub-agent tools and cancellation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AishShell
participant AgentTool
participant BashTool
participant LlmSession
User->>AishShell: Press Ctrl-C
AishShell->>BashTool: Cancel PTY command
BashTool-->>LlmSession: Return user_cancelled short-circuit
LlmSession-->>AgentTool: Report cancelled result
AgentTool-->>AishShell: Cancel parent session
AishShell-->>User: Render shell.interrupted
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🧹 Nitpick comments (1)
crates/aish-llm/src/agents/tool_loop.rs (1)
425-457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the follow-up response remains queued.
The current assertions do not prove that another model turn was avoided. Consume the test queue after cancellation and verify the follow-up response is still present.
Proposed test assertion
assert!( session.cancellation_token().is_cancelled(), "session token must stay cancelled" ); + + let remaining = session + .chat_completion_raw(&[], None, false, None, None) + .await + .expect("follow-up response must remain queued"); + let LlmResponse::Json(json) = remaining else { + panic!("expected queued JSON response"); + }; + let (content, _, _, _) = StreamParser::parse_response(&json); + assert_eq!( + content.as_deref(), + Some("sleep was interrupted by built-in timeout") + ); }🤖 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 `@crates/aish-llm/src/agents/tool_loop.rs` around lines 425 - 457, Update test_loop_stops_after_tool_cancels_session to consume the session’s queued responses after asserting cancellation and verify the follow-up mock_text_response remains unconsumed. Use the session’s existing response-queue access mechanism and assert that the remaining response is the expected “sleep was interrupted by built-in timeout” message, proving no additional model turn occurred.
🤖 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 `@crates/aish-llm/src/agents/spawn.rs`:
- Around line 132-136: Validate the tool returned by for_sub_session before
registering it: compare its identity/name against the inherited allowlist, and
reject or skip it when the adapted tool is not allowlisted. Apply this
fail-closed check in the inherited_tools loop before sub.register_shared_tool,
preserving the original allowlist rather than trusting the adapted tool’s name.
In `@crates/aish-shell/src/app.rs`:
- Around line 2021-2025: Short-circuit cancelled successful results before
normal completion handling. In the response-processing branches around the
cancellation checks near the existing
`self.ai_handler.cancellation_token().is_cancelled()` logic, return or otherwise
exit the turn after printing the interrupted message so execution cannot reach
plan approval, success handling, or `record_history(input, 0)`; apply the same
fix to both affected branches.
In `@crates/aish-tools/src/bash/bash.rs`:
- Around line 417-419: Before the cancellation early return in the command
execution flow, synchronize the persistent PTY working directory using the same
cwd-update logic used on normal completion. Update the branch around
`self.outcome_if_cancelled(&cancel_token)` so cancellation preserves the shell’s
latest cwd before returning.
---
Nitpick comments:
In `@crates/aish-llm/src/agents/tool_loop.rs`:
- Around line 425-457: Update test_loop_stops_after_tool_cancels_session to
consume the session’s queued responses after asserting cancellation and verify
the follow-up mock_text_response remains unconsumed. Use the session’s existing
response-queue access mechanism and assert that the remaining response is the
expected “sleep was interrupted by built-in timeout” message, proving no
additional model turn occurred.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dd84e65-73a0-4972-adf1-badabfa1c41a
📒 Files selected for processing (13)
crates/aish-llm/src/agents/registry.rscrates/aish-llm/src/agents/spawn.rscrates/aish-llm/src/agents/tool_loop.rscrates/aish-llm/src/session.rscrates/aish-llm/src/types.rscrates/aish-pty/src/executor.rscrates/aish-pty/src/persistent.rscrates/aish-pty/src/types.rscrates/aish-shell/src/app.rscrates/aish-tools/src/agent_tool/agent_tool.rscrates/aish-tools/src/bash/bash.rscrates/aish-tools/src/lib.rscrates/aish-tools/tests/agent_tool_test.rs
Cancelled Ok("") must not run plan approval or record_history as success,
and persistent-PTY cancel should still apply the command's cwd.
6 tasks
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
ToolArcs (Claude Code–style) instead of re-registering per-type factories, so allowlist agents keep skill/WebFetch when present and bash binds the child cancel token.0x03) marks PTY cancel as user interrupt; bash cancels the session and short-circuits, the tool loop stops before another LLM turn, and the shell prints a single已中断.Test plan
cargo test -p aish-tools --lib -- test_user_interrupt_cancels_sessioncargo test -p aish-tools --test agent_tool_testcargo test -p aish-llm --lib -- test_loop_stops_after_tool_cancels;必须用 Agent(subagent_type=explore) 委派。子 agent 只用 bash 执行：sleep 90。→ wait for🔧 bash (sleep 90)→ Ctrl+C once → expect one已中断, no further explore/main “timeout” narrativeSummary by CodeRabbit