diff --git a/backend/packages/harness/deerflow/agents/lead_agent/agent.py b/backend/packages/harness/deerflow/agents/lead_agent/agent.py index 754e7bed1..fcf93b0e2 100644 --- a/backend/packages/harness/deerflow/agents/lead_agent/agent.py +++ b/backend/packages/harness/deerflow/agents/lead_agent/agent.py @@ -13,8 +13,12 @@ middleware reachable from this graph (e.g. ``TitleMiddleware``) — MUST pass Forgetting that flag emits duplicate spans (one rooted at the graph, one at the model) AND prevents the Langfuse handler's ``propagate_attributes`` path from firing, so ``session_id`` / ``user_id`` never reach the trace. -The four current sites are: bootstrap agent, default agent, summarization -middleware, and the async path inside ``TitleMiddleware``. Any new in-graph +The five current sites are: bootstrap agent, default agent, summarization +middleware, the async path inside ``TitleMiddleware``, and the skill security +scanner reached from the ``skill_manage`` tool (``skills/security_scanner.py``'s +``scan_skill_content``, which is dual-use: ``_scan_or_raise`` in +``tools/skill_manage_tool.py`` is the in-graph choke point and passes the flag, +while its standalone callers keep the default). Any new in-graph ``create_chat_model`` call must add to this list and pass the flag. """ diff --git a/backend/packages/harness/deerflow/skills/security_scanner.py b/backend/packages/harness/deerflow/skills/security_scanner.py index b38145399..5534250ef 100644 --- a/backend/packages/harness/deerflow/skills/security_scanner.py +++ b/backend/packages/harness/deerflow/skills/security_scanner.py @@ -96,8 +96,19 @@ async def scan_skill_content( location: str = SKILL_MD_FILE, app_config: AppConfig | None = None, static_findings: list[dict[str, Any]] | None = None, + attach_tracing: bool = True, ) -> ScanResult: - """Screen skill content before it is written to disk.""" + """Screen skill content before it is written to disk. + + ``attach_tracing`` follows the tracing INVARIANT in + ``agents/lead_agent/agent.py``: in-graph callers must pass ``False`` because + the graph root already attached the callbacks, and attaching again at the + model emits duplicate spans *and* blocks the Langfuse handler's + ``propagate_attributes`` path. This function is dual-use, so the flag is the + caller's to set — the in-graph choke point is ``_scan_or_raise`` in + ``tools/skill_manage_tool.py``. Standalone callers (Gateway skill routes, + ``skills/installer.py``) have no root to inherit from and keep the default. + """ rubric = ( "You are a security reviewer for AI agent skills. " "Classify the content as allow, warn, or block. " @@ -112,7 +123,8 @@ async def scan_skill_content( try: config = app_config or get_app_config() model_name = config.skill_evolution.moderation_model_name - model = create_chat_model(name=model_name, thinking_enabled=False, app_config=config) if model_name else create_chat_model(thinking_enabled=False, app_config=config) + model_kwargs = {"thinking_enabled": False, "app_config": config, "attach_tracing": attach_tracing} + model = create_chat_model(name=model_name, **model_kwargs) if model_name else create_chat_model(**model_kwargs) response = await model.ainvoke( [ {"role": "system", "content": rubric}, diff --git a/backend/packages/harness/deerflow/tools/skill_manage_tool.py b/backend/packages/harness/deerflow/tools/skill_manage_tool.py index e737b008a..a62286add 100644 --- a/backend/packages/harness/deerflow/tools/skill_manage_tool.py +++ b/backend/packages/harness/deerflow/tools/skill_manage_tool.py @@ -64,7 +64,9 @@ def _history_record(*, action: str, file_path: str, prev_content: str | None, ne async def _scan_or_raise(content: str, *, executable: bool, location: str, static_findings: list[StaticFinding] | None = None) -> dict[str, Any]: - result = await scan_skill_content(content, executable=executable, location=location, static_findings=static_findings or []) + # In-graph: the graph root already attached tracing (see the INVARIANT in + # agents/lead_agent/agent.py), so the scan model must not attach it again. + result = await scan_skill_content(content, executable=executable, location=location, static_findings=static_findings or [], attach_tracing=False) if result.decision == "block": raise ValueError(f"Security scan blocked the write: {result.reason}") if executable and result.decision != "allow": diff --git a/backend/tests/test_security_scanner.py b/backend/tests/test_security_scanner.py index 9265f8a37..1471c4533 100644 --- a/backend/tests/test_security_scanner.py +++ b/backend/tests/test_security_scanner.py @@ -17,8 +17,13 @@ def _make_env(monkeypatch, response_content): return fake_response model = FakeModel() + + def _fake_create_chat_model(**kwargs): + model.create_kwargs = kwargs + return model + monkeypatch.setattr("deerflow.skills.security_scanner.get_app_config", lambda: config) - monkeypatch.setattr("deerflow.skills.security_scanner.create_chat_model", lambda **kwargs: model) + monkeypatch.setattr("deerflow.skills.security_scanner.create_chat_model", _fake_create_chat_model) return model @@ -142,6 +147,39 @@ async def test_scan_distinguishes_unparseable_executable(monkeypatch): assert "unparseable" in result.reason +# --- tracing wiring: in-graph vs standalone (see the INVARIANT in +# packages/harness/deerflow/agents/lead_agent/agent.py and the Tracing System +# section of backend/AGENTS.md) --- + + +@pytest.mark.anyio +async def test_scan_skill_content_forwards_attach_tracing_to_the_model(monkeypatch): + """In-graph callers pass ``attach_tracing=False``; it must reach the factory. + + The graph root already attached the callbacks, so attaching again at the model + emits duplicate spans and blocks the Langfuse handler's ``propagate_attributes`` + path, meaning session_id/user_id never land on the trace. + """ + model = _make_env(monkeypatch, '{"decision":"allow","reason":"ok"}') + result = await scan_skill_content(SKILL_CONTENT, executable=False, attach_tracing=False) + assert result.decision == "allow" + assert model.create_kwargs["attach_tracing"] is False + + +@pytest.mark.anyio +async def test_scan_skill_content_attaches_model_tracing_by_default(monkeypatch): + """Standalone callers (Gateway skill routes, installer) have no graph root to + inherit from, so the default keeps model-level attachment. + + Anchors the other direction of the change: narrowing the fix into an + unconditional ``attach_tracing=False`` would silently drop their spans. + """ + model = _make_env(monkeypatch, '{"decision":"allow","reason":"ok"}') + result = await scan_skill_content(SKILL_CONTENT, executable=False) + assert result.decision == "allow" + assert model.create_kwargs["attach_tracing"] is True + + def _make_unavailable_env(monkeypatch, *, security_fail_closed): config = SimpleNamespace( skill_evolution=SimpleNamespace( diff --git a/backend/tests/test_skill_manage_tool.py b/backend/tests/test_skill_manage_tool.py index 8c54bfe5f..f94182d6e 100644 --- a/backend/tests/test_skill_manage_tool.py +++ b/backend/tests/test_skill_manage_tool.py @@ -340,3 +340,43 @@ def test_skill_manage_per_user_isolation(monkeypatch, tmp_path): # No cross-contamination assert not (tmp_path / "users" / "alice" / "skills" / "custom" / "bob-skill").exists() assert not (tmp_path / "users" / "bob" / "skills" / "custom" / "alice-skill").exists() + + +# --- tracing wiring: the in-graph choke point (see the INVARIANT in +# packages/harness/deerflow/agents/lead_agent/agent.py) --- + + +def test_scan_or_raise_does_not_attach_model_tracing(monkeypatch, tmp_path): + """``_scan_or_raise`` is the in-graph choke point for the skill security scan. + + The graph root already attached the tracing callbacks, so the scan model must + not attach them again: double-attaching emits duplicate spans and blocks the + Langfuse handler's ``propagate_attributes`` path, so session_id/user_id never + reach the trace. Drives the real ``scan_skill_content`` rather than stubbing it, + so the flag is pinned all the way to the model factory. + """ + config = _make_config(tmp_path / "skills") + monkeypatch.setattr("deerflow.skills.security_scanner.get_app_config", lambda: config) + + create_kwargs = {} + + class FakeModel: + async def ainvoke(self, *args, **kwargs): + return SimpleNamespace(content='{"decision":"allow","reason":"ok"}') + + def _fake_create_chat_model(**kwargs): + create_kwargs.update(kwargs) + return FakeModel() + + monkeypatch.setattr("deerflow.skills.security_scanner.create_chat_model", _fake_create_chat_model) + + result = anyio.run( + lambda: skill_manage_module._scan_or_raise( + _skill_content("demo-skill"), + executable=False, + location="demo-skill/SKILL.md", + ) + ) + + assert result["decision"] == "allow" + assert create_kwargs["attach_tracing"] is False