Skip to content

test(cli): round-trip cmd.exe argument escaping through the shipped CLI - #1420

Merged
Simon Ingeson (smonn) merged 2 commits into
mainfrom
wiz-11295-the-windows-ci-job-does-not-exercise-the-cmdexe-argument
Jul 31, 2026
Merged

test(cli): round-trip cmd.exe argument escaping through the shipped CLI#1420
Simon Ingeson (smonn) merged 2 commits into
mainfrom
wiz-11295-the-windows-ci-job-does-not-exercise-the-cmdexe-argument

Conversation

@smonn

@smonn Simon Ingeson (smonn) commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Closes WIZ-11295

Overview of Changes

The windows-smoke job only ever passed --version through cmd.exe, an argument with no meta character, so every escapeArgument branch it exists to protect ran on no runner. argv.cmd forwards %* to node, so twelve arguments now reach a real argv. doctorSmoke.mjs drives dist/cli.js instead of a re-bundled copy, so a call site that drops platform fails CI.

The unit suite was no help and still would not be: it stays green with the escaping halved and with it deleted, because four files assert the escaped shape rather than its effect. Both mutations were run on the runner to size that. The four quote-then-metacharacter cases are the interesting ones — cmd.exe ignores the backslash-escaped quote escapeArgument writes, so under one caret layer a"&echo pwned closes the quoted run and executes. That is what proves the second caret layer load-bearing, and it answers the open question the ticket carried.

Testing

Only Windows CI can verify this; local runs skip. Run 30593534552 is green with 12/12 arguments round-tripping.

bun run typecheck && bun run lint && bun run format:check && bun run knip && bun run test

Checklist

  • Changes follow the code style of this project
  • Self-review completed
  • Tests added/updated (or not applicable)
  • No breaking changes (or described below)

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds workflow-level concurrency to cancel superseded CI runs. Replaces the generic npm spawn smoke test with direct spawn.ts bundling and shipped CLI validation. Adds a Windows command-script argument round-trip smoke test covering cmd.exe-sensitive characters, plus a doctor --json smoke test validating npm-registry and Playwright checks. Documents a missing distinction in caret-escaping coverage.

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

Possibly related PRs

  • qawolf/cli#1405: Directly exercises Windows .cmd argument escaping and spawn behavior.
  • qawolf/cli#1408: Extends the Windows spawn smoke-test workflow and test/spawn coverage.
  • qawolf/cli#1412: Adds related Windows CLI smoke testing and CI workflow updates.

Suggested reviewers: chajac

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title follows Conventional Commits, uses an allowed type and scope, clearly describes the change, and is 71 characters without trailing punctuation.
Description check ✅ Passed The description includes an issue reference, detailed overview, concrete testing information, and a completed checklist matching the repository template.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch wiz-11295-the-windows-ci-job-does-not-exercise-the-cmdexe-argument

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

@smonn
Simon Ingeson (smonn) force-pushed the wiz-11295-the-windows-ci-job-does-not-exercise-the-cmdexe-argument branch from 1d87e0e to 705a205 Compare July 31, 2026 00:12
@smonn Simon Ingeson (smonn) changed the title test(cli): round-trip cmd.exe argument escaping [RED PROOF — do not merge] test(cli): round-trip cmd.exe argument escaping through the shipped CLI Jul 31, 2026
@smonn
Simon Ingeson (smonn) marked this pull request as ready for review July 31, 2026 00:15
The windows-smoke job only passed --version through cmd.exe, an argument with
no meta character, so every escapeArgument branch it guards ran on no runner.
The unit suite is no help: it stays green with the escaping halved and with it
removed, because four files assert the escaped shape rather than its effect.

argv.cmd forwards %* to node, so arguments reach a real argv. Deleting
escapeArgument reds 7 of the first 8 cases. The quote-then-metacharacter cases
red under one caret layer, which is what proves the second layer load-bearing:
cmd.exe ignores the backslash-escaped quote, so `a"&echo pwned` executes.

doctorSmoke.mjs drives dist/cli.js so a call site that drops platform fails CI.
@smonn
Simon Ingeson (smonn) force-pushed the wiz-11295-the-windows-ci-job-does-not-exercise-the-cmdexe-argument branch from 298c1d3 to 95b1d30 Compare July 31, 2026 00:26
@smonn
Simon Ingeson (smonn) merged commit 3916e3c into main Jul 31, 2026
7 checks passed
@smonn
Simon Ingeson (smonn) deleted the wiz-11295-the-windows-ci-job-does-not-exercise-the-cmdexe-argument branch July 31, 2026 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants