From cbdee67a770cc468f495c5bd131ede8ad9dbfa03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 09:59:31 -0300 Subject: [PATCH 1/5] feat: add supervised pause and human takeover (#21) Add a leased, fail-closed human takeover for desktop-capable sessions. `picklab watch --control` atomically acquires a TTL/heartbeat human lease, drains in-flight agent permits before granting control, switches the session's VNC to writable for the lease's duration, and hands control back with a fresh screenshot and evidence record on every exit path (return, cancellation, failed renewal, or crash recovery on the next VNC touch). Core (packages/core/src/takeover.ts): atomic O_EXCL lease file, agent permit files, withAgentPermit's fail-closed acquire/recheck/execute/ release protocol, staleness (identity-or-TTL), and takeover evidence recording. Desktop-linux (packages/desktop-linux/src/takeover.ts): VNC mode switching (read-only <-> writable) scoped to the lease, crash recovery wired into ensureSessionVnc so a stale writable VNC self-heals instead of blocking watch. Browser: the DevTools MCP relay gained a generic NDJSON intercept hook so a blocked tools/call is answered with a stable busy error directly, never forwarded to the child process. Desktop input (CLI + MCP): all seven input operations now go through withAgentPermit, so no permit or an active lease means no input delivery. New picklab takeover status / takeover_status expose read-only lease state; request_user_input's secret guidance now points at --control instead of the uncoordinated --vnc-control. --- README.md | 35 +- docs/releases/UNRELEASED.md | 84 +++ packages/browser/src/devtools-mcp.ts | 59 +- packages/browser/src/index.ts | 3 + packages/browser/src/ndjson.ts | 30 + packages/browser/test/devtools-mcp.test.ts | 79 ++- packages/browser/test/ndjson.test.ts | 36 ++ packages/cli/src/commands/desktop.ts | 12 +- packages/cli/src/commands/takeover.ts | 44 ++ packages/cli/src/commands/watch.ts | 126 ++++ packages/cli/src/program.ts | 21 +- .../cli/test/desktop-input-takeover.test.ts | 69 ++ packages/cli/test/takeover-status.test.ts | 67 ++ packages/cli/test/watch-control.test.ts | 199 ++++++ packages/core/src/index.ts | 29 + packages/core/src/takeover.ts | 606 ++++++++++++++++++ packages/core/test/takeover.test.ts | 303 +++++++++ .../test/workers/takeover-acquire-worker.ts | 30 + packages/desktop-linux/src/index.ts | 12 + packages/desktop-linux/src/input.ts | 116 ++-- packages/desktop-linux/src/session.ts | 92 ++- packages/desktop-linux/src/takeover.ts | 320 +++++++++ .../desktop-linux/test/integration.test.ts | 18 +- packages/desktop-linux/test/takeover.test.ts | 329 ++++++++++ packages/mcp-server/src/index.ts | 2 + packages/mcp-server/src/tools/desktop.ts | 14 +- packages/mcp-server/src/tools/takeover.ts | 47 ++ packages/mcp-server/src/tools/user.ts | 11 +- packages/mcp-server/test/takeover.test.ts | 112 ++++ packages/mcp-server/test/tools.test.ts | 1 + 30 files changed, 2828 insertions(+), 78 deletions(-) create mode 100644 packages/cli/src/commands/takeover.ts create mode 100644 packages/cli/test/desktop-input-takeover.test.ts create mode 100644 packages/cli/test/takeover-status.test.ts create mode 100644 packages/cli/test/watch-control.test.ts create mode 100644 packages/core/src/takeover.ts create mode 100644 packages/core/test/takeover.test.ts create mode 100644 packages/core/test/workers/takeover-acquire-worker.ts create mode 100644 packages/desktop-linux/src/takeover.ts create mode 100644 packages/desktop-linux/test/takeover.test.ts create mode 100644 packages/mcp-server/src/tools/takeover.ts create mode 100644 packages/mcp-server/test/takeover.test.ts diff --git a/README.md b/README.md index 4763b18..40c7ff6 100644 --- a/README.md +++ b/README.md @@ -159,6 +159,29 @@ project in `.picklab/config.json`: This does not block an explicitly requested screenshot command. Screenshot pixels cannot be redacted; see [SECURITY.md](SECURITY.md#recorded-evidence-and-screenshots). +### Supervised pause and human takeover + +```sh +picklab watch --session --control # pause the agent, take a temporary writable viewer +picklab takeover status --session # check whether a session is under human control +``` + +`picklab watch --control` pauses PickLab-managed agent input for a session, +grants a temporary writable VNC viewer for a human, and hands control back +with a fresh screenshot and an evidence record once the viewer closes (or the +terminal is interrupted). Unlike `--vnc-control`'s persistent writable +session, control here is leased: while a human holds it, every desktop input +call and every DevTools relay request fails closed with a stable busy error — +`takeover_status` (MCP) / `picklab takeover status` (CLI) let an agent check +before retrying, and `request_user_input` is the recommended way to ask a +human to run it. + +The lease is a 30-second TTL, heartbeat-renewed-every-5-seconds record in the +session's state directory. Closing the viewer, an interrupted terminal, or a +PickLab crash all end up releasing it and reverting VNC to read-only — a +crash is recovered automatically the next time the session's VNC is touched +(e.g. a later `picklab watch`), never left writable indefinitely. + ### Concurrent sessions Each session gets its own isolated display or emulator, so several agents and projects can run labs side by side. When a command or tool is called without an explicit session id, the default resolves per project: only running sessions created for the same project directory are considered. Pass `session` ids (CLI: `--session `) to target a specific lab, including one belonging to another project. @@ -215,7 +238,8 @@ picklab agents add --name my-agent --mcp-command "picklab mcp serve" | --- | --- | | Setup | `doctor`, `init`, `setup lab-user`, `setup android` | | Sessions | `session create`, `session status [id]`, `session destroy ` | -| Watch | `watch [--session ]` | +| Watch | `watch [--session ] [--control]` | +| Takeover | `takeover status [--session ]` | | Desktop | `desktop launch `, `desktop screenshot`, `desktop click `, `desktop move `, `desktop scroll `, `desktop drag `, `desktop double-click `, `desktop type `, `desktop key ` | | Android | `android start`, `android install-apk `, `android launch-app `, `android screenshot`, `android tap `, `android type `, `android back`, `android home`, `android ui-tree`, `android logcat`, `android adb [args...]` | | Artifacts | `artifacts list`, `artifacts open `, `artifacts report [runId]` | @@ -225,7 +249,7 @@ picklab agents add --name my-agent --mcp-command "picklab mcp serve" Session types: `desktop` (Xvfb, optional VNC), `android` (emulator on the dedicated AVD), `desktop+android`, and `browser` (isolated headed Chrome with loopback CDP). Most commands accept `--json` for machine-readable output and `--project-dir` to target another project. -`session create --vnc` is read-only. When a human must enter a password, API key, or OTP directly into the lab app, `--vnc-control` creates an explicitly writable VNC session instead. Pause agent input while using it; coordinated human takeover is tracked separately. +`session create --vnc` is read-only. `--vnc-control` creates an explicitly writable VNC session up front and does not coordinate with agent input — pause agent activity yourself while using it. For a coordinated, leased handoff instead, use `picklab watch --control` (see [Supervised pause and human takeover](#supervised-pause-and-human-takeover)), which fails agent input closed for the lease's duration and hands back a fresh screenshot automatically. Scroll deltas are integer wheel steps: positive `deltaY` scrolls down, negative up; positive `deltaX` scrolls right, negative left (put negative values after `--`, e.g. `picklab desktop scroll -- 0 -3`). `desktop scroll` accepts `--at ` to position the pointer first; `desktop drag` accepts `--button` and `--duration `; `desktop double-click` accepts `--button` and `--interval `. `picklab watch [--session ]` attaches a normal host-side VNC window to an @@ -265,12 +289,13 @@ reported as suppressed for an explicitly writable `--vnc-control` session. ## MCP surface -`picklab mcp serve` exposes 26 tools over stdio: +`picklab mcp serve` exposes 27 tools over stdio: - Sessions: `session_create`, `session_status`, `session_destroy` -- Desktop: `desktop_launch`, `desktop_screenshot`, `desktop_click`, `desktop_move`, `desktop_scroll`, `desktop_drag`, `desktop_double_click`, `desktop_type`, `desktop_key` +- Desktop: `desktop_launch`, `desktop_screenshot`, `desktop_click`, `desktop_move`, `desktop_scroll`, `desktop_drag`, `desktop_double_click`, `desktop_type`, `desktop_key` — the seven input tools fail closed with a busy error while a human lease is active - Android: `android_start`, `android_install_apk`, `android_launch_app`, `android_screenshot`, `android_tap`, `android_type`, `android_back`, `android_home`, `android_get_ui_tree`, `android_logcat`, `android_run_adb` - Artifacts: `artifact_list`, `artifact_report` +- Takeover: `takeover_status` — check whether a session is under human control (see [Supervised pause and human takeover](#supervised-pause-and-human-takeover)); read-only, always safe to call - User: `request_user_input` — ask the human a question (via MCP elicitation when the client supports it) and wait for the answer; never used for secrets Resources, addressable as `picklab://` URIs: @@ -308,7 +333,7 @@ A TypeScript monorepo. `@pickforge/picklab` is the published package; the rest a - All user inputs are spawned as argument arrays — never interpolated into shell strings. - The DevTools relay validates the installed upstream package name, exact version, declared bin, and confined real path before spawning Node with an argument array. Its browser URL is always derived as `http://127.0.0.1:`. - Relay stdout is protocol-only. A pending JSON-RPC record is capped at 16 MiB. Upstream diagnostic lines are capped at 64 KiB, redacted, and forwarded only to stderr; an over-limit line is dropped with a safe notice. Upstream update checks and usage statistics are disabled. -- VNC binds to loopback only by default: `x11vnc` is started with `-localhost`, so the server listens on `127.0.0.1` and is not reachable from the network. Tunnel over SSH for remote access. Normal `--vnc` and `picklab watch` observation is server-enforced read-only (`-viewonly`); viewer exit never stops the session or its Xvfb/VNC processes. `--vnc-control` is an explicit writable escape hatch for human secret entry and does not yet coordinate with agent input. +- VNC binds to loopback only by default: `x11vnc` is started with `-localhost`, so the server listens on `127.0.0.1` and is not reachable from the network. Tunnel over SSH for remote access. Normal `--vnc` and `picklab watch` observation is server-enforced read-only (`-viewonly`); viewer exit never stops the session or its Xvfb/VNC processes. `--vnc-control` is an explicit, persistent writable escape hatch for human secret entry and does not coordinate with agent input. `picklab watch --control` is the coordinated alternative: an atomic, TTL-bounded lease gates a temporary writable VNC server, and every agent desktop-input call and DevTools relay request fails closed (a live human lease is checked immediately before delivery) for as long as it is held; a crash on either side is recovered — writable VNC never outlives its lease. - Artifacts are redacted by default: logcat output strips tokens and secrets before it is stored or returned. Only `android adb` is raw, and it says so. - Evidence timelines persist only allowlisted metadata; typed values become length/type metadata, and network headers, bodies, and URL queries are dropped. Static HTML reports escape page-controlled text and use a no-script, no-network CSP. - Screenshot files contain raw pixels and cannot be redacted. Avoid explicit captures on screens containing secrets, and use `evidence.enabled: false` when an action timeline is not appropriate. See [SECURITY.md](SECURITY.md#recorded-evidence-and-screenshots). diff --git a/docs/releases/UNRELEASED.md b/docs/releases/UNRELEASED.md index 1418775..b0fca97 100644 --- a/docs/releases/UNRELEASED.md +++ b/docs/releases/UNRELEASED.md @@ -43,6 +43,23 @@ release description, then reset it after the release is published. - Added `picklab watch` plus configurable manual/automatic VNC viewer attachment for running desktop-capable sessions. VNC remains loopback-only and read-only by default; writable access requires the explicit `--vnc-control` path. +- **Added supervised pause and human takeover (`picklab watch --control`).** + Pauses PickLab-managed agent input for a session, atomically acquires a + 30-second-TTL / 5-second-heartbeat human lease, switches the session's VNC + to a temporary writable server for the duration, and hands control back — + reverting VNC to read-only, recording a fresh screenshot, and releasing the + lease — on every exit path: normal viewer close, terminal cancellation + (SIGINT/SIGTERM), a failed lease renewal, or a PickLab crash (recovered the + next time the session's VNC is touched). While a lease is held, every + desktop input call (CLI and MCP: click/move/scroll/drag/double-click/type/ + key) and every DevTools relay `tools/call` request fails closed with a + stable busy error, checked immediately before delivery — never delivered + concurrently with human input. New `takeover_status` MCP tool and + `picklab takeover status` CLI command report lease state (agent-active, + human-active, or a stale/recoverable lease) without side effects. + `request_user_input`'s secret guidance now points at this flow instead of + the persistent, uncoordinated `--vnc-control`. See the README's + "Supervised pause and human takeover" section. (pickforge/picklab#21) - Added desktop mouse move, scroll, drag, and double-click controls to both the CLI and MCP server. - Added end-to-end computer-use evidence recording for desktop, Android, and @@ -117,6 +134,26 @@ release description, then reset it after the release is published. `cancelled` executor status instead of a generic failure. - Added a framing-aware DevTools NDJSON relay with protocol validation, backpressure, bounded diagnostics, redacted failures, and evidence hooks. +- Added the human-lease/agent-permit storage primitives backing supervised + takeover (`packages/core/src/takeover.ts`): an atomic (`wx`) `human.lease.json` + per session directory, short-lived per-action agent permit files under + `permits/`, and a fail-closed `withAgentPermit` (acquire permit → recheck + lease → execute only if clear → release in `finally`). Lease acquisition + publishes the lease before draining permits that predate it (sweeping ones + owned by a dead process), so writable VNC is never granted while an agent + action from before the lease could still be in flight. Staleness is + identity-or-TTL based (`isHumanLeaseStale`); a second live acquisition + attempt fails fast (`HumanLeaseHeldError`, one winner, no queueing). Desktop- + linux's `takeover.ts` (`startHumanTakeover`/`endHumanTakeover`/ + `renewHumanTakeover`/`recoverStaleHumanLease`) layers VNC-mode switching, + fresh-screenshot capture, and lifecycle evidence (`takeover_start`/ + `takeover_return`/`takeover_timeout`/`takeover_cancelled`/ + `takeover_recovered`) on top; `ensureSessionVnc` (plain `watch`) now + self-heals a crash-orphaned writable VNC left by a stale lease instead of + refusing outright, while still refusing while a live lease holds it. The + DevTools relay gained a generic NDJSON `intercept` hook (answers a request + directly on the response stream instead of forwarding it to the child) used + for the same busy-error contract. - Added atomic, crash-recoverable evidence journals, active-run ownership, truncation markers, report publication, and symlink-safe resource access. - Hardened Android and evidence cleanup around process identity, atomic writes, @@ -185,6 +222,39 @@ release description, then reset it after the release is published. changing production code) and can occasionally need extra wall-clock time under the same full-parallel load — a scheduling artifact, not a logic defect, and it stays within the pre-existing 43-45 Darwin noise band above. +- New `core/test/takeover.test.ts` (15 tests): atomic lease acquisition/ + staleness/renewal/release, permit drain (including sweeping a permit owned + by a dead process), `withAgentPermit` fail-closed and mid-flight- + invalidation behavior, `getTakeoverStatus`, and a real separate-process + (`bun`-spawned) race proving two concurrent lease claims yield exactly one + winner. +- New `desktop-linux/test/takeover.test.ts` (11 tests): end-to-end + start/end-takeover VNC mode switching against real spawned fake-x11vnc + processes and real port listening (only the two `/proc`-dependent identity + functions are mocked, following `destroy.test.ts`'s existing pattern, so + this runs on Darwin too) — asserts the exact `-viewonly` flag sequence, + evidence transitions, lease renewal/expiry, crash recovery (stale lease + + orphaned writable VNC), and `ensureSessionVnc`'s self-heal vs. still-refuse- + while-live-lease branches. Real x11vnc/Xvfb hardware validation deferred. +- New `browser/test/ndjson.test.ts` and `devtools-mcp.test.ts` intercept + coverage: diversion to `interceptDestination` without invoking `hook` or + forwarding to the child, the `interceptDestination` requirement, an + end-to-end relay test proving an intercepted `tools/call` never reaches the + spawned child process, and `createTakeoverBusyIntercept` unit coverage + (blocks only `tools/call` with an id while a live lease exists; passes + through notifications, other methods, and once the lease clears). +- New `cli/test/watch-control.test.ts` (5, mocked desktop-linux takeover + functions), `cli/test/takeover-status.test.ts` (3), and + `cli/test/desktop-input-takeover.test.ts` (1, proves the CLI rejects + desktop input without ever invoking `xdotool` while a lease is held) — + cover `--control`'s reason selection (return/cancelled/timeout), the + `--control` + `waitForViewerExit: false` guard, and the no-viewer-available + path. +- New `mcp-server/test/takeover.test.ts` (3): `takeover_status` across + agent-active/human-active/stale, and `desktop_click`/`desktop_type`/ + `desktop_key` failing closed with the busy error while a lease is held, + using a synthetic session record (no real Xvfb/xdotool needed for the + fail-closed path). - `bun run build` ### Not tested yet @@ -201,6 +271,20 @@ release description, then reset it after the release is published. - Live remote SSH-tunnel smoke test. - Tag-triggered npm publish and draft-release creation (runs only after merge and tag push). +- Live supervised takeover proof on real hardware: a genuine `picklab watch + --control` against real Xvfb/x11vnc, taking control in an actual VNC + viewer, typing, and returning control, on a Linux desktop. All takeover + logic is proven with real spawned processes and real port listening except + the two `/proc`-only identity checks (mocked identically to the existing + `destroy.test.ts` pattern); this is a hardware/CI gap, not a logic gap. +- A bare `kill -TERM ` (not the whole foreground process group) + arriving while a real interactive VNC viewer window is still open: the + takeover's SIGTERM handler marks the session for a "cancelled" reason but + cannot itself close the already-open viewer (`openVncViewer`'s API has no + cancel handle), so cleanup only runs once the viewer exits on its own or a + process-group-wide SIGINT (the normal terminal Ctrl+C path, which reaches + the viewer child directly) closes both. Documented, not implemented; + extending `openVncViewer` with a cancel handle is a reasonable follow-up. ### Release blockers diff --git a/packages/browser/src/devtools-mcp.ts b/packages/browser/src/devtools-mcp.ts index 8b19670..00bb80a 100644 --- a/packages/browser/src/devtools-mcp.ts +++ b/packages/browser/src/devtools-mcp.ts @@ -9,6 +9,7 @@ import { import { setTimeout as delay } from "node:timers/promises"; import type { Readable, Writable } from "node:stream"; import { + checkHumanLeaseBusy, listSessions, redactSecrets, sanitizeErrorText, @@ -26,8 +27,46 @@ import { pumpJsonRpcNdjson, writeWithBackpressure, type JsonRpcHook, + type JsonRpcIntercept, + type JsonRpcMessage, } from "./ndjson.js"; +/** JSON-RPC error code for the stable "human control is active" busy response. */ +export const TAKEOVER_BUSY_ERROR_CODE = -32050; +const TAKEOVER_BUSY_MESSAGE = + "PickLab: human control is active; agent input is paused until control returns"; + +function jsonRpcRequestId(message: JsonRpcMessage): string | number | undefined { + return typeof message.id === "string" || typeof message.id === "number" + ? message.id + : undefined; +} + +/** + * Fail-closed human-takeover gate for the DevTools relay + * (pickforge/picklab#21): while the session's human lease is active, every + * `tools/call` request is answered directly with a stable busy error instead + * of ever reaching the child Chrome DevTools MCP process. Notifications (no + * `id`) and non-tool-call requests pass through untouched. + */ +export function createTakeoverBusyIntercept( + sessionId: string, + env: EnvLike | undefined, +): JsonRpcIntercept { + return async (message) => { + if (message.method !== "tools/call") return undefined; + const id = jsonRpcRequestId(message); + if (id === undefined) return undefined; + const lease = await checkHumanLeaseBusy(sessionId, env); + if (lease === undefined) return undefined; + return { + jsonrpc: "2.0", + id, + error: { code: TAKEOVER_BUSY_ERROR_CODE, message: TAKEOVER_BUSY_MESSAGE }, + }; + }; +} + export const CHROME_DEVTOOLS_MCP_PACKAGE = "chrome-devtools-mcp"; export const CHROME_DEVTOOLS_MCP_VERSION = "1.5.0"; export const CHROME_DEVTOOLS_MCP_BIN = "./build/src/bin/chrome-devtools-mcp.js"; @@ -180,6 +219,13 @@ export async function resolveLiveBrowserSession( export interface RelayHooks { beforeForward?: JsonRpcHook; afterResponse?: JsonRpcHook; + /** + * Checked before `beforeForward` on every agent -> child record. Returning + * a message answers the caller directly on `output` and the record is + * never forwarded to the child — used for the human-takeover fail-closed + * busy response (pickforge/picklab#21). See `JsonRpcIntercept`. + */ + intercept?: JsonRpcIntercept; } function composeEvidenceHooks( @@ -197,6 +243,7 @@ function composeEvidenceHooks( await evidence.afterResponse(message); return hooks?.afterResponse?.(message); }, + intercept: hooks?.intercept, }; } @@ -376,6 +423,8 @@ export async function runDevtoolsMcpRelay( let terminationRequested = false; const inputPump = pumpJsonRpcNdjson(input, child.stdin, { hook: opts.hooks?.beforeForward, + intercept: opts.hooks?.intercept, + interceptDestination: opts.hooks?.intercept === undefined ? undefined : output, signal: inputAbort.signal, endDestination: true, maxRecordBytes: opts.maxRecordBytes, @@ -546,6 +595,14 @@ export async function runProjectDevtoolsMcp( } catch (error) { reportEvidenceFailure(error); } + const takeoverIntercept = createTakeoverBusyIntercept(session.record.id, opts.env); + const hooksWithTakeoverGate: RelayHooks = { + ...opts.hooks, + intercept: async (message) => { + const busy = await takeoverIntercept(message); + return busy ?? (await opts.hooks?.intercept?.(message)); + }, + }; try { return await runDevtoolsMcpRelay({ session, @@ -555,7 +612,7 @@ export async function runProjectDevtoolsMcp( diagnostics, env: opts.env, cwd: path.resolve(opts.projectDir), - hooks: composeEvidenceHooks(opts.hooks, evidence), + hooks: composeEvidenceHooks(hooksWithTakeoverGate, evidence), shutdownTimeoutMs: opts.shutdownTimeoutMs, maxRecordBytes: opts.maxRecordBytes, maxDiagnosticLineBytes: opts.maxDiagnosticLineBytes, diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts index 4680ce6..783be24 100644 --- a/packages/browser/src/index.ts +++ b/packages/browser/src/index.ts @@ -53,6 +53,7 @@ export { writeWithBackpressure, type JsonRpcHook, type JsonRpcId, + type JsonRpcIntercept, type JsonRpcMessage, type JsonRpcRecord, type PumpJsonRpcNdjsonOptions, @@ -63,6 +64,8 @@ export { CHROME_DEVTOOLS_MCP_PACKAGE, CHROME_DEVTOOLS_MCP_VERSION, DEFAULT_MAX_DIAGNOSTIC_LINE_BYTES, + TAKEOVER_BUSY_ERROR_CODE, + createTakeoverBusyIntercept, resolveDevtoolsMcpExecutable, resolveLiveBrowserSession, runDevtoolsMcpRelay, diff --git a/packages/browser/src/ndjson.ts b/packages/browser/src/ndjson.ts index 01c8ae8..08f883d 100644 --- a/packages/browser/src/ndjson.ts +++ b/packages/browser/src/ndjson.ts @@ -24,6 +24,20 @@ export type JsonRpcHook = ( message: JsonRpcMessage, ) => JsonRpcMessage | void | Promise; +/** + * Per-record interception, checked before `hook`/forwarding. Returning a + * message short-circuits: that message is written to `interceptDestination` + * instead, and the original record is never forwarded to `destination`. + * Returning `undefined` falls through to the normal `hook`+forward path. + * Used by the DevTools relay's fail-closed human-takeover gate ( + * pickforge/picklab#21) to answer a blocked `tools/call` with a synthetic + * busy error on the *response* stream, without ever reaching the upstream + * child process. + */ +export type JsonRpcIntercept = ( + message: JsonRpcMessage, +) => JsonRpcMessage | undefined | Promise; + function isObject(value: unknown): value is Record { return typeof value === "object" && value !== null && !Array.isArray(value); } @@ -274,6 +288,10 @@ async function nextWithAbort( export interface PumpJsonRpcNdjsonOptions { hook?: JsonRpcHook; + /** Checked before `hook`; see `JsonRpcIntercept`. Requires `interceptDestination`. */ + intercept?: JsonRpcIntercept; + /** Where an `intercept` response is written instead of `destination`. */ + interceptDestination?: Writable; signal?: AbortSignal; endDestination?: boolean; maxRecordBytes?: number; @@ -284,6 +302,9 @@ export async function pumpJsonRpcNdjson( destination: Writable, opts: PumpJsonRpcNdjsonOptions = {}, ): Promise { + if (opts.intercept !== undefined && opts.interceptDestination === undefined) { + throw new Error("pumpJsonRpcNdjson: intercept requires interceptDestination"); + } const decoder = new JsonRpcNdjsonBuffer(opts.maxRecordBytes); const iterator = source.iterator({ destroyOnReturn: false })[Symbol.asyncIterator](); try { @@ -295,6 +316,15 @@ export async function pumpJsonRpcNdjson( const chunk = typeof item.value === "string" ? Buffer.from(item.value) : Buffer.from(item.value); for (const record of decoder.push(chunk)) { + const intercepted = + opts.intercept === undefined ? undefined : await opts.intercept(record.message); + if (intercepted !== undefined) { + await writeWithBackpressure( + opts.interceptDestination!, + serializeJsonRpcMessage(intercepted), + ); + continue; + } await writeWithBackpressure(destination, await applyHook(record, opts.hook)); } } diff --git a/packages/browser/test/devtools-mcp.test.ts b/packages/browser/test/devtools-mcp.test.ts index cb0e793..5fa9f93 100644 --- a/packages/browser/test/devtools-mcp.test.ts +++ b/packages/browser/test/devtools-mcp.test.ts @@ -6,11 +6,17 @@ import { EventEmitter } from "node:events"; import { PassThrough, Readable, Writable } from "node:stream"; import { setTimeout as delay } from "node:timers/promises"; import { afterEach, describe, expect, it } from "vitest"; -import type { SessionRecord } from "@pickforge/picklab-core"; +import { + acquireHumanLease, + releaseHumanLease, + type SessionRecord, +} from "@pickforge/picklab-core"; import { createDeferred, CHROME_DEVTOOLS_MCP_BIN, CHROME_DEVTOOLS_MCP_VERSION, + TAKEOVER_BUSY_ERROR_CODE, + createTakeoverBusyIntercept, resolveDevtoolsMcpExecutable, resolveLiveBrowserSession, runDevtoolsMcpRelay, @@ -226,6 +232,46 @@ describe("resolveLiveBrowserSession", () => { }); }); +describe("createTakeoverBusyIntercept", () => { + it("blocks tools/call while a human lease is active, and only that", async () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "picklab-takeover-intercept-")); + temporaryDirectories.push(tmp); + const env = { PICKLAB_HOME: path.join(tmp, "home") }; + const intercept = createTakeoverBusyIntercept("brow-aaaaaa11", env); + + // No lease yet: everything passes through. + expect( + await intercept({ jsonrpc: "2.0", id: 1, method: "tools/call" }), + ).toBeUndefined(); + + const lease = await acquireHumanLease("brow-aaaaaa11", env); + + // Non-tool-call requests and notifications (no id) are never blocked. + expect( + await intercept({ jsonrpc: "2.0", id: "x", method: "tools/list" }), + ).toBeUndefined(); + expect( + await intercept({ jsonrpc: "2.0", method: "notifications/cancelled" }), + ).toBeUndefined(); + + const blocked = await intercept({ jsonrpc: "2.0", id: 2, method: "tools/call" }); + expect(blocked).toEqual({ + jsonrpc: "2.0", + id: 2, + error: { + code: TAKEOVER_BUSY_ERROR_CODE, + message: + "PickLab: human control is active; agent input is paused until control returns", + }, + }); + + await releaseHumanLease("brow-aaaaaa11", lease.leaseId, env); + expect( + await intercept({ jsonrpc: "2.0", id: 3, method: "tools/call" }), + ).toBeUndefined(); + }); +}); + describe("runDevtoolsMcpRelay", () => { it("spawns Node with exact argv/env, relays protocol only, and redacts stderr", async () => { const dir = temporaryDirectory(); @@ -317,6 +363,37 @@ describe("runDevtoolsMcpRelay", () => { }); }); + it("intercepts a blocked request without ever reaching the child process", async () => { + const dir = temporaryDirectory(); + const script = path.join(dir, "intercept-upstream.mjs"); + fs.writeFileSync(script, 'process.stdin.pipe(process.stdout);\n'); + const output: Buffer[] = []; + await runDevtoolsMcpRelay({ + session: fakeSession(), + executable: fakeExecutable(script), + input: Readable.from(['{"jsonrpc":"2.0","id":9,"method":"tools/call"}\n']), + output: collect(output), + diagnostics: collect([]), + signalSource: new Signals(), + shutdownTimeoutMs: 200, + hooks: { + intercept: (message) => + message.method === "tools/call" + ? { + jsonrpc: "2.0", + id: message.id as number, + error: { code: TAKEOVER_BUSY_ERROR_CODE, message: "busy" }, + } + : undefined, + }, + }); + expect(JSON.parse(Buffer.concat(output).toString())).toEqual({ + jsonrpc: "2.0", + id: 9, + error: { code: TAKEOVER_BUSY_ERROR_CODE, message: "busy" }, + }); + }); + it("propagates the observed upstream exit code", async () => { const dir = temporaryDirectory(); const script = path.join(dir, "exit-upstream.mjs"); diff --git a/packages/browser/test/ndjson.test.ts b/packages/browser/test/ndjson.test.ts index 1fed7f6..1e0c743 100644 --- a/packages/browser/test/ndjson.test.ts +++ b/packages/browser/test/ndjson.test.ts @@ -171,4 +171,40 @@ describe("pumpJsonRpcNdjson", () => { '{"jsonrpc":"2.0","id":2,"method":"two"}\n', ]); }); + + it("diverts an intercepted record to interceptDestination instead of forwarding or hooking", async () => { + const raw = + '{"jsonrpc":"2.0","id":1,"method":"tools/call"}\n' + + '{"jsonrpc":"2.0","id":2,"method":"tools/list"}\n'; + const forwarded: Buffer[] = []; + const intercepted: Buffer[] = []; + let hookCalls = 0; + await pumpJsonRpcNdjson(Readable.from([raw]), collectingWritable(forwarded), { + hook: () => { + hookCalls += 1; + return undefined; + }, + intercept: (message) => + message.method === "tools/call" + ? { jsonrpc: "2.0", id: message.id as string | number, error: { code: -32050, message: "busy" } } + : undefined, + interceptDestination: collectingWritable(intercepted), + }); + expect(Buffer.concat(intercepted).toString()).toBe( + '{"jsonrpc":"2.0","id":1,"error":{"code":-32050,"message":"busy"}}\n', + ); + expect(Buffer.concat(forwarded).toString()).toBe( + '{"jsonrpc":"2.0","id":2,"method":"tools/list"}\n', + ); + // The hook only ever sees the record that fell through interception. + expect(hookCalls).toBe(1); + }); + + it("requires interceptDestination whenever intercept is set", async () => { + await expect( + pumpJsonRpcNdjson(Readable.from(['{"jsonrpc":"2.0","id":1,"method":"x"}\n']), collectingWritable([]), { + intercept: () => undefined, + }), + ).rejects.toThrow(/interceptDestination/); + }); }); diff --git a/packages/cli/src/commands/desktop.ts b/packages/cli/src/commands/desktop.ts index 82578e3..6df2de3 100644 --- a/packages/cli/src/commands/desktop.ts +++ b/packages/cli/src/commands/desktop.ts @@ -155,7 +155,7 @@ export async function runDesktopClick( const parsedY = parseIntArg(y, "y"); const button = parseButtonOption(opts.button); const { id, display } = await resolveDesktop(opts); - await click({ display, x: parsedX, y: parsedY, button }); + await click({ display, sessionId: id, x: parsedX, y: parsedY, button }); return { data: { sessionId: id, display, x: parsedX, y: parsedY, button: button ?? 1 }, lines: [`clicked (${parsedX}, ${parsedY}) on ${display}`], @@ -172,7 +172,7 @@ export async function runDesktopMove( const parsedX = parseIntArg(x, "x"); const parsedY = parseIntArg(y, "y"); const { id, display } = await resolveDesktop(opts); - await move({ display, x: parsedX, y: parsedY }); + await move({ display, sessionId: id, x: parsedX, y: parsedY }); return { data: { sessionId: id, display, x: parsedX, y: parsedY }, lines: [`moved pointer to (${parsedX}, ${parsedY}) on ${display}`], @@ -210,7 +210,7 @@ export async function runDesktopScroll( y = Number(match[2]); } const { id, display } = await resolveDesktop(opts); - await scroll({ display, deltaX: parsedDeltaX, deltaY: parsedDeltaY, x, y }); + await scroll({ display, sessionId: id, deltaX: parsedDeltaX, deltaY: parsedDeltaY, x, y }); const data: Record = { sessionId: id, display, @@ -256,6 +256,7 @@ export async function runDesktopDrag( const { id, display } = await resolveDesktop(opts); await drag({ display, + sessionId: id, fromX: parsedFromX, fromY: parsedFromY, toX: parsedToX, @@ -303,6 +304,7 @@ export async function runDesktopDoubleClick( const { id, display } = await resolveDesktop(opts); await doubleClick({ display, + sessionId: id, x: parsedX, y: parsedY, button, @@ -327,7 +329,7 @@ export async function runDesktopType( ): Promise { return runReported(opts, async () => { const { id, display } = await resolveDesktop(opts); - await typeText({ display, text }); + await typeText({ display, sessionId: id, text }); return { data: { sessionId: id, display, length: text.length }, lines: [`typed ${text.length} character(s) on ${display}`], @@ -341,7 +343,7 @@ export async function runDesktopKey( ): Promise { return runReported(opts, async () => { const { id, display } = await resolveDesktop(opts); - await pressKey({ display, key }); + await pressKey({ display, sessionId: id, key }); return { data: { sessionId: id, display, key }, lines: [`pressed ${key} on ${display}`], diff --git a/packages/cli/src/commands/takeover.ts b/packages/cli/src/commands/takeover.ts new file mode 100644 index 0000000..5e7c5fa --- /dev/null +++ b/packages/cli/src/commands/takeover.ts @@ -0,0 +1,44 @@ +import { getTakeoverStatus, resolveDesktopCapableSession } from "@pickforge/picklab-core"; +import { + resolveProjectDir, + runReported, + type BaseCliOptions, + type CommandResult, +} from "./shared.js"; + +export interface TakeoverStatusOptions extends BaseCliOptions { + session?: string; +} + +export async function takeoverStatus( + opts: TakeoverStatusOptions, +): Promise { + const record = await resolveDesktopCapableSession(opts.session, { + projectDir: resolveProjectDir(opts), + }); + const status = await getTakeoverStatus(record.id); + const data: Record = { + sessionId: record.id, + active: status.active, + }; + if (status.stale === true) data.stale = true; + if (status.lease !== undefined) { + data.lease = { + leaseId: status.lease.leaseId, + ownerPid: status.lease.ownerPid, + createdAt: status.lease.createdAt, + expiresAt: status.lease.expiresAt, + ...(status.lease.vncPort === undefined ? {} : { vncPort: status.lease.vncPort }), + }; + } + const line = status.active + ? `session ${record.id} is under human control (lease ${status.lease?.leaseId}, since ${status.lease?.createdAt})` + : status.stale === true + ? `session ${record.id} has a stale human lease pending recovery (lease ${status.lease?.leaseId})` + : `session ${record.id} is agent-active (no human lease)`; + return { data, lines: [line] }; +} + +export async function runTakeoverStatus(opts: TakeoverStatusOptions): Promise { + return runReported(opts, () => takeoverStatus(opts)); +} diff --git a/packages/cli/src/commands/watch.ts b/packages/cli/src/commands/watch.ts index 7fefa66..498c7a1 100644 --- a/packages/cli/src/commands/watch.ts +++ b/packages/cli/src/commands/watch.ts @@ -1,7 +1,12 @@ import { resolveDesktopCapableSession } from "@pickforge/picklab-core"; import { + endHumanTakeover, ensureSessionVnc, openVncViewer, + renewHumanTakeover, + startHumanTakeover, + type HumanTakeoverHandle, + type TakeoverEndReason, } from "@pickforge/picklab-desktop-linux"; import { resolveProjectDir, @@ -13,6 +18,119 @@ import { export interface WatchOptions extends BaseCliOptions { session?: string; waitForViewerExit?: boolean; + control?: boolean; +} + +const TAKEOVER_SIGNALS = ["SIGINT", "SIGTERM"] as const; + +/** + * Hold a human lease for the duration of the viewer wait, heartbeating it + * every `heartbeatMs` so it never lapses while the human is actually there. + * A terminal-wide SIGINT (the common interactive-cancel path) reaches the + * viewer child too, since `openVncViewer` spawns it in our process group — + * this handler exists so *our* process does not exit before running cleanup, + * not to itself interrupt the viewer. `runControlledViewer` always ends the + * takeover (VNC reverted, lease released) before returning or throwing. + */ +async function runControlledViewer( + handle: HumanTakeoverHandle, + port: number, + registryEnv: NodeJS.ProcessEnv, +): Promise<{ + viewer: Awaited>; + reason: TakeoverEndReason; + screenshotPath?: string; +}> { + let cancelled = false; + let leaseLost = false; + const onSignal = (): void => { + cancelled = true; + }; + for (const signal of TAKEOVER_SIGNALS) { + process.on(signal, onSignal); + } + const heartbeat = setInterval(() => { + void renewHumanTakeover(handle, registryEnv).then((renewed) => { + if (!renewed) { + leaseLost = true; + clearInterval(heartbeat); + } + }); + }, handle.heartbeatMs); + heartbeat.unref(); + + try { + const viewer = await openVncViewer({ port, waitForExit: true }); + const reason: TakeoverEndReason = leaseLost + ? "timeout" + : cancelled + ? "cancelled" + : "return"; + const result = await endHumanTakeover(handle, { registryEnv, reason }); + return { viewer, reason, screenshotPath: result.screenshotPath }; + } catch (error) { + await endHumanTakeover(handle, { + registryEnv, + reason: leaseLost ? "timeout" : "cancelled", + }).catch(() => {}); + throw error; + } finally { + clearInterval(heartbeat); + for (const signal of TAKEOVER_SIGNALS) { + process.off(signal, onSignal); + } + } +} + +async function watchWithControl( + sessionId: string, + registryEnv: NodeJS.ProcessEnv, +): Promise { + const handle = await startHumanTakeover(sessionId, { registryEnv }); + const data: Record = { + sessionId, + leaseId: handle.leaseId, + vncPid: handle.vncPid, + vncPort: handle.vncPort, + }; + + let controlled: Awaited>; + try { + controlled = await runControlledViewer(handle, handle.vncPort, registryEnv); + } catch (error) { + throw error instanceof Error + ? new Error(`Human takeover for session ${sessionId} ended abnormally: ${error.message}`) + : error; + } + const { viewer, reason, screenshotPath } = controlled; + data.opened = viewer.opened; + data.endpoint = viewer.endpoint; + data.controlReason = reason; + if (viewer.viewer !== undefined) data.viewer = viewer.viewer; + if (viewer.exitCode !== undefined) data.viewerExitCode = viewer.exitCode; + if (viewer.signal !== undefined) data.viewerSignal = viewer.signal; + if (viewer.guidance !== undefined) data.guidance = viewer.guidance; + if (screenshotPath !== undefined) data.resumeScreenshot = screenshotPath; + + if (!viewer.opened) { + return { + data, + errors: [ + `No writable VNC viewer could be opened for session ${sessionId}; ` + + "control was granted and immediately returned. " + + String(viewer.guidance ?? ""), + ], + }; + } + const lines = [ + reason === "timeout" + ? `human control lease for session ${sessionId} could not be renewed and was ended` + : `human control for session ${sessionId} returned (${reason}); VNC is read-only again`, + ]; + if (screenshotPath !== undefined) { + lines.push(`resume screenshot recorded: ${screenshotPath}`); + } + return { data, lines }; } export async function watchDesktopSession( @@ -21,6 +139,14 @@ export async function watchDesktopSession( const record = await resolveDesktopCapableSession(opts.session, { projectDir: resolveProjectDir(opts), }); + if (opts.control === true) { + if (opts.waitForViewerExit === false) { + throw new Error( + "--control requires waiting for the viewer to exit, to know when to end human control", + ); + } + return watchWithControl(record.id, process.env); + } const vnc = await ensureSessionVnc(record.id); const viewer = await openVncViewer({ port: vnc.port, diff --git a/packages/cli/src/program.ts b/packages/cli/src/program.ts index 0b7c929..910edad 100644 --- a/packages/cli/src/program.ts +++ b/packages/cli/src/program.ts @@ -47,6 +47,7 @@ import { } from "./commands/session.js"; import { runSetupAndroid } from "./commands/setup-android.js"; import { runSetupLabUser } from "./commands/setup-lab-user.js"; +import { runTakeoverStatus } from "./commands/takeover.js"; import { runWatch } from "./commands/watch.js"; const require = createRequire(import.meta.url); @@ -211,12 +212,30 @@ export function buildProgram(): Command { withDesktopSession( program .command("watch") - .description("Watch a running desktop-capable session read-only"), + .description("Watch a running desktop-capable session read-only") + .option( + "--control", + "pause agent input and grant a temporary writable viewer (supervised human takeover)", + ), ), ).action(async (opts) => { process.exitCode = await runWatch(opts); }); + const takeover = program + .command("takeover") + .description("Inspect supervised human-takeover state for a session"); + + withJson( + withDesktopSession( + takeover + .command("status") + .description("Show whether a session is under human control"), + ), + ).action(async (opts) => { + process.exitCode = await runTakeoverStatus(opts); + }); + const browser = program .command("browser") .description("Connect agent browser tooling to the active PickLab browser"); diff --git a/packages/cli/test/desktop-input-takeover.test.ts b/packages/cli/test/desktop-input-takeover.test.ts new file mode 100644 index 0000000..c3643af --- /dev/null +++ b/packages/cli/test/desktop-input-takeover.test.ts @@ -0,0 +1,69 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + acquireHumanLease, + createSession, + releaseHumanLease, + type EnvLike, +} from "@pickforge/picklab-core"; +import { runDesktopClick, runDesktopType, runDesktopKey } from "../src/commands/desktop.js"; + +let root: string; +let env: EnvLike; +let logs: string[]; + +beforeEach(async () => { + root = await fs.promises.mkdtemp(path.join(os.tmpdir(), "picklab-desktop-takeover-")); + env = { PICKLAB_HOME: path.join(root, "home") }; + process.env.PICKLAB_HOME = env.PICKLAB_HOME; + logs = []; + vi.spyOn(console, "log").mockImplementation((line: string) => { + logs.push(line); + }); + vi.spyOn(console, "error").mockImplementation(() => {}); +}); + +afterEach(async () => { + vi.restoreAllMocks(); + delete process.env.PICKLAB_HOME; + process.exitCode = 0; + await fs.promises.rm(root, { recursive: true, force: true }); +}); + +function lastReport(): Record { + return JSON.parse(logs[logs.length - 1]) as Record; +} + +async function createDesktop(): Promise { + const record = await createSession( + { type: "desktop", projectDir: root, status: "running", desktop: { display: ":42" } }, + env, + ); + return record.id; +} + +describe("desktop input commands fail closed under human control", () => { + it("rejects click, type, and key without ever touching xdotool", async () => { + const id = await createDesktop(); + const lease = await acquireHumanLease(id, env); + + expect(await runDesktopClick("1", "1", { session: id, projectDir: root, json: true })).toBe( + 1, + ); + expect(lastReport().errors[0]).toContain("human control is active"); + + expect( + await runDesktopType("hello", { session: id, projectDir: root, json: true }), + ).toBe(1); + expect(lastReport().errors[0]).toContain("human control is active"); + + expect( + await runDesktopKey("Return", { session: id, projectDir: root, json: true }), + ).toBe(1); + expect(lastReport().errors[0]).toContain("human control is active"); + + await releaseHumanLease(id, lease.leaseId, env); + }); +}); diff --git a/packages/cli/test/takeover-status.test.ts b/packages/cli/test/takeover-status.test.ts new file mode 100644 index 0000000..936fef1 --- /dev/null +++ b/packages/cli/test/takeover-status.test.ts @@ -0,0 +1,67 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + acquireHumanLease, + createSession, + releaseHumanLease, + type EnvLike, +} from "@pickforge/picklab-core"; +import { takeoverStatus } from "../src/commands/takeover.js"; + +let root: string; +let env: EnvLike; + +beforeEach(async () => { + root = await fs.promises.mkdtemp(path.join(os.tmpdir(), "picklab-takeover-status-")); + env = { PICKLAB_HOME: path.join(root, "home") }; + process.env.PICKLAB_HOME = env.PICKLAB_HOME; +}); + +afterEach(async () => { + delete process.env.PICKLAB_HOME; + await fs.promises.rm(root, { recursive: true, force: true }); +}); + +async function createDesktop(): Promise { + const record = await createSession( + { type: "desktop", projectDir: root, status: "running", desktop: { display: ":42" } }, + env, + ); + return record.id; +} + +describe("takeoverStatus", () => { + it("reports agent-active when no lease exists", async () => { + const id = await createDesktop(); + const result = await takeoverStatus({ session: id, projectDir: root }); + expect(result.data).toEqual({ sessionId: id, active: false }); + expect(result.lines?.join("\n")).toContain("agent-active"); + }); + + it("reports human-active with lease details while a live lease is held", async () => { + const id = await createDesktop(); + const lease = await acquireHumanLease(id, env); + const result = await takeoverStatus({ session: id, projectDir: root }); + expect(result.data).toMatchObject({ + sessionId: id, + active: true, + lease: { leaseId: lease.leaseId, ownerPid: lease.ownerPid }, + }); + expect(result.lines?.join("\n")).toContain("under human control"); + await releaseHumanLease(id, lease.leaseId, env); + }); + + it("reports a stale lease as recoverable, not active", async () => { + const id = await createDesktop(); + const lease = await acquireHumanLease(id, env); + const stalePath = path.join(env.PICKLAB_HOME as string, "sessions", id, "human.lease.json"); + const stale = { ...lease, ownerPid: 999_999 }; + await fs.promises.writeFile(stalePath, `${JSON.stringify(stale)}\n`); + + const result = await takeoverStatus({ session: id, projectDir: root }); + expect(result.data).toMatchObject({ sessionId: id, active: false, stale: true }); + expect(result.lines?.join("\n")).toContain("stale human lease"); + }); +}); diff --git a/packages/cli/test/watch-control.test.ts b/packages/cli/test/watch-control.test.ts new file mode 100644 index 0000000..cbcbd3f --- /dev/null +++ b/packages/cli/test/watch-control.test.ts @@ -0,0 +1,199 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { + EndHumanTakeoverResult, + HumanTakeoverHandle, + OpenVncViewerResult, +} from "@pickforge/picklab-desktop-linux"; + +const { startHumanTakeover, endHumanTakeover, renewHumanTakeover, openVncViewer } = + vi.hoisted(() => ({ + startHumanTakeover: vi.fn(), + endHumanTakeover: vi.fn(), + renewHumanTakeover: vi.fn(), + openVncViewer: vi.fn(), + })); + +vi.mock("@pickforge/picklab-desktop-linux", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + startHumanTakeover, + endHumanTakeover, + renewHumanTakeover, + openVncViewer, + }; +}); + +import { createSession } from "@pickforge/picklab-core"; +import { watchDesktopSession } from "../src/commands/watch.js"; + +let root: string; + +function fakeHandle(overrides: Partial = {}): HumanTakeoverHandle { + return { + sessionId: "desk-aaaaaa11", + leaseId: "lease-1", + display: ":42", + vncPid: 4242, + vncPort: 5942, + vncStartTimeTicks: 1, + ttlMs: 30_000, + heartbeatMs: 15, + projectDir: root, + ...overrides, + }; +} + +function fakeViewer(overrides: Partial = {}): OpenVncViewerResult { + return { + opened: true, + endpoint: "vnc://127.0.0.1:5942", + viewer: "remote-viewer", + exitCode: 0, + signal: null, + ...overrides, + }; +} + +async function delayedResolve(value: T, ms: number): Promise { + await new Promise((resolve) => setTimeout(resolve, ms)); + return value; +} + +async function createDesktop(): Promise { + const record = await createSession( + { type: "desktop", projectDir: root, status: "running", desktop: { display: ":42" } }, + { PICKLAB_HOME: path.join(root, "home") }, + ); + return record.id; +} + +beforeEach(async () => { + root = await fs.promises.mkdtemp(path.join(os.tmpdir(), "picklab-watch-control-")); + process.env.PICKLAB_HOME = path.join(root, "home"); + startHumanTakeover.mockReset(); + endHumanTakeover.mockReset(); + renewHumanTakeover.mockReset(); + openVncViewer.mockReset(); + endHumanTakeover.mockResolvedValue({ reason: "return" } satisfies EndHumanTakeoverResult); + renewHumanTakeover.mockResolvedValue(true); +}); + +afterEach(async () => { + delete process.env.PICKLAB_HOME; + await fs.promises.rm(root, { recursive: true, force: true }); +}); + +describe("watch --control (mocked)", () => { + it("rejects --control combined with waitForViewerExit: false before any side effect", async () => { + const id = await createDesktop(); + await expect( + watchDesktopSession({ + session: id, + projectDir: root, + control: true, + waitForViewerExit: false, + }), + ).rejects.toThrow(/requires waiting for the viewer to exit/); + expect(startHumanTakeover).not.toHaveBeenCalled(); + }); + + it("starts human takeover, waits for the viewer, and ends with reason 'return'", async () => { + const id = await createDesktop(); + const handle = fakeHandle({ sessionId: id }); + startHumanTakeover.mockResolvedValue(handle); + openVncViewer.mockResolvedValue(fakeViewer()); + endHumanTakeover.mockResolvedValue({ + reason: "return", + screenshotPath: "screenshots/abc.png", + } satisfies EndHumanTakeoverResult); + + const result = await watchDesktopSession({ session: id, projectDir: root, control: true }); + + expect(startHumanTakeover).toHaveBeenCalledWith(id, { registryEnv: process.env }); + expect(openVncViewer).toHaveBeenCalledWith({ port: handle.vncPort, waitForExit: true }); + expect(endHumanTakeover).toHaveBeenCalledWith(handle, { + registryEnv: process.env, + reason: "return", + }); + expect(result.data).toMatchObject({ + sessionId: id, + leaseId: handle.leaseId, + opened: true, + controlReason: "return", + resumeScreenshot: "screenshots/abc.png", + }); + expect(result.lines?.join("\n")).toContain("returned (return)"); + }); + + it("ends with reason 'cancelled' when a SIGINT arrives while the viewer is open", async () => { + const id = await createDesktop(); + const handle = fakeHandle({ sessionId: id, heartbeatMs: 10_000 }); + startHumanTakeover.mockResolvedValue(handle); + openVncViewer.mockImplementation(() => delayedResolve(fakeViewer({ signal: "SIGTERM" }), 60)); + endHumanTakeover.mockImplementation(async (_handle, opts) => ({ + reason: opts.reason, + })); + + const pending = watchDesktopSession({ session: id, projectDir: root, control: true }); + await new Promise((resolve) => setTimeout(resolve, 10)); + process.emit("SIGINT"); + const result = await pending; + + expect(endHumanTakeover).toHaveBeenCalledWith( + handle, + expect.objectContaining({ reason: "cancelled" }), + ); + expect(result.data?.controlReason).toBe("cancelled"); + }); + + it("ends with reason 'timeout' when the lease cannot be renewed", async () => { + const id = await createDesktop(); + const handle = fakeHandle({ sessionId: id, heartbeatMs: 10 }); + startHumanTakeover.mockResolvedValue(handle); + renewHumanTakeover.mockResolvedValue(false); + openVncViewer.mockImplementation(() => delayedResolve(fakeViewer(), 80)); + endHumanTakeover.mockImplementation(async (_handle, opts) => ({ + reason: opts.reason, + })); + + const result = await watchDesktopSession({ session: id, projectDir: root, control: true }); + + expect(renewHumanTakeover).toHaveBeenCalled(); + expect(endHumanTakeover).toHaveBeenCalledWith( + handle, + expect.objectContaining({ reason: "timeout" }), + ); + expect(result.data?.controlReason).toBe("timeout"); + expect(result.lines?.join("\n")).toContain("could not be renewed"); + }); + + it("ends control immediately and reports guidance when no viewer can be opened", async () => { + const id = await createDesktop(); + const handle = fakeHandle({ sessionId: id }); + startHumanTakeover.mockResolvedValue(handle); + openVncViewer.mockResolvedValue( + fakeViewer({ + opened: false, + viewer: undefined, + exitCode: undefined, + signal: undefined, + guidance: "No graphical host session is available.", + }), + ); + endHumanTakeover.mockResolvedValue({ reason: "return" } satisfies EndHumanTakeoverResult); + + const result = await watchDesktopSession({ session: id, projectDir: root, control: true }); + + expect(endHumanTakeover).toHaveBeenCalledWith(handle, { + registryEnv: process.env, + reason: "return", + }); + expect(result.errors?.join("\n")).toContain("No writable VNC viewer could be opened"); + expect(result.errors?.join("\n")).toContain("No graphical host session is available."); + }); +}); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 1b0c126..32071a5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -182,6 +182,35 @@ export { type LocalSessionTeardownRuntime, } from "./session-lifecycle.js"; +export { + AGENT_PERMITS_DIR, + HUMAN_LEASE_DRAIN_TIMEOUT_MS, + HUMAN_LEASE_FILE, + HUMAN_LEASE_HEARTBEAT_MS, + HUMAN_LEASE_TTL_MS, + HumanControlActiveError, + HumanLeaseDrainTimeoutError, + HumanLeaseHeldError, + StaleHumanLeaseError, + acquireAgentPermit, + acquireHumanLease, + checkHumanLeaseBusy, + clearStaleHumanLease, + getTakeoverStatus, + isHumanLeaseStale, + readHumanLease, + recordTakeoverEvidence, + releaseAgentPermit, + releaseHumanLease, + renewHumanLease, + withAgentPermit, + type AcquireHumanLeaseOptions, + type AgentPermit, + type HumanLease, + type RecordTakeoverEvidenceOptions, + type TakeoverStatusResult, +} from "./takeover.js"; + export { REAPER_CLEANUP_PENDING_META_KEY, createSession, diff --git a/packages/core/src/takeover.ts b/packages/core/src/takeover.ts new file mode 100644 index 0000000..3b43aab --- /dev/null +++ b/packages/core/src/takeover.ts @@ -0,0 +1,606 @@ +import crypto from "node:crypto"; +import fs from "node:fs"; +import path from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; +import { isEvidenceEnabled, loadConfig } from "./config.js"; +import { appendAction, beginEvidenceRun } from "./evidence.js"; +import { ensureDir, sessionsDir, writeFileAtomic, type EnvLike } from "./paths.js"; +import { isPidAlive, processIdentityMatches, readProcessStartTicks } from "./proc.js"; + +/** + * Supervised pause / human takeover (pickforge/picklab#21). + * + * State machine: `agent-active -> pause-requested -> human-active(lease, + * deadline) -> returning -> agent-active`. + * + * - `agent-active`: no `human.lease.json` in the session directory. Agent + * permits (short-lived files under `permits/`) are always granted. + * - `pause-requested`: `acquireHumanLease` atomically (`wx`) creates the + * lease file — the instant it exists, every agent permit recheck starts + * failing closed — then waits for permits that predate the lease to drain. + * - `human-active`: the lease is held; the caller (desktop-linux) switches + * VNC to writable and renews the lease every `heartbeatMs` until return. + * - `returning`: the caller reverts VNC to read-only and releases the lease + * on every exit path (normal return, cancellation, timeout). + * - Crash recovery: a lease whose owner process died, or whose `expiresAt` + * elapsed without a heartbeat, is "stale" and safe to reclaim — see + * `isHumanLeaseStale` / `StaleHumanLeaseError` / `clearStaleHumanLease`. + * + * This module owns only the storage-backed lease/permit primitives (pure + * `fs` + process-identity logic, testable without X11). VNC-mode switching + * and screenshot/evidence orchestration live in `@pickforge/picklab-desktop-linux`. + */ + +export const HUMAN_LEASE_FILE = "human.lease.json"; +export const AGENT_PERMITS_DIR = "permits"; + +/** Default time-to-live for a human lease between heartbeats (ms). */ +export const HUMAN_LEASE_TTL_MS = 30_000; +/** Default interval at which an active human lease is renewed (ms). */ +export const HUMAN_LEASE_HEARTBEAT_MS = 5_000; +/** Default budget to wait for pre-existing agent permits to drain (ms). */ +export const HUMAN_LEASE_DRAIN_TIMEOUT_MS = 5_000; +const DRAIN_POLL_MS = 25; + +const SAFE_SESSION_ID_PATTERN = /^[a-z0-9][a-z0-9._-]*$/i; + +function assertSafeSessionId(sessionId: string): void { + if (!SAFE_SESSION_ID_PATTERN.test(sessionId) || sessionId.includes("..")) { + throw new Error( + `Invalid session id "${sessionId}": must start with a letter or digit ` + + `and contain only letters, digits, ".", "_", or "-"`, + ); + } +} + +/** A process's liveness, verified by `/proc` start ticks when available. */ +function identityIsAlive(pid: number, startTicks?: number): boolean { + if (startTicks !== undefined) { + return processIdentityMatches({ pid, startTicks }); + } + return isPidAlive(pid); +} + +function sessionStateDir(sessionId: string, env: EnvLike): string { + return path.join(sessionsDir(env), sessionId); +} + +function humanLeasePath(sessionId: string, env: EnvLike): string { + return path.join(sessionStateDir(sessionId, env), HUMAN_LEASE_FILE); +} + +function agentPermitsDir(sessionId: string, env: EnvLike): string { + return path.join(sessionStateDir(sessionId, env), AGENT_PERMITS_DIR); +} + +/** Atomically-published record of who holds human control of a session. */ +export interface HumanLease { + leaseId: string; + sessionId: string; + ownerPid: number; + ownerStartTicks?: number; + createdAt: string; + /** Renewed every `heartbeatMs` while control is held; past this, stale. */ + expiresAt: string; + ttlMs: number; + heartbeatMs: number; + /** Writable VNC server metadata, patched in once VNC has switched mode. */ + vncPid?: number; + vncStartTimeTicks?: number; + vncPort?: number; +} + +/** A short-lived marker an agent action holds while it may deliver input. */ +export interface AgentPermit { + permitId: string; + sessionId: string; + ownerPid: number; + ownerStartTicks?: number; + createdAt: string; + path: string; +} + +/** Thrown when an agent action is refused because human control is active. */ +export class HumanControlActiveError extends Error { + readonly code = "human_control_active"; + readonly lease: HumanLease; + constructor(lease: HumanLease) { + super( + "PickLab: human control is active " + + `(lease ${lease.leaseId}, held since ${lease.createdAt}); ` + + "agent input is paused until control returns", + ); + this.name = "HumanControlActiveError"; + this.lease = lease; + } +} + +/** Thrown when acquiring a human lease finds another *live* owner. */ +export class HumanLeaseHeldError extends Error { + readonly code = "human_lease_held"; + readonly lease: HumanLease; + constructor(lease: HumanLease) { + super( + `Session ${lease.sessionId} is already under human control ` + + `(lease ${lease.leaseId}, owner pid ${lease.ownerPid}, ` + + `held since ${lease.createdAt})`, + ); + this.name = "HumanLeaseHeldError"; + this.lease = lease; + } +} + +/** + * Thrown when acquiring a human lease finds an existing lease file whose + * owner is dead or whose TTL has elapsed. Callers that can also manage the + * writable VNC side (desktop-linux) should stop any recorded VNC process, + * clear the stale file with `clearStaleHumanLease`, then retry. + */ +export class StaleHumanLeaseError extends Error { + readonly code = "stale_human_lease"; + readonly raw: string; + readonly lease?: HumanLease; + constructor(raw: string, lease?: HumanLease) { + super( + lease === undefined + ? "A stale, unreadable human lease file is blocking acquisition" + : `Stale human lease ${lease.leaseId} (owner pid ${lease.ownerPid}, ` + + `expired ${lease.expiresAt}) is blocking acquisition`, + ); + this.name = "StaleHumanLeaseError"; + this.raw = raw; + this.lease = lease; + } +} + +/** Thrown when in-flight agent permits do not drain before the deadline. */ +export class HumanLeaseDrainTimeoutError extends Error { + readonly code = "human_lease_drain_timeout"; + readonly pendingPermitIds: string[]; + constructor(pendingPermitIds: string[]) { + super( + `Timed out waiting for ${pendingPermitIds.length} in-flight agent ` + + "action(s) to finish before granting human control", + ); + this.name = "HumanLeaseDrainTimeoutError"; + this.pendingPermitIds = pendingPermitIds; + } +} + +async function readTextIfPresent(target: string): Promise { + try { + return await fs.promises.readFile(target, "utf8"); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return undefined; + throw error; + } +} + +async function unlinkIfPresent(target: string): Promise { + try { + await fs.promises.unlink(target); + return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return false; + throw error; + } +} + +async function unlinkIfMatches(target: string, expected: string): Promise { + const current = await readTextIfPresent(target); + if (current === undefined || current !== expected) return false; + return unlinkIfPresent(target); +} + +function parseHumanLease(raw: string): HumanLease | undefined { + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return undefined; + } + if (typeof parsed !== "object" || parsed === null) return undefined; + const c = parsed as Record; + if ( + typeof c.leaseId !== "string" || + typeof c.sessionId !== "string" || + typeof c.ownerPid !== "number" || + typeof c.createdAt !== "string" || + typeof c.expiresAt !== "string" || + typeof c.ttlMs !== "number" || + typeof c.heartbeatMs !== "number" + ) { + return undefined; + } + const lease: HumanLease = { + leaseId: c.leaseId, + sessionId: c.sessionId, + ownerPid: c.ownerPid, + createdAt: c.createdAt, + expiresAt: c.expiresAt, + ttlMs: c.ttlMs, + heartbeatMs: c.heartbeatMs, + }; + if (typeof c.ownerStartTicks === "number") lease.ownerStartTicks = c.ownerStartTicks; + if (typeof c.vncPid === "number") lease.vncPid = c.vncPid; + if (typeof c.vncStartTimeTicks === "number") lease.vncStartTimeTicks = c.vncStartTimeTicks; + if (typeof c.vncPort === "number") lease.vncPort = c.vncPort; + return lease; +} + +function parseAgentPermitRecord( + raw: string, +): Omit | undefined { + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return undefined; + } + if (typeof parsed !== "object" || parsed === null) return undefined; + const c = parsed as Record; + if ( + typeof c.permitId !== "string" || + typeof c.sessionId !== "string" || + typeof c.ownerPid !== "number" || + typeof c.createdAt !== "string" + ) { + return undefined; + } + const record: Omit = { + permitId: c.permitId, + sessionId: c.sessionId, + ownerPid: c.ownerPid, + createdAt: c.createdAt, + }; + if (typeof c.ownerStartTicks === "number") record.ownerStartTicks = c.ownerStartTicks; + return record; +} + +/** Read a session's human lease, or `undefined` if absent/unparseable. */ +export async function readHumanLease( + sessionId: string, + env: EnvLike = process.env, +): Promise { + assertSafeSessionId(sessionId); + const raw = await readTextIfPresent(humanLeasePath(sessionId, env)); + return raw === undefined ? undefined : parseHumanLease(raw); +} + +/** + * Whether a lease is stale: its owner process is dead (or its PID was + * reused), or its TTL elapsed without a heartbeat renewal. Either condition + * alone is sufficient — a hung-but-alive owner that stops heartbeating is + * just as reclaimable as a dead one. + */ +export function isHumanLeaseStale(lease: HumanLease, now: Date = new Date()): boolean { + if (!identityIsAlive(lease.ownerPid, lease.ownerStartTicks)) return true; + return now.getTime() > Date.parse(lease.expiresAt); +} + +export interface AcquireHumanLeaseOptions { + ttlMs?: number; + heartbeatMs?: number; + drainTimeoutMs?: number; + now?: Date; + vncPid?: number; + vncStartTimeTicks?: number; + vncPort?: number; + /** + * @internal Test hook, awaited right after the lease is durably created but + * before draining begins. Lets a test hold the lease open while a peer + * races it, proving a live lease is never stolen from under itself. + */ + _afterCreate?: () => void | Promise; +} + +/** + * Acquire exclusive human control of a session: atomically (`wx`) create the + * lease file, then wait for every agent permit that existed at that instant + * to drain (finish, or be recognized as owned by a dead process and swept). + * + * A second acquisition attempt while the lease is live throws + * `HumanLeaseHeldError` immediately — this is a single-winner primitive, not + * a queue. An existing lease whose owner is dead or whose TTL elapsed throws + * `StaleHumanLeaseError` so the caller can recover it (see + * `clearStaleHumanLease`) and retry. If permits fail to drain in time, the + * lease this call just created is released and `HumanLeaseDrainTimeoutError` + * is thrown — "timeout aborts cleanly." + */ +export async function acquireHumanLease( + sessionId: string, + env: EnvLike = process.env, + opts: AcquireHumanLeaseOptions = {}, +): Promise { + assertSafeSessionId(sessionId); + const dir = await ensureDir(sessionStateDir(sessionId, env)); + const leasePath = path.join(dir, HUMAN_LEASE_FILE); + const ttlMs = opts.ttlMs ?? HUMAN_LEASE_TTL_MS; + const heartbeatMs = opts.heartbeatMs ?? HUMAN_LEASE_HEARTBEAT_MS; + const now = opts.now ?? new Date(); + const ownerPid = process.pid; + const ownerStartTicks = readProcessStartTicks(ownerPid); + + const lease: HumanLease = { + leaseId: crypto.randomUUID(), + sessionId, + ownerPid, + createdAt: now.toISOString(), + expiresAt: new Date(now.getTime() + ttlMs).toISOString(), + ttlMs, + heartbeatMs, + }; + if (ownerStartTicks !== undefined) lease.ownerStartTicks = ownerStartTicks; + if (opts.vncPid !== undefined) lease.vncPid = opts.vncPid; + if (opts.vncStartTimeTicks !== undefined) lease.vncStartTimeTicks = opts.vncStartTimeTicks; + if (opts.vncPort !== undefined) lease.vncPort = opts.vncPort; + + try { + await fs.promises.writeFile(leasePath, `${JSON.stringify(lease)}\n`, { + encoding: "utf8", + flag: "wx", + }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; + const raw = await readTextIfPresent(leasePath); + if (raw === undefined) { + // Vanished between our failed create and this read (a peer released + // it). Safe to retry from the caller's side; report as contention + // rather than silently looping here. + throw new Error( + `Transient contention acquiring the human lease for session ${sessionId}; retry`, + ); + } + const existing = parseHumanLease(raw); + if (existing !== undefined && !isHumanLeaseStale(existing, now)) { + throw new HumanLeaseHeldError(existing); + } + throw new StaleHumanLeaseError(raw, existing); + } + + if (opts._afterCreate !== undefined) await opts._afterCreate(); + + try { + await drainAgentPermits( + sessionId, + env, + opts.drainTimeoutMs ?? HUMAN_LEASE_DRAIN_TIMEOUT_MS, + ); + } catch (error) { + await unlinkIfMatches(leasePath, `${JSON.stringify(lease)}\n`).catch(() => {}); + throw error; + } + return lease; +} + +/** + * Clear a lease file identified as stale by `acquireHumanLease` (compare-and- + * delete on its exact raw content), so a fresh acquisition can proceed. The + * caller is responsible for stopping any writable VNC process the stale + * lease recorded before calling this. + */ +export async function clearStaleHumanLease( + sessionId: string, + expectedRaw: string, + env: EnvLike = process.env, +): Promise { + assertSafeSessionId(sessionId); + return unlinkIfMatches(humanLeasePath(sessionId, env), expectedRaw); +} + +/** + * Renew a held lease's TTL (and optionally patch in VNC metadata once + * writable VNC has actually started). Verifies the caller still owns + * `leaseId` before writing, and again immediately after, so a lease this + * process no longer owns is never resurrected. Returns the updated lease, or + * `undefined` if the lease is gone or owned by someone else. + */ +export async function renewHumanLease( + sessionId: string, + leaseId: string, + env: EnvLike = process.env, + patch: { vncPid?: number; vncStartTimeTicks?: number; vncPort?: number } = {}, + now: Date = new Date(), +): Promise { + assertSafeSessionId(sessionId); + const leasePath = humanLeasePath(sessionId, env); + const current = await readHumanLease(sessionId, env); + if (current === undefined || current.leaseId !== leaseId) return undefined; + const updated: HumanLease = { + ...current, + expiresAt: new Date(now.getTime() + current.ttlMs).toISOString(), + }; + if (patch.vncPid !== undefined) updated.vncPid = patch.vncPid; + if (patch.vncStartTimeTicks !== undefined) updated.vncStartTimeTicks = patch.vncStartTimeTicks; + if (patch.vncPort !== undefined) updated.vncPort = patch.vncPort; + await writeFileAtomic(leasePath, `${JSON.stringify(updated)}\n`); + const confirmed = await readHumanLease(sessionId, env); + return confirmed?.leaseId === leaseId ? confirmed : undefined; +} + +/** Release a held lease by id (compare-and-delete). Best-effort/idempotent. */ +export async function releaseHumanLease( + sessionId: string, + leaseId: string, + env: EnvLike = process.env, +): Promise { + assertSafeSessionId(sessionId); + const leasePath = humanLeasePath(sessionId, env); + const raw = await readTextIfPresent(leasePath); + if (raw === undefined) return false; + const current = parseHumanLease(raw); + if (current?.leaseId !== leaseId) return false; + return unlinkIfMatches(leasePath, raw); +} + +export interface RecordTakeoverEvidenceOptions { + env?: EnvLike; + status?: "ok" | "error"; + artifacts?: string[]; +} + +/** + * Append one best-effort takeover lifecycle-transition evidence action + * (`takeover_start`, `takeover_return`, `takeover_timeout`, + * `takeover_cancelled`, `takeover_recovered`, ...). Never throws — a + * transition (VNC mode switch, lease release) must never fail because its + * evidence entry could not be written. + */ +export async function recordTakeoverEvidence( + projectDir: string, + sessionId: string, + tool: string, + opts: RecordTakeoverEvidenceOptions = {}, +): Promise { + try { + if (!isEvidenceEnabled(await loadConfig(projectDir, opts.env))) return; + const { run } = await beginEvidenceRun( + projectDir, + sessionId, + { slug: "computer-use" }, + opts.env, + ); + await appendAction(run.dir, { + actionId: crypto.randomUUID(), + source: "takeover", + tool, + sessionId, + startedAt: new Date().toISOString(), + status: opts.status ?? "ok", + ...(opts.artifacts === undefined ? {} : { artifacts: opts.artifacts }), + }); + } catch { + // Best-effort; see doc comment. + } +} + +/** Read-only classification of a session's takeover state. */ +export interface TakeoverStatusResult { + sessionId: string; + /** True only while a live (non-stale) human lease is held. */ + active: boolean; + lease?: HumanLease; + /** True when a lease file exists but its owner is dead/expired. */ + stale?: boolean; +} + +export async function getTakeoverStatus( + sessionId: string, + env: EnvLike = process.env, +): Promise { + assertSafeSessionId(sessionId); + const lease = await readHumanLease(sessionId, env); + if (lease === undefined) return { sessionId, active: false }; + if (isHumanLeaseStale(lease)) return { sessionId, active: false, lease, stale: true }; + return { sessionId, active: true, lease }; +} + +/** + * Read-only recheck used by the fail-closed input/relay gate: returns the + * live lease if human control is active, `undefined` otherwise (including + * when the only lease on disk is stale — a hung/dead human owner never + * blocks the agent). + */ +export async function checkHumanLeaseBusy( + sessionId: string, + env: EnvLike = process.env, +): Promise { + const lease = await readHumanLease(sessionId, env); + return lease !== undefined && !isHumanLeaseStale(lease) ? lease : undefined; +} + +/** Acquire a short-lived marker recording that an agent action may run. */ +export async function acquireAgentPermit( + sessionId: string, + env: EnvLike = process.env, +): Promise { + assertSafeSessionId(sessionId); + const dir = await ensureDir(agentPermitsDir(sessionId, env)); + const ownerPid = process.pid; + const ownerStartTicks = readProcessStartTicks(ownerPid); + const permitId = crypto.randomUUID(); + const record: Omit = { + permitId, + sessionId, + ownerPid, + createdAt: new Date().toISOString(), + }; + if (ownerStartTicks !== undefined) record.ownerStartTicks = ownerStartTicks; + const permitPath = path.join(dir, `${permitId}.json`); + await fs.promises.writeFile(permitPath, `${JSON.stringify(record)}\n`, { + encoding: "utf8", + flag: "wx", + }); + return { ...record, path: permitPath }; +} + +/** Release a previously-acquired agent permit. Best-effort/idempotent. */ +export async function releaseAgentPermit(permit: AgentPermit): Promise { + await unlinkIfPresent(permit.path); +} + +async function drainAgentPermits( + sessionId: string, + env: EnvLike, + timeoutMs: number, +): Promise { + const dir = agentPermitsDir(sessionId, env); + let entries: string[]; + try { + entries = (await fs.promises.readdir(dir)).filter((name) => name.endsWith(".json")); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return; + throw error; + } + const pending = new Set(entries); + const deadline = Date.now() + timeoutMs; + + while (pending.size > 0) { + for (const name of [...pending]) { + const full = path.join(dir, name); + const raw = await readTextIfPresent(full); + if (raw === undefined) { + pending.delete(name); + continue; + } + const record = parseAgentPermitRecord(raw); + if (record === undefined || !identityIsAlive(record.ownerPid, record.ownerStartTicks)) { + // Corrupt, or owned by a dead process (crashed mid-action): sweep it + // so a crash never permanently blocks a takeover. + await unlinkIfPresent(full).catch(() => {}); + pending.delete(name); + } + } + if (pending.size === 0) return; + if (Date.now() >= deadline) { + throw new HumanLeaseDrainTimeoutError( + [...pending].map((name) => name.replace(/\.json$/, "")), + ); + } + await delay(DRAIN_POLL_MS); + } +} + +/** + * Run `action` while holding a fail-closed agent permit: acquire the permit, + * recheck for a live human lease, execute only if none is found, then + * release the permit in `finally`. Throws `HumanControlActiveError` (never + * runs `action`) when human control is active — "no permit fitness ⇒ no + * input delivery." + */ +export async function withAgentPermit( + sessionId: string, + env: EnvLike, + action: () => Promise, +): Promise { + const permit = await acquireAgentPermit(sessionId, env); + try { + const lease = await checkHumanLeaseBusy(sessionId, env); + if (lease !== undefined) { + throw new HumanControlActiveError(lease); + } + return await action(); + } finally { + await releaseAgentPermit(permit); + } +} diff --git a/packages/core/test/takeover.test.ts b/packages/core/test/takeover.test.ts new file mode 100644 index 0000000..f31adef --- /dev/null +++ b/packages/core/test/takeover.test.ts @@ -0,0 +1,303 @@ +import { spawn } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + HumanControlActiveError, + HumanLeaseDrainTimeoutError, + HumanLeaseHeldError, + StaleHumanLeaseError, + acquireAgentPermit, + acquireHumanLease, + checkHumanLeaseBusy, + clearStaleHumanLease, + getTakeoverStatus, + isHumanLeaseStale, + readHumanLease, + releaseAgentPermit, + releaseHumanLease, + renewHumanLease, + withAgentPermit, + type EnvLike, + type HumanLease, +} from "../src/index.js"; + +const DEAD_PID = 999_999; + +let tmpRoot: string; +let env: EnvLike; + +beforeEach(() => { + tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), "picklab-takeover-test-")); + env = { ...process.env, PICKLAB_HOME: path.join(tmpRoot, "home") }; +}); + +afterEach(() => { + fs.rmSync(tmpRoot, { recursive: true, force: true }); +}); + +async function writeRawLease(sessionId: string, lease: HumanLease): Promise { + const dir = path.join(env.PICKLAB_HOME as string, "sessions", sessionId); + fs.mkdirSync(dir, { recursive: true }); + const raw = `${JSON.stringify(lease)}\n`; + fs.writeFileSync(path.join(dir, "human.lease.json"), raw); + return raw; +} + +describe("acquireHumanLease", () => { + it("acquires a fresh lease and persists it atomically", async () => { + const lease = await acquireHumanLease("desk-a1", env); + expect(lease.sessionId).toBe("desk-a1"); + expect(lease.ownerPid).toBe(process.pid); + expect(lease.ttlMs).toBe(30_000); + expect(lease.heartbeatMs).toBe(5_000); + const onDisk = await readHumanLease("desk-a1", env); + expect(onDisk).toEqual(lease); + }); + + it("refuses a second acquisition while the lease is live", async () => { + const first = await acquireHumanLease("desk-a2", env); + await expect(acquireHumanLease("desk-a2", env)).rejects.toThrow(HumanLeaseHeldError); + // The live lease is untouched by the failed attempt. + expect((await readHumanLease("desk-a2", env))?.leaseId).toBe(first.leaseId); + }); + + it("reports a dead-owner lease as stale and recoverable", async () => { + const stale: HumanLease = { + leaseId: "dead-lease", + sessionId: "desk-a3", + ownerPid: DEAD_PID, + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 60_000).toISOString(), + ttlMs: 30_000, + heartbeatMs: 5_000, + }; + const raw = await writeRawLease("desk-a3", stale); + expect(isHumanLeaseStale(stale)).toBe(true); + + let caught: StaleHumanLeaseError | undefined; + try { + await acquireHumanLease("desk-a3", env); + } catch (error) { + caught = error as StaleHumanLeaseError; + } + expect(caught).toBeInstanceOf(StaleHumanLeaseError); + expect(caught?.lease?.leaseId).toBe("dead-lease"); + + expect(await clearStaleHumanLease("desk-a3", raw, env)).toBe(true); + const fresh = await acquireHumanLease("desk-a3", env); + expect(fresh.leaseId).not.toBe("dead-lease"); + }); + + it("reports a TTL-expired lease as stale even with a live owner", async () => { + const expired: HumanLease = { + leaseId: "expired-lease", + sessionId: "desk-a4", + ownerPid: process.pid, + createdAt: new Date(Date.now() - 120_000).toISOString(), + expiresAt: new Date(Date.now() - 60_000).toISOString(), + ttlMs: 30_000, + heartbeatMs: 5_000, + }; + await writeRawLease("desk-a4", expired); + await expect(acquireHumanLease("desk-a4", env)).rejects.toThrow(StaleHumanLeaseError); + }); + + it("drains pre-existing agent permits before returning", async () => { + const permit = await acquireAgentPermit("desk-a5", env); + const acquiring = acquireHumanLease("desk-a5", env, { drainTimeoutMs: 2_000 }); + // Give the drain loop a moment to observe the permit as pending, then + // release it — acquisition must complete once it drains. + await new Promise((resolve) => setTimeout(resolve, 60)); + await releaseAgentPermit(permit); + await expect(acquiring).resolves.toMatchObject({ sessionId: "desk-a5" }); + }); + + it("times out and releases its own lease when permits do not drain", async () => { + const permit = await acquireAgentPermit("desk-a6", env); + await expect( + acquireHumanLease("desk-a6", env, { drainTimeoutMs: 80 }), + ).rejects.toThrow(HumanLeaseDrainTimeoutError); + // The lease created for the failed attempt must not linger. + expect(await readHumanLease("desk-a6", env)).toBeUndefined(); + await releaseAgentPermit(permit); + }); + + it("sweeps a permit owned by a dead process instead of blocking the drain", async () => { + const dir = path.join(env.PICKLAB_HOME as string, "sessions", "desk-a7", "permits"); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, "dead-permit.json"), + JSON.stringify({ + permitId: "dead-permit", + sessionId: "desk-a7", + ownerPid: DEAD_PID, + createdAt: new Date().toISOString(), + }), + ); + const lease = await acquireHumanLease("desk-a7", env, { drainTimeoutMs: 500 }); + expect(lease.sessionId).toBe("desk-a7"); + expect(fs.existsSync(path.join(dir, "dead-permit.json"))).toBe(false); + }); +}); + +describe("withAgentPermit", () => { + it("runs the action and cleans up its permit when no lease is held", async () => { + const result = await withAgentPermit("desk-b1", env, async () => "ran"); + expect(result).toBe("ran"); + const permitsDir = path.join(env.PICKLAB_HOME as string, "sessions", "desk-b1", "permits"); + expect(fs.existsSync(permitsDir) ? fs.readdirSync(permitsDir) : []).toEqual([]); + }); + + it("fails closed and never runs the action while human control is active", async () => { + await acquireHumanLease("desk-b2", env); + let ran = false; + await expect( + withAgentPermit("desk-b2", env, async () => { + ran = true; + }), + ).rejects.toThrow(HumanControlActiveError); + expect(ran).toBe(false); + const permitsDir = path.join(env.PICKLAB_HOME as string, "sessions", "desk-b2", "permits"); + expect(fs.existsSync(permitsDir) ? fs.readdirSync(permitsDir) : []).toEqual([]); + }); + + it("invalidates an in-flight permit the instant a lease appears mid-flight", async () => { + // Simulates the race window `withAgentPermit`'s recheck closes: an agent + // permit already exists (step 1 of the 4-step protocol) when a human + // lease is published concurrently (elsewhere), before this permit's + // holder gets to its recheck (step 2). Written directly rather than via + // `acquireHumanLease`, whose drain would otherwise wait on this same + // permit — the recheck ordering being asserted here is independent of + // that drain mechanics. + const permit = await acquireAgentPermit("desk-b3", env); + expect(await checkHumanLeaseBusy("desk-b3", env)).toBeUndefined(); + const lease: HumanLease = { + leaseId: "concurrent-lease", + sessionId: "desk-b3", + ownerPid: process.pid, + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 30_000).toISOString(), + ttlMs: 30_000, + heartbeatMs: 5_000, + }; + await writeRawLease("desk-b3", lease); + expect(await checkHumanLeaseBusy("desk-b3", env)).toMatchObject({ + leaseId: "concurrent-lease", + }); + await releaseAgentPermit(permit); + }); + + it("does not fail closed against a stale lease", async () => { + const stale: HumanLease = { + leaseId: "dead-lease-b4", + sessionId: "desk-b4", + ownerPid: DEAD_PID, + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 60_000).toISOString(), + ttlMs: 30_000, + heartbeatMs: 5_000, + }; + await writeRawLease("desk-b4", stale); + const result = await withAgentPermit("desk-b4", env, async () => "ran"); + expect(result).toBe("ran"); + }); +}); + +describe("renewHumanLease / releaseHumanLease", () => { + it("extends expiresAt only for the owning leaseId", async () => { + const lease = await acquireHumanLease("desk-c1", env); + const renewed = await renewHumanLease("desk-c1", lease.leaseId, env, { + vncPid: 12345, + vncPort: 5901, + }); + expect(renewed).toBeDefined(); + expect(Date.parse(renewed!.expiresAt)).toBeGreaterThanOrEqual( + Date.parse(lease.expiresAt), + ); + expect(renewed?.vncPid).toBe(12345); + expect(renewed?.vncPort).toBe(5901); + + expect(await renewHumanLease("desk-c1", "not-the-owner", env)).toBeUndefined(); + }); + + it("only releases the lease it owns", async () => { + const lease = await acquireHumanLease("desk-c2", env); + expect(await releaseHumanLease("desk-c2", "not-the-owner", env)).toBe(false); + expect(await readHumanLease("desk-c2", env)).toBeDefined(); + expect(await releaseHumanLease("desk-c2", lease.leaseId, env)).toBe(true); + expect(await readHumanLease("desk-c2", env)).toBeUndefined(); + }); +}); + +describe("getTakeoverStatus", () => { + it("reports agent-active, human-active, and stale", async () => { + expect(await getTakeoverStatus("desk-d1", env)).toEqual({ + sessionId: "desk-d1", + active: false, + }); + + const lease = await acquireHumanLease("desk-d1", env); + const active = await getTakeoverStatus("desk-d1", env); + expect(active.active).toBe(true); + expect(active.lease?.leaseId).toBe(lease.leaseId); + + await releaseHumanLease("desk-d1", lease.leaseId, env); + const stale: HumanLease = { + leaseId: "dead-lease-d1", + sessionId: "desk-d1", + ownerPid: DEAD_PID, + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 60_000).toISOString(), + ttlMs: 30_000, + heartbeatMs: 5_000, + }; + await writeRawLease("desk-d1", stale); + const staleStatus = await getTakeoverStatus("desk-d1", env); + expect(staleStatus).toMatchObject({ active: false, stale: true }); + }); +}); + +// Real separate-process race: spawns two genuine OS processes (via `bun`, the +// repo's test runtime) so the `wx` claim protocol is proven under real +// concurrency, not just in-process Promise.all with a single shared PID. +const BUN = /[\\/]bun$/.test(process.execPath) ? process.execPath : "bun"; +const acquireWorker = fileURLToPath( + new URL("./workers/takeover-acquire-worker.ts", import.meta.url), +); + +interface ProcResult { + code: number | null; + stdout: string; +} + +function run(args: string[]): Promise { + return new Promise((resolve, reject) => { + const child = spawn(BUN, args, { stdio: ["ignore", "pipe", "pipe"] }); + let stdout = ""; + child.stdout.on("data", (chunk) => (stdout += chunk)); + child.on("error", reject); + child.on("close", (code) => resolve({ code, stdout })); + }); +} + +describe("real separate-process concurrency", () => { + it( + "two concurrent claims yield exactly one winner", + async () => { + const home = path.join(tmpRoot, "race-home"); + const results = await Promise.all([ + run([acquireWorker, home, "desk-race", "150"]), + run([acquireWorker, home, "desk-race", "150"]), + ]); + const outcomes = results.map((r) => JSON.parse(r.stdout.trim()) as { won: boolean }); + const winners = outcomes.filter((o) => o.won === true); + const losers = outcomes.filter((o) => o.won === false); + expect(winners).toHaveLength(1); + expect(losers).toHaveLength(1); + }, + 10_000, + ); +}); diff --git a/packages/core/test/workers/takeover-acquire-worker.ts b/packages/core/test/workers/takeover-acquire-worker.ts new file mode 100644 index 0000000..6a45147 --- /dev/null +++ b/packages/core/test/workers/takeover-acquire-worker.ts @@ -0,0 +1,30 @@ +// Separate-process human-lease acquire worker, run with `bun`. Races real +// `acquireHumanLease` calls from distinct OS processes (genuinely distinct +// PIDs, unlike in-process races) to prove the `wx` claim protocol yields +// exactly one winner. Not a `*.test.ts` file, so vitest never runs it +// directly. +import { setTimeout as delay } from "node:timers/promises"; +import { acquireHumanLease, releaseHumanLease } from "../../src/takeover.js"; + +const home = process.argv[2]; +const sessionId = process.argv[3]; +const holdMs = process.argv[4] === undefined ? 0 : Number(process.argv[4]); +if (home === undefined || sessionId === undefined || !Number.isFinite(holdMs)) { + console.error("usage: takeover-acquire-worker [holdMs]"); + process.exit(2); +} + +const env = { ...process.env, PICKLAB_HOME: home }; + +try { + const lease = await acquireHumanLease(sessionId, env, { drainTimeoutMs: 2_000 }); + process.stdout.write(`${JSON.stringify({ won: true, leaseId: lease.leaseId })}\n`); + if (holdMs > 0) await delay(holdMs); + await releaseHumanLease(sessionId, lease.leaseId, env); + process.exit(0); +} catch (error) { + process.stdout.write( + `${JSON.stringify({ won: false, error: error instanceof Error ? error.name : String(error) })}\n`, + ); + process.exit(0); +} diff --git a/packages/desktop-linux/src/index.ts b/packages/desktop-linux/src/index.ts index 117fcf9..7e10662 100644 --- a/packages/desktop-linux/src/index.ts +++ b/packages/desktop-linux/src/index.ts @@ -103,3 +103,15 @@ export { } from "./session.js"; export { findOnPath } from "./util.js"; + +export { + endHumanTakeover, + recoverStaleHumanLease, + renewHumanTakeover, + startHumanTakeover, + type EndHumanTakeoverOptions, + type EndHumanTakeoverResult, + type HumanTakeoverHandle, + type StartHumanTakeoverOptions, + type TakeoverEndReason, +} from "./takeover.js"; diff --git a/packages/desktop-linux/src/input.ts b/packages/desktop-linux/src/input.ts index ca27515..0e42830 100644 --- a/packages/desktop-linux/src/input.ts +++ b/packages/desktop-linux/src/input.ts @@ -1,4 +1,4 @@ -import { runCommand } from "@pickforge/picklab-core"; +import { runCommand, withAgentPermit, type EnvLike } from "@pickforge/picklab-core"; import { parseDisplayNumber } from "./display.js"; const TYPE_DELAY_MS = 50; @@ -25,6 +25,8 @@ export interface ClickArgsOptions { export interface ClickOptions extends ClickArgsOptions { display: string; + sessionId: string; + env?: EnvLike; } export interface MoveArgsOptions { @@ -34,6 +36,8 @@ export interface MoveArgsOptions { export interface MoveOptions extends MoveArgsOptions { display: string; + sessionId: string; + env?: EnvLike; } export interface ScrollArgsOptions { @@ -45,6 +49,8 @@ export interface ScrollArgsOptions { export interface ScrollOptions extends ScrollArgsOptions { display: string; + sessionId: string; + env?: EnvLike; } export interface DragArgsOptions { @@ -58,6 +64,8 @@ export interface DragArgsOptions { export interface DragOptions extends DragArgsOptions { display: string; + sessionId: string; + env?: EnvLike; } export interface DoubleClickArgsOptions { @@ -69,16 +77,22 @@ export interface DoubleClickArgsOptions { export interface DoubleClickOptions extends DoubleClickArgsOptions { display: string; + sessionId: string; + env?: EnvLike; } export interface TypeTextOptions { display: string; text: string; + sessionId: string; + env?: EnvLike; } export interface PressKeyOptions { display: string; key: string; + sessionId: string; + env?: EnvLike; } function assertCoordinate(value: number, label: string): void { @@ -273,67 +287,89 @@ async function runXdotool( }); } +/** + * Every desktop input call below is gated by `withAgentPermit`: it acquires + * a short-lived agent permit, rechecks for a live human lease, delivers the + * input only if none is found, and releases the permit in `finally` — + * fail-closed, per pickforge/picklab#21. No permit fitness (or a lease + * appearing before the recheck) means no input delivery. + */ + export async function click(opts: ClickOptions): Promise { - await runXdotool( - opts.display, - buildClickArgs({ x: opts.x, y: opts.y, button: opts.button }), - INPUT_TIMEOUT_MS, + await withAgentPermit(opts.sessionId, opts.env ?? process.env, () => + runXdotool( + opts.display, + buildClickArgs({ x: opts.x, y: opts.y, button: opts.button }), + INPUT_TIMEOUT_MS, + ), ); } export async function move(opts: MoveOptions): Promise { - await runXdotool( - opts.display, - buildMoveArgs({ x: opts.x, y: opts.y }), - INPUT_TIMEOUT_MS, + await withAgentPermit(opts.sessionId, opts.env ?? process.env, () => + runXdotool( + opts.display, + buildMoveArgs({ x: opts.x, y: opts.y }), + INPUT_TIMEOUT_MS, + ), ); } export async function scroll(opts: ScrollOptions): Promise { - await runXdotool( - opts.display, - buildScrollArgs({ - deltaX: opts.deltaX, - deltaY: opts.deltaY, - x: opts.x, - y: opts.y, - }), - INPUT_TIMEOUT_MS, + await withAgentPermit(opts.sessionId, opts.env ?? process.env, () => + runXdotool( + opts.display, + buildScrollArgs({ + deltaX: opts.deltaX, + deltaY: opts.deltaY, + x: opts.x, + y: opts.y, + }), + INPUT_TIMEOUT_MS, + ), ); } export async function drag(opts: DragOptions): Promise { - await runXdotool( - opts.display, - buildDragArgs({ - fromX: opts.fromX, - fromY: opts.fromY, - toX: opts.toX, - toY: opts.toY, - button: opts.button, - durationMs: opts.durationMs, - }), - INPUT_TIMEOUT_MS + MAX_DRAG_DURATION_MS, + await withAgentPermit(opts.sessionId, opts.env ?? process.env, () => + runXdotool( + opts.display, + buildDragArgs({ + fromX: opts.fromX, + fromY: opts.fromY, + toX: opts.toX, + toY: opts.toY, + button: opts.button, + durationMs: opts.durationMs, + }), + INPUT_TIMEOUT_MS + MAX_DRAG_DURATION_MS, + ), ); } export async function doubleClick(opts: DoubleClickOptions): Promise { - await runXdotool( - opts.display, - buildDoubleClickArgs({ - x: opts.x, - y: opts.y, - button: opts.button, - intervalMs: opts.intervalMs, - }), - INPUT_TIMEOUT_MS + MAX_DOUBLE_CLICK_INTERVAL_MS, + await withAgentPermit(opts.sessionId, opts.env ?? process.env, () => + runXdotool( + opts.display, + buildDoubleClickArgs({ + x: opts.x, + y: opts.y, + button: opts.button, + intervalMs: opts.intervalMs, + }), + INPUT_TIMEOUT_MS + MAX_DOUBLE_CLICK_INTERVAL_MS, + ), ); } export async function typeText(opts: TypeTextOptions): Promise { - await runXdotool(opts.display, buildTypeArgs(opts.text), TYPE_TIMEOUT_MS); + await withAgentPermit(opts.sessionId, opts.env ?? process.env, () => + runXdotool(opts.display, buildTypeArgs(opts.text), TYPE_TIMEOUT_MS), + ); } export async function pressKey(opts: PressKeyOptions): Promise { - await runXdotool(opts.display, buildKeyArgs(opts.key), INPUT_TIMEOUT_MS); + await withAgentPermit(opts.sessionId, opts.env ?? process.env, () => + runXdotool(opts.display, buildKeyArgs(opts.key), INPUT_TIMEOUT_MS), + ); } diff --git a/packages/desktop-linux/src/session.ts b/packages/desktop-linux/src/session.ts index 2fd2acd..a035bd3 100644 --- a/packages/desktop-linux/src/session.ts +++ b/packages/desktop-linux/src/session.ts @@ -7,9 +7,13 @@ import { createSession, destroySessionRecord, getSession, + isHumanLeaseStale, isPidAlive, processIdentityMatches, reapDeadRunningSessions, + readHumanLease, + recordTakeoverEvidence, + releaseHumanLease, sessionsDir, stopPid, stopProcessGroupVerified, @@ -415,6 +419,56 @@ export async function stopOwnedSessionVnc( } } +/** + * Recover a session left with a writable VNC server by a takeover whose + * owner process is gone (crash) or whose heartbeat lapsed (stale TTL): stop + * the recorded writable VNC, clear its record, record a `takeover_recovered` + * evidence entry, and release the stale lease. A *live* lease is left + * untouched — this only reclaims genuinely stale state. Exported for + * `@pickforge/picklab-desktop-linux`'s `takeover.ts` (a sibling module, + * imported one-directionally from here to avoid a cycle since `takeover.ts` + * already depends on this file's VNC primitives). Assumes the caller already + * holds the session's VNC lock (`withSessionVncLock`). + */ +export async function recoverStaleTakeoverLocked( + id: string, + record: SessionRecord, + registryEnv: EnvLike, +): Promise<{ recovered: boolean }> { + const lease = await readHumanLease(id, registryEnv); + if (lease === undefined) return { recovered: false }; + if (!isHumanLeaseStale(lease)) return { recovered: false }; + + const desktop = record.desktop; + if (desktop !== undefined && desktop.vncPid !== undefined && desktop.vncViewOnly !== true) { + await stopOwnedSessionVnc(id, desktop).catch(() => {}); + await updateSession( + id, + { + desktop: { + ...desktop, + vncPid: undefined, + vncStartTimeTicks: undefined, + vncViewOnly: undefined, + }, + }, + registryEnv, + ).catch(() => {}); + } + + await recordTakeoverEvidence(record.projectDir, id, "takeover_recovered", { + env: registryEnv, + status: "error", + }); + + // Compare-and-delete by leaseId. A `false` result means a concurrent + // acquirer already replaced this lease (or released it themselves) between + // our read above and here — safe to leave alone either way; the VNC side + // effect above is idempotent and already reclaimed the stale writable VNC. + await releaseHumanLease(id, lease.leaseId, registryEnv); + return { recovered: true }; +} + export async function ensureSessionVnc( id: string, opts: EnsureSessionVncOptions = {}, @@ -424,11 +478,11 @@ export async function ensureSessionVnc( throw new Error(`Session not found: ${id}`); } return withSessionVncLock(id, registryEnv, async () => { - const record = await getSession(id, registryEnv); + let record = await getSession(id, registryEnv); if (record === undefined) { throw new Error(`Session not found: ${id}`); } - const desktop = record.desktop; + let desktop = record.desktop; if (desktop?.display === undefined) { throw new Error(`Session ${id} is not desktop-capable`); } @@ -452,16 +506,32 @@ export async function ensureSessionVnc( ); } if (desktop.vncViewOnly !== true) { - throw new Error( - `Session ${id} has an active writable VNC server; watch requires server-enforced read-only VNC`, - ); - } - if (desktop.vncPort === undefined) { - throw new Error( - `Session ${id} has an active VNC server with no port recorded`, - ); + // A writable VNC left running by a takeover whose owner is dead or + // whose heartbeat lapsed is recoverable: revert it to read-only and + // fall through to the normal ensure flow below. A *live* human lease + // is never touched — `recovered: false` keeps the original refusal. + const { recovered } = await recoverStaleTakeoverLocked(id, record, registryEnv); + if (!recovered) { + throw new Error( + `Session ${id} has an active writable VNC server; watch requires server-enforced read-only VNC`, + ); + } + record = await getSession(id, registryEnv); + if (record === undefined) { + throw new Error(`Session not found: ${id}`); + } + desktop = record.desktop; + if (desktop?.display === undefined) { + throw new Error(`Session ${id} is not desktop-capable`); + } + } else { + if (desktop.vncPort === undefined) { + throw new Error( + `Session ${id} has an active VNC server with no port recorded`, + ); + } + return { pid: desktop.vncPid, port: desktop.vncPort, reused: true }; } - return { pid: desktop.vncPid, port: desktop.vncPort, reused: true }; } const vnc = await startVnc({ diff --git a/packages/desktop-linux/src/takeover.ts b/packages/desktop-linux/src/takeover.ts new file mode 100644 index 0000000..564a495 --- /dev/null +++ b/packages/desktop-linux/src/takeover.ts @@ -0,0 +1,320 @@ +import crypto from "node:crypto"; +import path from "node:path"; +import { + acquireHumanLease, + appendAction, + beginEvidenceRun, + isEvidenceEnabled, + getSession, + loadConfig, + recordTakeoverEvidence, + releaseHumanLease, + renewHumanLease, + updateSession, + StaleHumanLeaseError, + type DesktopSessionInfo, + type EnvLike, + type HumanLease, + type SessionRecord, +} from "@pickforge/picklab-core"; +import { screenshot } from "./screenshot.js"; +import { + desktopSessionLogDir, + recoverStaleTakeoverLocked, + stopOwnedSessionVnc, + withSessionVncLock, +} from "./session.js"; +import { startVnc, type VncHandle } from "./vnc.js"; + +/** + * Desktop-linux orchestration for supervised pause / human takeover + * (pickforge/picklab#21): switches x11vnc between read-only and writable + * scoped to the human lease's lifetime, and recovers a session left with a + * writable VNC server after a crashed takeover. Lease/permit bookkeeping + * itself lives in `@pickforge/picklab-core` (`takeover.ts`), which this + * module wraps with VNC-mode and evidence side effects. + */ + +export type TakeoverEndReason = "return" | "timeout" | "cancelled"; + +export interface HumanTakeoverHandle { + sessionId: string; + leaseId: string; + display: string; + vncPid: number; + vncPort: number; + vncStartTimeTicks: number; + ttlMs: number; + heartbeatMs: number; + projectDir: string; +} + +export interface StartHumanTakeoverOptions { + registryEnv?: EnvLike; + env?: EnvLike; + ttlMs?: number; + heartbeatMs?: number; + drainTimeoutMs?: number; +} + +async function requireRunningDesktopSession( + id: string, + registryEnv: EnvLike, +): Promise<{ record: SessionRecord; desktop: DesktopSessionInfo }> { + const record = await getSession(id, registryEnv); + if (record === undefined) { + throw new Error(`Session not found: ${id}`); + } + if (record.desktop === undefined) { + throw new Error(`Session ${id} is not desktop-capable`); + } + if (record.status !== "running") { + throw new Error(`Session ${id} is not running`); + } + return { record, desktop: record.desktop }; +} + +async function evidenceEnabledForProject( + projectDir: string, + env: EnvLike | undefined, +): Promise { + try { + return isEvidenceEnabled(await loadConfig(projectDir, env)); + } catch { + return false; + } +} + +/** + * Public entry point for stale-lease recovery outside an already-locked VNC + * operation (e.g. `picklab takeover status`, or a standalone health check). + */ +export async function recoverStaleHumanLease( + id: string, + registryEnv: EnvLike = process.env, +): Promise<{ recovered: boolean }> { + return withSessionVncLock(id, registryEnv, async () => { + const record = await getSession(id, registryEnv); + if (record === undefined) return { recovered: false }; + return recoverStaleTakeoverLocked(id, record, registryEnv); + }); +} + +/** + * Acquire human control of a desktop-capable session: acquire the lease + * (self-healing a stale predecessor once), switch its VNC server to + * writable, and record the transition. Throws `HumanLeaseHeldError` if + * another live human lease already holds the session. + */ +export async function startHumanTakeover( + id: string, + opts: StartHumanTakeoverOptions = {}, +): Promise { + const registryEnv = opts.registryEnv ?? process.env; + return withSessionVncLock(id, registryEnv, async () => { + const { record, desktop } = await requireRunningDesktopSession(id, registryEnv); + + let lease: HumanLease; + try { + lease = await acquireHumanLease(id, registryEnv, { + ttlMs: opts.ttlMs, + heartbeatMs: opts.heartbeatMs, + drainTimeoutMs: opts.drainTimeoutMs, + }); + } catch (error) { + if (!(error instanceof StaleHumanLeaseError)) throw error; + await recoverStaleTakeoverLocked(id, record, registryEnv); + lease = await acquireHumanLease(id, registryEnv, { + ttlMs: opts.ttlMs, + heartbeatMs: opts.heartbeatMs, + drainTimeoutMs: opts.drainTimeoutMs, + }); + } + + let vnc: VncHandle; + try { + if (desktop.vncPid !== undefined && desktop.vncViewOnly === true) { + await stopOwnedSessionVnc(id, desktop); + } + vnc = await startVnc({ + display: desktop.display, + port: desktop.vncPort, + logDir: desktopSessionLogDir(id, registryEnv), + env: opts.env, + viewOnly: false, + }); + } catch (error) { + await releaseHumanLease(id, lease.leaseId, registryEnv).catch(() => {}); + throw error; + } + + try { + await updateSession( + id, + { + desktop: { + ...desktop, + vncPid: vnc.pid, + vncStartTimeTicks: vnc.startTimeTicks, + vncPort: vnc.port, + vncViewOnly: false, + }, + }, + registryEnv, + ); + await renewHumanLease(id, lease.leaseId, registryEnv, { + vncPid: vnc.pid, + vncStartTimeTicks: vnc.startTimeTicks, + vncPort: vnc.port, + }); + } catch (error) { + await stopOwnedSessionVnc(id, { + ...desktop, + vncPid: vnc.pid, + vncStartTimeTicks: vnc.startTimeTicks, + }).catch(() => {}); + await releaseHumanLease(id, lease.leaseId, registryEnv).catch(() => {}); + throw error; + } + + await recordTakeoverEvidence(record.projectDir, id, "takeover_start", { + env: registryEnv, + }); + + return { + sessionId: id, + leaseId: lease.leaseId, + display: desktop.display, + vncPid: vnc.pid, + vncPort: vnc.port, + vncStartTimeTicks: vnc.startTimeTicks, + ttlMs: lease.ttlMs, + heartbeatMs: lease.heartbeatMs, + projectDir: record.projectDir, + }; + }); +} + +/** + * Renew a held lease's TTL. Returns `false` (never throws) if the lease is + * no longer ours — the caller must then end the takeover with reason + * `"timeout"` rather than keep driving a writable VNC past its lease. + */ +export async function renewHumanTakeover( + handle: HumanTakeoverHandle, + registryEnv: EnvLike = process.env, +): Promise { + const renewed = await renewHumanLease(handle.sessionId, handle.leaseId, registryEnv).catch( + () => undefined, + ); + return renewed !== undefined; +} + +export interface EndHumanTakeoverOptions { + registryEnv?: EnvLike; + env?: EnvLike; + reason: TakeoverEndReason; +} + +export interface EndHumanTakeoverResult { + screenshotPath?: string; + reason: TakeoverEndReason; +} + +/** + * Revert a session's VNC to read-only, capture a fresh screenshot into + * evidence as the agent's resume state, record the transition, and release + * the lease. Safe to call on any exit path (normal return, cancellation, or + * a failed heartbeat); every step is best-effort past the VNC revert so a + * failure recording evidence never leaves writable VNC or the lease behind. + */ +export async function endHumanTakeover( + handle: HumanTakeoverHandle, + opts: EndHumanTakeoverOptions, +): Promise { + const registryEnv = opts.registryEnv ?? process.env; + return withSessionVncLock(handle.sessionId, registryEnv, async () => { + const record = await getSession(handle.sessionId, registryEnv); + const desktop = record?.desktop; + + if (desktop !== undefined && desktop.vncPid === handle.vncPid) { + await stopOwnedSessionVnc(handle.sessionId, desktop).catch(() => {}); + try { + const readOnly = await startVnc({ + display: handle.display, + port: handle.vncPort, + logDir: desktopSessionLogDir(handle.sessionId, registryEnv), + env: opts.env, + viewOnly: true, + }); + await updateSession( + handle.sessionId, + { + desktop: { + ...desktop, + vncPid: readOnly.pid, + vncStartTimeTicks: readOnly.startTimeTicks, + vncPort: readOnly.port, + vncViewOnly: true, + }, + }, + registryEnv, + ); + } catch { + await updateSession( + handle.sessionId, + { + desktop: { + ...desktop, + vncPid: undefined, + vncStartTimeTicks: undefined, + vncViewOnly: undefined, + }, + }, + registryEnv, + ).catch(() => {}); + } + } + + // The lifecycle-transition evidence entry is recorded regardless of + // whether the fresh-state screenshot can be captured (e.g. no screenshot + // tool on PATH) — a missing screenshot must never silently drop the + // transition record itself. + let screenshotPath: string | undefined; + if (record !== undefined && (await evidenceEnabledForProject(record.projectDir, opts.env))) { + try { + const { run } = await beginEvidenceRun( + record.projectDir, + handle.sessionId, + { slug: "computer-use" }, + opts.env, + ); + try { + const actionId = crypto.randomUUID(); + const outPath = path.join(run.dir, "screenshots", `${actionId}.png`); + await screenshot({ display: handle.display, outPath, env: opts.env }); + screenshotPath = path.join("screenshots", `${actionId}.png`); + } catch { + // Best-effort: the transition is still recorded without a screenshot. + } + await appendAction(run.dir, { + actionId: crypto.randomUUID(), + source: "takeover", + tool: `takeover_${opts.reason}`, + sessionId: handle.sessionId, + startedAt: new Date().toISOString(), + status: "ok", + ...(screenshotPath === undefined ? {} : { artifacts: [screenshotPath] }), + }); + } catch { + // Evidence failure must never block releasing the lease. + } + } + + await releaseHumanLease(handle.sessionId, handle.leaseId, registryEnv).catch( + () => {}, + ); + + return { screenshotPath, reason: opts.reason }; + }); +} diff --git a/packages/desktop-linux/test/integration.test.ts b/packages/desktop-linux/test/integration.test.ts index ca3e567..fbcdba3 100644 --- a/packages/desktop-linux/test/integration.test.ts +++ b/packages/desktop-linux/test/integration.test.ts @@ -582,20 +582,22 @@ describe.skipIf(!hasDesktopStack)("desktop integration (Xvfb + xdotool)", () => expect(win.id).toMatch(/^\d+$/); expect(win.name).toContain("picklab-itest"); - await click({ display: session.display, x: 40, y: 40 }); - await typeText({ display: session.display, text: "echo picklab" }); - await pressKey({ display: session.display, key: "Return" }); - await pressKey({ display: session.display, key: "ctrl+shift+t" }); + await click({ display: session.display, sessionId: session.id, env, x: 40, y: 40 }); + await typeText({ display: session.display, sessionId: session.id, env, text: "echo picklab" }); + await pressKey({ display: session.display, sessionId: session.id, env, key: "Return" }); + await pressKey({ display: session.display, sessionId: session.id, env, key: "ctrl+shift+t" }); - await move({ display: session.display, x: 120, y: 90 }); + await move({ display: session.display, sessionId: session.id, env, x: 120, y: 90 }); expect(await pointerLocation(session.display)).toEqual({ x: 120, y: 90, }); - await scroll({ display: session.display, deltaX: 0, deltaY: 2 }); + await scroll({ display: session.display, sessionId: session.id, env, deltaX: 0, deltaY: 2 }); await scroll({ display: session.display, + sessionId: session.id, + env, deltaX: -1, deltaY: -1, x: 60, @@ -608,6 +610,8 @@ describe.skipIf(!hasDesktopStack)("desktop integration (Xvfb + xdotool)", () => await drag({ display: session.display, + sessionId: session.id, + env, fromX: 30, fromY: 30, toX: 150, @@ -619,7 +623,7 @@ describe.skipIf(!hasDesktopStack)("desktop integration (Xvfb + xdotool)", () => y: 110, }); - await doubleClick({ display: session.display, x: 45, y: 45 }); + await doubleClick({ display: session.display, sessionId: session.id, env, x: 45, y: 45 }); expect(await pointerLocation(session.display)).toEqual({ x: 45, y: 45, diff --git a/packages/desktop-linux/test/takeover.test.ts b/packages/desktop-linux/test/takeover.test.ts new file mode 100644 index 0000000..5d837b4 --- /dev/null +++ b/packages/desktop-linux/test/takeover.test.ts @@ -0,0 +1,329 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +// `startVnc` requires a verified `/proc`-backed process identity for the +// spawned x11vnc daemon, which does not exist on Darwin. These tests prove +// the takeover orchestration (lease acquisition, VNC mode switching, +// evidence, recovery) end-to-end against *real* spawned fake-x11vnc +// processes and real port listening, with only the two `/proc`-dependent +// identity functions replaced by a deterministic, pid-keyed stand-in — the +// same technique `destroy.test.ts` uses. Real x11vnc/Xvfb hardware +// validation is deferred (see AGENTS.md / release notes). +vi.mock("@pickforge/picklab-core", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + readProcessIdentity: vi.fn((pid: number) => ({ pid, startTicks: pid })), + processIdentityMatches: vi.fn( + ({ pid, startTicks }: { pid: number; startTicks: number }) => startTicks === pid, + ), + }; +}); + +import { + HumanLeaseHeldError, + createSession, + getSession, + isPidAlive, + readActions, + readHumanLease, + resolveActivePointer, + resolveRunStorage, + stopPid, + type EnvLike, +} from "@pickforge/picklab-core"; +import { + endHumanTakeover, + recoverStaleHumanLease, + renewHumanTakeover, + startHumanTakeover, +} from "../src/takeover.js"; + +let root: string; +let binDir: string; +let env: EnvLike; +let argvLogPath: string; +// Monotonic across the whole file (never reset per test) so a test that fails +// before releasing its port never collides with the next test's port. +let syntheticPort = 15_900; + +function nextPort(): number { + syntheticPort += 1; + return syntheticPort; +} + +async function installFakeVnc(): Promise { + const script = path.join(binDir, "x11vnc"); + const source = [ + "const net = require('node:net');", + "const fs = require('node:fs');", + "const args = process.argv.slice(2);", + "fs.appendFileSync(process.env.ARGV_LOG, JSON.stringify(args) + '\\n');", + "const port = Number(args[args.indexOf('-rfbport') + 1]);", + "const server = net.createServer((socket) => socket.end());", + "server.listen(port, '127.0.0.1');", + "process.on('SIGTERM', () => server.close(() => process.exit(0)));", + ].join("\n"); + await fs.promises.writeFile(script, `#!${process.execPath}\n${source}`, "utf8"); + await fs.promises.chmod(script, 0o755); +} + +async function readArgvLog(): Promise { + const raw = await fs.promises.readFile(argvLogPath, "utf8").catch(() => ""); + return raw + .split("\n") + .filter((line) => line.trim() !== "") + .map((line) => JSON.parse(line) as string[]); +} + +async function createDesktop(desktop: Record = {}): Promise { + const record = await createSession( + { + type: "desktop", + projectDir: root, + status: "running", + desktop: { display: ":42", ...desktop }, + }, + env, + ); + return record.id; +} + +beforeEach(async () => { + root = await fs.promises.mkdtemp(path.join(os.tmpdir(), "picklab-takeover-dl-")); + binDir = path.join(root, "bin"); + await fs.promises.mkdir(binDir, { recursive: true }); + argvLogPath = path.join(root, "argv.log"); + env = { + ...process.env, + PICKLAB_HOME: path.join(root, "home"), + PATH: binDir, + ARGV_LOG: argvLogPath, + }; + await installFakeVnc(); +}); + +afterEach(async () => { + vi.clearAllMocks(); + // Force-kill any fake x11vnc process a failed assertion left running, so a + // failure in one test never blocks a later test's port. + const sessionsDir = path.join(env.PICKLAB_HOME as string, "sessions"); + const entries = await fs.promises.readdir(sessionsDir).catch(() => [] as string[]); + for (const entry of entries) { + if (!entry.endsWith(".json")) continue; + const id = entry.slice(0, -".json".length); + const record = await getSession(id, env).catch(() => undefined); + const pid = record?.desktop?.vncPid; + if (pid !== undefined && isPidAlive(pid)) { + await stopPid(pid, { timeoutMs: 500 }).catch(() => {}); + } + } + await fs.promises.rm(root, { recursive: true, force: true }); +}); + +describe("startHumanTakeover / endHumanTakeover", () => { + it("switches VNC writable on start and back to read-only on return, releasing the lease", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + + const handle = await startHumanTakeover(id, { + registryEnv: env, + env, + drainTimeoutMs: 500, + }); + expect(handle.sessionId).toBe(id); + expect(isPidAlive(handle.vncPid)).toBe(true); + expect((await readHumanLease(id, env))?.leaseId).toBe(handle.leaseId); + + const afterStart = await getSession(id, env); + expect(afterStart?.desktop?.vncViewOnly).toBe(false); + expect(afterStart?.desktop?.vncPid).toBe(handle.vncPid); + + const startArgv = await readArgvLog(); + expect(startArgv).toHaveLength(1); + expect(startArgv[0]).not.toContain("-viewonly"); + + const startedVncPid = handle.vncPid; + const result = await endHumanTakeover(handle, { + registryEnv: env, + env, + reason: "return", + }); + expect(result.reason).toBe("return"); + expect(isPidAlive(startedVncPid)).toBe(false); + expect(await readHumanLease(id, env)).toBeUndefined(); + + const afterEnd = await getSession(id, env); + expect(afterEnd?.desktop?.vncViewOnly).toBe(true); + expect(afterEnd?.desktop?.vncPid).not.toBe(startedVncPid); + + const fullArgv = await readArgvLog(); + expect(fullArgv).toHaveLength(2); + expect(fullArgv[1]).toContain("-viewonly"); + }); + + it("records a takeover_start and takeover_ evidence transition", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + const handle = await startHumanTakeover(id, { registryEnv: env, env }); + await endHumanTakeover(handle, { registryEnv: env, env, reason: "cancelled" }); + + const pointerless = await resolveActivePointer(root, id, env); + // The evidence run was finalized by nothing yet (no destroySessionRecord + // call), so it is still resolvable as the session's active run. + expect(pointerless.status === "active" || pointerless.status === "stale").toBe( + true, + ); + const runId = + pointerless.status === "active" ? pointerless.pointer.runId : undefined; + if (runId !== undefined) { + const { runsDir } = await resolveRunStorage(root, env); + const actions = await readActions(path.join(runsDir, runId)); + const tools = actions.map((a) => (a as { tool?: string }).tool); + expect(tools).toContain("takeover_start"); + expect(tools).toContain("takeover_cancelled"); + } + }); + + it("refuses a second takeover while the first is live", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + const handle = await startHumanTakeover(id, { registryEnv: env, env }); + await expect( + startHumanTakeover(id, { registryEnv: env, env }), + ).rejects.toThrow(HumanLeaseHeldError); + await endHumanTakeover(handle, { registryEnv: env, env, reason: "return" }); + }); + + it("renews the lease TTL while control is held", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + const handle = await startHumanTakeover(id, { registryEnv: env, env }); + const before = await readHumanLease(id, env); + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(await renewHumanTakeover(handle, env)).toBe(true); + const after = await readHumanLease(id, env); + expect(Date.parse(after!.expiresAt)).toBeGreaterThanOrEqual( + Date.parse(before!.expiresAt), + ); + await endHumanTakeover(handle, { registryEnv: env, env, reason: "timeout" }); + }); + + it("reports renewal failure once the lease is gone (post-timeout)", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + const handle = await startHumanTakeover(id, { registryEnv: env, env }); + await endHumanTakeover(handle, { registryEnv: env, env, reason: "timeout" }); + expect(await renewHumanTakeover(handle, env)).toBe(false); + }); +}); + +describe("recoverStaleHumanLease (crash recovery)", () => { + it("stops an orphaned writable VNC, clears the record, and releases a stale lease", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + const handle = await startHumanTakeover(id, { registryEnv: env, env }); + const vncPid = handle.vncPid; + expect(isPidAlive(vncPid)).toBe(true); + + // Simulate a crash: the owner process is gone and the TTL has lapsed, + // but nobody ran the graceful `endHumanTakeover` cleanup. + const crashed = { + ...(await readHumanLease(id, env))!, + ownerPid: 999_999, + expiresAt: new Date(Date.now() - 1_000).toISOString(), + }; + await fs.promises.writeFile( + path.join(env.PICKLAB_HOME as string, "sessions", id, "human.lease.json"), + `${JSON.stringify(crashed)}\n`, + ); + + const { recovered } = await recoverStaleHumanLease(id, env); + expect(recovered).toBe(true); + expect(isPidAlive(vncPid)).toBe(false); + expect(await readHumanLease(id, env)).toBeUndefined(); + const record = await getSession(id, env); + expect(record?.desktop?.vncPid).toBeUndefined(); + expect(record?.desktop?.vncViewOnly).toBeUndefined(); + }); + + it("leaves a live lease and its writable VNC untouched", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + const handle = await startHumanTakeover(id, { registryEnv: env, env }); + + const { recovered } = await recoverStaleHumanLease(id, env); + expect(recovered).toBe(false); + expect(isPidAlive(handle.vncPid)).toBe(true); + expect((await readHumanLease(id, env))?.leaseId).toBe(handle.leaseId); + + await endHumanTakeover(handle, { registryEnv: env, env, reason: "return" }); + }); + + it("is a no-op when there is no lease at all", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + const { recovered } = await recoverStaleHumanLease(id, env); + expect(recovered).toBe(false); + }); +}); + +describe("startHumanTakeover self-healing", () => { + it("recovers a stale lease left by a crashed takeover, then proceeds", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + const firstHandle = await startHumanTakeover(id, { registryEnv: env, env }); + const staleVncPid = firstHandle.vncPid; + + const crashed = { + ...(await readHumanLease(id, env))!, + ownerPid: 999_999, + expiresAt: new Date(Date.now() - 1_000).toISOString(), + }; + await fs.promises.writeFile( + path.join(env.PICKLAB_HOME as string, "sessions", id, "human.lease.json"), + `${JSON.stringify(crashed)}\n`, + ); + + const secondHandle = await startHumanTakeover(id, { + registryEnv: env, + env, + drainTimeoutMs: 500, + }); + expect(secondHandle.leaseId).not.toBe(firstHandle.leaseId); + expect(isPidAlive(staleVncPid)).toBe(false); + expect(isPidAlive(secondHandle.vncPid)).toBe(true); + + await endHumanTakeover(secondHandle, { registryEnv: env, env, reason: "return" }); + }); +}); + +describe("ensureSessionVnc recovery integration", () => { + it("recovers a crash-orphaned writable VNC instead of refusing to watch", async () => { + const { ensureSessionVnc } = await import("../src/session.js"); + const id = await createDesktop({ vncPort: nextPort() }); + const handle = await startHumanTakeover(id, { registryEnv: env, env }); + const staleVncPid = handle.vncPid; + + const crashed = { + ...(await readHumanLease(id, env))!, + ownerPid: 999_999, + expiresAt: new Date(Date.now() - 1_000).toISOString(), + }; + await fs.promises.writeFile( + path.join(env.PICKLAB_HOME as string, "sessions", id, "human.lease.json"), + `${JSON.stringify(crashed)}\n`, + ); + + const ensured = await ensureSessionVnc(id, { registryEnv: env, env }); + expect(ensured.reused).toBe(false); + expect(isPidAlive(staleVncPid)).toBe(false); + const record = await getSession(id, env); + expect(record?.desktop?.vncViewOnly).toBe(true); + }); + + it("still refuses to watch while a live human lease holds writable VNC", async () => { + const { ensureSessionVnc } = await import("../src/session.js"); + const id = await createDesktop({ vncPort: nextPort() }); + const handle = await startHumanTakeover(id, { registryEnv: env, env }); + + await expect(ensureSessionVnc(id, { registryEnv: env, env })).rejects.toThrow( + /server-enforced read-only VNC/, + ); + + await endHumanTakeover(handle, { registryEnv: env, env, reason: "return" }); + }); +}); diff --git a/packages/mcp-server/src/index.ts b/packages/mcp-server/src/index.ts index a56775e..6edfff7 100644 --- a/packages/mcp-server/src/index.ts +++ b/packages/mcp-server/src/index.ts @@ -7,6 +7,7 @@ import { registerAndroidTools } from "./tools/android.js"; import { registerArtifactTools } from "./tools/artifacts.js"; import { registerDesktopTools } from "./tools/desktop.js"; import { registerSessionTools } from "./tools/session.js"; +import { registerTakeoverTools } from "./tools/takeover.js"; import { registerUserTools } from "./tools/user.js"; const require = createRequire(import.meta.url); @@ -25,6 +26,7 @@ export function createMcpServer( registerDesktopTools(server, ctx); registerAndroidTools(server, ctx); registerArtifactTools(server, ctx); + registerTakeoverTools(server, ctx); registerUserTools(server); registerResources(server, ctx); registerPrompts(server); diff --git a/packages/mcp-server/src/tools/desktop.ts b/packages/mcp-server/src/tools/desktop.ts index 1600187..c9074d8 100644 --- a/packages/mcp-server/src/tools/desktop.ts +++ b/packages/mcp-server/src/tools/desktop.ts @@ -221,6 +221,8 @@ export function registerDesktopTools( async () => { await click({ display, + sessionId: id, + env: ctx.env, x: args.x, y: args.y, button: args.button, @@ -263,7 +265,7 @@ export function registerDesktopTools( target: { x: args.x, y: args.y }, }, async () => { - await move({ display, x: args.x, y: args.y }); + await move({ display, sessionId: id, env: ctx.env, x: args.x, y: args.y }); return { data: { sessionId: id, display, x: args.x, y: args.y }, }; @@ -318,6 +320,8 @@ export function registerDesktopTools( async () => { await scroll({ display, + sessionId: id, + env: ctx.env, deltaX: args.deltaX, deltaY: args.deltaY, x: args.x, @@ -375,6 +379,8 @@ export function registerDesktopTools( async () => { await drag({ display, + sessionId: id, + env: ctx.env, fromX: args.fromX, fromY: args.fromY, toX: args.toX, @@ -430,6 +436,8 @@ export function registerDesktopTools( async () => { await doubleClick({ display, + sessionId: id, + env: ctx.env, x: args.x, y: args.y, button: args.button, @@ -470,7 +478,7 @@ export function registerDesktopTools( typedValue: { value: args.text, inputType: "text" }, }, async () => { - await typeText({ display, text: args.text }); + await typeText({ display, sessionId: id, env: ctx.env, text: args.text }); return { data: { sessionId: id, display, length: args.text.length }, }; @@ -502,7 +510,7 @@ export function registerDesktopTools( typedValue: { value: args.key }, }, async () => { - await pressKey({ display, key: args.key }); + await pressKey({ display, sessionId: id, env: ctx.env, key: args.key }); return { data: { sessionId: id, display, key: args.key } }; }, ); diff --git a/packages/mcp-server/src/tools/takeover.ts b/packages/mcp-server/src/tools/takeover.ts new file mode 100644 index 0000000..32fd663 --- /dev/null +++ b/packages/mcp-server/src/tools/takeover.ts @@ -0,0 +1,47 @@ +import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { z } from "zod"; +import { getTakeoverStatus } from "@pickforge/picklab-core"; +import { resolveSessionRecord, runTool, type ServerContext } from "../context.js"; + +const sessionArg = { + session: z + .string() + .min(1) + .optional() + .describe("Desktop-capable session id (default: the single running session)"), +}; + +export function registerTakeoverTools(server: McpServer, ctx: ServerContext): void { + server.registerTool( + "takeover_status", + { + title: "Human takeover status", + description: + "Check whether a session is currently under supervised human control " + + "(pickforge/picklab#21). While active, desktop input and the DevTools " + + "relay fail closed with a busy error — call this before retrying, or " + + "use `request_user_input` to ask the human to finish and return " + + "control via `picklab watch --control`.", + inputSchema: { ...sessionArg }, + }, + (args) => + runTool(async () => { + const record = await resolveSessionRecord(ctx, "desktop", args.session); + const status = await getTakeoverStatus(record.id, ctx.env); + const data: Record = { + sessionId: record.id, + active: status.active, + }; + if (status.stale === true) data.stale = true; + if (status.lease !== undefined) { + data.lease = { + leaseId: status.lease.leaseId, + ownerPid: status.lease.ownerPid, + createdAt: status.lease.createdAt, + expiresAt: status.lease.expiresAt, + }; + } + return { data }; + }), + ); +} diff --git a/packages/mcp-server/src/tools/user.ts b/packages/mcp-server/src/tools/user.ts index 43fbefd..88a8a3f 100644 --- a/packages/mcp-server/src/tools/user.ts +++ b/packages/mcp-server/src/tools/user.ts @@ -9,10 +9,13 @@ const SECRET_QUESTION_PATTERN = const SECRET_GUIDANCE = "This looks like a request for a secret (password, API key, token, 2FA " + "code, or other credential). Never collect secrets through this tool. " + - "Ask the user to enter the secret directly into the lab app through an " + - "explicit writable VNC control session (`--vnc-control`), or into the environment, then " + - "confirm out-of-band with kind " + - '"confirm" (e.g. "I\'ve entered the password, continue?").'; + "Ask the user to run `picklab watch --control` to take temporary " + + "supervised control of the desktop over a writable VNC session, enter " + + "the secret themselves, and return control (or into the environment), " + + 'then confirm out-of-band with kind "confirm" (e.g. "I\'ve entered the ' + + 'password, continue?"). While control is held, agent desktop input and ' + + "the DevTools relay fail closed with a busy error; call " + + "`takeover_status` to check."; const NO_ELICITATION_GUIDANCE = "This client does not support elicitation. Relay the question to the " + diff --git a/packages/mcp-server/test/takeover.test.ts b/packages/mcp-server/test/takeover.test.ts new file mode 100644 index 0000000..5374865 --- /dev/null +++ b/packages/mcp-server/test/takeover.test.ts @@ -0,0 +1,112 @@ +import fs from "node:fs"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { acquireHumanLease, releaseHumanLease } from "@pickforge/picklab-core"; +import { + connectLab, + makeLabDirs, + parseToolJson, + writeDesktopSessionRecord, + type ConnectedLab, + type LabDirs, +} from "./helpers.js"; + +let dirs: LabDirs; +let lab: ConnectedLab; + +afterEach(async () => { + await lab?.close(); + if (dirs !== undefined) { + fs.rmSync(dirs.root, { recursive: true, force: true }); + } +}); + +describe("takeover_status", () => { + it("reports agent-active, then human-active, then stale", async () => { + dirs = makeLabDirs(); + const env = { PICKLAB_HOME: dirs.home }; + lab = await connectLab({ projectDir: dirs.projectDir, env }); + const id = writeDesktopSessionRecord(dirs.home, dirs.projectDir); + + const idle = parseToolJson( + await lab.client.callTool({ name: "takeover_status", arguments: { session: id } }), + ); + expect(idle.ok).toBe(true); + expect(idle.active).toBe(false); + expect(idle.lease).toBeUndefined(); + + const lease = await acquireHumanLease(id, env); + const active = parseToolJson( + await lab.client.callTool({ name: "takeover_status", arguments: { session: id } }), + ); + expect(active.active).toBe(true); + expect(active.lease).toMatchObject({ leaseId: lease.leaseId, ownerPid: lease.ownerPid }); + + await releaseHumanLease(id, lease.leaseId, env); + const stalePath = path.join(dirs.home, "sessions", id, "human.lease.json"); + fs.mkdirSync(path.dirname(stalePath), { recursive: true }); + fs.writeFileSync( + stalePath, + `${JSON.stringify({ + leaseId: "dead", + sessionId: id, + ownerPid: 999_999, + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 60_000).toISOString(), + ttlMs: 30_000, + heartbeatMs: 5_000, + })}\n`, + ); + const stale = parseToolJson( + await lab.client.callTool({ name: "takeover_status", arguments: { session: id } }), + ); + expect(stale.active).toBe(false); + expect(stale.stale).toBe(true); + }); +}); + +describe("desktop input tools fail closed under human control", () => { + it("rejects desktop_click with a stable busy error while a lease is held, without touching xdotool", async () => { + dirs = makeLabDirs(); + const env = { PICKLAB_HOME: dirs.home }; + lab = await connectLab({ projectDir: dirs.projectDir, env }); + const id = writeDesktopSessionRecord(dirs.home, dirs.projectDir); + + const lease = await acquireHumanLease(id, env); + const result = parseToolJson( + await lab.client.callTool({ + name: "desktop_click", + arguments: { session: id, x: 1, y: 1 }, + }), + ); + expect(result.ok).toBe(false); + expect(result.errors.join("\n")).toContain("human control is active"); + await releaseHumanLease(id, lease.leaseId, env); + }); + + it("rejects desktop_type and desktop_key the same way", async () => { + dirs = makeLabDirs(); + const env = { PICKLAB_HOME: dirs.home }; + lab = await connectLab({ projectDir: dirs.projectDir, env }); + const id = writeDesktopSessionRecord(dirs.home, dirs.projectDir); + await acquireHumanLease(id, env); + + const typed = parseToolJson( + await lab.client.callTool({ + name: "desktop_type", + arguments: { session: id, text: "hello" }, + }), + ); + expect(typed.ok).toBe(false); + expect(typed.errors.join("\n")).toContain("human control is active"); + + const keyed = parseToolJson( + await lab.client.callTool({ + name: "desktop_key", + arguments: { session: id, key: "Return" }, + }), + ); + expect(keyed.ok).toBe(false); + expect(keyed.errors.join("\n")).toContain("human control is active"); + }); +}); diff --git a/packages/mcp-server/test/tools.test.ts b/packages/mcp-server/test/tools.test.ts index d5a4413..d8b42b5 100644 --- a/packages/mcp-server/test/tools.test.ts +++ b/packages/mcp-server/test/tools.test.ts @@ -42,6 +42,7 @@ const EXPECTED_TOOLS = [ "android_run_adb", "artifact_list", "artifact_report", + "takeover_status", "request_user_input", ]; From 7aa45795b6a8aeae6d291a1e87d931f500406256 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 11:02:20 -0300 Subject: [PATCH 2/5] fix(core,desktop-linux,cli): close lease races and actively reclaim writable VNC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel-adjudicated fixes to the human-takeover invariant (writable VNC must never outlive its lease), addressing two P0 violations and a P1 TOCTOU: - renewHumanLease refuses to extend a lease that has already gone stale by TTL, even for its own live owner, so a straggling renewal can never resurrect a lease a recovery is already reclaiming (P0-B). - recoverStaleTakeoverLocked re-reads and re-checks staleness immediately before the destructive VNC stop, and releases via compare-and-delete on the exact raw bytes captured at that check (not just leaseId, which a renewal never changes) — closing the window where an unlocked renewal between the initial check and the stop could make recovery kill a now-live takeover's VNC (P1-C). - watch --control now holds three independent mechanisms so writable VNC cannot outlive its lease in wall-clock terms, not only "the next time something happens to touch the session": it ends the takeover immediately on the first failed heartbeat renewal rather than waiting for the viewer to close; a hard deadline timer, rescheduled on every successful renewal, force-ends it if wall-clock time ever passes expiresAt; and a detached watchdog process (spawned via a new hidden `picklab internal takeover-watchdog` command, immune to a SIGKILL of its parent) independently polls the lease and reclaims a stale writable VNC on its own — proven against a genuinely separate OS process, not just an in-process mock (P0-A). A detached process was chosen over OS-level parent-death coupling (e.g. Linux PR_SET_PDEATHSIG) because the latter has no portable Node.js API without a native addon or an external wrapper binary. renewHumanTakeover's return type changes from boolean to the renewed lease (or undefined), needed to read the fresh expiresAt for the deadline-timer reschedule. --- packages/cli/src/commands/takeover.ts | 28 +++ packages/cli/src/commands/watch.ts | 129 +++++++++- packages/cli/src/program.ts | 19 +- packages/cli/test/watch-control.test.ts | 139 +++++++++-- packages/core/package.json | 1 + packages/core/src/index.ts | 2 + packages/core/src/takeover.ts | 32 +++ packages/core/test/takeover.test.ts | 16 ++ packages/desktop-linux/src/index.ts | 6 + packages/desktop-linux/src/session.ts | 35 ++- .../desktop-linux/src/takeover-watchdog.ts | 71 ++++++ packages/desktop-linux/src/takeover.ts | 27 +- packages/desktop-linux/test/takeover.test.ts | 236 +++++++++++++++++- .../test/workers/takeover-watchdog-worker.ts | 22 ++ 14 files changed, 717 insertions(+), 46 deletions(-) create mode 100644 packages/desktop-linux/src/takeover-watchdog.ts create mode 100644 packages/desktop-linux/test/workers/takeover-watchdog-worker.ts diff --git a/packages/cli/src/commands/takeover.ts b/packages/cli/src/commands/takeover.ts index 5e7c5fa..195bf68 100644 --- a/packages/cli/src/commands/takeover.ts +++ b/packages/cli/src/commands/takeover.ts @@ -1,5 +1,7 @@ import { getTakeoverStatus, resolveDesktopCapableSession } from "@pickforge/picklab-core"; +import { runTakeoverWatchdogLoop } from "@pickforge/picklab-desktop-linux"; import { + parseIntArg, resolveProjectDir, runReported, type BaseCliOptions, @@ -42,3 +44,29 @@ export async function takeoverStatus( export async function runTakeoverStatus(opts: TakeoverStatusOptions): Promise { return runReported(opts, () => takeoverStatus(opts)); } + +export interface TakeoverWatchdogOptions { + session: string; + lease: string; + interval?: string; +} + +/** + * Internal command, not part of the public CLI surface: the actively + * polling half of the "writable VNC never outlives its lease" crash-recovery + * path (pickforge/picklab#21 P0-A). `picklab watch --control` spawns this as + * a detached process alongside a takeover; it runs until the lease it is + * watching ends, is superseded, or goes stale (in which case it reclaims the + * writable VNC itself and exits) — see `runTakeoverWatchdogLoop`. + */ +export async function runTakeoverWatchdog( + opts: TakeoverWatchdogOptions, +): Promise { + await runTakeoverWatchdogLoop({ + sessionId: opts.session, + leaseId: opts.lease, + pollIntervalMs: + opts.interval === undefined ? undefined : parseIntArg(opts.interval, "--interval"), + }); + return 0; +} diff --git a/packages/cli/src/commands/watch.ts b/packages/cli/src/commands/watch.ts index 498c7a1..89c3ead 100644 --- a/packages/cli/src/commands/watch.ts +++ b/packages/cli/src/commands/watch.ts @@ -1,3 +1,4 @@ +import { spawn } from "node:child_process"; import { resolveDesktopCapableSession } from "@pickforge/picklab-core"; import { endHumanTakeover, @@ -19,23 +20,99 @@ export interface WatchOptions extends BaseCliOptions { session?: string; waitForViewerExit?: boolean; control?: boolean; + /** @internal test hook: override how the crash-recovery watchdog is spawned. */ + _spawnWatchdog?: SpawnWatchdogFn; } const TAKEOVER_SIGNALS = ["SIGINT", "SIGTERM"] as const; +export interface TakeoverWatchdogHandle { + /** Best-effort, idempotent: stop the watchdog now (clean end). */ + kill(): void; +} + +export type SpawnWatchdogFn = (handle: HumanTakeoverHandle) => TakeoverWatchdogHandle; + +/** + * Spawn the crash-recovery watchdog (pickforge/picklab#21 P0-A) as a + * **detached** sibling process — its own process group, `stdio: "ignore"`, + * `unref()`'d — so a `SIGKILL` of *this* process (a crash of + * `watch --control` itself) does not kill it too. It re-invokes the same + * entry point currently running (`process.argv[1]`) with the hidden + * `internal takeover-watchdog` command, which polls the lease independently + * and actively reclaims a stale writable VNC on its own; see + * `runTakeoverWatchdogLoop` for why a detached process was chosen over + * OS-level parent-death coupling. If there is no known entry point to + * re-invoke (unexpected outside a real CLI process), spawning is skipped — + * the immediate-end-on-renew-failure and hard-deadline-timer mechanisms in + * *this* process still hold the invariant; only the crash-of-this-process + * backstop is unavailable in that case. + */ +function defaultSpawnWatchdog(handle: HumanTakeoverHandle): TakeoverWatchdogHandle { + const cliEntry = process.argv[1]; + if (cliEntry === undefined) { + return { kill(): void {} }; + } + const child = spawn( + process.execPath, + [ + cliEntry, + "internal", + "takeover-watchdog", + "--session", + handle.sessionId, + "--lease", + handle.leaseId, + "--interval", + String(handle.heartbeatMs), + ], + { detached: true, stdio: "ignore" }, + ); + child.unref(); + let killed = false; + return { + kill(): void { + if (killed) return; + killed = true; + try { + child.kill("SIGTERM"); + } catch { + // Already gone. + } + }, + }; +} + /** * Hold a human lease for the duration of the viewer wait, heartbeating it - * every `heartbeatMs` so it never lapses while the human is actually there. + * every `heartbeatMs`. Three mechanisms together keep writable VNC from + * outliving the lease in wall-clock terms (pickforge/picklab#21 P0-A), + * never only "the next time something happens to touch the session": + * + * 1. The first failed renewal ends the takeover *immediately* — it does not + * wait for the viewer to close, since a renewal failure means the lease + * is already gone (see `renewHumanTakeover`/#21 P0-B). + * 2. A hard deadline timer, rescheduled to the fresh `expiresAt` on every + * successful renewal, force-ends the takeover if wall-clock time ever + * passes the lease's expiry regardless of what the heartbeat interval + * itself observed — belt-and-suspenders against e.g. a stalled interval + * callback. + * 3. A detached watchdog process (`defaultSpawnWatchdog`) covers the case + * where *this* process itself crashes and neither of the above can run + * at all. + * * A terminal-wide SIGINT (the common interactive-cancel path) reaches the * viewer child too, since `openVncViewer` spawns it in our process group — - * this handler exists so *our* process does not exit before running cleanup, - * not to itself interrupt the viewer. `runControlledViewer` always ends the - * takeover (VNC reverted, lease released) before returning or throwing. + * the signal handler here exists so *our* process does not exit before + * running cleanup, not to itself interrupt the viewer. + * `runControlledViewer` always ends the takeover (VNC reverted, lease + * released, watchdog stopped) before returning or throwing. */ async function runControlledViewer( handle: HumanTakeoverHandle, port: number, registryEnv: NodeJS.ProcessEnv, + spawnWatchdog: SpawnWatchdogFn, ): Promise<{ viewer: Awaited>; reason: TakeoverEndReason; @@ -49,16 +126,44 @@ async function runControlledViewer( for (const signal of TAKEOVER_SIGNALS) { process.on(signal, onSignal); } + + let ending: Promise<{ screenshotPath?: string }> | undefined; + const endOnce = (reason: TakeoverEndReason): Promise<{ screenshotPath?: string }> => { + if (ending === undefined) { + ending = endHumanTakeover(handle, { registryEnv, reason }); + } + return ending; + }; + + let deadlineTimer: NodeJS.Timeout | undefined; + const scheduleDeadline = (expiresAt: string): void => { + if (deadlineTimer !== undefined) clearTimeout(deadlineTimer); + const delayMs = Math.max(0, Date.parse(expiresAt) - Date.now()); + deadlineTimer = setTimeout(() => { + leaseLost = true; + void endOnce("timeout"); + }, delayMs); + deadlineTimer.unref(); + }; + scheduleDeadline(handle.expiresAt); + const heartbeat = setInterval(() => { void renewHumanTakeover(handle, registryEnv).then((renewed) => { - if (!renewed) { + if (renewed === undefined) { leaseLost = true; clearInterval(heartbeat); + // P0-A item 1: end immediately on the first failed renewal — never + // wait for the viewer to close. + void endOnce("timeout"); + } else { + scheduleDeadline(renewed.expiresAt); } }); }, handle.heartbeatMs); heartbeat.unref(); + const watchdog = spawnWatchdog(handle); + try { const viewer = await openVncViewer({ port, waitForExit: true }); const reason: TakeoverEndReason = leaseLost @@ -66,16 +171,15 @@ async function runControlledViewer( : cancelled ? "cancelled" : "return"; - const result = await endHumanTakeover(handle, { registryEnv, reason }); + const result = await endOnce(reason); return { viewer, reason, screenshotPath: result.screenshotPath }; } catch (error) { - await endHumanTakeover(handle, { - registryEnv, - reason: leaseLost ? "timeout" : "cancelled", - }).catch(() => {}); + await endOnce(leaseLost ? "timeout" : "cancelled").catch(() => {}); throw error; } finally { clearInterval(heartbeat); + if (deadlineTimer !== undefined) clearTimeout(deadlineTimer); + watchdog.kill(); for (const signal of TAKEOVER_SIGNALS) { process.off(signal, onSignal); } @@ -85,6 +189,7 @@ async function runControlledViewer( async function watchWithControl( sessionId: string, registryEnv: NodeJS.ProcessEnv, + spawnWatchdog: SpawnWatchdogFn, ): Promise { const handle = await startHumanTakeover(sessionId, { registryEnv }); const data: Record = { @@ -96,7 +201,7 @@ async function watchWithControl( let controlled: Awaited>; try { - controlled = await runControlledViewer(handle, handle.vncPort, registryEnv); + controlled = await runControlledViewer(handle, handle.vncPort, registryEnv, spawnWatchdog); } catch (error) { throw error instanceof Error ? new Error(`Human takeover for session ${sessionId} ended abnormally: ${error.message}`) @@ -145,7 +250,7 @@ export async function watchDesktopSession( "--control requires waiting for the viewer to exit, to know when to end human control", ); } - return watchWithControl(record.id, process.env); + return watchWithControl(record.id, process.env, opts._spawnWatchdog ?? defaultSpawnWatchdog); } const vnc = await ensureSessionVnc(record.id); const viewer = await openVncViewer({ diff --git a/packages/cli/src/program.ts b/packages/cli/src/program.ts index 910edad..367cebe 100644 --- a/packages/cli/src/program.ts +++ b/packages/cli/src/program.ts @@ -47,7 +47,7 @@ import { } from "./commands/session.js"; import { runSetupAndroid } from "./commands/setup-android.js"; import { runSetupLabUser } from "./commands/setup-lab-user.js"; -import { runTakeoverStatus } from "./commands/takeover.js"; +import { runTakeoverStatus, runTakeoverWatchdog } from "./commands/takeover.js"; import { runWatch } from "./commands/watch.js"; const require = createRequire(import.meta.url); @@ -236,6 +236,23 @@ export function buildProgram(): Command { process.exitCode = await runTakeoverStatus(opts); }); + // Internal, undocumented commands spawned by picklab itself — not a + // supported public CLI surface. + const internal = program.command("internal", { hidden: true }); + + internal + .command("takeover-watchdog") + .description( + "Actively reclaim a writable VNC session if its human lease goes " + + "stale; spawned detached by `watch --control`, not meant to be run directly", + ) + .requiredOption("--session ", "session id") + .requiredOption("--lease ", "lease id to watch") + .option("--interval ", "poll interval in ms") + .action(async (opts) => { + process.exitCode = await runTakeoverWatchdog(opts); + }); + const browser = program .command("browser") .description("Connect agent browser tooling to the active PickLab browser"); diff --git a/packages/cli/test/watch-control.test.ts b/packages/cli/test/watch-control.test.ts index cbcbd3f..a14a754 100644 --- a/packages/cli/test/watch-control.test.ts +++ b/packages/cli/test/watch-control.test.ts @@ -29,10 +29,16 @@ vi.mock("@pickforge/picklab-desktop-linux", async (importOriginal) => { }); import { createSession } from "@pickforge/picklab-core"; -import { watchDesktopSession } from "../src/commands/watch.js"; +import { watchDesktopSession, type SpawnWatchdogFn } from "../src/commands/watch.js"; let root: string; +// Every test must inject a fake watchdog spawner: the real one re-execs +// `process.argv[1]` (the vitest worker under test) with `internal +// takeover-watchdog` argv, which is a real, unwanted side effect in tests. +let watchdogKill: ReturnType; +let spawnWatchdog: SpawnWatchdogFn; + function fakeHandle(overrides: Partial = {}): HumanTakeoverHandle { return { sessionId: "desk-aaaaaa11", @@ -43,11 +49,16 @@ function fakeHandle(overrides: Partial = {}): HumanTakeover vncStartTimeTicks: 1, ttlMs: 30_000, heartbeatMs: 15, + expiresAt: new Date(Date.now() + 30_000).toISOString(), projectDir: root, ...overrides, }; } +function fakeLease(handle: HumanTakeoverHandle, ttlMs = 30_000): { leaseId: string; expiresAt: string } { + return { leaseId: handle.leaseId, expiresAt: new Date(Date.now() + ttlMs).toISOString() }; +} + function fakeViewer(overrides: Partial = {}): OpenVncViewerResult { return { opened: true, @@ -80,7 +91,9 @@ beforeEach(async () => { renewHumanTakeover.mockReset(); openVncViewer.mockReset(); endHumanTakeover.mockResolvedValue({ reason: "return" } satisfies EndHumanTakeoverResult); - renewHumanTakeover.mockResolvedValue(true); + renewHumanTakeover.mockImplementation(async (handle: HumanTakeoverHandle) => fakeLease(handle)); + watchdogKill = vi.fn(); + spawnWatchdog = vi.fn(() => ({ kill: watchdogKill })); }); afterEach(async () => { @@ -97,12 +110,13 @@ describe("watch --control (mocked)", () => { projectDir: root, control: true, waitForViewerExit: false, + _spawnWatchdog: spawnWatchdog, }), ).rejects.toThrow(/requires waiting for the viewer to exit/); expect(startHumanTakeover).not.toHaveBeenCalled(); }); - it("starts human takeover, waits for the viewer, and ends with reason 'return'", async () => { + it("starts human takeover, spawns the watchdog, waits for the viewer, and ends with reason 'return'", async () => { const id = await createDesktop(); const handle = fakeHandle({ sessionId: id }); startHumanTakeover.mockResolvedValue(handle); @@ -112,14 +126,23 @@ describe("watch --control (mocked)", () => { screenshotPath: "screenshots/abc.png", } satisfies EndHumanTakeoverResult); - const result = await watchDesktopSession({ session: id, projectDir: root, control: true }); + const result = await watchDesktopSession({ + session: id, + projectDir: root, + control: true, + _spawnWatchdog: spawnWatchdog, + }); expect(startHumanTakeover).toHaveBeenCalledWith(id, { registryEnv: process.env }); + expect(spawnWatchdog).toHaveBeenCalledWith(handle); expect(openVncViewer).toHaveBeenCalledWith({ port: handle.vncPort, waitForExit: true }); expect(endHumanTakeover).toHaveBeenCalledWith(handle, { registryEnv: process.env, reason: "return", }); + // The watchdog is stopped once control returns cleanly — it must not + // linger polling a lease that no longer exists. + expect(watchdogKill).toHaveBeenCalledTimes(1); expect(result.data).toMatchObject({ sessionId: id, leaseId: handle.leaseId, @@ -139,7 +162,12 @@ describe("watch --control (mocked)", () => { reason: opts.reason, })); - const pending = watchDesktopSession({ session: id, projectDir: root, control: true }); + const pending = watchDesktopSession({ + session: id, + projectDir: root, + control: true, + _spawnWatchdog: spawnWatchdog, + }); await new Promise((resolve) => setTimeout(resolve, 10)); process.emit("SIGINT"); const result = await pending; @@ -149,29 +177,107 @@ describe("watch --control (mocked)", () => { expect.objectContaining({ reason: "cancelled" }), ); expect(result.data?.controlReason).toBe("cancelled"); + expect(watchdogKill).toHaveBeenCalledTimes(1); }); - it("ends with reason 'timeout' when the lease cannot be renewed", async () => { + it("ends the takeover IMMEDIATELY on the first failed renewal, without waiting for the viewer", async () => { const id = await createDesktop(); const handle = fakeHandle({ sessionId: id, heartbeatMs: 10 }); startHumanTakeover.mockResolvedValue(handle); - renewHumanTakeover.mockResolvedValue(false); - openVncViewer.mockImplementation(() => delayedResolve(fakeViewer(), 80)); - endHumanTakeover.mockImplementation(async (_handle, opts) => ({ - reason: opts.reason, - })); + renewHumanTakeover.mockResolvedValue(undefined); + const events: string[] = []; + let viewerResolvedAt = Number.POSITIVE_INFINITY; + openVncViewer.mockImplementation(async () => { + const viewer = await delayedResolve(fakeViewer(), 100); + viewerResolvedAt = Date.now(); + events.push("viewer-resolved"); + return viewer; + }); + let endedAt = Number.POSITIVE_INFINITY; + endHumanTakeover.mockImplementation(async (_handle, opts) => { + endedAt = Date.now(); + events.push(`end:${String(opts.reason)}`); + return { reason: opts.reason }; + }); - const result = await watchDesktopSession({ session: id, projectDir: root, control: true }); + const started = Date.now(); + const result = await watchDesktopSession({ + session: id, + projectDir: root, + control: true, + _spawnWatchdog: spawnWatchdog, + }); expect(renewHumanTakeover).toHaveBeenCalled(); + // The end happened well before the (100ms-delayed) viewer resolved, and + // close to the 10ms heartbeat — proving it did not wait for the viewer. + expect(endedAt - started).toBeLessThan(60); + expect(endedAt).toBeLessThan(viewerResolvedAt); + expect(events[0]).toBe("end:timeout"); + expect(result.data?.controlReason).toBe("timeout"); + expect(result.lines?.join("\n")).toContain("could not be renewed"); + expect(watchdogKill).toHaveBeenCalledTimes(1); + }); + + it("force-ends via the hard deadline timer even if the heartbeat itself never fires", async () => { + const id = await createDesktop(); + // A heartbeat interval far longer than the test window: the deadline + // timer, not the heartbeat, must be what ends this takeover. + const handle = fakeHandle({ + sessionId: id, + heartbeatMs: 60_000, + expiresAt: new Date(Date.now() + 30).toISOString(), + }); + startHumanTakeover.mockResolvedValue(handle); + openVncViewer.mockImplementation(() => delayedResolve(fakeViewer(), 200)); + endHumanTakeover.mockImplementation(async (_handle, opts) => ({ reason: opts.reason })); + + const result = await watchDesktopSession({ + session: id, + projectDir: root, + control: true, + _spawnWatchdog: spawnWatchdog, + }); + + expect(renewHumanTakeover).not.toHaveBeenCalled(); expect(endHumanTakeover).toHaveBeenCalledWith( handle, expect.objectContaining({ reason: "timeout" }), ); expect(result.data?.controlReason).toBe("timeout"); - expect(result.lines?.join("\n")).toContain("could not be renewed"); }); + it("reschedules the deadline timer to the freshly renewed expiresAt", async () => { + const id = await createDesktop(); + // TTL of 40ms with a 10ms heartbeat: if the deadline timer were only ever + // scheduled against the *original* expiresAt (never rescheduled), it + // would still fire around 40ms even though renewals keep succeeding — + // this proves the reschedule keeps the backstop from firing early. + const handle = fakeHandle({ + sessionId: id, + heartbeatMs: 10, + ttlMs: 40, + expiresAt: new Date(Date.now() + 40).toISOString(), + }); + startHumanTakeover.mockResolvedValue(handle); + renewHumanTakeover.mockImplementation(async () => fakeLease(handle, 40)); + openVncViewer.mockImplementation(() => delayedResolve(fakeViewer(), 90)); + endHumanTakeover.mockImplementation(async (_handle, opts) => ({ reason: opts.reason })); + + const result = await watchDesktopSession({ + session: id, + projectDir: root, + control: true, + _spawnWatchdog: spawnWatchdog, + }); + + expect(result.data?.controlReason).toBe("return"); + expect(endHumanTakeover).toHaveBeenCalledWith( + handle, + expect.objectContaining({ reason: "return" }), + ); + }, 10_000); + it("ends control immediately and reports guidance when no viewer can be opened", async () => { const id = await createDesktop(); const handle = fakeHandle({ sessionId: id }); @@ -187,7 +293,12 @@ describe("watch --control (mocked)", () => { ); endHumanTakeover.mockResolvedValue({ reason: "return" } satisfies EndHumanTakeoverResult); - const result = await watchDesktopSession({ session: id, projectDir: root, control: true }); + const result = await watchDesktopSession({ + session: id, + projectDir: root, + control: true, + _spawnWatchdog: spawnWatchdog, + }); expect(endHumanTakeover).toHaveBeenCalledWith(handle, { registryEnv: process.env, diff --git a/packages/core/package.json b/packages/core/package.json index db7a653..4f12e4d 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -10,6 +10,7 @@ "exports": { ".": { "types": "./src/index.ts", + "development": "./src/index.ts", "default": "./dist/index.js" } }, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 32071a5..c6adf76 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -199,6 +199,7 @@ export { getTakeoverStatus, isHumanLeaseStale, readHumanLease, + readHumanLeaseRaw, recordTakeoverEvidence, releaseAgentPermit, releaseHumanLease, @@ -207,6 +208,7 @@ export { type AcquireHumanLeaseOptions, type AgentPermit, type HumanLease, + type HumanLeaseSnapshot, type RecordTakeoverEvidenceOptions, type TakeoverStatusResult, } from "./takeover.js"; diff --git a/packages/core/src/takeover.ts b/packages/core/src/takeover.ts index 3b43aab..1003f9e 100644 --- a/packages/core/src/takeover.ts +++ b/packages/core/src/takeover.ts @@ -267,6 +267,32 @@ export async function readHumanLease( return raw === undefined ? undefined : parseHumanLease(raw); } +/** A lease read together with its exact on-disk bytes. */ +export interface HumanLeaseSnapshot { + raw: string; + /** `undefined` when the raw content does not parse as a lease (corrupt). */ + lease?: HumanLease; +} + +/** + * Read a session's human lease together with its exact raw bytes, for + * callers that need to compare-and-delete on precisely what they observed + * (recovery's re-check-immediately-before-acting protocol) rather than only + * on `leaseId`, which does not change across a renewal and so cannot by + * itself detect "this lease was renewed since I last looked." Returns + * `undefined` only when no lease file exists at all. + */ +export async function readHumanLeaseRaw( + sessionId: string, + env: EnvLike = process.env, +): Promise { + assertSafeSessionId(sessionId); + const raw = await readTextIfPresent(humanLeasePath(sessionId, env)); + if (raw === undefined) return undefined; + const lease = parseHumanLease(raw); + return lease === undefined ? { raw } : { raw, lease }; +} + /** * Whether a lease is stale: its owner process is dead (or its PID was * reused), or its TTL elapsed without a heartbeat renewal. Either condition @@ -406,6 +432,12 @@ export async function renewHumanLease( const leasePath = humanLeasePath(sessionId, env); const current = await readHumanLease(sessionId, env); if (current === undefined || current.leaseId !== leaseId) return undefined; + // A lease that has already gone stale (TTL elapsed, or its recorded owner + // no longer matches this call's identity — e.g. reaped and reused) must + // never be resurrected by a late renewal: the owner lost the lease the + // instant it went stale, and a straggling renew must not extend it back to + // life out from under a recovery that may already be in flight. + if (isHumanLeaseStale(current, now)) return undefined; const updated: HumanLease = { ...current, expiresAt: new Date(now.getTime() + current.ttlMs).toISOString(), diff --git a/packages/core/test/takeover.test.ts b/packages/core/test/takeover.test.ts index f31adef..ab16868 100644 --- a/packages/core/test/takeover.test.ts +++ b/packages/core/test/takeover.test.ts @@ -223,6 +223,22 @@ describe("renewHumanLease / releaseHumanLease", () => { expect(await renewHumanLease("desk-c1", "not-the-owner", env)).toBeUndefined(); }); + it("refuses to resurrect a lease that has already gone stale by TTL, even for its own owner (P0-B)", async () => { + const lease = await acquireHumanLease("desk-c1b", env); + // The owner is this very process (alive), but the TTL has elapsed + // without a timely renewal — the agent must be able to observe the + // lease as free the instant that happens, so a straggling renewal must + // never bring it back to life out from under a recovery in flight. + const now = new Date(Date.parse(lease.expiresAt) + 1); + expect(isHumanLeaseStale(lease, now)).toBe(true); + + expect( + await renewHumanLease("desk-c1b", lease.leaseId, env, {}, now), + ).toBeUndefined(); + // Untouched: no expiresAt extension, no partial write. + expect(await readHumanLease("desk-c1b", env)).toEqual(lease); + }); + it("only releases the lease it owns", async () => { const lease = await acquireHumanLease("desk-c2", env); expect(await releaseHumanLease("desk-c2", "not-the-owner", env)).toBe(false); diff --git a/packages/desktop-linux/src/index.ts b/packages/desktop-linux/src/index.ts index 7e10662..29c0659 100644 --- a/packages/desktop-linux/src/index.ts +++ b/packages/desktop-linux/src/index.ts @@ -115,3 +115,9 @@ export { type StartHumanTakeoverOptions, type TakeoverEndReason, } from "./takeover.js"; + +export { + DEFAULT_TAKEOVER_WATCHDOG_POLL_MS, + runTakeoverWatchdogLoop, + type RunTakeoverWatchdogLoopOptions, +} from "./takeover-watchdog.js"; diff --git a/packages/desktop-linux/src/session.ts b/packages/desktop-linux/src/session.ts index a035bd3..63291f2 100644 --- a/packages/desktop-linux/src/session.ts +++ b/packages/desktop-linux/src/session.ts @@ -4,6 +4,7 @@ import { setTimeout as sleep } from "node:timers/promises"; import path from "node:path"; import { REAPER_CLEANUP_PENDING_META_KEY, + clearStaleHumanLease, createSession, destroySessionRecord, getSession, @@ -12,8 +13,8 @@ import { processIdentityMatches, reapDeadRunningSessions, readHumanLease, + readHumanLeaseRaw, recordTakeoverEvidence, - releaseHumanLease, sessionsDir, stopPid, stopProcessGroupVerified, @@ -435,9 +436,22 @@ export async function recoverStaleTakeoverLocked( record: SessionRecord, registryEnv: EnvLike, ): Promise<{ recovered: boolean }> { - const lease = await readHumanLease(id, registryEnv); - if (lease === undefined) return { recovered: false }; - if (!isHumanLeaseStale(lease)) return { recovered: false }; + const initial = await readHumanLease(id, registryEnv); + if (initial === undefined) return { recovered: false }; + if (!isHumanLeaseStale(initial)) return { recovered: false }; + + // TOCTOU guard (pickforge/picklab#21 P1-C): the cheap check above can be + // arbitrarily stale by the time we act — a live owner's heartbeat may have + // renewed the lease in the gap. Re-read immediately before the destructive + // VNC stop and re-check staleness on THAT read; a lease that is no longer + // stale (renewed) is left completely untouched. `leaseId` alone cannot + // detect a renewal (it never changes), so the final release below + // compare-and-deletes on the exact raw bytes captured here, not just the id. + const snapshot = await readHumanLeaseRaw(id, registryEnv); + if (snapshot === undefined) return { recovered: false }; + if (snapshot.lease !== undefined && !isHumanLeaseStale(snapshot.lease)) { + return { recovered: false }; + } const desktop = record.desktop; if (desktop !== undefined && desktop.vncPid !== undefined && desktop.vncViewOnly !== true) { @@ -461,11 +475,14 @@ export async function recoverStaleTakeoverLocked( status: "error", }); - // Compare-and-delete by leaseId. A `false` result means a concurrent - // acquirer already replaced this lease (or released it themselves) between - // our read above and here — safe to leave alone either way; the VNC side - // effect above is idempotent and already reclaimed the stale writable VNC. - await releaseHumanLease(id, lease.leaseId, registryEnv); + // Compare-and-delete on the exact bytes captured at the final stale check + // above, not merely `leaseId` (which a renewal never changes). If the file + // changed again since — another renewal slipped in during the VNC stop + // itself — it is left alone rather than deleted out from under a possibly + // now-live claim. The VNC-stop decision above was correct at the instant it + // was made (two consecutive stale reads); this bounds the residual race to + // the width of `stopOwnedSessionVnc` alone, down from the whole function. + await clearStaleHumanLease(id, snapshot.raw, registryEnv); return { recovered: true }; } diff --git a/packages/desktop-linux/src/takeover-watchdog.ts b/packages/desktop-linux/src/takeover-watchdog.ts new file mode 100644 index 0000000..5e20ff5 --- /dev/null +++ b/packages/desktop-linux/src/takeover-watchdog.ts @@ -0,0 +1,71 @@ +import { setTimeout as delay } from "node:timers/promises"; +import { isHumanLeaseStale, readHumanLease, type EnvLike } from "@pickforge/picklab-core"; +import { recoverStaleHumanLease } from "./takeover.js"; + +/** Default interval between staleness checks (pickforge/picklab#21 P0-A). */ +export const DEFAULT_TAKEOVER_WATCHDOG_POLL_MS = 5_000; + +export interface RunTakeoverWatchdogLoopOptions { + sessionId: string; + leaseId: string; + registryEnv?: EnvLike; + pollIntervalMs?: number; + /** + * @internal Test hook, checked once per iteration right before sleeping. + * Returning true stops the loop even though the lease it is watching is + * still present and live — used to bound a test's runtime rather than + * waiting for the watched lease to end. + */ + _shouldStop?: () => boolean; +} + +/** + * Actively reclaim a writable VNC session if its human lease goes stale, + * independent of the takeover-owning process's own lifetime. + * + * This is the crash-path half of the "writable VNC never outlives its + * lease" invariant (pickforge/picklab#21 P0-A): `picklab watch --control` + * spawns this loop as a **detached** child process (own process group, not + * killed by a `SIGKILL` of its parent — see + * `packages/cli/src/commands/watch.ts`), so a crash of the controlling + * process does not leave writable VNC running until some later, unrelated + * operation happens to touch the session. That would be a *lazy* reclaim — + * correct eventually, but with no wall-clock bound — and is rejected: the + * watchdog polls on its own schedule, in its own process, and actively stops + * a stale writable VNC (via the same TOCTOU-safe `recoverStaleHumanLease` + * used elsewhere) the first time it observes the lease has gone stale. + * + * A short-lived detached process was chosen over OS-level parent-death + * coupling (e.g. Linux `PR_SET_PDEATHSIG`) because the latter has no + * portable Node.js API — it needs either a native addon or an external + * wrapper binary not guaranteed to be present — while a detached sibling + * process is plain, dependency-free Node/TypeScript consistent with the rest + * of this codebase, and the existing `recoverStaleHumanLease` primitive + * already does exactly the reclaim work this loop needs to trigger. + * + * Exits on its own — no external signal needed — as soon as the lease it is + * watching is gone or has been superseded by a different lease (the + * takeover it was watching ended, one way or another) or once it has + * actively reclaimed a stale one. The controlling process additionally + * terminates it directly on a clean end, for promptness; this self-exit is + * the backstop if that termination itself is lost (e.g. the controlling + * process crashes before it can send the signal). + */ +export async function runTakeoverWatchdogLoop( + opts: RunTakeoverWatchdogLoopOptions, +): Promise { + const registryEnv = opts.registryEnv ?? process.env; + const pollIntervalMs = opts.pollIntervalMs ?? DEFAULT_TAKEOVER_WATCHDOG_POLL_MS; + for (;;) { + const lease = await readHumanLease(opts.sessionId, registryEnv); + if (lease === undefined || lease.leaseId !== opts.leaseId) { + return; + } + if (isHumanLeaseStale(lease)) { + await recoverStaleHumanLease(opts.sessionId, registryEnv); + return; + } + if (opts._shouldStop?.() === true) return; + await delay(pollIntervalMs); + } +} diff --git a/packages/desktop-linux/src/takeover.ts b/packages/desktop-linux/src/takeover.ts index 564a495..cd5f412 100644 --- a/packages/desktop-linux/src/takeover.ts +++ b/packages/desktop-linux/src/takeover.ts @@ -46,6 +46,8 @@ export interface HumanTakeoverHandle { vncStartTimeTicks: number; ttlMs: number; heartbeatMs: number; + /** The lease's current expiry, as of the last successful renewal. */ + expiresAt: string; projectDir: string; } @@ -162,11 +164,17 @@ export async function startHumanTakeover( }, registryEnv, ); - await renewHumanLease(id, lease.leaseId, registryEnv, { + const patched = await renewHumanLease(id, lease.leaseId, registryEnv, { vncPid: vnc.pid, vncStartTimeTicks: vnc.startTimeTicks, vncPort: vnc.port, }); + if (patched === undefined) { + throw new Error( + `Human lease ${lease.leaseId} for session ${id} went stale before its writable VNC could be recorded`, + ); + } + lease.expiresAt = patched.expiresAt; } catch (error) { await stopOwnedSessionVnc(id, { ...desktop, @@ -190,6 +198,7 @@ export async function startHumanTakeover( vncStartTimeTicks: vnc.startTimeTicks, ttlMs: lease.ttlMs, heartbeatMs: lease.heartbeatMs, + expiresAt: lease.expiresAt, projectDir: record.projectDir, }; }); @@ -200,14 +209,20 @@ export async function startHumanTakeover( * no longer ours — the caller must then end the takeover with reason * `"timeout"` rather than keep driving a writable VNC past its lease. */ +/** + * Renew a held lease's TTL. Returns the renewed lease (so callers can read + * its fresh `expiresAt` and reschedule their own hard-deadline backstop), or + * `undefined` if the lease was no longer renewable — already stale (TTL + * elapsed without a timely renewal: `renewHumanLease` itself refuses to + * resurrect a stale lease, see pickforge/picklab#21 P0-B) or held by someone + * else. The caller must treat `undefined` as "the lease is gone" and end the + * takeover immediately, not merely stop trying to renew. + */ export async function renewHumanTakeover( handle: HumanTakeoverHandle, registryEnv: EnvLike = process.env, -): Promise { - const renewed = await renewHumanLease(handle.sessionId, handle.leaseId, registryEnv).catch( - () => undefined, - ); - return renewed !== undefined; +): Promise { + return renewHumanLease(handle.sessionId, handle.leaseId, registryEnv).catch(() => undefined); } export interface EndHumanTakeoverOptions { diff --git a/packages/desktop-linux/test/takeover.test.ts b/packages/desktop-linux/test/takeover.test.ts index 5d837b4..76fad1f 100644 --- a/packages/desktop-linux/test/takeover.test.ts +++ b/packages/desktop-linux/test/takeover.test.ts @@ -1,6 +1,8 @@ +import { spawn } from "node:child_process"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; +import { fileURLToPath } from "node:url"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; // `startVnc` requires a verified `/proc`-backed process identity for the @@ -11,6 +13,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; // identity functions replaced by a deterministic, pid-keyed stand-in — the // same technique `destroy.test.ts` uses. Real x11vnc/Xvfb hardware // validation is deferred (see AGENTS.md / release notes). +// `readHumanLeaseRaw` is wrapped (not replaced) so individual tests can +// inject an interleaving action between recovery's two staleness checks via +// `mockImplementationOnce`, while every other call still runs the real +// implementation. vi.mock("@pickforge/picklab-core", async (importOriginal) => { const actual = await importOriginal(); return { @@ -19,6 +25,7 @@ vi.mock("@pickforge/picklab-core", async (importOriginal) => { processIdentityMatches: vi.fn( ({ pid, startTicks }: { pid: number; startTicks: number }) => startTicks === pid, ), + readHumanLeaseRaw: vi.fn(actual.readHumanLeaseRaw), }; }); @@ -29,6 +36,7 @@ import { isPidAlive, readActions, readHumanLease, + readHumanLeaseRaw, resolveActivePointer, resolveRunStorage, stopPid, @@ -40,6 +48,7 @@ import { renewHumanTakeover, startHumanTakeover, } from "../src/takeover.js"; +import { startVnc } from "../src/vnc.js"; let root: string; let binDir: string; @@ -163,11 +172,20 @@ describe("startHumanTakeover / endHumanTakeover", () => { expect(fullArgv[1]).toContain("-viewonly"); }); - it("records a takeover_start and takeover_ evidence transition", async () => { + it("records a takeover_start and takeover_ evidence transition, and reverts VNC to read-only on the cancelled path", async () => { const id = await createDesktop({ vncPort: nextPort() }); const handle = await startHumanTakeover(id, { registryEnv: env, env }); await endHumanTakeover(handle, { registryEnv: env, env, reason: "cancelled" }); + // The cancelled exit path reverts VNC to read-only exactly like the + // return path: writable on start, `-viewonly` on end — never left + // writable because the reason for ending was "cancelled" rather than + // "return". + const cancelledArgv = await readArgvLog(); + expect(cancelledArgv).toHaveLength(2); + expect(cancelledArgv[0]).not.toContain("-viewonly"); + expect(cancelledArgv[1]).toContain("-viewonly"); + const pointerless = await resolveActivePointer(root, id, env); // The evidence run was finalized by nothing yet (no destroySessionRecord // call), so it is still resolvable as the session's active run. @@ -194,24 +212,57 @@ describe("startHumanTakeover / endHumanTakeover", () => { await endHumanTakeover(handle, { registryEnv: env, env, reason: "return" }); }); - it("renews the lease TTL while control is held", async () => { + it("renews the lease TTL while control is held, and reverts VNC to read-only on the timeout path", async () => { const id = await createDesktop({ vncPort: nextPort() }); const handle = await startHumanTakeover(id, { registryEnv: env, env }); const before = await readHumanLease(id, env); await new Promise((resolve) => setTimeout(resolve, 10)); - expect(await renewHumanTakeover(handle, env)).toBe(true); + const renewed = await renewHumanTakeover(handle, env); + expect(renewed).toBeDefined(); const after = await readHumanLease(id, env); expect(Date.parse(after!.expiresAt)).toBeGreaterThanOrEqual( Date.parse(before!.expiresAt), ); await endHumanTakeover(handle, { registryEnv: env, env, reason: "timeout" }); + + // The timeout exit path also reverts VNC to read-only — writable never + // outlives the lease on this path either. + const timeoutArgv = await readArgvLog(); + expect(timeoutArgv).toHaveLength(2); + expect(timeoutArgv[0]).not.toContain("-viewonly"); + expect(timeoutArgv[1]).toContain("-viewonly"); }); it("reports renewal failure once the lease is gone (post-timeout)", async () => { const id = await createDesktop({ vncPort: nextPort() }); const handle = await startHumanTakeover(id, { registryEnv: env, env }); await endHumanTakeover(handle, { registryEnv: env, env, reason: "timeout" }); - expect(await renewHumanTakeover(handle, env)).toBe(false); + expect(await renewHumanTakeover(handle, env)).toBeUndefined(); + }); + + it("refuses to renew a lease that has gone stale by TTL, even though its owner is alive (P0-B)", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + const handle = await startHumanTakeover(id, { registryEnv: env, env }); + // Force the on-disk lease's TTL into the past directly, rather than + // racing a real wall-clock sleep against real VNC startup timing: the + // owner (this process) is still very much alive, but the lease must be + // treated as free once its TTL has passed regardless. + const current = await readHumanLease(id, env); + await fs.promises.writeFile( + path.join(env.PICKLAB_HOME as string, "sessions", id, "human.lease.json"), + `${JSON.stringify({ ...current, expiresAt: new Date(Date.now() - 1_000).toISOString() })}\n`, + ); + expect(await readHumanLease(id, env)).toMatchObject({ + leaseId: handle.leaseId, + }); + expect(await renewHumanTakeover(handle, env)).toBeUndefined(); + // The stale lease is left exactly as it was — a late renewal must never + // resurrect it, so a concurrent acquirer can safely reclaim it. + expect(await readHumanLease(id, env)).toMatchObject({ leaseId: handle.leaseId }); + + const { recovered } = await recoverStaleHumanLease(id, env); + expect(recovered).toBe(true); + expect(await readHumanLease(id, env)).toBeUndefined(); }); }); @@ -255,6 +306,57 @@ describe("recoverStaleHumanLease (crash recovery)", () => { await endHumanTakeover(handle, { registryEnv: env, env, reason: "return" }); }); + it("bails without touching VNC when the lease is renewed between the check and the stop (P1-C TOCTOU)", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + const handle = await startHumanTakeover(id, { registryEnv: env, env, drainTimeoutMs: 500 }); + + // Make the lease appear stale (TTL elapsed) to the cheap *initial* + // check, while the owner (this process) is still very much alive. + const stale = { + ...(await readHumanLease(id, env))!, + expiresAt: new Date(Date.now() - 1_000).toISOString(), + }; + await fs.promises.writeFile( + path.join(env.PICKLAB_HOME as string, "sessions", id, "human.lease.json"), + `${JSON.stringify(stale)}\n`, + ); + + // Interleave: right as recovery performs its *final* re-check + // (immediately before the destructive VNC stop), simulate the owner's + // heartbeat winning the race and renewing the lease first. Written + // directly (not via `renewHumanTakeover`) because a renewal that itself + // re-validates staleness would correctly refuse to renew what is, at + // that exact instant, still on-disk as stale (P0-B) — this reproduces + // what a *successful*, just-in-time renewal would have left on disk a + // moment earlier, which is the scenario under test. + const renewed = { ...stale, expiresAt: new Date(Date.now() + 30_000).toISOString() }; + const readHumanLeaseRawMock = readHumanLeaseRaw as unknown as ReturnType; + readHumanLeaseRawMock.mockImplementationOnce( + async (sessionId: string, callEnv: EnvLike) => { + await fs.promises.writeFile( + path.join(callEnv.PICKLAB_HOME as string, "sessions", sessionId, "human.lease.json"), + `${JSON.stringify(renewed)}\n`, + ); + return { raw: `${JSON.stringify(renewed)}\n`, lease: renewed }; + }, + ); + + const { recovered } = await recoverStaleHumanLease(id, env); + + // Recovery must bail entirely: no VNC stop, no lease deletion — the + // renewal that landed mid-check made this a live takeover again. + expect(recovered).toBe(false); + expect(isPidAlive(handle.vncPid)).toBe(true); + const after = await readHumanLease(id, env); + expect(after?.leaseId).toBe(handle.leaseId); + expect(Date.parse(after!.expiresAt)).toBeGreaterThan(Date.parse(stale.expiresAt)); + const record = await getSession(id, env); + expect(record?.desktop?.vncPid).toBe(handle.vncPid); + expect(record?.desktop?.vncViewOnly).toBe(false); + + await endHumanTakeover(handle, { registryEnv: env, env, reason: "return" }); + }); + it("is a no-op when there is no lease at all", async () => { const id = await createDesktop({ vncPort: nextPort() }); const { recovered } = await recoverStaleHumanLease(id, env); @@ -291,6 +393,43 @@ describe("startHumanTakeover self-healing", () => { }); }); +describe("startHumanTakeover against a pre-existing --vnc-control session", () => { + it("degrades safely: fails cleanly, leaves the persistent writable VNC undisturbed, and rolls back the lease", async () => { + // Simulates `picklab session create --vnc-control`'s persistent, lease- + // uncoordinated writable VNC — a completely different mechanism (#22) + // from the leased takeover this module implements (#21). The two must + // never be silently conflated: a takeover attempt against a session + // already writable this way must not corrupt or hijack it. + const port = nextPort(); + const preExisting = await startVnc({ + display: ":42", + port, + logDir: path.join(root, "pre-existing-vnc-control"), + env, + viewOnly: false, + }); + const id = await createDesktop({ + vncPid: preExisting.pid, + vncStartTimeTicks: preExisting.startTimeTicks, + vncPort: port, + vncViewOnly: false, + }); + + await expect(startHumanTakeover(id, { registryEnv: env, env })).rejects.toThrow(); + + // Safe degradation: the pre-existing writable VNC is exactly as it was — + // never stopped, never restarted, never handed a lease it doesn't know + // about — and no orphaned lease is left behind for the failed attempt. + expect(isPidAlive(preExisting.pid)).toBe(true); + expect(await readHumanLease(id, env)).toBeUndefined(); + const record = await getSession(id, env); + expect(record?.desktop?.vncPid).toBe(preExisting.pid); + expect(record?.desktop?.vncViewOnly).toBe(false); + + await stopPid(preExisting.pid); + }); +}); + describe("ensureSessionVnc recovery integration", () => { it("recovers a crash-orphaned writable VNC instead of refusing to watch", async () => { const { ensureSessionVnc } = await import("../src/session.js"); @@ -327,3 +466,92 @@ describe("ensureSessionVnc recovery integration", () => { await endHumanTakeover(handle, { registryEnv: env, env, reason: "return" }); }); }); + +// Real separate-process proof (pickforge/picklab#21 P0-A): the watchdog must +// actively reclaim a stale lease running as a genuinely independent OS +// process — not merely "correct in-process against a mock" — since the whole +// point is surviving a `SIGKILL` of its sibling `watch --control` process. +// Spawned via `bun` (this repo's test runtime), matching the existing +// `evidence.concurrency.test.ts` separate-process pattern. VNC-stop +// verification itself (mocked-identity, real spawned x11vnc) is already +// covered above; this test only needs to prove the lease-level reclaim +// happens from an independent process, so it stays platform-portable by +// never depending on `/proc`-verified VNC identity. +const BUN = /[\\/]bun$/.test(process.execPath) ? process.execPath : "bun"; +const watchdogWorker = fileURLToPath( + new URL("./workers/takeover-watchdog-worker.ts", import.meta.url), +); +// Bun's default package resolution for a plain `bun ` invocation +// follows `@pickforge/picklab-core`'s published `exports.default` +// (`dist/index.js`) rather than vitest's own source alias, so the worker +// would otherwise run against whatever the core package's dist happened to +// contain at last build — stale during normal iteration, unlike every other +// test in this suite (which runs in-process through vitest's source alias). +// `--conditions=development` selects `packages/core/package.json`'s +// `development` export condition (source) instead, so this test is exercised +// against the same source the rest of the suite is. +const BUN_ARGS = ["--conditions=development"]; + +function spawnWorker(args: string[]) { + return spawn(BUN, [...BUN_ARGS, ...args], { stdio: "ignore" }); +} + +function runWorker(args: string[]): Promise<{ code: number | null }> { + return new Promise((resolve, reject) => { + const child = spawnWorker(args); + child.on("error", reject); + child.on("close", (code) => resolve({ code })); + }); +} + +describe("runTakeoverWatchdogLoop (real separate-process)", () => { + it("reclaims a stale lease from a genuinely independent OS process", async () => { + const id = await createDesktop(); + const stale = { + leaseId: "crashed-lease", + sessionId: id, + ownerPid: 999_999, + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() - 1_000).toISOString(), + ttlMs: 30_000, + heartbeatMs: 5_000, + }; + await fs.promises.mkdir(path.join(env.PICKLAB_HOME as string, "sessions", id), { + recursive: true, + }); + await fs.promises.writeFile( + path.join(env.PICKLAB_HOME as string, "sessions", id, "human.lease.json"), + `${JSON.stringify(stale)}\n`, + ); + expect(await readHumanLease(id, env)).toBeDefined(); + + await runWorker([ + watchdogWorker, + id, + "crashed-lease", + env.PICKLAB_HOME as string, + "20", + ]); + + // A separate process, with no shared in-memory state whatsoever with + // this test, independently discovered the stale lease and cleared it. + expect(await readHumanLease(id, env)).toBeUndefined(); + }); + + it("keeps polling without touching a live lease (never exits early)", async () => { + const id = await createDesktop({ vncPort: nextPort() }); + const handle = await startHumanTakeover(id, { registryEnv: env, env }); + + // A live lease never makes the watchdog exit on its own — that only + // happens once it goes stale, is released, or is superseded — so this + // proves the negative (no premature reclaim) by letting it poll for + // several cycles, then terminating it directly rather than waiting for + // a natural exit that would never come. + const child = spawnWorker([watchdogWorker, id, handle.leaseId, env.PICKLAB_HOME as string, "20"]); + await new Promise((resolve) => setTimeout(resolve, 150)); + expect((await readHumanLease(id, env))?.leaseId).toBe(handle.leaseId); + child.kill("SIGTERM"); + + await endHumanTakeover(handle, { registryEnv: env, env, reason: "return" }); + }); +}); diff --git a/packages/desktop-linux/test/workers/takeover-watchdog-worker.ts b/packages/desktop-linux/test/workers/takeover-watchdog-worker.ts new file mode 100644 index 0000000..452f8d0 --- /dev/null +++ b/packages/desktop-linux/test/workers/takeover-watchdog-worker.ts @@ -0,0 +1,22 @@ +// Separate-process watchdog worker, run with `bun`. Proves +// `runTakeoverWatchdogLoop`'s reclaim logic works as a genuinely independent +// OS process (not just in-process against a mock) — the property that lets +// `picklab watch --control` survive its own SIGKILL (pickforge/picklab#21 +// P0-A). Not a `*.test.ts` file, so vitest never runs it directly. +import { runTakeoverWatchdogLoop } from "../../src/takeover-watchdog.js"; + +const [sessionId, leaseId, home, pollIntervalMs] = process.argv.slice(2); +if (sessionId === undefined || leaseId === undefined || home === undefined) { + console.error( + "usage: takeover-watchdog-worker [pollIntervalMs]", + ); + process.exit(2); +} + +await runTakeoverWatchdogLoop({ + sessionId, + leaseId, + registryEnv: { ...process.env, PICKLAB_HOME: home }, + pollIntervalMs: pollIntervalMs === undefined ? undefined : Number(pollIntervalMs), +}); +process.exit(0); From c6e2c2f897b1526f539296133626317104efdf5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 11:02:35 -0300 Subject: [PATCH 3/5] fix(browser): serialize DevTools relay output writes across busy/forward pumps The takeover busy-intercept path and the normal child-response pump are two independent concurrent producers that can both write to the same client-facing output stream (the intercept answers a blocked tools/call directly on it; the forward pump also writes upstream responses to it). Add createJsonRpcWriteQueue, a small write-ordering queue, and thread it through pumpJsonRpcNdjson as writeSerializer / interceptWriteSerializer so every write either pump issues to a shared destination is fully ordered through one explicit gateway rather than left as an incidental property of whichever Writable happens to be passed in. Wired into runDevtoolsMcpRelay so a busy rejection racing an in-flight forwarded response can never interleave on the wire. --- packages/browser/src/devtools-mcp.ts | 20 +++++++ packages/browser/src/index.ts | 2 + packages/browser/src/ndjson.ts | 47 +++++++++++++++-- packages/browser/test/ndjson.test.ts | 78 ++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 4 deletions(-) diff --git a/packages/browser/src/devtools-mcp.ts b/packages/browser/src/devtools-mcp.ts index 00bb80a..7f3afbd 100644 --- a/packages/browser/src/devtools-mcp.ts +++ b/packages/browser/src/devtools-mcp.ts @@ -24,6 +24,7 @@ import { import { getBrowserSessionStatus, type BrowserSessionStatus } from "./session.js"; import { JsonRpcProtocolError, + createJsonRpcWriteQueue, pumpJsonRpcNdjson, writeWithBackpressure, type JsonRpcHook, @@ -48,6 +49,15 @@ function jsonRpcRequestId(message: JsonRpcMessage): string | number | undefined * `tools/call` request is answered directly with a stable busy error instead * of ever reaching the child Chrome DevTools MCP process. Notifications (no * `id`) and non-tool-call requests pass through untouched. + * + * Evidencing asymmetry, by design: an MCP desktop-input tool blocked by the + * same lease (`withAgentPermit`, in `@pickforge/picklab-desktop-linux`) is + * evidenced as an `"error"`-status action, because it runs inside + * `withMcpEvidence`'s existing per-call action lifecycle. A blocked DevTools + * relay request has no equivalent per-call evidence lifecycle to hook — + * `beforeForward`/`afterResponse` are never invoked for an intercepted + * record — so it is not recorded as an evidence action. Both still fail + * closed identically; only the audit trail differs. */ export function createTakeoverBusyIntercept( sessionId: string, @@ -421,16 +431,26 @@ export async function runDevtoolsMcpRelay( const { outcome: exit, exited } = observeChildExit(child); const inputAbort = new AbortController(); let terminationRequested = false; + // The intercept path (inputPump, answering directly on `output`) and the + // normal child-response forward path (outputPump, also writing to `output`) + // are two independent concurrent pumps that can both target the same + // client-facing stream. A shared write queue fully orders every write + // issued to `output` across both of them, so a busy-rejection response + // interleaved with an in-flight child response can never produce a torn or + // out-of-order frame on the wire (pickforge/picklab#21 P1-D). + const outputWriteQueue = createJsonRpcWriteQueue(); const inputPump = pumpJsonRpcNdjson(input, child.stdin, { hook: opts.hooks?.beforeForward, intercept: opts.hooks?.intercept, interceptDestination: opts.hooks?.intercept === undefined ? undefined : output, + interceptWriteSerializer: opts.hooks?.intercept === undefined ? undefined : outputWriteQueue, signal: inputAbort.signal, endDestination: true, maxRecordBytes: opts.maxRecordBytes, }); const outputPump = pumpJsonRpcNdjson(child.stdout, output, { hook: opts.hooks?.afterResponse, + writeSerializer: outputWriteQueue, maxRecordBytes: opts.maxRecordBytes, }); const diagnosticsPump = pumpRedactedDiagnostics( diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts index 783be24..12003b4 100644 --- a/packages/browser/src/index.ts +++ b/packages/browser/src/index.ts @@ -48,6 +48,7 @@ export { DEFAULT_MAX_JSON_RPC_RECORD_BYTES, JsonRpcProtocolError, assertJsonRpcMessage, + createJsonRpcWriteQueue, pumpJsonRpcNdjson, serializeJsonRpcMessage, writeWithBackpressure, @@ -56,6 +57,7 @@ export { type JsonRpcIntercept, type JsonRpcMessage, type JsonRpcRecord, + type JsonRpcWriteSerializer, type PumpJsonRpcNdjsonOptions, } from "./ndjson.js"; diff --git a/packages/browser/src/ndjson.ts b/packages/browser/src/ndjson.ts index 08f883d..21134b3 100644 --- a/packages/browser/src/ndjson.ts +++ b/packages/browser/src/ndjson.ts @@ -196,6 +196,30 @@ export function serializeJsonRpcMessage(message: JsonRpcMessage): Buffer { return Buffer.from(`${JSON.stringify(message)}\n`); } +/** + * Runs `write` in turn, never overlapping with another write issued through + * the same serializer. Used to fully order writes to a stream that more than + * one independent pump can target — e.g. the DevTools relay's fail-closed + * intercept answering directly on the client-facing output stream while the + * child-response pump also writes to it (pickforge/picklab#21 P1-D) — so two + * concurrent producers can never have overlapping in-flight writes to the + * same destination, made explicit in the code rather than left as an + * incidental property of whichever `Writable` happens to be passed in. + */ +export type JsonRpcWriteSerializer = (write: () => Promise) => Promise; + +/** Create a fresh, independent write-ordering queue for `JsonRpcWriteSerializer`. */ +export function createJsonRpcWriteQueue(): JsonRpcWriteSerializer { + let queue: Promise = Promise.resolve(); + return (write) => { + const result = queue.then(write); + // A failed write must not permanently wedge the queue for later writers; + // only THIS call's returned promise carries the rejection. + queue = result.catch(() => {}); + return result; + }; +} + export async function writeWithBackpressure( destination: Writable, bytes: Buffer, @@ -292,11 +316,23 @@ export interface PumpJsonRpcNdjsonOptions { intercept?: JsonRpcIntercept; /** Where an `intercept` response is written instead of `destination`. */ interceptDestination?: Writable; + /** + * Serializes writes to `destination` against any other pump sharing the + * same stream via its own serializer from the same `createJsonRpcWriteQueue()` + * instance. Defaults to running the write immediately (no cross-pump + * ordering) — pass a shared queue when `destination` is also written by + * another pump/writer. + */ + writeSerializer?: JsonRpcWriteSerializer; + /** Same as `writeSerializer`, but for writes to `interceptDestination`. */ + interceptWriteSerializer?: JsonRpcWriteSerializer; signal?: AbortSignal; endDestination?: boolean; maxRecordBytes?: number; } +const runWriteImmediately: JsonRpcWriteSerializer = (write) => write(); + export async function pumpJsonRpcNdjson( source: Readable, destination: Writable, @@ -305,6 +341,8 @@ export async function pumpJsonRpcNdjson( if (opts.intercept !== undefined && opts.interceptDestination === undefined) { throw new Error("pumpJsonRpcNdjson: intercept requires interceptDestination"); } + const writeSerializer = opts.writeSerializer ?? runWriteImmediately; + const interceptWriteSerializer = opts.interceptWriteSerializer ?? runWriteImmediately; const decoder = new JsonRpcNdjsonBuffer(opts.maxRecordBytes); const iterator = source.iterator({ destroyOnReturn: false })[Symbol.asyncIterator](); try { @@ -319,13 +357,14 @@ export async function pumpJsonRpcNdjson( const intercepted = opts.intercept === undefined ? undefined : await opts.intercept(record.message); if (intercepted !== undefined) { - await writeWithBackpressure( - opts.interceptDestination!, - serializeJsonRpcMessage(intercepted), + const interceptBytes = serializeJsonRpcMessage(intercepted); + await interceptWriteSerializer(() => + writeWithBackpressure(opts.interceptDestination!, interceptBytes), ); continue; } - await writeWithBackpressure(destination, await applyHook(record, opts.hook)); + const forwardBytes = await applyHook(record, opts.hook); + await writeSerializer(() => writeWithBackpressure(destination, forwardBytes)); } } decoder.end(); diff --git a/packages/browser/test/ndjson.test.ts b/packages/browser/test/ndjson.test.ts index 1e0c743..1f28047 100644 --- a/packages/browser/test/ndjson.test.ts +++ b/packages/browser/test/ndjson.test.ts @@ -2,6 +2,7 @@ import { Readable, Writable } from "node:stream"; import { describe, expect, it } from "vitest"; import { createDeferred, + createJsonRpcWriteQueue, JsonRpcNdjsonBuffer, pumpJsonRpcNdjson, type JsonRpcMessage, @@ -207,4 +208,81 @@ describe("pumpJsonRpcNdjson", () => { }), ).rejects.toThrow(/interceptDestination/); }); + + it("createJsonRpcWriteQueue never overlaps two writers on the same destination", async () => { + const events: string[] = []; + let inFlight = false; + const destination = collectingWritable([], async (chunk) => { + if (inFlight) { + throw new Error(`overlapping write detected: ${chunk.toString()}`); + } + inFlight = true; + events.push(`start:${chunk.toString().trim()}`); + // A slow write for the first record — a concurrent second writer must + // wait for this to fully finish before its own write begins. + await new Promise((resolve) => setTimeout(resolve, 20)); + events.push(`end:${chunk.toString().trim()}`); + inFlight = false; + }); + const queue = createJsonRpcWriteQueue(); + const first = queue(() => new Promise((resolve) => { + destination.write(Buffer.from("first\n"), () => resolve()); + })); + // Issued immediately after, while `first` is still in flight. + const second = queue(() => new Promise((resolve) => { + destination.write(Buffer.from("second\n"), () => resolve()); + })); + await Promise.all([first, second]); + expect(events).toEqual(["start:first", "end:first", "start:second", "end:second"]); + }); + + it("interleaving a busy rejection with an in-flight child response never produces a torn frame", async () => { + // Mirrors the DevTools relay's wiring: two independent pumps writing to + // the same client-facing stream through one shared write queue — the + // intercept path (busy rejection) and the normal forward path (child + // response), racing for real. + const output: Buffer[] = []; + const slowFirstWrite = collectingWritable(output, async () => { + // Only the *first* physical write to land is slowed, so the second + // writer's attempt genuinely overlaps in wall-clock time with the + // first's in-flight write — the scenario the queue must serialize. + if (output.length === 1) { + await new Promise((resolve) => setTimeout(resolve, 20)); + } + }); + const queue = createJsonRpcWriteQueue(); + + const childResponsePump = pumpJsonRpcNdjson( + Readable.from(['{"jsonrpc":"2.0","id":1,"result":{"ok":true}}\n']), + slowFirstWrite, + { writeSerializer: queue }, + ); + const busyRejectionPump = pumpJsonRpcNdjson( + Readable.from(['{"jsonrpc":"2.0","id":2,"method":"tools/call"}\n']), + new Writable({ write: (_c, _e, cb) => cb() }), // never forwarded (intercepted) + { + intercept: (message) => ({ + jsonrpc: "2.0", + id: message.id as number, + error: { code: -32050, message: "busy" }, + }), + interceptDestination: slowFirstWrite, + interceptWriteSerializer: queue, + }, + ); + + await Promise.all([childResponsePump, busyRejectionPump]); + + // Both writes landed as two complete, well-formed, individually parsed + // lines — never merged, torn, or interleaved mid-frame. + expect(output).toHaveLength(2); + const lines = output.map((chunk) => JSON.parse(chunk.toString()) as JsonRpcMessage); + const byId = new Map(lines.map((line) => [line.id, line])); + expect(byId.get(1)).toEqual({ jsonrpc: "2.0", id: 1, result: { ok: true } }); + expect(byId.get(2)).toEqual({ + jsonrpc: "2.0", + id: 2, + error: { code: -32050, message: "busy" }, + }); + }); }); From ce24c69e2cb3d61af6c33d1aee9ac41d18c5bca4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 11:02:46 -0300 Subject: [PATCH 4/5] fix(cli,mcp-server): gate desktop_launch behind the human-takeover permit Security adjudication: launchApp spawns a new client onto the shared X11 display a human may be actively controlling under a takeover lease. On bare Xvfb a new client can grab input focus, which is exactly what the lease exists to prevent when a human is mid-entry of a secret. Wrap the launchApp call in withAgentPermit at both the CLI (picklab desktop launch) and MCP (desktop_launch) call sites, symmetric with the seven desktop input tools. desktop_screenshot remains ungated (read-only, delivers no input). --- packages/cli/src/commands/desktop.ts | 20 +++++++---- .../cli/test/desktop-input-takeover.test.ts | 32 ++++++++++++++++- packages/mcp-server/src/tools/desktop.ts | 28 +++++++++------ packages/mcp-server/test/takeover.test.ts | 34 +++++++++++++++++++ 4 files changed, 95 insertions(+), 19 deletions(-) diff --git a/packages/cli/src/commands/desktop.ts b/packages/cli/src/commands/desktop.ts index 6df2de3..b21fd0a 100644 --- a/packages/cli/src/commands/desktop.ts +++ b/packages/cli/src/commands/desktop.ts @@ -1,3 +1,4 @@ +import { withAgentPermit } from "@pickforge/picklab-core"; import { click, desktopSessionLogDir, @@ -49,13 +50,18 @@ export async function runDesktopLaunch( ): Promise { return runReported(opts, async () => { const { id, display } = await resolveDesktop(opts); - const app = await launchApp({ - display, - command, - args, - logDir: desktopSessionLogDir(id), - cwd: opts.cwd, - }); + // A newly launched client on the shared display can grab input focus — + // gated the same as direct input, so it can never land while a human + // holds the takeover lease (pickforge/picklab#21 P1-E). + const app = await withAgentPermit(id, process.env, () => + launchApp({ + display, + command, + args, + logDir: desktopSessionLogDir(id), + cwd: opts.cwd, + }), + ); const data: Record = { sessionId: id, display, diff --git a/packages/cli/test/desktop-input-takeover.test.ts b/packages/cli/test/desktop-input-takeover.test.ts index c3643af..70e0aeb 100644 --- a/packages/cli/test/desktop-input-takeover.test.ts +++ b/packages/cli/test/desktop-input-takeover.test.ts @@ -8,7 +8,13 @@ import { releaseHumanLease, type EnvLike, } from "@pickforge/picklab-core"; -import { runDesktopClick, runDesktopType, runDesktopKey } from "../src/commands/desktop.js"; +import { + runDesktopClick, + runDesktopLaunch, + runDesktopScreenshot, + runDesktopType, + runDesktopKey, +} from "../src/commands/desktop.js"; let root: string; let env: EnvLike; @@ -66,4 +72,28 @@ describe("desktop input commands fail closed under human control", () => { await releaseHumanLease(id, lease.leaseId, env); }); + + it("rejects desktop launch (a new client can grab focus on the shared display)", async () => { + const id = await createDesktop(); + const lease = await acquireHumanLease(id, env); + + expect( + await runDesktopLaunch("xterm", [], { session: id, projectDir: root, json: true }), + ).toBe(1); + expect(lastReport().errors[0]).toContain("human control is active"); + + await releaseHumanLease(id, lease.leaseId, env); + }); + + it("does not gate desktop screenshot (read-only)", async () => { + const id = await createDesktop(); + const lease = await acquireHumanLease(id, env); + + await runDesktopScreenshot({ session: id, projectDir: root, json: true }); + // Fails only for lack of a real display/screenshot tool in this + // environment, never because of the human lease. + expect(lastReport().errors.join("\n")).not.toContain("human control is active"); + + await releaseHumanLease(id, lease.leaseId, env); + }); }); diff --git a/packages/mcp-server/src/tools/desktop.ts b/packages/mcp-server/src/tools/desktop.ts index c9074d8..3c3aa07 100644 --- a/packages/mcp-server/src/tools/desktop.ts +++ b/packages/mcp-server/src/tools/desktop.ts @@ -1,6 +1,7 @@ import path from "node:path"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { z } from "zod"; +import { withAgentPermit } from "@pickforge/picklab-core"; import { click, desktopSessionLogDir, @@ -100,17 +101,22 @@ export function registerDesktopTools( target: { name: args.command }, }, async () => { - const app = await launchApp({ - display, - command: args.command, - args: args.args ?? [], - env: ctx.env, - logDir: desktopSessionLogDir(id, ctx.env), - cwd: - args.cwd === undefined - ? undefined - : path.resolve(ctx.projectDir, args.cwd), - }); + // A newly launched client on the shared display can grab input + // focus — gated the same as direct input, so it can never land + // while a human holds the takeover lease (pickforge/picklab#21 P1-E). + const app = await withAgentPermit(id, ctx.env, () => + launchApp({ + display, + command: args.command, + args: args.args ?? [], + env: ctx.env, + logDir: desktopSessionLogDir(id, ctx.env), + cwd: + args.cwd === undefined + ? undefined + : path.resolve(ctx.projectDir, args.cwd), + }), + ); const data: Record = { sessionId: id, display, diff --git a/packages/mcp-server/test/takeover.test.ts b/packages/mcp-server/test/takeover.test.ts index 5374865..d15e926 100644 --- a/packages/mcp-server/test/takeover.test.ts +++ b/packages/mcp-server/test/takeover.test.ts @@ -109,4 +109,38 @@ describe("desktop input tools fail closed under human control", () => { expect(keyed.ok).toBe(false); expect(keyed.errors.join("\n")).toContain("human control is active"); }); + + it("rejects desktop_launch too (a new client can grab focus on the shared display)", async () => { + dirs = makeLabDirs(); + const env = { PICKLAB_HOME: dirs.home }; + lab = await connectLab({ projectDir: dirs.projectDir, env }); + const id = writeDesktopSessionRecord(dirs.home, dirs.projectDir); + const lease = await acquireHumanLease(id, env); + + const launched = parseToolJson( + await lab.client.callTool({ + name: "desktop_launch", + arguments: { session: id, command: "xterm" }, + }), + ); + expect(launched.ok).toBe(false); + expect(launched.errors.join("\n")).toContain("human control is active"); + + await releaseHumanLease(id, lease.leaseId, env); + }); + + it("leaves desktop_screenshot ungated (read-only, no input delivered)", async () => { + dirs = makeLabDirs(); + const env = { PICKLAB_HOME: dirs.home }; + lab = await connectLab({ projectDir: dirs.projectDir, env }); + const id = writeDesktopSessionRecord(dirs.home, dirs.projectDir); + await acquireHumanLease(id, env); + + const result = parseToolJson( + await lab.client.callTool({ name: "desktop_screenshot", arguments: { session: id } }), + ); + // Not gated: it fails only because there is no real display/screenshot + // tool in this test environment, never because of the human lease. + expect(result.errors.join("\n")).not.toContain("human control is active"); + }); }); From 2709302c380242d8bca802390b8de805ec8cfe5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 11:02:59 -0300 Subject: [PATCH 5/5] docs: document takeover hardening round (active reclaim, desktop_launch gating) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update the README's takeover section and security model bullet for the active (not lazy-only) crash-path VNC reclaim, the immediate-end and hard-deadline-timer mechanisms, and desktop_launch joining the gated tool set with desktop_screenshot as the only ungated one. Record the same in docs/releases/UNRELEASED.md, plus the evidencing asymmetry between a blocked MCP desktop tool call (evidenced) and a blocked DevTools relay request (not — no equivalent per-call evidence lifecycle to hook). --- README.md | 20 +++++++++++++------- docs/releases/UNRELEASED.md | 7 ++++++- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 40c7ff6..b3dd1fa 100644 --- a/README.md +++ b/README.md @@ -171,16 +171,22 @@ grants a temporary writable VNC viewer for a human, and hands control back with a fresh screenshot and an evidence record once the viewer closes (or the terminal is interrupted). Unlike `--vnc-control`'s persistent writable session, control here is leased: while a human holds it, every desktop input -call and every DevTools relay request fails closed with a stable busy error — +tool (`desktop_click`/`move`/`scroll`/`drag`/`double_click`/`type`/`key`), +`desktop_launch` (a newly launched client could otherwise grab input focus), +and every DevTools relay request fail closed with a stable busy error — `takeover_status` (MCP) / `picklab takeover status` (CLI) let an agent check before retrying, and `request_user_input` is the recommended way to ask a -human to run it. +human to run it. `desktop_screenshot` is the only desktop tool left ungated +(read-only). The lease is a 30-second TTL, heartbeat-renewed-every-5-seconds record in the session's state directory. Closing the viewer, an interrupted terminal, or a -PickLab crash all end up releasing it and reverting VNC to read-only — a -crash is recovered automatically the next time the session's VNC is touched -(e.g. a later `picklab watch`), never left writable indefinitely. +PickLab crash all release it and revert VNC to read-only. A crash of the +`watch --control` process itself is reclaimed *actively*, not only the next +time something else happens to touch the session: a detached watchdog +process, spawned alongside the takeover and immune to a `SIGKILL` of its +parent, polls the lease and stops a stale writable VNC on its own — writable +VNC does not survive its lease going stale, whichever side crashes. ### Concurrent sessions @@ -292,7 +298,7 @@ reported as suppressed for an explicitly writable `--vnc-control` session. `picklab mcp serve` exposes 27 tools over stdio: - Sessions: `session_create`, `session_status`, `session_destroy` -- Desktop: `desktop_launch`, `desktop_screenshot`, `desktop_click`, `desktop_move`, `desktop_scroll`, `desktop_drag`, `desktop_double_click`, `desktop_type`, `desktop_key` — the seven input tools fail closed with a busy error while a human lease is active +- Desktop: `desktop_launch`, `desktop_screenshot`, `desktop_click`, `desktop_move`, `desktop_scroll`, `desktop_drag`, `desktop_double_click`, `desktop_type`, `desktop_key` — all fail closed with a busy error while a human lease is active except `desktop_screenshot` (read-only). `desktop_launch` is gated too: a newly launched client can grab input focus on the shared display, which is exactly what the lease protects against. - Android: `android_start`, `android_install_apk`, `android_launch_app`, `android_screenshot`, `android_tap`, `android_type`, `android_back`, `android_home`, `android_get_ui_tree`, `android_logcat`, `android_run_adb` - Artifacts: `artifact_list`, `artifact_report` - Takeover: `takeover_status` — check whether a session is under human control (see [Supervised pause and human takeover](#supervised-pause-and-human-takeover)); read-only, always safe to call @@ -333,7 +339,7 @@ A TypeScript monorepo. `@pickforge/picklab` is the published package; the rest a - All user inputs are spawned as argument arrays — never interpolated into shell strings. - The DevTools relay validates the installed upstream package name, exact version, declared bin, and confined real path before spawning Node with an argument array. Its browser URL is always derived as `http://127.0.0.1:`. - Relay stdout is protocol-only. A pending JSON-RPC record is capped at 16 MiB. Upstream diagnostic lines are capped at 64 KiB, redacted, and forwarded only to stderr; an over-limit line is dropped with a safe notice. Upstream update checks and usage statistics are disabled. -- VNC binds to loopback only by default: `x11vnc` is started with `-localhost`, so the server listens on `127.0.0.1` and is not reachable from the network. Tunnel over SSH for remote access. Normal `--vnc` and `picklab watch` observation is server-enforced read-only (`-viewonly`); viewer exit never stops the session or its Xvfb/VNC processes. `--vnc-control` is an explicit, persistent writable escape hatch for human secret entry and does not coordinate with agent input. `picklab watch --control` is the coordinated alternative: an atomic, TTL-bounded lease gates a temporary writable VNC server, and every agent desktop-input call and DevTools relay request fails closed (a live human lease is checked immediately before delivery) for as long as it is held; a crash on either side is recovered — writable VNC never outlives its lease. +- VNC binds to loopback only by default: `x11vnc` is started with `-localhost`, so the server listens on `127.0.0.1` and is not reachable from the network. Tunnel over SSH for remote access. Normal `--vnc` and `picklab watch` observation is server-enforced read-only (`-viewonly`); viewer exit never stops the session or its Xvfb/VNC processes. `--vnc-control` is an explicit, persistent writable escape hatch for human secret entry and does not coordinate with agent input. `picklab watch --control` is the coordinated alternative: an atomic, TTL-bounded lease gates a temporary writable VNC server, and every agent desktop-input call (including `desktop_launch`, which could otherwise grab input focus on the shared display) and DevTools relay request fails closed (a live human lease is checked immediately before delivery) for as long as it is held. A crash on either side is reclaimed actively — the controlling process force-ends on the first failed lease renewal (never waiting for the viewer to close) and carries a hard deadline timer at the lease's `expiresAt` as a backstop; a detached watchdog process, immune to a `SIGKILL` of its parent, independently polls and stops a stale writable VNC. Writable VNC never outlives its lease in wall-clock terms, on any exit path. - Artifacts are redacted by default: logcat output strips tokens and secrets before it is stored or returned. Only `android adb` is raw, and it says so. - Evidence timelines persist only allowlisted metadata; typed values become length/type metadata, and network headers, bodies, and URL queries are dropped. Static HTML reports escape page-controlled text and use a no-script, no-network CSP. - Screenshot files contain raw pixels and cannot be redacted. Avoid explicit captures on screens containing secrets, and use `evidence.enabled: false` when an action timeline is not appropriate. See [SECURITY.md](SECURITY.md#recorded-evidence-and-screenshots). diff --git a/docs/releases/UNRELEASED.md b/docs/releases/UNRELEASED.md index b0fca97..93bfb85 100644 --- a/docs/releases/UNRELEASED.md +++ b/docs/releases/UNRELEASED.md @@ -153,7 +153,12 @@ release description, then reset it after the release is published. refusing outright, while still refusing while a live lease holds it. The DevTools relay gained a generic NDJSON `intercept` hook (answers a request directly on the response stream instead of forwarding it to the child) used - for the same busy-error contract. + for the same busy-error contract; its writes are funneled through a shared + write queue (`createJsonRpcWriteQueue`) alongside the normal child-response + pump so a busy rejection and an in-flight forwarded response can never + interleave on the wire. A blocked relay request is not recorded as an + evidence action, unlike a blocked MCP desktop-input tool call — the relay + has no equivalent per-call evidence lifecycle to hook. - Added atomic, crash-recoverable evidence journals, active-run ownership, truncation markers, report publication, and symlink-safe resource access. - Hardened Android and evidence cleanup around process identity, atomic writes,