Repository navigation
feat(v2): make prompt response an insertion receipt (JUNIE-5706) - #142
Conversation
There was a problem hiding this comment.
My major concern here is that you worked around the existing RequestOutcome and created your own setRespondingRequestHandler, which relies on implicit coroutine context variables (something I'm strongly against here). It also introduces a number of bugs, mostly concurrency-related, which the existing implementation using RequestOutcome didn't have.
I also think the number of tests can be reduced to leave only the essential ones.
863703e to
abf0f75
Compare
Align v2 with ACP schema tag schema-v2.0.0-alpha.7. - PromptResponse carries required messageId of the inserted user message - agent replies after first user_message/user_message_chunk update, via new internal Protocol.setRespondingRequestHandler - failure, completion or cancellation before insertion -> JSON-RPC error, no idle update; after insertion turn-closing rules unchanged - ClientSession.prompt() returns the receipt MessageId - DiffPatch wire key diff -> text (alpha.3) - add ToolCallUpdate.name (alpha.4) - add availableCommands to session new/resume/fork responses, filled from new AgentSession.availableCommands (alpha.6) Breaking for v2 agents: a prompt flow that never emits a user message fails every prompt. JVM name of ClientSession.prompt becomes prompt-z2_y7hQ (MessageId value class). Co-authored-by: Junie <junie@jetbrains.com>
Remove complexity found in an over-engineering review of the insertion receipt change. Behavior is unchanged, except as noted below. - Agent: drop the separate "ended before insertion" check. The responding request handler already answers a handler that returns without a reply with -32603. Merge the three catch blocks of runPrompt into one. - Protocol: drop the unreachable try/catch after respond. Its only caller already handles every failure after insertion. - Model: share the skip-invalid-item loop as JsonArray.decodeSkippingInvalid. Send an empty availableCommands list as [], the same as configOptions. Schema v2.0.0-alpha.7 treats an omitted and an empty list the same. - Tests: share turnUpdates and insertedUserMessage in acp-ktor-test utils.kt, reuse scriptedV2Support and sessionUpdate() in AgentTest, and remove repeated assertions in SessionResponsesTest. Co-authored-by: Junie <junie@jetbrains.com>
Use RequestOutcome for the insertion receipt, buffered update delivery, and collector cleanup. Remove setRespondingRequestHandler and the response slot carried through coroutine context. Reject cancellation before insertion with -32800. Discard buffered updates on pre-insertion failure or rejected receipt delivery. Keep the prompt Flow signature and avoid suspending user flows on batch reply completion. Cover batch locking, cancellation, callback errors, and rejected receipts. Remove obsolete responding-handler tests and align delivery-order documentation. Co-authored-by: Junie <junie@jetbrains.com>
abf0f75 to
f026a78
Compare
|
nit: Also, there are now 5 new v2 tests in AgentTest plus a new PromptReceiptTest with 4 tests, and the new class has its own FrameTransport/agent harness. Some scenarios overlap: "ends before inserting" vs failedInsertionDiscardsUpdates…, and "local cancellation fails a prompt before insertion" vs cancelBeforeInsertion…. Could the receipt tests live in one place with one harness, keeping one test per behavior? |
…y over the wire and wait for the first user message asyncronously instead
Summary
session/promptnow answers with an insertion receipt.PromptResponsehas a requiredmessageId, the id of the inserted user message.AgentSession.prompt()flow and replies tosession/promptafter the firstuser_messageoruser_message_chunkupdate.ClientSession.prompt()returns the receiptMessageId.DiffPatchuses the"text"wire key in place of"diff"(alpha.3). The Kotlin property name does not change.ToolCallUpdate.namepatch field (alpha.4).availableCommandson the V2 new, resume, and fork session responses (alpha.6), filled from the newAgentSession.availableCommands. An empty list is sent as[].null, a non-array value, and invalid items decode tolerantly.acp-model.apiandacp.api.Breaking changes (V2,
@UnstableApi)PromptResponserequiresmessageId.ClientSession.prompt()returnsMessageIdin place ofUnit. BecauseMessageIdis a value class, its JVM name is nowprompt-z2_y7hQ.ToolCallUpdatehas a new constructor parameter aftertitle. Positional callers must use named arguments.Protocol source and scope
schema-v2.0.0-alpha.7.errorstop reason, so a failure after insertion keepsidlewith anullstop reason.RequestOutcome.onCompletionhas no production user now. Its removal is a separate follow-up.