Conversation
The non-durable send path reports admission the moment the message reaches the transport (dbc3efe), because that is the only admission this path has. Nothing took it back when the run request then failed, so on /m a fresh session's rail row stayed marked accepted for a session the server will never list, and tapping it opened an empty chat. The AI SDK never rejects a send. `Chat.makeRequest` catches a refusal, a 5xx and a dropped connection alike and lands them as `status: "error"`, so the `.catch(ignoreStreamRejection)` on `sendQueued` was never going to see one. That status is this path's failure event, so the queue hook reports `onSendFailed` when it arrives while `lastSentRef` still holds the message, which means no turn was ever named. A stream that fails after the turn is named has already dropped the ref and stays a failed turn; a user stop lands on "ready" and never reaches the effect. The local session registry now carries the id of the send that admitted a row, so a retraction retires it while a later message's failure still leaves it alone, which is the guard dbc3efe relied on. Two review nits from the same release ride along: the model-source report picks its HTTP status from the first source whose error is not undefined, matching what the field documents, and the MCP acceptance skip reason says "is unavailable" rather than "did not answer", since the probe counts a 5xx as unreachable too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
Contributor
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe change adds send-aware provisional admission handling, centralizes outbound message-part construction, corrects source-status selection for null rejections, and updates upstream and rate-limit diagnostics. ChangesChat and local-session lifecycle
Outbound message parts
Source status reporting
Diagnostic messages
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ChatQueue
participant Conversation
participant LocalSession
participant Transcript
ChatQueue->>Conversation: onSendAccepted(message with id)
Conversation->>LocalSession: markLocalSessionAccepted(sessionId, sendId)
Transcript-->>ChatQueue: error without named turn
ChatQueue->>Conversation: onSendFailed(message with id)
Conversation->>LocalSession: dropUnacceptedLocalSession(sessionId, sendId)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Website previewPreview URL: https://pr-7028-agenta-website-preview.mahmoud-637.workers.dev Built from |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: fdb39e74-b412-4cfc-b0d9-d58aed085536
📒 Files selected for processing (9)
web/mobile/src/features/chat/LiveConversation.tsxweb/oss/tests/playwright/acceptance/utils/mcpConnections.tsweb/packages/agenta-chat/src/hooks/useAgentChatQueue.tsweb/packages/agenta-chat/src/hooks/useAgentConversation.tsweb/packages/agenta-chat/tests/unit/hooks/useAgentChatQueue.test.tsweb/packages/agenta-entities/src/session/core/localSessions.tsweb/packages/agenta-entities/src/workflow/state/agentModelCandidates.tsweb/packages/agenta-entities/tests/unit/agent-creation-failure-event.test.tsweb/packages/agenta-entities/tests/unit/session-local-sessions.test.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review.
Contributor
|
Attaching a file and pressing Enter with an empty composer sends a message whose
first part is `{type: "text", text: ""}`. The SDK keeps any text that is not None,
so the model receives an empty text content block, and Anthropic-family models
refuse those. On a first turn that is the whole run.
v0.119.0 used the AI SDK shorthand, which omitted the text entirely for a
file-only send. Two files replaced that shorthand with an explicit message object
in this release and dropped the guard with it; the durable path kept it, which is
the evidence it was an oversight.
Reach is narrower than it looks: `sendQueued` runs only on the non-durable
path, which a deployment takes only when the session capability probe reports
`queue: false`. `AGENTA_SESSIONS_QUEUE` defaults to true, so a default cloud or
self-host send goes durable and was never affected. Both hosts are gated the
same way; the desktop passes the same server adapter as /m.
All three send paths now build their parts through one `outboundUserParts`
helper, so the guard cannot go missing on one of them again. It guards on the
text actually sent, `executionText ?? text`, so a file-only send with a pending
display edit keeps that edit rather than dropping it. The text-then-files order
from this release is unchanged.
Also fixes the mobile traces list telling a rate-limited user to check their
connection: the shared `LoadError` supplies that line by default, and a 429 did
come back, so the list now passes `description={null}`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mmabrouk
changed the title
Member
Author
|
@coderabbitai review |
Contributor
|
Contributor
Railway Preview Environment
|
…stream `acceptedRunPending` comes from the `data-session-accepted` frame, which carries the execution id and arrives independently of the `message-metadata` frame that names the turn. So the server can have accepted the run while `latestTurnId` is still null and `lastSentRef` still holds the message. If the invoke stream then dropped, the retraction effect read that as a send that never left and retired a row for a turn the records will carry. The effect now skips a pending accepted run, which is how `canReleaseNow` already reads the same flag. Found by CodeRabbit on #7028. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`lastSentRef` is written only by the two `dispatchUnqueued` call sites, which `admit` returns before when the server advertises a durable queue, so the retraction effect cannot reach a durable send. Nothing pinned that, and a durable send already reports its own failure, so a retraction on top would show a healthy message as failed. Covers both shapes QA asked about: a durable send followed by `status: "error"`, and a new durable send made while the chat is ALREADY in error from an earlier turn. Setting `lastSentRef` in the durable branch fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five defects from the v0.119.1 review, all in the chat send path or next to it. Risk map entries 5, 16, 20 and review findings R9, R10.
1. An attachment-only send carries an empty text part
Attach a file, leave the composer empty, press Enter. The submit guard allows it (
AgentConversation.tsx:826anduseAgentConversation.ts:1301both require only text or files), and the message goes out with{type: "text", text: ""}in front of the attachment. The SDK keeps any text that is notNone(sdks/python/agenta/sdk/agents/adapters/vercel/messages.py:110), so the model receives an empty text content block, and Anthropic-family models refuse those. On a first turn that is the whole run.v0.119.0 did not do this. It used the AI SDK shorthand,
item.fileParts?.length ? (item.text ? {text, files} : {files}) : {text}, which omitted the text entirely for a file-only send. Two files replaced that shorthand with an explicit message object in this release and dropped the guard with it. The evidence that it was an oversight rather than a decision is that the durable path kept it (useServerSessionInputs.ts:331).The guard is back, and it now lives in one place.
outboundUserPartsin@agenta/chat/assetsbuilds the parts for all three send paths:It guards on the text that is actually sent,
executionText ?? text, rather than ontextalone. That is a hair stricter than the durable path was: a file-only send on a session with a pending display edit used to drop the edit context, and now carries it. Neither path can emit an empty text part any more, and the text-then-files order from this release is kept.2. A failed first send leaves a ghost session row on /m
On
/ma brand new session appears in the rail the moment you hit send, before the server knows it exists (#6776). The row follows that send: admitted keeps it until the server lists the session, failed takes it away.The non-durable path never reported a failure.
dispatchUnqueuedhands the message to the AI SDK and immediately reports it admitted, which is the fix fromdbc3efebf1for a row stuck atsubmitting. Nothing took that back when the run request failed, so the row stayed in the rail for the rest of the page session, pointing at a session the server will never list, and tapping it opened an empty chat.The review comment on #6783 read the cause as a swallowed promise rejection, because
sendQueuedends in.catch(ignoreStreamRejection). That is not it.Chat.makeRequestin the AI SDK catches a refusal, a 5xx and a dropped connection alike and lands them asstatus: "error"without rethrowing, sosendMessageresolves and that.catchnever sees anything. Handing the promise back would have carried nothing.useAgentChatQueuestill reports the send as admitted and now treats that admission as provisional.lastSentRefalready holds the message until a turn is named, so a new effect reportsonSendFailedwhenstatusturns"error"while it still does."ready", so it never reaches the effect.acceptedRunPending) is a disconnect, not a failed send: the server sentdata-session-acceptedand the shared run owns a turn the records will carry, before anymessage-metadataframe names it.canReleaseNowalready reads the flag the same way.takeLastSentfrom an effect that runs later.markLocalSessionAcceptedanddropUnacceptedLocalSessionnow carry the send's id, so a retraction retires the row while a later message's failure still leaves it alone, which is the guarddbc3efebf1relied on.How far defects 1 and 2 reach
Both sit behind the same gate, and it is worth stating plainly because it is easy to get wrong.
sendQueuedhas exactly one caller,dispatchUnqueued, which has exactly two: the local-send branch ofsubmitand the queue-release effect. When the capability probe reportsqueue: true,submitreturns inside the durable branch before either therecoverablebranch or the local-send branch runs, so nothing is ever pushed into the localqueuedarray and the release effect returns early onqueued.length === 0.dispatchUnqueuedtherefore never runs, on desktop or on/m: both hosts pass the sameserver: serverInputsadapter (AgentConversation.tsx:506,useAgentConversation.ts:835).So the non-durable path runs only when the probe reports
queue: false, which meansAGENTA_SESSIONS_QUEUEset to a non-truthy value, or a backend whose/sessions/streams/response carries nocapabilitiesobject at all, sincesessionStreamResponseSchemadefaults it to all-false.AGENTA_SESSIONS_QUEUEdefaults totrue(api/oss/src/utils/env.py:724) and is set nowhere in this repo.Both fixes are therefore for self-hosted and mid-upgrade deployments. The risk map's entry 5 says the empty-text send "is reachable" on the strength of the submit guard alone; that guard is necessary but not sufficient, because the message still has to travel the non-durable path to pick up the empty part. The durable path kept its guard all along.
3. A rate-limited trace list tells you to check your connection
TracesList.tsxpassed only a title to the sharedLoadError, which supplies a hard-coded "The request did not come back. Check your connection and try again." A 429 did come back. It now passesdescription={null}, which the component already handles.4 and 5. Two review nits
agentModelCandidates.tspicked the reported HTTP status with??, which skips anullerror althoughsourceOutcomecountsnullas an error. It now takes the first source whose error is notundefined, which is what the field's own documentation says it reports.Tests
@agenta/chat,@agenta/entitiesand@agenta/mobileunit suites, plustsc --noEmitfor those three and@agenta/oss, andturbo lint --fix.displayContent.test.ts, the non-durable path end to end inuseAgentConversation.test.ts(realuseChat, mocked transport, asserting the outbound message the request builder receives), and the durable path inuseServerSessionInputs.test.ts. All three fail if the guard is removed.useAgentChatQueue.test.ts, filed under the existing "every way that send can die" block, which until now tested only the durable path. Three of them fail onrelease/v0.119.1.agent-creation-failure-event.test.tsgains a case for a source that rejected withnull.Risk map entry 21, the dead template tap on the mobile first-run screen, is not fixed here and the suggested fix would be a regression.
CONNECT_STEP_MODEis on unlessNEXT_PUBLIC_AGENT_CONNECT_STEP=false, which nothing sets, sopickTemplateopens the setup step and returns before it ever reaches the!entityIdbail. Addingdisabled={... || !entityId}would disable a strip that works today. The tap is dead only for a template whosestep.opendeclines because it has nothing to connect, and the honest fix there is to queue the tap untilentityIdarrives, the way the?template=arrival already does. That is new state and an effect, which is not a change to make on the last day of a release.No demo yet. This branch has no deployed stack, and defect 2 needs a backend reporting
queue: falseplus a blocked run request, which no component harness can show honestly. A recording goes on this PR once QA runs the steps below.What to QA
Defects 1 and 2 both need a stack whose API reports
queue: false, for the reason above. SetAGENTA_SESSIONS_QUEUE=falseon the API container and restart it, then confirmGET /sessions/streams/?session_id=<any-uuid>&project_id=<pid>answers"queue": false. On a default stack neither defect reproduces, and that is the first thing to check.Defect 1, desktop and
/m. Open an agent chat, attach a file, leave the composer empty, press Enter. On an Anthropic model the turn now runs instead of failing its first request. Regression: send text plus a file and confirm the text still arrives, still ahead of the file. Regression on a defaultqueue: truestack: attachment-only sends keep working, since they already did.Defect 2, same
queue: falsestack./m, open an agent and start a fresh session with+. Block the run request (DevTools request blocking on*/services/*/invoke*, or stop theservicescontainer) and send a message. The transcript shows the run error, and the session rail does not keep a row for that session. Before the fix, the row stays and opens an empty chat until you reload.queue: truestack: a fresh session's first send still puts a row in the rail, and a refused send still takes it away.Defect 3. Trigger a 429 on the mobile traces list. The error card shows the rate-limit message and no "check your connection" line, with Try again still there.