From fcc734815601722cb342bbb3f52c3fb6c7019e89 Mon Sep 17 00:00:00 2001 From: Arda Erzin Date: Fri, 17 Jul 2026 22:57:08 +0200 Subject: [PATCH 01/11] fix(runner): render a deferred parallel-tool sibling as a retry nudge When the model fires several tool calls that each need approval, the runner pauses on one and force-settles the rest with DEFERRED_NOT_EXECUTED. On cold replay messageTranscript rendered that as `[ error: ...]`, which the model read as a denial and abandoned the call. Render it instead as the same "call it again with the same arguments" nudge an approved-but-unexecuted call gets, so the model re-issues it and its own gate surfaces. --- .../src/engines/sandbox_agent/transcript.ts | 30 ++++++++--- services/runner/src/responder.ts | 10 ++-- services/runner/tests/unit/transcript.test.ts | 52 +++++++++++++++++-- 3 files changed, 77 insertions(+), 15 deletions(-) diff --git a/services/runner/src/engines/sandbox_agent/transcript.ts b/services/runner/src/engines/sandbox_agent/transcript.ts index db5851aea6..84a3ea7af4 100644 --- a/services/runner/src/engines/sandbox_agent/transcript.ts +++ b/services/runner/src/engines/sandbox_agent/transcript.ts @@ -5,7 +5,7 @@ import { messageText, resolvePromptText, } from "../../protocol.ts"; -import { approvalDecisionOf } from "../../responder.ts"; +import { approvalDecisionOf, isDeferredNotExecuted } from "../../responder.ts"; export type ApprovalRenderHint = "executed" | "lastPending" | "stalePending"; @@ -33,12 +33,17 @@ export function priorMessages(request: AgentRunRequest): ChatMessage[] { // whose text matches the prompt being sent, not every matching turn. let lastMatch = -1; for (let i = messages.length - 1; i >= 0; i--) { - if (messages[i].role === "user" && messageText(messages[i].content) === latest) { + if ( + messages[i].role === "user" && + messageText(messages[i].content) === latest + ) { lastMatch = i; break; } } - return lastMatch === -1 ? messages : messages.filter((_, i) => i !== lastMatch); + return lastMatch === -1 + ? messages + : messages.filter((_, i) => i !== lastMatch); } /** @@ -148,7 +153,9 @@ export function messageTranscript( if (block.type === "text" && typeof block.text === "string") { parts.push(block.text); } else if (block.type === "tool_call") { - parts.push(`[called ${block.toolName ?? "tool"}(${safeJson(block.input)})]`); + parts.push( + `[called ${block.toolName ?? "tool"}(${safeJson(block.input)})]`, + ); } else if (block.type === "tool_result") { const decision = approvalDecisionOf(block); if (decision !== undefined) { @@ -167,6 +174,17 @@ export function messageTranscript( } else { parts.push(`[user DENIED ${toolName}; the call was not executed.]`); } + } else if (isDeferredNotExecuted(block)) { + // A sibling of a parallel tool batch: the turn paused on ANOTHER call's approval, so this + // one was skipped this turn — NOT denied, NOT failed. On the generic path below it renders + // as `[ error: DEFERRED_NOT_EXECUTED …]`, and the model reads that "error" as a + // refusal and abandons the call. Render it instead as the same "call it again now" nudge + // an approved-but-unexecuted call gets (they are the same situation to the model: the call + // has not run yet and must be re-issued), so the model retries and its fresh gate surfaces. + const toolName = block.toolName ?? "tool"; + parts.push( + `[${toolName} was NOT run — the turn paused for another approval first, so it was skipped, not denied. Call ${toolName} again with the same arguments now to run it.]`, + ); } else { const body = capToolResultBody(safeJson(block.output)); parts.push( @@ -273,9 +291,7 @@ export function buildTurnText( const maxChars = Number( process.env.AGENTA_AGENT_HISTORY_MAX_CHARS ?? DEFAULT_HISTORY_MAX_CHARS, ); - const lines = history.map( - ({ message, text }) => `${message.role}: ${text}`, - ); + const lines = history.map(({ message, text }) => `${message.role}: ${text}`); const full = lines.join("\n"); let transcript = full; let evicted = 0; diff --git a/services/runner/src/responder.ts b/services/runner/src/responder.ts index 21b5ec328a..be37c3868f 100644 --- a/services/runner/src/responder.ts +++ b/services/runner/src/responder.ts @@ -484,11 +484,15 @@ function isPermissionDecision(value: unknown): value is PermissionDecision { } /** - * A sibling force-settle result: the turn paused on another interaction, so this client tool was - * never executed and carries the `DEFERRED_NOT_EXECUTED` sentinel. It is not a browser output — the + * A sibling force-settle result: the turn paused on another interaction, so this tool call was + * never executed and carries the `DEFERRED_NOT_EXECUTED` sentinel. It is not a real output — the * model is meant to re-issue the same call, which must re-park rather than resolve against this. + * + * Exported because the cold-replay transcript renderer (`messageTranscript`) must recognise the same + * block: left on the generic tool-result path it renders as `[ error: …]`, which the model + * reads as a denial and gives up instead of retrying — the parallel-approval bug. */ -function isDeferredNotExecuted(block: ContentBlock): boolean { +export function isDeferredNotExecuted(block: ContentBlock): boolean { return ( typeof block.output === "string" && block.output.startsWith(DEFERRED_NOT_EXECUTED_PREFIX) diff --git a/services/runner/tests/unit/transcript.test.ts b/services/runner/tests/unit/transcript.test.ts index 9c26d817bc..fd55d754af 100644 --- a/services/runner/tests/unit/transcript.test.ts +++ b/services/runner/tests/unit/transcript.test.ts @@ -6,6 +6,7 @@ import { messageTranscript, TOOL_RESULT_RENDER_MAX_CHARS, } from "../../src/engines/sandbox_agent/transcript.ts"; +import { TOOL_NOT_EXECUTED_PAUSED } from "../../src/tracing/otel.ts"; import type { AgentRunRequest, ContentBlock } from "../../src/protocol.ts"; const COMMIT_TOOL = "mcp__agenta-tools__commit_revision"; @@ -64,7 +65,10 @@ describe("messageTranscript", () => { assert.match(transcript, /APPROVED mcp__agenta-tools__commit_revision/); assert.match(transcript, /NOT run yet/); - assert.match(transcript, /Call the tool again with the same arguments now to execute it/); + assert.match( + transcript, + /Call the tool again with the same arguments now to execute it/, + ); assert.doesNotMatch(transcript, /returned: \{"approved":true\}/); }); @@ -103,6 +107,34 @@ describe("messageTranscript", () => { assert.match(transcript, /APPROVED approval_status/); assert.match(transcript, /NOT run yet/); }); + + // The parallel-approval bug: when the model fires several gated tool calls at once, the runner + // pauses on ONE and force-settles the siblings with `TOOL_NOT_EXECUTED_PAUSED` (isError=true). + // On the generic path that renders as `[ error: …]`, which the model reads as a refusal — + // so it abandons the call instead of re-issuing it, and the second approval never surfaces. + it("renders a deferred (paused-sibling) result as a retry nudge, never as an error", () => { + const transcript = messageTranscript([ + { + type: "tool_result", + toolCallId: "call-2", + toolName: "Read File", + output: TOOL_NOT_EXECUTED_PAUSED, + isError: true, + }, + ]); + + // The model must be told to re-issue the call, in the same "call it again now" language an + // approved-but-unexecuted call already gets. + assert.match(transcript, /Read File was NOT run/); + assert.match(transcript, /skipped, not denied/); + assert.match( + transcript, + /Call Read File again with the same arguments now to run it/, + ); + // The load-bearing regression guard: the literal word the model was reading as a refusal. + assert.doesNotMatch(transcript, /Read File error:/); + assert.doesNotMatch(transcript, /DEFERRED_NOT_EXECUTED/); + }); }); describe("tool result render cap", () => { @@ -202,7 +234,8 @@ describe("buildTurnText history window", () => { describe("buildTurnText approval-resume closing frame", () => { it("closes with the resume instruction when the newest content is an approved pending call", () => { - const staleCommand = "search for tools and add them if needed use the skill"; + const staleCommand = + "search for tools and add them if needed use the skill"; const request: AgentRunRequest = { messages: [ { role: "user", content: staleCommand }, @@ -217,8 +250,14 @@ describe("buildTurnText approval-resume closing frame", () => { assert.doesNotMatch(text, /The user now says/); // The stale command stays in the replayed history, not in the closing frame. const closing = text.slice(text.lastIndexOf("\n\n")); - assert.ok(!closing.includes(staleCommand), "stale command is out of the frame"); - assert.ok(text.includes(`user: ${staleCommand}`), "stale command stays in history"); + assert.ok( + !closing.includes(staleCommand), + "stale command is out of the frame", + ); + assert.ok( + text.includes(`user: ${staleCommand}`), + "stale command stays in history", + ); }); it("keeps the resume frame when the approval envelope rides a trailing user turn", () => { @@ -252,7 +291,10 @@ describe("buildTurnText approval-resume closing frame", () => { { role: "assistant", content: toolApprovalContent(true) }, ]); - assert.match(text, /Continue the conversation\. The user now says:\ncontinue/); + assert.match( + text, + /Continue the conversation\. The user now says:\ncontinue/, + ); assert.doesNotMatch(text, /responded to the pending approval above/); }); From 7cb1d35403227c0dff577610fae281fd8ccd3ce6 Mon Sep 17 00:00:00 2001 From: Arda Erzin Date: Fri, 17 Jul 2026 22:57:21 +0200 Subject: [PATCH 02/11] feat(runner): hold parallel approval gates and resume them together The harness delivers parallel tool-call permission requests concurrently (~0.5s apart), but the runner paused on the first and dropped the rest, so a multi-file approval took one round-trip per file. Collect-then-pause: emit every concurrent gate and debounce the pause (env AGENTA_RUNNER_APPROVAL_COLLECT_MS, default 800ms) so the whole batch parks together; the resume answers every gate on the live session before the blocked prompt continues. A gate that arrives after the window still falls back to the cold retry-nudge path. --- .../engines/sandbox_agent/acp-interactions.ts | 32 ++- .../sandbox_agent/environment-setup.ts | 3 +- .../runner/src/engines/sandbox_agent/pause.ts | 42 +++ .../src/engines/sandbox_agent/run-turn.ts | 179 ++++++------ .../sandbox_agent/runtime-contracts.ts | 47 +++- services/runner/src/server.ts | 127 +++++---- services/runner/src/tracing/otel.ts | 29 +- .../unit/session-keepalive-approval.test.ts | 254 ++++++++++++++---- 8 files changed, 506 insertions(+), 207 deletions(-) diff --git a/services/runner/src/engines/sandbox_agent/acp-interactions.ts b/services/runner/src/engines/sandbox_agent/acp-interactions.ts index 89cd808360..3102e2037b 100644 --- a/services/runner/src/engines/sandbox_agent/acp-interactions.ts +++ b/services/runner/src/engines/sandbox_agent/acp-interactions.ts @@ -51,6 +51,13 @@ export interface AttachPermissionResponderInput { * gracefully because a paused Claude turn never resolves `session.prompt()` on its own. */ onPause?: () => void; + /** + * Collect-then-pause seam for parkable APPROVAL gates (keep-alive only). Parallel gates arrive + * staggered, so instead of ending the turn on the first, each gate calls this to (re)arm a + * debounced pause — the turn parks once the harness goes quiet, with the whole batch of gates + * emitted. When absent (cold path, client tools) the gate falls back to `onPause` (pause now). + */ + onScheduleApprovalPause?: () => void; log?: (msg: string) => void; /** Called with the ACP tool-call id when a gate pauses the turn. */ onPausedToolCall?: (id: string) => void; @@ -112,6 +119,7 @@ export function attachPermissionResponder({ latch, serverPermissions = new Map(), onPause, + onScheduleApprovalPause, log, onPausedToolCall, onCreateInteraction, @@ -155,27 +163,33 @@ export function attachPermissionResponder({ gate: GateDescriptor, gateType: ParkedApprovalGateType, ): void => { - // Signal the parkable gate BEFORE the latch so a keep-alive resume can record the pending - // permission id and the multi-gate detector counts every pending gate (not just the first). + // Record the parkable gate so a keep-alive resume can answer it via `respondPermission`. In + // collect-then-pause mode every gate is recorded (the whole parallel batch parks together). const gateToolCallId = stringValue(req?.toolCall?.toolCallId); + const eventId = interactionEventId(id, gateToolCallId); onUserApprovalGate?.({ permissionId: id, toolCallId: gateToolCallId ?? "", toolName: gate.toolName, args: gate.args, - interactionToken: interactionEventId(id, gateToolCallId), + interactionToken: eventId, gateType, }); - if (!latch.tryAcquire()) return; - const toolCallId = stringValue(req?.toolCall?.toolCallId); - const eventId = interactionEventId(id, toolCallId); - if (toolCallId) onPausedToolCall?.(toolCallId); + // Keep-alive (`onScheduleApprovalPause` present): emit EVERY concurrent gate and debounce the + // pause, so a parallel batch surfaces as N approval cards that park together. Cold path keeps + // the one-pause-per-turn latch — a single gate emits, and a multi-gate turn tears down cold as + // before (its siblings are settled and replayed as retry nudges). + const collecting = Boolean(onScheduleApprovalPause); + if (!collecting && !latch.tryAcquire()) return; + // A duplicate delivery of the SAME gate (same id + tool-call) must not double-emit its card. + if (createdInteractionIds.has(eventId)) return; + if (gateToolCallId) onPausedToolCall?.(gateToolCallId); run.emitEvent({ type: "interaction_request", id: eventId, kind: "user_approval", payload: { - toolCallId, + toolCallId: gateToolCallId, toolCall: stampResolvedName(req?.toolCall, gate), availableReplies: stringArray(req?.availableReplies), options: req?.options, @@ -183,7 +197,7 @@ export function attachPermissionResponder({ }); createdInteractionIds.add(eventId); onCreateInteraction?.(eventId, gate.toolName, gate.args, "user_approval"); - onPause?.(); + (onScheduleApprovalPause ?? onPause)?.(); }; const pauseClientTool = ( diff --git a/services/runner/src/engines/sandbox_agent/environment-setup.ts b/services/runner/src/engines/sandbox_agent/environment-setup.ts index e27d25fbb7..fee4bc3a97 100644 --- a/services/runner/src/engines/sandbox_agent/environment-setup.ts +++ b/services/runner/src/engines/sandbox_agent/environment-setup.ts @@ -347,8 +347,9 @@ export async function prepareEnvironmentSetup( closeToolMcp: undefined, currentTurn: undefined, lastTurnToolCallIds: [], - parkedApproval: undefined, + parkedApprovals: [], approvalGateCount: 0, + deferredSiblingSettled: false, destroyed: false, destroy: async () => {}, clearTurn: () => {}, diff --git a/services/runner/src/engines/sandbox_agent/pause.ts b/services/runner/src/engines/sandbox_agent/pause.ts index 15bfb4d22b..98e054590f 100644 --- a/services/runner/src/engines/sandbox_agent/pause.ts +++ b/services/runner/src/engines/sandbox_agent/pause.ts @@ -13,6 +13,7 @@ export class PendingApprovalPauseController { private readonly pausedToolCallIds = new Set(); private resolvePause: (() => void) | undefined; private eventDrain: Promise = Promise.resolve(); + private pauseTimer: ReturnType | undefined; readonly signal: Promise; @@ -24,8 +25,49 @@ export class PendingApprovalPauseController { }); } + /** + * Debounced pause: (re)arm a timer so the turn pauses only once the harness has been quiet for + * `delayMs`. Parallel approval gates arrive staggered (~0.5s apart, measured), so pausing on the + * FIRST would strand the rest; each incoming gate calls this again to extend the window, and the + * turn parks once with the whole batch. A late gate that lands after the timer fires falls back to + * the straggler path (force-settled → cold retry), exactly as a non-collected gate does today. + * `pause()` (client tool, non-parkable, teardown) still ends the turn immediately. + */ + schedulePause(delayMs: number): void { + if (this.pendingApproval) return; // already paused; the batch is closed + // No collection window: pause on this gate immediately (synchronous), i.e. today's + // one-gate-per-turn behavior. Also the deterministic path for tests. + if (delayMs <= 0) { + this.pause(); + return; + } + if (this.pauseTimer) clearTimeout(this.pauseTimer); + this.pauseTimer = setTimeout(() => { + this.pauseTimer = undefined; + this.pause(); + }, delayMs); + // Do not keep the event loop alive solely for the collection window. + this.pauseTimer.unref?.(); + } + + /** + * Cancel any armed collect-window timer without pausing. Turn-scoped, like `runLimits.dispose()`: + * `run-turn.ts` calls it in `finally` so a timer this turn armed can never fire against the pooled + * `env` after the turn exited some other way (run-limit trip, throw). No-op once `pause()` ran. + */ + dispose(): void { + if (this.pauseTimer) { + clearTimeout(this.pauseTimer); + this.pauseTimer = undefined; + } + } + pause(): void { if (this.pendingApproval) return; + if (this.pauseTimer) { + clearTimeout(this.pauseTimer); + this.pauseTimer = undefined; + } this.pendingApproval = true; let destroyResult: Promise | void | undefined; try { diff --git a/services/runner/src/engines/sandbox_agent/run-turn.ts b/services/runner/src/engines/sandbox_agent/run-turn.ts index 7139732658..09c276ab11 100644 --- a/services/runner/src/engines/sandbox_agent/run-turn.ts +++ b/services/runner/src/engines/sandbox_agent/run-turn.ts @@ -51,10 +51,10 @@ import { resolveRunLimits, } from "./run-limits.ts"; import { + approvalCollectWindowMs, RUN_LIMIT_TRIPPED, sendLastMessageOnly, type CurrentTurn, - type ParkedApproval, type RunTurnOptions, type SessionEnvironment, } from "./runtime-contracts.ts"; @@ -92,13 +92,16 @@ export async function runTurn( // Reset the per-turn approval-park bookkeeping. A fresh turn starts with no parked gate; this // turn re-records it only if it pauses on a Claude ACP permission gate. (The dispatch has // already captured any prior park into `opts.resume` before calling us.) - env.parkedApproval = undefined; + env.parkedApprovals = []; env.approvalGateCount = 0; + env.deferredSiblingSettled = false; // Hoisted so the catch can flush a partial trace (mirroring the pre-split `otel?` handling — // a createOtel throw must still return `{ ok: false }`, not propagate raw) and the finally can // stop this turn's relay on EVERY exit path (a cleared sink must never orphan it). let otel: ReturnType | undefined; let activeTurn: CurrentTurn | undefined; + // Exposed to `finally` so an armed collect-window timer is cancelled on every exit path (see dispose). + let cancelPauseSchedule: (() => void) | undefined; // Time-based run deadlines (total/idle/TTFB/per-tool-call) for THIS turn: an idle/wedged harness // has no deadline anywhere, so a silent or hung turn would hold its sandbox forever. Tripping a @@ -166,25 +169,30 @@ export async function runTurn( // announced before the winning gate can never execute this turn, and skipping the settle // here would leave them as orphaned open parts whenever the dispatch later refuses the park // (multi-gate, pool full) — `env.destroy()` does not re-run it. The exclusion keeps the - // gated (paused) call itself open, so the live resume is untouched. - run.settleOpenToolCalls( - (id) => pause.isPausedToolCall(id), - TOOL_NOT_EXECUTED_PAUSED, - ); - // Park mode: a parkable permission gate (Claude ACP or Pi ACP) recorded - // `env.parkedApproval` BEFORE firing this pause (the onUserApprovalGate hook runs before - // the single-pause latch). Keep the live session — the gated tool runs on the resume — so - // skip ONLY the mcpAbort and the destroySession. The teardown is not lost: the dispatch - // either parks the session or, if it decides not to (multi-gate, pool full), calls - // `env.destroy()` which runs them. A non-parkable pause (keep-alive off, client tool) - // never records `parkedApproval`, so it still tears down here exactly as today. - if (opts.approvalParkMode && env.parkedApproval) return; + // gated (paused) call itself open, so the live resume is untouched. A non-zero count means a + // real sibling was cut: flag it so the dispatch declines the keep-alive park (see + // `deferredSiblingSettled`) and the retry-nudge replay path runs instead. + if ( + run.settleOpenToolCalls( + (id) => pause.isPausedToolCall(id), + TOOL_NOT_EXECUTED_PAUSED, + ) > 0 + ) + env.deferredSiblingSettled = true; + // Park mode: a parkable permission gate (Claude ACP or Pi ACP) recorded a gate in + // `env.parkedApprovals` BEFORE firing this pause (the onUserApprovalGate hook runs before + // the pause). Keep the live session — the gated tool runs on the resume — so skip ONLY the + // mcpAbort and the destroySession. The teardown is not lost: the dispatch either parks the + // session or, if it decides not to (pool full), calls `env.destroy()` which runs them. A + // non-parkable pause (keep-alive off, client tool) records none, so it tears down as today. + if (opts.approvalParkMode && env.parkedApprovals.length) return; // Abort any in-flight loopback `tools/call` (a paused Claude client tool) BEFORE the // session teardown, so its handler cannot write a result after the turn ends. env.mcpAbort.abort(); env.sessionDestroyRequested = true; return env.sandbox.destroySession?.(env.session.id); }); + cancelPauseSchedule = () => pause.dispose(); // A human pause resolves this signal exactly once, the moment the turn parks for input — the one // place every pause path converges, so the one place to retire the run-limits deadlines for good. void pause.signal.then(() => runLimits.notePaused()); @@ -230,12 +238,18 @@ export async function runTurn( } run.handleUpdate(update); // A sibling announced AFTER the pause won the latch can never execute; settle it - // immediately so the client never holds an orphaned part (idempotent re-sweep). + // immediately so the client never holds an orphaned part (idempotent re-sweep). This is + // the common shape of the parallel-approval case (the winner gated first; this sibling + // never got to raise its own gate) — flag it so the dispatch declines the keep-alive park + // and the retry-nudge replay runs instead. if (pause.active) { - run.settleOpenToolCalls( - (id) => pause.isPausedToolCall(id), - TOOL_NOT_EXECUTED_PAUSED, - ); + if ( + run.settleOpenToolCalls( + (id) => pause.isPausedToolCall(id), + TOOL_NOT_EXECUTED_PAUSED, + ) > 0 + ) + env.deferredSiblingSettled = true; } } }, @@ -319,6 +333,12 @@ export async function runTurn( serverPermissions, log: logger, onPause: () => pause.pause(), + // Collect-then-pause for keep-alive approval gates: debounce the pause so a parallel batch of + // gates surfaces together (they arrive staggered). Only in park mode — the cold path keeps + // the immediate one-gate pause via `onPause`. + onScheduleApprovalPause: opts.approvalParkMode + ? () => pause.schedulePause(approvalCollectWindowMs()) + : undefined, onPausedToolCall: (id) => pause.markPausedToolCall(id), onCreateInteraction: recordPendingInteraction, onResolveInteraction: resolveInteractionToken, @@ -346,27 +366,28 @@ export async function runTurn( // only a dialog-approved (or policy-allowed) call ever executes from the relay dir. onPiGateAllowed: (info) => executionGrants.grant(info.toolName, info.args), - // Record the parkable permission gate (only in keep-alive park mode) so the dispatch can - // resume it live. Fires per pending gate (before the latch) so a parallel gate is counted; - // the single-gate resume records only the FIRST gate's answer target. `info.gateType` names - // the plane (Claude ACP vs Pi ACP) so the resume answers on the right one. + // Record EVERY parkable permission gate this turn so the dispatch can resume them live. A + // parallel batch records several (collect-then-pause); the resume answers each one whose + // decision the request carries. `info.gateType` names the plane (Claude ACP vs Pi ACP). onUserApprovalGate: opts.approvalParkMode ? (info) => { env.approvalGateCount += 1; + if (!info.permissionId || !info.toolCallId) return; + // Idempotent: a duplicate delivery of the same gate must not double-record it. if ( - env.approvalGateCount === 1 && - info.permissionId && - info.toolCallId - ) { - env.parkedApproval = { - gateType: info.gateType, - permissionId: info.permissionId, - toolCallId: info.toolCallId, - toolName: info.toolName, - args: info.args, - interactionToken: info.interactionToken, - }; - } + env.parkedApprovals.some( + (g) => g.permissionId === info.permissionId, + ) + ) + return; + env.parkedApprovals.push({ + gateType: info.gateType, + permissionId: info.permissionId, + toolCallId: info.toolCallId, + toolName: info.toolName, + args: info.args, + interactionToken: info.interactionToken, + }); } : undefined, }); @@ -437,41 +458,41 @@ export async function runTurn( if (opts.resume) { // The new (resume) turn owns streaming + tracing; the environment is already wired to route // continued events into this turn's sink (env.currentTurn was set above). Seed this run's - // trace with the parked tool call so the completing `tool_call_update` closes it and the FE + // trace with each parked tool call so the completing `tool_call_update` closes it and the FE // approval part flips to output-available even if the adapter re-announces nothing. Then - // answer the gate on the live session — the original prompt continues from here. - run.handleUpdate({ - sessionUpdate: "tool_call", - toolCallId: opts.resume.toolCallId, - title: opts.resume.toolName, - kind: opts.resume.toolName, - rawInput: opts.resume.args, - }); + // answer every gate on the live session — the original prompt continues once all are in. + const gates = opts.resume.gates; + for (const g of gates) { + run.handleUpdate({ + sessionUpdate: "tool_call", + toolCallId: g.toolCallId, + title: g.toolName, + kind: g.toolName, + rawInput: g.args, + }); + } promptPromise = Promise.resolve(opts.resume.promptPromise); promptPromise.catch(() => {}); // A parked Pi dialog gate resumes on a FRESH turn whose relay and grant ledger are new; - // grant the approved call here so the extension's execute record (written right after the - // confirm resolves) passes the relay guard. Claude resumes grant too — harmlessly, no - // guard consults it. - if (opts.resume.reply === "once") { - executionGrants.grant(opts.resume.toolName, opts.resume.args); + // grant each approved call here so the extension's execute record (written right after the + // confirm resolves) passes the relay guard. Claude resumes grant too — harmlessly, no guard + // consults it. A live-resume deny closes the seeded call as a failed tool call; flag it so the + // egress projects `tool-output-denied` (a decline), mirroring the cold decision-map deny path. + for (const g of gates) { + if (g.reply === "once") executionGrants.grant(g.toolName, g.args); + else run.markToolCallDenied(g.toolCallId); } - // A live-resume deny closes the seeded call as a failed tool call; flag it so the egress - // projects `tool-output-denied` (a decline), mirroring the cold decision-map deny path. - if (opts.resume.reply === "reject") { - run.markToolCallDenied(opts.resume.toolCallId); + // Answer every gate. The harness was blocked on ALL of them (a parallel batch), so it only + // proceeds once each `respondPermission` has landed. The fresh per-turn pause controller + // starts with an EMPTY pausedToolCallIds set, so the resumed calls' `tool_call_update` frames + // are no longer suppressed and stream through. + for (const g of gates) { + await env.session.respondPermission(g.permissionId, g.reply); + resolveInteractionToken(g.interactionToken); } - await env.session.respondPermission( - opts.resume.permissionId, - opts.resume.reply, - ); - // The gate is answered: resolve the durable interaction row (the parked pending row the cold - // path would otherwise resolve via its decision map). The fresh per-turn pause controller - // starts with an EMPTY pausedToolCallIds set, so the resumed call's `tool_call_update` frames - // are no longer suppressed and stream through — the "clear pausedToolCallIds on resume" step. - resolveInteractionToken(opts.resume.interactionToken); logger( - `[keepalive] resume answered gate reply=${opts.resume.reply} tool=${opts.resume.toolName ?? "?"}`, + `[keepalive] resume answered ${gates.length} gate(s): ` + + gates.map((g) => `${g.toolName ?? "?"}=${g.reply}`).join(", "), ); } else { promptPromise = Promise.resolve( @@ -496,19 +517,23 @@ export async function runTurn( // cancellation receives exactly one deterministic terminal result before `done`. if (stopReason === "paused") { await pause.waitForEventDrain(); - run.settleOpenToolCalls( - (id) => pause.isPausedToolCall(id), - TOOL_NOT_EXECUTED_PAUSED, - ); + if ( + run.settleOpenToolCalls( + (id) => pause.isPausedToolCall(id), + TOOL_NOT_EXECUTED_PAUSED, + ) > 0 + ) + env.deferredSiblingSettled = true; } const result = raced === PAUSED ? undefined : raced; - // A parkable pause this turn: hand the still-pending prompt promise to the parked record so a - // later resume can await the same continuation. (Set after the race so `promptPromise` exists. - // The read is asserted because the onUserApprovalGate callback set the field via an async - // mutation TS's flow analysis cannot see, so it would otherwise narrow the reset to `never`.) - const parkedThisTurn = env.parkedApproval as ParkedApproval | undefined; - if (opts.approvalParkMode && pause.active && parkedThisTurn) { - parkedThisTurn.promptPromise = promptPromise; + // A parkable pause this turn: hand the still-pending prompt promise to EVERY parked gate so a + // later resume can await the same continuation. All gates of a parallel batch share this one + // blocked prompt — it only resolves once the harness has every permission answer. (Set after + // the race so `promptPromise` exists.) + if (opts.approvalParkMode && pause.active && env.parkedApprovals.length) { + for (const parked of env.parkedApprovals) { + parked.promptPromise = promptPromise; + } } await turn.toolRelay?.stop(); logger(`prompt stopReason=${stopReason}`); @@ -613,6 +638,8 @@ export async function runTurn( } finally { // Release every run-limits timer (idempotent, never re-arms on a late event) on EVERY path. runLimits.dispose(); + // Cancel any armed collect-window timer so it can't fire against the pooled env post-exit. + cancelPauseSchedule?.(); // This turn owns its relay: stop it on EVERY exit path (the happy path already stopped it // after the prompt; stop is safe to repeat, matching the old finally). Null it afterwards so // a later `destroy()` — possibly after the dispatch cleared the sink — cannot double-stop or diff --git a/services/runner/src/engines/sandbox_agent/runtime-contracts.ts b/services/runner/src/engines/sandbox_agent/runtime-contracts.ts index 01e82d9b1e..cee255af6f 100644 --- a/services/runner/src/engines/sandbox_agent/runtime-contracts.ts +++ b/services/runner/src/engines/sandbox_agent/runtime-contracts.ts @@ -127,14 +127,24 @@ export interface ParkedApproval { promptPromise?: Promise; } -/** Answer a parked Claude ACP permission gate on the live session (the keep-alive resume input). */ -export interface ResumeApprovalInput { +/** One parked gate's answer, applied on the live session via `respondPermission`. */ +export interface ResumeApprovalGate { permissionId: string; reply: "once" | "reject"; toolCallId: string; toolName: string | undefined; args: unknown; interactionToken: string; +} + +/** + * Answer parked ACP permission gate(s) on the live session (the keep-alive resume input). A turn + * can park SEVERAL concurrent gates (a parallel tool batch); they share the ONE blocked `prompt()` + * promise, which only continues once every gate has been answered. + */ +export interface ResumeApprovalInput { + gates: ResumeApprovalGate[]; + /** The held original `prompt()` promise (one per turn, shared by every gate). */ promptPromise?: Promise; } @@ -170,6 +180,18 @@ export function sendLastMessageOnly(opts: RunTurnOptions): boolean { return Boolean(opts.continuation || opts.loaded); } +/** + * How long the runner keeps a keep-alive approval turn alive after a gate, collecting concurrent + * parallel gates before it parks (see `PendingApprovalPauseController.schedulePause`). Read per-call + * so a test can set it small. Default 800ms — comfortably above the ~0.5s inter-gate spacing the + * harness delivers at, and mostly hidden behind the human reading the first card. `0` disables + * collection (pause on the first gate), for A/B or if a harness ever streams gates too far apart. + */ +export function approvalCollectWindowMs(): number { + const raw = Number(process.env.AGENTA_RUNNER_APPROVAL_COLLECT_MS); + return Number.isFinite(raw) && raw >= 0 ? raw : 800; +} + /** * A session-scoped environment that can serve many turns. Everything expensive to build lives * here (sandbox, session, internal tool-MCP server, mounted cwd, relay/temp dirs); `destroy()` @@ -223,17 +245,26 @@ export interface SessionEnvironment { */ lastTurnToolCallIds: string[]; /** - * The Claude ACP permission gate the LAST turn paused on, or undefined. Set only for a harness - * ACP permission gate, reset at each turn start; the dispatch reads it after a paused turn to - * decide whether to park in `awaiting_approval` and, on the next request, how to resume. + * The parkable ACP permission gates the LAST turn paused on (empty if it did not pause on one). + * A parallel tool batch parks SEVERAL at once (collect-then-pause); the dispatch parks the + * session while any are pending and, on the next request, answers every one whose decision the + * resume carries. Reset at each turn start. */ - parkedApproval?: ParkedApproval; + parkedApprovals: ParkedApproval[]; /** * How many Claude ACP permission gates resolved to pendingApproval THIS turn (reset at turn - * start). More than one means parallel gates the single-gate resume cannot answer, so the - * dispatch does not park (tears down cold as today). + * start). */ approvalGateCount: number; + /** + * A parallel-batch sibling was force-settled with `TOOL_NOT_EXECUTED_PAUSED` this turn (reset at + * turn start): the winning gate paused before this call could gate, so it was skipped, not run. + * This must NOT keep-alive park — a live resume continues the harness session, where the sibling + * reads as a denial and the model gives up. Going cold routes the next turn through + * `buildTurnText`/`messageTranscript`, which replays it as a "call it again" nudge so the model + * re-issues it and its own gate surfaces. + */ + deferredSiblingSettled: boolean; destroyed: boolean; /** Complete, idempotent teardown selected from the typed teardown reason. */ destroy: (opts?: { reason?: TeardownReason }) => Promise; diff --git a/services/runner/src/server.ts b/services/runner/src/server.ts index d2ddc2987e..e64a339c93 100644 --- a/services/runner/src/server.ts +++ b/services/runner/src/server.ts @@ -412,21 +412,27 @@ export async function runWithKeepalive( } }; - // Whether a paused turn holds a single, parkable permission gate (a Claude ACP gate or a Pi - // ACP gate). Only such a gate carries a `respondPermission`-answerable id; a client-tool MCP - // pause never records `parkedApproval`, and more than one pending gate cannot be answered by - // the single-gate resume — both stay on the cold path, logged. + // Whether a paused turn holds parkable permission gate(s) (Claude ACP or Pi ACP) we can answer + // live. Each carries a `respondPermission`-answerable id; a client-tool MCP pause records none. + // A parallel BATCH now parks together (collect-then-pause) and the resume answers every gate, so + // multi-gate no longer forces cold — only a client-tool pause or a stragglered sibling does. const approvalToPark = ( env: SessionEnvironment, result: AgentRunResult, ): boolean => { if (result.stopReason !== "paused") return false; - if (!env.parkedApproval) { + if (!env.parkedApprovals?.length) { klog(`non-parkable-gate-no-park key=${key}`); return false; } - if ((env.approvalGateCount ?? 0) > 1) { - klog(`multi-gate-no-park key=${key} gates=${env.approvalGateCount}`); + // A sibling force-settled as "not run this turn" (see `deferredSiblingSettled`): a gate that + // arrived AFTER the collection window closed and the batch had already paused. Keep-alive would + // resume the live session where the harness surfaces it to the model as a denial; going cold + // replays it as a "call it again" nudge (buildTurnText/messageTranscript) so the model + // re-issues it and its own gate surfaces. (Collect-then-pause makes this rare — it only fires + // for a gate delivered slower than the window.) + if (env.deferredSiblingSettled) { + klog(`deferred-sibling-no-park key=${key}`); return false; } // An approval park waits for the HUMAN, who is still on the page even if the streaming client @@ -445,7 +451,8 @@ export async function runWithKeepalive( // one destroy, so no double-destroy is possible. The promise already carries runTurn's // swallowing catch, so no unhandled rejection is introduced. const watchParkedPrompt = (env: SessionEnvironment): void => { - const promptPromise = env.parkedApproval?.promptPromise; + // Every parked gate shares the one blocked prompt; watch it via the first. + const promptPromise = env.parkedApprovals[0]?.promptPromise; const entry = pool.get(key); if (!promptPromise || !entry || entry.environment !== env) return; promptPromise.catch(() => { @@ -472,7 +479,7 @@ export async function runWithKeepalive( }; if (approvalToPark(env, result)) { klog( - `park-approval key=${key} tool=${env.parkedApproval?.toolName ?? "?"}`, + `park-approval key=${key} gates=${env.parkedApprovals.length} tools=${env.parkedApprovals.map((g) => g.toolName ?? "?").join(",")}`, ); if ( !(await pool.park(input, config.approvalTtlMs, "awaiting_approval")) @@ -507,7 +514,7 @@ export async function runWithKeepalive( }; if (approvalToPark(env, result)) { klog( - `park-approval key=${key} tool=${env.parkedApproval?.toolName ?? "?"}`, + `park-approval key=${key} gates=${env.parkedApprovals.length} tools=${env.parkedApprovals.map((g) => g.toolName ?? "?").join(",")}`, ); if ( !(await pool.repark( @@ -657,29 +664,38 @@ export async function runWithKeepalive( // history fingerprint (an edited transcript must not continue wrongly), and a hard mount-expiry // bound — if the parked session's mount credentials are past expiry, its durable cwd can no // longer be written, so evict to cold. - const parked = existing.environment.parkedApproval; - const decision = parked - ? approvalDecisionForToolCall(request, parked.toolCallId) - : undefined; + const parkedGates = existing.environment.parkedApprovals; + // Pair every parked gate with the human's decision from THIS request. The FE only resumes once + // it has answered ALL pending gates (its resume predicate waits for every tool part to settle), + // so the whole batch resolves in one turn; a gate still missing a decision means the request is + // not that resume (fresh text, or an answer for another id) → treat as a mismatch, go cold. + const answers = parkedGates.map((gate) => ({ + gate, + decision: approvalDecisionForToolCall(request, gate.toolCallId), + })); const priorFp = historyFingerprint(priorConversation(request)); let mismatch: string | undefined; - if ( - !parked || - (parked.gateType !== "claude-acp-permission" && - parked.gateType !== "pi-acp-permission") + if (!parkedGates.length) { + mismatch = "no-parked-gate"; + } else if ( + parkedGates.some( + (g) => + g.gateType !== "claude-acp-permission" && + g.gateType !== "pi-acp-permission", + ) ) { // Defensive: only a parkable gate type (Claude ACP or Pi ACP) ever parks here. Both // resume via `respondPermission` on the live session; the daemon maps the reply by kind. mismatch = "unrecognized-gate-type"; - } else if (!decision) { - mismatch = "no-matching-approval"; // fresh user text, or an approval for another id + } else if (answers.some((a) => !a.decision)) { + mismatch = "no-matching-approval"; // fresh user text, or not every gate answered yet } else if (priorFp !== existing.historyFingerprint) { mismatch = "history"; } else if (mountCredentialsExpired(existing.credentialEpoch)) { mismatch = "credentials-expired"; } - if (mismatch || !parked || !decision) { + if (mismatch) { klog( `approval-mismatch (${mismatch ?? "unknown"}) key=${key}; evict + cold`, ); @@ -693,16 +709,25 @@ export async function runWithKeepalive( const live = pool.checkoutApproval(key); if (live) { - const reply = decision === "allow" ? "once" : "reject"; + const gates = answers.map((a) => ({ + permissionId: a.gate.permissionId, + reply: (a.decision === "allow" ? "once" : "reject") as + "once" | "reject", + toolCallId: a.gate.toolCallId, + toolName: a.gate.toolName, + args: a.gate.args, + interactionToken: a.gate.interactionToken, + })); klog( - `${reply === "once" ? "resume-approve" : "resume-reject"} key=${key} ` + - `tool=${parked.toolName ?? "?"}`, + `resume key=${key} gates=${gates.length} ` + + gates.map((g) => `${g.toolName ?? "?"}:${g.reply}`).join(","), ); let result: AgentRunResult; try { - // Answer the parked gate on the SAME live session; the original prompt continues and this - // (new) turn owns streaming + tracing. The gated tool runs with its original byte-exact - // args — no model re-issues anything, so argument drift/task restart cannot happen. + // Answer every parked gate on the SAME live session; the original prompt continues once the + // harness has all answers, and this (new) turn owns streaming + tracing. Each gated tool + // runs with its original byte-exact args — no model re-issues anything, so argument drift / + // task restart cannot happen. result = await engine.runTurn( live.environment, request, @@ -711,13 +736,8 @@ export async function runWithKeepalive( { approvalParkMode: true, resume: { - permissionId: parked.permissionId, - reply, - toolCallId: parked.toolCallId, - toolName: parked.toolName, - args: parked.args, - interactionToken: parked.interactionToken, - promptPromise: parked.promptPromise, + gates, + promptPromise: parkedGates[0]?.promptPromise, }, }, ); @@ -796,25 +816,26 @@ const runAgent: RunAgent = (request, emit, signal, options) => { }; /** - * The durable interaction token of a parked approval gate this request answers in-band, if - * any. The turn-start cancel-stale sweep must spare it: the gate belongs to the PREVIOUS turn - * (so the sweep's own `turn_id` exemption misses it), an in-band answer never transitions the - * row off `pending` (only the interactions-plane respond endpoint does), and the resume - * resolves the token after consuming the decision. Swept first, the granted gate's record - * lands as `cancelled` and the resolve 404s. + * The durable interaction tokens of parked approval gate(s) this request answers in-band. The + * turn-start cancel-stale sweep must spare them: the gates belong to the PREVIOUS turn (so the + * sweep's own `turn_id` exemption misses them), an in-band answer never transitions the row off + * `pending` (only the interactions-plane respond endpoint does), and the resume resolves each token + * after consuming its decision. Swept first, a granted gate's record lands as `cancelled` and the + * resolve 404s. A parallel batch parks several gates, so this returns every answered one. */ -function inBandAnswerToken(request: AgentRunRequest): string | undefined { +function inBandAnsweredTokens(request: AgentRunRequest): string[] { const sessionId = request.sessionId?.trim(); - if (!sessionId) return undefined; + if (!sessionId) return []; const provider = resolveKeepaliveDispatch(request, keepaliveConfigs); - if (!provider) return undefined; - const parked = + if (!provider) return []; + const parkedGates = keepalivePools[provider].awaitingApproval(sessionId)?.environment - .parkedApproval; - if (!parked) return undefined; - return approvalDecisionForToolCall(request, parked.toolCallId) !== undefined - ? parked.interactionToken - : undefined; + .parkedApprovals ?? []; + return parkedGates + .filter( + (g) => approvalDecisionForToolCall(request, g.toolCallId) !== undefined, + ) + .map((g) => g.interactionToken); } /** @@ -915,11 +936,11 @@ async function runAndStreamWithApiBaseResolved( // A new turn supersedes any prior turn's unanswered gate: cancel stale pending // interactions (sparing this turn's own, plus a parked gate this turn answers in-band — // the resume resolves that one). Best-effort, never blocks the turn. - const answeredToken = inBandAnswerToken(request); + const answeredTokens = inBandAnsweredTokens(request); void cancelStaleInteractions( sessionId, turnId, - answeredToken ? [answeredToken] : undefined, + answeredTokens.length ? answeredTokens : undefined, watchdog.credential, ); // Deny-set from THIS run's resolved provider keys + run credential (not process env, @@ -1150,7 +1171,7 @@ export function createRequestListener( // Only .message goes on the wire: the raw thrown value (even via String()) is // stack-trace-tainted to CodeQL, and the stack itself stays server-side. const message = err instanceof Error ? err.message : "Internal error"; - console.error(err instanceof Error ? err.stack ?? err.message : err); + console.error(err instanceof Error ? (err.stack ?? err.message) : err); return send(res, 500, { ok: false, error: message }); } }; @@ -1208,7 +1229,7 @@ if (isEntrypoint(import.meta.url)) { // run still returns its own error to its caller. process.on("unhandledRejection", (reason) => { process.stderr.write( - `[sandbox-agent] unhandledRejection: ${reason instanceof Error ? reason.stack ?? reason.message : String(reason)}\n`, + `[sandbox-agent] unhandledRejection: ${reason instanceof Error ? (reason.stack ?? reason.message) : String(reason)}\n`, ); }); process.on("uncaughtException", (err) => { diff --git a/services/runner/src/tracing/otel.ts b/services/runner/src/tracing/otel.ts index 5a78582b97..1d47cf10f4 100644 --- a/services/runner/src/tracing/otel.ts +++ b/services/runner/src/tracing/otel.ts @@ -172,12 +172,16 @@ function redactSpan(span: ReadableSpan, redactors: Iterable): void { } const status = span.status as { message?: string }; if (typeof status.message === "string") { - status.message = redactor.redactString(status.message, "spans") ?? status.message; + status.message = + redactor.redactString(status.message, "spans") ?? status.message; } } } -function redactAttributes(attrs: Record, redactor: Redactor): void { +function redactAttributes( + attrs: Record, + redactor: Redactor, +): void { for (const [key, value] of Object.entries(attrs)) { if (typeof value === "string") { attrs[key] = redactor.redactString(value, "spans"); @@ -296,7 +300,8 @@ class TraceBatchProcessor implements SpanProcessor { // Fall back to the env default only for a span whose OWN run's target is unknown // (untagged span, or the run already released) — never to another run's target, or a // batch could still land on an unintended endpoint/auth. - const target = (runId ? byRun?.get(runId) : undefined) ?? defaultTarget(); + const target = + (runId ? byRun?.get(runId) : undefined) ?? defaultTarget(); return new Promise((resolve) => { try { getExporter(target).export(orderParentFirst(group), (result) => { @@ -1081,11 +1086,11 @@ export interface SandboxAgentOtel { output(): string; /** The structured event log built from the ACP stream (tool calls, usage, final message). */ events(): AgentEvent[]; - /** Settle open tool calls except those intentionally left pending. */ + /** Settle open tool calls except those intentionally left pending; returns how many it settled. */ settleOpenToolCalls( isExcluded: (id: string) => boolean, message: string, - ): void; + ): number; /** * Mark a tool-call id as DENIED (the runner replied `reject` to its gate). Its closing failed * result then carries `denied: true` so the egress projects `tool-output-denied` (a decline) @@ -1435,8 +1440,8 @@ export function createSandboxAgentOtel( usage = { input: usage?.input ?? 0, output: usage?.output ?? 0, - total: typeof total === "number" ? total : usage?.total ?? 0, - cost: typeof cost === "number" ? cost : usage?.cost ?? 0, + total: typeof total === "number" ? total : (usage?.total ?? 0), + cost: typeof cost === "number" ? cost : (usage?.cost ?? 0), }; record({ type: "usage", ...usage }); } @@ -1476,10 +1481,16 @@ export function createSandboxAgentOtel( if (toolCallId) deniedToolCallIds.add(toolCallId); } + /** + * Force-settle every still-open tool call except the excluded (paused) one, with a deterministic + * result. Returns HOW MANY were settled, so the caller can tell a real parallel sibling was cut + * (>0) from a no-op re-sweep (0) — the dispatch keys its "don't keep-alive park" decision off it. + */ function settleOpenToolCalls( isExcluded: (id: string) => boolean, message: string, - ): void { + ): number { + let settled = 0; for (const [id, entry] of [...toolSpans.entries()]) { if (isExcluded(id)) continue; if (entry.span) { @@ -1488,7 +1499,9 @@ export function createSandboxAgentOtel( } toolSpans.delete(id); record({ type: "tool_result", id, output: message, isError: true }); + settled += 1; } + return settled; } /** diff --git a/services/runner/tests/unit/session-keepalive-approval.test.ts b/services/runner/tests/unit/session-keepalive-approval.test.ts index d0db503bbc..c00a2d1e2c 100644 --- a/services/runner/tests/unit/session-keepalive-approval.test.ts +++ b/services/runner/tests/unit/session-keepalive-approval.test.ts @@ -10,7 +10,7 @@ * * Run: pnpm test (or: pnpm exec vitest run tests/unit/session-keepalive-approval.test.ts) */ -import { describe, it, vi } from "vitest"; +import { describe, it, vi, beforeEach, afterEach } from "vitest"; import assert from "node:assert/strict"; import type { @@ -57,6 +57,8 @@ interface TurnScript { toolName?: string; /** How many gates pended (>1 = multi-gate; still records the first). Default 1. */ gates?: number; + /** A parallel sibling was force-settled as "not run" this turn (single gate, but not parkable). */ + deferredSibling?: boolean; /** The parked gate plane; default the Claude ACP gate. */ gateType?: ParkedApproval["gateType"]; }; @@ -75,8 +77,9 @@ interface DispatchFakeEnv { destroyed: number; turnsCleared: number; lastTurnToolCallIds: string[]; - parkedApproval?: ParkedApproval; + parkedApprovals: ParkedApproval[]; approvalGateCount: number; + deferredSiblingSettled: boolean; clearTurn: () => void; destroyImpl: () => Promise; destroy: () => Promise; @@ -112,8 +115,9 @@ function makeApprovalEngine( destroyed: 0, turnsCleared: 0, lastTurnToolCallIds: [], - parkedApproval: undefined, + parkedApprovals: [], approvalGateCount: 0, + deferredSiblingSettled: false, clearTurn: () => { env.turnsCleared += 1; }, @@ -142,38 +146,48 @@ function makeApprovalEngine( const idx = calls.turns.length; const script = scripts[idx] ?? {}; // Mirror the real runTurn per-turn reset. - env.parkedApproval = undefined; + env.parkedApprovals = []; env.approvalGateCount = 0; + env.deferredSiblingSettled = false; env.lastTurnToolCallIds = script.toolCallIds ?? []; calls.turns.push({ env, opts, idx }); if (opts?.resume) { - calls.resumes.push({ - permissionId: opts.resume.permissionId, - reply: opts.resume.reply, - toolCallId: opts.resume.toolCallId, - }); + for (const g of opts.resume.gates ?? []) { + calls.resumes.push({ + permissionId: g.permissionId, + reply: g.reply, + toolCallId: g.toolCallId, + }); + } } if (script.hold) { await new Promise((resolve) => holds.set(idx, resolve)); } if (script.approvalPause) { - env.approvalGateCount = script.approvalPause.gates ?? 1; + const gateCount = script.approvalPause.gates ?? 1; + env.approvalGateCount = gateCount; + env.deferredSiblingSettled = + script.approvalPause.deferredSibling ?? false; // The held original prompt: pending until the test settles it (mirrors the real Claude // prompt that never resolves on an unanswered gate). Carries the same swallowing catch - // runTurn attaches, so a test-driven rejection is never unhandled. + // runTurn attaches, so a test-driven rejection is never unhandled. Every parked gate of a + // batch shares this one promise. const promptPromise = new Promise((resolve, reject) => { calls.promptControls.push({ resolve, reject }); }); promptPromise.catch(() => {}); - env.parkedApproval = { - gateType: script.approvalPause.gateType ?? "claude-acp-permission", - permissionId: script.approvalPause.permissionId, - toolCallId: script.approvalPause.toolCallId, - toolName: script.approvalPause.toolName, + const ap = script.approvalPause; + // Build the parked batch: the first gate uses the script's ids, extra gates get suffixed ids + // so a multi-gate park has distinct permission/tool-call ids like the real harness. + env.parkedApprovals = Array.from({ length: gateCount }, (_, i) => ({ + gateType: ap.gateType ?? "claude-acp-permission", + permissionId: i === 0 ? ap.permissionId : `${ap.permissionId}-${i}`, + toolCallId: i === 0 ? ap.toolCallId : `${ap.toolCallId}-${i}`, + toolName: ap.toolName, args: {}, - interactionToken: script.approvalPause.toolCallId, + interactionToken: i === 0 ? ap.toolCallId : `${ap.toolCallId}-${i}`, promptPromise, - }; + })); return { ok: true, stopReason: "paused" }; } if (script.nonClaudePause) return { ok: true, stopReason: "paused" }; @@ -399,7 +413,7 @@ describe("runWithKeepalive: approval park + resume", () => { ); }); - it("logs park-approval and resume-approve/reject", async () => { + it("logs park-approval and the resume", async () => { const cap = captureStderr(); try { const { engine } = makeApprovalEngine([ @@ -416,7 +430,9 @@ describe("runWithKeepalive: approval park + resume", () => { await runWithKeepalive(pauseTurn(), undefined, undefined, ctx); await runWithKeepalive(approveResume(true), undefined, undefined, ctx); assert.ok(cap.lines.some((l) => l.includes("park-approval"))); - assert.ok(cap.lines.some((l) => l.includes("resume-approve"))); + assert.ok( + cap.lines.some((l) => l.includes("resume key=") && l.includes(":once")), + ); } finally { cap.restore(); } @@ -443,7 +459,9 @@ describe("runWithKeepalive: never-park gate types stay cold", () => { } }); - it("a multi-gate pause (parallel gates) never parks, tears down cold", async () => { + it("a multi-gate pause (parallel gates) now PARKS the whole batch", async () => { + // Behavior change (multi-gate hold): collect-then-pause records every gate and the resume + // answers them all, so a parallel batch parks live instead of tearing down cold. const cap = captureStderr(); try { const { engine, calls } = makeApprovalEngine([ @@ -454,7 +472,45 @@ describe("runWithKeepalive: never-park gate types stay cold", () => { toolName: "commit", gates: 2, }, - toolCallIds: ["tc-gate"], + toolCallIds: ["tc-gate", "tc-gate-1"], + }, + ]); + const ctx = makeCtx(engine); + const r = await runWithKeepalive(pauseTurn(), undefined, undefined, ctx); + assert.equal(r.stopReason, "paused"); + assert.equal( + calls.acquiredEnvs[0].destroyed, + 0, + "the batch parks live; nothing is destroyed", + ); + assert.equal(ctx.pool.size(), 1, "the multi-gate batch is parked"); + assert.ok( + cap.lines.some( + (l) => l.includes("park-approval") && l.includes("gates=2"), + ), + ); + } finally { + cap.restore(); + } + }); + + it("a single gate with a force-settled parallel sibling never parks, tears down cold", async () => { + // The winning gate paused before the sibling could raise its own gate, so approvalGateCount is + // 1 (the multi-gate check above misses it) but a sibling was skipped. Keep-alive here would + // resume the live session, where the harness surfaces the skipped sibling to the model as a + // denial; going cold instead replays it as a "call it again" nudge (see deferredSiblingSettled). + const cap = captureStderr(); + try { + const { engine, calls } = makeApprovalEngine([ + { + approvalPause: { + permissionId: "perm-1", + toolCallId: "tc-gate", + toolName: "Read File", + gates: 1, + deferredSibling: true, + }, + toolCallIds: ["tc-gate", "tc-sibling"], }, ]); const ctx = makeCtx(engine); @@ -463,10 +519,10 @@ describe("runWithKeepalive: never-park gate types stay cold", () => { assert.equal( calls.acquiredEnvs[0].destroyed, 1, - "the single-gate resume cannot answer >1 gate -> cold", + "a deferred sibling must route to cold, not keep-alive", ); assert.equal(ctx.pool.size(), 0); - assert.ok(cap.lines.some((l) => l.includes("multi-gate-no-park"))); + assert.ok(cap.lines.some((l) => l.includes("deferred-sibling-no-park"))); } finally { cap.restore(); } @@ -972,11 +1028,14 @@ function pausableHarness(opts: { clientTool?: boolean } = {}) { isExcluded: (id: string) => boolean, message: string, ) { + let settled = 0; for (const id of [...run.open]) { if (isExcluded(id)) continue; run.open.delete(id); run.settled.push({ id, message }); + settled += 1; } + return settled; }, denied: [] as string[], markToolCallDenied(id: string | undefined) { @@ -1043,6 +1102,19 @@ function updateEvent(update: Record) { } describe("runTurn: real approval park + respondPermission resume", () => { + // These drive the REAL pause/park/resume through a fake ACP session. Default the collect window + // to 0 so a single gate pauses synchronously (deterministic); the multi-gate test overrides it to + // exercise collect-then-pause. + const savedCollect = process.env.AGENTA_RUNNER_APPROVAL_COLLECT_MS; + beforeEach(() => { + process.env.AGENTA_RUNNER_APPROVAL_COLLECT_MS = "0"; + }); + afterEach(() => { + if (savedCollect === undefined) + delete process.env.AGENTA_RUNNER_APPROVAL_COLLECT_MS; + else process.env.AGENTA_RUNNER_APPROVAL_COLLECT_MS = savedCollect; + }); + it("parks a Claude ACP gate (session alive), then answers it live and streams the continuation", async () => { const { calls, deps, captured } = pausableHarness(); const acquired = await acquireEnvironment(engineReq, deps); @@ -1076,14 +1148,16 @@ describe("runTurn: real approval park + respondPermission resume", () => { const r1 = await p1; assert.equal(r1.stopReason, "paused"); - assert.ok(env.parkedApproval, "the parked approval was recorded"); - assert.equal(env.parkedApproval!.gateType, "claude-acp-permission"); - assert.equal(env.parkedApproval!.permissionId, "perm-1"); - assert.equal(env.parkedApproval!.toolCallId, "tc-gate"); - assert.ok( - env.parkedApproval!.promptPromise, - "the held prompt promise is captured", + assert.equal( + env.parkedApprovals.length, + 1, + "the parked approval was recorded", ); + const parked = env.parkedApprovals[0]; + assert.equal(parked.gateType, "claude-acp-permission"); + assert.equal(parked.permissionId, "perm-1"); + assert.equal(parked.toolCallId, "tc-gate"); + assert.ok(parked.promptPromise, "the held prompt promise is captured"); assert.deepEqual(env.lastTurnToolCallIds, ["tc-gate"]); assert.equal( calls.sessionDestroyed, @@ -1094,16 +1168,20 @@ describe("runTurn: real approval park + respondPermission resume", () => { // Turn 2 (resume): the dispatch cleared the sink; answer the parked gate live. env.clearTurn(); - const held = env.parkedApproval!.promptPromise!; + const held = parked.promptPromise!; const p2 = runTurn(env, approveResume(true), undefined, undefined, { approvalParkMode: true, resume: { - permissionId: "perm-1", - reply: "once", - toolCallId: "tc-gate", - toolName: "commit", - args: { message: "hi" }, - interactionToken: "tc-gate", + gates: [ + { + permissionId: "perm-1", + reply: "once", + toolCallId: "tc-gate", + toolName: "commit", + args: { message: "hi" }, + interactionToken: "tc-gate", + }, + ], promptPromise: held, }, }); @@ -1138,8 +1216,8 @@ describe("runTurn: real approval park + respondPermission resume", () => { "no new prompt was sent; the original continued", ); assert.equal( - env.parkedApproval, - undefined, + env.parkedApprovals.length, + 0, "the consumed approval was reset", ); assert.equal( @@ -1187,16 +1265,20 @@ describe("runTurn: real approval park + respondPermission resume", () => { await p1; env.clearTurn(); - const held = env.parkedApproval!.promptPromise!; + const held = env.parkedApprovals[0].promptPromise!; const p2 = runTurn(env, approveResume(false), undefined, undefined, { approvalParkMode: true, resume: { - permissionId: "perm-1", - reply: "reject", - toolCallId: "tc-gate", - toolName: "commit", - args: {}, - interactionToken: "tc-gate", + gates: [ + { + permissionId: "perm-1", + reply: "reject", + toolCallId: "tc-gate", + toolName: "commit", + args: {}, + interactionToken: "tc-gate", + }, + ], promptPromise: held, }, }); @@ -1239,8 +1321,8 @@ describe("runTurn: real approval park + respondPermission resume", () => { assert.equal(r1.stopReason, "paused"); assert.equal( - env.parkedApproval, - undefined, + env.parkedApprovals.length, + 0, "a client-tool pause is not parkable", ); assert.equal( @@ -1294,7 +1376,7 @@ describe("runTurn: real approval park + respondPermission resume", () => { const r1 = await p1; assert.equal(r1.stopReason, "paused"); - assert.ok(env.parkedApproval, "the gate parked (park path)"); + assert.equal(env.parkedApprovals.length, 1, "the gate parked (park path)"); const run1 = calls.runs[0]; assert.deepEqual( run1.settled, @@ -1361,10 +1443,78 @@ describe("runTurn: real approval park + respondPermission resume", () => { ); assert.ok(run1.open.has("tc-g1"), "the winning gate's call stays open"); - // The dispatch refuses to park a multi-gate pause and destroys: the teardown the park - // skipped (destroySession, sandbox teardown) runs through env.destroy(). + // With the collection window OFF (0), the first gate pauses before the second is processed, so + // the second is force-settled as a sibling (the straggler fallback). env.destroy() then runs the + // teardown the park skipped. await env.destroy(); assert.equal(calls.sessionDestroyed, 1, "destroy tore the session down"); assert.equal(calls.sandboxDestroyed, 1); }); + + it("collect-then-pause: two concurrent gates within the window BOTH park (no sibling settle)", async () => { + // The multi-gate-hold path. A non-zero window keeps the turn alive after the first gate so the + // second is emitted and recorded too — both park, and neither is force-settled. + process.env.AGENTA_RUNNER_APPROVAL_COLLECT_MS = "40"; + const { calls, deps, captured } = pausableHarness(); + const acquired = await acquireEnvironment(engineReq, deps); + assert.equal(acquired.ok, true); + if (!acquired.ok) return; + const env = acquired.env; + + const p1 = runTurn(env, engineReq, undefined, undefined, { + approvalParkMode: true, + }); + await flush(); + // Two parallel Read gates arrive close together (as the harness delivers them). + captured.onEvent!( + updateEvent({ + sessionUpdate: "tool_call", + toolCallId: "tc-a", + title: "Read File", + }), + ); + captured.onEvent!( + updateEvent({ + sessionUpdate: "tool_call", + toolCallId: "tc-b", + title: "Read File", + }), + ); + captured.onPermissionRequest!({ + id: "perm-a", + availableReplies: ["once", "reject"], + toolCall: { toolCallId: "tc-a", name: "Read File", rawInput: {} }, + }); + captured.onPermissionRequest!({ + id: "perm-b", + availableReplies: ["once", "reject"], + toolCall: { toolCallId: "tc-b", name: "Read File", rawInput: {} }, + }); + await flush(); + const r1 = await p1; + + assert.equal(r1.stopReason, "paused"); + assert.equal( + env.parkedApprovals.length, + 2, + "both concurrent gates parked as one batch", + ); + assert.deepEqual(env.parkedApprovals.map((g) => g.permissionId).sort(), [ + "perm-a", + "perm-b", + ]); + assert.deepEqual( + calls.runs[0].settled, + [], + "no gate was force-settled as a sibling — both were collected", + ); + // Two approval cards were emitted for the FE to show together. + assert.equal( + calls.runs[0].emitted.filter( + (e: AgentEvent) => e.type === "interaction_request", + ).length, + 2, + ); + await env.destroy(); + }); }); From b79dbf99308beb8d0367cd69543b39b7a91ed178 Mon Sep 17 00:00:00 2001 From: Arda Erzin Date: Fri, 17 Jul 2026 22:57:37 +0200 Subject: [PATCH 03/11] feat(ui): shared HeightCollapse + config-section animation primitives One CSS-native, reduced-motion-proof collapse primitive (HeightCollapse) for everything that enters/leaves the composer region and config pane, plus the config-section header shimmer + draft/agent indicator tones on ConfigAccordionSection (and a useRecentFlag helper). Migrates RevealCollapse and AgentCommitNotice onto it so the motion language is consistent. Adds the motion dependency. --- .../components/RevealCollapse.tsx | 25 ++- .../Components/AgentCommitNotice.tsx | 135 ++++++--------- web/oss/tailwind.config.ts | 13 ++ web/packages/agenta-ui/package.json | 1 + .../src/components/HeightCollapse.tsx | 66 +++++++- .../src/components/presentational/index.ts | 2 + .../section/ConfigAccordionSection.tsx | 159 ++++++++++++++++-- .../presentational/section/index.tsx | 2 + .../presentational/section/useRecentFlag.ts | 26 +++ web/pnpm-lock.yaml | 3 + 10 files changed, 318 insertions(+), 114 deletions(-) create mode 100644 web/packages/agenta-ui/src/components/presentational/section/useRecentFlag.ts diff --git a/web/oss/src/components/AgentChatSlice/components/RevealCollapse.tsx b/web/oss/src/components/AgentChatSlice/components/RevealCollapse.tsx index d009d38e7d..cea7128ff1 100644 --- a/web/oss/src/components/AgentChatSlice/components/RevealCollapse.tsx +++ b/web/oss/src/components/AgentChatSlice/components/RevealCollapse.tsx @@ -1,12 +1,14 @@ import {type ReactNode} from "react" +import {HeightCollapse} from "@agenta/ui" + /** - * Appear/disappear collapse for chat-composer chrome (connect-model banner, queued messages, HITL dock). - * Animates height 0↔auto via the grid `0fr`↔`1fr` trick plus opacity — the same idiom the ApprovalDock - * and the config sections use, so everything that enters/leaves the composer region does so consistently. - * Content is clipped by `overflow-hidden` while collapsing; `inert` drops the hidden subtree from tab - * order + a11y. Callers that render nothing when "closed" should latch their last content so it persists - * through the leave (see `ConnectModelBanner`). + * Appear/disappear collapse for chat-composer chrome (connect-model banner, queued messages, HITL + * dock). A thin wrapper over the shared {@link HeightCollapse} with `fade` + `inert`, so everything + * that enters/leaves the composer region collapses the SAME CSS-native, reduced-motion-proof way as + * the config accordion sections and the config-pane notices — one language, no `motion-safe` gate. + * Callers that render nothing when "closed" should latch their last content so it persists through + * the leave (see `ConnectModelBanner`). */ const RevealCollapse = ({ open, @@ -17,14 +19,9 @@ const RevealCollapse = ({ className?: string children: ReactNode }) => ( -
-
{children}
-
+ + {children} + ) export default RevealCollapse diff --git a/web/oss/src/components/Playground/Components/AgentCommitNotice.tsx b/web/oss/src/components/Playground/Components/AgentCommitNotice.tsx index 5d2542d3d7..06417e2724 100644 --- a/web/oss/src/components/Playground/Components/AgentCommitNotice.tsx +++ b/web/oss/src/components/Playground/Components/AgentCommitNotice.tsx @@ -1,50 +1,29 @@ -import {useEffect, useLayoutEffect, useRef, useState} from "react" +import {useLayoutEffect, useRef, useState} from "react" import {workflowMolecule} from "@agenta/entities/workflow" import {agentSelfCommitSignalAtom} from "@agenta/shared/state" +import {HeightCollapse} from "@agenta/ui" import {Robot} from "@phosphor-icons/react" import {Button} from "antd" import {useAtom, useAtomValue} from "jotai" /** - * Agent self-commit notice, rendered by MainLayout as the LAST row of the config pane's - * flex column — BELOW the scrolling sections, so it is pinned to the pane's bottom edge - * regardless of content height or scroll position, and can never shift the sections. - * Shown while the shared signal targets the displayed revision; Dismiss clears it (and - * the teal section dots with it). Enters/exits with an opacity + rise transition. + * Agent self-commit notice, rendered by MainLayout as the LAST row of the config pane's flex column + * — BELOW the scrolling sections, so it is pinned to the pane's bottom edge regardless of content + * height or scroll position, and can never shift the sections. Shown while the shared signal targets + * the displayed revision; Dismiss clears it (and the teal section dots with it). Collapses in/out via + * the shared {@link HeightCollapse} (fade + a small Y slide) — the same CSS-native, reduced-motion- + * proof primitive the composer dock and the draft-change notice use. */ const AgentCommitNotice = ({revisionId}: {revisionId: string}) => { const [signal, setSignal] = useAtom(agentSelfCommitSignalAtom) const active = Boolean(signal && revisionId && signal.revisionId === revisionId) - // Latch the last matching signal so the content stays rendered through the exit fade. + // Latch the last matching signal so the content stays rendered through the collapse-out. const lastSignalRef = useRef(signal) if (signal && active) lastSignalRef.current = signal const shownSignal = lastSignalRef.current - // Enter/exit: `render` keeps the node mounted through the exit transition; `shown` - // drives the opacity/translate classes. Double rAF so the hidden state paints first. - const [render, setRender] = useState(active) - const [shown, setShown] = useState(false) - useEffect(() => { - if (active) { - setRender(true) - // Cancel BOTH frames: killing only the outer id lets an already-scheduled inner - // rAF flash the notice back in after the hide branch ran. - let innerRaf = 0 - const raf = requestAnimationFrame(() => { - innerRaf = requestAnimationFrame(() => setShown(true)) - }) - return () => { - cancelAnimationFrame(raf) - cancelAnimationFrame(innerRaf) - } - } - setShown(false) - const t = window.setTimeout(() => setRender(false), 240) - return () => window.clearTimeout(t) - }, [active]) - // Commit message comes from the committed revision entity (the stream part carries only // id/version) — also covers the notice surviving a reload while the signal is set. const revisionData = useAtomValue( @@ -66,63 +45,61 @@ const AgentCommitNotice = ({revisionId}: {revisionId: string}) => { const el = messageRef.current if (!el || expanded) return setOverflowing(el.scrollHeight - el.clientHeight > 1) - }, [commitMessage, expanded, render]) - - if (!render || !shownSignal) return null + }, [commitMessage, expanded, active]) - const rawVersion = shownSignal.version ? String(shownSignal.version) : null + const rawVersion = shownSignal?.version ? String(shownSignal.version) : null const version = rawVersion ? (rawVersion.startsWith("v") ? rawVersion : `v${rawVersion}`) : null return ( -
-
-
- - - -
- - Agent updated this configuration{version ? ` in ${version}` : ""} — - changed sections are marked - - {commitMessage ? ( -
-

- “{commitMessage}” -

- {overflowing || expanded ? ( - + + {shownSignal ? ( +
+
+
+ + + +
+ + Agent updated this configuration + {version ? ` in ${version}` : ""} — changed sections are marked + + {commitMessage ? ( +
+

+ “{commitMessage}” +

+ {overflowing || expanded ? ( + + ) : null} +
) : null}
- ) : null} +
+
- -
-
+ ) : null} + ) } diff --git a/web/oss/tailwind.config.ts b/web/oss/tailwind.config.ts index 43687d98fe..170a6ebfee 100644 --- a/web/oss/tailwind.config.ts +++ b/web/oss/tailwind.config.ts @@ -139,6 +139,19 @@ export const createConfig = (content: string[] = []): Config => { fontFamily: { sans: ["var(--font-inter)"], }, + // Config-section title shimmer: sweeps a masked highlight overlay across the + // title (see ConfigAccordionSection). Ungated by motion-safe on purpose, to match + // the motion/react dot ripple it pairs with. + keyframes: { + "config-shimmer": { + "0%": {maskPosition: "180% 0", WebkitMaskPosition: "180% 0"}, + "100%": {maskPosition: "-80% 0", WebkitMaskPosition: "-80% 0"}, + }, + }, + // 2 sweeps, then hold off-screen (forwards) so it ends invisibly — no end flash. + animation: { + "config-shimmer": "config-shimmer 1.8s ease-in-out 2 forwards", + }, colors: { ...antdTailwind, // Theme-aware scales (override the static antd-tailwind values diff --git a/web/packages/agenta-ui/package.json b/web/packages/agenta-ui/package.json index 1746b64c73..68bc98078a 100644 --- a/web/packages/agenta-ui/package.json +++ b/web/packages/agenta-ui/package.json @@ -97,6 +97,7 @@ "lexical": "^0.46.0", "lucide-react": "^0.479.0", "marked": "^17.0.4", + "motion": "^12.0.0", "prismjs": "^1.30.0", "react-resizable": "^3.0.5", "uuid": "^11.1.1" diff --git a/web/packages/agenta-ui/src/components/HeightCollapse.tsx b/web/packages/agenta-ui/src/components/HeightCollapse.tsx index 5a91812c6f..e9f74ff2f8 100644 --- a/web/packages/agenta-ui/src/components/HeightCollapse.tsx +++ b/web/packages/agenta-ui/src/components/HeightCollapse.tsx @@ -10,13 +10,34 @@ export interface HeightCollapseProps { durationMs?: number animate?: boolean collapsedHeight?: number + /** + * Also fade opacity 0↔1 with the height. Use for docked chrome/notices where content appearing + * sharply as the box unfolds looks abrupt; omit to keep the plain height-only collapse used by + * the tool gutter, accordion sections, etc. Default false. + */ + fade?: boolean + /** + * Also translate the content on the Y axis: it sits `slideY`px below its resting place while + * closed and eases to 0 on open (and back on close) — a subtle slide for bottom-docked notices. + * Pair with `fade` so the collapsing frames read cleanly. Default 0 (no translate). + */ + slideY?: number + /** + * Apply the `inert` attribute while FULLY closed, dropping the hidden subtree from tab order + + * a11y (e.g. an approval dock's buttons must not be reachable when collapsed). Opt-in so a + * `collapsedHeight > 0` peek keeps its still-visible content interactive. Default false. + */ + inert?: boolean } /** * HeightCollapse * - * CSS-native collapse that transitions `height` between pixel values and `auto` - * using `interpolate-size: allow-keywords`. + * CSS-native collapse that transitions `height` between pixel values and `auto` using + * `interpolate-size: allow-keywords`. Plain CSS transitions (NOT `motion-safe`-gated), so it + * animates regardless of the OS reduced-motion setting. The single collapse primitive for chrome + * that enters/leaves a column — accordion sections, the tool gutter, and (via `fade`/`slideY`) the + * composer dock, queued messages, and the config-pane notices — so everything moves the same way. */ export function HeightCollapse({ open, @@ -26,24 +47,53 @@ export function HeightCollapse({ durationMs = 300, animate = true, collapsedHeight = 0, + fade = false, + slideY = 0, + inert = false, }: HeightCollapseProps) { const collapsedHeightPx = useMemo(() => `${Math.max(0, collapsedHeight)}px`, [collapsedHeight]) - const style = useMemo( + const easing = "cubic-bezier(0.4, 0, 0.2, 1)" + + const outerStyle = useMemo( () => ({ height: open ? "auto" : collapsedHeightPx, + opacity: fade ? (open ? 1 : 0) : undefined, interpolateSize: "allow-keywords", - transitionProperty: animate ? "height" : "none", + transitionProperty: animate ? (fade ? "height, opacity" : "height") : "none", transitionDuration: animate ? `${durationMs}ms` : "0ms", - transitionTimingFunction: animate ? "cubic-bezier(0.4, 0, 0.2, 1)" : "linear", + transitionTimingFunction: animate ? easing : "linear", }) as React.CSSProperties, - [open, collapsedHeightPx, animate, durationMs], + [open, collapsedHeightPx, animate, fade, durationMs], ) + const innerStyle = useMemo( + () => + slideY + ? { + transform: open ? "translateY(0)" : `translateY(${slideY}px)`, + transitionProperty: animate ? "transform" : "none", + transitionDuration: animate ? `${durationMs}ms` : "0ms", + transitionTimingFunction: animate ? easing : "linear", + } + : undefined, + [slideY, open, animate, durationMs], + ) + + // `inert` only while fully collapsed — a peek (collapsedHeight > 0) stays interactive. + const isInert = inert && !open && collapsedHeight === 0 + return ( -
-
{children}
+
+
+ {children} +
) } diff --git a/web/packages/agenta-ui/src/components/presentational/index.ts b/web/packages/agenta-ui/src/components/presentational/index.ts index 61ed6c848f..b2e7767d9b 100644 --- a/web/packages/agenta-ui/src/components/presentational/index.ts +++ b/web/packages/agenta-ui/src/components/presentational/index.ts @@ -63,6 +63,8 @@ export { SectionSkeleton, ConfigAccordionSection, sectionIndicatorColor, + useAccordionSectionOpen, + useRecentFlag, type SectionCardProps, type SectionHeaderRowProps, type SectionLabelProps, diff --git a/web/packages/agenta-ui/src/components/presentational/section/ConfigAccordionSection.tsx b/web/packages/agenta-ui/src/components/presentational/section/ConfigAccordionSection.tsx index e70f9855c7..2b05b29ed8 100644 --- a/web/packages/agenta-ui/src/components/presentational/section/ConfigAccordionSection.tsx +++ b/web/packages/agenta-ui/src/components/presentational/section/ConfigAccordionSection.tsx @@ -26,16 +26,39 @@ * * ``` */ -import {type ReactNode, useCallback, useEffect, useRef, useState} from "react" +import { + createContext, + type ReactNode, + useCallback, + useContext, + useEffect, + useRef, + useState, +} from "react" import {CaretDown, CaretRight, Lock} from "@phosphor-icons/react" import {Tooltip, Typography} from "antd" +import {motion} from "motion/react" import {cn} from "../../../utils/styles" import {HeightCollapse} from "../../HeightCollapse" const {Text} = Typography +/** + * Whether the enclosing accordion section is currently expanded. Because the body stays MOUNTED while + * collapsed (height-0 via {@link HeightCollapse}), a child can't rely on mount to know it just became + * visible — an `autoFocus` fires while hidden and never again. Read this instead to (re)act on open, + * e.g. focus a field when its section unfolds. Defaults to `true` outside a section, so a child used + * elsewhere behaves as always-open. + */ +const SectionOpenContext = createContext(true) + +/** Read whether the enclosing {@link ConfigAccordionSection} is expanded. See {@link SectionOpenContext}. */ +export function useAccordionSectionOpen(): boolean { + return useContext(SectionOpenContext) +} + export type SectionIndicatorTone = "draft" | "invalid" | "incomplete" | "agent" /** @@ -105,9 +128,11 @@ export interface ConfigAccordionSectionProps { * Change/validation indicator for the leading icon: tints the icon, adds a status dot, * and (with `tooltip`) explains it on hover. Takes precedence over `status`. Used by the * agent config panel to flag sections with unsaved edits (`"draft"`), a blocking problem - * (`"invalid"`), or an optional gap (`"incomplete"`). + * (`"invalid"`), or an optional gap (`"incomplete"`). `pulse` plays a one-shot attention + * ring behind the dot (e.g. a change made from another pane); the caller owns how long it + * stays true (see `useRecentFlag`). */ - indicator?: {tone: SectionIndicatorTone; tooltip?: ReactNode} + indicator?: {tone: SectionIndicatorTone; tooltip?: ReactNode; pulse?: boolean} /** Only show `summary` while the section is collapsed. @default false (always). */ summaryCollapsedOnly?: boolean /** @@ -117,6 +142,17 @@ export interface ConfigAccordionSectionProps { titleBadge?: ReactNode /** Additional CSS class for the section wrapper. */ className?: string + /** + * Opt in to the expanded-header "band" — a faint fill + bottom divider while the section is open, + * so the header reads as a header rather than blending into its content. + * + * The value is the BLEED class the host needs to pull the fill out to its own edges while the + * header text stays aligned with the content below, e.g. `"-mx-4 px-4"` inside a `px-4` container. + * It has to come from the host because this primitive is used in containers with different + * padding (config panel, drawers, nested sections) and a hardcoded bleed would overflow or + * under-fill in the others. Omit for no band (the default — every existing usage is unchanged). + */ + headerBand?: string /** * Fade + subtle rise the section in on mount (opacity/transform only — no layout impact). Off by * default so existing usages are unchanged; the agent config panel opts in (staggered via @@ -132,6 +168,13 @@ export interface ConfigAccordionSectionProps { * collapsible, `defaultOpen` sections only — a no-op everywhere else. */ animateInitialOpen?: boolean + /** + * Override the body wrapper classes (the padded flex column around `children`). Defaults to + * `"flex flex-col gap-3 pb-4 pt-3"`. Pass a padding-free value when the body owns its own spacing + * — e.g. content that must collapse to zero height with no residual padding for an exit + * transition. + */ + bodyClassName?: string /** Section body. */ children?: ReactNode } @@ -158,9 +201,11 @@ export function ConfigAccordionSection({ summaryCollapsedOnly = false, titleBadge, className, + headerBand, revealOnMount = false, revealDelayMs = 0, animateInitialOpen = false, + bodyClassName = "flex flex-col gap-3 pb-4 pt-3", children, }: ConfigAccordionSectionProps) { // Height (0→auto via the grid `0fr`→`1fr` trick) + opacity reveal on mount (opt-in). `revealed` @@ -209,6 +254,9 @@ export function ConfigAccordionSection({ !opensDrawer && !locked && (collapsible ? (isControlled ? open : internalOpen) : true) const canToggle = !opensDrawer && collapsible && !locked const headerActs = canToggle || opensDrawer + // Whether the expanded-header band (fill + bottom divider) is currently shown. `isOpen` is already + // false when the section opens a drawer or is locked. + const banded = isOpen && !!headerBand const activate = useCallback(() => { if (opensDrawer) { @@ -225,7 +273,32 @@ export function ConfigAccordionSection({
@@ -246,6 +319,14 @@ export function ConfigAccordionSection({ "flex items-center justify-between gap-2 py-3 select-none", locked && "cursor-not-allowed opacity-60", headerActs && "cursor-pointer", + // Header fill: bled to the section edges and layered over the section's body fill so + // the header reads a touch stronger than the content beneath it. Fades with the band. + headerBand && + cn( + headerBand, + "transition-colors duration-300 ease-out", + banded ? "bg-[var(--ag-colorFillQuaternary)]" : "bg-transparent", + ), )} >
@@ -256,6 +337,23 @@ export function ConfigAccordionSection({ style={{color: iconColor}} > {icon} + {/* Attention ripple: expands + fades out from behind the dot, + repeating while the caller holds `pulse` true (e.g. a change + from another pane). Uses `motion` so it animates reliably. */} + {indicator?.pulse ? ( + + ) : null} {/* Always mounted: state changes play as scale/opacity/color transitions instead of the dot popping in and out. */} ) : null} - - {title} - + {/* Title. The base text stays fully opaque/crisp; while `pulse` holds, a blue + DUPLICATE laid exactly on top is revealed through a moving mask, so a glint + sweeps across the letters (in cadence with the dot ripple) without ever + fading the real text. Sweep = the `config-shimmer` CSS keyframe (motion is + unreliable at animating mask/background position). */} + + + {title} + + {indicator?.pulse ? ( + + {title} + + ) : null} + {titleBadge ? {titleBadge} : null}
@@ -316,7 +443,13 @@ export function ConfigAccordionSection({ {opensDrawer ? null : ( -
{children}
+ {/* Top padding gives the body room below the header band (which carries the + fill + bottom divider), so it reads as content, not a header continuation. */} +
+ + {children} + +
)}
diff --git a/web/packages/agenta-ui/src/components/presentational/section/index.tsx b/web/packages/agenta-ui/src/components/presentational/section/index.tsx index af361561a3..3e946e228f 100644 --- a/web/packages/agenta-ui/src/components/presentational/section/index.tsx +++ b/web/packages/agenta-ui/src/components/presentational/section/index.tsx @@ -174,4 +174,6 @@ export { type ConfigAccordionSectionProps, sectionIndicatorColor, type SectionIndicatorTone, + useAccordionSectionOpen, } from "./ConfigAccordionSection" +export {useRecentFlag} from "./useRecentFlag" diff --git a/web/packages/agenta-ui/src/components/presentational/section/useRecentFlag.ts b/web/packages/agenta-ui/src/components/presentational/section/useRecentFlag.ts new file mode 100644 index 0000000000..f803f33e8c --- /dev/null +++ b/web/packages/agenta-ui/src/components/presentational/section/useRecentFlag.ts @@ -0,0 +1,26 @@ +import {useEffect, useState} from "react" + +/** + * True for `windowMs` after `at`, then false. A caller-owned "just happened" clock for + * transient attention cues (e.g. a pulsing section indicator). Re-arms whenever `at` + * changes, and self-stops with a single timer so idle surfaces don't re-render. Mirrors + * the time-based recency the Drives files use, in a package-safe form. + */ +export function useRecentFlag(at: number | null | undefined, windowMs = 8000): boolean { + const [recent, setRecent] = useState(() => at != null && Date.now() - at < windowMs) + useEffect(() => { + if (at == null) { + setRecent(false) + return + } + const elapsed = Date.now() - at + if (elapsed >= windowMs) { + setRecent(false) + return + } + setRecent(true) + const id = window.setTimeout(() => setRecent(false), windowMs - elapsed) + return () => window.clearTimeout(id) + }, [at, windowMs]) + return recent +} diff --git a/web/pnpm-lock.yaml b/web/pnpm-lock.yaml index 0b7e7b6bec..9aa4096d8f 100644 --- a/web/pnpm-lock.yaml +++ b/web/pnpm-lock.yaml @@ -1388,6 +1388,9 @@ importers: marked: specifier: ^17.0.4 version: 17.0.6 + motion: + specifier: ^12.0.0 + version: 12.38.0(@emotion/is-prop-valid@0.7.3)(react-dom@19.2.6(react@19.2.6))(react@19.2.6) prismjs: specifier: ^1.30.0 version: 1.30.0 From 351be6cd06c7b544b981fe6aae3330dfed343759 Mon Sep 17 00:00:00 2001 From: Arda Erzin Date: Fri, 17 Jul 2026 23:00:19 +0200 Subject: [PATCH 04/11] feat(config): changed-path + focus primitives for config sections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reusable, dependency-free primitives for showing 'what changed' in a config surface: ChangedPathsContext (which dot-paths have uncommitted changes + a host-supplied revert), FocusPathsContext (narrow a surface to just the paths that matter — the same idea as rendering an input only when needed), and RailField opting into both via a path. sectionChanges maps the commit-diff classifier to the config panel's sections. Adds the draft-change and provider-key-added signal atoms the config pane consumes. --- .../agentTemplate/sectionChanges.ts | 262 ++++++++++++++++++ .../drawers/shared/ChangedPathsContext.tsx | 109 ++++++++ .../src/drawers/shared/FocusPathsContext.tsx | 74 +++++ .../src/drawers/shared/RailField.tsx | 105 ++++++- .../src/drawers/shared/index.ts | 22 ++ .../tests/unit/formatCommitted.test.ts | 51 ++++ .../tests/unit/sectionChanges.test.ts | 236 ++++++++++++++++ .../src/state/draftConfigChangeSignal.ts | 31 +++ web/packages/agenta-shared/src/state/index.ts | 4 + .../src/state/providerKeyAddedSignal.ts | 18 ++ 10 files changed, 906 insertions(+), 6 deletions(-) create mode 100644 web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/sectionChanges.ts create mode 100644 web/packages/agenta-entity-ui/src/drawers/shared/ChangedPathsContext.tsx create mode 100644 web/packages/agenta-entity-ui/src/drawers/shared/FocusPathsContext.tsx create mode 100644 web/packages/agenta-entity-ui/tests/unit/formatCommitted.test.ts create mode 100644 web/packages/agenta-entity-ui/tests/unit/sectionChanges.test.ts create mode 100644 web/packages/agenta-shared/src/state/draftConfigChangeSignal.ts create mode 100644 web/packages/agenta-shared/src/state/providerKeyAddedSignal.ts diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/sectionChanges.ts b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/sectionChanges.ts new file mode 100644 index 0000000000..966ac3401d --- /dev/null +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/sectionChanges.ts @@ -0,0 +1,262 @@ +/** + * sectionChanges — one source of truth for "what changed in this agent config, per config-panel + * section". + * + * The commit-diff classifier's sections were deliberately built to mirror this panel's ("Grouped to + * mirror the agent-template control sections", `commitDiff/classify.ts`), so the panel reuses it + * verbatim instead of re-diffing: `model` → Model & harness, `params` → Advanced (the runner / + * sandbox / harness knobs), and the lists map 1:1. This module owns that mapping plus the lookups + * the panel and its drawers need: + * - which sections changed (header indicators), + * - the section's render-ready `ChangeSection` (the inline "what changed" body), + * - the changed scalar dot-paths (`harness.permissions.allow`) so a drawer can mark the exact + * property row and open the sub-section that holds it. + * + * Two independent sources, one shape: + * - `draft` — the live template vs the COMMITTED one. Durable: it stands until committed. + * - `agent` — what the agent's own self-commit changed, frozen at the signal so the user's later + * edits don't drift into "the agent changed this". + * + * Neither is `draftConfigChangeSignalAtom`: that is a TRANSIENT "look here" pulse (~4s), not a + * description of what changed. Anything durable must read from here. + */ +import {useMemo, useRef} from "react" + +import {workflowMolecule} from "@agenta/entities/workflow" +import { + classifyAgentChanges, + type ChangeSection, + type ScalarChange, + type SectionId, +} from "@agenta/entities/workflow/commitDiff" +import {agentSelfCommitSignalAtom} from "@agenta/shared/state" +import {stripAgentaMetadataDeep} from "@agenta/shared/utils" +import {useAtomValue} from "jotai" + +/** The accordion keys the config panel renders. */ +export type PanelSectionKey = + | "model-harness" + | "instructions" + | "tools" + | "mcp" + | "skills" + | "advanced" + +/** + * Classifier section id → config-panel key. Typed against `SectionId`, so adding or renaming a + * classifier section fails the build here instead of silently dropping that section from every + * rollup (the previous two hand-rolled `Record` copies did the latter). + */ +export const SECTION_ID_TO_PANEL_KEY: Record = { + model: "model-harness", + instructions: "instructions", + tools: "tools", + mcps: "mcp", + skills: "skills", + params: "advanced", +} + +export interface SectionChanges { + /** Render-ready change detail, keyed by the PANEL key the caller renders (not `SectionId`). */ + sectionsByKey: Map + /** Panel keys carrying any change — the header-indicator rollup. */ + panelKeys: Set + /** Every changed scalar dot-path, e.g. `harness.permissions.allow`, `runner.permissions.default`. */ + changedPaths: Set + /** Whether this exact dot-path changed (marks one property row). */ + isChanged: (path: string) => boolean + /** Whether anything under this dotted subtree changed (opens the sub-section that owns it). */ + hasChangedUnder: (prefix: string) => boolean + /** Changed paths under a subtree (all of them with no prefix) — the input to a scoped revert. */ + pathsUnder: (prefix?: string) => string[] + /** The recorded before → after for a property, so a row can say what it changed FROM. */ + changeFor: (path: string) => ScalarChange | undefined +} + +/** Fold classifier output into the panel's lookups. Pure — the hook below just supplies the diffs. */ +export function toSectionChanges(sections: ChangeSection[]): SectionChanges { + const sectionsByKey = new Map() + const changedPaths = new Set() + const byPath = new Map() + for (const section of sections) { + const key = SECTION_ID_TO_PANEL_KEY[section.id] + if (!key) continue + sectionsByKey.set(key, section) + for (const scalar of section.scalarChanges ?? []) { + changedPaths.add(scalar.key) + byPath.set(scalar.key, scalar) + } + } + const under = (prefix?: string) => { + if (!prefix) return [...changedPaths] + const scoped = `${prefix}.` + return [...changedPaths].filter((path) => path === prefix || path.startsWith(scoped)) + } + return { + sectionsByKey, + panelKeys: new Set(sectionsByKey.keys()), + changedPaths, + isChanged: (path) => changedPaths.has(path), + hasChangedUnder: (prefix) => under(prefix).length > 0, + pathsUnder: under, + changeFor: (path) => byPath.get(path), + } +} + +// --------------------------------------------------------------------------- +// Revert +// --------------------------------------------------------------------------- + +const isObj = (v: unknown): v is Record => + typeof v === "object" && v !== null && !Array.isArray(v) + +/** Read a dot-path out of an object, or `undefined`. */ +function getPath(source: Record, path: string): unknown { + let cursor: unknown = source + for (const key of path.split(".")) { + if (!isObj(cursor)) return undefined + cursor = cursor[key] + } + return cursor +} + +/** Whether a dot-path exists (so "present but undefined" isn't confused with "absent"). */ +function hasPath(source: Record, path: string): boolean { + const keys = path.split(".") + let cursor: unknown = source + for (const key of keys.slice(0, -1)) { + if (!isObj(cursor)) return false + cursor = cursor[key] + } + return isObj(cursor) && keys[keys.length - 1] in cursor +} + +/** + * Immutably set (or delete) a dot-path. Deleting prunes ancestor objects that are left empty, so + * reverting the only key of a slice the commit never had doesn't leave `permissions: {}` behind — + * which would read as "still changed" to a shape comparison even though nothing is. + */ +function withPath( + source: Record, + path: string, + value: unknown, + remove: boolean, +): Record { + const [key, ...rest] = path.split(".") + const next = {...source} + if (!rest.length) { + if (remove) delete next[key] + else next[key] = value + return next + } + const child = isObj(source[key]) ? (source[key] as Record) : {} + const updated = withPath(child, rest.join("."), value, remove) + if (remove && Object.keys(updated).length === 0) delete next[key] + else next[key] = updated + return next +} + +/** + * Restore `paths` in `draft` to their `committed` values — deleting the key entirely when the commit + * doesn't have it (i.e. the draft ADDED it). Returns a new object; never mutates. Paths are the + * classifier's flattened dot-paths, so an array leaf (`harness.permissions.allow`) is restored whole. + */ +export function revertPathsTo( + draft: Record, + committed: Record | null, + paths: string[], +): Record { + if (!committed) return draft + let next = draft + for (const path of paths) { + next = hasPath(committed, path) + ? withPath(next, path, getPath(committed, path), false) + : withPath(next, path, undefined, true) + } + return next +} + +export const EMPTY_SECTION_CHANGES: SectionChanges = toSectionChanges([]) + +/** Unwrap `parameters` to the agent template, stripping metadata so both sides compare like-for-like. */ +function templateOf(parameters: Record | null): Record | null { + if (!parameters) return null + const agent = parameters.agent + const template = + agent && typeof agent === "object" && !Array.isArray(agent) ? agent : parameters + return stripAgentaMetadataDeep(template) as Record +} + +export interface AgentSectionChanges { + /** Live template vs committed — the unsaved-changes diff. Empty for a never-saved draft. */ + draft: SectionChanges + /** What the agent's last self-commit changed, or null when there is no live signal for this revision. */ + agent: SectionChanges | null + /** Normalized version label for the agent commit, e.g. "v7". */ + agentVersion: string | null + /** + * The committed (server) template this diff is against — metadata-stripped, agent-wrapper + * unwrapped. Exposed so per-item baselines (new/edited row status) read the SAME baseline as the + * section diff instead of re-deriving it. Null for a never-saved draft. + */ + committed: Record | null +} + +/** + * Both change sources for one revision. Replaces the panel's two ad-hoc `classifyAgentChanges` call + * sites (and their duplicated id→key maps) so the diff is computed once and every consumer — header + * indicators, the inline summary, the drawer's property marks — reads the same result. + */ +export function useAgentSectionChanges( + revisionId: string | null | undefined, + config: Record, +): AgentSectionChanges { + // Committed (server) template. Null for a never-saved draft → no draft diff, validation only. + const committedConfig = useAtomValue( + useMemo( + () => workflowMolecule.selectors.serverConfiguration(revisionId ?? ""), + [revisionId], + ), + ) as Record | null + const committed = useMemo(() => templateOf(committedConfig), [committedConfig]) + + const draft = useMemo(() => { + if (!committed) return EMPTY_SECTION_CHANGES + try { + const local = stripAgentaMetadataDeep(config) as Record + return toSectionChanges(classifyAgentChanges(local, committed)) + } catch { + return EMPTY_SECTION_CHANGES + } + }, [config, committed]) + + // The agent commits itself and the playground switches in place; once this control renders the + // NEW revision we diff it against the captured previous parameters. FROZEN on the first + // non-empty result so the user's own later edits don't drift into "the agent changed this". + const commitSignal = useAtomValue(agentSelfCommitSignalAtom) + const frozenRef = useRef<{signalAt: number; changes: SectionChanges} | null>(null) + const agent = useMemo(() => { + if (!commitSignal || !revisionId || commitSignal.revisionId !== revisionId) return null + if (frozenRef.current?.signalAt === commitSignal.at) return frozenRef.current.changes + try { + // local = the NEW config, remote = the PREVIOUS revision's. Order is load-bearing now + // that we render before→after: reversing it would report every add as a removal. + const local = templateOf({agent: config}) + const remote = templateOf(commitSignal.prevParameters as Record) + if (!local || !remote) return null + const changes = toSectionChanges(classifyAgentChanges(local, remote)) + if (!changes.panelKeys.size) return null + frozenRef.current = {signalAt: commitSignal.at, changes} + return changes + } catch { + return null + } + }, [commitSignal, revisionId, config]) + + const agentVersion = useMemo(() => { + const raw = commitSignal?.version ? String(commitSignal.version) : null + return raw ? (raw.startsWith("v") ? raw : `v${raw}`) : null + }, [commitSignal?.version]) + + return {draft, agent, agentVersion, committed} +} diff --git a/web/packages/agenta-entity-ui/src/drawers/shared/ChangedPathsContext.tsx b/web/packages/agenta-entity-ui/src/drawers/shared/ChangedPathsContext.tsx new file mode 100644 index 0000000000..a0a496690f --- /dev/null +++ b/web/packages/agenta-entity-ui/src/drawers/shared/ChangedPathsContext.tsx @@ -0,0 +1,109 @@ +/** + * ChangedPathsContext + * + * Carries "which config properties have uncommitted changes" — as dot-paths like + * `harness.permissions.allow` / `runner.permissions.default` / `sandbox.kind` — down to the rail + * rows that render them. A drawer can then mark the exact property row that changed and open the + * sub-section that holds it, without prop-drilling a path map through a large render. + * + * Deliberately structural and dependency-free: it knows nothing about the agent template or the + * commit-diff classifier, so this shared drawer primitive stays usable by any config surface. The + * agent panel's `SectionChanges` (see `SchemaControls/agentTemplate/sectionChanges.ts`) satisfies + * `ChangedPaths` by shape and can be passed straight in. + * + * With no provider, nothing is marked — every existing caller keeps working untouched. + */ +import {createContext, useContext, useMemo, type ReactNode} from "react" + +export interface ChangedPaths { + /** Whether this exact property path changed. */ + isChanged: (path: string) => boolean + /** Whether anything under this dotted subtree changed. */ + hasChangedUnder: (prefix: string) => boolean + /** Changed paths under a subtree (all of them with no prefix) — the input to a scoped revert. */ + pathsUnder: (prefix?: string) => string[] + /** + * What this property changed FROM — so a row can answer "changed, but from what?" without the + * reader opening a commit diff. Pre-formatted display strings (the classifier already renders + * them); `before: undefined` means the committed config had no value for it. + */ + changeFor?: (path: string) => {before?: string; after?: string} | undefined + /** + * Restore these paths to their committed values. Supplied by the HOST, because where the write + * lands differs by surface: inside a section drawer it must go through that drawer's scoped + * draft (so Cancel/Save still mean what they say), while the panel writes the entity draft + * directly. Absent = the surface offers no revert, and the affordance stays hidden. + */ + revert?: (paths: string[]) => void +} + +const NONE: ChangedPaths = { + isChanged: () => false, + hasChangedUnder: () => false, + pathsUnder: () => [], +} + +const ChangedPathsContext = createContext(NONE) + +export function ChangedPathsProvider({ + changes, + children, +}: { + changes: ChangedPaths + children: ReactNode +}) { + return {children} +} + +/** Whether this exact property path has an uncommitted change (marks one rail row). */ +export function useChangedPath(path: string | undefined): boolean { + const changes = useContext(ChangedPathsContext) + return useMemo(() => (path ? changes.isChanged(path) : false), [changes, path]) +} + +/** + * What this property changed from → to, when the surface can say. Null when it's unchanged, so a + * caller can render the row's "changed" affordance and its explanation from one lookup. + */ +export function useChangedDetail( + path: string | undefined, +): {before?: string; after?: string} | null { + const changes = useContext(ChangedPathsContext) + return useMemo(() => { + if (!path || !changes.isChanged(path)) return null + return changes.changeFor?.(path) ?? null + }, [changes, path]) +} + +/** + * Revert ONE property, for a key-scoped undo on its row. Null when the path is unchanged or the + * surface offers no revert, so the caller renders a plain marker instead of an action. + */ +export function useRevertPath(path: string | undefined): (() => void) | null { + const changes = useContext(ChangedPathsContext) + return useMemo(() => { + const {revert, isChanged} = changes + if (!revert || !path || !isChanged(path)) return null + return () => revert([path]) + }, [changes, path]) +} + +/** Whether anything under this dotted subtree changed — drives a sub-section's `defaultOpen`. */ +export function useHasChangedUnder(prefix: string | undefined): boolean { + const changes = useContext(ChangedPathsContext) + return useMemo(() => (prefix ? changes.hasChangedUnder(prefix) : false), [changes, prefix]) +} + +/** + * Revert a subtree, for a section-scoped "undo these changes" action. Returns null when the surface + * offers no revert or the subtree is unchanged, so the caller renders nothing. + */ +export function useRevertUnder(prefix: string | undefined): (() => void) | null { + const changes = useContext(ChangedPathsContext) + return useMemo(() => { + const {revert, pathsUnder} = changes + if (!revert || !prefix) return null + const paths = pathsUnder(prefix) + return paths.length ? () => revert(paths) : null + }, [changes, prefix]) +} diff --git a/web/packages/agenta-entity-ui/src/drawers/shared/FocusPathsContext.tsx b/web/packages/agenta-entity-ui/src/drawers/shared/FocusPathsContext.tsx new file mode 100644 index 0000000000..65fc591dc6 --- /dev/null +++ b/web/packages/agenta-entity-ui/src/drawers/shared/FocusPathsContext.tsx @@ -0,0 +1,74 @@ +/** + * FocusPathsContext + * + * Narrows a config surface to just the properties that matter right now — the section's REAL + * controls, filtered, rather than a second rendering of them. + * + * This is the general form of the Model & harness "Connect key" affordance: when something needs + * attention, show the control that owns it and link out for the rest. "Needs a key" and "changed + * since the commit" are then the same pattern with different filters, over one set of controls. + * + * Mechanism: rows already declare their `path` (see {@link RailField}), so a filter is all that's + * needed — a focused row renders itself, an unfocused one renders nothing, and the group that owns + * no focused path hides. No parallel "what changed" UI to build or keep in sync. + * + * Inactive by default (no provider = everything renders), so the drawers and every other host are + * untouched. + */ +import {createContext, useContext, useMemo, type ReactNode} from "react" + +export interface FocusPaths { + /** A filter is in force — rows and groups outside it hide. */ + active: boolean + /** Whether this exact property is in focus. */ + isFocused: (path: string) => boolean + /** Whether this dotted subtree contains anything in focus (does this group survive?). */ + hasFocusUnder: (prefix: string) => boolean +} + +/** No filter: everything renders. */ +const NONE: FocusPaths = {active: false, isFocused: () => true, hasFocusUnder: () => true} + +const FocusPathsContext = createContext(NONE) + +export function FocusPathsProvider({ + paths, + children, +}: { + /** The properties to narrow to. `null` = no filter (render everything). */ + paths: string[] | null + children: ReactNode +}) { + const value = useMemo(() => { + if (!paths) return NONE + const set = new Set(paths) + return { + active: true, + isFocused: (path) => set.has(path), + hasFocusUnder: (prefix) => + [...set].some((path) => path === prefix || path.startsWith(`${prefix}.`)), + } + }, [paths]) + return {children} +} + +/** The active filter — for a caller that needs to branch on it (e.g. flat vs grouped chrome). */ +export function useFocusPaths(): FocusPaths { + return useContext(FocusPathsContext) +} + +/** Whether a row should render: true unless a filter is in force that excludes it. A row with no + * `path` can't be matched, so it hides under a filter rather than leaking in as noise. */ +export function useIsPathVisible(path: string | undefined): boolean { + const focus = useContext(FocusPathsContext) + return useMemo(() => !focus.active || (!!path && focus.isFocused(path)), [focus, path]) +} + +/** Whether a group survives the filter — i.e. it owns at least one focused property. */ +export function useHasFocusUnder(prefix: string | undefined): boolean { + const focus = useContext(FocusPathsContext) + return useMemo( + () => !focus.active || (!!prefix && focus.hasFocusUnder(prefix)), + [focus, prefix], + ) +} diff --git a/web/packages/agenta-entity-ui/src/drawers/shared/RailField.tsx b/web/packages/agenta-entity-ui/src/drawers/shared/RailField.tsx index c69a410bc4..afdc43b459 100644 --- a/web/packages/agenta-entity-ui/src/drawers/shared/RailField.tsx +++ b/web/packages/agenta-entity-ui/src/drawers/shared/RailField.tsx @@ -11,13 +11,23 @@ */ import type {ReactNode} from "react" -import {Info} from "@phosphor-icons/react" -import {Tooltip} from "antd" +import {ArrowCounterClockwise, Info} from "@phosphor-icons/react" +import {Button, Popover, Tooltip} from "antd" + +import {useChangedDetail, useChangedPath, useRevertPath} from "./ChangedPathsContext" +import {useIsPathVisible} from "./FocusPathsContext" export interface RailFieldProps { label: ReactNode /** Vertical alignment of the label against the content. @default "top" */ align?: "top" | "center" + /** + * This row's config dot-path (e.g. `runner.permissions.default`). When it has an uncommitted + * change — per the surrounding {@link ChangedPathsProvider} — the label marks itself and opens + * the change's detail, so a surface shows WHICH property changed rather than just that + * something did. Opt-in: without a path (or a provider) the row renders exactly as before. + */ + path?: string children: ReactNode } @@ -34,15 +44,98 @@ export const railInfoLabel = (label: ReactNode, hint: ReactNode): ReactNode => ( ) -export function RailField({label, align = "top", children}: RailFieldProps) { +// Unpack the classifier's JSON.stringify'd scalar back to what the field shows (one rule per line), +// naming the empty cases rather than printing wire syntax like `[]`. +export function formatCommitted(before: string | undefined): {text: string; muted: boolean} { + if (before === undefined) return {text: "Not set", muted: true} + let value: unknown = before + try { + value = JSON.parse(before) + } catch { + // A plain string the classifier passed through as-is. + } + if (Array.isArray(value)) { + return value.length + ? {text: value.map((entry) => String(entry)).join("\n"), muted: false} + : {text: "Empty", muted: true} + } + if (value && typeof value === "object") { + return Object.keys(value).length + ? {text: JSON.stringify(value, null, 2), muted: false} + : {text: "Empty", muted: true} + } + const text = String(value ?? "") + return text.trim() ? {text, muted: false} : {text: "Empty", muted: true} +} + +// The committed value ("changed from what?") and its undo on one surface: the value shown IS the +// revert's confirmation, so no separate confirm step is needed. +function ChangedDetail({ + before, + onRevert, +}: { + before: string | undefined + onRevert: (() => void) | null +}) { + const {text, muted} = formatCommitted(before) + return ( +
+
+ Committed value +
+
+ {text} +
+ {onRevert ? ( + + ) : null} +
+ ) +} + +export function RailField({label, align = "top", path, children}: RailFieldProps) { + const changed = useChangedPath(path) + const detail = useChangedDetail(path) + const revert = useRevertPath(path) + // Focus filter (see FocusPathsContext): each row self-filters on its own `path`. + const visible = useIsPathVisible(path) + if (!visible) return null return (
- {label} + {/* The label carries the change via emphasis (colorTextSecondary → colorText) plus a + colorInfo dotted underline — no marker glyph, so changed and unchanged rows share + the same box. */} + {changed ? ( + } + > + + {label} + + + ) : ( + label + )}
{children} diff --git a/web/packages/agenta-entity-ui/src/drawers/shared/index.ts b/web/packages/agenta-entity-ui/src/drawers/shared/index.ts index 85b73ab750..08a863e456 100644 --- a/web/packages/agenta-entity-ui/src/drawers/shared/index.ts +++ b/web/packages/agenta-entity-ui/src/drawers/shared/index.ts @@ -6,6 +6,28 @@ export {RailField, railInfoLabel, type RailFieldProps} from "./RailField" export {SectionRail, type SectionRailItem, type SectionRailProps} from "./SectionRail" +// "Which properties have uncommitted changes" — lets a rail row mark the exact changed property and +// a sub-section open itself when it owns one. Structural, so any config surface can provide it. +export { + ChangedPathsProvider, + useChangedDetail, + useChangedPath, + useHasChangedUnder, + useRevertPath, + useRevertUnder, + type ChangedPaths, +} from "./ChangedPathsContext" + +// Narrow a surface to the properties that matter right now — the real controls, filtered, rather +// than a second rendering of them. Rows self-filter on the `path` they already declare. +export { + FocusPathsProvider, + useFocusPaths, + useIsPathVisible, + useHasFocusUnder, + type FocusPaths, +} from "./FocusPathsContext" + // Grouped-section primitives (uppercase sub-headers + collapsible provider cards) shared with the // config panel's Tools/Triggers sections, so app-layer previews render identical provider groups. export { diff --git a/web/packages/agenta-entity-ui/tests/unit/formatCommitted.test.ts b/web/packages/agenta-entity-ui/tests/unit/formatCommitted.test.ts new file mode 100644 index 0000000000..04ff2d1361 --- /dev/null +++ b/web/packages/agenta-entity-ui/tests/unit/formatCommitted.test.ts @@ -0,0 +1,51 @@ +/** + * Unit tests for `formatCommitted`, the pure helper behind a changed rail row's "committed value" + * popover. + * + * It exists because the two sides disagree on what a value looks like: the commit-diff classifier + * stores scalars through `JSON.stringify` (`commitDiff/classify.ts` → `fmtScalar`) because it is + * COMPARING them, while the popover is SHOWING them to a person. Left unformatted that leaked wire + * syntax into the UI — an untouched allow-list rendered as a bare `[]`. These pin the unwrapping, + * and that the empty/absent cases get named rather than printing punctuation at the reader. + * + * Runs under @agenta/entity-ui's vitest runner. + */ +import {describe, expect, it} from "vitest" + +import {formatCommitted} from "../../src/drawers/shared/RailField" + +describe("formatCommitted — the classifier's wire value, as a reader sees it", () => { + it("names the absent and empty cases instead of showing their punctuation", () => { + // `[]` is what the popover actually leaked before this existed. + expect(formatCommitted(undefined)).toEqual({text: "Not set", muted: true}) + expect(formatCommitted("[]")).toEqual({text: "Empty", muted: true}) + expect(formatCommitted("{}")).toEqual({text: "Empty", muted: true}) + expect(formatCommitted('""')).toEqual({text: "Empty", muted: true}) + expect(formatCommitted(" ")).toEqual({text: "Empty", muted: true}) + }) + + it("unwraps a stringified list to one entry per line, matching the control that renders it", () => { + // The allow-rules field is a textarea of one rule per line — not `["Terminal","Write"]`. + expect(formatCommitted('["Terminal","Write"]')).toEqual({ + text: "Terminal\nWrite", + muted: false, + }) + }) + + it("keeps a plain scalar as itself", () => { + expect(formatCommitted("ask")).toEqual({text: "ask", muted: false}) + expect(formatCommitted("42")).toEqual({text: "42", muted: false}) + }) + + it("pretty-prints a non-empty object rather than showing one long line", () => { + expect(formatCommitted('{"mode":"deny"}')).toEqual({ + text: '{\n "mode": "deny"\n}', + muted: false, + }) + }) + + it("passes through a string that only looks like JSON, without throwing", () => { + // fmtScalar sends non-objects through `String(v)`, so an instruction can reach here unquoted. + expect(formatCommitted("{not json")).toEqual({text: "{not json", muted: false}) + }) +}) diff --git a/web/packages/agenta-entity-ui/tests/unit/sectionChanges.test.ts b/web/packages/agenta-entity-ui/tests/unit/sectionChanges.test.ts new file mode 100644 index 0000000000..a92e123749 --- /dev/null +++ b/web/packages/agenta-entity-ui/tests/unit/sectionChanges.test.ts @@ -0,0 +1,236 @@ +/** + * Pins the CONTRACT between the commit-diff classifier and the config panel/drawer. + * + * The drawer marks a changed property by hardcoding its dot-path on a `RailField` + * (`path="harness.permissions.allow"`), and sub-sections open via `hasChangedUnder("harness")`. + * Those strings only work if they match what `classifyAgentChanges` actually flattens to — and a + * mismatch fails SILENTLY (no mark, no error), so it must be asserted rather than assumed. + * + * These tests therefore diff realistic before/after templates the same way the panel does (bare + * agent template on both sides) and assert the emitted `scalarChanges` keys verbatim. + */ +import {classifyAgentChanges} from "@agenta/entities/workflow/commitDiff" +import {describe, expect, it} from "vitest" + +import { + revertPathsTo, + SECTION_ID_TO_PANEL_KEY, + toSectionChanges, +} from "../../src/DrillInView/SchemaControls/agentTemplate/sectionChanges" + +/** The bare agent template both sides of the panel's diff use (`parameters.agent`, unwrapped). */ +const template = (overrides: Record = {}) => ({ + instructions: {agents_md: "You are a friendly hello-world agent."}, + llm: {model: "gpt-5", provider: "openai"}, + tools: [], + harness: {kind: "claude"}, + runner: {permissions: {default: "allow_reads"}}, + sandbox: {kind: "local"}, + ...overrides, +}) + +const changesFor = (local: Record, remote: Record) => + toSectionChanges(classifyAgentChanges(local, remote)) + +describe("SECTION_ID_TO_PANEL_KEY", () => { + it("maps every classifier section id to a panel key", () => { + expect(SECTION_ID_TO_PANEL_KEY).toEqual({ + model: "model-harness", + instructions: "instructions", + tools: "tools", + mcps: "mcp", + skills: "skills", + params: "advanced", + }) + }) +}) + +describe("changed paths — the strings the drawer hardcodes", () => { + it("an approval grant to harness.permissions.allow lands on the Advanced section at that exact path", () => { + const committed = template() + const draft = template({harness: {kind: "claude", permissions: {allow: ["Terminal"]}}}) + const changes = changesFor(draft, committed) + + expect(changes.panelKeys.has("advanced")).toBe(true) + expect(changes.changedPaths.has("harness.permissions.allow")).toBe(true) + expect(changes.isChanged("harness.permissions.allow")).toBe(true) + }) + + // The row's popover answers "changed from what?" with `before` — so a change the classifier + // reports MUST also be retrievable by its path, or the row marks itself and then has nothing to + // say. `before: undefined` is meaningful (the commit didn't set it) and must survive as itself. + it("recalls what a changed property was committed as, by path", () => { + const changes = changesFor( + template({runner: {permissions: {default: "allow"}}}), + template({runner: {permissions: {default: "ask"}}}), + ) + + expect(changes.changeFor("runner.permissions.default")?.before).toBe("ask") + expect(changes.changeFor("runner.permissions.default")?.after).toBe("allow") + expect(changes.changeFor("sandbox.kind")).toBeUndefined() + }) + + it("reports a property the commit never had as added, with no before value", () => { + const changes = changesFor( + template({harness: {kind: "claude", permissions: {allow: ["Terminal"]}}}), + template(), + ) + const change = changes.changeFor("harness.permissions.allow") + + expect(change).toBeDefined() + expect(change?.before).toBeUndefined() + }) + + it("opens the Permissions group (harness/runner) but not Execution environment (sandbox)", () => { + const committed = template() + const draft = template({harness: {kind: "claude", permissions: {allow: ["Terminal"]}}}) + const changes = changesFor(draft, committed) + + expect(changes.hasChangedUnder("harness")).toBe(true) + expect(changes.hasChangedUnder("runner")).toBe(false) + expect(changes.hasChangedUnder("sandbox")).toBe(false) + }) + + it("emits runner.permissions.default for a Policy change", () => { + const committed = template() + const draft = template({runner: {permissions: {default: "allow"}}}) + const changes = changesFor(draft, committed) + + expect(changes.changedPaths.has("runner.permissions.default")).toBe(true) + expect(changes.hasChangedUnder("runner")).toBe(true) + }) + + it("emits sandbox.kind and the nested sandbox.permissions.* paths", () => { + const committed = template() + const draft = template({ + sandbox: { + kind: "daytona", + permissions: {network: {mode: "off"}, filesystem: "readonly"}, + }, + }) + const changes = changesFor(draft, committed) + + expect(changes.changedPaths.has("sandbox.kind")).toBe(true) + expect(changes.changedPaths.has("sandbox.permissions.network.mode")).toBe(true) + expect(changes.changedPaths.has("sandbox.permissions.filesystem")).toBe(true) + expect(changes.hasChangedUnder("sandbox")).toBe(true) + }) + + it("keeps harness.kind on Model & harness, NOT Advanced (the buckets are split there)", () => { + const committed = template() + const draft = template({harness: {kind: "pi_core"}}) + const changes = changesFor(draft, committed) + + expect(changes.panelKeys.has("model-harness")).toBe(true) + expect(changes.panelKeys.has("advanced")).toBe(false) + expect(changes.changedPaths.has("harness.kind")).toBe(true) + }) + + it("reports nothing changed for an identical template", () => { + const changes = changesFor(template(), template()) + expect(changes.panelKeys.size).toBe(0) + expect(changes.changedPaths.size).toBe(0) + expect(changes.hasChangedUnder("harness")).toBe(false) + }) +}) + +describe("revertPathsTo", () => { + it("restores a changed value to the committed one, and the diff then reports clean", () => { + const committed = template() + const draft = template({runner: {permissions: {default: "allow"}}}) + + const reverted = revertPathsTo(draft, committed, ["runner.permissions.default"]) + + expect(reverted).toEqual(committed) + expect(changesFor(reverted, committed).changedPaths.size).toBe(0) + }) + + it("DELETES a key the commit never had (an added property), pruning the empty slice it leaves", () => { + const committed = template() // harness: {kind: "claude"} — no `permissions` + const draft = template({harness: {kind: "claude", permissions: {allow: ["Terminal"]}}}) + + const reverted = revertPathsTo(draft, committed, ["harness.permissions.allow"]) + + // Not `permissions: {}` left behind — the slice is pruned, so the diff is truly clean. + expect(reverted.harness).toEqual({kind: "claude"}) + expect(changesFor(reverted, committed).changedPaths.size).toBe(0) + }) + + it("reverts only the named path, leaving other changes intact (key-scoped undo)", () => { + const committed = template() + const draft = template({ + harness: {kind: "claude", permissions: {allow: ["Terminal"]}}, + runner: {permissions: {default: "allow"}}, + }) + + const reverted = revertPathsTo(draft, committed, ["harness.permissions.allow"]) + const changes = changesFor(reverted, committed) + + expect(changes.changedPaths.has("harness.permissions.allow")).toBe(false) + expect(changes.changedPaths.has("runner.permissions.default")).toBe(true) + }) + + it("reverts a whole subtree from pathsUnder (section-scoped undo)", () => { + const committed = template() + const draft = template({ + sandbox: {kind: "daytona", permissions: {filesystem: "readonly"}}, + }) + const paths = changesFor(draft, committed).pathsUnder("sandbox") + + const reverted = revertPathsTo(draft, committed, paths) + + expect(reverted).toEqual(committed) + }) + + it("restores an array leaf whole, and never mutates the input", () => { + const committed = template({harness: {kind: "claude", permissions: {allow: ["Read"]}}}) + const draft = template({ + harness: {kind: "claude", permissions: {allow: ["Read", "Terminal"]}}, + }) + const snapshot = JSON.stringify(draft) + + const reverted = revertPathsTo(draft, committed, ["harness.permissions.allow"]) as { + harness: {permissions: {allow: string[]}} + } + + expect(reverted.harness.permissions.allow).toEqual(["Read"]) + expect(JSON.stringify(draft)).toBe(snapshot) + }) + + it("is a no-op without a committed baseline (a never-saved draft has nothing to revert to)", () => { + const draft = template() + expect(revertPathsTo(draft, null, ["runner.permissions.default"])).toBe(draft) + }) +}) + +describe("hasChangedUnder", () => { + it("matches the subtree, not a same-prefixed sibling key", () => { + const changes = toSectionChanges([ + { + id: "params", + title: "Advanced", + tags: [], + totalCount: 1, + scalarChanges: [ + {key: "harnessing.thing", before: "a", after: "b", kind: "changed"}, + ], + }, + ]) + // "harnessing.thing" must NOT make the `harness` group look changed. + expect(changes.hasChangedUnder("harness")).toBe(false) + expect(changes.hasChangedUnder("harnessing")).toBe(true) + }) + + it("treats an exact path as changed under itself", () => { + const changes = toSectionChanges([ + { + id: "params", + title: "Advanced", + tags: [], + totalCount: 1, + scalarChanges: [{key: "harness", before: "a", after: "b", kind: "changed"}], + }, + ]) + expect(changes.hasChangedUnder("harness")).toBe(true) + }) +}) diff --git a/web/packages/agenta-shared/src/state/draftConfigChangeSignal.ts b/web/packages/agenta-shared/src/state/draftConfigChangeSignal.ts new file mode 100644 index 0000000000..d5e415dcf4 --- /dev/null +++ b/web/packages/agenta-shared/src/state/draftConfigChangeSignal.ts @@ -0,0 +1,31 @@ +import {atom} from "jotai" + +/** + * Raised when an interaction OUTSIDE the config pane mutates the DRAFT agent config + * (e.g. flipping "always allow" in the approval dock writes a per-tool permission). + * The draft/uncommitted counterpart of {@link agentSelfCommitSignalAtom}: that one marks + * a COMMITTED self-commit in agent teal; this one marks an UNCOMMITTED, user-initiated + * change in draft blue, so the config sections it touched can pulse for attention even + * when the user's eyes are on the chat. + * + * Config-scoped by design — files (time-based recency) and triggers (persisted, not draft) + * keep their own "changed" engines; only the shared visual language is reused. + * Cleared by the next draft change or by dismissal. + */ +export interface DraftConfigChangeSignal { + /** The draft revision whose config changed. */ + revisionId: string + /** Config section keys to light up, e.g. ["tools"] or ["model-harness"]. */ + sectionKeys: string[] + /** Where the change came from — extensible provenance for future callers. */ + origin: "approval-dock" + /** Short human summary for the tooltip, e.g. "Always allow search_web". */ + summary?: string + /** Friendly label for the config-pane banner, e.g. "Send email". */ + label?: string + /** The tool the change targeted, so the banner's Undo can revert it. */ + toolName?: string + at: number +} + +export const draftConfigChangeSignalAtom = atom(null) diff --git a/web/packages/agenta-shared/src/state/index.ts b/web/packages/agenta-shared/src/state/index.ts index 793d32c326..ffd51d49bb 100644 --- a/web/packages/agenta-shared/src/state/index.ts +++ b/web/packages/agenta-shared/src/state/index.ts @@ -11,6 +11,10 @@ export {openAgentConfigSectionAtom} from "./openConfigSection" export type {AgentConfigSection} from "./openConfigSection" export {agentSelfCommitSignalAtom} from "./agentCommitSignal" export type {AgentSelfCommitSignal} from "./agentCommitSignal" +export {draftConfigChangeSignalAtom} from "./draftConfigChangeSignal" +export type {DraftConfigChangeSignal} from "./draftConfigChangeSignal" +export {providerKeyAddedSignalAtom} from "./providerKeyAddedSignal" +export type {ProviderKeyAddedSignal} from "./providerKeyAddedSignal" export {atomWithRefresh} from "jotai/utils" export { atomWithCompare, diff --git a/web/packages/agenta-shared/src/state/providerKeyAddedSignal.ts b/web/packages/agenta-shared/src/state/providerKeyAddedSignal.ts new file mode 100644 index 0000000000..48fc0b0139 --- /dev/null +++ b/web/packages/agenta-shared/src/state/providerKeyAddedSignal.ts @@ -0,0 +1,18 @@ +import {atom} from "jotai" + +/** + * Raised when a provider API key is saved from the config pane's "Connect key" flow (the inline + * provider-credentials pane or its drawer). Drives the success banner pinned to the bottom of the + * config pane — the same pattern as {@link agentSelfCommitSignalAtom} / + * {@link draftConfigChangeSignalAtom}, in success green: it confirms the agent can now run without + * pulling focus away with a floating toast. Cleared by dismissal or after an auto-dismiss timeout. + */ +export interface ProviderKeyAddedSignal { + /** The displayed revision the key was connected for. */ + revisionId: string + /** Friendly provider name for the banner, e.g. "OpenAI". */ + provider?: string + at: number +} + +export const providerKeyAddedSignalAtom = atom(null) From 2fe28f9b746562b5b9bfe5eea5df97f24165e576 Mon Sep 17 00:00:00 2001 From: Arda Erzin Date: Fri, 17 Jul 2026 23:09:56 +0200 Subject: [PATCH 05/11] =?UTF-8?q?feat(config):=20context-driven=20config?= =?UTF-8?q?=20sections=20=E2=80=94=20what-changed=20focus=20+=20inline=20p?= =?UTF-8?q?rovider=20key?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drawer-backed config sections (Model & harness, Advanced) surface only what needs attention inline instead of routing to the drawer: a missing provider key shows the key field right there, and uncommitted changes show the changed controls (focus-filtered via FocusPaths/ChangedPaths), each row marked and revertable, with a link out to the full drawer. Wires the sandbox/harness permission controls and the provider-credentials/key fields with their dot-paths; adds the provider-key-added notice. --- .../Components/ProviderKeyNotice.tsx | 73 +++ .../AgentOperationsSections.tsx | 4 +- .../SchemaControls/AgentTemplateControl.tsx | 379 ++++++++---- .../ClaudePermissionsControl.tsx | 13 +- .../SchemaControls/PiAutoApproveControl.tsx | 80 +++ .../SandboxPermissionControl.tsx | 25 +- .../SchemaControls/SectionQuickAction.tsx | 45 ++ .../ProviderCredentialsSection.tsx | 532 +++++++++++------ .../agentTemplate/ProviderKeyField.tsx | 81 ++- .../agentTemplate/SectionChangeBody.tsx | 58 ++ .../agentTemplate/useModelHarness.tsx | 552 ++++++++++++------ .../components/PlaygroundConfigSection.tsx | 22 +- 12 files changed, 1369 insertions(+), 495 deletions(-) create mode 100644 web/oss/src/components/Playground/Components/ProviderKeyNotice.tsx create mode 100644 web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiAutoApproveControl.tsx create mode 100644 web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/SectionQuickAction.tsx create mode 100644 web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/SectionChangeBody.tsx diff --git a/web/oss/src/components/Playground/Components/ProviderKeyNotice.tsx b/web/oss/src/components/Playground/Components/ProviderKeyNotice.tsx new file mode 100644 index 0000000000..0217760f14 --- /dev/null +++ b/web/oss/src/components/Playground/Components/ProviderKeyNotice.tsx @@ -0,0 +1,73 @@ +import {useEffect, useRef} from "react" + +import {providerKeyAddedSignalAtom} from "@agenta/shared/state" +import {HeightCollapse} from "@agenta/ui" +import {CheckCircle, X} from "@phosphor-icons/react" +import {Button} from "antd" +import {useAtom} from "jotai" + +/** + * "API key added" notice — the success-green sibling of {@link AgentCommitNotice} / + * {@link AlwaysAllowedNotice}, pinned to the bottom of the config pane. Confirms that a provider key + * the user just connected from the "Connect key" flow has landed and the agent can now run, CONTAINED + * to the config panel (where the change happened) instead of a floating toast that pulls focus away. + * Collapses in/out via the shared {@link HeightCollapse} (fade + a small Y slide) — the same + * CSS-native, reduced-motion-proof primitive the other notices use. Auto-dismisses after a few + * seconds; Dismiss clears it early. + */ +const ProviderKeyNotice = ({revisionId}: {revisionId: string}) => { + const [signal, setSignal] = useAtom(providerKeyAddedSignalAtom) + const active = Boolean(signal && revisionId && signal.revisionId === revisionId) + + // Latch the last matching signal so content stays rendered through the collapse-out. + const lastRef = useRef(signal) + if (signal && active) lastRef.current = signal + const shown = lastRef.current + + // Auto-dismiss 6s after the key lands (keyed on `at`, so a fresh save restarts the clock). + useEffect(() => { + if (!active) return + const t = window.setTimeout(() => setSignal(null), 6000) + return () => window.clearTimeout(t) + }, [active, signal?.at, setSignal]) + + const provider = shown?.provider + + return ( + +
+
+
+ + + +
+ + {provider ? ( + <> + {provider} API key + added + + ) : ( + "API key added" + )} + + + The agent is ready to run. + +
+
+
+
+
+ ) +} + +export default ProviderKeyNotice diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentOperationsSections.tsx b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentOperationsSections.tsx index 619e3c1b64..1cfee9ed63 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentOperationsSections.tsx +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentOperationsSections.tsx @@ -90,7 +90,9 @@ export function AgentOperationsSections({ {countSummary(triggerCount, "trigger")}
-
+ {/* pb-3 only: the first ConfigAccordionSection row carries its own py-3 header, so a + top py-3 here would double into a dead band under the header bar (mirrors Config). */} +
diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsx b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsx index 96f2d502f0..de7b309f76 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsx +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsx @@ -25,20 +25,14 @@ import {useCallback, useEffect, useMemo, useRef, useState} from "react" import {toolActionAvailabilityKey, useToolActionAvailability} from "@agenta/entities/gatewayTool" import type {SchemaProperty} from "@agenta/entities/shared" -import { - agentCreationPrefsAtom, - workflowBuildKitEnabledAtomFamily, - workflowMolecule, -} from "@agenta/entities/workflow" -import { - agentItemIdentity, - classifyAgentChanges, - stableStringify, -} from "@agenta/entities/workflow/commitDiff" -import {agentSelfCommitSignalAtom, openAgentConfigSectionAtom} from "@agenta/shared/state" +import {agentCreationPrefsAtom, workflowBuildKitEnabledAtomFamily} from "@agenta/entities/workflow" +import {agentItemIdentity, stableStringify} from "@agenta/entities/workflow/commitDiff" +import {draftConfigChangeSignalAtom, openAgentConfigSectionAtom} from "@agenta/shared/state" import {stripAgentaMetadataDeep} from "@agenta/shared/utils" +import {HeightCollapse} from "@agenta/ui/components" import { ConfigAccordionSection, + useRecentFlag, type SectionIndicatorTone, } from "@agenta/ui/components/presentational" import {useDrillInUI} from "@agenta/ui/drill-in" @@ -56,6 +50,7 @@ import {Button, Tooltip, Typography} from "antd" import deepEqual from "fast-deep-equal" import {useAtom, useAtomValue, useStore} from "jotai" +import {ChangedPathsProvider} from "../../drawers/shared" import {useOptionalDrillIn} from "../components/MoleculeDrillInContext" import {AddTextLink} from "./AddTextLink" @@ -66,6 +61,12 @@ import {AgentToolSelectorPopover} from "./agentTemplate/AgentToolSelectorPopover import {ConfigItemList} from "./agentTemplate/ConfigItemList" import {ITEM_KINDS, type ItemKind} from "./agentTemplate/itemKinds" import {InstructionsFileRow, type ItemRowStatus} from "./agentTemplate/ItemRow" +import {SectionChangeBody} from "./agentTemplate/SectionChangeBody" +import { + revertPathsTo, + useAgentSectionChanges, + type PanelSectionKey, +} from "./agentTemplate/sectionChanges" import {ToolManagementList} from "./agentTemplate/ToolManagementList" import {useAgentTools} from "./agentTemplate/useAgentTools" import {useConfigItemDrawer} from "./agentTemplate/useConfigItemDrawer" @@ -75,6 +76,7 @@ import {connectionFromConfig, modelIdFromConfig} from "./connectionUtils" import {InstructionsDrawer} from "./InstructionsDrawer" import {JsonObjectEditor} from "./JsonObjectEditor" import {SectionDrawer} from "./SectionDrawer" +import {SectionQuickAction} from "./SectionQuickAction" import {parseGatewayTool, type ParsedGatewayTool, type ToolObj} from "./toolUtils" import {useAgentTriggers} from "./TriggerManagementSection" import {WorkflowReferenceSelector} from "./WorkflowReferenceSelector" @@ -105,17 +107,27 @@ export interface AgentTemplateControlProps { className?: string } -// Draft body for the Model & harness / Advanced section drawers. Isolated into its own component so -// `useModelHarness` (harness-catalog + vault-secrets + build-kit-overlay subscriptions) runs ONLY -// while a section drawer is open — `SectionDrawer` uses `destroyOnClose`, so this mounts on open and -// unmounts on close. Previously a second `useModelHarness` ran in the always-mounted parent, -// subscribing on every agent-config render even with both drawers shut. -const ModelHarnessSectionDrawerBody = ({ +// A section's body, in its own component so `useModelHarness` runs HERE rather than in the parent. +// Two reasons, both load-bearing: +// 1. Context. The hook itself reads the changed-paths / focus filters (to mark rows, open the group +// that changed, and narrow to it). React resolves context at the READER's position, so the hook +// must run BELOW the providers — wrapping its output in them does nothing. A body rendered from +// the parent's `mh` silently ignores both filters. +// 2. Cost. `useModelHarness` carries harness-catalog + vault-secrets + build-kit-overlay +// subscriptions; mounting it per body keeps them scoped to a body that is actually on screen +// (`SectionDrawer` uses `destroyOnClose`; the inline body mounts only while a section has +// uncommitted changes). +const ModelHarnessSectionBody = ({ section, ...params -}: {section: "model-harness" | "advanced"} & Parameters[0]) => { +}: { + section: "model-harness" | "advanced" +} & Parameters[0]) => { const mh = useModelHarness(params) - return <>{section === "advanced" ? mh.advancedDrawerBody : mh.modelHarnessDrawerBody} + if (section === "advanced") { + return <>{mh.advancedDrawerBody} + } + return <>{mh.modelHarnessDrawerBody} } // The four list sections whose open-state is controlled so the accordion can auto-expand when @@ -293,45 +305,51 @@ export function AgentTemplateControl({ // NEW revision, diff the configs per section and mark the changed ones. The computed // set is FROZEN on first non-empty result so the user's own subsequent edits don't // drift into the "agent changed this" indication. Dismiss (or the next commit) clears. - const commitSignal = useAtomValue(agentSelfCommitSignalAtom) - const frozenAgentDiffRef = useRef<{signalAt: number; keys: Set} | null>(null) - const agentChangedKeys = useMemo(() => { - if (!commitSignal || !revisionId || commitSignal.revisionId !== revisionId) return null - if (frozenAgentDiffRef.current?.signalAt === commitSignal.at) { - return frozenAgentDiffRef.current.keys - } - try { - const sectionIdToKey: Record = { - model: "model-harness", - instructions: "instructions", - tools: "tools", - mcps: "mcp", - skills: "skills", - params: "advanced", - } - const changed = classifyAgentChanges(commitSignal.prevParameters, {agent: value}) - const keys = new Set( - changed.map((s) => sectionIdToKey[s.id]).filter((k): k is string => Boolean(k)), - ) - if (keys.size === 0) return null - frozenAgentDiffRef.current = {signalAt: commitSignal.at, keys} - return keys - } catch { - return null - } - }, [commitSignal, revisionId, value]) + // Both change sources for this revision, computed ONCE (see `sectionChanges`): `draft` is the + // durable live-vs-committed diff, `agent` is the frozen self-commit diff. Every consumer — + // header indicators, the drawers' property marks — reads this same result. + const sectionChanges = useAgentSectionChanges(revisionId, config) + const agentChangedKeys = sectionChanges.agent?.panelKeys ?? null const agentChangeIndicator = useCallback( (sectionKey: string) => { - if (!agentChangedKeys?.has(sectionKey)) return undefined - const raw = commitSignal?.version ? String(commitSignal.version) : null - const version = raw ? (raw.startsWith("v") ? raw : `v${raw}`) : null + if (!agentChangedKeys?.has(sectionKey as PanelSectionKey)) return undefined + const version = sectionChanges.agentVersion return { tone: "agent" as const, tooltip: `Updated by the agent${version ? ` in ${version}` : ""}`, } }, - [agentChangedKeys, commitSignal?.version], + [agentChangedKeys, sectionChanges.agentVersion], + ) + + // ── Draft change from another pane (e.g. "always allow" in the approval dock) ──────── + // A user-initiated, UNCOMMITTED write to this revision's draft config already shows the + // section's static "draft" dot (headerIndicator). When the write came from another pane, + // make THAT dot pulse briefly for attention — the user acted in the dock, not here — instead + // of adding a competing indicator. Rides on whatever tone the section already carries. + const draftSignal = useAtomValue(draftConfigChangeSignalAtom) + const draftActive = Boolean(draftSignal && revisionId && draftSignal.revisionId === revisionId) + // Covers the two 1.8s pulse sweeps (~3.6s) plus a small buffer, so the ripple/shimmer finish + // on their invisible end frame before unmounting — no end-of-pulse flash. + const draftRecent = useRecentFlag(draftActive ? draftSignal!.at : null, 3900) + const withDraftPulse = useCallback( + ( + sectionKey: string, + base: {tone: SectionIndicatorTone; tooltip?: React.ReactNode} | undefined, + ): {tone: SectionIndicatorTone; tooltip?: React.ReactNode; pulse?: boolean} | undefined => { + if (!draftActive || !draftRecent || !draftSignal!.sectionKeys.includes(sectionKey)) { + return base + } + if (base) return {...base, pulse: true} + return { + tone: "draft", + tooltip: `${draftSignal!.summary ?? "Changed here"} — pending, applies on the next run`, + pulse: true, + } + }, + [draftActive, draftRecent, draftSignal], ) + // Model & harness + Advanced own a lot of coupled, stateful logic (the model/connection state // feeds both sections), so they live in their own hook that returns the summaries + bodies. // @@ -340,7 +358,7 @@ export function AgentTemplateControl({ // means a section header NEVER reflects the drawer's unsaved draft // (the reported bug: editing in the open drawer updated the background summary). // - The DRAFT instance (config + build-kit buffer) that drives the OPEN section drawer's body - // now lives inside `ModelHarnessSectionDrawerBody`, mounted only while the drawer is open, so + // now lives inside `ModelHarnessSectionBody`, mounted only while a drawer/inline body is on screen, so // its harness/vault/overlay subscriptions don't run in the background. const mh = useModelHarness({schema, config, onChange, disabled, withTooltip, revisionId}) const draftBuildKitOverride = useMemo( @@ -441,41 +459,81 @@ export function AgentTemplateControl({ // ── Draft + validation indicators ───────────────────────────────────────── // Committed (server) template to diff the live config against. Null for a never-saved // draft — then only validation (not draft) indicators show. - const committedConfig = useAtomValue( - useMemo( - () => workflowMolecule.selectors.serverConfiguration(revisionId ?? ""), - [revisionId], - ), - ) as Record | null - const committed = useMemo(() => { - if (!committedConfig) return null - const agent = committedConfig.agent - const template = - agent && typeof agent === "object" && !Array.isArray(agent) ? agent : committedConfig - // Strip agenta metadata so it compares like-for-like against the live value. - return stripAgentaMetadataDeep(template) as Record - }, [committedConfig]) + // The same committed baseline the section diff uses (see `useAgentSectionChanges`) — read once + // there rather than subscribing to `serverConfiguration` a second time and re-unwrapping it. + const committed = sectionChanges.committed - // Header rollup: which sections changed vs the commit. Reuses the commit-diff classifier so - // the grouping matches (model+harness together; advanced = runner/sandbox/params). - const draftSectionKeys = useMemo(() => { - const keys = new Set() - if (!committed) return keys - const map: Record = { - model: "model-harness", - instructions: "instructions", - tools: "tools", - mcps: "mcp", - skills: "skills", - params: "advanced", - } - const local = stripAgentaMetadataDeep(config) as Record - for (const s of classifyAgentChanges(local, committed)) { - const k = map[s.id] - if (k) keys.add(k) - } - return keys - }, [config, committed]) + // Header rollup: which sections changed vs the commit — from the shared diff above, so the + // grouping matches the classifier's (model+harness together; advanced = runner/sandbox/params) + // and the indicators, the drawers' property marks, and any summary can never disagree. + const draftSectionKeys = sectionChanges.draft.panelKeys + + // Revert for the SECTION DRAWERS: restore the given paths to their committed values through the + // drawer's scoped draft (`applyDraftConfig`), never straight to the entity — so an undo behaves + // like any other edit in there and Cancel/Save still mean what they say. + const drawerChangedPaths = useMemo( + () => ({ + ...sectionChanges.draft, + revert: (paths: string[]) => + applyDraftConfig(revertPathsTo(draftConfig ?? config, committed, paths)), + }), + [sectionChanges.draft, applyDraftConfig, committed, draftConfig, config], + ) + + // Revert for the PANEL's own inline bodies: writes the entity draft straight through `onChange`, + // matching how the panel's other inline sections (Tools, Instructions) already behave. Distinct + // from `drawerChangedPaths` above — the drawer has a scoped draft to respect, the panel doesn't. + const panelChangedPaths = useMemo( + () => ({ + ...sectionChanges.draft, + revert: (paths: string[]) => onChange(revertPathsTo(config, committed, paths)), + }), + [sectionChanges.draft, onChange, config, committed], + ) + // The inline body for a drawer-backed section: its own controls, narrowed to what changed — the + // same affordance as the Connect-key field, with a different filter (see SectionChangeBody). + // The body is a COMPONENT rendered inside the providers, never `mh`'s pre-built output: the hook + // reads these filters itself, and context resolves at the reader's position. + const changeBodyFor = useCallback( + (key: PanelSectionKey) => { + // The classifier already assigns each changed path to its own section bucket; read this + // section's paths from there so Advanced never picks up an instructions/tools/mcp/skills edit. + const section = sectionChanges.draft.sectionsByKey.get(key) + const sectionPaths = (section?.scalarChanges ?? []).map((c) => c.key) + if (!sectionPaths.length) return null + return ( + openSectionDrawer(key as "model-harness" | "advanced")} + disabled={disabled} + changes={panelChangedPaths} + > + + + ) + }, + [ + sectionChanges.draft, + openSectionDrawer, + panelChangedPaths, + disabled, + schema, + config, + onChange, + withTooltip, + revisionId, + savedHarnessValue, + ], + ) // Match unchanged values before identity so deleting an earlier item cannot make positional // identities mark every surviving row as edited. Identity then distinguishes new from edited. @@ -646,7 +704,7 @@ export function AgentTemplateControl({ if (invalid) return {tone: "invalid", tooltip: invalid} const incomplete = sectionIncompleteTip(key) if (incomplete) return {tone: "incomplete", tooltip: incomplete} - if (draftSectionKeys.has(key)) + if (draftSectionKeys.has(key as PanelSectionKey)) return { tone: "draft", tooltip: DRAFT_TIP[key] ?? "Unsaved changes — not yet committed.", @@ -709,7 +767,43 @@ export function AgentTemplateControl({ ) - // Each config section as a descriptor rendered by the accordion. Schema-gated, like before. + // The inline "what changed" bodies for the two drawer-backed sections. Null when the section is + // clean (or the variant is off), which is what keeps it a plain drawer-opening row. + const modelChangeBody = changeBodyFor("model-harness") + const advancedChangeBody = changeBodyFor("advanced") + + /** + * Only a BLOCKING problem opens a section by itself. "Can't run without a provider key" is a + * demand and earns the interruption; "you changed something" is an answer to a question the + * reader may not be asking — the header indicator already says a change is there, and expanding + * every changed section on mount would greet them with an unfolded panel every edit. So the + * change body is what the section shows WHEN OPENED, not a reason to open it. + */ + const needsProviderKeyInline = Boolean(mh.needsProviderKey && mh.providerCredentialsInline) + + // Graceful exit: when the key lands, `needsProviderKeyInline` flips false and the section would + // swap straight to its resolved (drawer-opening) row — the inline pane vanishing in one frame. + // Hold the inline branch mounted for one collapse cycle so the pane can animate closed instead. + // The transition is detected during render (not in an effect) so there's never an un-held frame + // where the pane has already been replaced by the resolved row. + const KEY_PANE_EXIT_MS = 320 + const prevNeedsKeyInlineRef = useRef(needsProviderKeyInline) + const [keyPaneExiting, setKeyPaneExiting] = useState(false) + if (prevNeedsKeyInlineRef.current !== needsProviderKeyInline) { + if (prevNeedsKeyInlineRef.current && !needsProviderKeyInline) setKeyPaneExiting(true) + prevNeedsKeyInlineRef.current = needsProviderKeyInline + } + useEffect(() => { + if (!keyPaneExiting) return + const t = window.setTimeout(() => setKeyPaneExiting(false), KEY_PANE_EXIT_MS) + return () => window.clearTimeout(t) + }, [keyPaneExiting]) + // Show (and keep mounting) the inline pane while it's needed OR animating out. + const showKeyPane = + (needsProviderKeyInline || keyPaneExiting) && Boolean(mh.providerCredentialsInline) + + // Each config section as a descriptor, so it can be rendered in any layout (accordion / + // tabs / cards) without duplicating the content. Schema-gated, like before. const sections = [ mh.hasModelOrHarness && { key: "model-harness", @@ -717,8 +811,37 @@ export function AgentTemplateControl({ title: "Model & harness", summary: mh.modelSummary, indicator: headerIndicator("model-harness"), - defaultOpen: true, - onOpen: () => openSectionDrawer("model-harness"), + defaultOpen: needsProviderKeyInline, + // What the section surfaces inline, in precedence order. Dropping `onOpen` is what makes + // a section expand inline instead of routing to the drawer. + // 1. Required info missing (no provider key) — BLOCKING, so it wins: the same key field + // the drawer uses, right here. + // 2. Uncommitted changes — informational: what changed (see `changeBodyFor`). + // 3. Neither — the plain drawer row it has always been. + ...(showKeyPane + ? { + // The body owns its padding (inside the collapse) so it can collapse to zero + // height with no residual — the section body wrapper adds none. + bodyClassName: "", + content: ( + +
+ openSectionDrawer("model-harness")} + disabled={disabled} + > + {mh.providerCredentialsInline} + +
+
+ ), + } + : modelChangeBody + ? {content: modelChangeBody} + : { + onOpen: () => openSectionDrawer("model-harness"), + content: mh.modelHarnessDrawerBody, + }), }, hasInstructions && { key: "instructions", @@ -841,9 +964,17 @@ export function AgentTemplateControl({ icon: , title: "Advanced", indicator: headerIndicator("advanced"), + // Never self-opening: nothing in Advanced blocks a run (see `needsProviderKeyInline`). defaultOpen: false, summary: mh.advancedSummary, - onOpen: () => openSectionDrawer("advanced"), + // Uncommitted changes → show them inline (dropping `onOpen` is what expands a section + // instead of routing to the drawer). Nothing changed → the plain drawer row. + ...(advancedChangeBody + ? {content: advancedChangeBody} + : { + onOpen: () => openSectionDrawer("advanced"), + content: mh.advancedDrawerBody, + }), }, ].filter(Boolean) as { key: string @@ -854,8 +985,8 @@ export function AgentTemplateControl({ indicator?: {tone: SectionIndicatorTone; tooltip?: string} defaultOpen?: boolean onOpen?: () => void - // Only the inline (non-`onOpen`) sections render a body; drawer-opening sections omit it. - content?: React.ReactNode + bodyClassName?: string + content: React.ReactNode }[] // Keep the item + instruction drawers MOUNTED while they animate closed. Their editing state @@ -887,8 +1018,17 @@ export function AgentTemplateControl({ titleBadge={sectionBadge(s.key)} summary={s.summary} extra={s.extra} - indicator={s.indicator ?? agentChangeIndicator(s.key)} + indicator={withDraftPulse( + s.key, + s.indicator ?? agentChangeIndicator(s.key), + )} onOpen={s.onOpen} + bodyClassName={s.bodyClassName} + // The panel body sits in the config section's `px-4` field wrapper + // (PlaygroundConfigSection), so the expanded header's fill bleeds 16px + // each side to the panel edges while its text stays aligned with the + // content below. + headerBand="-mx-4 px-4" noDivider={index === sections.length - 1} {...(controlled ? { @@ -981,17 +1121,24 @@ export function AgentTemplateControl({ dirty={sectionDirty} width={mh.modelHarnessDrawerWidth} > - + {/* Marks the exact properties that changed since the commit (and opens the + sub-sections holding them). Must sit ABOVE the body: `useModelHarness` reads the + context at its own position, so a provider rendered *by* the body wouldn't reach + it. Scoped to the draft diff — the drawer's own unsaved edits are its Save gate's + business, not "what changed since the commit". */} + + + - + + + {workflowReference?.enabled && ( diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/ClaudePermissionsControl.tsx b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/ClaudePermissionsControl.tsx index 66dcd3b25a..7ce6a2ad8e 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/ClaudePermissionsControl.tsx +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/ClaudePermissionsControl.tsx @@ -135,7 +135,15 @@ export const ClaudePermissionsControl = memo(function ClaudePermissionsControl({ return ( <> - + {/* `path` marks the row when it has an uncommitted change (see ChangedPathsProvider). + These are the flattened dot-paths the commit-diff classifier emits for this slice — + `harness.permissions.*` — so an approval card's "Always allow" grant (which appends + to `allow`) marks the Allow-rules row it actually wrote to. */} + value={current.defaultMode ?? undefined} onChange={(v) => write({defaultMode: v ?? null})} @@ -148,6 +156,7 @@ export const ClaudePermissionsControl = memo(function ClaudePermissionsControl({ " toggle: a grant made + * there lands here and can be removed here. It lists what was actually granted rather than a fixed + * menu, because the patterns must match the runner's gate names VERBATIM (`bash`, `Terminal`, …), + * which are harness/runtime-specific and not knowable up front. + */ +import {memo, useCallback, useMemo} from "react" + +import {Tag, Typography} from "antd" + +import {RailField, railInfoLabel} from "../../drawers/shared/RailField" + +export interface PiAutoApproveControlProps { + /** The harness `permissions` object (`harness.permissions`); may be absent. */ + value?: Record | null + /** Called with the next `harness.permissions` object. */ + onChange: (permissions: Record) => void + disabled?: boolean +} + +export const PiAutoApproveControl = memo(function PiAutoApproveControl({ + value, + onChange, + disabled = false, +}: PiAutoApproveControlProps) { + const allow = useMemo(() => { + const list = + value && typeof value === "object" && Array.isArray((value as {allow?: unknown}).allow) + ? ((value as {allow: unknown[]}).allow as unknown[]) + : [] + return list.filter((v): v is string => typeof v === "string") + }, [value]) + + const remove = useCallback( + (pattern: string) => { + const base = value && typeof value === "object" ? value : {} + onChange({...base, allow: allow.filter((entry) => entry !== pattern)}) + }, + [allow, value, onChange], + ) + + return ( + + {allow.length ? ( +
+ {allow.map((pattern) => ( + remove(pattern)} + className="!m-0 !font-mono !text-[11px]" + > + {pattern} + + ))} +
+ ) : ( + + Nothing auto-approved — every gated tool asks each time. + + )} +
+ ) +}) diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/SandboxPermissionControl.tsx b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/SandboxPermissionControl.tsx index 4b0f84286a..9b2544f3b5 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/SandboxPermissionControl.tsx +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/SandboxPermissionControl.tsx @@ -110,7 +110,13 @@ export function SandboxPermissionControl({ return ( <> - + {/* `path` = the dot-path the commit-diff classifier flattens this knob to, so a row with + an uncommitted change marks itself (see ChangedPathsProvider). */} + value={current.networkMode} onChange={(v) => write({networkMode: v})} @@ -121,7 +127,10 @@ export function SandboxPermissionControl({ {current.networkMode === "allowlist" ? ( - + @@ -140,7 +149,11 @@ export function SandboxPermissionControl({ ) : null} - + value={current.filesystem ?? undefined} onChange={(v) => write({filesystem: (v as FilesystemMode | undefined) ?? null})} @@ -152,7 +165,11 @@ export function SandboxPermissionControl({ /> - + value={current.enforcement} onChange={(v) => write({enforcement: v})} diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/SectionQuickAction.tsx b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/SectionQuickAction.tsx new file mode 100644 index 0000000000..e2c507e6c1 --- /dev/null +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/SectionQuickAction.tsx @@ -0,0 +1,45 @@ +/** + * SectionQuickAction + * + * The inline body a config section shows when required info is missing — a "resolve it right here" + * affordance instead of the plain row that only opens a drawer. It pairs the minimal control that + * fixes the gap (the SAME input the section's drawer uses, passed as `children`) with a link to the + * full drawer for everything else. First use: the Model & harness section's "Connect key" state + * (the provider API-key field). Kept generic so other sections can adopt the same pattern. + */ +import type {ReactNode} from "react" + +import {ArrowSquareOut} from "@phosphor-icons/react" +import {Button} from "antd" + +export interface SectionQuickActionProps { + /** The minimal control that resolves the missing info (e.g. the provider API-key field). */ + children: ReactNode + /** Opens the section's full configuration drawer. */ + onOpenDetails: () => void + /** Link label; defaults to "Detailed configuration". */ + detailsLabel?: string + disabled?: boolean +} + +export function SectionQuickAction({ + children, + onOpenDetails, + detailsLabel = "Detailed configuration", + disabled, +}: SectionQuickActionProps) { + return ( +
+ {children} + +
+ ) +} diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderCredentialsSection.tsx b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderCredentialsSection.tsx index 5f91c8c9ff..0b30c9a261 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderCredentialsSection.tsx +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderCredentialsSection.tsx @@ -73,6 +73,25 @@ export interface ProviderCredentialsSectionProps { // presentation disabled?: boolean + /** + * Compact/inline variant: no `ConfigAccordionSection` wrapper (no "Provider credentials" header + * or badge — the host section already owns those) and no provider rail (the selected model + * already fixes the provider). Just the mode toggle over the selected provider's key form or the + * self-managed card. Used when the pane is embedded inline under another section's header. + */ + bare?: boolean + /** Bare variant only: the model picker node, rendered in the header row to the left of the mode + * toggle (`[ model select ⋯ API key | Subscription ]`). */ + modelControl?: ReactNode + /** The displayed revision, threaded to the key form so a save can raise the "API key added" + * config-pane banner scoped to it. */ + revisionId?: string | null + + /** Uncommitted-change indicator for the section header (drawer / change surfaces) — mirrors the + * Harness/Model sections. Ignored in the `bare` variant (it renders no section header). */ + indicator?: {tone: "draft" | "invalid" | "incomplete" | "agent"; tooltip?: ReactNode} + /** Section-scoped revert control, rendered in the header beside the mode toggle. */ + revertControl?: ReactNode } const STANDARD_PREFIX = "std:" @@ -180,6 +199,49 @@ function RailRow({ ) } +/** The same rail entry laid out horizontally — a chip for the top-rail (rotated) layout. `dashed` + * marks an "add" action (opens the Configure drawer) vs a selectable provider. */ +function RailChip({ + active, + dashed, + disabled, + onClick, + tile, + label, + trailing, +}: { + active?: boolean + dashed?: boolean + disabled?: boolean + onClick: () => void + tile: ReactNode + label: string + trailing?: ReactNode +}) { + return ( + + ) +} + export function ProviderCredentialsSection({ mode, onModeChange, @@ -191,6 +253,11 @@ export function ProviderCredentialsSection({ providerNeedsKey, openConfigureProvider, disabled, + bare, + modelControl, + revisionId, + indicator, + revertControl, }: ProviderCredentialsSectionProps) { const standardSecrets = useAtomValue(standardSecretsAtom) const customSecrets = useAtomValue(customSecretsAtom) @@ -276,9 +343,9 @@ export function ProviderCredentialsSection({ const toggleOptions = useMemo( () => [ - modeOptions.includes("agenta") ? {label: "Use API key", value: "agenta"} : null, + modeOptions.includes("agenta") ? {label: "API key", value: "agenta"} : null, modeOptions.includes("self_managed") - ? {label: "Use subscription", value: "self_managed"} + ? {label: "Subscription", value: "self_managed"} : null, ].filter((option): option is {label: string; value: ConnectionMode} => option !== null), [modeOptions], @@ -287,12 +354,287 @@ export function ProviderCredentialsSection({ const showToggle = toggleOptions.length > 1 const guideUrl = selfHostingGuideUrl || DEFAULT_SELF_HOSTING_GUIDE_URL + // Shared segmented styling: a subtle elevated track (not a stark black rectangle on the near-black + // panel) with a theme-inverted selected fill (antd's default thumb resolves near-white in light + // mode, so the active segment would be invisible). Reused by the mode + provider-type toggles. + const segmentedClassName = cn( + "rounded-md border border-solid border-[var(--ag-colorBorder)] !bg-[var(--ag-colorFillTertiary)]", + "[&_.ant-segmented-item-selected]:!bg-[var(--ag-colorText)] [&_.ant-segmented-item-selected]:!text-[var(--ag-colorBgContainer)] [&_.ant-segmented-item-selected]:!shadow-none", + "[&_.ant-segmented-thumb]:!bg-[var(--ag-colorText)] [&_.ant-segmented-thumb]:!shadow-none", + ) + + const toggle = showToggle ? ( + onModeChange(value as ConnectionMode)} + options={toggleOptions} + className={segmentedClassName} + /> + ) : null + + const selfManagedCard = ( +
+
+ +
+
+ + Self-managed + +
    +
  • + + Use a Claude Code or Codex subscription, or any credential the harness + reads from its own environment (env vars, prior logins). + +
  • +
  • + + Agenta stores and injects no key. + +
  • +
  • + + Requires a self-hosted Agenta deployment. + +
  • +
+
+
+ + Read the self-hosting guide → + + {isCloud ? ( + // fallback until colorErrorBg token lands + + Unavailable in the cloud + + ) : null} +
+
+ ) + + // Read-only summary for a configured custom connection (its key edit lives in Settings → Secrets). + const renderCustomSummary = (secret: LlmProvider) => ( +
+
+ + {secret.name} + + + {PROVIDER_LABELS[secret.provider ?? ""] ?? secret.provider} + {" · manage this connection in Settings → Secrets."} + +
+ {secret.models?.length ? ( +
+ + Models + + + {secret.models.join(" · ")} + +
+ ) : null} +
+ ) + + // The selected provider's key form (or a read-only custom-provider summary / empty note). Shared + // by the section's master-detail pane and the bare inline variant. + const providerDetail = selectedStandardSecret ? ( + // Keyed by provider name: ProviderKeyField's internal draft-key useState otherwise survives a + // rail switch, letting provider A's half-typed key get saved under provider B. In the bare + // (compact) view the selected chip already names the provider, so drop the detail header. + + ) : selectedCustomProvider ? ( + renderCustomSummary(selectedCustomProvider) + ) : ( + + No provider configured for this model yet — add one from the list. + + ) + + // Custom-provider "Use custom provider" rows (open the Configure drawer), shared by rail + compact. + const addProviderRows = + openConfigureProvider && visibleAddRows.length + ? visibleAddRows.map((row) => ( + openConfigureProvider(row.kind)} + tile={} + label={row.label} + trailing={ + + } + /> + )) + : null + + const railDetail = ( +
+
+ {visibleStandardSecrets.map((secret) => ( + setManualKey(`${STANDARD_PREFIX}${secret.name}`)} + tile={ + + } + label={secret.title ?? secret.name ?? "Provider"} + trailing={ + secret.key ? ( + + ) : undefined + } + /> + ))} + {visibleCustomSecrets.map((secret) => ( + setManualKey(`${CUSTOM_PREFIX}${secret.name}`)} + tile={ + + } + label={secret.name ?? "Custom provider"} + trailing={ + + } + /> + ))} + {addProviderRows ? ( +
+ + Use custom provider + + {addProviderRows} +
+ ) : null} +
+ +
{providerDetail}
+
+ ) + + // Top-rail (rotated master-detail): the same provider list as `railDetail`, but the rail runs + // horizontally across the top and the detail (the OpenAI heading + key form) fills the width + // below. Suits the narrow inline column, where a side rail leaves the key form cramped. Reuses + // the shared `activeKey`/`setManualKey`/`providerDetail` so selection stays consistent. + const topRail = ( +
+
+ {visibleStandardSecrets.map((secret) => ( + setManualKey(`${STANDARD_PREFIX}${secret.name}`)} + tile={ + + } + label={secret.title ?? secret.name ?? "Provider"} + trailing={ + secret.key ? ( + + ) : undefined + } + /> + ))} + {visibleCustomSecrets.map((secret) => ( + setManualKey(`${CUSTOM_PREFIX}${secret.name}`)} + tile={ + + } + label={secret.name ?? "Custom provider"} + trailing={ + + } + /> + ))} + {openConfigureProvider && visibleAddRows.length + ? visibleAddRows.map((row) => ( + openConfigureProvider(row.kind)} + tile={} + label={row.label} + trailing={ + + } + /> + )) + : null} +
+
{providerDetail}
+
+ ) + + // Compact inline variant: a `[ model select ⋯ API key | Subscription ]` header row over the + // top-rail provider strip (or the self-managed card) — no accordion header/badge; the host + // section owns those. + if (bare) { + return ( +
+ {/* Header: model picker takes the remaining width, mode toggle sits right. */} + {modelControl || toggle ? ( +
+
{modelControl}
+ {toggle} +
+ ) : null} + {mode === "self_managed" ? selfManagedCard : topRail} +
+ ) + } + return ( } title="Provider credentials" status={providerNeedsKey ? "warning" : "complete"} + indicator={indicator} titleBadge={ providerNeedsKey ? ( @@ -304,189 +646,15 @@ export function ProviderCredentialsSection({ summaryCollapsedOnly noDivider extra={ - showToggle ? ( - onModeChange(value as ConnectionMode)} - options={toggleOptions} - className={cn( - "rounded-md border border-solid border-[var(--ag-colorBorder)]", - // antd's default selected-thumb bg/track bg both resolve to - // near-white in light mode, so the "active" segment is invisible — - // force a strong, theme-inverted fill instead (dark-navy-on-white - // in light mode, near-white-on-near-black in dark mode). - "[&_.ant-segmented-item-selected]:!bg-[var(--ag-colorText)] [&_.ant-segmented-item-selected]:!text-[var(--ag-colorBgContainer)] [&_.ant-segmented-item-selected]:!shadow-none", - "[&_.ant-segmented-thumb]:!bg-[var(--ag-colorText)] [&_.ant-segmented-thumb]:!shadow-none", - )} - /> + revertControl || toggle ? ( +
+ {revertControl} + {toggle} +
) : undefined } > - {mode === "self_managed" ? ( -
-
- -
-
- - Self-managed - -
    -
  • - - Use a Claude Code or Codex subscription, or any credential the - harness reads from its own environment (env vars, prior logins). - -
  • -
  • - - Agenta stores and injects no key. - -
  • -
  • - - Requires a self-hosted Agenta deployment. - -
  • -
-
-
- - Read the self-hosting guide → - - {isCloud ? ( - // fallback until colorErrorBg token lands - - Unavailable in the cloud - - ) : null} -
-
- ) : ( -
-
- {visibleStandardSecrets.map((secret) => ( - setManualKey(`${STANDARD_PREFIX}${secret.name}`)} - tile={ - - } - label={secret.title ?? secret.name ?? "Provider"} - trailing={ - secret.key ? ( - - ) : undefined - } - /> - ))} - {visibleCustomSecrets.map((secret) => ( - setManualKey(`${CUSTOM_PREFIX}${secret.name}`)} - tile={ - - } - label={secret.name ?? "OpenAI-compatible endpoint"} - trailing={ - - } - /> - ))} - {openConfigureProvider && visibleAddRows.length ? ( -
- - Add custom provider - - {visibleAddRows.map((row) => ( - openConfigureProvider(row.kind)} - tile={} - label={row.label} - trailing={ - - } - /> - ))} -
- ) : null} -
- -
- {selectedStandardSecret ? ( - // Keyed by provider name: ProviderKeyField's internal draft-key - // useState otherwise survives a rail switch, letting provider A's - // half-typed key get saved under provider B. - - ) : selectedCustomProvider ? ( -
-
- - {selectedCustomProvider.name} - - - {PROVIDER_LABELS[selectedCustomProvider.provider ?? ""] ?? - selectedCustomProvider.provider} - {" · manage this connection in Settings → Secrets."} - -
- {selectedCustomProvider.models?.length ? ( -
- - Models - - - {selectedCustomProvider.models.join(" · ")} - -
- ) : null} -
- ) : ( - - No provider configured for this model yet — add one from the list. - - )} -
-
- )} + {mode === "self_managed" ? selfManagedCard : railDetail}
) } diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderKeyField.tsx b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderKeyField.tsx index 250e7079e6..12bc8579e8 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderKeyField.tsx +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderKeyField.tsx @@ -1,9 +1,13 @@ -import {useState} from "react" +import {useEffect, useRef, useState} from "react" import {useVaultSecret} from "@agenta/entities/secret" +import {providerKeyAddedSignalAtom} from "@agenta/shared/state" import type {LlmProvider} from "@agenta/shared/types" +import {useAccordionSectionOpen} from "@agenta/ui/components/presentational" import {CheckCircle} from "@phosphor-icons/react" import {App, Button, Input, Typography} from "antd" +import type {InputRef} from "antd" +import {useSetAtom} from "jotai" /** * Right-pane "API key" form for a standard provider — the Provider credentials section's key form @@ -12,19 +16,55 @@ import {App, Button, Input, Typography} from "antd" * exists, and an encryption footnote. Saves to the project vault via `useVaultSecret`, which also * arms `providerKeySetupDoneAtom` — no drawer Save step, so the "Connect key" gate clears reactively. */ -const ProviderKeyField = ({provider, disabled}: {provider: LlmProvider; disabled?: boolean}) => { +const ProviderKeyField = ({ + provider, + disabled, + hideHeader, + revisionId, +}: { + provider: LlmProvider + disabled?: boolean + /** Drop the name + "Standard provider · …" subtitle — used where a selected rail chip already + * names the provider, so repeating it in the detail is noise. */ + hideHeader?: boolean + /** The displayed revision, so a successful save can raise the "API key added" config-pane banner + * scoped to it. */ + revisionId?: string | null +}) => { const {message} = App.useApp() const {handleModifyVaultSecret} = useVaultSecret() + const raiseKeyAddedSignal = useSetAtom(providerKeyAddedSignalAtom) const [key, setKey] = useState("") const [saving, setSaving] = useState(false) + const inputRef = useRef(null) + + // Focus the key input when the enclosing section is open (and each time it re-opens), not just on + // mount: the section body stays mounted while collapsed, so a mount-time `autoFocus` would fire + // while hidden. Only when there's no key yet — an existing key isn't waiting to be typed. + const sectionOpen = useAccordionSectionOpen() + const hasKey = !!provider.key + useEffect(() => { + if (!sectionOpen || hasKey || disabled) return + const t = window.setTimeout(() => inputRef.current?.focus(), 0) + return () => window.clearTimeout(t) + }, [sectionOpen, hasKey, disabled]) const save = async () => { const trimmed = key.trim() if (!trimmed || saving || disabled) return + const isFirstKey = !provider.key setSaving(true) try { await handleModifyVaultSecret({...provider, key: trimmed}) setKey("") + // Only the FIRST key connection unblocks the run — a replace doesn't need the banner. + if (isFirstKey && revisionId) { + raiseKeyAddedSignal({ + revisionId, + provider: provider.title ?? provider.name ?? undefined, + at: Date.now(), + }) + } } catch { // Security-sensitive write — never fail silently; keep the typed value so the user can retry. message.error("Couldn't save the provider key. Please try again.") @@ -33,36 +73,43 @@ const ProviderKeyField = ({provider, disabled}: {provider: LlmProvider; disabled } } - const hasKey = !!provider.key - return ( -
-
- - {provider.title} - - - Standard provider · add your key and we auto-list its models. - - {hasKey ? ( - +
+ {hideHeader ? ( + hasKey ? ( + Key configured · enter a new value to replace it. - ) : null} -
+ ) : null + ) : ( +
+ + {provider.title} + + + Standard provider · add your key and we auto-list its models. + + {hasKey ? ( + + + Key configured · enter a new value to replace it. + + ) : null} +
+ )}
API key *
setKey(e.target.value)} onPressEnter={save} placeholder="sk-…" className="flex-1 font-mono" - autoFocus={!hasKey} disabled={disabled} /> + + ) : undefined + + // FOCUS (see FocusPathsContext): when a surface narrows to the properties that matter — e.g. the + // config panel showing only what changed — a group renders only if it owns one of them, and the + // rows filter themselves. Chrome follows the content: with changes in ONE group there is nothing + // to disambiguate, so it renders FLAT (just the controls, like the Connect-key field); spread + // across several, the group headers earn their keep by saying which change belongs where. + const focus = useFocusPaths() + const sandboxInFocus = useHasFocusUnder("sandbox") + const runnerInFocus = useHasFocusUnder("runner") + // Split like the changed/revert side: harness.kind focuses the Model & harness section, + // harness.permissions the Permissions group — so neither pulls the other into focus. + const harnessKindInFocus = useHasFocusUnder("harness.kind") + const harnessPermsInFocus = useHasFocusUnder("harness.permissions") + const permissionsInFocus = runnerInFocus || harnessPermsInFocus + const focusedGroupCount = (sandboxInFocus ? 1 : 0) + (permissionsInFocus ? 1 : 0) + const flatFocus = focus.active && focusedGroupCount <= 1 + + // Same, for the Model & harness inline body: its three groups own harness.kind / llm.model / + // llm.connection.*, so under a change filter only the group(s) that actually changed render (a + // model swap shows the Model group, and Provider credentials too only if it moved the connection), + // flat when just one survives — instead of unfolding the entire section. + const modelInFocus = useHasFocusUnder("llm.model") + const credentialsInFocus = useHasFocusUnder("llm.connection") + const modelHarnessFocusedCount = + (harnessKindInFocus ? 1 : 0) + (modelInFocus ? 1 : 0) + (credentialsInFocus ? 1 : 0) + const modelHarnessFlatFocus = focus.active && modelHarnessFocusedCount <= 1 + const hasAdvanced = Boolean( sandboxProps.kind || sandboxProps.permissions || @@ -468,6 +564,7 @@ export function useModelHarness({ : "Model" } align="center" + path="llm.model" > {modelControl} @@ -596,18 +693,33 @@ export function useModelHarness({ // Provider credentials section: identical in both the capability-aware and flat layouts below, // rendered once and reused so the two branches don't carry a duplicate prop list. - const providerCredentialsSection = props.llm ? ( + const providerCredentialsProps = props.llm + ? { + mode: connection.mode, + onModeChange: (m: ConnectionMode) => writeModel({mode: m}), + selectedProviderFamily, + selectedConnectionSlug: connection.slug ?? null, + modeOptions, + isCloud, + selfHostingGuideUrl: deployment?.selfHostingGuideUrl, + providerNeedsKey, + openConfigureProvider: openConfigureProviderAdopting, + disabled, + revisionId: revisionId ?? null, + indicator: changedIndicator(credentialsChanged), + revertControl: revertAction(revertCredentials), + } + : null + // The full pane (own header + rail) for the drawer body; the `bare` variant (toggle + key form / + // self-managed, no header, no rail) for the inline "Connect key" quick-action under the section. + const providerCredentialsSection = providerCredentialsProps ? ( + + ) : null + const providerCredentialsInline = providerCredentialsProps ? ( writeModel({mode: m})} - selectedProviderFamily={selectedProviderFamily} - selectedConnectionSlug={connection.slug ?? null} - modeOptions={modeOptions} - isCloud={isCloud} - selfHostingGuideUrl={deployment?.selfHostingGuideUrl} - providerNeedsKey={providerNeedsKey} - openConfigureProvider={openConfigureProviderAdopting} - disabled={disabled} + {...providerCredentialsProps} + bare + modelControl={modelControl} /> ) : null @@ -615,54 +727,83 @@ export function useModelHarness({ // left (each card owns its model-compat state), version history on the right — same two-panel // shape as the Advanced drawer. Without capabilities: the plain harness select, single column. // Shared Model & harness controls — rendered by both the wide drawer body and the tabs-inline body. + // A focused (change/connect-key) surface renders only the groups that own a changed property, and + // drops to FLAT chrome when just one survives; unfocused (drawer/tabs) everything renders as-is. + const modelControlGroup = ( +
+ {modelControl} + {!focus.active && hasInspectModels ? ( + + Filtered to the models this harness can reach. Selecting a model also sets its + provider. + + ) : null} +
+ ) + const modelHarnessControls = capabilities ? ( <> - } - title="Harness" - status={harnessValue ? "complete" : "default"} - summary={selectedHarnessLabel ?? undefined} - summaryCollapsedOnly - > -
- - - The harness is the runtime that executes your agent. It decides which - providers, hosting and connection options you can use. - -
- {harnessSection} -
+ {harnessKindInFocus ? ( + modelHarnessFlatFocus ? ( + harnessSection + ) : ( + } + title="Harness" + status={harnessValue ? "complete" : "default"} + indicator={changedIndicator(harnessKindChanged)} + extra={revertAction(revertHarnessKind)} + summary={selectedHarnessLabel ?? undefined} + summaryCollapsedOnly + > + {focus.active ? null : ( +
+ + + The harness is the runtime that executes your agent. It decides + which providers, hosting and connection options you can use. + +
+ )} + {harnessSection} +
+ ) + ) : null} - } - title="Model" - status={!modelId || !selectedKeepsModel ? "warning" : "complete"} - summary={modelId ?? undefined} - summaryCollapsedOnly - > -
- {modelControl} - {hasInspectModels ? ( - - Filtered to the models this harness can reach. Selecting a model also - sets its provider. - - ) : null} -
-
+ {modelInFocus ? ( + modelHarnessFlatFocus ? ( + // Flat (a change surface narrowed to just the model): a labelled RailField row, so + // the changed control marks itself and its label opens the committed-value + Restore + // popover — the same property-scoped affordance the Advanced rows get. + + {modelControl} + + ) : ( + } + title="Model" + status={!modelId || !selectedKeepsModel ? "warning" : "complete"} + indicator={changedIndicator(modelChanged)} + extra={revertAction(revertModel)} + summary={modelId ?? undefined} + summaryCollapsedOnly + > + {modelControlGroup} + + ) + ) : null} - {providerCredentialsSection} + {credentialsInFocus ? providerCredentialsSection : null} ) : ( <> - {harnessProps.kind && ( - + {harnessKindInFocus && harnessProps.kind && ( + )} - {modelPicker} - {providerCredentialsSection} + {modelInFocus ? modelPicker : null} + {credentialsInFocus ? providerCredentialsSection : null} ) @@ -701,122 +842,197 @@ export function useModelHarness({ // Each group is a `ConfigAccordionSection` (the shared drawer section shell used by the trigger // and tools drawers); inside, configuration reads as the drawer's `[rail | content]` rhythm via // `SectionRail` (mode groups) and `RailField` (labelled control rows). - const advancedControls = ( + /** + * One Advanced group. Under a focus filter it disappears unless it owns a focused property, and + * drops its chrome entirely when it's the only survivor — the body (the real controls) is the + * same either way, so nothing is rendered twice. + */ + const advancedGroup = ( + opts: { + inFocus: boolean + defaultOpen: boolean + indicator: ReturnType + extra: ReactNode + icon: ReactNode + title: string + summary?: string + caption: ReactNode + }, + body: ReactNode, + ) => { + if (!opts.inFocus) return null + // Flat: just the focused control(s) — the group header would only restate the section. + if (flatFocus) return
{body}
+ return ( + + {opts.caption} + {body} + + ) + } + + const executionBody = ( <> - {buildKitSection} - - {hasExecutionGroup ? ( - } - title="Execution environment" - summary={sandbox.kind ? `Sandbox: ${String(sandbox.kind)}` : undefined} - summaryCollapsedOnly - > - - Where the agent's tools and code run, and what that sandbox may touch. - - {sandboxProps.kind ? ( - - setSection("sandbox", {...sandbox, kind: v})} - withTooltip={withTooltip} - disabled={disabled} - /> - - ) : null} - {sandboxProps.permissions && sandbox.kind !== "local" ? ( - <> - {permissionOverrideHint} - {/* Renders its knobs as peer RailField rows (Network egress / Filesystem - / Enforcement) sharing this section's rail — no nested sub-form. */} - | null) ?? null - } - onChange={(v) => - setSection("sandbox", {...sandbox, permissions: v}) - } - disabled={disabled} - /> - - ) : null} - + {sandboxProps.kind ? ( + + setSection("sandbox", {...sandbox, kind: v})} + withTooltip={withTooltip} + disabled={disabled} + /> + ) : null} + {sandboxProps.permissions && sandbox.kind !== "local" ? ( + <> + {focus.active ? null : permissionOverrideHint} + {/* Renders its knobs as peer RailField rows (Network egress / Filesystem + / Enforcement) sharing this section's rail — no nested sub-form. */} + | null) ?? null} + onChange={(v) => setSection("sandbox", {...sandbox, permissions: v})} + disabled={disabled} + /> + + ) : null} + + ) - {hasPermissionsGroup ? ( - } - title="Permissions" - summary={runnerPermissionSummary} - summaryCollapsedOnly - > - - What the agent may do on its own before it must ask. - - {runnerPermissionSchema ? ( - - - value={currentRunnerPermission} - onChange={(v) => - setSection("runner", { - ...runner, - permissions: {...runnerPermissions, default: v}, - }) - } - options={runnerPermissionOptions} - optionLabelProp="title" - disabled={disabled} - className="w-full" - /> - - ) : null} - {hasClaudePermissions ? ( - <> - {/* Caption then peer rail rows (mode / allow / ask / deny) sharing the - section rail — the control renders its own RailField rows. */} - - Claude harness - - - | undefined - )?.default_mode - } - /> - - ) : null} - {hasPiSettings ? ( - <> - - Pi harness - - - - ) : null} - + const permissionsBody = ( + <> + {runnerPermissionSchema ? ( + + + value={currentRunnerPermission} + onChange={(v) => + setSection("runner", { + ...runner, + permissions: {...runnerPermissions, default: v}, + }) + } + options={runnerPermissionOptions} + optionLabelProp="title" + disabled={disabled} + className="w-full" + /> + + ) : null} + {hasClaudePermissions ? ( + <> + {/* Caption then peer rail rows (mode / allow / ask / deny) sharing the + section rail — the control renders its own RailField rows. */} + {focus.active ? null : ( + + Claude harness + + )} + + | undefined + )?.default_mode + } + /> + + ) : null} + {hasPiSettings ? ( + <> + {focus.active ? null : ( + + Pi harness + + )} + {/* Availability, not permission — it declares no config path, so a focus + filter drops it and only the changed permission rows remain. */} + {focus.active ? null : ( + + )} + | null) ?? null} + onChange={(permissions) => setSection("harness", {...harness, permissions})} + disabled={disabled} + /> + ) : null} ) + const advancedControls = ( + <> + {/* Playground-only overlay — it owns no committed property, so a focus filter drops it. */} + {focus.active ? null : buildKitSection} + + {hasExecutionGroup + ? advancedGroup( + { + inFocus: sandboxInFocus, + defaultOpen: sandboxChanged, + indicator: changedIndicator(sandboxChanged), + extra: revertAction(revertSandbox), + icon: , + title: "Execution environment", + summary: sandbox.kind ? `Sandbox: ${String(sandbox.kind)}` : undefined, + caption: ( + + Where the agent's tools and code run, and what that sandbox + may touch. + + ), + }, + executionBody, + ) + : null} + + {hasPermissionsGroup + ? advancedGroup( + { + inFocus: permissionsInFocus, + defaultOpen: permissionsChanged, + indicator: changedIndicator(permissionsChanged), + extra: permissionsChanged ? revertAction(revertPermissions) : undefined, + icon: , + title: "Permissions", + summary: runnerPermissionSummary, + caption: ( + + What the agent may do on its own before it must ask. + + ), + }, + permissionsBody, + ) + : null} + + ) + // The stacked sections carry their own dividers; drop the trailing one on whichever section // renders last (they're conditional, so target the last child rather than a fixed section). const advancedDrawerBody = ( @@ -834,6 +1050,10 @@ export function useModelHarness({ // The selected model's provider has a standard vault slot but no key yet — the config panel // highlights the Model & harness section and the chat gates on it until it's connected. needsProviderKey: providerNeedsKey, + // The compact `bare` credentials pane (mode toggle + selected provider's key form or + // self-managed card; no nested header/badge, no rail) for the inline "Connect key" + // quick-action under the section header — aligned with the drawer without duplicating it. + providerCredentialsInline, // A model is selected but the chosen harness can't run it — a *model* problem (the harness // itself stays valid), so the config panel flags the Model & harness section as invalid. modelUnsupported: !!modelId && !selectedKeepsModel, diff --git a/web/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection.tsx b/web/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection.tsx index 07f41d8cf7..1e6c8f3a59 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection.tsx +++ b/web/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection.tsx @@ -1824,16 +1824,20 @@ function PlaygroundConfigSection({ return
{props.defaultRender()}
} - // The agent_config body is always expanded (its header is suppressed above), so it renders - // without the HeightCollapse toggle. + // The agent body is always expanded (its header is suppressed above), so it renders + // without the HeightCollapse toggle. Match BOTH markers (modern `agent-template`, + // legacy `agent_config`) — same detection as the header suppressor above. const fieldSchema = schema?.properties ? (schema.properties as Record>)[fieldKey] : null + const isAgentBody = (v: unknown) => v === "agent-template" || v === "agent_config" if ( - fieldSchema?.["x-ag-type-ref"] === "agent_config" || - fieldSchema?.["x-ag-type"] === "agent_config" + isAgentBody(fieldSchema?.["x-ag-type-ref"]) || + isAgentBody(fieldSchema?.["x-ag-type"]) ) { - return
{props.defaultRender()}
+ // pb-3 only: the first section row (ConfigAccordionSection) carries its own py-3 + // header, so a top py-3 here would double into a dead 24px band under the header bar. + return
{props.defaultRender()}
} return ( @@ -1858,10 +1862,10 @@ function PlaygroundConfigSection({ if (isConfigLoading) { if (loadingFallback) { - // px-4 py-3 mirrors the field-content wrapper the real sections (and the lazy - // control's Suspense fallback) render inside — without it the fallback skeleton - // sits 16px wider / 12px higher and visibly shifts when the schema lands. - return
{loadingFallback}
+ // px-4 pb-3 mirrors the field-content wrapper the real sections (and the lazy + // control's Suspense fallback) render inside — same inset, so the fallback skeleton + // doesn't shift when the schema lands. + return
{loadingFallback}
} return (
From bf96673da9de7d11359abf557ee041b2d7cbcc1e Mon Sep 17 00:00:00 2001 From: Arda Erzin Date: Fri, 17 Jul 2026 23:10:12 +0200 Subject: [PATCH 06/11] =?UTF-8?q?feat(agent-chat):=20reduce=20approval=20f?= =?UTF-8?q?riction=20=E2=80=94=20always-allow=20+=20batch=20resolve?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approval dock gains an 'Always allow for this agent' toggle that writes a per-tool permission (gateway/custom-function) or harness allow-rule into the draft config (toolPermission helpers), so the tool stops gating on resume and future runs; a contained notice offers Undo. Answering resolves the whole parked batch in one step — 'Approve all', or 'Approve' with the toggle on also clears that tool's sibling gates — instead of stepping through 1-of-N. --- .../components/ApprovalDock.tsx | 121 +++++++-- .../Components/AlwaysAllowedNotice.tsx | 88 +++++++ .../Components/MainLayout/index.tsx | 8 +- web/oss/src/hooks/useAlwaysAllowTool.tsx | 128 ++++++++++ .../src/DrillInView/SchemaControls/index.ts | 9 + .../SchemaControls/toolPermission.ts | 230 ++++++++++++++++++ .../agenta-entity-ui/src/DrillInView/index.ts | 9 + .../tests/unit/toolPermission.test.ts | 224 +++++++++++++++++ 8 files changed, 798 insertions(+), 19 deletions(-) create mode 100644 web/oss/src/components/Playground/Components/AlwaysAllowedNotice.tsx create mode 100644 web/oss/src/hooks/useAlwaysAllowTool.tsx create mode 100644 web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/toolPermission.ts create mode 100644 web/packages/agenta-entity-ui/tests/unit/toolPermission.test.ts diff --git a/web/oss/src/components/AgentChatSlice/components/ApprovalDock.tsx b/web/oss/src/components/AgentChatSlice/components/ApprovalDock.tsx index ad4a9865c3..2f2ba36e0c 100644 --- a/web/oss/src/components/AgentChatSlice/components/ApprovalDock.tsx +++ b/web/oss/src/components/AgentChatSlice/components/ApprovalDock.tsx @@ -3,9 +3,11 @@ import {memo, useEffect, useMemo, useRef, useState} from "react" import {HeightCollapse} from "@agenta/ui" import {ArrowSquareOut, CaretRight, ShieldCheck} from "@phosphor-icons/react" import type {ToolUIPart, UIMessage} from "ai" -import {Button, Typography} from "antd" +import {Button, Switch, Typography} from "antd" import {useAtomValue} from "jotai" +import {useAlwaysAllowTool} from "@/oss/hooks/useAlwaysAllowTool" + import {partToolName, resolveToolDisplay} from "../assets/toolDisplay" import {chatPanelMaximizedAtom} from "../state/panelLayout" @@ -125,21 +127,47 @@ const ApprovalDock = ({ className, }: ApprovalDockProps) => { const open = approvals.length > 0 - // Latch the last non-empty set so the card stays visible while the dock animates closed — a - // leave transition needs its content to persist through the height collapse. + // A resolve can answer SEVERAL gates at once — "Approve all", or "Approve" with the always-allow + // toggle on (which also clears that tool's other pending gates, since the user asked not to be + // prompted for it again). Each response settles asynchronously (the SDK's serial job queue), so + // the pending set shrinks across renders; without a latch the dock would step through the batch + // ("1 of 3 → 1 of 2"). `resolvingIds` holds the gates we fired responses for; while any is still + // pending we FREEZE the shown set so the card holds steady and the dock closes in one step (or, + // if only some gates were covered, then steps to the uncovered remainder). + const [resolvingIds, setResolvingIds] = useState(null) + const [resolveSource, setResolveSource] = useState<"all" | "one" | null>(null) + const resolving = + resolvingIds !== null && approvals.some((a) => resolvingIds.includes(a.approvalId)) + // Latch the last non-empty set so the card stays visible while the dock animates closed (a leave + // transition needs its content through the height collapse) AND so a multi-gate resolve doesn't + // step through the batch. const shownRef = useRef(approvals) - if (open) shownRef.current = approvals + if (open && !resolving) shownRef.current = approvals const shown = shownRef.current const current = shown[0] const count = shown.length const [responding, setResponding] = useState(false) + // Armed "always allow this tool" intent for the current gate — applied only when the user + // clicks Approve, never on its own (the switch must not progress the flow). + const [alwaysAllowArmed, setAlwaysAllowArmed] = useState(false) - // The current gate changed (we answered one, the next slid in) — re-enable. + // The current gate changed (we answered one, the next slid in) — re-enable and disarm. Held + // during a resolve (current is frozen), so it fires only on a real step or a new batch. useEffect(() => { setResponding(false) + setResolveSource(null) + setAlwaysAllowArmed(false) }, [current?.approvalId]) + // Once every gate we fired has settled (left the pending set), drop the latch — the dock then + // closes if nothing remains, or re-latches onto the uncovered gates (a mixed-tool batch). + useEffect(() => { + if (resolvingIds !== null && !approvals.some((a) => resolvingIds.includes(a.approvalId))) { + setResolvingIds(null) + } + }, [approvals, resolvingIds]) + // Friendly bodies are Chat-mode (maximized) sugar and need a revision to diff against; // Build and the entityId-less host keep the exact-payload card. const chatMode = useAtomValue(chatPanelMaximizedAtom) @@ -152,28 +180,54 @@ const ApprovalDock = ({ // A source badge we can state factually from the tool name — not a guessed risk level. const source = friendly?.kind === "mcp" ? "MCP tool" : null + // "Always allow this tool": writes a config permission so the runner stops gating this tool + // (per-tool `permission` for gateway/custom-function tools; `harness.permissions.allow` for + // harness builtins like bash). Platform ops (commit_revision, schedules) and MCP are not + // eligible, so they always stay gated. The write happens on APPROVE (see `respond`), never when + // the switch is toggled — the switch only arms the intent. buildAgentRequest re-reads the draft + // on resume, so the grant takes effect for the current run and every future one. + const {infoFor, grant} = useAlwaysAllowTool(entityId) + const grantInfo = current ? infoFor(current.toolName) : null + const canAlwaysAllow = Boolean(grantInfo?.eligible && !grantInfo.alreadyAllowed) + const respond = (approved: boolean) => { if (responding || !current) return setResponding(true) + setResolveSource("one") + // Apply the armed grant only on approve — never on deny, and never from the switch alone. + if (approved && alwaysAllowArmed && canAlwaysAllow) { + grant(current.toolName) + // "Always allow " also clears this tool's OTHER pending gates in the batch: the + // user said they don't want to be prompted for it again, so its siblings auto-approve + // in one step instead of making them click through 2/3, 3/3. Other tools stay gated and + // are shown next. + const covered = shown.filter((a) => a.toolName === current.toolName) + if (covered.length > 1) { + setResolvingIds(covered.map((a) => a.approvalId)) + covered.forEach((a) => onApprovalResponse({id: a.approvalId, approved: true})) + return + } + } onApprovalResponse({id: current.approvalId, approved}) } const approveAll = () => { if (responding) return setResponding(true) + setResolveSource("all") + if (alwaysAllowArmed && canAlwaysAllow && current) grant(current.toolName) + // Freeze the card so the dock doesn't step through the batch as each response settles — it + // holds "1 of N" and closes once all are answered (see `resolvingIds`). + setResolvingIds(shown.map((a) => a.approvalId)) shown.forEach((a) => onApprovalResponse({id: a.approvalId, approved: true})) } - // Always mounted; enter + leave animate via the grid-rows 0fr↔1fr height collapse (+ opacity), - // the same idiom as the reasoning block and composer attachments. `inert` while closed drops the - // (clipped, latched) card from tab order + a11y so a keyboard user can't reach hidden buttons. + // Always mounted; enter + leave animate via the shared HeightCollapse (CSS height + fade, + // reduced-motion-proof) — the same primitive the queue, connect banner, and config sections use. + // `inert` while closed drops the (clipped, latched) card from tab order + a11y so a keyboard user + // can't reach hidden buttons. return ( -
-
+ +
{current ? ( // The friendly two-pane body needs more air than the one-line payload card.
{count > 1 ? ( - ) : null} @@ -284,17 +342,44 @@ const ApprovalDock = ({
+ + {/* Always-allow: arms a config write-through so this tool stops asking. The + switch only ARMS the intent (it must not progress the flow); the grant is + applied when the user clicks Approve. Shown only for gateway / + custom-function / builtin gates that aren't already allowed. */} + {canAlwaysAllow ? ( +
+ +
+ + Always allow{" "} + + {friendly?.label ?? current.toolName} + {" "} + for this agent + + + Applies when you approve; commit to use it in triggers. + +
+
+ ) : null}
) : null}
-
+ ) } diff --git a/web/oss/src/components/Playground/Components/AlwaysAllowedNotice.tsx b/web/oss/src/components/Playground/Components/AlwaysAllowedNotice.tsx new file mode 100644 index 0000000000..8b24282480 --- /dev/null +++ b/web/oss/src/components/Playground/Components/AlwaysAllowedNotice.tsx @@ -0,0 +1,88 @@ +import {useEffect, useRef} from "react" + +import {draftConfigChangeSignalAtom} from "@agenta/shared/state" +import {HeightCollapse} from "@agenta/ui" +import {ArrowCounterClockwise, ShieldCheck, X} from "@phosphor-icons/react" +import {Button} from "antd" +import {useAtom} from "jotai" + +import {useAlwaysAllowTool} from "@/oss/hooks/useAlwaysAllowTool" + +/** + * "Always allowed" notice — the draft-blue counterpart of {@link AgentCommitNotice}, pinned to the + * bottom of the config pane. Surfaces a per-tool permission the user just granted from the approval + * dock, with Undo, CONTAINED to the config panel (where the write lands) rather than a floating + * toast that pulls focus away. Reads the same draft-change signal that pulses the affected section. + * Undo reverts the write and clears; Dismiss just clears. Collapses in/out via the shared + * {@link HeightCollapse} (fade + a small Y slide) — the same CSS-native, reduced-motion-proof + * primitive the composer dock, queued messages, and accordion sections use. + */ +const AlwaysAllowedNotice = ({revisionId}: {revisionId: string}) => { + const [signal, setSignal] = useAtom(draftConfigChangeSignalAtom) + const active = Boolean( + signal && + revisionId && + signal.revisionId === revisionId && + signal.origin === "approval-dock", + ) + const {revoke} = useAlwaysAllowTool(revisionId) + + // Latch the last matching signal so content stays rendered through the collapse-out. + const lastRef = useRef(signal) + if (signal && active) lastRef.current = signal + const shown = lastRef.current + + // Auto-dismiss 5s after the change (keyed on `at`, so a fresh grant restarts the clock). The + // persistent section draft dot is independent of this signal, so it stays after the banner goes. + useEffect(() => { + if (!active) return + const t = window.setTimeout(() => setSignal(null), 5000) + return () => window.clearTimeout(t) + }, [active, signal?.at, setSignal]) + + const label = shown?.label ?? "this tool" + + return ( + +
+
+
+ + + +
+ + Always allowing {label} + + + Saved to this draft — this tool won't ask again. + +
+
+
+ +
+
+
+
+ ) +} + +export default AlwaysAllowedNotice diff --git a/web/oss/src/components/Playground/Components/MainLayout/index.tsx b/web/oss/src/components/Playground/Components/MainLayout/index.tsx index ee6e5870aa..34ad20995c 100644 --- a/web/oss/src/components/Playground/Components/MainLayout/index.tsx +++ b/web/oss/src/components/Playground/Components/MainLayout/index.tsx @@ -27,7 +27,9 @@ import {playgroundEarlyAgentStateAtom} from "@/oss/state/workflow" import {usePlaygroundScrollSync} from "../../hooks/usePlaygroundScrollSync" import AgentCommitNotice from "../AgentCommitNotice" +import AlwaysAllowedNotice from "../AlwaysAllowedNotice" import PlaygroundVariantConfig from "../PlaygroundVariantConfig" +import ProviderKeyNotice from "../ProviderKeyNotice" import type {BaseContainerProps} from "../types" const PlaygroundFocusDrawer = dynamic(() => import("../PlaygroundFocusDrawerAdapter"), { ssr: false, @@ -437,7 +439,11 @@ const PlaygroundMainView = ({ {!isComparisonView && isAgentConfig && primaryConfigId ? ( - + <> + + + + ) : null}
diff --git a/web/oss/src/hooks/useAlwaysAllowTool.tsx b/web/oss/src/hooks/useAlwaysAllowTool.tsx new file mode 100644 index 0000000000..c645d82c9b --- /dev/null +++ b/web/oss/src/hooks/useAlwaysAllowTool.tsx @@ -0,0 +1,128 @@ +import {useCallback, useMemo, useRef} from "react" + +import {workflowMolecule} from "@agenta/entities/workflow" +import { + findGrantableHarnessTool, + findGrantableTool, + gateRulePattern, + withHarnessToolAllow, + withToolPermission, +} from "@agenta/entity-ui/drill-in" +import {draftConfigChangeSignalAtom} from "@agenta/shared/state" +import {useAtomValue, useSetAtom} from "jotai" + +import {resolveToolDisplay} from "@/oss/components/AgentChatSlice/assets/toolDisplay" + +export interface ToolGrantInfo { + /** The gate maps to a per-tool-config tool (gateway or custom function) whose permission we can set. */ + eligible: boolean + /** The matched tool is already `allow` — the affordance would be a no-op, so hide it. */ + alreadyAllowed: boolean +} + +const INELIGIBLE: ToolGrantInfo = {eligible: false, alreadyAllowed: false} + +/** + * "Always allow this tool" for the approval card. + * + * Config write-through into the draft agent config; `buildAgentRequest` reads the draft, so a grant + * takes effect on the paused run's resume and every future run, and a commit carries it to triggers. + * Two fields, routed by tool class: + * - a gateway / custom-function tool has a `tools[]` entry → per-tool `permission: "allow"` + * (`specPermission`, the highest-precedence gate). Checked FIRST — it outranks any rule, and a + * verbatim rule pattern would otherwise also match its slug. + * - any other harness tool (`bash`, `Terminal`, `Write`, …) has no enforceable per-tool permission + * → an allow-rule in `harness.permissions.allow`, keyed by the gate name VERBATIM: the runner + * matches `pattern === gate.toolName`, and that string is exactly what the card shows (the + * runner stamps it as `resolvedName`, which the egress prefers). Never canonicalize it. + * Platform ops (`commit_revision`, schedules), client tools, and MCP tools return `eligible: false` + * and always stay gated (see `gateRulePattern`). + * + * On grant we raise a single draft-change signal that the config pane consumes two ways: the section + * it landed in pulses for attention, and a contained banner (`AlwaysAllowedNotice`) offers Undo — + * both kept inside the config panel where the change is, rather than a floating toast. `revoke` is + * the exact inverse (`"ask"` for tools, `allowed:false` for harness rules). + */ +export function useAlwaysAllowTool(entityId?: string) { + const config = useAtomValue( + useMemo(() => workflowMolecule.selectors.configuration(entityId ?? ""), [entityId]), + ) + // Latest config for the deferred Undo click, so it never reverts against a stale snapshot. + const configRef = useRef(config) + configRef.current = config + const setConfiguration = useSetAtom(workflowMolecule.actions.updateConfiguration) + // Marks the config section this grant lands in so it can pulse for attention — the user + // acted here in the dock, but the write shows up over in the (maybe off-screen) config pane. + const raiseDraftSignal = useSetAtom(draftConfigChangeSignalAtom) + + const infoFor = useCallback( + (toolName: string): ToolGrantInfo => { + if (!entityId) return INELIGIBLE + // Gateway / custom-function tools carry a per-tool `permission` in `tools[]`. First: + // it outranks a rule, and a verbatim rule pattern would also match its slug. + const tool = findGrantableTool(config, toolName) + if (tool) return {eligible: true, alreadyAllowed: tool.permission === "allow"} + // Any other harness tool (bash, Terminal, Write, …) → `harness.permissions.allow`. + const harnessTool = findGrantableHarnessTool(config, toolName) + if (harnessTool) return {eligible: true, alreadyAllowed: harnessTool.allowed} + // Platform ops (commit_revision, schedules), client tools, MCP → never grantable. + return INELIGIBLE + }, + [entityId, config], + ) + + // Inverse of grant: put the tool back to gated. Reads the LATEST config (ref), since Undo fires + // seconds after the grant and the draft may have moved on. + const revoke = useCallback( + (toolName: string): boolean => { + if (!entityId) return false + const cfg = configRef.current + // Same routing as `grant` — `tools[]` first, then the harness allow-rule. + const tool = findGrantableTool(cfg, toolName) + const pattern = tool ? null : gateRulePattern(toolName) + const next = tool + ? withToolPermission(cfg, toolName, "ask") + : pattern + ? withHarnessToolAllow(cfg, pattern, false) + : null + if (!next) return false + setConfiguration(entityId, next) + return true + }, + [entityId, setConfiguration], + ) + + const grant = useCallback( + (toolName: string): boolean => { + if (!entityId) return false + // Route to the field that matches the gate's tool class (see infoFor). `tools[]` first: + // its per-tool permission outranks a rule, and a verbatim pattern would match its slug. + const tool = findGrantableTool(config, toolName) + const pattern = tool ? null : gateRulePattern(toolName) + const next = tool + ? withToolPermission(config, toolName, "allow") + : pattern + ? withHarnessToolAllow(config, pattern, true) + : null + if (!next) return false + setConfiguration(entityId, next) + // A harness allow-rule writes `harness.permissions`, which surfaces in the Advanced → + // Permissions group (and classifies as an "advanced" draft change); gateway/custom-function + // tools write `tools[]`, surfaced in the Tools section. Pulse the section the change lands in. + raiseDraftSignal({ + revisionId: entityId, + sectionKeys: [tool ? "tools" : "advanced"], + origin: "approval-dock", + summary: `Always allow ${toolName}`, + // Friendly display (matches the approval card) — a gateway tool's raw name is a slug. + label: resolveToolDisplay(toolName).label, + toolName, + at: Date.now(), + }) + return true + }, + [entityId, config, setConfiguration, raiseDraftSignal], + ) + + return {infoFor, grant, revoke} +} diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/index.ts b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/index.ts index fabb730ebb..9e2b344826 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/index.ts +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/index.ts @@ -69,6 +69,15 @@ export {ToolSelectorPopover} from "./ToolSelectorPopover" export type {ToolSelectorPopoverProps} from "./ToolSelectorPopover" export {TOOL_PROVIDERS_META, TOOL_SPECS} from "./toolUtils" export type {ToolObj, ToolFunction} from "./toolUtils" +export { + findGrantableTool, + withToolPermission, + gateRulePattern, + readHarnessAllowList, + findGrantableHarnessTool, + withHarnessToolAllow, +} from "./toolPermission" +export type {GrantableTool, ToolPermission, GrantableHarnessTool} from "./toolPermission" export {McpServerItemControl} from "./McpServerItemControl" export type {McpServerItemControlProps} from "./McpServerItemControl" diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/toolPermission.ts b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/toolPermission.ts new file mode 100644 index 0000000000..2d0fac044c --- /dev/null +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/toolPermission.ts @@ -0,0 +1,230 @@ +/** + * toolPermission — locate a runtime tool gate's config entry and set its per-tool permission. + * + * The approval card's "always allow this tool" writes a per-tool `permission` onto the matching + * entry in the agent template's `tools[]` (config write-through: the run reads the draft config, so + * the change takes effect on the in-flight resume and every future run; a commit carries it to + * triggers). Only tools that live in the committed `tools[]` AND carry a per-tool `permission` slot + * are grantable: gateway (Composio) tools and custom function tools. Platform ops (overlay-injected, + * e.g. `commit_revision`), builtins, MCP servers (per-server permission, not per-tool), and workflow + * references are deliberately NOT matched here, so they can never be auto-allowed from the card — + * `commit_revision` and destructive ops stay gated by construction. + * + * The matcher mirrors the runtime wire name: a gateway gate arrives as the slug + * `tools__{provider}__{integration}__{action}__{connection}` (see `parseGatewayToolName`), and a + * custom function tool arrives as its bare `function.name`. + */ +import {parseGatewayToolSlug} from "@agenta/shared/utils" + +import {parseGatewayTool} from "./toolUtils" + +export type ToolPermission = "allow" | "ask" | "deny" + +const isRecord = (v: unknown): v is Record => + Boolean(v && typeof v === "object" && !Array.isArray(v)) + +const asPermission = (v: unknown): ToolPermission | undefined => + v === "allow" || v === "ask" || v === "deny" ? v : undefined + +interface TemplateLocation { + template: Record + /** Rebuild the full `parameters` object from an updated template. */ + wrap: (nextTemplate: Record) => Record +} + +/** + * The agent template lives at `parameters.agent` (the playground shape) or IS the parameters (a bare + * template). Mirror `buildAgentRequest`'s `withAgentRunDefaults` so a write lands exactly where the + * run reads from. + */ +function locateTemplate(parameters: Record): TemplateLocation { + if (isRecord(parameters.agent)) { + const agent = parameters.agent + return {template: agent, wrap: (next) => ({...parameters, agent: next})} + } + return {template: parameters, wrap: (next) => next} +} + +/** + * Index of the `tools[]` entry a runtime gate `toolName` refers to, or -1. A gateway tool (canonical + * `{type:"gateway"}` or a legacy function-name slug) matches by its {provider, integration, action, + * connection} identity; a custom function tool matches by `function.name`. + */ +function matchToolIndex(tools: unknown[], toolName: string): number { + const slug = parseGatewayToolSlug(toolName) + for (let i = 0; i < tools.length; i++) { + const entry = tools[i] + if (!isRecord(entry)) continue + if (slug) { + const g = parseGatewayTool(entry) + if ( + g && + g.provider === slug.provider && + g.integration === slug.integration && + g.action === slug.action && + g.connection === slug.connection + ) { + return i + } + continue + } + const fn = isRecord(entry.function) ? entry.function : null + if (fn && typeof fn.name === "string" && fn.name === toolName) return i + } + return -1 +} + +export interface GrantableTool { + /** The current per-tool permission on the matched entry, if one is set. */ + permission?: ToolPermission +} + +// --------------------------------------------------------------------------- +// Harness tools (bash / Terminal / Write / …): the `harness.permissions.allow` path +// --------------------------------------------------------------------------- +// +// A harness tool carries NO per-tool `permission` (a builtin's own permission is dropped as +// unenforceable). The only lever is an authored allow-rule: `harness.permissions.allow += []` flows through `wire_author_permission_rules` into a runner rule +// `{pattern, permission:"allow"}`, while `runner.permissions.default` stays as authored so platform +// ops (commit_revision, schedules) keep gating. Works on Pi AND Claude — `_parse_harness_slice` +// reads `harness.permissions` for any harness. +// +// THE PATTERN IS THE GATE NAME, VERBATIM. The runner matches rules with `pattern === gate.toolName` +// (permission-plan.ts `ruleMatches`), and `gate.toolName` is exactly what the approval card shows: +// the runner stamps it onto the gate as `resolvedName` (acp-interactions.ts) and the SDK's +// `_approval_tool_name` prefers that field for the part. Do NOT "canonicalize" the name — an ACP +// gate reports `bash`/`Terminal` verbatim (buildGateDescriptor: `spec?.name ?? displayName`), so a +// rule for `Bash` would silently never match. + +/** Platform ops (overlay-injected). These must ALWAYS gate — never auto-allowable from the card. */ +const PLATFORM_OPS = new Set([ + "discover_tools", + "query_workflows", + "query_spans", + "test_run", + "commit_revision", + "annotate_trace", + "discover_triggers", + "create_schedule", + "create_subscription", + "list_schedules", + "list_subscriptions", + "list_deliveries", + "list_connections", + "test_subscription", + "remove_schedule", + "remove_subscription", + "pause_schedule", + "resume_schedule", + "pause_subscription", + "resume_subscription", +]) + +/** Browser-fulfilled client tools — they carry their own widget/decline UI; never auto-allowable. */ +const CLIENT_TOOLS = new Set(["request_connection", "request_input"]) + +/** + * The runner rule pattern for a gate, or `null` when the gate must never be auto-allowed: + * a platform op, a client tool, or an MCP tool — `wire_author_permission_rules` DROPS `mcp__` + * patterns from the runner plan, so such a rule would silently never take effect (MCP is governed + * per-server instead). + */ +export function gateRulePattern(toolName: string): string | null { + if (!toolName) return null + if (PLATFORM_OPS.has(toolName) || CLIENT_TOOLS.has(toolName)) return null + if (toolName.startsWith("mcp__")) return null + return toolName +} + +/** The authored `harness.permissions.allow` patterns. */ +export function readHarnessAllowList(parameters: unknown): string[] { + if (!isRecord(parameters)) return [] + const {template} = locateTemplate(parameters) + const harness = isRecord(template.harness) ? template.harness : {} + const permissions = isRecord(harness.permissions) ? harness.permissions : {} + return Array.isArray(permissions.allow) + ? (permissions.allow.filter((v) => typeof v === "string") as string[]) + : [] +} + +export interface GrantableHarnessTool { + /** The runner rule pattern — the gate name, verbatim. */ + pattern: string + /** Already present in `harness.permissions.allow`. */ + allowed: boolean +} + +/** + * Classify a gate as an allow-rule-able harness tool and report whether it's already allowed, or + * `null` when it must stay gated. Callers must check `findGrantableTool` FIRST: a gateway/custom + * tool has a `tools[]` entry whose per-tool `permission` outranks any rule. + */ +export function findGrantableHarnessTool( + parameters: unknown, + toolName: string, +): GrantableHarnessTool | null { + const pattern = gateRulePattern(toolName) + if (!pattern || !isRecord(parameters)) return null + return {pattern, allowed: readHarnessAllowList(parameters).includes(pattern)} +} + +/** Return a new `parameters` with `pattern` present (or absent, when `allowed` is false) in + * `harness.permissions.allow`. Preserves the rest of the harness/permissions object. */ +export function withHarnessToolAllow( + parameters: unknown, + pattern: string, + allowed: boolean, +): Record | null { + if (!isRecord(parameters) || !pattern) return null + const {template, wrap} = locateTemplate(parameters) + const harness = isRecord(template.harness) ? {...template.harness} : {} + const permissions = isRecord(harness.permissions) ? {...harness.permissions} : {} + const current = Array.isArray(permissions.allow) + ? (permissions.allow.filter((v) => typeof v === "string") as string[]) + : [] + const has = current.includes(pattern) + if (allowed === has) return wrap({...template, harness: {...harness, permissions}}) + const nextAllow = allowed ? [...current, pattern] : current.filter((name) => name !== pattern) + return wrap({ + ...template, + harness: {...harness, permissions: {...permissions, allow: nextAllow}}, + }) +} + +/** + * Find the grantable `tools[]` entry for a gate, or `null` when the gate is not a per-tool-config + * tool (platform op, builtin, MCP, reference, or an unknown name). A `null` result is the signal to + * hide the "always allow" affordance. + */ +export function findGrantableTool(parameters: unknown, toolName: string): GrantableTool | null { + if (!isRecord(parameters) || !toolName) return null + const {template} = locateTemplate(parameters) + const tools = Array.isArray(template.tools) ? (template.tools as unknown[]) : [] + const i = matchToolIndex(tools, toolName) + if (i < 0) return null + return {permission: asPermission((tools[i] as Record).permission)} +} + +/** + * Return a new `parameters` with the gate's tool entry set to `permission` (or its `permission` key + * removed when `undefined` = inherit). Returns `null` when the gate is not grantable, so the caller + * leaves the config untouched. + */ +export function withToolPermission( + parameters: unknown, + toolName: string, + permission: ToolPermission | undefined, +): Record | null { + if (!isRecord(parameters) || !toolName) return null + const {template, wrap} = locateTemplate(parameters) + const tools = Array.isArray(template.tools) ? (template.tools as unknown[]) : [] + const i = matchToolIndex(tools, toolName) + if (i < 0) return null + const entry = isRecord(tools[i]) ? {...(tools[i] as Record)} : {} + if (permission === undefined) delete entry.permission + else entry.permission = permission + const nextTools = tools.slice() + nextTools[i] = entry + return wrap({...template, tools: nextTools}) +} diff --git a/web/packages/agenta-entity-ui/src/DrillInView/index.ts b/web/packages/agenta-entity-ui/src/DrillInView/index.ts index f489604117..4a2aba8cba 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/index.ts +++ b/web/packages/agenta-entity-ui/src/DrillInView/index.ts @@ -267,6 +267,12 @@ export { normalizeMessages, denormalizeMessages, getOptionsFromSchema, + findGrantableTool, + withToolPermission, + gateRulePattern, + readHarnessAllowList, + findGrantableHarnessTool, + withHarnessToolAllow, type OptionGroup, } from "./SchemaControls" @@ -291,6 +297,9 @@ export type { ObjectSchemaControlProps, SchemaPropertyRendererProps, FieldsDetectionContextValue, + GrantableTool, + ToolPermission, + GrantableHarnessTool, } from "./SchemaControls" // Operational panel regions (Triggers, Mounts) — siblings of the Configuration section. diff --git a/web/packages/agenta-entity-ui/tests/unit/toolPermission.test.ts b/web/packages/agenta-entity-ui/tests/unit/toolPermission.test.ts new file mode 100644 index 0000000000..6c61813d88 --- /dev/null +++ b/web/packages/agenta-entity-ui/tests/unit/toolPermission.test.ts @@ -0,0 +1,224 @@ +/** + * Unit tests for the approval-card "always allow this tool" config write-through. + * + * `findGrantableTool` / `withToolPermission` map a runtime gate's wire `toolName` back to its entry + * in the agent template's `tools[]` and set a per-tool `permission`. Only gateway (canonical or + * legacy slug) and custom function tools are grantable; platform ops, builtins, MCP, and references + * must be left ungrantable so `commit_revision` and destructive ops stay gated. Runs under + * @agenta/entity-ui's own vitest runner. + */ +import {describe, expect, it} from "vitest" + +import { + findGrantableHarnessTool, + findGrantableTool, + gateRulePattern, + readHarnessAllowList, + withHarnessToolAllow, + withToolPermission, +} from "../../src/DrillInView/SchemaControls/toolPermission" + +const GATEWAY_SLUG = "tools__composio__gmail__GMAIL_SEND_EMAIL__conn1" + +const canonicalGateway = (extra: Record = {}) => ({ + type: "gateway", + provider: "composio", + integration: "gmail", + action: "GMAIL_SEND_EMAIL", + connection: "conn1", + ...extra, +}) + +const legacyGateway = (extra: Record = {}) => ({ + type: "function", + function: {name: GATEWAY_SLUG}, + ...extra, +}) + +const customFn = (name: string, extra: Record = {}) => ({ + type: "function", + function: {name, parameters: {type: "object", properties: {}}}, + ...extra, +}) + +const wrap = (tools: unknown[]) => ({ + agent: {tools, runner: {permissions: {default: "allow_reads"}}}, +}) + +describe("findGrantableTool", () => { + it("matches a canonical gateway entry by its {provider,integration,action,connection} identity", () => { + const params = wrap([canonicalGateway()]) + expect(findGrantableTool(params, GATEWAY_SLUG)).toEqual({permission: undefined}) + }) + + it("matches a legacy gateway function-name slug", () => { + const params = wrap([legacyGateway()]) + expect(findGrantableTool(params, GATEWAY_SLUG)).not.toBeNull() + }) + + it("matches a custom function tool by function.name", () => { + const params = wrap([customFn("get_weather")]) + expect(findGrantableTool(params, "get_weather")).toEqual({permission: undefined}) + }) + + it("reports the current permission when one is set", () => { + const params = wrap([canonicalGateway({permission: "allow"})]) + expect(findGrantableTool(params, GATEWAY_SLUG)).toEqual({permission: "allow"}) + }) + + it("does not match a platform op, builtin, reference, or unknown gate", () => { + const params = wrap([ + {type: "platform", op: "commit_revision"}, + {type: "builtin", name: "read"}, + {type: "reference", name: "some_workflow"}, + ]) + expect(findGrantableTool(params, "commit_revision")).toBeNull() + expect(findGrantableTool(params, "read")).toBeNull() + expect(findGrantableTool(params, "mcp__linear__create_issue")).toBeNull() + }) + + it("returns null for a missing config / empty tools", () => { + expect(findGrantableTool(null, GATEWAY_SLUG)).toBeNull() + expect(findGrantableTool(wrap([]), GATEWAY_SLUG)).toBeNull() + }) +}) + +describe("withToolPermission", () => { + it("sets permission on the matched entry and leaves the others untouched", () => { + const other = customFn("keep_me") + const params = wrap([canonicalGateway(), other]) + const next = withToolPermission(params, GATEWAY_SLUG, "allow") as { + agent: {tools: Record[]} + } + expect(next.agent.tools[0].permission).toBe("allow") + expect(next.agent.tools[1]).toEqual(other) + }) + + it("removes the permission key when inheriting (undefined)", () => { + const params = wrap([canonicalGateway({permission: "allow"})]) + const next = withToolPermission(params, GATEWAY_SLUG, undefined) as { + agent: {tools: Record[]} + } + expect("permission" in next.agent.tools[0]).toBe(false) + }) + + it("returns null (no write) for an ungrantable gate", () => { + const params = wrap([{type: "platform", op: "commit_revision"}]) + expect(withToolPermission(params, "commit_revision", "allow")).toBeNull() + }) + + it("does not mutate the input parameters", () => { + const params = wrap([canonicalGateway()]) + const snapshot = JSON.stringify(params) + withToolPermission(params, GATEWAY_SLUG, "allow") + expect(JSON.stringify(params)).toBe(snapshot) + }) + + it("supports a bare template (no agent wrapper)", () => { + const bare = {tools: [canonicalGateway()]} + const next = withToolPermission(bare, GATEWAY_SLUG, "allow") as { + tools: Record[] + } + expect(next.tools[0].permission).toBe("allow") + }) +}) + +const wrapHarness = (permissions?: Record) => ({ + agent: { + tools: [], + runner: {permissions: {default: "allow_reads"}}, + harness: {kind: "pi_agenta", ...(permissions ? {permissions} : {})}, + }, +}) + +describe("gateRulePattern", () => { + // The runner matches `pattern === gate.toolName`, and the card shows that exact string + // (stamped as `resolvedName`). Canonicalizing would silently never match — an ACP gate + // reports `bash`/`Terminal` verbatim, so a rule for `Bash` would be a no-op. + it("returns the gate name VERBATIM — never canonicalized", () => { + expect(gateRulePattern("bash")).toBe("bash") + expect(gateRulePattern("Terminal")).toBe("Terminal") + expect(gateRulePattern("Bash")).toBe("Bash") + expect(gateRulePattern("Write")).toBe("Write") + }) + + it("refuses platform ops so commit/destructive ops always gate", () => { + expect(gateRulePattern("commit_revision")).toBeNull() + expect(gateRulePattern("create_schedule")).toBeNull() + expect(gateRulePattern("remove_subscription")).toBeNull() + expect(gateRulePattern("test_run")).toBeNull() + }) + + it("refuses client tools and MCP tools (mcp__ rules are dropped from the runner plan)", () => { + expect(gateRulePattern("request_connection")).toBeNull() + expect(gateRulePattern("request_input")).toBeNull() + expect(gateRulePattern("mcp__linear__create_issue")).toBeNull() + expect(gateRulePattern("")).toBeNull() + }) +}) + +describe("findGrantableHarnessTool", () => { + it("classifies an arbitrary harness gate verbatim and reports not-yet-allowed", () => { + expect(findGrantableHarnessTool(wrapHarness(), "Terminal")).toEqual({ + pattern: "Terminal", + allowed: false, + }) + expect(findGrantableHarnessTool(wrapHarness(), "bash")).toEqual({ + pattern: "bash", + allowed: false, + }) + }) + + it("reports allowed when the pattern is in harness.permissions.allow", () => { + const params = wrapHarness({allow: ["Terminal"]}) + expect(findGrantableHarnessTool(params, "Terminal")).toEqual({ + pattern: "Terminal", + allowed: true, + }) + expect(readHarnessAllowList(params)).toEqual(["Terminal"]) + }) + + it("returns null for a platform op", () => { + expect(findGrantableHarnessTool(wrapHarness(), "commit_revision")).toBeNull() + }) +}) + +describe("withHarnessToolAllow", () => { + it("adds the pattern to harness.permissions.allow (creating the slice)", () => { + const next = withHarnessToolAllow(wrapHarness(), "Terminal", true) as { + agent: {harness: {permissions: {allow: string[]}}} + } + expect(next.agent.harness.permissions.allow).toEqual(["Terminal"]) + }) + + it("is idempotent — no duplicate when already present", () => { + const next = withHarnessToolAllow(wrapHarness({allow: ["bash"]}), "bash", true) as { + agent: {harness: {permissions: {allow: string[]}}} + } + expect(next.agent.harness.permissions.allow).toEqual(["bash"]) + }) + + it("removes the pattern when allowed is false, preserving other entries", () => { + const next = withHarnessToolAllow( + wrapHarness({allow: ["bash", "Terminal"]}), + "bash", + false, + ) as {agent: {harness: {permissions: {allow: string[]}}}} + expect(next.agent.harness.permissions.allow).toEqual(["Terminal"]) + }) + + it("preserves other permission keys (e.g. default_mode)", () => { + const next = withHarnessToolAllow(wrapHarness({default_mode: "default"}), "Bash", true) as { + agent: {harness: {permissions: Record}} + } + expect(next.agent.harness.permissions.default_mode).toBe("default") + expect(next.agent.harness.permissions.allow).toEqual(["Bash"]) + }) + + it("does not mutate the input parameters", () => { + const params = wrapHarness({allow: ["Terminal"]}) + const snapshot = JSON.stringify(params) + withHarnessToolAllow(params, "bash", true) + expect(JSON.stringify(params)).toBe(snapshot) + }) +}) From 9c4ffd1831fd3f20cd6ecbf699738a037874f225 Mon Sep 17 00:00:00 2001 From: Arda Erzin Date: Fri, 17 Jul 2026 23:10:12 +0200 Subject: [PATCH 07/11] fix(ui): use size prop for TriggerDeliveriesDrawer width --- .../src/gatewayTrigger/drawers/TriggerDeliveriesDrawer.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/TriggerDeliveriesDrawer.tsx b/web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/TriggerDeliveriesDrawer.tsx index 259f011ed4..6be9ffd170 100644 --- a/web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/TriggerDeliveriesDrawer.tsx +++ b/web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/TriggerDeliveriesDrawer.tsx @@ -257,7 +257,7 @@ export default function TriggerDeliveriesDrawer() { setState(null) }} title={`Deliveries${state?.name ? ` · ${state.name}` : ""}`} - width={820} + size={820} destroyOnClose styles={{ body: {padding: 0, display: "flex", flexDirection: "column", overflow: "hidden"}, From 4c0af833b5756067d7e6cee3a99613737c660143 Mon Sep 17 00:00:00 2001 From: Arda Erzin Date: Sat, 18 Jul 2026 01:45:31 +0200 Subject: [PATCH 08/11] fix(runner): don't corrupt a parked+resumed turn's persisted transcript A paused turn persisted a terminal `done` and the resume run re-persisted the inbound user message. On reload that split the parked gate from the resume that settles it (gate stuck at 'Awaiting approval') and duplicated the user turn. Tag the paused turn's `done` with stopReason so replay can tell it isn't a real boundary, and skip re-persisting the user turn on a resume run (only a fresh user message is recorded, via tailIsFreshUserMessage). --- .../src/engines/sandbox_agent/run-turn.ts | 4 +- services/runner/src/server.ts | 12 +- services/runner/src/tracing/otel.ts | 17 +- .../runner/tests/unit/stream-events.test.ts | 356 +++++++++++++++--- 4 files changed, 324 insertions(+), 65 deletions(-) diff --git a/services/runner/src/engines/sandbox_agent/run-turn.ts b/services/runner/src/engines/sandbox_agent/run-turn.ts index 09c276ab11..f97109bd1b 100644 --- a/services/runner/src/engines/sandbox_agent/run-turn.ts +++ b/services/runner/src/engines/sandbox_agent/run-turn.ts @@ -567,7 +567,9 @@ export async function runTurn( run.emitEvent({ type: "error", message: swallowedError }); } - const output = run.finish(); + // Pass the stop reason so a paused turn's `done` is tagged — the replay must not treat it as a + // turn boundary (the turn continues on the resume run). + const output = run.finish({ stopReason }); await run.flush(); if (swallowedError) { diff --git a/services/runner/src/server.ts b/services/runner/src/server.ts index e64a339c93..f681cba87f 100644 --- a/services/runner/src/server.ts +++ b/services/runner/src/server.ts @@ -955,11 +955,15 @@ async function runAndStreamWithApiBaseResolved( liveEmit, seedForRun(request), ); - // Record the inbound user turn first so the session record is the full conversation, - // not just agent output. Interaction replies ride tool_result blocks (no text) and are - // already recorded on the interaction, so an empty prompt persists nothing. + // Record the inbound user turn first so the session record is the full conversation, not just + // agent output. Only for a FRESH user turn: an approval resume re-sends the SAME prompt text + // (the request still carries it), so persisting unconditionally would append a duplicate user + // message on every resume — which then splits the parked turn from its resume on reload. An + // approval reply rides tool_result blocks (no fresh text) and is already recorded on the + // interaction, so `tailIsFreshUserMessage` is false for it. const promptText = resolvePromptText(request); - if (promptText) persist({ type: "message", text: promptText }, "user"); + if (promptText && tailIsFreshUserMessage(request)) + persist({ type: "message", text: promptText }, "user"); emitFn = persistingEmit; flushPersist = flush; persistError = (message) => persist({ type: "error", message }, "agent"); diff --git a/services/runner/src/tracing/otel.ts b/services/runner/src/tracing/otel.ts index 1d47cf10f4..051da69d63 100644 --- a/services/runner/src/tracing/otel.ts +++ b/services/runner/src/tracing/otel.ts @@ -1068,8 +1068,12 @@ export interface SandboxAgentOtel { * lands in both the live sink and the batch `events()` log in build order. */ emitEvent(event: AgentEvent): void; - /** End all open spans. Returns the accumulated assistant text. */ - finish(): string; + /** + * End all open spans. Returns the accumulated assistant text. `stopReason` is stamped on the + * terminal `done` event: a `"paused"` turn is NOT a real turn boundary (it continues on the + * resume run), so a replay must not close the message there — see the FE `transcriptToMessages`. + */ + finish(opts?: { stopReason?: string }): string; /** * Record a run-level error on the agent span: the user-facing message (F-030) plus the * provider that failed, and an OTel exception event, so a trace carries the same diagnostic @@ -1562,7 +1566,7 @@ export function createSandboxAgentOtel( } } - function finish(): string { + function finish(opts?: { stopReason?: string }): string { const text = stripStartupBanner(accumulated.trim()); // The event log is independent of span emission, so build its tail either way. closeText(); @@ -1586,7 +1590,12 @@ export function createSandboxAgentOtel( } // Stamp the run's trace id on the turn's terminal event so a persisted transcript can link a // replayed turn back to its trace (undefined only in span-less mode with no valid traceparent). - record({ type: "done", ...(runTraceId ? { traceId: runTraceId } : {}) }); + // `stopReason` rides too: a paused turn's `done` must not read as a turn boundary on replay. + record({ + type: "done", + ...(opts?.stopReason ? { stopReason: opts.stopReason } : {}), + ...(runTraceId ? { traceId: runTraceId } : {}), + }); if (!emitSpans) return text; if (llmSpan) { emitMessages( diff --git a/services/runner/tests/unit/stream-events.test.ts b/services/runner/tests/unit/stream-events.test.ts index bc03101709..00cc174920 100644 --- a/services/runner/tests/unit/stream-events.test.ts +++ b/services/runner/tests/unit/stream-events.test.ts @@ -37,7 +37,11 @@ const toolDone = (id: string, text: string) => ({ status: "completed", content: [{ content: { type: "text", text } }], }); -const usage = () => ({ sessionUpdate: "usage_update", used: 100, cost: { amount: 0.01 } }); +const usage = () => ({ + sessionUpdate: "usage_update", + used: 100, + cost: { amount: 0.01 }, +}); // The same ACP sequence drives both modes: two text runs around a tool call, then reasoning. function drive(run: ReturnType): void { @@ -58,13 +62,25 @@ const ofType = (events: AgentEvent[], t: T) => describe("createSandboxAgentOtel state machine", () => { it("scenario 1: streaming (emit set) yields pure deltas and balanced lifecycle", () => { const emitted: AgentEvent[] = []; - const run = createSandboxAgentOtel({ harness: "claude", model: "anthropic/x", emit: (e) => emitted.push(e) }); + const run = createSandboxAgentOtel({ + harness: "claude", + model: "anthropic/x", + emit: (e) => emitted.push(e), + }); drive(run); const finalText = run.finish(); // No coalesced text events on the streaming path. - assert.equal(ofType(emitted, "message").length, 0, "no coalesced message when streaming"); - assert.equal(ofType(emitted, "thought").length, 0, "no coalesced thought when streaming"); + assert.equal( + ofType(emitted, "message").length, + 0, + "no coalesced message when streaming", + ); + assert.equal( + ofType(emitted, "thought").length, + 0, + "no coalesced thought when streaming", + ); // Exactly one terminal done. assert.equal(ofType(emitted, "done").length, 1, "exactly one done"); @@ -74,35 +90,92 @@ describe("createSandboxAgentOtel state machine", () => { const mEnd = ofType(emitted, "message_end"); assert.equal(mStart.length, 2, "two message_start"); assert.equal(mEnd.length, 2, "two message_end"); - assert.deepEqual(mStart.map((e) => e.id), ["msg-0", "msg-1"], "stable monotonic text ids"); + assert.deepEqual( + mStart.map((e) => e.id), + ["msg-0", "msg-1"], + "stable monotonic text ids", + ); const rStart = ofType(emitted, "thought_start"); const rEnd = ofType(emitted, "thought_end"); assert.equal(rStart.length, 1, "one thought_start"); assert.equal(rEnd.length, 1, "one thought_end"); // Deltas are pure and reconstruct the full text, with no overlap/repeat. - const text = ofType(emitted, "message_delta").map((e) => e.delta).join(""); - assert.equal(text, "Hello world It is sunny.", "concatenated deltas == full text"); + const text = ofType(emitted, "message_delta") + .map((e) => e.delta) + .join(""); + assert.equal( + text, + "Hello world It is sunny.", + "concatenated deltas == full text", + ); assert.equal(text, finalText, "deltas match finish() output"); - const reasoning = ofType(emitted, "thought_delta").map((e) => e.delta).join(""); + const reasoning = ofType(emitted, "thought_delta") + .map((e) => e.delta) + .join(""); assert.equal(reasoning, "thinking...", "concatenated thought deltas"); // Ordering invariant: each block's start precedes its deltas precede its end; tool result // lands before the second text block opens. const seq = types(emitted); - assert.ok(seq.indexOf("message_end") < seq.indexOf("tool_call"), "first text block closes before the tool call"); - assert.ok(seq.indexOf("tool_result") < seq.lastIndexOf("message_start"), "tool result precedes the second text block"); + assert.ok( + seq.indexOf("message_end") < seq.indexOf("tool_call"), + "first text block closes before the tool call", + ); + assert.ok( + seq.indexOf("tool_result") < seq.lastIndexOf("message_start"), + "tool result precedes the second text block", + ); for (const id of ["msg-0", "msg-1", "reason-2"]) { const idxs = emitted .map((e, i) => ((e as any).id === id ? { i, t: e.type } : null)) .filter(Boolean) as { i: number; t: string }[]; assert.ok(idxs[0].t.endsWith("_start"), `${id} starts with *_start`); - assert.ok(idxs[idxs.length - 1].t.endsWith("_end"), `${id} ends with *_end`); + assert.ok( + idxs[idxs.length - 1].t.endsWith("_end"), + `${id} ends with *_end`, + ); } }); + it("stamps stopReason on the terminal done so a paused turn is distinguishable on replay", () => { + // A paused turn's `done` must carry stopReason=paused: the FE replay uses it to NOT treat the + // paused turn as a boundary (which would strand a parked gate from its resume). Default finish() + // (a completed turn) leaves it unset. + const paused: AgentEvent[] = []; + createSandboxAgentOtel({ + harness: "claude", + model: "anthropic/x", + emit: (e) => paused.push(e), + }).finish({ + stopReason: "paused", + }); + const pausedDone = ofType(paused, "done"); + assert.equal(pausedDone.length, 1, "one done"); + assert.equal( + (pausedDone[0] as any).stopReason, + "paused", + "paused done carries stopReason", + ); + + const completed: AgentEvent[] = []; + createSandboxAgentOtel({ + harness: "claude", + model: "anthropic/x", + emit: (e) => completed.push(e), + }).finish(); + assert.equal( + (ofType(completed, "done")[0] as any).stopReason, + undefined, + "a completed turn's done has no stopReason", + ); + }); + it("scenario 2: one-shot (no emit) coalesces text/thought and keeps structured events", () => { - const run = createSandboxAgentOtel({ harness: "claude", model: "anthropic/x" }); + const run = createSandboxAgentOtel({ + harness: "claude", + model: "anthropic/x", + }); drive(run); const finalText = run.finish(); const events = run.events(); @@ -110,34 +183,77 @@ describe("createSandboxAgentOtel state machine", () => { // Coalesced text/thought, no delta lifecycle events. const messages = ofType(events, "message"); assert.equal(messages.length, 1, "one coalesced message"); - assert.equal(messages[0].text, "Hello world It is sunny.", "coalesced text == final"); + assert.equal( + messages[0].text, + "Hello world It is sunny.", + "coalesced text == final", + ); assert.equal(messages[0].text, finalText); assert.equal(ofType(events, "thought").length, 1, "one coalesced thought"); - for (const t of ["message_start", "message_delta", "message_end", "thought_start", "thought_delta", "thought_end"]) { - assert.equal(events.filter((e) => e.type === t).length, 0, `no ${t} on the one-shot path`); + for (const t of [ + "message_start", + "message_delta", + "message_end", + "thought_start", + "thought_delta", + "thought_end", + ]) { + assert.equal( + events.filter((e) => e.type === t).length, + 0, + `no ${t} on the one-shot path`, + ); } // The structured tool/usage events are still present, with exactly one done. const calls = ofType(events, "tool_call"); assert.equal(calls.length, 1, "tool_call present"); - assert.deepEqual(calls[0].input, { city: "Paris" }, "tool_call carries the args from the initial notification"); - assert.equal(ofType(events, "tool_result").length, 1, "tool_result present"); + assert.deepEqual( + calls[0].input, + { city: "Paris" }, + "tool_call carries the args from the initial notification", + ); + assert.equal( + ofType(events, "tool_result").length, + 1, + "tool_result present", + ); assert.equal(ofType(events, "usage").length, 1, "usage present"); assert.equal(ofType(events, "done").length, 1, "exactly one done"); }); it("scenario 3: span-less mode still records ACP events and final usage", () => { - const run = createSandboxAgentOtel({ harness: "pi", model: "openai-codex/x", emitSpans: false }); + const run = createSandboxAgentOtel({ + harness: "pi", + model: "openai-codex/x", + emitSpans: false, + }); drive(run); run.setUsage({ input: 4, output: 6, total: 10, cost: 0.02 }); const finalText = run.finish(); const events = run.events(); assert.equal(finalText, "Hello world It is sunny."); - assert.equal(ofType(events, "message").length, 1, "message present without spans"); - assert.equal(ofType(events, "thought").length, 1, "thought present without spans"); - assert.equal(ofType(events, "tool_call").length, 1, "tool_call present without spans"); - assert.equal(ofType(events, "tool_result").length, 1, "tool_result present without spans"); + assert.equal( + ofType(events, "message").length, + 1, + "message present without spans", + ); + assert.equal( + ofType(events, "thought").length, + 1, + "thought present without spans", + ); + assert.equal( + ofType(events, "tool_call").length, + 1, + "tool_call present without spans", + ); + assert.equal( + ofType(events, "tool_result").length, + 1, + "tool_result present without spans", + ); const usageEvents = ofType(events, "usage"); assert.equal(usageEvents.length, 1, "usage present without spans"); assert.deepEqual( @@ -145,8 +261,15 @@ describe("createSandboxAgentOtel state machine", () => { { type: "usage", input: 4, output: 6, total: 10, cost: 0.02 }, "final usage replaces stream-only usage before done", ); - assert.equal(ofType(events, "done").length, 1, "exactly one done without spans"); - assert.ok(types(events).indexOf("usage") < types(events).indexOf("done"), "usage precedes done"); + assert.equal( + ofType(events, "done").length, + 1, + "exactly one done without spans", + ); + assert.ok( + types(events).indexOf("usage") < types(events).indexOf("done"), + "usage precedes done", + ); }); it("scenario 4: surfaces the tool_call up front, then refreshes its input from a later tool_call_update", () => { @@ -155,38 +278,90 @@ describe("createSandboxAgentOtel state machine", () => { // tool part + HITL approval attach to it), and then a second tool_call REFRESHES the input // once the real args arrive — so a non-gated tool shows its args instead of `{}`. const emitted: AgentEvent[] = []; - const run = createSandboxAgentOtel({ harness: "pi", model: "openai-codex/x", emit: (e) => emitted.push(e), emitSpans: false }); + const run = createSandboxAgentOtel({ + harness: "pi", + model: "openai-codex/x", + emit: (e) => emitted.push(e), + emitSpans: false, + }); run.start({ prompt: "list connections" }); - run.handleUpdate({ sessionUpdate: "tool_call", toolCallId: "c1", title: "list_connections" }); // no rawInput yet + run.handleUpdate({ + sessionUpdate: "tool_call", + toolCallId: "c1", + title: "list_connections", + }); // no rawInput yet // The call surfaces immediately (emit-first invariant), before any args or result. - assert.equal(ofType(emitted, "tool_call").length, 1, "tool_call emitted up front, on the initial notification"); - run.handleUpdate({ sessionUpdate: "tool_call_update", toolCallId: "c1", rawInput: { limit: 50 } }); // args land here - run.handleUpdate({ sessionUpdate: "tool_call_update", toolCallId: "c1", status: "completed", content: [{ content: { type: "text", text: "ok" } }] }); + assert.equal( + ofType(emitted, "tool_call").length, + 1, + "tool_call emitted up front, on the initial notification", + ); + run.handleUpdate({ + sessionUpdate: "tool_call_update", + toolCallId: "c1", + rawInput: { limit: 50 }, + }); // args land here + run.handleUpdate({ + sessionUpdate: "tool_call_update", + toolCallId: "c1", + status: "completed", + content: [{ content: { type: "text", text: "ok" } }], + }); run.finish(); const calls = ofType(emitted, "tool_call"); assert.equal(calls.length, 2, "one initial surface + one input refresh"); - assert.deepEqual(calls[calls.length - 1].input, { limit: 50 }, "the refresh carries the real args from the tool_call_update"); + assert.deepEqual( + calls[calls.length - 1].input, + { limit: 50 }, + "the refresh carries the real args from the tool_call_update", + ); const seq = types(emitted); - assert.ok(seq.indexOf("tool_call") !== -1 && seq.indexOf("tool_call") < seq.indexOf("tool_result"), "tool_call precedes its result"); + assert.ok( + seq.indexOf("tool_call") !== -1 && + seq.indexOf("tool_call") < seq.indexOf("tool_result"), + "tool_call precedes its result", + ); }); it("scenario 5: an empty initial input is not refreshed when no real args ever arrive", () => { // An `{}` announcement with no later args stays a single surfaced call — no phantom refresh, // still a clean tool_call -> tool_result pair. const emitted: AgentEvent[] = []; - const run = createSandboxAgentOtel({ harness: "pi", model: "openai-codex/x", emit: (e) => emitted.push(e), emitSpans: false }); + const run = createSandboxAgentOtel({ + harness: "pi", + model: "openai-codex/x", + emit: (e) => emitted.push(e), + emitSpans: false, + }); run.start({ prompt: "x" }); - run.handleUpdate({ sessionUpdate: "tool_call", toolCallId: "c1", title: "noArgs", rawInput: {} }); // empty placeholder - run.handleUpdate({ sessionUpdate: "tool_call_update", toolCallId: "c1", status: "completed", content: [{ content: { type: "text", text: "done" } }] }); + run.handleUpdate({ + sessionUpdate: "tool_call", + toolCallId: "c1", + title: "noArgs", + rawInput: {}, + }); // empty placeholder + run.handleUpdate({ + sessionUpdate: "tool_call_update", + toolCallId: "c1", + status: "completed", + content: [{ content: { type: "text", text: "done" } }], + }); run.finish(); const calls = ofType(emitted, "tool_call"); assert.equal(calls.length, 1, "single surfaced call, no refresh"); assert.deepEqual(calls[0].input, {}, "keeps the placeholder input"); - assert.equal(ofType(emitted, "tool_result").length, 1, "tool_result present"); + assert.equal( + ofType(emitted, "tool_result").length, + 1, + "tool_result present", + ); const seq = types(emitted); - assert.ok(seq.indexOf("tool_call") < seq.indexOf("tool_result"), "tool_call precedes its result"); + assert.ok( + seq.indexOf("tool_call") < seq.indexOf("tool_result"), + "tool_call precedes its result", + ); }); it("settleOpenToolCalls excludes paused calls and is idempotent per open sibling", () => { @@ -201,10 +376,7 @@ describe("createSandboxAgentOtel state machine", () => { run.handleUpdate(toolCall("call_1", "needsApproval", { a: 1 })); run.handleUpdate(toolCall("call_2", "sibling", { b: 2 })); - run.settleOpenToolCalls( - (id) => id === "call_1", - TOOL_NOT_EXECUTED_PAUSED, - ); + run.settleOpenToolCalls((id) => id === "call_1", TOOL_NOT_EXECUTED_PAUSED); let results = ofType(emitted, "tool_result"); assert.equal( @@ -240,49 +412,121 @@ describe("createSandboxAgentOtel state machine", () => { // final args — a refresh-once gate that stops at the first partial delta records a lie // (the executor demonstrably ran with the full args). const emitted: AgentEvent[] = []; - const run = createSandboxAgentOtel({ harness: "pi", model: "openai-codex/x", emit: (e) => emitted.push(e), emitSpans: false }); + const run = createSandboxAgentOtel({ + harness: "pi", + model: "openai-codex/x", + emit: (e) => emitted.push(e), + emitSpans: false, + }); run.start({ prompt: "x" }); - run.handleUpdate({ sessionUpdate: "tool_call", toolCallId: "c1", title: "compare", rawInput: {} }); - run.handleUpdate({ sessionUpdate: "tool_call_update", toolCallId: "c1", rawInput: { use_cases: [""] } }); // early partial delta - run.handleUpdate({ sessionUpdate: "tool_call_update", toolCallId: "c1", rawInput: { use_cases: ["a", "b"], limit: 5 } }); // final args - run.handleUpdate({ sessionUpdate: "tool_call_update", toolCallId: "c1", status: "completed", content: [{ content: { type: "text", text: "ok" } }] }); + run.handleUpdate({ + sessionUpdate: "tool_call", + toolCallId: "c1", + title: "compare", + rawInput: {}, + }); + run.handleUpdate({ + sessionUpdate: "tool_call_update", + toolCallId: "c1", + rawInput: { use_cases: [""] }, + }); // early partial delta + run.handleUpdate({ + sessionUpdate: "tool_call_update", + toolCallId: "c1", + rawInput: { use_cases: ["a", "b"], limit: 5 }, + }); // final args + run.handleUpdate({ + sessionUpdate: "tool_call_update", + toolCallId: "c1", + status: "completed", + content: [{ content: { type: "text", text: "ok" } }], + }); run.finish(); const calls = ofType(emitted, "tool_call"); - assert.deepEqual(calls[calls.length - 1].input, { use_cases: ["a", "b"], limit: 5 }, "the LAST refresh carries the final args"); + assert.deepEqual( + calls[calls.length - 1].input, + { use_cases: ["a", "b"], limit: 5 }, + "the LAST refresh carries the final args", + ); const seq = types(emitted); - assert.ok(seq.lastIndexOf("tool_call") < seq.indexOf("tool_result"), "every refresh precedes the result"); + assert.ok( + seq.lastIndexOf("tool_call") < seq.indexOf("tool_result"), + "every refresh precedes the result", + ); }); it("scenario 8: a call announced with PARTIAL args still refreshes when the full args arrive", () => { // The initial notification itself can carry an early partial parse (non-empty), so a // has-args-at-announce gate must not suppress the refresh with the real args. const emitted: AgentEvent[] = []; - const run = createSandboxAgentOtel({ harness: "pi", model: "openai-codex/x", emit: (e) => emitted.push(e), emitSpans: false }); + const run = createSandboxAgentOtel({ + harness: "pi", + model: "openai-codex/x", + emit: (e) => emitted.push(e), + emitSpans: false, + }); run.start({ prompt: "x" }); - run.handleUpdate({ sessionUpdate: "tool_call", toolCallId: "c1", title: "read", rawInput: { path: "/" } }); // partial - run.handleUpdate({ sessionUpdate: "tool_call_update", toolCallId: "c1", rawInput: { path: "/etc/hosts" } }); // real args - run.handleUpdate({ sessionUpdate: "tool_call_update", toolCallId: "c1", status: "completed", content: [{ content: { type: "text", text: "ok" } }] }); + run.handleUpdate({ + sessionUpdate: "tool_call", + toolCallId: "c1", + title: "read", + rawInput: { path: "/" }, + }); // partial + run.handleUpdate({ + sessionUpdate: "tool_call_update", + toolCallId: "c1", + rawInput: { path: "/etc/hosts" }, + }); // real args + run.handleUpdate({ + sessionUpdate: "tool_call_update", + toolCallId: "c1", + status: "completed", + content: [{ content: { type: "text", text: "ok" } }], + }); run.finish(); const calls = ofType(emitted, "tool_call"); assert.equal(calls.length, 2, "one initial surface + one input refresh"); - assert.deepEqual(calls[calls.length - 1].input, { path: "/etc/hosts" }, "the refresh carries the real args"); + assert.deepEqual( + calls[calls.length - 1].input, + { path: "/etc/hosts" }, + "the refresh carries the real args", + ); }); it("scenario 6: a call announced WITH args refreshes only on genuinely new args", () => { // If the initial notification already has real args, that's the input — a later update that // merely repeats/omits args must NOT emit a duplicate tool_call. const emitted: AgentEvent[] = []; - const run = createSandboxAgentOtel({ harness: "pi", model: "openai-codex/x", emit: (e) => emitted.push(e), emitSpans: false }); + const run = createSandboxAgentOtel({ + harness: "pi", + model: "openai-codex/x", + emit: (e) => emitted.push(e), + emitSpans: false, + }); run.start({ prompt: "x" }); - run.handleUpdate({ sessionUpdate: "tool_call", toolCallId: "c1", title: "getWeather", rawInput: { city: "Paris" } }); - run.handleUpdate({ sessionUpdate: "tool_call_update", toolCallId: "c1", status: "completed", content: [{ content: { type: "text", text: "sunny" } }] }); + run.handleUpdate({ + sessionUpdate: "tool_call", + toolCallId: "c1", + title: "getWeather", + rawInput: { city: "Paris" }, + }); + run.handleUpdate({ + sessionUpdate: "tool_call_update", + toolCallId: "c1", + status: "completed", + content: [{ content: { type: "text", text: "sunny" } }], + }); run.finish(); const calls = ofType(emitted, "tool_call"); assert.equal(calls.length, 1, "no refresh — args were present up front"); - assert.deepEqual(calls[0].input, { city: "Paris" }, "keeps the initial args"); + assert.deepEqual( + calls[0].input, + { city: "Paris" }, + "keeps the initial args", + ); }); it("scenario 7: a denied gate stamps `denied` on the closing failed tool_result", () => { From cfee3355f0257353e123b79139470a05a2522d85 Mon Sep 17 00:00:00 2001 From: Arda Erzin Date: Sat, 18 Jul 2026 01:46:11 +0200 Subject: [PATCH 09/11] fix(agent-chat): restore a parked+resumed approval correctly on reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit transcriptToMessages keyed tool parts per-draft and closed a turn on any `done`, so a gate parked in one run and settled in the resume run landed in different drafts — the settling tool_result was dropped and the gate stuck at 'Awaiting approval'. Key tool parts transcript-wide (a resume's result settles the earlier gate in place) and don't treat a paused `done` as a turn boundary. Defense in depth alongside the runner-side persist fix — un-sticks the gate even for records written before it. --- .../assets/transcriptToMessages.test.ts | 110 ++++++++++++++++++ .../assets/transcriptToMessages.ts | 47 ++++++-- 2 files changed, 146 insertions(+), 11 deletions(-) create mode 100644 web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.ts diff --git a/web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.ts b/web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.ts new file mode 100644 index 0000000000..c66fa77798 --- /dev/null +++ b/web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.ts @@ -0,0 +1,110 @@ +import type {SessionRecord} from "@agenta/entities/session" +import {describe, it, expect} from "vitest" + +import {transcriptToMessages} from "./transcriptToMessages" + +/** + * Regression tests for restoring a session whose turn PAUSED on a HITL approval and RESUMED. + * + * The bug: on reload a resolved gate showed stuck at "Awaiting approval" and the turn looked + * duplicated. Root cause was persist-side — a paused run persisted a `done` (a false turn + * boundary) and the resume run re-persisted the user message — so the parked gate and the + * `tool_result` that settled it landed in different restored drafts, and the result was dropped. + * These pin the restore side: a paused `done` is not a boundary, and tool parts settle across + * drafts (defense-in-depth for records produced before the runner-side fixes). + */ + +let seq = 0 +/** Build a post-transform SessionRecord as transcriptToMessages consumes it. */ +const rec = ( + sender: "user" | "agent", + type: string, + payload: Record = {}, +): SessionRecord => + ({ + id: `rec-${seq++}`, + sender, + session_update: type, + payload: {type, ...payload}, + }) as unknown as SessionRecord + +const toolPart = (m: ReturnType, tool: string) => + m?.flatMap((msg) => msg.parts).find((p) => (p as {type?: string}).type === `tool-${tool}`) as + | {state?: string; output?: unknown} + | undefined + +describe("transcriptToMessages — parked-then-resumed approval restore", () => { + it("settles the gate and keeps ONE turn when the runner tags the paused done (both fixes)", () => { + const messages = transcriptToMessages([ + // Run 1 (paused): user prompt, the gated Terminal, the approval gate, a PAUSED done. + rec("user", "message", {text: "look at the agent-files folder"}), + rec("agent", "tool_call", {id: "T", name: "Terminal"}), + rec("agent", "interaction_request", { + id: "E", + kind: "user_approval", + payload: {toolCallId: "T", toolCall: {name: "Terminal"}}, + }), + rec("agent", "done", {stopReason: "paused"}), + // Run 2 (resume): the Terminal executes, then the answer, then a real done. + rec("agent", "tool_result", {id: "T", output: "README.md ..."}), + rec("agent", "message", {text: "Hi there! I checked the folder."}), + rec("agent", "done"), + ]) + + // One user + one assistant message — the paused done did NOT split the turn, and no + // duplicate user turn (the resume didn't re-persist it). + expect(messages?.map((m) => m.role)).toEqual(["user", "assistant"]) + // The gate is resolved, NOT stuck awaiting approval. + const terminal = toolPart(messages, "Terminal") + expect(terminal?.state).toBe("output-available") + expect(terminal?.output).toBe("README.md ...") + // Exactly one Terminal part — the resume's re-emit did not duplicate it. + const terminals = messages + ?.flatMap((m) => m.parts) + .filter((p) => (p as {type?: string}).type === "tool-Terminal") + expect(terminals?.length).toBe(1) + }) + + it("still settles the gate for OLD records (untagged paused done + duplicate user)", () => { + // Defense-in-depth: records produced before the runner fixes — the paused done is untagged + // (splits the turn) and the resume re-persisted the user message. The transcript-global + // tool map must still let the run-2 tool_result settle the run-1 gate part. + const messages = transcriptToMessages([ + rec("user", "message", {text: "look at the agent-files folder"}), + rec("agent", "tool_call", {id: "T", name: "Terminal"}), + rec("agent", "interaction_request", { + id: "E", + kind: "user_approval", + payload: {toolCallId: "T", toolCall: {name: "Terminal"}}, + }), + rec("agent", "done"), // untagged → still a boundary + rec("user", "message", {text: "look at the agent-files folder"}), // duplicate + rec("agent", "tool_result", {id: "T", output: "README.md ..."}), + rec("agent", "message", {text: "Hi there!"}), + rec("agent", "done"), + ]) + + // No part is left stuck awaiting approval — the gate settled across the draft split. + const stuck = messages + ?.flatMap((m) => m.parts) + .some((p) => (p as {state?: string}).state === "approval-requested") + expect(stuck).toBe(false) + const terminal = toolPart(messages, "Terminal") + expect(terminal?.state).toBe("output-available") + }) + + it("leaves a genuinely-unresumed gate awaiting approval", () => { + // A turn that paused and was never resumed (no tool_result) must still show the gate. + const messages = transcriptToMessages([ + rec("user", "message", {text: "do it"}), + rec("agent", "tool_call", {id: "T", name: "Terminal"}), + rec("agent", "interaction_request", { + id: "E", + kind: "user_approval", + payload: {toolCallId: "T", toolCall: {name: "Terminal"}}, + }), + rec("agent", "done", {stopReason: "paused"}), + ]) + expect(toolPart(messages, "Terminal")?.state).toBe("approval-requested") + }) +}) diff --git a/web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts b/web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts index b7c3aec300..c87f03fc2f 100644 --- a/web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts +++ b/web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts @@ -25,8 +25,6 @@ interface DraftMessage { /** Open streamed text/reasoning parts keyed by event id, for delta accumulation. */ text: Map reasoning: Map - /** Tool parts keyed by toolCallId so results/approvals attach to the right call. */ - tools: Map /** The turn's observability trace id, if the durable record carries one (see below). */ traceId?: string /** Token/cost totals from the turn's persisted `usage` event, in the raw stream shape. */ @@ -74,13 +72,25 @@ const newDraft = (id: string, role: "user" | "assistant"): DraftMessage => ({ parts: [], text: new Map(), reasoning: new Map(), - tools: new Map(), }) const toolPartType = (name?: string | null): string => (name ? `tool-${name}` : "dynamic-tool") -/** Apply one transcript event's payload onto the current assistant/user draft message. */ -function applyEvent(draft: DraftMessage, payload: Record): void { +/** + * Apply one transcript event's payload onto the current draft. + * + * `tools` is TRANSCRIPT-GLOBAL, not per-draft: a parked tool call and the `tool_result` that + * settles it can land in DIFFERENT turns (the gate is emitted in the paused run, the result in the + * resume run). Keying tool parts globally lets that later result settle the earlier gate part in + * place, instead of being dropped for want of a matching part in its own draft — which left the + * gate stuck at "approval-requested" on reload. The part still lives in the draft where its + * `tool_call` first appeared. + */ +function applyEvent( + draft: DraftMessage, + payload: Record, + tools: Map, +): void { const type = payload.type as string | undefined const str = (v: unknown): string => (typeof v === "string" ? v : v == null ? "" : String(v)) @@ -117,6 +127,15 @@ function applyEvent(draft: DraftMessage, payload: Record): void } case "tool_call": { const toolCallId = str(payload.id) + // A keep-alive resume re-emits the parked call's `tool_call` (so the live FE flips it to + // done); on replay that repeat must UPDATE the existing part, not push a duplicate that + // orphans the original approval-requested one. Keyed globally, so it also finds a part + // created in an earlier draft. + const existing = tools.get(toolCallId) + if (existing) { + if (payload.input !== undefined) existing.input = payload.input + return + } const part: Part = { type: toolPartType(payload.name as string), toolCallId, @@ -124,11 +143,11 @@ function applyEvent(draft: DraftMessage, payload: Record): void input: payload.input, } draft.parts.push(part) - draft.tools.set(toolCallId, part) + tools.set(toolCallId, part) return } case "tool_result": { - const part = draft.tools.get(str(payload.id)) + const part = tools.get(str(payload.id)) if (!part) return if (payload.denied) { part.state = "output-denied" @@ -151,7 +170,7 @@ function applyEvent(draft: DraftMessage, payload: Record): void const toolCallId = str( reqPayload.toolCallId ?? toolCall.id ?? toolCall.toolCallId ?? payload.id, ) - let part = draft.tools.get(toolCallId) + let part = tools.get(toolCallId) if (!part) { // The runner parked without first surfacing the tool call — synthesize one. part = { @@ -165,7 +184,7 @@ function applyEvent(draft: DraftMessage, payload: Record): void input: toolCall.rawInput ?? toolCall.input, } draft.parts.push(part) - draft.tools.set(toolCallId, part) + tools.set(toolCallId, part) } // Only park if still unsettled — a later `tool_result` overwrites this. if (part.state === "input-available") { @@ -218,6 +237,9 @@ function applyEvent(draft: DraftMessage, payload: Record): void export function transcriptToMessages(records: SessionRecord[]): UIMessage[] | null { const drafts: DraftMessage[] = [] let current: DraftMessage | null = null + // Tool parts are keyed transcript-wide (see applyEvent): a parked gate and its settling result + // can span two turns (paused run → resume run). + const tools = new Map() for (const row of records) { const payload = row.payload @@ -228,9 +250,12 @@ export function transcriptToMessages(records: SessionRecord[]): UIMessage[] | nu const traceId = extractTraceId(row, p) // `done` terminates a turn. Records are runner-output-only (no user rows), so without // this every turn folds into one assistant bubble; closing the draft here starts a - // fresh message per turn. + // fresh message per turn. EXCEPT a PAUSED turn's `done`: it is not a real boundary — the + // turn continues on the resume run — so closing here would sever a parked gate from the + // result that settles it (gate stuck on reload). Keep the draft open across it. if (row.session_update === "done" || p.type === "done") { if (current && traceId && !current.traceId) current.traceId = traceId + if (p.stopReason === "paused") continue current = null continue } @@ -240,7 +265,7 @@ export function transcriptToMessages(records: SessionRecord[]): UIMessage[] | nu drafts.push(current) } if (traceId && !current.traceId) current.traceId = traceId - applyEvent(current, p) + applyEvent(current, p, tools) } const messages = drafts From 99ca257e5c1a12719cd3a15bab627762d3c61d82 Mon Sep 17 00:00:00 2001 From: Arda Erzin Date: Mon, 20 Jul 2026 03:59:08 +0200 Subject: [PATCH 10/11] chore(hosting): Claude Code harness config in EE dev compose --- hosting/docker-compose/ee/docker-compose.dev.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/hosting/docker-compose/ee/docker-compose.dev.yml b/hosting/docker-compose/ee/docker-compose.dev.yml index 19956cb1c8..bdbe492bd9 100644 --- a/hosting/docker-compose/ee/docker-compose.dev.yml +++ b/hosting/docker-compose/ee/docker-compose.dev.yml @@ -403,6 +403,12 @@ services: AGENTA_RUNNER_HOST: ${AGENTA_RUNNER_HOST:-0.0.0.0} AGENTA_RUNNER_TOKEN: ${AGENTA_RUNNER_TOKEN:?AGENTA_RUNNER_TOKEN is required} PI_CODING_AGENT_DIR: /pi-agent + # Claude Code harness (subscription / runtime_provided auth). CLAUDE_CONFIG_DIR is the + # writable config/state mount the run-plan requires; CLAUDE_CODE_OAUTH_TOKEN is the login + # (macOS keeps the Claude login in Keychain, which can't be bind-mounted, so the token is + # the local auth path). Both resolve from the loaded env file. + CLAUDE_CONFIG_DIR: /claude-config + CLAUDE_CODE_OAUTH_TOKEN: ${CLAUDE_CODE_OAUTH_TOKEN:-} # The API as reached FROM this container — by its compose service name, over the # network (the same `http://api:8000` the workers use), NOT the public # `http://localhost/api` (which doesn't resolve inside a container). Used by the @@ -430,6 +436,10 @@ services: # Read-write on purpose: the harness refreshes its own OAuth token here and the new # token must persist back to the host login (a copy would discard it). - ${HOME}/.pi/agent:/pi-agent:rw + # Claude Code config/state (writable). The login comes from CLAUDE_CODE_OAUTH_TOKEN; this + # dir holds the settings/session state the harness reads and refreshes. Dedicated dir (not + # ~/.claude) so the container never touches the host's real Claude config. + - ${HOME}/.agenta-claude-config:/claude-config:rw # === NETWORK ============================================== # networks: - agenta-network From b1e05c58a459f3cea9a6bb8d3ac0ae878fc6da95 Mon Sep 17 00:00:00 2001 From: Arda Erzin Date: Mon, 20 Jul 2026 13:09:06 +0200 Subject: [PATCH 11/11] style(runner): prettier-format the rebased parallel-approval port --- .../sandbox_agent/environment-setup.ts | 15 ++------ .../src/engines/sandbox_agent/run-turn.ts | 14 ++------ .../sandbox_agent/runtime-contracts.ts | 35 +++++++++++++++---- 3 files changed, 34 insertions(+), 30 deletions(-) diff --git a/services/runner/src/engines/sandbox_agent/environment-setup.ts b/services/runner/src/engines/sandbox_agent/environment-setup.ts index fee4bc3a97..0bb90b35ff 100644 --- a/services/runner/src/engines/sandbox_agent/environment-setup.ts +++ b/services/runner/src/engines/sandbox_agent/environment-setup.ts @@ -2,23 +2,14 @@ import { rmSync } from "node:fs"; import { apiBase } from "../../apiBase.ts"; -import { - resolveRunSessionId, - type AgentRunRequest, -} from "../../protocol.ts"; +import { resolveRunSessionId, type AgentRunRequest } from "../../protocol.ts"; import { type ClientToolOutcome } from "../../responder.ts"; import type { ClientToolRelay } from "../../tools/client-tool-relay.ts"; -import { - agentMountPath, - signAgentMountCredentials, -} from "./agent-mount.ts"; +import { agentMountPath, signAgentMountCredentials } from "./agent-mount.ts"; import { createToolCallCorrelationIndex } from "./client-tools.ts"; import { buildDaemonEnv, resolveDaemonBinary } from "./daemon.ts"; import { conciseError } from "./errors.ts"; -import { - signSessionMountCredentials, - type MountCredentials, -} from "./mount.ts"; +import { signSessionMountCredentials, type MountCredentials } from "./mount.ts"; import { buildPiExtensionEnv, configurePiSessionWorkspace, diff --git a/services/runner/src/engines/sandbox_agent/run-turn.ts b/services/runner/src/engines/sandbox_agent/run-turn.ts index f97109bd1b..87bf354d52 100644 --- a/services/runner/src/engines/sandbox_agent/run-turn.ts +++ b/services/runner/src/engines/sandbox_agent/run-turn.ts @@ -40,16 +40,10 @@ import { } from "./client-tools.ts"; import { invalidateContinuity } from "./environment.ts"; import { conciseError } from "./errors.ts"; -import { - PAUSED, - PendingApprovalPauseController, -} from "./pause.ts"; +import { PAUSED, PendingApprovalPauseController } from "./pause.ts"; import { findSwallowedPiError } from "./pi-error.ts"; import { buildRelayExecutionGuard } from "./relay-guard.ts"; -import { - createRunLimits, - resolveRunLimits, -} from "./run-limits.ts"; +import { createRunLimits, resolveRunLimits } from "./run-limits.ts"; import { approvalCollectWindowMs, RUN_LIMIT_TRIPPED, @@ -63,9 +57,7 @@ import { serverPermissionsFromRequest, shouldSuppressPausedToolCallUpdate, } from "./runtime-policy.ts"; -import { - syncHarnessSessionDurable, -} from "./session-continuity-durable.ts"; +import { syncHarnessSessionDurable } from "./session-continuity-durable.ts"; import { sessionContinuityStore } from "./session-continuity.ts"; import { priorMessages } from "./transcript.ts"; import { resolveRunUsage } from "./usage.ts"; diff --git a/services/runner/src/engines/sandbox_agent/runtime-contracts.ts b/services/runner/src/engines/sandbox_agent/runtime-contracts.ts index cee255af6f..187fd0c168 100644 --- a/services/runner/src/engines/sandbox_agent/runtime-contracts.ts +++ b/services/runner/src/engines/sandbox_agent/runtime-contracts.ts @@ -1,9 +1,16 @@ import { InMemorySessionPersistDriver, SandboxAgent } from "sandbox-agent"; -import { type AgentRunRequest, type HarnessCapabilities } from "../../protocol.ts"; +import { + type AgentRunRequest, + type HarnessCapabilities, +} from "../../protocol.ts"; import { type Responder } from "../../responder.ts"; import type { ClientToolRelay } from "../../tools/client-tool-relay.ts"; -import { localRelayHost, sandboxRelayHost, startToolRelay } from "../../tools/relay.ts"; +import { + localRelayHost, + sandboxRelayHost, + startToolRelay, +} from "../../tools/relay.ts"; import { createSandboxAgentOtel } from "../../tracing/otel.ts"; import { createAcpFetch } from "./acp-fetch.ts"; import { type ParkedApprovalGateType } from "./acp-interactions.ts"; @@ -13,13 +20,28 @@ import { createToolCallCorrelationIndex } from "./client-tools.ts"; import { buildDaemonEnv, resolveDaemonBinary } from "./daemon.ts"; import { createCookieFetch, prepareDaytonaPiAssets } from "./daytona.ts"; import { applyModel } from "./model.ts"; -import { discoverTunnelEndpoint, mountHarnessSessionDirs, mountStorage, mountStorageRemote, signSessionMountCredentials, unmountStorage, type MountCredentials } from "./mount.ts"; +import { + discoverTunnelEndpoint, + mountHarnessSessionDirs, + mountStorage, + mountStorageRemote, + signSessionMountCredentials, + unmountStorage, + type MountCredentials, +} from "./mount.ts"; import { PendingApprovalPauseController } from "./pause.ts"; import { buildSandboxProvider } from "./provider.ts"; import { createRunLimits, resolveRunLimits } from "./run-limits.ts"; import { type BuildRunPlanDeps, type RunPlan } from "./run-plan.ts"; -import { clearSandboxPointer, readStoredSandboxPointer, writeSandboxPointer } from "./sandbox-reconnect.ts"; -import { hydrateHarnessSessionFromDurable, syncHarnessSessionDurable } from "./session-continuity-durable.ts"; +import { + clearSandboxPointer, + readStoredSandboxPointer, + writeSandboxPointer, +} from "./sandbox-reconnect.ts"; +import { + hydrateHarnessSessionFromDurable, + syncHarnessSessionDurable, +} from "./session-continuity-durable.ts"; import { type SessionContinuityStore } from "./session-continuity.ts"; import { type TeardownReason } from "./teardown.ts"; import { uploadToolMcpAssets } from "./tool-mcp-assets.ts"; @@ -273,5 +295,4 @@ export interface SessionEnvironment { } export type AcquireEnvironmentResult = - | { ok: true; env: SessionEnvironment } - | { ok: false; error: string }; + { ok: true; env: SessionEnvironment } | { ok: false; error: string };