From cceb9e4839b0e4650ae571c2a1985df09cd8d65a Mon Sep 17 00:00:00 2001 From: Julian lechner Date: Thu, 1 Oct 2026 13:59:10 +0200 Subject: [PATCH] fix(build): build privately and swap generated/ after validation Build into a per-run sibling directory and replace generated/ only after validation, with a lock around the swap, so a failed or parallel build never leaves generated/ partial. Co-Authored-By: Claude Opus 5.5 --- .gitignore | 3 ++ scripts/commands/build.ps1 | 43 +++++++++++++++++++++++-- scripts/commands/build.sh | 41 +++++++++++++++++++++-- scripts/tests/test-build-validation.ps1 | 20 ++++++++++-- scripts/tests/test-build-validation.sh | 15 +++++++-- 5 files changed, 113 insertions(+), 9 deletions(-) diff --git a/.gitignore b/.gitignore index 2425ead..373cf75 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,7 @@ generated/ +generated.build-*/ +generated.old-*/ +generated.lock .superpowers/ *.log *.tmp diff --git a/scripts/commands/build.ps1 b/scripts/commands/build.ps1 index 5441443..21d9c0e 100644 --- a/scripts/commands/build.ps1 +++ b/scripts/commands/build.ps1 @@ -3,7 +3,11 @@ param([switch]$Summary) $ErrorActionPreference = 'Stop' $root = Split-Path (Split-Path $PSScriptRoot -Parent) -Parent $shared = Join-Path $root 'shared' -$output = Join-Path $root 'generated' +$final = Join-Path $root 'generated' +# Keep the name short: it lengthens every generated path, and Windows PowerShell 5.1 fails beyond 260 characters. +$output = Join-Path $root ("generated.build-ps$PID-" + [Guid]::NewGuid().ToString('N').Substring(0, 6)) +# A trap covers the whole script scope, so every terminating error discards this run's tree. +trap { Remove-Item -LiteralPath $output -Recurse -Force -ErrorAction SilentlyContinue; break } $sandboxSupported = [Environment]::OSVersion.Platform -eq [PlatformID]::Unix if ($sandboxSupported) { $kernel = (& uname -r 2>$null | Out-String).Trim() @@ -32,8 +36,16 @@ foreach ($entry in @(Import-Csv -LiteralPath $capabilityManifest -Delimiter ([ch $agentTools[$entry.role] = $entry.tools } -Remove-Item $output -Recurse -Force -ErrorAction SilentlyContinue -New-Item $output -ItemType Directory -Force | Out-Null +# Build into a private sibling directory and swap it into place only after validation, so a failed +# or parallel build never leaves generated/ partial. Leftovers of crashed runs are removed here: +# old trees always, build trees only when their PowerShell owner process has exited. +# ponytail: Bash-owned leftovers are left to the Bash build, since PIDs are not comparable across shells. +foreach ($leftover in @(Get-ChildItem -LiteralPath $root -Directory -Filter 'generated.*')) { + if ($leftover.Name -like 'generated.old-*' -or ($leftover.Name -match '^generated\.build-ps(\d+)-' -and -not (Get-Process -Id ([int]$Matches[1]) -ErrorAction SilentlyContinue))) { + Remove-Item -LiteralPath $leftover.FullName -Recurse -Force -ErrorAction SilentlyContinue + } +} +New-Item $output -ItemType Directory | Out-Null function Copy-Directory($source, $destination) { New-Item $destination -ItemType Directory -Force | Out-Null @@ -220,5 +232,30 @@ $leftoverPlaceholders = @(Get-ChildItem $output -File -Recurse | ForEach-Object }) if ($leftoverPlaceholders.Count) { throw "Unresolved template placeholders in: $($leftoverPlaceholders -join ', ')" } +$lockPath = Join-Path $root 'generated.lock' +$lockDeadline = (Get-Date).AddSeconds(30) +while ($true) { + try { [System.IO.File]::Open($lockPath, [System.IO.FileMode]::CreateNew, [System.IO.FileAccess]::Write, [System.IO.FileShare]::None).Dispose(); break } + catch [System.IO.IOException], [System.UnauthorizedAccessException] { + if ((Get-Date) -gt $lockDeadline) { throw "Another build holds $lockPath; wait for it to finish, or remove the file if no build is running." } + Start-Sleep -Milliseconds 200 + } +} +$retired = $null +try { + if (Test-Path -LiteralPath $final) { + $retired = Join-Path $root ("generated.old-ps$PID-" + [Guid]::NewGuid().ToString('N').Substring(0, 6)) + [System.IO.Directory]::Move($final, $retired) + } + try { [System.IO.Directory]::Move($output, $final) } + catch { + if ($retired) { [System.IO.Directory]::Move($retired, $final); $retired = $null } + throw + } +} finally { + [System.IO.File]::Delete($lockPath) +} +if ($retired) { Remove-Item -LiteralPath $retired -Recurse -Force -ErrorAction SilentlyContinue } + if ($Summary) { Write-Output 'Build: PASS | 4 packages' } else { Write-Output 'PASS build: codex-powershell, claude-powershell, codex-bash, claude-bash' } exit 0 diff --git a/scripts/commands/build.sh b/scripts/commands/build.sh index f074068..dd6ad21 100644 --- a/scripts/commands/build.sh +++ b/scripts/commands/build.sh @@ -10,7 +10,7 @@ done root=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/../.." && pwd) shared="$root/shared" -output="$root/generated" +final="$root/generated" sandbox_supported=false platform=$(uname -s 2>/dev/null || true) case "$platform" in @@ -66,7 +66,27 @@ if [ "$summary" = true ]; then else printf '%s\n' "$session_output" fi -rm -rf -- "$output" +# Build into a private sibling directory and swap it into place only after validation, so a failed +# or parallel build never leaves generated/ partial. Leftovers of crashed runs are removed here: +# old trees always, build trees only when their Bash owner process has exited. +# ponytail: PowerShell-owned leftovers are left to the PowerShell build, since PIDs are not comparable across shells. +for leftover in "$root"/generated.old-* "$root"/generated.build-sh*; do + [ -d "$leftover" ] || continue + if [[ "$leftover" == */generated.build-sh* ]]; then + leftover_pid=${leftover##*/generated.build-sh} + leftover_pid=${leftover_pid%%-*} + ! kill -0 "$leftover_pid" 2>/dev/null || continue + fi + rm -rf -- "$leftover" +done +lock="$root/generated.lock" +locked=false +retired='' +output='' +trap 'rm -rf -- ${output:+"$output"}; [ "$locked" != true ] || rm -f -- "$lock"' EXIT +# Keep the name short: it lengthens every generated path, which matters on Windows. +output=$(mktemp -d "$root/generated.build-sh$$-XXXXXX") +chmod "$(printf '%o' $((0777 & ~$(umask))))" "$output" agent_dirs=("$shared"/agents/*/) declare -A agent_seen @@ -231,4 +251,21 @@ if [ -n "$leftover_tokens" ]; then exit 1 fi +for ((attempt = 0; attempt < 150; attempt++)); do + if (set -o noclobber; : > "$lock") 2>/dev/null; then locked=true; break; fi + sleep 0.2 +done +[ "$locked" = true ] || { printf 'Another build holds %s; wait for it to finish, or remove the file if no build is running.\n' "$lock" >&2; exit 1; } +if [ -e "$final" ]; then + retired="$root/generated.old-sh$$-$RANDOM" + mv -- "$final" "$retired" +fi +if ! mv -- "$output" "$final"; then + [ -z "$retired" ] || mv -- "$retired" "$final" + exit 1 +fi +rm -f -- "$lock" +locked=false +[ -z "$retired" ] || rm -rf -- "$retired" + if [ "$summary" = true ]; then printf 'Build: PASS | 4 packages\n'; else printf 'PASS build: codex-powershell, claude-powershell, codex-bash, claude-bash\n'; fi diff --git a/scripts/tests/test-build-validation.ps1 b/scripts/tests/test-build-validation.ps1 index 90b0e29..9572062 100644 --- a/scripts/tests/test-build-validation.ps1 +++ b/scripts/tests/test-build-validation.ps1 @@ -8,7 +8,7 @@ New-Item $work -ItemType Directory -Force | Out-Null function New-RepoCopy { $copy = Join-Path $work ("case-" + [Guid]::NewGuid()) New-Item $copy -ItemType Directory -Force | Out-Null - Get-ChildItem $root -Force | Where-Object { $_.Name -notin @('.git','generated') } | ForEach-Object { + Get-ChildItem $root -Force | Where-Object { $_.Name -ne '.git' -and $_.Name -ne 'generated' -and $_.Name -notlike 'generated.*' } | ForEach-Object { Copy-Item $_.FullName (Join-Path $copy $_.Name) -Recurse -Force } return $copy @@ -20,12 +20,28 @@ function Test-BuildFails([string]$Label, [scriptblock]$Mutate) { $failed = $false try { & (Join-Path $copy 'scripts/commands/build.ps1') 2>&1 | Out-Null } catch { $failed = $true } + $leftovers = @(Get-ChildItem $copy -Force -Filter 'generated*') Remove-Item $copy -Recurse -Force -ErrorAction SilentlyContinue if (-not $failed) { throw "Expected build to fail for case '$Label' but it succeeded." } + if ($leftovers.Count) { throw "Failed build for case '$Label' left output behind: $($leftovers.Name -join ', ')" } } $baseline = New-RepoCopy & (Join-Path $baseline 'scripts/commands/build.ps1') -Summary | Out-Null +$baselineGenerated = Join-Path $baseline 'generated' +$expectedFiles = @(Get-ChildItem $baselineGenerated -File -Recurse | ForEach-Object { $_.FullName.Substring($baselineGenerated.Length) } | Sort-Object) +$builds = @(1, 2 | ForEach-Object { + $build = Start-Process (Get-Process -Id $PID).Path -ArgumentList @('-NoProfile', '-ExecutionPolicy', 'Bypass', '-File', ('"' + (Join-Path $baseline 'scripts/commands/build.ps1') + '"'), '-Summary') -NoNewWindow -PassThru -RedirectStandardOutput (Join-Path $work "parallel-$_.out") -RedirectStandardError (Join-Path $work "parallel-$_.err") + $null = $build.Handle # Windows PowerShell 5.1 reports ExitCode only when the handle was opened. + $build +}) +foreach ($build in $builds) { $build.WaitForExit() } +foreach ($index in 0, 1) { + if ($builds[$index].ExitCode -ne 0) { throw "Parallel build $($index + 1) failed: $(Get-Content (Join-Path $work "parallel-$($index + 1).err") -Raw)" } +} +$actualFiles = @(Get-ChildItem $baselineGenerated -File -Recurse | ForEach-Object { $_.FullName.Substring($baselineGenerated.Length) } | Sort-Object) +if (Compare-Object $expectedFiles $actualFiles) { throw 'generated/ is incomplete after two parallel builds.' } +if (@(Get-ChildItem $baseline -Force | Where-Object Name -like 'generated.*').Count) { throw 'Parallel builds left a build directory, retired tree, or lock behind.' } Test-BuildFails 'invalid Claude settings.json' { param($copy) @@ -64,5 +80,5 @@ Test-BuildFails 'unknown placeholder beside an allowed installer placeholder' { } Remove-Item $work -Recurse -Force -ErrorAction SilentlyContinue -if ($Summary) { Write-Output 'Tests: PASS | build validation' } else { Write-Output 'PASS build validation: invalid JSON, duplicate agent, unknown Claude tool name, duplicate plugin, and leftover placeholders are all rejected' } +if ($Summary) { Write-Output 'Tests: PASS | build validation' } else { Write-Output 'PASS build validation: invalid JSON, duplicate agent, unknown Claude tool name, duplicate plugin, and leftover placeholders are all rejected without leaving output; parallel builds swap generated/ safely' } exit 0 diff --git a/scripts/tests/test-build-validation.sh b/scripts/tests/test-build-validation.sh index 0d15c50..b2ebe26 100644 --- a/scripts/tests/test-build-validation.sh +++ b/scripts/tests/test-build-validation.sh @@ -16,7 +16,7 @@ trap cleanup EXIT new_repo_copy() { local copy copy=$(mktemp -d "$work/case-XXXXXX") - (cd "$root" && tar -cf - --exclude='.git' --exclude='generated' .) | (cd "$copy" && tar -xf -) + (cd "$root" && tar -cf - --exclude='.git' --exclude='generated' --exclude='generated.*' .) | (cd "$copy" && tar -xf -) printf '%s' "$copy" } @@ -28,11 +28,22 @@ test_build_fails() { printf 'Expected build to fail for case "%s" but it succeeded.\n' "$label" >&2 exit 1 fi + if [ -n "$(find "$copy" -maxdepth 1 -name 'generated*' -print -quit)" ]; then + printf 'Failed build for case "%s" left output behind.\n' "$label" >&2 + exit 1 + fi rm -rf -- "$copy" } baseline=$(new_repo_copy) bash "$baseline/scripts/commands/build.sh" --summary >/dev/null +expected_files=$(cd "$baseline/generated" && find . -type f | sort) +bash "$baseline/scripts/commands/build.sh" --summary > "$work/parallel-1.out" 2>&1 & first_build=$! +bash "$baseline/scripts/commands/build.sh" --summary > "$work/parallel-2.out" 2>&1 & second_build=$! +wait "$first_build" || { printf 'Parallel build 1 failed:\n%s\n' "$(cat "$work/parallel-1.out")" >&2; exit 1; } +wait "$second_build" || { printf 'Parallel build 2 failed:\n%s\n' "$(cat "$work/parallel-2.out")" >&2; exit 1; } +[ "$(cd "$baseline/generated" && find . -type f | sort)" = "$expected_files" ] || { printf 'generated/ is incomplete after two parallel builds.\n' >&2; exit 1; } +[ -z "$(find "$baseline" -maxdepth 1 -name 'generated.*' -print -quit)" ] || { printf 'Parallel builds left a build directory, retired tree, or lock behind.\n' >&2; exit 1; } mutate_invalid_json() { printf '} this is not json {' >> "$1/adapters/claude/config/settings.json" @@ -65,4 +76,4 @@ mutate_mixed_placeholders() { } test_build_fails 'unknown placeholder beside an allowed installer placeholder' mutate_mixed_placeholders -if [ "$summary" = true ]; then printf 'Tests: PASS | build validation\n'; else printf 'PASS build validation: invalid JSON, duplicate agent, unknown Claude tool name, duplicate plugin, and leftover placeholders are all rejected\n'; fi +if [ "$summary" = true ]; then printf 'Tests: PASS | build validation\n'; else printf 'PASS build validation: invalid JSON, duplicate agent, unknown Claude tool name, duplicate plugin, and leftover placeholders are all rejected without leaving output; parallel builds swap generated/ safely\n'; fi