From 44c41334f8145dcd23f9c8082a59b550cfa98f90 Mon Sep 17 00:00:00 2001 From: Justin Emerson Date: Tue, 25 Aug 2026 12:19:19 -0700 Subject: [PATCH] feat(scripts): add -UpdateCommitted to the derived-artifacts gate When the gate reports a stale artifact it already holds, in its scratch tree, exactly the content the comparison demanded. Until now the caller was told to copy those files over the committed copies by hand -- a mechanical step with two ways to get it wrong (missing a file of a .json/.md pair, or round-tripping through an editor and adding a BOM). -UpdateCommitted writes them instead, then exits 0 and lists the repo-relative path of every file it wrote, so the paths paste straight into git diff or git add. Only artifacts the run found stale are written, and only within -Artifact when that narrows the set, so git status afterwards names exactly what moved. Exiting 0 is deliberate: remediating is what the switch asks for, and a non-zero exit from a run that just fixed the problem breaks any caller chaining on it. Without the switch the behaviour is unchanged -- stale still throws, and CI, which passes only -WorkDirectory and -KeepWorkDirectory, cannot reach the write path. The script's header previously said the working tree is never touched. That reasoning was about side effects, not writes, so it is amended rather than dropped: an explicit opt-in is not a side effect, and nothing infers the switch from the environment. Copy-Item rather than read-transform-write, because the committed artifacts carry no BOM and a Set-Content round-trip is one -Encoding default away from adding one. Preserving bytes also settles the line-ending question -- verified by corrupting an artifact, running with the switch, and confirming git status came back empty. Co-Authored-By: Claude Opus 5 (1M context) --- Reports/README.md | 21 +++++-- Tests/AssertDerivedArtifacts.Tests.ps1 | 50 +++++++++++++++ scripts/Assert-PfbDerivedArtifacts.ps1 | 85 +++++++++++++++++++++++--- 3 files changed, 144 insertions(+), 12 deletions(-) diff --git a/Reports/README.md b/Reports/README.md index 5f6a790f..1312ee33 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 200e0774..3a0adeb1 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 e05302bf..1df13110 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) {