From e1352bcdc0ca0015b7ac1fd7293bdaa1ae9a5fb1 Mon Sep 17 00:00:00 2001 From: JasonH Date: Tue, 22 Sep 2026 11:36:28 +0800 Subject: [PATCH] fix(agents): distinguish undeclared and empty skill allowed-tools (#5669) * fix(agents): distinguish undeclared and empty skill allowed-tools * fix(ci): reduce guidance and isolate PostgreSQL test mocks --------- Co-authored-by: Willem Jiang --- .../harness/deerflow/agents/AGENTS.md | 23 +++++++++---------- .../deerflow/agents/assembly_descriptor.py | 3 ++- .../tests/test_agent_assembly_descriptor.py | 14 ++++++++++- ...test_persistence_engine_postgres_config.py | 17 +++++++------- 4 files changed, 35 insertions(+), 22 deletions(-) diff --git a/backend/packages/harness/deerflow/agents/AGENTS.md b/backend/packages/harness/deerflow/agents/AGENTS.md index 67cd74f0c..a5fda61a2 100644 --- a/backend/packages/harness/deerflow/agents/AGENTS.md +++ b/backend/packages/harness/deerflow/agents/AGENTS.md @@ -1,18 +1,17 @@ ### Agent System **Lead Agent** (`packages/harness/deerflow/agents/lead_agent/agent.py`): -- Entry point: `make_lead_agent(config: RunnableConfig)` registered in `langgraph.json`. - Its signature and bare-graph return type are a published ABI: LangGraph Server calls it - directly, so neither may change. -- `assemble_lead_agent(config, *, app_config=None) -> LeadAgentAssembly(graph, descriptor)` - is the richer entry point the Gateway uses; `make_lead_agent` is a thin wrapper returning - `.graph`. The descriptor is built by - `deerflow/agents/assembly_descriptor.py::build_assembly_descriptor()` and captures what - only the factory knows — the model resolved after runtime overrides, the rendered prompt - hash, the tool list left by authorization, and the composed middleware stack in order. - Consumers of a factory result must unwrap `.graph` defensively (see - `runtime/runs/worker.py::_agent_graph`), because a third-party factory still returns a - bare graph. +- `make_lead_agent(config: RunnableConfig)` is the published `langgraph.json` + entry point; preserve its signature and bare-graph return type. +- Gateway calls `assemble_lead_agent(config, *, app_config=None)` for a + `LeadAgentAssembly(graph, descriptor)`; `make_lead_agent` returns `.graph`. + `assembly_descriptor.py::build_assembly_descriptor()` records the model after + runtime overrides, rendered prompt hash, authorized tools and middleware order. + Consumers must unwrap via `runtime/runs/worker.py::_agent_graph` to also accept + third-party factories returning bare graphs. + The hashed skill catalog preserves `allowed_tools=None` (legacy allow-all), + `()` (explicit allow-none for business tools), and non-empty allowlists as + distinct policy states; allowlist ordering does not affect the fingerprint. - Dynamic model selection via `create_chat_model()` with thinking/vision support - Tools loaded via `get_available_tools()` - combines sandbox, built-in, MCP, community, and subagent tools - System prompt generated by `apply_prompt_template()` with skills, memory, and subagent instructions diff --git a/backend/packages/harness/deerflow/agents/assembly_descriptor.py b/backend/packages/harness/deerflow/agents/assembly_descriptor.py index e26406e73..df9063eed 100644 --- a/backend/packages/harness/deerflow/agents/assembly_descriptor.py +++ b/backend/packages/harness/deerflow/agents/assembly_descriptor.py @@ -467,7 +467,8 @@ def build_assembly_descriptor( { "name": str(getattr(skill, "name", "")), "description": str(getattr(skill, "description", "")), - "allowed_tools": sorted(str(item) for item in (getattr(skill, "allowed_tools", None) or ())), + # None preserves legacy allow-all; an empty declaration allows no business tools. + "allowed_tools": None if (allowed_tools := getattr(skill, "allowed_tools", None)) is None else sorted(str(item) for item in allowed_tools), "content_hash": _skill_content_hash(skill), "secrets_autonomous": bool(getattr(skill, "secrets_autonomous", True)), "required_secrets": sorted(f"{getattr(requirement, 'name', '')}:{bool(getattr(requirement, 'optional', False))}" for requirement in (getattr(skill, "required_secrets", None) or ())), diff --git a/backend/tests/test_agent_assembly_descriptor.py b/backend/tests/test_agent_assembly_descriptor.py index 94e148a29..0c350d466 100644 --- a/backend/tests/test_agent_assembly_descriptor.py +++ b/backend/tests/test_agent_assembly_descriptor.py @@ -662,7 +662,7 @@ class TestSkillCatalogHashesContent: allowed-tools are untouched.""" @staticmethod - def _skill(skill_dir: Path, *, required_secrets=(), secrets_autonomous=True): + def _skill(skill_dir: Path, *, allowed_tools=None, required_secrets=(), secrets_autonomous=True): from deerflow.skills.types import Skill, SkillCategory skill_file = skill_dir / "SKILL.md" @@ -674,6 +674,7 @@ class TestSkillCatalogHashesContent: skill_file=skill_file, relative_path=Path(skill_dir.name), category=SkillCategory.CUSTOM, + allowed_tools=allowed_tools, required_secrets=required_secrets, secrets_autonomous=secrets_autonomous, ) @@ -719,6 +720,17 @@ class TestSkillCatalogHashesContent: with_secret = self._skill(Path("/nonexistent/skill-b"), required_secrets=(SecretRequirement(name="API_KEY"),)) assert self._build([no_secrets]).fingerprint != self._build([with_secret]).fingerprint + def test_allowed_tools_declaration_states_have_distinct_fingerprints(self, tmp_path): + descriptors = [self._build([self._skill(tmp_path, allowed_tools=allowed_tools)]) for allowed_tools in (None, (), ("bash",))] + + assert len({descriptor.fingerprint for descriptor in descriptors}) == 3 + + def test_allowed_tools_order_does_not_change_the_fingerprint(self, tmp_path): + before = self._build([self._skill(tmp_path, allowed_tools=("bash", "read_file"))]) + reordered = self._build([self._skill(tmp_path, allowed_tools=("read_file", "bash"))]) + + assert before.fingerprint == reordered.fingerprint + def test_a_missing_skill_file_is_undescribable_not_fatal(self, tmp_path): skill = self._skill(tmp_path / "missing-skill") descriptor = self._build([skill]) diff --git a/backend/tests/test_persistence_engine_postgres_config.py b/backend/tests/test_persistence_engine_postgres_config.py index e3c9cf39f..81bca3c57 100644 --- a/backend/tests/test_persistence_engine_postgres_config.py +++ b/backend/tests/test_persistence_engine_postgres_config.py @@ -55,7 +55,7 @@ def test_postgres_engine_kwargs_allow_command_timeout_opt_out() -> None: @pytest.mark.asyncio -async def test_configured_command_timeout_ends_stalled_command() -> None: +async def test_configured_command_timeout_ends_stalled_command(monkeypatch) -> None: config = DatabaseConfig( backend="postgres", postgres_url="postgresql://user:password@localhost/deerflow", @@ -79,8 +79,9 @@ async def test_configured_command_timeout_ends_stalled_command() -> None: bootstrap_schema = AsyncMock() + # Patch one key: patch.dict restores all of sys.modules and races with background imports. + monkeypatch.setitem(sys.modules, "asyncpg", ModuleType("asyncpg")) with ( - patch.dict(sys.modules, {"asyncpg": ModuleType("asyncpg")}), patch.object(engine_mod, "create_async_engine", side_effect=_create_engine), patch.object(engine_mod, "async_sessionmaker", return_value=MagicMock()), patch("deerflow.persistence.bootstrap.bootstrap_schema", new=bootstrap_schema), @@ -101,7 +102,7 @@ async def test_configured_command_timeout_ends_stalled_command() -> None: @pytest.mark.asyncio -async def test_init_engine_from_config_preserves_longer_command_timeout_override() -> None: +async def test_init_engine_from_config_preserves_longer_command_timeout_override(monkeypatch) -> None: config = DatabaseConfig( backend="postgres", postgres_url="postgresql://user:password@localhost/deerflow", @@ -112,8 +113,8 @@ async def test_init_engine_from_config_preserves_longer_command_timeout_override mock_engine.dispose = AsyncMock() bootstrap_schema = AsyncMock() + monkeypatch.setitem(sys.modules, "asyncpg", ModuleType("asyncpg")) with ( - patch.dict(sys.modules, {"asyncpg": ModuleType("asyncpg")}), patch.object(engine_mod, "create_async_engine", return_value=mock_engine) as create_engine, patch.object(engine_mod, "async_sessionmaker", return_value=MagicMock()), patch("deerflow.persistence.bootstrap.bootstrap_schema", new=bootstrap_schema), @@ -129,14 +130,14 @@ async def test_init_engine_from_config_preserves_longer_command_timeout_override @pytest.mark.asyncio -async def test_init_engine_postgres_uses_hardened_kwargs() -> None: +async def test_init_engine_postgres_uses_hardened_kwargs(monkeypatch) -> None: url = "postgresql+asyncpg://user:password@localhost/deerflow" mock_engine = MagicMock() mock_engine.dispose = AsyncMock() bootstrap_schema = AsyncMock() + monkeypatch.setitem(sys.modules, "asyncpg", ModuleType("asyncpg")) with ( - patch.dict(sys.modules, {"asyncpg": ModuleType("asyncpg")}), patch.object(engine_mod, "create_async_engine", return_value=mock_engine) as create_engine, patch.object(engine_mod, "async_sessionmaker", return_value=MagicMock()), patch("deerflow.persistence.bootstrap.bootstrap_schema", new=bootstrap_schema), @@ -151,7 +152,7 @@ async def test_init_engine_postgres_uses_hardened_kwargs() -> None: @pytest.mark.asyncio -async def test_init_engine_postgres_retry_uses_hardened_kwargs() -> None: +async def test_init_engine_postgres_retry_uses_hardened_kwargs(monkeypatch) -> None: url = "postgresql+asyncpg://user:password@localhost/deerflow" initial_engine = MagicMock() initial_engine.dispose = AsyncMock() @@ -160,8 +161,8 @@ async def test_init_engine_postgres_retry_uses_hardened_kwargs() -> None: bootstrap_schema = AsyncMock(side_effect=[Exception("database does not exist"), None]) auto_create = AsyncMock() + monkeypatch.setitem(sys.modules, "asyncpg", ModuleType("asyncpg")) with ( - patch.dict(sys.modules, {"asyncpg": ModuleType("asyncpg")}), patch.object(engine_mod, "create_async_engine", side_effect=[initial_engine, retry_engine]) as create_engine, patch.object(engine_mod, "async_sessionmaker", return_value=MagicMock()), patch.object(engine_mod, "_auto_create_postgres_db", new=auto_create),