Skip to content

Conversation

@jamadeo
Copy link
Collaborator

@jamadeo jamadeo commented Dec 15, 2025

follow up from #5825 (comment)

Copilot AI review requested due to automatic review settings December 15, 2025 20:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR prevents subagent tools from being shown when operating within a subagent context by adding a check in the tool visibility logic.

  • Added SessionType import to support session type checks
  • Refactored existing subagent tool execution prevention for better readability
  • Added new check in tool visibility logic to hide subagent tools when the session is already a subagent

Comment on lines +655 to +665
if let Some(ref session_id) = self.extension_manager.get_context().await.session_id {
if matches!(
SessionManager::get_session(session_id, false)
.await
.ok()
.map(|session| session.session_type),
Some(SessionType::SubAgent)
) {
return false;
}
}
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SessionManager::get_session() call could fail silently with .ok(), which would cause subagent tools to be shown even when there's an error fetching the session. Consider logging the error case to help diagnose issues where the subagent tool incorrectly appears.

Copilot uses AI. Check for mistakes.
@jamadeo jamadeo requested a review from DOsinga December 15, 2025 20:50
{
return false;
}
if let Some(ref session_id) = self.extension_manager.get_context().await.session_id {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DOsinga I'm reading the session type here by way of the extension manager + platform extension context, which feels a bit roundabout but is also currently the most direct way.

Do we want to tie an agent to a session more directly? we of course map session id to agent, but the agent doesn't necessarily know its session

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the long run agents should be per-session right? with multiple sessions being possible per app instance and those two concepts coupled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we map session ID to agent right now, I'm just thinking should we keep a pointer in the other direction too

@jamadeo jamadeo merged commit 3737624 into main Dec 16, 2025
17 checks passed
@jamadeo jamadeo deleted the jackamadeo/dont-show-subagent-tool-to-subagents branch December 16, 2025 14:23
aharvard added a commit that referenced this pull request Dec 16, 2025
…erer

* origin/main: (26 commits)
  Don't persist ephemeral extensions when resuming sessions (#5974)
  chore(deps): bump mdast-util-to-hast from 13.2.0 to 13.2.1 in /ui/desktop (#5939)
  chore(deps): bump node-forge from 1.3.1 to 1.3.2 in /documentation (#5898)
  Add Scorecard supply-chain security workflow (#5810)
  Don't show subagent tool when we're a subagent (#6125)
  Fix keyboard shortcut conflict for Focus Goose Window (#5809)
  feat(goose-cli): add feature to disable update (#5886)
  workflow: enable docs-update-recipe-ref (#6132)
  fix: filter tools in Ollama streaming when chat mode is enabled (#6118)
  feat(mcp): platform extension for "code mode" MCP tool calling (#6030)
  workflow: auto-update recipe-reference on release (#5988)
  Document recipe slash commands feature (#6075)
  docs: add GitHub Copilot device flow authentication details (#6123)
  Disallow subagents with no extensions (#5825)
  chore(deps): bump js-yaml in /documentation (#6093)
  feat: external goosed server (#5978)
  fix: Make datetime info message more explicit to prevent LLM confusion about current year (#6101)
  refactor: unify subagent and subrecipe tools into single tool (#5893)
  goose repo is too big for the issue solver workflow worker (#6099)
  fix: use system not developer role in db (#6098)
  ...
zanesq added a commit that referenced this pull request Dec 16, 2025
* 'main' of github.com:block/goose: (22 commits)
  OpenRouter & Xai streaming (#5873)
  fix: resolve mcp-hermit cleanup path expansion issue (#5953)
  feat: add goose PR reviewer workflow (#6124)
  perf: Avoid repeated MCP queries during streaming responses (#6138)
  Fix YAML serialization for recipes with special characters (#5796)
  Add more posthog analytics (privacy aware) (#6122)
  docs: add Sugar MCP server to extensions registry (#6077)
  Fix tokenState loading on new sessions (#6129)
  bump bedrock dep versions (#6090)
  Don't persist ephemeral extensions when resuming sessions (#5974)
  chore(deps): bump mdast-util-to-hast from 13.2.0 to 13.2.1 in /ui/desktop (#5939)
  chore(deps): bump node-forge from 1.3.1 to 1.3.2 in /documentation (#5898)
  Add Scorecard supply-chain security workflow (#5810)
  Don't show subagent tool when we're a subagent (#6125)
  Fix keyboard shortcut conflict for Focus Goose Window (#5809)
  feat(goose-cli): add feature to disable update (#5886)
  workflow: enable docs-update-recipe-ref (#6132)
  fix: filter tools in Ollama streaming when chat mode is enabled (#6118)
  feat(mcp): platform extension for "code mode" MCP tool calling (#6030)
  workflow: auto-update recipe-reference on release (#5988)
  ...

# Conflicts:
#	ui/desktop/src/App.tsx
#	ui/desktop/src/api/sdk.gen.ts
#	ui/desktop/src/components/ChatInput.tsx
#	ui/desktop/src/components/recipes/RecipesView.tsx
zanesq added a commit that referenced this pull request Dec 16, 2025
…s-predefined-models

* 'main' of github.com:block/goose: (81 commits)
  fix: display shell output as static text instead of spinner (#6041)
  fix : Custom providers with empty API keys show as configured in desktop (#6105)
  Add .agents/skills and ~/.config/agent/skills to skills discovery paths (#6139)
  fix: use instructions for system prompt and prompt for user message in subagents (#6121)
  Fix compaction loop for small models or large input (#5803)
  feat: Centralize theme management with ThemeContext (#6137)
  OpenRouter & Xai streaming (#5873)
  fix: resolve mcp-hermit cleanup path expansion issue (#5953)
  feat: add goose PR reviewer workflow (#6124)
  perf: Avoid repeated MCP queries during streaming responses (#6138)
  Fix YAML serialization for recipes with special characters (#5796)
  Add more posthog analytics (privacy aware) (#6122)
  docs: add Sugar MCP server to extensions registry (#6077)
  Fix tokenState loading on new sessions (#6129)
  bump bedrock dep versions (#6090)
  Don't persist ephemeral extensions when resuming sessions (#5974)
  chore(deps): bump mdast-util-to-hast from 13.2.0 to 13.2.1 in /ui/desktop (#5939)
  chore(deps): bump node-forge from 1.3.1 to 1.3.2 in /documentation (#5898)
  Add Scorecard supply-chain security workflow (#5810)
  Don't show subagent tool when we're a subagent (#6125)
  ...

# Conflicts:
#	crates/goose/src/providers/formats/databricks.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants