Skip to content

Commit 24fd4e1

Browse files
mmabroukmmabrouk
committed
fix(web): dispatch re-parked approvals and reopen sentinel-sealed cards on rebuild
1 parent ba5c6b5 commit 24fd4e1

5 files changed

Lines changed: 145 additions & 34 deletions

File tree

‎web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.ts‎

Lines changed: 54 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type {SessionRecord} from "@agenta/entities/session"
22
import {describe, expect, it} from "vitest"
33

4-
import {transcriptToMessages} from "./transcriptToMessages"
4+
import {APPROVED_EXECUTION_RESULT_UNKNOWN, transcriptToMessages} from "./transcriptToMessages"
55

66
const record = (id: string, payload: Record<string, unknown>, sender = "agent"): SessionRecord => ({
77
id,
@@ -99,7 +99,7 @@ describe("transcriptToMessages approval hydration", () => {
9999
expect(part.approval).toEqual({id: "approval-1", approved: false})
100100
})
101101

102-
it("rebuilds the persisted incident order with call a executed and call b pending", () => {
102+
it("reopens deferred call b when its turn-2 approval request arrives", () => {
103103
const messages = transcriptToMessages([
104104
record("record-user", {type: "message", text: "run both writes"}, "user"),
105105
record("record-call-a", {
@@ -108,6 +108,12 @@ describe("transcriptToMessages approval hydration", () => {
108108
name: "bash",
109109
input: {command: "write a"},
110110
}),
111+
record("record-call-b", {
112+
type: "tool_call",
113+
id: "tool-b",
114+
name: "bash",
115+
input: {command: "write b"},
116+
}),
111117
record("record-request-a", {
112118
type: "interaction_request",
113119
id: "approval-a",
@@ -121,18 +127,13 @@ describe("transcriptToMessages approval hydration", () => {
121127
isError: true,
122128
}),
123129
record("record-done-turn-1", {type: "done"}),
130+
record("record-user-turn-2", {type: "message", text: "run both writes"}, "user"),
124131
record("record-response-a", {
125132
type: "interaction_response",
126133
id: "approval-a",
127134
kind: "user_approval",
128135
payload: {toolCallId: "tool-a", approved: true},
129136
}),
130-
record("record-result-a", {
131-
type: "tool_result",
132-
id: "tool-a",
133-
output: "tool-a real output",
134-
isError: false,
135-
}),
136137
record("record-request-b", {
137138
type: "interaction_request",
138139
id: "approval-b",
@@ -146,6 +147,12 @@ describe("transcriptToMessages approval hydration", () => {
146147
},
147148
},
148149
}),
150+
record("record-result-a", {
151+
type: "tool_result",
152+
id: "tool-a",
153+
output: APPROVED_EXECUTION_RESULT_UNKNOWN,
154+
isError: true,
155+
}),
149156
record("record-done-turn-2", {type: "done"}),
150157
])
151158

@@ -161,15 +168,51 @@ describe("transcriptToMessages approval hydration", () => {
161168
const callB = assistantParts.find((part) => part.toolCallId === "tool-b")
162169

163170
expect(callA).toMatchObject({
164-
state: "output-available",
165-
output: "tool-a real output",
171+
state: "output-error",
172+
errorText: APPROVED_EXECUTION_RESULT_UNKNOWN,
173+
approval: {id: "approval-a", approved: true},
166174
})
167-
expect(callB).toMatchObject({
175+
expect(callB).toEqual({
176+
type: "tool-bash",
177+
toolCallId: "tool-b",
168178
state: "approval-requested",
179+
input: {command: "write b"},
169180
approval: {id: "approval-b"},
170181
})
171182
expect(assistantParts.filter((part) => part.state === "approval-requested")).toEqual([
172183
callB,
173184
])
174185
})
186+
187+
it("keeps a real tool error closed when a late approval request arrives", () => {
188+
const part = firstPart([
189+
record("record-call-b", {
190+
type: "tool_call",
191+
id: "tool-b",
192+
name: "bash",
193+
input: {command: "write b"},
194+
}),
195+
record("record-result-b", {
196+
type: "tool_result",
197+
id: "tool-b",
198+
output: "permission denied",
199+
isError: true,
200+
}),
201+
record("record-done-turn-1", {type: "done"}),
202+
record("record-request-b", {
203+
type: "interaction_request",
204+
id: "approval-b",
205+
kind: "user_approval",
206+
payload: {toolCallId: "tool-b"},
207+
}),
208+
])
209+
210+
expect(part).toEqual({
211+
type: "tool-bash",
212+
toolCallId: "tool-b",
213+
state: "output-error",
214+
input: {command: "write b"},
215+
errorText: "permission denied",
216+
})
217+
})
175218
})

‎web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,15 @@ import type {UIMessage} from "ai"
1818

1919
type Part = Record<string, unknown>
2020

21+
// Mirrors services/runner/src/tracing/otel.ts; park sentinels report skipped or unobserved work, not final results.
22+
export const DEFERRED_NOT_EXECUTED_PREFIX = "DEFERRED_NOT_EXECUTED"
23+
export const APPROVED_EXECUTION_RESULT_UNKNOWN =
24+
"APPROVED_EXECUTION_RESULT_UNKNOWN: the approved call started but its result was not observed before the pause ended the turn; do not assume it failed and do not retry a side-effecting call."
25+
export const APPROVED_EXECUTION_RESULT_UNKNOWN_PREFIX = APPROVED_EXECUTION_RESULT_UNKNOWN.slice(
26+
0,
27+
APPROVED_EXECUTION_RESULT_UNKNOWN.indexOf(":"),
28+
)
29+
2130
interface DraftMessage {
2231
id: string
2332
role: "user" | "assistant"
@@ -81,6 +90,14 @@ const newDraft = (id: string, role: "user" | "assistant"): DraftMessage => ({
8190

8291
const toolPartType = (name?: string | null): string => (name ? `tool-${name}` : "dynamic-tool")
8392

93+
const isRunnerSentinelError = (part: Part): boolean => {
94+
const errorText = typeof part.errorText === "string" ? part.errorText : ""
95+
return (
96+
errorText.startsWith(DEFERRED_NOT_EXECUTED_PREFIX) ||
97+
errorText === APPROVED_EXECUTION_RESULT_UNKNOWN
98+
)
99+
}
100+
84101
/** Apply one transcript event's payload onto the current assistant/user draft message. */
85102
function applyEvent(
86103
draft: DraftMessage,
@@ -174,8 +191,12 @@ function applyEvent(
174191
index.tools.set(toolCallId, part)
175192
}
176193
index.approvals.set(str(payload.id), part)
177-
// Only park if still unsettled — a later `tool_result` overwrites this.
178-
if (part.state === "input-available") {
194+
const canRequestApproval =
195+
part.state === "input-available" ||
196+
(part.state === "output-error" && isRunnerSentinelError(part))
197+
if (canRequestApproval) {
198+
delete part.errorText
199+
delete part.output
179200
part.state = "approval-requested"
180201
part.approval = {id: str(payload.id)}
181202
}

‎web/oss/src/components/AgentChatSlice/components/ToolActivity.tsx‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@ import {DriveFileCard} from "@/oss/components/Drives/DriveFileCard"
2121

2222
import {partToolName, resolveToolDisplay, type ToolDisplay} from "../assets/toolDisplay"
2323
import {formatToolValue, stripFence} from "../assets/toolFormat"
24+
import {
25+
APPROVED_EXECUTION_RESULT_UNKNOWN_PREFIX,
26+
DEFERRED_NOT_EXECUTED_PREFIX,
27+
} from "../assets/transcriptToMessages"
2428
import {
2529
expandedValueAtomFamily,
2630
setExpandedAtom,
@@ -35,9 +39,12 @@ const {Text} = Typography
3539
const SETTLED = new Set(["output-available", "output-error", "output-denied"])
3640
const isSettled = (state: string) => SETTLED.has(state)
3741

38-
const DEFERRED_PREFIX = "DEFERRED_NOT_EXECUTED:"
3942
const isDeferredError = (errorText: string | undefined): boolean =>
40-
!!errorText && errorText.startsWith(DEFERRED_PREFIX)
43+
!!errorText && errorText.startsWith(DEFERRED_NOT_EXECUTED_PREFIX)
44+
const isUnknownResultError = (errorText: string | undefined): boolean =>
45+
!!errorText && errorText.startsWith(APPROVED_EXECUTION_RESULT_UNKNOWN_PREFIX)
46+
const isNonFinalRunnerError = (errorText: string | undefined): boolean =>
47+
isDeferredError(errorText) || isUnknownResultError(errorText)
4148

4249
const isNotHandledOutput = (output: unknown): boolean =>
4350
!!output &&
@@ -85,7 +92,9 @@ const rowSummary = (part: ToolUIPart, display?: ToolDisplay): string | null => {
8592
}
8693
if (part.state === "output-error") {
8794
const errorText = (part as {errorText?: string}).errorText
88-
return isDeferredError(errorText) ? "waiting on another approval" : "failed"
95+
if (isDeferredError(errorText)) return "waiting on another approval"
96+
if (isUnknownResultError(errorText)) return "approved, result unknown"
97+
return "failed"
8998
}
9099
if (part.state === "output-denied") return "denied"
91100
return null
@@ -100,7 +109,7 @@ const StatusIcon = ({part}: {part: ToolUIPart}) => {
100109
return <CheckCircle size={13} weight="fill" className="shrink-0 text-colorSuccess" />
101110
}
102111
if (state === "output-error") {
103-
if (isDeferredError((part as {errorText?: string}).errorText))
112+
if (isNonFinalRunnerError((part as {errorText?: string}).errorText))
104113
return <Clock size={13} className="shrink-0 text-colorTextTertiary" />
105114
return <Warning size={13} weight="fill" className="shrink-0 text-colorError" />
106115
}
@@ -153,7 +162,7 @@ const ToolRow = ({
153162
const input = (part as {input?: unknown}).input
154163
const output = (part as {output?: unknown}).output
155164
const errorText = (part as {errorText?: string}).errorText
156-
const deferred = state === "output-error" && isDeferredError(errorText)
165+
const nonFinalError = state === "output-error" && isNonFinalRunnerError(errorText)
157166
const notHandled = state === "output-available" && isNotHandledOutput(output)
158167
// `approval-responded` is resolved (the user answered) — not "running". Its execution shows on
159168
// a sibling part, so this must not spin forever (the cold-replay lingering-gate spinner).
@@ -169,8 +178,8 @@ const ToolRow = ({
169178
: live && running
170179
? "running…"
171180
: detailed
172-
? deferred
173-
? "waiting on another approval"
181+
? nonFinalError
182+
? rowSummary(part, display)
174183
: state === "output-error"
175184
? "failed"
176185
: state === "output-denied"
@@ -207,7 +216,7 @@ const ToolRow = ({
207216
) : null}
208217
{midText ? (
209218
<Text
210-
type={state === "output-error" && !deferred ? "danger" : "secondary"}
219+
type={state === "output-error" && !nonFinalError ? "danger" : "secondary"}
211220
className="!text-xs min-w-0 truncate"
212221
title={typeof midText === "string" ? midText : undefined}
213222
>
@@ -245,9 +254,9 @@ const ToolRow = ({
245254
{hasInput ? <IOBlock label="input" value={formatToolValue(input)} /> : null}
246255
{hasError ? (
247256
<IOBlock
248-
label={deferred ? "note" : "error"}
257+
label={nonFinalError ? "note" : "error"}
249258
value={stripFence(errorText)}
250-
danger={!deferred}
259+
danger={!nonFinalError}
251260
/>
252261
) : hasOutput ? (
253262
<IOBlock label="output" value={formatToolValue(output)} />
@@ -351,7 +360,7 @@ const ToolActivity = ({
351360
const failed = parts.filter(
352361
(p) =>
353362
(p.state as string) === "output-error" &&
354-
!isDeferredError((p as {errorText?: string}).errorText),
363+
!isNonFinalRunnerError((p as {errorText?: string}).errorText),
355364
).length
356365
const count = parts.length
357366
const single = count === 1 ? resolveToolDisplay(partToolName(parts[0])) : null

‎web/packages/agenta-playground/src/state/execution/agentApprovalResume.ts‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -172,16 +172,13 @@ export function agentShouldResumeAfterApproval({
172172
}
173173
if (lastResolvedIdx === -1) return false
174174

175-
// ALREADY RESUMED guard (the post-resolve loop). The cold-replay runner re-issues the approved
176-
// tool under a FRESH id, so its execution output attaches to a NEW part and the original
177-
// `approval-responded` part LINGERS in this same assistant message forever. Once the model has
178-
// continued past the approval, a new step begins — a `step-start` part appears AFTER it. Without
179-
// this guard the predicate keeps seeing the lingering `approval-responded` and auto-resends after
180-
// every completion, re-running the whole turn endlessly (the loop the HITL fix exposed).
181-
const resumedAlready = parts
182-
.slice(lastResolvedIdx + 1)
183-
.some((part) => part.type === "step-start")
184-
if (resumedAlready) return false
175+
if (!liveInteraction) {
176+
// Restored tails need this guard; exact live markers are single-use and may target a part before `step-start`.
177+
const resumedAlready = parts
178+
.slice(lastResolvedIdx + 1)
179+
.some((part) => part.type === "step-start")
180+
if (resumedAlready) return false
181+
}
185182

186183
// The AI SDK re-evaluates after message updates and waits for an in-flight stream to finish,
187184
// so an approval clicked during a resume dispatches when that stream finishes.

‎web/packages/agenta-playground/tests/unit/agentApprovalResume.test.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,30 @@ const assistantWithClientTool = (state: string, output?: unknown) => ({
4343
],
4444
})
4545

46+
const warmResumedSecondApproval = () => ({
47+
id: "a1",
48+
role: "assistant",
49+
parts: [
50+
{type: "step-start"},
51+
{
52+
type: "tool-bash",
53+
toolCallId: "call_1",
54+
state: "output-error",
55+
input: {command: "command a"},
56+
errorText: "APPROVED_EXECUTION_RESULT_UNKNOWN: result was not observed",
57+
approval: {id: "perm_1", approved: true},
58+
},
59+
{
60+
type: "tool-bash",
61+
toolCallId: "call_2",
62+
state: "approval-responded",
63+
input: {command: "command b"},
64+
approval: {id: "perm_2", approved: true},
65+
},
66+
{type: "step-start"},
67+
],
68+
})
69+
4670
describe("agentShouldResumeAfterApproval", () => {
4771
it("RESUMES on a deny-only decision (the F-036 dead-end fix)", () => {
4872
const messages = [user("do it"), assistantWithTool("approval-responded", false)]
@@ -434,6 +458,23 @@ describe("agentShouldResumeAfterApproval", () => {
434458
expect(agentShouldResumeAfterApproval({messages})).toBe(false)
435459
})
436460

461+
it("dispatches a live second-card answer before a warm-resume step tail", () => {
462+
const messages = [user("run two commands"), warmResumedSecondApproval()]
463+
464+
expect(
465+
agentShouldResumeAfterApproval({
466+
messages,
467+
liveInteraction: {kind: "approval", id: "perm_2"},
468+
}),
469+
).toBe(true)
470+
})
471+
472+
it("keeps the same warm-resume step tail inert without a live marker", () => {
473+
const messages = [user("run two commands"), warmResumedSecondApproval()]
474+
475+
expect(agentShouldResumeAfterApproval({messages})).toBe(false)
476+
})
477+
437478
it("STILL resumes on a chained second approval later in the turn", () => {
438479
// Two gates in one turn: the first was approved-and-resumed (step-start follows it), then a
439480
// SECOND gate was just approved and is the tail — its approval still needs the resume.

0 commit comments

Comments
 (0)