Fix --uploadTimeout defaulting to 0 when the option is omitted

[Tanmen (tanmen)]

Fixes #162.

Problem

UploadTimeoutOption declares a CustomParser that returns 100 for the empty-token case, but no DefaultValueFactory. System.CommandLine only invokes CustomParser when the option is present on the command line, so omitting it entirely yields default(long) — zero.

That zero reaches AzureBlobManager.UploadFileAsync:

blobClientOptions.Retry.NetworkTimeout = TimeSpan.FromSeconds(uploadTimeout);

TimeSpan.FromSeconds(0) cancels every request the instant it starts, so msstore publish fails at Uploading Bundle to Azure blob: 0% after six retries — on every invocation that does not pass --uploadTimeout explicitly.

Fix

Add DefaultValueFactory = _ => DefaultUploadTimeoutSeconds so the documented 100-second default applies when the option is absent.

Worth noting: the CustomParser's empty-token branch can never cover that case. The option takes exactly one argument, so writing --uploadTimeout without a value is a parse error, not an empty-token parse. One of the new tests pins that behaviour so the two paths stay distinguishable.

I also lifted the 100 / 100000 literals into named constants, so the default, the range check and the error message cannot drift apart. Happy to drop that part if you would rather keep the diff to the single line.

Tests

8 new cases in PublishCommandUnitTests:

• the option omitted → 100 (this is the regression)
• the option present without a value → parse error
• 100, 300, 100000 → used as given
• 99, 100001, not-a-number → parse error

Verified the regression test actually catches the bug: with the DefaultValueFactory line removed, PublishCommandUploadTimeoutShouldDefaultWhenOptionIsOmitted fails; with it, all 8 pass.

The suite has 10 pre-existing failures on my machine (MSBuild / WinUI / settings related). They are identical with and without this change — 138 tests / 10 failures before, 146 tests / 10 failures after.

Why this was hard to spot

The console shows only Error while uploading the application package. — the exception goes to logger.LogError(ex, ...) while ansiConsole.WriteLine gets a bare sentence, so it reads as a network or service problem. The 0:00:00 only appears with --verbose. It cost several release runs before that was visible.

A guard rejecting a non-positive uploadTimeout in AzureBlobManager.UploadFileAsync would make this self-explanatory if it ever regresses. I left it out to keep this focused, but happy to add it.

[Tanmen (tanmen)]

@microsoft-github-policy-service agree

[isourabh]

can you add asset on the error message containing some text ?

[isourabh]

This test means that

if (result.Tokens.Count == 0)
                    {
                        return DefaultUploadTimeoutSeconds;
                    }

is not reached so we should remove that is that code is never reachable

[Tanmen (tanmen)]

Done. This one now asserts the error is the parser's own missing-value error rather than just being non-empty. It only matches on the option name — the rest of that message comes from System.CommandLine and is localized, so asserting the full English string fails on a non-English machine (it came back in Japanese on mine). It also asserts the message is not the range error, which is what makes it evidence for the point below.

🤖 Addressed by Claude Code

[Tanmen (tanmen)]

You are right — removed.

I checked it against System.CommandLine 2.0.10 rather than inferring it from the test, and the branch is dead for two independent reasons:

1. Arity. ArgumentArity.Default only returns ZeroOrOne for a non-boolean, non-collection argument when parent is Command. An option's argument has the Option as its parent, so it falls through to ExactlyOne — a value-less --uploadTimeout is rejected by the parser before CustomParser runs.
2. Omitted option. In ArgumentResult.ValidateAndConvert, the Argument.HasDefaultValue && Parent.UseDefaultValueFor(this) check returns the DefaultValueFactory value and returns before Argument.ConvertArguments (the CustomParser) is reached. So omitting the option never enters the parser either.

Confirmed empirically too: with the branch gone, result.Tokens.Single() would throw on an empty token list, and the omitted-option test passes rather than throwing.

I kept ShouldRequireAValueWhenTheOptionIsPresent, since it is the thing that demonstrates the branch is unreachable — and left a comment on the option noting that a "no tokens" branch has to come back if the arity is ever relaxed to ZeroOrOne.

🤖 Addressed by Claude Code

[isourabh]

same here, can you add asset on the error message containing some text?

[Tanmen (tanmen)]

Done. This now asserts the exact message: Invalid seconds value. The value must be between 100 and 100000. It is built from MinUploadTimeoutSeconds/MaxUploadTimeoutSeconds rather than hard-coded, so the test follows the constants if the range is ever changed. Both constants are now internal for that reason.

🤖 Addressed by Claude Code

[isourabh]

is this code reachable ?

[Tanmen (tanmen)]

No, it is not reachable — removed in 799a67a.

The option's arity is ExactlyOne (ArgumentArity.Default only relaxes to ZeroOrOne when the argument's parent is a Command, which is not the case for an option's argument), so --uploadTimeout with no value is a parse error before CustomParser runs. And when the option is omitted entirely, ArgumentResult.ValidateAndConvert returns the DefaultValueFactory value before it ever reaches ConvertArguments. Either way the parser is never called with zero tokens.

Note this branch was also not what made the omitted case work — that is exactly the bug this PR fixes: without a DefaultValueFactory the omitted case never reached this code at all and yielded default(long) = 0. Verified by removing the factory locally and watching the omitted-option test fail with found 0L.

🤖 Addressed by Claude Code

[Alexandre Zollinger Chohfi (azchohfi)]

Approving — I reproduced the bug and validated the fix locally.

Reverting PublishCommand.cs to the merge-base makes PublishCommandUploadTimeoutShouldDefaultWhenOptionIsOmitted fail with found 0L, and it passes on HEAD, so the regression test genuinely pins the bug. Full suite on net10.0: 146 total, 136 passed, 10 skipped (OS-conditional), 0 failed. Release build with TreatWarningsAsErrors is clean.

The root-cause analysis is accurate — System.CommandLine 2.0.10 doesn't invoke CustomParser for an absent option, so the old Tokens.Count == 0 branch was dead and default(long) flowed into BlobClientOptions.Retry.NetworkTimeout. Removing that branch is the right call, and the comment explaining why it's gone (plus the arity caveat) is worth keeping. Nice bonus that UploadTimeoutOption is a shared static, so msstore init --publish gets the fix too.

Two non-blocking notes:

1. Sibling options still carry the same dead branch. PackageRolloutPercentageOption and InputDirectoryOption both still have unreachable Tokens.Count == 0 branches. They're harmless today because default(T) is null for both, and downstream treats packageRolloutPercentage == null as "no rollout" (IStorePackagedAPIExtensions.cs:487). Mostly flagging it so a later cleanup doesn't "fix" them by mirroring this PR — adding DefaultValueFactory = _ => 100f to the rollout option would silently force 100% rollout on every publish.

2. GetRequiredValue would be more idiomatic now. With a DefaultValueFactory in place, PublishCommand.cs:170 and InitCommand.cs:168 could use GetRequiredValue the way NoCommitOption already does. GetValue works correctly, so this is purely a consistency nit.