approval required based on annotations

[allisoneer]

Pretty sloppy and in-process.

---

Important

Adds tool approval logic based on annotations, updating controllers and managers to handle new metadata.

• Behavior:
  • Introduces ToolAnnotation struct in mcpserver_types.go and toolcall_types.go to store tool metadata.
  • Updates ToolCallReconciler to handle tool approval based on annotations, including session-level approvals.
  • Adds logic to TaskReconciler to manage tool calls with annotations.
• Controllers:
  • ToolCallReconciler now checks ToolAnnotation for approval logic, including ReadOnlyHint to skip approvals.
  • TaskReconciler creates tool calls with annotations and manages their execution.
• Managers:
  • MCPServerManager updated to include tool annotations when listing tools.
• Misc:
  • Adds createToolApprovalRequest function in toolcall_controller.go for building approval requests.
  • Updates llm_client.go to include ACPToolAnnotations in Tool struct.

^{This description was created by for 987031a. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 987031a in 1 minute and 43 seconds. Click for details.

• Reviewed 646 lines of code in 7 files
• Skipped 0 files when reviewing.
• Skipped posting 9 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. acp/api/v1alpha1/mcpserver_types.go:89

• Draft comment:
  The new ToolAnnotation struct looks useful. Consider expanding the documentation to explain the default behavior when these boolean hints are omitted, and how they interact with tool execution (for instance, what should a nil value imply versus false).
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

2. acp/api/v1alpha1/toolcall_types.go:46

• Draft comment:
  The ToolAnnotations field has been added to ToolCallSpec. Ensure that any upstream validation or conversion logic correctly handles missing annotations and that the JSON schema documentation is updated if needed.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

3. acp/internal/adapters/mcp_adapter.go:43

• Draft comment:
  The client tool creation now includes 'ACPToolAnnotations' taken from the MCPTool. This propagation appears correct; verify that the LLM client later leverages these annotations for approval or execution decisions.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

4. acp/internal/controller/task/task_controller.go:528

• Draft comment:
  In the createToolCalls function, a map (toolAnnotationsMap) is built keyed by tool.Function.Name. Ensure that the tool names are always unique across different MCP servers to avoid accidentally overwriting annotations.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that tool names are unique across different servers. This is a request for confirmation of intention, which violates the rules. The comment does not provide a specific code suggestion or ask for a specific test to be written.

5. acp/internal/controller/toolcall/toolcall_controller.go:535

• Draft comment:
  The handlePendingApproval function distinguishes between a human contact flow and a standard function call flow. The nested if-conditions and branch for responseOption are a bit complex; consider refactoring for clarity and reducing nesting in a future cleanup.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. acp/internal/controller/toolcall/toolcall_controller.go:757

• Draft comment:
  In the requestHumanApproval function, when task.Spec.ResponseURL is present, the HTTP request is built and sent. Please review that the HTTP client enforces secure (TLS) connections and that errors (especially during JSON decoding for call ID extraction) are logged carefully. The fallback logic that generates a placeholder call ID should be reviewed to avoid potential collisions or ambiguity.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 50% <= threshold 50% The comment is asking the PR author to review the HTTP client for secure connections and error logging, which is not allowed. However, it also suggests reviewing the fallback logic for potential issues, which is a valid concern. The comment is partially useful.

7. acp/internal/controller/toolcall/toolcall_controller.go:1228

• Draft comment:
  The createToolApprovalRequest helper builds a payload with multiple response options. The implementation is sound; just ensure that these approval option names (approve, approve_session, reject) are consistently used throughout the system and that any TODOs regarding session-level approvals are tracked.
• Reason this comment was not posted:
  Confidence changes required: 40% <= threshold 50% None

8. acp/internal/llmclient/llm_client.go:32

• Draft comment:
  The LLMClient interface and LLMRequestError type are straightforward. Consider whether additional error context (or wrapping the original error) might be useful for debugging LLM failures.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

9. acp/internal/mcpmanager/mcpmanager.go:202

• Draft comment:
  In the MCPServerManager, tool conversion now includes copying the ToolAnnotation. The code that resolves secret values for environment variables is good; however, take care to avoid logging sensitive data when errors occur. Additionally, ensure that any changes in secret resolution or annotations are properly tested.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

Workflow ID: wflow_EcJxnAECkPRBpXF2

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}