fix(delivery): return the delivered content, in one shape (#250) by OjusWiZard · Pull Request #251 · valory-xyz/mech-client

OjusWiZard · 2026-08-11T19:53:23Z

Fixes #250.

The problem

delivery_results[request_id] had a different meaning depending on how the response arrived:

Path	What you got
On-chain	An IPFS URL string
Off-chain	The mech's raw envelope dict

So a caller could not write one handler for both.

The on-chain URL was also not usable as-is. A mech delivers by pinning a directory, so the hash in the Deliver event addresses the directory, not the answer — fetching it returns an HTML listing. The result file inside is named after the request ID in decimal, while the hex form used everywhere else in this client 404s.

The change

Both watchers now build the full result-file path, read it, and return a DeliveryResult carrying the parsed content plus the URL it came from. send_request splits that into two keys with the same shape on both paths:

delivery_results[request_id] — the parsed result file
delivery_urls[request_id] — the gateway URL it was read from (None for off-chain mechs answering inline)

mechx request prints the mech's answer, decoding the JSON-encoded result field when the payload has one, followed by the result-file URL.

Result files are read concurrently and off the event loop. A batch delivers one file per request, and reading them serially would stack a gateway timeout per request after the caller's --timeout budget is already spent.

Unreadable files degrade rather than fail: delivery_results is None and the URL is still reported, so the answer stays retrievable by hand.

Breaking change

Callers reading delivery_results as a URL must move to delivery_urls. Documented in the CHANGELOG, README, and docs/index.md.

Verification

790 unit tests pass
100% line coverage on all touched modules; request_cmd.py at 100% branch coverage
black, isort, flake8, mypy, pylint, darglint, bandit, and the copyright check all pass

🤖 Generated with Claude Code

`delivery_results[request_id]` was an IPFS URL string on the on-chain path and the mech's raw envelope on the off-chain one, so a caller could not write a single handler. Worse, the on-chain URL addressed the delivery *directory*, which serves an HTML listing rather than the answer: the result file inside is named after the request ID in decimal. Both watchers now build that full path, read the file, and return a `DeliveryResult` carrying the parsed content plus the URL it came from. `send_request` splits those into `delivery_results` and the new `delivery_urls`, identical in shape across both paths. `mechx request` prints the answer, decoding the JSON-encoded `result` field when the payload has one, followed by the result-file URL. The result files are read concurrently and off the event loop: a batch delivers one file per request, and reading them serially would stack a gateway timeout per request after the caller's wait budget is spent. Unreadable files degrade rather than fail — `delivery_results` is `None` and the URL is still reported, so the answer can be retrieved by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

bennyjo

One note on the offchain path, complementing Divya's threads.

… read Addresses review on #251. `send_request` returned `delivery_results` and `delivery_urls` as two parallel dicts that the CLI immediately re-paired by key, so the two were kept in lockstep by convention rather than by type. Collapse them into a single `deliveries` key holding the `DeliveryResult` the watchers already build: one key, one object, `.data` and `.url` on it. This is the moment for it, since the delivery contract is already documented as breaking in this release. `asyncio.gather` defaulted to `return_exceptions=False`, so a single raising result-file read propagated out of `watch()` and discarded the whole batch — including the transaction hash of requests already paid for. Collect exceptions instead and degrade that one request to `data=None`, leaving its URL to retrieve the answer by hand. The offchain watcher read its result file inline, blocking the polling loop for up to the gateway timeout per delivered response and serialising the very round-trips the on-chain path takes off the loop. Move it to `asyncio.to_thread` too, so the claim holds on both paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

bennyjo

Round-2 changes verified: the off-chain result-file read now runs through asyncio.to_thread so the polling loop no longer blocks, gather degrades a bad read to data=None with the URL kept instead of sinking the batch, and the single deliveries map of DeliveryResult objects removes the parallel-dict drift risk. The remaining response.text guard question lives in Divya's open thread and its blast radius is now contained on both paths.

Addresses the second review round on #251, plus a self-review pass. `send_request` returned `delivery_results` and `delivery_urls` as two parallel dicts that the CLI immediately re-paired by key, so content and its location were kept in step by convention rather than by type. They collapse into one `deliveries` key holding the `DeliveryResult` the watchers already build. Since the delivery contract is already breaking in this release, this is the moment to do it. An unexpected error reading a result file was degrading the delivery to `data=None` with no diagnostic at all. Both watchers now log it: the read failures `fetch_result_file` anticipates are already logged there, so anything reaching these handlers is unforeseen and worth the noise. The offchain watcher keeps retrying such a read for as long as there is timeout budget, rather than recording the failure after a single attempt and marking the request done. Only at timeout does it report the request with `data=None` and the URL to fetch the answer by hand — the same shape the on-chain path returns, without giving up the retries that shape would otherwise have cost. `fetch_result_file` takes the hex request ID for its log line: the URL carries only the decimal form, and the rest of the client keys off hex. Types: `RequestResult` spells out `send_request`'s four keys, and `JSONValue` replaces `Any` for decoded result-file content so consumers must narrow before use. `aea.common.JSONLike` does not fit — it is `Dict[str, ...]`, while a result file can be a list, a string, or null, and `[mypy-aea.*] ignore_missing_imports` would reduce it to `Any` here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`fetch_result_file` returns `None` for every HTTP failure rather than raising, so writing the delivery on a `None` read marked the request done after one attempt and the next poll skipped it. The retry added in the previous commit therefore only ever covered an unforeseen *raise* — not the 404 a freshly pinned file returns while the gateway catches up, which is the case retrying exists for. An unreadable read now leaves the request out of `results`, so the next cycle tries again, and the timeout backfill still reports it with its URL. A file that is genuinely absent consequently waits out the timeout instead of reporting straight away — the right trade for a watcher with a budget. The test that was meant to cover this drove `fetch_result_file` with a `RuntimeError` side effect, a path production never takes, so it passed against the broken behaviour. It now drives the `None` that the real function returns. Dropped `test_unreadable_result_file_keeps_url`, which would have polled for its full 60s timeout under the corrected semantics; the timeout test covers what it asserted. `RequestResult` becomes a discriminated union: `tx_hash` and `receipt` are set together or absent together, and `Optional` on both admitted the two mixed states that never occur. Verified that mypy now rejects them. `mechx request` no longer promises a URL below an unreadable result, since that line only prints when there is a URL to print. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

DIvyaNautiyal07

Approving — the delta addresses the earlier concerns cleanly (single deliveries key, typed RequestResult union, retry-on-None, loud logging on unforeseen onchain errors, hex request ID in the gateway warning). A handful of non-blocking follow-ups below, worth picking up in a small chase-PR.

Both watchers logged the message of an error nothing anticipated but not its stack. The on-chain path passes the exception through `exc_info` — `gather` preserves `__traceback__` on what it hands back — and the offchain path uses `logger.exception`. `task_result` is now required to be a 32-byte hex digest before it is turned into a URL. A mech reporting status through that field instead, `"pending"` or `"error"`, would otherwise produce a URL that 404s; since the previous commit retries an unreadable file, that would have spent the whole wait budget on a request that had in fact answered inline. Decoded with `bytes.fromhex` rather than `int(value, 16)`, which accepts a `0x` prefix the bare digest never carries. The timeout backfill logs each unresolved request with its URL: the warning above it only counts how many arrived, which does not separate "never delivered" from "delivered but unreadable". A successful read now pops the request from `pending_urls`, so it holds only unresolved requests and the backfill needs no guard against overwriting a result that was already read. `OnchainRequestResult.receipt` is typed `TxReceipt`; the runtime value is web3's `AttributeDict`, which `Dict[str, Any]` hid from consumers. Tests: assert the logged arguments positionally, assert `.url` alongside `.data`, cover a batch where one file reads and the other never does, and cover the status-string shapes that must not become a URL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

DIvyaNautiyal07

All follow-ups from the previous round are in: exc_info=data on the onchain error log and logger.exception on the offchain one, _is_delivery_hash rejecting status strings before they become a URL, per-request warning at the backfill, TxReceipt on the TypedDict, pending_urls.pop on success (backfill can now assign directly), and a mixed-batch test. Nothing new to add.

bennyjo

Round-3 changes verified: an unreadable result file now stays pending and is retried each poll cycle within the budget (the common gateway-propagation 404 case), with the URL backfilled at timeout so the answer stays reachable by hand; unforeseen read errors are logged loudly on both paths instead of degrading silently; and the typed RequestResult union plus JSONValue alias pin the public contract. This also contains the remaining response.text concern in practice on both paths.

OjusWiZard requested review from DIvyaNautiyal07 and bennyjo August 11, 2026 20:02