feat(mech): read the delivered content mech-client now returns - #55
Conversation
Tanya-atatakai
left a comment
There was a problem hiding this comment.
verdict: approve-with-caveat — three small concrete fixes, nothing structural. checked and fine: result() guards key not in data before touching split[key].url, so no KeyError on an undelivered poll; tests importing the real DeliveryResult instead of a stand-in is the right trade, and deleting the dual-shape reader once the pin moved to a release was good judgment.
| if "deliveries" in result: | ||
| payload["delivery_results"] = delivered | ||
| if urls: | ||
| payload["delivery_urls"] = urls |
There was a problem hiding this comment.
the replay path drops the URL this PR just introduced: in _replay (l.719), delivered[key] = report["result"] discards report["url"], and the merged report never gains delivery_urls for resumed ids — so "an unreadable gateway leaves the answer locatable instead of unrecoverable" holds for first-shot deliveries but not replay-resumed ones, which are precisely the lost-response cases. can we collect them, e.g.
if report.get("delivered"):
delivered[key] = report["result"]
if report.get("url"):
urls[key] = report["url"]and merge urls into delivery_urls alongside delivery_results?
There was a problem hiding this comment.
Fixed in 15a46ea — and you put your finger on the one path where losing it actually costs something. _replay took report["result"] and left report["url"] on the floor, so the locatability claim held for first-shot deliveries and failed for replay-resumed ones, which are precisely the lost-response cases the caller-chosen id exists to serve.
Implemented as you wrote it, with urls seeded from the stored payload so URLs collected by an earlier replay survive the merge. New test test_a_resumed_delivery_keeps_the_url_it_came_from; mutation-checked, it fails if the two collecting lines are removed. _replay needed a too-many-locals pragma at 16/15, consistent with _prepare and _with_pending.
| # First use of one chain therefore stalls the others; the fix | ||
| # is a constructor arg upstream (valory-xyz/mech-client#247). | ||
| # The exact ==0.21.3 pin keeps the construction-time-read | ||
| # The exact commit pin keeps the construction-time-read |
There was a problem hiding this comment.
"The exact commit pin" is stale — the pin is the ==0.22.0 release now, the commit pin died with the draft
| # The exact commit pin keeps the construction-time-read | |
| # The exact ==0.22.0 pin keeps the construction-time-read |
There was a problem hiding this comment.
You're right that it's stale — I deleted the sentence rather than applying the reword, which I want to flag rather than have you notice as a missing suggestion.
The reasoning: CLAUDE.md already records why the pin is exact, and this PR records the move from commit to release, so the sentence adds nothing that git blame → commit → PR doesn't already answer — and a comment that has already gone stale once will do it again. The lines above it stay: the construction-lock ordering constraint genuinely isn't recoverable from anywhere else, and it predates this work (ecc38f8).
If you'd rather have the corrected sentence than none, say so and I'll put your suggestion in as written.
| ids, so an id replayed long after that many others pays again. An id is also | ||
| bound to what it asked — reusing one for a different prompt, tool or mech is | ||
| refused rather than answering the wrong question. | ||
| bound to what it asked — reusing one for a different prompt, tool, chain, mech |
There was a problem hiding this comment.
the two shape notes you addressed "to whoever writes the docs next" — delivery_results values shaped differently per flow, delivery_urls/url populated on-chain only — shouldn't they land in this PR? the repo rule is docs don't drift in any commit, and the agent parsing delivery_results is the consumer who needs exactly those two lines (here or the pearl-connect SKILL.md)
There was a problem hiding this comment.
Agreed, and the "whoever writes the docs next" framing was a dodge — that's this PR. They're in pearl-connect/SKILL.md beside the other mech_request notes, since the agent parsing delivery_results is the consumer you identified.
They also came out wrong the first time, and you reviewed against my wrong description, so: the two shapes are not off-chain versus on-chain. offchain_watcher.py:118-150 fetches the result file and returns it with its URL exactly as the on-chain watcher does — except when the mech answers inline (url is None, "the envelope is the answer"), where you get the envelope and no URL. The real axis is inline versus result-file. My live run happened to use a mech that answers inline, and I generalised one mech into a flow property.
Which makes the shipped lines say: normally the result file, envelope-with-response when a mech answers inline, delivery_urls absent in that case with content_cid naming the document instead.
It also makes @OjusWiZard's question — why not just make it uniform — the better fix, and it's noted rather than done here. Normalising means unwrapping the envelope, and since mech-client doesn't tag it that means shape-sniffing: the exact thing I deleted from this PR and you called good judgment. Upstream owns both branches, so that's where uniformity belongs, the same route as #250/#251. Kept as follow-up so this PR stays documentation-only.
cb3c73e to
507e248
Compare
bac6597 to
2fe82d7
Compare
507e248 to
bf383fb
Compare
mech-client used to hand back `delivery_results[request_id]` as a bare IPFS directory URL on the on-chain path and a raw envelope off-chain — two types under one key, and on-chain the URL did not even resolve to the answer (valory-xyz/mech-client#250). It now returns `deliveries[request_id]` as a DeliveryResult carrying the parsed result file and the URL it came from. This reads that shape. `delivery_results` keeps its meaning — the content a caller acts on — and the gateway URL travels alongside as `delivery_urls` rather than in its place, so an unreadable gateway leaves the answer locatable instead of unrecoverable. The pin is a commit, not a release: no tagged mech-client ships this yet. That needs allow-direct-references, which hatchling otherwise refuses. Both facts are recorded in CLAUDE.md, with a note to move back to a version pin once the change is released. An earlier draft read both shapes. That was wrong: the pin is exact and lives in the same commit as the code reading it, so only one shape is ever in play and the other path had no reader. Dropping it also let the tests use the real DeliveryResult instead of a hand-rolled stand-in. Refs OPE-1864. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0.22.0 ships the deliveries/DeliveryResult shape connect/mech.py reads, so the commit pin that stood in for it can go — and with it the hatchling allow-direct-references escape hatch, whose only reason was the direct git reference. No code path changes: 0.22.0 was verified to expose DeliveryResult with the same fields the commit did before the pin moved. Refs OPE-1864. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The refusal said "prompt, tool or mech". The stamp compares five things — prompt, tool, chain, mech and flow — so a caller who reused an id having changed only the chain or flipped legacy_on_chain was told the reason was something it had not touched, and had no way to see what had actually changed. Found running the stack against a live Gnosis service: replaying an on-chain request under the same id with the same prompt but legacy_on_chain=false is refused, correctly, and the message names none of the reason. Behaviour is unchanged; the stamp already covered all five. The test that pinned the old wording now pins the new tail, and a new one covers the flow flip, which nothing exercised before — same prompt, tool, chain and mech, only the flow differing, refused with the payment never reached. Refs OPE-1864. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The URL this branch introduced was dropped on exactly the path that needs it. _replay collected report["result"] but not report["url"], and the merged report never gained delivery_urls — so "an unreadable gateway leaves the answer locatable" held for first-shot deliveries and failed for replay-resumed ones, which are the lost-response cases a caller-chosen id exists to serve. SKILL.md now describes what a delivery looks like, which the agent parsing it had nowhere to learn. Investigating that for the docs corrected the claim I had been making: the two shapes are not off-chain versus on-chain. The off-chain watcher returns the result file and its URL like the on-chain one, except when the mech answers inline, where the envelope is the answer and there is no URL. The rule is inline versus result-file, and a mech that answers off-chain by file is indistinguishable from an on-chain one here. Also dropped a stale sentence claiming the exact commit pin held the construction-time read in place; the pin is a release now, and CLAUDE.md already records why it is exact. Raised in review by @Tanya-atatakai. Refs OPE-1864. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
15a46ea to
603b773
Compare
533814c
into
ojuswirastogi/ope-1864-adjustments-to-existing-pearl-connect-skills
bennyjo
left a comment
There was a problem hiding this comment.
LGTM. One-word nit: the connect/mech.py comment still says "exact commit pin" after the second commit moved to the ==0.22.0 release pin — worth updating since that comment exists to explain why the pin must stay exact.
Proposed changes
Top of the OPE-1864 stack. Adapts
connect/mech.pyto the delivery shape valory-xyz/mech-client#251 introduced, and pins mech-client to 0.22.0, the first release carrying it.mech-client used to hand back
delivery_results[request_id]as a bare IPFS directory URL on the on-chain path and a raw envelope off-chain — two types under one key, and on-chain the URL did not resolve to the answer at all (#250). It now returnsdeliveries[request_id]as aDeliveryResultcarrying the parsed result file plus the URL it came from.This reads that shape.
delivery_resultskeeps its meaning — the content a caller acts on — and the gateway URL travels alongside asdelivery_urlsrather than in its place, so an unreadable gateway leaves the answer locatable instead of unrecoverable.On the pin
mech-client==0.22.0. #251 has since been released, so the commit pin that stood in for it is gone — and with it[tool.hatch.metadata] allow-direct-references = true, whose only reason was the direct git reference. 0.22.0 was verified to exposeDeliveryResultwith the same fields the pinned commit did. The reason for the exact pin is recorded in CLAUDE.md: 0.21.3 returned a bare IPFS directory URL underdelivery_resultsinstead.What an earlier draft got wrong
It read both shapes, on the theory that the pin could move independently. It can't: the pin is exact and lives in the same commit as the code reading it, so only one shape is ever in play and the other path had no reader. Deleting it also removed a duck-typed
hasattrcheck and let the tests use the realDeliveryResultrather than a hand-rolled stand-in.Fixes
Refs OPE-1864.
Types of changes
Checklist
mainbranch (left side). Also you should start your branch off ourmain— stacked: based on the guardrail-dry-run branch, see belowpackages/, I have re-locked the Olas package hashes — n/a,packages/untouchedFurther comments
The stack
#53is based on#52. The guardrail-dry-run branch has no PR yet, so there is a gap between it and this one on GitHub — happy to open it.Verification
557 unit tests, 100% coverage on
connect/mech.py, all lint envs green. Tests import the realDeliveryResultfrom the released package rather than a hand-rolled stand-in.Verified live against a real Gnosis service (safe
0x4C0Fec…fA40), five paid mech requests across both flows. Content came back as content on both:delivery_results[id]={request_id, status, content_cid, response}, theresponsecarrying the parsed result, tool,cost_dictandmetadata;{schema_version, requestId, result, tool, …}), plusdelivery_urls.The decisive check is that URL, which is where #250 actually lived. It ends in the decimal request id, and fetching it both ways:
That is the bug and its fix, reproduced end to end.
Two shape notes for whoever writes the docs next, both mech-client's doing and passed through verbatim by
mech.py:874:delivery_resultsvalues are shaped differently per flow (wrapped off-chain, bare on-chain), so an agent parsing them must handle both;delivery_urlsis populated on-chain only —d.urlisNoneoff-chain, wherecontent_cidcarries the same information.Also in this branch
bac6597— the replay-refusal message named only three of the five fields_request_stampcompares. Found by the live run: replaying an on-chain request under the same id with the same prompt butlegacy_on_chain=falseis refused, correctly, and the message named none of the reason. Message and README corrected, plus a unit test for the flow flip that nothing covered before. Behaviour unchanged.🤖 Generated with Claude Code