Repository navigation
Make dotnet run pass its arguments to the app, as it does without the package - #724
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes #723 by preserving application arguments passed through dotnet run in the NuGet integration.
Changes:
- Adds the required
--forwarding separator to generated WinApp CLI arguments. - Adds regression coverage for separator placement and
WinAppLaunchArgsordering. - Documents argument forwarding across NuGet and .NET guidance.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/winapp-NuGet/tests/NuGet.Tests.ps1 |
Tests computed run-argument ordering. |
src/winapp-NuGet/README.md |
Documents NuGet argument forwarding. |
src/winapp-NuGet/build/Microsoft.Windows.SDK.BuildTools.WinApp.targets |
Restores the application-argument separator. |
plugins/winapp/skills/winapp-frameworks/SKILL.md |
Updates shipped framework guidance. |
docs/usage.md |
Adds forwarding usage example. |
docs/guides/dotnet.md |
Updates the .NET workflow guide. |
docs/dotnet-run-support.md |
Documents transient and persistent arguments. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Build Metrics ReportBinary Sizes
Test Results❌ 4571 passed, 1 failed, 5 skipped out of 4577 tests in 558.9s (+10 tests, -79.8s vs. baseline) Test Coverage✅ 89.1% line coverage, 82.4% branch coverage · ✅ no change vs. baseline CLI Startup Time48ms median (x64, Updated 2026-08-13 18:15:31 UTC · commit |
Alexandre Zollinger Chohfi (azchohfi)
changed the title
16611a5 to
d54255d
Compare
Alexandre Zollinger Chohfi (azchohfi)
changed the title
… package `dotnet run --devtools` failed with "Unrecognized argument" for a project that references this package, and `dotnet run -- --devtools` failed identically. The same commands work on any other project, so referencing the package silently changed what `dotnet run` means. The .NET SDK appends application arguments to $(RunArguments) verbatim, and System.CommandLine consumes any standalone separator while parsing `dotnet run` itself and never re-emits it, so both spellings arrive at winapp as a bare token in winapp's own option namespace. RunArguments now ends with a separator, which puts everything the SDK appends into winapp's passthrough region instead. That is a one line change and needs no CLI change: `winapp run` keeps rejecting unknown options, so a typo in a hand-typed invocation still fails loudly. BREAKING: options written after `dotnet run` used to configure the launcher. `dotnet run --detach` now detaches nothing and passes --detach to the app. Use the WinAppRun* properties instead, for example -p:WinAppRunDetach=true. MSBuild cannot warn about this because it never sees those tokens, so winapp does: when invoked as the NuGet caller and a forwarded argument matches one of its own options, it names the property that replaces it. Unknown arguments stay silent -- they never had a winapp meaning -- and a direct `winapp run . -- x` stays silent too, since that is an explicit request to forward. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a209763e-7185-4986-bf0b-98e52c06b4be
d54255d to
2846641
Compare
|
Copilot review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/winapp-CLI/WinApp.Cli/Commands/RunCommand.cs:795
isJsononly reflects launcher options parsed before the separator. The NuGet path places forwarded arguments after--, andGlobalOptionPreScanexplicitly stops there, sodotnet run --jsonleavesisJsonfalse and this method later emits a notice for--json. The same applies to forwarded--quiet, contradicting the PR's promised JSON/quiet suppression. Check the forwarded option names as well, including aliases and attached-value forms.
if (isJson || forwardedArgs.Count == 0 || !logger.IsEnabled(LogLevel.Information))
{
return;
}
docs/dotnet-run-support.md:210
- This PR introduces the routing change, so saying it happened "in a previous release" gives users incorrect migration timing. Describe it as a breaking change in this release (or name the exact version where it starts).
> This changed in a previous release. Options written directly after `dotnet run` used to configure
> WinApp, so `dotnet run --detach` detached the launcher; now it reaches your application. If winapp
src/winapp-CLI/WinApp.Cli/Commands/RunCommand.cs:837
- Exact token matching misses accepted attached-value spellings such as
--executable=foo.exeand--detach=true. Those forms previously configured winapp and now reach the app, but receive no migration notice. Normalize the portion before the first=for option lookup, deduplication, and property mapping while retaining the original token in the message/fallback.
foreach (var arg in forwardedArgs)
{
if (!knownOptions.Contains(arg) || !alreadyReported.Add(arg))
{
Reviewed the current PR state. I found no actionable code changes to make; the latest CI runs for the current commit are green, and the older failed build was superseded by the current passing runs. |
Three review follow-ups. An option attached to its value (--executable=foo.exe, --detach=true) configured winapp before this change just as the separated spelling did, but exact token matching missed it, so those invocations changed meaning with no notice at all. Lookup, de-duplication and property mapping now use the name before the '=', while the message still quotes the token as the user typed it. The docs said this changed "in a previous release". It is introduced here, so the note now says so and is marked as breaking. The claim that --json and --quiet are suppressed was wrong for the NuGet path. With the trailing separator, `dotnet run --json` forwards --json to the app rather than putting winapp in JSON mode, so isJson stays false and the notice fires -- which is the correct behavior, because the token really did go to the app. The guard still does its job when winapp itself is in JSON or quiet mode. Only the description was inaccurate; no behavior change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a209763e-7185-4986-bf0b-98e52c06b4be
WaitForPidFile_EmptyThenPopulated_WaitsForAParsableValue failed on CI with a 15 second TimeoutException. Both tests added in #732 populated or released the file from a Task.Run continuation and then waited on the helper, so each depended on the thread pool scheduling that continuation promptly. On a loaded agent running four test workers that is not guaranteed -- reintroducing, in the tests meant to remove flakiness, exactly the kind of timing dependence they were fixing. Neither test needs concurrency to prove its point: - The sharing-violation test now asserts the exception TYPE with nothing ever releasing the handle. The old code threw IOException on the first poll; the retry swallows it and runs out the clock, so TimeoutException is the signal. Verified it still fails in 72ms when the retry is removed. - The empty-file test asserts that a permanently empty file times out, which is what the parse guard is for. The populated path is already covered by the tree-kill test end to end. Both now run in well under a second and cannot be perturbed by machine load. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a209763e-7185-4986-bf0b-98e52c06b4be
Review follow-up. Rewriting the exclusive-handle test to assert a timeout left both remaining tests asserting failure modes, so nothing at unit level proved a readable PID file is actually parsed -- only the tree-kill test did, and that exercises the whole launcher. The comment claiming otherwise was stale. Adds WaitForPidFile_PopulatedFile_ReturnsThePid: write a value, call the helper, assert the value. No concurrency, so it keeps the determinism the rest of this change is about, and corrects the comment to point at it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a209763e-7185-4986-bf0b-98e52c06b4be
|
Addressing the three suppressed comments from the last Copilot review, since they don't create reply-able threads. All three were legitimate; two were fixed and one was a documentation error on my side rather than a behavior bug. 1. Attached-value spellings missed the notice — fixed in Confirmed on the built binary:
2. Docs said "in a previous release" — fixed in 3. You were right that the claim was false, and I verified it: But the behavior is correct as-is. With the separator in place, Two later commits also landed here: |
|
Copilot review |
Reviewed the latest four commits. I found no actionable code changes to make; the only potential concern overlaps with the intentional migration notice behavior already called out above. I also checked CI: the older |
Co-authored-by: azchohfi <527713+azchohfi@users.noreply.github.com>
## Problem `WaitForPidFile_EmptyThenPopulated_WaitsForAParsableValue` — added by me in #732 and now on `main` — failed on CI: ``` System.TimeoutException: The descendant PID file '...empty_....pid' was not written within 00:00:15. ``` Both tests #732 added did the same thing: populate (or release) the file from a `Task.Run` continuation, then `await` the helper. That makes each one depend on the thread pool scheduling that continuation promptly. On a loaded agent running 4 test workers, it isn't. Which is the irony worth naming: these are the tests meant to *remove* flakiness from this file, and they reintroduced exactly the kind of timing dependence they were fixing. A 150 ms delay under a 15 s budget looks generous right up until the pool is saturated. ## Fix Neither test needs concurrency to prove its point. **Sharing violation** — assert the exception *type*, with nothing ever releasing the handle: ```csharp using var exclusive = new FileStream(pidFile, FileMode.Open, FileAccess.Write, FileShare.None); await Assert.ThrowsAsync<TimeoutException>( async () => await WaitForPidFileAsync(pidFile, TimeSpan.FromMilliseconds(300), ct)); ``` The old code threw `IOException` on the very first poll; the retry swallows it and runs out the clock. `TimeoutException` *is* the signal that the retry works — no second thread required. **Empty file** — assert that a permanently empty file times out, which is precisely what the parse guard is for. The populated path is already covered end-to-end by the tree-kill test. ## Validation - 3/3 pass, both rewritten tests now finishing in well under a second instead of 15. - Regression coverage intact: with the `catch (IOException)` removed, `WaitForPidFile_WriterHoldsFileExclusively_RetriesInsteadOfThrowing` still fails in **72 ms**. - No production code touched — `DotNetService` is unchanged; this is tests only. This unblocks `build-and-package`, which is currently red on `main` and on #724. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: azchohfi <527713+azchohfi@users.noreply.github.com> Copilot-Session: a209763e-7185-4986-bf0b-98e52c06b4be
Nikola Metulev (nmetulev)
left a comment
Nikola Metulev (nmetulev)
left a comment
There was a problem hiding this comment.
🤖 AI-generated review (winappcli pr-review skill) — verify before acting.
Requesting changes for three validated NuGet-path issues. The core separator change is correctly isolated: direct winapp run, npm-tagged calls, project mode, and RunPackagedApp retain their existing behavior.
Review follow-up on three validated findings. The notice matched every option name reachable from the parser, so ordinary application flags triggered it. `--help`, `--configuration`, `-p`, `--no-build` and the other project-mode options are ignored in folder mode -- the only mode the NuGet targets use -- so they never had a winapp meaning on this path and there is nothing to migrate. The generic fallback also emitted advice that fails. It suggested WinAppRunArgs="<option>" without the value, so for `--configuration Release` the recommended command errors with "Required argument missing for option: '--configuration'". OptionToMSBuildProperty is now the whole trigger set, which removes the false positives and the broken suggestion together: every notice names a property that actually replaces the option. The docs claimed a standalone separator is optional and has no effect. That holds only until the app's flag collides with a `dotnet run` option: verified that `dotnet run --configuration Release` reaches the app with nothing, while `dotnet run -- --configuration Release` forwards both tokens. Qualified in dotnet-run-support.md, usage.md, guides/dotnet.md, the package README and the frameworks skill. The .NET guide also showed WinAppRunDebugOutput and WinAppRunDetach together, which the CLI rejects as mutually exclusive -- in the same PR that documents that constraint. Reduced to one property. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a209763e-7185-4986-bf0b-98e52c06b4be
|
Nikola Metulev (@nmetulev) — all three addressed in 1. Migration notices on legitimate app flags. Took both of your options rather than either: Verified before/after:
Your framing was the useful part: the project-mode options are ignored in folder mode, which is the only mode the NuGet targets use, so they never had a winapp meaning on this path. Nothing to migrate. Guarded by a 2. Impossible example. Confirmed — 3. Qualified in all five places you listed, each naming the colliding option set and showing the working form. Code can't fix this one — the SDK consumes those tokens before winapp is involved — so it stays a docs qualification. 138/138 |
7604c72 to
4e899c6
Compare
Problem
For a project that references this package, both of these fail:
The second one is the trap: the error tells you to use
--, and you did. Both commands work on any project that doesn't reference the package, so adding it silently changed whatdotnet runmeans.Why both spellings fail identically
The .NET SDK appends your application arguments to
$(RunArguments)verbatim, and System.CommandLine consumes any standalone--while parsingdotnet runitself and never re-emits it (RunProperties.WithApplicationArguments→CommonRunHelpers.CombineRunArguments).Verified with a probe app that echoes its argv —
dotnet run --devtoolsanddotnet run -- --devtoolsproduce byte-identical results. The separator is unrecoverable, so the targets file cannot tell the two apart.Because the package overrides
RunCommandto an intermediary launcher that has its own options, those tokens land in winapp's option namespace instead of your app's.Fix
One line.
RunArgumentsnow ends with a separator:Everything the SDK appends lands in winapp's passthrough region and reaches your app unchanged.
No CLI change is needed —
winapp runkeeps rejecting unknown options, so a typo likewinapp run . --debug-outptstill fails loudly in a hand-typed invocation.dotnet run --devtools--devtools✅dotnet run -- --devtools--devtools✅dotnet run --detach--detachdotnet run -p:WinAppRunDetach=truewinapp run . --detach(direct CLI)winapp run . --debug-outpt(direct CLI)Options written after
dotnet runused to configure the launcher.dotnet run --detachnow passes--detachto your app instead. Use the properties:-p:WinAppRunDetach=true.MSBuild can't warn about this, because it never sees those tokens — so winapp does. When invoked as the NuGet caller and a forwarded argument matches one of its own options:
Deliberately narrow: unknown arguments stay silent (they never had a winapp meaning, so a notice would be noise on every run), a direct
winapp run . -- --detachstays silent (an explicit request to forward), and attached-value spellings such as--executable=foo.exeare matched on the option name, since those configured winapp before this change too.Note that a forwarded
--jsonor--quietdoes produce the notice: with the separator in place those tokens go to your app rather than putting winapp into JSON/quiet mode, so the notice is accurate. The suppression guard still applies when winapp itself is in that mode (winapp run --json, or-p:WinAppRunArgs="--json").Validation
RunCommandTestspass, including 4 new tests covering each notice case..nupkg._WinAppRunArgsstays separator-free, soRunPackagedAppis provably unaffected.--devtoolsand for direct CLI use, and covers attached-value forms like--executable=foo.exe.Docs updated across
docs/dotnet-run-support.md,docs/usage.md,docs/guides/dotnet.md, the package README, and the frameworks skill.