diff --git a/Reports/README.md b/Reports/README.md index 5f6a790..1312ee3 100644 --- a/Reports/README.md +++ b/Reports/README.md @@ -39,11 +39,22 @@ feeding it get run: ./scripts/Assert-PfbDerivedArtifacts.ps1 -Artifact Reports/PfbDeadKeyReport.json ``` -Add `-KeepWorkDirectory` to keep the regenerated output on disk: when the check reports a -difference, that output is by definition what the comparison demanded, so copying it over the -committed copies is the reliable fix. The gate pins its spec set to `Data/PfbCapabilityMap.json`'s -own `generatedFrom` list rather than reading whatever `tools/specs/` happens to hold, so a -newly published REST version does not show up as artifact drift. +When the check reports a difference, the output that run produced is by definition what the +comparison demanded, so writing it over the committed copies is the reliable fix. `-UpdateCommitted` +does exactly that and then exits 0, listing the repo-relative path of every file it wrote: + +```powershell +./scripts/Assert-PfbDerivedArtifacts.ps1 -Artifact Reports/PfbDeadKeyReport.json -UpdateCommitted +``` + +Only artifacts the run found stale are written, so `git status` afterwards names exactly what +moved. Nothing turns the switch on implicitly — without it the gate never touches the working +tree, which is what lets the same script serve as the CI check. To inspect the regenerated output +before overwriting anything, use `-KeepWorkDirectory` instead and copy the files by hand. + +The gate pins its spec set to `Data/PfbCapabilityMap.json`'s own `generatedFrom` list rather than +reading whatever `tools/specs/` happens to hold, so a newly published REST version does not show +up as artifact drift. | File | Answers | Generated by | |---|---|---| diff --git a/Tests/AssertDerivedArtifacts.Tests.ps1 b/Tests/AssertDerivedArtifacts.Tests.ps1 index 200e077..3a0adeb 100644 --- a/Tests/AssertDerivedArtifacts.Tests.ps1 +++ b/Tests/AssertDerivedArtifacts.Tests.ps1 @@ -175,11 +175,61 @@ Describe 'Assert-PfbDerivedArtifacts parameter contract' { $names | Should -Contain 'KeepWorkDirectory' } + It 'exposes -UpdateCommitted as a switch, so remediating is opt-in and takes no value' { + # A [switch], specifically. As a [bool] it would bind positionally and by conversion -- + # -UpdateCommitted $false still enters the branch under a truthy conversion mistake, and + # a stray positional argument could set it. The whole safety property here is that + # nothing turns this on except a caller typing it. + $parameter = @($gateAst.ParamBlock.Parameters | + Where-Object { $_.Name.VariablePath.UserPath -eq 'UpdateCommitted' }) + + $parameter.Count | Should -Be 1 -Because 'the remediation switch is the documented fix path in the stale-artifact message' + $parameter[0].StaticType.Name | Should -Be 'SwitchParameter' + $parameter[0].DefaultValue | Should -BeNullOrEmpty -Because 'a default that enabled writing would make the gate rewrite the working tree on an ordinary check' + } + It 'declares PowerShell 7, since the generators it invokes require it' { $gateSource | Should -Match '#Requires -Version 7\.0' } } +Describe 'Assert-PfbDerivedArtifacts remediation semantics' { + + It 'writes into the repository only underneath the -UpdateCommitted guard' { + # The load-bearing property of this gate is that checking does not mutate. There are two + # Copy-Item calls in the script: one stages specs into the scratch tree, one overwrites a + # committed artifact. Only the second may run unconditionally-never. Asserted by + # containment rather than by grepping for the switch name, because a Copy-Item moved one + # brace out of the guard still sits a few lines from the word "UpdateCommitted" and would + # keep a text match green while the gate silently rewrote Data/ on every run. + $guard = @($gateAst.FindAll({ + param($node) + $node -is [System.Management.Automation.Language.IfStatementAst] + }, $true) | Where-Object { $_.Clauses[0].Item1.Extent.Text -match '\$UpdateCommitted' }) + + $guard.Count | Should -Be 1 -Because 'the remediation branch must be a single identifiable guard, not scattered conditionals' + + $writes = @($gateAst.FindAll({ + param($node) + $node -is [System.Management.Automation.Language.CommandAst] -and + $node.GetCommandName() -eq 'Copy-Item' + }, $true) | Where-Object { $_.Extent.Text -match '\$committedPath' }) + + $writes.Count | Should -Be 1 -Because 'exactly one write targets a committed artifact; a second would need its own review' + $writes[0].Extent.StartOffset | Should -BeGreaterThan $guard[0].Extent.StartOffset + $writes[0].Extent.EndOffset | Should -BeLessThan $guard[0].Extent.EndOffset + } + + It 'reports the updated artifacts by repo-relative path' { + # The printed list is the handoff to git: a reader pastes those paths into git diff or + # git add. Absolute scratch paths would look identical in the output and be useless in + # both commands, which is why the report is built from $_.Artifact -- the plan's + # repo-relative key -- rather than from the Copy-Item destination. + $gateSource | Should -Match '\$updatedNames\s*=\s*@\(\$updated \| ForEach-Object \{ \$_\.Artifact \}' + $gateSource | Should -Match 'git diff --stat --' + } +} + Describe 'Assert-PfbDerivedArtifacts comparison semantics' { It 'normalizes CRLF to LF before hashing' { diff --git a/scripts/Assert-PfbDerivedArtifacts.ps1 b/scripts/Assert-PfbDerivedArtifacts.ps1 index e05302b..1df1311 100644 --- a/scripts/Assert-PfbDerivedArtifacts.ps1 +++ b/scripts/Assert-PfbDerivedArtifacts.ps1 @@ -26,8 +26,12 @@ run while passing in CI -- a gate that is wrong in the direction that trains people to ignore it. - THE WORKING TREE IS NEVER TOUCHED. Regeneration goes to a scratch directory; a gate that - rewrites Data/ as a side effect of checking it is not a gate. + THE WORKING TREE IS NEVER TOUCHED unless -UpdateCommitted is passed. Regeneration goes to a + scratch directory; a gate that rewrites Data/ as a side effect of checking it is not a gate. + The objection there is to the side effect, not to the write: -UpdateCommitted is an explicit + request to remediate, so a caller who passes it has asked for exactly the thing the default + withholds. Nothing infers it -- no environment variable, no CI default -- because the value + of the default is that a check stays a check. Lives in scripts/ rather than tools/ because the workflows call scripts/ (see scripts/Publish-Gallery.ps1, scripts/Invoke-PfbCiPester.ps1) and because a developer must @@ -50,17 +54,33 @@ .PARAMETER KeepWorkDirectory Leave the scratch tree in place and print its path, so a reported difference can be diffed by hand instead of being re-derived. +.PARAMETER UpdateCommitted + Overwrite each stale committed artifact with this run's regenerated copy, then exit 0 and + list what changed. Without it, a stale artifact is reported and the caller is told to copy + the files out of the scratch tree by hand -- the same copy, done manually. + + Only artifacts this run found stale are written, and only within -Artifact when that + narrows the set: an up-to-date artifact is left alone rather than rewritten byte-identically, + so `git status` after the run names exactly what moved. + + Exit code is 0 on a successful update. Remediating is what was asked for, so doing it is + success -- and a non-zero exit from a run that just fixed the problem breaks any caller + chaining on it. Callers that want the unremediated answer are the ones that must not pass + this switch. .EXAMPLE ./scripts/Assert-PfbDerivedArtifacts.ps1 .EXAMPLE ./scripts/Assert-PfbDerivedArtifacts.ps1 -Artifact Data/PfbCapabilityMap.json -KeepWorkDirectory +.EXAMPLE + ./scripts/Assert-PfbDerivedArtifacts.ps1 -Artifact Reports/PfbDeadKeyReport.json -UpdateCommitted #> [CmdletBinding()] param( [string]$SpecsDirectory, [string]$WorkDirectory, [string[]]$Artifact, - [switch]$KeepWorkDirectory + [switch]$KeepWorkDirectory, + [switch]$UpdateCommitted ) $ErrorActionPreference = 'Stop' @@ -326,6 +346,35 @@ try { $results.Add([pscustomobject]@{ Artifact = $relative; State = $state; Detail = $detail }) } + # --- Remediate ------------------------------------------------------------------------ + + # Done before the report so one pass of output describes what is now true on disk, rather + # than printing STALE and contradicting it two lines later. + $updated = @() + if ($UpdateCommitted) { + foreach ($result in @($results | Where-Object { $_.State -eq 'STALE' })) { + $committedPath = Join-Path $repoRoot $result.Artifact + $regeneratedPath = Join-Path $outRoot $result.Artifact + + # Copy-Item rather than read-transform-write: the committed artifacts carry no BOM, + # and a round-trip through Set-Content is one -Encoding default away from adding + # one. Preserving bytes also settles the line-ending question instead of reopening + # it -- the comparison above already normalized CRLF, so a whole-file copy lands + # exactly the content the gate demanded, and git's autocrlf handles the checkout + # form on commit. A partial rewrite is what would produce mixed endings; this is not + # one. + $parent = Split-Path -Parent $committedPath + if (-not (Test-Path -LiteralPath $parent)) { + New-Item -ItemType Directory -Path $parent -Force | Out-Null + } + Copy-Item -LiteralPath $regeneratedPath -Destination $committedPath -Force + + $result.State = 'UPDATED' + $result.Detail = 'overwritten from this run''s regeneration' + } + $updated = @($results | Where-Object { $_.State -eq 'UPDATED' }) + } + # --- Report --------------------------------------------------------------------------- foreach ($result in $results) { @@ -340,11 +389,15 @@ try { $rows = foreach ($result in $results) { "| ``$($result.Artifact)`` | $($result.State) |" } $heading = '### Derived artifact check' if ($stale.Count -gt 0) { $heading = "$heading -- $($stale.Count) stale" } + if ($updated.Count -gt 0) { $heading = "$heading -- $($updated.Count) updated" } $lines = @($heading, '', "Pinned to REST $($pinnedVersions[0])-$($pinnedVersions[-1]) via ``Data/PfbCapabilityMap.json`` ``generatedFrom``.", '', '| Artifact | Result |', '|---|---|') + @($rows) ($lines -join [Environment]::NewLine) | Out-File -FilePath $env:GITHUB_STEP_SUMMARY -Append } if ($env:GITHUB_OUTPUT) { - "stale_count=$($stale.Count)" | Out-File -FilePath $env:GITHUB_OUTPUT -Append + @( + "stale_count=$($stale.Count)" + "updated_count=$($updated.Count)" + ) | Out-File -FilePath $env:GITHUB_OUTPUT -Append } if ($stale.Count -gt 0) { @@ -359,9 +412,9 @@ try { throw @" $($stale.Count) committed artifact(s) do not match a fresh regeneration: $($staleNames -join ', '). -RELIABLE FIX -- take the output this run already produced, which is by definition exactly what the comparison demanded: - ./scripts/Assert-PfbDerivedArtifacts.ps1 -Artifact $($staleNames -join ',') -KeepWorkDirectory -then copy /out/Data/* and /out/Reports/* over the committed copies and commit them. +RELIABLE FIX -- rerun with -UpdateCommitted, which writes the output this run already produced, and which is by definition exactly what the comparison demanded: + ./scripts/Assert-PfbDerivedArtifacts.ps1 -Artifact $($staleNames -join ',') -UpdateCommitted +then review the diff and commit. To inspect before overwriting anything, use -KeepWorkDirectory instead and copy /out/Data/* and /out/Reports/* over the committed copies by hand. Running the generators bare instead: $($fixSteps -join "`n ") @@ -369,6 +422,24 @@ only reproduces this output when tools/specs/ holds EXACTLY the $($pinnedVersion "@ } + if ($updated.Count -gt 0) { + # The paths are printed bare, one per line, so each is selectable on its own AND the + # block as a whole pastes into a git command -- the two things a reader does next. + $updatedNames = @($updated | ForEach-Object { $_.Artifact } | Sort-Object) + $unchanged = $results.Count - $updated.Count + + Write-Host '' + Write-Host "Updated $($updated.Count) committed artifact(s) from this run's regeneration:" + foreach ($name in $updatedNames) { Write-Host " $name" } + if ($unchanged -gt 0) { + Write-Host "$unchanged checked artifact(s) were already up to date and were not rewritten." + } + Write-Host '' + Write-Host 'Review, then commit:' + Write-Host " git diff --stat -- $($updatedNames -join ' ')" + return + } + Write-Host "All $($results.Count) checked artifact(s) are up to date." } finally { if ($KeepWorkDirectory) {