Migrate run to daemon client + add --detach#98
Merged
obj-p merged 4 commits intocli-mcp-parityfrom Apr 15, 2026
Merged
Conversation
The `run` command becomes a thin MCP client that talks to the daemon introduced in the previous commit. The daemon now owns the preview window, file watcher, and NSApplication lifecycle; `run` is a short-lived subprocess that creates a session and (by default) blocks until Ctrl+C. Changes: - DaemonClient: auto-starts the daemon via `previewsmcp serve --daemon` if not already running, polls the socket, and returns a connected MCP.Client. - RunCommand: rewritten as AsyncParsableCommand. No longer imports PreviewsMacOS or drives AppKit. Calls preview_start over UDS, relays log notifications to stderr, and on SIGINT calls preview_stop and exits cleanly. - --detach flag: prints the session UUID to stdout and exits, leaving the session running in the daemon. Scriptable for chaining with other commands. - PreviewsMCPApp.main: dispatches AsyncParsableCommand via dispatchMain so NWConnection callbacks (which NetworkTransport schedules on DispatchQueue.main) can fire while the Task drives the command to completion. Earlier semaphore-based wait deadlocked. - RunCommand removed from the "needs AppKit" branch — it's a pure daemon client now. Tests: 3 new integration tests (RunCommandTests) — detach prints a bare UUID, attached mode blocks until SIGINT, detach reuses an existing daemon without spawning a second. Second PR in the CLI/MCP parity stack; see docs/cli-mcp-parity-spec.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 14, 2026
Four fixes from self-review: 1. Detach daemon from parent's process/terminal group (setsid on startup in DaemonLifecycle.register). Without this, terminal close (SIGHUP) cascades from client to daemon, violating the "daemon survives client exit" invariant the spec claims. 2. Error formatting parity for async commands. PreviewsMCPApp.main's async-dispatch path now delegates to PreviewsMCPCommand.exit( withError:) like the sync path, so ValidationError and ExitCode get ArgumentParser's formatter instead of the raw struct description. 3. Strengthen attachedBlocksUntilSignal test. Previous version waited for the daemon socket to appear — which happens before preview_start completes — and could pass even if run never called preview_start. Now reads client stderr for the daemon's "Session ID:" response, proving a session was actually rendered before sending SIGINT. Wall-clock rose from 0.3s to ~4s, matching the true workload. 4. Detach-mode message no longer references a not-yet-implemented `stop` subcommand. Also fixes a cross-suite test race: RunCommandTests and DaemonLifecycleTests both touch the same ~/.previewsmcp state. `@Suite(.serialized)` only orders tests within one suite; Swift Testing may run the two suites in parallel, causing one test's cleanSlate to kill another's daemon mid-run. Added a DaemonTestLock (flock-based) that both suites acquire around test bodies to force mutual exclusion. Follow-up tracked: - #99 (DaemonClient error paths) - #100 (self-path via _NSGetExecutablePath) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three Important items from subagent review: 1. setsid() ordering. Previously called after the listener bound the socket, leaving a window where SIGHUP from the parent could cascade to the daemon. Now extracted to DaemonLifecycle.detachFromTerminal() and called first in ServeCommand.runDaemon, before DaemonListener.start(). New regression test daemonSurvivesClientSIGHUP verifies a live daemon survives SIGHUP to the `run` client. 2. Better preview_stop failure message. The warning now explicitly tells the user the session may still be running in the daemon and suggests kill-daemon as cleanup — surfacing actionable info rather than a bare error string. 3. Log notification registration race. onNotification was registered *after* client.connect() returned, so log messages emitted during the MCP initialize handshake were dropped. DaemonClient.connect() now takes a `configure` closure that runs before connect(), guaranteeing handlers catch all notifications including handshake-phase ones. Plus nits: - Remove redundant `command is ListCommand` check in PreviewsMCPApp (the `!(...)` already covers it). - Inline test bodies in RunCommandTests (drop the runXxx indirection that the review flagged as awkward). - DaemonClient validates the self-path is an executable file before spawning, producing a clearer error than Process.run()'s generic POSIX failure on a missing binary. Plus a real bug uncovered by the review's regression-sweep failure (originally tracked as #101): SetupBuilder.linkDynamicLibrary had a "file missing" race. It did `removeItem(at: dylibPath)` then `swiftc -emit-library -o dylibPath`; concurrent preview compiles linking -lPreviewSetup would hit `ld: library 'PreviewSetup' not found` during that window. Fixed by building to a UUID-suffixed temp path, codesigning the temp file, then atomically renaming to the final path. rename(2) is atomic on Darwin — concurrent readers always see either the old inode or the new one, never a missing file. With this fix the full non-iOS test sweep is 243/243 green, including under the concurrent-suite load that previously triggered the race. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two nits from the second-round review. 1. `linkDynamicLibrary` now builds to a UUID-suffixed `.dylib.tmp` and atomically renames to the final path. If the process crashes between emit-library and rename, the temp file lingers. Extend `cleanStaleArchives` to sweep `libPreviewSetup.*.dylib.tmp` alongside legacy `.a` files. Verified manually: planting a fake temp file and triggering a rebuild removes it. 2. `DaemonClient.spawnDaemon`'s comment claimed the `isExecutableFile` check guards against spoofed argv[0]. It doesn't — a spoofed path that happens to point at any real executable would still pass. Soften the comment to describe the actual behavior (clear error when binary is missing) and point to #100 for authoritative self-path resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
This was referenced Apr 15, 2026
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
1 participant
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.
Summary
Second PR in the CLI/MCP parity stack. Migrates `run` from an in-process preview owner to a lightweight daemon client.
What didn't change
Test plan
3 new integration tests in `RunCommandTests`:
Regression coverage:
Related
🤖 Generated with Claude Code