Make the PR-finalize symlink tests self-sufficient and drop a localized assertion - #37039
Conversation
Follow-up to #36919, which merged with three optional non-blocking review notes outstanding. Two were real and are fixed here; the third turns out not to be applicable, so it is documented in place rather than acted on. 1. The symlink tests leaned on backstops (kubaflo) He was right, and I reproduced it. A mutant that ignores the -NameGenerator seam and picks its own random name PASSED both symlink tests — only the exhaustion/attempt-count tests caught it. That means the planted symlink was never in the mutant's path, so the skip logic went unexercised: the tests guarding the symlink behaviour could not, on their own, tell a skipping implementation from one that never looked. Both tests now assert the generator advanced past the planted index and that the returned path is the *next* candidate. The seam-ignoring mutant now fails both symlink tests directly, and the original write-through mutant still fails them too. 2. An assertion on a localized .NET message (kubaflo) 'Could not find a part of the path' is a localized resource string and would fail on a non-en-US agent. Now asserts the exception type (DirectoryNotFoundException), which is culture-invariant. Practical risk was low since AzDO agents are en-US, but the type assertion is also more precise about what the test actually cares about. 3. New-Item -Path -> -LiteralPath: not applicable Suggested by the Copilot reviewer (filed low-confidence) and passed through by kubaflo as cosmetic. It is neither: New-Item has no -LiteralPath parameter, so binding it throws ParameterBindingException. The premise is also wrong -- -Path does not glob when creating. Verified both: New-Item -Path 't*rget.md' next to an existing target.md created a file literally named 't*rget.md' and left target.md untouched. Added a comment recording this so it stops being re-raised. Tests: still 40 in Apply-PRFinalize.Tests.ps1 (strengthened, not added to); 102/102 across the three affected suites. Scripts parse clean and the stubbed-gh end-to-end run still reaches --body-file with the right content and cleans up. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40f61a36-c005-42d8-af25-e1228194d196
GitHub Actions
Inactive
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 37039Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37039" |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability and portability of the PR-finalize temp-file hardening tests, and documents a prior review suggestion that is not applicable to New-Item.
Changes:
- Strengthen the symlink-collision Pester tests to prove the
-NameGeneratorseam is actually exercised (and the planted symlink candidate is reached and skipped). - Replace a localized exception message assertion with a culture-invariant exception type assertion (
DirectoryNotFoundException). - Add an in-code note explaining why
New-Item -Pathis used (and why-LiteralPathis not applicable).
Show a summary per file
| File | Description |
|---|---|
.github/scripts/Apply-PRFinalize.Tests.ps1 |
Makes symlink tests self-sufficient by asserting the generator advanced past the planted candidate; switches to exception type assertion to avoid localization fragility. |
.github/scripts/apply-pr-finalize.ps1 |
Adds a clarifying comment documenting why New-Item uses -Path (no -LiteralPath parameter) and why wildcard concerns don’t apply here. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
the
area-infrastructure
GitHub Actions
Inactive
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
@PureWeen — AI-generated adversarial review: independent Claude Opus 5, GPT-5.6 Sol, and Gemini 3.1 Pro passes; cross-pollination; MAUI expert verification; final GPT-5.6 Terra arbitration.
NEEDS_DISCUSSION
At exact head 9ee6132a0a, the production change is comment-only, the typed exception assertion is sound, and the New-Item -Path explanation is correct. The new assertions genuinely kill the seam-ignoring mutant described in the PR.
One low-severity test-contract gap remains inline: the assertions prove generator consumption and the returned second name, but not that New-Item attempted the planted symlink. A skip-first mutant still passes both symlink tests and is caught only by a separate test, so the stated self-sufficiency is not yet complete.
Prior review reconciliation: the #36919 production/security fixes remain intact; this refines only its optional self-sufficiency follow-up. No prior current-PR finding is duplicated.
CI: exact-head Pester (.github/scripts) is green (1499/1499 overall; 40/40 in the changed suite). Required maui-pr was path-filtered/skipped and Build Analysis remains pending, so repository review rules cap confidence at low and prohibit LGTM despite the clean code assessment.
Title/description: the narrower mutation evidence is accurate, but “self-sufficient” overstates what the changed tests directly prove. Confidence: low for merge safety; high in the reproduced inline finding.
| # name also passes — the planted symlink would never have been in its way, so the | ||
| # skip logic goes unexercised and the test leans on the exhaustion test to catch | ||
| # seam-ignoring. Asserting the generator advanced makes this test self-sufficient. | ||
| $script:ForcedIndex | Should -BeGreaterThan 1 |
There was a problem hiding this comment.
New-Item was attempted against $planted. I reproduced a mutant that generates forced0, continues before New-Item, then creates forced1: both symlink tests pass, and only the separate non-collision test fails (39/40). The tests therefore still rely on another backstop. Use a call-through Mock New-Item/spy to record attempted -Path values and assert the first is $planted (and dangle0), or narrow the self-sufficiency claim.
There was a problem hiding this comment.
Fixed in f845978. You were right: generator consumption did not prove New-Item saw the planted path. Both symlink tests now install a call-through Pester Mock New-Item that records attempted paths while invoking the real cmdlet, then assert the planted link was attempt 1 and the next candidate was attempt 2. I replayed your exact skip-first mutant (forced0; continue before New-Item; create forced1): both symlink tests now fail directly (the real suite remains 40/40; all related suites 102/102).
…/pr-finalize-testquality
The existing assertions proved that the name generator advanced past the planted candidate, but not that New-Item actually attempted that candidate. A mutant could consume forced0, continue before New-Item, then create forced1; both symlink tests still passed and relied on an unrelated test as a backstop. Use a call-through Pester mock for New-Item that records each attempted path while preserving the real filesystem behavior. Both symlink tests now assert that the planted link is the first path passed to New-Item and that the next candidate is attempted second. Replayed the reported skip-first mutant: both symlink tests now fail directly. The real suite remains 40/40, and all three related suites pass 102/102. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40f61a36-c005-42d8-af25-e1228194d196
The adversarial review found 2/3 consensus that the existing comment was too broad. New-Item treats a complete leaf -Path literally for this call, but its -Path parameter can expand wildcards when combined with -Name. Document the actual invariant: this invocation passes a complete leaf path and no -Name, so it cannot resolve onto an existing file. Also warn not to infer a general no-globbing guarantee from the call. Tests remain 102/102 across the three related suites. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40f61a36-c005-42d8-af25-e1228194d196
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial review — no remaining findings
Three independent reviewers evaluated the authoritative two-file diff, full source context, MAUI regression/testing rules, and the prior review thread.
- Prior finding: The call-through
New-Itemspy now proves the planted symlink path is actually attempted. The exact skip-first mutant fails both symlink tests directly. ✅ - Consensus finding: One reviewer flagged the
New-Item -Pathcomment as broader than the verified invocation. After the required dispute round, this reached 2/3 consensus and was fixed in91bc8d1e42fby scoping the guarantee to a complete leaf path without-Name. ✅ - Coverage: The changed tests exercise existing and dangling symlinks, exact attempted paths, localized-error portability, exhaustion, attempt count, and fallback behavior.
No unresolved correctness, security, regression, or test-quality findings remain.
Methodology: 3 independent reviewers with adversarial consensus. Review event is COMMENT only; human approval remains required.
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!
Description of Change
Follow-up to #36919. This PR closes the remaining test-contract gaps around
New-ExclusiveTempFile; production behavior is unchanged.Prove
New-Itemactually attempts the planted symlinkThe first version asserted that the deterministic name generator advanced from
forced0toforced1. @kubaflo correctly demonstrated that this did not proveNew-Itemsawforced0: a mutant could consume that name, skip beforeNew-Item, and createforced1. Both symlink tests still passed and relied on a separate test as a backstop.The tests now install a call-through Pester
Mock New-Item. It records each attempted-Pathwhile invoking the real cmdlet, preserving the filesystem behavior under test. Each symlink test asserts:New-Item.The exact reported mutant now fails both symlink tests directly:
Result: 37 passed, 3 failed, including both symlink tests. The real implementation remains 40/40.
Assert a culture-invariant exception contract
The missing-directory test previously matched the localized text
Could not find a part of the path. It now captures the exception and assertsSystem.IO.DirectoryNotFoundException, which is both culture-invariant and more precise about the contract being tested.Document the actual
New-Item -PathinvariantNew-Itemhas no-LiteralPathparameter, so replacing-Pathwould throwParameterBindingException. For this invocation, the helper supplies a complete leaf path and does not pass-Name; that path is treated literally and cannot glob onto an existing file.A three-reviewer adversarial pass found 2/3 consensus that the original comment stated this too broadly:
New-Item -Path ... -Name ...can expand wildcards across matching directories. The comment now scopes the guarantee to this exact invocation and explicitly warns not to infer a general no-globbing guarantee.Issues Fixed
Follow-up to #36919; no separate issue.
Testing
Apply-PRFinalize.Tests.ps1: 40/40Apply-PRFinalize.Tests.ps1,Review-PR.Tests.ps1, andPost-AISummaryComment.Tests.ps1: 102/102maui-pris path-filtered for script-only changes, so the Pester suites are the meaningful validation for this PR.