feat: ucan endpoint supports agent message #187
Conversation
vasco-santos
force-pushed
the
feat/ucan-endpoint-supports-agent-message
branch
from
4210567 to
d415b12
Compare
View stack outputs
|
vasco-santos
force-pushed
the
feat/ucan-endpoint-supports-agent-message
branch
from
d415b12 to
0f11106
Compare
There was a problem hiding this comment.
Awesome! Thanks for getting it all done so quickly! It all looks good although I made some minor suggestions here and there.
I do however find naming misleading however, because through out agentMessage is used to refer to the CAR file containing an AgentMessage block in a root. I would suggest update naming to call it agentArchive or agentMessageArchive instead to disambiguate CAR containing the message from the message block itself.
docs/ucan-invocation-stream.md
Outdated
| @@ -29,11 +30,11 @@ Note that at the time of writing Event Archival flow is still to be implemented. | |||
|
|
|||
| UCAN Invocation Stack contains 3 buckets so that it can keep an audit of the entire system, while allowing this information to be queried in multiple fashions. | |||
|
|
|||
| Firstly, the **`workflow-store` bucket** stores the entire encoded file containing one or more invocations to be executed. It is stored as received from UCAN services interacting with UCAN Invocation Stream. It is keyed as `${workflow.cid}/${workflow.cid}` and its value is likely in CAR format. However, CID codec should tell if it is something else. | |||
| Firstly, the **`workflow-store` bucket** stores the entire encoded agent message files containing invocations to be executed, and/or created receipts for ran invocations. It is stored as received from UCAN services interacting with UCAN Invocation Stream. It is keyed as `${agentMessage.cid}/${agentMessage.cid}` and its value is likely in CAR format. However, CID codec should tell if it is something else. | |||
There was a problem hiding this comment.
I think this is misleading, because AgentMessage is a root block inside the incoming / outgoing CAR and therefor has it's own CID, yet I think here we mean CAR CID not the block cid.
There was a problem hiding this comment.
Yeah you are right. Wanted to be generic to enable non CAR encodings in the future. Should we keep the workflow.cid naming then?
There was a problem hiding this comment.
Per suggestion in review text, I changed this into agentMessageArchive.cid. Let me know if it is more clear this way
| const agentMessageWithInvocationCid = await ctx.invocationBucket.getInLink(invocationCid) | ||
|
|
||
| if (!agentMessageWithInvocationCid) { | ||
| throw new NoCarFoundForGivenReceiptError() |
There was a problem hiding this comment.
Why do we want to error here and more importantly:
- Who is supposed to handle this error ?
- What can they do when it occurs ?
It seems like we could run into this due to concurrency when handler returns quickly, because receipt may make it sooner then invocation. Overall seems to me that it would be better to not crash if we do not have an invocation, but rather store receipt and report such instances so someone can take a look and figure out what's up.
There was a problem hiding this comment.
So, if we do not have the invocation we won't be able to get the needed information into the UCAN LOG. We could have a way around putting this to a queue and do it async (even though it might be the case that we never receive this and only a receipt for some reason).
Not sure I follow the concurrency issue though. We currently send the invocation before executing it (and block on caller) until POST /ucan succeeds. Once POST succeeds, invocation is stored and this code path will work without errors for receipts.
The error case is mostly for when we receive receipts for unknown invocations. When we implement invocation spec, I assume everything will be equal? We store invocation before executing it.
With the above said, we return error for caller because they are sending us a receipt for an invocation that was not first reported. What they can do is sending invocation that is going to be executed first
Co-authored-by: Irakli Gozalishvili <[email protected]>
|
@Gozala addressed all your review, looks like we might need to still align on 1 point: Given PR is approved, I will move forward and merge this in, so that we can start working on |
As agreed, first iteration adds support for
POST /ucaninto the API.upload-apiis still using old world, therefore we need to keep bothucantodependencies for this first iteration. Code changes aim to just drop most of the functions in place in favour of the added ones once in next PR we update ucanto inupload-api.Decisions:
"@ucanto/core-next": "npm:@ucanto/core@^7.0.1"+"@ucanto/transport-legacy": "npm:@ucanto/transport@^5.1.1"-legacyto easily drop them in next PR. Sadly, given otherucantopackages depend on the 5th line of core, I could not work around having"@ucanto/core-legacy": "npm:@ucanto/core@^5.1.0"because dependencies would struggle to find correct one...ucanto/coreis barely usedCloses #186