Repository navigation
Conversation
…sage Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…path Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kanat
left a comment
There was a problem hiding this comment.
Round-1 review of e5564ac merged onto accelerate (eee6d6665, and again onto 977b2eb24 after #2017 landed).
Merge note: 977b2eb24 (#2017) conflicts with this branch in acceleration/internal/agent/agent.go: this PR adds WriterOf beside WriterLabel, and #2017 replaced WriterLabel/labelPart with Authored. Keeping WriterOf and taking Authored builds. AppHooks, SessionConnectors, MessageHooks, ChatLogins, ChatEpisodes, session, conversation and agent all pass on that merge.
CI: 55 checks pass. Two fail: integration (internal/api) TestMemorySuite and integration (internal/memory/mem0). Both get mem0 429 rate-limit answers from the vendor. Base eee6d6665's integration run failed too. Not caused by this PR.
[Nit] R1.5 acceleration/internal/api/conversations.go:86: if the owner == customer filter becomes true, every test still passes. That is safe, because AnswerConnectorApproval checks the customer again (connector_approval.go:179). The only visible change is when two customers run sessions on the same cid string: newest() may pick the other customer's session, and the right writer gets a 404. Ticket: add a test with a second app's session on the same cid.
Author questions:
- Q1 (new endpoint): it follows AGENTS.md «HTTP API». It uses Huma,
doc:tags and length limits, rejects unknown fields, returnserrXvalues, is markedx-client-accessible, and ships the regenerated openapi.yaml and clients (generated.shfinds no drift). The SDK change is Go only, with the sdk skill note. Auth holds: a user token answers only when it is the asked person and still a member, a server key answers any call of its customer, and an anonymous caller gets a 403. Blocked only by R1.1. - Q2 (declared tools,
DecideCommandTool): ticket. It is not a regression:DecideCommandTool(session.go:1737) checks no user, so whoever could settle a call on the socket before still can.
Checked and sound: agent 1:1 conversations still ask the session's user, and the socket rule is still by.UserID == Caller.UserID. A shared agent conversation still asks nobody (TestAHeldCallInASharedConversationAsksNobody; dropping canAsk fails it). A finished reply asks nobody. Ben gets a 403, a non-member a 404, an anonymous caller a 403, and the writer a 204 that works once. I re-ran the author's 28 mutants and 7 of my own. Two of mine survived: R1.3 and R1.5 (one more was equivalent).
Verdict: request changes, for R1.1 (the POST fails whenever it lands on another router pod), R1.2 (an approval in a channel type expires after 2 minutes, not 10) and R1.3 (the control for agent-type thread channels is not pinned).
| if !backend && !by.Kind.Verified() { | ||
| return nil, errApprovalNeedsPerson | ||
| } | ||
| found, running := s.sessions.ByConversationWhere(req.Cid, func(owner string, _ int64) bool { return owner == customer }) |
There was a problem hiding this comment.
[Should fix] R1.1 This finds only a session running on the router that received the POST. The router's session for a channel type runs on whichever router took the hook (the turn lease in chat_channels.go:166-190 is shared by every router). onOwningNode (forward.go:35) forwards only /v1/agents/sessions/{id}/... (sessionInPath, forward.go:118). node.Directory stores only session-id and agent-id keys, and here the agent id is the config id, which every channel of the type shares. So with two or more router pods, as on staging and prod, the writer's answer and the backend's answer return 404 whenever the load balancer picks another pod.
Shown by a probe in AppHooksSuite with s.otherNode() (shared Redis directory). Anya's call is held on the suite's node. Her POST to the other node returns 404, and so does the backend's. The same POST on the holding node returns 204.
Fix: reuse the directory and the forwarder. Record the node of a session by its ConversationID in node.Directory (Directory.write already sets the session and agent keys), and have onOwningNode forward /v1/agents/conversations/{cid}/tool_approvals to that node with s.forwarder, as it does for session paths. Do not add a second mechanism. Test: the probe above, with the POST on the other node expecting 204.
There was a problem hiding this comment.
Fixed in d55829d: AppHooksSuite/TestAnApprovalAnsweredOnAnotherNodeReachesTheCall (your probe: Ben 403 and Anya 204 from the other node, call runs). The node directory also keeps the node by the conversation a session answers in (Directory.Hold(..., conversationID), NodeByConversation), and onOwningNode forwards POST .../conversations/{cid}/tool_approvals there with the existing forwarder (nodeFor, approvalInPath in forward.go). Mutations: forward rule off -> 404 "her answer from the other node never reached the call"; conversation key not written (manager passes "") -> same failure.
| minutes, the call is not sent and the model is told. A session with nobody to ask (incognito, a | ||
| run, a shared chat, an MCP event's) refuses the call at once. An entry must match a tool the | ||
| run, a shared chat, an MCP event's) refuses the call at once. In a channel of a type | ||
| `chat_channels` names, the call asks the writer of the message the reply answers, who answers |
There was a problem hiding this comment.
[Should fix] R1.2 In a channel of a named type, a held call waits at most askTimeout (2 min, messagehooks.go:29), not ten minutes. answerHosted waits for found.Busy() && turn.Err() == nil on a turn context of askTimeout, then its deferred Close ends the session it opened (threadhooks.go:181-232). That cancels the held call. While the call waits, the channel's turn lease is held too, so other members' messages wait behind it (from reading chat_channels.go:170-185; not probed).
Shown by a probe: AppHooksSuite with approvalWait set to 10 min. Anya answers 2m15s after the step appears and gets a 404, and the step is cancelled. This paragraph sits right after "not answered in ten minutes", so readers will expect ten minutes here too.
Fix, one of two:
- keep the router's turn and its session open while a call waits for approval, up to
approvalWait; - or document the 2-minute limit here, in the CHANGELOG line and in the operation's description, and pin it with a test.
There was a problem hiding this comment.
Fixed in be79c5a + 78d1dc1: the skill, CHANGELOG and the operation description now say two minutes when the Router opened the session for the message (openapi + clients regenerated, generated.sh in step). Pinned by ChannelTypeTurnSuite/TestAWritersApprovalAfterTheTurnEndsIsTooLate (approval wait 10 min; step cancelled ~2m5s after Anya wrote, her answer then 404, no provider hit). Mutations: askTimeout = 3 min -> fails "the call outlived the two minutes the docs give"; session not closed at turn end -> same failure.
| return persistent.CommandReceipt{}, "", stack.Wrap(errNothingToAnswer) | ||
| } | ||
| receipt, err := s.persisted.BeginFollowUp(id, text) | ||
| receipt, err := s.persisted.BeginFollowUp(id, text, agent.WriterOf(ctx)) |
There was a problem hiding this comment.
[Should fix] R1.3 The hard rule "agent-type channels keep today's approval rule" is not tested where the hook answers them. Thread channels are agent type and go through answerHosted. Mutant: make agent.WriterOf fall back to the id WithAuthor sets. Then a Slack or SMS thread's held call is put to the person texting, who cannot answer it, and the thread waits; on base the call is refused at once. This mutant passes AppHooks, SlackChannel, SessionConnectors, MessageHooks, ChatLogins, conversation and session tests.
Test row to add, SessionConnectorsSuite/TestAThreadChannelsHeldCallAsksNobody:
- set up a thread channel (
s.threadChannel) whose crm binding setsrequire_approval: [echo]; - send a
message.newhook inagent:<thread>fromslack-U0000ALICE; - expect the reply to finish (
support_runtime.statecompleted) with nocrm__echostep stored.
Checked: it passes on base eee6d6665, passes on this head, and fails under the mutant ("the thread's reply never finished").
There was a problem hiding this comment.
Fixed in b1a3ffe: SessionConnectorsSuite/TestAThreadChannelsHeldCallAsksNobody (your probe). Passes on head; with WriterOf falling back to the WithAuthor id it fails "the thread's reply never finished".
|
|
||
| The model answering in a channel of a named type sees each person's message under their name and Stream user id, `Anya (user anya-1): ...`, from the first message, both live and read back from the channel, and is told that only that opening label says who wrote a message and that the id grants no access. A name is cut at 256 characters, and a colon, a bracket, a line break or an invisible format character in a name or id becomes a space. `agent` channels and thread channels are labelled as before (AI-3153). | ||
|
|
||
| In a channel of a named type, a connector call whose binding sets `require_approval` asks the person whose message the reply answers: its `ai_tool_call` step's `target_user_id` is that writer, not the session's user. The writer answers with their own user token through `POST /v1/agents/conversations/{cid}/tool_approvals` (`tool_call_id`, `request_id`, `allowed`, `summary`), while they are a member of the channel; the app's backend may answer any. Another person is refused with a 403, and a call nobody waits on, or a channel the caller is not in, is a 404. `agent` channels and shared conversations ask whom they asked before; the session's user may answer through the new operation too (AI-3154). |
There was a problem hiding this comment.
[Nit] R1.4 "shared conversations ask whom they asked before" is true only for the agent type. A channel of a named type with support_access: members is shared too (Shared(custom)). There askedFor returns the writer before it checks canAsk, so it now asks the writer; base asked nobody. That behaviour seems right, but the sentence should say it. Ticket.
There was a problem hiding this comment.
Fixed in a4a46d0: the line now says agent channels ask whom they asked before and a shared conversation in one still asks nobody; a channel of a named type shared with other members now asks the writer, where it asked nobody before (parts.go askedFor: Writer first, then canAsk).
| if initiator := c.data.Commands[m.CommandID].Initiator; initiator != "" { | ||
| part.TargetUserID = initiator | ||
| record := c.data.Commands[m.CommandID] | ||
| if asked := cmp.Or(record.Writer, record.Initiator); asked != "" { |
There was a problem hiding this comment.
[Question] R1.6 A client tool with an approval (display.Client and awaiting_client), in a session a caller opened through the API on a channel of a named type: here target_user_id becomes the writer, while target_client_id is still the initiator's install (line 205). So the step asks Anya to approve a call that runs on another person's device. Neither she nor the POST can settle it. Should a client tool keep the Initiator? Ticket, together with the declared-tool follow-up (Q2).
…pproval-to-writer # Conflicts: # acceleration/internal/agent/agent.go
The router-hosted session of a channel type runs on whichever node took
the hook, so an answer posted to another node found no session (404).
The node directory now also names the node by the conversation a session
answers in, and onOwningNode forwards POST
/v1/agents/conversations/{cid}/tool_approvals there with the existing
forwarder (R1.1).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Router answers one message in a channel of a named type for at most askTimeout and then closes the session it opened, which stops a held call; the skill said ten minutes. The skill, CHANGELOG and the operation's description now say two minutes, and ChannelTypeTurnSuite pins it (R1.2). Clients regenerated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A thread channel (type agent) answered by the hook keeps the rule it had before: the Router's own session asks nobody, so a held call is refused at once and never put to the writer (R1.3). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Why
In a channel of a type an agent config answers in (
chat_channels, #1927), the router answers through its own session, and that session is opened for nobody:spec.Caller.UserIDis empty. Because of that, a connector call whose binding setsrequire_approvalasked nobody and was refused at once:holdForApprovalasksspec.Caller.UserIDandcanAsk("")is false. Example: Anya writes "can you check the crm for my shift?" incallouts:team-1. The reply's call tocrm__echois declined with "nobody can be asked here". The call should go to Anya, and only Anya (or the app's backend) should answer it.The only answer path on the base is
tool_approvalon the session socket. A writer's user token cannot open the socket of the router's session (Manager.Getneeds the session's owner,canReadSessionneedsspec.Caller), and no client learns that session's id. So this PR adds an answer on the conversation the step is shown in.How it works
The socket's
tool_approvalcalls the sameAnswerConnectorApproval. Its rule is now "the person asked, or the backend". In anagentconversation the person asked is stillspec.Caller.UserID, so the socket answers as before.agent1:1 conversationsupport_access: members)Writeris recorded beside the command'sInitiator. The Initiator stays the owner's, so the owner and the backend still reach the command (Command,CancelCommand). A declared tool's approval step (parts.gocalled) names the writer too.Changes
conversation:BeginFollowUp(id, text, writer),commandRecord.Writer,askedFor,AwaitApprovalreturns who was asked,Service.Member(Stream QueryMembers).session: the follow-up passesagent.WriterOf(ctx); the held call keeps who was asked;AnswerConnectorApprovalreturnsErrNoApprovalHeld/ErrApprovalAskedOfAnother.api:POST /v1/agents/conversations/{cid}/tool_approvals(client-accessible,ToolApprovalAnswer), openapi and every client regenerated.stream.Backend.AnswerApproval. Note for the other SDKs in thesdkskill; theconnectorsskill and CHANGELOG say who is asked.chattest: QueryMembers.Validation
AppHooksSuite/TestAnApprovalInAChannelTypeGoesToTheWriterOfTheMessageAppHooksSuite/TestTheAppsBackendMayAnswerAWritersApprovalAppHooksSuite/TestEachWritersApprovalGoesToThemAppHooksSuite/TestTheNextWritersApprovalGoesToThemAppHooksSuite/TestAWriterWhoLeftTheChannelCannotAnswerSessionConnectorsSuite/TestTheUserMayAnswerThroughTheirConversationChatChannelSuite/TestAHeldCallAsksTheWriterOfEachMessageChatChannelSuite/TestAHeldCallInASharedConversationAsksNobodyDisplaySuite/TestAWritersFollowUpAsksTheWriterNotTheOwnerPartsSuite/TestAServerToolsQuestionAsksTheWriterOfTheMessage,TestAServerToolsQuestionInAReplyNobodyAskedForAsksNobodystream/TestAnswerApprovalPostsToTheConversationThe existing socket tests (
TestACallNeedingApprovalRunsOnceTheUserAllowsIt,TestAnotherUserCannotAnswerTheCall,TestTheUsersBackendMayAllowTheCall,TestOnlyTheSessionsUserOrItsBackendMayAnswer) pass unchanged in what they assert.Mutations: 28 mutants, one per changed condition, branch and call site (who is asked, the held call's asked, each refusal of the POST, the socket's call, the SDK's summary and status), all killed. The script applies, tests and restores each in turn.
Not run: the full integration run (CI) and a staging live check.
🤖 Generated with Claude Code