fix: stop forwarding unvalidated ni output to the shell in the CLI - #3422
fix: stop forwarding unvalidated ni output to the shell in the CLI#3422vlaux wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe CLI replaces manual package-manager probing with ChangesPackage Manager Resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Start.run
participant resolvePackageManager
participant antfu_ni as "`@antfu/ni`"
participant FastStore
participant Next.js
Start.run->>resolvePackageManager: resolve command and argv
resolvePackageManager->>antfu_ni: detect agent and command availability
antfu_ni-->>resolvePackageManager: agent and Volta-prefixed command
resolvePackageManager-->>Start.run: command and argv
Start.run->>FastStore: run faststore build with command
Start.run->>Next.js: spawn runner args plus next start arguments
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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. |
31a73e2 to
4f7722c
Compare
vlaux
changed the title
@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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/cli/src/utils/commands.test.ts (1)
32-33: 📐 Maintainability & Code Quality | 🔵 TrivialTests hit the real filesystem instead of mocking it.
fs.mkdtempSync/fs.writeFileSync/fs.rmSyncperform real disk I/O for every test case. As per path instructions,packages/cli/**/*.test.ts: "Command tests must mock filesystem access and external processes." Consider mockingnode:fs'sexistsSync(already imported directly incommands.ts) instead of creating/removing real temp directories.Also applies to: 43-43, 121-123, 135-136
🤖 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/utils/commands.test.ts` around lines 32 - 33, Update the command tests around the shared cwd setup and file operations to mock node:fs rather than calling fs.mkdtempSync, fs.writeFileSync, or fs.rmSync against the real filesystem. Mock the existsSync dependency used by commands.ts and configure its return value per test, preserving the existing command assertions and cleanup behavior without creating temporary directories or files.Source: Path instructions
🤖 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/commands/start.ts`:
- Around line 30-37: Check the result returned by spawnSync in the build branch
of the start command and stop or propagate the failure when faststore build does
not succeed. Ensure next start is only spawned after a successful build, while
preserving the existing behavior when .next already exists.
In `@packages/cli/src/utils/commands.test.ts`:
- Around line 155-159: Update the mocked detect result in the test “throws
instead of forwarding an unknown agent to a shell” to use a package-manager
string not recognized by `@antfu/ni`, ensuring resolvePackageManager(cwd) reaches
the UnknownAgentError path rather than substituteAgent.
In `@packages/cli/src/utils/dependencies.ts`:
- Around line 15-16: Pass the target directory to resolvePackageManager at both
call sites: use the existing cwd parameter in
packages/cli/src/utils/dependencies.ts (lines 15-16), and use getRoot() in
packages/cli/src/commands/start.ts (lines 30-31), so package-manager detection
evaluates the directory each command operates on.
---
Nitpick comments:
In `@packages/cli/src/utils/commands.test.ts`:
- Around line 32-33: Update the command tests around the shared cwd setup and
file operations to mock node:fs rather than calling fs.mkdtempSync,
fs.writeFileSync, or fs.rmSync against the real filesystem. Mock the existsSync
dependency used by commands.ts and configure its return value per test,
preserving the existing command assertions and cleanup behavior without creating
temporary directories or files.
🪄 Autofix (Beta)
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 Plus
Run ID: b4d6dddf-08b3-406d-ac96-d20797ff66cd
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by nonespecs/package-manager-resolution.mdis excluded by none and included by none
📒 Files selected for processing (5)
packages/cli/package.jsonpackages/cli/src/commands/start.tspackages/cli/src/utils/commands.test.tspackages/cli/src/utils/commands.tspackages/cli/src/utils/dependencies.ts
getPreferredPackageManager() returned the raw stdout of `na ?` with no
validation, and every caller interpolates that value into a shell command.
`ni` ran in non-programmatic mode, where an agent that is not on PATH makes
it render a confirm prompt to stdout instead of failing. Both the
terminalLink non-TTY fallback and the "(y/N)" option contain "(", so the
command became unparseable:
sh -c "Would you like to globally install pnpm (https://…)? > (y/N) run build"
-> /bin/sh: syntax error: unexpected "(" -> exit 2
The diagnostic that would have explained it goes to stderr, which spawnSync
discards.
Resolution now uses the ni library API with programmatic: true, so the
prompt path is unreachable, and the agent is validated against ni's known
agents before it can reach a shell. When the agent is not installed, the CLI
logs what it detected and substitutes an available one instead of failing,
so every build that works today keeps working.
The return value is split into { agent, command, argv }, which also fixes
two latent bugs: a `volta run` prefix broke start.ts, where the value was
passed as argv[0] of a spawn without a shell, and dependencies.ts, which
compared it with === 'npm'. Both now have regression tests.
getPackageRootDir and getDepPackageJSON existed only to locate the `na`
binary and nothing else imports them, so they are removed along with the now
unused resolve-pkg dependency.
Ref: FAS-1199
4f7722c to
84eb880
Compare
|
Added regression tests for the two latent bugs this PR fixes, which had no coverage before:
Coverage of the changed files is now 100% ( The other Sonar condition, |
Review feedback on #3422. resolvePackageManager() takes a cwd, but both call sites were calling it with no argument, so detection ran against process.cwd() instead of the directory the command operates on. That is wrong whenever the CLI is invoked against another path, which `faststore start <path>` supports. Pass the store root in start.ts and the install directory in dependencies.ts. `faststore start` also ignored the exit status of the build it triggers, so a failed build was followed by `next start` against a missing .next. It now stops with the reason instead. The unknown-agent test used "deno", which is not in ni 0.21.12's agent list but would become one if the dependency were bumped, silently turning the assertion into a different code path. It now uses a string that cannot ever be a real agent. Ref: FAS-1199
renatomaurovtex
left a comment
renatomaurovtex
left a comment
There was a problem hiding this comment.
Reviewed the full diff (cli source, tests, spec doc; skimmed the lockfile churn). Impressive root-cause work — the diagnosis of ni's interactive fallback leaking a confirm prompt into a shell command is exactly right, and moving to the library API with programmatic: true is the only correct fix (the na binary indeed doesn't expose it). I verified the claims that matter:
@antfu/ni@0.21.12(catalog) exports everything used here:detect({ programmatic, cwd }),agents,LOCKS,cmdExists,getCommand,getVoltaPrefix,Agent.getPackageRootDir/getDepPackageJSONhave no other consumers, andresolve-pkgis only otherwise used by@faststore/diagnostics, which declares its own copy — so the manifest removal is clean (and it's a dep reduction on a published package, no Dependency Discipline concern).- The remaining
getPreferredPackageManagercallers (build.ts,dev.ts,test.ts,generate-graphql.ts) all interpolate intoshell: truecommands, so keeping the wrapper returning the Volta-prefixedcommandstring preserves their semantics exactly. - The
agent/command/argvsplit does fix the two latent Volta bugs described (start.tsshell-lessspawnwith the whole string asargv[0];dependencies.ts=== 'npm'comparison). specs/already has precedent ondev(contract-switcher.md), so the spec doc placement is fine.
Findings, none blocking:
🟡 [edge case] Substituting on the install path can create the exact dual-lockfile state this PR diagnoses. installDependencies is a mutating operation: if pnpm is detected-but-missing in a store with only pnpm-lock.yaml, the substitution runs yarn add …, which writes a yarn.lock next to pnpm-lock.yaml — manufacturing the ambiguity that caused FAS-1199 (and yarn may choke on workspace: protocol deps in a pnpm project). Substitute-don't-fail is the right call for read-only commands (build, start), but consider failing hard (or refusing to substitute when a foreign lockfile is present) specifically in installDependencies. Non-blocking since the incident path is the build one, but worth deciding deliberately.
🟡 [ci] SonarQube red — confirm it's the known cli coverage-feed gate, not a regression. Same pattern as #3419: cli tests don't feed coverage to Sonar, so the new-code gate fails despite this PR adding 15+ test cases. If that's what the Sonar report shows, fine to merge past it; if not, worth a look.
💬 [question] FALLBACK_AGENTS = ['yarn', 'npm'] ignores an installed pnpm/bun. If the detected agent is missing and only pnpm is on PATH, this throws NoAvailablePackageManagerError even though a usable PM exists. Deliberate (yarn/npm being the only images you support), or worth appending the remaining known agents as last resorts?
💬 [follow-up] Detection cwd for the remaining callers. start.ts now correctly resolves against basePath, but build/dev/test/generate-graphql still detect at process.cwd() through the wrapper. Pre-existing behavior, but now that resolvePackageManager(cwd) exists, threading basePath through them would make faststore build <path> consistent. Same for start.ts's build fallback: spawnSync(${command} faststore build) doesn't forward args.path or set cwd, so serving a non-cwd store still builds the wrong directory — also pre-existing, fits the same follow-up.
The new build-failure guard in start.ts (throw instead of serving a stale/absent build) is a good behavior fix, and the test suites are genuinely strong — the not-a-package-manager as never future-proofing on the UnknownAgentError path is a nice touch.
Verdict: Approved with comments
Blocking (🔴/🟠):
- None.
Non-blocking (🟡/💬):
- Substitution on the mutating
installDependenciespath can create the dual-lockfile ambiguity — consider fail-hard there. - Confirm SonarQube red is the known cli coverage-feed gate (#3419 pattern).
FALLBACK_AGENTSomits pnpm/bun; detection cwd follow-up for the remaining callers.
Checks to confirm before merge: pnpm lint · build · pnpm turbo run test --filter=@faststore/cli · SonarQube triage as above.
Review feedback on #3422. installDependencies is the one call site that mutates the project: substituting e.g. yarn into a pnpm store would run `yarn add`, writing a yarn.lock next to pnpm-lock.yaml — manufacturing exactly the dual-lockfile ambiguity behind FAS-1199 (and yarn may not understand workspace: ranges in a pnpm project). resolvePackageManager() now takes { substitute }, and the install path resolves with substitute: false, throwing an actionable error instead of writing a conflicting lockfile. The missing-dependencies loop in generate.ts was forEach(async ...), so that throw would surface as an unhandled rejection instead of going through oclif's error handling. It is now a for...of, with the spinner stopped in a finally. FALLBACK_AGENTS gains pnpm and bun as last resorts: the branch where yarn and npm are both missing previously threw even when a usable package manager was installed. Substitution order is unchanged for every case that resolved before. Ref: FAS-1199 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/cli/src/utils/commands.ts (1)
77-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the avoidable tuple assertion.
Construct
argvas a non-empty tuple instead of asserting thatsplit()produced one.Proposed refactor
- const voltaPrefix = getVoltaPrefix() - const command = voltaPrefix ? `${voltaPrefix} ${binOf(agent)}` : binOf(agent) + const argv: [string, ...string[]] = [binOf(agent)] + const voltaPrefix = getVoltaPrefix() - return { agent, command, argv: command.split(' ') as [string, ...string[]] } + if (voltaPrefix) { + argv.unshift(...voltaPrefix.split(' ')) + } + + return { agent, command: argv.join(' '), argv }As per coding guidelines, TypeScript files must “avoid type assertions when possible.”
🤖 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/utils/commands.ts` around lines 77 - 80, Update the command construction near binOf(agent) so argv is created as a guaranteed non-empty tuple without relying on a TypeScript type assertion on command.split(' '). Preserve the existing command value and argument ordering while using tuple construction or another assertion-free approach.Source: Coding guidelines
packages/cli/src/commands/start.test.ts (1)
49-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the build step is skipped when
.nextalready exists.This test only checks the
spawncall; addingexpect(spawnSyncMock).not.toHaveBeenCalled()would close the loop and make the "no build" branch explicit.✅ Suggested addition
expect(spawnMock).toHaveBeenCalledWith( 'yarn', ['next', 'start', path.join(storeDir, '.faststore'), '-p', '3000'], { stdio: 'inherit' } ) + expect(spawnSyncMock).not.toHaveBeenCalled()🤖 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/commands/start.test.ts` around lines 49 - 64, Update the test case “spawns the package manager binary without a shell” to also assert that spawnSyncMock is not called, explicitly verifying the build step is skipped when the existing .next directory is present. Keep the current spawn and package-manager assertions unchanged.
🤖 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/commands/start.test.ts`:
- Around line 38-47: Update the beforeEach and afterEach hooks in the start
command tests to mock filesystem operations instead of calling fs.mkdtempSync,
fs.mkdirSync, and fs.rmSync against the real filesystem. Reuse the shared
filesystem-mocking approach established for commands.test.ts while preserving
the temporary storeDir and .next setup behavior.
In `@packages/cli/src/utils/commands.test.ts`:
- Around line 31-44: Update the test setup and teardown around beforeEach and
afterEach to mock node:fs filesystem operations instead of calling mkdtempSync
and rmSync on the real disk. Ensure the mocked temporary-directory behavior
still provides each test with an isolated cwd and preserves the existing
command-test behavior, including any writeFileSync usage in this suite.
---
Nitpick comments:
In `@packages/cli/src/commands/start.test.ts`:
- Around line 49-64: Update the test case “spawns the package manager binary
without a shell” to also assert that spawnSyncMock is not called, explicitly
verifying the build step is skipped when the existing .next directory is
present. Keep the current spawn and package-manager assertions unchanged.
In `@packages/cli/src/utils/commands.ts`:
- Around line 77-80: Update the command construction near binOf(agent) so argv
is created as a guaranteed non-empty tuple without relying on a TypeScript type
assertion on command.split(' '). Preserve the existing command value and
argument ordering while using tuple construction or another assertion-free
approach.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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 Plus
Run ID: a9d68b87-1027-4938-bb87-2e8975c6560b
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by nonespecs/package-manager-resolution.mdis excluded by none and included by none
📒 Files selected for processing (8)
packages/cli/package.jsonpackages/cli/src/commands/start.test.tspackages/cli/src/commands/start.tspackages/cli/src/utils/commands.test.tspackages/cli/src/utils/commands.tspackages/cli/src/utils/dependencies.test.tspackages/cli/src/utils/dependencies.tspackages/cli/src/utils/generate.ts
Review feedback on #3422 (CodeRabbit). Constructing argv as a non-empty tuple and deriving command from it removes the type assertion on command.split(' '). Same values, one source of truth for both forms. Also asserts in start.test.ts that the build step is skipped when .next already exists, making the no-build branch explicit. Ref: FAS-1199 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
2 participants
What's the purpose of this pull request?
A store build failed with
/bin/sh: syntax error: unexpected "("and exit code 2, with nothing in the log tying the error to its cause.getPreferredPackageManager()returns the raw stdout ofna ?with no validation, and every caller interpolates that value into a shell command.niruns in non-programmatic mode, where an agent that is not onPATHdoes not make it fail: it renders a confirm prompt to stdout. TheterminalLinknon-TTY fallback and the(y/N)option both contain(, so the command becomes unparseable:The diagnostic that explains it,
[ni] Detected pnpm but it doesn't seem to be installed., goes to stderr, whichspawnSyncdiscards.The store had both
pnpm-lock.yamlandyarn.lockcommitted. The build image installs fromyarn.lockfirst, so pnpm was never present, whilenipreferspnpm-lock.yaml. Two opposite precedence rules over the same repo.How it works?
Resolution moves to the
nilibrary API withprogrammatic: true, which is the only way to disable the interactive fallback (thenabinary does not expose it). The agent is then validated againstni's knownagentsbefore it can reach a shell.When the resolved agent is not on
PATH, the CLI reports it and substitutes an available one instead of failing:Substituting rather than failing keeps regression risk at zero:
cmdExistsiswhich.sync, which does not always agree with what a shell would resolve. In the incident, yarn is also the correct choice, since yarn is what installednode_modules.The return value is split into
{ agent, command, argv }, so callers stop pattern-matching a command string:agent'yarn'— for comparisonscommand'volta run yarn'— forshell: trueargv['volta', 'run', 'yarn']— forspawnwithout a shellThat fixes two latent bugs a Volta prefix would trigger today:
start.tspassed the whole string asargv[0]of aspawnwithout a shell, anddependencies.tscompared it with=== 'npm'.getPackageRootDirandgetDepPackageJSONexisted only to locate thenabinary and nothing else inpackages/imports them, so they are removed. That leavesresolve-pkgunused here (@faststore/diagnosticsdeclares its own), so it is dropped from the manifest.How to test it?
cd packages/cli && pnpm test— 12 new cases insrc/utils/commands.test.tscover detection, the Volta prefix, substitution with and without multiple lockfiles, thenulldetection default, and both error paths.pnpm-lock.yamlandyarn.lock, withpnpmnot onPATH, runfaststore build. Before this change it exits 2 with a shell syntax error; now it warns and builds with yarn.References
specs/package-manager-resolution.md, added here3.xfollows, widening the existingagent === ''guard. It is the only thing that reaches stores on that line, which is where the incident happened.Checklist
PR Title and Commit Messages
Dependencies
pnpm-lock.yamlfile when there were changes to the packagesDocumentation
🤖 Generated with Claude Code
Summary by CodeRabbit