Conversation
…ywhere `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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxtF2u7HBiZLmduthmmfvn
… need `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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxtF2u7HBiZLmduthmmfvn
… daemon 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxtF2u7HBiZLmduthmmfvn
…ite queue 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/<pid>/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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxtF2u7HBiZLmduthmmfvn
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…fied 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxtF2u7HBiZLmduthmmfvn
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #39.
Why
The
appductsuite is reliable inside one vitest run (fileParallelism: false) and flaky when several runs share a machine, which is exactly what parallel worktrees and agent sessions do. Three causes, all confirmed onmain:daemon.test.ts,rpc-client.test.tsanddaemon-cli.integration.test.tswrote noconfig.jsonand so bound the default port 8443. Everything else carried its own copy of apickFreePorthelper (13 copies), which is itself a pick-then-bind race.client.e2e.test.tsread the audit day file straight after the calls, across a process boundary, whiledaemon/audit.tswas still draining its write queue.daemon-restart.e2e.test.tsSIGKILLs a daemon whose parent CLI has exited. In a container whose PID 1 does not reap, the daemon stays a zombie,process.kill(pid, 0)keeps succeeding, and the stale-pidfile takeover never fires. feat(cli)!: require an explicit app id for android/ios-device delivery #64 and feat(cli): agent-friendly output — signature tools listing with filter/paging, table-driven global flags, --pretty/--verbose, compact JSON #67 both hit this onmain.The suite's design (real TLS listener, real UDS, real wire protocol, a scripted fake app) is deliberate and is kept. What changes is where the port comes from, which files count as unit tests, and what the MCP server's own tests need behind them.
What changed, one commit per part
1.
wssPort: 0binds an OS-assigned port (feat(daemon)).daemon/config.tsvalidateswssPortwith its own rule (0 to 65535) so no other key gains a meaningless zero.daemon/daemon.tsreads the bound port off the listener once it is up and uses it fordaemon.statusand for thegetEndpointclosure, so minted links, deep links and QR codes carry the real port. Every other consumer already reads the port off RPC results. One sharedmakeTempStateDirin__tests__/fixtures.tswriteswssPort: 0; all 13pickFreePortcopies are deleted. Tests that need the number read it back (listener.port()in-process,daemon status --jsonacross a process, or the decoded bootstrap payload, which is what a real app dials). Documented indocs/ARCHITECTURE.md§3 andCHANGELOG.md.2. Honest tiering (
test(appduct)). Cases that start a real daemon moved out ofdaemon.test.tsandrpc-client.test.tsinto newdaemon.integration.test.tsandrpc-client.integration.test.ts. Test bodies are unchanged; counts preserved (29 = 24 + 5 for rpc-client). The pure pidfile logic gets its ownpidfile.test.tsin part 4.3. MCP server tests on an in-memory daemon (
test(appduct)).createMcpServergains one optionalopenStreamoption defaulting toopenDaemonStream; that is the whole seam.__tests__/mcp-daemon-fake.tsis an in-memoryDaemonStreamansweringsessions.list,tools.list,tools.call,events.subscribeand pushingeventnotifications; anything else throws. The ten mapping, degradation, namespacing andlist_changedcases moved verbatim tomcp-server.test.tswith their snapshot, and run in under a second instead of about fourteen. The SDKClientoverInMemoryTransportis unchanged, so what a client sees is still what a real client sees. Progress correlation, cancellation, delivery paths, the events tools, the sessions resource, stdout purity and version drift stay in the integration file against a real daemon.4. The two process-boundary races (
fix(daemon)).waitForAuditRecordsin the e2e harness polls the day file until a predicate holds, tolerating the lazily-created file and a half-written last line; used byclient.e2e.test.tsandpolicy-audit.e2e.test.ts.isProcessAliveindaemon/pidfile.tsadditionally reads/proc/<pid>/statuson Linux and treatsState: Zas dead, behind an injectable reader. It only ever adds a "dead" verdict on positive evidence, so macOS and hardened containers keep today's behaviour.docs/ARCHITECTURE.md§4 andCHANGELOG.mdexplain why.Production code touched
daemon/config.ts,daemon/daemon.ts,daemon/pidfile.ts, and the 22-lineopenStreamseam inmcp/server.ts. Nothing underpackages/native/,packages/react-native/orskills/.Verification
pnpm -r typecheck: clean.pnpm --filter appduct test, twice: 60 files, 689 passed, 1 skipped each run.daemon-restart.e2e.test.tsin a non-reaping sandbox: fails withmain'spidfile.ts, passes with part 4.vitest runprocesses over the daemon-starting files: onmain,EADDRINUSE 0.0.0.0:8443in both; on this branch, both exit 0 with zero occurrences.pnpm lintcovers only@appduct/react-nativeandplaygroundand fails onmainfor an unrelated reason (playgroundcannot resolve the unbuilt@appduct/react-native); not touched here.Note
A real zombie cannot be arranged from a test: libuv reaps Node's own children, so producing one needs a grandchild orphaned to a non-reaping PID 1, the exact condition the check exists for.
pidfile.test.tstherefore uses real processes for the alive and exited states and the injected reader for the procfs bytes, with that reasoning in the file.🤖 Generated with Claude Code
https://claude.ai/code/session_01NxtF2u7HBiZLmduthmmfvn
Generated by Claude Code