Validate TerminalOptions columns and rows are positive - #18992
Validate TerminalOptions columns and rows are positive#18992Karol Zadora-Przylecki (karolz-ms) merged 3 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18992Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18992" |
github-actions
Bot
added
the
area-app-model
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Adds fail-fast validation for terminal dimensions, aligning AppHost configuration with TerminalHost requirements.
Changes:
- Rejects zero or negative terminal columns and rows.
- Documents constraints and adds boundary/regression tests.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting/ApplicationModel/TerminalAnnotation.cs |
Validates terminal dimensions in property setters. |
tests/Aspire.Hosting.Tests/WithTerminalTests.cs |
Tests invalid dimensions and boundary value 1. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
TerminalOptions.Columns and Rows previously accepted zero or negative values during AppHost model construction, but Aspire.TerminalHost rejects any dimension below 1 (its --columns / --rows validators). That mismatch turned an invalid value into a hidden terminal-host startup failure that could block the parent resource instead of a clear error at the call site. Validate both dimensions in the property setters using ArgumentOutOfRangeException.ThrowIfNegativeOrZero, matching the existing ProcessCommandOptions.MaxOutputLineCount convention. This surfaces the error at the WithTerminal() call site. The polyglot dispatcher uses the built-in defaults (120x30), which remain valid. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: dc0094eb-cce9-48f8-ad4d-304d8d87f48a
Keep the public TerminalOptions documentation focused on the positive-value contract after terminal implementation types moved internal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: dc0094eb-cce9-48f8-ad4d-304d8d87f48a
22785a2 to
498439e
Compare
This comment has been minimized.
This comment has been minimized.
…erminal-dimensions
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. 48 / 100 test projects · 4 jobs, from 2 changed files. Selected test projects (48 / 100)
Selected jobs (4)
How these were chosen — grouped by what changed
🔧 show 43
🧪 📦 affected project Job reasons
Selection computed for commit |
PR Testing ReportPR Information
Artifact Version Verification
Changes AnalyzedFiles Changed
Change Categories
Test Scenarios ExecutedScenario 1: PR artifact and package verificationObjective: Verify the tested CLI and template packages were produced from the latest PR head. Steps:
Evidence:
Observations:
Scenario 2: Default and minimum terminal dimensionsObjective: Verify default Steps:
Evidence:
Observations:
Scenario 3: Zero terminal columnsObjective: Verify zero columns fail during AppHost model construction rather than later in TerminalHost startup. Steps:
Evidence:
Expected Unhappy-Path Outcome: A clear Observations:
Scenario 4: Negative terminal rowsObjective: Verify negative rows fail during AppHost model construction rather than later in TerminalHost startup. Steps:
Evidence:
Expected Unhappy-Path Outcome: A clear Observations:
Scenario 5: Focused source regression testsObjective: Verify invalid values, defaults, the accepted lower boundary, and the internal terminal implementation shape. Steps:
Evidence:
Observations:
GitHub CI Status
Summary
Overall Result✅ PR VERIFIED The PR artifact matches the latest commit, valid terminal dimensions preserve existing behavior, invalid dimensions fail at AppHost model construction with clear exceptions, focused regression tests pass, and all GitHub checks have completed without failures. |
|
Documentation was required for this PR ( See the workflow run for details: https://github.com/microsoft/aspire/actions/runs/30958805880 |
Description
WithTerminal()lets you attach an interactive terminal to a resource and optionally set the initial PTY size viaTerminalOptions.Columns/TerminalOptions.Rows. Previously those properties accepted zero or negative values during AppHost model construction, butAspire.TerminalHostrejects any dimension below1(its--columns/--rowsvalidators). The mismatch turned a bad value into a hidden terminal-host startup failure that could block the parent resource, with no clear indication of what went wrong.Now an invalid width/height fails fast at the
WithTerminal()call site with a clearArgumentOutOfRangeException, instead of surfacing later as an opaque host failure.User-facing usage
Valid usage is unchanged:
Invalid dimensions now throw immediately at configuration time:
The boundary value
1is still accepted (matches TerminalHost's>= 1rule). The polyglotwithTerminaldispatcher uses the built-in defaults (120×30), which remain valid, so polyglot AppHosts are unaffected.Implementation
TerminalOptions.ColumnsandRowsvalidate in their setters withArgumentOutOfRangeException.ThrowIfNegativeOrZero, matching the existingProcessCommandOptions.MaxOutputLineCountconvention. No generatedapi/*.csfiles were modified (this is an experimental API and did not add new surface). Regression tests were added inWithTerminalTestscovering zero/negative columns and rows and the accepted boundary value; they fail before the fix and pass after.Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?