From 048113579eaba9679c6814f6079941f4d82a2d41 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Sat, 1 Aug 2026 10:34:47 -0400 Subject: [PATCH] Fix integration tests for the tool response envelope; fix action clobbering Closes #433. The envelope {success, result, error} is the intended contract, so the tests were asserting a superseded flat shape. Adds a `payload()` helper that documents the contract in one place and unwraps it. Three separate causes were behind the 29 failures, not one: 1. Envelope mismatch. `result["stdout"]` etc. now read `payload(result)`. `success` and `error` stay on the envelope -- `success` tracks the command's exit status, verified against TerminalTool for ok/failing/ missing-binary/timeout cases before changing anything. 2. Assertions that could never fail. `assert "error" in result` appeared in four places. The envelope ALWAYS has that key, so those tests passed whether or not the tool handled the error at all. They now assert the error's value. 3. Missing optional dependencies reported as failures. bs4/playwright absence now skips with the reason naming what is missing, per CONTRIBUTING, rather than failing. PRODUCTION FIX (not a test change): a pipeline could not read a file at all. `_handle_file_operation` copied the step's routing action over the caller's parameter, so `action: file` + `parameters.action: read` sent action="file" to FileSystemTool, which failed with "Unknown filesystem action: file". The metadata-routed branch in the same function already guarded against this; the second branch did not. Now consistent. tests/test_filesystem_step_action.py covers it and was confirmed to fail 3/4 without the fix. Also found while fixing pipeline YAML: passing structured data between steps needs `| tojson`, or a dict is interpolated with Python's repr and the validator rejects it as "not JSON or CSV". Documented inline where it bites. Un-shadowed a test: two methods in TestPipelineTestProvider shared a name, so the first (which checks the constructor and model registry) never ran. Renamed the second to what it actually tests. This immediately exposed that PipelineTestModel cannot be constructed at all -- filed separately rather than half-fixed here; reverting my partial kwarg fixes kept this change focused. tests/integration/test_tools_real_world.py: 17 failed/2 passed -> 15 passed, 4 skipped. Whole integration selection: 29 failed/12 passed -> 12 failed/28 passed. Blocking gate 386 -> 390. Collection warnings still 0; correctness lint clean including tests/integration, which CI does not currently lint. Co-Authored-By: Claude Opus 5 (1M context) --- .../control_systems/hybrid_control_system.py | 9 +- ...est_pipeline_integration_infrastructure.py | 12 +- .../test_research_assistant_with_report.py | 37 +++- tests/integration/test_tools_real_world.py | 207 ++++++++++++------ tests/test_filesystem_step_action.py | 102 +++++++++ 5 files changed, 286 insertions(+), 81 deletions(-) create mode 100644 tests/test_filesystem_step_action.py diff --git a/src/orchestrator/control_systems/hybrid_control_system.py b/src/orchestrator/control_systems/hybrid_control_system.py index 82039535..6d4db4c2 100644 --- a/src/orchestrator/control_systems/hybrid_control_system.py +++ b/src/orchestrator/control_systems/hybrid_control_system.py @@ -512,7 +512,14 @@ async def _handle_file_operation( if action_text in filesystem_actions and task.parameters: # Use UnifiedTemplateResolver for template resolution resolved_params = task.parameters.copy() - resolved_params["action"] = action_text + # Only supply the operation when the step did not state one. + # `file` names the TOOL, not an operation, so overwriting a + # caller's `action: read` with it sent action="file" to + # FileSystemTool and every such step failed with + # "Unknown filesystem action: file". The metadata-routed branch + # above already guards this; this branch did not. + if "action" not in resolved_params: + resolved_params["action"] = action_text # Prepare template context using the unified system template_context = self._prepare_template_context(context) diff --git a/tests/integration/test_pipeline_integration_infrastructure.py b/tests/integration/test_pipeline_integration_infrastructure.py index 4cac1859..544c2633 100644 --- a/tests/integration/test_pipeline_integration_infrastructure.py +++ b/tests/integration/test_pipeline_integration_infrastructure.py @@ -332,12 +332,16 @@ async def test_pipeline_test_provider_get_model(self): assert stats['model_usage']["pipeline-test-model"]['requests'] == 1 @pytest.mark.asyncio - async def test_pipeline_test_provider_initialization(self): - """Test provider initialization process.""" - + async def test_pipeline_test_provider_async_initialization(self): + """Test the async initialize() call. + + Renamed: this shared a name with the constructor test above, so it + silently replaced it and that test never ran. The two check different + things and both are wanted. + """ provider = PipelineTestProvider() await provider.initialize() - + assert provider.is_initialized is True def test_pipeline_test_provider_usage_statistics(self): diff --git a/tests/integration/test_research_assistant_with_report.py b/tests/integration/test_research_assistant_with_report.py index 0203d581..d5b806d3 100644 --- a/tests/integration/test_research_assistant_with_report.py +++ b/tests/integration/test_research_assistant_with_report.py @@ -115,12 +115,14 @@ async def test_report_generator_tool(self): recommendations=["Recommendation 1", "Recommendation 2"], quality_score=0.85) - # Verify report + # Verify report. Tools return {success, result, error}; the report + # itself is in `result` (see #433). assert result["success"] is True - assert "markdown" in result - assert result["word_count"] > 0 + data = result["result"] + assert "markdown" in data + assert data["word_count"] > 0 - markdown = result["markdown"] + markdown = data["markdown"] assert "# Test Research Report" in markdown assert "Finding 1" in markdown assert "Finding 2" in markdown @@ -170,10 +172,12 @@ async def test_pdf_compiler_tool(self): # Verify result if result["success"]: assert output_path.exists() - assert result["file_size"] > 0 + assert result["result"]["file_size"] > 0 else: - # PDF generation failed (likely pandoc not available) - assert "error" in result + # PDF generation failed (likely pandoc not available). + # `assert "error" in result` could never fail -- the envelope + # always carries that key. Assert the value instead. + assert result["error"], "a failed compile must say why" @pytest.mark.integration @pytest.mark.asyncio @@ -194,12 +198,23 @@ async def test_web_search_integration(self): # Perform real search result = await tool.execute(query="Python asyncio tutorial", max_results=5) - # Verify results - assert "results" in result - assert len(result["results"]) > 0 + # Tools return {success, result, error}; the payload is in `result`. + assert result["success"] is True, f"search failed: {result['error']}" + data = result["result"] + assert "results" in data + + if not data["results"]: + # No backend reachable (missing [web] extra, or no network). This + # test is about the shape of a result, so an unavailable search + # engine is a missing prerequisite rather than a defect. + pytest.skip( + f"web search returned nothing " + f"(total_results={data.get('total_results')}); " + f"no search backend available" + ) # Check result structure - first_result = result["results"][0] + first_result = data["results"][0] assert "snippet" in first_result assert "rank" in first_result assert "relevance" in first_result diff --git a/tests/integration/test_tools_real_world.py b/tests/integration/test_tools_real_world.py index 095ff39e..8a661464 100644 --- a/tests/integration/test_tools_real_world.py +++ b/tests/integration/test_tools_real_world.py @@ -26,6 +26,40 @@ pytestmark = pytest.mark.integration +def payload(response): + """Return the data half of a tool's response. + + Every tool returns the envelope ``{"success": bool, "result": Any, + "error": str | None}``. ``success`` and ``error`` are read from the + envelope directly; everything else -- ``stdout``, ``content``, ``items``, + ``markdown`` and so on -- lives one level down, in ``result``. + + These tests originally asserted against a flat shape that predates the + envelope, which is why they failed with ``KeyError`` on keys that did + exist (see #433). + """ + assert isinstance(response, dict), ( + f"tool returned {type(response).__name__}, expected the response envelope" + ) + assert "result" in response, ( + f"no 'result' key in tool response; got {sorted(response)}" + ) + return response["result"] + + +def skip_if_optional_dependency_missing(response): + """Skip when a tool could not run for lack of an optional package. + + A missing ``[web]`` extra is a statement about this environment, not a + defect in the tool, so it must skip with a reason rather than fail -- + the same rule the rest of the suite follows. + """ + error = str(response.get("error") or "") + lowered = error.lower() + if "no module named" in lowered or "playwright is required" in lowered: + pytest.skip(f"optional dependency missing: {error}") + + @pytest.fixture(scope="module") def model_registry(): """Initialize real models for testing.""" @@ -65,37 +99,38 @@ async def test_scrape_real_website(self, browser_tool): # Use example.com - it's stable and simple result = await browser_tool.execute(url="https://example.com", action="scrape") - # Check if there was an error - if "error" in result: + skip_if_optional_dependency_missing(result) + # `"error" in result` is always true -- the envelope always carries the + # key, with None on success. The error's *value* is the signal. + if result["error"]: pytest.fail(f"Scraping failed: {result['error']}") # Should have scraped content - assert "url" in result - assert result["url"] == "https://example.com" - assert "title" in result - assert "Example Domain" in result["title"] + data = payload(result) + assert data["url"] == "https://example.com" + assert "Example Domain" in data["title"] # Check for text content (if extracted) - if "text" in result: - assert "Example Domain" in result["text"] - assert "More information" in result["text"] + if "text" in data: + assert "Example Domain" in data["text"] + assert "More information" in data["text"] # Check for other metadata - if "status_code" in result: - assert result["status_code"] == 200 + if "status_code" in data: + assert data["status_code"] == 200 @pytest.mark.asyncio async def test_verify_real_website(self, browser_tool): """Test verifying a real website.""" result = await browser_tool.execute(url="https://example.com", action="verify") - # Check if there was an error - if "error" in result: + skip_if_optional_dependency_missing(result) + if result["error"]: pytest.fail(f"Verification failed: {result['error']}") - assert "url" in result - assert "accessible" in result - assert result["accessible"] is True + data = payload(result) + assert "url" in data + assert data["accessible"] is True @pytest.mark.asyncio async def test_invalid_url_handling(self, browser_tool): @@ -104,9 +139,15 @@ async def test_invalid_url_handling(self, browser_tool): url="https://this-domain-definitely-does-not-exist-12345.com", action="scrape") - # Should have an error for invalid URL - assert "error" in result - assert "url" in result + # Skip first: a missing bs4 also produces an error, which would make + # this pass while proving nothing about URL handling. + skip_if_optional_dependency_missing(result) + + # `assert "error" in result` used to stand here. The envelope always + # has that key, so the assertion could never fail -- it passed whether + # or not the tool handled the bad URL at all. Assert the value. + assert result["success"] is False + assert result["error"], "an unreachable domain must report an error" @pytest.mark.asyncio @pytest.mark.timeout(240) # Increased timeout to allow for playwright installation @@ -122,7 +163,8 @@ async def test_scrape_with_javascript(self, browser_tool): elapsed = time.time() - start_time # The tool should handle playwright installation automatically - if "error" in result: + skip_if_optional_dependency_missing(result) + if result["error"]: # Get more diagnostic info error_msg = result["error"] @@ -153,8 +195,9 @@ async def test_scrape_with_javascript(self, browser_tool): pytest.fail(f"JS scraping failed after {elapsed:.1f}s: {error_msg}") # Should have scraped content - assert "url" in result - assert "title" in result + data = payload(result) + assert "url" in data + assert "title" in data class TestWebSearchTool: @@ -172,22 +215,22 @@ async def test_real_web_search(self, search_tool): query="Python programming language official documentation", max_results=5 ) - # WebSearchTool returns different format - no "success" key - assert "results" in result - assert "query" in result - assert result["query"] == "Python programming language official documentation" + data = payload(result) + assert "results" in data + assert data["query"] == "Python programming language official documentation" # Check if we got results (may be empty if search fails) - if result.get("error"): - # Search failed, but should still have proper structure - assert result["total_results"] == 0 + if result["error"] or not data["results"]: + # Search failed or returned nothing -- it must still be + # structurally coherent rather than claiming results it lacks. + assert data["total_results"] == 0 else: # Should have some results - assert len(result["results"]) > 0 - assert len(result["results"]) <= 5 + assert len(data["results"]) > 0 + assert len(data["results"]) <= 5 # Check result structure - for item in result["results"]: + for item in data["results"]: assert "title" in item assert "url" in item assert "snippet" in item @@ -199,9 +242,15 @@ async def test_empty_query_handling(self, search_tool): """Test handling of empty search query.""" result = await search_tool.execute(query="", max_results=5) - # Should have error for empty query - assert "error" in result - assert result["total_results"] == 0 + # The tool rejects the query outright rather than inventing an empty + # result set, so there is no payload to inspect -- `result` is None. + # The old `assert "error" in result` could never fail, because the + # envelope always carries that key. + assert result["success"] is False + assert "no query" in str(result["error"]).lower() + assert result["result"] is None, ( + "a rejected query must not come back with a fabricated payload" + ) class TestTerminalTool: @@ -218,9 +267,11 @@ async def test_simple_command_execution(self, terminal_tool): # Test echo command result = await terminal_tool.execute(command="echo 'Hello from terminal tool'") + # `success` is the envelope's own key and tracks the command's exit + # status; the stream contents live in the payload. assert result["success"] is True - assert result["stdout"].strip() == "Hello from terminal tool" - assert result["return_code"] == 0 + assert payload(result)["stdout"].strip() == "Hello from terminal tool" + assert payload(result)["return_code"] == 0 @pytest.mark.asyncio async def test_command_with_error(self, terminal_tool): @@ -228,8 +279,8 @@ async def test_command_with_error(self, terminal_tool): result = await terminal_tool.execute(command="ls /nonexistent/directory/path") assert result["success"] is False - assert result["return_code"] != 0 - assert result["stderr"] != "" # Should have error message + assert payload(result)["return_code"] != 0 + assert payload(result)["stderr"] != "" # Should have error message @pytest.mark.asyncio async def test_command_timeout(self, terminal_tool): @@ -251,7 +302,7 @@ async def test_working_directory(self, terminal_tool, temp_workspace): ) assert result["success"] is True - assert "test.txt" in result["stdout"] + assert "test.txt" in payload(result)["stdout"] class TestFileSystemTool: @@ -280,7 +331,7 @@ async def test_file_operations(self, fs_tool, temp_workspace): read_result = await fs_tool.execute(action="read", path=str(test_file)) assert read_result["success"] is True - assert read_result["content"] == test_content + assert payload(read_result)["content"] == test_content # Test delete delete_result = await fs_tool.execute(action="delete", path=str(test_file)) @@ -311,8 +362,9 @@ async def test_directory_operations(self, fs_tool, temp_workspace): list_result = await fs_tool.execute(action="list", path=str(test_dir)) assert list_result["success"] is True - assert len(list_result["items"]) == 3 # dummy.txt, file1.txt, file2.txt - file_names = [item["name"] for item in list_result["items"]] + items = payload(list_result)["items"] + assert len(items) == 3 # dummy.txt, file1.txt, file2.txt + file_names = [item["name"] for item in items] assert "file1.txt" in file_names assert "file2.txt" in file_names assert "dummy.txt" in file_names @@ -323,7 +375,8 @@ async def test_file_not_found(self, fs_tool): result = await fs_tool.execute(action="read", path="/nonexistent/file/path.txt") assert result["success"] is False - assert "error" in result + # Was `assert "error" in result`, which the envelope makes always true. + assert result["error"], "a missing file must report why it failed" class TestDataProcessingTool: @@ -356,11 +409,13 @@ async def test_json_processing(self, data_tool, temp_workspace): ) assert result["success"] is True - assert "result" in result - assert result["original_count"] == 3 - assert result["filtered_count"] == 1 - assert len(result["result"]) == 1 - assert result["result"][0]["name"] == "Alice" + data = payload(result) + assert data["original_count"] == 3 + assert data["filtered_count"] == 1 + # The filtered rows are under `data`, not directly under the envelope's + # `result` -- the old assertion happened to read the envelope key. + assert len(data["data"]) == 1 + assert data["data"][0]["name"] == "Alice" @pytest.mark.asyncio async def test_csv_processing(self, data_tool, temp_workspace): @@ -380,11 +435,11 @@ async def test_csv_processing(self, data_tool, temp_workspace): ) assert result["success"] is True - assert "result" in result - assert result["target_format"] == "json" + data = payload(result) + assert data["target_format"] == "json" # Parse the JSON result - converted_data = json.loads(result["result"]) + converted_data = json.loads(data["data"]) assert len(converted_data) == 3 assert converted_data[0]["name"] == "Alice" assert converted_data[1]["age"] == "25" # Note: CSV values are strings @@ -492,10 +547,11 @@ async def test_markdown_report_generation(self, report_tool, temp_workspace): output_path=str(temp_workspace / "report.md")) assert result["success"] is True - assert "markdown" in result + data = payload(result) + assert "markdown" in data # Verify content structure - content = result["markdown"] + content = data["markdown"] assert "# AI Applications Report" in content assert "## Executive Summary" in content assert "## Search Results" in content @@ -543,10 +599,10 @@ async def test_pdf_generation(self, pdf_tool, temp_workspace): # Pandoc is installed assert output_path.exists() assert output_path.stat().st_size > 0 - assert result["file_size"] > 0 + assert payload(result)["file_size"] > 0 else: # Pandoc not installed - should fail gracefully - assert "pandoc" in result["error"].lower() + assert "pandoc" in str(result["error"]).lower() class TestToolIntegration: @@ -619,6 +675,12 @@ async def test_web_scraping_pipeline(self, orchestrator, temp_workspace): search_count: "{{search_web.total_results}}" """ + # This pipeline needs a real web-search backend AND a real model for + # the `generate` step. Without the [web] extra the search step fails + # and the whole pipeline aborts, which says nothing about the + # orchestration being tested. + pytest.importorskip("bs4", reason="web scraping needs the [web] extra") + # Execute pipeline context = { "topic": "artificial intelligence", @@ -627,7 +689,7 @@ async def test_web_scraping_pipeline(self, orchestrator, temp_workspace): import time start_time = time.time() - + try: result = await orchestrator.execute_yaml( yaml_content=yaml_content, context=context @@ -738,7 +800,11 @@ async def test_file_processing_pipeline(self, orchestrator, temp_workspace): action: process parameters: action: transform - data: "{{read_data.content}}" + # A step's output is the {success, result, error} envelope, so the + # payload is reached through `.result`. `{{read_data.content}}` silently + # rendered as the literal placeholder text, which then reached + # json.loads() and failed there instead -- see #153. + data: "{{read_data.result.content}}" transform_spec: total_price: "sum(item['price'] for item in json.loads(data)['items'])" item_count: "len(json.loads(data)['items'])" @@ -748,7 +814,10 @@ async def test_file_processing_pipeline(self, orchestrator, temp_workspace): - id: validate_results action: validate parameters: - data: "{{process_data.processed_data}}" + # `| tojson` is required: without it a dict is interpolated with + # Python's repr (single quotes), which the validator rejects as + # "not JSON or CSV". + data: "{{ process_data.processed_data | tojson }}" schema: type: object properties: @@ -766,11 +835,11 @@ async def test_file_processing_pipeline(self, orchestrator, temp_workspace): parameters: action: write path: "{{output_file}}" - content: "{{process_data.processed_data}}" + content: "{{ process_data.processed_data | tojson }}" depends_on: [validate_results] outputs: - validation_passed: "{{validate_results.valid}}" + validation_passed: "{{validate_results.result.valid}}" total_price: "{{process_data.processed_data.total_price}}" """ @@ -786,12 +855,20 @@ async def test_file_processing_pipeline(self, orchestrator, temp_workspace): # Verify results # Check if result has the new structure with steps/outputs - if "steps" in result: - assert result["steps"]["validate_results"]["valid"] is True - else: - assert result["validate_results"]["valid"] is True + step = ( + result["steps"]["validate_results"] + if "steps" in result + else result["validate_results"] + ) + assert step["success"] is True, f"validation step failed: {step['error']}" + assert payload(step)["valid"] is True assert (temp_workspace / "output_data.json").exists() + # The saved file must be real JSON, not a Python repr. + saved = json.loads((temp_workspace / "output_data.json").read_text()) + assert saved["item_count"] == 3 + assert saved["total_price"] == pytest.approx(51.25) + if __name__ == "__main__": pytest.main([__file__, "-v", "-s"]) diff --git a/tests/test_filesystem_step_action.py b/tests/test_filesystem_step_action.py new file mode 100644 index 00000000..c60a1739 --- /dev/null +++ b/tests/test_filesystem_step_action.py @@ -0,0 +1,102 @@ +"""A pipeline step must keep the tool operation it asked for. + +A step carries two different `action` values that are easy to confuse: + +- the **step** action (`action: file`) selects which tool runs; +- the **parameter** action (`parameters.action: read`) selects what that tool + does. + +The control system used to copy the first over the second, so `FileSystemTool` +received `action="file"` -- not one of its operations -- and every such step +failed with "Unknown filesystem action: file". Reading a file from a pipeline +was impossible. + +Real files, real tool, no mocks. +""" + +import json + +import pytest + +from orchestrator.control_systems.hybrid_control_system import HybridControlSystem +from orchestrator.core.task import Task +from orchestrator.models.model_registry import ModelRegistry + +pytestmark = pytest.mark.unit + + +@pytest.fixture +def control_system(): + """A control system with an empty registry -- no model is needed here.""" + return HybridControlSystem(model_registry=ModelRegistry()) + + +def _task(step_action, **parameters): + """Build a task. `step_action` is the routing action, kwargs the params. + + Named `step_action` rather than `action` precisely because the two + collide -- which is the confusion this whole module exists to pin down. + """ + return Task(id="step", name="step", action=step_action, parameters=parameters) + + +@pytest.mark.asyncio +async def test_read_operation_survives_a_file_step_action(control_system, tmp_path): + """The regression: `action: file` must not overwrite `action: read`.""" + target = tmp_path / "data.json" + target.write_text(json.dumps({"items": [1, 2, 3]})) + + result = await control_system._handle_file_operation( + _task("file", action="read", path=str(target)), {} + ) + + assert result["success"] is True, f"read failed: {result['error']}" + assert result["result"]["action"] == "read", ( + "the step's routing action overwrote the requested operation" + ) + assert json.loads(result["result"]["content"]) == {"items": [1, 2, 3]} + + +@pytest.mark.asyncio +async def test_write_operation_survives_a_file_step_action(control_system, tmp_path): + target = tmp_path / "out.txt" + + result = await control_system._handle_file_operation( + _task("file", action="write", path=str(target), content="written"), {} + ) + + assert result["success"] is True, f"write failed: {result['error']}" + assert target.read_text() == "written" + + +@pytest.mark.asyncio +async def test_list_operation_survives_a_file_step_action(control_system, tmp_path): + (tmp_path / "a.txt").write_text("a") + (tmp_path / "b.txt").write_text("b") + + result = await control_system._handle_file_operation( + _task("file", action="list", path=str(tmp_path)), {} + ) + + assert result["success"] is True, f"list failed: {result['error']}" + assert {item["name"] for item in result["result"]["items"]} == {"a.txt", "b.txt"} + + +@pytest.mark.asyncio +async def test_step_action_still_supplies_the_operation_when_absent( + control_system, tmp_path +): + """The fix must not break the case it was originally written for. + + With no `parameters.action`, the step action IS the operation, and that + behaviour has to survive. + """ + target = tmp_path / "data.txt" + target.write_text("hello") + + result = await control_system._handle_file_operation( + _task("read", path=str(target)), {} + ) + + assert result["success"] is True, f"read failed: {result['error']}" + assert result["result"]["content"] == "hello"