refactor: inline non-sandbox actions, strip registry, drop schemas
Cleanup pass after the migration: #1 Inline ``*_actions.py`` into wrapper ``tool[s].py`` for the non-sandbox tools (think, todo, notes, reporting, web_search, finish_scan). One file per tool family now. Helpers + public function bodies live alongside the ``@strix_tool``-decorated wrappers that call them. For notes, the sync helpers are renamed to ``_create_note_impl`` / ``_list_notes_impl`` / etc. so the public names ``create_note`` / ``list_notes`` / etc. can be the FunctionTool instances the agent factory imports. ``append_note_content`` (used by the agents-graph wiki-update hook) calls the impl helpers directly. #2 Delete ``strix/tools/_state_adapter.py``. The ``AgentStateAdapter`` shim only existed to feed legacy ``*_actions.py`` functions a ``state.agent_id`` they could read. With the actions inlined, the wrappers read ``ctx.context['agent_id']`` directly. #3 Strip ``strix/tools/registry.py`` from ~250 LOC to ~110. Deleted: XML schema loading, ``_parse_param_schema``, ``get_tools_prompt``, ``get_tool_param_schema``, ``needs_agent_state``, ``should_execute_in_sandbox``, ``validate_tool_availability`` — all for the host-side legacy dispatcher path. Kept the ``register_tool`` decorator (sandbox side), ``get_tool_by_name``, ``get_tool_names``, ``tools`` list, ``clear_registry``. The Jinja prompt template's ``{{ get_tools_prompt() }}`` injection is dropped — the SDK auto-generates tool descriptions from function signatures, so the legacy XML tool block was redundant and stale. #4 Delete every ``*_actions_schema.xml`` (12 files). They were read by the now-removed ``_load_xml_schema`` to build the legacy prompt's tool descriptions. No consumer remains. Side fixes: - ``reporting_renderer.py`` updated to import ``_parse_*_xml`` from the new location with leading underscore. - ``test_local_tools.py``, ``test_notes_jsonl_concurrency.py``, ``test_notes_wiki.py`` updated to point at the new module paths and call the ``_*_impl`` sync helpers. Tests: 279/279 passing. ~1500 LOC of action files moved into the tool wrappers; ~140 LOC of registry boilerplate removed; ~400 lines of dead XML deleted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -20,7 +20,7 @@ from unittest.mock import patch
|
||||
import pytest
|
||||
from agents.tool import FunctionTool
|
||||
|
||||
from strix.tools.notes import notes_actions as _notes_impl
|
||||
from strix.tools.notes import tools as _notes_impl
|
||||
from strix.tools.notes.tools import (
|
||||
create_note,
|
||||
delete_note,
|
||||
@@ -98,9 +98,9 @@ async def test_think_rejects_empty() -> None:
|
||||
@pytest.fixture(autouse=True)
|
||||
def _isolate_todo_storage() -> None:
|
||||
"""Each test starts with an empty todo store so tests don't bleed."""
|
||||
from strix.tools.todo import todo_actions
|
||||
from strix.tools.todo import tools as todo_module
|
||||
|
||||
todo_actions._todos_storage.clear()
|
||||
todo_module._todos_storage.clear()
|
||||
|
||||
|
||||
def test_todo_tools_are_function_tools() -> None:
|
||||
|
||||
@@ -17,7 +17,7 @@ from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
from strix.tools.notes.notes_actions import _append_note_event
|
||||
from strix.tools.notes.tools import _append_note_event
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
@@ -27,7 +27,7 @@ def notes_path(tmp_path: Path) -> Iterator[Path]:
|
||||
target.parent.mkdir(parents=True, exist_ok=True)
|
||||
|
||||
with patch(
|
||||
"strix.tools.notes.notes_actions._get_notes_jsonl_path",
|
||||
"strix.tools.notes.tools._get_notes_jsonl_path",
|
||||
return_value=target,
|
||||
):
|
||||
yield target
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
from pathlib import Path
|
||||
|
||||
from strix.telemetry.tracer import Tracer, get_global_tracer, set_global_tracer
|
||||
from strix.tools.notes import notes_actions
|
||||
from strix.tools.notes import tools as notes_actions
|
||||
|
||||
|
||||
def _reset_notes_state() -> None:
|
||||
@@ -18,7 +18,7 @@ def test_wiki_notes_are_persisted_and_removed(tmp_path: Path, monkeypatch) -> No
|
||||
set_global_tracer(tracer)
|
||||
|
||||
try:
|
||||
created = notes_actions.create_note(
|
||||
created = notes_actions._create_note_impl(
|
||||
title="Repo Map",
|
||||
content="## Architecture\n- monolith",
|
||||
category="wiki",
|
||||
@@ -36,14 +36,14 @@ def test_wiki_notes_are_persisted_and_removed(tmp_path: Path, monkeypatch) -> No
|
||||
assert wiki_path.exists()
|
||||
assert "## Architecture" in wiki_path.read_text(encoding="utf-8")
|
||||
|
||||
updated = notes_actions.update_note(
|
||||
updated = notes_actions._update_note_impl(
|
||||
note_id=note_id,
|
||||
content="## Architecture\n- service-oriented",
|
||||
)
|
||||
assert updated["success"] is True
|
||||
assert "service-oriented" in wiki_path.read_text(encoding="utf-8")
|
||||
|
||||
deleted = notes_actions.delete_note(note_id=note_id)
|
||||
deleted = notes_actions._delete_note_impl(note_id=note_id)
|
||||
assert deleted["success"] is True
|
||||
assert wiki_path.exists() is False
|
||||
finally:
|
||||
@@ -60,7 +60,7 @@ def test_notes_jsonl_replay_survives_memory_reset(tmp_path: Path, monkeypatch) -
|
||||
set_global_tracer(tracer)
|
||||
|
||||
try:
|
||||
created = notes_actions.create_note(
|
||||
created = notes_actions._create_note_impl(
|
||||
title="Auth findings",
|
||||
content="initial finding",
|
||||
category="findings",
|
||||
@@ -74,24 +74,24 @@ def test_notes_jsonl_replay_survives_memory_reset(tmp_path: Path, monkeypatch) -
|
||||
assert notes_path.exists() is True
|
||||
|
||||
_reset_notes_state()
|
||||
listed = notes_actions.list_notes(category="findings")
|
||||
listed = notes_actions._list_notes_impl(category="findings")
|
||||
assert listed["success"] is True
|
||||
assert listed["total_count"] == 1
|
||||
assert listed["notes"][0]["note_id"] == note_id
|
||||
assert "content" not in listed["notes"][0]
|
||||
assert "content_preview" in listed["notes"][0]
|
||||
|
||||
updated = notes_actions.update_note(note_id=note_id, content="updated finding")
|
||||
updated = notes_actions._update_note_impl(note_id=note_id, content="updated finding")
|
||||
assert updated["success"] is True
|
||||
|
||||
_reset_notes_state()
|
||||
listed_after_update = notes_actions.list_notes(search="updated finding")
|
||||
listed_after_update = notes_actions._list_notes_impl(search="updated finding")
|
||||
assert listed_after_update["success"] is True
|
||||
assert listed_after_update["total_count"] == 1
|
||||
assert listed_after_update["notes"][0]["note_id"] == note_id
|
||||
assert listed_after_update["notes"][0]["content_preview"] == "updated finding"
|
||||
|
||||
listed_with_content = notes_actions.list_notes(
|
||||
listed_with_content = notes_actions._list_notes_impl(
|
||||
category="findings",
|
||||
include_content=True,
|
||||
)
|
||||
@@ -99,11 +99,11 @@ def test_notes_jsonl_replay_survives_memory_reset(tmp_path: Path, monkeypatch) -
|
||||
assert listed_with_content["total_count"] == 1
|
||||
assert listed_with_content["notes"][0]["content"] == "updated finding"
|
||||
|
||||
deleted = notes_actions.delete_note(note_id=note_id)
|
||||
deleted = notes_actions._delete_note_impl(note_id=note_id)
|
||||
assert deleted["success"] is True
|
||||
|
||||
_reset_notes_state()
|
||||
listed_after_delete = notes_actions.list_notes(category="findings")
|
||||
listed_after_delete = notes_actions._list_notes_impl(category="findings")
|
||||
assert listed_after_delete["success"] is True
|
||||
assert listed_after_delete["total_count"] == 0
|
||||
finally:
|
||||
@@ -120,7 +120,7 @@ def test_get_note_returns_full_note(tmp_path: Path, monkeypatch) -> None:
|
||||
set_global_tracer(tracer)
|
||||
|
||||
try:
|
||||
created = notes_actions.create_note(
|
||||
created = notes_actions._create_note_impl(
|
||||
title="Repo wiki",
|
||||
content="entrypoints and sinks",
|
||||
category="wiki",
|
||||
@@ -130,7 +130,7 @@ def test_get_note_returns_full_note(tmp_path: Path, monkeypatch) -> None:
|
||||
note_id = created["note_id"]
|
||||
assert isinstance(note_id, str)
|
||||
|
||||
result = notes_actions.get_note(note_id=note_id)
|
||||
result = notes_actions._get_note_impl(note_id=note_id)
|
||||
assert result["success"] is True
|
||||
assert result["note"]["note_id"] == note_id
|
||||
assert result["note"]["content"] == "entrypoints and sinks"
|
||||
@@ -148,7 +148,7 @@ def test_append_note_content_appends_delta(tmp_path: Path, monkeypatch) -> None:
|
||||
set_global_tracer(tracer)
|
||||
|
||||
try:
|
||||
created = notes_actions.create_note(
|
||||
created = notes_actions._create_note_impl(
|
||||
title="Repo wiki",
|
||||
content="base",
|
||||
category="wiki",
|
||||
@@ -164,7 +164,7 @@ def test_append_note_content_appends_delta(tmp_path: Path, monkeypatch) -> None:
|
||||
)
|
||||
assert appended["success"] is True
|
||||
|
||||
loaded = notes_actions.get_note(note_id=note_id)
|
||||
loaded = notes_actions._get_note_impl(note_id=note_id)
|
||||
assert loaded["success"] is True
|
||||
assert loaded["note"]["content"] == "base\n\n## Agent Update: worker\nSummary: done"
|
||||
finally:
|
||||
@@ -183,7 +183,7 @@ def test_list_and_get_note_handle_wiki_repersist_oserror_gracefully(
|
||||
set_global_tracer(tracer)
|
||||
|
||||
try:
|
||||
created = notes_actions.create_note(
|
||||
created = notes_actions._create_note_impl(
|
||||
title="Repo wiki",
|
||||
content="initial wiki content",
|
||||
category="wiki",
|
||||
@@ -200,12 +200,12 @@ def test_list_and_get_note_handle_wiki_repersist_oserror_gracefully(
|
||||
|
||||
monkeypatch.setattr(notes_actions, "_persist_wiki_note", _raise_oserror)
|
||||
|
||||
listed = notes_actions.list_notes(category="wiki")
|
||||
listed = notes_actions._list_notes_impl(category="wiki")
|
||||
assert listed["success"] is True
|
||||
assert listed["total_count"] == 1
|
||||
assert listed["notes"][0]["note_id"] == note_id
|
||||
|
||||
fetched = notes_actions.get_note(note_id=note_id)
|
||||
fetched = notes_actions._get_note_impl(note_id=note_id)
|
||||
assert fetched["success"] is True
|
||||
assert fetched["note"]["note_id"] == note_id
|
||||
assert fetched["note"]["content"] == "initial wiki content"
|
||||
|
||||
@@ -85,8 +85,8 @@ async def test_web_search_no_api_key_returns_structured_error(
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_web_search_delegates_to_impl(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Legacy ``web_search`` returns dict; wrapper JSON-encodes it."""
|
||||
async def test_web_search_delegates_to_perplexity(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""The wrapper invokes the Perplexity HTTP path on a thread."""
|
||||
monkeypatch.setenv("PERPLEXITY_API_KEY", "fake-key")
|
||||
|
||||
fake_result = {
|
||||
@@ -96,13 +96,13 @@ async def test_web_search_delegates_to_impl(monkeypatch: pytest.MonkeyPatch) ->
|
||||
"message": "Web search completed successfully",
|
||||
}
|
||||
with patch(
|
||||
"strix.tools.web_search.tool._impl.web_search",
|
||||
"strix.tools.web_search.tool._do_search",
|
||||
return_value=fake_result,
|
||||
) as legacy:
|
||||
) as do_search:
|
||||
out = await _invoke(web_search, _ctx_for(), query="xss techniques")
|
||||
|
||||
assert out == fake_result
|
||||
legacy.assert_called_once_with(query="xss techniques")
|
||||
do_search.assert_called_once_with("xss techniques")
|
||||
|
||||
|
||||
# --- file_edit (sandbox-bound) -------------------------------------------
|
||||
@@ -197,7 +197,7 @@ async def test_create_vulnerability_report_validates_required_fields() -> None:
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_vulnerability_report_delegates_to_impl() -> None:
|
||||
"""Verify the wrapper passes all params through to the legacy function."""
|
||||
"""Verify the wrapper threads its kwargs through to the implementation."""
|
||||
fake_result = {
|
||||
"success": True,
|
||||
"message": "Vulnerability report 'X' created successfully",
|
||||
@@ -206,9 +206,9 @@ async def test_create_vulnerability_report_delegates_to_impl() -> None:
|
||||
"cvss_score": 7.5,
|
||||
}
|
||||
with patch(
|
||||
"strix.tools.reporting.tool._impl.create_vulnerability_report",
|
||||
"strix.tools.reporting.tool._do_create",
|
||||
return_value=fake_result,
|
||||
) as legacy:
|
||||
) as do_create:
|
||||
out = await _invoke(
|
||||
create_vulnerability_report,
|
||||
_ctx_for(),
|
||||
@@ -225,7 +225,7 @@ async def test_create_vulnerability_report_delegates_to_impl() -> None:
|
||||
)
|
||||
|
||||
assert out == fake_result
|
||||
kwargs = legacy.call_args.kwargs
|
||||
kwargs = do_create.call_args.kwargs
|
||||
assert kwargs["title"] == "t"
|
||||
assert kwargs["cve"] == "CVE-2024-12345"
|
||||
# Optional params we didn't pass should still be forwarded as None.
|
||||
@@ -252,18 +252,40 @@ async def test_finish_scan_validates_empty_fields() -> None:
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_finish_scan_delegates_to_impl() -> None:
|
||||
"""Wrapper must pass the legacy adapter and the four sections through."""
|
||||
fake_result = {
|
||||
"success": True,
|
||||
"scan_completed": True,
|
||||
"message": "Scan completed successfully",
|
||||
"vulnerabilities_found": 3,
|
||||
}
|
||||
async def test_finish_scan_rejects_subagent() -> None:
|
||||
"""A subagent (parent_id is set) must not be able to finish the scan."""
|
||||
ctx = _Ctx(
|
||||
context={
|
||||
"agent_id": "child-1",
|
||||
"parent_id": "root-1",
|
||||
"tool_server_host_port": 12345,
|
||||
"sandbox_token": "test-token",
|
||||
},
|
||||
)
|
||||
out = await _invoke(
|
||||
finish_scan,
|
||||
ctx,
|
||||
executive_summary="es",
|
||||
methodology="m",
|
||||
technical_analysis="ta",
|
||||
recommendations="r",
|
||||
)
|
||||
assert out["success"] is False
|
||||
assert out["error"] == "finish_scan_wrong_agent"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_finish_scan_persists_via_tracer() -> None:
|
||||
"""When a global tracer exists, finish_scan should write the four sections."""
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
fake_tracer = MagicMock()
|
||||
fake_tracer.vulnerability_reports = [{}, {}, {}]
|
||||
|
||||
with patch(
|
||||
"strix.tools.finish.tool._impl.finish_scan",
|
||||
return_value=fake_result,
|
||||
) as legacy:
|
||||
"strix.telemetry.tracer.get_global_tracer",
|
||||
return_value=fake_tracer,
|
||||
):
|
||||
out = await _invoke(
|
||||
finish_scan,
|
||||
_ctx_for("root-agent"),
|
||||
@@ -273,7 +295,11 @@ async def test_finish_scan_delegates_to_impl() -> None:
|
||||
recommendations="r",
|
||||
)
|
||||
|
||||
assert out == fake_result
|
||||
kwargs = legacy.call_args.kwargs
|
||||
assert kwargs["executive_summary"] == "es"
|
||||
assert kwargs["agent_state"].agent_id == "root-agent"
|
||||
assert out["success"] is True
|
||||
assert out["vulnerabilities_found"] == 3
|
||||
fake_tracer.update_scan_final_fields.assert_called_once_with(
|
||||
executive_summary="es",
|
||||
methodology="m",
|
||||
technical_analysis="ta",
|
||||
recommendations="r",
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user