From e4f917a8c64a220b23e5276b9eea64fb324fb2c0 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Fri, 31 Jul 2026 15:52:01 -0700 Subject: [PATCH] [eric] workflows: enforce a frozen Actions tool set at launch, it was computed and then discarded --- backend/apps/agents/core/models.py | 3 +- backend/apps/agents/manager/AgentLaunch.py | 17 ++++- backend/apps/workflows/executor.py | 5 +- backend/apps/workflows/workflows.py | 11 +-- backend/tests/test_frozen_workflow_tools.py | 77 +++++++++++++++++++++ 5 files changed, 103 insertions(+), 10 deletions(-) create mode 100644 backend/tests/test_frozen_workflow_tools.py diff --git a/backend/apps/agents/core/models.py b/backend/apps/agents/core/models.py index bc93cdff..41964275 100644 --- a/backend/apps/agents/core/models.py +++ b/backend/apps/agents/core/models.py @@ -9,7 +9,8 @@ class AgentConfig(BaseModel): mode: str = "agent" provider: str = "anthropic" system_prompt: Optional[str] = None - allowed_tools: list[str] = Field(default_factory=lambda: ["Read", "Edit", "Write", "Bash", "Glob", "Grep", "AskUserQuestion"]) + # None means "whatever the mode allows". A list is an actual restriction and IS enforced at launch, so it must stay None unless the caller really means to narrow the surface. + allowed_tools: Optional[list[str]] = None max_turns: Optional[int] = None target_directory: Optional[str] = None dashboard_id: Optional[str] = None diff --git a/backend/apps/agents/manager/AgentLaunch.py b/backend/apps/agents/manager/AgentLaunch.py index abf3a806..179110ae 100644 --- a/backend/apps/agents/manager/AgentLaunch.py +++ b/backend/apps/agents/manager/AgentLaunch.py @@ -29,6 +29,21 @@ from backend.apps.agents.manager.prompt.prompt_context import resolve_mode logger = logging.getLogger(__name__) +@typechecked +def resolve_launch_tools(mode_tools: List[str], allowed: Optional[List[str]]) -> List[str]: + """The tool surface a session actually launches with. + + None means "whatever the mode allows". A list is a real restriction: a workflow whose Actions + set the user froze. This used to be computed and then discarded, so the Actions page sold a + boundary the runtime never enforced. Intersecting rather than trusting means a stale saved set + can only ever narrow the mode, never widen it. + """ + if allowed is None: + return mode_tools + permitted = set(allowed) + return [t for t in mode_tools if t in permitted] + + from backend.apps.agents.manager.AgentManagerProtocol import AgentManagerProtocol @@ -50,7 +65,7 @@ class AgentLaunch(AgentManagerProtocol): config.target_directory = bound mode_tools, _, mode_folder = resolve_mode(config.mode, get_all_tool_names) - tools = mode_tools + tools = resolve_launch_tools(mode_tools, config.allowed_tools) global_settings = load_settings() effective_cwd = ( diff --git a/backend/apps/workflows/executor.py b/backend/apps/workflows/executor.py index 35cbb5f9..e8db1e48 100644 --- a/backend/apps/workflows/executor.py +++ b/backend/apps/workflows/executor.py @@ -273,9 +273,8 @@ async def execute( mode=wf.mode or "agent", provider=wf.provider or "anthropic", system_prompt=_resolve_system_prompt(wf), - allowed_tools=resolved_allowed_tools if resolved_allowed_tools is not None else [ - "Read", "Edit", "Write", "Bash", "Glob", "Grep", "AskUserQuestion", - ], + # None when the user has not frozen the Actions set, which means the workflow runs with the mode's full surface exactly like a chat does. + allowed_tools=resolved_allowed_tools, dashboard_id=resolve_workflow_dashboard_id(wf), workflow_run_id=run.id, ) diff --git a/backend/apps/workflows/workflows.py b/backend/apps/workflows/workflows.py index 49a4d563..0da67e17 100644 --- a/backend/apps/workflows/workflows.py +++ b/backend/apps/workflows/workflows.py @@ -1022,7 +1022,8 @@ async def edit_agent_session(workflow_id: str): mode=wf.mode or "agent", provider=wf.provider or "anthropic", system_prompt=system_prompt, - allowed_tools=[], + # None, not [], since an empty list is now a real restriction and would leave this chat with no tools at all. + allowed_tools=None, dashboard_id=edit_dashboard_id, workflow_edit_id=wf.id, ) @@ -1261,9 +1262,8 @@ async def test_run_workflow(workflow_id: str, body: dict): mode=wf.mode or "agent", provider=wf.provider or "anthropic", system_prompt=executor._resolve_system_prompt(wf), - allowed_tools=resolved_allowed_tools if resolved_allowed_tools is not None else [ - "Read", "Edit", "Write", "Bash", "Glob", "Grep", "AskUserQuestion", - ], + # A test run has to hit the same tool surface the scheduled run will, or the test proves nothing. + allowed_tools=resolved_allowed_tools, dashboard_id=test_dashboard_id, ) session = await agent_manager.launch_agent(config) @@ -1400,7 +1400,8 @@ async def schedule_agent_session(workflow_id: str): mode=wf.mode or "agent", provider=wf.provider or "anthropic", system_prompt=system_prompt, - allowed_tools=[], + # None, not [], since an empty list is now a real restriction and would leave this chat with no tools at all. + allowed_tools=None, dashboard_id=wf.dashboard_id, ) session = await agent_manager.launch_agent(config) diff --git a/backend/tests/test_frozen_workflow_tools.py b/backend/tests/test_frozen_workflow_tools.py new file mode 100644 index 00000000..db6cde75 --- /dev/null +++ b/backend/tests/test_frozen_workflow_tools.py @@ -0,0 +1,77 @@ +"""A workflow's frozen Actions set has to actually restrict the run. + +It did not. `AgentConfig.allowed_tools` was computed by the executor, passed to launch, and then +dropped on the floor: launch resolved tools purely from the mode. So the Actions page offered a +permission toggle that no dispatch code read, which is worse than offering nothing, because it +sells a boundary that is not there. An unattended 3am run with Bash and nobody to deny an approval +is exactly the case the toggle exists for. + +None still means "whatever the mode allows", so an unfrozen workflow keeps the full surface. + +Run: + cd backend && .venv/bin/python -m pytest tests/test_frozen_workflow_tools.py -v +""" + +from __future__ import annotations + +from typing import List + +from backend.apps.agents.core.models import AgentConfig +from backend.apps.agents.manager.AgentLaunch import resolve_launch_tools + + +def p_resolve(config: AgentConfig, mode_tools: List[str]) -> List[str]: + """Drives the REAL launch-time resolver, not a copy of it. A mirrored implementation here would + keep passing even if launch went back to ignoring the frozen set entirely.""" + return resolve_launch_tools(mode_tools, config.allowed_tools) + + +P_MODE = ["Read", "Edit", "Write", "Bash", "Glob", "Grep", "WebSearch", "AskUserQuestion"] + + +def test_default_is_unrestricted(): + """Every normal chat posts a config with no allowed_tools. If the default were a list instead of + None, honouring it would silently strip the whole app down to that list.""" + assert AgentConfig(name="chat").allowed_tools is None + + +def test_unfrozen_workflow_keeps_the_full_mode_surface(): + assert p_resolve(AgentConfig(name="wf"), P_MODE) == P_MODE + + +def test_frozen_set_actually_removes_tools(): + config = AgentConfig(name="wf", allowed_tools=["Read", "Glob"]) + resolved = p_resolve(config, P_MODE) + assert resolved == ["Read", "Glob"] + assert "Bash" not in resolved, "the whole point: a frozen set must be able to withhold Bash" + assert "Write" not in resolved + + +def test_a_frozen_set_cannot_widen_the_mode(): + """Intersect, never trust. A stale saved set naming a tool the mode does not grant must not + smuggle it back in.""" + config = AgentConfig(name="wf", allowed_tools=["Read", "NotebookEdit", "BrowserClick"]) + resolved = p_resolve(config, ["Read", "Bash"]) + assert resolved == ["Read"] + + +def test_an_explicitly_empty_set_grants_nothing(): + """Distinct from None on purpose. This is why the workflow edit and scheduling chats had to move + off [] and onto None: under the old dead code [] was harmless, now it means zero tools.""" + assert p_resolve(AgentConfig(name="wf", allowed_tools=[]), P_MODE) == [] + + +def test_order_follows_the_mode_not_the_saved_set(): + config = AgentConfig(name="wf", allowed_tools=["Grep", "Read"]) + assert p_resolve(config, P_MODE) == ["Read", "Grep"] + + +def test_workflow_call_sites_do_not_pass_an_empty_list(): + """Guards the migration: an [] left behind at any AgentConfig site is now a silently toolless + agent, which reads to the user as the agent being broken.""" + import re + from pathlib import Path + root = Path(__file__).resolve().parents[1] + for rel in ("apps/workflows/workflows.py", "apps/workflows/executor.py"): + src = (root / rel).read_text() + assert not re.search(r"allowed_tools=\[\]", src), f"{rel} still passes an empty allowed_tools"