Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 39 additions & 3 deletions .github/scripts/Apply-PRFinalize.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ Describe 'New-ExclusiveTempFile' {
BeforeAll {
$script:SandboxDir = Join-Path ([System.IO.Path]::GetTempPath()) "apply-prfinalize-tests-$([System.IO.Path]::GetRandomFileName())"
New-Item -ItemType Directory -Path $script:SandboxDir -Force | Out-Null
$script:RealNewItem = Get-Command New-Item -CommandType Cmdlet
$script:OriginalAgentTemp = $env:AGENT_TEMPDIRECTORY
$env:AGENT_TEMPDIRECTORY = $script:SandboxDir
}
Expand Down Expand Up @@ -367,11 +368,24 @@ Describe 'New-ExclusiveTempFile' {
# $script: scope is required — a plain $i++ inside the scriptblock would mutate a
# local copy, so every attempt would re-request the planted name.
$script:ForcedIndex = 0
$script:AttemptedPaths = @()
Mock New-Item {
$script:AttemptedPaths += $Path
& $script:RealNewItem -ItemType $ItemType -Path $Path -ErrorAction Stop
} -ParameterFilter { $ItemType -eq 'File' }

$path = New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' -NameGenerator {
$n = "forced$($script:ForcedIndex)"; $script:ForcedIndex++; $n
}
try {
# It must have skipped the planted path entirely...
# The spy proves New-Item actually attempted the planted path before moving on.
# Generator consumption alone is insufficient: an implementation could generate
# forced0, skip it without calling New-Item, then successfully create forced1.
$script:AttemptedPaths[0] | Should -Be $planted
$script:AttemptedPaths[1] | Should -Be (Join-Path $script:SandboxDir 'pr-finalize-body-123-forced1.md')
$script:ForcedIndex | Should -BeGreaterThan 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ These assertions prove generator consumption and the returned second name, not that 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

$path | Should -Be (Join-Path $script:SandboxDir 'pr-finalize-body-123-forced1.md')

$path | Should -Not -Be $planted
'REPLACEMENT BODY' | Set-Content -LiteralPath $path -Encoding UTF8
# ...so the symlink target is untouched, and the link is still a link.
Expand All @@ -390,10 +404,22 @@ Describe 'New-ExclusiveTempFile' {
New-Item -ItemType SymbolicLink -Path $planted -Target $missingTarget | Out-Null

$script:DangleIndex = 0
$script:AttemptedPaths = @()
Mock New-Item {
$script:AttemptedPaths += $Path
& $script:RealNewItem -ItemType $ItemType -Path $Path -ErrorAction Stop
} -ParameterFilter { $ItemType -eq 'File' }

$path = New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' -NameGenerator {
$n = "dangle$($script:DangleIndex)"; $script:DangleIndex++; $n
}
try {
# As above: pins that the planted path was attempted and skipped, not bypassed.
$script:AttemptedPaths[0] | Should -Be $planted
$script:AttemptedPaths[1] | Should -Be (Join-Path $script:SandboxDir 'pr-finalize-body-123-dangle1.md')
$script:DangleIndex | Should -BeGreaterThan 1
$path | Should -Be (Join-Path $script:SandboxDir 'pr-finalize-body-123-dangle1.md')

$path | Should -Not -Be $planted
'REPLACEMENT BODY' | Set-Content -LiteralPath $path -Encoding UTF8
# Writing through a dangling link would have created the target.
Expand Down Expand Up @@ -452,8 +478,18 @@ Describe 'New-ExclusiveTempFile' {
$env:AGENT_TEMPDIRECTORY = $script:SandboxDir
try {
$script:Calls = 0
{ New-ExclusiveTempFile -Prefix 'missing-dir/nope/body' -NameGenerator { $script:Calls++; 'x' } } |
Should -Throw -ExpectedMessage '*Could not find a part of the path*'
# Assert on the exception *type*, not the message: .NET message strings are
# localized, so matching "Could not find a part of the path" would fail on a
# non-en-US agent. The type is culture-invariant.
$thrown = $null
try {
New-ExclusiveTempFile -Prefix 'missing-dir/nope/body' -NameGenerator { $script:Calls++; 'x' }
} catch {
$thrown = $_.Exception
}

$thrown | Should -Not -BeNullOrEmpty
$thrown | Should -BeOfType ([System.IO.DirectoryNotFoundException])
$script:Calls | Should -Be 1
} finally {
$env:AGENT_TEMPDIRECTORY = $saved
Expand Down
6 changes: 6 additions & 0 deletions .github/scripts/apply-pr-finalize.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,12 @@ function New-ExclusiveTempFile {
for ($attempt = 0; $attempt -lt $MaxAttempts; $attempt++) {
$candidate = Join-Path $baseDir "$Prefix-$(& $NameGenerator).md"
try {
# -Path, not -LiteralPath: New-Item has no -LiteralPath parameter (binding it
# throws ParameterBindingException). For this invocation, a complete leaf path
# without -Name is treated literally, so it cannot resolve onto an existing file.
# New-Item can expand wildcards when -Path is combined with -Name; do not infer
# a general no-globbing guarantee from this call. Reviewers have suggested
# -LiteralPath here twice, but it is not applicable.
$file = New-Item -ItemType File -Path $candidate -ErrorAction Stop
return $file.FullName
} catch [System.IO.DirectoryNotFoundException] {
Expand Down
Loading