fix(cli-app): parse app:exec loosely when command separator is missing (PER-9997)

[prklm10]

Summary

• Some environments strip the -- separator before argv reaches the CLI (notably npm's npx PowerShell shim on Windows), which made percy app:exec -- dotnet test fail with ParseError: Unexpected argument 'dotnet' while the equivalent percy exec invocation only warned and ran the command.
• Reuse the exec command's loose definition for app:exec so both commands degrade the same way when the separator is missing.
• Update the usage help text of both exec and app:exec to [options] [--] <command> since the separator is now optional for both.

Known trade-off (deliberate)

With loose parsing, app:exec no longer hard-rejects unrecognized flags: a typo such as --allowed-hostname used to fail with Unknown option '--allowed-hostname' and now falls through to command lookup, failing with Command not found "--allowed-hostname" after the loose warning. This matches the long-standing behavior of percy exec and is the accepted cost of tolerating a stripped -- separator; it is covered by the degrades unrecognized exec options into the command (loose parsing trade-off) spec.

Test plan

• yarn test in packages/cli-app — 45/45 specs pass. The parses loosely when the command separator is missing spec is hermetic (mocks which.sync, no real subprocess) and asserts both the loose warning and that the argv is parsed as a command.
• yarn test in packages/cli-exec — 40/40 specs pass.

🤖 Generated with Claude Code

[prklm10]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[prklm10]

[High] New spec is non-hermetic and doesn't assert the fixed behavior

This spec mocks neither which.sync nor cross-spawn (unlike packages/cli-exec/test/exec.test.js, which mocks both), so on any machine with the .NET SDK on PATH — precisely the environments this fix targets — exec(['dotnet', 'test']) resolves and spawns a real dotnet test subprocess: slow, flaky, side-effectful. It also only asserts that one specific error message is absent; any other downstream failure (e.g. Command not found "dotnet") also passes, so it doesn't guard the regression it claims to cover.

Suggestion: Assert directly against the parser (import parse from @percy/cli-command and check argv parses as command='dotnet', args=['test'] with the loose warning logged), or at minimum mock which.sync and cross-spawn like cli-exec/test/exec.test.js does and spy on log.warn to confirm the loose message is emitted.

Reviewer: stack:code-reviewer

[prklm10]

Fixed in 0fa31dd: the spec now mocks which.sync (no real binary resolution or subprocess), asserts the rejection Command not found "dotnet" — proving dotnet was parsed as the command rather than rejected as an unexpected argument — and asserts the loose warning is emitted on stderr.

[prklm10]

[Medium] Blanket loose masks genuine flag typos with a less actionable error

loose swallows all unrecognized input, not just the missing--- case: a flag typo such as --allowed-hostname (previously a clear ParseError: Unknown option '--allowed-hostname', deliberately tested since #1043/#2261) now falls through to which.sync('--allowed-hostname') and produces the confusing Command not found "--allowed-hostname". This silently weakens a previously-tested guarantee specific to app:exec (which sets skipDiscovery: true, unlike plain exec).

Suggestion: Call the trade-off out explicitly in the PR description/changelog as a deliberate match of percy exec behavior, and rename/split the updated does not accept asset discovery options spec so its title matches what it now verifies.

Reviewer: stack:code-reviewer

[prklm10]

Addressed in 0fa31dd: the trade-off is now documented in the PR description as deliberate (matching percy exec's long-standing behavior), the old does not accept asset discovery options spec was split so the exec case is named for what it verifies (degrades unrecognized exec options into the command (loose parsing trade-off)), and the stale usage text was updated to [options] [--] <command> in both exec and app:exec.

[prklm10]

Claude Code PR Review

PR: #2334 • Head: 9ab5b6f • Reviewers: stack:code-reviewer

Summary

Copies the percy exec command's loose definition onto app:exec so that when the -- command separator is stripped from argv (e.g. by npx's Windows PowerShell shim), the command warns and runs instead of throwing ParseError: Unexpected argument.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets in the diff
High	Security	Authentication/authorization checks present	N/A	CLI arg parsing only
High	Security	Input validation and sanitization	Pass	Loosened parsing is deliberate and mirrors percy exec; command still resolved via which before spawn
High	Security	No IDOR — resource ownership validated	N/A	No resource access
High	Security	No SQL injection (parameterized queries)	N/A	No database code
High	Correctness	Logic is correct, handles edge cases	Pass	Reviewer traced maybeParseUnknown in packages/cli-command/src/parse.js and confirmed the mechanism; fix correctly mirrors packages/cli-exec/src/exec.js and shares the warning string by reference
High	Correctness	Error handling is explicit, no swallowed exceptions	Fail	loose swallows all unrecognized input, so a flag typo like --allowed-hostname now surfaces as Command not found "--allowed-hostname" instead of Unknown option (Finding 2)
High	Correctness	No race conditions or concurrency issues	N/A	No concurrency in change
Medium	Testing	New code has corresponding tests	Pass	New spec added; existing spec updated
Medium	Testing	Error paths and edge cases tested	Fail	New spec is non-hermetic (can spawn a real dotnet test subprocess) and only asserts a specific error is absent, not the intended parse-and-warn behavior (Finding 1)
Medium	Testing	Existing tests still pass (no regressions)	Pass	Reviewer pulled the branch and ran the packages/cli-app suite locally: 44/44 pass
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	Not applicable
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors the established percy exec loose behavior
Medium	Quality	Changes are focused (single concern)	Pass	One-line production change plus tests
Low	Quality	Meaningful names, no dead code	Pass	—
Low	Quality	Comments explain why, not what	Pass	Comment explains the PowerShell-shim rationale
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies

Findings

• File: packages/cli-app/test/exec.test.js:42

• Severity: High

• Reviewer: stack:code-reviewer

• Issue: The new spec parses loosely when the command separator is missing neither mocks which.sync nor cross-spawn (unlike packages/cli-exec/test/exec.test.js, which mocks both), so on any machine with the .NET SDK on PATH — precisely the environments this fix targets — exec(['dotnet', 'test']) resolves and spawns a real dotnet test subprocess, making the unit test slow, flaky, and side-effectful. It also only asserts that one specific error message is absent (.not.toBeRejectedWithError("Unexpected argument 'dotnet'")); any other downstream failure (e.g. Command not found "dotnet") also passes, so the spec does not actually guard the regression it claims to cover.

• Suggestion: Assert directly against the parser (import parse from @percy/cli-command and check the argv is parsed as command='dotnet', args=['test'] with the loose warning logged), or at minimum mock which.sync and cross-spawn the same way cli-exec/test/exec.test.js does and add a log.warn spy assertion that the loose message was emitted.

• File: packages/cli-app/src/exec.js:194

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: loose swallows all unrecognized input, not just the missing--- case: a genuine flag typo such as --allowed-hostname (previously a clear ParseError: Unknown option '--allowed-hostname', deliberately tested since ✨ Add hidden CLI app command for future SDKs #1043/feat(cli): self-hosted (non-BrowserStack) Maestro + Percy support — V1 #2261) now falls through to which.sync('--allowed-hostname') and produces the confusing Command not found "--allowed-hostname". This silently weakens a previously-tested guarantee specific to app:exec (which sets skipDiscovery: true, unlike plain exec).

• Suggestion: Acknowledge the trade-off explicitly (PR description/changelog note) as a deliberate decision to match percy exec, and rename or split the updated does not accept asset discovery options spec so its title matches what it now verifies (unrecognized options degrade to command-not-found, not hard rejection).

• File: packages/cli-app/src/exec.js:185

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: usage: '[options] -- <command>' (and the pre-existing equivalent in packages/cli-exec/src/exec.js) documents -- as mandatory, but it is now optional (degrades to a warning), so percy app:exec --help misleads users.

• Suggestion: Change to '[options] [--] <command>' or add a help note about the fallback; fine as a follow-up.

---

Verdict: FAIL — the one-line production fix is correct and consistent with percy exec, but the accompanying test is non-hermetic (can spawn a real dotnet test subprocess) and does not verify the fixed behavior; strengthen it per Finding 1 before merging.

[prklm10]

Claude Code PR Review

PR: #2334 • Head: 0fa31dd • Reviewers: stack:code-reviewer (re-review of fixes for the prior FAIL on 9ab5b6f)

Summary

Copies percy exec's loose definition onto app:exec so a stripped -- separator (e.g. npx's Windows PowerShell shim) warns and runs instead of throwing ParseError; this push addresses all three prior review findings (hermetic loose-parsing spec, documented flag-typo trade-off with a renamed spec, and [options] [--] <command> usage text in both commands).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets in the diff
High	Security	Authentication/authorization checks present	N/A	CLI arg parsing only
High	Security	Input validation and sanitization	Pass	Loosened parsing is deliberate, mirrors percy exec; command still resolved via which before spawn
High	Security	No IDOR — resource ownership validated	N/A	No resource access
High	Security	No SQL injection (parameterized queries)	N/A	No database code
High	Correctness	Logic is correct, handles edge cases	Pass	Mechanism traced through maybeParseUnknown in packages/cli-command/src/parse.js; mirrors packages/cli-exec/src/exec.js and shares the warning string by reference
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	The flag-typo → Command not found degradation is now an explicitly documented, tested trade-off (PR description + dedicated spec), matching percy exec's long-standing behavior
High	Correctness	No race conditions or concurrency issues	N/A	No concurrency in change
Medium	Testing	New code has corresponding tests	Pass	Loose-parsing spec plus a dedicated trade-off spec
Medium	Testing	Error paths and edge cases tested	Pass	Specs are hermetic (which.sync stubbed, no subprocess) and positively assert the loose warning and parse outcome; reviewer mutation-tested by removing loose: and confirmed both specs fail
Medium	Testing	Existing tests still pass (no regressions)	Pass	Reviewer pulled the branch: cli-app 45/45 pass; cli-exec 40/40 pass; no test hardcodes the old usage string
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	Not applicable
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors percy exec loose behavior; test mocks follow cli-exec's own patterns
Medium	Quality	Changes are focused (single concern)	Pass	One-line production change, tests, and directly related help-text fix
Low	Quality	Meaningful names, no dead code	Pass	Trade-off spec renamed to describe what it verifies
Low	Quality	Comments explain why, not what	Pass	Comments explain the PowerShell-shim rationale and mock intent
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies

Findings

No blocking findings. All three findings from the prior review of 9ab5b6f are resolved and were independently verified by the reviewer:

1. [High → Resolved] packages/cli-app/test/exec.test.js — the loose-parsing spec now stubs which.sync (no real binary lookup or cross-spawn), asserts rejection with Command not found "dotnet" (proving dotnet parsed as the command), and positively asserts the loose warning on stderr. Verified to fail when the loose: line is removed.
2. [Medium → Resolved] The flag-typo UX trade-off is documented in the PR description as deliberate (matching percy exec) and covered by the renamed spec degrades unrecognized exec options into the command (loose parsing trade-off).
3. [Low → Resolved] usage updated to [options] [--] <command> in both packages/cli-app/src/exec.js and packages/cli-exec/src/exec.js; no test hardcodes the old string.

Informational, non-blocking: there is no regression guard asserting the new usage string itself — very low risk.

---

Verdict: PASS — all prior findings resolved and independently verified (including mutation-testing the new specs); the production change is minimal, correct, and now backed by hermetic, meaningful tests.

[prklm10]

Claude Code PR Review

PR: #2334 • Head: 0fa31dd • Reviewers: stack:code-reviewer

Summary

Reuses percy exec's loose definition on app:exec so a stripped -- separator (e.g. npx's Windows PowerShell shim) degrades to a warning and command execution instead of ParseError: Unexpected argument, with hermetic regression tests, a documented flag-typo trade-off, and [options] [--] <command> usage text in both commands.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets in the diff
High	Security	Authentication/authorization checks present	N/A	CLI arg parsing only
High	Security	Input validation and sanitization	Pass	Loosened parsing is deliberate, mirrors percy exec; command still resolved via which before spawn
High	Security	No IDOR — resource ownership validated	N/A	No resource access
High	Security	No SQL injection (parameterized queries)	N/A	No database code
High	Correctness	Logic is correct, handles edge cases	Pass	One-line fix plugs correctly into maybeParseUnknown (packages/cli-command/src/parse.js); loose shared by reference so the warning text can't drift between commands; start remains strictly parsed
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Flag-typo → Command not found degradation is an explicitly documented, named, and tested trade-off matching percy exec's shipped behavior
High	Correctness	No race conditions or concurrency issues	N/A	No concurrency in change
Medium	Testing	New code has corresponding tests	Pass	Loose-parsing spec plus a dedicated trade-off spec
Medium	Testing	Error paths and edge cases tested	Pass	Specs are hermetic (which.sync stubbed to null — no real binary lookup or subprocess) with positive assertions on the loose warning and parse outcome; reviewer mutation-tested by removing loose: and confirmed both specs fail meaningfully
Medium	Testing	Existing tests still pass (no regressions)	Pass	packages/cli-app 45/45 pass locally on this head
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	Not applicable
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors percy exec loose behavior; mocks follow cli-exec's own test patterns
Medium	Quality	Changes are focused (single concern)	Pass	One-line production change, tests, and directly related help-text fix
Low	Quality	Meaningful names, no dead code	Pass	Trade-off spec named for what it verifies
Low	Quality	Comments explain why, not what	Pass	Comments explain the PowerShell-shim rationale and mock intent
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies

Findings

No blocking findings. All findings from the review of the previous head (9ab5b6f) are resolved on this commit and were independently verified by the reviewer (local test run, mutation check confirming the new specs fail without the fix).

Informational, non-blocking:

• Neither cli-app nor cli-exec asserts the literal '[options] [--] <command>' usage string, so an accidental revert of that cosmetic help text wouldn't be caught by tests — very low risk; a one-line help-output assertion would close it if desired.

---

Verdict: PASS — the fix is minimal and correct, the tests are hermetic and verified to guard the regression, and the UX trade-off is explicitly documented; only a cosmetic, non-blocking coverage note remains.