feat: parallel sub-agent execution + read-only classifier hardening - #429
Merged
Merged
Conversation
- fd redirects: treat `>&N`/`>&-` as read-only fd duplication, but
`>&word` (non-digit, e.g. `echo x >& /tmp/f`) as a write redirect.
Same distinction applied in split_compound_segments and
has_background_operator so `2>&1` is not split or misread as a
background job.
- globs: allow unquoted globs; writes (`rm *.txt`, `cp *.x /d`) are
still caught by the blocked-command list and `echo *.x > f` by the
write-redirect check.
- find -exec: recursively classify the embedded command so
`find -exec grep {} \;` passes and `find -exec rm {} \;` is blocked;
`-delete` stays destructive. Handles `\;` and `+` terminators.
Closes AI-Shell-Team#428
When a tool-call batch is entirely `Agent` calls and len > 1, dispatch them concurrently via futures::join_all; all other batches stay sequential to preserve stop-on-first-short-circuit semantics. Each sub-agent is an isolated SubSession (own history, tools, cancellation, counters) so there is no shared mutable parent state under the concurrent execute_tool(&self) calls. - session.rs: extract process_tool_call_result (shared by both loops); add run_tool_calls with the parallel/sequential branch. - agent_tool.rs: wrap each sub-agent run in catch_unwind so a panicking sub-session degrades to ToolResult::error instead of unwinding through join_all and aborting its siblings. - app.rs: replace the AtomicBool sub-agent flag with an AtomicU32 active count (saturating decrement; ignore sub-agent OpEnd) so the spinner survives a fast-finishing sibling. - prompt.rs: steer the model to fan out multiple independent Agent calls in one response. - ApprovalMemory migrated to parking_lot::Mutex (no poisoning) so an unwound sub-agent cannot poison a lock shared via the parent and cascade a panic. Tests: join_all+zip preserves tool_call id order; concurrency probe proves real overlap (peak >= 2). Closes AI-Shell-Team#427
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
|
Template check passed. Thanks for updating the pull request description. |
|
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 ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe PR executes independent Agent calls concurrently, preserves result order, converts sub-agent panics into tool errors, tracks active sub-agents with an atomic counter, migrates approval memory to ChangesConcurrent Agent execution
Read-only Bash classification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LlmSession
participant AgentTool
participant SubSession
participant Shell
LlmSession->>AgentTool: dispatch independent Agent calls
AgentTool->>SubSession: execute calls concurrently
SubSession->>Shell: emit start and completion events
SubSession-->>AgentTool: return results or panic errors
AgentTool-->>LlmSession: return results
LlmSession->>LlmSession: process results in original order
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: 1
🧹 Nitpick comments (2)
crates/aish-tools/src/agent_tool/agent_tool.rs (1)
259-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese two assertions couple the test to example wording.
"scan"and"are healthy"come from two illustrative examples inUSAGE_SECTION. A prompt rewrite that keeps the parallel guidance but rephrases the examples breaks this test without any behavior change. The first four assertions already cover the contract the test describes.♻️ Proposed fix to drop the wording-coupled assertions
- assert!( - desc.contains("scan"), - "parallel guidance must cover file-scan scenarios" - ); - assert!( - desc.contains("are healthy"), - "parallel guidance must cover multi-service diagnosis scenarios" - );🤖 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-tools/src/agent_tool/agent_tool.rs` around lines 259 - 266, Remove the two wording-specific assertions checking desc.contains("scan") and desc.contains("are healthy") from the relevant test in agent_tool.rs. Keep the first four assertions unchanged, so the test validates the parallel-guidance contract without depending on illustrative example text in USAGE_SECTION.crates/aish-llm/src/session.rs (1)
1496-1504: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the
Agentsub-agent fan-out.
run_tool_callsstarts everyAgentcall in the batch at once withjoin_all. TheAgentprompt encourages emitting multipleAgentcalls, and each sub-agent runs its own LLM loop, so a large batch can multiply concurrent provider requests and hit rate limits.Use a small cap, such as
futures::stream::iter(...).buffered(N).collect().await, so the ordered results still work with the existingzip.🤖 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/session.rs` around lines 1496 - 1504, Bound concurrency in the parallel branch of run_tool_calls when executing multiple Agent tool calls: replace the unbounded futures::future::join_all over self.execute_tool(tc) with an ordered buffered stream using a small concurrency limit. Preserve result ordering and the existing results-to-tool-calls zip 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.
Inline comments:
In `@crates/aish-llm/src/session.rs`:
- Around line 1406-1434: Update the short-circuit branch in
process_tool_call_result to append a ChatMessage::tool_result for the current
call and every remaining parallel tool call before returning. Ensure
new_messages contains a matching tool_result for each assistant tool_call,
including sibling calls that completed before the short circuit, while
preserving the existing cancellation text behavior.
---
Nitpick comments:
In `@crates/aish-llm/src/session.rs`:
- Around line 1496-1504: Bound concurrency in the parallel branch of
run_tool_calls when executing multiple Agent tool calls: replace the unbounded
futures::future::join_all over self.execute_tool(tc) with an ordered buffered
stream using a small concurrency limit. Preserve result ordering and the
existing results-to-tool-calls zip behavior.
In `@crates/aish-tools/src/agent_tool/agent_tool.rs`:
- Around line 259-266: Remove the two wording-specific assertions checking
desc.contains("scan") and desc.contains("are healthy") from the relevant test in
agent_tool.rs. Keep the first four assertions unchanged, so the test validates
the parallel-guidance contract without depending on illustrative example text in
USAGE_SECTION.
🪄 Autofix
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: 8cdca27c-170e-467f-825b-16430efc0bca
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/aish-llm/Cargo.tomlcrates/aish-llm/src/session.rscrates/aish-shell/src/app.rscrates/aish-tools/src/agent_tool/agent_tool.rscrates/aish-tools/src/agent_tool/prompt.rscrates/aish-tools/src/bash/read_only.rs
jexShain
added a commit
to jexShain/aish
that referenced
this pull request
Aug 5, 2026
A short-circuit / failure-threshold return from the tool loop left the remaining assistant tool_calls without a matching tool_result. The shell persists new_messages verbatim, so the next turn would send the provider an assistant message with dangling tool_call_ids (rejected by OpenAI / Anthropic, which require a tool_result for every tool_call). - process_tool_call_result: append the short-circuiting call's own tool_result before returning (previously skipped). - run_tool_calls: on early return, backfill tool_results for every remaining call — the already-computed result in the parallel branch, a synthetic "[skipped ...]" in the sequential branch (those calls never executed). - Add regression test: a parallel Agent batch whose first call short-circuits must still produce a tool_result for every tool_call_id. Also drop two wording-coupled assertions in the Agent prompt test (scan / are healthy) that broke on example rewording without any behavior change; the remaining four still cover the parallelism contract. Addresses CodeRabbit review on AI-Shell-Team#429.
A short-circuit / failure-threshold return from the tool loop left the remaining assistant tool_calls without a matching tool_result. The shell persists new_messages verbatim, so the next turn would send the provider an assistant message with dangling tool_call_ids (rejected by OpenAI / Anthropic, which require a tool_result for every tool_call). - process_tool_call_result: append the short-circuiting call's own tool_result before returning (previously skipped). - run_tool_calls: on early return, backfill tool_results for every remaining call — the already-computed result in the parallel branch, a synthetic "[skipped ...]" in the sequential branch (those calls never executed). - Add regression test: a parallel Agent batch whose first call short-circuits must still produce a tool_result for every tool_call_id. Also drop two wording-coupled assertions in the Agent prompt test (scan / are healthy) that broke on example rewording without any behavior change; the remaining four still cover the parallelism contract. Addresses CodeRabbit review on AI-Shell-Team#429.
jexShain
force-pushed
the
feat/parallel-sub-agents
branch
from
August 5, 2026 05:35
6cd9e09 to
23d6357
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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
Agent(sub-agent) calls run sequentially, so multi-area requests take the sum of every sub-agent; a fast-finishing sibling also reset the shell's sub-agent flag prematurely. (2) The read-only bash classifier over-blocks harmless inspection (ls 2>&1,ls *.txt,find -exec grep) and let a real write (echo x >& /tmp/f) through as read-only.run_tool_callsdispatches all-Agentbatches (len>1) concurrently viajoin_all; other batches stay sequential (preserve stop-on-first-short-circuit).catch_unwind→ panic degrades toToolResult::error, not an unwind through siblings.AtomicBool→AtomicU32active count (saturating decrement; ignore sub-agentOpEnd).ApprovalMemory→parking_lot::Mutex(no poisoning) so an unwound sub-agent can't cascade a.lock().unwrap()panic.>&N/>&-read-only;>&word(non-digit) correctly a write; unquoted globs allowed;find -exec <cmd>recursively classified.tool_resultfor every assistanttool_call(the short-circuiting call + backfilled siblings), so the persisted history never carries a danglingtool_call_idthat providers reject next turn.Change Type
Scope
User-visible Changes
2>&1, unquoted globs, orfind -exec grep/wc/head;find -exec rmand>&filewrites stay blocked.Compatibility
Testing
cargo clippy -p aish-llm -p aish-tools -p aish-shell -- -D warnings— cleancargo test --workspace— all green (CI: 1823 passed / 0 failed on Rust 1.95.0)parallel_short_circuit_pairs_every_tool_call(every tool_call_id matched after a short-circuit); read-only redirect/glob/find-exec regressions.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
findactions and redirect operators.