From 56a74c653b91eef8c028378f3b01e63f9f4bbdac Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 13:20:25 +0000 Subject: [PATCH 1/7] feat(daemon): let wssPort 0 bind an OS-assigned port, and use it everywhere MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `config.json`'s `wssPort` used to go through the same positive-integer rule as every other numeric key, so `0` was rejected and every daemon had to be told a port. That made the test suite pick one for itself: `pickFreePort` — a probe socket bound to 0, read, and closed — was copy-pasted into twelve test files and the e2e harness, and three files skipped it entirely and so bound the default 8443. Both are races. Pre-picking leaves a window between the probe closing and the daemon binding in which anything else on the machine can take the number, and 8443 is simply shared: several vitest processes (parallel worktrees, concurrent agent sessions) collide on it with EADDRINUSE. `0` now means "let the OS assign one". The listener binds 0 and everything that reports or advertises the port afterwards — `daemon.status`'s `wssPort`, a minted link's `endpoint.port`, and so the bootstrap payload, deep link and QR composed from it — reads the *bound* port off the listener rather than the configured value. Echoing the configured `0` back would report the one number no app can ever connect to. Tests get one shared `makeTempStateDir` fixture that writes `wssPort: 0` plus a throwaway host key; every `pickFreePort` copy and every local state-dir helper is deleted in favour of it. Where a test needs the number, it reads it back from the daemon that bound it (`RunningDaemon.listener.port()` in-process, `daemon status --json` or the decoded bootstrap payload across a process boundary) instead of deciding it up front. `daemon.test.ts`'s `wssPort: 8443` assertion becomes an assertion that the reported port is the listener's. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01NxtF2u7HBiZLmduthmmfvn --- CHANGELOG.md | 10 ++ docs/ARCHITECTURE.md | 7 + .../src/__tests__/cli-v2.integration.test.ts | 42 ++--- .../__tests__/daemon-cli.integration.test.ts | 20 ++- packages/appduct/src/__tests__/daemon.test.ts | 151 +++++++++++++----- .../src/__tests__/e2e/churn.e2e.test.ts | 6 +- .../e2e/client-bootstrap.e2e.test.ts | 12 +- .../src/__tests__/e2e/client.e2e.test.ts | 27 +++- .../src/__tests__/e2e/cold-start.e2e.test.ts | 6 +- .../__tests__/e2e/daemon-restart.e2e.test.ts | 6 +- .../e2e/daemon-version-mismatch.e2e.test.ts | 28 +++- .../src/__tests__/e2e/events.e2e.test.ts | 6 +- packages/appduct/src/__tests__/e2e/harness.ts | 68 ++++---- .../src/__tests__/e2e/hostility.e2e.test.ts | 6 +- .../__tests__/e2e/invoke-cancel.e2e.test.ts | 6 +- .../appduct/src/__tests__/e2e/mcp.e2e.test.ts | 6 +- .../__tests__/e2e/multi-device.e2e.test.ts | 6 +- .../__tests__/e2e/policy-audit.e2e.test.ts | 6 +- .../src/__tests__/events.integration.test.ts | 51 ++---- .../__tests__/exit-codes.integration.test.ts | 40 ++--- packages/appduct/src/__tests__/fixtures.ts | 59 ++++++- .../__tests__/link-open.integration.test.ts | 38 ++--- .../__tests__/mcp-command.integration.test.ts | 28 +--- .../__tests__/mcp-server.integration.test.ts | 34 +--- .../policy-and-audit.integration.test.ts | 34 ++-- .../appduct/src/__tests__/rpc-client.test.ts | 18 ++- .../scheme-discovery.integration.test.ts | 23 +-- .../session-engine.integration.test.ts | 47 ++---- .../__tests__/tls-refresh.integration.test.ts | 32 +--- .../tool-invocation.integration.test.ts | 35 +--- packages/appduct/src/daemon/config.ts | 24 ++- packages/appduct/src/daemon/daemon.ts | 29 +++- 32 files changed, 493 insertions(+), 418 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 582515c7..fe52336c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ 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`. + ## 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..8ff7d2ff 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. 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.test.ts b/packages/appduct/src/__tests__/daemon.test.ts index df785a7f..a75ab591 100644 --- a/packages/appduct/src/__tests__/daemon.test.ts +++ b/packages/appduct/src/__tests__/daemon.test.ts @@ -1,18 +1,20 @@ 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 { connect, type Socket } from "node:net"; +import { connect as tlsConnect, type TLSSocket as TlsSocket } from "node:tls"; +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 { 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 { 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"; +import { makeTempStateDir as makeSharedStateDir, removeStateDir } from "./fixtures.js"; const runningDaemons: RunningDaemon[] = []; @@ -29,10 +31,14 @@ afterEach(async () => { } }); +/** + * 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. */ @@ -103,16 +109,61 @@ describe("daemon lifecycle", () => { expect(response.id).toBe(1); expect(response.result).toMatchObject({ pid: process.pid, - wssPort: 8443, 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 rm(stateDir, { force: true, recursive: true }); + 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. + const probe = await new Promise((resolve, reject) => { + const connection = tlsConnect({ host: "127.0.0.1", port: bound!, rejectUnauthorized: false }, () => + 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 () => { @@ -133,7 +184,7 @@ describe("daemon lifecycle", () => { expect(goodLineResponse.result.pid).toBe(process.pid); socket.destroy(); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); test("unknown method returns JSON-RPC -32601", async () => { @@ -150,7 +201,7 @@ describe("daemon lifecycle", () => { expect(response.error.code).toBe(-32601); socket.destroy(); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); test("a line beyond the configured cap gets an error and the connection is dropped", async () => { @@ -186,7 +237,7 @@ describe("daemon lifecycle", () => { expect(socket.destroyed).toBe(true); } finally { await server.close(); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); } }); @@ -209,7 +260,7 @@ describe("daemon lifecycle", () => { await expect(stat(paths.pidFilePath)).rejects.toThrow(); socket.destroy(); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); test("second daemon against the same state dir throws DaemonAlreadyRunningError", async () => { @@ -219,7 +270,7 @@ describe("daemon lifecycle", () => { await expect(startDaemon({ stateDir })).rejects.toThrow(DaemonAlreadyRunningError); await first.shutdown(); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); test("takes over a stale pidfile and stale socket left by a dead process", async () => { @@ -250,7 +301,7 @@ describe("daemon lifecycle", () => { socket.destroy(); void daemon; - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); test("config.json invalid values throw a clear error naming the key", async () => { @@ -262,7 +313,7 @@ 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 () => { @@ -270,7 +321,7 @@ describe("daemon lifecycle", () => { 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 })); + await writeFile(paths.configPath, JSON.stringify({ totallyUnknownKey: true, wssPort: 0 })); const warnings: string[] = []; const daemon = await startTrackedDaemon(stateDir); @@ -282,10 +333,10 @@ describe("daemon lifecycle", () => { warn: (message) => warnings.push(message), }); - expect(config.wssPort).toBe(9000); + expect(config.wssPort).toBe(0); expect(warnings.some((message) => message.includes("totallyUnknownKey"))).toBe(true); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); test("config.json iosBundleId is a known key, loaded as-is and validated as a non-empty string", async () => { @@ -317,11 +368,40 @@ describe("daemon lifecycle", () => { // 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" })); + // `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 rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); + }); + + 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"); + const { loadConfig } = await import("../daemon/config.js"); + await mkdir(stateDir, { recursive: true }); + + // The documented default when the key is absent (ARCHITECTURE.md §3). + await writeFile(paths.configPath, JSON.stringify({})); + expect((await loadConfig(paths)).wssPort).toBe(8443); + + // `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({ wssPort: 8443 })); + expect((await loadConfig(paths)).wssPort).toBe(8443); + + // 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 removeStateDir(stateDir); }); test("restartDaemonOnVersionMismatch defaults to false and must be a boolean", async () => { @@ -343,7 +423,7 @@ 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); }); }); @@ -382,21 +462,6 @@ describe("daemon: audit retention", () => { 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 => { @@ -419,7 +484,7 @@ describe("daemon: audit retention", () => { 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 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 @@ -444,7 +509,7 @@ describe("daemon: audit retention", () => { await daemon.shutdown(); expect(intervals[0]!.cleared).toBe(true); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); test("daemon.status reports the audit footprint and the effective retention", async () => { @@ -452,7 +517,7 @@ describe("daemon: audit retention", () => { 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() })); + 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 @@ -480,7 +545,7 @@ describe("daemon: audit retention", () => { }); socket.destroy(); - await rm(stateDir, { force: true, recursive: true }); + await removeStateDir(stateDir); }); test("daemon status degrades cleanly against a daemon that predates retention", async () => { @@ -535,7 +600,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 +615,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..fa2651ba 100644 --- a/packages/appduct/src/__tests__/e2e/client.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/client.e2e.test.ts @@ -12,7 +12,7 @@ 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 } from "./harness.js"; afterEach(cleanupAfterEach); @@ -49,8 +49,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); @@ -115,8 +118,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 +153,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 +195,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 +233,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..0f5de2a6 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); 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..2dfb3dd5 100644 --- a/packages/appduct/src/__tests__/e2e/harness.ts +++ b/packages/appduct/src/__tests__/e2e/harness.ts @@ -14,17 +14,22 @@ */ 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 path from "node:path"; +import { connect as connectUds, type Socket } from "node:net"; import { text } from "node:stream/consumers"; 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 +73,7 @@ export const cleanupAfterEach = async (): Promise => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }; @@ -91,37 +96,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 +174,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); 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..78fbf2fb 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, @@ -51,8 +52,11 @@ 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); diff --git a/packages/appduct/src/__tests__/events.integration.test.ts b/packages/appduct/src/__tests__/events.integration.test.ts index b0ce343e..3e8b9faa 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,7 +86,9 @@ const claimAppOverCli = async ( const payload = (linkResult.data.deepLink as string).split("appduct=")[1]!.split("&")[0]!; const decoded = decodeBootstrap(payload)!; - const socket = new WebSocket(`wss://127.0.0.1:${port}`, { rejectUnauthorized: false }); + // 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 socket = new WebSocket(`wss://127.0.0.1:${decoded.port}`, { rejectUnauthorized: false }); await new Promise((resolve, reject) => { socket.once("open", () => resolve()); socket.once("error", reject); @@ -194,13 +175,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-server.integration.test.ts b/packages/appduct/src/__tests__/mcp-server.integration.test.ts index eede55fb..134af7d1 100644 --- a/packages/appduct/src/__tests__/mcp-server.integration.test.ts +++ b/packages/appduct/src/__tests__/mcp-server.integration.test.ts @@ -6,12 +6,10 @@ * plain Node streams for the stdout-purity assertion). */ +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"; @@ -34,7 +32,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 +55,7 @@ afterEach(async () => { } while (stateDirs.length > 0) { - await rm(stateDirs.pop()!, { force: true, recursive: true }); + await removeStateDir(stateDirs.pop()!); } }); @@ -65,18 +63,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 +70,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 => { 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.test.ts b/packages/appduct/src/__tests__/rpc-client.test.ts index 26e3ef02..a45f377f 100644 --- a/packages/appduct/src/__tests__/rpc-client.test.ts +++ b/packages/appduct/src/__tests__/rpc-client.test.ts @@ -1,7 +1,5 @@ -import { mkdtemp, rm, utimes, writeFile } from "node:fs/promises"; +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"; @@ -17,7 +15,7 @@ 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[] = []; @@ -33,11 +31,19 @@ afterEach(async () => { } resetDaemonVersionChecks(); + + while (stateDirs.length > 0) { + await removeStateDir(stateDirs.pop()!); + } }); +const stateDirs: string[] = []; + +/** The shared fixture's `wssPort: 0` matters here: this file used to write no `config.json`, so + * every `startDaemon` below bound the default 8443 and raced every other daemon on the machine. */ 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; }; 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(() => { From bb091cf396fdba8dbcdf10a465892fae317debea Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 13:24:13 +0000 Subject: [PATCH 2/7] test(appduct): tier daemon and rpc-client tests by what they actually need MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `daemon.test.ts` and `rpc-client.test.ts` were named as unit tests but mostly were not: between them they started a real daemon — pidfile, TLS material, wss listener, UDS control socket — in twenty cases, several of which only ever assert on a string in an error message or on the shape a config value parses to. That cost is paid on every run, and it makes those cases fail for reasons that have nothing to do with what they test. Each file now keeps only what needs no listener and hands the rest to a sibling `*.integration.test.ts`. No test body changed beyond what the move required (the moved `rm(stateDir, ...)` calls now go through the shared `removeStateDir`). `daemon.test.ts` keeps: the RPC line-length cap (a hand-rolled `startRpcServer`), invalid `config.json` values, `wssPort` parsing, `restartDaemonOnVersionMismatch`, `daemon status` against a hand-rolled pre-retention daemon, and invalid retention keys. `daemon.integration.test.ts` takes: `daemon.status` over the real UDS, the `wssPort: 0` round-trip, malformed-JSON and unknown-method framing, `daemon.shutdown`, the second-daemon conflict, stale-pidfile takeover, the two config cases that go on to boot a daemon, and the two audit-retention wiring cases. `rpc-client.test.ts` keeps every case the client decides on its own — the auto-spawn guard, the spawn-lock, and all of issue #30's version-drift logic, which has always run against the hand-rolled `FakeDaemon` because a real daemon can only ever report this build's own version. `rpc-client.integration.test.ts` takes the five that put a real daemon on the other end of the socket. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01NxtF2u7HBiZLmduthmmfvn --- .../src/__tests__/daemon.integration.test.ts | 458 ++++++++++++++++++ packages/appduct/src/__tests__/daemon.test.ts | 387 +-------------- .../__tests__/rpc-client.integration.test.ts | 167 +++++++ .../appduct/src/__tests__/rpc-client.test.ts | 134 +---- 4 files changed, 653 insertions(+), 493 deletions(-) create mode 100644 packages/appduct/src/__tests__/daemon.integration.test.ts create mode 100644 packages/appduct/src/__tests__/rpc-client.integration.test.ts 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..0614e724 --- /dev/null +++ b/packages/appduct/src/__tests__/daemon.integration.test.ts @@ -0,0 +1,458 @@ +/** + * 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 { connect as tlsConnect, type TLSSocket as TlsSocket } from "node:tls"; +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. + const probe = await new Promise((resolve, reject) => { + const connection = tlsConnect({ host: "127.0.0.1", port: bound!, rejectUnauthorized: false }, () => + 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 a75ab591..b1532526 100644 --- a/packages/appduct/src/__tests__/daemon.test.ts +++ b/packages/appduct/src/__tests__/daemon.test.ts @@ -1,19 +1,23 @@ -import { spawnSync } from "node:child_process"; +/** + * 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 { connect as tlsConnect, type TLSSocket as TlsSocket } from "node:tls"; -import { readdir, readFile, stat, writeFile } from "node:fs/promises"; -import path from "node:path"; +import { writeFile } from "node:fs/promises"; import { afterEach, describe, expect, test } from "vitest"; -import { decodeBootstrap } from "@appduct/shared"; - 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, type RunningDaemon } 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 { makeTempStateDir as makeSharedStateDir, removeStateDir } from "./fixtures.js"; const runningDaemons: RunningDaemon[] = []; @@ -87,123 +91,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, - 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. - const probe = await new Promise((resolve, reject) => { - const connection = tlsConnect({ host: "127.0.0.1", port: bound!, rejectUnauthorized: false }, () => - 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); - }); - +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); @@ -241,69 +129,6 @@ describe("daemon lifecycle", () => { } }); - 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 invalid values throw a clear error naming the key", async () => { const stateDir = await makeTempStateDir(); const paths = getStateDirPaths(stateDir); @@ -316,66 +141,6 @@ describe("daemon lifecycle", () => { 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); - }); - test("wssPort accepts 0 (OS-assigned) and rejects anything that is not a port number", async () => { const stateDir = await makeTempStateDir(); const paths = getStateDirPaths(stateDir); @@ -428,126 +193,12 @@ describe("daemon lifecycle", () => { }); /** - * 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 }); - }; - - /** 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); - }); - +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); 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 a45f377f..bc3dc33c 100644 --- a/packages/appduct/src/__tests__/rpc-client.test.ts +++ b/packages/appduct/src/__tests__/rpc-client.test.ts @@ -1,9 +1,16 @@ +/** + * `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 { afterEach, describe, expect, test } from "vitest"; -import { startDaemon, type RunningDaemon } from "../daemon/daemon.js"; import { getStateDirPaths } from "../daemon/state-dir.js"; import { callDaemon, @@ -17,7 +24,6 @@ import { } from "../rpc/client.js"; import { makeTempStateDir as makeSharedStateDir, removeStateDir } from "./fixtures.js"; -const runningDaemons: RunningDaemon[] = []; const fakeDaemons: FakeDaemon[] = []; afterEach(async () => { @@ -25,11 +31,6 @@ afterEach(async () => { await fakeDaemons.pop()?.stop(); } - while (runningDaemons.length > 0) { - const daemon = runningDaemons.pop(); - await daemon?.shutdown(); - } - resetDaemonVersionChecks(); while (stateDirs.length > 0) { @@ -191,34 +192,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(); @@ -227,64 +200,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 = () => { @@ -303,38 +218,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"; @@ -895,3 +778,4 @@ describe("daemon version check: a contended lock is not an ineffective restart ( await rm(stateDir, { force: true, recursive: true }); }); }); + From 4ed928ef4593ac7eb5e5328806195b8b2594593c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 13:28:55 +0000 Subject: [PATCH 3/7] test(appduct): run the MCP server's own mapping tests on an in-memory daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ten cases in `mcp-server.integration.test.ts` booted a real daemon — pidfile, self-signed certificate, wss listener — and scripted a fake app over a real WebSocket, in order to assert on a JSON schema the MCP server had rewritten on its way out, or on which name a tool was listed under. None of that depends on the transport underneath, and all of it inherited the transport's failure modes. `createMcpServer` already reached the daemon through exactly one thing, `DaemonStream`, so the seam is a single new `openStream` option defaulting to `openDaemonStream` — the two existing call sites (the persistent startup stream and each short-lived progress stream) now go through it, and nothing else in `server.ts` changes. `mcp-daemon-fake.ts` is an in-memory `DaemonStream` answering the four methods the server actually calls (`sessions.list`, `tools.list`, `tools.call`, `events.subscribe`) and pushing `event` notifications, which is how `list_changed` is driven. Any other method throws with a message naming both possible causes, so a server that grows a new daemon dependency cannot pass by accident. `mcp-server.test.ts` takes the ten cases verbatim (the SDK `Client` over `InMemoryTransport` is unchanged, so what the client sees is still what a real client sees) and the locked-mapping snapshot moves with them. No assertion was weakened or dropped: the file's 10 plus the integration file's remaining 34 are the 44 there were. They now run in under a second. `mcp-server.integration.test.ts` keeps what needs the real thing: a declared `timeout_ms` surviving the round trip, progress correlation over the second stream, cancellation reaching the app as `tool_cancel`, the `appduct_connect` delivery paths, the events tools, the resource, stdout purity and version drift. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01NxtF2u7HBiZLmduthmmfvn --- ...n.test.ts.snap => mcp-server.test.ts.snap} | 0 .../appduct/src/__tests__/mcp-daemon-fake.ts | 217 ++++++++++ .../__tests__/mcp-server.integration.test.ts | 394 +----------------- .../appduct/src/__tests__/mcp-server.test.ts | 346 +++++++++++++++ packages/appduct/src/mcp/server.ts | 22 +- 5 files changed, 595 insertions(+), 384 deletions(-) rename packages/appduct/src/__tests__/__snapshots__/{mcp-server.integration.test.ts.snap => mcp-server.test.ts.snap} (100%) create mode 100644 packages/appduct/src/__tests__/mcp-daemon-fake.ts create mode 100644 packages/appduct/src/__tests__/mcp-server.test.ts 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__/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 134af7d1..e266d0ee 100644 --- a/packages/appduct/src/__tests__/mcp-server.integration.test.ts +++ b/packages/appduct/src/__tests__/mcp-server.integration.test.ts @@ -1,9 +1,16 @@ /** - * 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"; @@ -22,7 +29,6 @@ import { ListResourcesResultSchema, ListToolsResultSchema, ReadResourceResultSchema, - ToolListChangedNotificationSchema, } from "@modelcontextprotocol/sdk/types.js"; import { decodeBootstrap, type ToolDescriptor } from "@appduct/shared"; @@ -256,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); @@ -624,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/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, { From 7c0184a18a5d29f4f3ace5d833d19559663f34da Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 13:34:56 +0000 Subject: [PATCH 4/7] fix(daemon): treat a zombie pid as dead, and stop racing the audit write queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two races across a process boundary, one in a test and one in the daemon. **The audit read.** `daemon/audit.ts` serializes writes on an internal promise queue and `record()` returns the moment it has *enqueued* one — deliberately, so a slow disk cannot stall the `tools.call` response path. So the response a CLI subprocess has already returned proves the call finished, never that its audit line is on disk. In-process that gap is closed by `AuditLogger.flush()`; from another process there is nothing to await, and `client.e2e.test.ts` read today's day file straight after the calls and intermittently missed the last records. The harness now has `waitForAuditRecords(stateDir, predicate)`, which re-reads until the predicate holds (tolerating a missing file and a half-written final line) and, on timeout, says how many records it saw and what they were — "expected 3, saw 2" and "expected 3, saw 0" are different bugs. `client.e2e.test.ts` and `policy-audit.e2e.test.ts` both go through it. **The zombie.** `process.kill(pid, 0)` succeeds for an exited-but-unreaped process: a zombie still holds a pid table entry. That is the right answer for signalling and the wrong one for "is a daemon still there?" — the process is gone and its socket is closed. Normally 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 SIGKILLed after its parent CLI exited stays a zombie for the life of the container. `daemon/pidfile.ts`'s stale-pidfile takeover then never fires and every later command reports a daemon that is already dead — which is exactly why `daemon-restart.e2e.test.ts` fails on `main` in such sandboxes (PRs #64, #67). `isProcessAlive` now additionally reads `/proc//status` and treats `State: Z` as dead. It 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 procfs read is behind an injectable reader, because a real zombie cannot be arranged from a test: Node reaps its own children automatically, so producing one needs a grandchild orphaned to a non-reaping PID 1 — the very condition this check exists for, and therefore the one thing a test may not assume. `pidfile.test.ts` covers the real live and real dead pids for real, and supplies the procfs bytes for the states. `daemon-restart.e2e.test.ts`'s post-restart claim now takes its port from the fresh link rather than from the daemon that was killed: the replacement asks the OS for a port of its own. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01NxtF2u7HBiZLmduthmmfvn --- CHANGELOG.md | 7 ++ docs/ARCHITECTURE.md | 14 +++- .../src/__tests__/e2e/client.e2e.test.ts | 41 ++++++---- .../__tests__/e2e/daemon-restart.e2e.test.ts | 7 +- packages/appduct/src/__tests__/e2e/harness.ts | 74 ++++++++++++++++++ .../__tests__/e2e/policy-audit.e2e.test.ts | 20 +++-- .../appduct/src/__tests__/pidfile.test.ts | 77 +++++++++++++++++++ .../appduct/src/__tests__/rpc-client.test.ts | 5 +- packages/appduct/src/daemon/pidfile.ts | 68 ++++++++++++++-- 9 files changed, 273 insertions(+), 40 deletions(-) create mode 100644 packages/appduct/src/__tests__/pidfile.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index fe52336c..b3d4a294 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,13 @@ package versions for a release. 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 8ff7d2ff..75b6c536 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -160,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__/e2e/client.e2e.test.ts b/packages/appduct/src/__tests__/e2e/client.e2e.test.ts index fa2651ba..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, daemonWssPort, 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 @@ -97,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" }), 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 0f5de2a6..a5f6d6fd 100644 --- a/packages/appduct/src/__tests__/e2e/daemon-restart.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/daemon-restart.e2e.test.ts @@ -65,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/harness.ts b/packages/appduct/src/__tests__/e2e/harness.ts index 2dfb3dd5..a82a7793 100644 --- a/packages/appduct/src/__tests__/e2e/harness.ts +++ b/packages/appduct/src/__tests__/e2e/harness.ts @@ -14,7 +14,9 @@ */ import { createHash, X509Certificate } from "node:crypto"; +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"; @@ -333,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/policy-audit.e2e.test.ts b/packages/appduct/src/__tests__/e2e/policy-audit.e2e.test.ts index 78fbf2fb..6b473f79 100644 --- a/packages/appduct/src/__tests__/e2e/policy-audit.e2e.test.ts +++ b/packages/appduct/src/__tests__/e2e/policy-audit.e2e.test.ts @@ -20,6 +20,7 @@ import { mintLink, runCliJson, subscribeToEvents, + waitForAuditRecords, } from "./harness.js"; afterEach(cleanupAfterEach); @@ -36,16 +37,13 @@ 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", () => { @@ -88,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__/pidfile.test.ts b/packages/appduct/src/__tests__/pidfile.test.ts new file mode 100644 index 00000000..bceff524 --- /dev/null +++ b/packages/appduct/src/__tests__/pidfile.test.ts @@ -0,0 +1,77 @@ +/** + * `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 { spawnSync } from "node:child_process"; + +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)`. + * + * A *real* zombie cannot be arranged from here. Node reaps its own children automatically (libuv + * installs a SIGCHLD handler), so a child of this process is never a zombie; producing one needs a + * grandchild orphaned to a PID 1 that does not reap, which is precisely the container-specific + * condition this whole check exists for and therefore the one thing a test may not assume. The + * seam is the reader, so the test supplies the bytes `/proc` would have contained and the + * decision under test — "a pid that `kill(pid, 0)` accepts is still dead if procfs says Z" — is + * exercised for real. + */ +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); + }); +}); diff --git a/packages/appduct/src/__tests__/rpc-client.test.ts b/packages/appduct/src/__tests__/rpc-client.test.ts index bc3dc33c..2ad8259c 100644 --- a/packages/appduct/src/__tests__/rpc-client.test.ts +++ b/packages/appduct/src/__tests__/rpc-client.test.ts @@ -40,8 +40,9 @@ afterEach(async () => { const stateDirs: string[] = []; -/** The shared fixture's `wssPort: 0` matters here: this file used to write no `config.json`, so - * every `startDaemon` below bound the default 8443 and raced every other daemon on the machine. */ +/** 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 makeSharedStateDir({}, { prefix: "appduct-rpc-client-test-" }); stateDirs.push(stateDir); 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 => { From 16bd177cc329eac1153ee9d2725059e4a806e82a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 13:50:30 +0000 Subject: [PATCH 5/7] test(appduct): probe the ephemeral port over plain TCP, not an unverified TLS handshake CodeQL flagged two `rejectUnauthorized: false` sites in the previous commits as new "disabling certificate validation" alerts. The suite disables client-side verification throughout its integration tests on purpose (the daemon mints a throwaway self-signed certificate per state dir, and the fake app verifies its SPKI pin separately, which is the trust decision a real app makes), so the pattern is not new, but the two touched lines were: - `daemon.integration.test.ts`'s "is the bound port reachable" probe only needs a TCP connection to succeed; what the listener then does with it (TLS, pinning, the wire protocol) has its own tests. It now uses `net.connect`, and no longer touches certificate validation at all. - `events.integration.test.ts` rewrote the pre-existing `WebSocket` line to read the port off the decoded bootstrap payload. The port is now read into a local first and the connection line is left exactly as it was. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01NxtF2u7HBiZLmduthmmfvn --- .../appduct/src/__tests__/daemon.integration.test.ts | 11 +++++------ .../appduct/src/__tests__/events.integration.test.ts | 4 +++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/appduct/src/__tests__/daemon.integration.test.ts b/packages/appduct/src/__tests__/daemon.integration.test.ts index 0614e724..373e66a9 100644 --- a/packages/appduct/src/__tests__/daemon.integration.test.ts +++ b/packages/appduct/src/__tests__/daemon.integration.test.ts @@ -9,7 +9,6 @@ import { spawnSync } from "node:child_process"; import { connect, type Socket } from "node:net"; -import { connect as tlsConnect, type TLSSocket as TlsSocket } from "node:tls"; import { readdir, readFile, stat, writeFile } from "node:fs/promises"; import path from "node:path"; @@ -160,11 +159,11 @@ describe("daemon lifecycle", () => { expect(decoded).not.toBeNull(); expect(decoded!.port).toBe(bound); - // And the bound port is genuinely reachable — `0` was a request, not a literal bind. - const probe = await new Promise((resolve, reject) => { - const connection = tlsConnect({ host: "127.0.0.1", port: bound!, rejectUnauthorized: false }, () => - resolve(connection), - ); + // 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(); diff --git a/packages/appduct/src/__tests__/events.integration.test.ts b/packages/appduct/src/__tests__/events.integration.test.ts index 3e8b9faa..4996405c 100644 --- a/packages/appduct/src/__tests__/events.integration.test.ts +++ b/packages/appduct/src/__tests__/events.integration.test.ts @@ -88,7 +88,9 @@ const claimAppOverCli = async ( // 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 socket = new WebSocket(`wss://127.0.0.1:${decoded.port}`, { rejectUnauthorized: false }); + 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()); socket.once("error", reject); From 78646f032690c5fe1a6ca59551569734b33b1077 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 21 Sep 2026 08:16:46 +0200 Subject: [PATCH 6/7] test(appduct): check zombie detection against a real zombie on Linux The injected reader covered the decision, but the default /proc reader and the State-line parse never ran against kernel output. A shell that backgrounds a sleep and then execs into another sleep leaves a real, unreaped zombie with no container needed; assert isProcessAlive calls it dead while kill(pid, 0) still succeeds. --- .../appduct/src/__tests__/pidfile.test.ts | 60 ++++++++++++++++--- 1 file changed, 52 insertions(+), 8 deletions(-) diff --git a/packages/appduct/src/__tests__/pidfile.test.ts b/packages/appduct/src/__tests__/pidfile.test.ts index bceff524..2328f5f3 100644 --- a/packages/appduct/src/__tests__/pidfile.test.ts +++ b/packages/appduct/src/__tests__/pidfile.test.ts @@ -4,7 +4,8 @@ * stale-socket unlink, log rotation — so what it answers for a zombie decides all three. */ -import { spawnSync } from "node:child_process"; +import { spawn, spawnSync } from "node:child_process"; +import { readFileSync } from "node:fs"; import { describe, expect, test } from "vitest"; @@ -24,13 +25,12 @@ const deadPid = (): number => { /** * A `/proc//status` body, in the shape the kernel writes it: `Name`, then `State:\tX (word)`. * - * A *real* zombie cannot be arranged from here. Node reaps its own children automatically (libuv - * installs a SIGCHLD handler), so a child of this process is never a zombie; producing one needs a - * grandchild orphaned to a PID 1 that does not reap, which is precisely the container-specific - * condition this whole check exists for and therefore the one thing a test may not assume. The - * seam is the reader, so the test supplies the bytes `/proc` would have contained and the - * decision under test — "a pid that `kill(pid, 0)` accepts is still dead if procfs says Z" — is - * exercised for real. + * 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`; @@ -74,4 +74,48 @@ describe("isProcessAlive", () => { // 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, + ); }); From d68eb032ac1a51f304c4768561a2380cabca55bb Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 21 Sep 2026 08:16:46 +0200 Subject: [PATCH 7/7] test(appduct): drop the unused daemon tracker from the listener-free daemon tests daemon.test.ts no longer starts a daemon that has to be shut down, so the startTrackedDaemon helper and its afterEach were dead code that contradicted the file's own header. --- packages/appduct/src/__tests__/daemon.test.ts | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/packages/appduct/src/__tests__/daemon.test.ts b/packages/appduct/src/__tests__/daemon.test.ts index b1532526..85cb41b0 100644 --- a/packages/appduct/src/__tests__/daemon.test.ts +++ b/packages/appduct/src/__tests__/daemon.test.ts @@ -15,26 +15,11 @@ import { writeFile } from "node:fs/promises"; import { afterEach, describe, expect, test } from "vitest"; import { handleDaemonStatusCommand } from "../commands/daemon/status.js"; -import { startDaemon, type RunningDaemon } from "../daemon/daemon.js"; +import { startDaemon } from "../daemon/daemon.js"; import { startRpcServer } from "../daemon/rpc-server.js"; import { getStateDirPaths } from "../daemon/state-dir.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