npm test no longer drives the host's real service manager (#602)

[philcunliffe]

Root cause

test/core/attach-enable-resume.test.js and test/core/attach-endpoint-fallback.test.js both wrote a service marker under the real service label (com.hyperparam.hypaware / hypaware.service) into a temp HOME, so serviceDaemonStatus would report installed: true, and then let the daemon code run for real. The fixture comment recorded the assumption:

The environment this suite runs in has no systemctl / launchctl binary reachable, so the subsequent restartServiceDaemon call genuinely throws (spawn ENOENT).

That is true of the CI container and false of every developer machine.

A temp HOME does not sandbox the service manager. It relocates the plist/unit file, which is the only thing serviceDaemonStatus's installed-check reads. launchd and systemd address a service by label inside a per-uid namespace, and resolveTarget in src/core/daemon/macos.js derives userDomain from process.getuid(), never from HOME. So the command that actually ran on a macOS host was launchctl kickstart -k gui/<uid>/com.hyperparam.hypaware: the developer's own daemon, once per npm test.

Fix

runServiceCommand in src/core/daemon/service_ops.js is the single seam every service-manager spawn passes through. It now returns a rejected ServiceManagerSandboxError instead of spawning when NODE_TEST_CONTEXT is set (the Node test runner sets it in every test child, and it is inherited by anything a test spawns), unless HYP_ALLOW_REAL_SERVICE_MANAGER=1.

The guard is deliberately at the seam rather than in each fixture, because fixture-by-fixture discipline is exactly what failed here. Three properties are deliberate:

• It is a rejection, not a synchronous throw: callers such as installLaunchAgent's best-effort bootout attach a .catch() to the returned promise, which a synchronous throw would sail past.
• It binds to the test runner only. Hermetic smokes (hyp smoke ...) and the packaged CLI do not set NODE_TEST_CONTEXT, so the acceptance tier in docs/ACCEPTANCE.md that is supposed to install and start a real daemon is untouched and needs no opt-in. npm run smoke -- daemon_install_render and core_boot_noop both stay green.
• The refusal message names the way out (inject a fake adapter, or the opt-in env var) so the next author does not guess.

Alongside it:

• The duplicated installFakeDaemonService fixture moves to test/helpers/daemon_service_fixture.js, carrying the rule in a comment where the next author of this exact mistake will read it.
• attach-enable-resume.test.js's test body now says why the restart fails (the guard, not a missing binary).
• LLP 0181 (llp/0181-tests-never-drive-the-real-service-manager.decision.md, Systems: Daemon) records the rule, the guard, and the two alternatives rejected.

On the reporter's "pass fake adapters in both test files"

enableClientAdapter does take a restartDaemon injection point, but it is not threaded through runAttach, which is what these two tests exercise end to end. Widening a command entrypoint's signature to serve a test, and then remembering to thread it at every future call site, is strictly weaker than refusing at the seam. LLP 0181 §Alternatives records this and keeps the seam as the right move if attach ever needs to script daemon behaviour rather than merely avoid it.

Regression test

The macOS symptom cannot be reproduced in this Linux CI container: it has neither launchctl nor systemctl, which is precisely why the bug was invisible here. So test/core/service-manager-test-sandbox.test.js pins the mechanism, not the symptom. It puts recording launchctl and systemctl stubs on PATH (this is the only thing a developer machine has that the container does not), runs the two attach fixtures in a child node --test, and asserts that neither stub was ever invoked. Two further tests cover the guard directly and its opt-in.

Note the recorded commands are systemctl ones: on a Linux host with systemd this is the same escape, hitting the user's real hypaware.service. It is not a macOS-only bug, just a macOS-only report.

Before the fix

not ok 1 - the attach fixtures never reach a real service manager, even when one is on PATH
---
duration_ms: 30241.869824
type: 'test'
failureType: 'testCodeFailure'
error: |-
a fixture spawned the host service manager: a temp HOME does not sandbox launchd/systemd, so this command named the developer's own daemon (LLP 0181)
+ actual - expected
+ [
+ 'systemctl --user show hypaware.service --property=LoadState,ActiveState,MainPID',
+ 'systemctl --user show hypaware.service --property=LoadState,ActiveState,MainPID',
+ 'systemctl --user restart hypaware.service'
+ ]
- []
code: 'ERR_ASSERTION'
operator: 'deepStrictEqual'
...
not ok 2 - runServiceCommand refuses to spawn under the test runner
---
error: |-
Expected values to be strictly equal:
+ actual - expected
+ 'Error'
- 'ServiceManagerSandboxError'
...

(The 30s duration is itself the bug: the stubbed systemctl restart "succeeded", so the enable flow went on to wait for a gateway bind that no daemon was ever going to publish.)

After the fix

TAP version 13
# Subtest: the attach fixtures never reach a real service manager, even when one is on PATH
ok 1 - the attach fixtures never reach a real service manager, even when one is on PATH
---
duration_ms: 214.317006
type: 'test'
...
# Subtest: runServiceCommand refuses to spawn under the test runner
ok 2 - runServiceCommand refuses to spawn under the test runner
---
duration_ms: 0.442863
type: 'test'
...
# Subtest: the explicit opt-in still spawns
ok 3 - the explicit opt-in still spawns
---
duration_ms: 21.746198
type: 'test'
...
1..3
# tests 3
# suites 0
# pass 3
# fail 0

Full suite

# tests 3380
# suites 0
# pass 3379
# fail 0
# cancelled 0
# skipped 1
# todo 0
# duration_ms 15816.011856

npm run typecheck exits 0.

Out of scope: the shutdown-wedge amplifier

The reporter's second finding is confirmed and deliberately not fixed here. hypaware-core/plugins-workspace/ai-gateway/src/proxy.js:91 is:

asyncstop(){awaitnewPromise((resolve,reject)=>{server.close((err)=>(err ? reject(err) : resolve(undefined)))})awaitPromise.allSettled(Array.from(pendingFinalizers))}

server.close() stops accepting new connections and then waits for every existing one to end; it never calls closeIdleConnections() / closeAllConnections(). An open SSE stream never ends on its own, so a graceful shutdown wedges until launchd escalates to SIGKILL (the observed exit status -9), which is what turned each kick into a mid-stream truncation rather than a clean restart. That is a real daemon-shutdown defect independent of the test suite and deserves its own issue and its own fix.

Fixes#602

[philcunliffe]

Review round 1 - findings (all three fixed)

Reviewed 6ba9897 (no Codex on this host, so single-family review). One blocker, two preferences. All addressed in 8c1fe48.

1. blocker - the guard missed two invocation modes that still drove the host's real service manager

src/core/daemon/service_ops.js:75 (if (process.env.NODE_TEST_CONTEXT === undefined) return undefined).

NODE_TEST_CONTEXT is set by node --testonly in process-isolation mode. Two shapes leave it unset, and in both the pre-fix behaviour returned in full. Measured at the PR head with recording launchctl/systemctl stubs on PATH:

MODE A: node test/core/attach-enable-resume.test.js (no --test)
systemctl --user show hypaware.service --property=LoadState,ActiveState,MainPID
systemctl --user restart hypaware.service <-- the developer's own daemon
MODE B: node --test --experimental-test-isolation=none <the two attach fixtures>
systemctl --user show hypaware.service ...
systemctl --user restart hypaware.service <-- same

Probe: node --test file gives NODE_TEST_CONTEXT="child-v8"; node file gives undefined; node --test --experimental-test-isolation=none file gives undefined.

Mode A is a normal habit (running one node:test file directly). Mode B is reachable through the repo's own entrypoint, since scripts/run-tests.js forwards extra args verbatim, so npm test -- --experimental-test-isolation=none disabled the guard for the whole suite. The original regression test could not catch either: it spawns node --test in default isolation, the one mode the guard covered. So #602 was still open on the very paths a developer iterating on these files would use, while LLP 0181 declared it closed.

Fixed with underTestRunner(), covering all three shapes (NODE_TEST_CONTEXT, --test in execArgv, a .test.js in argv), plus a Mode A and a Mode B test so the hole stays closed. Verified post-fix: stub log empty in both modes.

False-positive analysis (the risk of widening a refusal): --test is not permitted in NODE_OPTIONS, so no production launch carries it. Every caller of runServiceCommand was traced up through restartServiceDaemon / startServiceDaemon / the installers to hyp daemon *, hyp join/central, the walkthrough, and attach - none takes a filesystem path argument, so no real invocation puts a .test.js in argv. A contrived one fails safe and loud (a ServiceManagerSandboxError naming the opt-in), not silently. Confirmed empirically that node bin/hypaware.js daemon restart is still unrefused, and both daemon smokes pass.

2. preference - LLP 0181 attributed an agent-generated decision to a human

Author: Phil / Claude on a doc neutral minted in this PR. The corpus already has the provenance marker: 35 LLPs carry Generated-by: neutral. Status: Accepted is consistent with precedent (0078, 0154), so only the attribution was wrong. Fixed: now Generated-by: neutral.

3. preference - the doc forbade the opt-in in the traditional suite, and the test landing with it used the opt-in

LLP 0181 said "Nothing in the traditional suite may use it", while the new test set HYP_ALLOW_REAL_SERVICE_MANAGER=1 in-process. The spawn was harmless (node -e), but the mutation is process-global, so under --experimental-test-isolation=none other tests would run with the guard disabled. Fixed: the rule is narrowed to its intent ("no test may use the opt-in to reach a real service manager"), the opt-in test now runs in a child process, and LLP 0017 gained an Extended-by: LLP 0181 forward-ref (a mechanical edit; nothing 0017 decided was touched).

Verified clean

Single seam confirmed: launchctl/systemctl appear only in src/core/daemon/{service_ops,macos,linux,install}.js; every adapter method routes through runServiceCommand, and service_ops.js:102 is the only spawn in the daemon tree. Install/uninstall/start/restart/status all pass through it.

No production leakage: nothing in src/, bin/, or hypaware-core/ sets NODE_TEST_CONTEXT. Read-only probes hitting the refusal are absorbed by serviceDaemonStatus's catch (degrades to loaded:false with a warn); state-changing ops surface it through describeError as a legible "restart step failed" naming the opt-in.

Acceptance tier unaffected - the point worth checking hardest, since a guard that silently disabled real daemon testing would be a worse regression than the bug: npm run smoke is node ./hypaware-core/smoke/index.js (no --test, no NODE_TEST_CONTEXT), no smoke file is named *.test.js, no smoke flow spawns a service manager, and no test invokes a smoke. daemon_install_render and daemon_foreground_start_stop both ok before and after.

Test quality: with service_ops.js reverted, the original test failed 2/3 and took 30.3s (the gateway-bind timeout caused by the stub's exit-0 "restart"), vs 0.3s guarded - so it genuinely pins the bug and is fast and hermetic. Conventions clean (no U+2014, no semicolons, no new @typedef, root-anchored type imports). LLP 0181's number was free and its cross-refs check out.

Post-fix: npm test 3381 pass / 0 fail / 1 skipped; npm run typecheck clean; both daemon smokes ok. Head advanced 6ba9897 to 8c1fe48, so the next tick re-reviews the new head.

Note for the maintainer, outside this PR's scope: the reporter's separately-flagged amplifier is confirmed real. hypaware-core/plugins-workspace/ai-gateway/src/proxy.js:91 awaits server.close() with no closeIdleConnections()/closeAllConnections(), so an SSE stream never ends on its own and graceful shutdown wedges until launchd escalates to SIGKILL. That is why a stray kick truncated streams instead of restarting cleanly. Worth its own issue.

[philcunliffe]

Review round 2 - findings (1 preference, fixed; no blockers)

Re-reviewed at head 8c1fe48 (round 1 reviewed 6ba9897). The round-1 blocker fix (underTestRunner() widening) holds up under independent measurement. One preference on the regression test's durability, fixed in 30998f1. Verdict: ready to merge.

Independent verification of the fix (not just re-reading the claims)

Built recording launchctl / systemctl stubs, put them first on PATH, and ran the whole suite - not just the two named fixtures, which is the strongest available proof of the issue's actual claim:

run	result	service-manager invocations
PATH=<stubs> npm test	3382 tests, 3381 pass, 0 fail, 1 skipped	0
PATH=<stubs> npm test -- --experimental-test-isolation=none	3382 tests, 3379 pass, 2 fail (unrelated, see below)	0

Production shape is genuinely unaffected (the risk of a widened refusal): with the same stubs on PATH, a plain node probe.js calling serviceDaemonStatus against a home holding a real unit marker still spawns systemctl --user show hypaware.service --property=LoadState,ActiveState,MainPID. So the third predicate arm does not false-positive outside a test.

Regression test genuinely pins the bug: with src/core/daemon/service_ops.js reverted to 9dbba3e, test/core/service-manager-test-sandbox.test.js fails 4 of 6; at head it passes 6 of 6.

Seam is still singular: launchctl / systemctl appear in src/ only under src/core/daemon/{service_ops,macos,linux,install}.js, and service_ops.js:129 is the only spawn in that tree. Every caller of restartServiceDaemon / startServiceDaemon (src/core/cli/walkthrough.js:1016, src/core/commands/central.js:406, src/core/commands/daemon.js:140,278, src/core/config/client_enable.js:207) takes no filesystem path, confirming the doc's claim that no real invocation carries a .test.js. Smoke tier untouched: no *.test.js under hypaware-core/, no smoke passes --test; daemon_install_render, daemon_foreground_start_stop, core_boot_noop all ok.

1. preference - the mechanism test hand-listed the fixtures it protects

test/core/service-manager-test-sandbox.test.js:43 (pre-fix):

/** * The fixtures that put a real service label on disk and then drive daemon * code for real. Add to this list, do not remove from it. */constREAL_LABEL_FIXTURES=['test/core/attach-enable-resume.test.js','test/core/attach-endpoint-fallback.test.js',]

LLP 0181#the-guard puts the refusal at the seam precisely because "fixture-by-fixture discipline is the thing that already failed". This list is that same discipline, one level up: the next author to reach for installFakeDaemonService would not think to add themselves, and the test would go on proving the rule for two files while a third escaped unobserved. Nothing enforced the comment.

Not a blocker - the seam guard protects an unlisted fixture regardless; only the evidence would go stale.

Fixed in 30998f1: realLabelFixtures() walks test/**/*.test.js for importers of helpers/daemon_service_fixture.js, so the three no-spawn runs cover every such fixture by construction. A discovery assertion runs first, because the failure mode of discovery is silent vacuity - a renamed or moved helper would otherwise leave all three runs executing zero files and passing green. (The walk skips the test file itself, which names the helper in a constant and would otherwise recurse into running itself.)

Verified landed: git show HEAD:test/core/service-manager-test-sandbox.test.js contains realLabelFixtures at line 54 and const REAL_LABEL_FIXTURES = realLabelFixtures() at line 77; the list literal is gone. Test goes 6/6 at head and 2/6 with the guard reverted, so the added test did not weaken the pre-fix failure signal.

Verified clean

• Conventions: no U+2014, no trailing semicolons, no new @typedef, no inline import('...') types, no unused imports left by the fixture extraction (both attach files still use every import). npm run typecheck exits 0.
• @ref honesty: LLP 0181#the-guard and #the-rule both exist as explicit anchors; LLP 0181's own links resolve (0017#install-global-package-then-service-manager matches ## Install: global package, then service manager; 0174#prompt exists). Number 0181 was free; Generated-by: neutral matches the 35-doc precedent; the Extended-by: line on 0017 is a mechanical forward-ref, permitted on an Accepted doc.
• Status contract preserved: the refusal is absorbed by serviceRuntimeStatus's catch (src/core/daemon/install.js:308), so LLP 0017#status-queries-never-raise still holds; only state-changing ops surface it.

Out of scope, now tracked

The --experimental-test-isolation=none run's 2 failures (hyp policy on a corrupt store..., a query whose heap growth exceeds the execution budget...) are pre-existing and unrelated: re-running that mode with all three PR-touched test files excluded reproduces exactly the same 2. Shared-process interference in an unsupported mode; not this PR's.

The shutdown-wedge amplifier this PR confirms and deliberately defers had no tracking issue, and #602 closes with this PR, so the finding would have been lost. Filed as #610 (ai-gateway proxy stop() never drains SSE connections, so daemon shutdown wedges until SIGKILL).

Post-fix numbers

npm test: 3383 tests, 3382 pass, 0 fail, 1 skipped. npm run typecheck: exit 0. Head advanced 8c1fe48 to 30998f1.

[philcunliffe]

Triage (rung: judgement) - no residual findings

Parked at triage after 2 review rounds with the head unreviewed. Verified round 1 and round 2's findings independently against the committed tree at 30998f1 rather than trusting the "ready to merge" verdict at face value.

What was checked

Round 1 (6ba9897f): 1 blocker, 2 preferences, all reported fixed in 8c1fe48.

• Blocker (underTestRunner() missing two invocation modes) - confirmed present in src/core/daemon/service_ops.js:70-79, all three predicate arms (NODE_TEST_CONTEXT, --test in execArgv, a .test.js in argv) intact.
• Preference (LLP 0181 attribution) - confirmed **Generated-by:** neutral in llp/0181-tests-never-drive-the-real-service-manager.decision.md.
• Preference (opt-in test mutating process-global env under shared isolation) - confirmed the opt-in test spawns a child process rather than mutating process.env in-process, and LLP 0181's text now reads "No test may use it to reach a real service manager" (narrowed from the blanket ban); LLP 0017 carries the Extended-by: LLP 0181 forward-ref.

Round 2 (8c1fe481…): 1 preference, reported fixed in 30998f1.

• Preference (REAL_LABEL_FIXTURES hand-listing) - confirmed the hardcoded array is gone; test/core/service-manager-test-sandbox.test.js now derives the list via realLabelFixtures(), which walks test/**/*.test.js for importers of test/helpers/daemon_service_fixture.js, with a discovery assertion that runs first to catch silent vacuity.

Independent evidence gathered in a fresh worktree at head 30998f1

• npm test: 3383 tests, 3382 pass, 0 fail, 1 skipped - matches the round-2 numbers exactly.
• node --test test/core/service-manager-test-sandbox.test.js: 6/6 pass.
• npm run typecheck: exit 0.
• Single-seam claim reconfirmed: grep -rn "spawn(" src/core/daemon/*.js returns exactly one hit, service_ops.js:129.
• The out-of-scope shutdown-wedge amplifier (ai-gateway proxy.jsstop() never draining SSE connections) was already filed as its own tracked issue, ai-gateway proxy stop() never drains SSE connections, so daemon shutdown wedges until SIGKILL #610, by the round-2 review - it is a distinct production defect, correctly kept out of this PR's diff, and already has a backlink, so it needs no further follow-up here.

Verdict

Every finding raised across both review rounds is genuinely resolved in the code at 30998f1, not merely asserted resolved. Nothing residual, blocking or otherwise. No follow-up issue opened.