[alex-powell]

Fixes #1385

Problem

Under --fix, package manager commands were built as shell-shaped strings and then re-split on && and whitespace before spawn. A workspace directory whose path contains && (for example pkgs/evil&&touch PWNED) was split into a second argv list, so --fix spawned an extra executable taken from the path name.

Fix

• src/utils/fix-runner.ts: build apply argv directly from structured target fields (package, version, workspaces, package manager) for npm, pnpm, yarn and bun installs and parent-updates. The && re-split of target.command is gone, and each workspace path is passed as one argv element to spawn (still no shell).
• src/remediation/fix-commands.ts: shell-quote workspace paths and package specs in the human-facing plan strings so a pasted command stays one shell word. Yarn parent-update plan text now matches the argv used by --fix after workspace merge.

Tests

• New apply tests compare the exact argv arrays for && and spaced && workspace paths (direct install and parent-update). On main these produce an extra ["touch", "PWNED"] spawn; on this branch they produce a single package manager spawn.
• New plan quoting test in tests/fix-commands-shell-quote.test.ts.
• Full suite: 174 suites, 2422 tests passing. tsc --noEmit clean.

Summary by CodeRabbit

• Bug Fixes
  • Fix commands now handle package names and workspace paths containing spaces or shell operators more reliably across npm, pnpm, Yarn, and Bun.
  • Workspace-specific and parent-package updates now use the appropriate package manager options, including when refreshing the root workspace.

[Copilot]

🔵 Needs a closer look

The security-sensitive execution rewrite covers four package managers, but the added execution regressions cover only npm, warranting final human validation.

No open issues 🎉

What changed in this PR

Addresses #1385 by separating executable arguments from human-facing fix commands, preventing workspace paths containing && from becoming extra commands.

Changes:

• Builds --fix argument arrays directly from structured targets for npm, pnpm, yarn, and bun.
• Quotes plan output and rebuilds parent-update commands from merged workspace data.
• Adds regression tests for workspace paths containing && and spaces.

File	Description
tests/​fix-runner-apply.test.ts	Checks exact npm arguments and prevents extra executable invocations.
tests/​fix-commands-shell-quote.test.ts	Verifies workspace quoting in displayed commands.
src/​utils/​fix-runner.ts	Replaces command-string splitting with structured argument construction.
src/​remediation/​fix-commands.ts	Quotes display tokens and preserves parent-update workspace context.

_{Review effort: Balanced}

[sonukapoor]

Approving, and this one is worth spelling out, because it is command execution rather than cosmetics.

A human-readable command string was being joined together and then taken apart again by splitting on the && separator, with each piece spawned. So a workspace whose directory name contains that separator stops being one command and becomes two, and the second one runs. A directory name is not trusted input: it can arrive with a repository someone else wrote.

I built a workspace whose member is named with the separator and an injected command, confirmed we emit a workspace-scoped command for it, then pasted that command into a real shell. On main the stub standing in for the package manager receives three arguments and the injected command announces itself as having run. On your branch every payload I tried, including an embedded single quote, a command substitution and backticks, arrives as one intact argument with the path whole.

Ordinary output is unchanged. I compared the printed commands for a tree with nothing unusual in its paths and they are identical to main, which matters because the quoting is conditional rather than applied to every token.

Merge result clean against current main at 178 suites and 2,470 tests. Two mutations: neutering the quoting fails 2 of 13 tests, and restoring the old split in the apply path fails 4 of 11.

One limit worth stating rather than leaving implied. The copy-and-paste half is proven end to end, in a shell, with a real payload. The spawn half is proven at unit level only, because every workspace-scoped target my fixtures produced turned out to be a kind that --fix deliberately skips. The project's own pre-existing test models the kind that does get applied, so the path is covered, but I did not reach it from a real tree.

[sonukapoor]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Walkthrough

Fix-command generation now quotes package and workspace arguments in displayed commands. The fix runner builds structured executable and argument arrays for direct installs and parent updates instead of splitting display strings.

Changes: Fix command execution

Layer / File(s)	Summary
Command and target construction src/remediation/fix-commands.ts, tests/fix-commands-shell-quote.test.ts	Parent-update targets retain workspace lists, including the root workspace entry. Package-manager builders quote package and workspace arguments, and tests check quoting for workspace paths containing &&.
Structured command execution src/utils/fix-runner.ts, tests/fix-runner-apply.test.ts	The runner builds package-manager commands and argv arrays from target fields. Tests check that workspace paths remain single arguments for direct installs and parent updates.

Priority: High

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: High

Possibly related issues

• fix(fix): workspace path with && is treated as a command separator under --fix #1385 — This issue describes workspace paths being split during command execution; this change builds structured argv values and quotes displayed commands.

Possibly related PRs

• OWASP/cve-lite-cli#1436 — Both changes modify the fix runner’s install-failure flow; this PR builds structured command arguments, while #1436 adds a redacted error code to failure messages.

Suggested labels: bug

Merge Risk: Minimal · up to ded8e

The changed commands preserve workspace paths as single arguments, and no current issue has been established that would prevent merging after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly identifies the main change: building --fix argv without splitting workspace paths on &&.
Description check		The description explains the problem, the implementation, the tests, and the linked issue. It does not use the template heading or the exact Closes syntax, but it provides the required change rational…
Linked Issues check		Issue #1385 is the only active direct link. Its coding requirements are met by the code at the reviewed head. 1. Apply argv built from structured fields, not from a shell string. buildApplyCommands…
Out of Scope Changes check		All changed areas connect to issue #1385 or to the argv and plan-text parity it requires. - fix-runner.ts apply argv builders: these implement the requested structured-argv change. - `fix-commands.…

Full details: Description check

Explanation

The description explains the problem, the implementation, the tests, and the linked issue. It does not use the template heading or the exact Closes syntax, but it provides the required change rationale and validation details.

Full details: Linked Issues check

Explanation

Issue #1385 is the only active direct link. Its coding requirements are met by the code at the reviewed head.

1. Apply argv built from structured fields, not from a shell string. buildApplyCommands in src/utils/fix-runner.ts no longer calls buildTargetCommand, and no longer uses .split("&&") or .split(/\s+/). Direct installs use buildInstallApplyArgv. Parent updates use buildParentUpdateApplyArgv. Both read package, targetVersion, childPackage, workspaces, and isDev. This covers npm, pnpm, yarn, and bun.
2. Workspace paths stay single argv elements. npm uses -w <ws>, parent updates use --workspace=<ws>, pnpm uses --filter ./<ws>, yarn uses workspace <ws>, and bun uses --filter <ws>. Each is passed to spawn as one element. No shell is used.
3. Human-facing plan strings are quoted. shellQuote in src/remediation/fix-commands.ts is applied to workspace paths and package specs in buildWorkspaceInstallCommands, buildNpmUpdateCommand, buildPnpmLockfileRefreshCommand, the new Yarn and Bun builders, and buildTargetCommand.
4. Parent-update workspaces are carried on the target. parentUpdateTarget now keeps workspaces. The final loop in buildSuggestedFixCommandPlan rebuilds parent-update command from the merged workspace list, so the plan matches the apply argv.
5. Automated tests cover the requirement. tests/fix-runner-apply.test.ts asserts exact argv arrays for && and spaced && workspace paths, for direct installs and parent updates. It also asserts that only npm is spawned. tests/fix-commands-shell-quote.test.ts covers plan quoting. I did not read the new test file bodies directly. The result is taken from the diff summary and the test-file diff.

Residual uncertainty: the bun dev flag in buildInstallApplyArgv is hard-coded as --dev. The previous path used devFlag("bun"). The devFlag helper was not visible in my truncated read, so flag parity with the old apply path is unverified. This does not affect the issue's stated requirements.

Full details: Out of Scope Changes check

Explanation

All changed areas connect to issue #1385 or to the argv and plan-text parity it requires.

• fix-runner.ts apply argv builders: these implement the requested structured-argv change.
• fix-commands.ts shell quoting: this implements the issue's request for quoted human-facing output.
• fix-commands.ts Yarn and Bun parent-update builders (buildYarnUpgradeCommand, buildBunUpdateCommand): the issue asks for the same treatment of parent-update commands that embed workspace or filter tokens.
• Preserving "." in parentUpdateTarget workspaces: the pnpm branch of buildParentUpdateApplyArgv depends on a root-only "." workspace to choose a plain refresh instead of --recursive. Without this, the apply argv would change. This change is a plan-text behavior change for pnpm root-only refreshes, and it is needed for the apply and plan to agree.
• Test changes in tests/fix-runner-apply.test.ts: these support the new argv requirement. The existing -w expectation was changed to --workspace= to match the new Yarn and npm parent-update syntax.

I found no unrelated file, feature, or refactor in the summarized change set. Extending the fix to pnpm, yarn, and bun follows from the issue's root cause, which lies in the shared buildWorkspaceInstallCommands path.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 12 functions across 4 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🔇 Additional comments: 1

src/remediation/fix-commands.ts: 1

558-594: Trivial · Maintainability & Code Quality · Heavy lift

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Recommended: derive the display strings and the --fix argv from one structured builder.

fix-commands.ts builds display strings with buildNpmUpdateCommand, buildPnpmLockfileRefreshCommand, buildYarnUpgradeCommand and buildWorkspaceInstallCommands. fix-runner.ts builds the matching argv in buildInstallApplyArgv and buildParentUpdateApplyArgv. The two sets of logic encode the same rules: the "." handling, the pnpm recursive fallback, the npm --workspaces fallback, and the flag order.

These rules can drift apart. The comment on Line 474 already notes that the printed plan must match what --fix spawns, but no test enforces it. One builder that returns string[][] would remove the problem. The display layer would then join the argv elements and apply shellQuote to each one.

---

Review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: c0ef330b-8fb0-48eb-9ef9-3554d34b8b7c

Commits

Reviewing files that changed from the base of the PR and between 4a17a94 and ded8e76.

Files selected for processing: 4

• src/remediation/fix-commands.ts
• src/utils/fix-runner.ts
• tests/fix-commands-shell-quote.test.ts
• tests/fix-runner-apply.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

[sonukapoor]

Merged - thank you @alex-powell!