From 197718bc00b080539e3506987d6c6128cbfd84ec Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Mon, 27 Jul 2026 09:27:00 -0700 Subject: [PATCH 01/87] feat(acpx): per-step round-trip + provider-latency attribution for sandbox startup (#10222) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Sandbox-backed runs need more precise startup observability so operators can see where time is spent before an adapter is invoked > - Aggregate startup timing hides which boundary is actually slow, especially in remote execution where the bottleneck can move between host round-trips, provider-boundary latency, and handshake phases > - This pull request keeps the existing startup timing channel additive while attributing the latency to the specific startup steps that caused it > - The benefit is better diagnosis of sandbox startup regressions without changing control flow or introducing a schema migration ## Linked Issues or Issue Description Refs: #10204 This PR extends the existing sandbox run-startup timing observability with per-step round-trip and provider-latency attribution for the Daytona startup path. It keeps the event payload additive and free-form, and it leaves the control flow, database schema, and external adapter interfaces unchanged. ## What Changed - Added per-step round-trip counting for the host-to-sandbox execute seam - Added provider-boundary duration accumulation for the Daytona execute and re-fetch steps - Split the ACP handshake timing into `createRuntimeMs` and `ensureSessionMs` while preserving the warm-handle skip - Kept the startup timing payload additive and did not add a schema migration ## Verification - `adapter-utils` acpx-engine and startup-timing suites: pass - Daytona plugin suite: pass with mocked SDK and injected-clock duration assertions - `server` environment-execution-target suite: pass - `tsc --noEmit` for adapter-utils and server: pass - Git validation: fetched `origin/feat/sandbox-start-step-timing-attribution`, confirmed it matches the authorized submit SHA `53c573266e618af05c57a0720aaa9d9e0452de61`, and confirmed the branch contains only the expected commit on top of `origin/master` - Searched GitHub for duplicate or related PRs/issues; found one closely related merged PR and no open duplicate on this branch - Checked `ROADMAP.md`; the broad sandbox-agent roadmap section does not call out this specific startup-timing attribution work as a duplicate ## Risks - Low risk: the change is additive and only enriches existing timing data - Downstream consumers that assume aggregate-only startup timing may need to tolerate the additional per-step fields - The finer spawn/initialize/session split remains a follow-up in the external ACP client because that hook is not available here yet ## Model Used OpenAI GPT-5, tool-using coding agent ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Harold Kim Co-authored-by: Paperclip --- .../src/acpx-engine/execute.test.ts | 143 +++++++++++++++++- .../adapter-utils/src/acpx-engine/execute.ts | 105 +++++++++++-- .../src/acpx-engine/startup-timing.test.ts | 80 ++++++++++ .../src/acpx-engine/startup-timing.ts | 62 +++++++- .../src/command-managed-runtime.ts | 19 +++ .../daytona/src/plugin.test.ts | 87 ++++++++--- .../sandbox-providers/daytona/src/plugin.ts | 49 +++++- .../environment-execution-target.test.ts | 95 ++++++++++++ .../services/environment-execution-target.ts | 25 +++ 9 files changed, 628 insertions(+), 37 deletions(-) diff --git a/packages/adapter-utils/src/acpx-engine/execute.test.ts b/packages/adapter-utils/src/acpx-engine/execute.test.ts index 3b064cadb0c..f08288b3d00 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.test.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.test.ts @@ -46,7 +46,18 @@ async function makeTempRoot() { } afterEach(async () => { - await Promise.all(tempRoots.splice(0).map((root) => fs.rm(root, { recursive: true, force: true }))); + // A remote run stages a process-session bridge whose detached event writer can + // still be flushing a trailing event file into `.../process-sessions//events` + // when the run's own best-effort `client.remove(sessionDir)` (which production + // catch-wraps) has already returned. Under CI load that trailing write can land + // between this recursive delete's directory snapshot and its `rmdir`, surfacing as + // `ENOTEMPTY`. `maxRetries`/`retryDelay` make the cleanup ride out that window the + // same way production tolerates it, instead of failing the just-passed test. + await Promise.all( + tempRoots.splice(0).map((root) => + fs.rm(root, { recursive: true, force: true, maxRetries: 5, retryDelay: 50 }), + ), + ); }); async function pathExists(candidate: string): Promise { @@ -80,7 +91,16 @@ function createLocalSandboxRunner( }) => void, ) { let counter = 0; + // Synthetic provider-duration accumulators so per-step payload assertions can + // verify the `providerExecMs`/`providerGetMs` threading end-to-end (the real + // sandbox runner sources these from the Daytona plugin's result metadata; this + // double stands in for that with a fixed per-exec cost). + let providerExecMs = 0; + let providerGetMs = 0; return { + execCount: () => counter, + providerExecMs: () => providerExecMs, + providerGetMs: () => providerGetMs, execute: async (input: { command: string; args?: string[]; @@ -92,6 +112,8 @@ function createLocalSandboxRunner( onSpawn?: (meta: { pid: number; startedAt: string }) => Promise; }) => { counter += 1; + providerExecMs += 600; + providerGetMs += 15; onExecute?.(input); const command = input.command === "bash" ? "/bin/bash" : input.command; return await runChildProcess(`acpx-sandbox-run-${counter}`, command, input.args ?? [], { @@ -2773,6 +2795,125 @@ describe("ACPX engine per-step startup timing (run.startup.step events)", () => } }); + it("carries per-step roundTrips + provider durations sourced from the sandbox runner counter", async () => { + const root = await makeTempRoot(); + const stateDir = path.join(root, "state"); + const localCwd = path.join(root, "worktree"); + const remoteCwd = path.join(root, "remote-workspace"); + await fs.mkdir(localCwd, { recursive: true }); + await fs.mkdir(remoteCwd, { recursive: true }); + const executionTarget = { + kind: "remote", + transport: "sandbox", + providerKey: "fake-plugin", + remoteCwd, + runner: createLocalSandboxRunner(), + }; + + const { events } = await runExecutor( + { agent: "custom", agentCommand: "node ./fake-acp.js", stateDir, cwd: localCwd }, + { authToken: "real-run-jwt", executionTarget }, + ); + + const steps = stepEvents(events); + const seen = new Map(steps.map((event) => [String(event.payload?.step), event])); + + // Every timed boundary carries a numeric roundTrips (the runner exposes + // execCount), even the ones that never exec. + for (const event of steps) { + expect(typeof event.payload?.roundTrips).toBe("number"); + } + // workspace.resolve is host-only → zero host→sandbox execs. + expect(seen.get("workspace.resolve")?.payload?.roundTrips).toBe(0); + // stage.sync ships the workspace over the exec seam → at least one round-trip, + // and the accumulated provider durations scale with it. + const stageSync = seen.get("stage.sync"); + expect(stageSync?.payload?.roundTrips as number).toBeGreaterThan(0); + expect(stageSync?.payload?.providerExecMs).toBe( + (stageSync?.payload?.roundTrips as number) * 600, + ); + expect(stageSync?.payload?.providerGetMs).toBe( + (stageSync?.payload?.roundTrips as number) * 15, + ); + // The external ACP client crosses no host exec seam. + expect(seen.get("acp.handshake")?.payload?.roundTrips).toBe(0); + }); + + it("splits acp.handshake into createRuntimeMs and ensureSessionMs sub-phases", async () => { + const root = await makeTempRoot(); + const stateDir = path.join(root, "state"); + const localCwd = path.join(root, "worktree"); + const remoteCwd = path.join(root, "remote-workspace"); + await fs.mkdir(localCwd, { recursive: true }); + await fs.mkdir(remoteCwd, { recursive: true }); + const executionTarget = { + kind: "remote", + transport: "sandbox", + providerKey: "fake-plugin", + remoteCwd, + runner: createLocalSandboxRunner(), + }; + + const { events } = await runExecutor( + { agent: "custom", agentCommand: "node ./fake-acp.js", stateDir, cwd: localCwd }, + { authToken: "real-run-jwt", executionTarget }, + ); + + const handshake = stepEvents(events).find((event) => event.payload?.step === "acp.handshake"); + expect(handshake).toBeTruthy(); + expect(typeof handshake!.payload?.createRuntimeMs).toBe("number"); + expect(handshake!.payload?.createRuntimeMs as number).toBeGreaterThanOrEqual(0); + expect(typeof handshake!.payload?.ensureSessionMs).toBe("number"); + expect(handshake!.payload?.ensureSessionMs as number).toBeGreaterThanOrEqual(0); + }); + + it("emits no acp.handshake event when a warm-handle hit skips the handshake", async () => { + const root = await makeTempRoot(); + const stateDir = path.join(root, "state"); + const warmHandles = new Map(); + const secondEvents: Array<{ eventType: string; payload?: Record }> = []; + const execute = createAcpxEngineExecutor({ + warmHandles, + createRuntime: () => buildRuntime() as never, + }); + const config = { + agent: "custom", + agentCommand: "node ./fake-acp.js", + stateDir, + mode: "persistent", + warmHandleIdleMs: 60_000, + }; + const first = await execute({ + runId: "run-warm-1", + agent: { id: "agent-1", companyId: "company-1" }, + runtime: {}, + config, + context: {}, + onLog: async () => {}, + onMeta: async () => {}, + onEvent: async () => {}, + } as never); + // The second run reuses the warm handle, so the whole handshake block is + // skipped — it must emit NO acp.handshake event (not a zero-duration one). + await execute({ + runId: "run-warm-2", + agent: { id: "agent-1", companyId: "company-1" }, + runtime: { sessionParams: (first as { sessionParams?: unknown }).sessionParams }, + config, + context: {}, + onLog: async () => {}, + onMeta: async () => {}, + onEvent: async (event: { eventType: string; payload?: Record }) => { + secondEvents.push(event); + }, + } as never); + + const handshakeEmitted = secondEvents.some( + (event) => event.eventType === "run.startup.step" && event.payload?.step === "acp.handshake", + ); + expect(handshakeEmitted).toBe(false); + }); + it("does not emit startup-step events on a local (non-sandbox) run except workspace.resolve", async () => { const root = await makeTempRoot(); const localCwd = path.join(root, "worktree"); diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index 00661dde9fa..5c1ec5937e5 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -82,7 +82,8 @@ import { DEFAULT_ACP_ENGINE_TIMEOUT_SEC, DEFAULT_ACP_ENGINE_WARM_HANDLE_IDLE_MS, } from "./constants.js"; -import { measureStartupStep } from "./startup-timing.js"; +import { measureStartupStep, type StartupStepMeasureOptions } from "./startup-timing.js"; +import type { CommandManagedRuntimeRunner } from "../command-managed-runtime.js"; const defaultModuleDir = path.dirname(fileURLToPath(import.meta.url)); const PAPERCLIP_MANAGED_CODEX_SKILLS_MANIFEST = ".paperclip-managed-skills.json"; @@ -381,6 +382,12 @@ interface AcpxPreparedRuntime { paperclipClaudeSettings: PaperclipClaudeSettingsResult | null; mcpServers: NonNullable; mcpIdentity: Array<{ name: string; url: string; connectionId: string }>; + // Per-step round-trip / provider-duration readers sourced from the sandbox + // runner's counters (Open Q1). Empty for local runs and the runner-less + // fallback, where no host→sandbox exec seam exists. Threaded into the + // `acp.handshake` `measureStartupStep` call in the executor (the other six + // boundaries live inside `buildRuntime` and read it directly). + stepMetrics: StartupStepMeasureOptions; } const defaultWarmHandles = new Map(); @@ -838,6 +845,11 @@ async function prepareCodexSkillRuntime(input: { // `onEvent`), so the codex skill prep behaves identically when unmeasured. onEvent?: AdapterExecutionContext["onEvent"]; now?: () => number; + // Round-trip / provider-duration readers for the nested `skills.reconcile` + // boundary (Open Q1). Threaded from `buildRuntime` so the step reports the + // same host→sandbox counters as its siblings (0 here — skill prep is + // host-only — which is itself the answer to "does this step exec?"). + stepMetrics?: StartupStepMeasureOptions; }): Promise<{ identity: Record; commandNotes: string[] }> { const now = input.now ?? (() => Date.now()); const envConfig = parseObject(input.config.env); @@ -870,6 +882,7 @@ async function prepareCodexSkillRuntime(input: { selectedSkills, onLog: input.onLog, }), + input.stepMetrics ?? {}, ); for (const entry of selectedSkills) { @@ -1216,6 +1229,22 @@ async function stageAcpRemoteRuntime(input: { }); } +// Bind a startup-step round-trip/provider-duration reader set to a runner's +// cumulative counters (Open Q1). Only the sandbox runner instruments the exec +// seam, so a runner without `execCount` (SSH, or none) yields an empty option +// set and the affected steps omit the fields entirely. Reader closures are +// passed — not the runner — so `measureStartupStep` stays runner-agnostic. +function buildStartupStepMetrics( + runner: CommandManagedRuntimeRunner | undefined, +): StartupStepMeasureOptions { + if (!runner) return {}; + return { + ...(runner.execCount ? { roundTrips: () => runner.execCount!() } : {}), + ...(runner.providerExecMs ? { providerExecMs: () => runner.providerExecMs!() } : {}), + ...(runner.providerGetMs ? { providerGetMs: () => runner.providerGetMs!() } : {}), + }; +} + async function buildRuntime(input: { ctx: AdapterExecutionContext; engine: AcpxEngineSettings; @@ -1252,6 +1281,17 @@ async function buildRuntime(input: { ? remoteExecutionIdentity.remoteCwd : cwd; const executionTargetIsRemote = remoteExecutionIdentity !== null; + // Round-trip / provider-duration readers for per-step attribution (Open Q1), + // sourced from the sandbox runner's cumulative counters. `measureStartupStep` + // reads each as a `() => number` closure (never the runner itself, Risk R1) + // and emits the per-step delta. Empty when there is no runner (local runs, + // the runner-less ACP→CLI fallback, or an SSH runner that does not + // instrument the seam), so those steps simply omit the fields. + const stepMetrics = buildStartupStepMetrics( + executionTarget?.kind === "remote" && executionTarget.transport === "sandbox" + ? executionTarget.runner + : undefined, + ); const shapedWorkspaceEnv = shapePaperclipWorkspaceEnvForExecution({ workspaceCwd: effectiveWorkspaceCwd, workspaceWorktreePath, @@ -1262,6 +1302,7 @@ async function buildRuntime(input: { // here on the awaited directory materialization. await measureStartupStep(input.ctx, nowMs, "workspace.resolve", () => ensureAbsoluteDirectory(cwd, { createIfMissing: true }), + stepMetrics, ); const acpxAgent = normalizeAgent(config); @@ -1422,7 +1463,9 @@ async function buildRuntime(input: { onLog: input.ctx.onLog, onEvent: input.ctx.onEvent, now: nowMs, + stepMetrics, }), + stepMetrics, ); skillsIdentity = preparedSkills.identity; skillCommandNotes.push(...preparedSkills.commandNotes); @@ -1675,7 +1718,7 @@ async function buildRuntime(input: { }; } return { stagedRuntime: await stage([]), teardown: null, dispose: null }; - }); + }, stepMetrics); const delta: Record = {}; for (const [key, value] of Object.entries(env)) { if (envBeforeStage[key] !== value) delta[key] = value; @@ -1715,6 +1758,7 @@ async function buildRuntime(input: { hostApiToken: env.PAPERCLIP_API_KEY, onLog: input.ctx.onLog, }), + stepMetrics, ); if (paperclipBridge) { Object.assign(env, paperclipBridge.env); @@ -1741,6 +1785,7 @@ async function buildRuntime(input: { timeoutSec, onLog: input.ctx.onLog, }), + stepMetrics, ) : null; } catch (err) { @@ -1813,6 +1858,7 @@ async function buildRuntime(input: { paperclipClaudeSettings, mcpServers, mcpIdentity, + stepMetrics, }; } @@ -2594,7 +2640,22 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { ? (chunk) => routeChildStderr(childStderrState, chunk) : undefined, }; - const runtime = cached?.runtime ?? createRuntime(runtimeOptions); + // Open Q2: split the ~7s `acp.handshake` into the two in-repo-observable + // sub-phases — the ACP runtime construction (`createRuntime`) vs the session + // establishment envelope (`ensureSession`). The finer spawn/`initialize`/ + // `session/new` split lives inside external `acpx` and is gated on an + // upstream lifecycle hook (not bundled here). `createRuntime` runs once and + // only on a cold start; a warm-handle hit reuses `cached.runtime`, so + // `createRuntimeMs` stays undefined and the split reports nothing for it. + let createRuntimeMs: number | undefined; + let runtime: AcpRuntime; + if (cached?.runtime) { + runtime = cached.runtime; + } else { + const createRuntimeStart = now(); + runtime = createRuntime(runtimeOptions); + createRuntimeMs = now() - createRuntimeStart; + } if (cached) clearWarmHandleTimer(cached); if (!canResume && asString(previousParams.runtimeSessionName, "")) { await ctx.onLog( @@ -2612,17 +2673,29 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { try { // Step 7 — acp.handshake: ACP session establishment (session/new or // resume). A throwing handshake still reports its duration before the - // resume-retry path below runs. - handle = await measureStartupStep(ctx, now, "acp.handshake", () => - runtime.ensureSession({ + // resume-retry path below runs. `roundTrips` is expected to be 0 (the + // ACP client is external, not the host exec seam); the payload also + // carries the createRuntime/ensureSession sub-split (Open Q2). + let ensureSessionMs: number | undefined; + handle = await measureStartupStep(ctx, now, "acp.handshake", async () => { + const ensureSessionStart = now(); + const established = await runtime.ensureSession({ sessionKey: prepared.sessionKey, agent: prepared.acpxAgent, mode: prepared.mode, cwd: prepared.cwd, resumeSessionId, sessionOptions: { env: prepared.env }, + }); + ensureSessionMs = now() - ensureSessionStart; + return established; + }, { + ...prepared.stepMetrics, + extra: () => ({ + ...(createRuntimeMs !== undefined ? { createRuntimeMs } : {}), + ...(ensureSessionMs !== undefined ? { ensureSessionMs } : {}), }), - ); + }); } catch (err) { if (!resumeSessionId || !isResumeFailure(err)) throw err; clearSession = true; @@ -2631,15 +2704,27 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { "stdout", `[paperclip] ACPX resume session "${resumeSessionId}" is unavailable; retrying with a fresh session.\n`, ); - handle = await measureStartupStep(ctx, now, "acp.handshake", () => - runtime.ensureSession({ + // Fresh-session retry: the runtime was already constructed on the + // first attempt (never re-created), so this event reports only its + // own `ensureSessionMs` — no `createRuntimeMs`. + let retryEnsureSessionMs: number | undefined; + handle = await measureStartupStep(ctx, now, "acp.handshake", async () => { + const ensureSessionStart = now(); + const established = await runtime.ensureSession({ sessionKey: prepared.sessionKey, agent: prepared.acpxAgent, mode: prepared.mode, cwd: prepared.cwd, sessionOptions: { env: prepared.env }, + }); + retryEnsureSessionMs = now() - ensureSessionStart; + return established; + }, { + ...prepared.stepMetrics, + extra: () => ({ + ...(retryEnsureSessionMs !== undefined ? { ensureSessionMs: retryEnsureSessionMs } : {}), }), - ); + }); } } } catch (err) { diff --git a/packages/adapter-utils/src/acpx-engine/startup-timing.test.ts b/packages/adapter-utils/src/acpx-engine/startup-timing.test.ts index 6db874f7304..70a51899a7a 100644 --- a/packages/adapter-utils/src/acpx-engine/startup-timing.test.ts +++ b/packages/adapter-utils/src/acpx-engine/startup-timing.test.ts @@ -28,6 +28,86 @@ describe("measureStartupStep", () => { expect(events[0]!.message).toBe("startup step: stage.sync (150ms)"); }); + it("includes the roundTrips delta in the payload when a round-trip reader is supplied", async () => { + let t = 0; + const now = () => t; + // Cumulative host→sandbox exec counter; the step performs 3 execs. + let execCount = 5; + const events: AdapterRuntimeEvent[] = []; + const onEvent = vi.fn(async (event: AdapterRuntimeEvent) => { + events.push(event); + }); + + await measureStartupStep({ onEvent }, now, "stage.sync", async () => { + t = 90; + execCount += 3; + return "ok"; + }, { roundTrips: () => execCount }); + + expect(events[0]!.payload).toMatchObject({ + step: "stage.sync", + durationMs: 90, + roundTrips: 3, + }); + }); + + it("reports roundTrips: 0 for a step that performs no execs (reader supplied)", async () => { + const now = () => 0; + const events: AdapterRuntimeEvent[] = []; + const onEvent = vi.fn(async (event: AdapterRuntimeEvent) => { + events.push(event); + }); + + await measureStartupStep({ onEvent }, now, "workspace.resolve", async () => "ok", { + roundTrips: () => 7, + }); + + expect(events[0]!.payload).toMatchObject({ step: "workspace.resolve", roundTrips: 0 }); + }); + + it("omits roundTrips from the payload when no reader is supplied", async () => { + const now = () => 0; + const events: AdapterRuntimeEvent[] = []; + const onEvent = vi.fn(async (event: AdapterRuntimeEvent) => { + events.push(event); + }); + + await measureStartupStep({ onEvent }, now, "workspace.resolve", async () => "ok"); + + expect(events[0]!.payload).not.toHaveProperty("roundTrips"); + }); + + it("accumulates provider exec/get durations and merges extra fields into the payload", async () => { + let t = 0; + const now = () => t; + let execMs = 100; + let getMs = 40; + const events: AdapterRuntimeEvent[] = []; + const onEvent = vi.fn(async (event: AdapterRuntimeEvent) => { + events.push(event); + }); + + await measureStartupStep({ onEvent }, now, "acp.handshake", async () => { + t = 7000; + execMs += 600; // one provider executeCommand round-trip + getMs += 15; // one client.get re-fetch + return "handle"; + }, { + providerExecMs: () => execMs, + providerGetMs: () => getMs, + extra: () => ({ createRuntimeMs: 12, ensureSessionMs: 6988 }), + }); + + expect(events[0]!.payload).toMatchObject({ + step: "acp.handshake", + durationMs: 7000, + providerExecMs: 600, + providerGetMs: 15, + createRuntimeMs: 12, + ensureSessionMs: 6988, + }); + }); + it("returns the wrapped fn result unchanged", async () => { const now = () => 0; const onEvent = vi.fn(async () => {}); diff --git a/packages/adapter-utils/src/acpx-engine/startup-timing.ts b/packages/adapter-utils/src/acpx-engine/startup-timing.ts index 06e46f13138..7e3f55941dd 100644 --- a/packages/adapter-utils/src/acpx-engine/startup-timing.ts +++ b/packages/adapter-utils/src/acpx-engine/startup-timing.ts @@ -9,37 +9,83 @@ import type { AdapterExecutionContext, AdapterRuntimeEvent } from "../types.js"; */ export const RUN_STARTUP_STEP_EVENT_TYPE = "run.startup.step"; -function buildStepEvent(step: string, durationMs: number): AdapterRuntimeEvent { +/** + * Optional per-step attribution attached to a `run.startup.step` event, all + * additive to the free-form jsonb payload (no schema change). Each reader is a + * plain `() => number` closure so the timing helper stays decoupled from the + * runner/provider it reads (Risk R1): `measureStartupStep` snapshots the reader + * before `fn` and again in `finally`, emitting the delta. + * + * - `roundTrips` — cumulative host→sandbox `runner.execute` count; the delta is + * how many round-trips the step performed (Open Q1, host boundary). + * - `providerExecMs` / `providerGetMs` — cumulative provider-reported wall-time + * (ms) for the `executeCommand` REST call vs the `client.get` sandbox + * re-fetch; the delta attributes the step's round-trip time to its parts + * (Open Q1, finer provider attribution). + * - `extra` — a reader (called once in `finally`, after `fn` settles) returning + * any additional numeric fields to merge into the payload; used by + * `acp.handshake` to carry its `createRuntimeMs` / `ensureSessionMs` sub-split + * (Open Q2), which are measured by the caller rather than read from a counter. + */ +export interface StartupStepMeasureOptions { + roundTrips?: () => number; + providerExecMs?: () => number; + providerGetMs?: () => number; + extra?: () => Record; +} + +function buildStepEvent(payload: Record): AdapterRuntimeEvent { + const step = String(payload.step); + const durationMs = payload.durationMs as number; return { eventType: RUN_STARTUP_STEP_EVENT_TYPE, stream: "system", level: "info", message: `startup step: ${step} (${durationMs}ms)`, - payload: { step, durationMs }, + payload, }; } /** * Time `fn` with the injected `now` clock and emit exactly one - * `run.startup.step` event carrying `{ step, durationMs }`. The event fires in a - * `finally`, so a throwing step still reports its duration before the error is - * re-thrown. `now` is injected (never `Date.now()` here) so callers/tests stay - * deterministic, and `ctx.onEvent` is optional — a missing sink is a no-op that - * neither throws nor swallows `fn`'s return value or error. + * `run.startup.step` event carrying `{ step, durationMs }` plus any counters + * supplied via `options`. The event fires in a `finally`, so a throwing step + * still reports its duration before the error is re-thrown. `now` is injected + * (never `Date.now()` here) so callers/tests stay deterministic, and + * `ctx.onEvent` is optional — a missing sink is a no-op that neither throws nor + * swallows `fn`'s return value or error. A step skipped by a warm cache never + * calls this helper, so it emits no event (never a zero). */ export async function measureStartupStep( ctx: Pick, now: () => number, step: string, fn: () => Promise, + options: StartupStepMeasureOptions = {}, ): Promise { const start = now(); + const roundTripsStart = options.roundTrips?.(); + const providerExecStart = options.providerExecMs?.(); + const providerGetStart = options.providerGetMs?.(); try { return await fn(); } finally { const durationMs = now() - start; + const payload: Record = { step, durationMs }; + if (options.roundTrips) { + payload.roundTrips = options.roundTrips() - (roundTripsStart ?? 0); + } + if (options.providerExecMs) { + payload.providerExecMs = options.providerExecMs() - (providerExecStart ?? 0); + } + if (options.providerGetMs) { + payload.providerGetMs = options.providerGetMs() - (providerGetStart ?? 0); + } + if (options.extra) { + Object.assign(payload, options.extra()); + } try { - await ctx.onEvent?.(buildStepEvent(step, durationMs)); + await ctx.onEvent?.(buildStepEvent(payload)); } catch { // Observability must not change startup control flow. } diff --git a/packages/adapter-utils/src/command-managed-runtime.ts b/packages/adapter-utils/src/command-managed-runtime.ts index 0e93ebb1751..399eca94796 100644 --- a/packages/adapter-utils/src/command-managed-runtime.ts +++ b/packages/adapter-utils/src/command-managed-runtime.ts @@ -20,6 +20,25 @@ export interface CommandManagedRuntimeRunner { * and let the caller choose a chunked upload path when progress is requested. */ supportsSingleStreamStdinProgress?: boolean; + /** + * Cumulative count of host→sandbox `execute` round-trips this runner has + * performed (Open Q1). Present only on runners that instrument the single + * exec seam (the sandbox runner); the per-step delta is emitted as + * `run.startup.step` `payload.roundTrips`. A `() => number` reader, never the + * runner itself, is threaded into `measureStartupStep` so the timing helper + * stays runner-agnostic. + */ + execCount?(): number; + /** + * Cumulative provider-reported wall-time (ms) for the `executeCommand` REST + * call ({@link providerExecMs}) vs the `client.get` sandbox re-fetch that + * precedes it ({@link providerGetMs}), accumulated across every `execute` + * round-trip (Open Q1, finer attribution). Present only when the provider + * surfaces these durations on its result metadata; the per-step deltas are + * emitted as `payload.providerExecMs` / `payload.providerGetMs`. + */ + providerExecMs?(): number; + providerGetMs?(): number; execute(input: { command: string; args?: string[]; diff --git a/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts b/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts index 999ca869a76..4fb9a13a2f0 100644 --- a/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts +++ b/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts @@ -28,7 +28,7 @@ vi.mock("@daytonaio/sdk", () => ({ DaytonaTimeoutError: MockDaytonaTimeoutError, })); -import plugin from "./plugin.js"; +import plugin, { setDaytonaTimingClockForTest } from "./plugin.js"; import manifest from "./manifest.js"; function createMockSandbox(overrides: { @@ -1098,12 +1098,49 @@ describe("Daytona sandbox provider plugin", () => { expect(cwdArg).toBeUndefined(); expect(envArg).toBeUndefined(); expect(timeoutArg).toBe(1); - expect(result).toEqual({ + expect(result).toMatchObject({ exitCode: 7, timedOut: false, stdout: "stdout\nstderr\n", stderr: "", }); + // Provider-boundary timings ride the free-form result metadata (Open Q1). + expect(typeof (result!.metadata as Record)?.durationMs).toBe("number"); + expect(typeof (result!.metadata as Record)?.getDurationMs).toBe("number"); + }); + + it("reports provider executeCommand and client.get durations via result metadata (injected clock)", async () => { + process.env.DAYTONA_API_KEY = "host-key"; + const sandbox = createMockSandbox(); + sandbox.process.executeCommand.mockResolvedValue({ + exitCode: 0, + result: "ok", + artifacts: { stdout: "ok" }, + }); + mockGet.mockResolvedValue(sandbox); + + // Deterministic clock: getSandbox spans 40ms, executeCommand spans 600ms. + // Call order across the execute path is: getStart, getEnd, execStart, execEnd. + const ticks = [1000, 1040, 1040, 1640]; + let i = 0; + const restoreClock = setDaytonaTimingClockForTest(() => ticks[Math.min(i++, ticks.length - 1)]!); + try { + const result = await plugin.definition.onEnvironmentExecute?.({ + driverKey: "daytona", + companyId: "company-1", + environmentId: "env-1", + config: { timeoutMs: 300000, reuseLease: false }, + lease: { providerLeaseId: "sandbox-123", metadata: {} }, + command: "printf", + args: ["hello"], + cwd: "/workspace", + timeoutMs: 1000, + }); + + expect(result!.metadata).toMatchObject({ durationMs: 600, getDurationMs: 40 }); + } finally { + restoreClock(); + } }); it("stages stdin in the sandbox filesystem when execution needs redirected input", async () => { @@ -1172,27 +1209,43 @@ describe("Daytona sandbox provider plugin", () => { sandbox.process.executeCommand.mockRejectedValue(new MockDaytonaTimeoutError("command timed out")); mockGet.mockResolvedValue(sandbox); - const result = await plugin.definition.onEnvironmentExecute?.({ - driverKey: "daytona", - companyId: "company-1", - environmentId: "env-1", - config: { - timeoutMs: 300000, - reuseLease: false, - }, - lease: { providerLeaseId: "sandbox-123", metadata: {} }, - command: "sleep", - args: ["60"], - cwd: "/workspace", - timeoutMs: 1000, - }); + // Injected clock: getStart=0, getEnd=10 (getDurationMs=10), execStart=20, + // execEnd/timeout=1520 (durationMs=1500). Deterministic so the timeout path's + // provider-exec attribution is asserted exactly. + const ticks = [0, 10, 20, 1520]; + let i = 0; + const restoreClock = setDaytonaTimingClockForTest(() => ticks[Math.min(i++, ticks.length - 1)]!); - expect(result).toEqual({ + let result; + try { + result = await plugin.definition.onEnvironmentExecute?.({ + driverKey: "daytona", + companyId: "company-1", + environmentId: "env-1", + config: { + timeoutMs: 300000, + reuseLease: false, + }, + lease: { providerLeaseId: "sandbox-123", metadata: {} }, + command: "sleep", + args: ["60"], + cwd: "/workspace", + timeoutMs: 1000, + }); + } finally { + restoreClock(); + } + + expect(result).toMatchObject({ exitCode: null, timedOut: true, stdout: "", stderr: "command timed out\n", }); + // The exec reached executeCommand before timing out, so its wall-time is + // preserved as durationMs (provider-exec attribution is not dropped on the + // timeout path); the getSandbox re-fetch duration is also reported. + expect(result!.metadata).toMatchObject({ durationMs: 1500, getDurationMs: 10 }); }); it("injects noninteractive git credential defaults for every one-shot command", async () => { diff --git a/packages/plugins/sandbox-providers/daytona/src/plugin.ts b/packages/plugins/sandbox-providers/daytona/src/plugin.ts index 83e7696a2e3..a2a0edd7b5c 100644 --- a/packages/plugins/sandbox-providers/daytona/src/plugin.ts +++ b/packages/plugins/sandbox-providers/daytona/src/plugin.ts @@ -39,6 +39,24 @@ import type { } from "@paperclipai/plugin-sdk"; import { performSyncIn, performSyncOut } from "./file-sync.js"; +// Injectable monotonic clock for provider-boundary timing (Open Q1). Defaults +// to the real wall clock; `plugin.test.ts` overrides it via +// `setDaytonaTimingClockForTest` so the measured `durationMs`/`getDurationMs` +// are deterministic. The timing path never calls `Date.now()` directly. +let timingNow: () => number = () => Date.now(); + +/** + * Test seam: override the provider-timing clock and return a restore function. + * Not used in production, where the default wall clock always applies. + */ +export function setDaytonaTimingClockForTest(now: () => number): () => void { + const previous = timingNow; + timingNow = now; + return () => { + timingNow = previous; + }; +} + interface DaytonaDriverConfig { apiKey: string | null; apiUrl: string | null; @@ -743,6 +761,13 @@ async function executeOneShot( const timeoutSeconds = toTimeoutSeconds(effectiveTimeoutMs); const stdinPath = params.stdin != null ? `/tmp/paperclip-stdin-${randomUUID()}` : null; + // Marks the start of the `executeCommand` REST round-trip. Hoisted out of the + // try so the timeout path below can still attribute the exec wall-time it spent + // before the SDK aborted — a slow failed exec is exactly what we want to + // measure. Stays null until we are about to call `executeCommand`, so a timeout + // during the earlier `uploadFile` step honestly reports no `durationMs`. + let execStart: number | null = null; + try { if (stdinPath) { await sandbox.fs.uploadFile(Buffer.from(params.stdin ?? "", "utf8"), stdinPath, timeoutSeconds); @@ -760,24 +785,36 @@ async function executeOneShot( // profile sourcing when params.cwd is set, and the Daytona executor's own // cwd argument runs before our login-shell init, which is the wrong order // (env from .bashrc would override caller env). + // Time only the `executeCommand` REST round-trip (Open Q1) — the ~600ms + // login-shell wrapper — so the caller can attribute a step's exec time to + // the provider boundary via the free-form `metadata.durationMs`. + execStart = timingNow(); const result = await sandbox.process.executeCommand(command, undefined, undefined, timeoutSeconds); + const durationMs = timingNow() - execStart; return { exitCode: typeof result.exitCode === "number" ? result.exitCode : 1, timedOut: false, stdout: result.result ?? result.artifacts?.stdout ?? "", stderr: "", + metadata: { durationMs }, }; } catch (error) { if (error instanceof DaytonaTimeoutError) { const timeoutMessage = gitNet ? `Git network operation timed out after ${Math.round(effectiveTimeoutMs / 1000)} s — the remote may be unreachable or noninteractive credentials are not configured.` : error.message.trim(); + // Preserve provider-boundary exec attribution on the timeout path: if the + // SDK aborted the `executeCommand` call itself, report how long it ran + // before timing out so slow failed startup exec is attributed to the + // provider, not silently dropped. + const durationMs = execStart != null ? timingNow() - execStart : undefined; return { exitCode: null, timedOut: true, stdout: "", stderr: `${timeoutMessage}\n`, + ...(durationMs != null ? { metadata: { durationMs } } : {}), }; } throw error; @@ -1256,9 +1293,19 @@ const plugin = definePlugin({ } const config = parseDriverConfig(params.config); + // Time the `client.get` sandbox re-fetch (Open Q1) separately from the + // `executeCommand` round-trip so telemetry can split the per-call REST-get + // cost from the exec cost. `ensureSandboxStarted` is a no-op for an + // already-started sandbox, so it is excluded from the get measurement. + const getStart = timingNow(); const sandbox = await getSandbox(config, params.lease.providerLeaseId); + const getDurationMs = timingNow() - getStart; await ensureSandboxStarted(sandbox, toTimeoutSeconds(resolveTimeoutMs(params.timeoutMs, config))); - return await executeOneShot(sandbox, params, config); + const result = await executeOneShot(sandbox, params, config); + return { + ...result, + metadata: { ...(result.metadata ?? {}), getDurationMs }, + }; }, // Opt-in native inbound transfer. Defining this hook (with onEnvironmentSyncOut) diff --git a/server/src/__tests__/environment-execution-target.test.ts b/server/src/__tests__/environment-execution-target.test.ts index a627f105cab..dadcea97d94 100644 --- a/server/src/__tests__/environment-execution-target.test.ts +++ b/server/src/__tests__/environment-execution-target.test.ts @@ -221,4 +221,99 @@ describe("resolveEnvironmentExecutionTarget", () => { }); expect(target).not.toHaveProperty("paperclipApiUrl"); }); + + it("exposes a sandbox runner that counts round-trips and accumulates provider durations", async () => { + mockResolveEnvironmentDriverConfigForRuntime.mockResolvedValue({ + driver: "sandbox", + config: { + provider: "fake-plugin", + reuseLease: false, + timeoutMs: 30_000, + }, + }); + + // Each exec reports its provider-boundary durations on the free-form result + // metadata (the Daytona plugin does this); the runner accumulates them. + const environmentRuntime = { + execute: vi.fn().mockResolvedValue({ + exitCode: 0, + signal: null, + timedOut: false, + stdout: "ok", + stderr: "", + metadata: { durationMs: 600, getDurationMs: 15 }, + }), + supportsSync: vi.fn().mockReturnValue(false), + }; + + const target = await resolveEnvironmentExecutionTarget({ + db: {} as never, + companyId: "company-1", + adapterType: "codex_local", + environment: { id: "env-1", driver: "sandbox", config: { provider: "fake-plugin" } }, + leaseId: "lease-1", + leaseMetadata: { remoteCwd: "/workspace" }, + lease: { id: "lease-1" } as never, + environmentRuntime: environmentRuntime as never, + }); + + const runner = (target as { runner?: { + execCount(): number; + providerExecMs(): number; + providerGetMs(): number; + execute(input: { command: string; args?: string[] }): Promise; + } }).runner; + expect(runner).toBeTruthy(); + expect(runner!.execCount()).toBe(0); + expect(runner!.providerExecMs()).toBe(0); + expect(runner!.providerGetMs()).toBe(0); + + await runner!.execute({ command: "echo", args: ["a"] }); + await runner!.execute({ command: "echo", args: ["b"] }); + + expect(runner!.execCount()).toBe(2); + expect(runner!.providerExecMs()).toBe(1200); + expect(runner!.providerGetMs()).toBe(30); + }); + + it("tolerates a provider result with no timing metadata (counts the round-trip, accumulates nothing)", async () => { + mockResolveEnvironmentDriverConfigForRuntime.mockResolvedValue({ + driver: "sandbox", + config: { provider: "fake-plugin", reuseLease: false, timeoutMs: 30_000 }, + }); + + const environmentRuntime = { + execute: vi.fn().mockResolvedValue({ + exitCode: 0, + signal: null, + timedOut: false, + stdout: "", + stderr: "", + }), + supportsSync: vi.fn().mockReturnValue(false), + }; + + const target = await resolveEnvironmentExecutionTarget({ + db: {} as never, + companyId: "company-1", + adapterType: "codex_local", + environment: { id: "env-1", driver: "sandbox", config: { provider: "fake-plugin" } }, + leaseId: "lease-1", + leaseMetadata: { remoteCwd: "/workspace" }, + lease: { id: "lease-1" } as never, + environmentRuntime: environmentRuntime as never, + }); + + const runner = (target as { runner?: { + execCount(): number; + providerExecMs(): number; + providerGetMs(): number; + execute(input: { command: string }): Promise; + } }).runner; + await runner!.execute({ command: "echo" }); + + expect(runner!.execCount()).toBe(1); + expect(runner!.providerExecMs()).toBe(0); + expect(runner!.providerGetMs()).toBe(0); + }); }); diff --git a/server/src/services/environment-execution-target.ts b/server/src/services/environment-execution-target.ts index bbd86d8ce5a..e609646ef4f 100644 --- a/server/src/services/environment-execution-target.ts +++ b/server/src/services/environment-execution-target.ts @@ -63,6 +63,20 @@ export async function resolveEnvironmentExecutionTarget(input: { ? input.leaseMetadata.shellCommand : null; + // Per-lease-runner cumulative counters for startup-step attribution (Open + // Q1). Closed over by the `runner.execute` seam below and read back as + // deltas by `measureStartupStep`. + let execCount = 0; + let providerExecMs = 0; + let providerGetMs = 0; + const accumulateProviderDurations = (metadata: Record | undefined): void => { + if (!metadata) return; + const exec = metadata.durationMs; + const get = metadata.getDurationMs; + if (typeof exec === "number" && Number.isFinite(exec)) providerExecMs += exec; + if (typeof get === "number" && Number.isFinite(get)) providerGetMs += get; + }; + return { kind: "remote", transport: "sandbox", @@ -79,7 +93,17 @@ export async function resolveEnvironmentExecutionTarget(input: { runner: input.environmentRuntime && input.lease ? { supportsSingleStreamStdinProgress: false, + // Round-trip counter + provider-duration accumulators on the single + // host→sandbox exec seam (Open Q1). `measureStartupStep` reads the + // per-step delta of each via the `() => number` closures below. The + // provider durations ride the exec result's free-form `metadata` + // (set by the Daytona plugin), so no protocol/schema change is + // needed and providers that omit them simply accumulate nothing. + execCount: () => execCount, + providerExecMs: () => providerExecMs, + providerGetMs: () => providerGetMs, execute: async (commandInput) => { + execCount += 1; const startedAt = new Date().toISOString(); const result = await input.environmentRuntime!.execute({ environment: input.environment as Environment, @@ -91,6 +115,7 @@ export async function resolveEnvironmentExecutionTarget(input: { stdin: commandInput.stdin, timeoutMs: commandInput.timeoutMs, }); + accumulateProviderDurations(result.metadata); if (result.stdout) await commandInput.onLog?.("stdout", result.stdout); if (result.stderr) await commandInput.onLog?.("stderr", result.stderr); return { From 273315a4d01a002cc4ecc5e9e7192ad7a32e91b6 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Mon, 27 Jul 2026 12:00:21 -0700 Subject: [PATCH 02/87] feat(server): provision managed sandbox environments from the managed config (#10324) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Hosted/managed deployments configure instances entirely from the control plane: `PAPERCLIP_MANAGED_CONFIG` already delivers feature flags and `plugins.autoInstall` (bundled sandbox provider plugins), parsed fail-closed at boot > - A plugin alone is not usable for execution: runs need an instance-level `driver: "sandbox"` environment row pointing at the provider, and today only Kubernetes has a boot path for that (`PAPERCLIP_EXECUTION_MODE` → `ensureKubernetesEnvironment`); every other provider requires a manual product-API call the control plane cannot make on a managed instance > - Adding one `ensureXxxEnvironment` per provider would multiply near-identical boot hooks and env-var surfaces > - This pull request generalizes the existing Kubernetes machinery: the managed-config document gains an optional `environments` section that declares a sandbox environment for any bundled provider, ensured idempotently at boot by a provider-agnostic service function (the Kubernetes hook becomes a thin wrapper over it) > - The benefit is that a managed fleet can provision any sandbox provider (Daytona, Modal, E2B, …) purely from configuration — no per-provider code, no manual API calls, no secrets in the document — while self-hosted behavior is untouched ## Linked Issues or Issue Description No public issue exists. Refs #10157 (the cloud image variant that bundles sandbox provider plugins — this PR is the configuration half that makes an installed provider usable). **Problem (feature-request shape):** on a managed instance the control plane can auto-install a bundled sandbox provider plugin via `PAPERCLIP_MANAGED_CONFIG.plugins.autoInstall`, but cannot create the environment row that makes the provider schedulable. The only boot-time environment provisioning is Kubernetes-specific (`PAPERCLIP_EXECUTION_MODE=kubernetes` + `PAPERCLIP_K8S_*`). A generic, config-driven path is needed so any bundled provider can be provisioned without per-plugin code or manual API calls. ## What Changed - `server/src/services/managed-config.ts`: optional `environments` top-level section — `[{ name, description?, provider, config? }]` — validated fail-closed: unknown keys, more than one entry (the DB permits exactly one Paperclip-managed sandbox row, `environments_managed_sandbox_idx`), a `provider` not present in `plugins.autoInstall`, `config.provider`, or secret-looking config keys at any depth (`api_key`/`token`/`secret`/`password`/`credential`) all refuse startup. Absent section ⇒ `environments: []`, so pre-section documents keep booting newer builds. - `server/src/services/environments.ts`: new provider-agnostic `ensureManagedSandboxEnvironment({ name, description?, provider, config?, extraMetadata? })` — idempotently owns the single managed sandbox row: refreshes name/description/config each call, adopts the slot across provider switches (dropping the stale `managedKubernetesSandbox` marker), adopts a same-name unmanaged sandbox row (stamping it managed) instead of colliding on `environments_name_idx` every boot, and falls back to keeping the current name if the desired name belongs to a different row. `ensureKubernetesEnvironment` is now a thin wrapper that pins `provider: "kubernetes"` and stamps the legacy marker. - `server/src/services/managed-environments.ts` (new): `applyManagedEnvironments` boot step — no-op for self-hosted/empty; throws (fail startup) when `PAPERCLIP_EXECUTION_MODE` is also set, since both would own the same managed sandbox row; otherwise ensures each declared environment fail-safe per entry (log + continue boot, matching bundled-plugin provisioning posture). - `server/src/index.ts`: runs the new boot step right after the execution-policy bootstrap, before the heartbeat resumes queued runs. - `server/src/services/index.ts`: exports `applyManagedEnvironments` and `ManagedEnvironmentSpec`. - Secrets stay out of the document by construction: provider credentials reach managed instances only as process env vars (each provider's documented fallback, e.g. `DAYTONA_API_KEY` for the Daytona plugin). ## Verification ```sh cd server pnpm exec tsc --noEmit -p tsconfig.json pnpm exec vitest run \ src/__tests__/managed-config.test.ts \ src/services/managed-environments.test.ts \ src/services/execution-policy-bootstrap.test.ts \ src/__tests__/environment-service.test.ts \ src/__tests__/environment-instance-routes.test.ts \ src/__tests__/environment-routes.test.ts \ src/__tests__/plugin-install-guard.test.ts \ src/__tests__/environment-execution-target.test.ts \ src/__tests__/instance-settings-managed-overlay.test.ts \ src/__tests__/bundled-plugins.test.ts ``` All pass locally (typecheck clean; environment-service suite runs against embedded Postgres and exercises the refactored Kubernetes wrapper plus the new generic ensure: create/refresh, provider switch, unmanaged-row adoption, name-conflict fallback). New tests cover the parser (12 cases incl. secret-key rejection at depth) and the boot step (no-op, mutual exclusion, pass-through, fail-safe). ## Risks - **Self-hosted: none intended.** Without `PAPERCLIP_MANAGED_CONFIG` nothing new executes; the `PAPERCLIP_EXECUTION_MODE=kubernetes` path is regression-covered by the existing bootstrap/service/route suites (all green). - **Behavioral shift in `ensureKubernetesEnvironment` (deliberate):** it now also refreshes `name`/`description` to their managed defaults each boot (desired-state semantics, same as config today) and adopts a `managedByPaperclip` sandbox row that lacks the Kubernetes marker — previously that state made the ensure throw every boot. - **New startup failure modes are all explicit misconfigurations** (malformed section, provider not auto-installed, secret in config, execution-mode conflict) and fail with precise errors; DB-side ensure failures never block boot (fail-safe per entry, logged). - No migrations; no API surface changes. ## Model Used Claude Fable 5 (Anthropic, model ID `claude-fable-5`) with extended thinking and tool use, driving the change end-to-end inside a Claude Code / agent-harness session (code, tests, and verification runs). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes (the managed-config module header is the contract doc) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --- .../src/__tests__/environment-service.test.ts | 141 ++++++ server/src/__tests__/managed-config.test.ts | 111 +++++ .../server-startup-feedback-export.test.ts | 1 + server/src/app.ts | 9 +- server/src/index.ts | 26 ++ server/src/services/environments.ts | 301 +++++++++---- server/src/services/index.ts | 2 + server/src/services/managed-config.ts | 136 +++++- .../src/services/managed-environments.test.ts | 415 ++++++++++++++++++ server/src/services/managed-environments.ts | 239 ++++++++++ 10 files changed, 1282 insertions(+), 99 deletions(-) create mode 100644 server/src/services/managed-environments.test.ts create mode 100644 server/src/services/managed-environments.ts diff --git a/server/src/__tests__/environment-service.test.ts b/server/src/__tests__/environment-service.test.ts index eff92293fd2..4b3ef48810e 100644 --- a/server/src/__tests__/environment-service.test.ts +++ b/server/src/__tests__/environment-service.test.ts @@ -630,6 +630,147 @@ describeEmbeddedPostgres("environmentService leases", () => { expect((rows[0]?.metadata as Record)?.managedKubernetesSandbox).toBe(true); }); + it("ensures and refreshes a managed sandbox environment for an arbitrary provider", async () => { + const created = await svc.ensureManagedSandboxEnvironment({ + name: "Daytona", + description: "Managed Daytona sandbox environment.", + provider: "daytona", + config: { target: "us" }, + }); + + expect(created.driver).toBe("sandbox"); + expect(created.name).toBe("Daytona"); + expect(created.config.provider).toBe("daytona"); + expect(created.config.target).toBe("us"); + expect(created.metadata?.managedByPaperclip).toBe(true); + expect(created.metadata?.managedSandboxProvider).toBe("daytona"); + + // Idempotent: a second call refreshes config and name in place, and a + // description omitted from the spec is cleared, not pinned forever. + const refreshed = await svc.ensureManagedSandboxEnvironment({ + name: "Daytona (EU)", + provider: "daytona", + config: { target: "eu" }, + }); + expect(refreshed.id).toBe(created.id); + expect(refreshed.name).toBe("Daytona (EU)"); + expect(refreshed.config.target).toBe("eu"); + expect(refreshed.description).toBeNull(); + + const rows = await db + .select() + .from(environments) + .where(eq(environments.driver, "sandbox")); + expect(rows).toHaveLength(1); + }); + + it("adopts the managed slot on a provider switch and drops the stale kubernetes marker", async () => { + const kubernetes = await svc.ensureKubernetesEnvironment({ inCluster: true, backend: "job" }); + expect(kubernetes.metadata?.managedKubernetesSandbox).toBe(true); + + const daytona = await svc.ensureManagedSandboxEnvironment({ + name: "Daytona", + provider: "daytona", + config: { target: "us" }, + }); + + expect(daytona.id).toBe(kubernetes.id); + expect(daytona.name).toBe("Daytona"); + expect(daytona.config.provider).toBe("daytona"); + expect(daytona.config.backend).toBeUndefined(); + expect(daytona.metadata?.managedSandboxProvider).toBe("daytona"); + expect(daytona.metadata?.managedKubernetesSandbox).toBeUndefined(); + expect(await svc.findKubernetesEnvironment()).toBeNull(); + + // And back: the kubernetes wrapper re-adopts the same row. + const restored = await svc.ensureKubernetesEnvironment({ inCluster: true, backend: "job" }); + expect(restored.id).toBe(kubernetes.id); + expect(restored.metadata?.managedKubernetesSandbox).toBe(true); + }); + + it("archives the managed sandbox row only for its own provider and reactivates on ensure", async () => { + // Nothing provisioned yet: archiving is a no-op. + expect(await svc.archiveManagedSandboxEnvironment({ provider: "daytona" })).toBeNull(); + + const created = await svc.ensureManagedSandboxEnvironment({ + name: "Daytona", + provider: "daytona", + config: { target: "us" }, + }); + expect(created.status).toBe("active"); + + // Another provider's unavailability leaves this provider's row alone. + expect(await svc.archiveManagedSandboxEnvironment({ provider: "kubernetes" })).toBeNull(); + + const archived = await svc.archiveManagedSandboxEnvironment({ provider: "daytona" }); + expect(archived?.id).toBe(created.id); + expect(archived?.status).toBe("archived"); + + // Already archived: a repeat call is a no-op. + expect(await svc.archiveManagedSandboxEnvironment({ provider: "daytona" })).toBeNull(); + + // The next healthy boot's ensure re-activates the same row. + const restored = await svc.ensureManagedSandboxEnvironment({ + name: "Daytona", + provider: "daytona", + config: { target: "us" }, + }); + expect(restored.id).toBe(created.id); + expect(restored.status).toBe("active"); + }); + + it("adopts an existing unmanaged sandbox row holding the desired name", async () => { + const handMade = await svc.create({ + name: "Daytona", + driver: "sandbox", + status: "active", + config: { provider: "daytona", target: "us" }, + }); + expect(handMade.metadata?.managedByPaperclip).toBeUndefined(); + + const adopted = await svc.ensureManagedSandboxEnvironment({ + name: "Daytona", + provider: "daytona", + config: { target: "eu" }, + }); + expect(adopted.id).toBe(handMade.id); + expect(adopted.config.target).toBe("eu"); + expect(adopted.metadata?.managedByPaperclip).toBe(true); + expect(adopted.metadata?.managedSandboxProvider).toBe("daytona"); + + const rows = await db + .select() + .from(environments) + .where(eq(environments.driver, "sandbox")); + expect(rows).toHaveLength(1); + }); + + it("keeps the current name when the desired name belongs to another row", async () => { + await svc.create({ + name: "Daytona", + driver: "ssh", + status: "active", + config: { + host: "fixture.example.test", + port: 22, + username: "fixture", + remoteWorkspacePath: "/srv/paperclip", + }, + }); + const kubernetes = await svc.ensureKubernetesEnvironment({ inCluster: true }); + + // The managed slot is adopted, but the rename would collide with the ssh + // row on environments_name_idx; the ensure keeps the existing name. + const adopted = await svc.ensureManagedSandboxEnvironment({ + name: "Daytona", + provider: "daytona", + config: { target: "us" }, + }); + expect(adopted.id).toBe(kubernetes.id); + expect(adopted.name).toBe(kubernetes.name); + expect(adopted.config.provider).toBe("daytona"); + }); + it("returns a conflict when creating a second environment with the same name", async () => { await seedEnvironment(); diff --git a/server/src/__tests__/managed-config.test.ts b/server/src/__tests__/managed-config.test.ts index 1717b332884..370999e348c 100644 --- a/server/src/__tests__/managed-config.test.ts +++ b/server/src/__tests__/managed-config.test.ts @@ -53,6 +53,7 @@ describe("parseManagedConfigEnv", () => { catalogVersion: "2026.720.0", features: { enableApps: false, enablePipelines: true }, plugins: { autoInstall: ["daytona", "kubernetes"] }, + environments: [], }); }); @@ -66,6 +67,7 @@ describe("parseManagedConfigEnv", () => { catalogVersion: "2026.720.0", features: {}, plugins: { autoInstall: [] }, + environments: [], }); }); @@ -223,6 +225,115 @@ describe("parseManagedConfigEnv", () => { }); }); +describe("parseManagedConfigEnv environments section", () => { + const entry = (overrides: Record = {}) => ({ + name: "Daytona", + provider: "daytona", + config: { target: "us" }, + ...overrides, + }); + + it("defaults to an empty list when the section is absent (pre-section documents keep booting)", () => { + const config = parseManagedConfigEnv(envWith(validDoc())); + expect(config?.environments).toEqual([]); + }); + + it("parses a declared environment and freezes it", () => { + const config = parseManagedConfigEnv( + envWith(validDoc({ environments: [entry({ description: "Managed Daytona sandbox." })] })), + ); + expect(config?.environments).toHaveLength(1); + const spec = config?.environments[0]; + expect(spec?.name).toBe("Daytona"); + expect(spec?.description).toBe("Managed Daytona sandbox."); + expect(spec?.provider).toBe("daytona"); + expect(spec?.config).toEqual({ target: "us" }); + expect(Object.isFrozen(config?.environments)).toBe(true); + expect(Object.isFrozen(spec)).toBe(true); + expect(Object.isFrozen(spec?.config)).toBe(true); + }); + + it("treats config and description as optional", () => { + const config = parseManagedConfigEnv( + envWith(validDoc({ environments: [{ name: "Daytona", provider: "daytona" }] })), + ); + expect(config?.environments[0]?.config).toEqual({}); + expect(config?.environments[0]?.description).toBeUndefined(); + }); + + it("rejects a non-array section and non-object entries", () => { + expect(() => + parseManagedConfigEnv(envWith(validDoc({ environments: {} }))), + ).toThrow(/"environments" must be an array/); + expect(() => + parseManagedConfigEnv(envWith(validDoc({ environments: ["daytona"] }))), + ).toThrow(/"environments\[0\]" must be an object/); + }); + + it("rejects more than one entry (single managed sandbox slot)", () => { + expect(() => + parseManagedConfigEnv( + envWith(validDoc({ environments: [entry(), entry({ name: "Other", provider: "kubernetes" })] })), + ), + ).toThrow(/at most one entry/); + }); + + it("rejects unknown entry keys", () => { + expect(() => + parseManagedConfigEnv(envWith(validDoc({ environments: [entry({ envVars: {} })] }))), + ).toThrow(/unknown key "envVars"/); + }); + + it("rejects malformed names, descriptions, and providers", () => { + expect(() => + parseManagedConfigEnv(envWith(validDoc({ environments: [entry({ name: "" })] }))), + ).toThrow(/"environments\[0\].name"/); + expect(() => + parseManagedConfigEnv(envWith(validDoc({ environments: [entry({ name: " Daytona" })] }))), + ).toThrow(/"environments\[0\].name"/); + expect(() => + parseManagedConfigEnv(envWith(validDoc({ environments: [entry({ description: " " })] }))), + ).toThrow(/"environments\[0\].description"/); + expect(() => + parseManagedConfigEnv(envWith(validDoc({ environments: [entry({ provider: 7 })] }))), + ).toThrow(/"environments\[0\].provider"/); + }); + + it("rejects a provider that plugins.autoInstall does not provision", () => { + expect(() => + parseManagedConfigEnv(envWith(validDoc({ environments: [entry({ provider: "modal" })] }))), + ).toThrow(/not in "plugins.autoInstall"/); + }); + + it("rejects a config that sets provider", () => { + expect(() => + parseManagedConfigEnv( + envWith(validDoc({ environments: [entry({ config: { provider: "daytona" } })] })), + ), + ).toThrow(/must not set "provider"/); + }); + + it("rejects secret-bearing config keys at any depth (secrets travel as env vars)", () => { + expect(() => + parseManagedConfigEnv( + envWith(validDoc({ environments: [entry({ config: { apiKey: "not-a-real-key" } })] })), + ), + ).toThrow(/looks secret-bearing/); + expect(() => + parseManagedConfigEnv( + envWith(validDoc({ environments: [entry({ config: { auth: { accessToken: "t" } } })] })), + ), + ).toThrow(/auth.accessToken/); + expect(() => + parseManagedConfigEnv( + envWith( + validDoc({ environments: [entry({ config: { adapters: [{ clientSecret: "s" }] } })] }), + ), + ), + ).toThrow(/adapters\[0\].clientSecret/); + }); +}); + describe("getManagedInstanceConfig", () => { it("caches by raw env value and reparses when it changes", () => { const raw = validDoc(); diff --git a/server/src/__tests__/server-startup-feedback-export.test.ts b/server/src/__tests__/server-startup-feedback-export.test.ts index f7a277fd71e..d309533dde4 100644 --- a/server/src/__tests__/server-startup-feedback-export.test.ts +++ b/server/src/__tests__/server-startup-feedback-export.test.ts @@ -206,6 +206,7 @@ vi.mock("../services/index.js", () => ({ })), feedbackService: feedbackServiceFactoryMock, bootstrapExecutionPolicyFromEnv: vi.fn(async () => null), + applyManagedEnvironments: vi.fn(async () => null), environmentCustomImageService: environmentCustomImagesServiceFactoryMock, heartbeatService: heartbeatServiceFactoryMock, issueService: vi.fn(() => ({ update: vi.fn(async () => null) })), diff --git a/server/src/app.ts b/server/src/app.ts index d6f68296846..5eeeb751c0b 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -590,7 +590,13 @@ export async function createApp( // swallowed per plugin so the server ALWAYS finishes booting. A degraded // boot (a provider unavailable, some agents cannot run) is strictly // preferable to a crash loop. - void ensureBundledPlugins( + // + // The chain is not awaited here (createApp stays fast), but the settled + // promise is exposed via `app.locals.bundledPluginsStartup` so boot steps + // that must not outrun plugin availability — managed sandbox environments + // (`applyManagedEnvironments`) run before the heartbeat resumes queued + // runs — can sequence on it. It never rejects. + const bundledPluginsStartup = ensureBundledPlugins( bundledPluginInstalls, { registry: pluginRegistry, loader, lifecycle, logger }, // Managed mode reinstalls soft-uninstalled bundles (the control plane @@ -609,6 +615,7 @@ export async function createApp( }).catch((err) => { logger.error({ err }, "Failed to load ready plugins on startup"); }); + app.locals.bundledPluginsStartup = bundledPluginsStartup; let appServicesShutdown = false; const shutdownAppServices = () => { if (appServicesShutdown) return; diff --git a/server/src/index.ts b/server/src/index.ts index 8161c2cd375..3908d1cfc1a 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -41,6 +41,7 @@ import { setupEnvironmentCustomImageTerminalWebSocketServer } from "./realtime/e import { setupLiveEventsWebSocketServer } from "./realtime/live-events-ws.js"; import { feedbackService, + applyManagedEnvironments, backfillPrincipalAccessCompatibility, backfillLegacyToolOAuthTokens, bootstrapExecutionPolicyFromEnv, @@ -860,6 +861,31 @@ export async function startServer(): Promise { throw err; } + // Ensure sandbox environments declared in the managed-config document + // (`environments` section) before the heartbeat resumes queued runs. The + // document already parsed fail-closed above; the ensure step itself is + // fail-safe per entry (a degraded boot beats a fleet-wide crash loop), but + // a contradictory deployment that also forces PAPERCLIP_EXECUTION_MODE + // throws here and fails startup. `pluginsReady` sequences the ensure after + // the bundled-plugin install/load pass so a declared environment never + // activates before its provider driver is registered; the worker manager + // additionally gates each entry on a live plugin worker (and archives the + // row of a provider that did not come up). + try { + const bundledPluginsStartup = (app as { locals?: { bundledPluginsStartup?: Promise } }) + .locals?.bundledPluginsStartup; + const managedEnvironmentsResult = await applyManagedEnvironments(db as any, managedConfig, { + pluginsReady: bundledPluginsStartup, + workerManager: pluginWorkerManager, + }); + if (managedEnvironmentsResult) { + logger.warn(managedEnvironmentsResult, "managed sandbox environments ensured from managed config"); + } + } catch (err) { + logger.error({ err }, "failed to apply managed environments from managed config"); + throw err; + } + let drainHeartbeatRunsForShutdown: ((signal: "SIGINT" | "SIGTERM") => Promise) | null = null; let prepareHotRestartShutdown: ((signal: "SIGINT" | "SIGTERM") => Promise<{ skipDrain: boolean }>) | null = null; let heartbeatSchedulerStopped = false; diff --git a/server/src/services/environments.ts b/server/src/services/environments.ts index 6487f7a1b9e..d9a44661683 100644 --- a/server/src/services/environments.ts +++ b/server/src/services/environments.ts @@ -73,6 +73,26 @@ export interface KubernetesEnvironmentConfigInput { [key: string]: unknown; } +/** + * Input to `ensureManagedSandboxEnvironment`. Provider-agnostic: `provider` + * is the sandbox plugin's driver key and is forced into `config.provider`; + * the rest of `config` is stored verbatim for the plugin to validate at + * lease time. + */ +export interface ManagedSandboxEnvironmentInput { + name: string; + description?: string; + /** Sandbox provider key (the plugin's driverKey, e.g. "kubernetes", "daytona"). */ + provider: string; + config?: Record; + /** + * Extra metadata markers stamped on the managed row (e.g. the legacy + * kubernetes marker `managedKubernetesSandbox` that + * `findKubernetesEnvironment` keys on). + */ + extraMetadata?: Record; +} + function cloneRecord(value: unknown, fallback: Record | null = null): Record | null { if (!value || typeof value !== "object" || Array.isArray(value)) return fallback; return { ...(value as Record) }; @@ -181,6 +201,177 @@ function countFromRows(rows: Array<{ count: number | string | null | undefined } } export function environmentService(db: Db) { + /** The single Paperclip-managed sandbox row (`environments_managed_sandbox_idx`), if present. */ + const findManagedSandboxRow = () => + db + .select() + .from(environments) + .where(eq(environments.driver, "sandbox")) + .then( + (rows) => + rows.find( + (row) => + (row.metadata as Record | null)?.managedByPaperclip === true, + ) ?? null, + ); + + /** + * Idempotently ensure THE Paperclip-managed sandbox environment for this + * instance, configured for an arbitrary sandbox provider plugin. Mirrors + * `ensureLocalEnvironment`; the partial unique index + * `environments_managed_sandbox_idx` enforces at most one managed sandbox + * row per instance, so this function owns that single slot regardless of + * provider: + * + * - An existing managed row is adopted and refreshed (name, description, + * config, provider) on every call, so operator/control-plane changes flow + * via redeploy without recreating the row — including a provider switch, + * which also drops a stale provider-specific metadata marker. + * - An existing UNmanaged sandbox row holding the desired name is adopted + * and stamped as managed, so a row created by hand before the instance + * became config-managed converges instead of colliding on + * `environments_name_idx` on every boot. + */ + const ensureManagedSandboxEnvironment = async ( + input: ManagedSandboxEnvironmentInput, + ): Promise => { + const desiredConfig: Record = { + ...(input.config ?? {}), + provider: input.provider, + }; + const desiredMetadata: Record = { + managedByPaperclip: true, + managedSandboxProvider: input.provider, + ...(input.extraMetadata ?? {}), + }; + + const adopt = async (row: EnvironmentRow): Promise => { + const metadata: Record = { ...(row.metadata ?? {}), ...desiredMetadata }; + // A provider switch must not leave the previous provider's marker + // behind (`findKubernetesEnvironment` keys on it). + if (desiredMetadata[KUBERNETES_MANAGED_MARKER] !== true) { + delete metadata[KUBERNETES_MANAGED_MARKER]; + } + const now = new Date(); + const runUpdate = (values: { name?: string }) => + db + .update(environments) + .set({ + ...values, + // The row mirrors the managed spec: omitting `description` clears + // a previously configured one rather than pinning it forever. + description: input.description ?? null, + config: desiredConfig, + metadata, + status: "active", + updatedAt: now, + }) + .where(eq(environments.id, row.id)) + .returning() + .then((rows) => rows[0] ?? row); + const updated = await runUpdate({ name: input.name }).catch((error: unknown) => { + // Another row already holds the desired name; keep the current name + // rather than failing a boot-time ensure over a display label. + if (hasConstraintName(error, "environments_name_idx")) { + return runUpdate({}); + } + throw error; + }); + return toEnvironment(updated); + }; + + const existing = await findManagedSandboxRow(); + if (existing) return adopt(existing); + + // The partial unique index `environments_managed_sandbox_idx` enforces + // "at most one Paperclip-managed sandbox row per instance" at the DB + // level. Use ON CONFLICT DO NOTHING keyed on that index so concurrent + // callers can race the INSERT; losers re-read the surviving row. + const now = new Date(); + const inserted = await db + .insert(environments) + .values({ + name: input.name, + description: input.description ?? null, + driver: "sandbox", + status: "active", + config: desiredConfig, + envVars: {}, + metadata: desiredMetadata, + createdAt: now, + updatedAt: now, + }) + .onConflictDoNothing({ + target: [environments.driver], + where: + sql`${environments.driver} = 'sandbox' AND (${environments.metadata} ->> 'managedByPaperclip')::boolean = true`, + }) + .returning() + .then((rows) => rows[0] ?? null) + .catch((error) => { + if ( + hasConstraintName(error, "environments_name_idx") + || hasConstraintName(error, "environments_managed_sandbox_idx") + ) { + return null; + } + throw error; + }); + if (inserted) return toEnvironment(inserted); + + // Either a concurrent caller won the managed slot, or an unmanaged row + // holds the desired name. Adopt whichever exists. + const winner = await findManagedSandboxRow(); + if (winner) return adopt(winner); + const sameName = await db + .select() + .from(environments) + .where(eq(environments.name, input.name)) + .then((rows) => rows[0] ?? null); + if (sameName) { + if (sameName.driver !== "sandbox") { + throw new Error( + `Failed to ensure managed sandbox environment: environment "${input.name}" already exists with driver "${sameName.driver}"`, + ); + } + return adopt(sameName); + } + throw new Error("Failed to ensure managed sandbox environment"); + }; + + /** + * Archive the Paperclip-managed sandbox row when its provider became + * unavailable (plugin missing, not ready, or its worker not running), so + * run scheduling stops selecting an environment whose lease acquisition + * cannot succeed (`resolveEnvironment` rejects non-active rows). + * + * Scoped to the row provisioned for the SAME provider: a row that a + * provider switch left on a different provider is not touched (the ensure + * path adopts it once the new provider is healthy). Reactivation is + * automatic — the next successful `ensureManagedSandboxEnvironment` stamps + * the row `active` again. + * + * Returns the archived environment, or null when there is no active + * managed row for this provider. + */ + const archiveManagedSandboxEnvironment = async ( + input: { provider: string }, + ): Promise => { + const existing = await findManagedSandboxRow(); + if (!existing || existing.status !== "active") return null; + const rowProvider = (existing.metadata as Record | null) + ?.managedSandboxProvider; + if (rowProvider !== input.provider) return null; + const archived = await db + .update(environments) + .set({ status: "archived", updatedAt: new Date() }) + // Guarded on status so a concurrent re-activation is not clobbered. + .where(and(eq(environments.id, existing.id), eq(environments.status, "active"))) + .returning() + .then((rows) => rows[0] ?? null); + return archived ? toEnvironment(archived) : null; + }; + return { list: async ( companyIdOrFilters?: string | EnvironmentListFilters, @@ -256,15 +447,16 @@ export function environmentService(db: Db) { return toEnvironment(existing); }, + ensureManagedSandboxEnvironment, + + archiveManagedSandboxEnvironment, + /** - * Idempotently ensure a managed Kubernetes sandbox environment exists for a - * instance, configured from instance/operator-supplied config. Mirrors - * `ensureLocalEnvironment`, but there is no DB unique index for sandbox - * drivers, so idempotency is by metadata marker + driver lookup. - * - * The environment is `driver: "sandbox"` with `config.provider: - * "kubernetes"` so it resolves to the first-party Kubernetes sandbox - * provider. On subsequent calls the config is refreshed (so operators can + * Idempotently ensure a managed Kubernetes sandbox environment exists for + * an instance, configured from instance/operator-supplied config. A thin + * wrapper over `ensureManagedSandboxEnvironment` that pins the provider to + * "kubernetes" and stamps the legacy marker `findKubernetesEnvironment` + * keys on. On subsequent calls the config is refreshed (so operators can * update egress/runtimeClass via gitops without recreating the row). */ ensureKubernetesEnvironment: async ( @@ -272,94 +464,13 @@ export function environmentService(db: Db) { maybeConfig?: KubernetesEnvironmentConfigInput, ): Promise => { const config = resolveKubernetesConfig(companyIdOrConfig, maybeConfig); - const desiredConfig: Record = { - ...config, + return ensureManagedSandboxEnvironment({ + name: DEFAULT_KUBERNETES_ENVIRONMENT_NAME, + description: DEFAULT_KUBERNETES_ENVIRONMENT_DESCRIPTION, provider: KUBERNETES_PROVIDER_KEY, - }; - const desiredMetadata: Record = { - managedByPaperclip: true, - [KUBERNETES_MANAGED_MARKER]: true, - }; - - const existing = await db - .select() - .from(environments) - .where(eq(environments.driver, "sandbox")) - .then((rows) => - rows.find( - (row) => - (row.metadata as Record | null)?.[KUBERNETES_MANAGED_MARKER] === true, - ) ?? null, - ); - - const now = new Date(); - if (existing) { - const updated = await db - .update(environments) - .set({ - config: desiredConfig, - metadata: { ...(existing.metadata ?? {}), ...desiredMetadata }, - status: "active", - updatedAt: now, - }) - .where(eq(environments.id, existing.id)) - .returning() - .then((rows) => rows[0] ?? existing); - return toEnvironment(updated); - } - - // The partial unique index `environments_managed_sandbox_idx` enforces - // "at most one Paperclip-managed sandbox row per instance" at the DB - // level. Use ON CONFLICT DO NOTHING keyed on that index so concurrent - // callers can race the INSERT; losers re-read the surviving row. - const inserted = await db - .insert(environments) - .values({ - name: DEFAULT_KUBERNETES_ENVIRONMENT_NAME, - description: DEFAULT_KUBERNETES_ENVIRONMENT_DESCRIPTION, - driver: "sandbox", - status: "active", - config: desiredConfig, - envVars: {}, - metadata: desiredMetadata, - createdAt: now, - updatedAt: now, - }) - .onConflictDoNothing({ - target: [environments.driver], - where: - sql`${environments.driver} = 'sandbox' AND (${environments.metadata} ->> 'managedByPaperclip')::boolean = true`, - }) - .returning() - .then((rows) => rows[0] ?? null) - .catch((error) => { - if ( - hasConstraintName(error, "environments_name_idx") - || hasConstraintName(error, "environments_managed_sandbox_idx") - ) { - return null; - } - throw error; - }); - if (inserted) return toEnvironment(inserted); - - const winner = await db - .select() - .from(environments) - .where(eq(environments.driver, "sandbox")) - .then( - (rows) => - rows.find( - (candidate) => - (candidate.metadata as Record | null)?.[ - KUBERNETES_MANAGED_MARKER - ] === true, - ) ?? null, - ); - if (!winner) { - throw new Error("Failed to ensure kubernetes environment"); - } - return toEnvironment(winner); + config, + extraMetadata: { [KUBERNETES_MANAGED_MARKER]: true }, + }); }, /** diff --git a/server/src/services/index.ts b/server/src/services/index.ts index 5f05bcf00db..08e0386c6dc 100644 --- a/server/src/services/index.ts +++ b/server/src/services/index.ts @@ -116,9 +116,11 @@ export { managedFeatureKeySet, parseManagedConfigEnv, MANAGED_CONFIG_ENV_KEY, + type ManagedEnvironmentSpec, type ManagedInstanceConfig, } from "./managed-config.js"; export { bootstrapExecutionPolicyFromEnv } from "./execution-policy-bootstrap.js"; +export { applyManagedEnvironments } from "./managed-environments.js"; export { cloudUpstreamService, reconcileCloudUpstreamRunsOnStartup } from "./cloud-upstreams.js"; export { companyPortabilityService } from "./company-portability.js"; export { teamsCatalogService } from "./teams-catalog.js"; diff --git a/server/src/services/managed-config.ts b/server/src/services/managed-config.ts index 87c37b5332b..b7239302010 100644 --- a/server/src/services/managed-config.ts +++ b/server/src/services/managed-config.ts @@ -9,7 +9,10 @@ * "mode": "cloud", * "catalogVersion": "2026.720.0", * "features": { "": true | false, ... }, - * "plugins": { "autoInstall": ["daytona", "kubernetes"] } + * "plugins": { "autoInstall": ["daytona", "kubernetes"] }, + * "environments": [ + * { "name": "Daytona", "provider": "daytona", "config": { "target": "us" } } + * ] * } * * Parsing follows the `execution-policy-bootstrap.ts` doctrine: a pure @@ -47,6 +50,52 @@ export interface ManagedInstanceConfig { catalogVersion: string; features: Readonly>>; plugins: { readonly autoInstall: readonly string[] }; + /** Sandbox environments the control plane provisions at boot (empty when the section is absent). */ + environments: readonly ManagedEnvironmentSpec[]; +} + +/** + * One declared managed sandbox environment. Provider-agnostic: `provider` is + * the sandbox plugin's driver key (== its `plugins.autoInstall` catalog key), + * and `config` is stored verbatim in the environment row for the plugin to + * validate at lease time — the same split `ensureKubernetesEnvironment` has + * always used, generalized to any bundled sandbox provider. + */ +export interface ManagedEnvironmentSpec { + /** Display name of the instance-level environment row (unique per instance). */ + name: string; + description?: string; + /** Sandbox provider key (the plugin's driverKey, e.g. "daytona"). */ + provider: string; + /** Provider config stored in `environment.config`; never carries secrets. */ + config: Readonly>; +} + +/** + * Managed-config documents deliver NO secrets, ever. Provider credentials + * reach a managed instance as process environment variables (every bundled + * sandbox provider falls back to its env var when `config` omits the key, + * e.g. `DAYTONA_API_KEY`), so any secret-looking config key in the document + * is a misrouted credential and fails startup. + */ +const SECRET_LIKE_CONFIG_KEY_PATTERN = /(api[-_]?key|token|secret|password|credential)/i; + +function findSecretLikeConfigKey(value: Record, path: string): string | null { + for (const [key, child] of Object.entries(value)) { + const childPath = path.length > 0 ? `${path}.${key}` : key; + if (SECRET_LIKE_CONFIG_KEY_PATTERN.test(key)) return childPath; + if (isPlainObject(child)) { + const nested = findSecretLikeConfigKey(child, childPath); + if (nested) return nested; + } else if (Array.isArray(child)) { + for (const [index, element] of child.entries()) { + if (!isPlainObject(element)) continue; + const nested = findSecretLikeConfigKey(element, `${childPath}[${index}]`); + if (nested) return nested; + } + } + } + return null; } let cachedFeatureKeys: ReadonlySet | null = null; @@ -106,10 +155,10 @@ export function parseManagedConfigEnv(env: ManagedConfigEnv): ManagedInstanceCon fail(`must be a JSON object (got ${describeJsonValue(doc)})`); } - const allowedTopLevelKeys = new Set(["v", "mode", "catalogVersion", "features", "plugins"]); + const allowedTopLevelKeys = new Set(["v", "mode", "catalogVersion", "features", "plugins", "environments"]); for (const key of Object.keys(doc)) { if (!allowedTopLevelKeys.has(key)) { - fail(`has unknown top-level key "${key}" (allowed: v, mode, catalogVersion, features, plugins)`); + fail(`has unknown top-level key "${key}" (allowed: v, mode, catalogVersion, features, plugins, environments)`); } } @@ -195,12 +244,93 @@ export function parseManagedConfigEnv(env: ManagedConfigEnv): ManagedInstanceCon autoInstall.push(entry); } + // `environments` is OPTIONAL, unlike `features` and `plugins`: documents + // delivered before the section existed must keep booting newer builds (a + // fleet image roll cannot be lockstepped with a config re-delivery), and + // absence is not a dropped security control — it simply declares no managed + // environments. When present, the section is validated fail-closed like + // everything else. + const environmentSpecs: ManagedEnvironmentSpec[] = []; + if (doc.environments !== undefined) { + if (!Array.isArray(doc.environments)) { + fail(`"environments" must be an array of environment objects (got ${describeJsonValue(doc.environments)})`); + } + // The DB enforces at most ONE Paperclip-managed sandbox row per instance + // (partial unique index `environments_managed_sandbox_idx`); every entry + // here provisions that row, so a longer list can never be satisfied. + if (doc.environments.length > 1) { + fail( + `"environments" supports at most one entry: each entry provisions the single Paperclip-managed sandbox environment (DB invariant environments_managed_sandbox_idx)`, + ); + } + for (const [index, entry] of doc.environments.entries()) { + if (!isPlainObject(entry)) { + fail(`"environments[${index}]" must be an object (got ${describeJsonValue(entry)})`); + } + const allowedEntryKeys = new Set(["name", "description", "provider", "config"]); + for (const key of Object.keys(entry)) { + if (!allowedEntryKeys.has(key)) { + fail(`"environments[${index}]" has unknown key "${key}" (allowed: name, description, provider, config)`); + } + } + if (typeof entry.name !== "string" || entry.name.length === 0 || entry.name.trim() !== entry.name) { + fail( + `"environments[${index}].name" must be a non-empty string without surrounding whitespace (got ${describeJsonValue(entry.name)})`, + ); + } + if ( + entry.description !== undefined + && (typeof entry.description !== "string" || entry.description.trim().length === 0) + ) { + fail(`"environments[${index}].description" must be a non-empty string when present (got ${describeJsonValue(entry.description)})`); + } + if (typeof entry.provider !== "string" || entry.provider.length === 0 || entry.provider.trim() !== entry.provider) { + fail( + `"environments[${index}].provider" must be a non-empty string without surrounding whitespace (got ${describeJsonValue(entry.provider)})`, + ); + } + // Coherence: on a managed instance the control plane is the only plugin + // install path, so an environment whose provider plugin is not + // auto-installed could never serve a lease. Catch the skew at parse time. + if (!autoInstall.includes(entry.provider)) { + fail( + `"environments[${index}].provider" is "${entry.provider}", which is not in "plugins.autoInstall"; a managed environment requires its provider plugin to be provisioned`, + ); + } + const config: Record = {}; + if (entry.config !== undefined) { + if (!isPlainObject(entry.config)) { + fail(`"environments[${index}].config" must be an object (got ${describeJsonValue(entry.config)})`); + } + if (entry.config.provider !== undefined) { + fail(`"environments[${index}].config" must not set "provider"; it is forced from the entry's provider key`); + } + const secretLikeKey = findSecretLikeConfigKey(entry.config, ""); + if (secretLikeKey) { + fail( + `"environments[${index}].config" key "${secretLikeKey}" looks secret-bearing; credentials are delivered to managed instances as process environment variables (the provider's documented env fallback), never in the managed-config document`, + ); + } + Object.assign(config, entry.config); + } + environmentSpecs.push( + Object.freeze({ + name: entry.name, + ...(entry.description !== undefined ? { description: entry.description } : {}), + provider: entry.provider, + config: Object.freeze(config), + }) as ManagedEnvironmentSpec, + ); + } + } + return Object.freeze({ v: SUPPORTED_MANAGED_CONFIG_VERSION, mode: "cloud", catalogVersion: doc.catalogVersion, features: Object.freeze(features), plugins: Object.freeze({ autoInstall: Object.freeze(autoInstall) }), + environments: Object.freeze(environmentSpecs), }) as ManagedInstanceConfig; } diff --git a/server/src/services/managed-environments.test.ts b/server/src/services/managed-environments.test.ts new file mode 100644 index 00000000000..4857c26a8b6 --- /dev/null +++ b/server/src/services/managed-environments.test.ts @@ -0,0 +1,415 @@ +import { describe, expect, it, vi } from "vitest"; +import type { Db } from "@paperclipai/db"; +import { MANAGED_CONFIG_ENV_KEY, parseManagedConfigEnv } from "./managed-config.js"; +import { + applyManagedEnvironments, + type ApplyManagedEnvironmentsOptions, +} from "./managed-environments.js"; + +const noDb = null as unknown as Db; + +function parsedConfig(overrides: Record = {}) { + const config = parseManagedConfigEnv({ + [MANAGED_CONFIG_ENV_KEY]: JSON.stringify({ + v: 1, + mode: "cloud", + catalogVersion: "2026.720.0", + features: {}, + plugins: { autoInstall: ["daytona"] }, + ...overrides, + }), + }); + if (!config) throw new Error("expected a parsed managed config"); + return config; +} + +function environmentRow(overrides: Record = {}) { + const now = new Date(); + return { + id: "env-1", + name: "Daytona", + description: null, + driver: "sandbox", + status: "active", + config: {}, + envVars: {}, + metadata: {}, + createdAt: now, + updatedAt: now, + ...overrides, + }; +} + +function readyDriverResolver(status = "ready") { + return vi.fn(async () => ({ + plugin: { id: "plugin-1", pluginKey: "sandbox-providers/daytona", status }, + })); +} + +function runningWorkerManager(running = true) { + return { isRunning: vi.fn(() => running), getWorker: vi.fn(() => undefined) }; +} + +type WorkerManagerSeam = NonNullable; +type RecoveryHandle = NonNullable>; + +/** A worker handle fake that records `ready` listeners so tests can fire them. */ +function recoveryHandle() { + const readyListeners: Array<(payload: { pluginId: string }) => void> = []; + const handle: RecoveryHandle = { + on: vi.fn((_event, listener) => { + readyListeners.push(listener); + }), + off: vi.fn((_event, listener) => { + const at = readyListeners.indexOf(listener); + if (at !== -1) readyListeners.splice(at, 1); + }), + }; + return { + handle, + fireReady: () => { + for (const listener of [...readyListeners]) listener({ pluginId: "plugin-1" }); + }, + }; +} + +/** Lets the fire-and-forget recovery ensure settle. */ +function tick() { + return new Promise((resolve) => setImmediate(resolve)); +} + +type EnvironmentsSeam = NonNullable; + +function environmentsSeam(overrides: Partial = {}): EnvironmentsSeam { + return { + ensureManagedSandboxEnvironment: + overrides.ensureManagedSandboxEnvironment ?? vi.fn().mockResolvedValue(environmentRow()), + archiveManagedSandboxEnvironment: + overrides.archiveManagedSandboxEnvironment ?? vi.fn().mockResolvedValue(null), + }; +} + +describe("applyManagedEnvironments", () => { + it("no-ops for self-hosted instances and for documents without environments", async () => { + expect(await applyManagedEnvironments(noDb, null)).toBeNull(); + expect(await applyManagedEnvironments(noDb, parsedConfig())).toBeNull(); + }); + + it("refuses startup when a forced execution mode also claims the managed sandbox slot", async () => { + const config = parsedConfig({ + environments: [{ name: "Daytona", provider: "daytona" }], + }); + await expect( + applyManagedEnvironments(noDb, config, { + env: { PAPERCLIP_EXECUTION_MODE: "kubernetes" }, + }), + ).rejects.toThrow(/mutually exclusive/); + }); + + it("ensures each declared environment through the provider-agnostic service call", async () => { + const ensureManagedSandboxEnvironment = vi + .fn() + .mockResolvedValue(environmentRow()); + const config = parsedConfig({ + environments: [ + { + name: "Daytona", + description: "Managed Daytona sandbox.", + provider: "daytona", + config: { target: "us" }, + }, + ], + }); + + const resolveSandboxProviderDriver = readyDriverResolver(); + const workerManager = runningWorkerManager(); + const result = await applyManagedEnvironments(noDb, config, { + env: {}, + workerManager, + environments: environmentsSeam({ ensureManagedSandboxEnvironment }), + resolveSandboxProviderDriver, + }); + + expect(result).toEqual({ ensured: 1, failed: 0 }); + expect(resolveSandboxProviderDriver).toHaveBeenCalledWith({ db: noDb, driverKey: "daytona" }); + expect(workerManager.isRunning).toHaveBeenCalledWith("plugin-1"); + expect(ensureManagedSandboxEnvironment).toHaveBeenCalledTimes(1); + expect(ensureManagedSandboxEnvironment).toHaveBeenCalledWith({ + name: "Daytona", + description: "Managed Daytona sandbox.", + provider: "daytona", + config: { target: "us" }, + }); + // The frozen parsed config must not leak into the service (the row's + // config is mutated downstream when the provider key is forced in). + const passedConfig = ensureManagedSandboxEnvironment.mock.calls[0]?.[0]?.config; + expect(Object.isFrozen(passedConfig)).toBe(false); + }); + + it("waits for the bundled-plugin startup pass before ensuring anything", async () => { + const ensureManagedSandboxEnvironment = vi + .fn() + .mockResolvedValue(environmentRow()); + const config = parsedConfig({ + environments: [{ name: "Daytona", provider: "daytona" }], + }); + + let releasePlugins!: () => void; + const pluginsReady = new Promise((resolve) => { + releasePlugins = resolve; + }); + + const pending = applyManagedEnvironments(noDb, config, { + env: {}, + pluginsReady, + workerManager: runningWorkerManager(), + environments: environmentsSeam({ ensureManagedSandboxEnvironment }), + resolveSandboxProviderDriver: readyDriverResolver(), + }); + + // Give the ensure every chance to (incorrectly) run early. + await new Promise((resolve) => setImmediate(resolve)); + expect(ensureManagedSandboxEnvironment).not.toHaveBeenCalled(); + + releasePlugins(); + expect(await pending).toEqual({ ensured: 1, failed: 0 }); + expect(ensureManagedSandboxEnvironment).toHaveBeenCalledTimes(1); + }); + + it("skips an entry whose provider plugin is missing and archives its stale row", async () => { + const environments = environmentsSeam({ + archiveManagedSandboxEnvironment: vi.fn().mockResolvedValue(environmentRow({ status: "archived" })), + }); + const config = parsedConfig({ + environments: [{ name: "Daytona", provider: "daytona" }], + }); + + const result = await applyManagedEnvironments(noDb, config, { + env: {}, + workerManager: runningWorkerManager(), + environments, + resolveSandboxProviderDriver: vi.fn(async () => null), + }); + + expect(result).toEqual({ ensured: 0, failed: 1 }); + expect(environments.ensureManagedSandboxEnvironment).not.toHaveBeenCalled(); + expect(environments.archiveManagedSandboxEnvironment).toHaveBeenCalledWith({ + provider: "daytona", + }); + }); + + it("skips (and counts failed) an entry whose provider plugin is not ready", async () => { + const environments = environmentsSeam(); + const config = parsedConfig({ + environments: [{ name: "Daytona", provider: "daytona" }], + }); + + const result = await applyManagedEnvironments(noDb, config, { + env: {}, + workerManager: runningWorkerManager(), + environments, + resolveSandboxProviderDriver: readyDriverResolver("disabled"), + }); + + expect(result).toEqual({ ensured: 0, failed: 1 }); + expect(environments.ensureManagedSandboxEnvironment).not.toHaveBeenCalled(); + expect(environments.archiveManagedSandboxEnvironment).toHaveBeenCalledWith({ + provider: "daytona", + }); + }); + + it("skips an entry whose plugin record is ready but whose worker is not running", async () => { + const environments = environmentsSeam(); + const config = parsedConfig({ + environments: [{ name: "Daytona", provider: "daytona" }], + }); + + const workerManager = runningWorkerManager(false); + const result = await applyManagedEnvironments(noDb, config, { + env: {}, + workerManager, + environments, + resolveSandboxProviderDriver: readyDriverResolver(), + }); + + expect(result).toEqual({ ensured: 0, failed: 1 }); + expect(workerManager.isRunning).toHaveBeenCalledWith("plugin-1"); + expect(environments.ensureManagedSandboxEnvironment).not.toHaveBeenCalled(); + expect(environments.archiveManagedSandboxEnvironment).toHaveBeenCalledWith({ + provider: "daytona", + }); + // No registered handle → no respawn is coming; degraded until next boot. + expect(workerManager.getWorker).toHaveBeenCalledWith("plugin-1"); + }); + + it("reactivates the archived environment once when the provider worker recovers", async () => { + const environments = environmentsSeam(); + const config = parsedConfig({ + environments: [ + { name: "Daytona", provider: "daytona", config: { target: "us" } }, + ], + }); + + const { handle, fireReady } = recoveryHandle(); + const workerManager = { + isRunning: vi.fn(() => false), + getWorker: vi.fn(() => handle), + }; + const result = await applyManagedEnvironments(noDb, config, { + env: {}, + workerManager, + environments, + resolveSandboxProviderDriver: readyDriverResolver(), + }); + + // The boot pass itself stays degraded: archived, counted failed. + expect(result).toEqual({ ensured: 0, failed: 1 }); + expect(environments.archiveManagedSandboxEnvironment).toHaveBeenCalledWith({ + provider: "daytona", + }); + expect(environments.ensureManagedSandboxEnvironment).not.toHaveBeenCalled(); + + // Worker manager respawns the worker → the handle re-emits `ready`. + fireReady(); + fireReady(); + await tick(); + + expect(environments.ensureManagedSandboxEnvironment).toHaveBeenCalledTimes(1); + expect(environments.ensureManagedSandboxEnvironment).toHaveBeenCalledWith({ + name: "Daytona", + description: undefined, + provider: "daytona", + config: { target: "us" }, + }); + expect(handle.off).toHaveBeenCalledTimes(1); + }); + + it("re-checks isRunning after subscribing so a recovery during the archive is not missed", async () => { + const environments = environmentsSeam(); + const config = parsedConfig({ + environments: [{ name: "Daytona", provider: "daytona" }], + }); + + const { handle } = recoveryHandle(); + // Down at the gate check, already back up by the time the listener is + // registered — the `ready` event has been missed. + const workerManager = { + isRunning: vi + .fn() + .mockReturnValueOnce(false) + .mockReturnValue(true), + getWorker: vi.fn(() => handle), + }; + const result = await applyManagedEnvironments(noDb, config, { + env: {}, + workerManager, + environments, + resolveSandboxProviderDriver: readyDriverResolver(), + }); + await tick(); + + expect(result).toEqual({ ensured: 0, failed: 1 }); + expect(environments.ensureManagedSandboxEnvironment).toHaveBeenCalledTimes(1); + }); + + it("does not subscribe for recovery when the plugin record is missing or not ready", async () => { + const config = parsedConfig({ + environments: [{ name: "Daytona", provider: "daytona" }], + }); + + for (const resolveSandboxProviderDriver of [ + vi.fn(async () => null), + readyDriverResolver("disabled"), + ]) { + const workerManager = runningWorkerManager(false); + await applyManagedEnvironments(noDb, config, { + env: {}, + workerManager, + environments: environmentsSeam(), + resolveSandboxProviderDriver, + }); + expect(workerManager.getWorker).not.toHaveBeenCalled(); + } + }); + + it("logs, not throws, when the recovery re-ensure fails", async () => { + const environments = environmentsSeam({ + ensureManagedSandboxEnvironment: vi.fn().mockRejectedValue(new Error("db exploded")), + }); + const config = parsedConfig({ + environments: [{ name: "Daytona", provider: "daytona" }], + }); + + const { handle, fireReady } = recoveryHandle(); + const workerManager = { + isRunning: vi.fn(() => false), + getWorker: vi.fn(() => handle), + }; + await applyManagedEnvironments(noDb, config, { + env: {}, + workerManager, + environments, + resolveSandboxProviderDriver: readyDriverResolver(), + }); + + fireReady(); + await tick(); + // The rejection is swallowed into a log line; reaching this point without + // an unhandled rejection is the assertion. + expect(environments.ensureManagedSandboxEnvironment).toHaveBeenCalledTimes(1); + }); + + it("fails closed when no worker manager is provided", async () => { + const environments = environmentsSeam(); + const config = parsedConfig({ + environments: [{ name: "Daytona", provider: "daytona" }], + }); + + const result = await applyManagedEnvironments(noDb, config, { + env: {}, + environments, + resolveSandboxProviderDriver: readyDriverResolver(), + }); + + expect(result).toEqual({ ensured: 0, failed: 1 }); + expect(environments.ensureManagedSandboxEnvironment).not.toHaveBeenCalled(); + }); + + it("counts a skipped entry once even when archiving its stale row fails", async () => { + const environments = environmentsSeam({ + archiveManagedSandboxEnvironment: vi.fn().mockRejectedValue(new Error("db exploded")), + }); + const config = parsedConfig({ + environments: [{ name: "Daytona", provider: "daytona" }], + }); + + const result = await applyManagedEnvironments(noDb, config, { + env: {}, + workerManager: runningWorkerManager(), + environments, + resolveSandboxProviderDriver: vi.fn(async () => null), + }); + + expect(result).toEqual({ ensured: 0, failed: 1 }); + expect(environments.archiveManagedSandboxEnvironment).toHaveBeenCalledTimes(1); + }); + + it("is fail-safe per entry: an ensure failure is counted, not thrown", async () => { + const environments = environmentsSeam({ + ensureManagedSandboxEnvironment: vi.fn().mockRejectedValue(new Error("db exploded")), + }); + const config = parsedConfig({ + environments: [{ name: "Daytona", provider: "daytona" }], + }); + + const result = await applyManagedEnvironments(noDb, config, { + env: {}, + workerManager: runningWorkerManager(), + environments, + resolveSandboxProviderDriver: readyDriverResolver(), + }); + + expect(result).toEqual({ ensured: 0, failed: 1 }); + expect(environments.archiveManagedSandboxEnvironment).not.toHaveBeenCalled(); + }); +}); diff --git a/server/src/services/managed-environments.ts b/server/src/services/managed-environments.ts new file mode 100644 index 00000000000..fbdf2c69851 --- /dev/null +++ b/server/src/services/managed-environments.ts @@ -0,0 +1,239 @@ +/** + * Managed-environment bootstrap (harness → app contract). + * + * Managed-cloud instances may declare sandbox environments in the + * `environments` section of `PAPERCLIP_MANAGED_CONFIG` (parsed fail-closed in + * `managed-config.ts`). On boot, each declared environment is idempotently + * ensured as the instance-level Paperclip-managed sandbox row via the + * provider-agnostic `ensureManagedSandboxEnvironment` — the control plane + * provisions, tenants use, for any bundled sandbox provider plugin. + * + * The failure posture mirrors bundled plugin provisioning + * (`bundled-plugins.ts`), deliberately split: + * + * 1. **Validation fails closed at parse time** (`managed-config.ts`): a + * malformed section refuses startup with a precise error. + * 2. **The DB ensure step is fail-safe per entry**: an ensure failure is + * logged and boot continues degraded (environment unavailable) rather + * than crash-looping a fleet. + * + * Ensuring is additionally synchronized with provider availability: the + * caller's `pluginsReady` promise (the bundled-plugin install/load pass) is + * awaited first, and an entry whose provider plugin is not installed, `ready`, + * AND running a live worker (`workerManager.isRunning`; a `ready` record whose + * activation failed has no worker and cannot serve leases) afterwards is + * skipped (counted failed) instead of being written as an active row — + * otherwise the heartbeat would resume queued runs against an environment + * whose lease acquisition cannot succeed yet. When such an entry was + * provisioned by an earlier boot, its still-active row is archived + * (`archiveManagedSandboxEnvironment`) for the same reason. Re-activation + * happens on the next healthy boot's ensure, or earlier: when the plugin + * record is `ready` and only the worker is down (a crash in restart-backoff), + * a one-shot `ready` listener on the worker handle re-runs the ensure as soon + * as the worker manager respawns the worker, so a transient crash does not + * leave the environment archived until someone restarts the server. + * + * Removing an entry from the document stops future refreshes but never + * deletes or archives the row — there is intentionally no unprovision path + * here, matching `plugins.autoInstall` semantics (leases may still reference + * the row; withdrawal is an explicit operator action). Archiving above is + * scoped to a declared-but-unavailable provider, not to document removal. + * + * Provider credentials are never part of the declared config: every bundled + * sandbox provider falls back to its documented process environment variable + * (e.g. `DAYTONA_API_KEY`) when `config` omits the key, so the deployment + * delivers secrets as env vars and the managed document stays secret-free. + */ + +import type { Db } from "@paperclipai/db"; +import { logger } from "../middleware/logger.js"; +import { environmentService } from "./environments.js"; +import type { ManagedInstanceConfig } from "./managed-config.js"; +import { parseExecutionPolicyBootstrapEnv } from "./execution-policy-bootstrap.js"; +import { resolvePluginSandboxProviderDriverByKey } from "./plugin-environment-driver.js"; +import type { PluginWorkerManager } from "./plugin-worker-manager.js"; + +export interface ApplyManagedEnvironmentsOptions { + env?: Record; + /** + * Resolves when the bundled-plugin startup pass (install + load; the + * `ensureBundledPlugins` chain in `createApp`) has finished. Awaited before + * any environment is ensured so an active row never precedes its provider + * driver. The promise never rejects (the chain catches internally). + */ + pluginsReady?: Promise; + /** + * The app's plugin worker manager, consulted per entry so an environment is + * only ensured while its provider plugin has a live worker (a `ready` + * registry record whose activation failed cannot serve leases). Fails + * closed: without a worker manager every entry is treated as unavailable. + * + * `getWorker` is `PluginWorkerManager.getWorker` narrowed to the `ready` + * subscription used for post-archive recovery: a worker in crash-restart + * backoff re-emits `ready` when the manager respawns it, and the listener + * re-ensures the archived environment (see `applyManagedEnvironments`). + */ + workerManager?: Pick & { + getWorker(pluginId: string): + | { + on(event: "ready", listener: (payload: { pluginId: string }) => void): void; + off(event: "ready", listener: (payload: { pluginId: string }) => void): void; + } + | undefined; + }; + /** Test seam: overrides the environment service built from `db`. */ + environments?: Pick< + ReturnType, + "ensureManagedSandboxEnvironment" | "archiveManagedSandboxEnvironment" + >; + /** Test seam: overrides the sandbox-provider plugin driver lookup. */ + resolveSandboxProviderDriver?: (input: { + db: Db; + driverKey: string; + }) => Promise<{ plugin: { id: string; pluginKey: string; status: string } } | null>; +} + +/** + * Ensure every environment declared in the managed-config document. Returns + * null when there is nothing to do (self-hosted, or no `environments` + * section); otherwise the ensured/failed counts. Idempotent; safe to call on + * every boot. + */ +export async function applyManagedEnvironments( + db: Db, + managedConfig: ManagedInstanceConfig | null, + opts: ApplyManagedEnvironmentsOptions = {}, +): Promise<{ ensured: number; failed: number } | null> { + if (!managedConfig || managedConfig.environments.length === 0) return null; + + // The forced-execution-mode bootstrap (`PAPERCLIP_EXECUTION_MODE=kubernetes`) + // and this one both own the single Paperclip-managed sandbox row + // (`environments_managed_sandbox_idx`). Configuring both is contradictory; + // refuse startup rather than let bootstrap ordering pick a winner. + const env = opts.env ?? process.env; + if (parseExecutionPolicyBootstrapEnv(env)) { + throw new Error( + `PAPERCLIP_EXECUTION_MODE and the PAPERCLIP_MANAGED_CONFIG "environments" section are mutually exclusive: both manage the single instance sandbox environment`, + ); + } + + // The heartbeat resumes queued runs right after this bootstrap step, and + // lease acquisition fails hard on a provider whose plugin is missing or not + // ready. Wait for the bundled-plugin startup pass to finish, then refuse to + // ensure (and in particular to re-activate) a row whose provider driver did + // not come up — a degraded boot without the row beats an active environment + // that fails every lease until the plugin recovers. + await opts.pluginsReady; + + const resolveDriver = opts.resolveSandboxProviderDriver ?? resolvePluginSandboxProviderDriverByKey; + const environments = opts.environments ?? environmentService(db); + + // Recovery path for a `ready` plugin whose worker was down at check time: + // that shape is usually a crash in restart-backoff, and the manager's + // respawn re-emits `ready` on the same handle. A one-shot listener re-runs + // the idempotent ensure so the recovered provider becomes selectable again + // without waiting for the next boot (this pass has already archived the + // row). The post-subscribe `isRunning` re-check closes the race where the + // worker recovered between the gate check and the subscription. No handle + // means no respawn is coming (activation never started a worker), so the + // degraded-until-next-boot posture stands. + const scheduleRecoveryReactivation = ( + spec: ManagedInstanceConfig["environments"][number], + pluginId: string, + ): void => { + const manager = opts.workerManager; + const handle = manager?.getWorker(pluginId); + if (!handle) return; + let reactivated = false; + const reactivate = (): void => { + if (reactivated) return; + reactivated = true; + handle.off("ready", reactivate); + void environments + .ensureManagedSandboxEnvironment({ + name: spec.name, + description: spec.description, + provider: spec.provider, + config: { ...spec.config }, + }) + .then((environment) => { + logger.info( + { environmentId: environment.id, name: spec.name, provider: spec.provider }, + "managed sandbox environment reactivated after provider worker recovery", + ); + }) + .catch((err: unknown) => { + logger.error( + { err, name: spec.name, provider: spec.provider }, + "failed to reactivate managed sandbox environment after provider worker recovery (degraded: environment unavailable until next boot)", + ); + }); + }; + handle.on("ready", reactivate); + if (manager?.isRunning(pluginId)) reactivate(); + }; + + let ensured = 0; + let failed = 0; + for (const spec of managedConfig.environments) { + try { + const resolved = await resolveDriver({ db, driverKey: spec.provider }); + // `ready` in the registry is necessary but not sufficient: activation + // can fail after install leaves the record `ready`, in which case no + // worker is running and the driver cannot serve leases. + const workerRunning = + resolved != null && (opts.workerManager?.isRunning(resolved.plugin.id) ?? false); + if (!resolved || resolved.plugin.status !== "ready" || !workerRunning) { + failed += 1; + // A row provisioned by an earlier boot must not stay active either — + // archive it so run scheduling stops selecting it. Best-effort: an + // archive failure is logged, and the entry is already counted failed. + const archived = await environments + .archiveManagedSandboxEnvironment({ provider: spec.provider }) + .catch((archiveErr: unknown) => { + logger.error( + { err: archiveErr, name: spec.name, provider: spec.provider }, + "failed to archive the managed sandbox environment of an unavailable provider", + ); + return null; + }); + logger.error( + { + name: spec.name, + provider: spec.provider, + pluginKey: resolved?.plugin.pluginKey ?? null, + pluginStatus: resolved?.plugin.status ?? null, + workerRunning, + archivedEnvironmentId: archived?.id ?? null, + }, + "managed sandbox environment provider plugin is not installed, ready, and running; skipping ensure and archiving any previously provisioned row (degraded: environment unavailable)", + ); + // Only a `ready` record can recover without another boot: the worker + // manager restarts crashed workers, but nothing (re)installs a missing + // or non-ready plugin at runtime. + if (resolved && resolved.plugin.status === "ready") { + scheduleRecoveryReactivation(spec, resolved.plugin.id); + } + continue; + } + const environment = await environments.ensureManagedSandboxEnvironment({ + name: spec.name, + description: spec.description, + provider: spec.provider, + config: { ...spec.config }, + }); + ensured += 1; + logger.info( + { environmentId: environment.id, name: environment.name, provider: spec.provider }, + "managed sandbox environment ensured", + ); + } catch (err) { + failed += 1; + logger.error( + { err, name: spec.name, provider: spec.provider }, + "failed to ensure managed sandbox environment; continuing boot (degraded: environment unavailable)", + ); + } + } + return { ensured, failed }; +} From a3b293e26d460521248cecc7ca4d5472f4b316af Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Mon, 27 Jul 2026 15:26:11 -0700 Subject: [PATCH 03/87] perf(adapter-utils): parallelize the two Daytona sandbox bridge setups (#10334) ## Thinking Path > - Paperclip runs AI-agent work through adapter-backed execution paths. > - Sandbox-backed local adapters pay startup overhead in two host-side bridge setups. > - Those setups were happening serially even though most of the work is independent. > - The remaining dependency is only the merged env that must reach the process-session launch. > - This pull request overlaps the independent setup work while preserving that launch-time dependency. > - The benefit is lower end-to-end startup latency without changing execution semantics. ## Linked Issues or Issue Description No public GitHub issue exists for this change. Problem / motivation: - Sandbox-backed local adapter startup waited for two largely independent bridge setups in series, so users paid roughly the sum of both setup times. - The only hard dependency is the merged Paperclip env that must reach the process-session launch. Proposed solution: - Start the paperclip callback bridge and the process-session bridge concurrently. - Keep the env merge as the single sequencing point before process-session launch. - Stop whichever bridge started if either startup path fails. - Keep the concurrent bridge telemetry duration-only so shared runner counters are not double-counted. Alternatives considered: - Keep the bridges serial, which preserves the current telemetry shape but leaves the startup latency unchanged. - Move env merging earlier, which would complicate launch ordering and risk changing runtime semantics. Roadmap alignment: - This is the approved Daytona start-speedup work, focused on bridge startup concurrency rather than broader runtime behavior. ## What Changed - Started the paperclip callback bridge and the process-session bridge concurrently in `execute.ts`. - Added a memoized env finalizer so the process-session launch still waits for the merged paperclip env at the correct moment. - Updated `execution-target.ts` so the process-session bridge can accept a deferred env resolver and consume it only at launch time. - Added tests that cover the overlapped launch path and the failure-cleanup path when one bridge start fails. ## Verification - `pnpm --filter @paperclipai/adapter-utils exec vitest run src/acpx-engine/execute.test.ts` - `pnpm --filter @paperclipai/adapter-utils test` - `pnpm --filter @paperclipai/adapter-utils exec tsc --noEmit` - `git log --oneline origin/master..origin/perf/parallelize-daytona-bridge-setups` - `git diff --stat origin/master...origin/perf/parallelize-daytona-bridge-setups` ## Risks - A regression in the launch-time env merge could change what the process-session bridge sees at startup. - The concurrent start/cleanup logic could leak a bridge if the stop paths were incorrect, which is why the failure cleanup test matters. - This is low-to-moderate risk because the change is isolated to adapter-utils startup plumbing and is covered by targeted tests. ## Model Used OpenAI Codex, GPT-5, tool-using code execution mode. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Harold Kim Co-authored-by: Paperclip --- .../src/acpx-engine/execute.test.ts | 97 ++++++++++- .../adapter-utils/src/acpx-engine/execute.ts | 150 ++++++++++++++---- .../adapter-utils/src/execution-target.ts | 13 +- 3 files changed, 223 insertions(+), 37 deletions(-) diff --git a/packages/adapter-utils/src/acpx-engine/execute.test.ts b/packages/adapter-utils/src/acpx-engine/execute.test.ts index f08288b3d00..c1fe7c2c2d6 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.test.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.test.ts @@ -1851,6 +1851,85 @@ describe("ACPX engine remote sandbox staging seam (PR 1: workspace + cwd)", () = expect(sessionInputs[0]?.cwd).toBe(remoteCwd); }); + it("hands the merged paperclip env to the process-session launch when the setups overlap", async () => { + const { stateDir, localCwd, remoteCwd, executionTarget } = await setupRemoteSandbox(); + // Decode the process-session LAUNCH payload (the base64 command blob) — the + // in-sandbox process env is carried there, NOT in the exec's own `env`. + let launchPayload: Record | null = null; + (executionTarget as { runner: unknown }).runner = createLocalSandboxRunner((input) => { + if (input.env?.PAPERCLIP_SANDBOX_EXEC_CHANNEL === "bridge") { + const script = input.args?.[1] ?? ""; + const match = script.match(/PAPERCLIP_PROCESS_SESSION_COMMAND_B64='([^']+)'/); + if (match) { + launchPayload = JSON.parse(Buffer.from(match[1]!, "base64").toString("utf8")) as Record< + string, + unknown + >; + } + } + }); + + await runExecutor( + { agent: "custom", agentCommand: "node ./fake-acp.js", stateDir, cwd: localCwd }, + { authToken: "real-run-jwt", executionTarget }, + ); + + // The process-session bridge receives its launch env as a DEFERRED thunk — + // the seam that lets its env-independent setup overlap the paperclip bridge + // start instead of running strictly after it. + const processArgs = vi.mocked(startAdapterExecutionTargetProcessSessionBridge).mock.calls[0]![0]; + expect(typeof processArgs.env).toBe("function"); + + // ...and despite the overlap the launch still observes the MERGED paperclip + // env: the paperclip-`env` → process-session-launch hand-off stays sequenced + // under concurrency (bridge base URL + minted bridge token both present, and + // the token is NOT the host run JWT). + const payloadEnv = ((launchPayload as Record | null)?.env ?? {}) as Record< + string, + unknown + >; + expect(payloadEnv).toMatchObject({ PAPERCLIP_API_BRIDGE_MODE: "queue_v1" }); + expect(String(payloadEnv.PAPERCLIP_API_URL ?? "")).toMatch(/^http:\/\/127\.0\.0\.1:\d+$/); + expect(payloadEnv.PAPERCLIP_API_KEY).toBeTruthy(); + expect(payloadEnv.PAPERCLIP_API_KEY).not.toBe("real-run-jwt"); + }); + + it("stops the process-session bridge when the paperclip bridge fails under concurrency", async () => { + const { stateDir, localCwd, executionTarget } = await setupRemoteSandbox(); + // The paperclip bridge fails; the process-session bridge — started CONCURRENTLY + // with it — still resolves a live handle. The abandon path must stop that + // handle so no started bridge leaks on partial failure. + const stop = vi.fn(async () => {}); + vi.mocked(startAdapterExecutionTargetPaperclipBridge).mockImplementationOnce(async () => { + throw new Error("paperclip bridge boom"); + }); + vi.mocked(startAdapterExecutionTargetProcessSessionBridge).mockImplementationOnce( + async () => ({ agentCommand: null, stop }) as never, + ); + + const execute = createAcpxEngineExecutor({ + createRuntime: () => buildRuntime() as never, + }); + + await expect( + execute({ + runId: "run-bridge-fail", + agent: { id: "agent-1", companyId: "company-1" }, + runtime: {}, + config: { agent: "custom", agentCommand: "node ./fake-acp.js", stateDir, cwd: localCwd }, + context: {}, + authToken: "real-run-jwt", + executionTarget, + onLog: async () => {}, + onMeta: async () => {}, + onEvent: async () => {}, + } as never), + ).rejects.toThrow("paperclip bridge boom"); + + // The concurrently-started process-session bridge was stopped exactly once. + expect(stop).toHaveBeenCalledTimes(1); + }); + it("test_remote_session_new_uses_in_sandbox_cwd", async () => { const { stateDir, localCwd, remoteCwd, executionTarget } = await setupRemoteSandbox(); const { sessionInputs, runtimeOptions } = await runExecutor( @@ -2795,7 +2874,7 @@ describe("ACPX engine per-step startup timing (run.startup.step events)", () => } }); - it("carries per-step roundTrips + provider durations sourced from the sandbox runner counter", async () => { + it("carries roundTrips + provider durations for sequential startup steps and keeps concurrent bridge steps duration-only", async () => { const root = await makeTempRoot(); const stateDir = path.join(root, "state"); const localCwd = path.join(root, "worktree"); @@ -2818,10 +2897,13 @@ describe("ACPX engine per-step startup timing (run.startup.step events)", () => const steps = stepEvents(events); const seen = new Map(steps.map((event) => [String(event.payload?.step), event])); - // Every timed boundary carries a numeric roundTrips (the runner exposes - // execCount), even the ones that never exec. + // Every timed boundary still records duration. for (const event of steps) { - expect(typeof event.payload?.roundTrips).toBe("number"); + expect(typeof event.payload?.durationMs).toBe("number"); + } + // Sequential boundaries retain runner-counter attribution. + for (const step of ["workspace.resolve", "stage.sync", "acp.handshake"]) { + expect(typeof seen.get(step)?.payload?.roundTrips).toBe("number"); } // workspace.resolve is host-only → zero host→sandbox execs. expect(seen.get("workspace.resolve")?.payload?.roundTrips).toBe(0); @@ -2835,6 +2917,13 @@ describe("ACPX engine per-step startup timing (run.startup.step events)", () => expect(stageSync?.payload?.providerGetMs).toBe( (stageSync?.payload?.roundTrips as number) * 15, ); + // Concurrent bridge steps are duration-only so they do not double-count + // shared runner counters while their lifecycles overlap. + for (const step of ["bridge.paperclip", "bridge.process-session"]) { + expect(seen.get(step)?.payload?.roundTrips).toBeUndefined(); + expect(seen.get(step)?.payload?.providerExecMs).toBeUndefined(); + expect(seen.get(step)?.payload?.providerGetMs).toBeUndefined(); + } // The external ACP client crosses no host exec seam. expect(seen.get("acp.handshake")?.payload?.roundTrips).toBe(0); }); diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index 5c1ec5937e5..67c4aae5a2c 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -1292,6 +1292,12 @@ async function buildRuntime(input: { ? executionTarget.runner : undefined, ); + // The two bridge-start steps intentionally overlap, so their runner counters + // would double-count each other if we sampled them here. Keep the shared + // counter attribution on the sequential startup phases only; the concurrent + // bridge steps still emit duration telemetry, just not misleading per-step + // round-trip/provider deltas. + const concurrentBridgeStepMetrics: StartupStepMeasureOptions = {}; const shapedWorkspaceEnv = shapePaperclipWorkspaceEnvForExecution({ workspaceCwd: effectiveWorkspaceCwd, workspaceWorktreePath, @@ -1747,8 +1753,21 @@ async function buildRuntime(input: { let runtimeEnv: Record = {}; try { if (useRemoteProcessSession) { - // Step 5 — bridge.paperclip: start the sandbox ACP API callback bridge. - paperclipBridge = await measureStartupStep(input.ctx, nowMs, "bridge.paperclip", () => + // Steps 5 + 6 — bring up BOTH host-side sandbox bridges concurrently. Their + // remote subtrees are disjoint (`…/paperclip-bridge/…` vs + // `…/process-sessions/…`), so the env-INDEPENDENT setup of each overlaps, + // trending wall time from serial (~bridge.paperclip + ~bridge.process-session) + // toward ~max(the two). The ONE real dependency — the paperclip bridge's + // returned `env` must reach the process-session LAUNCH — is sequenced by + // `finalizeLaunchEnv`: the process-session bridge runs its env-independent + // dir/script setup first, then awaits that thunk right before its launch, so + // the launch always observes the merged paperclip env. + // + // Measurement caveat: both starts share ONE runner counter, so their + // overlapping `providerExecMs`/`roundTrips` deltas are approximate (the same + // caveat as `acp.handshake`). Both `run.startup.step` events still emit — + // `measureStartupStep` records them in a `finally`, even on a start failure. + const paperclipStart = measureStartupStep(input.ctx, nowMs, "bridge.paperclip", () => startAdapterExecutionTargetPaperclipBridge({ runId, target: { ...executionTarget, streamRunLogs: false }, @@ -1758,38 +1777,59 @@ async function buildRuntime(input: { hostApiToken: env.PAPERCLIP_API_KEY, onLog: input.ctx.onLog, }), - stepMetrics, + concurrentBridgeStepMetrics, ); - if (paperclipBridge) { - Object.assign(env, paperclipBridge.env); - await input.ctx.onLog("stdout", "[paperclip] Sandbox ACP API callback bridge enabled for this run.\n"); - } + // The single sequencing point (paperclip `env` → process-session launch). + // Memoized so the merge + log + `runtimeEnv` build run EXACTLY once whether + // the process-session bridge consumes it at launch or we finalize it below. + let launchEnvPromise: Promise> | null = null; + const finalizeLaunchEnv = (): Promise> => + (launchEnvPromise ??= (async () => { + const paperclip = await paperclipStart; + if (paperclip) { + Object.assign(env, paperclip.env); + await input.ctx.onLog("stdout", "[paperclip] Sandbox ACP API callback bridge enabled for this run.\n"); + } + return (runtimeEnv = resolveRuntimeEnv(env)); + })()); + const processSessionStart = measureStartupStep(input.ctx, nowMs, "bridge.process-session", () => + startAdapterExecutionTargetProcessSessionBridge({ + runId, + target: executionTarget, + runtimeRootDir: stagedRuntime?.runtimeRootDir ?? null, + adapterKey: input.engine.adapterType, + command: "sh", + args: ["-lc", `exec ${agentCommandShell}`], + cwd: sessionCwd, + // Deferred: the process-session bridge runs its env-independent setup, + // then calls this to get the launch env AFTER the paperclip env merge. + env: finalizeLaunchEnv, + timeoutSec, + onLog: input.ctx.onLog, + }), + concurrentBridgeStepMetrics, + ); + // Settle BOTH starts (mirrors `cleanupRemoteBridges`' `Promise.allSettled`): + // collect whichever handles started plus the first failure. Both handles + // stay individually declared so the catch below can stop whichever started. + const started = await settleRemoteBridgeStarts(paperclipStart, processSessionStart); + paperclipBridge = started.paperclipBridge; + processSessionBridge = started.processSessionBridge; + if (started.failure) throw started.failure; + // Guarantee the paperclip env merge ran even if the process-session bridge + // returned without consuming the launch env (memoized ⇒ a no-op if it did). + await finalizeLaunchEnv(); + } else { + // Local / runner-less lanes never start a bridge, but the returned prepared + // runtime and the log builder still read `runtimeEnv`. + runtimeEnv = resolveRuntimeEnv(env); } - runtimeEnv = Object.fromEntries( - Object.entries(ensurePathInEnv({ ...process.env, ...env })).filter( - (entry): entry is [string, string] => typeof entry[1] === "string", - ), - ); - // Step 6 — bridge.process-session: start the in-sandbox process session. - processSessionBridge = useRemoteProcessSession - ? await measureStartupStep(input.ctx, nowMs, "bridge.process-session", () => - startAdapterExecutionTargetProcessSessionBridge({ - runId, - target: executionTarget, - runtimeRootDir: stagedRuntime?.runtimeRootDir ?? null, - adapterKey: input.engine.adapterType, - command: "sh", - args: ["-lc", `exec ${agentCommandShell}`], - cwd: sessionCwd, - env: runtimeEnv, - timeoutSec, - onLog: input.ctx.onLog, - }), - stepMetrics, - ) - : null; } catch (err) { - await paperclipBridge?.stop().catch(() => {}); + // On a partial concurrent bring-up failure, ONE bridge may have started while + // the other threw; `Promise.allSettled` stops whichever started so no live + // bridge leaks (mirrors `cleanupRemoteBridges`). Both handles are individually + // declared above, so either may be non-null here. + await Promise.allSettled([paperclipBridge?.stop(), processSessionBridge?.stop()]); // The staged home / copy-back teardown must run even if a bridge fails to // start after the workspace + managed home were already staged into the // sandbox, so a refreshed credential is copied back on this error path too. @@ -1916,6 +1956,54 @@ async function applySessionConfigOptions(input: { } } +/** + * Build the process-session launch env: the host env overlaid with the run's + * `env` (so the merged paperclip bridge vars win) and a guaranteed `PATH`, + * narrowed to string values. Shared by the remote concurrent bring-up and the + * local / runner-less lane so both resolve the runtime env identically. + */ +function resolveRuntimeEnv(env: Record): Record { + return Object.fromEntries( + Object.entries(ensurePathInEnv({ ...process.env, ...env })).filter( + (entry): entry is [string, string] => typeof entry[1] === "string", + ), + ); +} + +/** + * Bring up the two host-side sandbox bridges concurrently and settle both. + * + * Mirrors `cleanupRemoteBridges`' `Promise.allSettled` idiom (settle, not + * `Promise.all`): running BOTH starts to completion is what lets the caller STOP + * a bridge that DID start when its sibling threw — so a partial failure never + * leaks a live bridge. Returns whichever handles started plus the first failure + * (paperclip before process-session) for the caller to rethrow through the + * shared abandon path. + */ +async function settleRemoteBridgeStarts( + paperclipStart: Promise, + processSessionStart: Promise, +): Promise<{ + paperclipBridge: AdapterExecutionTargetPaperclipBridgeHandle | null; + processSessionBridge: AdapterExecutionTargetProcessSessionBridgeHandle | null; + failure: unknown; +}> { + const [paperclip, processSession] = await Promise.allSettled([ + paperclipStart, + processSessionStart, + ]); + return { + paperclipBridge: paperclip.status === "fulfilled" ? paperclip.value : null, + processSessionBridge: processSession.status === "fulfilled" ? processSession.value : null, + failure: + paperclip.status === "rejected" + ? paperclip.reason + : processSession.status === "rejected" + ? processSession.reason + : null, + }; +} + async function cleanupRemoteBridges(prepared: AcpxPreparedRuntime): Promise { await Promise.allSettled([ prepared.processSessionBridge?.stop(), diff --git a/packages/adapter-utils/src/execution-target.ts b/packages/adapter-utils/src/execution-target.ts index b0a93bed059..3bf072f552b 100644 --- a/packages/adapter-utils/src/execution-target.ts +++ b/packages/adapter-utils/src/execution-target.ts @@ -1303,7 +1303,12 @@ export async function startAdapterExecutionTargetProcessSessionBridge(input: { command: string; args: string[]; cwd: string; - env: Record; + // The launch env is consumed ONLY when building the base64 `commandPayload` + // below — never during the env-INDEPENDENT dir/script setup. Accepting a + // resolver (in addition to a plain object) lets a caller overlap that setup + // with other work — e.g. starting the paperclip callback bridge — and hand the + // merged env in right before the launch. + env: Record | (() => Promise>); timeoutSec?: number | null; onLog?: (stream: "stdout" | "stderr", chunk: string) => Promise; }): Promise { @@ -1339,11 +1344,15 @@ export async function startAdapterExecutionTargetProcessSessionBridge(input: { await client.makeDir(eventsDir); await syncProcessSessionRemoteScript({ client, remoteScriptPath }); + // Resolve the launch env AFTER the env-independent setup above, so a caller + // can defer it until an upstream dependency (e.g. the paperclip bridge's env) + // is ready without blocking the dir/script setup. + const launchEnv = typeof input.env === "function" ? await input.env() : input.env; const commandPayload = Buffer.from(JSON.stringify({ command: input.command, args: input.args, cwd: input.cwd || target.remoteCwd, - env: sanitizeRemoteExecutionEnv(input.env), + env: sanitizeRemoteExecutionEnv(launchEnv), }), "utf8").toString("base64"); await onLog("stdout", `[paperclip] Starting ACP process session bridge in sandbox (${target.providerKey ?? "provider"}).\n`); From f6ab82d490cb314f021a0187f07c56096a70f24e Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:04:46 -0500 Subject: [PATCH 04/87] feat(interactions): add interaction withdrawal and terminal-issue expiry (#10251) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agents and boards coordinate through issue-thread interactions (request_confirmation, ask_user_questions, suggest_tasks, …) that wait as `pending` cards until someone resolves them > - Two lifecycle gaps existed: an interaction's creator could not take back a card it no longer stands behind, and interactions left `pending` on issues that reached a terminal status lingered forever as live-looking approval requests > - Stale pending cards mislead humans (they look actionable), distort attention/liveness signals, and in the worst case invite acting on a proposal whose issue is already closed or cancelled > - This pull request adds an explicit withdraw route for pending interactions and automatically expires pending interactions when their issue reaches a terminal status (including a catch-up sweep for issues closed before this change) > - The benefit is that interaction cards now faithfully reflect reality: only genuinely actionable requests stay pending, and creators can retract requests that events have overtaken ## Linked Issues or Issue Description Fixes #5787 Refs #7403 Related prior PRs found while searching for duplicates (all overlap partially; none combine both lifecycle paths or the route-level authorization used here): #6709 and #7312 (creator-withdraw attempts), #8169 (terminal expiry), #8081 and #5137 (generalized cancel/expire endpoints), #6094 (stale confirmation auto-resolve). Related merged context: #9568 (agent cancel for ask_user_questions), #10119 (tolerating legacy `withdrawn_by_creator` result outcomes — the reader side of the outcome this PR writes). ## What Changed - New route `POST /issues/:id/interactions/:interactionId/withdraw` that resolves a `pending` interaction to status `withdrawn` with a structured result (`outcome: "withdrawn"`, optional trimmed `reason`), stamps `resolvedBy*`/`resolvedAt`, touches the issue, logs activity, and emits resolved-interaction telemetry - Withdrawal authorization: board users, the interaction's creator agent, or the issue's current assignee agent (assignees additionally pass the standard issue-mutation gate); task-watchdog runs are explicitly rejected, and authorization-boundary plus low-trust control-plane checks apply - Withdrawing an already-resolved interaction returns `409`; unknown/cross-issue/cross-company interaction ids return `404` - New service method `expirePendingInteractionsForTerminalIssue`: when an issue transitions to a terminal status, all of its `pending` interactions are resolved to `expired` with `outcome: "issue_closed"`, guarded by a `status = 'pending'` predicate so concurrent resolutions are not overwritten - The same expiry runs as a catch-up when interactions are listed on an already-terminal issue, so cards stranded by issues closed before this change also get cleaned up; expired request_confirmations are logged with a distinguishing source - Shared package: new `withdrawIssueThreadInteractionSchema` validator, `WithdrawIssueThreadInteraction` type, and `withdrawn` / `issue_closed` result-outcome support for all interaction kinds (kind-aware result shapes for `ask_user_questions` and `request_item_verdicts`) - UI helper `ui/src/lib/issue-thread-interactions.ts` recognizes the new outcomes for card rendering - Docs: bundled skill API reference updated with the withdraw endpoint - Review follow-up: terminal expiry moved from the HTTP route hooks into `issueService.update`'s status-transition block, so direct service callers (tree control, recovery, pipelines, status cards) expire pending cards too; the list-endpoint catch-up remains for issues closed before this change - Review follow-up: withdrawing or issue-close-expiring a `request_confirmation` also settles its linked `tool_action_requests` row (withdraw -> `cancelled`, issue closed -> `expired`), so a parked tool call cannot stay approvable after its card is gone - Review follow-up: interaction cards render dedicated copy for the new outcomes ("Withdrawn" with the reason, "Expired when issue closed") instead of falling through to superseded-by-comment / stale-target variants; withdrawn plan reviews badge as "Withdrawn" rather than "Changes requested" ## Screenshots Card states rendered from a local ux-lab harness with mocked data ([full gallery](https://pages.paperclip.ing/pr-10251-interaction-withdrawal-cards/)): | Light | Dark | | --- | --- | | ![All card states, light](https://pages.paperclip.ing/pr-10251-interaction-withdrawal-cards/all-light.png) | ![All card states, dark](https://pages.paperclip.ing/pr-10251-interaction-withdrawal-cards/all-dark.png) | ## Verification - `pnpm --filter @paperclipai/shared build` — clean tsc - `cd server && npx vitest run src/__tests__/issue-thread-interaction-routes.test.ts` — 22 tests pass, including new coverage for: creator-agent withdraw success, non-creator/non-assignee agent 403, watchdog-run 403, double-withdraw 409, and board-user withdraw - `cd server && npx vitest run src/services/issue-thread-interactions.test.ts` — 4 tests pass, including terminal-issue expiry writing `issue_closed` results and leaving already-resolved interactions untouched - `cd ui && pnpm typecheck` — clean - `cd server && npx vitest run src/__tests__/issues-service.test.ts` — includes a new embedded-Postgres test proving a direct `issueService.update` terminal transition expires pending interactions and writes the activity-log entry - `cd ui && npx vitest run src/components/IssueThreadInteractionCard.test.tsx` — 32 tests, including new coverage for withdrawn / issue-closed confirmation and question cards - `cd server && npx tsc --noEmit` — matches the pre-existing repo error baseline exactly (no new errors) - Manual: `POST /issues/:id/interactions/:interactionId/withdraw` with `{"reason":"superseded"}` as the creator agent resolves the card to `withdrawn`; closing an issue with a pending confirmation flips it to `expired` with `outcome: "issue_closed"` ## Risks - Interactions on terminal issues now auto-expire (including retroactively via the list-time catch-up), so consumers that expected to resolve a pending interaction on a closed issue will get `409`; this is the intended semantics and matches how the attention feed already wants to treat dead cards - New result outcomes (`withdrawn`, `issue_closed`) are written to stored results; readers were already made tolerant of these outcome strings in #10119, so mixed-version reads are safe - No schema/migration changes; per-row conditional updates (`status = 'pending'`) avoid clobbering concurrent resolutions - Withdrawal is a new mutation surface, but it is strictly narrower than existing resolve paths (board, creator, or assignee only; watchdog runs blocked) ## Model Used Claude Fable 5 (`claude-fable-5`, Anthropic Mythos-class tier) with extended thinking and agentic tool use (Claude Code harness); commit authored in a Paperclip-managed engineering session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- packages/shared/src/index.ts | 2 + packages/shared/src/types/issue.ts | 9 +- packages/shared/src/validators/index.ts | 2 + packages/shared/src/validators/issue.ts | 14 +- .../issue-thread-interaction-routes.test.ts | 70 +++++ .../issue-thread-interactions-service.test.ts | 22 ++ server/src/__tests__/issues-service.test.ts | 58 ++++ .../__tests__/tool-gateway-service.test.ts | 37 +++ server/src/routes/issues.ts | 99 ++++++- server/src/routes/openapi.ts | 9 + .../issue-thread-interactions.test.ts | 115 ++++++++ .../src/services/issue-thread-interactions.ts | 265 ++++++++++++++++-- server/src/services/issues.ts | 30 ++ server/src/services/tool-gateway.ts | 55 +++- skills/paperclip/SKILL.md | 3 +- skills/paperclip/references/api-reference.md | 4 + .../IssueThreadInteractionCard.test.tsx | 70 +++++ .../components/IssueThreadInteractionCard.tsx | 50 +++- ui/src/lib/issue-thread-interactions.ts | 5 + 19 files changed, 882 insertions(+), 37 deletions(-) diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 66c1227f41f..1c0241ad4a9 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -1615,6 +1615,7 @@ export { acceptIssueThreadInteractionSchema, rejectIssueThreadInteractionSchema, cancelIssueThreadInteractionSchema, + withdrawIssueThreadInteractionSchema, respondIssueThreadInteractionSchema, submitIssueThreadInteractionVerdictsSchema, linkIssueApprovalSchema, @@ -1677,6 +1678,7 @@ export { type AcceptIssueThreadInteraction, type RejectIssueThreadInteraction, type CancelIssueThreadInteraction, + type WithdrawIssueThreadInteraction, type RespondIssueThreadInteraction, type SubmitIssueThreadInteractionVerdicts, type LinkIssueApproval, diff --git a/packages/shared/src/types/issue.ts b/packages/shared/src/types/issue.ts index e4639f87f74..e1e5466ae0d 100644 --- a/packages/shared/src/types/issue.ts +++ b/packages/shared/src/types/issue.ts @@ -996,6 +996,8 @@ export interface SuggestTasksResultCreatedTask { export interface SuggestTasksResult { version: 1; + outcome?: "withdrawn" | "issue_closed"; + reason?: string | null; createdTasks?: SuggestTasksResultCreatedTask[]; skippedClientKeys?: string[]; rejectionReason?: string | null; @@ -1032,6 +1034,8 @@ export interface AskUserQuestionsAnswer { export interface AskUserQuestionsResult { version: 1; + outcome?: "withdrawn" | "issue_closed"; + reason?: string | null; answers: AskUserQuestionsAnswer[]; cancelled?: true; cancellationReason?: string | null; @@ -1166,7 +1170,7 @@ export interface RequestItemVerdictsPayload { export interface RequestConfirmationResult { version: 1; - outcome: "accepted" | "rejected" | "superseded_by_comment" | "stale_target"; + outcome: "accepted" | "rejected" | "superseded_by_comment" | "stale_target" | "withdrawn" | "issue_closed"; reason?: string | null; commentId?: string | null; staleTarget?: RequestConfirmationTarget | null; @@ -1198,7 +1202,8 @@ export interface RequestItemVerdictsResultItem { export interface RequestItemVerdictsResult { version: 1; - outcome: "resolved" | "superseded_by_comment" | "stale_target" | "cancelled"; + outcome: "resolved" | "superseded_by_comment" | "stale_target" | "cancelled" | "withdrawn" | "issue_closed"; + reason?: string | null; complete: boolean; items: RequestItemVerdictsResultItem[]; commentId?: string | null; diff --git a/packages/shared/src/validators/index.ts b/packages/shared/src/validators/index.ts index 69eac14bdf7..d9c01950949 100644 --- a/packages/shared/src/validators/index.ts +++ b/packages/shared/src/validators/index.ts @@ -419,6 +419,7 @@ export { acceptIssueThreadInteractionSchema, rejectIssueThreadInteractionSchema, cancelIssueThreadInteractionSchema, + withdrawIssueThreadInteractionSchema, respondIssueThreadInteractionSchema, submitIssueThreadInteractionVerdictsSchema, linkIssueApprovalSchema, @@ -442,6 +443,7 @@ export { type AcceptIssueThreadInteraction, type RejectIssueThreadInteraction, type CancelIssueThreadInteraction, + type WithdrawIssueThreadInteraction, type RespondIssueThreadInteraction, type SubmitIssueThreadInteractionVerdicts, type LinkIssueApproval, diff --git a/packages/shared/src/validators/issue.ts b/packages/shared/src/validators/issue.ts index af6b7efcff9..ed935a8b38a 100644 --- a/packages/shared/src/validators/issue.ts +++ b/packages/shared/src/validators/issue.ts @@ -718,6 +718,8 @@ export const suggestTasksResultCreatedTaskSchema = z.object({ export const suggestTasksResultSchema = z.object({ version: z.literal(1), + outcome: z.enum(["withdrawn", "issue_closed"]).optional(), + reason: z.string().trim().max(4000).nullable().optional(), createdTasks: z.array(suggestTasksResultCreatedTaskSchema).max(50).optional(), skippedClientKeys: z.array(z.string().trim().min(1).max(120)).max(50).optional(), rejectionReason: z.string().trim().max(4000).nullable().optional(), @@ -778,6 +780,8 @@ export const askUserQuestionsAnswerSchema = z.object({ export const askUserQuestionsResultSchema = z.object({ version: z.literal(1), + outcome: z.enum(["withdrawn", "issue_closed"]).optional(), + reason: z.string().trim().max(4000).nullable().optional(), answers: z.array(askUserQuestionsAnswerSchema).max(20), cancelled: z.literal(true).optional(), cancellationReason: z.string().trim().max(4000).nullable().optional(), @@ -975,7 +979,7 @@ export const requestConfirmationToolActionResultSchema = z.object({ export const requestConfirmationResultSchema = z.object({ version: z.literal(1), - outcome: z.enum(["accepted", "rejected", "superseded_by_comment", "stale_target"]), + outcome: z.enum(["accepted", "rejected", "superseded_by_comment", "stale_target", "withdrawn", "issue_closed"]), reason: z.string().trim().max(4000).nullable().optional(), commentId: z.string().uuid().nullable().optional(), staleTarget: requestConfirmationTargetSchema.nullable().optional(), @@ -1097,7 +1101,8 @@ export const requestItemVerdictsResultItemSchema = z.object({ export const requestItemVerdictsResultSchema = z.object({ version: z.literal(1), - outcome: z.enum(["resolved", "superseded_by_comment", "stale_target", "cancelled"]), + outcome: z.enum(["resolved", "superseded_by_comment", "stale_target", "cancelled", "withdrawn", "issue_closed"]), + reason: z.string().trim().max(4000).nullable().optional(), complete: z.boolean(), items: z.array(requestItemVerdictsResultItemSchema) .max(REQUEST_ITEM_VERDICTS_ITEM_LIMIT), @@ -1216,6 +1221,11 @@ export const cancelIssueThreadInteractionSchema = z.object({ }); export type CancelIssueThreadInteraction = z.infer; +export const withdrawIssueThreadInteractionSchema = z.object({ + reason: z.string().trim().max(4000).optional(), +}); +export type WithdrawIssueThreadInteraction = z.infer; + export const respondIssueThreadInteractionSchema = z.object({ answers: z.array(askUserQuestionsAnswerSchema).max(20), summaryMarkdown: multilineTextSchema.pipe(z.string().max(20000)).nullable().optional(), diff --git a/server/src/__tests__/issue-thread-interaction-routes.test.ts b/server/src/__tests__/issue-thread-interaction-routes.test.ts index 70577f6e722..9476303e82b 100644 --- a/server/src/__tests__/issue-thread-interaction-routes.test.ts +++ b/server/src/__tests__/issue-thread-interaction-routes.test.ts @@ -11,15 +11,18 @@ const mockIssueService = vi.hoisted(() => ({ const mockInteractionService = vi.hoisted(() => ({ listForIssue: vi.fn(), + getForIssue: vi.fn(), create: vi.fn(), acceptInteraction: vi.fn(), acceptSuggestedTasks: vi.fn(), rejectInteraction: vi.fn(), rejectSuggestedTasks: vi.fn(), expireRequestConfirmationsSupersededByHistoricalComments: vi.fn(), + expirePendingInteractionsForTerminalIssue: vi.fn(), answerQuestions: vi.fn(), submitItemVerdicts: vi.fn(), cancelQuestions: vi.fn(), + withdrawInteraction: vi.fn(), })); const mockHeartbeatService = vi.hoisted(() => ({ @@ -186,6 +189,24 @@ describe.sequential("issue thread interaction routes", () => { mockIssueService.getById.mockResolvedValue(createIssue()); mockInteractionService.listForIssue.mockResolvedValue([]); mockInteractionService.expireRequestConfirmationsSupersededByHistoricalComments.mockResolvedValue([]); + mockInteractionService.expirePendingInteractionsForTerminalIssue.mockResolvedValue([]); + mockInteractionService.getForIssue.mockResolvedValue({ + id: "interaction-withdraw", + createdByAgentId: CREATED_AGENT_ID, + continuationPolicy: "wake_assignee", + status: "pending", + }); + mockInteractionService.withdrawInteraction.mockResolvedValue({ + id: "interaction-withdraw", + companyId: "company-1", + issueId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + kind: "request_confirmation", + createdByAgentId: CREATED_AGENT_ID, + status: "cancelled", + continuationPolicy: "wake_assignee", + payload: { version: 1, prompt: "Proceed?" }, + result: { version: 1, outcome: "withdrawn", reason: "Replanning" }, + }); mockInteractionService.create.mockResolvedValue({ id: "interaction-1", companyId: "company-1", @@ -574,6 +595,55 @@ describe.sequential("issue thread interaction routes", () => { ); }); + it("allows a board user to withdraw and wakes the assignee", async () => { + const app = await createApp(); + const res = await request(app) + .post("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/interactions/interaction-withdraw/withdraw") + .send({ reason: "Replanning" }); + + expect(res.status).toBe(200); + expect(mockInteractionService.withdrawInteraction).toHaveBeenCalledWith( + expect.objectContaining({ id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" }), + "interaction-withdraw", + { reason: "Replanning" }, + expect.objectContaining({ userId: "local-board" }), + ); + expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith(ASSIGNEE_AGENT_ID, expect.objectContaining({ + payload: expect.objectContaining({ interactionStatus: "cancelled" }), + })); + expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + action: "issue.thread_interaction_withdrawn", + })); + }); + + it("allows the creator agent to withdraw and wakes a different assignee", async () => { + const app = await createApp({ type: "agent", agentId: CREATED_AGENT_ID, companyId: "company-1", runId: "run-1" }); + const res = await request(app) + .post("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/interactions/interaction-withdraw/withdraw") + .send({}); + expect(res.status).toBe(200); + expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith(ASSIGNEE_AGENT_ID, expect.anything()); + }); + + it("allows the assignee agent to withdraw without waking itself", async () => { + mockIssueService.getById.mockResolvedValueOnce(createIssue({ status: "todo" })); + const app = await createApp({ type: "agent", agentId: ASSIGNEE_AGENT_ID, companyId: "company-1", runId: "run-2" }); + const res = await request(app) + .post("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/interactions/interaction-withdraw/withdraw") + .send({}); + expect(res.status).toBe(200); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); + + it("rejects withdrawal by an unrelated agent", async () => { + const app = await createApp({ type: "agent", agentId: "33333333-3333-4333-8333-333333333333", companyId: "company-1", runId: "run-3" }); + const res = await request(app) + .post("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/interactions/interaction-withdraw/withdraw") + .send({}); + expect(res.status).toBe(403); + expect(mockInteractionService.withdrawInteraction).not.toHaveBeenCalled(); + }); + it("cancels question interactions and emits a continuation wake", async () => { const app = await createApp(); diff --git a/server/src/__tests__/issue-thread-interactions-service.test.ts b/server/src/__tests__/issue-thread-interactions-service.test.ts index f44b968cf4b..096dd95a11d 100644 --- a/server/src/__tests__/issue-thread-interactions-service.test.ts +++ b/server/src/__tests__/issue-thread-interactions-service.test.ts @@ -892,6 +892,28 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { expect(rows[0]?.idempotencyKey).toBe("run-1:questionnaire"); }); + it("refuses to create an interaction on a closed issue", async () => { + const { companyId, issueId } = await seedConfirmationIssue("Closed issue create guard"); + await db.update(issues).set({ status: "done" }).where(eq(issues.id, issueId)); + + await expect(interactionsSvc.create({ + id: issueId, + companyId, + }, { + kind: "request_confirmation", + continuationPolicy: "wake_assignee", + payload: { version: 1, prompt: "Approve after close?" }, + }, { + userId: "local-board", + })).rejects.toMatchObject({ status: 409 }); + + const rows = await db + .select() + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.issueId, issueId)); + expect(rows).toHaveLength(0); + }); + it("accepts request_confirmation interactions without creating child issues", async () => { const companyId = randomUUID(); const goalId = randomUUID(); diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index f238c96493b..677d8a8d188 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -464,6 +464,64 @@ describeEmbeddedPostgres("issueService.list participantAgentId", () => { }); }); + it("expires pending thread interactions on any service-level terminal transition", async () => { + const companyId = await seedAssignableAgentCompany(); + const issue = await svc.create(companyId, { + title: "Close me with a pending card", + description: null, + status: "todo", + priority: "medium", + }); + const interactionId = randomUUID(); + await db.insert(issueThreadInteractions).values({ + id: interactionId, + companyId, + issueId: issue.id, + kind: "ask_user_questions", + status: "pending", + continuationPolicy: "wake_assignee", + payload: { + version: 1, + questions: [{ + id: "scope", + prompt: "Pick one", + selectionMode: "single", + options: [{ id: "a", label: "A" }], + }], + } as never, + }); + + // Direct service callers (tree control, recovery, pipelines, status cards) + // never pass through the HTTP routes, so the expiry must fire here. + const updated = await svc.update(issue.id, { status: "cancelled", actorUserId: "local-board" }); + expect(updated?.status).toBe("cancelled"); + + const interaction = await db + .select() + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, interactionId)) + .then((rows) => rows[0] ?? null); + expect(interaction).toMatchObject({ + status: "expired", + resolvedByUserId: "local-board", + }); + expect(interaction?.result).toMatchObject({ version: 1, outcome: "issue_closed" }); + expect(interaction?.resolvedAt).not.toBeNull(); + + const logged = await db + .select() + .from(activityLog) + .where(eq(activityLog.action, "issue.thread_interaction_expired")); + expect(logged).toHaveLength(1); + // details.source is dot-separated and trips the JWT-shaped redaction + // heuristic in sanitizeRecord, so assert the identifying fields instead. + expect(logged[0]?.details).toMatchObject({ + interactionId, + interactionKind: "ask_user_questions", + interactionStatus: "expired", + }); + }); + it("rejects moving an existing terminated assignment into progress without clearing it", async () => { const companyId = await seedAssignableAgentCompany(); const assigneeAgentId = randomUUID(); diff --git a/server/src/__tests__/tool-gateway-service.test.ts b/server/src/__tests__/tool-gateway-service.test.ts index 2ce5d2f1261..01d953032a1 100644 --- a/server/src/__tests__/tool-gateway-service.test.ts +++ b/server/src/__tests__/tool-gateway-service.test.ts @@ -437,6 +437,43 @@ describeEmbeddedPostgres("tool gateway service", () => { expect(executionEvents).toHaveLength(1); }); + it("refuses to execute an approved action after its issue closes", async () => { + const { company, agent, issue, run } = await createRunFixture(db); + await db.insert(toolPolicies).values({ + companyId: company.id, + name: "Review note writes", + policyType: "require_approval", + selectors: { toolName: "mcp-remote-fixture:update_note" }, + }); + const gateway = createTestToolGatewayService(db); + const session = await gateway.createSession({ companyId: company.id, agentId: agent.id, runId: run.id }); + const parameters = { noteId: "n1", body: "post-close body" }; + + await expect(gateway.executeTool({ + sessionToken: session.token, + tool: "mcp-remote-fixture:update_note", + parameters, + })).rejects.toMatchObject({ reasonCode: "approval_required" }); + const [actionRequest] = await db.select().from(toolActionRequests); + const now = new Date(); + await db.update(toolActionRequests).set({ status: "approved", decidedAt: now, resolvedAt: now }).where(eq(toolActionRequests.id, actionRequest.id)); + await db.update(issues).set({ status: "done" }).where(eq(issues.id, issue.id)); + + await expect(gateway.executeTool({ + sessionToken: session.token, + tool: "mcp-remote-fixture:update_note", + parameters, + })).rejects.toMatchObject({ reasonCode: "action_issue_closed" }); + + const [settled] = await db.select().from(toolActionRequests).where(eq(toolActionRequests.id, actionRequest.id)); + expect(settled?.status).toBe("expired"); + const executionEvents = await db.select().from(toolCallEvents).where(and( + eq(toolCallEvents.actionRequestId, actionRequest.id), + eq(toolCallEvents.reasonCode, "approved_action_executed"), + )); + expect(executionEvents).toHaveLength(0); + }); + it("keeps pre-execute-on-approve approved requests inert", async () => { const { company, agent, run } = await createRunFixture(db); await db.insert(toolPolicies).values({ diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 6af7134cda2..adaaa35aced 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -31,6 +31,7 @@ import { acceptIssueThreadInteractionSchema, attachmentArtifactWorkProductMetadataSchema, cancelIssueThreadInteractionSchema, + withdrawIssueThreadInteractionSchema, companySearchExtractQuerySchema, companySearchQuerySchema, createIssueAttachmentMetadataSchema, @@ -3733,6 +3734,41 @@ export function issueRoutes( return true; } + async function assertIssueThreadInteractionWithdrawalAllowed( + req: Request, + res: Response, + issue: Parameters[2], + interaction: { createdByAgentId?: string | null }, + ) { + if (req.actor.type !== "agent") { + assertBoard(req); + return true; + } + const actorAgentId = req.actor.agentId; + if (!actorAgentId || !requireAgentRunId(req, res)) return false; + + const watchdogScope = await resolveTaskWatchdogMutationScope(db, req.actor); + if (watchdogScope.kind !== "none") { + res.status(403).json({ error: "Task-watchdog runs cannot withdraw issue-thread interactions" }); + return false; + } + const boundaryDecision = await decideIssueAccess(req, issue, "issue:mutate"); + if (!boundaryDecision.allowed) { + res.status(403).json({ error: "Issue is outside this actor's authorization boundary" }); + return false; + } + if (await assertLowTrustControlPlaneDenied(req, res, issue.companyId, issue)) return false; + + const isCreator = interaction.createdByAgentId === actorAgentId; + const isAssignee = issue.assigneeAgentId === actorAgentId; + if (!isCreator && !isAssignee) { + res.status(403).json({ error: "Only the interaction creator, current issue assignee, or a board user may withdraw it" }); + return false; + } + if (isAssignee) return assertAgentIssueMutationAllowed(req, res, issue); + return true; + } + async function assertTaskWatchdogCreateIssueAllowed( req: Request, res: Response, @@ -9211,13 +9247,23 @@ export function issueRoutes( if (!(await assertIssueReadAllowed(req, res, issue))) return; const actor = getActorInfo(req); const interactionSvc = issueThreadInteractionService(db); - const expiredInteractions = await interactionSvc.expireRequestConfirmationsSupersededByHistoricalComments(issue); + const supersededInteractions = await interactionSvc.expireRequestConfirmationsSupersededByHistoricalComments(issue); await logExpiredRequestConfirmations({ issue, - interactions: expiredInteractions, + interactions: supersededInteractions, actor, source: "issue.interactions.catchup_superseded_by_comment", }); + const closedIssueInteractions = await interactionSvc.expirePendingInteractionsForTerminalIssue(issue, { + agentId: actor.agentId, + userId: actor.actorType === "user" ? actor.actorId : null, + }); + await logExpiredRequestConfirmations({ + issue, + interactions: closedIssueInteractions, + actor, + source: "issue.interactions.catchup_issue_closed", + }); const interactions = await interactionSvc.listForIssue(id); res.json(interactions); @@ -9586,6 +9632,55 @@ export function issueRoutes( }, ); + router.post( + "/issues/:id/interactions/:interactionId/withdraw", + validate(withdrawIssueThreadInteractionSchema), + async (req, res) => { + const id = req.params.id as string; + const interactionId = req.params.interactionId as string; + const issue = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); + if (!issue) return; + + const interactionSvc = issueThreadInteractionService(db); + const current = await interactionSvc.getForIssue(issue, interactionId); + if (!(await assertIssueThreadInteractionWithdrawalAllowed(req, res, issue, current))) return; + + const actor = getActorInfo(req); + const interaction = await interactionSvc.withdrawInteraction(issue, interactionId, req.body, { + agentId: actor.agentId, + userId: actor.actorType === "user" ? actor.actorId : null, + }); + await logActivity(db, { + companyId: issue.companyId, + actorType: actor.actorType, + actorId: actor.actorId, + agentId: actor.agentId, + runId: actor.runId, + agentApiKeyId: actor.agentApiKeyId, + action: "issue.thread_interaction_withdrawn", + entityType: "issue", + entityId: issue.id, + details: { + interactionId: interaction.id, + interactionKind: interaction.kind, + interactionStatus: interaction.status, + reason: interaction.result && "reason" in interaction.result ? interaction.result.reason ?? null : null, + }, + }); + + if (actor.agentId !== issue.assigneeAgentId) { + queueResolvedInteractionContinuationWakeup({ + heartbeat, + issue, + interaction, + actor, + source: "issue.interaction.withdraw", + }); + } + res.json(interaction); + }, + ); + router.post( "/issues/:id/interactions/:interactionId/cancel", validate(cancelIssueThreadInteractionSchema), diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index 9ec800e9f89..6db43c67616 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -164,6 +164,7 @@ import { createAcceptedPlanDecompositionSchema, resolveIssueRecoveryActionSchema, cancelIssueThreadInteractionSchema, + withdrawIssueThreadInteractionSchema, // Secret provider configs and remote import createSecretProviderConfigSchema, updateSecretProviderConfigSchema, @@ -5909,6 +5910,14 @@ registerCurrentRoute({ body: cancelIssueThreadInteractionSchema, }); +registerCurrentRoute({ + method: "post", + path: "/api/issues/{id}/interactions/{interactionId}/withdraw", + tags: ["issues"], + summary: "Withdraw a pending issue thread interaction", + body: withdrawIssueThreadInteractionSchema, +}); + for (const route of [ ["get", "/api/routines/{id}/revisions", "List routine revisions"], ["post", "/api/routines/{id}/revisions/{revisionId}/restore", "Restore a routine revision"], diff --git a/server/src/services/issue-thread-interactions.test.ts b/server/src/services/issue-thread-interactions.test.ts index d2cd4205ce4..27d132867b6 100644 --- a/server/src/services/issue-thread-interactions.test.ts +++ b/server/src/services/issue-thread-interactions.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; +import { getTableName } from "drizzle-orm"; const mockCreateChild = vi.fn(); @@ -33,6 +34,7 @@ function createFakeDb(args: { let interactionRow = { ...args.interactionRow }; const issueTouches: Array> = []; const interactionUpdates: Array> = []; + const toolActionRequestUpdates: Array> = []; let selectCallCount = 0; const db: any = { @@ -44,6 +46,10 @@ function createFakeDb(args: { set(values: Record) { return { where() { + if (getTableName(table as never) === "tool_action_requests") { + toolActionRequestUpdates.push(values); + return Promise.resolve(undefined); + } if ("status" in values || "result" in values || "resolvedAt" in values) { interactionUpdates.push(values); interactionRow = { ...interactionRow, ...values }; @@ -69,6 +75,7 @@ function createFakeDb(args: { getInteractionRow: () => interactionRow, issueTouches, interactionUpdates, + toolActionRequestUpdates, }; } @@ -212,4 +219,112 @@ describe("issueThreadInteractionService", () => { expect(state.interactionUpdates).toHaveLength(1); expect(state.issueTouches).toHaveLength(1); }); + + it("withdraws a pending interaction with attribution and rejects repeats", async () => { + const { issueThreadInteractionService } = await import("./issue-thread-interactions.js"); + const interactionRow = { + id: "interaction-withdraw", companyId: "company-1", issueId: "11111111-1111-4111-8111-111111111111", + kind: "request_confirmation", status: "pending", continuationPolicy: "wake_assignee", + sourceCommentId: null, sourceRunId: null, title: null, summary: null, + createdByAgentId: "agent-1", createdByUserId: null, resolvedByAgentId: null, resolvedByUserId: null, + payload: { version: 1, prompt: "Proceed?" }, result: null, resolvedAt: null, + createdAt: new Date("2026-07-25T10:00:00.000Z"), updatedAt: new Date("2026-07-25T10:00:00.000Z"), + }; + const state = createFakeDb({ interactionRow }); + const svc = issueThreadInteractionService(state.db as never); + const withdrawn = await svc.withdrawInteraction({ id: interactionRow.issueId, companyId: "company-1" }, interactionRow.id, { reason: "Replanning" }, { agentId: "agent-1" }); + expect(withdrawn.status).toBe("cancelled"); + expect(withdrawn.result).toEqual({ version: 1, outcome: "withdrawn", reason: "Replanning" }); + expect(withdrawn.resolvedByAgentId).toBe("agent-1"); + expect(state.toolActionRequestUpdates).toHaveLength(1); + expect(state.toolActionRequestUpdates[0]).toMatchObject({ status: "cancelled", resolvedByAgentId: "agent-1" }); + const resolvedState = createFakeDb({ interactionRow: { ...interactionRow, status: "accepted" } }); + const resolvedSvc = issueThreadInteractionService(resolvedState.db as never); + await expect(resolvedSvc.withdrawInteraction( + { id: interactionRow.issueId, companyId: "company-1" }, + interactionRow.id, + {}, + { agentId: "agent-1" }, + )).rejects.toMatchObject({ status: 409 }); + }); + + it("refuses withdrawal when the linked tool action is already executing", async () => { + const { issueThreadInteractionService } = await import("./issue-thread-interactions.js"); + const interactionRow = { + id: "interaction-executing", companyId: "company-1", issueId: "11111111-1111-4111-8111-111111111111", + kind: "request_confirmation", status: "pending", continuationPolicy: "wake_assignee", + sourceCommentId: null, sourceRunId: null, title: null, summary: null, + createdByAgentId: "agent-1", createdByUserId: null, resolvedByAgentId: null, resolvedByUserId: null, + payload: { version: 1, prompt: "Proceed?" }, result: null, resolvedAt: null, + createdAt: new Date("2026-07-25T10:00:00.000Z"), updatedAt: new Date("2026-07-25T10:00:00.000Z"), + }; + const state = createFakeDb({ interactionRow, parentRows: [{ id: "action-request-1" }] }); + const svc = issueThreadInteractionService(state.db as never); + await expect(svc.withdrawInteraction( + { id: interactionRow.issueId, companyId: "company-1" }, + interactionRow.id, + {}, + { agentId: "agent-1" }, + )).rejects.toMatchObject({ status: 409 }); + expect(state.interactionUpdates).toHaveLength(0); + }); + + it("expires pending interactions when the issue is terminal", async () => { + const { issueThreadInteractionService } = await import("./issue-thread-interactions.js"); + const interactionRow = { + id: "interaction-close", companyId: "company-1", issueId: "11111111-1111-4111-8111-111111111111", + kind: "ask_user_questions", status: "pending", continuationPolicy: "wake_assignee", + sourceCommentId: null, sourceRunId: null, title: null, summary: null, + createdByAgentId: "agent-1", createdByUserId: null, resolvedByAgentId: null, resolvedByUserId: null, + payload: { version: 1, questions: [{ id: "q", prompt: "Q?", selectionMode: "single", options: [{ id: "a", label: "A" }] }] }, + result: null, resolvedAt: null, createdAt: new Date("2026-07-25T10:00:00.000Z"), updatedAt: new Date("2026-07-25T10:00:00.000Z"), + }; + const state = createFakeDb({ interactionRow }); + const svc = issueThreadInteractionService(state.db as never); + const expired = await svc.expirePendingInteractionsForTerminalIssue({ id: interactionRow.issueId, companyId: "company-1", status: "done" }); + expect(expired).toHaveLength(1); + expect(expired[0]?.status).toBe("expired"); + expect(expired[0]?.result).toMatchObject({ version: 1, outcome: "issue_closed", answers: [] }); + expect(state.toolActionRequestUpdates).toHaveLength(0); + }); + + it("expires the linked tool action request when a terminal issue closes a confirmation card", async () => { + const { issueThreadInteractionService } = await import("./issue-thread-interactions.js"); + const interactionRow = { + id: "interaction-tool", companyId: "company-1", issueId: "11111111-1111-4111-8111-111111111111", + kind: "request_confirmation", status: "pending", continuationPolicy: "wake_assignee", + sourceCommentId: null, sourceRunId: null, title: null, summary: null, + createdByAgentId: "agent-1", createdByUserId: null, resolvedByAgentId: null, resolvedByUserId: null, + payload: { + version: 1, + prompt: "Run the parked tool call?", + toolAction: { + version: 1, + actionRequestId: "33333333-3333-4333-8333-333333333333", + invocationId: "44444444-4444-4444-8444-444444444444", + toolName: "deploy", + toolDisplayName: "Deploy", + connectionId: null, + applicationId: null, + appDisplayName: null, + risk: "write", + previewMarkdown: "Deploy the current build.", + argumentsSummaryJson: "{}", + argumentsHash: "hash-1", + expiresAt: "2026-07-25T11:00:00.000Z", + }, + }, + result: null, resolvedAt: null, createdAt: new Date("2026-07-25T10:00:00.000Z"), updatedAt: new Date("2026-07-25T10:00:00.000Z"), + }; + const state = createFakeDb({ interactionRow }); + const svc = issueThreadInteractionService(state.db as never); + const expired = await svc.expirePendingInteractionsForTerminalIssue( + { id: interactionRow.issueId, companyId: "company-1", status: "cancelled" }, + { userId: "local-board" }, + ); + expect(expired).toHaveLength(1); + expect(expired[0]?.result).toMatchObject({ version: 1, outcome: "issue_closed" }); + expect(state.toolActionRequestUpdates).toHaveLength(1); + expect(state.toolActionRequestUpdates[0]).toMatchObject({ status: "expired", resolvedByUserId: "local-board" }); + }); }); diff --git a/server/src/services/issue-thread-interactions.ts b/server/src/services/issue-thread-interactions.ts index 3d3ef7fd7bf..5c7820b34fa 100644 --- a/server/src/services/issue-thread-interactions.ts +++ b/server/src/services/issue-thread-interactions.ts @@ -9,6 +9,7 @@ import { issueDocuments, issueThreadInteractions, issues, + toolActionRequests, } from "@paperclipai/db"; import { trackInteractionResolved } from "@paperclipai/shared/telemetry"; import type { @@ -29,6 +30,7 @@ import type { SuggestTasksInteraction, SuggestTasksResultCreatedTask, SubmitIssueThreadInteractionVerdicts, + WithdrawIssueThreadInteraction, } from "@paperclipai/shared"; import { acceptIssueThreadInteractionSchema, @@ -46,6 +48,7 @@ import { suggestTasksPayloadSchema, suggestTasksResultSchema, submitIssueThreadInteractionVerdictsSchema, + withdrawIssueThreadInteractionSchema, } from "@paperclipai/shared"; import { z } from "zod"; import { conflict, notFound, unprocessable } from "../errors.js"; @@ -345,6 +348,67 @@ function buildStaleTargetResult( } as const; } +function buildAdministrativeOutcomeResult( + row: IssueThreadInteractionRow, + outcome: "withdrawn" | "issue_closed", + reason: string | null = null, +) { + if (row.kind === "ask_user_questions") { + return { version: 1, outcome, reason, answers: [], summaryMarkdown: null } as const; + } + if (row.kind === "request_item_verdicts") { + const interaction = hydrateInteraction(row) as RequestItemVerdictsInteraction; + return { + version: 1, + outcome, + reason, + complete: false, + items: interaction.result?.items ?? [], + } satisfies RequestItemVerdictsResult; + } + return { version: 1, outcome, reason } as const; +} + +// Rollback sentinel: the interaction was resolved by another actor between the +// pending-rows read and the conditional update, so the enclosing transaction's +// tool-action revocation must be undone. +class InteractionResolvedConcurrentlyError extends Error { + constructor() { + super("Interaction was resolved concurrently"); + } +} + +// A request_confirmation card can govern a parked tool call via a linked +// tool_action_requests row. Administrative resolutions (withdraw, terminal-issue +// expiry) must settle that row too, or the parked call stays approvable under +// its own one-hour lifecycle after its card is gone. +async function resolveLinkedToolActionRequests( + db: Pick, + interaction: Pick, + outcome: { + status: "expired" | "cancelled"; + fromStatuses: Array<"pending" | "approved">; + actor: InteractionActor; + now: Date; + }, +) { + if (interaction.kind !== "request_confirmation") return; + await db + .update(toolActionRequests) + .set({ + status: outcome.status, + resolvedByAgentId: outcome.actor.agentId ?? null, + resolvedByUserId: outcome.actor.userId ?? null, + resolvedAt: outcome.now, + updatedAt: outcome.now, + }) + .where(and( + eq(toolActionRequests.companyId, interaction.companyId), + eq(toolActionRequests.interactionId, interaction.id), + inArray(toolActionRequests.status, outcome.fromStatuses), + )); +} + function resolveActorKind(interaction: Pick) { if (interaction.resolvedByAgentId) return "agent"; if (interaction.resolvedByUserId) return "user"; @@ -900,6 +964,18 @@ export function issueThreadInteractionService(db: Db) { .then((rows) => rows[0] ?? null); } + async function getForIssue(issue: { id: string; companyId: string }, interactionId: string) { + const current = await db + .select() + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, interactionId)) + .then((rows) => rows[0] ?? null); + if (!current || current.companyId !== issue.companyId || current.issueId !== issue.id) { + throw notFound("Interaction not found"); + } + return hydrateInteraction(current); + } + async function assertIssueWorkspaceFinalizedForAccept(args: { db: Pick; issue: { id: string; companyId: string }; @@ -1110,6 +1186,7 @@ export function issueThreadInteractionService(db: Db) { } return { + getForIssue, listForIssue: async (issueId: string) => { const rows = await db .select() @@ -1194,24 +1271,42 @@ export function issueThreadInteractionService(db: Db) { let created: IssueThreadInteractionRow; try { - [created] = await db - .insert(issueThreadInteractions) - .values({ - companyId: issue.companyId, - issueId: issue.id, - kind: data.kind, - status: "pending", - continuationPolicy: data.continuationPolicy, - idempotencyKey: data.idempotencyKey ?? null, - sourceCommentId: data.sourceCommentId ?? null, - sourceRunId: data.sourceRunId ?? null, - title: data.title ?? null, - summary: data.summary ?? null, - createdByAgentId: actor.agentId ?? null, - createdByUserId: actor.userId ?? null, - payload: data.payload, - }) - .returning(); + // A terminal issue must not regain pending actionable cards. FOR SHARE + // on the issue row serializes this insert against the terminal status + // transition's row lock: either the close committed first and this + // read rejects the create, or the insert commits before the close + // proceeds and the close's expiry sweep collects the new row. + // Idempotent reuse above stays allowed so retries of a pre-close + // create keep returning the (by now expired) original. + created = await db.transaction(async (tx) => { + const [issueRow] = await tx + .select({ status: issues.status }) + .from(issues) + .where(and(eq(issues.id, issue.id), eq(issues.companyId, issue.companyId))) + .for("share"); + if (!issueRow || isTerminalIssueStatus(issueRow.status)) { + throw conflict("Cannot create an interaction on a closed issue"); + } + const [row] = await tx + .insert(issueThreadInteractions) + .values({ + companyId: issue.companyId, + issueId: issue.id, + kind: data.kind, + status: "pending", + continuationPolicy: data.continuationPolicy, + idempotencyKey: data.idempotencyKey ?? null, + sourceCommentId: data.sourceCommentId ?? null, + sourceRunId: data.sourceRunId ?? null, + title: data.title ?? null, + summary: data.summary ?? null, + createdByAgentId: actor.agentId ?? null, + createdByUserId: actor.userId ?? null, + payload: data.payload, + }) + .returning(); + return row; + }); } catch (error) { if (!data.idempotencyKey || !isIssueThreadInteractionIdempotencyConflict(error)) { throw error; @@ -1878,6 +1973,140 @@ export function issueThreadInteractionService(db: Db) { return expired; }, + expirePendingInteractionsForTerminalIssue: async ( + issue: { id: string; companyId: string; status: string }, + actor: InteractionActor = {}, + ) => { + if (!isTerminalIssueStatus(issue.status)) return []; + const rows = await db + .select() + .from(issueThreadInteractions) + .where(and( + eq(issueThreadInteractions.companyId, issue.companyId), + eq(issueThreadInteractions.issueId, issue.id), + eq(issueThreadInteractions.status, "pending"), + )); + if (rows.length === 0) return []; + + const now = new Date(); + const expired: IssueThreadInteraction[] = []; + for (const row of rows) { + // Same ordering as withdrawal: revoke the linked tool action before + // resolving the card, inside one transaction. A concurrent gateway + // claim (approved -> executing) blocks on the revocation's row lock + // and then aborts; if the card was concurrently resolved instead, the + // no-row update below rolls the revocation back. A claim that already + // committed is in flight and cannot be recalled — the card still + // expires and the execution result lands on it via the gateway's + // lifecycle reflection. + const updated = await db.transaction(async (tx) => { + await resolveLinkedToolActionRequests(tx, row, { + status: "expired", + fromStatuses: ["pending", "approved"], + actor, + now, + }); + const [resolved] = await tx + .update(issueThreadInteractions) + .set({ + status: "expired", + result: buildAdministrativeOutcomeResult(row, "issue_closed"), + resolvedByAgentId: actor.agentId ?? null, + resolvedByUserId: actor.userId ?? null, + resolvedAt: now, + updatedAt: now, + }) + .where(and( + eq(issueThreadInteractions.id, row.id), + eq(issueThreadInteractions.status, "pending"), + )) + .returning(); + if (!resolved) throw new InteractionResolvedConcurrentlyError(); + return resolved; + }).catch((err: unknown) => { + if (err instanceof InteractionResolvedConcurrentlyError) return null; + throw err; + }); + if (updated) expired.push(hydrateInteraction(updated)); + } + if (expired.length > 0) { + await touchIssue(db, issue.id); + await emitResolvedInteractionsTelemetry(db, expired); + } + return expired; + }, + + withdrawInteraction: async ( + issue: { id: string; companyId: string }, + interactionId: string, + input: WithdrawIssueThreadInteraction, + actor: InteractionActor, + ) => { + const data = withdrawIssueThreadInteractionSchema.parse(input); + const current = await db + .select() + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, interactionId)) + .then((rows) => rows[0] ?? null); + if (!current || current.companyId !== issue.companyId || current.issueId !== issue.id) { + throw notFound("Interaction not found"); + } + if (current.status !== "pending") throw conflict("Interaction has already been resolved"); + + const reason = data.reason?.trim() || null; + const now = new Date(); + // One transaction, linked tool action first: revoking pending/approved + // requests before resolving the card means a concurrent gateway claim + // (approved -> executing) either loses to the revocation's row lock or + // is detected below and aborts the withdrawal; a concurrent card + // resolution rolls the revocation back via the status="pending" guard. + // "approved" is revoked too — the request can be approved from the tool + // review queue while the card is still pending, and an executable + // request must not outlive a withdrawn card. + const updated = await db.transaction(async (tx) => { + await resolveLinkedToolActionRequests(tx, current, { + status: "cancelled", + fromStatuses: ["pending", "approved"], + actor, + now, + }); + if (current.kind === "request_confirmation") { + const active = await tx + .select({ id: toolActionRequests.id }) + .from(toolActionRequests) + .where(and( + eq(toolActionRequests.companyId, current.companyId), + eq(toolActionRequests.interactionId, current.id), + inArray(toolActionRequests.status, ["executing", "executed"]), + )) + .then((rows) => rows[0] ?? null); + if (active) throw conflict("The linked tool action is already executing and can no longer be withdrawn"); + } + const [row] = await tx + .update(issueThreadInteractions) + .set({ + status: "cancelled", + result: buildAdministrativeOutcomeResult(current, "withdrawn", reason), + resolvedByAgentId: actor.agentId ?? null, + resolvedByUserId: actor.userId ?? null, + resolvedAt: now, + updatedAt: now, + }) + .where(and( + eq(issueThreadInteractions.id, interactionId), + eq(issueThreadInteractions.status, "pending"), + )) + .returning(); + if (!row) throw conflict("Interaction has already been resolved"); + return row; + }); + + await touchIssue(db, issue.id); + const withdrawn = hydrateInteraction(updated); + await emitInteractionResolvedTelemetry(db, withdrawn); + return withdrawn; + }, + answerQuestions: async ( issue: { id: string; companyId: string }, interactionId: string, diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index aa9cf5a89fc..b5e6811cf30 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -109,6 +109,7 @@ import { classifyIssueGraphLiveness, type IssueLivenessFinding } from "./recover import { visibleIssueCondition } from "./issue-visibility.js"; import { finalizeStatusCardsForStalledGeneration } from "./status-card-finalization.js"; import { finalizeSummarySlotsForTerminalIssue } from "./summary-slot-finalization.js"; +import { logActivity } from "./activity-log.js"; const ALL_ISSUE_STATUSES = ["backlog", "todo", "in_progress", "in_review", "blocked", "done", "cancelled"]; const MAX_ISSUE_COMMENT_PAGE_LIMIT = 500; @@ -6747,6 +6748,35 @@ export function issueService(db: Db) { if (existing.status !== updated.status) { if (updated.status === "done" || updated.status === "cancelled") { await finalizeSummarySlotsForTerminalIssue(tx, updated); + // Every terminal transition funnels through here, including direct + // service callers (tree control, recovery, pipelines, status cards) + // that never touch the HTTP routes, so pending interaction cards + // cannot outlive their issue. Dynamic import breaks the module + // cycle (issue-thread-interactions.js imports issueService). + const { issueThreadInteractionService } = await import("./issue-thread-interactions.js"); + const expiredInteractions = await issueThreadInteractionService(tx).expirePendingInteractionsForTerminalIssue( + updated, + { agentId: actorAgentId ?? null, userId: actorUserId ?? null }, + ); + for (const interaction of expiredInteractions) { + await logActivity(tx as unknown as Db, { + companyId: updated.companyId, + actorType: actorAgentId ? "agent" : actorUserId ? "user" : "system", + actorId: actorAgentId ?? actorUserId ?? "issue_service", + agentId: actorAgentId ?? null, + action: "issue.thread_interaction_expired", + entityType: "issue", + entityId: updated.id, + details: { + identifier: updated.identifier ?? null, + interactionId: interaction.id, + interactionKind: interaction.kind, + interactionStatus: interaction.status, + source: "issue.status_transition.issue_closed", + result: interaction.result ?? null, + }, + }); + } } // A status-card generation task that goes done/cancelled/blocked stops // making progress; release the card's generation claim so the board tile diff --git a/server/src/services/tool-gateway.ts b/server/src/services/tool-gateway.ts index 9ec481000ac..dcb76806941 100644 --- a/server/src/services/tool-gateway.ts +++ b/server/src/services/tool-gateway.ts @@ -4070,6 +4070,39 @@ export function createToolGatewayService( return { reasonCode, message }; } + // Guard for approved-action execution: the issue must still be open. Expires + // the claimed request and reflects the interaction lifecycle when it is not. + // Called twice — after winning the executing claim, and again immediately + // before provider dispatch, because tool/snapshot resolution between the two + // involves network calls and leaves a seconds-wide window for the issue to + // close. + async function assertIssueOpenForApprovedAction(input: { + claimed: typeof toolActionRequests.$inferSelect; + invocation: typeof toolInvocations.$inferSelect; + }): Promise<{ projectId: string | null }> { + const { claimed, invocation } = input; + const [issue] = await db + .select({ status: issues.status, projectId: issues.projectId }) + .from(issues) + .where(and(eq(issues.id, invocation.issueId!), eq(issues.companyId, invocation.companyId))) + .limit(1); + if (issue && issue.status !== "done" && issue.status !== "cancelled") { + return { projectId: issue.projectId }; + } + const expiredAt = new Date(); + await db + .update(toolActionRequests) + .set({ status: "expired", resolvedAt: expiredAt, updatedAt: expiredAt }) + .where(eq(toolActionRequests.id, claimed.id)); + await reflectToolActionInteractionLifecycle({ actionRequestId: claimed.id, status: "expired" }); + throw new ToolGatewayHttpError( + 409, + "The issue for this tool action is closed; the approval has expired", + "action_issue_closed", + { actionRequestId: claimed.id, invocationId: invocation.id }, + ); + } + async function executeApprovedAgentInvocation(input: { actionRequest: typeof toolActionRequests.$inferSelect; invocation: typeof toolInvocations.$inferSelect; @@ -4105,6 +4138,12 @@ export function createToolGatewayService( throw new ToolGatewayHttpError(409, "Tool action request was already consumed", "action_already_consumed"); } + // Terminal-issue expiry revokes pending/approved requests, but a claim that + // committed just before the issue closed slips past that revocation. Recheck + // the issue after winning the claim so a governed action never runs external + // side effects for an issue that is already done or cancelled. + const issue = await assertIssueOpenForApprovedAction({ claimed, invocation }); + const signedPayload = readSignedToolArgumentsPayload({ signedArguments: claimed.signedArguments, invocationId: invocation.id, @@ -4124,11 +4163,6 @@ export function createToolGatewayService( ); } - const [issue] = await db - .select({ projectId: issues.projectId }) - .from(issues) - .where(and(eq(issues.id, invocation.issueId), eq(issues.companyId, invocation.companyId))) - .limit(1); const session: ToolGatewaySession = { id: `approved-action:${claimed.id}`, token: "", @@ -4184,6 +4218,17 @@ export function createToolGatewayService( sensitiveMode: "redact", promptInjectionMode: "ignore", }).summary; + // Final recheck at the last DB write before dispatch: tool and snapshot + // resolution above involve network calls, so re-verify the issue is still + // open now that only the provider call remains. A close that commits after + // this read has raced an execution that was approved, claimed, and verified + // while the issue was open; that instant is irreducible for an external + // side effect gated by DB state (holding a DB lock across a remote provider + // call is not an option), and the accepted linearization is that the + // execution wins — its result still lands on the expired card via + // reflectToolActionInteractionLifecycle. + await assertIssueOpenForApprovedAction({ claimed, invocation }); + const startedAt = Date.now(); await db .update(toolInvocations) diff --git a/skills/paperclip/SKILL.md b/skills/paperclip/SKILL.md index 718b0302811..91658f4535d 100644 --- a/skills/paperclip/SKILL.md +++ b/skills/paperclip/SKILL.md @@ -234,6 +234,7 @@ Key shared semantics: - **Continuation policy.** `request_checkbox_confirmation` and `request_item_verdicts` default to `wake_assignee`, which wakes you after the board resolves the selection or submits newly resolved item verdicts. `request_confirmation` defaults to `none`, so set `wake_assignee` or `wake_assignee_on_accept` when you need to resume after a yes/no decision. `none` never wakes you — only use it when you truly do not need to resume. - **Target binding and staleness.** `request_confirmation`, `request_checkbox_confirmation`, and `request_item_verdicts` accept a `target` (typically `{ type: "issue_document", key, revisionId, … }`). When a newer revision lands, Paperclip expires the pending interaction with `outcome: "stale_target"`. Rebuild against the latest revision and create a fresh interaction. - **Supersede on user comment.** Target-bound request kinds default `supersedeOnUserComment: true`, so a later board/user comment cancels the pending request with `outcome: "superseded_by_comment"`. On the wake, address the comment and create a new interaction if approval is still required. +- **Withdraw and terminal expiry.** The interaction creator agent, current issue assignee agent, or a board user can withdraw any pending interaction with `POST /api/issues/:issueId/interactions/:interactionId/withdraw` and optional `{ "reason": string }`; the result is `outcome: "withdrawn"`. Closing an issue as `done` or `cancelled` expires all remaining pending interactions with `outcome: "issue_closed"` and never wakes the closed issue. - **Idempotency.** Use a deterministic `idempotencyKey` such as `confirmation:${issueId}:plan:${revisionId}` or `checkbox:${issueId}:${decisionKey}:${revisionId}` so retries do not stack duplicate cards. - **Source issue posture.** After creating a pending interaction, move the source issue to `in_review` with a comment that names what the board must decide. The pending interaction is the explicit waiting path. @@ -501,7 +502,7 @@ If `plan` already exists, fetch the current document first and send its latest ` | Update task | `PATCH /api/issues/:issueId` (optional `comment` field) | | Get comments / delta / single | `GET /api/issues/:issueId/comments[?after=:commentId&order=asc]` • `/comments/:commentId` | | Add comment | `POST /api/issues/:issueId/comments` | -| Issue-thread interactions | `GET\|POST /api/issues/:issueId/interactions` • `POST /api/issues/:issueId/interactions/:interactionId/{accept,reject,respond}` | +| Issue-thread interactions | `GET\|POST /api/issues/:issueId/interactions` • `POST /api/issues/:issueId/interactions/:interactionId/{accept,reject,respond,withdraw}` | | Create subtask | `POST /api/companies/:companyId/issues` | | Release task | `POST /api/issues/:issueId/release` | | Search issues | `GET /api/companies/:companyId/issues?q=search+term` | diff --git a/skills/paperclip/references/api-reference.md b/skills/paperclip/references/api-reference.md index 9ee8821c10e..a21a3eae6a2 100644 --- a/skills/paperclip/references/api-reference.md +++ b/skills/paperclip/references/api-reference.md @@ -974,6 +974,9 @@ Resolved result (`RequestCheckboxConfirmationResult`): Other outcomes match `request_confirmation`: +- `withdrawn` — `{ outcome: "withdrawn", reason }`. Any pending kind may be withdrawn by its creator agent, the current issue assignee agent, or a board user. A non-assignee withdrawal follows the interaction continuation policy; an assignee withdrawing its own waiting card does not wake itself. +- `issue_closed` — `{ outcome: "issue_closed" }`. Transitioning the issue to `done` or `cancelled` expires all pending interactions without continuation wakes; listing a terminal issue also performs a catch-up sweep for historical residue. + - `rejected` — `{ outcome: "rejected", reason, commentId }`. `selectedOptionIds` is absent. - `superseded_by_comment` — `{ outcome: "superseded_by_comment", commentId }`. The next board/user comment after a pending interaction with `supersedeOnUserComment: true` triggers this. - `stale_target` — `{ outcome: "stale_target", staleTarget }`. Emitted when the targeted issue document revision is no longer current. @@ -1210,6 +1213,7 @@ Terminal states: `done`, `cancelled` | POST | `/api/issues/:issueId/interactions/:interactionId/reject` | Reject suggested tasks or confirmation | | POST | `/api/issues/:issueId/interactions/:interactionId/respond` | Respond to structured questions | | POST | `/api/issues/:issueId/interactions/:interactionId/verdicts` | Submit partial item verdicts for `request_item_verdicts` | +| POST | `/api/issues/:issueId/interactions/:interactionId/withdraw` | Withdraw any pending interaction; optional `{ "reason": string }`; creator agent, current assignee agent, or board user | | GET | `/api/issues/:issueId/documents` | List issue documents | | GET | `/api/issues/:issueId/documents/:key` | Get issue document by key | | PUT | `/api/issues/:issueId/documents/:key` | Create or update issue document (send `baseRevisionId` when updating) | diff --git a/ui/src/components/IssueThreadInteractionCard.test.tsx b/ui/src/components/IssueThreadInteractionCard.test.tsx index ec2f79a7ee0..e2ebc7b8828 100644 --- a/ui/src/components/IssueThreadInteractionCard.test.tsx +++ b/ui/src/components/IssueThreadInteractionCard.test.tsx @@ -236,6 +236,76 @@ describe("IssueThreadInteractionCard", () => { expect(host.textContent).not.toContain("Questions expired by comment"); }); + it("renders withdrawn confirmations with the withdraw reason", () => { + const host = renderCard({ + interaction: { + ...pendingRequestConfirmationInteraction, + status: "cancelled", + result: { version: 1, outcome: "withdrawn", reason: "Superseded by the hotfix plan." }, + }, + onAcceptInteraction: vi.fn(), + onRejectInteraction: vi.fn(), + }); + + expect(host.textContent).toContain("Withdrawn"); + expect(host.textContent).toContain("Superseded by the hotfix plan."); + expect(host.textContent).not.toContain("Decline"); + }); + + it("renders confirmations expired by issue closure with dedicated copy", () => { + const host = renderCard({ + interaction: { + ...pendingRequestConfirmationInteraction, + status: "expired", + result: { version: 1, outcome: "issue_closed", reason: null }, + }, + }); + + expect(host.textContent).toContain("Expired when issue closed"); + expect(host.textContent).toContain("The issue was closed before this confirmation was resolved."); + expect(host.textContent).not.toContain("Expired by target change"); + }); + + it("renders withdrawn question interactions with the withdraw reason", () => { + const host = renderCard({ + interaction: { + ...pendingAskUserQuestionsInteraction, + status: "cancelled", + result: { + version: 1, + outcome: "withdrawn", + reason: "Scope was decided on the parent issue.", + answers: [], + summaryMarkdown: null, + }, + }, + }); + + expect(host.textContent).toContain("Questions withdrawn"); + expect(host.textContent).toContain("Scope was decided on the parent issue."); + expect(host.textContent).not.toContain("Question cancelled"); + }); + + it("renders question interactions expired by issue closure with dedicated copy", () => { + const host = renderCard({ + interaction: { + ...pendingAskUserQuestionsInteraction, + status: "expired", + result: { + version: 1, + outcome: "issue_closed", + reason: null, + answers: [], + summaryMarkdown: null, + }, + }, + }); + + expect(host.textContent).toContain("Questions expired when issue closed"); + expect(host.textContent).toContain("The issue was closed before these questions were answered."); + expect(host.textContent).not.toContain("expired by comment"); + }); + it("makes child tasks explicit in suggested task trees", () => { const host = renderCard({ interaction: pendingSuggestedTasksInteraction, diff --git a/ui/src/components/IssueThreadInteractionCard.tsx b/ui/src/components/IssueThreadInteractionCard.tsx index ed946654c84..364a68a173f 100644 --- a/ui/src/components/IssueThreadInteractionCard.tsx +++ b/ui/src/components/IssueThreadInteractionCard.tsx @@ -192,6 +192,7 @@ function requestConfirmationResumeFailure(interaction: IssueThreadInteraction) { function planStatusClasses( status: IssueThreadInteraction["status"], resumeFailure?: ReturnType, + outcome?: string | null, ) { switch (status) { case "accepted": @@ -215,7 +216,7 @@ function planStatusClasses( return { shell: "border-2 border-red-500/80 bg-transparent", badge: "border-red-500/60 bg-red-500/10 text-red-900 dark:bg-red-500/15 dark:text-red-100", - label: "Changes requested", + label: outcome === "withdrawn" ? "Withdrawn" : "Changes requested", Icon: XCircle, }; case "failed": @@ -1154,9 +1155,15 @@ function AskUserQuestionsCard({ ) : interaction.status === "cancelled" ? (
-
Question cancelled
+
+ {interaction.result?.outcome === "withdrawn" + ? questions.length === 1 ? "Question withdrawn" : "Questions withdrawn" + : "Question cancelled"} +
{interaction.result?.cancellationReason ? (

{interaction.result.cancellationReason}

+ ) : interaction.result?.reason ? ( +

{interaction.result.reason}

) : (

No answer was recorded.

)} @@ -1165,10 +1172,14 @@ function AskUserQuestionsCard({
- {questions.length === 1 ? "Question expired by comment" : "Questions expired by comment"} + {interaction.result?.outcome === "issue_closed" + ? questions.length === 1 ? "Question expired when issue closed" : "Questions expired when issue closed" + : questions.length === 1 ? "Question expired by comment" : "Questions expired by comment"}

- A later board/user comment superseded this question request. Create a fresh request if answers are still needed. + {interaction.result?.outcome === "issue_closed" + ? "The issue was closed before these questions were answered." + : "A later board/user comment superseded this question request. Create a fresh request if answers are still needed."}

{interaction.result?.commentId ? ( +
+ Withdrawn + +
+ {interaction.result?.reason ? ( +
+ {interaction.result.reason} +
+ ) : null} +
+ ); + } + if (interaction.status === "expired") { const expiredByComment = outcome === "superseded_by_comment"; + const expiredWithIssue = outcome === "issue_closed"; const expiredByTargetChange = outcome === "stale_target"; return (
- {expiredByComment ? "Expired by comment" : "Expired by target change"} + {expiredByComment ? "Expired by comment" : expiredWithIssue ? "Expired when issue closed" : "Expired by target change"}

{expiredByComment ? "A board comment superseded this confirmation before it was resolved." - : "The requested target changed before this confirmation was resolved."} + : expiredWithIssue + ? "The issue was closed before this confirmation was resolved." + : "The requested target changed before this confirmation was resolved."}

{expiredByComment && interaction.result?.commentId ? ( + @@ -2964,14 +3014,26 @@ export function Secrets() { - {selectedSecret?.managedMode === "external_reference" ? "Update external reference" : "Update secret value"} + {selectedSecret?.managedMode === "external_reference" && rotateMode === "reference" + ? "Update external reference" + : "Update secret value"} - {selectedSecret?.managedMode === "external_reference" - ? "Creates a new Paperclip metadata version that points at an existing provider secret. Paperclip does not write a new provider value." - : "Creates a new provider-backed version. Consumers pinned to latest pick up the new value on the next run."} + {selectedSecret?.managedMode !== "external_reference" + ? "Creates a new provider-backed version. Consumers pinned to latest pick up the new value on the next run." + : rotateMode === "reference" + ? "Creates a new Paperclip metadata version that points at an existing provider secret. Paperclip does not write a new provider value." + : "Writes a new version of the referenced provider secret. The new value becomes current for every consumer of that secret, in and outside Paperclip."} + {selectedSecret && secretSupportsExternalValueWrite(selectedSecret) ? ( + setRotateMode(value as RotateMode)}> + + Write new value + Change reference + + + ) : null}
+ {selectedSecret?.managedMode === "external_reference" ? ( +

+ Written to {selectedSecret.externalRef} in the provider. +

+ ) : null}
)} {rotateError ?

{rotateError}

: null} @@ -3040,13 +3107,15 @@ export function Secrets() { disabled={ rotateMutation.isPending || Boolean(rotateProviderBlockReason) || - (selectedSecret?.managedMode === "external_reference" + (selectedSecret?.managedMode === "external_reference" && rotateMode === "reference" ? !rotateExternalRef.trim() && !selectedSecret?.externalRef : !rotateValue) } > {rotateMutation.isPending ? : null} - {selectedSecret?.managedMode === "external_reference" ? "Update reference" : "Update value"} + {selectedSecret?.managedMode === "external_reference" && rotateMode === "reference" + ? "Update reference" + : "Update value"}
@@ -4546,7 +4615,14 @@ function SecretDetailsTab({ {formatRelative(secret.lastRotatedAt)} {formatRelative(secret.lastResolvedAt)}
- {modeDescription(secret.managedMode)} Paperclip never re-displays stored values. + {modeDescription( + secret.managedMode, + Boolean( + secret.externalRef && + providers.find((provider) => provider.id === secret.provider)?.supportsExternalValueWrites, + ), + )}{" "} + Paperclip never re-displays stored values.
); From 8339bcc7e7be7442a868d19692d4b888e813c715 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:08:36 -0500 Subject: [PATCH 07/87] fix(ui): keep decision training under decisions (#10181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Operators review decisions and turn useful outcomes into training examples > - The training library belongs to the Decisions surface, but its routes and entry point appeared as a separate top-level destination > - That mismatch made the training icon feel disconnected and left internal links pointing at legacy `/training` URLs > - This pull request nests the training library and inspector under `/decisions/training`, keeps legacy URLs working through redirects, and uses one route helper everywhere > - The benefit is a clearer Decisions workflow without breaking existing bookmarks or links ## Linked Issues or Issue Description Refs: #9718 Refs: #9779 **Problem:** The Decisions page exposed training as a labeled header action while the training library lived at the unrelated top-level `/training` route. Links across the UI duplicated that legacy path. **Expected behavior:** Training should be an icon action alongside the other Decisions controls, and training library/record URLs should live under `/decisions/training` while old URLs continue to redirect. **Steps to reproduce:** 1. Open the Decisions page. 2. Observe the separate labeled Training button in the page heading. 3. Open a training record and note the top-level `/training/...` URL. **Version/commit:** Current `master` before this PR. **Deployment mode:** All UI deployment modes. ## What Changed - Move the training library and inspector routes under `/decisions/training`. - Redirect legacy `/training` and `/training/:id` URLs to the new locations. - Add `decisionTrainingHref()` and use it for training links and navigation. - Place the Training icon action with the Decisions filter and sort controls. - Add focused tests for library and record URL generation. ## Verification - `pnpm exec vitest run ui/src/lib/decisionTraining.test.ts` - `pnpm --filter @paperclipai/ui typecheck` - `pnpm --filter @paperclipai/ui build` - `pnpm check:token-gates` ## Risks - Low risk: the change is limited to client-side routes and navigation. - Existing `/training` bookmarks remain supported through replacement redirects. - No API, database, migration, dependency, or workflow changes. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - OpenAI Codex using GPT-5.4 with reasoning, repository tool use, shell execution, and code/test verification. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip --- ui/src/App.tsx | 11 +++++++++-- ui/src/components/DecisionTrainingDrawer.tsx | 12 ++++++------ ui/src/lib/decisionTraining.test.ts | 12 ++++++++++++ ui/src/lib/decisionTraining.ts | 4 ++++ ui/src/pages/Training.tsx | 7 ++++--- ui/src/pages/WhatNeedsMe.tsx | 19 +++++++++++++------ 6 files changed, 48 insertions(+), 17 deletions(-) create mode 100644 ui/src/lib/decisionTraining.test.ts diff --git a/ui/src/App.tsx b/ui/src/App.tsx index 7c941d2615a..1011501dfe7 100644 --- a/ui/src/App.tsx +++ b/ui/src/App.tsx @@ -269,8 +269,10 @@ function boardRoutes() { } /> } /> - } /> - } /> + } /> + } /> + } /> + } /> } /> } /> } /> @@ -298,6 +300,11 @@ function InboxRootRedirect() { return ; } +function LegacyTrainingRedirect() { + const { id } = useParams<{ id: string }>(); + return ; +} + function LegacySkillStudioRedirect() { const location = useLocation(); const { companies, selectedCompany, loading } = useCompany(); diff --git a/ui/src/components/DecisionTrainingDrawer.tsx b/ui/src/components/DecisionTrainingDrawer.tsx index 5b5c4362904..7e31f83b1e0 100644 --- a/ui/src/components/DecisionTrainingDrawer.tsx +++ b/ui/src/components/DecisionTrainingDrawer.tsx @@ -18,7 +18,7 @@ import { Link } from "@/lib/router"; import { decisionTrainingApi, type DecisionTrainingTarget } from "../api/decisionTraining"; import { useToastActions } from "../context/ToastContext"; import { queryKeys } from "../lib/queryKeys"; -import { codeResolutionLabel, trainingTargetForItem } from "../lib/decisionTraining"; +import { codeResolutionLabel, decisionTrainingHref, trainingTargetForItem } from "../lib/decisionTraining"; import { relativeTime } from "../lib/utils"; import { Button } from "./ui/button"; import { Textarea } from "./ui/textarea"; @@ -385,7 +385,7 @@ function SavedState({
)} diff --git a/ui/src/lib/decisionTraining.test.ts b/ui/src/lib/decisionTraining.test.ts new file mode 100644 index 00000000000..b12b78d979f --- /dev/null +++ b/ui/src/lib/decisionTraining.test.ts @@ -0,0 +1,12 @@ +import { describe, expect, it } from "vitest"; +import { decisionTrainingHref } from "./decisionTraining"; + +describe("decisionTrainingHref", () => { + it("keeps the training library under decisions", () => { + expect(decisionTrainingHref()).toBe("/decisions/training"); + }); + + it("keeps training records under decisions", () => { + expect(decisionTrainingHref("example-1")).toBe("/decisions/training/example-1"); + }); +}); diff --git a/ui/src/lib/decisionTraining.ts b/ui/src/lib/decisionTraining.ts index fec49a8004b..f6f9b474925 100644 --- a/ui/src/lib/decisionTraining.ts +++ b/ui/src/lib/decisionTraining.ts @@ -1,6 +1,10 @@ import type { AttentionItem, DecisionTrainingSnapshotV1 } from "@paperclipai/shared"; import type { DecisionTrainingTarget } from "../api/decisionTraining"; +export function decisionTrainingHref(exampleId?: string): string { + return exampleId ? `/decisions/training/${exampleId}` : "/decisions/training"; +} + /** * Resolve the durable (source + issue) target a Decisions row would train * against, or `null` when the row is not trainable. diff --git a/ui/src/pages/Training.tsx b/ui/src/pages/Training.tsx index 751385455da..699a3c5b9d6 100644 --- a/ui/src/pages/Training.tsx +++ b/ui/src/pages/Training.tsx @@ -14,6 +14,7 @@ import { Input } from "@/components/ui/input"; import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@/components/ui/select"; import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs"; import { Textarea } from "@/components/ui/textarea"; +import { decisionTrainingHref } from "@/lib/decisionTraining"; import { queryKeys } from "@/lib/queryKeys"; import { cn, formatDate, formatDateTime } from "@/lib/utils"; @@ -143,7 +144,7 @@ export function TrainingLibrary() { const projectName = projectNames.get(stringValue(issue, "projectId", "project_id") ?? "") ?? "No project"; const edited = example.updatedAt !== example.createdAt; return ( -

{decisionTitle(example)}

{issueIdentifier} · {outcomeLabel(example.decisionOutcome)} · cutoff {formatDateTime(example.cutoffAt)}

+

{decisionTitle(example)}

{issueIdentifier} · {outcomeLabel(example.decisionOutcome)} · cutoff {formatDateTime(example.cutoffAt)}

Training notes

Last edited {formatDateTime(example.updatedAt)} · edits are versioned

{!editing ? : null}
{editing ?