fix(sessions): paused-turn transcript hygiene + pause-sentinel replay nudges - #5451
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPaused stop reasons now flow through runner finalization and transcript replay. Approval sentinel outputs receive explicit transcript guidance, resume tails avoid duplicate user persistence, and the web client marks paused messages and reconciles them with completed server transcripts. ChangesPaused turn flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SandboxAgent
participant SandboxAgentOtel
participant RunnerTranscript
participant WebReplay
participant AgentConversation
SandboxAgent->>SandboxAgentOtel: finish(stopReason)
SandboxAgentOtel->>WebReplay: emit done with stopReason paused
SandboxAgent->>RunnerTranscript: render approval tool results
RunnerTranscript-->>WebReplay: emit retry or do-not-retry guidance
WebReplay->>AgentConversation: set paused metadata
AgentConversation->>AgentConversation: reconcile server and local tails
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Railway Preview Environment
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b3871f6-d134-4462-a223-3be98c4b6f10
📒 Files selected for processing (8)
services/runner/src/engines/sandbox_agent/run-turn.tsservices/runner/src/engines/sandbox_agent/transcript.tsservices/runner/src/server.tsservices/runner/src/tracing/otel.tsservices/runner/tests/unit/transcript.test.tsweb/oss/src/components/AgentChatSlice/AgentConversation.tsxweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.tsweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
7787459 to
7fd5ac7
Compare
Two coupled defects made a parked-then-resumed approval turn rehydrate wrong on a cold reload: - The runner re-persisted the user prompt on every resume. An approval reply's tail is the tool_result envelope (no text), so resolvePromptText fell back to the ORIGINAL prompt and wrote a duplicate user row. Guard the write with tailIsFreshUserMessage so the prompt persists only on the turn that introduced it. - Nothing marked a turn that ended mid-approval. finish() now stamps stopReason:"paused" on the terminal `done` record (existing wire field, no contract change); transcriptToMessages carries it to message.metadata.paused. With the duplicate row gone, AgentConversation's server-transcript adoption can no longer lean on a bumped message count. Make it state-aware: adopt when the server is ahead by count OR when the local tail is stuck paused while the server turn is terminal (a resume that completed elsewhere). Adds paused-end-marker coverage to transcriptToMessages.test.ts.
On cold replay `messageTranscript` fed both pause-terminalization sentinels to the model through the generic `[<tool> error: …]` branch. The "error" framing makes the model read a parked call as a refusal and abandon it (the parallel-approval bug), and for an approved-but-unobserved call it invites a retry of something that may already have run. Render each sentinel as an explicit, non-error nudge with opposite guidance: - DEFERRED_NOT_EXECUTED — skipped for another approval, not denied; re-issue the call. - APPROVED_EXECUTION_RESULT_UNKNOWN — may already have run; do not assume failure, do not retry a side-effecting call. The user-facing render already distinguishes both (ToolActivity); this restores the model-facing half dropped when the fix train folded the deferred check into the combined isPauseSyntheticResult.
…local tail The paused-tail adoption exception could fire when the server transcript was shorter than the local one: a lagging snapshot ending in a settled assistant message would discard newer locally persisted approval state. The intended case (a resume that completed elsewhere) always has the server at least as long, so require serverMsgs.length >= prev.length and an assistant tail for the exception.
… replay A cold reload rendered a paused-then-approved turn as two bubbles: a dangling "Write — awaiting approval" (the paused turn) plus a second "Write ✓" (the resume), because the runner re-emits the approved tool_call in the resume turn and every 'done' record closed the message. Verified against the durable records: both calls share one toolCallId; the resume turn carries no user row (the dedup fix), so it folds cleanly. transcriptToMessages now keeps the draft open on stopReason:"paused" so the resume folds into the same message, clears the paused flag once it completes, and dedupes a re-emitted tool_call by toolCallId so the call settles to a single ✓ — matching the live view.
7ff8d50 to
83df825
Compare
…tale trace comment Trace hover works on cold reload now that the runner stamps traceId on the done event, but two loose ends around it: extractTraceId's docstring still claimed records carry no trace link (a no-op until BE stamps), which is no longer true; and the paused-turn fold took the FIRST done's traceId, so a merged HITL turn's 'View full trace' pointed at the paused trace instead of the resume trace where the approved tool actually ran. Switch the done handler to last-wins so the merged turn takes the resume trace; a normal single-done turn is unaffected.
Records are the source for server-side history reconstruction, so a dropped record silently corrupts the reconstructed context. Behind AGENTA_RECORDS_DURABLE (off by default → the legacy fire-and-forget path is byte-for-byte unchanged), the ingest retry becomes stronger (more attempts, exponential backoff; count env-tunable via AGENTA_RECORDS_INGEST_MAX_RETRIES) and a record that still exhausts its retries is COUNTED per session. takePersistFailures lets the turn-end drain learn whether the session's durable history is complete — the signal a later phase uses to decide whether reconstruction is safe or the client must resend full history.
Over the 64 KB cap, publish_record replaced the whole record body with {"_truncated": True},
losing the event's type/id and all content — fine for a replay convenience, disqualifying when
records are the authoritative source for server-side history reconstruction. Behind
AGENTA_RECORDS_SMART_TRUNCATION (off by default → legacy whole-body drop), keep the event
structure and trim only the largest string field(s) to fit, each marked, with a fallback to a
discriminator-only shape when non-string bloat can't be trimmed. New env group agenta.sessions.records.
… log The server-side inverse of buildPersistingEmitter: fold ordered records into ChatMessage[] keyed on record_source (user record flushes the assistant turn + starts a user turn; agent records accumulate as ACP content blocks). Output matches the vercel adapter's shape exactly — text, tool_call, and tool_result blocks with toolCallId/toolName paired across the stream — so buildTurnText/priorMessages and the responder's tool_call↔tool_result binding consume it unchanged, and a still-parked tool_call survives so a HITL answer on the last message can bind. Pure, no I/O; reasoning/usage/done/interaction-lifecycle events are dropped as non-context.
… minimal history Wires the reconstructor into the run: fetchSessionRecords reads a session's durable record log (POST /sessions/records/query), and reconstructHistoryIfNeeded rebuilds prior turns when the flag AGENTA_SESSIONS_RECONSTRUCT is on AND the client sent a minimal history (messages.length <= 1). runTurn reassigns request to [...reconstructed, ...inbound] before building turnText, so the cold-path transcript, priorMessages, the responder's tool_call binding, and the otel run all see the same rebuilt history. Strict no-op by default: flag off or a full inbound history skips it entirely (never even queries), and any miss/fetch failure falls back to the inbound history. 1261 runner unit tests pass.
…abled Behind NEXT_PUBLIC_SESSIONS_LAST_MESSAGE_ONLY (default off; MUST pair with the backend's AGENTA_SESSIONS_RECONSTRUCT), buildAgentRequest sends just the newest user message and lets the runner rebuild prior turns from the durable record log — shrinking request and trace payloads (the AGE-3970 trace-drawer OOM driver). Only a fresh user turn is trimmed: a HITL resume, whose trailing turn carries the settled answer, keeps the full history so the answer still binds to its tool call, as does any run without a session id. New isSessionsLastMessageOnlyEnabled helper.
… examples The runner service deliberately has no env_file, so AGENTA_SESSIONS_RECONSTRUCT and AGENTA_RECORDS_DURABLE were unreachable in every edition and could not be turned on outside a unit test. List them alongside the other runner variables and document all four flags in the env examples. Claude-Session: https://claude.ai/code/session_01KM69J7uHafgciiN5zfG7qR
…st.ts Three unformatted lines in this test (added upstream in fix/transcript-hygiene) fail both the format and lint CI jobs, and that red is inherited by every PR stacked on top, including this one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…st.ts Three unformatted lines in this new test fail both the format and lint CI jobs, and that red is inherited by every PR stacked on top of this one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… nullability Two build-breaking type errors left over from the storage rework's schema change (fetchState now hits the stream endpoint, not the old session_state one): - StatesTab read sandbox_id/data off the stream row. Both moved: sandbox_id now lives on the latest session_turns row (session_states no longer exists), so fetch it from there instead. - The local sessionStreamSchema required a non-null id, but the backend's Identifier mixin has always declared id as Optional[UUID]. That made the Fern-generated type unassignable where StreamsTab's deriveStreamNest call expected the local schema's type. Loosened to match backend reality. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fix(hosting): wire the session history flags into compose and the env examples
[feat] Last-message-only turns: runner rebuilds history from records
Stacks on
feat/sessions-storage-rework(PR #5436). Two of the approval fixes from the handoff, rebuilt against the merged code.1. Paused-turn transcript hygiene (handoff task 2)
A parked-then-resumed approval turn rehydrated wrong on a cold reload:
tool_resultenvelope (no text), soresolvePromptTextfell back to the original prompt and wrote a second user row. Guarded withtailIsFreshUserMessage.finish()now stampsstopReason:"paused"on the terminaldonerecord (existing wire field — no contract change);transcriptToMessagescarries it tomessage.metadata.paused.AgentConversation's adoption heuristic can no longer lean on a bumped message count — it's now state-aware (adopt when the server is ahead by count OR the local tail is stuck paused while the server turn is terminal).2. Pause-sentinel replay nudges (handoff task 3)
On cold replay
messageTranscriptfed both pause sentinels to the model through the generic[<tool> error: …]branch. "error" makes the model read a parked call as a refusal and abandon it, and for an approved-but-unobserved call it invites a retry of something that may already have run. Each now renders as an explicit, non-error nudge with opposite guidance:DEFERRED_NOT_EXECUTED→ skipped for another approval, not denied; re-issue the call.APPROVED_EXECUTION_RESULT_UNKNOWN→ may already have run; do not assume failure, do not retry a side-effecting call.The user-facing render already distinguished both (
ToolActivity); this restores the model-facing half dropped when the fix train folded the deferred check intoisPauseSyntheticResult.Verification
tsc --noEmitclean; 1246/1246 unit tests pass (incl. the wire-contract golden)pnpm --filter @agenta/oss exec tsc --noEmit— zero errors in the changed filestranscriptToMessages.test.ts8/8,transcript.test.ts22/22