Remove duplicate Empty/empty template parameters causing wrong .NET version selection#34876
Remove duplicate Empty/empty template parameters causing wrong .NET version selection#34876
Conversation
…ersion selection The maui-blazor and maui-blazor-solution templates defined both `Empty` and `empty` as separate JSON keys. The .NET 11 SDK (preview.3+) has stricter case-insensitive duplicate key handling, which caused the template engine to not match the .NET 11 template when `--Empty` was used, falling back to the .NET 10 template. Fixes #34873 Agent-Logs-Url: https://github.com/dotnet/maui/sessions/889ee180-333b-4280-8a09-50e09ab2d591 Co-authored-by: jfversluis <939291+jfversluis@users.noreply.github.com>
|
@copilot can you determine where the duplicate empty parameter originates from? Why was it introduced? |
The duplicate Context: In .NET 9, the CLI option was The PR description even noted: "hopefully in .NET 11 we can start removing the duplicates since they are hidden." Unfortunately, .NET 11 Preview 3's template engine (via dotnet/templating#10049) introduced stricter case-insensitive duplicate key handling, which means these two keys ( |
…, security hardening (#34678) ## Description Overhauls the `copilot-evaluate-tests` gh-aw workflow — switches to on-demand triggers only (`/evaluate-tests` slash command + manual `workflow_dispatch`), adds security hardening, and improves error handling. No auto-runs on PR create/update. ### What Changed **Triggers (on-demand only — no auto-runs)** - Add `slash_command: evaluate-tests` — comment `/evaluate-tests` on a PR to trigger - Keep `workflow_dispatch` — manual trigger from Actions tab with PR number input - Disable `pull_request_target` — no auto-evaluation on PR create/update - Add `bots: ["copilot-swe-agent[bot]"]` — Copilot-authored PRs can be evaluated - Add `labels: ["pr-review", "testing"]` — workflow runs are labeled **Gate step (fast-fail for invalid requests)** - Check PR is OPEN before evaluating (rejects closed/merged PRs with clear message) - Check for test source files in diff before spinning up agent - Fall back to REST API for PRs with 300+ files (where `gh pr diff` returns HTTP 406) - All API errors surfaced with clear messages — no silent masking - `exit 1` stops the workflow immediately — no wasted agent compute **Access gating (`Checkout-GhAwPr.ps1`)** - Reject fork PRs (`isCrossRepository` check) - Verify PR author has write access (admin/write/maintain roles) - Fix `ConvertFrom-Json` ordering — check exit code before JSON parsing - Make infrastructure restore fatal on failure (was soft warning) - Remove pre-delete pattern — `git checkout` overwrites in-place **Workflow improvements** - `hide-older-comments: true` — previous evaluations auto-collapse - `report-as-issue: false` for noop — no issue created when nothing to evaluate - Timeout bumped 15 → 20 minutes - Dry-run mode via `suppress_output` input (workflow_dispatch only) - `Gather-TestContext.ps1` now receives `-PrNumber` parameter **Security documentation (`gh-aw-workflows.instructions.md`)** - Add "Before You Build" anti-patterns table — prefer built-in gh-aw features - Add Security Boundaries section with defense layers table - Add Rules for gh-aw Workflow Authors (DO/DON'T list) - Document `COPILOT_TOKEN` exposure and mitigations - Add `slash_command` to fork behavior table - Update `Checkout-GhAwPr.ps1` description to match current behavior ### Trigger Behavior | Trigger | When it fires | Who can trigger | |---------|---------------|-----------------| | `/evaluate-tests` comment | Comment on a PR | Write-access collaborators + copilot-swe-agent[bot] | | `workflow_dispatch` | Actions tab → "Run workflow" → enter PR number | Write-access collaborators | | ~~`pull_request_target`~~ | ~~Auto on PR create/update~~ | ~~Disabled~~ | ### Security Model Based on [GitHub Security Lab guidance](https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/): - PR contents treated as **passive data** (read/analyze, never built or executed) - Agent runs in **sandboxed container** with `GITHUB_TOKEN` and `gh` CLI scrubbed - Write operations in **separate `safe_outputs` job** (not the agent) - Agent output limited to `max: 1` comment via safe-outputs - `Checkout-GhAwPr.ps1` rejects fork PRs and verifies write access before checkout - Infrastructure restore is fatal on failure — prevents running with untrusted infra ### Validation | Test | PR | Result | |------|-----|--------| | Open PR with tests | #34983 | ✅ Full success (gate → checkout → agent → comment) | | No-test PR | #34876 | ✅ Gate fast-fail ("no test source files") | | Merged PR | #34932 | ✅ Gate fast-fail ("MERGED — skipping") | ### Known Limitations - Fork PRs via `/evaluate-tests` can supply modified `.github/skills/` — accepted residual risk (agent sandboxed, output bounded). Tracked as [gh-aw#18481](github/gh-aw#18481) - `exit 1` in gate step shows ❌ in GitHub checks for no-test/closed PRs — intentional (no built-in "skip" mechanism in gh-aw steps) - `pull_request_target` commented out — can be re-enabled later for auto-evaluation --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🤖 AI Summary
📊 Review Session —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34876 | Remove duplicate empty symbol; simplify SampleContent expression |
maui-blazor/template.json, maui-blazor-solution/template.json |
Original PR |
🔬 Code Review — Deep Analysis
Code Review — PR #34876
Independent Assessment
What this changes: Both maui-blazor and maui-blazor-solution had duplicate symbols Empty (PascalCase) + empty (lowercase) — identical in type, datatype, default, and description. The PR removes the lowercase empty symbol and simplifies SampleContent from (!Empty && !empty && isCLI) to (!Empty && isCLI).
Inferred motivation: The .NET 11 template engine enforces case-insensitive uniqueness of symbol names. Two symbols mapping to the same case-insensitive name (empty/Empty) causes the engine to reject/mis-match the template, falling back to the .NET 10 template — generating net10 projects instead of net11.
Reconciliation with PR Narrative
Author claims: dotnet new maui-blazor --Empty on .NET 11 Preview 3 generates a net10 project; --empty works. Root cause: duplicate case-insensitively-equivalent keys causing the .NET 11 engine to fall back to the .NET 10 template.
Agreement: The root cause analysis is correct and the fix is the minimal correct approach.
Logic Equivalence Verification
| Scenario | Old result (CLI host) | New result (CLI host) |
|---|---|---|
| No flags passed | !false && !false = true (include) |
!false = true ✓ |
--empty or --Empty |
!true && !false or !false && !true = false (exclude) |
!true = false ✓ |
The simplification is logically equivalent. The template engine maps both --empty and --Empty to the single Empty symbol via case-insensitive resolution.
Do Other Templates Need This Fix?
No. Only maui-blazor and maui-blazor-solution defined both Empty and empty. The maui-mobile template uses only IncludeSampleContent. All other templates have neither parameter.
Findings
⚠️ Warning — Blazor integration tests failing on CI
maui-pr (Run Integration Tests Blazor macOS) and maui-pr (Run Integration Tests Blazor windows) are both red. These jobs directly test maui-blazor and maui-blazor-solution — the exact templates changed by this PR. Run Integration Tests Build macOS/Windows are also failing. Without detailed logs, it's unclear whether these are pre-existing infrastructure failures or regressions from removing empty. The PR author should confirm before merge.
💡 Suggestion — Redundant triple-nesting in SampleContent value (both files)
Pre-existing but touched in this diff: (((IncludeSampleContent && ...))) has two unnecessary outer parens. Cosmetic only, but the PR is already editing this line.
Devil's Advocate
- The
Build macOSfailure is broad and may cascade to Blazor — could be pre-existing. But Blazor-specific jobs failing on a Blazor template PR needs confirmation. - Logic equivalence is confident — independently verified all
Empty/emptycombinations. - "No other templates" finding is high-confidence (grep across all template.json confirmed).
Verdict: NEEDS_DISCUSSION
Confidence: medium
Summary: The code fix is logically correct and well-scoped. No other templates need the same change. The blocker is the failing Blazor integration CI jobs — they test the exact templates modified here, and the PR cannot be safely LGTM'd until a reviewer confirms those failures are pre-existing infrastructure issues and not regressions from removing the empty symbol.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34876 | Remove duplicate empty (lowercase) symbol; simplify SampleContent to !Empty |
✅ PASS (Gate: build-only) | 2 template.json | Original PR fix |
| 1 | try-fix-1 (claude-opus-4.6) | Remove Empty (PascalCase), keep empty (lowercase); SampleContent references !empty |
✅ PASS | 2 template.json | Opposite of PR fix — keeps lowercase, which is CLI convention |
| 2 | try-fix-2 (claude-sonnet-4.6) | Keep both symbols; convert empty from parameter to computed type with value (Empty) |
✅ PASS | 2 template.json | Non-removal approach; risk: unverified runtime behavior of "(Empty)" computed reference |
| 3 | try-fix-3 (gpt-5.3-codex) | Keep PR's template fix; add --Empty PascalCase test rows to integration tests |
✅ PASS | 2 test files | Test-focused; does not change template logic, only adds regression coverage |
| 4 | try-fix-4 (gpt-5.2) | PR fix + simplified SampleContent (remove triple-nesting) + --Empty PascalCase tests + $comment for maintainability |
✅ PASS | 2 template.json + 2 test files | Most comprehensive: fixes bug + improves expression readability + adds coverage |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | NO NEW IDEAS | All fix strategies explored (remove-lowercase, remove-uppercase, computed-alias, tests-only, combined) |
| claude-sonnet-4.6 | 2 | NO NEW IDEAS | The longName/shortName binding approach was considered in Attempt 2 exploration; no new directions |
| gpt-5.3-codex | 2 | NO NEW IDEAS | Attempt 4 already combines template + tests |
| gpt-5.2 | 2 | NO NEW IDEAS | Space exhausted |
Exhausted: Yes
Selected Fix: try-fix-4 (combined fix + simplified expression + regression tests) — most comprehensive approach, directly addresses the bug AND adds the missing test coverage for the exact failure scenario
📋 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | 2 template.json files changed, no test files |
| Code Review | NEEDS_DISCUSSION (medium) | 0 errors, 1 warning (CI), 1 suggestion (cosmetic) |
| Gate | No tests detected in this PR | |
| Try-Fix | ✅ COMPLETE | 4 attempts, all 4 passing |
| Report | ✅ COMPLETE |
Code Review Impact on Try-Fix
The code review found no ❌ Errors — verdict was NEEDS_DISCUSSION due to Blazor integration CI failures that need confirmation. The code-review --Empty PascalCase regression tests directly shaped all 4 try-fix attempts: Attempts 3 and 4 both focused on adding that coverage; the pr-plus-reviewer candidate was constructed by applying the expert reviewer's explicit feedback.
Summary
PR #34876 removes the duplicate "empty" (lowercase) symbol from maui-blazor and maui-blazor-solution template.json files, resolving a .NET 11 template engine regression where stricter case-insensitive key enforcement causes the engine to fall back to the .NET 10 template when --Empty (PascalCase) is used.
The core fix is correct and minimal. However, the PR has no regression tests for the exact failure scenario (--Empty PascalCase), and the existing test suite only covers --empty (lowercase). Adding --Empty test rows directly prevents future regressions from this class of issue.
Request changes: Author should add [InlineData] rows for --Empty (PascalCase) to SimpleTemplateTest.cs and BlazorTemplateTest.cs. The pr-plus-reviewer candidate (PR fix + minimal expert-recommended test additions) is the recommended path.
Root Cause
The .NET 11 SDK introduced stricter case-insensitive duplicate symbol enforcement in the template engine (dotnet/templating#10049). Both maui-blazor and maui-blazor-solution defined Empty (PascalCase) and empty (lowercase) as separate "parameter" symbols, which are now treated as duplicate CLI parameter registrations. The conflict causes the engine to skip the .NET 11 template entirely and fall back to the .NET 10 template, resulting in generated projects targeting net10.
Fix Quality
The PR's template fix is logically sound:
- Removing the
empty(lowercase) symbol leaves onlyEmpty(PascalCase), which aligns with MAUI template conventions (IncludeSampleContent,AllInteractive, etc. are all PascalCase) - The
SampleContentsimplification from!Empty && !empty && isCLIto!Empty && isCLIis logically equivalent (both symbols were always in sync) - No other templates require the same fix
What's missing: Regression test coverage for --Empty (PascalCase). The existing tests pass --empty (lowercase), which would have passed even before this fix. The pr-plus-reviewer candidate adds [InlineData(..., "--Empty", ...)] rows to both test files, covering exactly the scenario that triggered this bug.
Candidate Comparison
| Candidate | Template Fix | Test Coverage | Risk |
|---|---|---|---|
pr |
✅ Removes empty, keeps Empty |
❌ No --Empty tests |
Low |
pr-plus-reviewer |
✅ Same as PR | ✅ Adds --Empty rows (minimal) |
Low |
try-fix-1 |
✅ Removes Empty, keeps empty (opposite) |
❌ None | Low, but inconsistent with conventions |
try-fix-2 |
Converts empty to computed alias |
❌ None | Medium (unverified runtime behavior) |
try-fix-3 |
❌ Template already fixed by PR | ✅ Adds --Empty rows |
Low |
try-fix-4 |
✅ + simplified expression + $comment | ✅ Comprehensive (all interactivity modes) + TFM assertions | Medium ($comment field, broad TFM assertion) |
Selected Fix: pr-plus-reviewer — the PR's correct template fix with the expert reviewer's minimal test additions. Simpler and safer than try-fix-4 while fully addressing the regression coverage gap.
MauiBot
added
s/agent-changes-requested
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
dotnet new maui-blazor --Emptyon .NET 11 Preview 3 generates a project targeting net10 instead of net11.--empty(lowercase) works correctly. Root cause: the template engine in .NET 11 SDK has stricter case-insensitive duplicate key handling, and both templates define"Empty"and"empty"as separate JSON keys.When the duplicate keys cause the .NET 11 template to fail matching, the engine falls back to the .NET 10 template.
Changes
maui-blazor/.template.config/template.json— Remove duplicate"empty"parameter; simplifySampleContentcomputed value to reference onlyEmptymaui-blazor-solution/.template.config/template.json— Same fixThe template engine already matches CLI parameters case-insensitively, so a single
Emptyparameter handles both--Emptyand--empty.