/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?

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

/goose

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

/goose

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

I think I need to make goose reviewer even more critical