feat(sessions): record card answers first and give replay one rule - #5919
Conversation
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds durable interaction-row settlement for approval, input, and client-tool cards. It updates API validation, client persistence, transcript replay, HITL selection, connection retry handling, hydration guards, and release-gate coverage. ChangesInteraction lifecycle persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
A short guide for reading this diff.
Suggested order:
api/oss/src/apis/fastapi/sessions/models.pyandrouter.py. This is the contract: what a card may record, and on which lifecycle edge.web/oss/src/components/AgentChatSlice/assets/clientToolAnswer.ts. The ordering that makes the record durable before the resume starts a new turn.web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts. The one rule replay follows, mirrored in the@agenta/chatcopy.web/oss/src/components/AgentChatSlice/hooks/useSessionHydration.ts. When the browser may replace its own chat with the server's copy.- The widgets:
ConnectToolWidget.tsx,ElicitationWidget.tsx,InteractionDock.tsx,useConnectFlow.ts. Cards act where they appear. - The tests and the two new release gate cells.
The full story, with the evidence behind every claim, is the design docs PR #5916. Start with research.md if you want to know why the old behavior was possible.
The branch went through five internal fix rounds, five review rounds, and two live QA passes on the dev stack before it was opened. The fix rounds and what each one found are logged in docs/design/client-tool-interaction-lifecycle/status.md.
One thing needs a decision before merge. See the comment on useAgentChatSession.ts: two lines from the base branch #5860 are missing here, and it does not look deliberate.
| status: SessionInteractionStatus | ||
| resolution: Optional[SessionInteractionResolution] = None | ||
| # The router owns kind-specific validation because the row kind is not known here. | ||
| resolution: Optional[Dict[str, Any]] = None |
There was a problem hiding this comment.
This line lets a card send any kind of answer, not only an approve or deny verdict. It has to be open here. At this point the code does not yet know which kind of card the answer belongs to.
Check that nothing became loose. The router reads the card kind first. If the card is an approval, it still checks the answer against the strict verdict shape, and a bad payload still gets a 422 error.
The rule just below is the other half of the change. It lets one call carry the new status and the answer together. One call means the row is never half written.
| ) | ||
| if source.kind != SessionInteractionKind.user_approval: | ||
| if ( | ||
| body.status == SessionInteractionStatus.resolved |
There was a problem hiding this comment.
Check this guard. The word resolved still belongs to approval cards only. A form card or a connect card that asks for resolved gets a 409 error.
That is on purpose. Form and connect cards end at responded, with the answer saved beside it. The matrix_i1_settlement gate script tests both refusals.
| * dead API costs the user one beat rather than the turn. */ | ||
| export const RECORD_ANSWER_TIMEOUT_MS = 2_000 | ||
|
|
||
| export const recordAnswerThenResume = async ({ |
There was a problem hiding this comment.
This small file holds the order of two steps. The order is the fix.
Step one saves the answer on the server. Step two resumes the run. The resume starts a new turn, and every new turn runs a cleanup job that closes cards nobody answered. If the resume went first, that cleanup could close the card before the answer arrived.
Check the safety valves, not the normal path. The wait for step one stops after 2 seconds. Step one is written so it can never fail loudly. Step two always runs. So a slow or dead server costs the user a short pause, never the answer.
| * whose token the interactions join marked cancelled. Runs once, after the full record sweep, so | ||
| * a LATER `tool_result` for the same toolCallId (a real settle) always wins over this fallback. */ | ||
| function applyCancelledInteractions( | ||
| function applyInteractionRowStates( |
There was a problem hiding this comment.
This function decides what an old card looks like when the browser rebuilds a chat from the server. It is the heart of the fix, so read it slowly.
It tries four things in order and stops at the first one that fits:
- The chat history already holds a real result for this card. Leave the card alone.
- The card's database row holds a saved answer. Show that answer.
- The row is closed and holds no answer. Show a dead "request ended" card.
- None of the above. The card is still open, so leave it live and clickable.
The old code guessed at step 3. It showed "Dismissed the request." even when the user had answered. Now the card says nothing more than we know.
A second copy of this file lives in web/packages/agenta-chat. This block is the same in both copies, character for character. If you change it here, change it there in the same PR.
| * watermark, and release the composer against a truncated history. The zombie case clears both | ||
| * floors with equality, so requiring them costs nothing. | ||
| */ | ||
| export const shouldAdoptTranscript = ({ |
There was a problem hiding this comment.
This function answers one question. May the browser throw away the chat on screen and take the server's copy instead?
Two parts matter.
The first check protects a card that is still waiting. The browser refuses the server copy unless that copy also finishes the same card. Without this, a background refresh can wipe out an answer the user just typed.
The last check is new, and it fixes old chats. The normal rule only accepts the server copy when the history has grown. A finished chat never grows. So a chat saved in the browser before this fix kept showing its dead card forever. The new path accepts the server copy when that copy finishes every waiting card, even with no growth.
That path cannot accept a short or stale copy. It also asks for two floors. The server copy must hold at least as many records as the browser last saw. It must hold at least as many messages as the screen shows. The path is also switched off while an answer is still on its way to the server.
|
|
||
| /** One derivation for both halves of a failed create: what the card shows, what the settle reason | ||
| * tells the agent, and whether Retry is worth offering. They are one value so they cannot drift. */ | ||
| export const connectFailureFrom = (err: unknown): {message: string; retryable: boolean} => ({ |
There was a problem hiding this comment.
One place here decides two things at the same time. It decides the text on the card, and the reason we send to the agent. Both come from one value, so the two can never say different things.
When the server refuses because the connection already exists, the text names the service. For example: "A connection for telegram already exists in this project." The same value marks the failure as not worth repeating, so the card hides the Retry button. Ordinary failures keep Retry.
One limit is deliberate. This refresh only lives in the open tab. After a page reload the card offers Retry one more time. That click asks the server again and brings back the honest text with no Retry. We do not save the state, because saving it would mean finishing a card that is already finished, and guards exist to stop that.
| request.toolName, | ||
| request.input, | ||
| "client_tool", | ||
| correlatedId, |
There was a problem hiding this comment.
One line. A new card row now stores the id of the tool call that created it.
That gives the browser a real key to find the row for a card. Before, the browser compared two ids that happened to match. The match was luck, not a rule. Rows written before this change still use the old way, so old chats keep working.
| component memory, inspect card geometry, or observe the "running somewhere else" strip. Each | ||
| journey's own docstring names the browser action it represents and the browser-only claim it | ||
| cannot cover. A reload or reopen is represented by fresh reads of stored interaction rows and | ||
| session records. A browser answer is represented by the same ONE atomic `/transition` call the |
There was a problem hiding this comment.
Read this note before the six journeys below it.
Saving the answer is the browser's job. This script has no browser, so it sends the same single call the browser sends. If it only resumed the run, the row would stay open and the cleanup job would close it. That would test a user who walked away, not a user who answered.
The Telegram journey goes as far as a script can. It creates a real connection, removes it, then creates it again. It stops at the provider's own web page, because typing a secret there needs a person. The script reports that gap instead of pretending to cover it.
| api: "", | ||
| prepareSendMessagesRequest: async ({messages, id}) => { | ||
| const req = await buildAgentRequest(entityIdRef.current, messages, { | ||
| sessionId: id ?? sessionId, |
There was a problem hiding this comment.
Resolved. No action needed. I am leaving this note so the history is clear.
An earlier push of this branch had dropped two lines that belong to the base branch #5860. The lines are the replayHistory flag on this call, and the clearUnloggedHistory call in onFinish further down. Without them, a rewound chat stops sending the messages it kept.
Both lines are back on the current head of this branch. I read the pushed branch to confirm it.
94c43e8 to
39a53a0
Compare
|
The review's ninth comment (the accidental revert of the base's rewind fix in useAgentChatSession.ts) is fixed. The three lines (the sessionEphemera import, the replayHistory flag, and the clearUnloggedHistory call) are restored exactly as the base has them, amended into the single commit, and force-pushed (39a53a0). Verified after the fix: the branch diff no longer touches any rewind line, tsc is clean, eslint is clean, and the AgentChatSlice suite passes 245 tests. Root cause: an implementation agent rewrote surrounding regions from a pre-rewind read of the file; the working tree and the live dev stack carried the same revert and both are healed. |
Railway Preview Environment
Updated at 2026-08-11T11:25:27.532Z |
Where this code lives, and what changed in each placeA map for reading the diff. The full story is in the design docs PR #5916. The chat screen. The two chat builders. There is a live path that adds messages as the agent streams them, and a rebuild path that reconstructs the chat from the server's saved history. The rebuild path is The browser's shared state and API layer. The server. The runner. Tests and release gates. They exist because this bug class lived between the parts, and every part had green tests.
The short versionThe user's answer is now written down before anything else happens. Everything else reads that one record instead of guessing. |
…first, one replay rule, adoption guards, cards act inline
39a53a0 to
4ad5784
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/oss/src/components/AgentChatSlice/components/clientTools/useConnectFlow.ts (1)
284-315: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIgnore connection creation after unmount.
The cleanup at Line 295 only closes resources that already exist. It does not cancel or invalidate the pending
handleCreatepromise.If the card unmounts while
handleCreateis pending, its continuation can still callonSuccessor the catch block.finishthen callssettlewith stalemeta.settleddata. This can settle a card after its row was removed or replaced.Track component activity and return immediately after
await handleCreate(...)and in the catch path when the component is inactive.Proposed fix
+ const activeRef = useRef(true) + + useEffect( + () => () => { + activeRef.current = false + teardown() + }, + [teardown], + ) + const runConnect = useCallback( async (settleParkedCall: boolean) => { // ... try { const result = await handleCreate({slug, name: slug, mode}) + if (!activeRef.current) return const redirectUrl = typeof result.connection?.data?.redirect_url === "string" ? result.connection.data.redirect_url : undefined // ... } catch (err) { + if (!activeRef.current) return const {message, retryable} = connectFailureFrom(err) // ... } },web/packages/agenta-playground/src/state/execution/agentMessageQueue.ts (1)
48-63: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPrevent abandoned interaction cards from blocking the queue.
cancelStaleInteractionsruns only when a new runner turn starts, butcanReleaseQueuedMessageblocks that turn while the card remains pending. Reload also replays the pending card unless its row becomes terminal. Add a recovery path that settles stale rows or releases the queue hold.
🧹 Nitpick comments (13)
api/oss/tests/pytest/unit/sessions/test_transition_interaction_resolution.py (2)
179-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the router-level approval validation produced the 422.
FastAPI returns 422 for any body that fails
SessionInteractionTransitionRequestvalidation. The test asserts only the status code, so it passes even when the request never reaches the approval-kind check in the router. Sinceresolutionis nowDict[str, Any], that distinction matters: the model no longer rejects malformed approval payloads, and only the router does.Add an assertion that the router looked the row up, or check the error detail shape.
♻️ Proposed assertion
assert response.status_code == 422 + # Proves the router's approval-kind validation ran, not FastAPI body parsing. + interactions_service.query_interactions.assert_awaited() + interactions_service.transition_interaction.assert_not_awaited()
267-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for
resolvedwith noresolutionon a non-approval row.The matrix and this 409 test always send a
resolution. The router evaluates the approval-only guard only inside theresolution is not Nonebranch, so aresolvedtransition without aresolutionis not covered here. See the comment onapi/oss/src/apis/fastapi/sessions/router.pyLines 891-898 for the root cause..agents/skills/agent-release-gate/resources/matrix_i1_settlement.py (1)
209-234: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlso refuse
resolvedwhen the request carries no resolution.The docstring at Line 26 states that this cell proves
resolvedremains approval-only.resolved_refusalalways sends a resolution, so it exercises only the guarded path. The router evaluates the approval-only check inside itsresolution is not Nonebranch, so aresolvedtransition with no resolution currently escapes the 409. See the comment onapi/oss/src/apis/fastapi/sessions/router.pyLines 891-898.Add a second refusal probe that omits
resolution, so the gate pins the full invariant..agents/skills/agent-release-gate/resources/matrix_i2_card_journeys.py (1)
708-710: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport cleanup failures instead of discarding them.
delete_connectionreturns a status code and never raises. Thefinallyloop ignores that code. If a delete fails, the run leaves a real Composio connection in the QA project and the result gives no signal. Repeated gate runs then accumulate connections.Collect the failed ids and add them to the returned result.
.agents/skills/agent-release-gate/resources/matrix_l4_client_tool_lifecycle.py (1)
188-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the interaction-transition request into
qa_matrix_lib.Three gate scripts now build the same
POST /sessions/interactions/transitionrequest:
- this inline block,
transitionin.agents/skills/agent-release-gate/resources/matrix_i2_card_journeys.pyLines 110-125,answerin.agents/skills/agent-release-gate/resources/matrix_i1_settlement.pyLines 77-93.The three copies differ only in error handling. If the wire contract changes, all three need the same edit. Add one helper next to
api_callinqa_matrix_lib.pyand let each cell keep its own failure reporting on top of it.api/oss/src/apis/fastapi/sessions/router.py (1)
874-885: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a token filter to
SessionInteractionQuery. Whenresolutionis provided, this handler fetches every interaction for the session and scans the results in Python. Addtokento the query and apply the predicate in the DAO.web/packages/agenta-entities/src/session/state/interactionAnswer.ts (1)
46-63: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider skipping the write when the matched row is already terminal.
tokenForToolCallreturns the token for any row, including a row whose status is alreadyresponded,resolved, orcancelled. The atom then sends arespondedtransition for that row. The server can reject that transition, or it can overwrite a stored resolution with a newer one. The failure path only logs a warning, so the outcome is silent.If the intended contract is "record the answer once", read the matched row state and return early for a non-pending row.
♻️ Suggested guard
- if (!token) return + if (!token) return + const matched = states.get(token) + if (matched && matched.status !== "pending") returnweb/oss/src/components/AgentChatSlice/assets/loadSession.ts (1)
45-60: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueBoth loaders reuse one interaction-row snapshot for the refreshed transcript.
interactionRowStatesresolves once in the initialPromise.all, and therefreshedhandler reuses that snapshot. The refreshed path is where the server has moved on, so a row that settled in between replays as still pending. The fallback is the documented degradation, so this is a hardening item, not a break.
web/oss/src/components/AgentChatSlice/assets/loadSession.ts#L45-L60: refetch the row states inside therefreshedhandler before callingtranscriptToMessages(fresh, ...).web/packages/agenta-chat/src/assets/loadSession.ts#L52-L61: apply the same change so the two copies stay identical.web/packages/agenta-chat/src/assets/transcriptToMessages.ts (1)
133-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the settlement helpers into a shared module.
settleClientToolPart,settleApprovalPart, andapplyInteractionRowStatesare duplicated character for character inweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts(lines 112-178). The three functions are pure and depend only onSessionInteractionRowStatefrom@agenta/entities/sessionandCLIENT_TOOL_INTERACTION_ENDED_OUTPUTfrom@agenta/shared/clientTools, which both packages already import.Move them next to
CLIENT_TOOL_INTERACTION_ENDED_OUTPUTin@agenta/shared/clientToolsand import them in both transcript builders. That removes the manual "change both copies" rule and the associated drift risk. TheParttype is the only local dependency; pass it through a small structural interface.web/packages/agenta-chat/tests/unit/assets/__fixtures__/abandonedFormSession.json (1)
1-845: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne golden capture is stored as two 845-line files. Both fixtures hold the same session capture for the same golden test. A future re-capture updates one copy and leaves the other asserting pre-fix behavior.
web/packages/agenta-chat/tests/unit/assets/__fixtures__/abandonedFormSession.json#L1-L845: keep this copy as the single source and export it from a shared test-fixture entry point.web/oss/src/components/AgentChatSlice/assets/__fixtures__/abandonedFormSession.json#L1-L845: delete this copy and import the shared fixture fromweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.ts.web/packages/agenta-playground/src/state/execution/agentMessageQueue.ts (1)
48-50: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider narrowing the scan cost on the release path.
messages.some(messageHasPendingHitl)short-circuits on the first pending message. During normal streaming there is no pending gate, so every call walks the entire transcript and allocates onebuildRenderMapper assistant message.canReleaseQueuedMessageruns on message updates, so this cost repeats per commit on a growing transcript.Two low-cost options:
- Iterate from the newest message backward. A pending gate is usually recent, so the common hit case exits sooner.
- Skip
buildRenderMapwhen the message has no tool parts.This is optional. Raise it only if profiling shows the release path is hot.
web/packages/agenta-playground/tests/unit/agentApprovalResume.test.ts (1)
331-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrengthen the negative assertion.
The markerless assertion on Line 345 returns
falsebecause the tail messagea2has no tool parts, sotoolParts.length === 0exits early. It does not exercise the tail-only selection rule the marker path changed. The assertion would still pass if the markerless branch scanned the whole transcript.To target the rule, make the tail an assistant message that holds its own settled tool part. Then
falseproves the earlier message was excluded rather than proving the tail was empty.web/packages/agenta-playground/tests/unit/agentMessageQueue.test.ts (1)
115-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the companion assertion for
canReleaseQueuedMessage.This test locks the new transcript-wide behavior of
isHitlPending. The consumer whose behavior changed most iscanReleaseQueuedMessage, which now holds the queue for a pending card in any earlier message.Add an assertion on the same message shape so the release gate is covered directly:
💚 Proposed companion assertion
expect(isHitlPending(messages)).toBe(true) + expect(canReleaseQueuedMessage("ready", messages)).toBe(false) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d3687c2e-c822-438f-bb90-7b6799db7802
📒 Files selected for processing (54)
.agents/skills/agent-release-gate/resources/coverage.md.agents/skills/agent-release-gate/resources/matrix_i1_settlement.py.agents/skills/agent-release-gate/resources/matrix_i2_card_journeys.py.agents/skills/agent-release-gate/resources/matrix_l4_client_tool_lifecycle.pyapi/oss/src/apis/fastapi/sessions/models.pyapi/oss/src/apis/fastapi/sessions/router.pyapi/oss/tests/pytest/acceptance/sessions/test_interaction_sweep_race.pyapi/oss/tests/pytest/unit/sessions/test_transition_interaction_resolution.pyservices/runner/src/engines/sandbox_agent/client-tools.tsservices/runner/tests/unit/client-tools.test.tsweb/oss/src/components/AgentChatSlice/AgentConversation.tsxweb/oss/src/components/AgentChatSlice/assets/__fixtures__/abandonedFormSession.jsonweb/oss/src/components/AgentChatSlice/assets/clientToolAnswer.test.tsweb/oss/src/components/AgentChatSlice/assets/clientToolAnswer.tsweb/oss/src/components/AgentChatSlice/assets/loadSession.tsweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.tsweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.tsweb/oss/src/components/AgentChatSlice/components/AgentComposerDock.tsxweb/oss/src/components/AgentChatSlice/components/ApprovalDock.tsxweb/oss/src/components/AgentChatSlice/components/InteractionDock.tsxweb/oss/src/components/AgentChatSlice/components/clientTools/ClientToolPart.tsxweb/oss/src/components/AgentChatSlice/components/clientTools/ConnectToolWidget.tsxweb/oss/src/components/AgentChatSlice/components/clientTools/ElicitationWidget.tsxweb/oss/src/components/AgentChatSlice/components/clientTools/meta.test.tsweb/oss/src/components/AgentChatSlice/components/clientTools/meta.tsweb/oss/src/components/AgentChatSlice/components/clientTools/registry.tsxweb/oss/src/components/AgentChatSlice/components/clientTools/useConnectFlow.test.tsweb/oss/src/components/AgentChatSlice/components/clientTools/useConnectFlow.tsweb/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.tsweb/oss/src/components/AgentChatSlice/hooks/useSessionHydration.test.tsweb/oss/src/components/AgentChatSlice/hooks/useSessionHydration.tsweb/oss/src/components/AgentChatSlice/hooks/useSessionRecordsWatch.tsweb/packages/agenta-chat/src/assets/loadSession.tsweb/packages/agenta-chat/src/assets/transcriptToMessages.tsweb/packages/agenta-chat/src/model/approvals.tsweb/packages/agenta-chat/tests/unit/assets/__fixtures__/abandonedFormSession.jsonweb/packages/agenta-chat/tests/unit/assets/loadSession.test.tsweb/packages/agenta-chat/tests/unit/assets/transcriptToMessages.test.tsweb/packages/agenta-chat/tests/unit/hooks/useAgentConversation.test.tsweb/packages/agenta-chat/tests/unit/model/approvals.test.tsweb/packages/agenta-entities/src/session/api/api.tsweb/packages/agenta-entities/src/session/index.tsweb/packages/agenta-entities/src/session/state/interactionAnswer.tsweb/packages/agenta-entities/src/session/state/interactionStatus.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/toolPermission.tsweb/packages/agenta-playground/src/index.tsweb/packages/agenta-playground/src/state/execution/agentApprovalResume.tsweb/packages/agenta-playground/src/state/execution/agentMessageQueue.tsweb/packages/agenta-playground/src/state/execution/index.tsweb/packages/agenta-playground/src/state/index.tsweb/packages/agenta-playground/tests/unit/agentApprovalResume.test.tsweb/packages/agenta-playground/tests/unit/agentMessageQueue.test.tsweb/packages/agenta-shared/package.jsonweb/packages/agenta-shared/src/clientTools/index.ts
| | Cell | Tier | What it pins | Extra requirement | | ||
| |---|---|---|---| | ||
| | `matrix_i1_settlement.py` | coached, mechanism-level | The 3 card kinds x complete/decline/walk-away table against the live API. Answered form/connect rows must be `responded` with their exact resolution; approvals must be `resolved` with a strict verdict; abandoned rows must be swept from `pending` to `cancelled` without an invented answer; non-approval `resolved` attempts must return 409. The script sends the atomic transition itself because that write belongs to the browser. | none beyond the three gate environment variables | | ||
| | `matrix_i2_card_journeys.py` | coached, mechanism-level | The six scripted journeys from `docs/design/client-tool-interaction-lifecycle/qa.md`: compound form/reload/connect-decline/schedule, form then connect, two connects, close/reopen, real Telegram create/remove/re-create, and decline/retry. Reload and reopen are fresh row/record reads, not browser automation; each journey names its wire-level limit. The Telegram journey validates a real bot against Telegram's own API and drives Agenta's connection lifecycle, but STOPS before entering the credential on the provider's hosted page — that step is browser-only, so the connection never reaches `is_valid` and the journey reports the gap in `not_covered`. Run qa.md journey 5 by hand in exploratory QA. | a funded model connection for the two same-session/record probes; `TELEGRAM_BOT_TOKEN` for the real Telegram journey | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate the referenced qa.md and check its journey numbering.
set -euo pipefail
fd -t f 'qa.md' docs || echo "no qa.md under docs/"
target="docs/design/client-tool-interaction-lifecycle/qa.md"
if [ -f "$target" ]; then
echo "FOUND: $target"
rg -n -i 'journey' "$target" | head -40
else
echo "MISSING: $target"
rg -rn 'client-tool-interaction-lifecycle' --glob '!node_modules' . | head -20
fiRepository: Agenta-AI/agenta
Length of output: 2048
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- referenced coverage entry ---'
sed -n '165,180p' .agents/skills/agent-release-gate/resources/coverage.md
printf '%s\n' '--- matrix_i2_card_journeys.py docstring ---'
sed -n '1,45p' .agents/skills/agent-release-gate/resources/matrix_i2_card_journeys.py
printf '%s\n' '--- related lifecycle design paths and journey headings ---'
fd -t f . docs/design | rg '(^|/)(client-tool-interaction-lifecycle|n)/|client-tool-interaction-lifecycle|(^|/)n/qa\.md$' || true
rg -n -i '^(#{1,6}.*journey|journey[[:space:]]+[0-9]+|journey[[:space:]]+[ivx]+)' docs/design --glob '*.md' | head -80Repository: Agenta-AI/agenta
Length of output: 4842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lifecycle-related files ---'
git ls-files | rg -i 'client.?tool|interaction.?lifecycle|(^|/)n/qa\.md$|qa\.md$' | head -120
printf '%s\n' '--- exact journey descriptions ---'
rg -n -i -C 2 \
'compound form|form then connect|two connects|close/reopen|create/remove/re-create|decline/retry|real Telegram|Telegram.*journey|journey 5' \
.agents docs --glob '*.md' --glob '*.py' | head -160
printf '%s\n' '--- design directory candidates ---'
find docs/design -maxdepth 3 -type d -print | rg -i 'client|tool|interaction|lifecycle|^docs/design/n$' || trueRepository: Agenta-AI/agenta
Length of output: 2948
Fix the missing QA reference
docs/design/client-tool-interaction-lifecycle/qa.md does not exist, and no replacement document defines these six journeys. Add the document or update both references to the authoritative QA document before directing readers to journey 5.
There was a problem hiding this comment.
The document exists, it just lands in a sibling PR. It is added by #5916 (branch docs/client-tool-interaction-lifecycle), which is the docs lane for this work and targets the same base, release/v0.112.0.
I checked that the reference is accurate and not only present. Section 5 of qa.md defines exactly six numbered journeys, in the same order this row lists them, and journey 5 is the real Telegram create/remove/re-create one, which is the journey the row asks a human to run by hand. So the pointer resolves correctly once both PRs are on the release branch.
No change here.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 8 minutes and 13 seconds before sending another message.
| // A still-pending interaction in an EARLIER message must keep the status at `awaiting`, or the | ||
| // status collapses to idle, the settle stamp lands, and the running-elsewhere strip flickers in | ||
| // the very tab that owns the parked widget (Mahmoud's session e627d80a). `hitlPending` scans | ||
| // the whole transcript for exactly that, approvals included. | ||
| useEffect(() => { | ||
| const status: SessionRunStatus = error | ||
| ? "error" | ||
| : hitlPending || anyPendingInteraction | ||
| : hitlPending | ||
| ? "awaiting" | ||
| : busy | ||
| ? "running" | ||
| : "idle" | ||
| setSessionStatus({id: sessionId, status}) | ||
| }, [error, hitlPending, anyPendingInteraction, busy, sessionId, setSessionStatus]) | ||
| }, [error, hitlPending, busy, sessionId, setSessionStatus]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the stopped handling between the session status and the two other pending-gate consumers.
The status effect derives awaiting from hitlPending alone. It does not consider stopped. The two other consumers of the same pending state do consider it:
- Line 349:
pendingInteractionreturnsnullwhenstopped. - Line 575:
showWaitingisfalsewhenstopped.
After a user stop, the transcript still holds the parked gate, so hitlPending stays true. The tab-bar dot and the live-watcher signal then report awaiting while the transcript shows no waiting marker and the connect dock is hidden.
Confirm this is intended. If it is not, gate the awaiting branch on !stopped as well.
🔧 Proposed alignment
useEffect(() => {
const status: SessionRunStatus = error
? "error"
- : hitlPending
+ : hitlPending && !stopped
? "awaiting"
: busy
? "running"
: "idle"
setSessionStatus({id: sessionId, status})
- }, [error, hitlPending, busy, sessionId, setSessionStatus])
+ }, [error, hitlPending, stopped, busy, sessionId, setSessionStatus])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // A still-pending interaction in an EARLIER message must keep the status at `awaiting`, or the | |
| // status collapses to idle, the settle stamp lands, and the running-elsewhere strip flickers in | |
| // the very tab that owns the parked widget (Mahmoud's session e627d80a). `hitlPending` scans | |
| // the whole transcript for exactly that, approvals included. | |
| useEffect(() => { | |
| const status: SessionRunStatus = error | |
| ? "error" | |
| : hitlPending || anyPendingInteraction | |
| : hitlPending | |
| ? "awaiting" | |
| : busy | |
| ? "running" | |
| : "idle" | |
| setSessionStatus({id: sessionId, status}) | |
| }, [error, hitlPending, anyPendingInteraction, busy, sessionId, setSessionStatus]) | |
| }, [error, hitlPending, busy, sessionId, setSessionStatus]) | |
| // A still-pending interaction in an EARLIER message must keep the status at `awaiting`, or the | |
| // status collapses to idle, the settle stamp lands, and the running-elsewhere strip flickers in | |
| // the very tab that owns the parked widget (Mahmoud's session e627d80a). `hitlPending` scans | |
| // the whole transcript for exactly that, approvals included. | |
| useEffect(() => { | |
| const status: SessionRunStatus = error | |
| ? "error" | |
| : hitlPending && !stopped | |
| ? "awaiting" | |
| : busy | |
| ? "running" | |
| : "idle" | |
| setSessionStatus({id: sessionId, status}) | |
| }, [error, hitlPending, stopped, busy, sessionId, setSessionStatus]) |
There was a problem hiding this comment.
This one is already handled, so the extra guard would be a no-op.
hitlPending does not come from the transcript directly. It comes from useAgentChatQueue, which computes it as const hitlPending = !stopped && isHitlPending(messages) at web/oss/src/components/AgentChatSlice/hooks/useAgentChatQueue.ts:76. The comment right above that line describes the exact failure you predict, a stopped turn whose lingering approval-requested part keeps reading as awaiting, and says the stop is folded in at the source so it stays in lockstep with the release decision.
So after a user stop hitlPending is already false and the status effect falls through to busy or idle. The two consumers you cite gate on stopped because they read the transcript themselves, not because the status effect is missing the check.
Declining, no change.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 8 minutes and 10 seconds before sending another message.
| const scrollToCard = useCallback(() => { | ||
| if (!toolCallId) return | ||
| // Virtualized transcript cards may not be mounted. | ||
| document | ||
| .querySelector(`[data-client-tool-call-id="${CSS.escape(toolCallId)}"]`) | ||
| ?.scrollIntoView({behavior: "smooth", block: "center"}) | ||
| }, [toolCallId]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the transcript card emits `data-client-tool-call-id`, and locate the Virtuoso scroll seam.
rg -n -C4 'data-client-tool-call-id' --type=ts --type=tsx web/oss web/packages
# Locate the Virtuoso transcript ref/scroll API available to the dock.
rg -n -C4 'scrollToIndex|virtuosoRef|useVirtuosoTranscript' --type=ts --type=tsx web/oss/src/components/AgentChatSliceRepository: Agenta-AI/agenta
Length of output: 218
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- matching data attributes ---'
rg -n -C4 'data-client-tool-call-id' web/oss web/packages || true
printf '%s\n' '--- Virtuoso scroll seams in AgentChatSlice ---'
rg -n -C4 'scrollToIndex|virtuosoRef|useVirtuosoTranscript|Virtuoso' web/oss/src/components/AgentChatSlice || true
printf '%s\n' '--- InteractionDock ---'
cat -n web/oss/src/components/AgentChatSlice/components/InteractionDock.tsx | sed -n '1,130p'Repository: Agenta-AI/agenta
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- InteractionDock component ---'
cat -n web/oss/src/components/AgentChatSlice/components/InteractionDock.tsx | sed -n '38,115p'
printf '%s\n' '--- AgentConversation dock and transcript wiring ---'
cat -n web/oss/src/components/AgentChatSlice/AgentConversation.tsx | sed -n '380,430p;540,610p;650,730p'
printf '%s\n' '--- AgentTranscript Virtuoso wiring ---'
cat -n web/oss/src/components/AgentChatSlice/components/AgentTranscript.tsx | sed -n '18,115p'
printf '%s\n' '--- useVirtuosoTranscript return and scroll API ---'
cat -n web/oss/src/components/AgentChatSlice/hooks/useVirtuosoTranscript.tsx | sed -n '31,55p;76,145p;146,170p'Repository: Agenta-AI/agenta
Length of output: 28659
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- InteractionDock call sites and pending interaction flow ---'
rg -n -C6 'InteractionDock|pendingInteraction|pending=' web/oss/src/components/AgentChatSlice
printf '%s\n' '--- activeStart and pending interaction definitions ---'
rg -n -C8 'activeStart|getPendingConnectInteraction|pendingInteraction' web/oss/src/components/AgentChatSlice/AgentConversation.tsx web/oss/src/components/AgentChatSlice
printf '%s\n' '--- client-tool rendering path ---'
cat -n web/oss/src/components/AgentChatSlice/components/clientTools/ClientToolPart.tsx | sed -n '1,90p'
rg -n -C6 'ClientToolPart|clientToolMeta|renderMessage' web/oss/src/components/AgentChatSlice/components/AgentTurn.tsx web/oss/src/components/AgentChatSlice/componentsRepository: Agenta-AI/agenta
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- AgentTurn client-tool path ---'
rg -n -C8 'ClientToolPart|clientTools|renderMap|parts.map|part' web/oss/src/components/AgentChatSlice/components/AgentTurn.tsx
printf '%s\n' '--- pending-client-tool tests and semantics ---'
rg -n -C8 'getPendingConnectInteraction|pending.*connect|isPendingClientToolInteraction|request_connection' web/oss/src/components/AgentChatSlice/components/clientTools/meta.test.ts web/oss/src/components/AgentChatSlice/components/clientTools web/oss/src/components/AgentChatSlice
printf '%s\n' '--- composer placement ---'
cat -n web/oss/src/components/AgentChatSlice/components/AgentComposerDock.tsx | sed -n '180,230p'Repository: Agenta-AI/agenta
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- pending interaction fixture ---'
cat -n web/oss/src/components/AgentChatSlice/components/clientTools/meta.test.ts | sed -n '21,80p'
printf '%s\n' '--- AgentMessage client-tool rendering ---'
rg -n -C10 'ClientToolPart|isClientToolPart|resolveClientToolHandler' web/oss/src/components/AgentChatSlice/components/AgentMessage.tsx
printf '%s\n' '--- AgentTurn message rendering ---'
cat -n web/oss/src/components/AgentChatSlice/components/AgentTurn.tsx | sed -n '1,180p'Repository: Agenta-AI/agenta
Length of output: 14555
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
client_tool = Path("web/oss/src/components/AgentChatSlice/components/clientTools/ClientToolPart.tsx").read_text()
transcript = Path("web/oss/src/components/AgentChatSlice/components/AgentTranscript.tsx").read_text()
virt_hook = Path("web/oss/src/components/AgentChatSlice/hooks/useVirtuosoTranscript.tsx").read_text()
dock = Path("web/oss/src/components/AgentChatSlice/components/InteractionDock.tsx").read_text()
assert "data-client-tool-call-id={meta.toolCallId}" in client_tool
assert "data={messages.slice(0, activeStart)}" in transcript
assert "virtuosoRef" in virt_hook
assert "querySelector" in dock
assert "scrollToIndex" not in dock
# The repository's pending-interaction fixture: the parked card is message 0,
# while the later user message makes activeStart equal to 1.
messages = [
{"role": "assistant", "pending": True},
{"role": "user"},
{"role": "assistant"},
]
last_user = max(i for i, message in enumerate(messages) if message["role"] == "user")
active_start = last_user
assert 0 < active_start
assert 0 < active_start # target index is included in Virtuoso's data slice
print("ClientToolPart emits the target attribute.")
print("The pending card can be an earlier message included in the Virtuoso data slice.")
print("InteractionDock has no Virtuoso index-scroll fallback.")
PYRepository: Agenta-AI/agenta
Length of output: 332
Route the dock through the transcript scroll seam.
ClientToolPart emits data-client-tool-call-id, but getPendingConnectInteraction can select an earlier parked card. Virtuoso can unmount that card, so the DOM query can return null. Pass the target message index and virt.virtuosoRef to InteractionDock; use scrollToIndex for virtualized targets and retain scrollIntoView for mounted cards.
There was a problem hiding this comment.
Leaving this as is, with reasons.
The dock already anticipates the unmounted card. The query is optional-chained at InteractionDock.tsx:59-62 with the comment "Virtualized transcript cards may not be mounted", so the click is an inert no-op rather than a crash.
Virtualization is also not a live code path. It is a spike, off by default and double gated, by an env flag and a user setting: isAgentChatVirtualizationAvailable() && virtEnabledInSettings at hooks/useVirtuosoTranscript.tsx:48. The hook's own docstring labels it SPIKE(react-virtuoso) and says it is being evaluated against content-visibility.
Threading virtuosoRef and a message index from the transcript down into a composer-level dock is real coupling between the dock and an experimental scroll engine. That work belongs with the virtualization spike, if and when it ships on by default, rather than with this lifecycle fix. Noting it as a follow-up for that spike instead of changing it here.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 8 minutes and 10 seconds before sending another message.
…t form settle, clear the live-gate marker on stop and error
1 participant
Context
An agent can stop in the middle of a run and ask the user for something. The chat then shows a card: a form to fill in, a service to connect, or a tool call to approve.
In production the user answered those cards and the system never wrote the answer down. The row that holds a card's state stayed
pending. At the start of the next turn, a cleanup job closed every oldpendingrow and marked itcancelled. In the database an answered card and an abandoned card then looked the same.Everything the user saw followed from that. After a reload, an answered form came back empty and asked to be filled again. A finished connect card came back dead, with buttons that did nothing, and it held the composer so no new message could be sent. A warning strip said the run was going on somewhere else, in the same tab that owned the run.
The full story, with the evidence for each claim, is in the design docs PR #5916.
The base of this PR is #5860, because both branches edit the same chat files. GitHub therefore shows only this PR's own diff.
Changes
1. Record the answer first, then resume
When the user answers a card, the browser now makes one API call before it sends the resume. That call tells the server "this card is answered, and here is the answer". It writes the status and the answer together, so the row is never half written.
The order is what closes the race. The cleanup job only closes rows that are still
pending. A row that already saysrespondedis invisible to it. The cleanup job itself needed no change at all.respondedplus a saved answer is the settled state for form and connect cards.resolvedstays approval-only.Before this PR:
resolved, verdict savedresolved, verdict savedpending, latercancelledcancelled, nothing savedcancelled, nothing savedpending, latercancelledcancelled, nothing savedpending, never touchedpending, never touchedAfter this PR:
resolved, verdict savedresolved, verdict savedpending, latercancelledresponded, answer savedresponded, decline savedpending, latercancelledresponded, result savedresponded, decline savedpending, latercancelledOnly the approval card was ever right. Now all three record what happened, and
cancelledmeans one thing again: nobody answered.2. One rule for what replay shows
When the browser rebuilds a chat from the server's history, each card takes the first state that applies:
The old code guessed. It turned every closed row into "Dismissed the request." or "Connection not completed" with a Retry button, which is why answered cards came back looking abandoned.
There are two copies of the replay code, one in the app and one in the
@agenta/chatpackage. The precedence block is identical in both, byte for byte.3. The browser listens, and stops overwriting
The server already publishes an event when a card's row changes. Only the mobile app listened to it. The desktop chat now listens too, re-reads the rows, and repaints the cards.
The browser also refuses to adopt the server's copy of the chat while a card is waiting, unless that copy settles the same card. This protects an answer the user has typed but not yet sent.
One extra case fixed the oldest sessions. The browser used to adopt a server copy only when the history had grown. A dead session never grows, so a chat cached before this fix kept showing its live card forever. A server copy that settles every waiting card is now adopted even with no growth. Two floors keep that path safe: the server copy may not carry fewer records than the browser's watermark, and it may not carry fewer messages than the screen.
4. Cards act where they appear
The Connect and Not-now buttons now live on the card itself, wherever the card sits in the chat. The dock at the bottom of the composer stays, but only as a shortcut that scrolls to the card. That kills the dead-card bug at its root, because a card no longer loses its buttons when a new turn pushes it up the page.
The scans that decide the tab status, the message queue hold, and which approvals are open now read the whole chat instead of only the last message.
A connect create that fails now shows the server's real reason. A duplicate reads "A connection for telegram already exists in this project." instead of "Connection failed. Please try again." The card and the reason the agent reads come from one value, so they cannot drift apart. Retry is hidden when a retry cannot work.
Three things this PR does not change
After a reload, a settled failed connect card offers one more Retry click. That click re-derives the truthful state and the buttons go away. The refresh is live-only on purpose, because writing it down would mean settling a part that is already settled, which the double-settle guards exist to prevent. One wasted click per reload, never a loop.
Reusing a connection that already exists stays out of scope. It is issue #5911. This PR only makes the real reason visible.
Answering form and connect cards from mobile never existed. It stays its own ticket.
Tests
resolvedapproval-only.respondedwith its answer intact.matrix_i1_settlementandmatrix_i2_card_journeys.matrix_l4_client_tool_lifecyclewas tightened, because its docstring recorded the defect this PR removes.Live QA ran on the dev stack against the full journey list: answer and reload, decline and reload, close the tab and open it again, two connects in one conversation, old pre-fix sessions with and without a warm cache, and a real Telegram bot token through create, remove, and create again.
What to QA