From d51edcae5698ad59a39b1596bba043e43cbc474c Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Date: Wed, 23 Sep 2026 18:53:26 -0300 Subject: [PATCH 1/5] feat(sdk): add client methods skills already cite by MCP name SDK agents can call the AI automation family, verified member removal, and phase fill without a separate MCP-only path. The AI filter, the removal check, and the editable-field filter live on the client. Signed-off-by: Adrianno Esnarriaga --- packages/sdk/src/pipefy_sdk/__init__.py | 2 + packages/sdk/src/pipefy_sdk/client.py | 117 ++++++ packages/sdk/src/pipefy_sdk/member_removal.py | 56 +++ packages/sdk/tests/test_mcp_named_facade.py | 386 ++++++++++++++++++ packages/sdk/tests/test_member_removal.py | 160 ++++++++ 5 files changed, 721 insertions(+) create mode 100644 packages/sdk/src/pipefy_sdk/member_removal.py create mode 100644 packages/sdk/tests/test_mcp_named_facade.py create mode 100644 packages/sdk/tests/test_member_removal.py diff --git a/packages/sdk/src/pipefy_sdk/__init__.py b/packages/sdk/src/pipefy_sdk/__init__.py index 95f612d7..fca71cb2 100644 --- a/packages/sdk/src/pipefy_sdk/__init__.py +++ b/packages/sdk/src/pipefy_sdk/__init__.py @@ -18,6 +18,7 @@ classify_exception, classify_graphql_error_dicts, ) +from pipefy_sdk.member_removal import MemberRemovalResult from pipefy_sdk.models import ( CONDITION_OPERATIONS, Attachment, @@ -157,6 +158,7 @@ "filter_fields_by_definitions", "MePayload", "MemberInvite", + "MemberRemovalResult", "NonBlankStr", "PipefyAPIError", "PipefyClient", diff --git a/packages/sdk/src/pipefy_sdk/client.py b/packages/sdk/src/pipefy_sdk/client.py index 7f77bd95..61e7f787 100644 --- a/packages/sdk/src/pipefy_sdk/client.py +++ b/packages/sdk/src/pipefy_sdk/client.py @@ -11,6 +11,7 @@ from pipefy_sdk import __version__ from pipefy_sdk.ai_pipe_validation import resolve_and_populate_field_refs from pipefy_sdk.ai_preflight import ( + filter_ai_automation_summaries, validate_ai_agent_behaviors_sdk, validate_ai_automation_prompt_sdk, ) @@ -20,11 +21,17 @@ validate_traditional_automation_move_transition, ) from pipefy_sdk.exceptions import AiAgentConfigureError +from pipefy_sdk.field_filters import ( + filter_editable_field_definitions, + filter_fields_by_definitions, + skipped_field_ids, +) from pipefy_sdk.graphql_executor import ( AuthenticatedExecutor, GraphQLEndpoint, GraphQLExecutor, ) +from pipefy_sdk.member_removal import MemberRemovalResult, verify_member_removal from pipefy_sdk.models.ai_agent import ( BehaviorInput, CreateAiAgentInput, @@ -764,6 +771,19 @@ async def remove_members_from_pipe( """ return await self._member_service.remove_members_from_pipe(pipe_id, user_ids) + async def remove_member_from_pipe( + self, pipe_id: str, user_ids: list[str] + ) -> MemberRemovalResult: + """Remove users from a pipe, then read members back to detect a no-op. + + Runs the same mutation as :meth:`remove_members_from_pipe` (the raw + mutation with no read-back), then verifies whether any requested user + remains. Org-level permissions can override pipe-level removal. + """ + raw = await self.remove_members_from_pipe(pipe_id, user_ids) + warning = await verify_member_removal(self, pipe_id, user_ids) + return {"data": raw, "warning": warning} + async def set_role( self, pipe_id: str, member_id: str, role_name: str ) -> dict[str, Any]: @@ -935,6 +955,33 @@ async def get_automations( after=after, ) + async def get_ai_automation( + self, automation_id: str + ) -> AutomationRuleRecord | None: + """MCP-named alias for :meth:`get_automation`.""" + return await self.get_automation(automation_id) + + async def get_ai_automations( + self, + pipe_id: str, + organization_id: str | None = None, + *, + first: int | None = None, + after: str | None = None, + ) -> AutomationListPage: + """List one page of AI (``generate_with_ai``) automation rules for a pipe. + + The row filter runs after the page, so ``totalCount`` and ``pageInfo`` + describe the mixed connection. + """ + page = await self.get_automations( + organization_id=organization_id, + pipe_id=pipe_id, + first=first, + after=after, + ) + return {**page, "nodes": filter_ai_automation_summaries(page["nodes"])} + async def get_automation_actions(self, pipe_id: str) -> list[AutomationActionRow]: """List available automation action types for a pipe (for building create/update payloads).""" return await self._automation_service.get_automation_actions(pipe_id) @@ -1095,6 +1142,12 @@ async def delete_automation( """Delete a traditional automation rule by ID (permanent).""" return await self._automation_service.delete_automation(automation_id) + async def delete_ai_automation( + self, automation_id: str + ) -> DeleteAutomationServiceResult: + """MCP-named alias for :meth:`delete_automation`.""" + return await self.delete_automation(automation_id) + async def get_ai_agent(self, agent_uuid: str) -> AiAgentGraphPayload: """Get an AI Agent by UUID (name, instruction, behaviors).""" return await self._ai_agent_service.get_agent(agent_uuid) @@ -1634,6 +1687,70 @@ async def update_card( field_updates=field_updates, ) + async def fill_card_phase_fields( + self, + card_id: str | int, + phase_id: str | int, + fields: dict[str, Any] | None, + *, + required_fields_only: bool = False, + ) -> dict[str, Any]: + """Fill a card's phase fields using only IDs the phase exposes as editable. + + Reads :meth:`get_phase_fields` once, then writes via :meth:`update_card` + only when at least one value survives the editable-id filter. Dropped + keys are returned in ``skipped_field_ids``; a write that drops nothing + omits that key. When nothing survives, no + write is issued; the result reports ``success``, ``message``, + ``phase_id``, ``phase_name``, and ``skipped_field_ids``. Pass + ``required_fields_only=True`` to filter phase definitions to required + fields only (forwarded as ``required_only`` on :meth:`get_phase_fields`). + """ + phase_fields_result = await self.get_phase_fields( + phase_id, required_only=required_fields_only + ) + expected_fields = filter_editable_field_definitions( + phase_fields_result.get("fields", []) + ) + phase_name = phase_fields_result.get("phase_name") or f"Phase {phase_id}" + given_fields = fields or {} + field_data = filter_fields_by_definitions(given_fields, expected_fields) + dropped = skipped_field_ids(given_fields, field_data) + if not field_data: + if expected_fields: + message = ( + "No field values were collected, so nothing was updated. " + f"Phase '{phase_name}' has {len(expected_fields)} editable " + "field(s); pass 'fields' keyed by the IDs from " + "get_phase_fields(phase_id)." + ) + else: + read_message = phase_fields_result.get("message") + if required_fields_only and read_message: + message = f"{read_message} Nothing was updated." + elif given_fields: + message = ( + f"Phase '{phase_name}' has no editable fields; " + "nothing was updated." + ) + else: + message = "No fields to update." + return { + "success": True, + "message": message, + "phase_id": phase_id, + "phase_name": phase_name, + "skipped_field_ids": dropped, + } + field_updates = [ + {"field_id": field_id, "value": value} + for field_id, value in field_data.items() + ] + api_response = await self.update_card(card_id, field_updates=field_updates) + if dropped: + return {**api_response, "skipped_field_ids": dropped} + return api_response + async def delete_card(self, card_id: str | int) -> dict: """Delete a card by its ID.""" return await self._card_service.delete_card(card_id) diff --git a/packages/sdk/src/pipefy_sdk/member_removal.py b/packages/sdk/src/pipefy_sdk/member_removal.py new file mode 100644 index 00000000..40ee076c --- /dev/null +++ b/packages/sdk/src/pipefy_sdk/member_removal.py @@ -0,0 +1,56 @@ +"""Read-back after pipe member removal: warn when a user is still present.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +from typing_extensions import TypedDict + +if TYPE_CHECKING: + from pipefy_sdk.client import PipefyClient + + +class MemberRemovalResult(TypedDict): + data: dict[str, Any] + warning: str | None + + +async def verify_member_removal( + client: PipefyClient, + pipe_id: str, + user_ids: list[str], +) -> str | None: + """Return a warning when requested users remain on the pipe after removal. + + Returns ``None`` when every requested id or uuid is gone, when ``pipe_id`` + is not numeric (the members read needs a numeric id), or when + ``get_pipe_members`` raises. + """ + pipe_id_str = str(pipe_id).strip() + if not pipe_id_str.isdigit(): + return None + + try: + members_data = await client.get_pipe_members(pipe_id_str) + except Exception: # noqa: BLE001 + return None + + members = (members_data.get("pipe") or {}).get("members") or [] + remaining_ids: set[str] = set() + for member in members: + user = member.get("user") if isinstance(member.get("user"), dict) else {} + if user.get("id"): + remaining_ids.add(str(user["id"])) + if user.get("uuid"): + remaining_ids.add(str(user["uuid"])) + + requested = {str(uid) for uid in user_ids} + still_present = requested & remaining_ids + if not still_present: + return None + + ids_str = ", ".join(sorted(still_present)) + return ( + f"API returned success but member(s) [{ids_str}] are still present in the pipe. " + "They may have org-level permissions that override pipe-level removal." + ) diff --git a/packages/sdk/tests/test_mcp_named_facade.py b/packages/sdk/tests/test_mcp_named_facade.py new file mode 100644 index 00000000..004fea3d --- /dev/null +++ b/packages/sdk/tests/test_mcp_named_facade.py @@ -0,0 +1,386 @@ +"""Facade-level tests: MCP-named methods on ``PipefyClient``.""" + +from __future__ import annotations + +from unittest.mock import AsyncMock + +import pytest +from _shared.fixture_ids import ( + EXAMPLE_FIELD_INTERNAL_ID, + EXAMPLE_NUMERIC_ORG_ID, + EXAMPLE_ORG_UUID, + EXAMPLE_PHASE_ID, + EXAMPLE_PIPE_ID, +) + +from pipefy_sdk import PipefyClient +from pipefy_sdk.models.form import MalformedFieldDefinitionError + +AI_ROW = {"id": "1", "name": "AI", "action_id": "generate_with_ai"} +MOVE_ROW = {"id": "2", "name": "Move", "action_id": "move_single_card"} +AUTOMATION_RECORD = { + "id": "501", + "name": "AI rule", + "action_id": "generate_with_ai", +} +MIXED_PAGE = { + "nodes": [AI_ROW, MOVE_ROW], + "totalCount": 2, + "pageInfo": {"hasNextPage": True, "endCursor": "cursor-1"}, +} +MUTATION_RESULT = {"removeMembersFromPipe": {"success": True}} +REMAINING_USER_ID = EXAMPLE_FIELD_INTERNAL_ID +STILL_PRESENT_WARNING = ( + f"API returned success but member(s) [{REMAINING_USER_ID}] are still present in the pipe. " + "They may have org-level permissions that override pipe-level removal." +) +CARD_ID = "99" +PHASE_NAME = "Review" +TWO_EDITABLE_FIELDS = [ + {"id": "status", "editable": True}, + {"id": "title", "editable": True}, +] +NON_EDITABLE_FIELDS = [ + {"id": "readonly", "editable": False}, + {"id": "locked", "editable": False}, +] +UPDATE_CARD_RESULT = {"updateFieldsValues": {"success": True}} +MALFORMED_PHASE_FIELDS = MalformedFieldDefinitionError( + "Cannot return phase fields: 1 field definition(s) from Pipefy are " + "missing required 'id' or 'type'. The pipe configuration may be " + "incomplete or unsupported." +) + + +def _phase_fields(fields: list[dict]) -> dict: + return { + "phase_id": EXAMPLE_PHASE_ID, + "phase_name": PHASE_NAME, + "fields": fields, + } + + +def _collected_nothing_message(editable_count: int) -> str: + return ( + "No field values were collected, so nothing was updated. " + f"Phase '{PHASE_NAME}' has {editable_count} editable field(s); " + "pass 'fields' keyed by the IDs from get_phase_fields(phase_id)." + ) + + +@pytest.fixture +def facade_client() -> PipefyClient: + client = PipefyClient.__new__(PipefyClient) + client.get_automation = AsyncMock(return_value=AUTOMATION_RECORD) + client.get_automations = AsyncMock(return_value=MIXED_PAGE) + client.delete_automation = AsyncMock(return_value={"success": True}) + client.remove_members_from_pipe = AsyncMock(return_value=MUTATION_RESULT) + client.get_pipe_members = AsyncMock(return_value={"pipe": {"members": []}}) + client.get_phase_fields = AsyncMock(return_value=_phase_fields(TWO_EDITABLE_FIELDS)) + client.update_card = AsyncMock(return_value=UPDATE_CARD_RESULT) + return client + + +@pytest.mark.anyio +async def test_get_ai_automation_forwards_id_and_returns_record( + facade_client: PipefyClient, +): + record = await facade_client.get_ai_automation("501") + + assert record == AUTOMATION_RECORD + facade_client.get_automation.assert_awaited_once_with("501") + + +@pytest.mark.anyio +async def test_get_ai_automations_filters_nodes_keeps_mixed_page_pagination( + facade_client: PipefyClient, +): + page = await facade_client.get_ai_automations( + EXAMPLE_PIPE_ID, + organization_id=EXAMPLE_NUMERIC_ORG_ID, + first=10, + after="cursor-0", + ) + + facade_client.get_automations.assert_awaited_once_with( + organization_id=EXAMPLE_NUMERIC_ORG_ID, + pipe_id=EXAMPLE_PIPE_ID, + first=10, + after="cursor-0", + ) + assert page["nodes"] == [AI_ROW] + assert page["totalCount"] == MIXED_PAGE["totalCount"] + assert page["pageInfo"] == MIXED_PAGE["pageInfo"] + + +@pytest.mark.anyio +async def test_delete_ai_automation_forwards_to_delete_automation( + facade_client: PipefyClient, +): + result = await facade_client.delete_ai_automation("501") + + assert result == {"success": True} + facade_client.delete_automation.assert_awaited_once_with("501") + + +@pytest.mark.anyio +async def test_remove_member_from_pipe_returns_data_and_null_warning_when_gone( + facade_client: PipefyClient, +): + result = await facade_client.remove_member_from_pipe( + EXAMPLE_PIPE_ID, [REMAINING_USER_ID] + ) + + facade_client.remove_members_from_pipe.assert_awaited_once_with( + EXAMPLE_PIPE_ID, [REMAINING_USER_ID] + ) + assert result == {"data": MUTATION_RESULT, "warning": None} + + +@pytest.mark.anyio +async def test_remove_member_from_pipe_returns_warning_when_member_still_present( + facade_client: PipefyClient, +): + facade_client.get_pipe_members.return_value = { + "pipe": { + "members": [ + {"user": {"id": REMAINING_USER_ID, "uuid": EXAMPLE_ORG_UUID}}, + ] + } + } + + result = await facade_client.remove_member_from_pipe( + EXAMPLE_PIPE_ID, [REMAINING_USER_ID] + ) + + facade_client.remove_members_from_pipe.assert_awaited_once_with( + EXAMPLE_PIPE_ID, [REMAINING_USER_ID] + ) + assert result == {"data": MUTATION_RESULT, "warning": STILL_PRESENT_WARNING} + + +@pytest.mark.anyio +async def test_fill_card_phase_fields_writes_one_editable_and_skips_unknown( + facade_client: PipefyClient, +): + result = await facade_client.fill_card_phase_fields( + CARD_ID, + EXAMPLE_PHASE_ID, + {"status": "done", "unknown": "x"}, + ) + + facade_client.get_phase_fields.assert_awaited_once_with( + EXAMPLE_PHASE_ID, required_only=False + ) + facade_client.update_card.assert_awaited_once_with( + CARD_ID, + field_updates=[{"field_id": "status", "value": "done"}], + ) + assert result == {**UPDATE_CARD_RESULT, "skipped_field_ids": ["unknown"]} + + +@pytest.mark.anyio +async def test_fill_card_phase_fields_write_omits_skipped_ids_when_none_dropped( + facade_client: PipefyClient, +): + result = await facade_client.fill_card_phase_fields( + CARD_ID, + EXAMPLE_PHASE_ID, + {"status": "done"}, + ) + + facade_client.get_phase_fields.assert_awaited_once_with( + EXAMPLE_PHASE_ID, required_only=False + ) + facade_client.update_card.assert_awaited_once_with( + CARD_ID, + field_updates=[{"field_id": "status", "value": "done"}], + ) + assert result == UPDATE_CARD_RESULT + assert "skipped_field_ids" not in result + + +@pytest.mark.anyio +async def test_fill_card_phase_fields_skips_write_when_only_unknown_keys( + facade_client: PipefyClient, +): + result = await facade_client.fill_card_phase_fields( + CARD_ID, + EXAMPLE_PHASE_ID, + {"unknown": "x", "other": "y"}, + ) + + facade_client.get_phase_fields.assert_awaited_once_with( + EXAMPLE_PHASE_ID, required_only=False + ) + facade_client.update_card.assert_not_awaited() + assert result == { + "success": True, + "message": _collected_nothing_message(2), + "phase_id": EXAMPLE_PHASE_ID, + "phase_name": PHASE_NAME, + "skipped_field_ids": ["unknown", "other"], + } + + +@pytest.mark.anyio +@pytest.mark.parametrize("fields", [None, {}]) +async def test_fill_card_phase_fields_skips_write_when_fields_empty_or_none( + facade_client: PipefyClient, + fields: dict | None, +): + result = await facade_client.fill_card_phase_fields( + CARD_ID, EXAMPLE_PHASE_ID, fields + ) + + facade_client.get_phase_fields.assert_awaited_once_with( + EXAMPLE_PHASE_ID, required_only=False + ) + facade_client.update_card.assert_not_awaited() + assert result == { + "success": True, + "message": _collected_nothing_message(2), + "phase_id": EXAMPLE_PHASE_ID, + "phase_name": PHASE_NAME, + "skipped_field_ids": [], + } + + +@pytest.mark.anyio +async def test_fill_card_phase_fields_skips_write_when_no_editable_fields( + facade_client: PipefyClient, +): + facade_client.get_phase_fields.return_value = _phase_fields(NON_EDITABLE_FIELDS) + + result = await facade_client.fill_card_phase_fields( + CARD_ID, + EXAMPLE_PHASE_ID, + {"readonly": "nope", "locked": "nope"}, + ) + + facade_client.get_phase_fields.assert_awaited_once_with( + EXAMPLE_PHASE_ID, required_only=False + ) + facade_client.update_card.assert_not_awaited() + assert result == { + "success": True, + "message": ( + f"Phase '{PHASE_NAME}' has no editable fields; nothing was updated." + ), + "phase_id": EXAMPLE_PHASE_ID, + "phase_name": PHASE_NAME, + "skipped_field_ids": ["readonly", "locked"], + } + + +@pytest.mark.anyio +async def test_fill_card_phase_fields_skips_write_when_required_only_finds_none( + facade_client: PipefyClient, +): + facade_client.get_phase_fields.return_value = { + "phase_id": EXAMPLE_PHASE_ID, + "phase_name": PHASE_NAME, + "fields": [], + "message": "This phase has no required fields.", + } + + result = await facade_client.fill_card_phase_fields( + CARD_ID, + EXAMPLE_PHASE_ID, + {"status": "done"}, + required_fields_only=True, + ) + + facade_client.get_phase_fields.assert_awaited_once_with( + EXAMPLE_PHASE_ID, required_only=True + ) + facade_client.update_card.assert_not_awaited() + assert result == { + "success": True, + "message": "This phase has no required fields. Nothing was updated.", + "phase_id": EXAMPLE_PHASE_ID, + "phase_name": PHASE_NAME, + "skipped_field_ids": ["status"], + } + + +@pytest.mark.anyio +async def test_fill_card_phase_fields_skips_write_when_required_only_fields_are_not_editable( + facade_client: PipefyClient, +): + facade_client.get_phase_fields.return_value = _phase_fields( + [{"id": EXAMPLE_FIELD_INTERNAL_ID, "required": True, "editable": False}] + ) + + result = await facade_client.fill_card_phase_fields( + CARD_ID, + EXAMPLE_PHASE_ID, + {EXAMPLE_FIELD_INTERNAL_ID: "x"}, + required_fields_only=True, + ) + + facade_client.get_phase_fields.assert_awaited_once_with( + EXAMPLE_PHASE_ID, required_only=True + ) + facade_client.update_card.assert_not_awaited() + assert result == { + "success": True, + "message": ( + f"Phase '{PHASE_NAME}' has no editable fields; nothing was updated." + ), + "phase_id": EXAMPLE_PHASE_ID, + "phase_name": PHASE_NAME, + "skipped_field_ids": [EXAMPLE_FIELD_INTERNAL_ID], + } + + +@pytest.mark.anyio +async def test_fill_card_phase_fields_no_fields_to_update_when_empty_and_none_editable( + facade_client: PipefyClient, +): + facade_client.get_phase_fields.return_value = _phase_fields(NON_EDITABLE_FIELDS) + + result = await facade_client.fill_card_phase_fields(CARD_ID, EXAMPLE_PHASE_ID, {}) + + facade_client.get_phase_fields.assert_awaited_once_with( + EXAMPLE_PHASE_ID, required_only=False + ) + facade_client.update_card.assert_not_awaited() + assert result == { + "success": True, + "message": "No fields to update.", + "phase_id": EXAMPLE_PHASE_ID, + "phase_name": PHASE_NAME, + "skipped_field_ids": [], + } + + +@pytest.mark.anyio +async def test_fill_card_phase_fields_forwards_required_fields_only( + facade_client: PipefyClient, +): + await facade_client.fill_card_phase_fields( + CARD_ID, + EXAMPLE_PHASE_ID, + {"status": "done"}, + required_fields_only=True, + ) + + facade_client.get_phase_fields.assert_awaited_once_with( + EXAMPLE_PHASE_ID, required_only=True + ) + + +@pytest.mark.anyio +async def test_fill_card_phase_fields_propagates_malformed_field_definition( + facade_client: PipefyClient, +): + facade_client.get_phase_fields.side_effect = MALFORMED_PHASE_FIELDS + + with pytest.raises(MalformedFieldDefinitionError, match="return phase fields"): + await facade_client.fill_card_phase_fields( + CARD_ID, EXAMPLE_PHASE_ID, {"status": "done"} + ) + + facade_client.get_phase_fields.assert_awaited_once() + facade_client.update_card.assert_not_awaited() diff --git a/packages/sdk/tests/test_member_removal.py b/packages/sdk/tests/test_member_removal.py new file mode 100644 index 00000000..b649560a --- /dev/null +++ b/packages/sdk/tests/test_member_removal.py @@ -0,0 +1,160 @@ +"""Tests for ``pipefy_sdk.member_removal.verify_member_removal``.""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock + +import pytest +from _shared.fixture_ids import ( + EXAMPLE_FIELD_INTERNAL_ID, + EXAMPLE_FIELD_INTERNAL_ID_2, + EXAMPLE_ORG_UUID, + EXAMPLE_OTHER_ORG_UUID, + EXAMPLE_PIPE_ID, +) + +from pipefy_sdk.member_removal import verify_member_removal + +_STILL_PRESENT_WARNING = ( + "API returned success but member(s) [{ids}] are still present in the pipe. " + "They may have org-level permissions that override pipe-level removal." +) + + +def _client(*, members=None, side_effect=None): + client = MagicMock() + client.get_pipe_members = AsyncMock( + return_value={"pipe": {"members": members or []}}, + side_effect=side_effect, + ) + return client + + +@pytest.mark.anyio +async def test_verify_member_removal_returns_none_when_requested_ids_are_gone(): + client = _client( + members=[ + { + "user": { + "id": EXAMPLE_FIELD_INTERNAL_ID_2, + "uuid": EXAMPLE_OTHER_ORG_UUID, + "name": "Other", + "email": "other@x.com", + }, + "role_name": "member", + }, + ] + ) + + warning = await verify_member_removal(client, EXAMPLE_PIPE_ID, ["user-1", "user-2"]) + + assert warning is None + client.get_pipe_members.assert_awaited_once_with(EXAMPLE_PIPE_ID) + + +@pytest.mark.anyio +async def test_verify_member_removal_warns_with_sorted_ids_when_one_remains(): + client = _client( + members=[ + { + "user": { + "id": EXAMPLE_FIELD_INTERNAL_ID, + "uuid": EXAMPLE_ORG_UUID, + "name": "User", + "email": "user@x.com", + }, + "role_name": "admin", + }, + { + "user": { + "id": EXAMPLE_FIELD_INTERNAL_ID_2, + "uuid": EXAMPLE_OTHER_ORG_UUID, + "name": "Other", + "email": "other@x.com", + }, + "role_name": "member", + }, + ] + ) + + warning = await verify_member_removal( + client, EXAMPLE_PIPE_ID, [EXAMPLE_FIELD_INTERNAL_ID] + ) + + assert warning == _STILL_PRESENT_WARNING.format(ids=EXAMPLE_FIELD_INTERNAL_ID) + + +@pytest.mark.anyio +async def test_verify_member_removal_warns_when_requested_uuid_remains(): + client = _client( + members=[ + { + "user": { + "id": EXAMPLE_FIELD_INTERNAL_ID, + "uuid": EXAMPLE_ORG_UUID, + "name": "User", + "email": "user@x.com", + }, + "role_name": "admin", + }, + ] + ) + + warning = await verify_member_removal(client, EXAMPLE_PIPE_ID, [EXAMPLE_ORG_UUID]) + + assert warning == _STILL_PRESENT_WARNING.format(ids=EXAMPLE_ORG_UUID) + + +@pytest.mark.anyio +async def test_verify_member_removal_names_several_remaining_ids_sorted(): + client = _client( + members=[ + { + "user": { + "id": EXAMPLE_FIELD_INTERNAL_ID, + "uuid": EXAMPLE_ORG_UUID, + "name": "User", + "email": "user@x.com", + }, + "role_name": "admin", + }, + { + "user": { + "id": EXAMPLE_FIELD_INTERNAL_ID_2, + "uuid": EXAMPLE_OTHER_ORG_UUID, + "name": "Other", + "email": "other@x.com", + }, + "role_name": "member", + }, + ] + ) + + warning = await verify_member_removal( + client, + EXAMPLE_PIPE_ID, + [EXAMPLE_FIELD_INTERNAL_ID_2, EXAMPLE_FIELD_INTERNAL_ID], + ) + + assert warning == _STILL_PRESENT_WARNING.format( + ids=f"{EXAMPLE_FIELD_INTERNAL_ID}, {EXAMPLE_FIELD_INTERNAL_ID_2}" + ) + + +@pytest.mark.anyio +async def test_verify_member_removal_returns_none_when_pipe_id_is_not_numeric(): + client = _client() + + warning = await verify_member_removal(client, "pipe-1", ["user-1"]) + + assert warning is None + client.get_pipe_members.assert_not_awaited() + + +@pytest.mark.anyio +async def test_verify_member_removal_returns_none_when_get_pipe_members_raises(): + client = _client(side_effect=Exception("network error")) + + warning = await verify_member_removal(client, EXAMPLE_PIPE_ID, ["user-1"]) + + assert warning is None From c30214c77809bdd2465c5e4eae5630b5dc115ba3 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Date: Wed, 23 Sep 2026 18:53:26 -0300 Subject: [PATCH 2/5] feat(mcp): call the shared client methods for AI, members, and phase fill The tools keep the confirm token and elicitation. Skip and no-back-channel fills delegate before a second phase read, and a phase with nothing editable does not write. Signed-off-by: Adrianno Esnarriaga --- .../pipefy_mcp/tools/ai_automation_tools.py | 16 +- .../mcp/src/pipefy_mcp/tools/member_tools.py | 57 +-- .../mcp/src/pipefy_mcp/tools/pipe_tools.py | 125 ++++-- .../tests/tools/test_ai_automation_tools.py | 136 +++--- packages/mcp/tests/tools/test_member_tools.py | 179 ++------ packages/mcp/tests/tools/test_pipe_tools.py | 388 +++++++++++++++++- 6 files changed, 554 insertions(+), 347 deletions(-) diff --git a/packages/mcp/src/pipefy_mcp/tools/ai_automation_tools.py b/packages/mcp/src/pipefy_mcp/tools/ai_automation_tools.py index 225b6d6f..38637d7d 100644 --- a/packages/mcp/src/pipefy_mcp/tools/ai_automation_tools.py +++ b/packages/mcp/src/pipefy_mcp/tools/ai_automation_tools.py @@ -10,7 +10,6 @@ PipefyId, UpdateAiAutomationInput, ) -from pipefy_sdk.ai_preflight import filter_ai_automation_summaries from pydantic import ValidationError from pipefy_mcp.core.tool_error_envelope import tool_error_message @@ -158,7 +157,7 @@ async def get_ai_automation( if err is not None: return build_automation_error_payload(message=tool_error_message(err)) try: - raw = await client.get_automation(aid) + raw = await client.get_ai_automation(aid) except Exception as exc: # noqa: BLE001 return await handle_automation_tool_graphql_error( exc, @@ -188,8 +187,8 @@ async def get_ai_automations( ) -> dict: """List AI automations (``action_id`` = ``generate_with_ai``) for a pipe. - Delegates to ``get_automations`` with this ``pipe_id`` and optional - ``organization_id``. Results are filtered to AI prompt automations only. + Delegates to ``get_ai_automations`` with this ``pipe_id`` and optional + ``organization_id``. Results are already filtered to AI prompt automations. The API returns at most 50 rules per call, mixed action types. Filtering happens after that page, so ``pagination`` describes the mixed connection, @@ -235,9 +234,9 @@ async def get_ai_automations( return size_err cursor = after.strip() if isinstance(after, str) and after.strip() else None try: - page = await client.get_automations( + page = await client.get_ai_automations( + pid, organization_id=org, - pipe_id=pid, first=page_size, after=cursor, ) @@ -253,9 +252,8 @@ async def get_ai_automations( page_info=page["pageInfo"], page_size=page_size ) pagination["total_count"] = page["totalCount"] - filtered = filter_ai_automation_summaries(page["nodes"]) return build_automation_read_success_payload( - filtered, + page["nodes"], "AI automations listed.", pagination=pagination, ) @@ -308,7 +306,7 @@ async def delete_ai_automation( return guard try: - raw = await client.delete_automation(rid) + raw = await client.delete_ai_automation(rid) except Exception as exc: # noqa: BLE001 return await handle_automation_tool_graphql_error( exc, diff --git a/packages/mcp/src/pipefy_mcp/tools/member_tools.py b/packages/mcp/src/pipefy_mcp/tools/member_tools.py index c405aac6..11e68699 100644 --- a/packages/mcp/src/pipefy_mcp/tools/member_tools.py +++ b/packages/mcp/src/pipefy_mcp/tools/member_tools.py @@ -6,10 +6,7 @@ from mcp.server.mcpserver import Context, MCPServer from mcp.types import ToolAnnotations -from pipefy_sdk import ( - PipefyClient, - PipefyId, -) +from pipefy_sdk import PipefyId from pipefy_mcp.tools.destructive_tool_guard import check_destructive_confirmation from pipefy_mcp.tools.member_tool_helpers import ( @@ -228,7 +225,7 @@ async def remove_member_from_pipe( f"user_ids={user_ids!r}" ) try: - raw = await client.remove_members_from_pipe(pipe_id, user_ids) + result = await client.remove_member_from_pipe(pipe_id, user_ids) except ValueError as exc: return build_member_error_payload(message=str(exc)) except Exception as exc: # noqa: BLE001 @@ -244,14 +241,10 @@ async def remove_member_from_pipe( invalid_args_hint="Use 'get_pipe_members(pipe_id)' to list current members.", ) - await ctx.debug( - "remove_member_from_pipe: mutation succeeded, verifying removal" - ) - warning = await _verify_removal(client, pipe_id, user_ids) return build_member_success_payload( message="Members removed from pipe.", - data=raw, - warning=warning, + data=result["data"], + warning=result["warning"], ) @mcp.tool( @@ -299,45 +292,3 @@ async def set_role( message="Role updated.", data=raw, ) - - -async def _verify_removal( - client: PipefyClient, - pipe_id: str, - user_ids: list[str], -) -> str | None: - """Check whether removed members are actually gone from the pipe. - - Returns a warning string when any requested user IDs are still present, - or ``None`` when all were successfully removed. Silently returns - ``None`` on non-numeric ``pipe_id`` (verification requires ``int``) - or if the verification query itself fails. - """ - pipe_id_str = str(pipe_id).strip() - if not pipe_id_str.isdigit(): - return None - - try: - members_data = await client.get_pipe_members(pipe_id_str) - except Exception: # noqa: BLE001 - return None - - members = (members_data.get("pipe") or {}).get("members") or [] - remaining_ids: set[str] = set() - for m in members: - user = m.get("user") if isinstance(m.get("user"), dict) else {} - if user.get("id"): - remaining_ids.add(str(user["id"])) - if user.get("uuid"): - remaining_ids.add(str(user["uuid"])) - - requested = {str(uid) for uid in user_ids} - still_present = requested & remaining_ids - if not still_present: - return None - - ids_str = ", ".join(sorted(still_present)) - return ( - f"API returned success but member(s) [{ids_str}] are still present in the pipe. " - "They may have org-level permissions that override pipe-level removal." - ) diff --git a/packages/mcp/src/pipefy_mcp/tools/pipe_tools.py b/packages/mcp/src/pipefy_mcp/tools/pipe_tools.py index 30ce14ca..c243889c 100644 --- a/packages/mcp/src/pipefy_mcp/tools/pipe_tools.py +++ b/packages/mcp/src/pipefy_mcp/tools/pipe_tools.py @@ -22,6 +22,7 @@ from pipefy_sdk import ( filter_fields_by_definitions as _filter_fields_by_definitions, ) +from pipefy_sdk import skipped_field_ids as _skipped_field_ids from pipefy_sdk.models.form import MalformedFieldDefinitionError from pydantic import ValidationError @@ -1189,6 +1190,8 @@ async def fill_card_phase_fields( When ``skip_elicitation`` is True, field values from ``fields`` are filtered to editable phase field IDs and sent directly to the API. AI agents should set this to True when they already know the values. + Keys the phase does not expose as editable are never written; they + come back in ``skipped_field_ids``. When ``skip_elicitation`` is False (default) and the client supports elicitation, an interactive form is presented — ``fields`` pre-fills @@ -1216,9 +1219,33 @@ async def fill_card_phase_fields( ``fields`` directly to the API. Recommended for AI agent workflows. Returns: - dict: GraphQL response with success status and updated card information. + A write returns the ``update_card`` response. A no-write envelope + carries ``success``, ``message``, ``phase_id``, and ``phase_name``. + ``skipped_field_ids`` is present on every no-write envelope when + no form was shown (the list may be empty); on a write result only + when at least one key was dropped; absent after an accepted form. """ client = get_pipefy_client(ctx) + can_elicit = supports_elicitation(ctx) + if not can_elicit and not skip_elicitation: + # Logged for the same reason the NoBackChannelError absorb in + # _elicit_field_details logs: a caller asked for a form and the + # result cannot say it never appeared. + await ctx.debug( + "Elicitation unavailable: no interactive form for this " + "connection; proceeding with the supplied fields" + ) + if skip_elicitation or not can_elicit: + try: + return await client.fill_card_phase_fields( + card_id, + phase_id, + fields, + required_fields_only=required_fields_only, + ) + except MalformedFieldDefinitionError as exc: + return tool_error(str(exc)) + try: phase_fields_result = await client.get_phase_fields( phase_id, required_fields_only @@ -1228,73 +1255,87 @@ async def fill_card_phase_fields( expected_fields = _filter_editable_field_definitions( phase_fields_result.get("fields", []) ) - phase_name = phase_fields_result.get("phase_name", f"Phase {phase_id}") + phase_name = phase_fields_result.get("phase_name") or f"Phase {phase_id}" + given_fields = fields or {} await ctx.debug(f"Expected fields for phase {phase_id}: {expected_fields}") await ctx.debug(f"Provided fields: {fields}") - field_data = fields or {} - can_elicit = supports_elicitation(ctx) - if not can_elicit and not skip_elicitation: - # Logged for the same reason the NoBackChannelError absorb in - # _elicit_field_details logs: a caller asked for a form and the - # result cannot say it never appeared. - await ctx.debug( - "Elicitation unavailable: no interactive form for this " - "connection; proceeding with the supplied fields" - ) + if not expected_fields: + read_message = phase_fields_result.get("message") + if required_fields_only and read_message: + message = f"{read_message} Nothing was updated." + elif given_fields: + message = ( + f"Phase '{phase_name}' has no editable fields; " + "nothing was updated." + ) + else: + message = "No fields to update." + return { + "success": True, + "message": message, + "phase_id": phase_id, + "phase_name": phase_name, + "skipped_field_ids": _skipped_field_ids(given_fields, {}), + } + field_data = given_fields elicited: dict[str, Any] | None = None - if can_elicit and expected_fields and not skip_elicitation: - try: - elicited = await PipeTools._elicit_field_details( - message=f"Filling fields for phase '{phase_name}' (ID: {phase_id})", - prefilled_fields=fields, - expected_fields=expected_fields, - ctx=ctx, - ) - except MalformedFieldDefinitionError as exc: - return tool_error(str(exc)) - except UserCancelledError: - return tool_error("Phase field update cancelled by user.") + try: + elicited = await PipeTools._elicit_field_details( + message=f"Filling fields for phase '{phase_name}' (ID: {phase_id})", + prefilled_fields=fields, + expected_fields=expected_fields, + ctx=ctx, + ) + except MalformedFieldDefinitionError as exc: + return tool_error(str(exc)) + except UserCancelledError: + return tool_error("Phase field update cancelled by user.") + dropped: list[str] = [] if elicited is not None: field_data = elicited - elif expected_fields: + else: field_data = _filter_fields_by_definitions(field_data, expected_fields) + dropped = _skipped_field_ids(given_fields, field_data) if not field_data: - if expected_fields: - # The phase does have editable fields, so "No fields to - # update." would be false: values were needed and none were - # collected. Reachable when no form could be shown and the - # caller passed no fields, or when every key it passed was - # dropped by the editable-field filter. An agent reading only - # the message must not conclude the card is complete. - message = ( - "No field values were collected, so nothing was updated. " - f"Phase '{phase_name}' has {len(expected_fields)} editable " - "field(s); pass 'fields' keyed by the IDs from " - "get_phase_fields(phase_id)." - ) - else: - message = "No fields to update." - return { + # The phase does have editable fields, so "No fields to + # update." would be false: values were needed and none were + # collected. Reachable when the caller passed no fields, or + # when every key it passed was dropped by the editable-field + # filter. An agent reading only the message must not + # conclude the card is complete. + message = ( + "No field values were collected, so nothing was updated. " + f"Phase '{phase_name}' has {len(expected_fields)} editable " + "field(s); pass 'fields' keyed by the IDs from " + "get_phase_fields(phase_id)." + ) + no_write: dict[str, Any] = { "success": True, "message": message, "phase_id": phase_id, "phase_name": phase_name, } + if elicited is None: + no_write["skipped_field_ids"] = dropped + return no_write field_updates = [ {"field_id": field_id, "value": value} for field_id, value in field_data.items() ] - return await client.update_card( + api_response = await client.update_card( card_id=card_id, field_updates=field_updates, ) + if dropped: + return {**api_response, "skipped_field_ids": dropped} + return api_response @mcp.tool( annotations=ToolAnnotations( diff --git a/packages/mcp/tests/tools/test_ai_automation_tools.py b/packages/mcp/tests/tools/test_ai_automation_tools.py index 3859bd4c..54af0977 100644 --- a/packages/mcp/tests/tools/test_ai_automation_tools.py +++ b/packages/mcp/tests/tools/test_ai_automation_tools.py @@ -24,9 +24,9 @@ def mock_pipefy_client(): client = MagicMock(spec=PipefyClient) client.create_ai_automation = AsyncMock() client.update_ai_automation = AsyncMock() - client.get_automation = AsyncMock() - client.get_automations = AsyncMock() - client.delete_automation = AsyncMock() + client.get_ai_automation = AsyncMock() + client.get_ai_automations = AsyncMock() + client.delete_ai_automation = AsyncMock() client.get_pipe_with_preferences = AsyncMock() client.get_automation_events = AsyncMock() client.get_ai_credit_usage = AsyncMock() @@ -40,9 +40,9 @@ def mock_pipefy_client(): def mock_pipefy_client_no_ai(): """Client wired only for the read/list/delete tools (public GraphQL path).""" client = MagicMock(spec=PipefyClient) - client.get_automation = AsyncMock() - client.get_automations = AsyncMock() - client.delete_automation = AsyncMock() + client.get_ai_automation = AsyncMock() + client.get_ai_automations = AsyncMock() + client.delete_ai_automation = AsyncMock() return client @@ -90,7 +90,7 @@ async def test_success( mock_pipefy_client, extract_payload, ): - mock_pipefy_client.get_automation.return_value = { + mock_pipefy_client.get_ai_automation.return_value = { "id": "501", "name": "AI rule", "action_id": "generate_with_ai", @@ -102,10 +102,10 @@ async def test_success( {"automation_id": "501"}, ) assert result.is_error is False - mock_pipefy_client.get_automation.assert_awaited_once_with("501") + mock_pipefy_client.get_ai_automation.assert_awaited_once_with("501") payload = extract_payload(result) assert payload["success"] is True - assert payload["data"] == mock_pipefy_client.get_automation.return_value + assert payload["data"] == mock_pipefy_client.get_ai_automation.return_value assert "AI automation retrieved" in payload["message"] async def test_graphql_error( @@ -114,7 +114,7 @@ async def test_graphql_error( mock_pipefy_client, extract_payload, ): - mock_pipefy_client.get_automation.side_effect = PipefyGraphQLError( + mock_pipefy_client.get_ai_automation.side_effect = PipefyGraphQLError( [{"message": "boom"}] ) async with client_session as session: @@ -133,7 +133,7 @@ async def test_not_found_empty_data( mock_pipefy_client, extract_payload, ): - mock_pipefy_client.get_automation.return_value = {} + mock_pipefy_client.get_ai_automation.return_value = {} async with client_session as session: result = await session.call_tool( "get_ai_automation", @@ -154,7 +154,7 @@ async def test_rejects_empty_automation_id( "get_ai_automation", {"automation_id": ""}, ) - mock_pipefy_client.get_automation.assert_not_called() + mock_pipefy_client.get_ai_automation.assert_not_called() assert_invalid_arguments_envelope(result) async def test_rejects_non_positive_int_id( @@ -168,7 +168,7 @@ async def test_rejects_non_positive_int_id( "get_ai_automation", {"automation_id": -1}, ) - mock_pipefy_client.get_automation.assert_not_called() + mock_pipefy_client.get_ai_automation.assert_not_called() assert extract_payload(result)["success"] is False async def test_debug_true_includes_codes_on_graphql_error( @@ -178,7 +178,7 @@ async def test_debug_true_includes_codes_on_graphql_error( extract_payload, ): err = PipefyGraphQLError([{"message": "nope", "extensions": {"code": "GONE"}}]) - mock_pipefy_client.get_automation.side_effect = err + mock_pipefy_client.get_ai_automation.side_effect = err async with client_session as session: result = await session.call_tool( "get_ai_automation", @@ -194,7 +194,7 @@ async def test_succeeds_when_oauth_not_configured_public_query( mock_pipefy_client_no_ai, extract_payload, ): - mock_pipefy_client_no_ai.get_automation.return_value = { + mock_pipefy_client_no_ai.get_ai_automation.return_value = { "id": "1", "action_id": "generate_with_ai", } @@ -206,7 +206,7 @@ async def test_succeeds_when_oauth_not_configured_public_query( assert result.is_error is False payload = extract_payload(result) assert payload["success"] is True - mock_pipefy_client_no_ai.get_automation.assert_awaited_once_with("1") + mock_pipefy_client_no_ai.get_ai_automation.assert_awaited_once_with("1") def _automation_page(rows, *, total=None, has_next=False, end_cursor=None): @@ -225,7 +225,7 @@ async def test_filters_to_generate_with_ai_only( mock_pipefy_client, extract_payload, ): - mock_pipefy_client.get_automations.return_value = _automation_page( + mock_pipefy_client.get_ai_automations.return_value = _automation_page( [ { "id": "1", @@ -233,19 +233,14 @@ async def test_filters_to_generate_with_ai_only( "active": True, "action_id": "generate_with_ai", }, - { - "id": "2", - "name": "HTTP", - "active": True, - "action_id": "send_http_request", - }, { "id": "3", "name": "AI 2", "active": True, "action_id": "generate_with_ai", }, - ] + ], + total=3, ) async with client_session as session: result = await session.call_tool( @@ -253,9 +248,9 @@ async def test_filters_to_generate_with_ai_only( {"pipe_id": "303"}, ) assert result.is_error is False - mock_pipefy_client.get_automations.assert_awaited_once_with( + mock_pipefy_client.get_ai_automations.assert_awaited_once_with( + "303", organization_id=None, - pipe_id="303", first=50, after=None, ) @@ -280,16 +275,7 @@ async def test_filter_ignores_camel_case_action_id( ): """The list query emits snake ``action_id``; a camel ``actionId`` never occurs and is not treated as an AI automation.""" - mock_pipefy_client.get_automations.return_value = _automation_page( - [ - { - "id": "9", - "name": "AI", - "active": True, - "actionId": "generate_with_ai", - }, - ] - ) + mock_pipefy_client.get_ai_automations.return_value = _automation_page([]) async with client_session as session: result = await session.call_tool( "get_ai_automations", @@ -304,16 +290,7 @@ async def test_empty_when_none_match( mock_pipefy_client, extract_payload, ): - mock_pipefy_client.get_automations.return_value = _automation_page( - [ - { - "id": "2", - "name": "HTTP", - "active": True, - "action_id": "send_http_request", - }, - ] - ) + mock_pipefy_client.get_ai_automations.return_value = _automation_page([]) async with client_session as session: result = await session.call_tool( "get_ai_automations", @@ -329,15 +306,8 @@ async def test_truncated_mixed_listing_exposes_pagination( mock_pipefy_client, extract_payload, ): - mock_pipefy_client.get_automations.return_value = _automation_page( - [ - { - "id": "1", - "name": "HTTP", - "active": True, - "action_id": "send_http_request", - } - ], + mock_pipefy_client.get_ai_automations.return_value = _automation_page( + [], total=210, has_next=True, end_cursor="cursor-50", @@ -347,9 +317,9 @@ async def test_truncated_mixed_listing_exposes_pagination( "get_ai_automations", {"pipe_id": "303", "first": 10, "after": "cursor-40"}, ) - mock_pipefy_client.get_automations.assert_awaited_once_with( + mock_pipefy_client.get_ai_automations.assert_awaited_once_with( + "303", organization_id=None, - pipe_id="303", first=10, after="cursor-40", ) @@ -371,7 +341,7 @@ async def test_rejects_page_size_outside_api_cap( "get_ai_automations", {"pipe_id": "303", "first": 51}, ) - mock_pipefy_client.get_automations.assert_not_called() + mock_pipefy_client.get_ai_automations.assert_not_called() assert_invalid_arguments_envelope(result) async def test_passes_organization_id_when_provided( @@ -380,16 +350,16 @@ async def test_passes_organization_id_when_provided( mock_pipefy_client, extract_payload, ): - mock_pipefy_client.get_automations.return_value = _automation_page([]) + mock_pipefy_client.get_ai_automations.return_value = _automation_page([]) async with client_session as session: result = await session.call_tool( "get_ai_automations", {"pipe_id": "303", "organization_id": "9001"}, ) assert result.is_error is False - mock_pipefy_client.get_automations.assert_awaited_once_with( + mock_pipefy_client.get_ai_automations.assert_awaited_once_with( + "303", organization_id="9001", - pipe_id="303", first=50, after=None, ) @@ -401,7 +371,7 @@ async def test_graphql_error( mock_pipefy_client, extract_payload, ): - mock_pipefy_client.get_automations.side_effect = PipefyGraphQLError( + mock_pipefy_client.get_ai_automations.side_effect = PipefyGraphQLError( [{"message": "no access"}] ) async with client_session as session: @@ -423,7 +393,7 @@ async def test_rejects_invalid_pipe_id( "get_ai_automations", {"pipe_id": ""}, ) - mock_pipefy_client.get_automations.assert_not_called() + mock_pipefy_client.get_ai_automations.assert_not_called() assert_invalid_arguments_envelope(result) async def test_rejects_invalid_organization_id( @@ -436,7 +406,7 @@ async def test_rejects_invalid_organization_id( "get_ai_automations", {"pipe_id": "1", "organization_id": ""}, ) - mock_pipefy_client.get_automations.assert_not_called() + mock_pipefy_client.get_ai_automations.assert_not_called() assert_invalid_arguments_envelope(result) async def test_works_without_oauth_config( @@ -445,7 +415,7 @@ async def test_works_without_oauth_config( mock_pipefy_client_no_ai, extract_payload, ): - mock_pipefy_client_no_ai.get_automations.return_value = _automation_page( + mock_pipefy_client_no_ai.get_ai_automations.return_value = _automation_page( [ { "id": "a", @@ -469,7 +439,7 @@ async def test_org_auto_resolution_omits_organization_id( extract_payload, ): """When ``organization_id`` is omitted, the client lists with ``None`` (org resolved inside the service).""" - mock_pipefy_client.get_automations.return_value = _automation_page( + mock_pipefy_client.get_ai_automations.return_value = _automation_page( [ { "id": "1", @@ -485,9 +455,9 @@ async def test_org_auto_resolution_omits_organization_id( {"pipe_id": "42"}, ) assert extract_payload(result)["success"] is True - mock_pipefy_client.get_automations.assert_awaited_once_with( + mock_pipefy_client.get_ai_automations.assert_awaited_once_with( + "42", organization_id=None, - pipe_id="42", first=50, after=None, ) @@ -507,7 +477,7 @@ async def test_confirm_false_returns_preview( {"automation_id": "rm-1", "confirm": False}, ) assert result.is_error is False - mock_pipefy_client.delete_automation.assert_not_called() + mock_pipefy_client.delete_ai_automation.assert_not_called() p = extract_payload(result) assert p["success"] is False assert p.get("requires_confirmation") is True @@ -518,14 +488,14 @@ async def test_confirm_true_success( client_session, mock_pipefy_client, ): - mock_pipefy_client.delete_automation.return_value = {"success": True} + mock_pipefy_client.delete_ai_automation.return_value = {"success": True} async with client_session as session: payload = await confirm_after_preview( session, "delete_ai_automation", {"automation_id": "rm-1", "confirm": True}, ) - mock_pipefy_client.delete_automation.assert_awaited_once_with("rm-1") + mock_pipefy_client.delete_ai_automation.assert_awaited_once_with("rm-1") assert payload["success"] is True async def test_graphql_error( @@ -533,7 +503,7 @@ async def test_graphql_error( client_session, mock_pipefy_client, ): - mock_pipefy_client.delete_automation.side_effect = PipefyGraphQLError( + mock_pipefy_client.delete_ai_automation.side_effect = PipefyGraphQLError( [{"message": "forbidden"}] ) async with client_session as session: @@ -551,14 +521,14 @@ async def test_works_without_oauth_config( mock_pipefy_client_no_ai, ): """Delete uses public GraphQL, not the Internal API. OAuth is not required.""" - mock_pipefy_client_no_ai.delete_automation.return_value = {"success": True} + mock_pipefy_client_no_ai.delete_ai_automation.return_value = {"success": True} async with client_session_no_ai as session: payload = await confirm_after_preview( session, "delete_ai_automation", {"automation_id": "1", "confirm": True}, ) - mock_pipefy_client_no_ai.delete_automation.assert_awaited_once_with("1") + mock_pipefy_client_no_ai.delete_ai_automation.assert_awaited_once_with("1") assert payload["success"] is True async def test_api_success_false_returns_error_payload( @@ -566,7 +536,7 @@ async def test_api_success_false_returns_error_payload( client_session, mock_pipefy_client, ): - mock_pipefy_client.delete_automation.return_value = {"success": False} + mock_pipefy_client.delete_ai_automation.return_value = {"success": False} async with client_session as session: payload = await confirm_after_preview( session, @@ -586,7 +556,7 @@ async def test_rejects_invalid_automation_id( "delete_ai_automation", {"automation_id": "", "confirm": True}, ) - mock_pipefy_client.delete_automation.assert_not_called() + mock_pipefy_client.delete_ai_automation.assert_not_called() assert_invalid_arguments_envelope(result) async def test_has_destructive_hint(self, client_session): @@ -1085,7 +1055,7 @@ async def test_get_ai_automation_coerces_int_automation_id( mock_pipefy_client, extract_payload, ): - mock_pipefy_client.get_automation.return_value = {"id": "900", "name": "x"} + mock_pipefy_client.get_ai_automation.return_value = {"id": "900", "name": "x"} async with client_session as session: result = await session.call_tool( "get_ai_automation", @@ -1093,7 +1063,7 @@ async def test_get_ai_automation_coerces_int_automation_id( ) assert result.is_error is False assert extract_payload(result)["success"] is True - mock_pipefy_client.get_automation.assert_awaited_once_with("900") + mock_pipefy_client.get_ai_automation.assert_awaited_once_with("900") async def test_get_ai_automations_coerces_int_pipe_and_org_ids( self, @@ -1101,16 +1071,16 @@ async def test_get_ai_automations_coerces_int_pipe_and_org_ids( mock_pipefy_client, extract_payload, ): - mock_pipefy_client.get_automations.return_value = _automation_page([]) + mock_pipefy_client.get_ai_automations.return_value = _automation_page([]) async with client_session as session: result = await session.call_tool( "get_ai_automations", {"pipe_id": 303, "organization_id": 9001}, ) assert extract_payload(result)["success"] is True - mock_pipefy_client.get_automations.assert_awaited_once_with( + mock_pipefy_client.get_ai_automations.assert_awaited_once_with( + "303", organization_id="9001", - pipe_id="303", first=50, after=None, ) @@ -1120,7 +1090,7 @@ async def test_delete_ai_automation_coerces_int_automation_id( client_session, mock_pipefy_client, ): - mock_pipefy_client.delete_automation.return_value = {"success": True} + mock_pipefy_client.delete_ai_automation.return_value = {"success": True} async with client_session as session: payload = await confirm_after_preview( session, @@ -1128,7 +1098,7 @@ async def test_delete_ai_automation_coerces_int_automation_id( {"automation_id": 501, "confirm": True}, ) assert payload["success"] is True - mock_pipefy_client.delete_automation.assert_awaited_once_with("501") + mock_pipefy_client.delete_ai_automation.assert_awaited_once_with("501") ## --------------------------------------------------------------------------- diff --git a/packages/mcp/tests/tools/test_member_tools.py b/packages/mcp/tests/tools/test_member_tools.py index 79ea18bb..e85d0c4d 100644 --- a/packages/mcp/tests/tools/test_member_tools.py +++ b/packages/mcp/tests/tools/test_member_tools.py @@ -21,6 +21,7 @@ def mock_member_client(): client.invite_members = AsyncMock() client.add_service_account_to_pipe = AsyncMock() client.remove_members_from_pipe = AsyncMock() + client.remove_member_from_pipe = AsyncMock() client.get_pipe_members = AsyncMock() client.set_role = AsyncMock() return client @@ -381,7 +382,7 @@ async def test_invite_members_graphql_error( async def test_remove_member_from_pipe_value_error_from_client( member_session, mock_member_client ): - mock_member_client.remove_members_from_pipe.side_effect = ValueError( + mock_member_client.remove_member_from_pipe.side_effect = ValueError( "pipe_id must be a numeric pipe ID or a pipe UUID, got 'bad'." ) @@ -397,71 +398,16 @@ async def test_remove_member_from_pipe_value_error_from_client( @pytest.mark.anyio -async def test_remove_member_verified_all_removed(member_session, mock_member_client): - mock_member_client.remove_members_from_pipe.return_value = { - "removeMembersFromPipe": {"success": True} - } - mock_member_client.get_pipe_members.return_value = { - "pipe": { - "members": [ - { - "user": { - "id": "99", - "uuid": "uuid-99", - "name": "Other", - "email": "other@x.com", - }, - "role_name": "member", - }, - ] - } - } - - async with member_session as session: - payload = await confirm_after_preview( - session, - "remove_member_from_pipe", - {"pipe_id": "100", "user_ids": ["user-1", "user-2"]}, - ) - - mock_member_client.remove_members_from_pipe.assert_awaited_once_with( - "100", ["user-1", "user-2"] - ) - mock_member_client.get_pipe_members.assert_awaited_once_with("100") - assert payload["success"] is True - assert "warning" not in payload - - -@pytest.mark.anyio -async def test_remove_member_warns_when_member_still_present( +async def test_remove_member_from_pipe_surfaces_client_warning( member_session, mock_member_client ): - mock_member_client.remove_members_from_pipe.return_value = { - "removeMembersFromPipe": {"success": True} - } - mock_member_client.get_pipe_members.return_value = { - "pipe": { - "members": [ - { - "user": { - "id": "160654", - "uuid": "uuid-160654", - "name": "Rodrigo", - "email": "rodrigo@x.com", - }, - "role_name": "admin", - }, - { - "user": { - "id": "99", - "uuid": "uuid-99", - "name": "Other", - "email": "other@x.com", - }, - "role_name": "member", - }, - ] - } + warning = ( + "API returned success but member(s) 160654 are still present in the pipe. " + "They may have org-level permissions that override pipe-level removal." + ) + mock_member_client.remove_member_from_pipe.return_value = { + "data": {"removeMembersFromPipe": {"success": True}}, + "warning": warning, } async with member_session as session: @@ -471,86 +417,13 @@ async def test_remove_member_warns_when_member_still_present( {"pipe_id": "100", "user_ids": ["160654"]}, ) - assert payload["success"] is True - assert "warning" in payload - assert "160654" in payload["warning"] - assert "org-level" in payload["warning"] - - -@pytest.mark.anyio -async def test_remove_member_warns_when_uuid_still_present( - member_session, mock_member_client -): - """Verification matches user UUIDs too, not just numeric IDs.""" - mock_member_client.remove_members_from_pipe.return_value = { - "removeMembersFromPipe": {"success": True} - } - mock_member_client.get_pipe_members.return_value = { - "pipe": { - "members": [ - { - "user": { - "id": "160654", - "uuid": "abc-def-123", - "name": "Rodrigo", - "email": "rodrigo@x.com", - }, - "role_name": "admin", - }, - ] - } - } - - async with member_session as session: - payload = await confirm_after_preview( - session, - "remove_member_from_pipe", - {"pipe_id": "100", "user_ids": ["abc-def-123"]}, - ) - - assert payload["success"] is True - assert "warning" in payload - assert "abc-def-123" in payload["warning"] - - -@pytest.mark.anyio -async def test_remove_member_skips_verification_for_non_numeric_pipe_id( - member_session, mock_member_client -): - mock_member_client.remove_members_from_pipe.return_value = { - "removeMembersFromPipe": {"success": True} - } - - async with member_session as session: - payload = await confirm_after_preview( - session, - "remove_member_from_pipe", - {"pipe_id": "pipe-1", "user_ids": ["user-1"]}, - ) - + mock_member_client.remove_member_from_pipe.assert_awaited_once_with( + "100", ["160654"] + ) mock_member_client.get_pipe_members.assert_not_awaited() + mock_member_client.remove_members_from_pipe.assert_not_awaited() assert payload["success"] is True - assert "warning" not in payload - - -@pytest.mark.anyio -async def test_remove_member_returns_success_when_verification_fails( - member_session, mock_member_client -): - """If get_pipe_members raises, don't fail the whole operation.""" - mock_member_client.remove_members_from_pipe.return_value = { - "removeMembersFromPipe": {"success": True} - } - mock_member_client.get_pipe_members.side_effect = Exception("network error") - - async with member_session as session: - payload = await confirm_after_preview( - session, - "remove_member_from_pipe", - {"pipe_id": "100", "user_ids": ["user-1"]}, - ) - - assert payload["success"] is True + assert payload["warning"] == warning @pytest.mark.anyio @@ -558,10 +431,10 @@ async def test_remove_member_coerces_int_user_ids_to_str( member_session, mock_member_client ): """Agent may re-serialize user_ids as ints on the confirm call.""" - mock_member_client.remove_members_from_pipe.return_value = { - "removeMembersFromPipe": {"success": True} + mock_member_client.remove_member_from_pipe.return_value = { + "data": {"removeMembersFromPipe": {"success": True}}, + "warning": None, } - mock_member_client.get_pipe_members.return_value = {"pipe": {"members": []}} async with member_session as session: payload = await confirm_after_preview( @@ -571,7 +444,7 @@ async def test_remove_member_coerces_int_user_ids_to_str( ) assert payload["success"] is True - mock_member_client.remove_members_from_pipe.assert_awaited_once_with( + mock_member_client.remove_member_from_pipe.assert_awaited_once_with( "100", ["307516938"] ) @@ -580,7 +453,7 @@ async def test_remove_member_coerces_int_user_ids_to_str( async def test_remove_member_from_pipe_graphql_error( member_session, mock_member_client ): - mock_member_client.remove_members_from_pipe.side_effect = PipefyGraphQLError( + mock_member_client.remove_member_from_pipe.side_effect = PipefyGraphQLError( [{"message": "forbidden"}] ) @@ -660,7 +533,7 @@ async def test_remove_member_preview_does_not_call_mutation( "remove_member_from_pipe", {"pipe_id": "100", "user_ids": ["user-1"]}, # no confirm → preview ) - mock_member_client.remove_members_from_pipe.assert_not_awaited() + mock_member_client.remove_member_from_pipe.assert_not_awaited() payload = extract_payload(result) assert payload["success"] is False assert payload.get("requires_confirmation") is True @@ -672,10 +545,10 @@ async def test_remove_member_preview_does_not_call_mutation( async def test_remove_member_rejects_token_when_user_ids_differ_same_length( member_session, mock_member_client, extract_payload ): - mock_member_client.remove_members_from_pipe.return_value = { - "removeMembersFromPipe": {"success": True} + mock_member_client.remove_member_from_pipe.return_value = { + "data": {"removeMembersFromPipe": {"success": True}}, + "warning": None, } - mock_member_client.get_pipe_members.return_value = {"pipe": {"members": []}} async with member_session as session: preview = await session.call_tool( @@ -693,7 +566,7 @@ async def test_remove_member_rejects_token_when_user_ids_differ_same_length( }, ) assert extract_payload(mismatch)["requires_confirmation"] is True - mock_member_client.remove_members_from_pipe.assert_not_awaited() + mock_member_client.remove_member_from_pipe.assert_not_awaited() matched = await confirm_after_preview( session, @@ -701,7 +574,7 @@ async def test_remove_member_rejects_token_when_user_ids_differ_same_length( {"pipe_id": "100", "user_ids": ["1", "2"]}, ) - mock_member_client.remove_members_from_pipe.assert_awaited_once_with( + mock_member_client.remove_member_from_pipe.assert_awaited_once_with( "100", ["1", "2"] ) assert matched["success"] is True diff --git a/packages/mcp/tests/tools/test_pipe_tools.py b/packages/mcp/tests/tools/test_pipe_tools.py index a04a1f99..d9f36cd4 100644 --- a/packages/mcp/tests/tools/test_pipe_tools.py +++ b/packages/mcp/tests/tools/test_pipe_tools.py @@ -1,7 +1,7 @@ import json from datetime import timedelta from random import randint -from types import SimpleNamespace +from types import MethodType, SimpleNamespace from typing import Any, cast from unittest.mock import AsyncMock, MagicMock, Mock @@ -80,6 +80,9 @@ def mock_pipefy_client(): ) client.update_card = AsyncMock() client.get_pipe_members = AsyncMock() + client.fill_card_phase_fields = AsyncMock( + side_effect=MethodType(PipefyClient.fill_card_phase_fields, client) + ) return client @@ -1919,7 +1922,7 @@ async def test_without_elicitation( assert result.is_error is False, "Unexpected tool error" mock_pipefy_client.update_card.assert_called_once_with( - card_id=str(card_id), + str(card_id), field_updates=[{"field_id": "status", "value": "completed"}], ) @@ -1969,7 +1972,7 @@ async def test_without_elicitation_filters_non_editable_fields( assert result.is_error is False, "Unexpected tool error" mock_pipefy_client.update_card.assert_called_once_with( - card_id=str(card_id), + str(card_id), field_updates=[{"field_id": "status", "value": "completed"}], ) @@ -2001,6 +2004,7 @@ async def test_no_fields_returns_message( mock_pipefy_client.update_card.assert_not_called() response = extract_payload(result) assert response.get("message") == "No fields to update." + assert response.get("skipped_field_ids") == [] async def test_permission_denied( self, @@ -2027,7 +2031,7 @@ async def test_permission_denied( assert result.is_error is True, "Expected tool error for permission denied" mock_pipefy_client.get_phase_fields.assert_called_once_with( - str(phase_id), False + str(phase_id), required_only=False ) mock_pipefy_client.update_card.assert_not_called() @@ -2802,12 +2806,255 @@ async def test_fill_phase_skip_elicitation_filters_editable_fields( }, ) assert result.is_error is False - # readonly should be filtered out + mock_pipefy_client.get_phase_fields.assert_awaited_once() mock_pipefy_client.update_card.assert_called_once_with( - card_id="99", + "99", field_updates=[{"field_id": "status", "value": "done"}], ) + async def test_fill_phase_skip_path_awaits_get_phase_fields_once( + self, client_session, mock_pipefy_client, extract_payload + ): + """ADR-002: skip path awaits get_phase_fields once, inside the client.""" + mock_pipefy_client.get_phase_fields = AsyncMock( + return_value={ + "phase_id": "100", + "phase_name": "Review", + "fields": [ + { + "id": "status", + "label": "Status", + "type": "select", + "editable": True, + }, + ], + } + ) + mock_pipefy_client.update_card = AsyncMock( + return_value={"updateFieldsValues": {"success": True}} + ) + + async with client_session as session: + result = await session.call_tool( + "fill_card_phase_fields", + { + "card_id": "99", + "phase_id": "100", + "fields": {"status": "done"}, + "skip_elicitation": True, + }, + ) + + assert result.is_error is False + mock_pipefy_client.fill_card_phase_fields.assert_awaited_once() + mock_pipefy_client.get_phase_fields.assert_awaited_once() + payload = extract_payload(result) + assert "skipped_field_ids" not in payload + + async def test_fill_phase_skip_elicitation_no_editable_fields_does_not_write( + self, client_session, mock_pipefy_client, extract_payload + ): + """ADR-002: skip_elicitation on a phase with no editable fields does not write.""" + mock_pipefy_client.get_phase_fields = AsyncMock( + return_value={ + "phase_id": "100", + "phase_name": "Review", + "fields": [ + { + "id": "readonly", + "label": "RO", + "type": "short_text", + "editable": False, + }, + ], + } + ) + mock_pipefy_client.update_card = AsyncMock() + + async with client_session as session: + result = await session.call_tool( + "fill_card_phase_fields", + { + "card_id": "99", + "phase_id": "100", + "fields": {"readonly": "nope", "bogus": "x"}, + "skip_elicitation": True, + }, + ) + + assert result.is_error is False + mock_pipefy_client.fill_card_phase_fields.assert_awaited_once() + mock_pipefy_client.get_phase_fields.assert_awaited_once() + mock_pipefy_client.update_card.assert_not_called() + payload = extract_payload(result) + assert payload["skipped_field_ids"] == ["readonly", "bogus"] + + async def test_fill_phase_skip_elicitation_malformed_returns_tool_error( + self, client_session, mock_pipefy_client, extract_payload + ): + from pipefy_sdk.models.field_definition import MalformedFieldDefinitionError + + message = ( + "Cannot return phase fields: 1 field definition(s) from Pipefy are " + "missing required 'id' or 'type'. The pipe configuration may be " + "incomplete or unsupported." + ) + mock_pipefy_client.fill_card_phase_fields = AsyncMock( + side_effect=MalformedFieldDefinitionError(message) + ) + + async with client_session as session: + result = await session.call_tool( + "fill_card_phase_fields", + { + "card_id": "99", + "phase_id": "100", + "fields": {"status": "done"}, + "skip_elicitation": True, + }, + ) + + payload = extract_payload(result) + assert payload == tool_error(message) + mock_pipefy_client.fill_card_phase_fields.assert_awaited_once() + mock_pipefy_client.get_phase_fields.assert_not_called() + + @pytest.mark.parametrize( + "client_session", + [elicitation_callback_for(action="accept", content={"readonly": "nope"})], + indirect=True, + ) + async def test_fill_phase_elicit_no_editable_fields_does_not_write( + self, client_session, mock_pipefy_client, extract_payload + ): + """ADR-002: elicitation-capable session, no editable fields, does not write.""" + mock_pipefy_client.get_phase_fields = AsyncMock( + return_value={ + "phase_id": "100", + "phase_name": "Review", + "fields": [ + { + "id": "readonly", + "label": "RO", + "type": "short_text", + "editable": False, + }, + ], + } + ) + mock_pipefy_client.update_card = AsyncMock() + + async with client_session as session: + result = await session.call_tool( + "fill_card_phase_fields", + { + "card_id": "99", + "phase_id": "100", + "fields": {"readonly": "nope", "bogus": "x"}, + }, + ) + + assert result.is_error is False + mock_pipefy_client.update_card.assert_not_called() + mock_pipefy_client.fill_card_phase_fields.assert_not_awaited() + mock_pipefy_client.get_phase_fields.assert_awaited_once() + payload = extract_payload(result) + assert payload["success"] is True + assert payload["phase_id"] == "100" + assert payload["phase_name"] == "Review" + assert payload["skipped_field_ids"] == ["readonly", "bogus"] + + @pytest.mark.parametrize( + "client_session", + [elicitation_callback_for(action="accept", content={"status": "done"})], + indirect=True, + ) + async def test_fill_phase_elicit_required_only_with_no_required_fields_does_not_write( + self, client_session, mock_pipefy_client, extract_payload + ): + mock_pipefy_client.get_phase_fields = AsyncMock( + return_value={ + "phase_id": "100", + "phase_name": "Review", + "fields": [], + "message": "This phase has no required fields.", + } + ) + mock_pipefy_client.update_card = AsyncMock() + + async with client_session as session: + result = await session.call_tool( + "fill_card_phase_fields", + { + "card_id": "99", + "phase_id": "100", + "fields": {"status": "done"}, + "required_fields_only": True, + }, + ) + + assert result.is_error is False + mock_pipefy_client.fill_card_phase_fields.assert_not_awaited() + mock_pipefy_client.update_card.assert_not_called() + mock_pipefy_client.get_phase_fields.assert_awaited_once() + payload = extract_payload(result) + assert payload == { + "success": True, + "message": "This phase has no required fields. Nothing was updated.", + "phase_id": "100", + "phase_name": "Review", + "skipped_field_ids": ["status"], + } + + @pytest.mark.parametrize( + "client_session", + [elicitation_callback_for(action="accept", content={"readonly": "nope"})], + indirect=True, + ) + async def test_fill_phase_elicit_required_only_non_editable_does_not_write( + self, client_session, mock_pipefy_client, extract_payload + ): + mock_pipefy_client.get_phase_fields = AsyncMock( + return_value={ + "phase_id": "100", + "phase_name": "Review", + "fields": [ + { + "id": "readonly", + "label": "RO", + "type": "short_text", + "required": True, + "editable": False, + }, + ], + } + ) + mock_pipefy_client.update_card = AsyncMock() + + async with client_session as session: + result = await session.call_tool( + "fill_card_phase_fields", + { + "card_id": "99", + "phase_id": "100", + "fields": {"readonly": "nope"}, + "required_fields_only": True, + }, + ) + + assert result.is_error is False + mock_pipefy_client.fill_card_phase_fields.assert_not_awaited() + mock_pipefy_client.update_card.assert_not_called() + mock_pipefy_client.get_phase_fields.assert_awaited_once() + payload = extract_payload(result) + assert payload == { + "success": True, + "message": "Phase 'Review' has no editable fields; nothing was updated.", + "phase_id": "100", + "phase_name": "Review", + "skipped_field_ids": ["readonly"], + } + @pytest.mark.parametrize( "client_session", [elicitation_callback_for(action="accept", content={"status": "approved"})], @@ -2976,8 +3223,10 @@ async def test_fill_card_phase_fields_uses_supplied_fields( ) assert result.is_error is False + mock_pipefy_client.get_phase_fields.assert_awaited_once() + mock_pipefy_client.fill_card_phase_fields.assert_awaited_once() mock_pipefy_client.update_card.assert_called_once_with( - card_id="99", + "99", field_updates=[{"field_id": "f1", "value": "from-arguments"}], ) @@ -3047,11 +3296,14 @@ async def test_fill_card_phase_fields_without_fields_reports_nothing_collected( ) assert result.is_error is False + mock_pipefy_client.get_phase_fields.assert_awaited_once() + mock_pipefy_client.fill_card_phase_fields.assert_awaited_once() mock_pipefy_client.update_card.assert_not_called() payload = extract_payload(result) assert payload["message"] != "No fields to update." assert "nothing was updated" in payload["message"] assert "1 editable field(s)" in payload["message"] + assert payload["skipped_field_ids"] == [] async def test_elicit_raising_no_back_channel_is_absorbed( self, mock_pipefy_client, pipe_id @@ -3109,6 +3361,128 @@ async def test_elicit_raising_no_back_channel_is_absorbed( ) assert result["createCard"]["card"]["id"] == "14" + async def test_fill_card_phase_fields_channel_closed_includes_skipped_field_ids( + self, mock_pipefy_client + ): + """Channel closed after the phase read: filter in-memory, no second read.""" + mock_pipefy_client.get_phase_fields = AsyncMock( + return_value={ + "phase_id": "100", + "phase_name": "Review", + "fields": [ + { + "id": "f1", + "label": "F1", + "type": "short_text", + "required": False, + "editable": True, + }, + { + "id": "readonly", + "label": "RO", + "type": "short_text", + "editable": False, + }, + ], + } + ) + mock_pipefy_client.update_card = AsyncMock( + return_value={"updateFieldsValues": {"success": True}} + ) + + mcp = build_tool_test_server( + "Pipefy MCP Test Server", PipeTools.register, mock_pipefy_client + ) + runtime = McpRuntime(settings, RequestScopedIdentity()) + runtime.session_for_request = lambda _req: mock_pipefy_client + + ctx = MagicMock() + ctx.debug = AsyncMock() + ctx.elicit = AsyncMock(side_effect=NoBackChannelError("elicitation/fill")) + ctx.session = SimpleNamespace( + client_params=SimpleNamespace( + capabilities=SimpleNamespace(elicitation=True) + ), + can_send_request=True, + ) + ctx.request_context = SimpleNamespace(lifespan_context=runtime, request=None) + + result = await mcp._tool_manager.call_tool( + "fill_card_phase_fields", + { + "card_id": "99", + "phase_id": "100", + "fields": {"f1": "from-arguments", "readonly": "nope"}, + }, + context=ctx, + convert_result=False, + ) + + ctx.elicit.assert_awaited_once() + mock_pipefy_client.get_phase_fields.assert_awaited_once() + mock_pipefy_client.fill_card_phase_fields.assert_not_awaited() + mock_pipefy_client.update_card.assert_called_once_with( + card_id="99", + field_updates=[{"field_id": "f1", "value": "from-arguments"}], + ) + assert result["skipped_field_ids"] == ["readonly"] + + async def test_fill_card_phase_fields_channel_closed_no_write_keeps_skipped_ids( + self, mock_pipefy_client + ): + """Channel closed after the phase read: no-write envelope keeps skipped keys.""" + mock_pipefy_client.get_phase_fields = AsyncMock( + return_value={ + "phase_id": "100", + "phase_name": "Review", + "fields": [ + { + "id": "f1", + "label": "F1", + "type": "short_text", + "required": False, + "editable": True, + }, + ], + } + ) + mock_pipefy_client.update_card = AsyncMock() + + mcp = build_tool_test_server( + "Pipefy MCP Test Server", PipeTools.register, mock_pipefy_client + ) + runtime = McpRuntime(settings, RequestScopedIdentity()) + runtime.session_for_request = lambda _req: mock_pipefy_client + + ctx = MagicMock() + ctx.debug = AsyncMock() + ctx.elicit = AsyncMock(side_effect=NoBackChannelError("elicitation/fill")) + ctx.session = SimpleNamespace( + client_params=SimpleNamespace( + capabilities=SimpleNamespace(elicitation=True) + ), + can_send_request=True, + ) + ctx.request_context = SimpleNamespace(lifespan_context=runtime, request=None) + + result = await mcp._tool_manager.call_tool( + "fill_card_phase_fields", + { + "card_id": "99", + "phase_id": "100", + "fields": {"readonly": "nope"}, + }, + context=ctx, + convert_result=False, + ) + + ctx.elicit.assert_awaited_once() + mock_pipefy_client.get_phase_fields.assert_awaited_once() + mock_pipefy_client.fill_card_phase_fields.assert_not_awaited() + mock_pipefy_client.update_card.assert_not_called() + assert result["success"] is True + assert result["skipped_field_ids"] == ["readonly"] + # ============================================================================= # structured_output=False on comment/card mutation tools From 7370ca65d48e4ea471d39de9a4ac3770f95c7993 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Date: Wed, 23 Sep 2026 18:53:26 -0300 Subject: [PATCH 3/5] feat(cli): call the shared client methods for AI, member remove, and card fill member remove now reads members back and returns warning. card fill uses the same no-write rule as the client instead of a local short-circuit. Signed-off-by: Adrianno Esnarriaga --- .../src/pipefy_cli/commands/ai_automation.py | 12 +- packages/cli/src/pipefy_cli/commands/card.py | 34 +---- .../cli/src/pipefy_cli/commands/member.py | 10 +- .../cli/tests/fixtures/cli_help_golden.txt | 10 +- packages/cli/tests/test_card_commands.py | 144 +++++++++--------- .../tests/test_cli_agent_automation_smoke.py | 8 +- .../tests/test_relation_member_commands.py | 37 ++++- 7 files changed, 131 insertions(+), 124 deletions(-) diff --git a/packages/cli/src/pipefy_cli/commands/ai_automation.py b/packages/cli/src/pipefy_cli/commands/ai_automation.py index e819bf8a..7fd928bc 100644 --- a/packages/cli/src/pipefy_cli/commands/ai_automation.py +++ b/packages/cli/src/pipefy_cli/commands/ai_automation.py @@ -11,7 +11,6 @@ PipefyClient, UpdateAiAutomationInput, ) -from pipefy_sdk.ai_preflight import filter_ai_automation_summaries from pydantic import ValidationError from pipefy_cli.commands._common import ( @@ -99,17 +98,16 @@ def ai_automation_list( page_size = first if first is not None else AUTOMATIONS_LIST_MAX_PAGE_SIZE async def factory(client: PipefyClient): - page = await client.get_automations( + page = await client.get_ai_automations( + pipe, organization_id=organization, - pipe_id=pipe, first=first, after=cursor, ) - filtered = filter_ai_automation_summaries(page["nodes"]) info = page["pageInfo"] return { "success": True, - "data": filtered, + "data": page["nodes"], "message": "AI automations listed.", "pagination": { "has_more": bool(info.get("hasNextPage")), @@ -131,7 +129,7 @@ def ai_automation_get( """Load one automation row (``get_ai_automation`` / ``get_automation``).""" async def factory(client: PipefyClient): - row = await client.get_automation(automation_id) + row = await client.get_ai_automation(automation_id) if row is None: return { "success": False, @@ -346,6 +344,6 @@ def ai_automation_delete( ) async def factory(client: PipefyClient): - return await client.delete_automation(automation_id) + return await client.delete_ai_automation(automation_id) run_cli_command(ctx, json_out, factory) diff --git a/packages/cli/src/pipefy_cli/commands/card.py b/packages/cli/src/pipefy_cli/commands/card.py index 90974d4b..e65037d0 100644 --- a/packages/cli/src/pipefy_cli/commands/card.py +++ b/packages/cli/src/pipefy_cli/commands/card.py @@ -12,9 +12,6 @@ PipefyClient, UpdateCommentInput, copy_card_search, - filter_editable_field_definitions, - filter_fields_by_definitions, - skipped_field_ids, ) from pydantic import ValidationError @@ -408,39 +405,16 @@ def card_fill( ) -> None: """Fill phase fields on a card (non-interactive). - Filters ``--fields`` to editable phase field IDs before ``update_card``. - Stricter than MCP ``fill_card_phase_fields`` when the phase reports no - editable fields (CLI no-ops; MCP may pass values through unfiltered). + Filters ``--fields`` to editable phase field IDs. Dropped keys come back in + ``skipped_field_ids``. When nothing survives, no write is issued. """ fields = parse_json_object(fields_json, "--fields") or {} async def factory(client: PipefyClient): - if not fields: - return {"success": True, "message": "No fields to update."} - - phase_fields_result = await client.get_phase_fields(phase_id, required_only) - expected_fields = filter_editable_field_definitions( - phase_fields_result.get("fields", []) + return await client.fill_card_phase_fields( + card_id, phase_id, fields, required_fields_only=required_only ) - field_data = filter_fields_by_definitions(fields, expected_fields) - dropped = skipped_field_ids(fields, field_data) - if not field_data: - result: dict[str, Any] = { - "success": True, - "message": "No fields to update.", - } - if dropped: - result["skipped_field_ids"] = dropped - return result - field_updates = [ - {"field_id": field_id, "value": value} - for field_id, value in field_data.items() - ] - api_response = await client.update_card(card_id, field_updates=field_updates) - if dropped: - return {**api_response, "skipped_field_ids": dropped} - return api_response run_cli_command(ctx, json_out, factory) diff --git a/packages/cli/src/pipefy_cli/commands/member.py b/packages/cli/src/pipefy_cli/commands/member.py index 3970494a..3d92fbe0 100644 --- a/packages/cli/src/pipefy_cli/commands/member.py +++ b/packages/cli/src/pipefy_cli/commands/member.py @@ -116,7 +116,13 @@ def member_remove( yes: bool = typer.Option(False, "--yes", "-y", help="Skip confirmation prompt."), json_out: bool = typer.Option(False, "--json", "-j"), ) -> None: - """Remove users from a pipe.""" + """Remove users from a pipe. + + Reads members back after the mutation. Output includes ``data`` (mutation + result) and ``warning`` (null when every requested user is gone). A warning + string means a requested user is still present; org-level permissions can + override pipe-level removal. + """ ids = _parse_user_ids(user_ids) @@ -125,7 +131,7 @@ def member_remove( ) async def factory(client: PipefyClient): - return await client.remove_members_from_pipe(pipe_id, ids) + return await client.remove_member_from_pipe(pipe_id, ids) run_cli_command(ctx, json_out, factory) diff --git a/packages/cli/tests/fixtures/cli_help_golden.txt b/packages/cli/tests/fixtures/cli_help_golden.txt index b74df54c..90e8b1fa 100644 --- a/packages/cli/tests/fixtures/cli_help_golden.txt +++ b/packages/cli/tests/fixtures/cli_help_golden.txt @@ -1351,9 +1351,8 @@ Usage: pipefy card fill [OPTIONS] CARD_ID Fill phase fields on a card (non-interactive). - Filters ``--fields`` to editable phase field IDs before ``update_card``. - Stricter than MCP ``fill_card_phase_fields`` when the phase reports no - editable fields (CLI no-ops; MCP may pass values through unfiltered). + Filters ``--fields`` to editable phase field IDs. Dropped keys come back in + ``skipped_field_ids``. When nothing survives, no write is issued. ╭─ Arguments ──────────────────────────────────────────────────────────────────╮ │ * card_id TEXT Card id. [required] │ @@ -2447,6 +2446,11 @@ Usage: pipefy member remove [OPTIONS] Remove users from a pipe. + Reads members back after the mutation. Output includes ``data`` (mutation + result) and ``warning`` (null when every requested user is gone). A warning + string means a requested user is still present; org-level permissions can + override pipe-level removal. + ╭─ Options ────────────────────────────────────────────────────────────────────╮ │ * --pipe TEXT Pipe id. [required] │ │ * --user-ids TEXT Comma-separated Pipefy user ids or UUIDs to │ diff --git a/packages/cli/tests/test_card_commands.py b/packages/cli/tests/test_card_commands.py index 72d0c5e8..7483cd75 100644 --- a/packages/cli/tests/test_card_commands.py +++ b/packages/cli/tests/test_card_commands.py @@ -366,28 +366,12 @@ def test_card_fill_filters_editable_and_updates( runner, clean_pipefy_env, saved_cwd, oauth_env ): oauth_env("fill-card") - phase_fields = { - "phase_id": "100", - "phase_name": "Review", - "fields": [ - { - "id": "status", - "label": "Status", - "type": "select", - "editable": True, - }, - { - "id": "readonly", - "label": "RO", - "type": "short_text", - "editable": False, - }, - ], + payload = { + "updateFieldsValues": {"success": True}, + "skipped_field_ids": ["readonly"], } - update_resp = {"updateFieldsValues": {"success": True}} mock_client = MagicMock() - mock_client.get_phase_fields = AsyncMock(return_value=phase_fields) - mock_client.update_card = AsyncMock(return_value=update_resp) + mock_client.fill_card_phase_fields = AsyncMock(return_value=payload) with patch( "pipefy_cli.commands._common.get_authenticated_client", return_value=mock_client, @@ -407,30 +391,23 @@ def test_card_fill_filters_editable_and_updates( ], ) assert result.exit_code == 0, result.stdout + (result.stderr or "") - assert json.loads(result.stdout) == { - **update_resp, - "skipped_field_ids": ["readonly"], - } - mock_client.get_phase_fields.assert_awaited_once_with("100", True) - mock_client.update_card.assert_awaited_once_with( + assert json.loads(result.stdout) == payload + mock_client.fill_card_phase_fields.assert_awaited_once_with( "99", - field_updates=[{"field_id": "status", "value": "done"}], + "100", + {"status": "done", "readonly": "nope"}, + required_fields_only=True, ) + mock_client.update_card.assert_not_called() def test_card_fill_missing_editable_key_counts_as_editable( runner, clean_pipefy_env, saved_cwd, oauth_env ): oauth_env("fill-card-missing-editable") - update_resp = {"updateFieldsValues": {"success": True}} + payload = {"updateFieldsValues": {"success": True}} mock_client = MagicMock() - mock_client.get_phase_fields = AsyncMock( - return_value={ - "phase_id": "100", - "fields": [{"id": "status", "type": "short_text"}], - } - ) - mock_client.update_card = AsyncMock(return_value=update_resp) + mock_client.fill_card_phase_fields = AsyncMock(return_value=payload) with patch( "pipefy_cli.commands._common.get_authenticated_client", return_value=mock_client, @@ -449,11 +426,14 @@ def test_card_fill_missing_editable_key_counts_as_editable( ], ) assert result.exit_code == 0, result.stdout + (result.stderr or "") - assert json.loads(result.stdout) == update_resp - mock_client.update_card.assert_awaited_once_with( + assert json.loads(result.stdout) == payload + mock_client.fill_card_phase_fields.assert_awaited_once_with( "99", - field_updates=[{"field_id": "status", "value": "done"}], + "100", + {"status": "done"}, + required_fields_only=False, ) + mock_client.update_card.assert_not_called() def test_card_fill_invalid_fields_exit_2( @@ -479,7 +459,7 @@ def test_card_fill_invalid_fields_exit_2( ], ) assert result.exit_code == 2 - mock_client.get_phase_fields.assert_not_called() + mock_client.fill_card_phase_fields.assert_not_called() mock_client.update_card.assert_not_called() @@ -487,14 +467,19 @@ def test_card_fill_no_fields_when_input_empty( runner, clean_pipefy_env, saved_cwd, oauth_env ): oauth_env("fill-card-empty") + payload = { + "success": True, + "message": ( + "No field values were collected, so nothing was updated. " + "Phase 'Review' has 1 editable field(s); pass 'fields' keyed by the " + "IDs from get_phase_fields(phase_id)." + ), + "phase_id": "100", + "phase_name": "Review", + "skipped_field_ids": [], + } mock_client = MagicMock() - mock_client.get_phase_fields = AsyncMock( - return_value={ - "phase_id": "100", - "fields": [{"id": "status", "editable": True}], - } - ) - mock_client.update_card = AsyncMock() + mock_client.fill_card_phase_fields = AsyncMock(return_value=payload) with patch( "pipefy_cli.commands._common.get_authenticated_client", return_value=mock_client, @@ -513,11 +498,10 @@ def test_card_fill_no_fields_when_input_empty( ], ) assert result.exit_code == 0, result.stdout + (result.stderr or "") - assert json.loads(result.stdout) == { - "success": True, - "message": "No fields to update.", - } - mock_client.get_phase_fields.assert_not_called() + assert json.loads(result.stdout) == payload + mock_client.fill_card_phase_fields.assert_awaited_once_with( + "99", "100", {}, required_fields_only=False + ) mock_client.update_card.assert_not_called() @@ -525,14 +509,19 @@ def test_card_fill_typo_reports_skipped_field_ids( runner, clean_pipefy_env, saved_cwd, oauth_env ): oauth_env("fill-card-typo") + payload = { + "success": True, + "message": ( + "No field values were collected, so nothing was updated. " + "Phase 'Review' has 1 editable field(s); pass 'fields' keyed by the " + "IDs from get_phase_fields(phase_id)." + ), + "phase_id": "100", + "phase_name": "Review", + "skipped_field_ids": ["stauts"], + } mock_client = MagicMock() - mock_client.get_phase_fields = AsyncMock( - return_value={ - "phase_id": "100", - "fields": [{"id": "status", "editable": True}], - } - ) - mock_client.update_card = AsyncMock() + mock_client.fill_card_phase_fields = AsyncMock(return_value=payload) with patch( "pipefy_cli.commands._common.get_authenticated_client", return_value=mock_client, @@ -551,11 +540,13 @@ def test_card_fill_typo_reports_skipped_field_ids( ], ) assert result.exit_code == 0, result.stdout + (result.stderr or "") - assert json.loads(result.stdout) == { - "success": True, - "message": "No fields to update.", - "skipped_field_ids": ["stauts"], - } + assert json.loads(result.stdout) == payload + mock_client.fill_card_phase_fields.assert_awaited_once_with( + "99", + "100", + {"stauts": "done"}, + required_fields_only=False, + ) mock_client.update_card.assert_not_called() @@ -563,14 +554,15 @@ def test_card_fill_no_fields_when_only_non_editable( runner, clean_pipefy_env, saved_cwd, oauth_env ): oauth_env("fill-card-non-editable") + payload = { + "success": True, + "message": "Phase 'Review' has no editable fields; nothing was updated.", + "phase_id": "100", + "phase_name": "Review", + "skipped_field_ids": ["readonly"], + } mock_client = MagicMock() - mock_client.get_phase_fields = AsyncMock( - return_value={ - "phase_id": "100", - "fields": [{"id": "readonly", "editable": False}], - } - ) - mock_client.update_card = AsyncMock() + mock_client.fill_card_phase_fields = AsyncMock(return_value=payload) with patch( "pipefy_cli.commands._common.get_authenticated_client", return_value=mock_client, @@ -589,11 +581,13 @@ def test_card_fill_no_fields_when_only_non_editable( ], ) assert result.exit_code == 0, result.stdout + (result.stderr or "") - assert json.loads(result.stdout) == { - "success": True, - "message": "No fields to update.", - "skipped_field_ids": ["readonly"], - } + assert json.loads(result.stdout) == payload + mock_client.fill_card_phase_fields.assert_awaited_once_with( + "99", + "100", + {"readonly": "nope"}, + required_fields_only=False, + ) mock_client.update_card.assert_not_called() diff --git a/packages/cli/tests/test_cli_agent_automation_smoke.py b/packages/cli/tests/test_cli_agent_automation_smoke.py index 8e0c6d97..48594b0c 100644 --- a/packages/cli/tests/test_cli_agent_automation_smoke.py +++ b/packages/cli/tests/test_cli_agent_automation_smoke.py @@ -1208,13 +1208,12 @@ def test_ai_automation_list_json_includes_pagination( page = { "nodes": [ {"id": "1", "name": "AI", "action_id": "generate_with_ai"}, - {"id": "2", "name": "HTTP", "action_id": "send_http_request"}, ], "totalCount": 210, "pageInfo": {"hasNextPage": True, "endCursor": "cursor-50"}, } mock_client = MagicMock() - mock_client.get_automations = AsyncMock(return_value=page) + mock_client.get_ai_automations = AsyncMock(return_value=page) with patch( "pipefy_cli.commands._common.get_authenticated_client", return_value=mock_client, @@ -1245,9 +1244,10 @@ def test_ai_automation_list_json_includes_pagination( "page_size": 10, "total_count": 210, } - mock_client.get_automations.assert_awaited_once_with( - organization_id=None, pipe_id="9", first=10, after="cursor-40" + mock_client.get_ai_automations.assert_awaited_once_with( + "9", organization_id=None, first=10, after="cursor-40" ) + mock_client.get_automations.assert_not_called() def test_automation_list_maps_value_error_to_exit_2( diff --git a/packages/cli/tests/test_relation_member_commands.py b/packages/cli/tests/test_relation_member_commands.py index 3c63c6a2..707e63ba 100644 --- a/packages/cli/tests/test_relation_member_commands.py +++ b/packages/cli/tests/test_relation_member_commands.py @@ -157,8 +157,8 @@ def test_member_add_service_account_blank_email_exit_2( def test_member_remove_happy_path_json(runner, clean_pipefy_env, saved_cwd, oauth_env): oauth_env("mem-rm-ok") mock_client = MagicMock() - mock_client.remove_members_from_pipe = AsyncMock( - return_value={"removeMembersFromPipe": {}} + mock_client.remove_member_from_pipe = AsyncMock( + return_value={"data": {"removeMembersFromPipe": {}}, "warning": None} ) with patch( "pipefy_cli.commands._common.get_authenticated_client", @@ -178,7 +178,38 @@ def test_member_remove_happy_path_json(runner, clean_pipefy_env, saved_cwd, oaut ], ) assert result.exit_code == 0 - mock_client.remove_members_from_pipe.assert_awaited_once_with("1", ["u1", "u2"]) + payload = json.loads(result.stdout) + assert "warning" in payload + assert payload["warning"] is None + assert payload["data"] == {"removeMembersFromPipe": {}} + mock_client.remove_member_from_pipe.assert_awaited_once_with("1", ["u1", "u2"]) + + +def test_member_remove_prints_warning_when_present( + runner, clean_pipefy_env, saved_cwd, oauth_env +): + oauth_env("mem-rm-warn") + warning = ( + "API returned success but member(s) [u1] are still present in the pipe. " + "They may have org-level permissions that override pipe-level removal." + ) + mock_client = MagicMock() + mock_client.remove_member_from_pipe = AsyncMock( + return_value={"data": {"removeMembersFromPipe": {}}, "warning": warning} + ) + with patch( + "pipefy_cli.commands._common.get_authenticated_client", + return_value=mock_client, + ): + result = runner.invoke( + app, + ["member", "remove", "--pipe", "1", "--user-ids", "u1", "--yes"], + ) + assert result.exit_code == 0 + compact = " ".join(result.stdout.split()) + assert "still present in the pipe" in compact + assert "org-level permissions" in compact + mock_client.remove_member_from_pipe.assert_awaited_once_with("1", ["u1"]) def test_member_invite_members_missing_role_bad_parameter( From 13d879e1ad0bd2cab3db5b79eb88b4ba418ef12c Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Date: Wed, 23 Sep 2026 18:53:26 -0300 Subject: [PATCH 4/5] docs(skills): cite get_pipe and get_pipe_reports for labels and one report Those two MCP names are projections. SDK callers use the methods that already exist, and the MCP aliases stay. Signed-off-by: Adrianno Esnarriaga --- skills/pipes-and-cards/pipefy-pipes-and-cards/SKILL.md | 2 +- skills/reports/pipefy-reports/SKILL.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/skills/pipes-and-cards/pipefy-pipes-and-cards/SKILL.md b/skills/pipes-and-cards/pipefy-pipes-and-cards/SKILL.md index 1070051c..aa483a2f 100644 --- a/skills/pipes-and-cards/pipefy-pipes-and-cards/SKILL.md +++ b/skills/pipes-and-cards/pipefy-pipes-and-cards/SKILL.md @@ -202,7 +202,7 @@ Read `pageInfo.hasNextPage` and `pageInfo.endCursor` from the response; pass `af | Tool (MCP) | CLI | Read-only | Purpose | |------------|-----|-----------|---------| -| `get_labels` | `pipefy label list --pipe ` | Yes | List pipe labels. | +| `get_pipe` | `pipefy label list --pipe ` | Yes | Pipe labels: use `get_pipe` (`labels` in the response) via MCP, or `pipefy label list` on the CLI. | | `create_label` | `pipefy label create` | No | Create a label with a color. | | `update_label` | `pipefy label update ` | No | Rename or recolor. | | `delete_label` | `pipefy label delete ` | No | **Two-step destructive.** | diff --git a/skills/reports/pipefy-reports/SKILL.md b/skills/reports/pipefy-reports/SKILL.md index f318f308..c8263020 100644 --- a/skills/reports/pipefy-reports/SKILL.md +++ b/skills/reports/pipefy-reports/SKILL.md @@ -26,7 +26,7 @@ Pipe reports and organization reports: discovery, CRUD, and async exports. **17 | Tool (MCP) | CLI | Read-only | Purpose | |------------|-----|-----------|---------| | `get_pipe_reports` | `pipefy report-pipe list` | Yes | List all reports for a pipe. | -| `get_pipe_report` | `pipefy report-pipe get` | Yes | Single report data. | +| `get_pipe_reports` | `pipefy report-pipe get` | Yes | Single report: use `get_pipe_reports` (`report_id`) via MCP, or `pipefy report-pipe get` on the CLI. | | `get_pipe_report_columns` | `pipefy report-pipe columns` | Yes | Discover available columns for a report filter. | | `get_pipe_report_filterable_fields` | `pipefy report-pipe filterable-fields` | Yes | Discover filterable fields for a report. | | `create_pipe_report` | `pipefy report-pipe create` | No | Create a new pipe report. | From 5ab5566be68261a504a69d6b0e77413578f64596 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Date: Wed, 23 Sep 2026 18:53:26 -0300 Subject: [PATCH 5/5] docs: record the MCP-named client methods and the fill and removal contracts Parity, the SDK readme, and the changelog describe which names exist on the client and when a phase fill skips the write. Signed-off-by: Adrianno Esnarriaga --- CHANGELOG.md | 6 ++++++ docs/mcp/tools/pipes-and-cards.md | 6 +++--- docs/parity.md | 4 ++-- docs/sdk/README.md | 12 ++++++++++++ 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fba0df99..2f1b59da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,12 +10,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **SDK methods named after MCP tools**: `PipefyClient.get_ai_automation`, `get_ai_automations`, `delete_ai_automation`, `remove_member_from_pipe`, and `fill_card_phase_fields` expose those operations as client methods, with the MCP tool names and parameters. The corresponding MCP tools and CLI commands now call these methods (MCP `fill_card_phase_fields` still elicits when a form can be shown). The AI-list filter, member-removal verification, and editable-field filter move into the SDK. Skills retarget `get_labels` to `get_pipe` and `get_pipe_report` to `get_pipe_reports`; those two names stay MCP aliases (projection remains in MCP and CLI). (#696) + - **SDK pre-write validation**: `PipefyClient.validate_ai_agent_behaviors` and `PipefyClient.validate_ai_automation_prompt` expose the two read-only validators as client methods, with the MCP tool names and parameters, so an agent that builds its tools from `PipefyClient` can validate before it writes. The MCP tools and CLI commands now call these methods. The `pipefy_sdk.ai_preflight` module functions stay. (#694) ### Changed - **SDK AI agent create**: `PipefyClient.create_ai_agent` now writes the agent's instruction and behaviors. It creates the agent and chains `update_ai_agent`, as the MCP tool and CLI command did on their own; before, it dropped the required `instruction` and `behaviors` and returned an empty, disabled agent. When the update fails, it raises the new `AiAgentConfigureError`, which carries the created `agent_uuid`. `CreateAiAgentInput` and `UpdateAiAgentInput` now expand `template_params` / `instruction_template` and normalize instruction token aliases while they validate, so SDK callers get the same prep as the MCP tools. As a result, a raw behavior dict with a literal `{{name}}` and no `template_params` now fails `CreateAiAgentInput` / `UpdateAiAgentInput` validation, as it already failed in the MCP tools. Callers that expanded behaviors themselves can drop that step: a second expansion fails when a substituted value contains `{{name}}`. The MCP tools and CLI commands call these methods. A CLI `agent create` whose update fails now prints the created agent's UUID. The unused `pipefy_mcp.tools.behavior_placeholder_interpolation` re-export is removed; import the helpers from `pipefy_sdk.behavior_placeholders`. (#695) +- **MCP `fill_card_phase_fields`**: with `skip_elicitation=true` or no back channel, no longer writes when the phase has no editable fields; dropped keys return in `skipped_field_ids`. CLI `pipefy card fill --fields {}` on a phase with editable fields now returns the collected-nothing envelope instead of short-circuiting with "No fields to update." + +- **CLI `pipefy member remove`**: verifies membership after the mutation and returns `warning` (`null` when every member is gone). + ### Fixed - **Automation listings**: SDK, MCP, and CLI now return trigger IDs, event parameters, conditions, `actionEnabled`, and `disabledReason` for organization and pipe listings, avoiding a detail call per rule to audit its filters and whether the action is enabled. Listings are paged: the API caps a page at 50 rules, so `get_automations` / `pipefy automation list` accept `first` / `after` and report `totalCount` and `hasNextPage` instead of silently returning the first 50. `get_ai_automations` / `pipefy ai-automation list` expose the same page block for the mixed connection they filter. Human `pipefy automation list` prints a table of each row's scalar columns plus the page counts, leaving the nested `event_params` and `condition` to `--json`. Phase-delete preview follows every page of rules, reads their details under a concurrency bound instead of one simultaneous call per rule, and says when the dependents list is a lower bound because a page or a detail read failed. An empty pipe no longer fails `get_automation_logs_by_repo`. The last page of an audit names itself so `11 of 61` is not read as a shortfall. (#612) diff --git a/docs/mcp/tools/pipes-and-cards.md b/docs/mcp/tools/pipes-and-cards.md index 1c475b68..82d7724c 100644 --- a/docs/mcp/tools/pipes-and-cards.md +++ b/docs/mcp/tools/pipes-and-cards.md @@ -77,10 +77,10 @@ Pipefy’s GraphQL API uses **string** IDs for pipes, phases, cards, and most ot When elicitation is unavailable, `create_card` and `fill_card_phase_fields` still work but behave differently. That covers agents, CLIs, and SDK consumers, and also **the hosted server**, which serves `json_response=True` and so has no server-to-client back channel at any protocol revision: 1. The tool fetches the start-form or phase field definitions internally. -2. Provided `fields` are **filtered to editable field IDs only** — keys that do not match an editable field are silently discarded (no error). -3. The filtered dict is sent directly to the Pipefy API. +2. For `create_card`, provided `fields` are **filtered to editable field IDs only** — keys that do not match an editable field are silently discarded (no error). The filtered dict is sent directly to the Pipefy API. +3. For `fill_card_phase_fields`, keys the phase does not expose as editable are returned in `skipped_field_ids`. When nothing survives the filter, nothing is written. -Because non-editable keys are dropped without warning, agents should discover fields first and pass all required values explicitly: +Agents should discover fields first and pass all required values explicitly: ``` get_start_form_fields(pipe_id) → learn field IDs, types, required flag diff --git a/docs/parity.md b/docs/parity.md index e807e7c6..c8c6a58a 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -94,7 +94,7 @@ MCP destructive tools use a two-step `confirmation_token` (see [Destructive oper | `export_organization_report` | `pipefy report-org export` | shipped | Exports + organization reports. | | `export_pipe_audit_logs` | `pipefy audit export` | shipped | (`--pipe`); API queues export (JSON payload only). | | `export_pipe_report` | `pipefy report-pipe export` | shipped | Exports + reports; `filter` preflight validates ReportCardsFilter shape (nested `operator` + `queries`). | -| `fill_card_phase_fields` | `pipefy card fill` | shipped | (`--phase`, `--fields` JSON, optional `--required-only`). Non-interactive; filters to editable phase field IDs before `update_card`. CLI is stricter than MCP when the phase has no editable fields (no-op vs unfiltered pass-through). Response may include `skipped_field_ids` for keys dropped by the filter. | +| `fill_card_phase_fields` | `pipefy card fill` | shipped | (`--phase`, `--fields` JSON, optional `--required-only`). Non-interactive. Both surfaces filter to editable phase field IDs and skip the write when nothing survives; `skipped_field_ids` lists dropped keys. | | `find_cards` | `pipefy card find` | shipped | (`--pipe`, `--field`, `--value`). | | `find_records` | `pipefy record find` | shipped | (`--filter` JSON with `field_id` + `field_value`). Unified MCP envelope: top-level `pagination` uses `has_more` / `end_cursor` / `page_size` (same as `get_table_records`). | | `get_agents_usage` | `pipefy usage agents` | shipped | (`--organization`, `--from`, `--to`, optional `--filters` / `--search` / `--sort` JSON). | @@ -166,7 +166,7 @@ MCP destructive tools use a two-step `confirmation_token` (see [Destructive oper | `list_portals` | `pipefy portal list` | shipped | `--organization-uuid`; at most one main portal per org. | | `move_card_to_phase` | `pipefy card move` | shipped | (`--phase`). On required-field failures MCP may return `success: false` naming the field (and an optional hide hint); CLI still returns the raw SDK / GraphQL error (known MCP-ahead behavior). | | `publish_sub_portal` | `pipefy portal sub-portal publish` | shipped | internal_api `updateSubPortalElement` on a templated `forms` element; check `subPortals[].published` via `get_portal`. | -| `remove_member_from_pipe` | `pipefy member remove` | shipped | MCP two-step with `confirmation_token`; CLI `--yes` or interactive prompt. | +| `remove_member_from_pipe` | `pipefy member remove` | shipped | MCP two-step with `confirmation_token`; CLI `--yes` or interactive prompt. Both surfaces read the members back and return `warning` when a user is still present. | | `reset_default_llm_provider` | `pipefy ai-provider default reset` | shipped | Organization-scoped; clears the org default (`--org-id`). | | `search_pipes` | `pipefy pipe list` | shipped | (`--name`, `--max-per-org`). | | `search_schema` | `pipefy introspect schema search` | shipped | (optional `--kind`). | diff --git a/docs/sdk/README.md b/docs/sdk/README.md index 3542b411..79fc4529 100644 --- a/docs/sdk/README.md +++ b/docs/sdk/README.md @@ -69,6 +69,18 @@ Two read-only `PipefyClient` methods dry-run a write before you make it. They ha Both return a dict. `valid` is true only when `problems` is empty, and `warnings` holds non-blocking notices. The agent result adds a `message`; the prompt result adds a `field_map` of the referenced field IDs to their labels. A failed pipe read sets `success` to false instead of raising: the agent result then gives the reason in `problems`, and the prompt result carries only `success`, `valid`, and `error`. +## Methods named after the MCP tools + +Five `PipefyClient` methods share names and parameters with MCP tools so an agent that builds its tools from the client can call the same operations. MCP and CLI call these methods; MCP `fill_card_phase_fields` still owns elicitation when a form can be shown and the caller did not skip it. + +- **`get_ai_automation(automation_id)`** delegates to `get_automation`. Returns the rule record, or `None` when the id is missing. +- **`get_ai_automations(pipe_id, organization_id=None, *, first=None, after=None)`** calls `get_automations`, then keeps only `generate_with_ai` rows in `nodes` (`pipefy_sdk.ai_preflight.filter_ai_automation_summaries`). `totalCount` and `pageInfo` still describe the mixed page. +- **`delete_ai_automation(automation_id)`** delegates to `delete_automation`. Same result as that method. +- **`remove_member_from_pipe(pipe_id, user_ids)`** runs the mutation, then reads members back. Returns `{"data": , "warning": }`. `warning` is `None` when every requested user is gone, when `pipe_id` is not numeric, or when the members read fails. Use the singular when the caller wants confirmation that the removal took effect. **`remove_members_from_pipe`** fetches the pipe before mutating and may fetch members when resolving numeric user ids; it skips post-mutation verification. The plural is not deprecated. +- **`fill_card_phase_fields(card_id, phase_id, fields, *, required_fields_only=False)`** reads `get_phase_fields` once, filters `fields` to editable ids, and does not write a field the phase does not expose. When nothing survives: `success`, `message`, `phase_id`, `phase_name`, `skipped_field_ids`. When a write runs: the `update_card` dict, plus `skipped_field_ids` only when a key was dropped. + +Skills cite `get_pipe` (labels are on the pipe) and `get_pipe_reports` (the reports connection). There is no `PipefyClient.get_labels` or `PipefyClient.get_pipe_report`; MCP keeps those names as aliases, and projection stays in MCP and CLI. + ## Configuration OAuth and endpoint variables are documented in **[`../config.md`](../config.md)** and **[`../../.env.example`](../../.env.example)**. Integration tests use `@pytest.mark.integration` and the same `PIPEFY_*` keys from local **`.env`** (e.g. `PIPEFY_PORTAL_ORG_UUID` for portal live tests). Unit tests use fictional ids in **[`../../packages/sdk/tests/_shared/fixture_ids.py`](../../packages/sdk/tests/_shared/fixture_ids.py)** — not production org UUIDs.