[malibio]

Summary

The run-app skill's debug-bridge recipe (added with #3776) said a scratch HOME plus a pre-seeded daemon keeps the user's dev daemon untouched. It does not: the debug launchd label (app.nodespace.daemon.dev) is per user, and the app re-registers it whenever it rebuilds or finds a changed sidecar, replacing the user's loaded job with one pointing at the scratch install. This happened during the #3739 trial and was restored by hand.

This corrects the recipe: it states the limit, asks for the user's go-ahead before launching, and adds the note-then-restore steps. The matching section in nodespace-docs/development/browser-dev-mode.md is already corrected on docs main.

Docs-only (one skill file). bun run quality:fix is clean.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3779

Review Type: Initial review. Docs-only change to .claude/skills/run-app/SKILL.md (+19/-9), a follow-up correcting the debug-bridge launch recipe.

Summary

The correction is accurate and a net improvement. The earlier text claimed a scratch HOME plus a pre-seeded daemon isolates the user's dev daemon; this PR states plainly that it cannot, asks for the user's go-ahead, and adds note-before and restore-after steps. I checked each claim against packages/desktop-app/app-lib/src/daemon_setup.rs and found no technical errors. There is one stale cross-reference left behind by the renumbering (below) that should be fixed.

Technical accuracy (against the code)

• Label is per user, not per HOME. launch_agent_label_for(true) returns app.nodespace.daemon.dev. bootstrap_launchd_agent targets gui/<uid>/<label>, so neither HOME nor NODESPACE_HOME scopes it. Correct.
• Mechanism of replacement. When the user's job is already loaded, launchctl bootstrap fails, and the fallback runs enable, then bootout of the plist path and of gui/<uid>/<label>, then bootstraps again with the app's own plist. That is exactly "replaces the loaded job with one pointing at the scratch install". Correct.
• "Registers it again whenever it rebuilds or finds a changed sidecar". In bring_up_daemon the early return Ok(()) ("already running and healthy") is reached only when evicted is false, binary_updated is false and the socket is healthy. A size change in extract_sidecar_if_changed (a rebuild) or an invalid signature sets binary_updated, which kills the daemon and falls through to write_plist plus bootstrap_launchd_agent. Correct, and it explains why pre-seeding is not a guarantee.
• Plist location. write_plist uses launch_agents_dir(&home) with home = dirs::home_dir(), which follows $HOME. Under HOME=/tmp/nsh the app writes /tmp/nsh/Library/LaunchAgents/app.nodespace.daemon.dev.plist and bootstraps that. The user's ~/Library/LaunchAgents/app.nodespace.daemon.dev.plist is untouched, so restoring with bootout then bootstrap of the user's own plist is correct.
• Commands. gui/$(id -u) is the right domain, and launchctl print gui/501/app.nodespace.daemon.dev shows path, program and state as the doc says (I confirmed read-only on this machine). pid appears while running. The restore commands are valid.
• Socket and home paths. daemon-dev.sock under .nodespace/ matches nodespace_proto::socket. Setting NODESPACED_SOCKET for the pre-seeded daemon but not for the app is consistent with evict_other_product_daemon, which skips its check when dialed != socket_path.
• kill_stale_daemon_sync. It kills only pids holding the home-relative socket whose argv0 is the installed binary, so under a scratch HOME it cannot touch the user's daemon. The doc's "kills a daemon on the default socket" is accurate under either HOME.

Findings

Critical Issues

None.

Suggested Improvements

• [Improvement] .claude/skills/run-app/SKILL.md (paragraph after the numbered list, "cannot do step 2 or 3"): stale step reference. The renumbering moved the HOME-dependent steps to 3 (start scratch daemon with HOME=/tmp/nsh) and 4 (launch with HOME=/tmp/nsh CARGO_HOME=… RUSTUP_HOME=…). Step 2 is now only the file pre-seed. A reader reading "step 2 or 3" is pointed at the wrong steps. Suggest "cannot do step 3 or 4". Notes for DRY and accuracy: step numbers in prose are brittle, so referring to "the scratch-HOME steps" avoids this recurring.
• [Improvement] same section, steps 2-4: the new lead paragraph says pre-seeding "does not stop" replacement, but nothing says what the pre-seed still buys. From the code, it avoids registration only while the bundled sidecar is unchanged in size and validly signed (the "healthy, unchanged" early return). dev:tauri runs dev:tauri:sidecars first, so a rebuild can defeat it. One clause such as "pre-seeding avoids the re-registration only while the sidecar is unchanged; do not rely on it" would stop a reader either skipping steps 2-3 or trusting them.
• [Improvement] step 5: ~/Library/LaunchAgents/... is correct only in a shell whose HOME is the real one. The recipe runs the app with an inline HOME=/tmp/nsh, so this is fine if followed literally, but if a session has exported HOME=/tmp/nsh the ~ expands to the scratch plist and "restores" the wrong job. Worth saying "the user's real home". Step 1's path already records the correct plist, so "bootstrap the plist path from step 1" would be unambiguous and self-checking.

Nitpicks

• Nit: step 5 says confirm path and program match step 1; pid will legitimately differ after the bootstrap restarts the job. A half-sentence saying so avoids a false alarm, since step 1 asked the reader to note pid.
• Nit: "registers it again" in the lead paragraph reads as if the app had registered it before; "re-registers it" would be clearer.

Consistency with browser-dev-mode.md

The corrected "What to know" bullet in nodespace-docs/development/browser-dev-mode.md (docs commit 585923b) makes the same claims: not isolatable, per-user label, re-registration on rebuild or changed sidecar, note launchctl print before, bootout then bootstrap the user's plist afterwards. The two agree. The SKILL adds the go-ahead requirement and the scratch-cleanup step, which are appropriate for an agent-facing recipe.

Documentation rules

• No issue numbers in the content, no new .md files, and no Sync-boundary wording ("NodeSpace Sync", tenant, Supabase) in the diff.
• File size is 10,278 bytes, well under the 20,000-character cap noted for SKILL.md; frontmatter is unchanged. gate-scope.test.ts already classifies this path as docs-only.

Recommendation

APPROVE. Technically correct and an improvement over the misleading recipe. Please fix the stale "step 2 or 3" reference (one-word change) before or alongside merge; the other items are optional polish.

[malibio]

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

[malibio]

Review response

Addressed (5/5)

• 🟡 Stale cross-reference: the HOME-dependent steps are now 3 and 4, so the note reads "cannot do step 3 or 4".
• 🟡 The lead paragraph now says what pre-seeding buys: it avoids re-registration only while the sidecars stay unchanged and validly signed, and dev:tauri rebuilds them first, so it should not be relied on.
• 🟡 Step 5 restores from "the plist path noted in step 1" instead of a hardcoded ~/Library/LaunchAgents/..., so it is correct in any shell.
• Nit: step 5 says the pid will differ (the job restarts) and compares only path and program.
• Nit: "registers it again" is now "re-registers it".

Docs-only wording; bun run quality:fix is clean.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: Wording corrections in one skill file, applied as the reviewer suggested; no new claims or commands.

🤖 Generated with Claude Code