Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
prklm10 merged 2 commits into from
Jul 14, 2026
+38 −5
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ab5b6f
fix(cli-app): parse app:exec loosely when command separator is missing
prklm10 Jul 10, 2026
0fa31dd
test(cli-app): address review — hermetic loose-parsing spec, usage text
prklm10 Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/cli-app/src/exec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,13 +182,17 @@ export function maybeInjectScreenshotDir(ctx, log) {

export const exec = command('exec', {
description: 'Start and stop Percy around a supplied command for native apps',
usage: '[options] -- <command>',
usage: '[options] [--] <command>',
commands: [start, stop, ping],

flags: ExecPlugin.default.definition
// grouped flags are built-in flags
.flags.filter(f => !f.group),

// some environments (e.g. npx PowerShell shims) strip the `--` separator;
// fall back to loose parsing like `percy exec` instead of hard failing
loose: ExecPlugin.default.definition.loose,

@prklm10 prklm10 Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 prklm10 Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


percy: {
server: true,
projectType: 'app',
Expand Down
35 changes: 32 additions & 3 deletions packages/cli-app/test/exec.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import fs from 'fs';
import os from 'os';
import path from 'path';
import { setupTest } from '@percy/cli-command/test/helpers';
import { logger, setupTest } from '@percy/cli-command/test/helpers';
import * as ExecPlugin from '@percy/cli-exec';
import {
exec, start, stop, ping,
Expand All @@ -28,12 +28,41 @@ describe('percy app:exec', () => {
});

it('does not accept asset discovery options', async () => {
await expectAsync(exec(['--allowed-hostname', 'percy.io']))
.toBeRejectedWithError("Unknown option '--allowed-hostname'");
await expectAsync(start(['--network-idle-timeout', '500']))
.toBeRejectedWithError("Unknown option '--network-idle-timeout'");
});

it('degrades unrecognized exec options into the command (loose parsing trade-off)', async () => {
// with loose parsing, exec no longer hard-rejects unknown flags with
// "Unknown option" — a typo like --allowed-hostname falls through to
// command lookup and fails there instead
let { default: which } = await import('which');
spyOn(which, 'sync').and.returnValue(null);

await expectAsync(exec(['--allowed-hostname', 'percy.io']))
.toBeRejectedWithError('Command not found "--allowed-hostname"');
});

it('parses loosely when the command separator is missing', async () => {
// some environments (e.g. npx PowerShell shims) strip the `--` separator;
// the command should warn and run instead of throwing a parse error
expect(exec.definition.loose).toEqual(ExecPlugin.default.definition.loose);

// stub command resolution so no real binary is looked up or spawned; the
// "Command not found" rejection proves `dotnet` was parsed as the command
// (not rejected as an unexpected argument) before reaching the lookup
let { default: which } = await import('which');
spyOn(which, 'sync').and.returnValue(null);

await expectAsync(exec(['dotnet', 'test']))
.toBeRejectedWithError('Command not found "dotnet"');

expect(logger.stderr).toEqual(jasmine.arrayContaining([
`[percy] ${ExecPlugin.default.definition.loose}`,
'[percy] Error: Command not found "dotnet"'
]));
});

describe('maybeInjectMaestroServer', () => {
function ctxFor(argv, addr = 'http://localhost:5338') {
return {
Expand Down
2 changes: 1 addition & 1 deletion packages/cli-exec/src/exec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { waitForTimeout } from '@percy/client/utils';

export const exec = command('exec', {
description: 'Start and stop Percy around a supplied command',
usage: '[options] -- <command>',
usage: '[options] [--] <command>',
commands: [start, stop, ping, replay],

flags: [{
Expand Down
Loading