fix(cli): propagate NODE_PATH to the generated service, not just PATH - #13
Draft
andromarces wants to merge 1 commit into
Draft
fix(cli): propagate NODE_PATH to the generated service, not just PATH#13andromarces wants to merge 1 commit into
andromarces wants to merge 1 commit into
Conversation
pnpm's .bin shim sets NODE_PATH to include the hidden hoist directory before invoking Node -- that's what lets the CLI resolve modules like bogon's undeclared b4a dependency. The generated service bypasses the shim and calls Node directly, so it inherited none of that. Module resolution then has no fallback specifically when normal ancestor resolution cannot reach the hidden hoist dir -- the pnpm global-virtual- store layout, where the resolved module's realpath sits inside the store rather than under the project tree -- and the service died with Cannot find module 'b4a'. The fix is a harmless no-op on other pnpm layouts, where resolution already succeeds without it. Probe the source install root (which exists at render time) for a node_modules/.pnpm/node_modules hoist dir, and if present emit NODE_PATH rooted at the service's eventual location (the durable copy when the invoking runtime is ephemeral, the source root otherwise) into all three platform generators: the Windows service script, the launchd plist, and the systemd environment file. Every prior committed assertion used a durable source checkout, where the source and destination roots are the same path, so a source/ destination swap in the derivation would have passed unnoticed. Added a dedicated case with its own installIo stub reporting the source hoist dir present while an ephemeral runtime is mid-copy, asserting NODE_PATH lands on the durable copy and contains no trace of the ephemeral source. Verified this case is load-bearing by temporarily inverting the probe/emit paths and confirming it fails, then reverting and confirming it passes again. Claude-Session: https://claude.ai/code/session_01RjMaDN6VbJP9pdbCY6u3Hk
andromarces
force-pushed
the
fix/service-node-path-propagation
branch
from
July 26, 2026 04:35
e83f7ae to
d0648b3
Compare
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
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.
Problem
pnpm's
.binshim setsNODE_PATHto include the hidden hoist directory (node_modules/.pnpm/node_modules) before invoking Node -- that's what lets the CLI resolve modules likebogon's undeclaredb4adependency (a separate upstream issue, not fixed here). The generated per-platform service bypasses the shim entirely and invokes Node directly, propagating onlyPATH. Module resolution then has no fallback, which is fatal specifically when normal ancestor-directory resolution cannot reach the hidden hoist dir -- the pnpm global-virtual-store layout, where the resolved module's realpath sits inside the store rather than under the project tree. There the service dies immediately withCannot find module 'b4a', while the CLI itself, launched through the shim, works fine. This is what installer step 4/5 surfaces asCodor did not become ready at http://127.0.0.1:8137. (The fix is a harmless no-op for other pnpm layouts, where module resolution already succeeds without it.)Fix
All three platform generators (Windows service script, launchd plist, systemd environment file) now emit
NODE_PATHalongsidePATH, derived the same way the entrypoint already is:installDurableRuntimemay copy the invoking runtime'snode_moduleswholesale to a durable location (~/.codor/runtime) before the service ever runs, so the hoist directory is checked for existence at the source install root (which exists at render time) but the value emitted is rooted at the destination (serviceLocation) -- the same splitsetup.tsalready makes for the entrypoint path atsetup.ts:667-668.NODE_PATHline is emitted at all -- a no-op for non-pnpm layouts.PATH=is already written there).Testing
packages/cli/src/setup-win32.spec.ts: added a case creating a.pnpm/node_moduleshoist dir under the fixture's checkout and asserting the rendered service script emits$env:NODE_PATH, plus negative assertions on the two existing cases (no hoist dir present -> noNODE_PATHline).packages/cli/src/index.spec.ts: updated the systemd dry-run inline snapshot to include the newNODE_PATH=line (this repo's own checkout is pnpm-linked, so the real hoist dir is detected); added positive assertions to the real systemd-write and macOS dry-run/install tests (both of which also run against this real checkout); added an explicit negative assertion to the existing npx-cache case, whose stubbedinstallIoreports no hoist dir at the source.installSource.installRootandserviceLocationhappen to be the same path -- so none of them alone would catch a source/destination swap in the derivation. Added a dedicated case (plainit, runs on every platform) sibling to the npx-cache test, with its owninstallIostub reporting the source hoist dir present while an ephemeral runtime is mid-copy to~/.codor/runtime, assertingNODE_PATHlands on the durable copy and contains no trace of the ephemeral source path. Verified this case is load-bearing by temporarily inverting the probe/emit paths locally (probe the destination, emit the source) and confirming it fails against this exact committed test -- the destination doesn't exist yet for a fresh install, so noNODE_PATHis emitted at all -- then reverting and confirming it passes again.origin/mainatbc47942):pnpm install --frozen-lockfileandpnpm -r buildboth succeed.cd packages/cli && pnpm exec vitest run(direct invocation --pnpm testcannot spawn Vitest on Windows before the separate Windows-portability PR lands) shows 177 passed, 18 skipped, 5 failed -- the same 5 pre-existing baseline failures tracked separately, no others.What could not be executed here
The remaining
index.spec.tsassertions this PR touches (the real systemd-write test and the macOS dry-run/install test) are insideposixHostIt-guarded tests, skipped on Windows because they assert exact POSIX permission-bit values (statSync(...).mode) that NTFS does not track -- unrelated to this change. To verify the derivation logic itself on Windows without that unrelated skip, I exercised the same two code paths (real systemd write, real darwin dry-run render) against throwaway scratch copies of those tests using plainit, confirmed each behaves as expected, then discarded the scratch files -- they are not part of this PR, and those two specific assertions were not executed by CI in this session. A Linux or macOS run is needed to confirm them directly. (The ephemeral-runtime case above, and the win32 case, are not in this category -- both run as plain tests and were verified directly, no scratch copy involved.)No real end-to-end service run was performed. I did not remove a source runtime, let a service start against the durable copy, and confirm it actually resolves
b4ain practice. Everything above verifies the derivation logic (the right path is computed and emitted); it does not verify the running service. That end-to-end check is a manual one a reviewer would need to perform.The launchd and systemd paths are otherwise unverified on macOS/Linux beyond the above.
NODE_PATHis consulted by CommonJSrequire()resolution only, not ESMimport-- this repairs the present CommonJS-based failure and any layout that fails the same way, not a general linker-independent guarantee.Notes for review
serviceLocation's durability currently depends onisEphemeralRuntimecorrectly classifying the invoking runtime (a separate, already-open PR extends that classifier to pnpm'sdlxcache). Neither fix subsumes the other: that PR decides whetherserviceLocationresolves to~/.codor/runtimeor a transient cache; this PR is what makesNODE_PATHfollowserviceLocationeither way.Touches
windows-setup-installs-private-task-service(setup.ts:556) andsetup-preserves-private-platform-service(setup.ts:636), bothactive. No.harn/assumptions/files were edited directly.