refactor: subagents as platform extension (enables subagents in code mode)

[tlongwell-block]

Moves the subagent tool from being a special case in Agent::dispatch_tool_call to a proper platform extension, following the same pattern as todo, skills, chatrecall, and code_execution extensions.

Motivation

1. Consistency: The subagent tool had special-case handling in the agent's dispatch logic
2. Code mode support: Platform extensions are accessible from code_execution - this change enables subagent delegation from JavaScript code

Changes

• New SubagentClient implementing McpClientTrait
• New call_tool_deferred trait method allowing extensions to return deferred futures (with sensible default for sync tools)
• PlatformExtensionContext extended with working_dir and sub_recipes
• TaskConfig simplified (session/working_dir now obtained from context)
• sub_recipes changed from Mutex to Arc<RwLock> for shared read access
• Change from "path" to "module_path" in handle_read_module fixes a pre-existing bug where the code looked for "path" but the schema defined module_path.

[tlongwell-block]

/goose trying to make subagent fit in as a platform extension rather than a special case. Also need it to work with code_execution. Is this an ideal move from the custom agent-registered subagent tool to a more standard platform extension? Any concerns? Is it at least as cleanly implemented/architected as the other platform extensions?

[github-actions]

Summary: This PR refactors the subagent tool from a special case in Agent::dispatch_tool_call to a standard platform extension. The implementation is clean, follows existing patterns well, and enables an important new capability: subagent delegation from code_execution mode (JavaScript). This is a solid architectural improvement.

✅ Highlights

• Clean abstraction: SubagentClient follows the same patterns as TodoClient, SkillsClient, and other platform extensions. The trait-based design is consistent.

• Defense in depth for nesting prevention: Two layers prevent subagents from spawning subagents:

  1. subagents_enabled() (agent.rs:615-624) returns false for SubAgent sessions
  2. SubagentClient.call_tool_deferred() (subagent_client.rs:156-165) explicitly checks session type

• Sensible trait design: The new call_tool_deferred method in McpClientTrait has a good default implementation that wraps sync call_tool results in future::ready(). Only clients needing async execution (like subagent) need to override it.

• RwLock vs Mutex: The change from Mutex<HashMap<String, SubRecipe>> to Arc<RwLock<HashMap<String, SubRecipe>>> is appropriate - sub_recipes are read frequently (for tool descriptions) and written rarely.

• Bundled bug fix: The path → module_path parameter rename in code_execution_extension.rs correctly aligns the handler with the ReadModuleParams struct definition.

🟢 Suggestions

• Consider looking up working_dir from session (subagent_client.rs:84-89): The current implementation falls back to std::env::current_dir() when context.working_dir is None. The old implementation used session.working_dir directly. While these are typically the same, you could make it more robust by looking up the session's working_dir via session_id:

  fn get_working_dir(&self) -> PathBuf {
      if let Some(working_dir) = &self.context.working_dir {
          return working_dir.clone();
      }
      // Could optionally look up from session here
      if let Some(session_id) = &self.context.session_id {
          if let Ok(session) = /* async lookup */ {
              return session.working_dir.clone();
          }
      }
      std::env::current_dir().unwrap_or_else(|_| PathBuf::from("."))
  }

  This is low priority since the fallback works in practice.

• Extension description consistency: The subagent extension is registered with slightly different descriptions in two places:

  • extension.rs:112: "Delegate tasks to independent subagents"
  • agent.rs:668: "Delegate tasks to independent subagents".to_string()

  Consider extracting to a constant for consistency (minor).

Overall: This is well-architected. The move to a platform extension is the right call for consistency and enabling code_execution integration. The implementation follows project patterns and has good defensive checks. Approve once you're happy with the working_dir behavior.

---

Review generated by goose

[tlongwell-block]

/goose

[github-actions]

Summary: This PR refactors the subagent tool from a special case in Agent::dispatch_tool_call to a proper platform extension, following the established pattern used by todo, skills, and code_execution extensions. The refactor enables subagent delegation from JavaScript code mode and improves architectural consistency. The implementation is well-executed with no blocking issues.

🟢 Suggestions

1. Bundled bug fix should be documented (code_execution_extension.rs:368-370)

   The change from "path" to "module_path" in handle_read_module fixes a pre-existing bug where the code looked for "path" but the schema defined module_path. Consider noting this bug fix in the PR description for transparency, or splitting it into a separate commit.

2. Minor: Documentation wording change (code_execution_extension.rs:611)

   The change from "- Last expression is the result" to "- To capture output: const r = <expression>; r" is clearer but slightly more verbose. This is fine, just noting it's included in this PR.

✅ Highlights

• Clean architecture: The new call_tool_deferred trait method with a sensible default implementation elegantly handles async tools without breaking existing extensions. (mcp_client.rs:68-82)

• Nested subagent prevention preserved: The removal of explicit check in dispatch_tool_call is safe because subagents_enabled() already returns false for SessionType::SubAgent sessions (agent.rs:615-624), preventing the extension from being enabled in subagent contexts.

• Appropriate lock upgrade: Changing sub_recipes from Mutex to Arc<RwLock> is correct since the extension context needs shared read access and writes are infrequent.

• Follows project patterns: SubagentClient structure matches other platform extensions (todo_extension, skills_extension) with consistent method signatures and error handling.

• TaskConfig simplification: Removing parent_session_id and parent_working_dir from TaskConfig in favor of obtaining them from context is a cleaner design.

---

Review generated by goose

[jamadeo]

just a thought (not feedback on this PR)

We could probably benefit from an extension construct that isn't necessarily an MCP extension. We're making good use of the platform extension construct, but having them "pretend" to be MCP servers results in us needing some odd code like this.

[jamadeo]

instead of a new function, how about changing this so that McpClientTrait::call_tool always returns a notification stream, with the default impl calling .subscribe()?

I'm also seeing that all of the platform extensions just return a dummy channel for subscribe, but ToolCallResult has an Option for the notifcations stream, so it would make more sense to have the trait fn itself return an Option

[tlongwell-block]

I think this is a good idea, but I don't necessarily want to do it in this PR.

[tlongwell-block]

/goose

[github-actions]

PR #6160: refactor: subagents as platform extension (enables subagents in code mode)

Summary: This PR successfully refactors the subagent tool from special-case handling in Agent::dispatch_tool_call to a proper platform extension following the established pattern. The architectural approach is sound, enables code_execution access to subagents, and reduces complexity in the agent dispatch logic.

✅ Highlights

1. Clean platform extension implementation (crates/goose/src/agents/subagent_client.rs)

   • The new SubagentClient follows the same pattern as TodoClient, SkillsClient, and other platform extensions, making the codebase more consistent.

2. Well-designed call_tool_deferred trait method (crates/goose/src/agents/mcp_client.rs:68-82)

   • Default implementation wraps call_tool result for backward compatibility with existing extensions.
   • Only extensions needing deferred execution (like subagent) override it.

3. Proper subagent spawn prevention (crates/goose/src/agents/subagent_client.rs:144-151)

   • Logic to prevent subagents from spawning other subagents is correctly preserved.
   • The extension also filters itself from the extensions list passed to spawned subagents (line 66-69).

4. Bug fix included (crates/goose/src/agents/code_execution_extension.rs:365-366)

   • Changed "path" to "module_path" to match the schema definition - good catch.

5. Appropriate concurrency primitive change (crates/goose/src/agents/agent.rs:85)

   • sub_recipes changed from Mutex to Arc<RwLock> is appropriate since sub_recipes is read-heavy.

6. Test improvements (crates/goose/src/agents/code_execution_extension.rs:339, 353)

   • Using PlatformExtensionContext::default() is cleaner than manually constructing with all None fields.

🟢 Suggestions

1. Consider adding tests for SubagentClient (crates/goose/src/agents/subagent_client.rs)

   • The new SubagentClient has no unit tests. Consider adding tests similar to what exists for CodeExecutionClient to verify:
     • Unknown tool name handling
     • Subagent-spawn-prevention logic
     • Provider-not-configured error case

2. Documentation opportunity (crates/goose/src/agents/mcp_client.rs:68)

   • The call_tool_deferred trait method could benefit from a doc comment explaining when to override it vs using the default implementation.

---

Review generated by goose

[tlongwell-block]

I think I need to make goose reviewer even more critical

[Copilot]

Pull request overview

This PR refactors subagent handling into a first-class platform extension and generalizes tool execution to support deferred results, enabling subagent delegation from both regular tool calls and code-execution mode.

Changes:

• Introduces DeferredToolCall and wires it through the agent, extension manager, router, and final-output tooling so extension tools can return futures plus optional notification streams.
• Converts the subagent functionality into a subagent platform extension (SubagentClient) using PlatformExtensionContext (now carrying sub_recipes) and a simplified TaskConfig, and auto-enables it when recipes include sub-recipes.
• Fixes code_execution’s read_module parameter name to module_path, updates tests accordingly, and aligns extension configuration enabling with PlatformExtensionDef::default_enabled.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/goose/tests/agent.rs	Updates extension manager tests to pass sub_recipes in PlatformExtensionContext, ensuring the test agent mirrors the new context shape.
crates/goose/src/execution/manager.rs	Ensures session-scoped ExtensionManager context now includes sub_recipes: Some(agent.sub_recipes()) so platform extensions can see recipe sub-recipes.
crates/goose/src/config/extensions.rs	Switches platform extension default enabled behavior to respect PlatformExtensionDef::default_enabled, allowing subagent/code-execution defaults to be controlled centrally.
crates/goose/src/agents/tool_route_manager.rs	Changes router search dispatch to return DeferredToolCall instead of the old wrapper type, aligning the router with the new deferred tool model.
crates/goose/src/agents/tool_execution.rs	Replaces the old ToolCallResult wrapper with DeferredToolCall (future + optional notification stream) and updates agent approval flow to consume it.
crates/goose/src/agents/subagent_tool.rs	Re-types handle_subagent_tool to produce DeferredToolCall while keeping validation and execution behavior the same.
crates/goose/src/agents/subagent_task_config.rs	Simplifies TaskConfig to hold only provider, extensions, and derived max_turns, dropping unused parent session/working-dir fields.
crates/goose/src/agents/subagent_client.rs	Adds the new SubagentClient MCP client exposing the subagent tool as a platform extension, enforcing no-subagents-from-subagents and wiring provider, extensions, sub-recipes, and working dir into handle_subagent_tool.
crates/goose/src/agents/mod.rs	Registers the new subagent_client module so the agent crate exports the subagent platform extension.
crates/goose/src/agents/mcp_client.rs	Extends McpClientTrait with a default call_tool_deferred that wraps call_tool into a DeferredToolCall, providing a unified async interface for extensions.
crates/goose/src/agents/final_output_tool.rs	Adapts final-output tool execution to return DeferredToolCall, keeping semantics while fitting the new tool execution abstraction.
crates/goose/src/agents/extension_manager.rs	Stores sub_recipes in the shared PlatformExtensionContext, exposes get_provider, and updates dispatch_tool_call to rely on call_tool_deferred and propagate notification streams cleanly.
crates/goose/src/agents/extension.rs	Adds the subagent platform extension definition and extends PlatformExtensionContext (with Default) to carry shared sub_recipes for platform clients.
crates/goose/src/agents/code_execution_extension.rs	Fixes handle_read_module to use module_path, updates instructions and tests, and simplifies tests to construct PlatformExtensionContext via Default.
crates/goose/src/agents/agent.rs	Switches sub-recipe storage to Arc<RwLock<_>>, exposes sub_recipes(), removes special-case subagent dispatch, adds ensure_subagent_for_recipes/enable_subagent_extension, and routes all tool calls through DeferredToolCall.
crates/goose-server/src/routes/recipe_utils.rs	After applying a recipe, calls agent.ensure_subagent_for_recipes() so recipe-driven sessions have the subagent extension enabled when appropriate.
crates/goose-cli/src/session/builder.rs	Passes sub_recipes into the CLI agent’s PlatformExtensionContext and calls ensure_subagent_for_recipes() so CLI sessions gain subagent support when recipes define sub-recipes.

[Copilot]

ensure_subagent_for_recipes and enable_subagent_extension now control when the new subagent platform extension is auto-enabled, but there don’t appear to be tests exercising cases like recipes with/without sub_recipes, non-autonomous mode, and subagent sessions; adding targeted tests around these branches would help prevent regressions in when the subagent tool is available and ensure the safety gating continues to behave as intended.

[jamadeo]

this makes me think that sub recipes should be part of the subagent platform extension itself, not the agent

at the least, I would say do this within the agent such that it isn't the caller's responsibility to always call this. But then you still have this situation where the Agent has sub_recipes, but they're non functional unless it's been configured with this extension

[tlongwell-block]

I agree. But I want to do that in a separate PR, potentially one for this overhaul #6202

[tlongwell-block]

/goose

[github-actions]

Summary: This PR moves the subagent tool from a special case in Agent::dispatch_tool_call to a proper platform extension, following the pattern of other platform extensions. While the architectural approach is sound and the refactor is well-structured, there is a critical bug where sub_recipes are never wired up to the new SubagentClient, which will cause named subrecipes to silently fail.

🔴 Blocking Issues

1. sub_recipes never passed to SubagentClient (HIGH confidence)

   The PlatformExtensionContext.sub_recipes field is added but never populated. When SubagentClient is created via the extension factory, its context always has sub_recipes: None.

   Evidence in crates/goose/src/agents/extension_manager.rs:447-457:

   Self {
       extensions: Mutex::new(HashMap::new()),
       context: PlatformExtensionContext {
           extension_manager: None,
           session_manager,
           sub_recipes: None,  // Always None!
       },
       ...
   }

   When the extension is created at line 594-596:

   let mut context = self.context.clone();  // Clones with sub_recipes: None
   context.extension_manager = Some(Arc::downgrade(self));
   (def.client_factory)(context)  // SubagentClient gets None for sub_recipes

   Result: SubagentClient.get_sub_recipes() (line 76-80) returns an empty HashMap, so named subrecipes defined in recipes will never be available to the subagent tool. Ad-hoc subagents (with instructions only) will still work, but the key recipe feature is broken.

   Fix: Either pass the Agent's sub_recipes Arc to ExtensionManager during construction, or add a setter method that ensure_subagent_for_recipes() can call to wire up the shared Arc before creating the extension.

🟡 Warnings

1. enable_subagent_extension() is dead code

   crates/goose/src/agents/agent.rs:788-809 defines enable_subagent_extension() but it's never called anywhere:

   $ rg "enable_subagent_extension\(" --type rust
   crates/goose/src/agents/agent.rs:    pub async fn enable_subagent_extension(...)
   # Only the definition, no callers

   This suggests the implementation may be incomplete. Consider either using this method or removing it.

2. call_tool() returns implementation-detail error

   In crates/goose/src/agents/subagent_client.rs:136-138:

   Ok(CallToolResult::error(vec![Content::text(
       "Subagent tool must be called via call_tool_deferred",
   )]))

   This leaks internal implementation details. If a different MCP client implementation calls call_tool() directly (which is valid per the trait), they get a confusing error. Consider either making call_tool() work by delegating to call_tool_deferred() and blocking on the result, or using a more user-friendly error message.

🟢 Suggestions

1. Consider default_enabled: false for subagent extension

   The extension is marked default_enabled: true in crates/goose/src/agents/extension.rs, but subagents are gated by GooseMode::Auto anyway. Making it default_enabled: false and only enabling via ensure_subagent_for_recipes() would be more consistent with the conditional nature of this feature.

✅ Highlights

• Clean DRY refactor of test helpers in code_execution_extension.rs with create_test_context()
• Good graceful fallback for working_dir when session lookup fails (lines 170-174 in subagent_client.rs)
• The call_tool_deferred trait extension is a reasonable pattern for deferred/async tool execution
• Proper filtering of the subagent extension from its own extension list (line 70-73) to prevent recursion

---

Review generated by goose

[tlongwell-block]

/goose

[github-actions]

Summary: This PR refactors the subagent tool from special-case handling in Agent::dispatch_tool_call to a proper platform extension following the established pattern (todo, skills, chatrecall, etc.). The approach is sound and well-executed, enabling subagent delegation from code_execution mode as intended.

🟡 Warnings

1. Missing ensure_subagent_extension call for non-recipe sessions (crates/goose-server/src/routes/agent.rs:421-445)

   In update_from_session, if session.recipe is None, apply_recipe_to_agent is never called, which means ensure_subagent_extension is skipped. Desktop sessions without recipes won't have the subagent extension enabled.

   if let Some(recipe) = session.recipe {
       // apply_recipe_to_agent calls ensure_subagent_extension
   }
   // But if no recipe, subagent extension is never added
   agent.extend_system_prompt(update_prompt).await;

   Consider adding agent.ensure_subagent_extension(&payload.session_id).await; after the recipe handling block, or documenting that subagents require recipes.

2. Redundant session fetch (crates/goose/src/agents/subagent_client.rs:157, 177-181)

   The session is fetched twice: once to check session_type and once to get working_dir. While not a blocking issue, this could be consolidated:

   // Currently:
   if let Ok(session) = session_manager.get_session(session_id, false).await {
       if session.session_type == SessionType::SubAgent { ... }
   }
   // ... later ...
   let working_dir = session_manager
       .get_session(session_id, false)  // second fetch
       .await
       .map(|s| s.working_dir)

🟢 Suggestions

1. Consider extracting gemini check to a shared utility (crates/goose/src/agents/subagent_client.rs:171)

   The starts_with("gemini") check exists in both agent.rs:subagents_enabled and subagent_client.rs:call_tool_deferred. While consistent, a shared is_gemini_model(&str) -> bool utility would ensure future changes stay in sync and could include other gemini-like model names (e.g., gemma).

2. Unused import Tool (crates/goose/src/agents/subagent_client.rs:17)

   The Tool type is imported but only used indirectly through build_tool() return type. Not blocking, but could be removed if the type inference works without it.

✅ Highlights

1. Excellent trait design: The call_tool_deferred trait method with a sensible default implementation is a clean pattern. Extensions that don't need deferred execution get it for free via the default, while SubagentClient can override it for async subagent spawning.

2. Good separation of concerns: The validation checks (goose_mode, session_type, gemini) in call_tool_deferred are appropriate - ensure_subagent_extension determines if the tool should be available, while call_tool_deferred performs runtime checks before execution.

3. Clean Mutex→RwLock migration: The change from Mutex<HashMap<String, SubRecipe>> to Arc<RwLock<HashMap<String, SubRecipe>>> is the right choice for shared read access from the extension.

4. Type rename improves clarity: Renaming ToolCallResult to DeferredToolCall better communicates that the type wraps a future that will eventually produce a result.

5. Proper extension filtering: Filtering out the subagent extension from extensions passed to subagents (line 70-73) correctly prevents subagent-spawning-subagent scenarios at the extension level.

---

Review generated by goose