Skip to content

fix: Validate max value in ProgressTask to ensure it is zero or greater - #2170

Open
Rain6435 wants to merge 1 commit into
spectreconsole:mainfrom
Rain6435:progress-maxvalue-bug
Open

fix: Validate max value in ProgressTask to ensure it is zero or greater#2170
Rain6435 wants to merge 1 commit into
spectreconsole:mainfrom
Rain6435:progress-maxvalue-bug

Conversation

@Rain6435

Copy link
Copy Markdown

Fixes #2167

  • I have read the Contribution Guidelines
  • I have checked that there isn't already another pull request that solves the above issue
  • All newly added code is adequately covered by tests
  • All existing tests are still running without errors

Changes

This change puts maxValue validation in ProgressTask at the exact points where invalid values can enter task state, so the invariant is enforced centrally instead of at call sites.

Constructor guard in ProgressTask.cs:142: validates initial maxValue when a task is created. This covers all AddTask overloads because they all flow into ProgressTask construction through ProgressContext internals.
Update-path guard in ProgressTask.cs:231: validates later changes via task.MaxValue setter, which routes through Update(maxValue: ...). This prevents a task from becoming invalid after successful creation.
Regression tests in ProgressTests.cs:142 and ProgressTests.cs:167: one test verifies creation-time rejection (ctx.AddTask(..., maxValue: -1)), and one verifies mutation-time rejection (task.MaxValue = -1), so both entry points are locked down.
Putting validation in these two locations ensures consistent runtime behavior for every API path without duplicating checks across ProgressContext overloads or user call sites.


Please upvote 👍 this pull request if you are interested in it.

@Rain6435

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@patriksvensson

Copy link
Copy Markdown
Contributor

Thanks for your contribution!

A question: Wouldn't having a max value of -1 and a min value of -10 be a valid configuration?

@Rain6435

Rain6435 commented Jul 24, 2026

Copy link
Copy Markdown
Author

@patriksvensson Good question. In ProgressTask we don’t have a MinValue concept, only Value and MaxValue, so a configuration like max=-1/min=-10 is outside the current model. Indeterminate progress is represented explicitly via IsIndeterminate, and completion is Value >= MaxValue, which means max=-1 would mark a new task as finished immediately. That’s why this change enforces MaxValue >= 0 and keeps indeterminate behavior on the dedicated IsIndeterminate path. Also, this is intuitive way of usage of the ProgressTask.

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.

ProgressTask accepts negative MaxValue without validation

2 participants