fix(cli): canonicalize Windows PATH env key so local builds can find bun#115
fix(cli): canonicalize Windows PATH env key so local builds can find bun#115ankur-arch wants to merge 3 commits into
Conversation
On Windows the PATH variable is typically keyed as `Path` (its registry casing). The compute SDK spreads process.env into a plain, case-sensitive object when it prepends node_modules/.bin directories for a local build, so `baseEnv.PATH` reads undefined: the rebuilt PATH drops every inherited entry and is written as a second, truncated `PATH` key next to `Path`. In the spawned shell's case-insensitive environment block the truncated key wins, and `bun run build` fails with "'bun' is not recognized as an internal or external command" even though Bun is installed and on Path. Collapse case-variants of PATH into the canonical `PATH` key at CLI startup, before any command delegates to the SDK. Windows resolves environment variables case-insensitively, so the rename is invisible to this process and its children. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary by CodeRabbit
WalkthroughThis PR adds 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Address review feedback on the Windows PATH canonicalization fix: - Give the multi-variant case a fixed precedence (existing `PATH`, then the Windows-native `Path`, then any remaining spelling) so the result no longer depends on Object.keys insertion order. - Trim the helper's top-level comment to the "why"; the detailed spread failure mode now lives next to the test that reproduces it. - Add an integration test that drives the real end-to-end path — process.env spread into a plain object, PATH rebuilt with node_modules/.bin prepended (the compute SDK's shape), then a spawned shell child that must still resolve a binary on the inherited path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed a follow-up addressing the review (commit deterministic PATH precedence + integration coverage). Merge-readiness — global Deterministic precedence. When several spellings coexist and there's no canonical Comment placement. Trimmed the helper's top-level block to the "why" and moved the detailed spread failure-mode explanation next to the test that reproduces it. Integration test. Added Full suite now 617 tests passing. The two lint errors in |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/shell/path-env.ts (1)
17-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
selectPreferredVariant's "Path" branch is effectively dead code.
selectPreferredVariantis only reached when bothenv.PATHandenv.Pathareundefined(line 36). Butvariantsis derived fromObject.keys(env), so if a literal"Path"key were amongvariants,env.Pathwould already be defined and the??chain would short-circuit beforeselectPreferredVariantis called. That meansvariants.includes("Path")inside the helper (line 49) can never betruein practice — it only ever returnsvariants[0].A consequence: for the truly order-dependent case — several non-canonical spellings that are not exactly
"PATH"or"Path"(e.g."PaTH","pATh") — the result does depend onObject.keysinsertion order, since it falls through tovariants[0]. This contradicts the intent described in the comment on lines 32-35 ("the result never depends on key insertion order").The test added for this ("prefers the Windows-native Path over other variants, independent of insertion order") doesn't actually exercise this fallback path either — it passes because
env.Pathis read directly, not because ofselectPreferredVariant's tie-break logic.Consider simplifying
selectPreferredVariantto justvariants[0](removing the misleading dead check), or, if genuine multi-variant, non-Path-cased inputs are a real concern, add explicit precedence there.♻️ Simplification
-/** The Windows-native `Path` if present, otherwise the first variant. */ -function selectPreferredVariant(variants: string[]): string { - return variants.includes("Path") ? "Path" : variants[0]; -} +/** Arbitrary but deterministic pick when neither `PATH` nor `Path` is present. */ +function selectPreferredVariant(variants: string[]): string { + return variants[0]; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/shell/path-env.ts` around lines 17 - 50, The dead `"Path"` check in selectPreferredVariant should be removed or replaced with logic that actually handles the fallback case reached by canonicalizeWindowsPathKey. Simplify the helper to match the real behavior (it only receives non-canonical variants when env.PATH and env.Path are both absent), and if order-independent precedence is still required for mixed-case keys like PaTH/pATh, make that selection explicit rather than relying on variants[0]. Update the tests around canonicalizeWindowsPathKey/selectPreferredVariant so they cover the true fallback path instead of only the env.Path shortcut.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/cli/src/shell/path-env.ts`:
- Around line 17-50: The dead `"Path"` check in selectPreferredVariant should be
removed or replaced with logic that actually handles the fallback case reached
by canonicalizeWindowsPathKey. Simplify the helper to match the real behavior
(it only receives non-canonical variants when env.PATH and env.Path are both
absent), and if order-independent precedence is still required for mixed-case
keys like PaTH/pATh, make that selection explicit rather than relying on
variants[0]. Update the tests around
canonicalizeWindowsPathKey/selectPreferredVariant so they cover the true
fallback path instead of only the env.Path shortcut.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 71ad0396-f1e8-4906-b48d-7610751308c6
📒 Files selected for processing (3)
packages/cli/src/shell/path-env.tspackages/cli/tests/path-env.integration.test.tspackages/cli/tests/path-env.test.ts
Apply review feedback from Kristof and CodeRabbit: - Remove `selectPreferredVariant`: its `"Path"` branch was dead code, since `env.PATH` already resolves `Path` case-insensitively on the real process.env and short-circuits before the helper runs. Replace with a sort over the remaining variants so the fallback is genuinely independent of key insertion order (the earlier claim relied on the dead branch). - Rework the precedence test to exercise that sorted fallback with exotic spellings (`PaTH`), the path CodeRabbit noted the old test never hit. - Strip comments across the change down to the non-trivial "why". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
88e6c30
|
Applied Kristof's and CodeRabbit's feedback in the latest commit:
Full suite still 617 passing; the SDK sandbox repro still confirms bug-without-fix / resolved-with-fix. |
|
Closing in favor of prisma/project-compute#105, which fixes this at the correct layer. The root cause lives in the compute SDK's This PR canonicalized Thanks @kristof-siket for the review, and @AmanVarshney01 for calling out the right layer. Reproduction details and a review left on #105. |
Bumps `@prisma/compute-sdk` 0.33.0 -> 0.34.0 to pick up the Windows build fix ([project-compute#105](prisma/project-compute#105), published as 0.34.0). ## What this fixes On Windows, `prisma-cli app build` and `app deploy` failed immediately with `'bun' is not recognized as an internal or external command` even with Bun installed. The SDK's `buildCommandEnv` wrote a literal `PATH` key over Windows' `Path`-cased variable, forking a duplicate key whose bin-dirs-only value shadowed the real system PATH in the spawned build shell. 0.34.0 resolves the key case-insensitively and extends it in place. Supersedes #115, which patched the same bug CLI-side; the root cause is SDK-owned. ## Notes - The lockfile was still resolving compute-sdk 0.31.0 despite the 0.33.0 pin in `packages/cli/package.json`; this refresh brings the installed version in line with the manifest. - Verified the published 0.34.0 tarball contains the fix, and the full CLI suite (45 files, 624 tests) plus build pass against it. - Windows follow-ups tracked SDK-side: execa migration for the `.cmd` launcher ladder and a `windows-latest` CI leg.
Fixes the Windows-only failure reported in Discord:
prisma-cli app build/app deployon a freshly scaffolded Prisma Compute + Next.js project fails immediately with:…even though Bun is installed, on
Path, andbun run buildsucceeds when run manually from the same directory. The same project works via WSL.How the issue was validated
The reporter's trace points at
@prisma/compute-sdk/dist/workspace.js, which is compiled and outside this repo, so the failure was reproduced in a sandboxed mini Windows environment built around the real installed SDK code:Path(registry casing); a child's environment block is case-insensitive, so keys differing only in case collide; andcmd.exeresolves bare command names by scanning the effectivePath+PATHEXT. The sandbox mirrors the reporter's machine: Bun under%USERPROFILE%\.bun\binon the systemPath, and a scaffolded project with"packageManager": "bun@1.3.14"and"build": "next build".buildCommandEnv()with that environment shows it emits an env containing both the originalPath(full) and a newPATHholding onlynode_modules/.bindirectories. In the child's case-insensitive block the truncatedPATHclobbersPath, andcmd.execan no longer resolvebun— reproducing the exact reported error. The control case (the user's own shell) resolvesbunfine, matching the report that manual builds work.runBuildCommand()(the failing frame,shell: true) in a process whose environment keys the path variable asPathmakes the spawned shell fail withbun: command not found(exit 127 — the POSIX twin of the Windows error). Re-running the identical build after applying this PR'scanonicalizeWindowsPathKey()succeeds.What the issue was
The scaffolded project sets
"packageManager": "bun@1.3.14", so the SDK resolves the local build command to the stringbun run buildand runs it through a shell. Before spawning, it constructs the child environment like this (workspace.js):On Windows the real variable is keyed
Path, andprocess.envhides that behind case-insensitive lookups. Spreadingprocess.envinto a plain object removes that magic:baseEnv.PATHreadsundefined(plain objects are case-sensitive), so the rebuiltPATHsilently drops every inherited entry — including~\.bun\bin;PATHforks a second key alongside the untouchedPath;PATHclobbers the realPath, andcmd.execannot resolvebun.This explains every observation in the report: manual
bun run buildworks (unmodified environment), WSL works (Linux keys the variable literallyPATH), every launcher (bunx/npx/nodedirectly) fails identically (the bug is downstream in the build env construction), and--build-type bunfails differently (different code path).How it was fixed
@prisma/compute-sdkis an external dependency, so the fix lands in the CLI, which owns the process the SDK inherits its environment from.runCli()now callscanonicalizeWindowsPathKey(process.env)(newpackages/cli/src/shell/path-env.ts) before dispatching any command: on Windows it collapses all case-variants of the path variable into the single canonicalPATHkey. After that, the SDK's spread-and-rebuild seesbaseEnv.PATH, keeps the full inherited path withnode_modules/.binprepended, and produces no case-colliding duplicate keys.Windows resolves environment variables case-insensitively, so renaming
Path→PATHis invisible to the CLI process and everything it spawns; on macOS/Linux the function is a no-op.Testing: new unit tests in
packages/cli/tests/path-env.test.tscover the registry-cased rename, the spread/rewrite failure mode from the field, multi-variant collapse, and the non-Windows no-op. Full suite: 46 files / 615 tests pass; lint and build clean.🤖 Generated with Claude Code