[mstomar125]

Held for design discussion: proposes how the app's daemon management follows NODESPACE_HOME. Do not merge before it is agreed.

Closes #3616

Problem

An app started with NODESPACE_HOME set managed the user's real daemon. Every path in daemon_setup came from dirs::home_dir().

• It registered the user's launchd job without NODESPACE_HOME.
• It dialed ~/.nodespace/daemon*.sock.
• On quit it SIGTERMed the user's daemon.

So an "isolated" app read and wrote the user's databases.

What changes

Implements the decision proposed in the docs PR below. A new daemon_home module resolves DaemonHome:

• User(home): NODESPACE_HOME unset, or naming the user's home however spelled.
• Other(home): anything else.

User home: nothing changes. Every path is still dirs::home_dir()'s. The registration code is moved verbatim into register_user_service, and a new golden test pins the plist byte for byte (run against origin/main's code too, below).

Another home: a headless child process, no service manager.

• Bin, logs, database dir, the UI pid file and the incompatible-database marker are all under <home>/.nodespace/. This uses daemon_setup::home_dir(), incompatible_database::marker_path() and grpc_client::default_socket_path().
• The socket is NODESPACED_SOCKET, else <home>/.nodespace/daemon[-dev].sock, passed explicitly to the child (managed_socket_path). On Windows the default pipe is \\.\pipe\nodespace-daemon-<sha256 prefix of the home>.
• A socket path too long to bind is refused before the start, with a message naming NODESPACED_SOCKET.
• start_daemon dispatches on the home and takes the registration and the child spawn as closures, so tests stand in for the service manager.
• The product check runs, but a mismatch for another home boots nothing out (evict_if_other_product takes the home).
• Windows: another home's daemon is stopped by the pid this app started, never by taskkill /IM. The log-rotation restart is skipped for another home.

Tests

All run with scratch homes. None runs launchctl, systemctl or reg: the service manager is a counting closure, and the child is /usr/bin/env, which prints its environment into the daemon log.

Each new test was mutation-checked on its own: one mutation applied, that test run, the file restored. Every mutation was caught by the assertion it targets.

Test	Mutation	Result
macos_plist_keepalive_tests::the_user_home_plist_is_unchanged_byte_for_byte (golden, full text)	RunAtLoad true → false in write_plist	caught
same golden test, against origin/main's code (main's four source files restored, only the test added)	none	passes: the user-home plist is byte-for-byte what main writes
daemon_home::tests::unset_or_the_users_own_directory_is_the_users_home	drop the same-directory arm (a trailing slash or symlink to $HOME becomes "another home")	caught
daemon_home::tests::any_other_directory_is_another_home	any set NODESPACE_HOME resolves to User	caught
daemon_home::tests::another_home_gets_its_own_pipe	another home dials the shared pipe	caught
daemon_home::tests::another_homes_daemon_is_stopped_by_process_id_never_by_name	another home stopped with taskkill /IM	caught
daemon_home::tests::the_child_runs_headless_on_the_homes_endpoint	child gets the launcher args (--tray)	caught
daemon_home::tests::a_socket_path_too_long_to_bind_is_refused_before_starting	remove the length check	caught
product_eviction_tests::another_homes_mismatch_never_boots_out	disable the another-home arm (mismatch boots out)	caught
start_daemon_tests::only_the_users_home_is_registered_with_the_service_manager	start_daemon registers another home	caught
other_home_tests::another_home_never_touches_the_users_home (end to end: HOME/NODESPACE_HOME scratch dirs, counting registration, /usr/bin/env as the child)	(a) home_dir() back to dirs::home_dir()	caught (UI pid file lands in the user's home)
	(b) marker path back to dirs::home_dir()	caught (user's marker would be cleared)
	(c) child not given NODESPACED_SOCKET	caught
	(d) client's default socket back under $HOME	caught
	(e) managed socket for another home uses the $HOME formula	caught
unix_quit_signal_tests::a_program_path_with_spaces_is_matched_whole (review round 1: pid ownership under a home path with spaces)	match only the first whitespace-separated word again	caught
	(review round 2) drop the empty-program guard	caught

Also run, all in the machine slot:

• cargo clippy -p nodespace-app-lib --all-targets -- -D warnings -D clippy::unused-async: clean.
• cargo nextest run -p nodespace-app-lib --lib: 349 of 349 pass (head 3ac18333).
• bun test scripts/: 1448 pass.
• bun run scripts/check-pro-boundary.ts: clean.

No frontend files changed. Windows-only lines (resolve_pipe_name, the two taskkill call sites, CREATE_NO_WINDOW, the rotation early return) are not compiled on this Mac. Their logic lives in the pure functions tested above.

Coordination

• PR Install coexistence checks and other-product notices (ADR-084 §4.3, §5) #3606 (install coexistence) rewrites ensure_daemon_running into record_outcome(bring_up_daemon(..)). This PR changes the first line of that body and the registration block, and adds no new return, so a rebase either way is a mechanical conflict.
• The CLI's half (uninstall and logs) is Make the CLI's uninstall and logs commands follow NODESPACE_HOME #3612; the CLI's socket fallback is untouched here.

Documentation

• https://github.com/NodeSpaceAI/nodespace-docs/pull/111 (held too): the decision, "Another NodeSpace home" in architecture/daemon-cli-lifecycle.md, plus a correction to daemon-cli-protocol.md's socket claim.

🤖 Generated with Claude Code

[mstomar125]

Pragmatic Code Review: PR #3637 (round 1)

Review Type: Initial Review (head c688d089, one commit on origin/main, medium depth: only high-confidence findings)

The PR is held for the design discussion on nodespace-docs#111. That is expected and is not a finding here.

Requirements Check (#3616)

• ✅ Decide and document first. nodespace-docs#111 adds "Another NodeSpace home" to daemon-cli-lifecycle.md, with the decision, the options considered and what is not covered. It also corrects the socket claim in daemon-cli-protocol.md.
• ✅ Implement it, tested with scratch homes and no real launchctl. The service manager is a counting closure in the tests and the child is /usr/bin/env. boot_out and the registration are injected.
• 📝 The docs and the code agree on every row of the decision table except two "Stop" caveats (findings 1 and 2 below). In both cases either the code or the docs row should change.

What I verified

1. Default home: unchanged from origin/main. DaemonHome::current() with NODESPACE_HOME unset gives User(dirs::home_dir()). With it naming the user's home it gives the same, still spelled the way dirs spells it.

• Paths: managed_socket_path(User) is still home.join(daemon_socket_relative()), ignoring NODESPACED_SOCKET as before. The UI pid file (home_dir()), the marker and the client's default socket all resolve to the same paths as main, including the /tmp fallback.
• macOS and Linux registration: register_user_service is the old block moved verbatim. The plist is pinned by the golden test, and the systemd unit gets the same arguments.
• Windows: the spawn and autorun get home.join(DAEMON_LOG_DIR), which is the old log_dir. The pipe is the shared one, and windows_stop_args(User) is the same /F /IM <image>.
• Stop paths: the kill and stop paths are the same except for log text.

2. Another home never reaches the user's registration or daemon.

• Service managers: launchctl is reached only from bootstrap_launchd_agent (in register_user_service) and from boot_out_service_registration, and evict_if_other_product now returns before the boot-out for Other. systemctl is the same. reg add is reached only from register_user_service, and from the Windows rotation restart, which now returns early for Other. taskkill /IM is reached only for User.
• Signals: kill_stale_daemon_sync, signal_daemon_to_stop and kill_running_daemon (Unix) match on <other>/.nodespace/bin/<daemon> as argv0. So even a NODESPACED_SOCKET pointing at the user's socket cannot SIGTERM or SIGKILL the user's daemon, and remove_socket_if_stale still deletes only a socket nobody answers on.
• Files and sockets: the gRPC client, daemon_status_body, the post-startup reachability probe and the reset command all resolve through resolve_socket_path, which follows the home. The marker matches the daemon's own nodespace_dir() marker. The lock sits beside the socket, so it is per home too.

3. Child lifecycle.

• Reaping: a waiter thread reaps the child. The parent's log fds close when command drops, so the daemon's own stdio rotation stays valid.
• Environment: the child is headless, since the daemon shows a tray only with --tray. NODESPACED_SOCKET is set explicitly, which matters because the daemon's fallback uses $HOME.
• Socket path length: the SUN_LEN check uses SocketAddr::from_pathname, which is the same >= sun_path.len() rule bind applies.
• After an app crash (Unix): the orphan is found again by socket holder plus argv0.

4. Windows and Linux lines compiled by reading. I found no compile or -D warnings problems. Specifically:

• #[cfg(unix)] check_socket_path(endpoint)?; as a statement is fine, and command stays mut on every platform.
• windows_stop_args and default_pipe_name are used on Windows outside tests. child_pid is cfg(windows) and is_other is used on both families.
• sha2 is a normal dependency, and as_encoded_bytes is stable.
• Everything else: the &Path and &PathBuf coercions at the moved systemd_user_service_dir, write_systemd_service, spawn_daemon_windows and register_autorun_windows call sites, home: &Path in check_and_rotate_live_logs, and the start_daemon closures borrowing socket_path before the wait.

No issue numbers in comments, no lint suppression, no Pro wording, no compatibility code.

Code Review Findings

🟡 1. [Improvement] Windows: another home's daemon can only be stopped through an in-memory pid, so after an app crash it can never be stopped again

📁 packages/desktop-app/app-lib/src/daemon_home.rs:105-127, :196-199
📁 packages/desktop-app/app-lib/src/daemon_setup.rs:345-366, :1013-1021

CHILD_PID lives only in the app process that spawned the child. A Windows child is not killed when its parent dies, so after a crash, a force-quit or a killed tauri dev, the daemon keeps serving the home's pipe. The next app run starts with CHILD_PID == 0:

• Binary unchanged: ensure_daemon_running finds the daemon Healthy and returns early. CHILD_PID stays 0, so on quit windows_stop_args returns None and signal_daemon_to_stop silently does nothing. The orphan outlives every later run until a reboot or a manual kill.
• Binary changed, or the pipe stayed busy: kill_running_daemon warns and returns without stopping it, so the update does not take effect.

This contradicts the docs' "Stop" row ("Quitting the app stops the daemon, as it does for the user's home") and the "Updates" row on Windows. Unix has no such gap, because it finds the daemon by socket holder plus argv0 rather than by a remembered pid.

Recommendation: persist the child's pid under the home, for example <home>/.nodespace/daemon[-dev].pid next to the UI pid file. Then windows_stop_args(Other, ..) can fall back to it, after checking that the process's image path is <home>\.nodespace\bin\<daemon>.exe (for example with tasklist /FI "PID eq n" plus a path query, or by comparing the executable path the daemon reports over gRPC). Alternatively, state the limitation in the docs' Stop row. Either way the decision record should match what the code does.

Engineering principle: process ownership that has to survive a crash of the owner cannot live only in the owner's memory. Make the handle durable, or make the limitation explicit.

🟡 2. [Improvement] A home path containing whitespace makes the app unable to stop or restart another home's daemon (Unix)

📁 packages/desktop-app/app-lib/src/daemon_setup.rs:278-290, used at :326-327, :186-187 and :964-965

args.split_whitespace().next() == Some(expected_path)

The child is spawned with argv[0] = <home>/.nodespace/bin/nodespaced and no arguments. For a NODESPACE_HOME such as ~/NodeSpace Scratch, ps -o args= prints /Users/me/NodeSpace Scratch/.nodespace/bin/nodespaced, and its first whitespace token never equals the path. So:

• quit never SIGTERMs the child;
• kill_stale_daemon_sync and kill_running_daemon never stop it on a binary update, so the renamed-in binary is not what runs;
• the newly spawned child exits on the single-instance lock, and the app keeps using the old daemon.

This parser predates the PR. It was harmless while argv0 was always under the user's home directory, which in practice never contains spaces. This PR makes it reachable through a path the developer picks freely.

Recommendation: match the prefix instead. The trimmed args should equal expected_path, or start with expected_path followed by a space. Every call site spawns with a known argument list, so this is unambiguous. Add a unit test with a spaced path. Alternatively, refuse such a home up front next to the SUN_LEN check, with a clear message.

Engineering principle: a pid-ownership check is a safety guard. Its parser should not silently turn "this is ours" into "not ours" for valid inputs.

🟢 3. [Suggestion] ensure_daemon_running's own path derivations are untested; derive them once

📁 packages/desktop-app/app-lib/src/daemon_setup.rs:754-767, plus :155-157, :319-324 and :960-963

The end-to-end test calls start_daemon and spawn_child directly. It cannot call ensure_daemon_running, which needs an AppHandle. So if someone changes ensure_daemon_running back to dirs::home_dir(), or computes bin_dir, log_dir, db_dir or the socket from the user's home, every test still passes, and that function is the main entry point. The installed-daemon path home.join(DAEMON_BIN_DIR).join(daemon_binary_name()) is also derived separately in four places.

Suggestion: a small pure DaemonPaths::for_home(&DaemonHome) -> { bin_dir, log_dir, db_dir, socket, installed }. Use it in ensure_daemon_running, kill_stale_daemon_sync, signal_daemon_to_stop and kill_running_daemon, and give it one test per variant. Then the scratch-home invariant is pinned where it matters.

Engineering principle: DRY plus testability. Derive a security-relevant mapping once, test it once, and have every caller use it.

Nitpicks

• Nit: daemon_setup.rs:770-778. The create_dir_all contexts still say ~/.nodespace/.... For another home that message points at the wrong directory. Use bin_dir.display() and the like.
• Nit: daemon_home.rs:118 and :196-199. CHILD_PID is written on Unix but read only on Windows (child_pid is cfg(windows)). Consider cfg(windows) on the store, or say in the doc comment that it is Windows-only bookkeeping.
• Nit (docs PR): "Deleting the other home removes everything the run made" in Why a child process overstates the isolation. install_skill runs on every startup and writes ~/.nodespace/setup.json and the agents' skill directories under the user's home, and onboarding writes ~/.nodespace/config.json. "Not covered" lists this, but the bullet should be scoped to what the daemon management creates.
• Nit (docs PR): the reverse direction on Windows is worth one line under "Not covered". The user's-home app still stops its daemon with taskkill /IM, so quitting it, or its binary-update restart, also kills a concurrently running other-home child, for example a dev-build run beside the release app. This is unchanged by design. It is the only remaining way one home's app can stop another home's daemon.

Summary

The design is clean and well scoped. The user's home is provably unchanged: the code moved verbatim and the golden plist test passes against main's own code. Another home is kept away from launchctl, systemctl, reg and taskkill /IM, and from the user's socket, pid file, marker, logs and daemon by construction. The test that "another home never touches the user's home" is strong. Neither 🟡 finding breaks isolation. Both are lifecycle gaps where the docs' "Stop" and "Updates" rows promise more than the code delivers: on Windows after an app crash, and on Unix for a home path containing whitespace. I'd fix both in code, or narrow the docs rows, before the PR leaves "held". There are no blockers.

Recommendation: APPROVE (address 🟡 1 and 🟡 2 in code or in the docs decision before merge; 🟢 3 and the nits are optional)

[mstomar125]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[mstomar125]

Address review, round 1 (head 38d675a2)

Addressed

• Finding 2 (spaced home path, Unix), fixed in code. process_argv0_matches now goes through a new pure command_line_runs(args, program). It matches the whole program path, followed by whitespace or the end of the line, instead of the first whitespace-separated word. For a path without spaces the answer is unchanged, so the user's home behaves as before. Quit, the stale-binary kill and the update restart now recognise a daemon under a spaced home.
  • New test: unix_quit_signal_tests::a_program_path_with_spaces_is_matched_whole.
  • Mutation M11 (back to first-word matching) is caught.
• Finding 1 (Windows orphan after an app crash), addressed in the decision record, not in code. The docs PR's Stop row now states the limit: the child's pid lives only in the running app, so a daemon left behind by a crashed app keeps running until stopped by hand, and the next run uses it. CHILD_PID's doc says the same.
  • Why not code: a pid persisted under the home is only safe to taskkill /PID after checking the process's image path, because otherwise pid reuse could hit the user's daemon. That check is Windows-API code this Mac can neither compile nor test, and Windows runs against another home are rare.
  • Revisit if Windows isolated runs become routine.
• Nit: error contexts. The create_dir_all errors in ensure_daemon_running now name the actual directory.
• Nit: CHILD_PID. It is documented as Windows-only bookkeeping. The Unix waiter thread still needs the child handle for reaping, so the store stays unconditional.
• Docs nits (nodespace-docs#111, 511c4b1).
  • "Deleting the other home removes everything the daemon management made" is now scoped to the daemon management.
  • "Not covered" adds the Windows reverse direction: the user's-home app's taskkill /IM also stops another home's daemon.

Skipped

• Suggestion 3 (DaemonPaths::for_home). It would rewrite the start of ensure_daemon_running's body. Open PR Install coexistence checks and other-product notices (ADR-084 §4.3, §5) #3606 restructures that function, and this PR deliberately leaves it nearly untouched so the two rebase mechanically. The other-home invariant is pinned by the end-to-end test through home_dir(), managed_socket_path, the marker and the client socket. Each of those reverted to the user's home was caught (M10a to M10e).
  • Worth doing once Install coexistence checks and other-product notices (ADR-084 §4.3, §5) #3606 has landed.

Checks on 38d675a2, all in the machine slot

• cargo clippy -p nodespace-app-lib --all-targets -- -D warnings -D clippy::unused-async: clean.
• nextest -p nodespace-app-lib --lib: 349 of 349 pass.
• M11: caught.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: the fix changes the pid-ownership guard that every Unix signal in daemon_setup goes through, the user's home included. A second look at that guard and at the narrowed docs rows is worth more than the round trip.

[mstomar125]

Pragmatic Code Review: PR #3637 (round 2)

Review Type: Re-Review. It covers only c688d089..38d675a2 (one commit) and nodespace-docs#111 511c4b1, at medium depth, so only high-confidence findings are reported. Both PRs are still held for the design discussion, which is expected and not a finding.

Previous Review Summary

• Number of previous recommendations: 7 (🟡1, 🟡2, 🟢3 and four nits)
• Addressed: 6. 🟡2 is fixed in code. 🟡1 is fixed in the decision record, which was one of the two options round 1 offered. All four nits are done.
• Not addressed: 1. 🟢3 was skipped with a reason, and the reason holds.
• New issues found: 0 blocking, 2 nits

Round-1 item	Status
🟡1 Windows orphan after an app crash	✅ Documented. The Stop row and the CHILD_PID doc comment both state the limit, and both match the code (see 4 below).
🟡2 Home path with whitespace (Unix)	✅ Fixed by command_line_runs, with a test. It is exact for paths without spaces and safe for paths with spaces (see 1 and 2 below).
🟢3 DaemonPaths::for_home	⏭️ Skipped, reasonably. #3606 is open and touches daemon_setup.rs. Keeping ensure_daemon_running nearly untouched keeps the rebase mechanical. The end-to-end test's M10a to M10e mutations pin the other-home paths that matter.
Nit: create_dir_all error context	✅ The messages now name the real directory, and the rewrite is equivalent (see 3 below). No code or test matched the old ~/.nodespace/... text.
Nit: CHILD_PID is Windows-only	✅ Documented, and the doc is accurate: the value is read only through cfg(windows) child_pid(). One correction to the stated reason: reaping needs the Child, not CHILD_PID, so the store could have been gated. Documenting it is equally fine.
Nit (docs): cleanup claim	✅ It is now scoped to "everything the daemon management made" (daemon-cli-lifecycle.md:126).
Nit (docs): Windows reverse direction	✅ It is added under "Not covered" (:135).

Requirements Check (#3616)

• ✅ Decide and document first. Unchanged from round 1, and the decision record now matches the code on every row, including Windows Stop.
• ✅ Implement it, tested with scratch homes and no real launchctl. Unchanged, and the spaced-home case is now covered too.

What I verified

1. command_line_runs gives exactly the old answer for any program path without whitespace.

Old: args.split_whitespace().next() == Some(p). New: !p.is_empty() && args.trim().strip_prefix(p).is_some_and(|r| r.is_empty() || r.starts_with(char::is_whitespace)) (📁 daemon_setup.rs:299-305). split_whitespace, trim and char::is_whitespace all use the same Unicode White_Space property. For a non-empty p with no whitespace:

• Leading whitespace: both skip it.
• First token: it equals p exactly when the trimmed line starts with p and the next character is whitespace or the end of the line. That is the new predicate.
• Trailing whitespace and the trailing \n: the extra trim_end cannot change the answer. p ends in a non-whitespace character, so trimming the end never eats into it. After the prefix, the rest is either empty, or still starts with the same whitespace (true in both versions), or starts with a non-whitespace character (false in both).
• Empty program: false in both. The early return in process_argv0_matches still guards it, and the old version could never match an empty token anyway.
• ps failure, or a pid that does not exist: empty stdout gives false in both.

So the user's-home SIGTERM and SIGKILL paths (:186-187, :341-342, :975-976) behave exactly as on origin/main.

2. A path with spaces cannot be mistaken for another process in any practical case.

• A match needs the whole of <home>/.nodespace/bin/<daemon> as a prefix, followed by whitespace or the end of the line.
  • …/nodespaced.old, any longer name such as …/nodespaced2, and …/nodespaced/x all fail, because the next character is not whitespace.
  • A shorter prefix such as /Users/me/NodeSpace --tray fails.
  • The user's daemon (~/.nodespace/bin/nodespaced --tray) can never start with another home's path.
  • The CLI (…/bin/nodespace …) fails on nodespace against nodespaced.
• One residual case remains. ps joins argv with spaces, so an executable literally at /Users/me/NodeSpace run with the argument Scratch/.nodespace/bin/nodespaced would print the same line. That process would also have to hold this home's socket, because every caller filters by pids_holding_unix_socket first. This is the same trust class as argv0 spoofing, which the guard already accepts, so it is not a finding.
• The new test covers .old, the shorter prefix, no arguments, and arguments plus a trailing newline. Mutation M11 shows it fails if the code goes back to first-word matching.

3. The for dir in [&bin_dir, &log_dir, &db_dir] loop is equivalent to the three old calls (📁 daemon_setup.rs:784-789).

• Behaviour: same order, sequential awaits, and the same early return on the first failure.
• Types: all three are PathBuf, and &PathBuf: AsRef<Path>.
• Error text: only the context message changed, from a hard-coded ~/.nodespace/... to the real path. with_context builds that message lazily.

4. The docs Stop row and the 🟡1 decision are reasonable and honest (📁 nodespace-docs: architecture/daemon-cli-lifecycle.md:107). I checked each clause against the code:

• "That id lives only in the running app": CHILD_PID is a process-local static. The waiter thread clears it after wait() while it still holds the Child, so the pid cannot be reused while it is recorded.
• "keeps running until it is stopped by hand": for another home with CHILD_PID == 0, windows_stop_args returns None. Quit is then a no-op, and kill_running_daemon warns and returns. A reboot or the user's-home app's taskkill /IM also ends the daemon, and the row's neighbours and the new "Not covered" line already say so. The wording does not overclaim.
• "the next run uses it": with the binary unchanged, ensure_daemon_running sees it Healthy and returns early.
• "an updated binary takes effect only after that": with the binary changed, the kill is skipped, so the running daemon keeps serving.

The reasoning against a persisted pid also holds. taskkill /PID on a pid read from disk is only safe after checking the image path, and that check is Windows-API code this machine can neither compile nor test. Shipping it untested would trade a documented, recoverable limit for a guard nobody has run, where one bug would be enough to kill the user's daemon. The doc comment on CHILD_PID (📁 daemon_home.rs:116-124) says the same, and accurately.

Code Review Findings

No 🔴 or 🟡 findings.

Nitpicks

• Nit: the assert!(!command_line_runs(spaced, "")) check at 📁 daemon_setup.rs:3494 does not pin the !program.is_empty() guard. spaced.strip_prefix("") is Some(spaced), and the rest starts with /, so the assertion passes with or without the guard. The guard only matters for blank input: without it, command_line_runs("", "") and command_line_runs("\n", "") return true. That has no production impact, because process_argv0_matches returns early on an empty path. If the guard is worth keeping, assert !command_line_runs("\n", "") so that removing it is caught.
  • Principle: each assertion should fail when the code it is named for is removed (mutation-test each new check).
• Nit: the doc on process_argv0_matches (📁 daemon_setup.rs:270-272) still says argv[0] "is exactly expected_path". With spaced paths the check is now "the command line starts with the path, then whitespace or the end". ps joins argv with spaces, so argv[0] cannot be isolated. Pointing to command_line_runs from that doc would keep the two in step.

Summary

38d675a2 does what round 1 asked, and does nothing else.

• Guard change: the pid-ownership guard is provably unchanged for every path without whitespace, so the user's-home kill and stop paths behave exactly as before. It now recognises a daemon under a spaced home without matching .old siblings, shorter prefixes, the CLI or the user's daemon.
• Directory loop: a behaviour-preserving cleanup.
• Docs: the Stop row now states the Windows crash limit plainly and correctly. The cleanup claim is scoped, and the reverse taskkill /IM direction is recorded.
• Skip of 🟢3: justified by the open Install coexistence checks and other-product notices (ADR-084 §4.3, §5) #3606.

Only two optional nits remain.

Recommendation: APPROVE. The two nits are optional. The PR stays held for the design discussion on nodespace-docs#111, as intended.

[mstomar125]

Automated pragmatic-code-review (round 2): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[mstomar125]

Address review, round 2 (head 3ac18333)

Round 2 was APPROVE with two optional nits. Both are addressed:

• Empty-program guard. The test now asserts !command_line_runs("\n", ""), which only the !program.is_empty() guard rejects. Mutation M12 (removing the guard) is caught.
• process_argv0_matches doc. It now describes the whole-path match (the path, then whitespace or the end of the line) and points to command_line_runs.

On the round-2 correction about CHILD_PID: agreed, reaping needs the Child handle, not the static. The doc comment says only that CHILD_PID is read on Windows, which stays accurate.

Checks, all in the machine slot:

• crate-scoped clippy with -D warnings -D clippy::unused-async: clean.
• nextest -p nodespace-app-lib --lib: 349 of 349 pass.
• M12: caught.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: one test assertion and one doc comment changed. There is no logic change, and the mutation check confirms the new assertion does its job.

Both this PR and nodespace-docs#111 stay held for the design discussion.

[mstomar125]

@malibio held for the design discussion. Decision: NodeSpaceAI/nodespace-docs#111. Summary on #3616.

🤖 Generated with Claude Code

[malibio]

Independent review for the design discussion (head 38d675a2)

Review type: the two earlier rounds on this PR are the author's own automated reviews and already cover this head; there are no new commits. This is a separate read on behalf of the owner, limited to whether the design in nodespace-docs#111 holds up and whether doc and code agree. I do not repeat the earlier findings. Nothing was built or run.

Verdict: the diff does what the decision table says, and the user's-home path is unchanged. No blocker inside the diff. Three things should be settled with the design before this leaves "held".

Requirements (issue 3616)

• ✅ Decided and documented first (nodespace-docs#111, still proposed).
• ✅ Implemented, tested with scratch homes and no real service manager. Registration and boot-out are injected closures; the child is /usr/bin/env.

What I confirmed by reading

• User's home unchanged: register_user_service is the old block moved; managed_socket_path(User) is the old expression; the golden test pins the whole plist.
• No service manager for another home: launchctl and systemctl are reachable only from register_user_service and the boot-out, and both are behind the User arm. The Windows rotation restart returns early.
• Unix recovery after an app crash: the next run finds the daemon by socket holder plus whole binary path, so reuse, stop on quit and update restart all work, provided the home is spelled the same way.
• Project rules: no compatibility code, no #[allow], no issue numbers in comments, no product naming, no cargo spawn.

Findings

🟡 1. Agents started by the other-home daemon dial the user's daemon

📁 packages/agent/src/pty/session.rs:64 (not in this diff), packages/cli/src/lib.rs:220-228

• What happens: the agent environment is cleared to HOME, PATH, TERM, SHELL, LANG, USER, TMPDIR. The child daemon has NODESPACED_SOCKET set by this PR, but does not hand it on, so the agent's CLI falls back to $HOME/.nodespace/.
• Scenario: an agent session started in a scratch-home app reads and writes the user's real database.
• Recommendation: forward the daemon's own socket to the agents it starts, in this PR or one that lands first. It is the one remaining path by which an other-home run reaches the user's data.
• Principle: complete mediation.

🟡 2. The child outlives a crashed app

📁 packages/desktop-app/app-lib/src/daemon_home.rs:151, :198-204

• What happens: stdin is null and nothing links the child to the app. A crash or force-quit leaves a daemon with the model loaded for a home that may already be deleted. On Windows it can then never be stopped by the app, as the doc records.
• Recommendation: give the child a pipe on stdin and have the daemon shut down on end-of-file, or use a Job Object on Windows. This would replace CHILD_PID, windows_stop_args' pid arm and the rotation special case with one rule.
• Principle: make the lifetime structural instead of tracked.

🟡 3. child_command drops the daemon profile's service_env

📁 packages/desktop-app/app-lib/src/daemon_home.rs:142-153

• What happens: write_plist renders profile.service_env into the registration. The child gets only NODESPACE_HOME and NODESPACED_SOCKET.
• Scenario: an app built on core that relies on that extension point gets a differently configured daemon for another home, with no error.
• Recommendation: apply daemon_profile::active().service_env in child_command and assert it in the_child_runs_headless_on_the_homes_endpoint, or state in the doc that the child does not carry it.

🟢 4. The other home is used as typed

📁 packages/desktop-app/app-lib/src/daemon_home.rs:41, :87

• Windows: the pipe hash is over the raw path bytes, so two spellings of one home give two pipes and two daemons on one database directory, with no lock file to stop the second.
• Unix: lsof matches the bound string, so a daemon started as /tmp/x is not recognised by a run with /private/tmp/x.
• Relative or empty value: every derived path becomes relative to the working directory.
• Recommendation: in resolve, require an absolute path and canonicalise the other home once it exists; hash that.

🟢 5. Tests

📁 packages/desktop-app/app-lib/src/daemon_setup.rs:4521-4525, packages/desktop-app/app-lib/src/services/grpc_client.rs:578

• Environment: another_home_never_touches_the_users_home sets HOME in-process and depends on nextest's process-per-test. Say so where a plain cargo test user will see it, or take the lock other env tests use.
• A now-fragile older test: resolve_socket_path_honors_env_override_then_falls_back asserts the default is under dirs::home_dir(). default_socket_path now follows NODESPACE_HOME, so that test fails when the variable is set in the runner's environment. Clear it in the test.
• Golden coverage: the plist is pinned byte for byte; the systemd unit is not.

Nitpicks

• Nit: bundled_model.rs:133 holds a second resolver of the same home rule. One should call the other.
• Nit: with NODESPACED_SOCKET, another home and a daemon started by hand, the first run waits out kill_running_daemon's five seconds and spawns a child that exits on the lock (inferred). Consider treating that combination as "manage nothing".
• Nit: the Settings commands windows_autorun_present and remove_windows_autorun still run reg against the user's Run value from an other-home app.

Recommendation: COMMENT. Hold as planned; settle findings 1 to 3 with the design in nodespace-docs#111.

[malibio]

Independent read for the design discussion, on behalf of the owner. Verdict: COMMENT. The diff matches the decision table in nodespace-docs#111 and leaves the user's home unchanged; no blocker inside the diff. Three points (agents reaching the user's daemon, the child's lifetime, service_env) should be settled with the design. Full findings are in the comment above.

[malibio]

🟢 The other home is kept as typed. A relative or empty NODESPACE_HOME makes every derived path relative to the working directory, and on Unix lsof matches the bound string, so a daemon started as /tmp/x is not recognised by a run with /private/tmp/x. Require an absolute path and canonicalise once the directory exists.

[malibio]

🟢 Hash of the raw spelling: C:\s\a and c:\s\a\ give two pipes, so two daemons on one database directory, with no lock file on Windows to stop the second. Hash the normalised home.

[malibio]

🟡 Two things in child_command.

• service_env: write_plist renders daemon_profile::active().service_env into the registration; the child gets none of it. Apply it here and assert it in the test below, or say in the doc that the child does not carry it.
• Lifetime: with stdin null nothing links the child to the app, so a crashed app leaves a daemon with the model loaded. A pipe here, with the daemon shutting down on end-of-file, would replace CHILD_PID and the Windows pid arm with one rule.

[malibio]

🟢 After this arm the caller goes on to use the mismatching daemon when the binary is unchanged. That is a fair choice, but the doc's Product check row should say it, and it interacts with the call hold proposed in nodespace-docs PR 112.

[malibio]

🟢 This sets HOME for the whole test process and relies on nextest's process-per-test. Related: grpc_client's resolve_socket_path_honors_env_override_then_falls_back still asserts the default is under dirs::home_dir(), which now fails when NODESPACE_HOME is set in the runner's environment; clear it there.

[malibio]

Not merging this. Please close it, and close #3616 as not planned.

The design it implements (NodeSpaceAI/nodespace-docs#111) is not accepted; the reasoning is in the comment there: https://github.com/NodeSpaceAI/nodespace-docs/pull/111

In short: NODESPACE_HOME is a test setting for the daemon and the CLI. The desktop app does not read it, and manages one daemon through the shared registration. So there is no child-process daemon, per-home socket or pid tracking to add.

One change in the other direction is wanted: the app's existing read of the variable in packages/desktop-app/app-lib/src/bundled_model.rs is removed, so the app copies the model where the daemon it registers looks for it.