From 6dd5cf472aaa8587421bca308467b5cd41d93cbe Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 9 Jul 2026 10:22:44 +0100 Subject: [PATCH] feat: improve workflow agent tool interation --- AGENTS.md | 12 +- application/agents/tool_executor.py | 35 ++++- .../agents/tools/artifact_generator.py | 53 ++++++- application/agents/workflows/node_agent.py | 43 ++---- application/celeryconfig.py | 10 ++ .../src/agents/workflow/WorkflowBuilder.tsx | 74 +-------- .../src/agents/workflow/simpleCel.test.ts | 86 +++++++++++ frontend/src/agents/workflow/simpleCel.ts | 81 ++++++++++ tests/agents/test_node_agent.py | 143 ++++++------------ tests/agents/test_tool_executor.py | 27 ++++ .../tools/test_artifact_generator_unit.py | 57 +++++++ tests/api/user/test_tasks.py | 13 ++ 12 files changed, 420 insertions(+), 214 deletions(-) create mode 100644 frontend/src/agents/workflow/simpleCel.test.ts create mode 100644 frontend/src/agents/workflow/simpleCel.ts diff --git a/AGENTS.md b/AGENTS.md index 959e2b8b..082e0209 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,18 +57,20 @@ full flag set. Run the Celery worker in a separate terminal (if needed): ```bash -celery -A application.app.celery worker -l INFO -Q docsgpt,parsing +celery -A application.app.celery worker -l INFO ``` On macOS, prefer the solo pool for Celery: ```bash -python -m celery -A application.app.celery worker -l INFO --pool=solo -Q docsgpt,parsing +python -m celery -A application.app.celery worker -l INFO --pool=solo ``` -The `parsing` queue serves document parsing (the `read_document` tool / workflow -native-file parse); without it those calls hang `DOCUMENT_PARSE_TIMEOUT` then -error. A dedicated `-Q parsing` worker can be GPU-enabled for heavier parsers. +A bare worker (no `-Q`) consumes every configured queue, so one worker does the +whole job — app tasks and document parsing (the `read_document` tool / workflow +native-file parse) alike. Use `-Q` only to split load: run the main worker with +`-Q docsgpt` and a dedicated (e.g. GPU-enabled) parser worker with `-Q parsing` +for heavy OCR. ### Frontend diff --git a/application/agents/tool_executor.py b/application/agents/tool_executor.py index 34d6c58d..a7d7b2ea 100644 --- a/application/agents/tool_executor.py +++ b/application/agents/tool_executor.py @@ -210,6 +210,10 @@ class ToolExecutor: self.tool_allowlist: set = {str(x) for x in tool_allowlist} if tool_allowlist else set() self.tool_calls: List[Dict] = [] self._loaded_tools: Dict[str, object] = {} + # Explicit tool-id scope (workflow agent nodes): when set (even empty), + # get_tools() resolves EXACTLY these ids — builtin synthetic ids and + # user_tools rows alike — with no defaults mixed in. None = unscoped. + self.allowed_tool_ids: Optional[List[str]] = None self.conversation_id: Optional[str] = None # Set by the workflow engine for agent nodes so run-scoped tools # (artifact_generator / code_executor) address artifacts by the @@ -232,7 +236,9 @@ class ToolExecutor: If *client_tools* have been set on this executor, they are automatically merged into the returned dict. """ - if self.user_api_key: + if self.allowed_tool_ids is not None: + tools = self._get_tools_by_ids(self.allowed_tool_ids) + elif self.user_api_key: tools = self._get_tools_by_api_key(self.user_api_key) else: tools = self._get_user_tools(self.user or "local") @@ -250,6 +256,29 @@ class ToolExecutor: """ return {str(tool["name"]) for tool in self.get_tools().values() if isinstance(tool, dict) and tool.get("name")} + def _get_tools_by_ids(self, tool_ids: List[str]) -> Dict[str, Dict]: + """Resolve an explicit tool-id scope — exactly these ids, no defaults. + + Used by workflow agent nodes: the node's configured tools (builtin + synthetic ids like Artifact/Code Executor/Read Document, or the user's + ``user_tools`` rows) are the node's WHOLE toolset. An unresolvable id + is dropped with a warning rather than failing the node. + """ + if not tool_ids: + return {} + with db_readonly() as conn: + tools_repo = UserToolsRepository(conn) + tools: List[Dict] = [] + for tid in tool_ids: + row = resolve_tool_by_id(tid, self.user, user_tools_repo=tools_repo) + if row is None: + logger.warning("tool id %s did not resolve; dropped from scoped toolset", tid) + continue + if self.headless and is_headless_excluded_tool(row.get("name")): + continue + tools.append(row) + return {str(tool["id"]): tool for tool in tools} + def _get_tools_by_api_key(self, api_key: str) -> Dict[str, Dict]: """Resolve an agent's toolset — exactly ``agents.tools``, no defaults.""" # Per-operation session: the answer pipeline spans a long-lived @@ -265,6 +294,10 @@ class ToolExecutor: row = resolve_tool_by_id(tid, owner, user_tools_repo=tools_repo) if row is None: continue + # Workflow-only builtins (read_document) never resolve for a + # chat/scheduled agent — nodes get them via the scoped-id path. + if row.get("workflow_only"): + continue # Headless runs (scheduled / webhook) drop chat-only tools # like ``scheduler`` so a fire-time LLM can't chain schedules. if self.headless and is_headless_excluded_tool(row.get("name")): diff --git a/application/agents/tools/artifact_generator.py b/application/agents/tools/artifact_generator.py index 8678a43c..7bee07f3 100644 --- a/application/agents/tools/artifact_generator.py +++ b/application/agents/tools/artifact_generator.py @@ -401,6 +401,26 @@ def merge_patch(target: Any, patch: Any) -> Any: return result +def _apply_spec_append(spec: Dict[str, Any], spec_append: Dict[str, Any]) -> Dict[str, Any]: + """Concatenate items onto the spec's top-level lists (blocks/sections/slides/sheets). + + RFC 7386 merge-patch replaces arrays wholesale, so "add a section" via + spec_patch silently wipes the existing ones unless the model resends the + full array. spec_append is the safe additive path: each key must name a + list (absent counts as empty) and its items are appended in order. + Returns {"spec": merged} or {"error": message}. + """ + result = copy.deepcopy(spec) + for key, items in spec_append.items(): + if not isinstance(items, list): + return {"error": f"spec_append[{key!r}] must be a list of items to append."} + current = result.get(key, []) + if not isinstance(current, list): + return {"error": f"spec_append target {key!r} is not a list in the current spec."} + result[key] = current + copy.deepcopy(items) + return {"spec": result} + + class ArtifactGeneratorTool(Tool): """Artifact Create, edit, and version documents - slides, docs, sheets, PDF, HTML. @@ -450,8 +470,10 @@ class ArtifactGeneratorTool(Tool): { "name": "edit_artifact", "description": ( - "Apply a JSON merge-patch (RFC 7386) to the current spec, re-render, and append a " - "new version. Preferred for small, targeted changes." + "Apply a JSON merge-patch (RFC 7386) and/or append items to the current spec, " + "re-render, and append a new version. Preferred for small, targeted changes. " + "CAUTION: an array in spec_patch REPLACES the whole existing array — to add " + "slides/sections/blocks/sheets while keeping the existing ones, use spec_append." ), "active": True, "parameters": { @@ -464,10 +486,17 @@ class ArtifactGeneratorTool(Tool): }, "spec_patch": { "type": "object", - "description": "RFC 7386 merge-patch; null values delete keys.", + "description": "RFC 7386 merge-patch; null values delete keys; arrays " + "replace the existing array wholesale.", + }, + "spec_append": { + "type": "object", + "description": "Additive edit: {key: [items]} appends items to the " + 'spec\'s top-level list, e.g. {"blocks": [{"type": "heading", "text": ' + '"Risks"}]} keeps existing blocks and adds these after them.', }, }, - "required": ["id", "spec_patch"], + "required": ["id"], }, }, { @@ -559,14 +588,24 @@ class ArtifactGeneratorTool(Tool): return {"status": "ok", **ref} def _edit(self, **kwargs: Any) -> Dict[str, Any]: - """Merge-patch the current spec, re-render, and append a version.""" + """Merge-patch and/or list-append the current spec, re-render, and append a version.""" spec_patch = kwargs.get("spec_patch") - if not isinstance(spec_patch, dict): + spec_append = kwargs.get("spec_append") + if spec_patch is None and spec_append is None: + return {"status": "error", "error": "edit_artifact needs spec_patch and/or spec_append."} + if spec_patch is not None and not isinstance(spec_patch, dict): return {"status": "error", "error": "spec_patch must be a JSON object (merge-patch)."} + if spec_append is not None and not isinstance(spec_append, dict): + return {"status": "error", "error": "spec_append must be a JSON object of {key: [items]}."} loaded = self._load_current(kwargs.get("id")) if loaded.get("error"): return {"status": "error", "error": loaded["error"]} - new_spec = merge_patch(loaded["spec"], spec_patch) + new_spec = merge_patch(loaded["spec"], spec_patch) if spec_patch else dict(loaded["spec"] or {}) + if spec_append: + appended = _apply_spec_append(new_spec, spec_append) + if "error" in appended: + return {"status": "error", "error": appended["error"]} + new_spec = appended["spec"] return self._reversion( loaded["artifact_id"], loaded["kind"], new_spec, "edit_artifact", loaded.get("title") ) diff --git a/application/agents/workflows/node_agent.py b/application/agents/workflows/node_agent.py index 67437e45..d0f61362 100644 --- a/application/agents/workflows/node_agent.py +++ b/application/agents/workflows/node_agent.py @@ -1,6 +1,6 @@ """Workflow Node Agents - defines specialized agents for workflow nodes.""" -from typing import Any, Dict, List, Optional, Type +from typing import Dict, List, Optional, Type from application.agents.agentic_agent import AgenticAgent from application.agents.base import BaseAgent @@ -9,34 +9,6 @@ from application.agents.research_agent import ResearchAgent from application.agents.workflows.schemas import AgentType -class ToolFilterMixin: - """Mixin that filters fetched tools to only those specified in tool_ids.""" - - _allowed_tool_ids: List[str] - - def _get_user_tools(self, user: str = "local") -> Dict[str, Dict[str, Any]]: - all_tools = super()._get_user_tools(user) - if not self._allowed_tool_ids: - return {} - filtered_tools = { - tool_id: tool - for tool_id, tool in all_tools.items() - if str(tool.get("_id", "")) in self._allowed_tool_ids - } - return filtered_tools - - def _get_tools(self, api_key: str = None) -> Dict[str, Dict[str, Any]]: - all_tools = super()._get_tools(api_key) - if not self._allowed_tool_ids: - return {} - filtered_tools = { - tool_id: tool - for tool_id, tool in all_tools.items() - if str(tool.get("_id", "")) in self._allowed_tool_ids - } - return filtered_tools - - class _WorkflowNodeMixin: """Common __init__ for all workflow node agents.""" @@ -56,18 +28,23 @@ class _WorkflowNodeMixin: api_key=api_key, **kwargs, ) - self._allowed_tool_ids = tool_ids or [] + # Scope the executor to exactly the node's configured tools. Agents + # fetch their toolset via ``tool_executor.get_tools()``, so the scope + # must live on the executor — it resolves builtin synthetic ids + # (Artifact / Code Executor / Read Document) and ``user_tools`` rows + # alike, and an empty list means the node's LLM gets no tools. + self.tool_executor.allowed_tool_ids = [str(t) for t in (tool_ids or [])] -class WorkflowNodeClassicAgent(ToolFilterMixin, _WorkflowNodeMixin, ClassicAgent): +class WorkflowNodeClassicAgent(_WorkflowNodeMixin, ClassicAgent): pass -class WorkflowNodeAgenticAgent(ToolFilterMixin, _WorkflowNodeMixin, AgenticAgent): +class WorkflowNodeAgenticAgent(_WorkflowNodeMixin, AgenticAgent): pass -class WorkflowNodeResearchAgent(ToolFilterMixin, _WorkflowNodeMixin, ResearchAgent): +class WorkflowNodeResearchAgent(_WorkflowNodeMixin, ResearchAgent): pass diff --git a/application/celeryconfig.py b/application/celeryconfig.py index e3e9942c..fd696402 100644 --- a/application/celeryconfig.py +++ b/application/celeryconfig.py @@ -1,3 +1,5 @@ +from kombu import Queue + from application.core.settings import settings # Pydantic loads .env into ``settings`` but does not inject values into @@ -27,6 +29,14 @@ task_routes = { "application.api.user.tasks.parse_document": {"queue": settings.DOCUMENT_PARSE_QUEUE}, } +# Declare every queue so a bare ``celery worker`` (no -Q) consumes ALL of them — +# the default worker does the whole job, parsing included. Operators who want +# heavy OCR isolated run one worker with ``-Q docsgpt`` and another with +# ``-Q parsing``. (dict.fromkeys dedupes if DOCUMENT_PARSE_QUEUE == "docsgpt".) +task_queues = tuple( + Queue(name) for name in dict.fromkeys(["docsgpt", settings.DOCUMENT_PARSE_QUEUE]) +) + beat_scheduler = "redbeat.RedBeatScheduler" redbeat_redis_url = broker_url redbeat_key_prefix = "redbeat:docsgpt:" diff --git a/frontend/src/agents/workflow/WorkflowBuilder.tsx b/frontend/src/agents/workflow/WorkflowBuilder.tsx index e982ea63..83bd5c8a 100644 --- a/frontend/src/agents/workflow/WorkflowBuilder.tsx +++ b/frontend/src/agents/workflow/WorkflowBuilder.tsx @@ -76,6 +76,7 @@ import { validateCodeJsonSchema, } from './codeNodeConfig'; import MobileBlocker from './components/MobileBlocker'; +import { buildSimpleCel, parseSimpleCel } from './simpleCel'; import NodeDocumentsControl from './components/NodeDocumentsControl'; import PromptTextArea, { extractUpstreamVariables, @@ -181,79 +182,6 @@ function canReachEnd( .some((e) => canReachEnd(e.target, edges, nodeIds, endIds, visited)); } -function parseSimpleCel(expression: string): { - variable: string; - operator: string; - value: string; -} { - const trimmedExpression = expression.trim(); - - let match = trimmedExpression.match( - /^(\w+)\.(contains|startsWith)\(["'](.*)["']\)$/, - ); - if (match) return { variable: match[1], operator: match[2], value: match[3] }; - - match = trimmedExpression.match(/^(\w+)\.(contains|startsWith)\((.*)\)$/); - if (match) { - const rawValue = match[3].trim(); - const unquotedValue = rawValue.replace(/^["'](.*)["']$/, '$1'); - return { - variable: match[1], - operator: match[2], - value: unquotedValue, - }; - } - - match = trimmedExpression.match(/^(contains|startsWith)\(["'](.*)["']\)$/); - if (match) return { variable: '', operator: match[1], value: match[2] }; - - match = trimmedExpression.match(/^(contains|startsWith)\((.*)\)$/); - if (match) { - const rawValue = match[2].trim(); - const unquotedValue = rawValue.replace(/^["'](.*)["']$/, '$1'); - return { variable: '', operator: match[1], value: unquotedValue }; - } - - match = trimmedExpression.match(/^(\w+)\s*(==|!=|>=|<=|>|<)\s*["'](.*)["']$/); - if (match) return { variable: match[1], operator: match[2], value: match[3] }; - - match = trimmedExpression.match(/^(==|!=|>=|<=|>|<)\s*["'](.*)["']$/); - if (match) return { variable: '', operator: match[1], value: match[2] }; - - match = trimmedExpression.match(/^(\w+)\s*(==|!=|>=|<=|>|<)\s*(.*)$/); - if (match) return { variable: match[1], operator: match[2], value: match[3] }; - - match = trimmedExpression.match(/^(==|!=|>=|<=|>|<)\s*(.*)$/); - if (match) return { variable: '', operator: match[1], value: match[2] }; - - return { variable: '', operator: '==', value: '' }; -} - -function buildSimpleCel( - variable: string, - operator: string, - value: string, -): string { - const trimmedValue = value.trim(); - const isNumeric = trimmedValue !== '' && !isNaN(Number(trimmedValue)); - const isBool = trimmedValue === 'true' || trimmedValue === 'false'; - const literalValue = - isNumeric || isBool ? trimmedValue : JSON.stringify(value); - const stringValue = JSON.stringify(value); - if (operator === 'contains') { - return variable - ? `${variable}.contains(${stringValue})` - : `contains(${stringValue})`; - } - if (operator === 'startsWith') { - return variable - ? `${variable}.startsWith(${stringValue})` - : `startsWith(${stringValue})`; - } - if (!variable) return `${operator} ${literalValue}`; - return `${variable} ${operator} ${literalValue}`; -} - function normalizeConditionCases(cases: ConditionCase[]): ConditionCase[] { const usedHandles = new Set(); let nextIndex = 0; diff --git a/frontend/src/agents/workflow/simpleCel.test.ts b/frontend/src/agents/workflow/simpleCel.test.ts new file mode 100644 index 00000000..b600e99c --- /dev/null +++ b/frontend/src/agents/workflow/simpleCel.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from 'vitest'; + +import { buildSimpleCel, parseSimpleCel } from './simpleCel'; + +describe('parseSimpleCel', () => { + it('parses a plain variable comparison', () => { + expect(parseSimpleCel('status == "open"')).toEqual({ + variable: 'status', + operator: '==', + value: 'open', + }); + }); + + it('parses a dotted variable path the backend CEL accepts', () => { + // Regression: Simple mode blocked `audit.high_risk_accounts >= 3` at + // Preview ("must specify a variable") while createWorkflow accepted it. + expect(parseSimpleCel('audit.high_risk_accounts >= 3')).toEqual({ + variable: 'audit.high_risk_accounts', + operator: '>=', + value: '3', + }); + }); + + it('parses dotted paths with contains/startsWith', () => { + expect(parseSimpleCel('audit.summary.contains("risk")')).toEqual({ + variable: 'audit.summary', + operator: 'contains', + value: 'risk', + }); + expect(parseSimpleCel('doc.name.startsWith("Q3")')).toEqual({ + variable: 'doc.name', + operator: 'startsWith', + value: 'Q3', + }); + }); + + it('parses quoted comparisons on dotted paths', () => { + expect(parseSimpleCel('meta.region == "EMEA"')).toEqual({ + variable: 'meta.region', + operator: '==', + value: 'EMEA', + }); + }); + + it('keeps variable-less forms working', () => { + expect(parseSimpleCel('>= 4')).toEqual({ + variable: '', + operator: '>=', + value: '4', + }); + expect(parseSimpleCel('contains("x")')).toEqual({ + variable: '', + operator: 'contains', + value: 'x', + }); + }); + + it('falls back to an empty template for unparseable input', () => { + expect(parseSimpleCel('!!not cel!!')).toEqual({ + variable: '', + operator: '==', + value: '', + }); + }); +}); + +describe('buildSimpleCel round-trip', () => { + it('round-trips a dotted numeric comparison', () => { + const built = buildSimpleCel('audit.high_risk_accounts', '>=', '3'); + expect(built).toBe('audit.high_risk_accounts >= 3'); + expect(parseSimpleCel(built)).toEqual({ + variable: 'audit.high_risk_accounts', + operator: '>=', + value: '3', + }); + }); + + it('round-trips a dotted contains', () => { + const built = buildSimpleCel('audit.summary', 'contains', 'risk'); + expect(parseSimpleCel(built)).toEqual({ + variable: 'audit.summary', + operator: 'contains', + value: 'risk', + }); + }); +}); diff --git a/frontend/src/agents/workflow/simpleCel.ts b/frontend/src/agents/workflow/simpleCel.ts new file mode 100644 index 00000000..caf77c20 --- /dev/null +++ b/frontend/src/agents/workflow/simpleCel.ts @@ -0,0 +1,81 @@ +/** + * Parse/build the Simple-mode condition expressions. The variable side accepts + * dotted paths (`audit.high_risk_accounts`) because the engine's CEL evaluator + * resolves them — Simple mode rejecting what the backend happily runs left + * saved workflows blocked at Preview with "must specify a variable". + */ + +export function parseSimpleCel(expression: string): { + variable: string; + operator: string; + value: string; +} { + const trimmedExpression = expression.trim(); + + let match = trimmedExpression.match( + /^([\w.]+)\.(contains|startsWith)\(["'](.*)["']\)$/, + ); + if (match) return { variable: match[1], operator: match[2], value: match[3] }; + + match = trimmedExpression.match(/^([\w.]+)\.(contains|startsWith)\((.*)\)$/); + if (match) { + const rawValue = match[3].trim(); + const unquotedValue = rawValue.replace(/^["'](.*)["']$/, '$1'); + return { + variable: match[1], + operator: match[2], + value: unquotedValue, + }; + } + + match = trimmedExpression.match(/^(contains|startsWith)\(["'](.*)["']\)$/); + if (match) return { variable: '', operator: match[1], value: match[2] }; + + match = trimmedExpression.match(/^(contains|startsWith)\((.*)\)$/); + if (match) { + const rawValue = match[2].trim(); + const unquotedValue = rawValue.replace(/^["'](.*)["']$/, '$1'); + return { variable: '', operator: match[1], value: unquotedValue }; + } + + match = trimmedExpression.match( + /^([\w.]+)\s*(==|!=|>=|<=|>|<)\s*["'](.*)["']$/, + ); + if (match) return { variable: match[1], operator: match[2], value: match[3] }; + + match = trimmedExpression.match(/^(==|!=|>=|<=|>|<)\s*["'](.*)["']$/); + if (match) return { variable: '', operator: match[1], value: match[2] }; + + match = trimmedExpression.match(/^([\w.]+)\s*(==|!=|>=|<=|>|<)\s*(.*)$/); + if (match) return { variable: match[1], operator: match[2], value: match[3] }; + + match = trimmedExpression.match(/^(==|!=|>=|<=|>|<)\s*(.*)$/); + if (match) return { variable: '', operator: match[1], value: match[2] }; + + return { variable: '', operator: '==', value: '' }; +} + +export function buildSimpleCel( + variable: string, + operator: string, + value: string, +): string { + const trimmedValue = value.trim(); + const isNumeric = trimmedValue !== '' && !isNaN(Number(trimmedValue)); + const isBool = trimmedValue === 'true' || trimmedValue === 'false'; + const literalValue = + isNumeric || isBool ? trimmedValue : JSON.stringify(value); + const stringValue = JSON.stringify(value); + if (operator === 'contains') { + return variable + ? `${variable}.contains(${stringValue})` + : `contains(${stringValue})`; + } + if (operator === 'startsWith') { + return variable + ? `${variable}.startsWith(${stringValue})` + : `startsWith(${stringValue})`; + } + if (!variable) return `${operator} ${literalValue}`; + return `${variable} ${operator} ${literalValue}`; +} diff --git a/tests/agents/test_node_agent.py b/tests/agents/test_node_agent.py index 7a4c6012..d85e67b4 100644 --- a/tests/agents/test_node_agent.py +++ b/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() == {} diff --git a/tests/agents/test_tool_executor.py b/tests/agents/test_tool_executor.py index 237c7562..d26f9327 100644 --- a/tests/agents/test_tool_executor.py +++ b/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 diff --git a/tests/agents/tools/test_artifact_generator_unit.py b/tests/agents/tools/test_artifact_generator_unit.py index cdd73938..223bdd90 100644 --- a/tests/agents/tools/test_artifact_generator_unit.py +++ b/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"] diff --git a/tests/api/user/test_tasks.py b/tests/api/user/test_tasks.py index 809d5a04..4a6544ce 100644 --- a/tests/api/user/test_tasks.py +++ b/tests/api/user/test_tasks.py @@ -791,3 +791,16 @@ class TestIngestPoisonEvent: ) assert published[0][0][2]["operation"] == "reingest" + + +@pytest.mark.unit +def test_bare_worker_consumes_app_and_parsing_queues(): + """task_queues declares every queue, so a worker started without -Q serves + both app tasks and document parsing — a -Q-less dev worker must never + silently strand attachment uploads or parse_document tasks.""" + import application.celeryconfig as celeryconfig + from application.core.settings import settings + + names = {queue.name for queue in celeryconfig.task_queues} + assert "docsgpt" in names + assert settings.DOCUMENT_PARSE_QUEUE in names