Fix integration tests for the tool response envelope (#433); fix filesystem action clobbering - #434
Merged
Conversation
…bering 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #433.
You confirmed the
{success, result, error}envelope is the intended contract, so these tests were asserting a shape that no longer exists.Three causes, not one
The 29 failures looked uniform but weren't:
payload()helper unwrapsresult;success/errorstay on the envelopeassert "error" in resultappeared 4×; the envelope always has that keybs4/playwrightabsence now skips with the reasonBefore changing anything I probed
TerminalToolfor ok / failing / missing-binary / timeout to establish that envelopesuccesstracks the command's exit status, not merely "the tool ran" — soresult["success"]assertions were already correct and only payload keys move.Production fix included
A pipeline could not read a file at all.
_handle_file_operationcopied the step's routing action over the caller's parameter:FileSystemToolreceivedaction="file"and failed withUnknown filesystem action: file. The metadata-routed branch in the same function already guarded this; the second branch didn't. Now consistent.tests/test_filesystem_step_action.pycovers read/write/list plus the no-parameter case the original behaviour existed for. Verified it fails 3/4 without the fix.Also
TestPipelineTestProvidershared a name, so the first (constructor + model registry checks) never ran. Renamed the second to what it actually tests. This immediately exposed thatPipelineTestModelcannot be constructed at all — filed separately rather than half-fixed here.| tojson, or a dict is interpolated with Python's repr and the validator rejects it as "not JSON or CSV".Numbers
test_tools_real_world.pyEvery skip names its missing prerequisite. Correctness lint clean including
tests/integration, which CI does not currently lint.Not fixed here
The remaining 12 failures / 9 errors are unrelated to the envelope and each other: YAML pipeline compilation (8), missing test fixture files, and tests needing registered models that aren't available without API keys.
🤖 Generated with Claude Code