diff --git a/surfsense_backend/app/agents/multi_agent_chat/middleware/shared/permissions/ask/payload.py b/surfsense_backend/app/agents/multi_agent_chat/middleware/shared/permissions/ask/payload.py index 317cecfad..6c5d011df 100644 --- a/surfsense_backend/app/agents/multi_agent_chat/middleware/shared/permissions/ask/payload.py +++ b/surfsense_backend/app/agents/multi_agent_chat/middleware/shared/permissions/ask/payload.py @@ -17,14 +17,21 @@ from app.agents.new_chat.permissions import Rule PERMISSION_ASK_INTERRUPT_TYPE = "permission_ask" -_PERMISSION_ASK_DECISIONS: list[str] = [ +_BASE_PERMISSION_ASK_DECISIONS: list[str] = [ LC_DECISION_APPROVE, LC_DECISION_REJECT, LC_DECISION_EDIT, - SURFSENSE_DECISION_APPROVE_ALWAYS, ] +def _is_mcp_tool(tool: BaseTool | None) -> bool: + """An MCP tool advertises a connector id in its langchain metadata.""" + if tool is None: + return False + metadata = getattr(tool, "metadata", None) or {} + return metadata.get("mcp_connector_id") is not None + + def _card_fields_from_tool(tool: BaseTool | None) -> dict[str, Any]: """Project the FE card's tool-scoped fields out of a BaseTool.""" if tool is None: @@ -52,10 +59,15 @@ def build_permission_ask_payload( ) -> dict[str, Any]: """Build the permission-ask interrupt payload. - ``tool`` carries the FE card's tool-scoped fields (description, MCP - connector). When omitted the card still renders, just without the - "Always Allow against this connected account" surface. + ``approve_always`` is added to the palette only for MCP tools, since that + is the only case where the user's choice can persist beyond the current + agent instance (saved to the connector's trusted-tools list). Native + tools fall back to the once/reject/edit triad. """ + allowed_decisions = list(_BASE_PERMISSION_ASK_DECISIONS) + if _is_mcp_tool(tool): + allowed_decisions.append(SURFSENSE_DECISION_APPROVE_ALWAYS) + context: dict[str, Any] = { "patterns": patterns, "rules": [ @@ -68,7 +80,7 @@ def build_permission_ask_payload( return build_lc_hitl_payload( tool_name=tool_name, args=args, - allowed_decisions=_PERMISSION_ASK_DECISIONS, + allowed_decisions=allowed_decisions, interrupt_type=PERMISSION_ASK_INTERRUPT_TYPE, context=context, ) diff --git a/surfsense_backend/tests/unit/agents/multi_agent_chat/middleware/shared/permissions/test_lc_hitl_wire.py b/surfsense_backend/tests/unit/agents/multi_agent_chat/middleware/shared/permissions/test_lc_hitl_wire.py index 599330116..f8399c031 100644 --- a/surfsense_backend/tests/unit/agents/multi_agent_chat/middleware/shared/permissions/test_lc_hitl_wire.py +++ b/surfsense_backend/tests/unit/agents/multi_agent_chat/middleware/shared/permissions/test_lc_hitl_wire.py @@ -64,8 +64,12 @@ async def test_permission_ask_payload_uses_lc_hitl_shape(): ], f"REGRESSION: permission ask reverted to legacy shape; got {value!r}" review = value.get("review_configs") assert isinstance(review, list) and len(review) == 1 - # ``approve_always`` must be in the palette so the FE can render the promote button. - assert "approve_always" in review[0]["allowed_decisions"] + palette = review[0]["allowed_decisions"] + # Native tool (no ``tool=`` argument): the palette must include the + # once/reject/edit triad. ``approve_always`` is gated on MCP-ness and + # therefore *omitted* here — palette content per tool kind is + # exercised in ``test_permission_ask_mcp_context``. + assert "approve" in palette and "reject" in palette and "edit" in palette assert value.get("interrupt_type") == "permission_ask" # SurfSense context rides through verbatim for FE explainability. assert value["context"]["patterns"] == ["rm/*"] diff --git a/surfsense_backend/tests/unit/agents/multi_agent_chat/middleware/shared/permissions/test_permission_ask_mcp_context.py b/surfsense_backend/tests/unit/agents/multi_agent_chat/middleware/shared/permissions/test_permission_ask_mcp_context.py index f70f027a9..c9bd4e142 100644 --- a/surfsense_backend/tests/unit/agents/multi_agent_chat/middleware/shared/permissions/test_permission_ask_mcp_context.py +++ b/surfsense_backend/tests/unit/agents/multi_agent_chat/middleware/shared/permissions/test_permission_ask_mcp_context.py @@ -81,6 +81,53 @@ def test_payload_omits_tool_fields_when_tool_is_none(): assert "tool_description" not in ctx +def test_palette_includes_approve_always_for_mcp_tool(): + """Saving to the connector's trusted-tools list is only possible for MCP tools.""" + tool = _make_mcp_tool( + name="linear_create_issue", connector_id=42, connector_name="Linear" + ) + palette = build_permission_ask_payload( + tool_name=tool.name, + args={}, + patterns=[tool.name], + rules=[_ask_rule(tool.name)], + tool=tool, + )["review_configs"][0]["allowed_decisions"] + assert "approve_always" in palette + + +def test_palette_excludes_approve_always_for_native_tool(): + """Native tools have no place to persist trust, so don't offer the button.""" + native = StructuredTool( + name="rm", + description="Remove a file.", + coroutine=_noop, + args_schema=_NoArgs, + metadata={"hitl": True}, + ) + palette = build_permission_ask_payload( + tool_name=native.name, + args={"path": "/tmp/x"}, + patterns=[native.name], + rules=[_ask_rule(native.name)], + tool=native, + )["review_configs"][0]["allowed_decisions"] + assert "approve_always" not in palette + assert palette == ["approve", "reject", "edit"] + + +def test_palette_excludes_approve_always_when_tool_is_none(): + """Without a tool object the middleware can't tell — fall back to the safe triad.""" + palette = build_permission_ask_payload( + tool_name="rm", + args={"path": "/tmp/x"}, + patterns=["rm"], + rules=[_ask_rule("rm")], + tool=None, + )["review_configs"][0]["allowed_decisions"] + assert palette == ["approve", "reject", "edit"] + + def test_payload_omits_falsy_mcp_metadata_fields(): tool = StructuredTool( name="anon_tool",