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 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..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, @@ -347,8 +338,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..87bf354d52 100644 --- a/services/runner/src/engines/sandbox_agent/run-turn.ts +++ b/services/runner/src/engines/sandbox_agent/run-turn.ts @@ -40,21 +40,15 @@ 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, sendLastMessageOnly, type CurrentTurn, - type ParkedApproval, type RunTurnOptions, type SessionEnvironment, } from "./runtime-contracts.ts"; @@ -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"; @@ -92,13 +84,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 +161,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 +230,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 +325,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 +358,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 +450,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 +509,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}`); @@ -542,7 +559,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) { @@ -613,6 +632,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..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"; @@ -127,14 +149,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 +202,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 +267,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; @@ -242,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 }; 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/src/server.ts b/services/runner/src/server.ts index d2ddc2987e..f681cba87f 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, @@ -934,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"); @@ -1150,7 +1175,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 +1233,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..051da69d63 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) => { @@ -1063,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 @@ -1081,11 +1090,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 +1444,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 +1485,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 +1503,9 @@ export function createSandboxAgentOtel( } toolSpans.delete(id); record({ type: "tool_result", id, output: message, isError: true }); + settled += 1; } + return settled; } /** @@ -1549,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(); @@ -1573,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/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(); + }); }); 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", () => { 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/); }); 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 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/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/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/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/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/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-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/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/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 (
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/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/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"}, 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-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) + }) +}) 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) 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