diff --git a/CHANGELOG.md b/CHANGELOG.md index 582515c7..b3d4a294 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,23 @@ This file is maintained by hand. There is no automated changelog tooling (see `docs/CI.md#release-policy` for why) — update this file as part of the commit that bumps the package versions for a release. +## Unreleased + +- **`config.json`'s `wssPort` accepts `0`, meaning "bind an OS-assigned port".** The pinned-wss + listener takes whatever ephemeral port the OS hands it, and everything that reports or advertises + the port — `daemon.status`'s `wssPort`, a minted link's `endpoint.port`, and so the deep link and + QR code composed from it — carries the *bound* port rather than the configured `0`. This lets + several daemons (separate state dirs) coexist on one machine without an operator hand-picking a + port for each. Every other value must still be a port number in `1..65535`; the default is + unchanged at `8443`. + +- **Fixed: a zombie daemon process no longer blocks pidfile takeover.** `process.kill(pid, 0)` + succeeds for an exited-but-unreaped process, so a daemon that was killed after its parent CLI had + exited could keep its pidfile looking live — in containers whose PID 1 does not reap, for the + life of the container, leaving every later command reporting a daemon that was already dead. The + liveness probe now also reads `/proc//status` on Linux and treats `State: Z` as dead; + everywhere `/proc` is absent or unreadable the previous behaviour is unchanged. + ## 0.10.0 (2026-09-16) - **New: native SDKs for apps without React Native.** The same Appduct core the React Native diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 2da6e841..75b6c536 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -101,6 +101,13 @@ The daemon refuses to load a key file that is group/world-readable. } ``` +`wssPort` is the pinned-wss listener's TCP port. **`0` binds an OS-assigned port**: the listener +takes whatever ephemeral port the OS hands it, and everything that reports or advertises the port +afterwards — `daemon.status`'s `wssPort` (§5) and a minted link's `endpoint.port` (§5, §8) — carries +the *bound* port, never the configured `0`. That is how several daemons coexist on one machine +without an operator hand-picking a port for each (the test suite's daemons all run this way). +Any other value must be a port number in `1..65535`. + `advertisedIp` overrides auto-detection of the address advertised in minted bootstrap payloads. `scheme` is the deep-link URI scheme composed into `appduct link`'s output when `--scheme` is not passed (§10) — set it once here instead of on every invocation. @@ -153,7 +160,19 @@ only-when-no-sessions-are-live (§4, "Version drift"). (3) polls the socket until ready (timeout 5 s), (4) retries the original request. A stale socket file with a dead pid is unlinked before spawning. - Single instance is enforced via the pidfile (write with `O_EXCL`; on conflict, check - liveness with `process.kill(pid, 0)` and take over only if dead). + liveness and take over only if dead). Liveness is `process.kill(pid, 0)` — with `EPERM` counted + as alive — plus, on Linux, a `/proc//status` read that treats `State: Z` (zombie) as **dead**. + A zombie is an exited process nobody has reaped: it still holds a pid table entry, so + `process.kill(pid, 0)` succeeds for it, but the daemon it names is gone and its socket is closed. + Normally that window is invisible because PID 1 reaps orphans immediately; in a container whose + PID 1 is a plain command rather than an init, nothing reaps, and a daemon killed after its parent + CLI exited stays a zombie for the life of the container — without this check the pidfile would + never look stale and every later command would report a daemon that is already dead. The procfs + read only ever adds a "dead" verdict on positive evidence: where `/proc` is absent or unreadable + (macOS, a hardened container, a pid we lack permission on) `process.kill(pid, 0)`'s answer stands, + because wrongly declaring a *live* daemon dead would clobber its state. The same probe + (`isProcessAlive`, `daemon/pidfile.ts`) answers every other "is a daemon still there?" question — + the auto-spawn path's stale-socket unlink and `daemon.log` rotation — so all three agree. - SIGINT/SIGTERM: close all device sockets with code 1001, remove `daemon.sock` and `daemon.pid`, flush audit, exit 0. - **Version drift:** the daemon outlives the CLI that spawned it, so `npm i -g appduct@` diff --git a/packages/appduct/src/__tests__/__snapshots__/mcp-server.integration.test.ts.snap b/packages/appduct/src/__tests__/__snapshots__/mcp-server.test.ts.snap similarity index 100% rename from packages/appduct/src/__tests__/__snapshots__/mcp-server.integration.test.ts.snap rename to packages/appduct/src/__tests__/__snapshots__/mcp-server.test.ts.snap diff --git a/packages/appduct/src/__tests__/cli-v2.integration.test.ts b/packages/appduct/src/__tests__/cli-v2.integration.test.ts index 881dcd8f..50ee9744 100644 --- a/packages/appduct/src/__tests__/cli-v2.integration.test.ts +++ b/packages/appduct/src/__tests__/cli-v2.integration.test.ts @@ -7,9 +7,6 @@ * `tool-invocation.integration.test.ts`, just through the CLI instead of raw UDS RPC. */ -import { createServer as createNetServer } from "node:net"; -import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; import path from "node:path"; import { text } from "node:stream/consumers"; @@ -18,7 +15,12 @@ import WebSocket from "ws"; import { decodeBootstrap } from "@appduct/shared"; -import { spawnCliBinary, waitForExit, writeTestHostKey } from "./fixtures.js"; +import { + makeTempStateDir as makeSharedStateDir, + removeStateDir, + spawnCliBinary, + waitForExit, +} from "./fixtures.js"; // The fake app client below skips pinning (that is the app SDK's job, exercised in // session-engine.integration.test.ts); the leaf-cert check is disabled process-wide for this @@ -50,30 +52,14 @@ afterEach(async () => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); -const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - const makeTempStateDir = async (configOverrides: Record = {}): Promise => { - const directory = await mkdtemp(path.join(tmpdir(), "appduct-cli-v2-")); - await writeTestHostKey(path.join(directory, "key.pem")); - - const port = await pickFreePort(); - await writeFile( - path.join(directory, "config.json"), - JSON.stringify({ wssPort: port, advertisedIp: "127.0.0.1", scheme: "appduct-e2e", ...configOverrides }), + const directory = await makeSharedStateDir( + { scheme: "appduct-e2e", ...configOverrides }, + { prefix: "appduct-cli-v2-" }, ); stateDirs.push(directory); @@ -132,7 +118,6 @@ describe("appduct CLI v2: end-to-end command table", () => { "keygen -> ls auto-spawns -> link -> claim -> ls ACTIVE -> tools/invoke round-trip -> revoke", async () => { const stateDir = await makeTempStateDir(); - const port = JSON.parse(await readFile(path.join(stateDir, "config.json"), "utf8")).wssPort as number; // keygen: fully non-interactive, refuses to overwrite without --force. const keygenPath = path.join(stateDir, "operator-key.pem"); @@ -152,6 +137,10 @@ describe("appduct CLI v2: end-to-end command table", () => { const status = await runCliJson(["daemon", "status"], stateDir); expect(status.ok).toBe(true); daemonPids.push((status.data as { daemon: { pid: number } }).daemon.pid); + // The state dir asks for an OS-assigned wss port (`wssPort: 0`), so the number is only + // knowable from the running daemon — which is also what a link must end up advertising. + const port = (status.data as { daemon: { wss_port: number } }).daemon.wss_port; + expect(port).toBeGreaterThan(0); // link: mint a pending session and decode its deep link. const linkResult = await runCliJson(["link", "--ttl", "60"], stateDir); @@ -274,7 +263,8 @@ describe("appduct CLI v2: end-to-end command table", () => { expect(status.ok).toBe(true); daemonPids.push((status.data as { daemon: { pid: number } }).daemon.pid); - const port = JSON.parse(await readFile(path.join(stateDir, "config.json"), "utf8")).wssPort as number; + const port = (status.data as { daemon: { wss_port: number } }).daemon.wss_port; + expect(port).toBeGreaterThan(0); const claimOne = async (deviceModel: string): Promise<{ socket: WebSocket; alias: string }> => { const linkResult = await runCliJson(["link", "--ttl", "60"], stateDir); diff --git a/packages/appduct/src/__tests__/daemon-cli.integration.test.ts b/packages/appduct/src/__tests__/daemon-cli.integration.test.ts index 525e5325..a72d17b8 100644 --- a/packages/appduct/src/__tests__/daemon-cli.integration.test.ts +++ b/packages/appduct/src/__tests__/daemon-cli.integration.test.ts @@ -1,17 +1,24 @@ -import { mkdtemp, rm, stat } from "node:fs/promises"; -import { tmpdir } from "node:os"; +import { stat } from "node:fs/promises"; import path from "node:path"; import { afterEach, describe, expect, test } from "vitest"; -import { runCliBinary, spawnCliBinary, waitForExit, writeTestHostKey } from "./fixtures.js"; +import { + makeTempStateDir as makeSharedStateDir, + removeStateDir, + runCliBinary, + spawnCliBinary, + waitForExit, +} from "./fixtures.js"; const stateDirs: string[] = []; const daemonPids: number[] = []; +/** The shared fixture's `wssPort: 0` matters here: every case below auto-spawns or runs a real + * daemon, and this file used to write no `config.json` at all, so each one bound the default 8443 + * and collided with any other daemon on the machine. */ const makeTempStateDir = async (): Promise => { - const directory = await mkdtemp(path.join(tmpdir(), "appduct-daemon-cli-")); - await writeTestHostKey(path.join(directory, "key.pem")); + const directory = await makeSharedStateDir({}, { prefix: "appduct-daemon-cli-" }); stateDirs.push(directory); return directory; }; @@ -38,8 +45,7 @@ afterEach(async () => { } while (stateDirs.length > 0) { - const directory = stateDirs.pop()!; - await rm(directory, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); diff --git a/packages/appduct/src/__tests__/daemon.integration.test.ts b/packages/appduct/src/__tests__/daemon.integration.test.ts new file mode 100644 index 00000000..373e66a9 --- /dev/null +++ b/packages/appduct/src/__tests__/daemon.integration.test.ts @@ -0,0 +1,457 @@ +/** + * Daemon lifecycle against a **real, running daemon**: `startDaemon` takes the pidfile, mints TLS + * material, binds an OS-assigned wss port (ARCHITECTURE.md §3's `wssPort: 0`) and serves the UDS + * control socket, and every case here drives that socket for real. + * + * Split out of `daemon.test.ts`, which keeps the cases that need none of it (config parsing, the + * line-length cap, `daemon status` against a hand-rolled legacy daemon). + */ + +import { spawnSync } from "node:child_process"; +import { connect, type Socket } from "node:net"; +import { readdir, readFile, stat, writeFile } from "node:fs/promises"; +import path from "node:path"; + +import { afterEach, describe, expect, test } from "vitest"; + +import { decodeBootstrap } from "@appduct/shared"; + +import { AUDIT_PRUNE_INTERVAL_MS, startDaemon, type RunningDaemon } from "../daemon/daemon.js"; +import { DaemonAlreadyRunningError } from "../daemon/pidfile.js"; +import { getStateDirPaths } from "../daemon/state-dir.js"; +import { systemTimers, type IntervalHandle, type TimerFns } from "../daemon/timers.js"; +import { makeTempStateDir as makeSharedStateDir, removeStateDir } from "./fixtures.js"; + +const runningDaemons: RunningDaemon[] = []; + +const startTrackedDaemon = async (stateDir: string): Promise => { + const daemon = await startDaemon({ stateDir }); + runningDaemons.push(daemon); + return daemon; +}; + +afterEach(async () => { + while (runningDaemons.length > 0) { + const daemon = runningDaemons.pop(); + await daemon?.shutdown(); + } +}); + +/** + * The shared fixture writes `wssPort: 0` ("bind an OS-assigned port", ARCHITECTURE.md §3). This + * file used to write no `config.json` at all, so every daemon it started bound the default 8443 + * and collided with any other daemon on the machine — including the ones a second vitest process + * (another worktree, another agent session) is running at the same time. + */ +const makeTempStateDir = async (): Promise => { + return makeSharedStateDir({}, { prefix: "appduct-daemon-test-" }); +}; + +/** Reads newline-delimited JSON-RPC responses off a raw socket, resolving each awaited line. */ +const createLineReader = (socket: Socket) => { + let buffer = ""; + const pendingLines: string[] = []; + const waiters: Array<(line: string) => void> = []; + + socket.on("data", (chunk: Buffer) => { + buffer += chunk.toString("utf8"); + let newlineIndex = buffer.indexOf("\n"); + + while (newlineIndex !== -1) { + const line = buffer.slice(0, newlineIndex); + buffer = buffer.slice(newlineIndex + 1); + newlineIndex = buffer.indexOf("\n"); + + const waiter = waiters.shift(); + if (waiter) { + waiter(line); + } else { + pendingLines.push(line); + } + } + }); + + return { + nextLine: (): Promise => { + const buffered = pendingLines.shift(); + if (buffered !== undefined) { + return Promise.resolve(buffered); + } + + return new Promise((resolve) => { + waiters.push(resolve); + }); + }, + }; +}; + +const connectRaw = (socketPath: string): Promise => { + return new Promise((resolve, reject) => { + const socket = connect(socketPath); + socket.once("connect", () => resolve(socket)); + socket.once("error", reject); + }); +}; + +describe("daemon lifecycle", () => { + test("daemon.status round-trips over the real UDS", async () => { + const stateDir = await makeTempStateDir(); + const daemon = await startTrackedDaemon(stateDir); + + const paths = getStateDirPaths(stateDir); + expect((await stat(paths.root)).mode & 0o777).toBe(0o700); + expect((await stat(paths.socketPath)).mode & 0o777).toBe(0o600); + // `audit/` must be tightened explicitly: `mkdir` honors the process umask, which on a common + // 0o022 umask would otherwise leave it drwxr-xr-x (ARCHITECTURE.md §3: mode 0700). + expect((await stat(paths.auditDir)).mode & 0o777).toBe(0o700); + + const socket = await connectRaw(paths.socketPath); + const reader = createLineReader(socket); + + socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 1, method: "daemon.status", params: {} })}\n`); + const line = await reader.nextLine(); + const response = JSON.parse(line); + + expect(response.id).toBe(1); + expect(response.result).toMatchObject({ + pid: process.pid, + sessions: [], + }); + // The state dir asks for an OS-assigned port, so the only correct assertion is that the + // *bound* port is reported — a status echoing the configured `0` back would be reporting the + // one number no app can ever connect to. + expect(response.result.wssPort).toBeGreaterThan(0); + expect(Number.isInteger(response.result.wssPort)).toBe(true); + expect(response.result.wssPort).toBe(daemon.listener.port()); + expect(response.result.pinnedKeys).toHaveLength(1); + expect(response.result.pinnedKeys[0]).toMatch(/^sha256\//u); + expect(response.result.version).toBeTypeOf("string"); + expect(response.result.startedAt).toBe(daemon.startedAt.toISOString()); + + socket.destroy(); + await removeStateDir(stateDir); + }); + + test("wssPort: 0 binds an OS-assigned port, and both daemon.status and a minted link carry it", async () => { + const stateDir = await makeTempStateDir(); + const daemon = await startTrackedDaemon(stateDir); + const paths = getStateDirPaths(stateDir); + + const bound = daemon.listener.port(); + expect(bound).toBeGreaterThan(0); + + const socket = await connectRaw(paths.socketPath); + const reader = createLineReader(socket); + + socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 1, method: "daemon.status", params: {} })}\n`); + const status = JSON.parse(await reader.nextLine()); + expect(status.result.wssPort).toBe(bound); + + // The link is the part that actually matters to an app: a bootstrap payload advertising the + // configured `0` would be undialable, and nothing downstream could tell it from a real port. + socket.write( + `${JSON.stringify({ jsonrpc: "2.0", id: 2, method: "link.create", params: { ttlSeconds: 60 } })}\n`, + ); + const link = JSON.parse(await reader.nextLine()); + expect(link.result.endpoint.port).toBe(bound); + + const decoded = decodeBootstrap(link.result.deepLinkPayload); + expect(decoded).not.toBeNull(); + expect(decoded!.port).toBe(bound); + + // And the bound port is genuinely reachable — `0` was a request, not a literal bind. A plain + // TCP connect is enough to prove that, and it is all this case is about: what the listener + // does with the connection (TLS, pinning, the wire protocol) has its own tests. + const probe = await new Promise((resolve, reject) => { + const connection = connect({ host: "127.0.0.1", port: bound! }, () => resolve(connection)); + connection.once("error", reject); + }); + probe.destroy(); + + socket.destroy(); + await removeStateDir(stateDir); + }); + + test("malformed JSON line gets a JSON-RPC parse error and the connection stays usable", async () => { + const stateDir = await makeTempStateDir(); + await startTrackedDaemon(stateDir); + const paths = getStateDirPaths(stateDir); + + const socket = await connectRaw(paths.socketPath); + const reader = createLineReader(socket); + + socket.write("{ not json \n"); + const badLineResponse = JSON.parse(await reader.nextLine()); + expect(badLineResponse.error.code).toBe(-32700); + + socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 7, method: "daemon.status" })}\n`); + const goodLineResponse = JSON.parse(await reader.nextLine()); + expect(goodLineResponse.id).toBe(7); + expect(goodLineResponse.result.pid).toBe(process.pid); + + socket.destroy(); + await removeStateDir(stateDir); + }); + + test("unknown method returns JSON-RPC -32601", async () => { + const stateDir = await makeTempStateDir(); + await startTrackedDaemon(stateDir); + const paths = getStateDirPaths(stateDir); + + const socket = await connectRaw(paths.socketPath); + const reader = createLineReader(socket); + + socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 3, method: "nonexistent.method" })}\n`); + const response = JSON.parse(await reader.nextLine()); + + expect(response.error.code).toBe(-32601); + + socket.destroy(); + await removeStateDir(stateDir); + }); + + test("daemon.shutdown acks then closes the socket and removes sock + pid files", async () => { + const stateDir = await makeTempStateDir(); + const daemon = await startTrackedDaemon(stateDir); + runningDaemons.pop(); // shutting down manually below; don't double-shutdown in afterEach. + const paths = getStateDirPaths(stateDir); + + const socket = await connectRaw(paths.socketPath); + const reader = createLineReader(socket); + + socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 9, method: "daemon.shutdown" })}\n`); + const response = JSON.parse(await reader.nextLine()); + expect(response.result).toEqual({ ok: true }); + + await daemon.exited; + + await expect(stat(paths.socketPath)).rejects.toThrow(); + await expect(stat(paths.pidFilePath)).rejects.toThrow(); + + socket.destroy(); + await removeStateDir(stateDir); + }); + + test("second daemon against the same state dir throws DaemonAlreadyRunningError", async () => { + const stateDir = await makeTempStateDir(); + const first = await startTrackedDaemon(stateDir); + + await expect(startDaemon({ stateDir })).rejects.toThrow(DaemonAlreadyRunningError); + + await first.shutdown(); + await removeStateDir(stateDir); + }); + + test("takes over a stale pidfile and stale socket left by a dead process", async () => { + const stateDir = await makeTempStateDir(); + const paths = getStateDirPaths(stateDir); + + // A genuinely-dead pid: spawn a no-op child and wait for it to exit. + const dead = spawnSync(process.execPath, ["-e", "process.exit(0)"]); + expect(dead.status).toBe(0); + const deadPid = dead.pid; + expect(deadPid).toBeGreaterThan(0); + + await writeFile(paths.pidFilePath, String(deadPid), { mode: 0o600 }); + // A stale socket file (not actually listening) left behind by the "crashed" daemon. + await writeFile(paths.socketPath, "", { mode: 0o600 }); + + const daemon = await startTrackedDaemon(stateDir); + + expect(Number((await readFile(paths.pidFilePath, "utf8")).trim())).toBe(process.pid); + expect((await stat(paths.socketPath)).mode & 0o777).toBe(0o600); + + // Confirm the socket now actually answers RPC (proof the stale placeholder file was replaced). + const socket = await connectRaw(paths.socketPath); + const reader = createLineReader(socket); + socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 1, method: "daemon.status" })}\n`); + const response = JSON.parse(await reader.nextLine()); + expect(response.result.pid).toBe(process.pid); + + socket.destroy(); + void daemon; + await removeStateDir(stateDir); + }); + + test("config.json unknown keys warn instead of throwing", async () => { + const stateDir = await makeTempStateDir(); + const paths = getStateDirPaths(stateDir); + const { mkdir } = await import("node:fs/promises"); + await mkdir(stateDir, { recursive: true }); + await writeFile(paths.configPath, JSON.stringify({ totallyUnknownKey: true, wssPort: 0 })); + + const warnings: string[] = []; + const daemon = await startTrackedDaemon(stateDir); + void daemon; + + // Re-load directly to also exercise the warn callback in isolation from the running daemon. + const { loadConfig } = await import("../daemon/config.js"); + const config = await loadConfig(getStateDirPaths(stateDir), { + warn: (message) => warnings.push(message), + }); + + expect(config.wssPort).toBe(0); + expect(warnings.some((message) => message.includes("totallyUnknownKey"))).toBe(true); + + await removeStateDir(stateDir); + }); + + test("config.json iosBundleId is a known key, loaded as-is and validated as a non-empty string", async () => { + const stateDir = await makeTempStateDir(); + const paths = getStateDirPaths(stateDir); + const { mkdir } = await import("node:fs/promises"); + await mkdir(stateDir, { recursive: true }); + const { loadConfig } = await import("../daemon/config.js"); + + await writeFile(paths.configPath, JSON.stringify({ iosBundleId: "com.example.playground" })); + + const warnings: string[] = []; + const config = await loadConfig(paths, { warn: (message) => warnings.push(message) }); + + expect(config.iosBundleId).toBe("com.example.playground"); + // A key that warned as unknown would still "work" via the `--bundle-id` flag, hiding a typo'd + // config from the operator for as long as they only ever passed the flag. + expect(warnings).toEqual([]); + + await writeFile(paths.configPath, JSON.stringify({ iosBundleId: "" })); + await expect(loadConfig(paths)).rejects.toThrow(/iosBundleId/u); + + await writeFile(paths.configPath, JSON.stringify({ iosBundleId: 42 })); + await expect(loadConfig(paths)).rejects.toThrow(/iosBundleId/u); + + await writeFile(paths.configPath, JSON.stringify({ iosBundleId: "com.example.my-app2" })); + await expect(loadConfig(paths)).resolves.toMatchObject({ iosBundleId: "com.example.my-app2" }); + + // Deliberately *not* charset-checked here, only where the value is used. This loader runs on + // every daemon start, so a typo in a CLI-side convenience key must not stop the daemon from + // starting — it surfaces as a usage error against the `link`/`connect` call that needed it. + // `wssPort: 0` because this line's config is the one the daemon below actually starts on. + await writeFile(paths.configPath, JSON.stringify({ iosBundleId: "--console", wssPort: 0 })); + await expect(loadConfig(paths)).resolves.toMatchObject({ iosBundleId: "--console" }); + await expect(startTrackedDaemon(stateDir)).resolves.toBeDefined(); + + await removeStateDir(stateDir); + }); +}); + + +/** + * Retention wiring (ARCHITECTURE.md §3, issue #32). The audit *policy* — which files are stale, how + * the day boundary is computed — is covered by `audit-retention.test.ts`; what matters here is that + * a real daemon actually runs it at startup, keeps running it on the daily seam, reports the + * footprint over RPC, and does not leave the timer behind on shutdown. + */ +describe("daemon: audit retention", () => { + /** Interval-only fake: `startDaemon` uses its `timers` seam solely for the daily audit sweep, so + * everything else (session grace/keepalive, listener pre-claim) keeps running on real timers. */ + const createIntervalRecorder = (): { + timers: TimerFns; + intervals: Array<{ callback: () => void; ms: number; cleared: boolean }>; + } => { + const intervals: Array<{ callback: () => void; ms: number; cleared: boolean }> = []; + + return { + intervals, + timers: { + ...systemTimers, + setInterval: (callback, ms) => { + const record = { callback, ms, cleared: false }; + intervals.push(record); + return record as unknown as IntervalHandle; + }, + clearInterval: (handle) => { + (handle as unknown as { cleared: boolean }).cleared = true; + }, + }, + }; + }; + + const writeDayFile = async (auditDir: string, stamp: string): Promise => { + await writeFile(path.join(auditDir, `${stamp}.jsonl`), "{}\n", { mode: 0o600 }); + }; + + /** Both sweeps are fire-and-forget (`void auditLogger.prune()`), so the assertion polls rather + * than sleeping on a guessed duration. */ + const waitForAuditDir = async (auditDir: string, expected: string[]): Promise => { + const deadline = Date.now() + 2000; + + for (;;) { + const names = (await readdir(auditDir)).sort(); + + if (names.join(",") === expected.join(",") || Date.now() > deadline) { + expect(names).toEqual(expected); + return; + } + + await new Promise((resolve) => setTimeout(resolve, 10)); + } + }; + test("prunes stale day files at startup, again on the daily timer, and clears the timer on shutdown", async () => { + const stateDir = await makeTempStateDir(); + const paths = getStateDirPaths(stateDir); + const { mkdir } = await import("node:fs/promises"); + await mkdir(paths.auditDir, { recursive: true }); + await writeFile(paths.configPath, JSON.stringify({ auditRetentionDays: 7, wssPort: 0 })); + + await writeDayFile(paths.auditDir, "2026-09-05"); // today per the clock below + await writeDayFile(paths.auditDir, "2026-08-01"); // stale at startup + + const { timers, intervals } = createIntervalRecorder(); + let now = new Date("2026-09-05T12:00:00.000Z"); + const daemon = await startDaemon({ stateDir, timers, clock: { now: () => now } }); + runningDaemons.push(daemon); + + await waitForAuditDir(paths.auditDir, ["2026-09-05.jsonl"]); + + expect(intervals).toHaveLength(1); + expect(intervals[0]!.ms).toBe(AUDIT_PRUNE_INTERVAL_MS); + + // A day later the daemon is still running and the sweep still fires — the file that was + // "today" at startup is now stale enough to go. + await writeDayFile(paths.auditDir, "2026-09-14"); + now = new Date("2026-09-14T12:00:00.000Z"); + intervals[0]!.callback(); + await waitForAuditDir(paths.auditDir, ["2026-09-14.jsonl"]); + + await daemon.shutdown(); + expect(intervals[0]!.cleared).toBe(true); + + await removeStateDir(stateDir); + }); + + test("daemon.status reports the audit footprint and the effective retention", async () => { + const stateDir = await makeTempStateDir(); + const paths = getStateDirPaths(stateDir); + const { mkdir } = await import("node:fs/promises"); + await mkdir(paths.auditDir, { recursive: true }); + await writeFile(paths.configPath, JSON.stringify({ auditRetentionDays: 45, wssPort: 0 })); + + // Clock-injected, and the fixture's name is derived from it, for two reasons: the file must + // be dated relative to the daemon's own idea of "today" rather than the calendar the suite + // happens to run on, and naming it *today* makes it immune to the fire-and-forget startup + // sweep that may still be in flight — today's file is the one file pruning can never take. + const now = new Date("2026-09-05T12:00:00.000Z"); + const daemon = await startDaemon({ stateDir, clock: { now: () => now } }); + runningDaemons.push(daemon); + + const todayFile = `${now.toISOString().slice(0, 10)}.jsonl`; + await writeFile(path.join(paths.auditDir, todayFile), "x".repeat(120), { mode: 0o600 }); + + const socket = await connectRaw(daemon.paths.socketPath); + const reader = createLineReader(socket); + socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 1, method: "daemon.status", params: {} })}\n`); + const response = JSON.parse(await reader.nextLine()); + + expect(response.result.audit).toEqual({ + path: paths.auditDir, + failedWrites: 0, + failedPrunes: 0, + retentionDays: 45, + files: 1, + bytes: 120, + }); + + socket.destroy(); + await removeStateDir(stateDir); + }); +}); diff --git a/packages/appduct/src/__tests__/daemon.test.ts b/packages/appduct/src/__tests__/daemon.test.ts index df785a7f..85cb41b0 100644 --- a/packages/appduct/src/__tests__/daemon.test.ts +++ b/packages/appduct/src/__tests__/daemon.test.ts @@ -1,38 +1,33 @@ -import { spawnSync } from "node:child_process"; -import { connect, createServer as createNetServer, type Socket } from "node:net"; -import { mkdtemp, readdir, readFile, rm, stat, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import path from "node:path"; +/** + * Daemon behaviour that needs **no listener**: `config.json` parsing and validation, the RPC + * server's own line-length cap, and how `daemon status` renders a daemon that predates a field. + * Nothing here binds a port, takes the pidfile, or mints TLS material. + * + * Everything that starts a real daemon lives in `daemon.integration.test.ts`. The split is not + * cosmetic: a case that boots the whole daemon to assert a string in an error message pays for a + * pidfile, a self-signed certificate and a socket it never uses, and fails for reasons that have + * nothing to do with what it is testing. + */ + +import { connect, type Socket } from "node:net"; +import { writeFile } from "node:fs/promises"; import { afterEach, describe, expect, test } from "vitest"; import { handleDaemonStatusCommand } from "../commands/daemon/status.js"; -import { AUDIT_PRUNE_INTERVAL_MS, startDaemon, type RunningDaemon } from "../daemon/daemon.js"; -import { DaemonAlreadyRunningError } from "../daemon/pidfile.js"; +import { startDaemon } from "../daemon/daemon.js"; import { startRpcServer } from "../daemon/rpc-server.js"; import { getStateDirPaths } from "../daemon/state-dir.js"; -import { systemTimers, type IntervalHandle, type TimerFns } from "../daemon/timers.js"; -import { writeTestHostKey } from "./fixtures.js"; - -const runningDaemons: RunningDaemon[] = []; - -const startTrackedDaemon = async (stateDir: string): Promise => { - const daemon = await startDaemon({ stateDir }); - runningDaemons.push(daemon); - return daemon; -}; - -afterEach(async () => { - while (runningDaemons.length > 0) { - const daemon = runningDaemons.pop(); - await daemon?.shutdown(); - } -}); +import { makeTempStateDir as makeSharedStateDir, removeStateDir } from "./fixtures.js"; +/** + * The shared fixture writes `wssPort: 0` ("bind an OS-assigned port", ARCHITECTURE.md §3). This + * file used to write no `config.json` at all, so every daemon it started bound the default 8443 + * and collided with any other daemon on the machine — including the ones a second vitest process + * (another worktree, another agent session) is running at the same time. + */ const makeTempStateDir = async (): Promise => { - const stateDir = await mkdtemp(path.join(tmpdir(), "appduct-daemon-test-")); - await writeTestHostKey(path.join(stateDir, "key.pem")); - return stateDir; + return makeSharedStateDir({}, { prefix: "appduct-daemon-test-" }); }; /** Reads newline-delimited JSON-RPC responses off a raw socket, resolving each awaited line. */ @@ -81,78 +76,7 @@ const connectRaw = (socketPath: string): Promise => { }); }; -describe("daemon lifecycle", () => { - test("daemon.status round-trips over the real UDS", async () => { - const stateDir = await makeTempStateDir(); - const daemon = await startTrackedDaemon(stateDir); - - const paths = getStateDirPaths(stateDir); - expect((await stat(paths.root)).mode & 0o777).toBe(0o700); - expect((await stat(paths.socketPath)).mode & 0o777).toBe(0o600); - // `audit/` must be tightened explicitly: `mkdir` honors the process umask, which on a common - // 0o022 umask would otherwise leave it drwxr-xr-x (ARCHITECTURE.md §3: mode 0700). - expect((await stat(paths.auditDir)).mode & 0o777).toBe(0o700); - - const socket = await connectRaw(paths.socketPath); - const reader = createLineReader(socket); - - socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 1, method: "daemon.status", params: {} })}\n`); - const line = await reader.nextLine(); - const response = JSON.parse(line); - - expect(response.id).toBe(1); - expect(response.result).toMatchObject({ - pid: process.pid, - wssPort: 8443, - sessions: [], - }); - expect(response.result.pinnedKeys).toHaveLength(1); - expect(response.result.pinnedKeys[0]).toMatch(/^sha256\//u); - expect(response.result.version).toBeTypeOf("string"); - expect(response.result.startedAt).toBe(daemon.startedAt.toISOString()); - - socket.destroy(); - await rm(stateDir, { force: true, recursive: true }); - }); - - test("malformed JSON line gets a JSON-RPC parse error and the connection stays usable", async () => { - const stateDir = await makeTempStateDir(); - await startTrackedDaemon(stateDir); - const paths = getStateDirPaths(stateDir); - - const socket = await connectRaw(paths.socketPath); - const reader = createLineReader(socket); - - socket.write("{ not json \n"); - const badLineResponse = JSON.parse(await reader.nextLine()); - expect(badLineResponse.error.code).toBe(-32700); - - socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 7, method: "daemon.status" })}\n`); - const goodLineResponse = JSON.parse(await reader.nextLine()); - expect(goodLineResponse.id).toBe(7); - expect(goodLineResponse.result.pid).toBe(process.pid); - - socket.destroy(); - await rm(stateDir, { force: true, recursive: true }); - }); - - test("unknown method returns JSON-RPC -32601", async () => { - const stateDir = await makeTempStateDir(); - await startTrackedDaemon(stateDir); - const paths = getStateDirPaths(stateDir); - - const socket = await connectRaw(paths.socketPath); - const reader = createLineReader(socket); - - socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 3, method: "nonexistent.method" })}\n`); - const response = JSON.parse(await reader.nextLine()); - - expect(response.error.code).toBe(-32601); - - socket.destroy(); - await rm(stateDir, { force: true, recursive: true }); - }); - +describe("daemon: config and RPC framing", () => { test("a line beyond the configured cap gets an error and the connection is dropped", async () => { const stateDir = await makeTempStateDir(); const paths = getStateDirPaths(stateDir); @@ -186,73 +110,10 @@ describe("daemon lifecycle", () => { expect(socket.destroyed).toBe(true); } finally { await server.close(); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); } }); - test("daemon.shutdown acks then closes the socket and removes sock + pid files", async () => { - const stateDir = await makeTempStateDir(); - const daemon = await startTrackedDaemon(stateDir); - runningDaemons.pop(); // shutting down manually below; don't double-shutdown in afterEach. - const paths = getStateDirPaths(stateDir); - - const socket = await connectRaw(paths.socketPath); - const reader = createLineReader(socket); - - socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 9, method: "daemon.shutdown" })}\n`); - const response = JSON.parse(await reader.nextLine()); - expect(response.result).toEqual({ ok: true }); - - await daemon.exited; - - await expect(stat(paths.socketPath)).rejects.toThrow(); - await expect(stat(paths.pidFilePath)).rejects.toThrow(); - - socket.destroy(); - await rm(stateDir, { force: true, recursive: true }); - }); - - test("second daemon against the same state dir throws DaemonAlreadyRunningError", async () => { - const stateDir = await makeTempStateDir(); - const first = await startTrackedDaemon(stateDir); - - await expect(startDaemon({ stateDir })).rejects.toThrow(DaemonAlreadyRunningError); - - await first.shutdown(); - await rm(stateDir, { force: true, recursive: true }); - }); - - test("takes over a stale pidfile and stale socket left by a dead process", async () => { - const stateDir = await makeTempStateDir(); - const paths = getStateDirPaths(stateDir); - - // A genuinely-dead pid: spawn a no-op child and wait for it to exit. - const dead = spawnSync(process.execPath, ["-e", "process.exit(0)"]); - expect(dead.status).toBe(0); - const deadPid = dead.pid; - expect(deadPid).toBeGreaterThan(0); - - await writeFile(paths.pidFilePath, String(deadPid), { mode: 0o600 }); - // A stale socket file (not actually listening) left behind by the "crashed" daemon. - await writeFile(paths.socketPath, "", { mode: 0o600 }); - - const daemon = await startTrackedDaemon(stateDir); - - expect(Number((await readFile(paths.pidFilePath, "utf8")).trim())).toBe(process.pid); - expect((await stat(paths.socketPath)).mode & 0o777).toBe(0o600); - - // Confirm the socket now actually answers RPC (proof the stale placeholder file was replaced). - const socket = await connectRaw(paths.socketPath); - const reader = createLineReader(socket); - socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 1, method: "daemon.status" })}\n`); - const response = JSON.parse(await reader.nextLine()); - expect(response.result.pid).toBe(process.pid); - - socket.destroy(); - void daemon; - await rm(stateDir, { force: true, recursive: true }); - }); - test("config.json invalid values throw a clear error naming the key", async () => { const stateDir = await makeTempStateDir(); const paths = getStateDirPaths(stateDir); @@ -262,66 +123,35 @@ describe("daemon lifecycle", () => { await expect(startDaemon({ stateDir })).rejects.toThrow(/wssPort/u); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); - test("config.json unknown keys warn instead of throwing", async () => { + test("wssPort accepts 0 (OS-assigned) and rejects anything that is not a port number", async () => { const stateDir = await makeTempStateDir(); const paths = getStateDirPaths(stateDir); const { mkdir } = await import("node:fs/promises"); - await mkdir(stateDir, { recursive: true }); - await writeFile(paths.configPath, JSON.stringify({ totallyUnknownKey: true, wssPort: 9000 })); - - const warnings: string[] = []; - const daemon = await startTrackedDaemon(stateDir); - void daemon; - - // Re-load directly to also exercise the warn callback in isolation from the running daemon. const { loadConfig } = await import("../daemon/config.js"); - const config = await loadConfig(getStateDirPaths(stateDir), { - warn: (message) => warnings.push(message), - }); - - expect(config.wssPort).toBe(9000); - expect(warnings.some((message) => message.includes("totallyUnknownKey"))).toBe(true); - - await rm(stateDir, { force: true, recursive: true }); - }); - - test("config.json iosBundleId is a known key, loaded as-is and validated as a non-empty string", async () => { - const stateDir = await makeTempStateDir(); - const paths = getStateDirPaths(stateDir); - const { mkdir } = await import("node:fs/promises"); await mkdir(stateDir, { recursive: true }); - const { loadConfig } = await import("../daemon/config.js"); - await writeFile(paths.configPath, JSON.stringify({ iosBundleId: "com.example.playground" })); - - const warnings: string[] = []; - const config = await loadConfig(paths, { warn: (message) => warnings.push(message) }); - - expect(config.iosBundleId).toBe("com.example.playground"); - // A key that warned as unknown would still "work" via the `--bundle-id` flag, hiding a typo'd - // config from the operator for as long as they only ever passed the flag. - expect(warnings).toEqual([]); - - await writeFile(paths.configPath, JSON.stringify({ iosBundleId: "" })); - await expect(loadConfig(paths)).rejects.toThrow(/iosBundleId/u); + // The documented default when the key is absent (ARCHITECTURE.md §3). + await writeFile(paths.configPath, JSON.stringify({})); + expect((await loadConfig(paths)).wssPort).toBe(8443); - await writeFile(paths.configPath, JSON.stringify({ iosBundleId: 42 })); - await expect(loadConfig(paths)).rejects.toThrow(/iosBundleId/u); + // `0` is not a degenerate port here but a request for an OS-assigned one, so it must load + // as-is rather than being rejected by the positive-integer rule every other numeric key uses. + await writeFile(paths.configPath, JSON.stringify({ wssPort: 0 })); + expect((await loadConfig(paths)).wssPort).toBe(0); - await writeFile(paths.configPath, JSON.stringify({ iosBundleId: "com.example.my-app2" })); - await expect(loadConfig(paths)).resolves.toMatchObject({ iosBundleId: "com.example.my-app2" }); + await writeFile(paths.configPath, JSON.stringify({ wssPort: 8443 })); + expect((await loadConfig(paths)).wssPort).toBe(8443); - // Deliberately *not* charset-checked here, only where the value is used. This loader runs on - // every daemon start, so a typo in a CLI-side convenience key must not stop the daemon from - // starting — it surfaces as a usage error against the `link`/`connect` call that needed it. - await writeFile(paths.configPath, JSON.stringify({ iosBundleId: "--console" })); - await expect(loadConfig(paths)).resolves.toMatchObject({ iosBundleId: "--console" }); - await expect(startTrackedDaemon(stateDir)).resolves.toBeDefined(); + // Everything that still is not a port: negatives, non-integers, out-of-range, wrong type. + for (const invalid of [-1, 1.5, 65_536, "8443", null]) { + await writeFile(paths.configPath, JSON.stringify({ wssPort: invalid })); + await expect(loadConfig(paths)).rejects.toThrow(/wssPort/u); + } - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); test("restartDaemonOnVersionMismatch defaults to false and must be a boolean", async () => { @@ -343,146 +173,17 @@ describe("daemon lifecycle", () => { await writeFile(paths.configPath, JSON.stringify({ restartDaemonOnVersionMismatch: "true" })); await expect(loadConfig(paths)).rejects.toThrow(/restartDaemonOnVersionMismatch/u); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); }); /** - * Retention wiring (ARCHITECTURE.md §3, issue #32). The audit *policy* — which files are stale, how - * the day boundary is computed — is covered by `audit-retention.test.ts`; what matters here is that - * a real daemon actually runs it at startup, keeps running it on the daily seam, reports the - * footprint over RPC, and does not leave the timer behind on shutdown. + * Retention *reporting* that needs no daemon: how `daemon status` renders a pre-retention daemon + * (a hand-rolled RPC server, not a real one), and that an invalid retention key fails config + * loading before anything is started. The wiring — a real daemon actually running the sweep — is + * in `daemon.integration.test.ts`; the policy itself is in `audit-retention.test.ts`. */ -describe("daemon: audit retention", () => { - /** Interval-only fake: `startDaemon` uses its `timers` seam solely for the daily audit sweep, so - * everything else (session grace/keepalive, listener pre-claim) keeps running on real timers. */ - const createIntervalRecorder = (): { - timers: TimerFns; - intervals: Array<{ callback: () => void; ms: number; cleared: boolean }>; - } => { - const intervals: Array<{ callback: () => void; ms: number; cleared: boolean }> = []; - - return { - intervals, - timers: { - ...systemTimers, - setInterval: (callback, ms) => { - const record = { callback, ms, cleared: false }; - intervals.push(record); - return record as unknown as IntervalHandle; - }, - clearInterval: (handle) => { - (handle as unknown as { cleared: boolean }).cleared = true; - }, - }, - }; - }; - - const writeDayFile = async (auditDir: string, stamp: string): Promise => { - await writeFile(path.join(auditDir, `${stamp}.jsonl`), "{}\n", { mode: 0o600 }); - }; - - /** These tests are about the audit directory, not the wss listener, so they pin a free port - * rather than inheriting the default 8443 — nothing here should fail because something else on - * the machine happens to hold it. */ - const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); - }; - - /** Both sweeps are fire-and-forget (`void auditLogger.prune()`), so the assertion polls rather - * than sleeping on a guessed duration. */ - const waitForAuditDir = async (auditDir: string, expected: string[]): Promise => { - const deadline = Date.now() + 2000; - - for (;;) { - const names = (await readdir(auditDir)).sort(); - - if (names.join(",") === expected.join(",") || Date.now() > deadline) { - expect(names).toEqual(expected); - return; - } - - await new Promise((resolve) => setTimeout(resolve, 10)); - } - }; - - test("prunes stale day files at startup, again on the daily timer, and clears the timer on shutdown", async () => { - const stateDir = await makeTempStateDir(); - const paths = getStateDirPaths(stateDir); - const { mkdir } = await import("node:fs/promises"); - await mkdir(paths.auditDir, { recursive: true }); - await writeFile(paths.configPath, JSON.stringify({ auditRetentionDays: 7, wssPort: await pickFreePort() })); - - await writeDayFile(paths.auditDir, "2026-09-05"); // today per the clock below - await writeDayFile(paths.auditDir, "2026-08-01"); // stale at startup - - const { timers, intervals } = createIntervalRecorder(); - let now = new Date("2026-09-05T12:00:00.000Z"); - const daemon = await startDaemon({ stateDir, timers, clock: { now: () => now } }); - runningDaemons.push(daemon); - - await waitForAuditDir(paths.auditDir, ["2026-09-05.jsonl"]); - - expect(intervals).toHaveLength(1); - expect(intervals[0]!.ms).toBe(AUDIT_PRUNE_INTERVAL_MS); - - // A day later the daemon is still running and the sweep still fires — the file that was - // "today" at startup is now stale enough to go. - await writeDayFile(paths.auditDir, "2026-09-14"); - now = new Date("2026-09-14T12:00:00.000Z"); - intervals[0]!.callback(); - await waitForAuditDir(paths.auditDir, ["2026-09-14.jsonl"]); - - await daemon.shutdown(); - expect(intervals[0]!.cleared).toBe(true); - - await rm(stateDir, { force: true, recursive: true }); - }); - - test("daemon.status reports the audit footprint and the effective retention", async () => { - const stateDir = await makeTempStateDir(); - const paths = getStateDirPaths(stateDir); - const { mkdir } = await import("node:fs/promises"); - await mkdir(paths.auditDir, { recursive: true }); - await writeFile(paths.configPath, JSON.stringify({ auditRetentionDays: 45, wssPort: await pickFreePort() })); - - // Clock-injected, and the fixture's name is derived from it, for two reasons: the file must - // be dated relative to the daemon's own idea of "today" rather than the calendar the suite - // happens to run on, and naming it *today* makes it immune to the fire-and-forget startup - // sweep that may still be in flight — today's file is the one file pruning can never take. - const now = new Date("2026-09-05T12:00:00.000Z"); - const daemon = await startDaemon({ stateDir, clock: { now: () => now } }); - runningDaemons.push(daemon); - - const todayFile = `${now.toISOString().slice(0, 10)}.jsonl`; - await writeFile(path.join(paths.auditDir, todayFile), "x".repeat(120), { mode: 0o600 }); - - const socket = await connectRaw(daemon.paths.socketPath); - const reader = createLineReader(socket); - socket.write(`${JSON.stringify({ jsonrpc: "2.0", id: 1, method: "daemon.status", params: {} })}\n`); - const response = JSON.parse(await reader.nextLine()); - - expect(response.result.audit).toEqual({ - path: paths.auditDir, - failedWrites: 0, - failedPrunes: 0, - retentionDays: 45, - files: 1, - bytes: 120, - }); - - socket.destroy(); - await rm(stateDir, { force: true, recursive: true }); - }); - +describe("daemon: audit retention config and reporting", () => { test("daemon status degrades cleanly against a daemon that predates retention", async () => { const stateDir = await makeTempStateDir(); const paths = getStateDirPaths(stateDir); @@ -535,7 +236,7 @@ describe("daemon: audit retention", () => { await legacyDaemon.close(); } - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); test("an invalid auditRetentionDays/daemonLogMaxBytes fails the daemon like any other config key", async () => { @@ -550,6 +251,6 @@ describe("daemon: audit retention", () => { await writeFile(paths.configPath, JSON.stringify({ daemonLogMaxBytes: 1.5 })); await expect(startDaemon({ stateDir })).rejects.toThrow(/daemonLogMaxBytes.*positive integer/u); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); }); diff --git a/packages/appduct/src/__tests__/e2e/churn.e2e.test.ts b/packages/appduct/src/__tests__/e2e/churn.e2e.test.ts index 28cda507..69ee84b7 100644 --- a/packages/appduct/src/__tests__/e2e/churn.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/churn.e2e.test.ts @@ -14,6 +14,7 @@ import { afterEach, describe, expect, test } from "vitest"; import { FakeAppClient } from "./app-client.js"; import { + daemonWssPort, cleanupAfterEach, ensureDaemon, fetchPinnedKeys, @@ -29,8 +30,11 @@ describe("e2e: churn", () => { test( "suspend on socket loss, session_suspended on invoke, resume, then grace expiry frees the alias", async () => { - const { stateDir, port } = await makeTempStateDir({ graceSeconds: 2 }); + const { stateDir } = await makeTempStateDir({ graceSeconds: 2 }); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const events = await subscribeToEvents(stateDir); diff --git a/packages/appduct/src/__tests__/e2e/client-bootstrap.e2e.test.ts b/packages/appduct/src/__tests__/e2e/client-bootstrap.e2e.test.ts index 6370b83d..b721be94 100644 --- a/packages/appduct/src/__tests__/e2e/client-bootstrap.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/client-bootstrap.e2e.test.ts @@ -8,14 +8,17 @@ import { afterEach, describe, expect, test } from "vitest"; import { link, waitForSession, AppductError } from "../../client/index.js"; import { FakeAppClient } from "./app-client.js"; -import { cleanupAfterEach, decodeDeepLink, ensureDaemon, fetchPinnedKeys, makeTempStateDir } from "./harness.js"; +import { cleanupAfterEach, daemonWssPort, decodeDeepLink, ensureDaemon, fetchPinnedKeys, makeTempStateDir } from "./harness.js"; afterEach(cleanupAfterEach); describe("e2e: appduct/client bootstrap", () => { test("link() mints a claimable deep link, and waitForSession() resolves once a fake app claims it concurrently with the subscribe", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const minted = await link({ stateDir, ttlSeconds: 60 }); @@ -41,8 +44,11 @@ describe("e2e: appduct/client bootstrap", () => { }); test("waitForSession() resolves immediately when the session is already claimed", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const minted = await link({ stateDir }); diff --git a/packages/appduct/src/__tests__/e2e/client.e2e.test.ts b/packages/appduct/src/__tests__/e2e/client.e2e.test.ts index 979f8074..129daf3f 100644 --- a/packages/appduct/src/__tests__/e2e/client.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/client.e2e.test.ts @@ -4,15 +4,20 @@ * call — entirely through `connect()`/`AppClient`, never through a CLI subprocess, and asserts the * audit trail attributes these calls to `caller: "client"`. */ -import { readFile } from "node:fs/promises"; -import path from "node:path"; - import { afterEach, describe, expect, test } from "vitest"; import { connect, AppductError } from "../../client/index.js"; -import { getStateDirPaths } from "../../daemon/state-dir.js"; import { FakeAppClient } from "./app-client.js"; -import { cleanupAfterEach, ensureDaemon, fetchPinnedKeys, makeTempStateDir, mintLink, subscribeToEvents } from "./harness.js"; +import { + cleanupAfterEach, + daemonWssPort, + ensureDaemon, + fetchPinnedKeys, + makeTempStateDir, + mintLink, + subscribeToEvents, + waitForAuditRecords, +} from "./harness.js"; afterEach(cleanupAfterEach); @@ -24,16 +29,20 @@ type AuditRecord = { caller: "cli" | "mcp" | "client"; }; -const readTodaysAuditRecords = async (stateDir: string): Promise => { - const paths = getStateDirPaths(stateDir); - const dateStamp = new Date().toISOString().slice(0, 10); - const raw = await readFile(path.join(paths.auditDir, `${dateStamp}.jsonl`), "utf8"); - - return raw - .trim() - .split("\n") - .filter((line) => line.length > 0) - .map((line) => JSON.parse(line) as AuditRecord); +/** The three calls this scenario makes, each on its own line of today's audit file. The daemon + * runs in a separate process and answers a call before its audit line is necessarily on disk + * (`daemon/audit.ts`'s write queue), so this waits for all three rather than reading once. */ +const waitForSessionAuditRecords = async (stateDir: string, alias: string): Promise => { + const forThisSession = (records: AuditRecord[]): AuditRecord[] => + records.filter((record) => record.alias === alias); + + return forThisSession( + await waitForAuditRecords( + stateDir, + (records) => forThisSession(records).length >= 3, + { description: `three audited calls for alias "${alias}"` }, + ), + ); }; /** A caller-declared tool map, `interface`-style (not a `type` alias) — regression coverage for @@ -49,8 +58,11 @@ describe("e2e: appduct/client", () => { test( "connect() -> tools() -> call() -> waitForEvent() round-trips against a real daemon and app, audited as caller \"client\"", async () => { - const { stateDir, port } = await makeTempStateDir({ policy: { destructive: "deny" } }); + const { stateDir } = await makeTempStateDir({ policy: { destructive: "deny" } }); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const events = await subscribeToEvents(stateDir); @@ -94,7 +106,7 @@ describe("e2e: appduct/client", () => { await expect(app.call("no_such_tool" as never, {})).rejects.toMatchObject({ type: "tool_not_found" }); await expect(app.call("deleteAll", {})).rejects.toMatchObject({ type: "policy_denied" }); - const records = (await readTodaysAuditRecords(stateDir)).filter((record) => record.alias === ack.alias); + const records = await waitForSessionAuditRecords(stateDir, ack.alias); expect(records).toContainEqual( expect.objectContaining({ tool: "sum", outcome: "ok", caller: "client" }), @@ -115,8 +127,11 @@ describe("e2e: appduct/client", () => { test( "call()'s transport timeout never fires before the daemon's own tool_timeout, even for a timeoutMs above the transport default", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const events = await subscribeToEvents(stateDir); @@ -147,8 +162,11 @@ describe("e2e: appduct/client", () => { ); test("waitForEvent() rejects (rather than crashing the connection) when its match predicate throws", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const events = await subscribeToEvents(stateDir); @@ -186,8 +204,11 @@ describe("e2e: appduct/client", () => { test( "waitForEvent() resolves from the retained buffer for an event emitted before it was called (no live-subscribe race)", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const events = await subscribeToEvents(stateDir); @@ -221,8 +242,11 @@ describe("e2e: appduct/client", () => { test( "events() drains the retained buffer, and waitForEvent()'s since skips events already seen", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const events = await subscribeToEvents(stateDir); diff --git a/packages/appduct/src/__tests__/e2e/cold-start.e2e.test.ts b/packages/appduct/src/__tests__/e2e/cold-start.e2e.test.ts index aae99b30..00c9e188 100644 --- a/packages/appduct/src/__tests__/e2e/cold-start.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/cold-start.e2e.test.ts @@ -16,6 +16,7 @@ import { afterEach, describe, expect, test } from "vitest"; import { FakeAppClient } from "./app-client.js"; import { + daemonWssPort, cleanupAfterEach, decodeDeepLink, fetchPinnedKeys, @@ -32,7 +33,10 @@ describe("e2e: cold start", () => { test( "keygen -> link auto-spawns -> claim (pin-verified) -> ls ACTIVE -> tools/invoke -> revoke -> daemon stop leaves no socket/pidfile", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); // keygen: fully non-interactive, refuses to overwrite without --force. const keygenPath = path.join(stateDir, "operator-key.pem"); diff --git a/packages/appduct/src/__tests__/e2e/daemon-restart.e2e.test.ts b/packages/appduct/src/__tests__/e2e/daemon-restart.e2e.test.ts index ec5bc082..a5f6d6fd 100644 --- a/packages/appduct/src/__tests__/e2e/daemon-restart.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/daemon-restart.e2e.test.ts @@ -10,6 +10,7 @@ import { afterEach, describe, expect, test } from "vitest"; import { FakeAppClient } from "./app-client.js"; import { + daemonWssPort, cleanupAfterEach, ensureDaemon, fetchPinnedKeys, @@ -26,8 +27,11 @@ describe("e2e: daemon restart", () => { test( "SIGKILL mid-session -> next command auto-spawns a fresh daemon, old session gone, new link/claim works", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); const firstPid = await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const link = await mintLink(stateDir); @@ -61,9 +65,12 @@ describe("e2e: daemon restart", () => { // Cleanup tracks the *new* daemon, not the one this test already killed. trackDaemonPid(secondPid); - // A brand-new link/claim against the fresh daemon works end-to-end. + // A brand-new link/claim against the fresh daemon works end-to-end. The port comes off the + // fresh link, not off the dead daemon: the replacement asked the OS for a port of its own + // (`wssPort: 0`) and will not be on the one its predecessor held — and the link is exactly + // where a real app would read it from. const freshLink = await mintLink(stateDir); - const freshApp = new FakeAppClient(port, pinnedKeys); + const freshApp = new FakeAppClient(freshLink.port, pinnedKeys); const freshAck = await freshApp.claim(freshLink, { model: "Pixel 8" }); expect(freshAck.status).toBe("ok"); expect(freshAck.alias).toBe("pixel-8"); diff --git a/packages/appduct/src/__tests__/e2e/daemon-version-mismatch.e2e.test.ts b/packages/appduct/src/__tests__/e2e/daemon-version-mismatch.e2e.test.ts index 7eeab1fb..e7da6875 100644 --- a/packages/appduct/src/__tests__/e2e/daemon-version-mismatch.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/daemon-version-mismatch.e2e.test.ts @@ -187,12 +187,15 @@ describe("e2e: daemon/CLI version drift", () => { test( "a stale daemon with a live session is reported, not restarted", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); const stalePid = await startStaleDaemon(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const link = await mintLinkWithoutCli(stateDir); - const app = new FakeAppClient(port, pinnedKeys); + // The port comes off the decoded bootstrap payload: reading it over the CLI first would + // auto-spawn a current-version daemon and defeat the staging above, and this is the number + // a real app dials anyway. + const app = new FakeAppClient(link.port, pinnedKeys); await app.claim(link, { model: "Pixel 8" }); try { @@ -228,12 +231,15 @@ describe("e2e: daemon/CLI version drift", () => { test( "--daemon-restart replaces a stale daemon even with a live session", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); const stalePid = await startStaleDaemon(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const link = await mintLinkWithoutCli(stateDir); - const app = new FakeAppClient(port, pinnedKeys); + // The port comes off the decoded bootstrap payload: reading it over the CLI first would + // auto-spawn a current-version daemon and defeat the staging above, and this is the number + // a real app dials anyway. + const app = new FakeAppClient(link.port, pinnedKeys); await app.claim(link, { model: "Pixel 8" }); const socketClosed = app.waitForClose(); @@ -289,12 +295,15 @@ describe("e2e: forcing a version-drift restart", () => { test( "APPDUCT_DAEMON_RESTART=1 forces the restart with no flag on the command line", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); const stalePid = await startStaleDaemon(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const link = await mintLinkWithoutCli(stateDir); - const app = new FakeAppClient(port, pinnedKeys); + // The port comes off the decoded bootstrap payload: reading it over the CLI first would + // auto-spawn a current-version daemon and defeat the staging above, and this is the number + // a real app dials anyway. + const app = new FakeAppClient(link.port, pinnedKeys); await app.claim(link, { model: "Pixel 8" }); // The env form exists for exactly this: an MCP launch config passes no CLI flags. @@ -320,12 +329,15 @@ describe("e2e: forcing a version-drift restart", () => { test( "--no-daemon-restart overrules restartDaemonOnVersionMismatch for one command", async () => { - const { stateDir, port } = await makeTempStateDir({ restartDaemonOnVersionMismatch: true }); + const { stateDir } = await makeTempStateDir({ restartDaemonOnVersionMismatch: true }); const stalePid = await startStaleDaemon(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const link = await mintLinkWithoutCli(stateDir); - const app = new FakeAppClient(port, pinnedKeys); + // The port comes off the decoded bootstrap payload: reading it over the CLI first would + // auto-spawn a current-version daemon and defeat the staging above, and this is the number + // a real app dials anyway. + const app = new FakeAppClient(link.port, pinnedKeys); await app.claim(link, { model: "Pixel 8" }); try { diff --git a/packages/appduct/src/__tests__/e2e/events.e2e.test.ts b/packages/appduct/src/__tests__/e2e/events.e2e.test.ts index 2cb63bd6..89a74d66 100644 --- a/packages/appduct/src/__tests__/e2e/events.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/events.e2e.test.ts @@ -9,6 +9,7 @@ import { afterEach, describe, expect, test } from "vitest"; import { FakeAppClient } from "./app-client.js"; import { + daemonWssPort, cleanupAfterEach, ensureDaemon, fetchPinnedKeys, @@ -73,8 +74,11 @@ describe("e2e: events --json", () => { test( "NDJSON stream captures claim -> tools_changed -> app_event -> tool_call_started/finished -> suspended, in order", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const eventsProcess = spawnCli(["events", "--json"], stateDir); diff --git a/packages/appduct/src/__tests__/e2e/harness.ts b/packages/appduct/src/__tests__/e2e/harness.ts index 23a0c363..a82a7793 100644 --- a/packages/appduct/src/__tests__/e2e/harness.ts +++ b/packages/appduct/src/__tests__/e2e/harness.ts @@ -14,9 +14,8 @@ */ import { createHash, X509Certificate } from "node:crypto"; -import { mkdtemp, rm, writeFile } from "node:fs/promises"; -import { connect as connectUds, createServer as createNetServer, type Socket } from "node:net"; -import { tmpdir } from "node:os"; +import { readFile } from "node:fs/promises"; +import { connect as connectUds, type Socket } from "node:net"; import path from "node:path"; import { text } from "node:stream/consumers"; import { connect as tlsConnect } from "node:tls"; @@ -24,7 +23,15 @@ import { connect as tlsConnect } from "node:tls"; import { decodeBootstrap, type EventKind, type EventNotification } from "@appduct/shared"; import { getStateDirPaths } from "../../daemon/state-dir.js"; -import { binEntry, packageRoot, spawnCliBinary, waitForExit, writeTestHostKey } from "../fixtures.js"; +import { + binEntry, + makeTempStateDir as makeSharedStateDir, + packageRoot, + removeStateDir, + spawnCliBinary, + waitForExit, + writeTestHostKey, +} from "../fixtures.js"; export { binEntry, packageRoot, writeTestHostKey }; export { waitForExit } from "../fixtures.js"; @@ -68,7 +75,7 @@ export const cleanupAfterEach = async (): Promise => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }; @@ -91,37 +98,26 @@ export const trackCleanup = (fn: () => void | Promise): void => { extraCleanups.push(fn); }; -export const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - export type TestStateDir = { stateDir: string; - port: number; }; -/** Always pins a free port and a throwaway host key: every e2e scenario runs several concurrent - * test files, each with its own real daemon subprocess/listener. */ +/** + * A throwaway host key plus a `config.json` asking for an OS-assigned wss port (`wssPort: 0`, + * ARCHITECTURE.md §3). Every e2e scenario runs its own real daemon subprocess with its own + * listener, and several vitest processes may be running this suite at once on one machine, so no + * scenario may name a port: pre-picking one and writing it into a config leaves a window in which + * anything else can take it. The port a scenario needs is read back from the running daemon + * ({@link daemonWssPort}) instead. + */ export const makeTempStateDir = async (configOverrides: Record = {}): Promise => { - const directory = await mkdtemp(path.join(tmpdir(), "appduct-e2e-")); - await writeTestHostKey(path.join(directory, "key.pem")); - - const port = await pickFreePort(); - await writeFile( - path.join(directory, "config.json"), - JSON.stringify({ wssPort: port, advertisedIp: "127.0.0.1", scheme: "appduct-e2e", ...configOverrides }), + const directory = await makeSharedStateDir( + { scheme: "appduct-e2e", ...configOverrides }, + { prefix: "appduct-e2e-" }, ); stateDirs.push(directory); - return { stateDir: directory, port }; + return { stateDir: directory }; }; export type CliJsonResult = { @@ -180,6 +176,22 @@ export const ensureDaemon = async (stateDir: string): Promise => { return pid; }; +/** + * The wss port the daemon for `stateDir` actually bound, read back over a real `daemon status + * --json` — the only way to learn it across a process boundary, since the state dir's config asks + * for an OS-assigned one rather than naming a number. Auto-spawns the daemon like any other CLI + * call, so a scenario can ask for the port before it has explicitly started one. + */ +export const daemonWssPort = async (stateDir: string): Promise => { + const status = await runCliJson<{ daemon: { wss_port: number } }>(["daemon", "status"], stateDir); + + if (!status.ok || !status.data) { + throw new Error(`Failed to read the wss port for "${stateDir}": ${JSON.stringify(status)}`); + } + + return status.data.daemon.wss_port; +}; + /** Fetches the daemon's advertised SPKI pin-set via a real `daemon status --json` CLI call. */ export const fetchPinnedKeys = async (stateDir: string): Promise => { const status = await runCliJson<{ daemon: { pinned_keys: string[] } }>(["daemon", "status"], stateDir); @@ -323,6 +335,78 @@ export const waitUntil = async ( throw new Error(`Timed out after ${timeoutMs}ms waiting for: ${options.description ?? "condition"}.`); }; +/** + * Polls today's `audit/.jsonl` until `predicate` holds over the records it contains, + * then returns them. + * + * Reading the file straight after the calls that should have produced its last lines is a race + * across a process boundary. `daemon/audit.ts` serializes writes on an internal promise queue and + * `record()` returns the moment it has *enqueued* one, precisely so a slow disk cannot stall the + * `tools.call` response path — so the response a CLI subprocess already returned proves the call + * finished, never that its audit line has landed. In-process that gap is closed by + * `AuditLogger.flush()`; from another process there is nothing to await, so the only honest answer + * is to re-read until the records are there. + * + * On timeout it throws with what it last saw (and how many records), because "expected 3, saw 2" + * names a different bug from "expected 3, saw 0" and a bare timeout tells them apart for nobody. + */ +export const waitForAuditRecords = async >( + stateDir: string, + predicate: (records: TRecord[]) => boolean, + options: { timeoutMs?: number; intervalMs?: number; description?: string } = {}, +): Promise => { + const timeoutMs = options.timeoutMs ?? 5000; + const intervalMs = options.intervalMs ?? 25; + const deadline = Date.now() + timeoutMs; + const dayFile = path.join(getStateDirPaths(stateDir).auditDir, `${new Date().toISOString().slice(0, 10)}.jsonl`); + + const read = async (): Promise => { + let raw: string; + + try { + raw = await readFile(dayFile, "utf8"); + } catch (error) { + // The audit directory and its day file are both created lazily, on the first record — an + // ENOENT here means "nothing audited yet", which is a state to keep waiting through, not an + // error to report. + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return []; + } + + throw error; + } + + return raw + .split("\n") + .filter((line) => line.trim().length > 0) + // A partially-flushed final line is possible while the daemon is mid-append; treat it as + // not-yet-there rather than failing the whole read. + .flatMap((line) => { + try { + return [JSON.parse(line) as TRecord]; + } catch { + return []; + } + }); + }; + + let records = await read(); + + while (!predicate(records) && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, intervalMs)); + records = await read(); + } + + if (!predicate(records)) { + throw new Error( + `Timed out after ${timeoutMs}ms waiting for ${options.description ?? "audit records"} in "${dayFile}". ` + + `Saw ${records.length} record(s): ${JSON.stringify(records)}`, + ); + } + + return records; +}; + export type EventWaiter = { waitFor: (kind: EventKind, predicate?: (event: EventNotification) => boolean) => Promise; close: () => void; diff --git a/packages/appduct/src/__tests__/e2e/hostility.e2e.test.ts b/packages/appduct/src/__tests__/e2e/hostility.e2e.test.ts index 9d3dcdd4..1b79ea31 100644 --- a/packages/appduct/src/__tests__/e2e/hostility.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/hostility.e2e.test.ts @@ -13,6 +13,7 @@ import WebSocket from "ws"; import { FakeAppClient } from "./app-client.js"; import { + daemonWssPort, cleanupAfterEach, ensureDaemon, fetchPinnedKeys, @@ -42,8 +43,11 @@ describe("e2e: hostility", () => { test( "the daemon survives a raw TLS flap, an oversized frame, a binary frame, garbage JSON, and a bad claim — ACTIVE session keeps invoking", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); const daemonPid = await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const events = await subscribeToEvents(stateDir); diff --git a/packages/appduct/src/__tests__/e2e/invoke-cancel.e2e.test.ts b/packages/appduct/src/__tests__/e2e/invoke-cancel.e2e.test.ts index 76133501..fffea2ca 100644 --- a/packages/appduct/src/__tests__/e2e/invoke-cancel.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/invoke-cancel.e2e.test.ts @@ -8,6 +8,7 @@ import { afterEach, describe, expect, test } from "vitest"; import { FakeAppClient } from "./app-client.js"; import { + daemonWssPort, cleanupAfterEach, ensureDaemon, fetchPinnedKeys, @@ -23,8 +24,11 @@ describe("e2e: appduct invoke + SIGINT", () => { test( "SIGINT cancels the in-flight call: the app receives tool_cancel and the CLI exits non-zero as tool_cancelled", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const link = await mintLink(stateDir); diff --git a/packages/appduct/src/__tests__/e2e/mcp.e2e.test.ts b/packages/appduct/src/__tests__/e2e/mcp.e2e.test.ts index 11b21230..76edf5f0 100644 --- a/packages/appduct/src/__tests__/e2e/mcp.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/mcp.e2e.test.ts @@ -12,6 +12,7 @@ import { CallToolResultSchema, ListToolsResultSchema, ToolListChangedNotificatio import { FakeAppClient } from "./app-client.js"; import { + daemonWssPort, binEntry, cleanupAfterEach, ensureDaemon, @@ -40,7 +41,10 @@ describe("e2e: mcp (real stdio subprocess)", () => { test( "tools/list, tools/call, and list_changed against a real `appduct mcp` subprocess", async () => { - const { stateDir, port } = await makeTempStateDir({ scheme: "appduct-mcp-e2e" }); + const { stateDir } = await makeTempStateDir({ scheme: "appduct-mcp-e2e" }); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); // The daemon is brought up first (via a real CLI subprocess) so the `mcp` subprocess never // needs to win an auto-spawn race with this test's own setup, and so the pin can be fetched // before the fake app ever connects. diff --git a/packages/appduct/src/__tests__/e2e/multi-device.e2e.test.ts b/packages/appduct/src/__tests__/e2e/multi-device.e2e.test.ts index b424b0be..5c48304a 100644 --- a/packages/appduct/src/__tests__/e2e/multi-device.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/multi-device.e2e.test.ts @@ -10,6 +10,7 @@ import { afterEach, describe, expect, test } from "vitest"; import { FakeAppClient } from "./app-client.js"; import { + daemonWssPort, cleanupAfterEach, ensureDaemon, fetchPinnedKeys, @@ -25,8 +26,11 @@ describe("e2e: multi-device", () => { test( "distinct aliases, ambiguous_session without a selector, revoke isolates the other session", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const events = await subscribeToEvents(stateDir); diff --git a/packages/appduct/src/__tests__/e2e/policy-audit.e2e.test.ts b/packages/appduct/src/__tests__/e2e/policy-audit.e2e.test.ts index 81d05d15..6b473f79 100644 --- a/packages/appduct/src/__tests__/e2e/policy-audit.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/policy-audit.e2e.test.ts @@ -12,6 +12,7 @@ import { afterEach, describe, expect, test } from "vitest"; import { getStateDirPaths } from "../../daemon/state-dir.js"; import { FakeAppClient } from "./app-client.js"; import { + daemonWssPort, cleanupAfterEach, ensureDaemon, fetchPinnedKeys, @@ -19,6 +20,7 @@ import { mintLink, runCliJson, subscribeToEvents, + waitForAuditRecords, } from "./harness.js"; afterEach(cleanupAfterEach); @@ -35,24 +37,24 @@ type AuditRecord = { caller: "cli" | "mcp"; }; -const readTodaysAuditRecords = async (stateDir: string): Promise => { - const paths = getStateDirPaths(stateDir); - const dateStamp = new Date().toISOString().slice(0, 10); - const raw = await readFile(path.join(paths.auditDir, `${dateStamp}.jsonl`), "utf8"); - - return raw - .trim() - .split("\n") - .filter((line) => line.length > 0) - .map((line) => JSON.parse(line) as AuditRecord); +/** The two `invoke`s below (the allowed one and the denied one) each land a line in today's audit + * file. The daemon is a separate process and answers a call before its line is necessarily on disk + * (`daemon/audit.ts`'s write queue), so this waits for both rather than reading once. */ +const waitForBothAuditRecords = async (stateDir: string): Promise => { + return waitForAuditRecords(stateDir, (records) => records.length >= 2, { + description: "the allowed and the denied invoke, both audited", + }); }; describe("e2e: policy and audit", () => { test( "a destructive-hinted tool is denied by policy via `appduct invoke`, and every attempt is audited without raw args", async () => { - const { stateDir, port } = await makeTempStateDir({ policy: { destructive: "deny" } }); + const { stateDir } = await makeTempStateDir({ policy: { destructive: "deny" } }); await ensureDaemon(stateDir); + // The daemon binds an OS-assigned wss port (`wssPort: 0`), so the port is read back + // from the daemon itself rather than chosen here — see harness.makeTempStateDir. + const port = await daemonWssPort(stateDir); const pinnedKeys = await fetchPinnedKeys(stateDir); const events = await subscribeToEvents(stateDir); @@ -84,7 +86,7 @@ describe("e2e: policy and audit", () => { // The hint names the config file the operator would edit to change this (ARCHITECTURE.md §12). expect(deniedInvoke.error?.details).toMatchObject({ hint: expect.stringContaining("config.json") }); - const records = await readTodaysAuditRecords(stateDir); + const records = await waitForBothAuditRecords(stateDir); expect(records.length).toBeGreaterThanOrEqual(2); const rawAuditContents = await readFile( diff --git a/packages/appduct/src/__tests__/events.integration.test.ts b/packages/appduct/src/__tests__/events.integration.test.ts index b0ce343e..4996405c 100644 --- a/packages/appduct/src/__tests__/events.integration.test.ts +++ b/packages/appduct/src/__tests__/events.integration.test.ts @@ -5,17 +5,18 @@ * NDJSON, then confirms Ctrl-C (SIGINT) ends the stream cleanly (exit 0). */ -import { createServer as createNetServer } from "node:net"; -import { mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import path from "node:path"; - import { afterEach, describe, expect, test } from "vitest"; import WebSocket from "ws"; import { decodeBootstrap } from "@appduct/shared"; -import { runCliBinary, spawnCliBinary, waitForExit, writeTestHostKey } from "./fixtures.js"; +import { + makeTempStateDir as makeSharedStateDir, + removeStateDir, + runCliBinary, + spawnCliBinary, + waitForExit, +} from "./fixtures.js"; // Client pinning is the app's job; this test skips it client-side for its throwaway self-signed key. process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; @@ -45,36 +46,15 @@ afterEach(async () => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); -const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - -const makeTempStateDir = async (): Promise<{ stateDir: string; port: number }> => { - const directory = await mkdtemp(path.join(tmpdir(), "appduct-events-cli-")); - await writeTestHostKey(path.join(directory, "key.pem")); - - // A free-port config avoids EADDRINUSE collisions with the other test files' daemons that also - // bind a wss listener concurrently when test files are run in parallel. - const port = await pickFreePort(); - await writeFile( - path.join(directory, "config.json"), - JSON.stringify({ wssPort: port, advertisedIp: "127.0.0.1" }), - ); +const makeTempStateDir = async (): Promise<{ stateDir: string }> => { + const directory = await makeSharedStateDir({}, { prefix: "appduct-events-cli-" }); stateDirs.push(directory); - return { stateDir: directory, port }; + return { stateDir: directory }; }; const runCliJson = (args: string[], stateDir: string) => { @@ -99,7 +79,6 @@ const nextMessage = (socket: WebSocket): Promise> => { * identity plus the open socket (caller closes it). */ const claimAppOverCli = async ( stateDir: string, - port: number, ): Promise<{ socket: WebSocket; alias: string; sessionId: string }> => { const linkResult = runCliJson(["link", "--ttl", "30", "--scheme", "appduct-events-since-test"], stateDir); expect(linkResult.ok).toBe(true); @@ -107,6 +86,10 @@ const claimAppOverCli = async ( const payload = (linkResult.data.deepLink as string).split("appduct=")[1]!.split("&")[0]!; const decoded = decodeBootstrap(payload)!; + // The bootstrap payload carries the port the daemon actually bound - the state dir's + // `wssPort: 0` deliberately names none, and this is the very number a real app would dial. + const port = decoded.port; + const socket = new WebSocket(`wss://127.0.0.1:${port}`, { rejectUnauthorized: false }); await new Promise((resolve, reject) => { socket.once("open", () => resolve()); @@ -194,13 +177,13 @@ describe("appduct events --json", () => { }, 15_000); test("--since pulls retained events one-shot for a claimed session, and a later pull with the returned cursor sees nothing new", async () => { - const { stateDir, port } = await makeTempStateDir(); + const { stateDir } = await makeTempStateDir(); const status = runCliJson(["daemon", "status"], stateDir); expect(status.ok).toBe(true); daemonPids.push(status.data.daemon.pid); - const { socket, alias, sessionId } = await claimAppOverCli(stateDir, port); + const { socket, alias, sessionId } = await claimAppOverCli(stateDir); socket.send(JSON.stringify({ type: "event", session_id: sessionId, name: "greeting", ts: Date.now() })); // The claim ack round-trip already guarantees `session_claimed` landed; give the `event` frame a // beat to reach the daemon and land in the retention buffer before pulling. diff --git a/packages/appduct/src/__tests__/exit-codes.integration.test.ts b/packages/appduct/src/__tests__/exit-codes.integration.test.ts index fca750ea..8bbd5556 100644 --- a/packages/appduct/src/__tests__/exit-codes.integration.test.ts +++ b/packages/appduct/src/__tests__/exit-codes.integration.test.ts @@ -4,9 +4,6 @@ * real daemon, asserting both the exit code and the JSON error's `type`. */ -import { createServer as createNetServer } from "node:net"; -import { mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; import path from "node:path"; import { afterEach, describe, expect, test } from "vitest"; @@ -14,7 +11,7 @@ import WebSocket from "ws"; import { decodeBootstrap } from "@appduct/shared"; -import { runCliBinary, writeTestHostKey } from "./fixtures.js"; +import { makeTempStateDir as makeSharedStateDir, removeStateDir, runCliBinary } from "./fixtures.js"; // The fake app client below skips pinning (that is the app SDK's job), so the leaf-cert check is // disabled process-wide for this file's throwaway self-signed daemon key. @@ -45,33 +42,15 @@ afterEach(async () => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); -const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - -/** Always pins a free port: several cases below auto-spawn a real daemon, and the shared default - * (8443) collides with the other test files' daemons when test files run concurrently. */ +/** Always an OS-assigned wss port (the shared fixture's `wssPort: 0`): several cases below + * auto-spawn a real daemon, and the default 8443 collides with every other daemon on the machine, + * including the ones another concurrent vitest process is running. */ const makeTempStateDir = async (configOverrides: Record = {}): Promise => { - const directory = await mkdtemp(path.join(tmpdir(), "appduct-exit-codes-")); - await writeTestHostKey(path.join(directory, "key.pem")); - - const port = await pickFreePort(); - await writeFile( - path.join(directory, "config.json"), - JSON.stringify({ wssPort: port, advertisedIp: "127.0.0.1", ...configOverrides }), - ); + const directory = await makeSharedStateDir(configOverrides, { prefix: "appduct-exit-codes-" }); stateDirs.push(directory); return directory; @@ -157,11 +136,14 @@ describe("exit codes: v2 command surface", () => { }); test("tool_error (72): invoke a name not registered on a real, claimed session", async () => { - const port = await pickFreePort(); - const stateDir = await makeTempStateDir({ wssPort: port, advertisedIp: "127.0.0.1" }); + const stateDir = await makeTempStateDir(); const status = runCli(["daemon", "status"], stateDir); daemonPids.push(status.payload.data.daemon.pid); + // The state dir asks for an OS-assigned port, so the daemon it just auto-spawned is the only + // source of the real one. + const port = status.payload.data.daemon.wss_port as number; + expect(port).toBeGreaterThan(0); const linkResult = runCli(["link", "--scheme", "appduct-exit-codes"], stateDir); expect(linkResult.exitCode).toBe(0); diff --git a/packages/appduct/src/__tests__/fixtures.ts b/packages/appduct/src/__tests__/fixtures.ts index f2fcb18f..e5e2b193 100644 --- a/packages/appduct/src/__tests__/fixtures.ts +++ b/packages/appduct/src/__tests__/fixtures.ts @@ -1,6 +1,7 @@ import { generateKeyPairSync } from "node:crypto"; import { spawn, spawnSync, type ChildProcessByStdio } from "node:child_process"; -import { writeFile } from "node:fs/promises"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; import path from "node:path"; import type { Readable } from "node:stream"; @@ -113,3 +114,59 @@ export const runCliWithCapture = async ( stderr, }; }; + +/** + * A temp state dir every daemon-starting test can share: a throwaway host key plus a `config.json` + * that asks for an OS-assigned wss port (`wssPort: 0`, ARCHITECTURE.md §3) and advertises + * loopback. + * + * This replaces the `pickFreePort` helper that used to be copy-pasted into a dozen test files. + * Pre-picking a port and then writing it into a config is a TOCTOU race *by construction*: between + * the probe socket closing and the daemon binding, any other process on the machine — including + * another vitest process running this same suite in another worktree — can take that port. Asking + * the OS to assign one at bind time has no window at all. A test that needs the number reads it + * back from the running daemon (`RunningDaemon.listener.port()` in-process, `daemon status --json` + * across a process boundary) rather than deciding it up front. + * + * Callers clean the directory up themselves ({@link removeStateDir}) — this suite's files all keep + * their own `afterEach` sweep, and a hook registered here would be this file's, not theirs. + */ +export const makeTempStateDir = async ( + configOverrides: Record = {}, + options: { prefix?: string } = {}, +): Promise => { + const directory = await mkdtemp(path.join(tmpdir(), options.prefix ?? "appduct-test-")); + await writeTestHostKey(path.join(directory, "key.pem")); + + await writeFile( + path.join(directory, "config.json"), + JSON.stringify({ wssPort: 0, advertisedIp: "127.0.0.1", ...configOverrides }), + { encoding: "utf8", mode: 0o600 }, + ); + + return directory; +}; + +/** Recursive, never-throwing removal of a temp state dir. */ +export const removeStateDir = async (directory: string): Promise => { + await rm(directory, { force: true, recursive: true }); +}; + +/** + * The port a daemon started from {@link makeTempStateDir} actually bound, read back over the real + * CLI (`daemon status --json`) — the only way to learn it across a process boundary, since the + * config deliberately does not name one. + */ +export const readDaemonWssPort = async (stateDir: string): Promise => { + const result = runCliBinary(["daemon", "status", "--json"], { stateDir }); + const payload = JSON.parse(result.stdout) as { + ok: boolean; + data?: { daemon: { wss_port: number; pid: number } }; + }; + + if (!payload.ok || !payload.data) { + throw new Error(`Failed to read daemon status for "${stateDir}": ${result.stdout}${result.stderr}`); + } + + return payload.data.daemon.wss_port; +}; diff --git a/packages/appduct/src/__tests__/link-open.integration.test.ts b/packages/appduct/src/__tests__/link-open.integration.test.ts index 9b4ea937..c306e19f 100644 --- a/packages/appduct/src/__tests__/link-open.integration.test.ts +++ b/packages/appduct/src/__tests__/link-open.integration.test.ts @@ -11,10 +11,7 @@ * *not* get the override — a physical iPhone reaches the daemon only over the LAN (issue #31). */ -import { createServer as createNetServer } from "node:net"; -import { mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import path from "node:path"; +import { writeFile } from "node:fs/promises"; import { afterEach, describe, expect, test } from "vitest"; @@ -23,7 +20,7 @@ import { decodeBootstrap } from "@appduct/shared"; import { handleLinkCommand } from "../commands/link.js"; import { startDaemon, type RunningDaemon } from "../daemon/daemon.js"; import type { ExecFn } from "../cli/open-target.js"; -import { writeTestHostKey } from "./fixtures.js"; +import { makeTempStateDir, removeStateDir } from "./fixtures.js"; const runningDaemons: RunningDaemon[] = []; const stateDirs: string[] = []; @@ -34,39 +31,26 @@ afterEach(async () => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); -const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - const startTestDaemon = async ( extraConfig: Record = {}, ): Promise<{ daemon: RunningDaemon; stateDir: string; port: number }> => { - const stateDir = await mkdtemp(path.join(tmpdir(), "appduct-link-open-")); - stateDirs.push(stateDir); - await writeTestHostKey(path.join(stateDir, "key.pem")); - - const port = await pickFreePort(); - await writeFile( - path.join(stateDir, "config.json"), - JSON.stringify({ wssPort: port, advertisedIp: "203.0.113.9", scheme: "playground", ...extraConfig }), + const stateDir = await makeTempStateDir( + { advertisedIp: "203.0.113.9", scheme: "playground", ...extraConfig }, + { prefix: "appduct-link-open-" }, ); + stateDirs.push(stateDir); const daemon = await startDaemon({ stateDir }); runningDaemons.push(daemon); - return { daemon, stateDir, port }; + // The daemon's `config.json` asks for an OS-assigned port (`wssPort: 0`), so the real port is + // only knowable from the listener that bound it — never pre-picked, which is what used to race + // another vitest process for the same number. + return { daemon, stateDir, port: daemon.listener.port()! }; }; /** Stub `exec` for the `ios-device` path: `devicectl list devices --json-output ` writes its diff --git a/packages/appduct/src/__tests__/mcp-command.integration.test.ts b/packages/appduct/src/__tests__/mcp-command.integration.test.ts index d72dacd5..b74e57aa 100644 --- a/packages/appduct/src/__tests__/mcp-command.integration.test.ts +++ b/packages/appduct/src/__tests__/mcp-command.integration.test.ts @@ -4,8 +4,7 @@ * (the transport-close path most other hosted commands rely on for graceful shutdown). */ -import { mkdtemp, rm, writeFile } from "node:fs/promises"; -import { createServer as createNetServer } from "node:net"; +import { mkdtemp, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; import { PassThrough } from "node:stream"; @@ -19,7 +18,7 @@ import { decodeBootstrap } from "@appduct/shared"; import { handleMcpCommand, type McpHostedResult } from "../commands/mcp.js"; import { startDaemon, type RunningDaemon } from "../daemon/daemon.js"; -import { writeTestHostKey } from "./fixtures.js"; +import { makeTempStateDir, removeStateDir } from "./fixtures.js"; const runningDaemons: RunningDaemon[] = []; const stateDirs: string[] = []; @@ -35,7 +34,7 @@ afterEach(async () => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); @@ -43,28 +42,9 @@ const failIfCalled = (): never => { throw new Error("auto-spawn should never be needed: the test daemon is already running."); }; -const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - const startTestDaemon = async (extraConfig: Record = {}): Promise<{ stateDir: string }> => { - const stateDir = await mkdtemp(path.join(tmpdir(), "appduct-mcp-cmd-")); + const stateDir = await makeTempStateDir(extraConfig, { prefix: "appduct-mcp-cmd-" }); stateDirs.push(stateDir); - await writeTestHostKey(path.join(stateDir, "key.pem")); - - const port = await pickFreePort(); - await writeFile( - path.join(stateDir, "config.json"), - JSON.stringify({ wssPort: port, advertisedIp: "127.0.0.1", ...extraConfig }), - ); const daemon = await startDaemon({ stateDir }); runningDaemons.push(daemon); diff --git a/packages/appduct/src/__tests__/mcp-daemon-fake.ts b/packages/appduct/src/__tests__/mcp-daemon-fake.ts new file mode 100644 index 00000000..7dadc6d1 --- /dev/null +++ b/packages/appduct/src/__tests__/mcp-daemon-fake.ts @@ -0,0 +1,217 @@ +/** + * An in-memory stand-in for the daemon's control-socket connection, for the MCP server tests that + * are about the MCP server and nothing else. + * + * `createMcpServer` talks to the daemon only through `DaemonStream` (`rpc/client.ts`), which is + * four functions: `call`, `onNotification`, `onClose`, `close`. Name mapping, output-schema + * degradation, `__` namespacing and `list_changed` are decided entirely from what + * comes back over those four — a real daemon adds a pidfile, a self-signed certificate, a wss + * listener and a scripted app on a WebSocket, none of which any of those behaviours depends on, + * and all of which can fail on their own. The cases that genuinely exercise the real transport + * (progress correlation over a second stream, cancellation, real policy denial) still run against + * a real daemon in `mcp-server.integration.test.ts`. + * + * This fake answers the four methods the server actually calls — `sessions.list`, `tools.list`, + * `tools.call`, `events.subscribe` — and can push `event` notifications, which is how + * `list_changed` is driven. Anything else throws, loudly, rather than returning a plausible + * nothing: a silently-answered method the server did not expect would make a test pass for the + * wrong reason. + */ + +import { + RPC_METHODS, + type ErrorType, + type EventNotification, + type SessionSummary, + type ToolDescriptor, + type ToolsListEntry, +} from "@appduct/shared"; + +import { DaemonRpcError } from "../rpc/client.js"; +import type { DaemonStream } from "../rpc/client.js"; + +/** What a fake tool does when called. Return a value for `tool_result`; throw + * {@link toolError} for a `tool_error` the daemon would have forwarded verbatim. */ +export type FakeToolHandler = (args: Record) => unknown; + +export type FakeSessionOptions = { + alias: string; + sessionId?: string; + deviceModel?: string; +}; + +/** The daemon's `tools.list` entry shape: a full descriptor plus the resolved effective policy. */ +type FakeToolEntry = ToolsListEntry; + +export type FakeSession = { + readonly alias: string; + readonly sessionId: string; + /** Replaces this session's registry, exactly as a `tool_registry_snapshot` frame would, and + * pushes the `tools_changed` event the daemon would have emitted. */ + setTools: (tools: Array & { name: string; policy?: FakeToolEntry["policy"] }>) => void; + /** Registers what a tool returns (or throws) when called. */ + onCall: (name: string, handler: FakeToolHandler) => void; +}; + +export type FakeDaemon = { + /** Pass as `createMcpServer`'s `openStream`. Every stream it hands out — the startup one and + * each short-lived progress one — is backed by this same state. */ + openStream: () => Promise; + addSession: (options: FakeSessionOptions) => FakeSession; + removeSession: (alias: string) => void; + /** Methods the server called, in order — so a test can assert on what reached "the daemon". */ + calls: () => ReadonlyArray<{ method: string; params: unknown }>; +}; + +/** A `tool_error` the daemon forwards verbatim to the caller (ARCHITECTURE.md §5: "App-side error + * types must be preserved **verbatim** end-to-end"). */ +export const toolError = (type: ErrorType, message: string, details?: unknown): DaemonRpcError => { + return new DaemonRpcError(-32000, message, { type, ...(details === undefined ? {} : { details }) }); +}; + +export const createFakeDaemon = (): FakeDaemon => { + const sessions: SessionSummary[] = []; + const toolsByAlias = new Map(); + const handlersByAlias = new Map>(); + const calls: Array<{ method: string; params: unknown }> = []; + // Every open stream that has subscribed, so a pushed event fans out the way the daemon's own + // `events.subscribe` fan-out does. + const subscribers = new Set<(payload: unknown) => void>(); + let nextSeq = 1; + + const emit = (event: Omit): void => { + const payload = { ...event, ts: Date.now(), seq: nextSeq++ } as EventNotification; + + for (const notify of [...subscribers]) { + notify(payload); + } + }; + + const addSession = (options: FakeSessionOptions): FakeSession => { + const sessionId = options.sessionId ?? `session-${options.alias}`; + const summary: SessionSummary = { + sessionId, + alias: options.alias, + state: "active", + device: { model: options.deviceModel ?? "Pixel 8" }, + createdAt: new Date(0).toISOString(), + toolCount: 0, + }; + + sessions.push(summary); + toolsByAlias.set(options.alias, []); + handlersByAlias.set(options.alias, new Map()); + emit({ kind: "session_claimed", sessionId, alias: options.alias, data: null }); + + return { + alias: options.alias, + sessionId, + setTools: (tools) => { + const entries = tools.map((tool) => ({ + description: "A test tool.", + policy: "allow" as const, + ...tool, + })) as FakeToolEntry[]; + + toolsByAlias.set(options.alias, entries); + summary.toolCount = entries.length; + emit({ kind: "tools_changed", sessionId, alias: options.alias, data: null }); + }, + onCall: (name, handler) => { + handlersByAlias.get(options.alias)!.set(name, handler); + }, + }; + }; + + const removeSession = (alias: string): void => { + const index = sessions.findIndex((session) => session.alias === alias); + + if (index === -1) { + return; + } + + const [removed] = sessions.splice(index, 1); + toolsByAlias.delete(alias); + handlersByAlias.delete(alias); + emit({ kind: "session_revoked", sessionId: removed!.sessionId, alias, data: null }); + }; + + const openStream = async (): Promise => { + let notify: ((payload: unknown) => void) | undefined; + const closeCallbacks = new Set<() => void>(); + const fanOut = (payload: unknown): void => notify?.(payload); + + const call = async (method: string, params?: unknown): Promise => { + calls.push({ method, params }); + + if (method === RPC_METHODS.sessionsList) { + return sessions.map((session) => ({ ...session })) as TResult; + } + + if (method === RPC_METHODS.toolsList) { + const selector = (params as { selector?: string } | undefined)?.selector; + const entries = selector === undefined ? undefined : toolsByAlias.get(selector); + + if (!entries) { + throw toolError("unknown_session", `No session matches "${selector}".`); + } + + return entries.map((entry) => ({ ...entry })) as TResult; + } + + if (method === RPC_METHODS.toolsCall) { + const { selector, name, args } = params as { + selector?: string; + name: string; + args: Record; + }; + const handler = selector === undefined ? undefined : handlersByAlias.get(selector)?.get(name); + + if (!handler) { + throw toolError("tool_not_found", `Tool "${name}" is not registered.`); + } + + // Synchronous by design: it runs the handler, which may throw a `DaemonRpcError` the way + // the real daemon rejects a call whose app answered `tool_error`. + return { result: handler(args ?? {}), callId: `call-${nextSeq++}` } as TResult; + } + + if (method === RPC_METHODS.eventsSubscribe) { + subscribers.add(fanOut); + return { ok: true } as TResult; + } + + throw new Error( + `The in-memory daemon fake was asked for "${method}", which it does not implement. ` + + "Either the MCP server grew a new daemon dependency (teach the fake about it) or this " + + "case needs the real daemon in mcp-server.integration.test.ts.", + ); + }; + + return { + call, + onNotification: (callback) => { + notify = callback; + return () => { + notify = undefined; + }; + }, + onClose: (callback) => { + closeCallbacks.add(callback); + return () => closeCallbacks.delete(callback); + }, + close: () => { + // Drop this stream's subscription *before* notifying: a closed stream that stayed in the + // fan-out set would keep receiving events, which no real closed socket ever does. + subscribers.delete(fanOut); + notify = undefined; + + for (const callback of closeCallbacks) { + callback(); + } + }, + }; + }; + + return { openStream, addSession, removeSession, calls: () => calls }; +}; diff --git a/packages/appduct/src/__tests__/mcp-server.integration.test.ts b/packages/appduct/src/__tests__/mcp-server.integration.test.ts index eede55fb..e266d0ee 100644 --- a/packages/appduct/src/__tests__/mcp-server.integration.test.ts +++ b/packages/appduct/src/__tests__/mcp-server.integration.test.ts @@ -1,17 +1,22 @@ /** - * The MCP server (ARCHITECTURE.md §9) proxying a real daemon's RPC - * surface. Drives a real daemon + fake app-client (same harness pattern as - * `tool-invocation.integration.test.ts`), and drives the MCP server with the SDK's own `Client` - * (over `InMemoryTransport` for the functional cases, over a real `StdioServerTransport` wired to - * plain Node streams for the stdout-purity assertion). + * The MCP server (ARCHITECTURE.md §9) proxying a **real** daemon's RPC surface. Drives a real + * daemon + fake app-client (same harness pattern as `tool-invocation.integration.test.ts`), and + * drives the MCP server with the SDK's own `Client` (over `InMemoryTransport` for the functional + * cases, over a real `StdioServerTransport` wired to plain Node streams for the stdout-purity + * assertion). + * + * Only what genuinely needs the real transport lives here: a declared `timeout_ms` surviving the + * whole round trip, progress correlation over the second daemon stream, cancellation reaching the + * app as `tool_cancel`, the `appduct_connect`/`appduct_wait_for_session` delivery paths, the + * events tools, the `appduct://sessions` resource, stdout purity, and version drift. The server's + * own mapping decisions — tool names, output-schema degradation, namespacing, `list_changed` — + * moved to `mcp-server.test.ts`, which runs them against an in-memory daemon. */ +import { writeFile } from "node:fs/promises"; import { connect as connectUds, type Socket } from "node:net"; -import { mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; import path from "node:path"; import { PassThrough } from "node:stream"; -import { createServer as createNetServer } from "node:net"; import { afterEach, describe, expect, test } from "vitest"; import WebSocket from "ws"; @@ -24,7 +29,6 @@ import { ListResourcesResultSchema, ListToolsResultSchema, ReadResourceResultSchema, - ToolListChangedNotificationSchema, } from "@modelcontextprotocol/sdk/types.js"; import { decodeBootstrap, type ToolDescriptor } from "@appduct/shared"; @@ -34,7 +38,7 @@ import { createMcpServer, type McpServerHandle } from "../mcp/server.js"; import type { ExecFn } from "../cli/open-target.js"; import { DAEMON_VERSION_OVERRIDE_ENV, getPackageVersion } from "../package-version.js"; import { resetDaemonVersionChecks, type SpawnFn } from "../rpc/client.js"; -import { writeTestHostKey } from "./fixtures.js"; +import { makeTempStateDir, removeStateDir } from "./fixtures.js"; process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; @@ -57,7 +61,7 @@ afterEach(async () => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); @@ -65,18 +69,6 @@ const failIfCalled = (): never => { throw new Error("auto-spawn should never be needed: the test daemon is already running."); }; -const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - type TestDaemon = { daemon: RunningDaemon; stateDir: string; @@ -84,20 +76,16 @@ type TestDaemon = { }; const startTestDaemon = async (extraConfig: Record = {}): Promise => { - const stateDir = await mkdtemp(path.join(tmpdir(), "appduct-mcp-")); + const stateDir = await makeTempStateDir({ scheme: "appduct", ...extraConfig }, { prefix: "appduct-mcp-" }); stateDirs.push(stateDir); - await writeTestHostKey(path.join(stateDir, "key.pem")); - - const port = await pickFreePort(); - await writeFile( - path.join(stateDir, "config.json"), - JSON.stringify({ wssPort: port, advertisedIp: "127.0.0.1", scheme: "appduct", ...extraConfig }), - ); const daemon = await startDaemon({ stateDir }); runningDaemons.push(daemon); - return { daemon, stateDir, port }; + // The daemon's `config.json` asks for an OS-assigned port (`wssPort: 0`), so the real port is + // only knowable from the listener that bound it — never pre-picked, which is what used to race + // another vitest process for the same number. + return { daemon, stateDir, port: daemon.listener.port()! }; }; const rpcCall = (socketPath: string, method: string, params?: unknown): Promise => { @@ -274,221 +262,6 @@ const connectInMemoryClient = async (handle: McpServerHandle): Promise = }; describe("mcp: tools/list and tools/call", () => { - test("a fake app's registered tools appear in tools/list with schemas and round-trip through tools/call", async () => { - const { daemon, stateDir, port } = await startTestDaemon(); - const app = await claimApp(daemon, port); - await snapshotTools(daemon, app, [ - { - name: "echo", - description: "Echoes its input.", - input_schema: { type: "object", properties: { text: { type: "string" } } }, - }, - ]); - - const handle = await createMcpHandle(stateDir); - const client = await connectInMemoryClient(handle); - - const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - const proxiedTools = withoutBuiltinTools(listed.tools); - expect(proxiedTools).toHaveLength(1); - expect(proxiedTools[0]!.name).toBe("echo"); - expect(proxiedTools[0]!.description).toBe("Echoes its input."); - expect(proxiedTools[0]!.inputSchema).toEqual({ type: "object", properties: { text: { type: "string" } } }); - - app.socket.on("message", (data) => { - const msg = JSON.parse(data.toString("utf8")) as Record; - - if (msg.type === "tool_call") { - app.socket.send( - JSON.stringify({ - type: "tool_result", - session_id: app.sessionId, - id: msg.id, - result: { echoed: (msg.args as Record).text }, - }), - ); - } - }); - - const called = await client.request( - { method: "tools/call", params: { name: "echo", arguments: { text: "hello" } } }, - CallToolResultSchema, - ); - - expect(called.isError).not.toBe(true); - expect(called.structuredContent).toEqual({ echoed: "hello" }); - - app.socket.close(); - }); - - test("a non-object output schema does not break tools/list: both tools list and both stay callable", async () => { - const { daemon, stateDir, port } = await startTestDaemon(); - const app = await claimApp(daemon, port); - await snapshotTools(daemon, app, [ - { - name: "get-profile", - description: "Returns the profile.", - output_schema: { - type: "object", - properties: { name: { type: "string" } }, - required: ["name"], - additionalProperties: false, - }, - }, - { - // `z.array(z.string())`: MCP's `Tool.outputSchema.type` is the literal `"object"`, so - // before issue #26 this single entry made the client reject the whole list. - name: "list-todos", - description: "Returns the todos.", - output_schema: { type: "array", items: { type: "string" } }, - }, - { - // `z.union([z.object(...), z.object(...)])`: `anyOf` with no root `type`, so MCP rejects - // it even though every branch — and every result — is an object. - name: "get-status", - description: "Returns one of two shapes.", - output_schema: { - anyOf: [ - { type: "object", properties: { ok: { type: "boolean" } }, required: ["ok"] }, - { type: "object", properties: { error: { type: "string" } }, required: ["error"] }, - ], - }, - }, - ]); - - const handle = await createMcpHandle(stateDir); - const client = await connectInMemoryClient(handle); - - const resultsByTool: Record = { - "get-profile": { name: "Ada" }, - "list-todos": ["write tests", "ship it"], - "get-status": { ok: true }, - }; - - app.socket.on("message", (data) => { - const msg = JSON.parse(data.toString("utf8")) as Record; - - if (msg.type === "tool_call") { - app.socket.send( - JSON.stringify({ - type: "tool_result", - session_id: app.sessionId, - id: msg.id, - result: resultsByTool[msg.name as string], - }), - ); - } - }); - - // The SDK's own `listTools`, so the result goes through `ListToolsResultSchema` *and* caches - // the output schemas `callTool` below enforces — exactly what a real client does. - const listed = await client.listTools(); - const proxiedTools = withoutBuiltinTools(listed.tools); - expect(proxiedTools.map((tool) => tool.name).sort()).toEqual(["get-profile", "get-status", "list-todos"]); - - const objectTool = proxiedTools.find((tool) => tool.name === "get-profile")!; - const arrayTool = proxiedTools.find((tool) => tool.name === "list-todos")!; - const unionTool = proxiedTools.find((tool) => tool.name === "get-status")!; - expect(objectTool.outputSchema).toEqual({ - type: "object", - properties: { name: { type: "string" } }, - required: ["name"], - additionalProperties: false, - }); - expect(arrayTool.outputSchema).toBeUndefined(); - expect(unionTool.outputSchema).toBeUndefined(); - - const profile = await client.callTool({ name: "get-profile", arguments: {} }); - expect(profile.isError).not.toBe(true); - expect(profile.structuredContent).toEqual({ name: "Ada" }); - - // The dropped schema means no `structuredContent` is required or expected; the value still - // reaches the agent as JSON text. - const todos = await client.callTool({ name: "list-todos", arguments: {} }); - expect(todos.isError).not.toBe(true); - expect(todos.structuredContent).toBeUndefined(); - expect(todos.content).toEqual([{ type: "text", text: JSON.stringify(["write tests", "ship it"]) }]); - - // A dropped schema never turns a good result into an error: the union tool's result *is* an - // object, so it still travels as `structuredContent` — the client just has no schema to - // validate it against, which is allowed. - const status = await client.callTool({ name: "get-status", arguments: {} }); - expect(status.isError).not.toBe(true); - expect(status.structuredContent).toEqual({ ok: true }); - expect(status.content).toEqual([{ type: "text", text: JSON.stringify({ ok: true }) }]); - - app.socket.close(); - }); - - test("an object output schema paired with a non-object result is a tool_output_validation_error, not a client protocol error", async () => { - const { daemon, stateDir, port } = await startTestDaemon(); - const app = await claimApp(daemon, port); - await snapshotTools(daemon, app, [ - { name: "lies", description: "Claims an object, returns a number.", output_schema: { type: "object" } }, - ]); - - const handle = await createMcpHandle(stateDir); - const client = await connectInMemoryClient(handle); - - app.socket.on("message", (data) => { - const msg = JSON.parse(data.toString("utf8")) as Record; - - if (msg.type === "tool_call") { - app.socket.send( - JSON.stringify({ type: "tool_result", session_id: app.sessionId, id: msg.id, result: 42 }), - ); - } - }); - - await client.listTools(); - - // `callTool` (not raw `request`) so the SDK's "has an output schema but did not return - // structured content" guard is live: an `isError` result is the one shape it accepts. - const result = await client.callTool({ name: "lies", arguments: {} }); - - expect(result.isError).toBe(true); - expect(result.structuredContent).toBeUndefined(); - expect((result.content as Array<{ text: string }>)[0]!.text).toContain("tool_output_validation_error"); - expect((result.content as Array<{ text: string }>)[0]!.text).toContain("a number"); - - app.socket.close(); - }); - - // The issue's own example of a result that breaks the `structuredContent` contract. - test("an object output schema paired with a null result is a tool_output_validation_error", async () => { - const { daemon, stateDir, port } = await startTestDaemon(); - const app = await claimApp(daemon, port); - await snapshotTools(daemon, app, [ - { name: "nullish", description: "Claims an object, returns null.", output_schema: { type: "object" } }, - ]); - - const handle = await createMcpHandle(stateDir); - const client = await connectInMemoryClient(handle); - - app.socket.on("message", (data) => { - const msg = JSON.parse(data.toString("utf8")) as Record; - - if (msg.type === "tool_call") { - app.socket.send( - JSON.stringify({ type: "tool_result", session_id: app.sessionId, id: msg.id, result: null }), - ); - } - }); - - await client.listTools(); - const result = await client.callTool({ name: "nullish", arguments: {} }); - - expect(result.isError).toBe(true); - const text = (result.content as Array<{ text: string }>)[0]!.text; - expect(text).toContain("tool_output_validation_error"); - expect(text).toContain("returned null"); - // Never the raw `typeof` wording: "a object" / "a undefined" would read as a bug in the tool. - expect(text).not.toContain("a undefined"); - expect(text).not.toContain("a object"); - - app.socket.close(); - }); - test("a tool declaring a timeoutMs above the daemon default gets it, over MCP, end to end (issue #25)", async () => { const { daemon, stateDir, port } = await startTestDaemon(); const app = await claimApp(daemon, port); @@ -642,167 +415,6 @@ describe("mcp: tools/list and tools/call", () => { app.socket.close(); }); - test("the generated MCP tool list (built-ins + one proxied tool) matches the locked mapping", async () => { - const { daemon, stateDir, port } = await startTestDaemon(); - const app = await claimApp(daemon, port); - await snapshotTools(daemon, app, [ - { - name: "echo", - description: "Echoes its input.", - input_schema: { type: "object", properties: { text: { type: "string" } } }, - }, - ]); - - const handle = await createMcpHandle(stateDir); - const client = await connectInMemoryClient(handle); - - const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - // Names only, sorted: full descriptors (incl. built-ins' free-text descriptions) would make this - // snapshot brittle against unrelated wording tweaks; the shape/schema mapping is what's locked. - const shapes = listed.tools - .map((tool) => ({ - name: tool.name, - inputSchema: tool.inputSchema, - outputSchema: tool.outputSchema, - annotations: tool.annotations, - })) - .sort((a, b) => a.name.localeCompare(b.name)); - - expect(shapes).toMatchSnapshot(); - - app.socket.close(); - }); - - test("a tool without an input_schema gets a permissive object schema", async () => { - const { daemon, stateDir, port } = await startTestDaemon(); - const app = await claimApp(daemon, port); - await snapshotTools(daemon, app, [{ name: "no-schema" }]); - - const handle = await createMcpHandle(stateDir); - const client = await connectInMemoryClient(handle); - - const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - const proxiedTools = withoutBuiltinTools(listed.tools); - expect(proxiedTools[0]!.inputSchema).toEqual({ type: "object", additionalProperties: true }); - - app.socket.close(); - }); - - test("annotations map verbatim onto the MCP tool", async () => { - const { daemon, stateDir, port } = await startTestDaemon(); - const app = await claimApp(daemon, port); - const toolsChanged = waitForEvent(daemon, "tools_changed"); - app.socket.send( - JSON.stringify({ - type: "tool_registry_snapshot", - session_id: app.sessionId, - tools: [ - { - name: "destructive-tool", - description: "Deletes things.", - annotations: { destructiveHint: true, readOnlyHint: false }, - }, - ], - }), - ); - await toolsChanged; - - const handle = await createMcpHandle(stateDir); - const client = await connectInMemoryClient(handle); - const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - const proxiedTools = withoutBuiltinTools(listed.tools); - - expect(proxiedTools[0]!.annotations).toEqual({ destructiveHint: true, readOnlyHint: false }); - - app.socket.close(); - }); - - test("an app tool_error's type and message are preserved in the MCP error content, not thrown as a protocol error", async () => { - const { daemon, stateDir, port } = await startTestDaemon(); - const app = await claimApp(daemon, port); - await snapshotTools(daemon, app, [{ name: "boom" }]); - - app.socket.on("message", (data) => { - const msg = JSON.parse(data.toString("utf8")) as Record; - - if (msg.type === "tool_call") { - app.socket.send( - JSON.stringify({ - type: "tool_error", - session_id: app.sessionId, - id: msg.id, - error: { type: "tool_execution_error", message: "boom failed" }, - }), - ); - } - }); - - const handle = await createMcpHandle(stateDir); - const client = await connectInMemoryClient(handle); - - const result = await client.request( - { method: "tools/call", params: { name: "boom", arguments: {} } }, - CallToolResultSchema, - ); - - expect(result.isError).toBe(true); - const text = (result.content[0] as { text: string }).text; - expect(text).toContain("tool_execution_error"); - expect(text).toContain("boom failed"); - - app.socket.close(); - }); - - test("calling an unregistered tool returns tool_not_found error content", async () => { - const { stateDir } = await startTestDaemon(); - const handle = await createMcpHandle(stateDir); - const client = await connectInMemoryClient(handle); - - const result = await client.request( - { method: "tools/call", params: { name: "does-not-exist", arguments: {} } }, - CallToolResultSchema, - ); - - expect(result.isError).toBe(true); - expect((result.content[0] as { text: string }).text).toContain("tool_not_found"); - }); -}); - -describe("mcp: namespacing and list_changed", () => { - test("a single live session exposes tools under their own names; a second flips to __ and fires list_changed", async () => { - const { daemon, stateDir, port } = await startTestDaemon(); - const appA = await claimApp(daemon, port, "Pixel 8"); - await snapshotTools(daemon, appA, [{ name: "echo" }]); - - const handle = await createMcpHandle(stateDir); - const client = await connectInMemoryClient(handle); - - const singleSessionListing = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - expect(withoutBuiltinTools(singleSessionListing.tools).map((tool) => tool.name)).toEqual(["echo"]); - - let listChangedCount = 0; - client.setNotificationHandler(ToolListChangedNotificationSchema, () => { - listChangedCount += 1; - }); - - const appB = await claimApp(daemon, port, "iPhone 15"); - await snapshotTools(daemon, appB, [{ name: "echo" }]); - - // The daemon event that flips the namespacing (appB's own tools_changed) has already fired by - // the time `snapshotTools` resolves; give the MCP server's own event handling a beat to catch up. - await new Promise((resolve) => setTimeout(resolve, 100)); - - expect(listChangedCount).toBeGreaterThan(0); - - const multiSessionListing = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); - const names = withoutBuiltinTools(multiSessionListing.tools) - .map((tool) => tool.name) - .sort(); - expect(names).toEqual([`${appA.alias}__echo`, `${appB.alias}__echo`].sort()); - - appA.socket.close(); - appB.socket.close(); - }); }); describe("mcp: appduct_connect / appduct_wait_for_session", () => { diff --git a/packages/appduct/src/__tests__/mcp-server.test.ts b/packages/appduct/src/__tests__/mcp-server.test.ts new file mode 100644 index 00000000..a1b8a0ea --- /dev/null +++ b/packages/appduct/src/__tests__/mcp-server.test.ts @@ -0,0 +1,346 @@ +/** + * The MCP server's own behaviour (ARCHITECTURE.md §9), against an in-memory daemon + * (`mcp-daemon-fake.ts`) rather than a real one: tool-name mapping, output-schema degradation + * (issue #26), `__` namespacing and `notifications/tools/list_changed`. + * + * None of that depends on the transport underneath. These cases used to boot a real daemon — a + * pidfile, a self-signed certificate, a wss listener — and script a fake app over a real + * WebSocket, to assert on a JSON schema the server rewrote on its way out. The server is driven by + * the SDK's own `Client` over `InMemoryTransport` exactly as before, so what the client sees is + * still what a real client would see; only the daemon behind it is a fake. + * + * `mcp-server.integration.test.ts` keeps everything that genuinely needs the real thing: progress + * correlation over the second daemon stream, cancellation, the `appduct_connect` delivery paths, + * the resource, stdout purity, and version drift. + */ + +import { afterEach, describe, expect, test } from "vitest"; + +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import { + CallToolResultSchema, + ListToolsResultSchema, + ToolListChangedNotificationSchema, +} from "@modelcontextprotocol/sdk/types.js"; + +import { createMcpServer, type McpServerHandle } from "../mcp/server.js"; +import { createFakeDaemon, toolError, type FakeDaemon } from "./mcp-daemon-fake.js"; + +const mcpHandles: McpServerHandle[] = []; + +afterEach(async () => { + while (mcpHandles.length > 0) { + await mcpHandles.pop()?.close(); + } +}); + +const BUILTIN_TOOL_NAMES = new Set([ + "appduct_connect", + "appduct_wait_for_session", + "appduct_events", + "appduct_wait_for_event", +]); + +/** Every `tools/list` response always includes the built-in management tools alongside whatever + * proxied device tools are live; tests that care only about the proxied tools filter them here. */ +const withoutBuiltinTools = (tools: T[]): T[] => { + return tools.filter((tool) => !BUILTIN_TOOL_NAMES.has(tool.name)); +}; + +/** Starts an MCP server over `daemon` and connects an SDK `Client` to it in-process. `stateDir` is + * never touched: nothing here reaches the filesystem, because `openStream` is the only path the + * server has to a daemon and it is the fake's. */ +const startServerWithClient = async (daemon: FakeDaemon): Promise => { + const handle = await createMcpServer({ + stateDir: "/nonexistent-state-dir", + openStream: daemon.openStream, + scheme: "appduct", + env: {}, + }); + mcpHandles.push(handle); + + const [serverTransport, clientTransport] = InMemoryTransport.createLinkedPair(); + await handle.connect(serverTransport); + + const client = new Client({ name: "test-client", version: "0.0.0" }); + await client.connect(clientTransport); + + return client; +}; + +describe("mcp: tools/list and tools/call", () => { + test("a fake app's registered tools appear in tools/list with schemas and round-trip through tools/call", async () => { + const daemon = createFakeDaemon(); + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([ + { + name: "echo", + description: "Echoes its input.", + input_schema: { type: "object", properties: { text: { type: "string" } } }, + }, + ]); + app.onCall("echo", (args) => ({ echoed: args.text })); + + const client = await startServerWithClient(daemon); + + const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); + const proxiedTools = withoutBuiltinTools(listed.tools); + expect(proxiedTools).toHaveLength(1); + expect(proxiedTools[0]!.name).toBe("echo"); + expect(proxiedTools[0]!.description).toBe("Echoes its input."); + expect(proxiedTools[0]!.inputSchema).toEqual({ type: "object", properties: { text: { type: "string" } } }); + + const called = await client.request( + { method: "tools/call", params: { name: "echo", arguments: { text: "hello" } } }, + CallToolResultSchema, + ); + + expect(called.isError).not.toBe(true); + expect(called.structuredContent).toEqual({ echoed: "hello" }); + }); + + test("a non-object output schema does not break tools/list: both tools list and both stay callable", async () => { + const daemon = createFakeDaemon(); + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([ + { + name: "get-profile", + description: "Returns the profile.", + output_schema: { + type: "object", + properties: { name: { type: "string" } }, + required: ["name"], + additionalProperties: false, + }, + }, + { + // `z.array(z.string())`: MCP's `Tool.outputSchema.type` is the literal `"object"`, so + // before issue #26 this single entry made the client reject the whole list. + name: "list-todos", + description: "Returns the todos.", + output_schema: { type: "array", items: { type: "string" } }, + }, + { + // `z.union([z.object(...), z.object(...)])`: `anyOf` with no root `type`, so MCP rejects + // it even though every branch — and every result — is an object. + name: "get-status", + description: "Returns one of two shapes.", + output_schema: { + anyOf: [ + { type: "object", properties: { ok: { type: "boolean" } }, required: ["ok"] }, + { type: "object", properties: { error: { type: "string" } }, required: ["error"] }, + ], + }, + }, + ]); + app.onCall("get-profile", () => ({ name: "Ada" })); + app.onCall("list-todos", () => ["write tests", "ship it"]); + app.onCall("get-status", () => ({ ok: true })); + + const client = await startServerWithClient(daemon); + + // The SDK's own `listTools`, so the result goes through `ListToolsResultSchema` *and* caches + // the output schemas `callTool` below enforces — exactly what a real client does. + const listed = await client.listTools(); + const proxiedTools = withoutBuiltinTools(listed.tools); + expect(proxiedTools.map((tool) => tool.name).sort()).toEqual(["get-profile", "get-status", "list-todos"]); + + const objectTool = proxiedTools.find((tool) => tool.name === "get-profile")!; + const arrayTool = proxiedTools.find((tool) => tool.name === "list-todos")!; + const unionTool = proxiedTools.find((tool) => tool.name === "get-status")!; + expect(objectTool.outputSchema).toEqual({ + type: "object", + properties: { name: { type: "string" } }, + required: ["name"], + additionalProperties: false, + }); + expect(arrayTool.outputSchema).toBeUndefined(); + expect(unionTool.outputSchema).toBeUndefined(); + + const profile = await client.callTool({ name: "get-profile", arguments: {} }); + expect(profile.isError).not.toBe(true); + expect(profile.structuredContent).toEqual({ name: "Ada" }); + + // The dropped schema means no `structuredContent` is required or expected; the value still + // reaches the agent as JSON text. + const todos = await client.callTool({ name: "list-todos", arguments: {} }); + expect(todos.isError).not.toBe(true); + expect(todos.structuredContent).toBeUndefined(); + expect(todos.content).toEqual([{ type: "text", text: JSON.stringify(["write tests", "ship it"]) }]); + + // A dropped schema never turns a good result into an error: the union tool's result *is* an + // object, so it still travels as `structuredContent` — the client just has no schema to + // validate it against, which is allowed. + const status = await client.callTool({ name: "get-status", arguments: {} }); + expect(status.isError).not.toBe(true); + expect(status.structuredContent).toEqual({ ok: true }); + expect(status.content).toEqual([{ type: "text", text: JSON.stringify({ ok: true }) }]); + }); + + test("an object output schema paired with a non-object result is a tool_output_validation_error, not a client protocol error", async () => { + const daemon = createFakeDaemon(); + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([ + { name: "lies", description: "Claims an object, returns a number.", output_schema: { type: "object" } }, + ]); + app.onCall("lies", () => 42); + + const client = await startServerWithClient(daemon); + await client.listTools(); + + // `callTool` (not raw `request`) so the SDK's "has an output schema but did not return + // structured content" guard is live: an `isError` result is the one shape it accepts. + const result = await client.callTool({ name: "lies", arguments: {} }); + + expect(result.isError).toBe(true); + expect(result.structuredContent).toBeUndefined(); + expect((result.content as Array<{ text: string }>)[0]!.text).toContain("tool_output_validation_error"); + expect((result.content as Array<{ text: string }>)[0]!.text).toContain("a number"); + }); + + // The issue's own example of a result that breaks the `structuredContent` contract. + test("an object output schema paired with a null result is a tool_output_validation_error", async () => { + const daemon = createFakeDaemon(); + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([ + { name: "nullish", description: "Claims an object, returns null.", output_schema: { type: "object" } }, + ]); + app.onCall("nullish", () => null); + + const client = await startServerWithClient(daemon); + await client.listTools(); + const result = await client.callTool({ name: "nullish", arguments: {} }); + + expect(result.isError).toBe(true); + const text = (result.content as Array<{ text: string }>)[0]!.text; + expect(text).toContain("tool_output_validation_error"); + expect(text).toContain("returned null"); + // Never the raw `typeof` wording: "a object" / "a undefined" would read as a bug in the tool. + expect(text).not.toContain("a undefined"); + expect(text).not.toContain("a object"); + }); + + test("the generated MCP tool list (built-ins + one proxied tool) matches the locked mapping", async () => { + const daemon = createFakeDaemon(); + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([ + { + name: "echo", + description: "Echoes its input.", + input_schema: { type: "object", properties: { text: { type: "string" } } }, + }, + ]); + + const client = await startServerWithClient(daemon); + + const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); + // Names only, sorted: full descriptors (incl. built-ins' free-text descriptions) would make this + // snapshot brittle against unrelated wording tweaks; the shape/schema mapping is what's locked. + const shapes = listed.tools + .map((tool) => ({ + name: tool.name, + inputSchema: tool.inputSchema, + outputSchema: tool.outputSchema, + annotations: tool.annotations, + })) + .sort((a, b) => a.name.localeCompare(b.name)); + + expect(shapes).toMatchSnapshot(); + }); + + test("a tool without an input_schema gets a permissive object schema", async () => { + const daemon = createFakeDaemon(); + daemon.addSession({ alias: "pixel-8" }).setTools([{ name: "no-schema" }]); + + const client = await startServerWithClient(daemon); + + const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); + const proxiedTools = withoutBuiltinTools(listed.tools); + expect(proxiedTools[0]!.inputSchema).toEqual({ type: "object", additionalProperties: true }); + }); + + test("annotations map verbatim onto the MCP tool", async () => { + const daemon = createFakeDaemon(); + daemon.addSession({ alias: "pixel-8" }).setTools([ + { + name: "destructive-tool", + description: "Deletes things.", + annotations: { destructiveHint: true, readOnlyHint: false }, + }, + ]); + + const client = await startServerWithClient(daemon); + const listed = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); + const proxiedTools = withoutBuiltinTools(listed.tools); + + expect(proxiedTools[0]!.annotations).toEqual({ destructiveHint: true, readOnlyHint: false }); + }); + + test("an app tool_error's type and message are preserved in the MCP error content, not thrown as a protocol error", async () => { + const daemon = createFakeDaemon(); + const app = daemon.addSession({ alias: "pixel-8" }); + app.setTools([{ name: "boom" }]); + app.onCall("boom", () => { + throw toolError("tool_execution_error", "boom failed"); + }); + + const client = await startServerWithClient(daemon); + + const result = await client.request( + { method: "tools/call", params: { name: "boom", arguments: {} } }, + CallToolResultSchema, + ); + + expect(result.isError).toBe(true); + const text = (result.content[0] as { text: string }).text; + expect(text).toContain("tool_execution_error"); + expect(text).toContain("boom failed"); + }); + + test("calling an unregistered tool returns tool_not_found error content", async () => { + const daemon = createFakeDaemon(); + const client = await startServerWithClient(daemon); + + const result = await client.request( + { method: "tools/call", params: { name: "does-not-exist", arguments: {} } }, + CallToolResultSchema, + ); + + expect(result.isError).toBe(true); + expect((result.content[0] as { text: string }).text).toContain("tool_not_found"); + }); +}); + +describe("mcp: namespacing and list_changed", () => { + test("a single live session exposes tools under their own names; a second flips to __ and fires list_changed", async () => { + const daemon = createFakeDaemon(); + const appA = daemon.addSession({ alias: "pixel-8", deviceModel: "Pixel 8" }); + appA.setTools([{ name: "echo" }]); + + const client = await startServerWithClient(daemon); + + const singleSessionListing = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); + expect(withoutBuiltinTools(singleSessionListing.tools).map((tool) => tool.name)).toEqual(["echo"]); + + let listChangedCount = 0; + client.setNotificationHandler(ToolListChangedNotificationSchema, () => { + listChangedCount += 1; + }); + + const appB = daemon.addSession({ alias: "iphone-15", deviceModel: "iPhone 15" }); + appB.setTools([{ name: "echo" }]); + + // The daemon event that flips the namespacing (appB's own tools_changed) is pushed + // synchronously; give the MCP server's own async refresh + notify a beat to catch up. + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(listChangedCount).toBeGreaterThan(0); + + const multiSessionListing = await client.request({ method: "tools/list", params: {} }, ListToolsResultSchema); + const names = withoutBuiltinTools(multiSessionListing.tools) + .map((tool) => tool.name) + .sort(); + expect(names).toEqual([`${appA.alias}__echo`, `${appB.alias}__echo`].sort()); + }); +}); diff --git a/packages/appduct/src/__tests__/pidfile.test.ts b/packages/appduct/src/__tests__/pidfile.test.ts new file mode 100644 index 00000000..2328f5f3 --- /dev/null +++ b/packages/appduct/src/__tests__/pidfile.test.ts @@ -0,0 +1,121 @@ +/** + * `daemon/pidfile.ts`'s liveness probe (ARCHITECTURE.md §4). Every "is a daemon still there?" + * decision in the codebase goes through `isProcessAlive` — pidfile takeover, the auto-spawn path's + * stale-socket unlink, log rotation — so what it answers for a zombie decides all three. + */ + +import { spawn, spawnSync } from "node:child_process"; +import { readFileSync } from "node:fs"; + +import { describe, expect, test } from "vitest"; + +import { isProcessAlive, type ProcStatusReader } from "../daemon/pidfile.js"; + +/** + * A pid that is genuinely gone: spawn a no-op child and wait for it to exit. `spawnSync` reaps it, + * so by the time this returns the pid is free of any table entry at all. + */ +const deadPid = (): number => { + const child = spawnSync(process.execPath, ["-e", "process.exit(0)"]); + expect(child.status).toBe(0); + expect(child.pid).toBeGreaterThan(0); + return child.pid!; +}; + +/** + * A `/proc//status` body, in the shape the kernel writes it: `Name`, then `State:\tX (word)`. + * + * Node reaps its own children automatically (libuv installs a SIGCHLD handler), so a child of + * this process is never a zombie. The cases below therefore supply the bytes `/proc` would have + * contained through the reader seam, so the decision under test — "a pid that `kill(pid, 0)` + * accepts is still dead if procfs says Z" — runs on every platform. On Linux one extra case also + * arranges a real zombie (a grandchild whose parent never waits) and goes through the default + * procfs reader, so the parsing is checked against what the kernel actually writes. + */ +const procStatus = (state: string, name = "appduct"): string => { + return `Name:\t${name}\nUmask:\t0022\nState:\t${state}\nTgid:\t1\n`; +}; + +describe("isProcessAlive", () => { + test("a running process is alive", () => { + expect(isProcessAlive(process.pid)).toBe(true); + }); + + test("a genuinely-exited process is dead", () => { + expect(isProcessAlive(deadPid())).toBe(false); + }); + + test("a zombie is dead, even though process.kill(pid, 0) succeeds for it", () => { + // `process.pid` is the one pid guaranteed to pass `kill(pid, 0)` here, so using it isolates + // the assertion to the procfs half: without the zombie check this is unconditionally `true`. + expect(isProcessAlive(process.pid, () => procStatus("Z (zombie)"))).toBe(false); + }); + + test("every other process state is left alone", () => { + for (const state of ["R (running)", "S (sleeping)", "D (disk sleep)", "T (stopped)", "I (idle)"]) { + expect(isProcessAlive(process.pid, () => procStatus(state))).toBe(true); + } + }); + + test("an unreadable /proc leaves process.kill(pid, 0)'s answer standing", () => { + // What macOS, Windows, a hardened container and a pid we lack permission on all look like. + // Falling back to "alive" here is the safe direction: declaring a live daemon dead would have + // the next command clobber its pidfile and socket out from under it. + const unreadable: ProcStatusReader = () => undefined; + + expect(isProcessAlive(process.pid, unreadable)).toBe(true); + expect(isProcessAlive(deadPid(), unreadable)).toBe(false); + }); + + test("a status that never mentions State is not read as a zombie", () => { + // Defensive: a truncated read, or a future procfs that reorders fields, must not be able to + // turn into a "dead" verdict by accident. + expect(isProcessAlive(process.pid, () => "Name:\tappduct\n")).toBe(true); + // And `Z` must be the *state*, not merely a letter somewhere on the line. + expect(isProcessAlive(process.pid, () => procStatus("S (sleeping)", "Zygote"))).toBe(true); + }); + + // Linux-only: the default reader reads procfs, which nothing else has. `sh` backgrounds a + // `sleep 1` and then execs into `sleep 30`, which never calls wait(), so once the `sleep 1` + // finishes it stays a zombie child of it for as long as the `sleep 30` lives — a real zombie, no + // container needed. (The background sleep outlives the `exec`, so `sh` never gets to reap it.) + test.runIf(process.platform === "linux")( + "a real zombie is dead through the default /proc reader", + async () => { + const parent = spawn("/bin/sh", ["-c", "sleep 1 & echo $!; exec sleep 30"], { + stdio: ["ignore", "pipe", "ignore"], + }); + + try { + const zombiePid = await new Promise((resolve, reject) => { + parent.once("error", reject); + parent.stdout.once("data", (chunk: Buffer) => resolve(Number.parseInt(chunk.toString("utf8"), 10))); + }); + expect(zombiePid).toBeGreaterThan(0); + + const stateOf = (): string | undefined => { + try { + return /^State:\s*(\S)/mu.exec(readFileSync(`/proc/${zombiePid}/status`, "utf8"))?.[1]; + } catch { + return undefined; + } + }; + const deadline = Date.now() + 4000; + + while (stateOf() !== "Z" && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + + expect(stateOf()).toBe("Z"); + // The premise: signalling still succeeds, which is exactly why `kill(pid, 0)` alone is wrong. + expect(() => process.kill(zombiePid, 0)).not.toThrow(); + expect(isProcessAlive(zombiePid)).toBe(false); + // And the parent that is merely sleeping reads as alive through the same reader. + expect(isProcessAlive(parent.pid!)).toBe(true); + } finally { + parent.kill("SIGKILL"); + } + }, + 10_000, + ); +}); diff --git a/packages/appduct/src/__tests__/policy-and-audit.integration.test.ts b/packages/appduct/src/__tests__/policy-and-audit.integration.test.ts index 5689e2e7..129ec4ae 100644 --- a/packages/appduct/src/__tests__/policy-and-audit.integration.test.ts +++ b/packages/appduct/src/__tests__/policy-and-audit.integration.test.ts @@ -5,8 +5,8 @@ * (same pattern as `mcp-server.integration.test.ts`) for the `caller` attribution case. */ -import { connect as connectUds, createServer as createNetServer, type Socket } from "node:net"; -import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { connect as connectUds, type Socket } from "node:net"; +import { mkdtemp, readFile, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; @@ -23,7 +23,7 @@ import { loadConfig } from "../daemon/config.js"; import { startDaemon, type RunningDaemon } from "../daemon/daemon.js"; import { getStateDirPaths } from "../daemon/state-dir.js"; import { createMcpServer, type McpServerHandle } from "../mcp/server.js"; -import { writeTestHostKey } from "./fixtures.js"; +import { makeTempStateDir, removeStateDir } from "./fixtures.js"; process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; @@ -41,22 +41,10 @@ afterEach(async () => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); -const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - type TestDaemon = { daemon: RunningDaemon; stateDir: string; @@ -64,20 +52,16 @@ type TestDaemon = { }; const startTestDaemon = async (configOverrides: Record = {}): Promise => { - const stateDir = await mkdtemp(path.join(tmpdir(), "appduct-policy-audit-")); + const stateDir = await makeTempStateDir(configOverrides, { prefix: "appduct-policy-audit-" }); stateDirs.push(stateDir); - await writeTestHostKey(path.join(stateDir, "key.pem")); - - const port = await pickFreePort(); - await writeFile( - path.join(stateDir, "config.json"), - JSON.stringify({ wssPort: port, advertisedIp: "127.0.0.1", ...configOverrides }), - ); const daemon = await startDaemon({ stateDir }); runningDaemons.push(daemon); - return { daemon, stateDir, port }; + // The daemon's `config.json` asks for an OS-assigned port (`wssPort: 0`), so the real port is + // only knowable from the listener that bound it — never pre-picked, which is what used to race + // another vitest process for the same number. + return { daemon, stateDir, port: daemon.listener.port()! }; }; /** Removes `daemon` from the tracked list and shuts it down immediately — used mid-test so a diff --git a/packages/appduct/src/__tests__/rpc-client.integration.test.ts b/packages/appduct/src/__tests__/rpc-client.integration.test.ts new file mode 100644 index 00000000..805abc3e --- /dev/null +++ b/packages/appduct/src/__tests__/rpc-client.integration.test.ts @@ -0,0 +1,167 @@ +/** + * `rpc/client.ts` against a **real daemon** on the other end of the control socket: the direct + * connect, a real JSON-RPC error, the injected-spawn auto-spawn path, the spawn-lock race, and + * `openDaemonStream`'s call + server-pushed-notification round-trip. + * + * Split out of `rpc-client.test.ts`, which keeps everything the client decides on its own — the + * version-drift logic in particular, which runs against a hand-rolled fake daemon because a real + * one can only ever report this build's own version. + */ + +import { afterEach, describe, expect, test } from "vitest"; + +import { startDaemon, type RunningDaemon } from "../daemon/daemon.js"; +import { + callDaemon, + DaemonUnavailableError, + openDaemonStream, + resetDaemonVersionChecks, + type SpawnFn, +} from "../rpc/client.js"; +import { makeTempStateDir as makeSharedStateDir, removeStateDir } from "./fixtures.js"; + +const runningDaemons: RunningDaemon[] = []; +const stateDirs: string[] = []; + +afterEach(async () => { + while (runningDaemons.length > 0) { + await runningDaemons.pop()?.shutdown(); + } + + // The per-process version-check cache is global; leaking it would skip the next test's check. + resetDaemonVersionChecks(); + + while (stateDirs.length > 0) { + await removeStateDir(stateDirs.pop()!); + } +}); + +/** The shared fixture's `wssPort: 0` matters here: this file's ancestor wrote no `config.json` at + * all, so every `startDaemon` below bound the default 8443 and raced every other daemon on the + * machine — including the ones a second vitest process is running. */ +const makeTempStateDir = async (): Promise => { + const stateDir = await makeSharedStateDir({}, { prefix: "appduct-rpc-client-integration-" }); + stateDirs.push(stateDir); + return stateDir; +}; + +describe("callDaemon against a real daemon", () => { + test("connects directly when a daemon is already listening", async () => { + const stateDir = await makeTempStateDir(); + const daemon = await startDaemon({ stateDir }); + runningDaemons.push(daemon); + + const status = await callDaemon<{ pid: number }>( + "daemon.status", + {}, + { stateDir, autoSpawn: false }, + ); + + expect(status.pid).toBe(process.pid); + + await removeStateDir(stateDir); + }); + + test("propagates a JSON-RPC error for an unknown method", async () => { + const stateDir = await makeTempStateDir(); + const daemon = await startDaemon({ stateDir }); + runningDaemons.push(daemon); + + await expect( + callDaemon("nonexistent.method", {}, { stateDir, autoSpawn: false }), + ).rejects.toThrow(/Method not found/u); + + await removeStateDir(stateDir); + }); + + test("auto-spawns an in-process daemon via the injected spawn fn and retries the request", async () => { + const stateDir = await makeTempStateDir(); + let spawnCalls = 0; + + const spawn: SpawnFn = async (args, context) => { + spawnCalls += 1; + expect(args).toEqual(["daemon", "run"]); + expect(context.stateDir).toBe(stateDir); + + const daemon = await startDaemon({ stateDir: context.stateDir }); + runningDaemons.push(daemon); + }; + + const status = await callDaemon<{ pid: number }>( + "daemon.status", + {}, + { stateDir, autoSpawn: true, spawn, spawnPollIntervalMs: 20, spawnWaitTimeoutMs: 2000 }, + ); + + expect(spawnCalls).toBe(1); + expect(status.pid).toBe(process.pid); + + await removeStateDir(stateDir); + }); + + test("concurrent auto-spawn race results in exactly one spawn (spawn-lock)", async () => { + const stateDir = await makeTempStateDir(); + let spawnCalls = 0; + + const spawn: SpawnFn = async (_args, context) => { + spawnCalls += 1; + // Simulate real spawn latency so both callers are genuinely racing. + await new Promise((resolve) => setTimeout(resolve, 50)); + const daemon = await startDaemon({ stateDir: context.stateDir }); + runningDaemons.push(daemon); + }; + + const options = { + stateDir, + autoSpawn: true, + spawn, + spawnPollIntervalMs: 20, + spawnWaitTimeoutMs: 3000, + } as const; + + const [first, second] = await Promise.all([ + callDaemon<{ pid: number }>("daemon.status", {}, options), + callDaemon<{ pid: number }>("daemon.status", {}, options), + ]); + + expect(spawnCalls).toBe(1); + expect(first.pid).toBe(process.pid); + expect(second.pid).toBe(process.pid); + expect(runningDaemons).toHaveLength(1); + + await removeStateDir(stateDir); + }); +}); + +describe("openDaemonStream", () => { + test("supports calls and delivers server-pushed notifications", async () => { + const stateDir = await makeTempStateDir(); + const daemon = await startDaemon({ stateDir }); + runningDaemons.push(daemon); + + const stream = await openDaemonStream({ stateDir, autoSpawn: false }); + + try { + const status = await stream.call<{ pid: number }>("daemon.status"); + expect(status.pid).toBe(process.pid); + + const received: unknown[] = []; + const unsubscribe = stream.onNotification((payload) => { + received.push(payload); + }); + + const [connection] = daemon.server.connections(); + expect(connection).toBeDefined(); + daemon.server.notify(connection!, { kind: "daemon_started", ts: 123, data: null }); + + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(received).toEqual([{ kind: "daemon_started", ts: 123, data: null }]); + + unsubscribe(); + } finally { + stream.close(); + } + + await removeStateDir(stateDir); + }); +}); diff --git a/packages/appduct/src/__tests__/rpc-client.test.ts b/packages/appduct/src/__tests__/rpc-client.test.ts index 26e3ef02..2ad8259c 100644 --- a/packages/appduct/src/__tests__/rpc-client.test.ts +++ b/packages/appduct/src/__tests__/rpc-client.test.ts @@ -1,11 +1,16 @@ -import { mkdtemp, rm, utimes, writeFile } from "node:fs/promises"; +/** + * `rpc/client.ts`'s decision logic, with **no real daemon anywhere**: the auto-spawn guard, the + * spawn-lock, and the whole of issue #30's version-drift check, which runs against the hand-rolled + * `FakeDaemon` below because a real `startDaemon` can only ever report this build's own version. + * + * The cases that genuinely need a real daemon on the other end of the socket live in + * `rpc-client.integration.test.ts`. + */ +import { rm, utimes, writeFile } from "node:fs/promises"; import { createServer, type Server, type Socket } from "node:net"; -import { tmpdir } from "node:os"; -import path from "node:path"; import { afterEach, describe, expect, test } from "vitest"; -import { startDaemon, type RunningDaemon } from "../daemon/daemon.js"; import { getStateDirPaths } from "../daemon/state-dir.js"; import { callDaemon, @@ -17,9 +22,8 @@ import { resetDaemonVersionChecks, type SpawnFn, } from "../rpc/client.js"; -import { writeTestHostKey } from "./fixtures.js"; +import { makeTempStateDir as makeSharedStateDir, removeStateDir } from "./fixtures.js"; -const runningDaemons: RunningDaemon[] = []; const fakeDaemons: FakeDaemon[] = []; afterEach(async () => { @@ -27,17 +31,21 @@ afterEach(async () => { await fakeDaemons.pop()?.stop(); } - while (runningDaemons.length > 0) { - const daemon = runningDaemons.pop(); - await daemon?.shutdown(); - } - resetDaemonVersionChecks(); + + while (stateDirs.length > 0) { + await removeStateDir(stateDirs.pop()!); + } }); +const stateDirs: string[] = []; + +/** A temp state dir for the fake daemons below. Nothing here binds a wss port — the fakes speak + * only the control socket — but it carries the shared fixture's `wssPort: 0` anyway, so that a + * case which ever does let a real daemon start cannot collide with another process's. */ const makeTempStateDir = async (): Promise => { - const stateDir = await mkdtemp(path.join(tmpdir(), "appduct-rpc-client-test-")); - await writeTestHostKey(path.join(stateDir, "key.pem")); + const stateDir = await makeSharedStateDir({}, { prefix: "appduct-rpc-client-test-" }); + stateDirs.push(stateDir); return stateDir; }; @@ -185,34 +193,6 @@ const startFakeDaemon = async ( }; describe("callDaemon", () => { - test("connects directly when a daemon is already listening", async () => { - const stateDir = await makeTempStateDir(); - const daemon = await startDaemon({ stateDir }); - runningDaemons.push(daemon); - - const status = await callDaemon<{ pid: number }>( - "daemon.status", - {}, - { stateDir, autoSpawn: false }, - ); - - expect(status.pid).toBe(process.pid); - - await rm(stateDir, { force: true, recursive: true }); - }); - - test("propagates a JSON-RPC error for an unknown method", async () => { - const stateDir = await makeTempStateDir(); - const daemon = await startDaemon({ stateDir }); - runningDaemons.push(daemon); - - await expect( - callDaemon("nonexistent.method", {}, { stateDir, autoSpawn: false }), - ).rejects.toThrow(/Method not found/u); - - await rm(stateDir, { force: true, recursive: true }); - }); - test("without autoSpawn, a missing daemon fails fast instead of spawning", async () => { const stateDir = await makeTempStateDir(); @@ -221,64 +201,6 @@ describe("callDaemon", () => { await rm(stateDir, { force: true, recursive: true }); }); - test("auto-spawns an in-process daemon via the injected spawn fn and retries the request", async () => { - const stateDir = await makeTempStateDir(); - let spawnCalls = 0; - - const spawn: SpawnFn = async (args, context) => { - spawnCalls += 1; - expect(args).toEqual(["daemon", "run"]); - expect(context.stateDir).toBe(stateDir); - - const daemon = await startDaemon({ stateDir: context.stateDir }); - runningDaemons.push(daemon); - }; - - const status = await callDaemon<{ pid: number }>( - "daemon.status", - {}, - { stateDir, autoSpawn: true, spawn, spawnPollIntervalMs: 20, spawnWaitTimeoutMs: 2000 }, - ); - - expect(spawnCalls).toBe(1); - expect(status.pid).toBe(process.pid); - - await rm(stateDir, { force: true, recursive: true }); - }); - - test("concurrent auto-spawn race results in exactly one spawn (spawn-lock)", async () => { - const stateDir = await makeTempStateDir(); - let spawnCalls = 0; - - const spawn: SpawnFn = async (_args, context) => { - spawnCalls += 1; - // Simulate real spawn latency so both callers are genuinely racing. - await new Promise((resolve) => setTimeout(resolve, 50)); - const daemon = await startDaemon({ stateDir: context.stateDir }); - runningDaemons.push(daemon); - }; - - const options = { - stateDir, - autoSpawn: true, - spawn, - spawnPollIntervalMs: 20, - spawnWaitTimeoutMs: 3000, - } as const; - - const [first, second] = await Promise.all([ - callDaemon<{ pid: number }>("daemon.status", {}, options), - callDaemon<{ pid: number }>("daemon.status", {}, options), - ]); - - expect(spawnCalls).toBe(1); - expect(first.pid).toBe(process.pid); - expect(second.pid).toBe(process.pid); - expect(runningDaemons).toHaveLength(1); - - await rm(stateDir, { force: true, recursive: true }); - }); - test("times out with DaemonUnavailableError when the spawned daemon never comes up", async () => { const stateDir = await makeTempStateDir(); const spawn: SpawnFn = () => { @@ -297,38 +219,6 @@ describe("callDaemon", () => { }); }); -describe("openDaemonStream", () => { - test("supports calls and delivers server-pushed notifications", async () => { - const stateDir = await makeTempStateDir(); - const daemon = await startDaemon({ stateDir }); - runningDaemons.push(daemon); - - const stream = await openDaemonStream({ stateDir, autoSpawn: false }); - - try { - const status = await stream.call<{ pid: number }>("daemon.status"); - expect(status.pid).toBe(process.pid); - - const received: unknown[] = []; - const unsubscribe = stream.onNotification((payload) => { - received.push(payload); - }); - - const [connection] = daemon.server.connections(); - expect(connection).toBeDefined(); - daemon.server.notify(connection!, { kind: "daemon_started", ts: 123, data: null }); - - await new Promise((resolve) => setTimeout(resolve, 20)); - expect(received).toEqual([{ kind: "daemon_started", ts: 123, data: null }]); - - unsubscribe(); - } finally { - stream.close(); - } - - await rm(stateDir, { force: true, recursive: true }); - }); -}); describe("daemon version check (issue #30)", () => { const CLIENT_VERSION = "9.9.9"; @@ -889,3 +779,4 @@ describe("daemon version check: a contended lock is not an ineffective restart ( await rm(stateDir, { force: true, recursive: true }); }); }); + diff --git a/packages/appduct/src/__tests__/scheme-discovery.integration.test.ts b/packages/appduct/src/__tests__/scheme-discovery.integration.test.ts index 124c7fbe..95dcf790 100644 --- a/packages/appduct/src/__tests__/scheme-discovery.integration.test.ts +++ b/packages/appduct/src/__tests__/scheme-discovery.integration.test.ts @@ -7,7 +7,6 @@ * free port and a key, but it never holds a `scheme`: that is the value under test. */ -import { createServer as createNetServer } from "node:net"; import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; @@ -16,7 +15,7 @@ import { afterEach, describe, expect, test } from "vitest"; import { handleLinkCommand } from "../commands/link.js"; import { startDaemon, type RunningDaemon } from "../daemon/daemon.js"; -import { writeTestHostKey } from "./fixtures.js"; +import { makeTempStateDir } from "./fixtures.js"; const runningDaemons: RunningDaemon[] = []; const directories: string[] = []; @@ -31,28 +30,10 @@ afterEach(async () => { } }); -const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - /** A daemon whose `config.json` deliberately carries no `scheme`. */ const startSchemelessDaemon = async (): Promise => { - const stateDir = await mkdtemp(path.join(tmpdir(), "appduct-discovery-state-")); + const stateDir = await makeTempStateDir({}, { prefix: "appduct-discovery-state-" }); directories.push(stateDir); - await writeTestHostKey(path.join(stateDir, "key.pem")); - - await writeFile( - path.join(stateDir, "config.json"), - JSON.stringify({ wssPort: await pickFreePort(), advertisedIp: "127.0.0.1" }), - ); runningDaemons.push(await startDaemon({ stateDir })); diff --git a/packages/appduct/src/__tests__/session-engine.integration.test.ts b/packages/appduct/src/__tests__/session-engine.integration.test.ts index adb6dfa1..c079ba07 100644 --- a/packages/appduct/src/__tests__/session-engine.integration.test.ts +++ b/packages/appduct/src/__tests__/session-engine.integration.test.ts @@ -6,10 +6,7 @@ * the WebSocket layer, and synchronizes on the in-process event bus instead of sleeping. */ -import { connect as connectUds, createServer as createNetServer, type Socket } from "node:net"; -import { mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import path from "node:path"; +import { connect as connectUds, type Socket } from "node:net"; import { afterEach, describe, expect, test } from "vitest"; import WebSocket from "ws"; @@ -17,7 +14,7 @@ import WebSocket from "ws"; import { decodeBootstrap, type EventKind, type EventNotification } from "@appduct/shared"; import { startDaemon, type RunningDaemon } from "../daemon/daemon.js"; -import { writeTestHostKey } from "./fixtures.js"; +import { makeTempStateDir, removeStateDir } from "./fixtures.js"; // Client pinning is the app's job (ARCHITECTURE.md task notes); tests skip it client-side. Under // Vitest runs these clients against a throwaway self-signed key, so the leaf-cert check is @@ -33,42 +30,26 @@ afterEach(async () => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); -const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - type TestDaemon = { daemon: RunningDaemon; port: number; }; const startTestDaemon = async (configOverrides: Record = {}): Promise => { - const stateDir = await mkdtemp(path.join(tmpdir(), "appduct-session-engine-")); + const stateDir = await makeTempStateDir(configOverrides, { prefix: "appduct-session-engine-" }); stateDirs.push(stateDir); - await writeTestHostKey(path.join(stateDir, "key.pem")); - - const port = await pickFreePort(); - await writeFile( - path.join(stateDir, "config.json"), - JSON.stringify({ wssPort: port, advertisedIp: "127.0.0.1", ...configOverrides }), - ); const daemon = await startDaemon({ stateDir }); runningDaemons.push(daemon); - return { daemon, port }; + // The daemon's `config.json` asks for an OS-assigned port (`wssPort: 0`), so the real port is + // only knowable from the listener that bound it — never pre-picked, which is what used to race + // another vitest process for the same number. + return { daemon, port: daemon.listener.port()! }; }; /** Raw newline-delimited JSON-RPC call over the daemon's UDS control socket. */ @@ -443,17 +424,7 @@ describe("session engine: rejection matrix (daemon and other sessions survive ev }); test("unclaimed socket idle past the pre-claim timeout closes 1008 pre_claim_timeout", async () => { - const stateDir = await mkdtemp(path.join(tmpdir(), "appduct-session-engine-")); - stateDirs.push(stateDir); - await writeTestHostKey(path.join(stateDir, "key.pem")); - const port = await pickFreePort(); - await writeFile( - path.join(stateDir, "config.json"), - JSON.stringify({ wssPort: port, advertisedIp: "127.0.0.1" }), - ); - - const daemon = await startDaemon({ stateDir }); - runningDaemons.push(daemon); + const { port } = await startTestDaemon(); const socket = await connectClient(port); const closed = nextClose(socket); diff --git a/packages/appduct/src/__tests__/tls-refresh.integration.test.ts b/packages/appduct/src/__tests__/tls-refresh.integration.test.ts index c9659afc..760c1344 100644 --- a/packages/appduct/src/__tests__/tls-refresh.integration.test.ts +++ b/packages/appduct/src/__tests__/tls-refresh.integration.test.ts @@ -11,16 +11,13 @@ * so the "network changed" case is deterministic without touching `os.networkInterfaces()` globally. */ -import { connect as connectUds, createServer as createNetServer, type Socket } from "node:net"; -import { mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import path from "node:path"; +import { connect as connectUds, type Socket } from "node:net"; import { afterEach, describe, expect, test } from "vitest"; import WebSocket from "ws"; import { startDaemon, type RunningDaemon } from "../daemon/daemon.js"; -import { writeTestHostKey } from "./fixtures.js"; +import { makeTempStateDir, removeStateDir } from "./fixtures.js"; // Client pinning is the app's job; tests skip it client-side for their throwaway self-signed key. process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; @@ -34,22 +31,10 @@ afterEach(async () => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); -const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - const rpcCall = (socketPath: string, method: string, params?: unknown): Promise => { return new Promise((resolve, reject) => { const socket: Socket = connectUds(socketPath); @@ -94,12 +79,10 @@ const connectClient = (port: number): Promise => { describe("TLS re-mint on advertised-IP change", () => { test("link.create re-detects the address, re-mints on change, and the listener keeps accepting connections", async () => { - const stateDir = await mkdtemp(path.join(tmpdir(), "appduct-tls-refresh-")); + // `advertisedIp` is deliberately left out: this case injects `detectAddress` below and the + // whole point is what detection returns, not a configured override. + const stateDir = await makeTempStateDir({ advertisedIp: undefined }, { prefix: "appduct-tls-refresh-" }); stateDirs.push(stateDir); - await writeTestHostKey(path.join(stateDir, "key.pem")); - - const port = await pickFreePort(); - await writeFile(path.join(stateDir, "config.json"), JSON.stringify({ wssPort: port })); // Startup consumes the first call (initial mint); the daemon starts advertising "127.0.0.1". // The next call ("still 127.0.0.1") must not force a re-mint; the call after that simulates the @@ -129,7 +112,8 @@ describe("TLS re-mint on advertised-IP change", () => { // must still succeed (this is exactly what a bare cert/key swap without `setSecureContext` // would fail to achieve: the old context would keep serving the stale SAN, or worse, the server // would need a restart). - const socket = await connectClient(port); + // The port the listener actually bound (the config asked for an OS-assigned one). + const socket = await connectClient(daemon.listener.port()!); socket.close(); const status = (await rpcCall(daemon.paths.socketPath, "daemon.status")) as { pid: number }; diff --git a/packages/appduct/src/__tests__/tool-invocation.integration.test.ts b/packages/appduct/src/__tests__/tool-invocation.integration.test.ts index f0cbd0d1..12af5e65 100644 --- a/packages/appduct/src/__tests__/tool-invocation.integration.test.ts +++ b/packages/appduct/src/__tests__/tool-invocation.integration.test.ts @@ -6,10 +6,7 @@ * daemon internals would only prove the mocks were called, not that the wire protocol works. */ -import { connect as connectUds, createServer as createNetServer, type Socket } from "node:net"; -import { mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import path from "node:path"; +import { connect as connectUds, type Socket } from "node:net"; import { afterEach, describe, expect, test } from "vitest"; import WebSocket from "ws"; @@ -25,7 +22,7 @@ import { import { connect } from "../client/index.js"; import { handleInvokeCommand } from "../commands/invoke.js"; import { startDaemon, type RunningDaemon } from "../daemon/daemon.js"; -import { writeTestHostKey } from "./fixtures.js"; +import { makeTempStateDir, removeStateDir } from "./fixtures.js"; // Client pinning is the app's job; tests skip it client-side for their throwaway self-signed key. process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; @@ -39,42 +36,26 @@ afterEach(async () => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); -const pickFreePort = async (): Promise => { - return new Promise((resolve, reject) => { - const server = createNetServer(); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const address = server.address(); - const port = address && typeof address !== "string" ? address.port : 0; - server.close(() => resolve(port)); - }); - }); -}; - type TestDaemon = { daemon: RunningDaemon; port: number; }; const startTestDaemon = async (configOverrides: Record = {}): Promise => { - const stateDir = await mkdtemp(path.join(tmpdir(), "appduct-tool-invocation-")); + const stateDir = await makeTempStateDir(configOverrides, { prefix: "appduct-tool-invocation-" }); stateDirs.push(stateDir); - await writeTestHostKey(path.join(stateDir, "key.pem")); - - const port = await pickFreePort(); - await writeFile( - path.join(stateDir, "config.json"), - JSON.stringify({ wssPort: port, advertisedIp: "127.0.0.1", ...configOverrides }), - ); const daemon = await startDaemon({ stateDir }); runningDaemons.push(daemon); - return { daemon, port }; + // The daemon's `config.json` asks for an OS-assigned port (`wssPort: 0`), so the real port is + // only knowable from the listener that bound it — never pre-picked, which is what used to race + // another vitest process for the same number. + return { daemon, port: daemon.listener.port()! }; }; /** Raw newline-delimited JSON-RPC call over the daemon's UDS control socket. */ diff --git a/packages/appduct/src/daemon/config.ts b/packages/appduct/src/daemon/config.ts index 6ef84cde..00820087 100644 --- a/packages/appduct/src/daemon/config.ts +++ b/packages/appduct/src/daemon/config.ts @@ -29,6 +29,15 @@ export type AppductPolicyConfig = { }; export type AppductConfig = { + /** + * TCP port for the pinned-wss listener (ARCHITECTURE.md §3). `0` is special and means + * "let the OS assign a free ephemeral port": the listener binds `0`, and everything that + * reports or advertises the port afterwards (`daemon.status`'s `wssPort`, a minted link's + * `endpoint.port`) reports the *bound* port instead. That is the only way several daemons can + * coexist on one machine without the operator hand-picking ports for each — which is exactly + * what the test suite needs when several vitest processes run concurrently. Every other value + * must be a positive integer. + */ wssPort: number; keyPath: string; graceSeconds: number; @@ -108,6 +117,19 @@ const configError = (key: string, message: string): AppductConfigError => { return new AppductConfigError(key, `Invalid Appduct config value for "${key}": ${message}`); }; +/** + * `wssPort` alone accepts `0` on top of the positive integers — it is not a degenerate port but a + * documented request for an OS-assigned one (see {@link AppductConfig.wssPort}). Kept separate + * from {@link requirePositiveInteger} so no *other* key silently gains a meaningless zero. + */ +const requireWssPort = (value: unknown, key: string): number => { + if (typeof value !== "number" || !Number.isInteger(value) || value < 0 || value > 65_535) { + throw configError(key, "must be a port number between 0 and 65535 (0 binds an OS-assigned port)."); + } + + return value; +}; + const requirePositiveInteger = (value: unknown, key: string): number => { if (typeof value !== "number" || !Number.isInteger(value) || value <= 0) { throw configError(key, "must be a positive integer."); @@ -193,7 +215,7 @@ export const loadConfig = async ( } if (parsed.wssPort !== undefined) { - config.wssPort = requirePositiveInteger(parsed.wssPort, "wssPort"); + config.wssPort = requireWssPort(parsed.wssPort, "wssPort"); } if (parsed.keyPath !== undefined) { diff --git a/packages/appduct/src/daemon/daemon.ts b/packages/appduct/src/daemon/daemon.ts index de74d252..7bc0e13d 100644 --- a/packages/appduct/src/daemon/daemon.ts +++ b/packages/appduct/src/daemon/daemon.ts @@ -98,6 +98,10 @@ export type RunningDaemon = { const buildStatusResult = async ( config: AppductConfig, + /** The port the listener actually bound. Differs from `config.wssPort` whenever that is `0` + * ("bind an OS-assigned port", ARCHITECTURE.md §3) — and a status that echoed the configured + * `0` back would be worse than useless: it is precisely the number nobody can connect to. */ + boundWssPort: number, startedAt: Date, tls: TlsManager, sessionManager: SessionManager, @@ -113,7 +117,7 @@ const buildStatusResult = async ( version: packageVersion(), pid: process.pid, startedAt: startedAt.toISOString(), - wssPort: config.wssPort, + wssPort: boundWssPort, pinnedKeys: tls.pinnedKeys(), sessions: sessionManager.list(), pendingLinks: sessionManager.pendingLinkCount(), @@ -345,6 +349,10 @@ export const startDaemon = async (options: DaemonOptions): Promise void; const exited = new Promise((resolve) => { @@ -401,7 +409,9 @@ export const startDaemon = async (options: DaemonOptions): Promise toAgentEndpoint(tls.current().advertisedAddress, config.wssPort), + // `boundWssPort`, not `config.wssPort`: a link minted by a daemon on an OS-assigned port + // must advertise the port the app can actually reach, not the `0` that asked for one. + getEndpoint: () => toAgentEndpoint(tls.current().advertisedAddress, boundWssPort), eventBus: activeEventBus, clock, onToolFrame: (message) => { @@ -443,6 +453,11 @@ export const startDaemon = async (options: DaemonOptions): Promise => { - return buildStatusResult(config, startedAt, tls, activeSessionManager, auditLogger, paths.auditDir); + return buildStatusResult( + config, + boundWssPort, + startedAt, + tls, + activeSessionManager, + auditLogger, + paths.auditDir, + ); }, [RPC_METHODS.daemonShutdown]: (_params, context): DaemonShutdownResult => { context.afterSend(() => { diff --git a/packages/appduct/src/daemon/pidfile.ts b/packages/appduct/src/daemon/pidfile.ts index 206a7a1f..6739322d 100644 --- a/packages/appduct/src/daemon/pidfile.ts +++ b/packages/appduct/src/daemon/pidfile.ts @@ -1,9 +1,10 @@ /** * Pidfile single-instancing (ARCHITECTURE.md §4). Acquired with `O_EXCL`; on conflict, liveness - * is checked with `process.kill(pid, 0)` and the pidfile is only taken over if the owning - * process is dead. + * is checked with `process.kill(pid, 0)` — plus, on Linux, a `/proc//status` read that treats + * a zombie as dead — and the pidfile is only taken over if the owning process is dead. */ +import { readFileSync } from "node:fs"; import { open, readFile, rm } from "node:fs/promises"; export class DaemonAlreadyRunningError extends Error { @@ -19,21 +20,72 @@ export type PidfileHandle = { release: () => Promise; }; +/** + * Reads `/proc//status`. Injectable purely so the zombie branch below can be tested without + * arranging a real unreaped child, which needs a process that outlives its parent *and* an init + * that does not reap — neither of which a test can rely on across platforms. + */ +export type ProcStatusReader = (pid: number) => string | undefined; + +const readProcStatus: ProcStatusReader = (pid) => { + try { + return readFileSync(`/proc/${pid}/status`, "utf8"); + } catch { + // No procfs, no such process any more, or no permission: the caller falls back to + // `process.kill(pid, 0)`'s answer, which is what this check was ever only refining. + return undefined; + } +}; + +/** + * True when `/proc` positively says this pid is a zombie — an exited process whose parent has not + * reaped it. + * + * A zombie still has a pid table entry, so `process.kill(pid, 0)` succeeds for it exactly as it + * does for a running process. That is the right answer for signalling (the pid is not free to be + * reused) and the wrong one for us: the daemon that pid names is gone, its socket is closed, and + * nothing will ever come back. Normally it is invisible, because PID 1 reaps orphans within + * milliseconds — but in a container whose PID 1 is a plain command rather than an init, nothing + * reaps, and a daemon that was SIGKILLed after its parent CLI exited stays a zombie for the life + * of the container. The pidfile then never looks stale, takeover never fires, and every later + * command reports a daemon that is already dead. + * + * Linux-only by construction: this reads procfs and returns false wherever it is absent or + * unreadable, which leaves `process.kill(pid, 0)` as the answer on macOS and everywhere else. + * There is no portable equivalent, and getting this wrong in the other direction — declaring a + * live daemon dead — would clobber a running daemon's state, so it only ever says "dead" on + * positive evidence. + */ +const isZombie = (pid: number, readStatus: ProcStatusReader): boolean => { + const status = readStatus(pid); + + if (status === undefined) { + return false; + } + + // `State:\tZ (zombie)` — one line, tab-separated, the state letter first. + return /^State:\s*Z\b/mu.test(status); +}; + /** * Liveness probe for a pid this process does not own (ARCHITECTURE.md §4's `process.kill(pid, 0)` - * check). Exported because every "is a daemon still there?" decision in the codebase must answer - * it the same way — pidfile takeover, the auto-spawn path's stale-socket unlink, and log rotation - * all hinge on it, and a second implementation that disagreed about `EPERM` would mean one of - * them quietly clobbering a live daemon's state. + * check, refined by the zombie check above). Exported because every "is a daemon still there?" + * decision in the codebase must answer it the same way — pidfile takeover, the auto-spawn path's + * stale-socket unlink, and log rotation all hinge on it, and a second implementation that + * disagreed about `EPERM` (or about zombies) would mean one of them quietly clobbering a live + * daemon's state. */ -export const isProcessAlive = (pid: number): boolean => { +export const isProcessAlive = (pid: number, readStatus: ProcStatusReader = readProcStatus): boolean => { try { process.kill(pid, 0); - return true; } catch (error) { // EPERM means the process exists but we lack permission to signal it — still alive. return (error as NodeJS.ErrnoException).code === "EPERM"; } + + // The signal landed, so a pid table entry exists. That is not the same as a process that can + // still serve anything. + return !isZombie(pid, readStatus); }; export const readPidFromFile = async (pidFilePath: string): Promise => { diff --git a/packages/appduct/src/mcp/server.ts b/packages/appduct/src/mcp/server.ts index 738e4350..fb4b061d 100644 --- a/packages/appduct/src/mcp/server.ts +++ b/packages/appduct/src/mcp/server.ts @@ -334,9 +334,26 @@ const proxiedToolResultContent = (tool: NamespacedTool, result: unknown): CallTo return toolSuccessContent(result); }; +/** + * How this server opens a daemon connection. Both the startup stream and each short-lived + * progress stream go through it, so a caller can put something other than a real daemon on the + * other end. The only production implementation is {@link openDaemonStream}; the seam exists so + * the behaviour that is purely this module's own — name mapping, schema degradation, consent + * flags, namespacing, `list_changed` — can be tested without a TLS listener, a pidfile and a + * subprocess, none of which those behaviours depend on. + */ +export type OpenDaemonStreamFn = (options: { + stateDir: string; + spawn?: SpawnFn; + checkVersion?: VersionCheckOptions; +}) => Promise; + export type CreateMcpServerOptions = { stateDir: string; spawn?: SpawnFn; + /** Overrides how daemon connections are opened; defaults to {@link openDaemonStream}. Test-only + * seam — every real caller (`cli/mcp-command.ts`) leaves it unset. */ + openStream?: OpenDaemonStreamFn; /** * Daemon/CLI version check (issue #30), applied to the startup stream only — never to the * short-lived progress streams below, which must never restart the daemon out from under a call @@ -371,10 +388,11 @@ export type McpServerHandle = { }; export const createMcpServer = async (options: CreateMcpServerOptions): Promise => { + const openStream = options.openStream ?? openDaemonStream; let stream: DaemonStream; try { - stream = await openDaemonStream({ + stream = await openStream({ stateDir: options.stateDir, spawn: options.spawn, checkVersion: options.checkVersion, @@ -501,7 +519,7 @@ export const createMcpServer = async (options: CreateMcpServerOptions): Promise< // `tool_call_started` it sees for this tool name is unambiguously this call — the daemon only // reveals `callId` once the call is already in flight (ARCHITECTURE.md §5's `ToolsCallResult` // doc comment), so this is the only way to correlate it before the call finishes. - const progressStream = await openDaemonStream({ stateDir: options.stateDir, spawn: options.spawn }); + const progressStream = await openStream({ stateDir: options.stateDir, spawn: options.spawn }); try { await progressStream.call(RPC_METHODS.eventsSubscribe, {