test: Assert genuine memory retrieval in the rehydration smoke - #622
Open
AlexanderZ-Band wants to merge 3 commits into
Open
test: Assert genuine memory retrieval in the rehydration smoke#622AlexanderZ-Band wants to merge 3 commits into
AlexanderZ-Band wants to merge 3 commits into
Conversation
_report_tool_result built its tool_result payload with only output and tool_call_id, never name -- unlike _report_tool_call, which already carries it. band.converters.parsing.parse_tool_result requires name and silently drops (logs, doesn't raise) any event missing it, so every OpenCode tool_result has been unparseable by the shared decoder since day one, for every tool, not just one framework's memory calls. This went unnoticed because nothing has asserted OpenCode tool_result *content* until now: the sibling file-transfer test that also reads tool_results is gated behind Capability.FILES/ff_file_transfer, which is off on every SaaS/CI environment, so it never actually exercised this path against real opencode either. Confirmed live: adding a tool_result content assertion to the memory rehydration smoke (next commit) reproduced the warning and zero matched results before this fix, and passed after it. Thread tool_name through to _report_tool_result the same way _report_tool_call already receives it, and lock the name into the existing tool-events unit test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8z6z1muD8yg9Y3pre92ec
… smoke test_memory_survives_adapter_rehydration proved a rebooted adapter recalled its stored memory only by checking the reply echoed a marker string -- a marker baked into the test's own instruction, so a model could satisfy this by restating its prompt without get_memory ever returning the right record. mem.calls only proves the tools fired (tool_call events, args only); mem.stored proves a record exists via a direct memories-API query, independent of the turn's own tool_result. Neither proves what band_get_memory's tool_result actually carried back during the turn. Assert on the tool result directly via capture.tool_results(), the same mechanism (and pattern) already proven for FileTool.READ in the file/image round-trip tests two tests below in this file. Verified live end-to-end: a deliberately wrong expected string made the assertion fail with the real ToolResults.assert_succeeded error and the actual observed payload; reverting to the correct marker passed. Confirmed against anthropic, opencode (see prior commit), and gemini. INT-1226 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8z6z1muD8yg9Y3pre92ec
_report_tool_result wrapped a failed tool's error text inside output
(e.g. {"error": "..."}), but never set ToolEventKey.IS_ERROR itself --
parse_tool_result defaults is_error=False when the key is absent, so a
genuinely failed OpenCode tool call was indistinguishable from a
successful one to any consumer gating on that flag, including this
same diff's own ToolResults.assert_succeeded. Same root cause as the
prior commit's missing-name bug: a mandatory ToolEventKey field silently
absent from this one adapter's tool_result payload.
Found by /code-review while verifying the memory-rehydration fix; fixing
it here since it's the same function this branch already touches.
Extends event_tool_part (the shared test fixture) with an error status
branch instead of hand-rolling the raw event, and adds a regression test
plus a name assertion on the pre-existing MCP-prefix-canonicalization
test's tool_result (that test already drives a tool to completion,
exercising the exact method the prior commit changed, but never checked
its result's name).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W8z6z1muD8yg9Y3pre92ec
AlexanderZ-Band
changed the title
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.
Summary
test_memory_survives_adapter_rehydrationonly checked that the reply echoed a marker string — a marker present in the test's own prompt, so it passes even ifget_memoryreturns nothing or the wrong record.mem.callsproves only that tools fired;mem.storedqueries the memory store directly, independent of the turn's tool_result. Neither proves whatband_get_memory's tool_result actually carried.results.assert_succeeded(MemoryTool.GET.value, output_contains=marker)viacapture.tool_results(), matching the existingFileTool.READassertion pattern two tests below in the same file.OpencodeAdapter._report_tool_resultnever includedToolEventKey.NAMEorToolEventKey.IS_ERRORin itstool_resultpayload.parse_tool_resultrequiresnameand silently drops any event missing it;is_errordefaults toFalsewhen absent. Net effect: every OpenCode tool_result was unparseable, and a failed tool call was indistinguishable from a successful one. Fixed by threadingtool_nameandis_errorthrough_report_tool_result, mirroring_report_tool_call's existing shape.Closes INT-1226.
Verification
output_contains=marker + "-WRONG"againstanthropicfails with the realToolResults.assert_succeedederror and the actual tool_result payload; reverted to the correct marker, passes.anthropic,opencode,geminiall pass against the real platform.tests/adapters/opencode/— 76 passed.tests/ --ignore=tests/integration/ --ignore=tests/e2e/— 5590 passed, 146 skipped, 0 failed.ruff check/ruff format --check/pyrefly check— clean.Not fixed here
ToolResults.assert_succeededfails on any same-named tool_result withis_error=Truein a turn, even if a different matching result succeeded and carried the expected content. Shared by everyassert_succeededcaller — needs its own review, not a drive-by here.src/band/adapters/letta.py::_report_execution_eventandsrc/band/integrations/crewai/reporting.py::EmitToolCallsReporterboth omitToolEventKey.TOOL_CALL_ID.Test plan
test_memory_survives_adapter_rehydration[anthropic],[opencode],[gemini]tests/adapters/opencode/— 76 passed🤖 Generated with Claude Code