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.}

[cschanhniem]

The ToolAnnotation approach for approval gating is a clean primitive — annotations are lightweight and don’t require a separate policy engine. A few thoughts from working on agent governance:

1. Annotation scope vs policy scope: ToolAnnotation couples approval logic to tool definitions. This works well when the tool author knows what needs approval. But there is a case where the repo maintainer, not the tool author, wants to gate tool access — especially in multi-tenant setups where different repos have different risk tolerances for the same tool. A ReadOnlyHint flag handles the simple case, but a require_approval at the repo level (e.g. "shell_execute always requires approval on this repo, regardless of tool annotations") is a separate concern. Consider whether annotations are the data model or just the default source — and whether a repo-level policy override makes sense.

2. Deterministic decision vs LLM decision: Annotation-based decisions are deterministic — has ReadOnlyHint → skip approval. That is the right default. The danger zone is when the approval decision itself involves an LLM call — that introduces non-determinism and potential prompt injection at the gate. AGENTOWNERS models this as a strict invariant: the enforcement path must be deterministic (no LLM). The escalation path (human review) can use LLM assistance, but the enforcement decision is pure logic.

3. Session-level approvals: The session-level approval scope is interesting — it is essentially a trust escrow. One thing to watch for: does a session-level approval for one tool implicitly trust other tool calls in the same session? If so, that is a policy elevation vector. Explicit scope narrowing (approve this specific tool call, not the whole session) is safer.