Skip to content

Conversation

@alexhancock
Copy link
Collaborator

Changing the way tool confirmation requests work to be powered by a more generalized "action required" infrastructure which will scale to other use-cases.

No user facing changes for desktop or CLI. Just reorganizing code.

The next thing that will use this is MCP elicitation support, which I have started in df0bd2f and I have seen enough to know this "action required" system is a good foundation.

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 refactors the tool confirmation system to use a more generalized "ActionRequired" infrastructure that can scale to support additional use cases beyond tool confirmations (such as MCP elicitation support mentioned in the description). The core change replaces ToolConfirmationRequest with a new ActionRequired type that wraps specific action types using a discriminated union pattern with actionType field. Both types are maintained in the MessageContent enum for backwards compatibility during the transition.

Key Changes:

  • Introduced ActionRequired and ActionRequiredData types with actionType discriminator for extensibility
  • Migrated tool confirmation requests to use the new ActionRequired infrastructure
  • Created new /action-required/tool-confirmation endpoint replacing /confirm
  • Updated all provider format functions to handle both legacy and new message content types

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
ui/desktop/src/types/message.ts Updated type imports and filtering logic to work with ActionRequired type
ui/desktop/src/components/ToolCallConfirmation.tsx Updated component to accept ActionRequired instead of ToolConfirmationRequest
ui/desktop/src/components/GooseMessage.tsx Updated to use confirmToolAction API with new data structure
ui/desktop/src/api/types.gen.ts Added ActionRequired types and ConfirmToolActionRequest, removed PermissionConfirmationRequest
ui/desktop/src/api/sdk.gen.ts Added confirmToolAction function, removed confirmPermission function
ui/desktop/openapi.json Added new endpoint schema and ActionRequired types to OpenAPI spec
crates/goose/src/providers/formats/*.rs Updated all provider format functions to skip both legacy and new action required messages
crates/goose/src/conversation/message.rs Added ActionRequired and ActionRequiredData types, updated helper methods
crates/goose/src/context_mgmt/mod.rs Added format handling for ActionRequired in context compaction
crates/goose/src/agents/tool_execution.rs Changed to use with_action_required instead of with_tool_confirmation_request
crates/goose-server/src/routes/reply.rs Removed confirm_permission endpoint and route
crates/goose-server/src/routes/mod.rs Added action_required module to route configuration
crates/goose-server/src/routes/action_required.rs New module with confirm_tool_action endpoint and request types
crates/goose-server/src/openapi.rs Updated OpenAPI documentation to include new types and endpoint
crates/goose-cli/src/session/output.rs Added rendering support for ActionRequired messages
crates/goose-cli/src/session/mod.rs Updated to extract tool confirmation from ActionRequired messages
crates/goose-cli/src/session/export.rs Added export formatting for ActionRequired messages

id: toolConfirmationContent.id,
action: 'deny',
sessionId,
action: "approve",
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

The action value "approve" is not handled by the backend. The backend's confirm_tool_action function only recognizes "always_allow", "allow_once", and "deny" (lines 39-44 in action_required.rs). An unrecognized action like "approve" will default to "deny", causing the opposite behavior of what's intended. This should be "deny" to cancel the tool when the message is cancelled.

Suggested change
action: "approve",
action: "deny",

Copilot uses AI. Check for mistakes.
);
}

export function getToolConfirmationContent(
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

Function name getToolConfirmationContent should be renamed to reflect that it now returns ActionRequired type, not ToolConfirmationRequest. Consider renaming to getActionRequiredContent or similar for clarity.

Suggested change
export function getToolConfirmationContent(
export function getActionRequiredContent(

Copilot uses AI. Check for mistakes.
@alexhancock alexhancock force-pushed the alexhancock/action-required branch 2 times, most recently from 45b00a9 to d21ccb7 Compare December 1, 2025 15:04
Copilot AI review requested due to automatic review settings December 1, 2025 15:04
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

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

@alexhancock alexhancock force-pushed the alexhancock/action-required branch from d21ccb7 to ca2da84 Compare December 1, 2025 15:20
@michaelneale
Copy link
Collaborator

nice @alexhancock, in this case was the copilot feedback on the money?

// Skip tool confirmation requests
}
MessageContent::ActionRequired(_action_required) => {
// Skip action required messages - they're for UI only
Copy link
Collaborator

Choose a reason for hiding this comment

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

this one ignores but other one like bedrock returns a value - expected to be different?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will look later tonight

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The bedrock one is more about what the provider gets as content in those cases I believe. It doesn't have any meaningful content either way so I will keep with the pattern in each file matching what ToolConfirmationRequest did in each format.

Copy link
Collaborator

@michaelneale michaelneale left a comment

Choose a reason for hiding this comment

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

tentative approve to not get in the way - just questions aorund what is in the providers and what is needed there and consistency etc.

Copilot AI review requested due to automatic review settings December 2, 2025 16:09
@alexhancock alexhancock force-pushed the alexhancock/action-required branch from ca2da84 to 5d382f5 Compare December 2, 2025 16:09
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

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

@alexhancock alexhancock merged commit 131c7e7 into main Dec 2, 2025
20 of 21 checks passed
@alexhancock alexhancock deleted the alexhancock/action-required branch December 2, 2025 16:48
zanesq added a commit that referenced this pull request Dec 2, 2025
* 'main' of github.com:block/goose:
  chore: upgrade npm packages (#5951)
  feat: ActionRequired (#5897)
  feat(acp): support loading sessions in acp (#5942)
  docs: add videos to multi-model page (#5938)
  docs: promote planning guide (#5934)
  fix: use a lock to ensure only need to run tunnel just in case multiple go… (#5885)
zanesq added a commit that referenced this pull request Dec 2, 2025
…0-5147

* 'main' of github.com:block/goose: (243 commits)
  chore: upgrade npm packages (#5951)
  feat: ActionRequired (#5897)
  feat(acp): support loading sessions in acp (#5942)
  docs: add videos to multi-model page (#5938)
  docs: promote planning guide (#5934)
  fix: use a lock to ensure only need to run tunnel just in case multiple go… (#5885)
  Feat: Added custom headers and toggle keyring CLI options (#5017)
  Feat/automatic update installation (#5345)
  fix: Added "Merged consecutive assistant messages" to the acceptable issues for moim injection check (#5933)
  fix: anthropic provider model fetching (#5932)
  [MCP-UI] add CSP for images to proxy HTML (#5931)
  fix: correct typo in blog post (AIMDOEL -> AIMODEL) (#5902)
  feat: @goose in terminal (native terminal support) (#5887)
  docs: adding AI-friendly features (#5918)
  Blog/advent of ai announcement (#5917)
  Extension selector behind ALPHA flag (#5892)
  blog: typo fixes (#5896)
  blog: fixing img url (#5895)
  blog: MCPs for Developers (#5884)
  docs: Extension Manager MCP (#5883)
  ...

# Conflicts:
#	crates/goose-server/src/routes/config_management.rs
#	crates/goose/src/providers/mod.rs
#	ui/desktop/openapi.json
#	ui/desktop/src/api/sdk.gen.ts
#	ui/desktop/src/api/types.gen.ts
#	ui/desktop/src/components/ProgressiveMessageList.tsx
tlongwell-block added a commit that referenced this pull request Dec 4, 2025
* origin/main:
  Update Anthropic and Google Gemini models to latest API versions (#5980)
  docs: chat recall tutorial (#5975)
  fix: `final assistant content cannot end with trailing whitespace` error from Anthropic (#5967)
  5527 multiple file popups (#5905)
  Groq configure fix  (#5833)
  added sidebar contextual information for firefox (#5433)
  docs: council of mine MCP (#5979)
  docs: nano banana extension (#5977)
  fix: remove prompt change, read model from config (#5976)
  Enable recipe deeplink parameters for pre-population (#5757)
  chore: upgrade npm packages (#5951)
  feat: ActionRequired (#5897)
  feat(acp): support loading sessions in acp (#5942)
  docs: add videos to multi-model page (#5938)
  docs: promote planning guide (#5934)
katzdave added a commit that referenced this pull request Dec 5, 2025
…nses-streaming

* 'main' of github.com:block/goose:
  blog: MCP Sampling (#5987)
  Update Anthropic and Google Gemini models to latest API versions (#5980)
  docs: chat recall tutorial (#5975)
  fix: `final assistant content cannot end with trailing whitespace` error from Anthropic (#5967)
  5527 multiple file popups (#5905)
  Groq configure fix  (#5833)
  added sidebar contextual information for firefox (#5433)
  docs: council of mine MCP (#5979)
  docs: nano banana extension (#5977)
  fix: remove prompt change, read model from config (#5976)
  Enable recipe deeplink parameters for pre-population (#5757)
  chore: upgrade npm packages (#5951)
  feat: ActionRequired (#5897)
  feat(acp): support loading sessions in acp (#5942)
  docs: add videos to multi-model page (#5938)
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