fix(offchain): warn on missing 'result' key in ok envelope; test coverage#100
Closed
DIvyaNautiyal07 wants to merge 1 commit into
Closed
fix(offchain): warn on missing 'result' key in ok envelope; test coverage#100DIvyaNautiyal07 wants to merge 1 commit into
DIvyaNautiyal07 wants to merge 1 commit into
Conversation
…rage Two follow-ups to review nits on #98: 1. Missing 'result' key silently yielded None: if the mech's response schema drifts (renamed field, truncated payload, new nesting), envelope.get("result") returned None, snapshot was constructed with result=None, and _apply_snapshot set target.result=None, target.error="Unknown". That is indistinguishable from a routine tool failure and lands in mech_responses as a fake failed prediction. Now logs a warning citing the observed envelope keys so the drift is visible. Every other structural failure in _parse either returns 'processing' or emits a warning; this one now matches that convention. 2. _serialise_result JSON-coercion branch was uncovered: the removed test used {"answer": 1} (dict) as the response value, exercising the non-string fallback (json.dumps). The two new tests on #98 feed string results, so nothing pinned the coercion branch. Added test_ok_status_serialises_non_string_inner_result_to_json which feeds result={"p_yes": 0.6, "p_no": 0.4} and asserts the snapshot result is the JSON-encoded string. Also added test_ok_status_envelope_missing_result_key_warns which asserts the schema-drift warning fires. Stub logger now captures warnings into a list so tests can assert on them. Co-Authored-By: Claude Opus 4.7 (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
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
Two follow-ups to review nits on #98.
1. Missing
resultkey silently yieldedNoneIf the mech's response schema drifts (renamed field, truncated payload, added nesting),
envelope.get("result")returnedNone,_PollSnapshotwas constructed withresult=None, and_apply_snapshotsettarget.result=None,target.error="Unknown". That is indistinguishable from a routine tool failure and lands inmech_responsesas a fake failed prediction.Now logs a warning citing the observed envelope keys so the drift is visible. Every other structural failure in
_parseeither returnsprocessingor emits a warning; this one now matches the convention.2. Restored
_serialise_resultJSON-coercion coverageThe removed test used
{"answer": 1}(dict) as the response value, exercising the non-string fallback (json.dumps). The two new tests on #98 feed string results, so nothing pinned the coercion branch.Added:
test_ok_status_serialises_non_string_inner_result_to_json— feedsresult={"p_yes": 0.6, "p_no": 0.4}and asserts the snapshot result is the JSON-encoded string.test_ok_status_envelope_missing_result_key_warns— asserts the schema-drift warning fires. Stub logger now captures warnings into a list so tests can assert on them.Verification
test_offchain_response_poller.py(12 pre-existing + 2 new)tomte tox -p -e black-check -e isort-check -e flake8 -e mypy -e pylint -e darglintall greenautonomy packages lock --checkgreenFollow-up to
Review comments on #98 (already merged into
feat/offchain-epic).🤖 Generated with Claude Code