Skip to content

feat(run): support -- passthrough for app arguments#506

Merged
nmetulev merged 18 commits into
microsoft:mainfrom
mattleibow:mattleibow/add-double-dash-passthrough-run
May 1, 2026
Merged

feat(run): support -- passthrough for app arguments#506
nmetulev merged 18 commits into
microsoft:mainfrom
mattleibow:mattleibow/add-double-dash-passthrough-run

Conversation

@mattleibow
Copy link
Copy Markdown
Member

Summary

Adds support for the -- passthrough separator in winapp run, so users can pass flags to their launched app without escaping:

# Before (requires escaping)
winapp run . --args "--my-flag value"

# After (no escaping needed)
winapp run . -- --my-flag value

Tokens after -- are collected and merged with any --args value. Unknown tokens before -- are still caught and reported as errors (e.g. typos like --detch).

tool and store already used TreatUnmatchedTokensAsErrors = false — no changes needed for those.

Changes

  • RunCommand.cs: Set TreatUnmatchedTokensAsErrors = false, collect post--- tokens, count-based pre-dash validation, merge with --args
  • RunCommandTests.cs: 14 tests covering parse-level behaviour, handler happy paths, rejection, and mode interactions (--no-launch, --detach, --debug-output)
  • docs/usage.md + docs/fragments/skills/winapp-cli/setup.md: Updated examples

Test coverage

Area Tests
Parse: bare --, no parse errors for unknown pre-dash tokens 3
Handler: bare --, multiple args, merge with --args, space-quoting 4
Handler rejection: unknown option, same-value duplicate bug case 3
Mode interaction: --no-launch, --detach, --debug-output 3

All 57 RunCommand tests pass.

mattleibow and others added 2 commits April 29, 2026 13:58
Add support for the standard Unix '--' separator so users can pass arguments
to the launched application without quoting or escaping:

  winapp run . -- --flag value

Previously the only way to pass flags was:

  winapp run . --args "--flag value"

Implementation:
- Set TreatUnmatchedTokensAsErrors = false on RunCommand so the parser
  accepts tokens after '--' without error
- In the handler, collect tokens tagged DoubleDash/Argument after '--' as
  passthrough args and merge them with any --args value
- Validate that unmatched tokens that appear BEFORE '--' are still
  rejected, preserving typo detection (--detch etc.)
- Update --args option description to document the -- alternative
- Add 5 new unit tests covering passthrough, merging, quoting, and
  error-on-bad-options-before-dash

Docs:
- docs/usage.md and docs/fragments/skills/winapp-cli/setup.md updated
  with -- examples

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ests

Replace HashSet-based passthrough detection with a Dictionary<string,int>
count-based approach. The previous HashSet would silently allow pre-dash
unknown tokens when the same value also appeared after '--' (e.g.
winapp run . --flag -- --flag).

Also expand the -- passthrough test suite from 5 to 14 tests, covering:
- Parse-level: bare --, unknown-before-dash stays out of parse errors
- Handler: bare --, multiple args, merge with --args, space-quoted values
- Handler rejection: unknown token without --, same-value duplicate (bug case)
- Mode interaction: --no-launch, --detach, --debug-output all work with --

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 29, 2026 12:25
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds -- passthrough support to winapp run so users can forward application arguments without needing to escape/quote them into --args, while still rejecting unknown winapp run options typed before the separator.

Changes:

  • Updated run command parsing to allow unmatched tokens, collect tokens after --, validate unknown tokens before --, and merge passthrough tokens with --args.
  • Added a new test region covering parse behavior, handler forwarding/merging, rejection of unknown options, and interactions with other run modes.
  • Updated docs to describe and demonstrate the new -- passthrough usage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/winapp-CLI/WinApp.Cli/Commands/RunCommand.cs Enables unmatched tokens for run, collects/validates passthrough args after --, merges with --args.
src/winapp-CLI/WinApp.Cli.Tests/RunCommandTests.cs Adds coverage for -- passthrough parsing and handler behavior across modes.
docs/usage.md Documents -- passthrough as an alternative to --args and adds an example.
docs/fragments/skills/winapp-cli/setup.md Adds setup documentation example showing passthrough usage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/winapp-CLI/WinApp.Cli/Commands/RunCommand.cs Outdated
Comment thread src/winapp-CLI/WinApp.Cli/Commands/RunCommand.cs Outdated
Comment thread src/winapp-CLI/WinApp.Cli.Tests/RunCommandTests.cs
mattleibow and others added 2 commits April 29, 2026 17:27
Address three issues raised in PR review:

1. Only treat the FIRST DoubleDash token as the -- separator; subsequent
   -- tokens (e.g. winapp run . -- -- --flag) are forwarded to the app
   as literal values instead of being dropped.

2. Replace the naive space-only quoting with a full Windows
   CommandLineToArgvW-compatible EscapeWindowsArg helper that correctly
   handles embedded double-quotes, backslashes before quotes, and
   trailing backslashes inside a quoted value.

3. Add two new tests covering these cases:
   - RunCommand_DoubleDashPassthrough_ForwardsLiteralDoubleDash
   - RunCommand_DoubleDashPassthrough_EmbeddedQuotesAndBackslashes_EscapesCorrectly

No existing dependencies (System.CommandLine 2.0.5, Spectre.Console, .NET 10
BCL) expose a public API for this escaping — the helper is self-contained.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add WindowsCommandLine.ExtractPassthroughArgs() encapsulating all
  double-dash parsing and pre-dash unknown-token validation
- Simplify RunCommand handler to a single WCL call (~50 lines removed)
- Revert ToolCommand.cs to original (ArgumentList improvement noted in PR)
- Move escaping-specific tests out of RunCommandTests into WCLTests
- Add 6 new ExtractPassthroughArgs unit tests in WindowsCommandLineTests

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mattleibow
Copy link
Copy Markdown
Member Author

Note: ToolCommand argument escaping

ToolCommand.cs currently uses a naive space-quoting approach for tool arguments:

Arguments = string.Join(" ", toolArgs.Select(a => a.Contains(' ') ? $"\"{a}\"" : a))

This has the same embedded-quotes / trailing-backslash issue documented in earlier review comments. The correct fix is to switch to ProcessStartInfo.ArgumentList:

foreach (var arg in toolArgs)
    processStartInfo.ArgumentList.Add(arg);

ProcessStartInfo.ArgumentList (available since .NET 5) lets the BCL handle CommandLineToArgvW-compatible escaping internally — there's no need to call WindowsCommandLine.EscapeArgument since we're launching a real process (not going through COM).

This was intentionally left out of the current PR to keep scope focused on the run command passthrough feature. Tracked as a follow-up improvement.

mattleibow and others added 3 commits April 29, 2026 21:05
…ting

Add RunCommand_DoubleDashPassthrough_ValueWithSpace_QuotedInLaunchArgs
to verify the full JoinArguments pipeline at the command level, not just
in WindowsCommandLineTests isolation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ription

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/winapp-CLI/WinApp.Cli/Commands/RunCommand.cs Outdated
mattleibow and others added 11 commits April 29, 2026 22:40
TreatUnmatchedTokensAsErrors = false (needed for -- passthrough) caused
OptionTypoValidator.FindLikelyLongOptionTypo to skip RunCommand entirely.
As a result, 'winapp run . -manifest foo' would emit a generic
'Unrecognized option' message instead of 'Did you mean --manifest?'.

Fix: add TryGetTypoSuggestion() to OptionTypoValidator — a single-token
check that can be called from the handler where parseResult.UnmatchedTokens
is available. The RunCommand handler now calls it for each unknown token
before emitting the error, surfacing the Did-you-mean? hint when applicable.

Also add 3 tests:
- ParseOptions_SingleDashTypo_NoParseError_HandlerValidates
- RunCommand_SingleDashTypo_ReturnsError
- ParseOptions_SingleDashTypo_AfterDoubleDash_NoError (passthrough safety)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…sthrough

Replace TreatUnmatchedTokensAsErrors=false + manual validation with a
ZeroOrMore Argument<string[]> PassthroughArgument approach. System.CommandLine
routes all post-'--' tokens into the argument, keeping all pre-'--' option
validation intact.

Because ZeroOrMore also absorbs unrecognised option-like tokens before '--' as
positionals, add WindowsCommandLine.SplitPassthroughTokens() which uses a
count-based diff between GetValue(PassthroughArgument) (all absorbed) and the
post-'--' token walk (legitimate) to identify and reject invalid pre-dash tokens
without needing to classify each token's type (option values and positionals both
have TokenType.Argument in the raw stream).

Remove ExtractPassthroughArgs (no longer needed), TryGetTypoSuggestion (no
longer needed since OptionTypoValidator is never bypassed), and the three typo
tests that were testing the old approach.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add SystemCommandLineDoubleDashBehaviourTests class to empirically
validate System.CommandLine's -- passthrough behaviour:

- Proves that without ZeroOrMore + TreatUnmatchedTokensAsErrors=true,
  post-'--' tokens produce parse errors (validates our design choice)
- Proves with ZeroOrMore, post-'--' tokens are cleanly captured
- Proves pre-'--' unknowns are absorbed silently into ZeroOrMore
  (documents the caveat our SplitPassthroughTokens solves)
- Proves option values do NOT appear in ZeroOrMore GetValue result

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two new empirical tests clarify the relationship between S.CL's
documented -- support and the ZeroOrMore-specific absorption caveat:

- SCL_DoubleDash_PostDashOptionLike_RoutedToArgument_NotTreatedAsOption:
  mirrors the exact S.CL docs example; proves post-'--' tokens go to
  the positional argument, not the option with the same name.

- SCL_DoubleDash_UnknownPreDash_WithExactlyOne_ProducesParseError:
  proves the pre-'--' absorption caveat is ZeroOrMore-specific; with
  ExactlyOne arity, an unknown pre-'--' token IS a parse error.

Together with the existing ZeroOrMore absorption test, this shows why
SplitPassthroughTokens is needed despite S.CL supporting -- natively.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
RunCommand_BadTokenBeforeDoubleDash_RejectsWithError_DoesNotForwardGoodToken
covers the exact case: winapp run . --badtoken -- --cooltoken

- exit code must be 1 (bad pre-dash token rejected)
- app must NOT be launched (command aborts before forwarding --cooltoken)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…h arg, JSON error in run, alias test

- Program.cs / GlobalOptionPreScan: pre-scan for --verbose/--quiet/--json/--cli-schema now stops at the first standalone '--' so 'winapp run . -- --json' no longer flips winapp into JSON mode (the token is a passthrough payload for the launched app).

- RunCommand.PassthroughArgument: marked Hidden so help and cli-schema no longer advertise the misleading 'winapp run <input-folder> [<app-args>...]' syntax.

- RunCommand handler: when --json is set, emit a structured PrintJson error body for unrecognized pre-'--' tokens instead of exiting 1 with empty stdout (logger is suppressed in JSON mode).

- RunCommand: extracted internal static BuildAliasProcessStartInfo so the --with-alias passthrough path is unit-testable; added 4 tests.

- Tests: 7 GlobalOptionPreScan tests for the '--' boundary; 5 new RunCommand tests covering JSON-mode error emission and alias arg construction.

- docs/cli-schema.json regenerated to reflect Hidden=true on app-args.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@nmetulev nmetulev enabled auto-merge (squash) May 1, 2026 17:32
@nmetulev nmetulev disabled auto-merge May 1, 2026 17:34
@nmetulev nmetulev merged commit b6c4d98 into microsoft:main May 1, 2026
19 of 20 checks passed
nmetulev added a commit that referenced this pull request May 1, 2026
…ork PRs) (#514)

## Why
PR builds from forks (e.g. #506) are failing the `build-and-package` job
because the in-build *Report build metrics* step tries to POST a PR
comment, but `GITHUB_TOKEN` is silently downgraded to read-only for the
`pull_request` event when the head repo is a fork — so the API returns
`403 Resource not accessible by integration` and the build is marked
failed even though build/test/package all succeeded.

## What
Move the comment-posting out of the build into a separate
`workflow_run`-triggered workflow that runs in the base repo context
with full token write access. Same fix pattern applied to
`mark-metrics-stale` (now on `pull_request_target`).

### Files

| File | Change |
|---|---|
| `.github/actions/report-metrics/action.yml` | Removed comment-posting
step; now stages current+baseline JSONs into a `metrics-data` artifact.
|
| `.github/workflows/post-metrics-comment.yml` (new) | Triggered on
`workflow_run` of *Build and Package*. Downloads `metrics-data`, builds
+ posts/updates the PR comment. |
| `.github/workflows/mark-metrics-stale.yml` (new) | Same job that lived
in build-package.yml, now on `pull_request_target` so fork PRs also get
write perms. |
| `.github/workflows/build-package.yml` | Removed `mark-metrics-stale`
job (relocated). |

## Security
The `metrics-data` artifact is produced by code from the PR (potentially
untrusted). The privileged `workflow_run` workflow treats it
accordingly:

- **PR identity is never trusted from the artifact.** The PR number is
resolved from the trusted `workflow_run.head_sha` via
`listPullRequestsAssociatedWithCommit` and verified against the PR's
current head.
- **head_sha + run id in the footer come from `workflow_run` metadata**,
not the artifact.
- **All metric values are coerced + range-checked**; binary-size keys
are filtered against an allowlist with no fallback rendering — a
malicious build can't inject Markdown, links, mentions, or HTML into the
bot-authored comment.
- **Gated on `workflow_run.conclusion == 'success'`** so failed builds
can't overwrite a previously good metrics comment with partial data.
- **Comment lookup is paginated** so the existing comment is reliably
found on noisy PRs.

## Validation
- YAML parses (`yaml.safe_load`).
- Inline JS body parses (`node --check`).
- The actual fork-PR scenario can only be verified end-to-end after
merge — once merged, re-running #506's build will exercise both new
workflows.

## Follow-ups (out of scope here)
- Consider adding `concurrency: pr-N` to `post-comment` so out-of-order
`workflow_run` arrivals on rapid pushes can't race the comment update.

---------

Co-authored-by: Nikola Metulev <711864+nmetulev@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mattleibow mattleibow deleted the mattleibow/add-double-dash-passthrough-run branch May 1, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants