fix: resolve node_modules bins when running scripts inside .faststore - #3440
Merged
Conversation
The generated .faststore package has no node_modules of its own, so predev ('na run partytown'), dev-only and build ('next') rely on binaries installed in the store root or, on hoisted monorepos, the workspace root. Neither the execSync of predev nor the spawns of dev-only and build added those directories to PATH, which fails on native Windows with 'na is not recognized as an internal command'.
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe CLI now adds ancestor ChangesCLI environment propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This change updates script execution so project binaries are found from ancestor node_modules directories while preserving environment behavior, with focused tests covering the path handling. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant BuildDevTestCommands
participant withNodeModulesBins
participant runCommandSync
participant execSync
BuildDevTestCommands->>withNodeModulesBins: Build environment from tmpDir
withNodeModulesBins-->>BuildDevTestCommands: Return environment with ancestor bin paths
BuildDevTestCommands->>runCommandSync: Pass augmented env
runCommandSync->>execSync: Execute command with env
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
@faststore/api
@faststore/cli
@faststore/components
@faststore/core
@faststore/diagnostics
@faststore/lighthouse
@faststore/sdk
@faststore/ui
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@packages/cli/src/utils/binPaths.test.ts`:
- Around line 80-87: Add a test for the binDirs.length === 0 branch of
withNodeModulesBins by creating a directory tree with no node_modules/.bin
directories, then assert the returned environment is identical to the input
environment.
In `@packages/cli/src/utils/binPaths.ts`:
- Around line 56-62: Update the PATH assembly in the bin-path utility around
missingBinDirs so all discovered binDirs are ordered first while removing their
existing entries from currentEntries, preserving nearest-bin precedence when
storeBinDir is already present. Keep unrelated PATH entries in their existing
order, and update the corresponding binPaths tests to expect the store bin
before the workspace bin.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3afeb663-25ef-45a0-a30b-56b456e7e79b
📒 Files selected for processing (5)
packages/cli/src/commands/build.tspackages/cli/src/commands/dev.tspackages/cli/src/utils/binPaths.test.tspackages/cli/src/utils/binPaths.tspackages/cli/src/utils/runCommandSync.ts
hellofanny
marked this pull request as ready for review
August 12, 2026 20:23
hellofanny
requested review from
gabpaladino and
lariciamota
and removed request for
a team
August 12, 2026 20:23
Only prepending the bin directories that were missing from PATH let a hoisted workspace-root binary jump ahead of the store-level one whenever the package manager had already put the store's bin on PATH, which is the common case. Reorder instead, so the nearest ancestor always wins. Cover the resolution end to end by spawning a child process against a PATH that cannot find the fixture on its own, so the assertion can only pass because of the directories the helper adds. Wire the helper into `faststore test` as well: it runs `test:e2e` inside `.faststore` and hits the same unresolved-binary gap as dev and build. Co-authored-by: Cursor <cursoragent@cursor.com>
Spawning the probe with `shell: true` raises a security hotspot on new code in the Sonar analysis. Dropping it keeps the assertion meaningful: the OS still resolves the bare name from the environment the helper builds, which is the behaviour under test, and the probe is a fixture we write ourselves, not user input. Assert on the exit status too, since without a shell an unresolved binary fails with ENOENT and a null stdout, which would otherwise surface as a confusing TypeError instead of a failed expectation. Co-authored-by: Cursor <cursoragent@cursor.com>
eduardoformiga
approved these changes
Aug 13, 2026
eduardoformiga
left a comment
eduardoformiga
left a comment
Member
There was a problem hiding this comment.
Conversei com a IA e parece OK, tentei validar a parte de segurança também.
SonarQube:
- No Sonar, marcar o hotspot do
chmodSync(..., 0o755)do teste como Safe. É um arquivo que o próprio teste cria. Sem isso o gate continua vermelho (0% hotspots reviewed). - Validar no Windows nativo, numa loja monorepo, com o preview do PR:
faststore devpassa dopredeve chega no Ready;faststore buildpassa doprebuild. Os unit tests não cobrem esse sintoma.
O resto do Sonar (String.raw e cobertura 32%) não bloqueia, mas seria bom cobrir.
CodeRabbit já foi endereçado.
The probe only needs to be executable by the user running the test, so writing it with mode 0o700 replaces the chmod to 0o755 that Sonar raised as a security hotspot. Using String.raw for the Windows PATH fixture clears the two remaining new issues on the same file. Co-authored-by: Cursor <cursoragent@cursor.com>
|
7 tasks
thiagopereira-vtex
pushed a commit
that referenced
this pull request
Aug 25, 2026
…#3440) ## What's the purpose of this pull request? The generated `.faststore` package has no `node_modules` of its own (`node_modules` is in the generate step's `ignorePaths`, and nothing installs dependencies in there). The scripts we inject into `.faststore/package.json` do rely on binaries, though: ``` 'dev-only': 'next dev --webpack', predev: 'na run partytown', prebuild: 'na run partytown', ``` Those binaries only exist in the `node_modules/.bin` of an ancestor directory — the store root or, on hoisted monorepos, the workspace root. We spawn all three scripts with `cwd` pointing at `.faststore` and never add those directories to `PATH`: `runCommandSync` calls `execSync` with `cwd` only, and the `spawn`/`spawnSync` calls pass `env` without touching `PATH`. On native Windows with a monorepo this fails outright: ``` yarn.cmd predev 'na' is not recognized as an internal command yarn.cmd dev-only --port 3001 'next' is not recognized as an internal command ``` This PR adds a `withNodeModulesBins` helper that walks up from `.faststore` collecting every existing `node_modules/.bin` and prepends them to `PATH`, and wires it into the four places that run a script inside `.faststore`: `predev` and `dev-only` in `dev`, `run build` in `build`, and `run test:e2e` in `test`. Note that `build` is affected for the same reason (`prebuild: 'na run partytown'`), so fixing only `dev` would leave `faststore build` broken on the same setups. `test` runs the `test:e2e` script inherited from `@faststore/core` (`cypress open`), which has the same gap. ## Implementation notes - **Nearest ancestor wins.** A dependency installed at the store level takes precedence over the workspace root one. This mirrors what npm/yarn already do when running a script. - **Reordering, not just prepending.** Package managers already put some of these directories in `PATH` when they run a script, so the helper cannot simply prepend the missing ones: doing that would let a workspace-root binary that was absent from `PATH` jump ahead of the store-level one that was already there, inverting the precedence above. It removes every discovered bin directory from its current position and reinserts the whole set nearest-first, leaving unrelated `PATH` entries in their original order. Each call rebuilds from `process.env`, so nothing accumulates across runs. - **Windows `PATH` casing.** Environment variables are case-insensitive on Windows, where the key is usually `Path`. The helper reuses whichever key already exists instead of blindly writing `PATH`, otherwise the child process would receive both keys and could keep using the old value — i.e. the fix would silently not work on the only platform that needs it. Covered by a test. - **No-op when there is nothing to add.** If no ancestor `node_modules/.bin` exists, the helper returns a copy of the environment, which is indistinguishable from today's behavior. ## Is this a breaking change? No. The main thing to be careful about is that passing `env` to `execSync`/`spawn` replaces the environment instead of extending it — the helper always starts from a spread of `process.env`, so nothing is lost. The other `runCommandSync` call sites (`cp-schema`, `generate-graphql`, `dependencies`) don't pass `env`, so they get `undefined`, which Node treats as "use `process.env`": same behavior as before, and covered by a test. The one behavioral change is that project binaries now take precedence over the system `PATH` inside these child processes, which is the same precedence a package manager applies to its own scripts. Left out on purpose: - `generate-graphql.ts` also runs `${packageManager} run generate:schema` inside `.faststore` and has the same gap, but that command is orphaned on v4 — nothing invokes it (`dev` and `build` call `generate-types`, `cache-graphql` and `generate-i18n`) and the `generate:schema` / `generate:codegen` scripts no longer exist in `@faststore/core`'s `package.json`. - `start.ts` looks similar but is **not** affected: it spawns from the store root without `cwd: tmpDir`, so the package manager resolves the binary the usual way. ## How to test it? Unit tests: `packages/cli/src/utils/binPaths.test.ts` and `packages/cli/src/utils/runCommandSync.test.ts` (`pnpm vitest run src/utils` in `packages/cli`). They build a fixture that mimics a hoisted monorepo and assert ordering, ancestor skipping, deduplication, the no-op case, the `env` passthrough and the Windows `Path` casing. One of them goes further than asserting on the returned object: it writes an executable probe into the fixture's `node_modules/.bin` and spawns it by bare name from `.faststore`, against a `PATH` that cannot resolve it on its own. The assertion can only pass because of the directories the helper adds, so it covers the resolution mechanism end to end rather than the string we build. It is skipped on Windows, where the fixture would need a `.cmd` shim. What the unit tests cannot prove is the original symptom, which is native Windows. That needs a manual run with the CodeSandbox preview of this PR, in a monorepo store: `faststore dev` should complete `predev` and reach Next's `Ready`, and `faststore build` should get past `prebuild`, with `na` and `next` resolved without a global install or a manual `PATH` change. ## References Reported by a partner alongside the FastStore v4 migration, as item 1 (P0) of their handoff document. The other two items are #3439 (argument order, merged) and #3419 (Windows glob for GraphQL typeDefs). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Build, development, and test commands now automatically locate locally installed command-line tools across nested project directories. * Command execution preserves existing environment variables and avoids duplicate path entries. * **Bug Fixes** * Improved compatibility with platform-specific PATH casing and empty or missing PATH environments. * **Tests** * Added coverage for nested installations, missing directories, path ordering, deduplication, environment preservation, and custom command environments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
thiagopereira-vtex
pushed a commit
that referenced
this pull request
Aug 25, 2026
…#3440) The generated `.faststore` package has no `node_modules` of its own (`node_modules` is in the generate step's `ignorePaths`, and nothing installs dependencies in there). The scripts we inject into `.faststore/package.json` do rely on binaries, though: ``` 'dev-only': 'next dev --webpack', predev: 'na run partytown', prebuild: 'na run partytown', ``` Those binaries only exist in the `node_modules/.bin` of an ancestor directory — the store root or, on hoisted monorepos, the workspace root. We spawn all three scripts with `cwd` pointing at `.faststore` and never add those directories to `PATH`: `runCommandSync` calls `execSync` with `cwd` only, and the `spawn`/`spawnSync` calls pass `env` without touching `PATH`. On native Windows with a monorepo this fails outright: ``` yarn.cmd predev 'na' is not recognized as an internal command yarn.cmd dev-only --port 3001 'next' is not recognized as an internal command ``` This PR adds a `withNodeModulesBins` helper that walks up from `.faststore` collecting every existing `node_modules/.bin` and prepends them to `PATH`, and wires it into the four places that run a script inside `.faststore`: `predev` and `dev-only` in `dev`, `run build` in `build`, and `run test:e2e` in `test`. Note that `build` is affected for the same reason (`prebuild: 'na run partytown'`), so fixing only `dev` would leave `faststore build` broken on the same setups. `test` runs the `test:e2e` script inherited from `@faststore/core` (`cypress open`), which has the same gap. - **Nearest ancestor wins.** A dependency installed at the store level takes precedence over the workspace root one. This mirrors what npm/yarn already do when running a script. - **Reordering, not just prepending.** Package managers already put some of these directories in `PATH` when they run a script, so the helper cannot simply prepend the missing ones: doing that would let a workspace-root binary that was absent from `PATH` jump ahead of the store-level one that was already there, inverting the precedence above. It removes every discovered bin directory from its current position and reinserts the whole set nearest-first, leaving unrelated `PATH` entries in their original order. Each call rebuilds from `process.env`, so nothing accumulates across runs. - **Windows `PATH` casing.** Environment variables are case-insensitive on Windows, where the key is usually `Path`. The helper reuses whichever key already exists instead of blindly writing `PATH`, otherwise the child process would receive both keys and could keep using the old value — i.e. the fix would silently not work on the only platform that needs it. Covered by a test. - **No-op when there is nothing to add.** If no ancestor `node_modules/.bin` exists, the helper returns a copy of the environment, which is indistinguishable from today's behavior. No. The main thing to be careful about is that passing `env` to `execSync`/`spawn` replaces the environment instead of extending it — the helper always starts from a spread of `process.env`, so nothing is lost. The other `runCommandSync` call sites (`cp-schema`, `generate-graphql`, `dependencies`) don't pass `env`, so they get `undefined`, which Node treats as "use `process.env`": same behavior as before, and covered by a test. The one behavioral change is that project binaries now take precedence over the system `PATH` inside these child processes, which is the same precedence a package manager applies to its own scripts. Left out on purpose: - `generate-graphql.ts` also runs `${packageManager} run generate:schema` inside `.faststore` and has the same gap, but that command is orphaned on v4 — nothing invokes it (`dev` and `build` call `generate-types`, `cache-graphql` and `generate-i18n`) and the `generate:schema` / `generate:codegen` scripts no longer exist in `@faststore/core`'s `package.json`. - `start.ts` looks similar but is **not** affected: it spawns from the store root without `cwd: tmpDir`, so the package manager resolves the binary the usual way. Unit tests: `packages/cli/src/utils/binPaths.test.ts` and `packages/cli/src/utils/runCommandSync.test.ts` (`pnpm vitest run src/utils` in `packages/cli`). They build a fixture that mimics a hoisted monorepo and assert ordering, ancestor skipping, deduplication, the no-op case, the `env` passthrough and the Windows `Path` casing. One of them goes further than asserting on the returned object: it writes an executable probe into the fixture's `node_modules/.bin` and spawns it by bare name from `.faststore`, against a `PATH` that cannot resolve it on its own. The assertion can only pass because of the directories the helper adds, so it covers the resolution mechanism end to end rather than the string we build. It is skipped on Windows, where the fixture would need a `.cmd` shim. What the unit tests cannot prove is the original symptom, which is native Windows. That needs a manual run with the CodeSandbox preview of this PR, in a monorepo store: `faststore dev` should complete `predev` and reach Next's `Ready`, and `faststore build` should get past `prebuild`, with `na` and `next` resolved without a global install or a manual `PATH` change. Reported by a partner alongside the FastStore v4 migration, as item 1 (P0) of their handoff document. The other two items are #3439 (argument order, merged) and #3419 (Windows glob for GraphQL typeDefs). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **New Features** * Build, development, and test commands now automatically locate locally installed command-line tools across nested project directories. * Command execution preserves existing environment variables and avoids duplicate path entries. * **Bug Fixes** * Improved compatibility with platform-specific PATH casing and empty or missing PATH environments. * **Tests** * Added coverage for nested installations, missing directories, path ordering, deduplication, environment preservation, and custom command environments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
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
2 participants
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.
What's the purpose of this pull request?
The generated
.faststorepackage has nonode_modulesof its own (node_modulesis in the generate step'signorePaths, and nothing installs dependencies in there). The scripts we inject into.faststore/package.jsondo rely on binaries, though:Those binaries only exist in the
node_modules/.binof an ancestor directory — the store root or, on hoisted monorepos, the workspace root. We spawn all three scripts withcwdpointing at.faststoreand never add those directories toPATH:runCommandSynccallsexecSyncwithcwdonly, and thespawn/spawnSynccalls passenvwithout touchingPATH.On native Windows with a monorepo this fails outright:
This PR adds a
withNodeModulesBinshelper that walks up from.faststorecollecting every existingnode_modules/.binand prepends them toPATH, and wires it into the four places that run a script inside.faststore:predevanddev-onlyindev,run buildinbuild, andrun test:e2eintest.Note that
buildis affected for the same reason (prebuild: 'na run partytown'), so fixing onlydevwould leavefaststore buildbroken on the same setups.testruns thetest:e2escript inherited from@faststore/core(cypress open), which has the same gap.Implementation notes
PATHwhen they run a script, so the helper cannot simply prepend the missing ones: doing that would let a workspace-root binary that was absent fromPATHjump ahead of the store-level one that was already there, inverting the precedence above. It removes every discovered bin directory from its current position and reinserts the whole set nearest-first, leaving unrelatedPATHentries in their original order. Each call rebuilds fromprocess.env, so nothing accumulates across runs.PATHcasing. Environment variables are case-insensitive on Windows, where the key is usuallyPath. The helper reuses whichever key already exists instead of blindly writingPATH, otherwise the child process would receive both keys and could keep using the old value — i.e. the fix would silently not work on the only platform that needs it. Covered by a test.node_modules/.binexists, the helper returns a copy of the environment, which is indistinguishable from today's behavior.Is this a breaking change?
No. The main thing to be careful about is that passing
envtoexecSync/spawnreplaces the environment instead of extending it — the helper always starts from a spread ofprocess.env, so nothing is lost. The otherrunCommandSynccall sites (cp-schema,generate-graphql,dependencies) don't passenv, so they getundefined, which Node treats as "useprocess.env": same behavior as before, and covered by a test.The one behavioral change is that project binaries now take precedence over the system
PATHinside these child processes, which is the same precedence a package manager applies to its own scripts.Left out on purpose:
generate-graphql.tsalso runs${packageManager} run generate:schemainside.faststoreand has the same gap, but that command is orphaned on v4 — nothing invokes it (devandbuildcallgenerate-types,cache-graphqlandgenerate-i18n) and thegenerate:schema/generate:codegenscripts no longer exist in@faststore/core'spackage.json.start.tslooks similar but is not affected: it spawns from the store root withoutcwd: tmpDir, so the package manager resolves the binary the usual way.How to test it?
Unit tests:
packages/cli/src/utils/binPaths.test.tsandpackages/cli/src/utils/runCommandSync.test.ts(pnpm vitest run src/utilsinpackages/cli). They build a fixture that mimics a hoisted monorepo and assert ordering, ancestor skipping, deduplication, the no-op case, theenvpassthrough and the WindowsPathcasing.One of them goes further than asserting on the returned object: it writes an executable probe into the fixture's
node_modules/.binand spawns it by bare name from.faststore, against aPATHthat cannot resolve it on its own. The assertion can only pass because of the directories the helper adds, so it covers the resolution mechanism end to end rather than the string we build. It is skipped on Windows, where the fixture would need a.cmdshim.What the unit tests cannot prove is the original symptom, which is native Windows. That needs a manual run with the CodeSandbox preview of this PR, in a monorepo store:
faststore devshould completepredevand reach Next'sReady, andfaststore buildshould get pastprebuild, withnaandnextresolved without a global install or a manualPATHchange.References
Reported by a partner alongside the FastStore v4 migration, as item 1 (P0) of their handoff document. The other two items are #3439 (argument order, merged) and #3419 (Windows glob for GraphQL typeDefs).
Summary by CodeRabbit