feat: improve workflow agent tool interation

tests/agents/test_node_agent.py

@@ -1,79 +1,28 @@
 
+from contextlib import contextmanager
+from unittest.mock import MagicMock
+
 import pytest
 
 
-@pytest.mark.unit
-class TestToolFilterMixin:
+@pytest.fixture()
+def _no_db(monkeypatch):
+    """Scoped-id resolution opens a read conn; synthetic ids never use it."""
 
-    def test_get_user_tools_filters_by_allowed_ids(self):
-        from application.agents.workflows.node_agent import ToolFilterMixin
+    @contextmanager
+    def fake_readonly():
+        yield MagicMock()
 
-        class FakeBase:
-            def _get_user_tools(self, user="local"):
-                return {
-                    "t1": {"_id": "id1", "name": "tool1"},
-                    "t2": {"_id": "id2", "name": "tool2"},
-                    "t3": {"_id": "id3", "name": "tool3"},
-                }
+    monkeypatch.setattr("application.agents.tool_executor.db_readonly", fake_readonly)
 
-        class TestClass(ToolFilterMixin, FakeBase):
-            pass
 
-        obj = TestClass()
-        obj._allowed_tool_ids = ["id1", "id3"]
-        result = obj._get_user_tools("user1")
-        assert "t1" in result
-        assert "t3" in result
-        assert "t2" not in result
+class _FakeExecutorBase:
+    """Stands in for BaseAgent: provides the tool_executor the mixin scopes."""
 
-    def test_get_user_tools_returns_empty_when_no_allowed(self):
-        from application.agents.workflows.node_agent import ToolFilterMixin
+    def __init__(self, *args, **kwargs):
+        from application.agents.tool_executor import ToolExecutor
 
-        class FakeBase:
-            def _get_user_tools(self, user="local"):
-                return {"t1": {"_id": "id1"}}
-
-        class TestClass(ToolFilterMixin, FakeBase):
-            pass
-
-        obj = TestClass()
-        obj._allowed_tool_ids = []
-        result = obj._get_user_tools()
-        assert result == {}
-
-    def test_get_tools_filters_by_allowed_ids(self):
-        from application.agents.workflows.node_agent import ToolFilterMixin
-
-        class FakeBase:
-            def _get_tools(self, api_key=None):
-                return {
-                    "t1": {"_id": "id1"},
-                    "t2": {"_id": "id2"},
-                }
-
-        class TestClass(ToolFilterMixin, FakeBase):
-            pass
-
-        obj = TestClass()
-        obj._allowed_tool_ids = ["id2"]
-        result = obj._get_tools("key")
-        assert "t2" in result
-        assert "t1" not in result
-
-    def test_get_tools_returns_empty_when_no_allowed(self):
-        from application.agents.workflows.node_agent import ToolFilterMixin
-
-        class FakeBase:
-            def _get_tools(self, api_key=None):
-                return {"t1": {"_id": "id1"}}
-
-        class TestClass(ToolFilterMixin, FakeBase):
-            pass
-
-        obj = TestClass()
-        obj._allowed_tool_ids = []
-        result = obj._get_tools()
-        assert result == {}
+        self.tool_executor = ToolExecutor()
 
 
 @pytest.mark.unit
@@ -92,49 +41,53 @@ class TestWorkflowNodeAgentFactory:
             )
 
 
-# =====================================================================
-# Coverage gap tests  (lines 52-59: _WorkflowNodeMixin.__init__)
-# =====================================================================
-
-
 @pytest.mark.unit
 class TestWorkflowNodeMixinInit:
+    """The mixin scopes the EXECUTOR: agents fetch tools via tool_executor.get_tools(),
+    so per-node tool filtering must live there (the old agent-method mixin was dead code)."""
 
-    def test_mixin_init_sets_allowed_tool_ids(self):
-        """Cover lines 52-59: _WorkflowNodeMixin.__init__ stores tool_ids."""
+    def _mixed(self, **kwargs):
         from application.agents.workflows.node_agent import _WorkflowNodeMixin
 
-        class FakeBase:
-            def __init__(self, *args, **kwargs):
-                pass
-
-        class TestMixin(_WorkflowNodeMixin, FakeBase):
+        class TestMixin(_WorkflowNodeMixin, _FakeExecutorBase):
             pass
 
-        obj = TestMixin(
+        return TestMixin(
             endpoint="http://example.com",
             llm_name="openai",
             model_id="gpt-4",
             api_key="key",
-            tool_ids=["tool1", "tool2"],
+            **kwargs,
         )
-        assert obj._allowed_tool_ids == ["tool1", "tool2"]
 
-    def test_mixin_init_defaults_empty_tool_ids(self):
-        """Cover: _WorkflowNodeMixin defaults to empty list."""
-        from application.agents.workflows.node_agent import _WorkflowNodeMixin
+    def test_mixin_scopes_executor_to_tool_ids(self):
+        obj = self._mixed(tool_ids=["tool1", "tool2"])
+        assert obj.tool_executor.allowed_tool_ids == ["tool1", "tool2"]
 
-        class FakeBase:
-            def __init__(self, *args, **kwargs):
-                pass
+    def test_mixin_defaults_to_empty_scope_not_unscoped(self):
+        # No tool_ids means the node gets NO tools — never the user's full set.
+        obj = self._mixed()
+        assert obj.tool_executor.allowed_tool_ids == []
 
-        class TestMixin(_WorkflowNodeMixin, FakeBase):
-            pass
+    def test_empty_scope_yields_no_tools(self):
+        obj = self._mixed()
+        assert obj.tool_executor.get_tools() == {}
 
-        obj = TestMixin(
-            endpoint="http://example.com",
-            llm_name="openai",
-            model_id="gpt-4",
-            api_key="key",
+    def test_scoped_executor_resolves_builtin_synthetic_ids(self, _no_db):
+        """A node whose Tools picker selected a builtin (Artifact / Read Document)
+        must get that tool at runtime — the P0 this design replaced."""
+        from application.agents.default_tools import default_tool_id
+
+        obj = self._mixed(
+            tool_ids=[
+                default_tool_id("artifact_generator"),
+                default_tool_id("read_document"),
+            ]
         )
-        assert obj._allowed_tool_ids == []
+        tools = obj.tool_executor.get_tools()
+        names = {t["name"] for t in tools.values()}
+        assert names == {"artifact_generator", "read_document"}
+
+    def test_scoped_executor_drops_unresolvable_ids(self, _no_db):
+        obj = self._mixed(tool_ids=["not-a-real-tool-id"])
+        assert obj.tool_executor.get_tools() == {}

tests/agents/test_tool_executor.py

@@ -66,6 +66,33 @@ class TestToolExecutorGetTools:
         assert str(tool["id"]) in tools
         assert tools[str(tool["id"])]["id"] == tool["id"]
 
+    def test_api_key_agent_never_resolves_workflow_only_builtins(self, pg_conn, monkeypatch):
+        """read_document is workflow-only: a chat/scheduled agent carrying its
+        builtin id must not get it; workflow nodes get it via allowed_tool_ids."""
+        from application.agents.default_tools import default_tool_id
+        from application.storage.db.repositories.agents import AgentsRepository
+
+        read_doc_id = default_tool_id("read_document")
+        artifact_id = default_tool_id("artifact_generator")
+        AgentsRepository(pg_conn).create(
+            user_id="alice",
+            name="a",
+            status="active",
+            key="wf_only_key",
+            tools=[read_doc_id, artifact_id],
+        )
+        self._patch_conn(monkeypatch, pg_conn)
+
+        chat_tools = ToolExecutor(user_api_key="wf_only_key", user="alice").get_tools()
+        chat_names = {t["name"] for t in chat_tools.values()}
+        assert "artifact_generator" in chat_names
+        assert "read_document" not in chat_names
+
+        scoped = ToolExecutor(user="alice")
+        scoped.allowed_tool_ids = [read_doc_id]
+        node_names = {t["name"] for t in scoped.get_tools().values()}
+        assert node_names == {"read_document"}
+
     def test_agentless_chat_synthesizes_defaults(self, pg_conn, monkeypatch):
         from application.agents.default_tools import loaded_default_tools
         from application.storage.db.repositories.user_tools import UserToolsRepository

tests/agents/tools/test_artifact_generator_unit.py

@@ -405,3 +405,60 @@ def test_create_and_rewrite_metadata_embed_spec_synopsis():
     rewrite_spec = actions["rewrite_artifact"]["parameters"]["properties"]["spec"]
     assert _SPEC_SYNOPSIS in create_spec["description"]
     assert _SPEC_SYNOPSIS in rewrite_spec["description"]
+
+
+# ---------------------------------------------------------------------------
+# spec_append: additive edits that don't clobber arrays
+# ---------------------------------------------------------------------------
+
+
+def test_spec_append_preserves_existing_items():
+    from application.agents.tools.artifact_generator import _apply_spec_append
+
+    spec = {"title": "Brief", "blocks": [{"type": "heading", "text": "Overview"}]}
+    out = _apply_spec_append(spec, {"blocks": [{"type": "heading", "text": "Risks"}]})
+    assert "error" not in out
+    assert [b["text"] for b in out["spec"]["blocks"]] == ["Overview", "Risks"]
+    # The input spec is not mutated.
+    assert len(spec["blocks"]) == 1
+
+
+def test_spec_append_creates_missing_list():
+    from application.agents.tools.artifact_generator import _apply_spec_append
+
+    out = _apply_spec_append({"title": "x"}, {"blocks": [{"type": "paragraph", "text": "p"}]})
+    assert out["spec"]["blocks"] == [{"type": "paragraph", "text": "p"}]
+
+
+def test_spec_append_rejects_non_list_values_and_targets():
+    from application.agents.tools.artifact_generator import _apply_spec_append
+
+    assert "error" in _apply_spec_append({}, {"blocks": "not-a-list"})
+    assert "error" in _apply_spec_append({"title": "t"}, {"title": ["x"]})
+
+
+def test_edit_with_only_spec_append_appends_to_loaded_spec(monkeypatch):
+    tool = _tool()
+    loaded_spec = {"blocks": [{"type": "heading", "text": "Overview"}]}
+    monkeypatch.setattr(
+        tool,
+        "_load_current",
+        lambda raw_id: {"artifact_id": "a-1", "kind": "html", "spec": loaded_spec, "title": "T"},
+    )
+    captured = {}
+
+    def fake_reversion(artifact_id, kind, spec, action, title=None):
+        captured["spec"] = spec
+        return {"status": "ok", "artifact_id": artifact_id}
+
+    monkeypatch.setattr(tool, "_reversion", fake_reversion)
+
+    result = tool._edit(id="A1", spec_append={"blocks": [{"type": "paragraph", "text": "Risks…"}]})
+    assert result["status"] == "ok"
+    assert [b["text"] for b in captured["spec"]["blocks"]] == ["Overview", "Risks…"]
+
+
+def test_edit_requires_patch_or_append():
+    err = _tool()._edit(id="A1")
+    assert err["status"] == "error"
+    assert "spec_patch and/or spec_append" in err["error"]