Skip to content

fix(offchain): extract inner result field to match on-chain response shape #98

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DIvyaNautiyal07 merged 1 commit into from
Jul 3, 2026
Merged

fix(offchain): extract inner result field to match on-chain response shape #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/packages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"contract/valory/balance_tracker_fixed_price_token/0.1.0": "bafybeienpn5nb7mgiw5uitm5mjwcgc2xd7bzx5rxu2mqsvjziwvlg6poxa",
"contract/valory/balance_tracker_fixed_price_native/0.1.0": "bafybeifobvptvc7mkrztfij664dqqegl6qi32773zewvyuftes32faemw4",
"contract/valory/mech_marketplace/0.1.0": "bafybeieadbmhzndxe6jcfwujcpjrljpc5kfwffkd3zzpg75uxksufn4l3i",
"skill/valory/mech_interact_abci/0.1.0": "bafybeihhwzpa6x5ypth426tzd2zjsnys6jmjaab4zvk4edisetzktft754"
"skill/valory/mech_interact_abci/0.1.0": "bafybeic3ar66g5gwty33wsx37lilvwxddecqbj6ql5g4m6ago3yqodykju"
},
"third_party": {
"protocol/valory/acn_data_share/0.1.0": "bafybeieqisfr6lhaaesq37lk7vyaxza2h6nucshksgyo2djqfyf2saqo4i",
Expand Down
12 changes: 11 additions & 1 deletion packages/valory/skills/mech_interact_abci/behaviours/offchain_response.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,19 @@ def _parse(self, response: Any) -> _PollSnapshot:
return _PollSnapshot(status="processing")
status = str(payload.get("status", "")).lower()
if status == "ok":
# Match the on-chain `mech_response` ApiSpec (`response_key: result`):
# extract the inner `result` field so downstream (DecisionReceive
# 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__`.

@OjusWiZard OjusWiZard Jul 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

envelope = payload.get("response")
inner_result = (
envelope.get("result") if isinstance(envelope, dict) else envelope
)

@OjusWiZard OjusWiZard Jul 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = envelope

plus 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.

return _PollSnapshot(
status="ok",
result=self._serialise_result(payload.get("response")),
result=self._serialise_result(inner_result),
)
if status == "rejected":
return _PollSnapshot(
Expand Down
4 changes: 2 additions & 2 deletions packages/valory/skills/mech_interact_abci/skill.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fingerprint:
behaviours/mech_info.py: bafybeidajqcqn6mo2rparhopcunubvymrnyuuwxzx4xyviydcrwbjfrfou
behaviours/mech_version.py: bafybeihxptnxmqslzfbquioym55l7holeykdtsd3avxy737qrocb5mmebu
behaviours/offchain_request.py: bafybeiclfkjgt6bxsnvlkmprweglvesbmbz7l3ee73dn4ukuii6joqsvnq
behaviours/offchain_response.py: bafybeief3wtiqj6ohtbylkphuctv67cfshy5fyfsbr6pmgxrqaoxpomhx4
behaviours/offchain_response.py: bafybeifaeznjs6kz6wcusb6jvbkepdzdudtkblo2shzintvogc6rkym4au
behaviours/purchase_subcription.py: bafybeieet7du56scd2cltnuy7xlvvrapvk7ugmnv426k2xfnmim4wj5bfy
behaviours/request.py: bafybeig3tyl6pcdmlm7odp7oqrgijygwyflcmkqrbalnukz6zzmxtb2j4y
behaviours/response.py: bafybeift7qcgnehlphp4muqdo5harabo3jpw5ufb7egsf6bqnj4slhv2gq
Expand Down Expand Up @@ -44,7 +44,7 @@ fingerprint:
tests/behaviours/test_base.py: bafybeih5gzn23htrkovh4mrlycygbpjdc4smq27hgfptrlg7ekndybavjy
tests/behaviours/test_mech_version.py: bafybeidmjgq2cwqafq36j7irm4nf53ca6imkyvtbtetscltqwiu6ebb444
tests/behaviours/test_offchain_request_helpers.py: bafybeifinbb4eqdo2o6wpzrikgxocm6hhaujduuh7hy4h4nd5np32x5fpe
tests/behaviours/test_offchain_response_poller.py: bafybeibtsj2ol7pe2blt2oshqpv6oiq7wbumyfvji7nun5jwwedbbtlnxu
tests/behaviours/test_offchain_response_poller.py: bafybeic5odn4yqnbam4pbapglyszua76p2d5ogvzgthl4svjppoz5hnijm
tests/behaviours/test_purchase_subscription.py: bafybeih2nd26wx6dpvycl2xlewmgqirjtwwtu6ge3gu4dhpby2aoogml5u
tests/behaviours/test_request.py: bafybeifng4uhlb3j6fg3wriwt2rvhqh3smwvfo5ugzqjdyt36uua6t3k7y
tests/behaviours/test_response.py: bafybeibni7zx4m7n3wktmlxyab4olt4rnduqzhaltfl7n3exru3om5fymy
Expand Down
54 changes: 48 additions & 6 deletions packages/valory/skills/mech_interact_abci/tests/behaviours/test_offchain_response_poller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,18 +147,60 @@ def test_5xx_counter_resets_on_success_status(self) -> None:
assert snapshot.status == "ok"
assert snapshot.result == "answer"

def test_ok_status_returns_serialised_result(self) -> None:
"""A 200 with ``status="ok"`` surfaces the serialised result.
def test_ok_status_extracts_inner_result_field(self) -> None:
"""A 200 with ``status="ok"`` surfaces only the envelope's ``result``.

The real mech wraps the tool payload in a ``response`` envelope
(``{schema_version, requestId, result, tool, executed_at}``). To match
the on-chain ``mech_response`` ApiSpec (``response_key: result``), we
extract just the inner ``result`` field so ``DecisionReceive`` on
trader sees the same string shape as the on-chain branch.
"""
body = json.dumps(

@OjusWiZard OjusWiZard Jul 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
"status": "ok",
"response": {
"schema_version": "2.0",
"requestId": "42",
"result": '{"p_yes": 0.6, "p_no": 0.4}',
"tool": "prediction-online",
"executed_at": "2026-07-02T14:24:27Z",
},
}
).encode()
stub = _StubBehaviour(http_responses=[_http_response(200, body)])
poller = OffchainResponsePoller(stub) # type: ignore[arg-type]
snapshot = _drive(poller._poll_until_terminal("https://m", "42"))
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(

@OjusWiZard OjusWiZard Jul 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self,
) -> None:
"""Tool-failure envelopes carry a non-JSON ``result`` (``"Invalid response"``).

Non-string ``response`` values are JSON-serialised for the
downstream consumer's wire shape.
The downstream ``_get_decision`` in trader parses ``mech_response.result``
with ``json.loads`` and catches ``JSONDecodeError`` to safely skip. If we
emitted the full envelope here, that catch would miss and the trader
would blow up in ``PredictionResponse(**dict)`` with ``KeyError('p_yes')``.
"""
body = json.dumps({"status": "ok", "response": {"answer": 1}}).encode()
body = json.dumps(
{
"status": "ok",
"response": {
"schema_version": "2.0",
"requestId": "42",
"result": "Invalid response",
"tool": "prediction-online",
"executed_at": "2026-07-02T14:24:27Z",
},
}
).encode()
stub = _StubBehaviour(http_responses=[_http_response(200, body)])
poller = OffchainResponsePoller(stub) # type: ignore[arg-type]
snapshot = _drive(poller._poll_until_terminal("https://m", "42"))
assert snapshot.status == "ok"
assert json.loads(snapshot.result or "{}") == {"answer": 1}
assert snapshot.result == "Invalid response"

def test_rejected_status_surfaces_reason(self) -> None:
"""A 200 with ``status="rejected"`` carries the reason as ``error``."""
Expand Down
Loading