refactor: nuke gratuitous XML serialization + delete argument_parser
Argument parser: - Delete ``strix/tools/argument_parser.py`` and its tests. The SDK validates and types tool arguments via Pydantic before they hit our wrappers, and the in-container tool server receives JSON-typed kwargs over the wire. The string-coercion belt-and-suspenders is no longer pulling its weight. XML → JSON / typed structures: - ``create_vulnerability_report``: ``cvss_breakdown`` is now a ``dict[str, str]`` of the 8 metrics; ``code_locations`` is a ``list[dict]``. No more XML parsing in the tool or the renderer. - ``check_duplicate``: the dedup judge now emits a single JSON object instead of an ``<dedupe_result>`` block. Strict JSON parser handles optional code-fence wrappers. - ``agent_finish``: completion report posted to the parent inbox is a JSON object (``kind``, ``from``, ``agent_id``, ``success``, ``summary``, ``findings``, ``recommendations``) rather than a hand-rolled ``<agent_completion_report>`` XML envelope. - ``create_agent``: identity preamble + inherited-context markers are plain bracketed labels rather than ``<agent_delegation>`` / ``<inherited_context_from_parent>`` envelopes. - ``inject_messages_filter``: peer messages get a ``[Message from agent <id> | type=... | priority=...]`` header line instead of an ``<inter_agent_message>`` envelope. - Crash + system-warning messages: bracketed labels, no XML. - System prompt: the inter-agent block now describes the new header format and drops the "never echo XML envelope" rule. - ``strix/llm/utils.py``: deleted. ``clean_content`` collapsed into a one-line blank-line normalizer in the agent-message renderer (the XML envelope scrub had nothing left to scrub). Tests updated to match the new shapes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -53,14 +53,14 @@ async def test_pending_messages_appended_in_order() -> None:
|
||||
|
||||
assert len(out.input) == 3
|
||||
assert out.input[0] == {"role": "user", "content": "task"}
|
||||
assert "<inter_agent_message from='b'" in out.input[1]["content"]
|
||||
assert "Message from agent b" in out.input[1]["content"]
|
||||
assert "hello" in out.input[1]["content"]
|
||||
assert "second" in out.input[2]["content"]
|
||||
assert "priority='high'" in out.input[2]["content"]
|
||||
assert "priority=high" in out.input[2]["content"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_user_sender_skips_xml_wrap() -> None:
|
||||
async def test_user_sender_skips_envelope() -> None:
|
||||
bus = AgentMessageBus()
|
||||
await bus.register("a1", "alpha", parent_id=None)
|
||||
await bus.send("a1", {"from": "user", "content": "follow-up question"})
|
||||
|
||||
@@ -93,7 +93,7 @@ async def test_on_llm_end_records_usage_and_increments_turn() -> None:
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_on_agent_end_detects_crash() -> None:
|
||||
"""C8 (AUDIT_R2): on_agent_end without agent_finish_called posts crash to parent."""
|
||||
"""on_agent_end without agent_finish_called posts crash message to parent."""
|
||||
hooks = StrixOrchestrationHooks()
|
||||
bus = AgentMessageBus()
|
||||
await bus.register("root", "root", parent_id=None)
|
||||
@@ -104,8 +104,9 @@ async def test_on_agent_end_detects_crash() -> None:
|
||||
|
||||
drained = await bus.drain("root")
|
||||
assert len(drained) == 1
|
||||
assert "<agent_crash" in drained[0]["content"]
|
||||
assert "agent_id='child'" in drained[0]["content"]
|
||||
assert "[Agent crash]" in drained[0]["content"]
|
||||
assert "child" in drained[0]["content"]
|
||||
assert drained[0]["type"] == "crash"
|
||||
assert bus.statuses["child"] == "crashed"
|
||||
|
||||
|
||||
|
||||
@@ -1,271 +0,0 @@
|
||||
from collections.abc import Callable
|
||||
|
||||
import pytest
|
||||
|
||||
from strix.tools.argument_parser import (
|
||||
ArgumentConversionError,
|
||||
_convert_basic_types,
|
||||
_convert_to_bool,
|
||||
_convert_to_dict,
|
||||
_convert_to_list,
|
||||
convert_arguments,
|
||||
convert_string_to_type,
|
||||
)
|
||||
|
||||
|
||||
class TestConvertToBool:
|
||||
"""Tests for the _convert_to_bool function."""
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"value",
|
||||
["true", "True", "TRUE", "1", "yes", "Yes", "YES", "on", "On", "ON"],
|
||||
)
|
||||
def test_truthy_values(self, value: str) -> None:
|
||||
"""Test that truthy string values are converted to True."""
|
||||
assert _convert_to_bool(value) is True
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"value",
|
||||
["false", "False", "FALSE", "0", "no", "No", "NO", "off", "Off", "OFF"],
|
||||
)
|
||||
def test_falsy_values(self, value: str) -> None:
|
||||
"""Test that falsy string values are converted to False."""
|
||||
assert _convert_to_bool(value) is False
|
||||
|
||||
def test_non_standard_truthy_string(self) -> None:
|
||||
"""Test that non-empty non-standard strings are truthy."""
|
||||
assert _convert_to_bool("anything") is True
|
||||
assert _convert_to_bool("hello") is True
|
||||
|
||||
def test_empty_string(self) -> None:
|
||||
"""Test that empty string is falsy."""
|
||||
assert _convert_to_bool("") is False
|
||||
|
||||
|
||||
class TestConvertToList:
|
||||
"""Tests for the _convert_to_list function."""
|
||||
|
||||
def test_json_array_string(self) -> None:
|
||||
"""Test parsing a JSON array string."""
|
||||
result = _convert_to_list('["a", "b", "c"]')
|
||||
assert result == ["a", "b", "c"]
|
||||
|
||||
def test_json_array_with_numbers(self) -> None:
|
||||
"""Test parsing a JSON array with numbers."""
|
||||
result = _convert_to_list("[1, 2, 3]")
|
||||
assert result == [1, 2, 3]
|
||||
|
||||
def test_comma_separated_string(self) -> None:
|
||||
"""Test parsing a comma-separated string."""
|
||||
result = _convert_to_list("a, b, c")
|
||||
assert result == ["a", "b", "c"]
|
||||
|
||||
def test_comma_separated_no_spaces(self) -> None:
|
||||
"""Test parsing comma-separated values without spaces."""
|
||||
result = _convert_to_list("x,y,z")
|
||||
assert result == ["x", "y", "z"]
|
||||
|
||||
def test_single_value(self) -> None:
|
||||
"""Test that a single value returns a list with one element."""
|
||||
result = _convert_to_list("single")
|
||||
assert result == ["single"]
|
||||
|
||||
def test_json_non_array_wraps_in_list(self) -> None:
|
||||
"""Test that a valid JSON non-array value is wrapped in a list."""
|
||||
result = _convert_to_list('"string"')
|
||||
assert result == ["string"]
|
||||
|
||||
def test_json_object_wraps_in_list(self) -> None:
|
||||
"""Test that a JSON object is wrapped in a list."""
|
||||
result = _convert_to_list('{"key": "value"}')
|
||||
assert result == [{"key": "value"}]
|
||||
|
||||
def test_empty_json_array(self) -> None:
|
||||
"""Test parsing an empty JSON array."""
|
||||
result = _convert_to_list("[]")
|
||||
assert result == []
|
||||
|
||||
|
||||
class TestConvertToDict:
|
||||
"""Tests for the _convert_to_dict function."""
|
||||
|
||||
def test_valid_json_object(self) -> None:
|
||||
"""Test parsing a valid JSON object string."""
|
||||
result = _convert_to_dict('{"key": "value", "number": 42}')
|
||||
assert result == {"key": "value", "number": 42}
|
||||
|
||||
def test_empty_json_object(self) -> None:
|
||||
"""Test parsing an empty JSON object."""
|
||||
result = _convert_to_dict("{}")
|
||||
assert result == {}
|
||||
|
||||
def test_invalid_json_returns_empty_dict(self) -> None:
|
||||
"""Test that invalid JSON returns an empty dictionary."""
|
||||
result = _convert_to_dict("not json")
|
||||
assert result == {}
|
||||
|
||||
def test_json_array_returns_empty_dict(self) -> None:
|
||||
"""Test that a JSON array returns an empty dictionary."""
|
||||
result = _convert_to_dict("[1, 2, 3]")
|
||||
assert result == {}
|
||||
|
||||
def test_nested_json_object(self) -> None:
|
||||
"""Test parsing a nested JSON object."""
|
||||
result = _convert_to_dict('{"outer": {"inner": "value"}}')
|
||||
assert result == {"outer": {"inner": "value"}}
|
||||
|
||||
|
||||
class TestConvertBasicTypes:
|
||||
"""Tests for the _convert_basic_types function."""
|
||||
|
||||
def test_convert_to_int(self) -> None:
|
||||
"""Test converting string to int."""
|
||||
assert _convert_basic_types("42", int) == 42
|
||||
assert _convert_basic_types("-10", int) == -10
|
||||
|
||||
def test_convert_to_float(self) -> None:
|
||||
"""Test converting string to float."""
|
||||
assert _convert_basic_types("3.14", float) == 3.14
|
||||
assert _convert_basic_types("-2.5", float) == -2.5
|
||||
|
||||
def test_convert_to_str(self) -> None:
|
||||
"""Test converting string to str (passthrough)."""
|
||||
assert _convert_basic_types("hello", str) == "hello"
|
||||
|
||||
def test_convert_to_bool(self) -> None:
|
||||
"""Test converting string to bool."""
|
||||
assert _convert_basic_types("true", bool) is True
|
||||
assert _convert_basic_types("false", bool) is False
|
||||
|
||||
def test_convert_to_list_type(self) -> None:
|
||||
"""Test converting to list type."""
|
||||
result = _convert_basic_types("[1, 2, 3]", list)
|
||||
assert result == [1, 2, 3]
|
||||
|
||||
def test_convert_to_dict_type(self) -> None:
|
||||
"""Test converting to dict type."""
|
||||
result = _convert_basic_types('{"a": 1}', dict)
|
||||
assert result == {"a": 1}
|
||||
|
||||
def test_unknown_type_attempts_json(self) -> None:
|
||||
"""Test that unknown types attempt JSON parsing."""
|
||||
result = _convert_basic_types('{"key": "value"}', object)
|
||||
assert result == {"key": "value"}
|
||||
|
||||
def test_unknown_type_returns_original(self) -> None:
|
||||
"""Test that unparseable values are returned as-is."""
|
||||
result = _convert_basic_types("plain text", object)
|
||||
assert result == "plain text"
|
||||
|
||||
|
||||
class TestConvertStringToType:
|
||||
"""Tests for the convert_string_to_type function."""
|
||||
|
||||
def test_basic_type_conversion(self) -> None:
|
||||
"""Test basic type conversions."""
|
||||
assert convert_string_to_type("42", int) == 42
|
||||
assert convert_string_to_type("3.14", float) == 3.14
|
||||
assert convert_string_to_type("true", bool) is True
|
||||
|
||||
def test_optional_type(self) -> None:
|
||||
"""Test conversion with Optional type."""
|
||||
result = convert_string_to_type("42", int | None)
|
||||
assert result == 42
|
||||
|
||||
def test_union_type(self) -> None:
|
||||
"""Test conversion with Union type."""
|
||||
result = convert_string_to_type("42", int | str)
|
||||
assert result == 42
|
||||
|
||||
def test_union_type_with_none(self) -> None:
|
||||
"""Test conversion with Union including None."""
|
||||
result = convert_string_to_type("hello", str | None)
|
||||
assert result == "hello"
|
||||
|
||||
def test_modern_union_syntax(self) -> None:
|
||||
"""Test conversion with modern union syntax (int | None)."""
|
||||
result = convert_string_to_type("100", int | None)
|
||||
assert result == 100
|
||||
|
||||
|
||||
class TestConvertArguments:
|
||||
"""Tests for the convert_arguments function."""
|
||||
|
||||
def test_converts_typed_arguments(
|
||||
self, sample_function_with_types: Callable[..., None]
|
||||
) -> None:
|
||||
"""Test that arguments are converted based on type annotations."""
|
||||
kwargs = {
|
||||
"name": "test",
|
||||
"count": "5",
|
||||
"enabled": "true",
|
||||
"ratio": "2.5",
|
||||
"items": "[1, 2, 3]",
|
||||
"config": '{"key": "value"}',
|
||||
}
|
||||
result = convert_arguments(sample_function_with_types, kwargs)
|
||||
|
||||
assert result["name"] == "test"
|
||||
assert result["count"] == 5
|
||||
assert result["enabled"] is True
|
||||
assert result["ratio"] == 2.5
|
||||
assert result["items"] == [1, 2, 3]
|
||||
assert result["config"] == {"key": "value"}
|
||||
|
||||
def test_passes_through_none_values(
|
||||
self, sample_function_with_types: Callable[..., None]
|
||||
) -> None:
|
||||
"""Test that None values are passed through unchanged."""
|
||||
kwargs = {"name": "test", "count": None}
|
||||
result = convert_arguments(sample_function_with_types, kwargs)
|
||||
assert result["count"] is None
|
||||
|
||||
def test_passes_through_non_string_values(
|
||||
self, sample_function_with_types: Callable[..., None]
|
||||
) -> None:
|
||||
"""Test that non-string values are passed through unchanged."""
|
||||
kwargs = {"name": "test", "count": 42}
|
||||
result = convert_arguments(sample_function_with_types, kwargs)
|
||||
assert result["count"] == 42
|
||||
|
||||
def test_unknown_parameter_passed_through(
|
||||
self, sample_function_with_types: Callable[..., None]
|
||||
) -> None:
|
||||
"""Test that parameters not in signature are passed through."""
|
||||
kwargs = {"name": "test", "unknown_param": "value"}
|
||||
result = convert_arguments(sample_function_with_types, kwargs)
|
||||
assert result["unknown_param"] == "value"
|
||||
|
||||
def test_function_without_annotations(
|
||||
self, sample_function_no_annotations: Callable[..., None]
|
||||
) -> None:
|
||||
"""Test handling of functions without type annotations."""
|
||||
kwargs = {"arg1": "value1", "arg2": "42"}
|
||||
result = convert_arguments(sample_function_no_annotations, kwargs)
|
||||
assert result["arg1"] == "value1"
|
||||
assert result["arg2"] == "42"
|
||||
|
||||
def test_raises_error_on_conversion_failure(
|
||||
self, sample_function_with_types: Callable[..., None]
|
||||
) -> None:
|
||||
"""Test that ArgumentConversionError is raised on conversion failure."""
|
||||
kwargs = {"count": "not_a_number"}
|
||||
with pytest.raises(ArgumentConversionError) as exc_info:
|
||||
convert_arguments(sample_function_with_types, kwargs)
|
||||
assert exc_info.value.param_name == "count"
|
||||
|
||||
|
||||
class TestArgumentConversionError:
|
||||
"""Tests for the ArgumentConversionError exception class."""
|
||||
|
||||
def test_error_with_param_name(self) -> None:
|
||||
"""Test creating error with parameter name."""
|
||||
error = ArgumentConversionError("Test error", param_name="test_param")
|
||||
assert error.param_name == "test_param"
|
||||
assert str(error) == "Test error"
|
||||
|
||||
def test_error_without_param_name(self) -> None:
|
||||
"""Test creating error without parameter name."""
|
||||
error = ArgumentConversionError("Test error")
|
||||
assert error.param_name is None
|
||||
assert str(error) == "Test error"
|
||||
@@ -315,10 +315,10 @@ async def test_create_agent_spawns_and_registers_child() -> None:
|
||||
assert bus.names[new_id] == "recon-bot"
|
||||
assert new_id in bus.tasks
|
||||
|
||||
# Initial input shape: identity block + task message at the end.
|
||||
# Initial input shape: identity preamble + task message at the end.
|
||||
initial_input = runner_calls[0]["kwargs"]["input"]
|
||||
assert any(
|
||||
isinstance(item, dict) and "agent_delegation" in item.get("content", "")
|
||||
isinstance(item, dict) and "You are agent recon-bot" in item.get("content", "")
|
||||
for item in initial_input
|
||||
)
|
||||
assert initial_input[-1]["content"] == "enumerate hosts"
|
||||
@@ -375,8 +375,8 @@ async def test_create_agent_inherits_parent_history() -> None:
|
||||
|
||||
initial_input = runner_calls[0]["input"]
|
||||
contents = [item.get("content", "") for item in initial_input]
|
||||
assert "<inherited_context_from_parent>" in contents
|
||||
assert "</inherited_context_from_parent>" in contents
|
||||
assert any("Inherited context from parent" in c for c in contents)
|
||||
assert any("End of inherited context" in c for c in contents)
|
||||
# Parent's exact items should be in between.
|
||||
assert any(c == "scope: example.com" for c in contents)
|
||||
|
||||
@@ -427,9 +427,11 @@ async def test_agent_finish_posts_report_to_parent_inbox() -> None:
|
||||
msg = parent_msgs[0]
|
||||
assert msg["type"] == "completion"
|
||||
assert msg["from"] == "child-A"
|
||||
assert "found 3 issues" in msg["content"]
|
||||
assert "<finding>xss in /search</finding>" in msg["content"]
|
||||
assert "sanitize search input" in msg["content"]
|
||||
payload = json.loads(msg["content"])
|
||||
assert payload["kind"] == "agent_completion_report"
|
||||
assert payload["summary"] == "found 3 issues"
|
||||
assert "xss in /search" in payload["findings"]
|
||||
assert "sanitize search input" in payload["recommendations"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
@@ -176,7 +176,7 @@ async def test_search_files_routes_to_sandbox() -> None:
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_vulnerability_report_validates_required_fields() -> None:
|
||||
"""Empty required fields should be rejected by the legacy validator."""
|
||||
"""Empty required fields should be rejected by the validator."""
|
||||
out = await _invoke(
|
||||
create_vulnerability_report,
|
||||
_ctx_for(),
|
||||
@@ -188,7 +188,7 @@ async def test_create_vulnerability_report_validates_required_fields() -> None:
|
||||
poc_description="pd",
|
||||
poc_script_code="curl ...",
|
||||
remediation_steps="rs",
|
||||
cvss_breakdown="<attack_vector>N</attack_vector>",
|
||||
cvss_breakdown={"attack_vector": "N"},
|
||||
)
|
||||
assert out["success"] is False
|
||||
assert "errors" in out
|
||||
@@ -220,7 +220,7 @@ async def test_create_vulnerability_report_delegates_to_impl() -> None:
|
||||
poc_description="pd",
|
||||
poc_script_code="pc",
|
||||
remediation_steps="rs",
|
||||
cvss_breakdown="<x/>",
|
||||
cvss_breakdown={"attack_vector": "N"},
|
||||
cve="CVE-2024-12345",
|
||||
)
|
||||
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
"""Phase 2.5 smoke tests for the sandbox-bound SDK tool wrappers.
|
||||
"""Smoke tests for the sandbox-bound SDK tool wrappers.
|
||||
|
||||
Covers: browser_action, terminal_execute, python_action, and the seven
|
||||
Caido proxy tools.
|
||||
Covers: browser_action, terminal_execute, python_action, file_edit
|
||||
helpers, and the seven Caido proxy tools.
|
||||
|
||||
These wrappers are pure pass-throughs to ``post_to_sandbox`` — there's
|
||||
no per-tool logic to assert, so the tests focus on:
|
||||
@@ -9,9 +9,9 @@ no per-tool logic to assert, so the tests focus on:
|
||||
- ``FunctionTool`` registration succeeds (which proves the SDK could
|
||||
derive a JSON schema from the type hints — a non-trivial check given
|
||||
Literal types, ``dict[str, str]``, and strict-mode opt-outs).
|
||||
- The dispatch payload to ``post_to_sandbox`` mirrors the legacy XML
|
||||
schema verbatim, so the in-container tool server gets the same
|
||||
``kwargs`` shape it always has.
|
||||
- The dispatch payload to ``post_to_sandbox`` carries the right
|
||||
``kwargs`` shape so the in-container tool server can dispatch to the
|
||||
matching action.
|
||||
- The ``send_request`` / ``repeat_request`` tools opt out of strict
|
||||
schema mode (their ``headers`` / ``modifications`` dicts are
|
||||
free-form and would otherwise fail registration).
|
||||
|
||||
Reference in New Issue
Block a user