fix(offchain): extract inner result field to match on-chain response shape#98
Conversation
…shape
The offchain poller was serialising the entire response envelope
(schema_version, requestId, result, tool, executed_at) as
mech_response.result, but downstream DecisionReceive._get_decision on
trader expects the same string shape the on-chain mech_response ApiSpec
produces (response_key: result — a bare string that's either JSON of a
prediction dict on success, or "Invalid response" on tool failure).
Under the old shape, tool-failure envelopes JSON-parsed cleanly into a
dict that DidNot have p_yes, hitting KeyError in
PredictionResponse.__init__ and crashing the trader agent. Successful
predictions also would have gone through PredictionResponse(**envelope),
missing p_yes/p_no because those fields live one nesting level deeper.
Aligning the offchain path with the on-chain ApiSpec extraction fixes
both cases: JSONDecodeError on "Invalid response" is caught upstream and
the FSM cleanly degrades, and successful predictions round-trip
correctly.
Reproduced live: trader on branch 402 (PR #1002) crashed with
KeyError('p_yes') after receiving a tool-failed offchain response from a
locally-run mech (service 2692 on Gnosis).
OjusWiZard
left a comment
There was a problem hiding this comment.
Multi-perspective review (correctness, silent failures, tests, types, comments, simplification). 0 critical, fix is correct and well-tested — the extraction matches the on-chain mech_response ApiSpec (response_key: result, verified against skill.yaml), the isinstance guard is type-sound, Optional[str] shapes are consistent with MechInteractionResponse.result end-to-end, and all 12 poller tests pass. Approving; inline comments are non-blocking hardening.
One finding that can't be inline-commented because the code is outside this diff: _run_offchain_response_cycle (behaviours/response.py:626-635) lacks the on-chain path's result is None error log (_process_responses does logger.error(...) at response.py:609-611; the off-chain cycle only INFO-dumps the serialized list, and async_act returns before ever reaching _process_responses when use_offchain=true). Log-based alerting keyed on ERROR will never fire for off-chain result failures. Pre-existing on feat/offchain-epic, but worth a follow-up since this PR's extraction adds a new way to reach result=None.
| envelope = payload.get("response") | ||
| inner_result = ( | ||
| envelope.get("result") if isinstance(envelope, dict) else envelope | ||
| ) |
There was a problem hiding this comment.
Non-blocking: if status == "ok" but the envelope dict has no result key (schema drift, truncated payload), this silently yields inner_result = None → _apply_snapshot records result=None, error="Unknown" with zero logging. Every other malformed-response branch in _parse logs (404 → error, 5xx → warning, non-JSON body → warning); this new extraction is the only structural failure that stays invisible. Suggest:
if isinstance(envelope, dict):
inner_result = envelope.get("result")
if inner_result is None:
self._logger.warning(
f"Offchain 'ok' response envelope missing 'result': {envelope!r}"
)
else:
inner_result = envelopeplus a test pinning the missing-result shape (currently untested). Downstream can't crash on it (MechInteractionResponse.result is Optional[str]), so purely an observability gap.
| # on trader) sees the same string shape as the on-chain branch. | ||
| # Passing the full envelope here broke `_get_decision` — on tool | ||
| # failure the envelope JSON-parses but has no `p_yes`, hitting | ||
| # KeyError in `PredictionResponse.__init__`. |
There was a problem hiding this comment.
Nit (comment-rot risk): the DecisionReceive/_get_decision/PredictionResponse.__init__/KeyError('p_yes') references describe trader-repo internals that nothing in this repo can verify or CI-pin — if trader refactors _get_decision, this goes stale silently. The ApiSpec half of the comment is verifiable here and is the load-bearing part; consider softening the trader specifics to "e.g. trader's DecisionReceive (as of trader#1002)" so a future reader knows it's a snapshot, not a contract. Also: the else envelope fallback branch (bare-string legacy probe, exercised by test_5xx_counter_resets_on_success_status) isn't mentioned in the comment at all — one line saying why the non-dict passthrough exists would help.
| extract just the inner ``result`` field so ``DecisionReceive`` on | ||
| trader sees the same string shape as the on-chain branch. | ||
| """ | ||
| body = json.dumps( |
There was a problem hiding this comment.
Nit: this envelope dict is duplicated near-verbatim in the test below (only the result value differs). A module-level helper next to _http_response would collapse ~40 lines:
def _ok_envelope_body(result: str) -> bytes:
"""Build a 200 ``status=ok`` body wrapping ``result`` in the mech envelope."""
return json.dumps(
{
"status": "ok",
"response": {
"schema_version": "2.0",
"requestId": "42",
"result": result,
"tool": "prediction-online",
"executed_at": "2026-07-02T14:24:27Z",
},
}
).encode()Keep the two tests separate (they pin distinct regressions). Separately: the deleted test_ok_status_returns_serialised_result pinned json.dumps coercion of non-string results; no test now covers a structured inner result ({"result": {"p_yes": 0.6}} → JSON string via _serialise_result) post-extraction — worth carrying that case forward.
| assert snapshot.status == "ok" | ||
| assert snapshot.result == '{"p_yes": 0.6, "p_no": 0.4}' | ||
|
|
||
| def test_ok_status_passes_invalid_response_string_through_unwrapped( |
There was a problem hiding this comment.
Nit: "passes ... through unwrapped" is easy to misread as exercising the else envelope (non-dict) fallback branch — but this fixture goes through the same envelope.get("result") dict-extraction as the test above, differing only in the result value (non-JSON string). "Unwrapped" here really means _serialise_result doesn't re-json.dumps an already-string value. Something like test_ok_status_extracts_non_json_result_string_verbatim would name the actual branch under test.
bennyjo
left a comment
There was a problem hiding this comment.
Verified the extraction against the mech server wire shape (task_execution/behaviours.py stores result at response.result for both success and tool-failure); matches the on-chain response_key: result contract. Scope holds — on-chain path untouched. LGTM.
Summary
{schema_version, requestId, result, tool, executed_at}) asmech_response.result, but downstreamDecisionReceive._get_decisionexpects the same string the on-chainmech_responseApiSpec produces (response_key: result).p_yes, crashing trader agents inPredictionResponse(**dict)withKeyError('p_yes'). Successful predictions also missp_yes/p_nobecause they live one nesting level deeper.payload["response"]["result"](with aisinstance(dict)guard so a bare-stringresponsestill passes through for legacy probes).Repro
Ran mech-interact off-chain path end-to-end against a locally-run mech (
mech runfrom mech repo on Gnosis service 2692), driven by trader on branch402withMECH_MARKETPLACE_CONFIG.use_offchain=true./send_signed_requests, mech verified + executed the tool"Invalid response"(dummy API key)/fetch_offchain_inforeceived the full envelopeDecisionReceiveBehaviourcrashed inPredictionResponse.__init__atbets.py:118withKeyError('p_yes')On-chain comparison
The on-chain branch's
mech_responsemodel has:so
mech_response.resultis the bareresultfield (a JSON string on success,\"Invalid response\"on failure). This PR aligns the offchain shape with that contract — the docstring of_run_offchain_response_cyclealready promises exactly this ("Downstream behaviour reads the same list shape the on-chain branch produces") but the implementation was passing the envelope through.What was actually a regression risk
The previous test
test_ok_status_returns_serialised_resultcodified the current (buggy) shape by assertingjson.loads(snapshot.result) == {\"answer\": 1}. Replaced with two more realistic tests that mirror the actual mech wire format (envelope with innerresultfield):test_ok_status_extracts_inner_result_field— success case, enveloperesultis a JSON string of a prediction dicttest_ok_status_passes_invalid_response_string_through_unwrapped— failure case, enveloperesultis the bare string\"Invalid response\"— pins the regression so a future change can't put it backTest plan
pytest packages/valory/skills/mech_interact_abci/tests/behaviours/test_offchain_response_poller.py— 12/12 passautonomy packages lock— hash refreshed forskill/valory/mech_interact_abciDecisionReceive → Event.MECH_INVALID_RESPONSEcleanly instead of crashingScope
Only touches the off-chain code path (
response.py:async_actguards this withif self.mech_marketplace_config.use_offchain is True). On-chain_process_responsespath is untouched — no risk to existing mech-interact consumers.🤖 Generated with Claude Code