diff --git a/host-setup/bootstrap.ps1 b/host-setup/bootstrap.ps1 index 0a1d08789..474eaea53 100644 --- a/host-setup/bootstrap.ps1 +++ b/host-setup/bootstrap.ps1 @@ -2,19 +2,30 @@ # It is the one file fetched on its own, because a host with no git and no checkout is what it exists to fix. # It reads no payload, no table, and no sibling module: it obtains a tree and runs one entry point inside that tree. # -# A tarball rather than a clone, because a clone needs git on a host that may not have it, and because a tarball of a resolved commit cannot be stale. +# It fetches a tarball rather than a clone, for these reasons: +# - A clone needs git on a host that may not have it. +# - A tarball of a resolved commit cannot be stale. # The commit it resolved is printed before anything runs, so a run says which revision of the fleet's tooling it used. # # A run that installs the skills keeps its tree rather than removing it, because the Claude Code marketplace it registers loads that directory in place. -# That tree is replaced whole by the next such run and is never a git checkout, so the bootstrap still needs no git and modifies no checkout of anyone's. +# The bootstrap still needs no git and modifies no checkout of anyone's, because of the following. +# - That tree is replaced whole by the next such run. +# - That tree is never a git checkout. # # An unverified loader is worse than none, which is why this one does more than its Linux peer before it trusts anything. -# It pins TLS 1.2 itself rather than assume a fresh host's default reaches GitHub, it checks a fetched tree for the marker it wrote before removing anything under -Dir, and it hands off to PowerShell 7 explicitly rather than assume the console it started in already carries it. +# It does the following: +# - It pins TLS 1.2 itself rather than assume a fresh host's default reaches GitHub. +# - It checks a fetched tree for the marker it wrote before removing anything under -Dir. +# - It hands off to PowerShell 7 explicitly rather than assume the console it started in already carries it. # -# Windows PowerShell 5.1 is the one shell a fresh Windows host guarantees, the way bash is guaranteed on a fresh Debian host, so everything up to and including finding or installing PowerShell 7 is written to run under it. -# Once pwsh is confirmed, this file hands the rest of the run to itself under pwsh, and every script under host-setup/windows it goes on to drive requires that version too. - -# The Host action binds to StandUpHost rather than Host, because Host is PowerShell's own automatic variable for the host program ($Host.Name, $Host.UI), and a parameter named Host would shadow it and fail PSAvoidAssignmentToAutomaticVariable. +# Windows PowerShell 5.1 is the one shell a fresh Windows host guarantees, the way bash is guaranteed on a fresh Debian host. +# Everything up to and including finding or installing PowerShell 7 is therefore written to run under it. +# Once pwsh is confirmed, this file hands the rest of the run to itself under pwsh. +# Every script under host-setup/windows that this file goes on to drive requires PowerShell 7 too. + +# The Host action binds to StandUpHost rather than Host, because: +# - Host is PowerShell's own automatic variable for the host program ($Host.Name, $Host.UI). +# - A parameter named Host would shadow that variable and fail PSAvoidAssignmentToAutomaticVariable. # The alias keeps -Host as the spelling a caller types, while leaving the automatic variable free for the console-detection checks below. [CmdletBinding()] param( @@ -36,7 +47,9 @@ param( [Alias('h')][switch]$Help ) -# Captured before anything else touches scope, so the pwsh handoff below can rebuild the argument list this process was bound with, rather than reaching for an automatic variable from inside a nested function. +# Captured before anything else touches scope. +# The pwsh handoff below can therefore rebuild the argument list this process was bound with. +# It does so rather than reaching for an automatic variable from inside a nested function. $SCRIPT_BOUND_PARAMETERS = $PSBoundParameters Set-StrictMode -Version Latest @@ -61,7 +74,8 @@ function warn { param([string]$Message) [Console]::Error.WriteLine("WARNING: $Me function die { param([string]$Message) [Console]::Error.WriteLine("ERROR: $Message"); exit 1 } # Every parameter is read into a variable here rather than from inside a function. -# A script parameter reached only from a nested scope reads as declared and never used, which is a finding on each one and hides a parameter that genuinely is unused. +# A script parameter reached only from a nested scope reads as declared and never used. +# That reading is a finding on each such parameter, and it hides a parameter that genuinely is unused. $ACTIONS = [ordered]@{ report = [bool]$Report upgrade = [bool]$Upgrade @@ -82,7 +96,9 @@ $KEEP = [bool]$Keep $REF = if ($Ref) { $Ref } else { $DEFAULT_REF } $MODE = '' -# No placeholder for $DIR here, unlike the four lines around it: PowerShell variable names are case-insensitive, so $DIR and the -Dir parameter $Dir are the same variable, and resetting it here would silently discard whatever -Dir the caller passed before Resolve-Directory ever reads it. +# No placeholder for $DIR here, unlike the four lines around it. +# PowerShell variable names are case-insensitive, so $DIR and the -Dir parameter $Dir are the same variable. +# Resetting $DIR here would silently discard whatever -Dir the caller passed before Resolve-Directory ever reads it. $RESOLVED = '' $TREE = '' $PWSH_PATH = '' @@ -141,13 +157,17 @@ Examples: # --- pwsh handoff --- # -# Everything above this point, and the four functions below, run under Windows PowerShell 5.1: no ternary or null-coalescing operator, no multi-argument Join-Path, nothing newer than that runtime parses. +# Everything above this point, and the four functions below, run under Windows PowerShell 5.1, which means: +# - No ternary or null-coalescing operator. +# - No multi-argument Join-Path. +# - Nothing newer than that runtime parses. # Everything past the handoff may use whatever pwsh 7 accepts, though it mostly does not need to. function Resolve-Pwsh { $command = Get-Command pwsh -ErrorAction SilentlyContinue if ($command) { return $command.Source } - # ProgramFiles(x86) carries no value on a host with no WOW64 layer, and Join-Path on a null path is a terminating error under Set-StrictMode, not an empty match to fall through. + # ProgramFiles(x86) carries no value on a host with no WOW64 layer. + # Join-Path on a null path is a terminating error under Set-StrictMode, not an empty match to fall through. $candidates = @((Join-Path $env:ProgramFiles 'PowerShell\7\pwsh.exe')) if (${env:ProgramFiles(x86)}) { $candidates += (Join-Path ${env:ProgramFiles(x86)} 'PowerShell\7\pwsh.exe') } foreach ($candidate in $candidates) { @@ -156,7 +176,8 @@ function Resolve-Pwsh { return $null } -# This is the one place that install-tools.ps1's own rule does not hold: pwsh is deliberately not part of any tool registry, because a host that cannot run these scripts cannot be repaired by them. +# This is the one place that install-tools.ps1's own rule does not hold. +# The pwsh runtime is deliberately not part of any tool registry, because a host that cannot run these scripts cannot be repaired by them. # Standing up pwsh is the whole reason this file exists rather than starting from a checkout. function Install-Pwsh { if (-not (Get-Command winget -ErrorAction SilentlyContinue)) { @@ -167,13 +188,16 @@ function Install-Pwsh { $wingetExit = $LASTEXITCODE $found = Resolve-Pwsh if (-not $found) { - # This names winget's own exit code even though pwsh's absence, not the code, is what decides this die: a non-zero code explains why, where "reported installing" alone does not, and winget answering 0 while pwsh is still missing is worth saying too. + # This names winget's own exit code even though pwsh's absence, not the code, is what decides this die. + # A non-zero code explains why, where "reported installing" alone does not. + # That winget answers 0 while pwsh is still missing is worth saying too. die "winget exited $wingetExit installing PowerShell 7, and pwsh could still not be found. Close this console and paste the setup lines again, or install it from https://aka.ms/PSWindows." } return $found } -# Rebuilds the arguments this process was bound with, since a param() bound script has no raw $args left to forward: -Report arrives as $Report = $true, not as a string in a list. +# Rebuilds the arguments this process was bound with, since a param() bound script has no raw $args left to forward. +# -Report arrives as $Report = $true, not as a string in a list. function Get-ForwardedArgument { $forward = @() foreach ($key in $script:SCRIPT_BOUND_PARAMETERS.Keys) { @@ -189,7 +213,9 @@ function Get-ForwardedArgument { } # The same parameters Get-ForwardedArgument forwards, rendered as one pasteable command-line string rather than split into argv elements. -# Classified by SCRIPT_BOUND_PARAMETERS's own type rather than by a value's shape: a value that happens to look like a flag (-Ref '-Yes') would otherwise print unquoted and bind as its own switch when pasted, which is exactly the bug guessing from shape produced here before. +# Classified by SCRIPT_BOUND_PARAMETERS's own type rather than by a value's shape. +# A value that happens to look like a flag (-Ref '-Yes') would otherwise print unquoted and bind as its own switch when pasted. +# That is exactly the bug guessing from shape produced here before. function Format-ForwardedArgumentForDisplay { $parts = foreach ($key in $script:SCRIPT_BOUND_PARAMETERS.Keys) { $value = $script:SCRIPT_BOUND_PARAMETERS[$key] @@ -239,7 +265,8 @@ function Test-KeepsTree { # The paths this loader creates under DIR, named in one place so the cleanup and the download agree. # DIR itself is never removed, since -Dir may name a directory the caller owns and put other things in. -# A kept tree and a transient one take different names, so a report run sharing a -Dir with a host run never removes the tree Claude Code loads. +# A kept tree and a transient one take different names. +# A report run sharing a -Dir with a host run therefore never removes the tree Claude Code loads. function Get-TreeName { if (Test-KeepsTree) { 'skills-tree' } else { 'tree' } } function Get-TreePath { Join-Path $script:DIR (Get-TreeName) } function Get-StagingPath { Join-Path $script:DIR "$(Get-TreeName).new" } @@ -249,15 +276,19 @@ function Get-LockPath { Join-Path $script:DIR "$(Get-TreeName).lock" } function Get-TarPath { Join-Path $env:SystemRoot 'System32\tar.exe' } # A tree carries a marker this loader wrote, and a tree without one is somebody else's. -# DIR is a caller-supplied path, so a tree under it is not necessarily ours: pointing -Dir at a directory that already holds one would otherwise have this remove it, both before extracting and again on exit. +# DIR is a caller-supplied path, so a tree under it is not necessarily ours. +# Pointing -Dir at a directory that already holds one would otherwise have this remove it, both before extracting and again on exit. function Test-Ownership { param([string]$Path) $item = Get-Item -LiteralPath $Path -Force -ErrorAction SilentlyContinue if ($item -and ($item.Attributes -band [IO.FileAttributes]::ReparsePoint)) { return $false } if (-not ($item -and $item.PSIsContainer)) { return $false } - # An empty directory counts as ours, since it holds nothing to lose and no live run can be filling it while this one holds the lock. + # An empty directory counts as ours for two reasons. + # - It holds nothing to lose. + # - No live run can be filling it while this one holds the lock. # It is what a removal leaves where clearing the tree worked and removing the directory itself did not, the marker having gone with the contents. - # A directory that cannot be read is not known to be either, so it stays somebody else's, which on Linux includes the marker check itself throwing. + # A directory that cannot be read is not known to be either, so it stays somebody else's. + # On Linux, that case includes the marker check itself throwing. try { if (Test-Path -LiteralPath (Join-Path $Path '.bootstrap-owned')) { return $true } return -not (Get-ChildItem -LiteralPath $Path -Force | Select-Object -First 1) @@ -266,17 +297,23 @@ function Test-Ownership { } } -# Two runs sharing a -Dir use the same fixed names, so without this one run's removal of its staging tree, or its cleanup, can delete a tree the other is still extracting into. -# The lock is the file system's rather than a file whose presence is the lock, so a run that dies releases it, and FileShare.None is the share mode .NET implements as a flock on Linux, which is the lock bootstrap.sh takes on the same file. -# The file is left in place, since removing it while another run holds it open would let a third run lock a new file under the same name. +# Two runs sharing a -Dir use the same fixed names. +# So without this lock, one run's removal of its staging tree, or its cleanup, can delete a tree the other is still extracting into. +# The lock is the file system's rather than a file whose presence is the lock, so a run that dies releases it. +# FileShare.None is the share mode .NET implements as a flock on Linux. +# That flock is the lock bootstrap.sh takes on the same file. +# The file is left in place. +# That is because removing it while another run holds it open would let a third run lock a new file under the same name. function Lock-Directory { $lock = Get-LockPath New-Item -ItemType Directory -Path $script:DIR -Force | Out-Null try { $script:LOCK = [IO.File]::Open($lock, 'OpenOrCreate', 'ReadWrite', 'None') } catch [System.Management.Automation.MethodInvocationException] { - # A sharing violation, and its Linux flock equivalent, is a plain IOException, where a path or permission failure is one of its subclasses or another type. - # A file system error .NET maps to no subclass, a read-only mount or a full disk, is a plain IOException too, so the message names the reason rather than asserting the cause. + # A sharing violation, and its Linux flock equivalent, is a plain IOException. + # By contrast, a path or permission failure is one of its subclasses or another type. + # A file system error .NET maps to no subclass, a read-only mount or a full disk, is a plain IOException too. + # The message therefore names the reason rather than asserting the cause. $reason = $_.Exception.InnerException if ($reason.GetType() -eq [IO.IOException]) { die "Could not lock ${lock}: $($reason.Message) Another bootstrap run using $script:DIR is the usual cause, so let it finish, then run this again." @@ -316,7 +353,8 @@ function Get-Tree { } # The archive holds one top-level directory named for the repository and the revision. - # It is extracted beside the tree rather than over it, so a failed download or extract leaves a kept tree, and the plugin loading it, as they were. + # It is extracted beside the tree rather than over it. + # A failed download or extract therefore leaves a kept tree, and the plugin loading it, as they were. $staging = Get-StagingPath Remove-Owned -Path $staging New-Item -ItemType Directory -Path $staging -Force | Out-Null @@ -332,14 +370,16 @@ function Get-Tree { if (-not (Test-KeepsTree)) { Invoke-SwapIn } } -# Removes the ownership marker last, so a removal that stops part way, on a file another process holds open, leaves a directory the next run still recognizes as its own. +# Removes the ownership marker last. +# A removal that stops part way, on a file another process holds open, therefore leaves a directory the next run still recognizes as its own. function Remove-Tree { param([string]$Path) Get-ChildItem -LiteralPath $Path -Force | Where-Object { $_.Name -ne '.bootstrap-owned' } | Remove-Item -Recurse -Force Remove-Item -LiteralPath $Path -Recurse -Force } -# Renames a tree whole or not at all, which Move-Item does not: where a held file stops the rename, it moves the tree entry by entry instead, marker included. +# Renames a tree whole or not at all, which Move-Item does not. +# Where a held file stops the rename, Move-Item moves the tree entry by entry instead, marker included. function Move-Tree { param([string]$Path, [string]$Destination) try { @@ -349,8 +389,10 @@ function Move-Tree { } } -# Moves the old tree aside before the new one takes its name, and removes it only after, so no failure part way leaves the name empty or half-deleted. -# A rename of a directory in use fails whole on Windows, where a delete fails part way, which is why the old tree is renamed rather than deleted in place. +# Moves the old tree aside before the new one takes its name, and removes it only after. +# No failure part way therefore leaves the name empty or half-deleted. +# A rename of a directory in use fails whole on Windows, where a delete fails part way. +# That is why the old tree is renamed rather than deleted in place. function Invoke-SwapIn { $staging = Get-StagingPath $tree = Get-TreePath @@ -399,13 +441,15 @@ function Invoke-SwapIn { info "Extracted to $script:TREE" } -# Removes each path this run can create by its fixed name rather than through TREE, since a failed extract leaves an archive and a part-written staging tree before TREE names anything. +# Removes each path this run can create by its fixed name. +# It does not go through TREE, since a failed extract leaves an archive and a part-written staging tree before TREE names anything. # A path that is not ours was already refused where it mattered, at the download. # Refusing again from here would print the same error a second time, after the one that actually stopped the run. # The old tree is restored before the staging tree is removed, since the lock that failed the swap's rename can fail that removal too. function Invoke-Cleanup { Remove-Item -LiteralPath (Get-ArchivePath) -Force -ErrorAction SilentlyContinue - # A swap stopped between its two renames leaves the old tree aside and nothing at the name, so the old tree goes back rather than away. + # A swap stopped between its two renames leaves the old tree aside and nothing at the name. + # The old tree therefore goes back rather than away. $retired = Get-RetiredPath if (Test-Ownership -Path $retired) { if (-not (Test-Path -LiteralPath (Get-TreePath))) { @@ -432,8 +476,11 @@ function Invoke-Cleanup { # --- Handoff --- # Every tool runs from inside the fetched tree, and this is the only place a path inside it is named. -# Written as one interpolated, forward-slashed string against the bare $TREE rather than $script:TREE or Join-Path, so this loader's one entry point into the tree reads as the literal pattern the Linux loader is checked by, and stays checkable by that same pattern. -# A read resolves $TREE up to script scope on its own, and only a write needs the script: prefix, which is why the assignment in Get-Tree still carries it. +# Written as one interpolated, forward-slashed string against the bare $TREE rather than $script:TREE or Join-Path. +# That form makes this loader's one entry point into the tree read as the literal pattern the Linux loader is checked by. +# That form also keeps the entry point checkable by that same pattern. +# A read resolves $TREE up to script scope on its own, and only a write needs the script: prefix. +# That is why the assignment in Get-Tree still carries it. function Invoke-Tool { param([Parameter(Mandatory)][string]$Tool, [switch]$ToleratesFailure, [Parameter(ValueFromRemainingArguments)][string[]]$Arguments) @@ -467,7 +514,8 @@ function Test-Elevated { $IsWindows -and ([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator) } -# The agent tools live in a user's profile, and an administrator's holds none on a host where a person uses them, so an elevated run skips them and says so. +# The agent tools live in a user's profile, and an administrator's holds none on a host where a person uses them. +# An elevated run therefore skips them and says so. function Test-AgentsUser { if (Test-Elevated) { info "Running elevated, so no user's coding-agent tools are read, run this as that user to include them" @@ -500,7 +548,8 @@ function Invoke-AgentsInstall { } # The order is fixed rather than chosen. -# Packages come first so install-tools.ps1 and setup-github.ps1 act on a host winget has just brought current, and GitHub comes last because it is the only step that waits on a person in a browser. +# Packages come first so install-tools.ps1 and setup-github.ps1 act on a host winget has just brought current. +# GitHub comes last because it is the only step that waits on a person in a browser. # The skills step runs after the tools, because install-tools.ps1 provides the interpreter it needs. function Invoke-StandUp { # Named Kind rather than Profile, which is PowerShell's own automatic variable for the current user's profile script. @@ -589,7 +638,8 @@ function Resolve-Directory { } # PowerShell records which switches were given and not the order they came in, so two actions is a refusal rather than the last one winning. -# Unlike the four scripts this loader drives, zero given is not this loader's own default: main tells the menu and the piped-in report apart, the way bootstrap.sh's own entry point does. +# Unlike the four scripts this loader drives, zero given is not this loader's own default. +# The main function tells the menu and the piped-in report apart, the way bootstrap.sh's own entry point does. function Resolve-Mode { $given = @($script:ACTIONS.Keys | Where-Object { $script:ACTIONS[$_] }) if ($given.Count -gt 1) { die "More than one action given ($($given -join ', ')), name one" } @@ -597,11 +647,15 @@ function Resolve-Mode { return $given[0] } -# The lines a piped-in run is told to paste instead, shared by both places that print them: the handoff below needs a real file to hand off to pwsh, and Test-Interactive needs a real console to ask on, and a run with neither reaches this the same way. +# The lines a piped-in run is told to paste instead, shared by both places that print them. +# The handoff below needs a real file to hand off to pwsh. +# Test-Interactive needs a real console to ask on. +# A run with neither reaches these lines the same way. function Show-DownloadAndRunRemedy { info ' [Net.ServicePointManager]::SecurityProtocol = [Net.ServicePointManager]::SecurityProtocol -bor [Net.SecurityProtocolType]::Tls12' info " Invoke-WebRequest -UseBasicParsing -Uri https://raw.githubusercontent.com/$script:REPO/$script:DEFAULT_REF/host-setup/bootstrap.ps1 -OutFile bootstrap.ps1" - # Reuses Format-ForwardedArgumentForDisplay rather than a second implementation: the original invocation's own action and flags belong on the re-run this remedy is telling the caller to make. + # Reuses Format-ForwardedArgumentForDisplay rather than a second implementation. + # The original invocation's own action and flags belong on the re-run this remedy is telling the caller to make. $forwarded = Format-ForwardedArgumentForDisplay $suffix = if ($forwarded) { " $forwarded" } else { '' } info " powershell -ExecutionPolicy Bypass -File bootstrap.ps1$suffix" @@ -611,7 +665,9 @@ function main { if ($script:WANT_HELP) { usage; exit 0 } if ($PSVersionTable.PSVersion.Major -lt 7) { - # $PSCommandPath is empty for a script read through Invoke-Expression or a similar pipe, and the handoff below needs a real path to re-invoke under pwsh, so this cannot wait for Test-Interactive to say the same thing for a different reason. + # $PSCommandPath is empty for a script read through Invoke-Expression or a similar pipe. + # The handoff below needs a real path to re-invoke under pwsh. + # This check therefore cannot wait for Test-Interactive to say the same thing for a different reason. if (-not $PSCommandPath) { warn 'This needs a real file to hand off to PowerShell 7, and a piped-in script has none.' info 'Download the file and run it, rather than piping it:' @@ -654,7 +710,9 @@ function main { 'tools' { Invoke-Tool -Tool 'install-tools.ps1' -Arguments '-Install' } 'github' { Invoke-Tool -Tool 'setup-github.ps1' -Arguments '-Configure' } 'skills' { Invoke-SkillsInstall } - # Only setup-wsl.ps1's -Status runs here: its -Install needs a distribution name, which no flag here collects, so choosing a default distro nobody asked for is exactly what -Wsl staying out of -Host and -Dev already exists to avoid. + # Only setup-wsl.ps1's -Status runs here. + # Its -Install needs a distribution name, which no flag here collects. + # Choosing a default distro nobody asked for is therefore exactly what -Wsl staying out of -Host and -Dev already exists to avoid. # Installing one by name is a checkout away, once this run has fetched it. 'wsl' { Invoke-Tool -Tool 'setup-wsl.ps1' -Arguments '-Status' } 'host' { Invoke-StandUp -Kind 'host' } diff --git a/host-setup/bootstrap.sh b/host-setup/bootstrap.sh index 911164b4b..95f5becc8 100755 --- a/host-setup/bootstrap.sh +++ b/host-setup/bootstrap.sh @@ -4,11 +4,16 @@ # It is the one file fetched on its own, because a host with no git and no checkout is what it exists to fix. # It reads no payload, no table, and no sibling module: it obtains a tree and runs one entry point inside that tree. # -# A tarball rather than a clone, because a clone needs git on a host that may not have it, and because a tarball of a resolved commit cannot be stale. +# It fetches a tarball rather than a clone, for these reasons: +# - A clone needs git on a host that may not have it. +# - A tarball of a resolved commit cannot be stale. # The commit it resolved is printed before anything runs, so a run says which revision of the fleet's tooling it used. # # A run that installs the skills keeps its tree rather than removing it, because the Claude Code marketplace it registers loads that directory in place. -# That tree lives under the data directory rather than the cache, is replaced whole by the next such run, and is never a git checkout, so the bootstrap still needs no git and modifies no checkout of anyone's. +# That tree lives under the data directory rather than the cache. +# The bootstrap still needs no git and modifies no checkout of anyone's, because of the following. +# - The next such run replaces that tree whole. +# - That tree is never a git checkout. set -Eeuo pipefail @@ -107,7 +112,9 @@ resolve_ref() { # An unauthenticated request is rate limited per address, so a busy network can lose the lookup while the download itself is fine. # The run continues and says it cannot name its own revision, which is worth a warning rather than a refusal. - # This fallback is deliberate and gates no mutation: download_tree falls back to fetching $REF by name when RESOLVED is empty, exactly as it would if resolve_ref did not exist, and it has its own die on a real download failure. + # This fallback is deliberate and gates no mutation. + # The download_tree function falls back to fetching $REF by name when RESOLVED is empty, exactly as it would if resolve_ref did not exist. + # The download_tree function also has its own die on a real download failure. warn "Could not resolve $REF to a commit, so this run cannot be attributed to one" RESOLVED="" return 0 @@ -131,7 +138,8 @@ default_dir() { # The paths this script creates under DIR, named in one place so the trap and the download agree. # DIR itself is never removed, since --dir may name a directory the caller owns and put other things in. -# A kept tree and a transient one take different names, so a report run sharing a --dir with a host run never removes the tree Claude Code loads. +# A kept tree and a transient one take different names. +# A report run sharing a --dir with a host run therefore never removes the tree Claude Code loads. tree_name() { if keeps_tree; then printf 'skills-tree'; else printf 'tree'; fi } @@ -141,9 +149,12 @@ retired_path() { printf '%s\n' "$DIR/$(tree_name).old"; } archive_path() { printf '%s\n' "$DIR/$(tree_name).tar.gz"; } lock_path() { printf '%s\n' "$DIR/$(tree_name).lock"; } -# Two runs sharing a --dir use the same fixed names, so without this one run's removal of its staging tree, or its exit cleanup, can delete a tree the other is still extracting into. -# The lock is the kernel's rather than a file whose presence is the lock, so a run that dies releases it, and bootstrap.ps1 opens the same file with FileShare.None, which .NET implements as a flock on Linux. -# The file is left in place, since removing it while another run holds it open would let a third run lock a new file under the same name. +# Two runs sharing a --dir use the same fixed names. +# So without this lock, one run's removal of its staging tree, or its exit cleanup, can delete a tree the other is still extracting into. +# The lock is the kernel's rather than a file whose presence is the lock, so a run that dies releases it. +# The bootstrap.ps1 script opens the same file with FileShare.None, which .NET implements as a flock on Linux. +# The file is left in place. +# That is because removing it while another run holds it open would let a third run lock a new file under the same name. lock_dir() { local lock lock=$(lock_path) @@ -155,8 +166,11 @@ lock_dir() { } # A tree carries a marker this loader wrote, and a tree without one is somebody else's. -# DIR is a caller-supplied path, so a tree under it is not necessarily ours: pointing --dir at a directory that already holds one would otherwise have this remove it, both before extracting and again on exit. -# An empty directory counts as ours, since it holds nothing to lose and no live run can be filling it while this one holds the lock. +# DIR is a caller-supplied path, so a tree under it is not necessarily ours. +# Pointing --dir at a directory that already holds one would otherwise have this remove it, both before extracting and again on exit. +# An empty directory counts as ours for two reasons. +# - It holds nothing to lose. +# - No live run can be filling it while this one holds the lock. # It is what a removal leaves where clearing the tree worked and removing the directory itself did not, the marker having gone with the contents. # A directory that cannot be listed is not known to be empty, so it stays somebody else's. is_ours() { @@ -197,7 +211,8 @@ download_tree() { die "Could not download $REPO at $REF. Check the ref exists and that this host reaches codeload.github.com." # The archive holds one top-level directory named for the repository and the revision. - # It is extracted beside the tree rather than over it, so a failed download or extract leaves a kept tree, and the plugin loading it, as they were. + # It is extracted beside the tree rather than over it. + # A failed download or extract therefore leaves a kept tree, and the plugin loading it, as they were. staging=$(staging_path) remove_owned "$staging" mkdir -p "$staging" @@ -214,7 +229,8 @@ download_tree() { return 0 } -# Moves the old tree aside before the new one takes its name, and removes it only after, so no failure part way leaves the name empty or half-deleted. +# Moves the old tree aside before the new one takes its name, and removes it only after. +# No failure part way therefore leaves the name empty or half-deleted. swap_in() { local staging tree retired moved=false staging=$(staging_path) @@ -243,11 +259,13 @@ swap_in() { } cleanup() { - # Removes each path this run can create by its fixed name rather than through TREE, since a failed extract leaves an archive and a part-written staging tree before TREE names anything. + # Removes each path this run can create by its fixed name. + # It does not go through TREE, since a failed extract leaves an archive and a part-written staging tree before TREE names anything. rm -f "$(archive_path)" # A path that is not ours was already refused where it mattered, at the download. # Refusing again from the exit trap would print the same error a second time, after the one that actually stopped the run. - # A swap stopped between its two renames leaves the old tree aside and nothing at the name, so the old tree goes back rather than away. + # A swap stopped between its two renames leaves the old tree aside and nothing at the name. + # The old tree therefore goes back rather than away. if is_ours "$(retired_path)"; then if is_ours "$(tree_path)"; then if ! remove_tree "$(retired_path)"; then @@ -299,7 +317,8 @@ report() { fi } -# The agent tools live in a user's home, and root's holds none on a host where a person uses them, so a root run skips them and says so. +# The agent tools live in a user's home, and root's holds none on a host where a person uses them. +# A root run therefore skips them and says so. agents_user() { if [[ $EUID -eq 0 ]]; then info "Running as root, so no user's coding-agent tools are read, run this as that user to include them" diff --git a/host-setup/linux/setup-github.sh b/host-setup/linux/setup-github.sh index d2abd1bd6..da978655e 100755 --- a/host-setup/linux/setup-github.sh +++ b/host-setup/linux/setup-github.sh @@ -3,16 +3,21 @@ # Sets up git and GitHub on a host: the SSH key, the git configuration, and commit signing. # Every step is idempotent, so a re-run repairs a half configured host rather than duplicating what is already there. # -# Two steps cannot be automated, because they happen in a browser: registering the public key as an authentication key, and registering the same key as a signing key. +# Two steps cannot be automated, because they happen in a browser: +# - Registering the public key as an authentication key. +# - Registering the same key as a signing key. # Both are gates rather than suggestions. -# The script stops at each, prints the key to paste and where to paste it, and checks afterwards that the registration took, by reading the keys GitHub publishes for the account. +# The script stops at each and prints the key to paste and where to paste it. +# The script checks afterwards that the registration took, by reading the keys GitHub publishes for the account. set -Eeuo pipefail readonly KEY="$HOME/.ssh/id_ed25519" readonly ALLOWED_SIGNERS="$HOME/.config/git/allowed_signers" -# Git expands a leading tilde in a path setting, and the hosts already configured by hand hold the tilde form, so writing that form leaves an already configured host untouched. +# Writing the tilde form leaves an already configured host untouched, because of the following. +# - Git expands a leading tilde in a path setting. +# - The hosts already configured by hand hold the tilde form. # File operations use the expanded paths above, since the shell does not expand a tilde inside a variable. # shellcheck disable=SC2088 # The tilde is meant to stay literal here, git is what expands it. readonly KEY_SETTING="~/.ssh/id_ed25519.pub" @@ -140,7 +145,8 @@ ensure_prerequisites() { readarray -t packages < <(prerequisites_missing) [[ ${#packages[@]} -eq 0 ]] && return 0 - # Everything above this point works anywhere, and installing is the first step that does not, so a host without apt is told that rather than meeting a missing command part way through. + # Everything above this point works anywhere. + # Installing is the first step that does not, so a host without apt is told that rather than meeting a missing command part way through. command -v apt-get >/dev/null || die "apt-get not found, and installing ${packages[*]} needs it. Install them with this host's package manager, then run this again." @@ -152,7 +158,9 @@ ensure_prerequisites() { # --- Key --- # The key as its type and body, without the trailing comment. -# That is the form both of GitHub's published lists carry: the .keys endpoint publishes two fields per line, and the signing keys API keeps the comment in a separate title field rather than in the key. +# That is the form both of GitHub's published lists carry: +# - The .keys endpoint publishes two fields per line. +# - The signing keys API keeps the comment in a separate title field rather than in the key. # A local public key carries three fields, so a comparison that skipped this would never match. key_body() { local file="${1:-$KEY.pub}" @@ -165,7 +173,10 @@ ensure_key() { return 0 fi - # A private key with no public half is the harder case, because everything downstream reads the public one: the registration paste, the allowed signers entry, and both registration checks. + # A private key with no public half is the harder case, because everything downstream reads the public one: + # - The registration paste. + # - The allowed signers entry. + # - Both registration checks. # The public key is derived from the private key rather than the pair being regenerated, since regenerating would invalidate a key that is already registered. if [[ -f $KEY ]]; then step "Deriving the missing public key from the private key" @@ -186,7 +197,8 @@ ensure_key() { } # The first enrollment is the one moment a substituted host key would be accepted for good, and every later connection trusts whatever was recorded here. -# So what github.com offers is checked against the fingerprints GitHub publishes before it is written down, and a check that cannot run is a refusal rather than a warning, since recording an unchecked key is what this prevents. +# So what github.com offers is checked against the fingerprints GitHub publishes before it is written down. +# A check that cannot run is a refusal rather than a warning, since recording an unchecked key is what this prevents. ensure_known_host() { ssh-keygen -F github.com >/dev/null 2>&1 && return 0 @@ -219,11 +231,14 @@ ensure_known_host() { # --- GitHub, read only --- -# Every probe refuses to write a host key, so asking whether authentication works cannot enroll github.com behind the reader's back and a status run stays a read-only action. +# Every probe refuses to write a host key. +# So asking whether authentication works cannot enroll github.com behind the reader's back, and a status run stays a read-only action. # Enrolling the host key is ensure_known_host's job, in configure mode, where changing the host is the point. # # A status run also refuses to prompt, since it may be run unattended and a passphrase prompt would hang it. -# Configure may prompt, because a passphrase for a key with no agent is a question only the operator can answer, and it is already an interactive action. +# Configure may prompt, because of the following. +# - A passphrase for a key with no agent is a question only the operator can answer. +# - Configure is already an interactive action. ssh_probe() { local -a opts=(-o StrictHostKeyChecking=yes -o ConnectTimeout=10) if [[ $MODE == "status" ]]; then @@ -242,10 +257,13 @@ github_auth_user() { } # The same question asked of the managed key alone. -# The ssh client offers the default identity files and an agent's keys besides whatever is named, so a host carrying another account's key authenticates as that account, and every check below would then be asking about the wrong one. +# The ssh client offers the default identity files and an agent's keys besides whatever is named. +# So a host carrying another account's key authenticates as that account, and every check below would then be asking about the wrong one. # IdentitiesOnly is not enough by itself, since the default files count as configured identities. # -# That isolation is also why this is a second probe rather than a replacement: a host reaching GitHub through a proxy or over port 443 needs the config this probe discards, so a failure here means the managed key did not answer rather than that the host cannot reach GitHub. +# That isolation is also why this is a second probe rather than a replacement. +# A host reaching GitHub through a proxy or over port 443 needs the config this probe discards. +# So a failure here means the managed key did not answer, rather than that the host cannot reach GitHub. github_auth_user_managed() { [[ -f $KEY ]] || return 1 @@ -254,7 +272,8 @@ github_auth_user_managed() { printf '%s' "$greeting" | sed -n 's/^Hi \([^!]*\)!.*/\1/p' } -# Prefer the account the managed key belongs to, since that is the one the registration checks are about, and say so when the host answers as somebody else. +# Prefer the account the managed key belongs to, since that is the one the registration checks are about. +# Say so when the host answers as somebody else. resolve_github_user() { local general managed general=$(github_auth_user) @@ -274,11 +293,14 @@ resolve_github_user() { return 0 } -# GitHub publishes both key lists for an account, so a registration can be checked from the host without a token and without the browser that made it. +# GitHub publishes both key lists for an account. +# So a registration can be checked from the host without a token and without the browser that made it. # Each fetch is captured before it is searched rather than piped into the search. -# Under pipefail a search that stops at its first match leaves the fetch writing to a closed pipe, so a key that is registered reports itself as missing on any list long enough for the timing to matter. +# Under pipefail a search that stops at its first match leaves the fetch writing to a closed pipe. +# So a key that is registered reports itself as missing on any list long enough for the timing to matter. # Each returns 0 registered, 1 not registered, and 2 when the answer could not be read at all. -# The third is not the second: a momentary failure to reach GitHub reported as "not registered" sends the reader to register a key that is already there. +# The third is not the second. +# A momentary failure to reach GitHub reported as "not registered" sends the reader to register a key that is already there. github_auth_key_registered() { local body keys body=$(key_body) || return 2 @@ -304,7 +326,9 @@ github_signing_key_registered() { # A registration is a browser step, so this prints what to paste and where, then stops. # The consequence of not registering differs per caller, so each says its own rather than this saying one for all. -# Authentication failing stops everything that reaches GitHub, signing not being registered stops nothing locally, and a host authenticating with another key is working and not managed. +# Authentication failing stops everything that reaches GitHub. +# Signing not being registered stops nothing locally. +# A host authenticating with another key is working and not managed. registration_needed() { local kind="$1" type="$2" log "" @@ -347,7 +371,8 @@ git_config_add_once() { } # The flag wins, then whatever the host already carries, then the default. -# Reading the host first is what keeps a machine configured for somebody else from being rewritten by a run meant to be safe to repeat, and it is why the defaults are not written into the variables at the top. +# Reading the host first is what keeps a machine configured for somebody else from being rewritten by a run meant to be safe to repeat. +# Reading the host first is also why the defaults are not written into the variables at the top. resolve_identity() { local existing @@ -381,7 +406,8 @@ configure_git() { } # A checkout several accounts share needs two settings that are relaxations rather than defaults, so they are applied only for a path the caller names. -# The safe.directory setting turns off the ownership check git added to stop a repository owned by someone else running its hooks as you, and widening it to every path on a host is a different decision from widening it to one directory that is known to be shared. +# The safe.directory setting turns off the ownership check git added to stop a repository owned by someone else running its hooks as you. +# Widening that setting to every path on a host is a different decision from widening it to one directory that is known to be shared. # A host with one account needs neither, which is why nothing here runs without the flag. configure_shared_checkout() { [[ -n $SHARED_CHECKOUT ]] || return 0 @@ -438,7 +464,8 @@ configure_signing() { } # Sign a commit in a throwaway repository and verify it. -# This proves the configuration end to end, which reading the settings back cannot: a wrong allowed signers entry reads as correct and fails only when a signature is checked. +# This proves the configuration end to end, which reading the settings back cannot. +# A wrong allowed signers entry reads as correct and fails only when a signature is checked. signing_works() { local repo="$TMP_DIR/signing-check" rm -rf "$repo" @@ -601,7 +628,9 @@ status() { fi done - # The shared checkout settings are reported apart, because absent is the right state for a host one account uses and listing them as missing would read as two gaps to close. + # The shared checkout settings are reported apart, because of the following. + # - Absent is the right state for a host one account uses. + # - Listing them as missing would read as two gaps to close. local -a shared=() readarray -t shared < <(git config --global --get-all safe.directory 2>/dev/null || true) if [[ ${#shared[@]} -gt 0 ]]; then @@ -638,7 +667,10 @@ configure() { ensure_known_host # The local configuration comes first, because none of it needs GitHub. - # A host that cannot authenticate yet, which is every host between creating its key and registering it, still ends this run with its identity, its signing configuration, and a commit that verifies locally. + # A host that cannot authenticate yet, which is every host between creating its key and registering it, still ends this run with: + # - Its identity. + # - Its signing configuration. + # - A commit that verifies locally. configure_git configure_shared_checkout configure_signing @@ -653,7 +685,8 @@ configure() { die "A commit did not verify. Check $ALLOWED_SIGNERS holds this host's key against $EMAIL." fi - # Authentication is the gate for everything GitHub answers: without it this host reaches no private repository, and the registration check below has no account to read. + # Authentication is the gate for everything GitHub answers. + # Without it this host reaches no private repository, and the registration check below has no account to read. step "Checking SSH authentication to GitHub" resolve_github_user if [[ -z $GITHUB_USER ]]; then @@ -664,13 +697,16 @@ configure() { info "Authenticated as $GITHUB_USER, with the managed key" else # The host reaches GitHub, so this is not a failure, and the key this script manages is still not the one doing it. - # Reporting it as an ordinary line let a run end in "Done" on a host whose managed key was registered nowhere, which is the state this script exists to leave behind. + # Reporting it as an ordinary line let a run end in "Done" on a host whose managed key was registered nowhere. + # A host whose managed key is registered nowhere is the state this script exists to leave behind. registration_needed "authentication" "Authentication" warn "This host authenticates with a key other than the managed one, so the managed key is registered nowhere and revoking this host alone would not cut its access" fi # Signing is the second gate. - # The configuration above is what signs and verifies locally, and the registration is what makes GitHub show the commit as verified, so only one of those two can be done from here. + # Only one of those two can be done from here, because of the following. + # - The configuration above is what signs and verifies locally. + # - The registration is what makes GitHub show the commit as verified. step "Checking the signing key registration" local signing_rc=0 github_signing_key_registered || signing_rc=$? diff --git a/host-setup/linux/upgrade-host.sh b/host-setup/linux/upgrade-host.sh index c853f9681..67e7d85bd 100755 --- a/host-setup/linux/upgrade-host.sh +++ b/host-setup/linux/upgrade-host.sh @@ -1,12 +1,19 @@ #!/bin/bash # Upgrades this host, on Debian and Ubuntu based hosts, Proxmox and WSL included. -# Two levels: the packages of the release the host is on, which is routine, and the release itself, which is not and so is a separate action behind its own flag. +# Two levels: +# - The packages of the release the host is on, which is routine. +# - The release itself, which is not and so is a separate action behind its own flag. # -# A release upgrade is refused on a host this script cannot carry across safely: Proxmox, whose major upgrades are a documented procedure with its own preconditions, and a distribution that is neither Debian nor Ubuntu, where the sources rewrite below has no meaning. -# Refusing is the whole point of running this rather than apt by hand: the package upgrade is the same everywhere, and the release upgrade is where hosts differ. +# A release upgrade is refused on a host this script cannot carry across safely: +# - Proxmox, whose major upgrades are a documented procedure with its own preconditions. +# - A distribution that is neither Debian nor Ubuntu, where the sources rewrite below has no meaning. +# Refusing is the whole point of running this rather than apt by hand. +# The reason is that the package upgrade is the same everywhere, and the release upgrade is where hosts differ. # -# One script rather than one per host: the package path, the WSL and reboot handling, and the command line are shared, and only the release path branches. +# One script rather than one per host: +# - The package path, the WSL and reboot handling, and the command line are shared. +# - Only the release path branches. set -Eeuo pipefail @@ -99,7 +106,9 @@ detect_host() { fi if [[ $ASSUME_YES == true ]]; then - # Keep the installed config file on a packaging conflict, since an unattended run has nobody to answer the prompt, and a replaced config is the harder half to notice afterwards. + # Keep the installed config file on a packaging conflict, because of the following. + # - An unattended run has nobody to answer the prompt. + # - A replaced config is the harder half to notice afterwards. export DEBIAN_FRONTEND=noninteractive APT_OPTS=(-y -o Dpkg::Options::=--force-confdef -o Dpkg::Options::=--force-confold) fi @@ -137,7 +146,8 @@ confirm() { [[ $reply == [yY] || $reply == [yY][eE][sS] ]] } -# A minimal install carries no curl, and without this guard the caller sees "command not found" from inside a command substitution, which reads as an answer rather than as a failure. +# A minimal install carries no curl. +# Without this guard the caller sees "command not found" from inside a command substitution, which reads as an answer rather than as a failure. fetch() { command -v curl >/dev/null || return 1 curl -fsSL --retry 2 --connect-timeout 10 "$@" @@ -174,12 +184,16 @@ refresh_snaps() { } upgradable_count() { - # A failed listing and a listing that genuinely found nothing upgradable both read as "no matches" through grep alone, so the two are told apart here rather than both printing 0. - # This only ever backs a status report, so the answer here is "unknown" rather than a die: nothing downstream mutates on the strength of this count. + # A failed listing and a listing that genuinely found nothing upgradable both read as "no matches" through grep alone. + # So the two are told apart here rather than both printing 0. + # The answer here is "unknown" rather than a die, because of the following. + # - This only ever backs a status report. + # - Nothing downstream mutates on the strength of this count. local out status=0 out=$(apt list --upgradable 2>/dev/null) || status=$? if [[ $status -ne 0 ]]; then - # Re-run once more, stdout discarded this time, so the diagnostic comes from a pipe head bounds in memory rather than a file this would otherwise have to size-cap and clean up itself. + # Re-run once more, stdout discarded this time. + # So the diagnostic comes from a pipe head bounds in memory, rather than a file this would otherwise have to size-cap and clean up itself. printf 'unknown, apt list --upgradable failed (exit %s): %s' "$status" "$(apt list --upgradable 2>&1 >/dev/null | head -c 200 | tr '\n' ' ')" return 0 fi @@ -250,7 +264,8 @@ release_preconditions() { die "Held packages block a release upgrade, unhold them first: $held" fi - # A dpkg --audit that fails to run is not the same as one that runs and finds nothing, and only the second one clears the way into a release upgrade. + # A dpkg --audit that fails to run is not the same as one that runs and finds nothing. + # Only the one that runs and finds nothing clears the way into a release upgrade. local audit status=0 audit=$("${SUDO[@]}" dpkg --audit 2>&1) || status=$? if [[ $status -ne 0 ]]; then @@ -289,7 +304,9 @@ backup_sources() { run_root install -d -m 0755 "$target" # A missing file is skipped, since a host on deb822 alone carries no sources.list. - # A copy that fails for any other reason stops the upgrade: this backup is the only way back, and finding out it was never taken belongs before the sources are rewritten rather than after. + # A copy that fails for any other reason stops the upgrade, because of the following. + # - This backup is the only way back. + # - Finding out it was never taken belongs before the sources are rewritten rather than after. if [[ -f /etc/apt/sources.list ]]; then run_root cp -a /etc/apt/sources.list "$target/" fi @@ -301,7 +318,8 @@ backup_sources() { # --- Release, Debian --- # The fetch is captured before it is parsed rather than piped into the parser. -# Under pipefail a reader that stops early, as a parser looking for one line does, leaves the fetch writing to a closed pipe, and a successful download then reports itself as a failure. +# Under pipefail a reader that stops early, as a parser looking for one line does, leaves the fetch writing to a closed pipe. +# A successful download then reports itself as a failure. debian_suite_codename() { local release release=$(fetch "https://deb.debian.org/debian/dists/$1/Release" 2>/dev/null) || return 1 @@ -309,9 +327,15 @@ debian_suite_codename() { } # The release after the one this host is on. -# Both distributions support one step at a time, so this never reaches past the next one, and a host further behind is upgraded by running the script again. -# Returns 0 and prints the next release, 1 when the host is already on the newest, and 2 when the index could not be read at all. -# The last two are not the same answer: a host that cannot reach the index is not a host with nothing to upgrade to, and reporting it as one is the dangerous reading. +# Both distributions support one step at a time, so this never reaches past the next one. +# A host further behind is upgraded by running the script again. +# Returns: +# - 0 and prints the next release. +# - 1 when the host is already on the newest. +# - 2 when the index could not be read at all. +# The last two are not the same answer. +# A host that cannot reach the index is not a host with nothing to upgrade to. +# Reporting it as one is the dangerous reading. debian_next_codename() { local current="$1" suite previous="" codename read_any=false @@ -331,8 +355,10 @@ debian_next_codename() { } # Sources that point somewhere other than Debian's own mirrors. -# The new release may not have a suite for them yet, so they are named rather than rewritten, and what to do about them is the operator's call. -# Each entry is judged on its own URI rather than by the file holding it, since one file can hold both a Debian mirror and a third party repository and the second one still breaks the update. +# The new release may not have a suite for them yet, so they are named rather than rewritten. +# What to do about them is the operator's call. +# Each entry is judged on its own URI rather than by the file holding it. +# That is because one file can hold both a Debian mirror and a third party repository, and the second one still breaks the update. debian_third_party_sources() { local file @@ -376,7 +402,8 @@ debian_third_party_sources() { } # Rewrite the release codename in the sources that point at Debian's own mirrors, in both the legacy one-line format and deb822. -# A deb822 stanza is rewritten as a unit, because the URI that decides whether it is Debian's sits on a different line from the suite that names the release. +# A deb822 stanza is rewritten as a unit. +# That is because the URI that decides whether it is Debian's sits on a different line from the suite that names the release. debian_rewrite_sources() { local from="$1" to="$2" file staged rewritten=0 @@ -432,7 +459,9 @@ debian_rewrite_sources() { info "Rewrote $file" done - # Naming what was looked for rather than what was missing: a host on a mirror of its own still names the codename, and being told no source names it would send the reader looking for the wrong thing. + # Naming what was looked for rather than what was missing: + # - A host on a mirror of its own still names the codename. + # - Being told no source names it would send the reader looking for the wrong thing. [[ $rewritten -gt 0 ]] || die "No apt source pointing at a Debian mirror names \"$from\", so there is nothing to rewrite. A host on a mirror other than one under debian.org, or one tracking \"stable\" rather than a codename, is moved by editing its sources itself." } @@ -471,7 +500,8 @@ debian_release_upgrade() { step "Reading the $next package lists" apt_get update - # The release notes take the minimal upgrade first, so the packages that need no new dependencies land before the ones that pull in a changed dependency set. + # The release notes take the minimal upgrade first. + # So the packages that need no new dependencies land before the ones that pull in a changed dependency set. step "Upgrading without new packages" apt_get upgrade --without-new-pkgs @@ -544,7 +574,8 @@ release_summary() { printf 'unknown until update-manager-core is installed' return 0 fi - # The check exits non-zero when nothing is offered, and prints its progress either way, so the exit code decides and the output only names the release. + # The check exits non-zero when nothing is offered, and prints its progress either way. + # So the exit code decides and the output only names the release. # It reads the same as any user, so it runs without sudo and a status report never asks for a password. local check summary if check=$(do-release-upgrade -c 2>&1); then diff --git a/host-setup/menu.ps1 b/host-setup/menu.ps1 index c3298933f..295b4ae7a 100644 --- a/host-setup/menu.ps1 +++ b/host-setup/menu.ps1 @@ -1,10 +1,18 @@ -# A human-facing front end over the scripts this fleet otherwise authors for an agent following instructions: the host tooling in host-setup\windows\, and the repo-level tools in scripts\ and spec\ that ptr727/ProjectTemplate hosts and every other repo reaches rather than carries. -# Menu options rather than a command a human has to already know, and a forcing function on the tools it fronts: a task with no discoverable menu entry is a gap in the tools themselves. +# A human-facing front end over the scripts this fleet otherwise authors for an agent following instructions. +# The scripts are: +# - the host tooling in host-setup\windows\ +# - the repo-level tools in scripts\ and spec\ that ptr727/ProjectTemplate hosts and every other repo reaches rather than carries +# Menu options rather than a command a human has to already know, and a forcing function on the tools it fronts. +# A task with no discoverable menu entry is a gap in the tools themselves. # -# Fetchable on its own, like bootstrap.ps1: run from a hub checkout directly, or download this one file into a downstream repo and it clones the hub itself. -# Where bootstrap.ps1 stands a host up and stops, this loops so a human answers more than one question in a sitting, and it knows the difference between "the hub" and "a repo this host happens to be sitting in" so it can offer each their own tasks. +# Fetchable on its own, like bootstrap.ps1. +# Run it from a hub checkout directly, or download this one file into a downstream repo and it clones the hub itself. +# Where bootstrap.ps1 stands a host up and stops, this loops so a human answers more than one question in a sitting. +# This menu knows the difference between "the hub" and "a repo this host happens to be sitting in". +# So this menu can offer each their own tasks. # -# Runs under Windows PowerShell 5.1, the one shell a fresh Windows host guarantees, and hands off to PowerShell 7 the same way bootstrap.ps1 does, since every host-setup\windows script it drives requires that version. +# Runs under Windows PowerShell 5.1, the one shell a fresh Windows host guarantees. +# This script hands off to PowerShell 7 the same way bootstrap.ps1 does, since every host-setup\windows script it drives requires that version. [CmdletBinding()] param( @@ -16,7 +24,8 @@ param( [Alias('h')][switch]$Help ) -# Captured before anything else touches scope, so the pwsh handoff below can rebuild the argument list this process was bound with, rather than reaching for an automatic variable from inside a nested function. +# Captured before anything else touches scope, so the pwsh handoff below can rebuild the argument list this process was bound with. +# This avoids reaching for an automatic variable from inside a nested function. $SCRIPT_BOUND_PARAMETERS = $PSBoundParameters Set-StrictMode -Version Latest @@ -28,14 +37,17 @@ $HUB_REPO = 'ptr727/ProjectTemplate' $HUB_URL = "https://github.com/$HUB_REPO" $DEFAULT_REF = 'main' -# Every parameter is read into a variable here rather than from inside a function, matching bootstrap.ps1: a parameter reached only from a nested scope reads as declared and never used. +# Every parameter is read into a variable here rather than from inside a function, matching bootstrap.ps1. +# A parameter reached only from a nested scope reads as declared and never used. $DRY_RUN = [bool]$DryRun $ASSUME_YES = [bool]$Yes $REF = if ($Ref) { $Ref } else { $DEFAULT_REF } $KEEP = [bool]$Keep $WANT_HELP = [bool]$Help -# No placeholder for $DIR here, matching bootstrap.ps1: PowerShell variable names are case-insensitive, so $DIR and the -Dir parameter $Dir are the same variable, and Resolve-Directory's own assignment in main is what gives it its resolved value. +# No placeholder for $DIR here, matching bootstrap.ps1. +# PowerShell variable names are case-insensitive, so $DIR and the -Dir parameter $Dir are the same variable. +# Resolve-Directory's own assignment in main is what gives that variable its resolved value. $HUB_ROOT = '' $HUB_FETCHED = $false $HUB_FETCH_TOKEN = '' @@ -54,7 +66,8 @@ function info { param([string]$Message) Write-Host " $Message" } function step { param([string]$Message) Write-Host "`n==> $Message" } function warn { param([string]$Message) [Console]::Error.WriteLine("WARNING: $Message") } function die { param([string]$Message) [Console]::Error.WriteLine("ERROR: $Message"); exit 1 } -# Reports a task-time error without ending the process, unlike die: a dispatched action's failure returns to the menu, and only a startup failure (bad arguments, no git) is fatal. +# Reports a task-time error without ending the process, unlike die. +# A dispatched action's failure returns to the menu, and only a startup failure (bad arguments, no git) is fatal. function fail { param([string]$Message) [Console]::Error.WriteLine("ERROR: $Message") } function usage { @@ -84,9 +97,11 @@ redirected run is not a place to answer a menu. # --- pwsh handoff --- # -# Everything above this point, and the four functions below, run under Windows PowerShell 5.1: no ternary or null-coalescing operator, no multi-argument Join-Path, nothing newer than that runtime parses. +# Everything above this point, and the four functions below, run under Windows PowerShell 5.1. +# That means no ternary or null-coalescing operator, no multi-argument Join-Path, nothing newer than that runtime parses. # Everything past the handoff may use whatever pwsh 7 accepts, though it mostly does not need to. -# Identical in shape to bootstrap.ps1's own handoff, kept as a separate copy rather than a shared module: a script fetched alone would otherwise fail on a missing sibling file. +# Identical in shape to bootstrap.ps1's own handoff, kept as a separate copy rather than a shared module. +# A script fetched alone would otherwise fail on a missing sibling file. function Resolve-Pwsh { $command = Get-Command pwsh -ErrorAction SilentlyContinue @@ -130,7 +145,9 @@ function Get-ForwardedArgument { } # The same parameters Get-ForwardedArgument forwards, rendered as one pasteable command-line string rather than split into argv elements. -# Classified by SCRIPT_BOUND_PARAMETERS's own type rather than by a value's shape: a value that happens to look like a flag (-Ref '-Yes') would otherwise print unquoted and bind as its own switch when pasted, which is exactly the bug guessing from shape produced here before. +# Classified by SCRIPT_BOUND_PARAMETERS's own type rather than by a value's shape. +# A value that happens to look like a flag (-Ref '-Yes') would otherwise print unquoted and bind as its own switch when pasted. +# That is exactly the bug guessing from shape produced here before. function Format-ForwardedArgumentForDisplay { $parts = foreach ($key in $script:SCRIPT_BOUND_PARAMETERS.Keys) { $value = $script:SCRIPT_BOUND_PARAMETERS[$key] @@ -154,7 +171,8 @@ function Invoke-PwshHandoff { # --- Hub resolution --- -# The owner/name from a remote.origin.url in any of the shapes git or the GitHub UI hand out, or empty where the checkout carries no origin at all. +# The owner/name from a remote.origin.url in any of the shapes git or the GitHub UI hand out. +# Empty where the checkout carries no origin at all. function Get-OriginSlug { param([Parameter(Mandatory)][string]$Root) $url = (& git -C $Root config --get remote.origin.url 2>$null) @@ -167,7 +185,8 @@ function Get-OriginSlug { return $slug } -# A marker sitting beside the clone rather than inside it, so a -Dir pointed at a directory this run does not own is never the one removed on exit. +# A marker sitting beside the clone rather than inside it. +# So a -Dir pointed at a directory this run does not own is never the one removed on exit. function Get-MarkerPath { Join-Path $script:DIR 'hub.owned' } # Answers whether an existing $DIR\hub may be removed: absent, or created by this run. @@ -187,8 +206,11 @@ function Test-HubFetchedHere { function Get-HubLockPath { Join-Path $script:DIR 'hub.lock' } -# The file menu.sh locks, held with FileShare.None, which .NET implements as an exclusive flock on Linux and which on Windows refuses any other open handle, so the two menus sharing one -Dir serialize against each other. -# A session that dies releases it, and the file stays in place, matching menu.sh, since removing it while held would let a third session lock a new file under the same name. +# The file menu.sh locks, held with FileShare.None. +# On Linux, .NET implements FileShare.None as an exclusive flock, and on Windows FileShare.None refuses any other open handle. +# So the two menus sharing one -Dir serialize against each other. +# A session that dies releases it, and the file stays in place, matching menu.sh. +# That is because removing it while held would let a third session lock a new file under the same name. function Lock-Hub { $lock = Get-HubLockPath try { @@ -202,7 +224,8 @@ function Lock-Hub { try { return [IO.File]::Open($lock, 'OpenOrCreate', 'ReadWrite', 'None') } catch [System.Management.Automation.MethodInvocationException] { - # A held lock is a sharing or lock violation on Windows and the flock's EWOULDBLOCK on Linux, each carried in the HResult, where a full disk or a read-only mount is a plain IOException too and would otherwise be waited on forever. + # A held lock is a sharing or lock violation on Windows and the flock's EWOULDBLOCK on Linux, each carried in the HResult. + # A full disk or a read-only mount is a plain IOException too, and would otherwise be waited on forever. # The open does not block on either platform, so a held lock is waited out by retrying it. $reason = $_.Exception.InnerException $code = $reason.HResult -band 0xFFFF @@ -222,14 +245,29 @@ function Lock-Hub { function Invoke-WithHubLock { # The single lock over $script:DIR\hub, shared by every writer (Invoke-FetchHub, Invoke-Cleanup) and every reader (Invoke-HostTool, and each action's Confirm-HubRoot-then-Invoke-HubUvScript span) alike. - # Unlike menu.sh, which gives readers a genuine shared mode, every caller here takes the same exclusive lock, trading reader concurrency for a locking scheme simple enough to get right. - # That trade is not always brief: Invoke-HostTool holds this lock for its entire spawned tool run, which can be a long OS package upgrade, so a second session waiting here can wait as long as that run takes, not just for a quick read. - # It still closes the actual TOCTOU: a concurrent Invoke-FetchHub's Remove-Item can no longer land between a reader confirming $HUB_ROOT is fresh and that reader actually using it, since both now hold this same lock for that whole span, not just around the read's own final call. - # A span already holding the lock runs the scriptblock as it is, since Invoke-FetchHub is reached from inside one through Confirm-HubRoot, and a second open of the file would wait on this session's own lock forever. - # -DryRun runs it unlocked too, matching menu.sh, since a dry run never fetches or reads $DIR\hub, and it must not create $DIR or a lock file where neither existed. - # A lock that cannot be taken at all returns $Failed without running the scriptblock, the value each caller already returns for its own task failing, since a $false would read as exit code 0 to a caller comparing it with -eq. + # Unlike menu.sh, which gives readers a genuine shared mode, every caller here takes the same exclusive lock. + # This trades reader concurrency for a locking scheme simple enough to get right. + # That trade is not always brief. + # Invoke-HostTool holds this lock for its entire spawned tool run, which can be a long OS package upgrade. + # So a second session waiting here can wait as long as that run takes, not just for a quick read. + # This locking scheme still closes the actual TOCTOU. + # A concurrent Invoke-FetchHub's Remove-Item can no longer land between a reader confirming $HUB_ROOT is fresh and that reader actually using it. + # That is because both the reader and the fetch now hold this same lock for that whole span. + # The lock is not held just around the read's own final call. + # A span already holding the lock runs the scriptblock as it is. + # That is because of the following. + # - Invoke-FetchHub is reached from inside such a span through Confirm-HubRoot. + # - A second open of the file would wait on this session's own lock forever. + # -DryRun runs it unlocked too, matching menu.sh, since a dry run never fetches or reads $DIR\hub. + # That is also because a dry run must not create $DIR or a lock file where neither existed. + # A lock that cannot be taken at all returns $Failed without running the scriptblock. + # That is the value each caller already returns for its own task failing. + # That is the caller's own value rather than a fixed $false. + # The reason is that a $false would read as exit code 0 to a caller comparing it with -eq. # ArgumentList is forwarded to the scriptblock positionally (its own param() block names them), rather than relying on the scriptblock closing over the caller's variables directly. - # Passed this way, PSScriptAnalyzer's PSReviewUnusedParameter sees the caller's own parameters referenced at the call site, where a bare closure reads as an unused parameter to it, since the rule does not trace a variable read inside a nested scriptblock back to the enclosing function's own param() block. + # Passed this way, PSScriptAnalyzer's PSReviewUnusedParameter sees the caller's own parameters referenced at the call site. + # A bare closure reads as an unused parameter to that rule instead. + # That is because the rule does not trace a variable read inside a nested scriptblock back to the enclosing function's own param() block. param([Parameter(Mandatory)][scriptblock]$ScriptBlock, [object[]]$ArgumentList = @(), [object]$Failed = 1) if ($script:HUB_LOCK -or $script:DRY_RUN) { return (& $ScriptBlock @ArgumentList) } $script:HUB_LOCK = Lock-Hub @@ -248,8 +286,11 @@ function Invoke-FetchHub { fail "This task needs a fetched hub checkout, and fetching one is itself a change -DryRun does not make. Run without -DryRun, or from inside a hub checkout already on $script:DEFAULT_REF." return $false } - # Held for the fetch's own span rather than for the rest of this session: Invoke-InteractiveMenu's loop keeps a session alive well past its one fetch, and holding the lock that long would block every other menu sharing this -Dir until this session quits. - # A second session starting its own fetch while this one is still reading the tree it just cloned is not a residual race: every read (Invoke-HostTool, and each action's Confirm-HubRoot-then-Invoke-HubUvScript span) takes this same lock itself, via Invoke-WithHubLock, for its own whole span. + # Held for the fetch's own span rather than for the rest of this session. + # Invoke-InteractiveMenu's loop keeps a session alive well past its one fetch. + # Holding the lock that long would block every other menu sharing this -Dir until this session quits. + # A second session starting its own fetch while this one is still reading the tree it just cloned is not a residual race. + # That is because every read (Invoke-HostTool, and each action's Confirm-HubRoot-then-Invoke-HubUvScript span) takes this same lock itself, via Invoke-WithHubLock, for its own whole span. return (Invoke-WithHubLock -Failed $false -ScriptBlock { Invoke-FetchHubLocked }) } @@ -265,7 +306,9 @@ function Invoke-FetchHubLocked { return $false } } - # Marked as ours before git can create anything under $hubPath, not only once the clone also succeeds: git can leave a partial directory behind on a failed or interrupted clone, and an unmarked one would then block every retry until removed by hand. + # Marked as ours before git can create anything under $hubPath, not only once the clone also succeeds. + # Git can leave a partial directory behind on a failed or interrupted clone. + # An unmarked one would then block every retry until removed by hand. $token = [guid]::NewGuid().ToString('N') try { Set-Content -LiteralPath (Get-MarkerPath) -Value $token @@ -274,17 +317,24 @@ function Invoke-FetchHubLocked { return $false } $script:HUB_FETCH_TOKEN = $token - # A full clone of the default branch first, whatever -Ref names: spec\audit.py walks the hub's own history to judge whether a carried copy is trailing the file it was copied from, and a shallow clone would read every file as changed at the truncation boundary. - # Piped to Out-Host rather than left bare: an unassigned native call's stdout otherwise joins this function's own return value, which return $true/$false below would then be appended to instead of replacing. + # A full clone of the default branch first, whatever -Ref names. + # That is because of the following. + # - spec\audit.py walks the hub's own history to judge whether a carried copy is trailing the file it was copied from. + # - A shallow clone would read every file as changed at the truncation boundary. + # Piped to Out-Host rather than left bare. + # An unassigned native call's stdout otherwise joins this function's own return value. + # The return $true/$false below would then be appended to that value instead of replacing it. & git clone --quiet --branch $script:DEFAULT_REF --single-branch $script:HUB_URL $hubPath | Out-Host if ($LASTEXITCODE -ne 0) { fail "Could not clone $script:HUB_REPO. Check that this host reaches github.com." - # The marker goes with the directory it marked: an orphaned one left behind would grant false ownership to whatever unrelated directory a person later puts at this same path. + # The marker goes with the directory it marked. + # An orphaned one left behind would grant false ownership to whatever unrelated directory a person later puts at this same path. Remove-Item -LiteralPath $hubPath -Recurse -Force -ErrorAction SilentlyContinue Remove-Item -LiteralPath (Get-MarkerPath) -Force -ErrorAction SilentlyContinue return $false } - # Set the moment the clone lands, whatever -Ref still has to do: Invoke-Cleanup is gated on this, and a later ref-specific failure below must still remove the tree this step already created. + # Set the moment the clone lands, whatever -Ref still has to do. + # Invoke-Cleanup is gated on this flag, and a later ref-specific failure below must still remove the tree this step already created. $script:HUB_FETCHED = $true if ($script:REF -ne $script:DEFAULT_REF) { & git -C $hubPath fetch --quiet origin $script:REF | Out-Host @@ -315,7 +365,8 @@ function Test-HubCheckout { } # Whether HUB_ROOT is clean and sitting on $DEFAULT_REF, with no assumption that it agrees with origin. -# A failed git call must never read the same as its emptiest possible success, so $LASTEXITCODE is checked after each one rather than only its output. +# A failed git call must never read the same as its emptiest possible success. +# So $LASTEXITCODE is checked after each one rather than only its output. function Test-HubCleanOnDefaultRef { $status = & git -C $script:HUB_ROOT status --porcelain if ($LASTEXITCODE -ne 0 -or $status) { return $false } @@ -343,7 +394,9 @@ function Get-HubRefCommit { function Confirm-HubRoot { if ($script:HUB_ROOT) { if ($script:DRY_RUN) { - # No network call, since confirming freshness against origin means fetching, which -DryRun does not do, but this still confirms the checkout is clean and actually on main: Test-HubCheckout checked only origin identity, and a dirty or feature-branch checkout must not be read as the hub's main either. + # No network call, since confirming freshness against origin means fetching, which -DryRun does not do. + # This still confirms the checkout is clean and actually on main. + # That is because Test-HubCheckout checked only origin identity, and a dirty or feature-branch checkout must not be read as the hub's main either. if (Test-HubCleanOnDefaultRef) { return $true } $script:HUB_ROOT = '' } else { @@ -381,12 +434,16 @@ function Test-DownstreamCheckout { function Invoke-Cleanup { if ($script:KEEP -or -not $script:HUB_FETCHED) { return } if (-not (Test-Path -LiteralPath (Get-MarkerPath))) { return } - # Same exclusive lock every reader and writer takes, via Invoke-WithHubLock, so this waits out another session still mid-use rather than deleting the tree out from under it. - # No -ArgumentList needed: the scriptblock only reads script-scoped state ($script:DIR, Get-MarkerPath), not a local parameter of this function, so there is nothing for PSReviewUnusedParameter to flag here the way Invoke-HostTool's own $Tool/$Arguments needed forwarding. + # Same exclusive lock every reader and writer takes, via Invoke-WithHubLock. + # So this waits out another session still mid-use rather than deleting the tree out from under it. + # No -ArgumentList needed. + # The scriptblock only reads script-scoped state ($script:DIR, Get-MarkerPath), not a local parameter of this function. + # So there is nothing for PSReviewUnusedParameter to flag here the way Invoke-HostTool's own $Tool/$Arguments needed forwarding. Invoke-WithHubLock -ScriptBlock { if (-not (Test-HubFetchedHere)) { return } $hubPath = Join-Path $script:DIR 'hub' - # The marker is removed only once the directory it marks is actually gone, rather than unconditionally alongside it: a suppressed removal failure must not leave a leftover hub with no marker to explain it. + # The marker is removed only once the directory it marks is actually gone, rather than unconditionally alongside it. + # A suppressed removal failure must not leave a leftover hub with no marker to explain it. Remove-Item -LiteralPath $hubPath -Recurse -Force -ErrorAction SilentlyContinue if (-not (Test-Path -LiteralPath $hubPath)) { Remove-Item -LiteralPath (Get-MarkerPath) -Force -ErrorAction SilentlyContinue } } @@ -395,10 +452,12 @@ function Invoke-Cleanup { # --- Running a tool --- # Every host tool runs from inside the hub tree, and this is the only place a path inside it is named. -# Spawned as its own pwsh process rather than dot-sourced, since every host-setup\windows script ends its own main with exit, which would otherwise end this menu too. +# Spawned as its own pwsh process rather than dot-sourced. +# That is because every host-setup\windows script ends its own main with exit, which would otherwise end this menu too. function Invoke-HostTool { param([Parameter(Mandatory)][string]$Tool, [Parameter(ValueFromRemainingArguments)][string[]]$Arguments) - # The whole body runs inside Invoke-WithHubLock, Confirm-HubRoot included, not just the final invocation, since locking only around the last call would still leave Confirm-HubRoot's own reads unlocked, the same gap menu.sh's equivalent fix closes. + # The whole body runs inside Invoke-WithHubLock, Confirm-HubRoot included, not just the final invocation. + # That is because locking only around the last call would still leave Confirm-HubRoot's own reads unlocked, the same gap menu.sh's equivalent fix closes. return (Invoke-WithHubLock -ArgumentList @($Tool, $Arguments) -ScriptBlock { param($Tool, $Arguments) if (-not (Confirm-HubRoot)) { return 1 } @@ -439,7 +498,8 @@ function Invoke-AgentsInstall { return (Invoke-HostTool -Tool 'install-agents.ps1' -Arguments (@('-Install') + $words)) } -# The Python tools under scripts\ and spec\ resolve their own root from __file__ rather than the working directory, so they are called by absolute path from wherever this script runs. +# The Python tools under scripts\ and spec\ resolve their own root from __file__ rather than the working directory. +# So they are called by absolute path from wherever this script runs. # Checked here rather than upfront in main, since a host with no uv yet can still use every host action. # Only the actions that need uv name it as their own prerequisite. # Every hub tool a task runs declares its libraries inline, so uv runs each one rather than a bare Python, which has none of them. @@ -470,7 +530,8 @@ function Invoke-AuditRepo { function Invoke-CheckSkillsDist { $rc = Invoke-WithHubLock -Failed 2 -ScriptBlock { # 2, not 1: scripts\build_dist.py --check documents 1 as its own "stale" result, and the switch below reads that value as a genuine check outcome. - # A Confirm-HubRoot failure must not collide with it, or a failed confirmation reports as "stale" with exit code 0 instead of the failure it actually is. + # A Confirm-HubRoot failure must not collide with it. + # Otherwise a failed confirmation reports as "stale" with exit code 0 instead of the failure it actually is. if (-not (Confirm-HubRoot)) { return 2 } return (Invoke-HubUvScript -ScriptPath 'scripts/build_dist.py' -Arguments '--check') } @@ -492,7 +553,8 @@ function Invoke-CarryAction { fail "No downstream repo checkout found. Run this menu from inside the target repo's own worktree." return 1 } - # A non-default -Ref checks out something other than the origin/main scripts/carry.py's hub argument requires, so this is refused here with the actual reason rather than failing deep inside that tool. + # A non-default -Ref checks out something other than the origin/main scripts/carry.py's hub argument requires. + # So this is refused here with the actual reason rather than failing deep inside that tool. if ($script:REF -ne $script:DEFAULT_REF) { fail "Pulling hub files needs the hub's $script:DEFAULT_REF branch, and this session was started with -Ref $script:REF. Run without -Ref, or start a separate session on $script:DEFAULT_REF for this task." return 1 @@ -531,7 +593,9 @@ function Show-Menu { log ' 5 Upgrade the host OS packages' log ' 6 Report git and GitHub setup' log ' 7 Configure git and GitHub' - # Only setup-wsl.ps1's -Status runs here: its -Install needs a distribution name, which no menu entry collects, and installing one nobody asked for is exactly what this stays out of. + # Only setup-wsl.ps1's -Status runs here. + # The -Install action of setup-wsl.ps1 needs a distribution name, which no menu entry collects. + # Installing one nobody asked for is exactly what this stays out of. log ' 8 Report the WSL platform and the distributions installed' log ' 9 Report fleet Skills install status' log ' 10 Install or update the fleet Skills' @@ -556,7 +620,9 @@ function Show-Menu { log '' } -# A failing task is reported and returns to the menu rather than ending the session, so this cannot double as "quit": QUIT is a separate flag the q/Q case sets, read by the loop after every dispatch regardless of whether the task it ran succeeded. +# A failing task is reported and returns to the menu rather than ending the session. +# So the dispatch return value cannot double as "quit". +# QUIT is a separate flag the q/Q case sets, read by the loop after every dispatch regardless of whether the task it ran succeeded. function Invoke-Dispatch { param([string]$Choice) switch ($Choice) { @@ -577,7 +643,8 @@ function Invoke-Dispatch { '15' { return (Invoke-CheckSkillsDist) } '16' { return (Invoke-CarryAction 'check') } '17' { - # -DryRun changes nothing, and scripts/carry.py itself has no dry-run mode, so a dry-run apply reads as its own check instead of silently mutating the downstream worktree. + # -DryRun changes nothing, and scripts/carry.py itself has no dry-run mode. + # So a dry-run apply reads as its own check instead of silently mutating the downstream worktree. if ($script:DRY_RUN) { return (Invoke-CarryAction 'check') } return (Invoke-CarryAction 'apply') } @@ -603,7 +670,9 @@ function Invoke-InteractiveMenu { $script:BAD_CHOICE = $false $rc = Invoke-Dispatch $choice if ($script:QUIT) { break } - # A dedicated flag rather than a reserved return code: every dispatched task's own exit code passes through unchanged, and a tool that happens to exit 2 for its own reason (scripts/carry.py's "not uniquely registered", for one) must not be misread as an unrecognized choice. + # A dedicated flag rather than a reserved return code. + # Every dispatched task's own exit code passes through unchanged. + # A tool that happens to exit 2 for its own reason (scripts/carry.py's "not uniquely registered", for one) must not be misread as an unrecognized choice. if ($script:BAD_CHOICE) { continue } if ($rc -eq 0) { step 'Done' @@ -635,11 +704,15 @@ function Resolve-Directory { return $trimmed } -# The lines a piped-in run is told to paste instead, shared by both places that print them: the handoff below needs a real file to hand off to pwsh, and Test-Interactive needs a real console to ask on, and a run with neither reaches this the same way. +# The lines a piped-in run is told to paste instead, shared by both places that print them. +# The handoff below needs a real file to hand off to pwsh. +# Test-Interactive needs a real console to ask on. +# A run with neither reaches this the same way. function Show-DownloadAndRunRemedy { info ' [Net.ServicePointManager]::SecurityProtocol = [Net.ServicePointManager]::SecurityProtocol -bor [Net.SecurityProtocolType]::Tls12' info " Invoke-WebRequest -UseBasicParsing -Uri https://raw.githubusercontent.com/$script:HUB_REPO/$script:DEFAULT_REF/host-setup/menu.ps1 -OutFile menu.ps1" - # Reuses Format-ForwardedArgumentForDisplay rather than a second implementation: the original invocation's own flags (-DryRun among them) belong on the re-run this remedy is telling the caller to make. + # Reuses Format-ForwardedArgumentForDisplay rather than a second implementation. + # The original invocation's own flags (-DryRun among them) belong on the re-run this remedy is telling the caller to make. $forwarded = Format-ForwardedArgumentForDisplay $suffix = if ($forwarded) { " $forwarded" } else { '' } info " powershell -ExecutionPolicy Bypass -File menu.ps1$suffix" @@ -649,7 +722,9 @@ function main { if ($script:WANT_HELP) { usage; exit 0 } if ($PSVersionTable.PSVersion.Major -lt 7) { - # $PSCommandPath is empty for a script read through Invoke-Expression or a similar pipe, and the handoff below needs a real path to re-invoke under pwsh, so this cannot wait for Test-Interactive to say the same thing for a different reason. + # $PSCommandPath is empty for a script read through Invoke-Expression or a similar pipe. + # The handoff below needs a real path to re-invoke under pwsh. + # So this cannot wait for Test-Interactive to say the same thing for a different reason. if (-not $PSCommandPath) { warn 'This needs a real file to hand off to PowerShell 7, and a piped-in script has none.' info 'Download the file and run it, rather than piping it:' @@ -672,8 +747,16 @@ function main { if (-not (Get-Command git -ErrorAction SilentlyContinue)) { die 'git is required' } - # Every task runs in a child process piped through this one, so its output is decoded by the child reading a native tool, encoded again by the child writing to this pipe, and decoded by this process reading it, while whatever a tool writes to stderr goes to the console and is rendered there. - # Each of those follows the console's code page, and on an OEM default a UTF-8 tool such as docker desktop prints its check mark as three unrelated characters, so the console is switched to UTF-8 for the menu's lifetime, which a child pwsh inherits at startup, and put back on the way out. + # Every task runs in a child process piped through this one. + # So its output passes through these steps: + # - decoded by the child reading a native tool + # - encoded again by the child writing to this pipe + # - decoded by this process reading it + # Whatever a tool writes to stderr goes to the console and is rendered there. + # Each of those follows the console's code page. + # On an OEM default, a UTF-8 tool such as docker desktop prints its check mark as three unrelated characters. + # So the console is switched to UTF-8 for the menu's lifetime, and put back on the way out. + # A child pwsh inherits the UTF-8 console at startup. $previousEncoding = [Console]::OutputEncoding $switchedEncoding = $false if (-not [Console]::IsOutputRedirected) { diff --git a/host-setup/menu.sh b/host-setup/menu.sh index e3d71f3b6..71387cbe2 100755 --- a/host-setup/menu.sh +++ b/host-setup/menu.sh @@ -1,10 +1,17 @@ #!/usr/bin/env bash -# A human-facing front end over the scripts this fleet otherwise authors for an agent following instructions: the host tooling in host-setup/linux/, and the repo-level tools in scripts/ and spec/ that ptr727/ProjectTemplate hosts and every other repo reaches rather than carries. -# Menu options rather than a command a human has to already know, and a forcing function on the tools it fronts: a task with no discoverable menu entry is a gap in the tools themselves. +# A human-facing front end over the scripts this fleet otherwise authors for an agent following instructions. +# The scripts are: +# - the host tooling in host-setup/linux/ +# - the repo-level tools in scripts/ and spec/ that ptr727/ProjectTemplate hosts and every other repo reaches rather than carries +# Menu options rather than a command a human has to already know, and a forcing function on the tools it fronts. +# A task with no discoverable menu entry is a gap in the tools themselves. # -# Fetchable on its own, like bootstrap.sh: run from a hub checkout directly, or curl this one file into a downstream repo and it clones the hub itself. -# Where bootstrap.sh stands a host up and stops, this loops so a human answers more than one question in a sitting, and it knows the difference between "the hub" and "a repo this host happens to be sitting in" so it can offer each their own tasks. +# Fetchable on its own, like bootstrap.sh. +# Run it from a hub checkout directly, or curl this one file into a downstream repo and it clones the hub itself. +# Where bootstrap.sh stands a host up and stops, this loops so a human answers more than one question in a sitting. +# This menu knows the difference between "the hub" and "a repo this host happens to be sitting in". +# So this menu can offer each their own tasks. set -Eeuo pipefail @@ -25,7 +32,9 @@ DOWNSTREAM_ROOT="" DOWNSTREAM_NAME="" # The reader half of the fetch/use reader-writer lock over $DIR/hub, the writer half being fetch_hub's own exclusive flock, unchanged. -# Global rather than function-local: the fd is opened by hub_read_lock_acquire in one function and closed by hub_read_lock_release in a later, separate call in the same caller, so it has to survive the return in between. +# Global rather than function-local. +# The fd is opened by hub_read_lock_acquire in one function and closed by hub_read_lock_release in a later, separate call in the same caller. +# So the fd has to survive the return in between. HUB_READ_LOCK_FD="" # --- Output --- @@ -38,7 +47,8 @@ die() { printf 'ERROR: %s\n' "$*" >&2 exit 1 } -# Reports a task-time error without ending the process, unlike die: a dispatched action's failure returns to the menu, and only a startup failure (bad arguments, no git) is fatal. +# Reports a task-time error without ending the process, unlike die. +# A dispatched action's failure returns to the menu, and only a startup failure (bad arguments, no git) is fatal. fail() { printf 'ERROR: %s\n' "$*" >&2; } usage() { @@ -67,7 +77,8 @@ EOF # --- Hub resolution --- -# The owner/name from a remote.origin.url in any of the shapes git or the GitHub UI hand out, or empty where the checkout carries no origin at all (a fresh init, or a worktree add mid-flight). +# The owner/name from a remote.origin.url in any of the shapes git or the GitHub UI hand out. +# Empty where the checkout carries no origin at all (a fresh init, or a worktree add mid-flight). origin_slug() { local root="$1" url slug url=$(git -C "$root" config --get remote.origin.url 2>/dev/null) || return 0 @@ -78,7 +89,8 @@ origin_slug() { printf '%s\n' "$slug" } -# A marker sitting beside the clone rather than inside it, so a --dir pointed at a directory this run does not own is never the one removed on exit. +# A marker sitting beside the clone rather than inside it. +# So a --dir pointed at a directory this run does not own is never the one removed on exit. # Inside the clone it would be an untracked file, and carry.py's own hub-is-clean check would then refuse the tree this run just fetched for it. marker_path() { printf '%s\n' "$DIR/hub.owned"; } @@ -98,23 +110,39 @@ hub_fetched_here() { } # Acquires the reader half of the fetch/use lock, held from immediately before ensure_hub_root's own freshness check through the caller's entire use of $HUB_ROOT. -# Scoping it to only a final tool invocation would leave ensure_hub_root's own git reads unlocked, the same interleaving one call frame up from the read this lock exists to close: a concurrent fetch_hub's rm -rf could still land between ensure_hub_root confirming $HUB_ROOT is fresh and the caller actually reading a file under it. -# Shared (flock -s) rather than exclusive, since concurrent reads don't conflict with each other, only with a writer, so this only blocks behind an in-progress fetch_hub or cleanup. -# A no-op in --dry-run: dry-run's own branch in ensure_hub_root never fetches and never reads $DIR/hub (it resolves HUB_ROOT from cwd or fails outright), so there is nothing here for it to protect, and --dry-run must not create $DIR or a lock file where neither existed, since fetching is documented as the one real host change this script makes. +# Scoping it to only a final tool invocation would leave ensure_hub_root's own git reads unlocked. +# That is the same interleaving one call frame up from the read this lock exists to close. +# A concurrent fetch_hub's rm -rf could still land between ensure_hub_root confirming $HUB_ROOT is fresh and the caller actually reading a file under it. +# Shared (flock -s) rather than exclusive, since concurrent reads don't conflict with each other, only with a writer. +# So this only blocks behind an in-progress fetch_hub or cleanup. +# A no-op in --dry-run. +# Dry-run's own branch in ensure_hub_root never fetches and never reads $DIR/hub (it resolves HUB_ROOT from cwd or fails outright). +# So there is nothing here for the reader lock to protect. +# Also, --dry-run must not create $DIR or a lock file where neither existed. +# That is because fetching is documented as the one real host change this script makes. hub_read_lock_acquire() { [[ $DRY_RUN == true ]] && return 0 mkdir -p "$DIR" - # Checked explicitly, not left to fail silently into the flock call below: this function is always called as `hub_read_lock_acquire || ...`, and set -e is suspended for a callee's own body in that shape, confirmed live, so an unwritable $DIR would otherwise leave HUB_READ_LOCK_FD unset and surface only flock's own confusing error about a missing fd. + # Checked explicitly, not left to fail silently into the flock call below. + # This function is always called as `hub_read_lock_acquire || ...`, and set -e is suspended for a callee's own body in that shape, confirmed live. + # So an unwritable $DIR would otherwise leave HUB_READ_LOCK_FD unset and surface only flock's own confusing error about a missing fd. if ! exec {HUB_READ_LOCK_FD}>"$DIR/hub.lock"; then fail "Could not open $DIR/hub.lock for locking (permission, or the directory is not writable)" return 1 fi - # A quick non-blocking probe first, so a session that has to wait says so instead of looking hung: another session's fetch_hub or host_tool can hold the exclusive lock for as long as a full clone or a long OS package upgrade takes. - # A busy lock's own failure is silent (confirmed live), so any stderr this specific call produces is a real error (a missing flock, a bad fd), not contention, and prints as one instead of the misleading "waiting" framing below. + # A quick non-blocking probe first, so a session that has to wait says so instead of looking hung. + # Another session's fetch_hub or host_tool can hold the exclusive lock for as long as a full clone or a long OS package upgrade takes. + # A busy lock's own failure is silent (confirmed live). + # So any stderr this specific call produces is a real error (a missing flock, a bad fd), not contention. + # Such an error prints as one instead of the misleading "waiting" framing below. local probe_err if ! probe_err=$(flock -sn "$HUB_READ_LOCK_FD" 2>&1); then if [[ -n $probe_err ]]; then - # A real error, not contention: retrying via the blocking call below would not help and risks a second, more confusing failure (or a hang, if the failure mode is one blocking mode does not handle the same way), so this fails immediately instead of falling through to it. + # A real error, not contention. + # Retrying via the blocking call below would not help. + # Retrying also risks a second, more confusing failure. + # Retrying risks a hang as well, if the failure mode is one blocking mode does not handle the same way. + # So this fails immediately instead of falling through to the blocking call. fail "flock -sn reported: $probe_err" exec {HUB_READ_LOCK_FD}>&- HUB_READ_LOCK_FD="" @@ -131,21 +159,33 @@ hub_read_lock_acquire() { } # Releases the fd hub_read_lock_acquire opened. -# Safe to call even when acquire failed or was never called, so a caller's cleanup path never has to track whether it actually holds the lock. +# Safe to call even when acquire failed or was never called. +# So a caller's cleanup path never has to track whether it actually holds the lock. hub_read_lock_release() { [[ -n $HUB_READ_LOCK_FD ]] || return 0 exec {HUB_READ_LOCK_FD}>&- HUB_READ_LOCK_FD="" } -# `ensure_hub_root` is `fetch_hub`'s only caller, and it only ever runs inside a `hub_read_lock_acquire` span (`host_tool`, `audit_repo`, `check_skills_dist`, `carry_action` all acquire it first), so $HUB_READ_LOCK_FD is always already open here. -# Escalating that same fd from shared to exclusive, rather than opening a second fd on the same lock file, is what makes this safe: flock treats two different fds on one file as independent lock holders even within one process, so a second fd's blocking exclusive wait would deadlock against the first fd's own shared hold forever, verified by reproducing exactly that hang before this fix. -# `flock` changes an already-held fd's own lock type in place with no such deadlock, since the kernel recognizes it as the same holder taking a different mode, not a second competing one. -# The conversion itself is not atomic (flock(2): the shared lock is dropped before the exclusive one is granted), so another session's own conversion could win the gap and clone first, but fetch_hub_locked's own remove_unowned_hub_check re-verifies ownership before removing or recloning, so that race costs a redundant clone at worst, never a torn one. -# Downgraded back to shared once the fetch itself is done, restoring reader concurrency for the rest of the caller's own read-and-use span, rather than holding exclusive (which would still be correct, only more conservative than necessary). +# `ensure_hub_root` is `fetch_hub`'s only caller. +# `ensure_hub_root` only ever runs inside a `hub_read_lock_acquire` span (`host_tool`, `audit_repo`, `check_skills_dist`, `carry_action` all acquire it first). +# So $HUB_READ_LOCK_FD is always already open here. +# Escalating that same fd from shared to exclusive, rather than opening a second fd on the same lock file, is what makes this safe. +# The reason is that flock treats two different fds on one file as independent lock holders even within one process. +# So a second fd's blocking exclusive wait would deadlock against the first fd's own shared hold forever. +# The deadlock was verified by reproducing exactly that hang before this fix. +# `flock` changes an already-held fd's own lock type in place with no such deadlock. +# That is because the kernel recognizes the already-held fd as the same holder taking a different mode, not a second competing one. +# The conversion itself is not atomic (flock(2): the shared lock is dropped before the exclusive one is granted). +# So another session's own conversion could win the gap and clone first. +# But fetch_hub_locked's own remove_unowned_hub_check re-verifies ownership before removing or recloning. +# So that race costs a redundant clone at worst, never a torn one. +# Downgraded back to shared once the fetch itself is done, restoring reader concurrency for the rest of the caller's own read-and-use span. +# That is done rather than holding exclusive, which would still be correct, only more conservative than necessary. fetch_hub() { # --dry-run promises to change nothing, and this clone (or re-clone) is the one real change this function itself makes to the host. - # The reader lock (hub_read_lock_acquire) already no-ops under --dry-run, never opening a lock at all, so without this check fetch_hub would instead fail on its own "no reader lock held" internal-error message below. + # The reader lock (hub_read_lock_acquire) already no-ops under --dry-run, never opening a lock at all. + # So without this check fetch_hub would instead fail on its own "no reader lock held" internal-error message below. # This check exists to give the clearer, task-specific diagnostic first. [[ $DRY_RUN == true ]] && { fail "This task needs a fetched hub checkout, and fetching one is itself a change --dry-run does not make. Run without --dry-run, or from inside a hub checkout already on $DEFAULT_REF." @@ -181,24 +221,32 @@ fetch_hub_locked() { fail "Could not remove $DIR/hub to fetch it again. Check that nothing holds a file under it." return 1 fi - # Marked as ours before git can create anything under $DIR/hub, not only once the clone also succeeds: git can leave a partial directory behind on a failed or interrupted clone, and an unmarked one would then block every retry until removed by hand. + # Marked as ours before git can create anything under $DIR/hub, not only once the clone also succeeds. + # Git can leave a partial directory behind on a failed or interrupted clone. + # An unmarked one would then block every retry until removed by hand. if ! printf '%s\n' "$token" >"$(marker_path)"; then fail "Could not write $(marker_path). Check that $DIR is writable." return 1 fi HUB_FETCH_TOKEN="$token" - # A full clone of the default branch first, whatever $REF names: spec/audit.py walks the hub's own history to judge whether a carried copy is trailing the file it was copied from, and a shallow clone would read every file as changed at the truncation boundary and misreport every repo as stale. + # A full clone of the default branch first, whatever $REF names. + # That is because of the following. + # - spec/audit.py walks the hub's own history to judge whether a carried copy is trailing the file it was copied from. + # - A shallow clone would read every file as changed at the truncation boundary and misreport every repo as stale. git clone --quiet --branch "$DEFAULT_REF" --single-branch "$HUB_URL" "$DIR/hub" || { fail "Could not clone $HUB_REPO. Check that this host reaches github.com." - # The marker goes with the directory it marked: an orphaned one left behind would grant false ownership to whatever unrelated directory a person later puts at this same path. + # The marker goes with the directory it marked. + # An orphaned one left behind would grant false ownership to whatever unrelated directory a person later puts at this same path. rm -rf "$DIR/hub" "$(marker_path)" return 1 } - # Set the moment the clone lands, whatever $REF still has to do: cleanup is gated on this, and a later ref-specific failure below must still remove the tree this step already created. + # Set the moment the clone lands, whatever $REF still has to do. + # Cleanup is gated on this flag, and a later ref-specific failure below must still remove the tree this step already created. HUB_FETCHED=true # A branch name is already checked out by the clone above. - # A tag, a pull request ref, or a commit needs an explicit fetch and checkout, since "git clone --branch" only takes a branch or a tag, not an arbitrary commit. + # A tag, a pull request ref, or a commit needs an explicit fetch and checkout. + # That is because "git clone --branch" only takes a branch or a tag, not an arbitrary commit. if [[ $REF != "$DEFAULT_REF" ]]; then git -C "$DIR/hub" fetch --quiet origin "$REF" || { @@ -219,9 +267,13 @@ fetch_hub_locked() { # Read by detect_downstream_root so the hub is never misclassified as a downstream repo, whatever ref was asked for. IS_HUB_CHECKOUT=false -# A checkout already sitting on the hub is a candidate to reuse as is, so a maintainer working in their own ProjectTemplate tree never pays for a second clone of the repo they are standing in. -# Only for the default ref: naming any other --ref always fetches fresh, even from inside the hub itself, since AUDIT.md and the sync procedure both rely on this loader reaching a ref other than whatever happens to be checked out locally. -# HUB_ROOT set here is tentative, verified against a freshly fetched origin/main by ensure_hub_root before any tool actually reads it, since a candidate this stale is exactly the wrong answer for an audit or a Skills-distribution check. +# A checkout already sitting on the hub is a candidate to reuse as is. +# So a maintainer working in their own ProjectTemplate tree never pays for a second clone of the repo they are standing in. +# Only for the default ref. +# Naming any other --ref always fetches fresh, even from inside the hub itself. +# That is because AUDIT.md and the sync procedure both rely on this loader reaching a ref other than whatever happens to be checked out locally. +# HUB_ROOT set here is tentative, verified against a freshly fetched origin/main by ensure_hub_root before any tool actually reads it. +# The verification matters since a candidate this stale is exactly the wrong answer for an audit or a Skills-distribution check. detect_hub_root() { local top top=$(git rev-parse --show-toplevel 2>/dev/null) || return 0 @@ -232,7 +284,8 @@ detect_hub_root() { } # Whether HUB_ROOT is clean and sitting on $DEFAULT_REF, with no assumption that it agrees with origin. -# A failed git command must never read the same as its emptiest possible success, so each one's own exit status is captured rather than only its output. +# A failed git command must never read the same as its emptiest possible success. +# So each one's own exit status is captured rather than only its output. hub_root_clean_on_default_ref() { local status branch status=$(git -C "$HUB_ROOT" status --porcelain) || return 1 @@ -241,7 +294,8 @@ hub_root_clean_on_default_ref() { [[ $branch == "$DEFAULT_REF" ]] } -# A ref other than $DEFAULT_REF is fetched into a ref named for it rather than read from FETCH_HEAD, since another session sharing $DIR/hub can fetch a different ref into FETCH_HEAD between this fetch and its read. +# A ref other than $DEFAULT_REF is fetched into a ref named for it rather than read from FETCH_HEAD. +# That is because another session sharing $DIR/hub can fetch a different ref into FETCH_HEAD between this fetch and its read. hub_ref_commit() { if [[ $REF == "$DEFAULT_REF" ]]; then git -C "$HUB_ROOT" fetch --quiet origin "$DEFAULT_REF" || return 1 @@ -252,16 +306,22 @@ hub_ref_commit() { git -C "$HUB_ROOT" rev-parse "refs/menu/$REF^{commit}" } -# Confirms a tentative HUB_ROOT is clean and still on the commit this session's own $REF resolves to on origin before any tool reads it, checked here rather than at startup so opening the menu costs no network call until a hub-dependent task actually runs. -# A local checkout that has moved on (a feature branch, a commit behind, an uncommitted edit) falls back to a real fetch rather than being trusted, the same freshness and cleanliness carry.py's own verify_hub already requires of its own hub argument. +# Confirms a tentative HUB_ROOT is clean and still on the commit this session's own $REF resolves to on origin before any tool reads it. +# The check runs here rather than at startup, so opening the menu costs no network call until a hub-dependent task actually runs. +# A local checkout that has moved on falls back to a real fetch rather than being trusted. +# Examples of a checkout that has moved on include a feature branch, a commit behind, and an uncommitted edit. +# This matches the same freshness and cleanliness carry.py's own verify_hub already requires of its own hub argument. ensure_hub_root() { if [[ -n $HUB_ROOT ]]; then if [[ $DRY_RUN == true ]]; then - # No network call, since confirming freshness against origin means fetching, which --dry-run does not do, but this still confirms the checkout is clean and actually on main: detect_hub_root checked only origin identity, and a dirty or feature-branch checkout must not be read as the hub's main either. + # No network call, since confirming freshness against origin means fetching, which --dry-run does not do. + # This still confirms the checkout is clean and actually on main. + # That is because detect_hub_root checked only origin identity, and a dirty or feature-branch checkout must not be read as the hub's main either. hub_root_clean_on_default_ref && return 0 HUB_ROOT="" else - # The freshness check below itself fetches, which updates FETCH_HEAD and the ref it fetches into even though it touches no working file, so it is as much a change as fetch_hub's own clone. + # The freshness check below itself fetches, which updates FETCH_HEAD and the ref it fetches into even though it touches no working file. + # So the freshness check is as much a change as fetch_hub's own clone. local status head ref_head if ref_head=$(hub_ref_commit) && status=$(git -C "$HUB_ROOT" status --porcelain) && [[ -z $status ]] && @@ -293,12 +353,17 @@ detect_downstream_root() { cleanup() { [[ $KEEP == true || $HUB_FETCHED == false ]] && return 0 [[ -e "$(marker_path)" ]] || return 0 - # Released first and unconditionally, even though a normal exit has already released it: an interrupt (Ctrl-C) fires this EXIT trap with $HUB_READ_LOCK_FD still open mid-dispatch, and this process's own still-held shared lock would otherwise deadlock the exclusive wait below against itself, reproduced and confirmed before this fix. + # Released first and unconditionally, even though a normal exit has already released it. + # An interrupt (Ctrl-C) fires this EXIT trap with $HUB_READ_LOCK_FD still open mid-dispatch. + # In that case, this process's own still-held shared lock would otherwise deadlock the exclusive wait below against itself. + # The deadlock was reproduced and confirmed before this fix. # The read that fd was protecting is being abandoned along with the rest of this process, so releasing it here costs nothing. hub_read_lock_release # Exclusive, the same writer half fetch_hub takes, so this waits out a concurrent reader still mid-use rather than deleting the tree out from under it. local lock_fd - # Checked explicitly, the same reasoning hub_read_lock_acquire's own open already applies: cleanup runs from an EXIT trap, not a plain top-level statement, so an unwritable $DIR here would otherwise leave lock_fd unset and surface only flock's own confusing error about a missing fd. + # Checked explicitly, the same reasoning hub_read_lock_acquire's own open already applies. + # Cleanup runs from an EXIT trap, not a plain top-level statement. + # So an unwritable $DIR here would otherwise leave lock_fd unset and surface only flock's own confusing error about a missing fd. if ! exec {lock_fd}>"$DIR/hub.lock"; then fail "Could not open $DIR/hub.lock for locking (permission, or the directory is not writable)" return 1 @@ -306,7 +371,8 @@ cleanup() { local probe_err if ! probe_err=$(flock -n "$lock_fd" 2>&1); then if [[ -n $probe_err ]]; then - # A real error, not contention, the same distinction hub_read_lock_acquire's own probe already makes: retrying via the blocking call below would not help and risks a second, more confusing failure. + # A real error, not contention, the same distinction hub_read_lock_acquire's own probe already makes. + # Retrying via the blocking call below would not help and risks a second, more confusing failure. fail "flock -n reported: $probe_err" exec {lock_fd}>&- return 1 @@ -322,7 +388,8 @@ cleanup() { exec {lock_fd}>&- return 0 fi - # The marker is removed only once the directory it marks is actually gone, rather than unconditionally alongside it: a suppressed removal failure must not leave a leftover hub with no marker to explain it. + # The marker is removed only once the directory it marks is actually gone, rather than unconditionally alongside it. + # A suppressed removal failure must not leave a leftover hub with no marker to explain it. rm -rf "$DIR/hub" [[ -e "$DIR/hub" ]] || rm -f "$(marker_path)" exec {lock_fd}>&- @@ -341,7 +408,9 @@ host_tool() { } # Every host tool runs from inside the hub tree, and this is the only place a path inside it is named, matching bootstrap.sh's run_tool. -# Resolves the hub root itself rather than assuming a caller already did: a host task must work standalone, off a downstream checkout, or off no checkout at all, none of which set HUB_ROOT on their own. +# Resolves the hub root itself rather than assuming a caller already did. +# A host task must work standalone, off a downstream checkout, or off no checkout at all. +# None of those set HUB_ROOT on their own. host_tool_locked() { local tool="$1" shift @@ -390,13 +459,15 @@ install_agents() { fi } -# The Python tools under scripts/ and spec/ resolve their own root from __file__ rather than the working directory, so they are called by absolute path from wherever this script runs and need no cd. +# The Python tools under scripts/ and spec/ resolve their own root from __file__ rather than the working directory. +# So they are called by absolute path from wherever this script runs and need no cd. # Checked here rather than upfront in main, the same reasoning host-setup/linux/install-skills.sh already carries. # A host with no uv yet can still use every host action, and only the actions that need uv name it as their own prerequisite. # The prerequisite failure returns 127, bash's own "command not found" convention. # So a caller reading a specific exit code from the tool itself, such as build_dist.py's 0-clean/1-stale contract, can tell "uv never ran" apart from "uv ran and returned 1". # Callers hold the reader lock themselves, hub_read_lock_acquire before their own ensure_hub_root call, rather than this function taking it. -# Every caller pairs this with ensure_hub_root and the lock has to cover both, so taking it only here would still leave ensure_hub_root's own reads unlocked, the gap this whole reader-half addition exists to close. +# Every caller pairs this with ensure_hub_root and the lock has to cover both. +# So taking it only here would still leave ensure_hub_root's own reads unlocked, the gap this whole reader-half addition exists to close. # Every hub tool a task runs declares its libraries inline, so uv runs each one rather than a bare python3, which has none of them. hub_uv_script() { command -v uv >/dev/null || { @@ -412,7 +483,10 @@ hub_uv_script() { # Not reentrant: hub_read_lock_acquire's exec unconditionally overwrites HUB_READ_LOCK_FD, so a nested call would leak the outer fd and silently drop the lock it represented. # No current caller nests, and this guard is what keeps it that way rather than a rule to remember. with_hub_read_lock() { - # 2, not 1, for every failure of this wrapper's own preconditions: check_skills_dist's own caller reads 1 as scripts/build_dist.py --check's "stale" result, so a wrapper-level failure must not collide with the wrapped command's own exit code, the same reasoning Invoke-CheckSkillsDist's own Confirm-HubRoot fix already applies on the PowerShell side. + # 2, not 1, for every failure of this wrapper's own preconditions. + # The function check_skills_dist, one caller of this wrapper, reads 1 as scripts/build_dist.py --check's "stale" result. + # So a wrapper-level failure must not collide with the wrapped command's own exit code. + # Invoke-CheckSkillsDist's own Confirm-HubRoot fix already applies the same reasoning on the PowerShell side. (($# > 0)) || { fail "with_hub_read_lock called with no command (internal error)" return 2 @@ -447,7 +521,8 @@ check_skills_dist() { local rc=0 with_hub_read_lock hub_uv_script scripts/build_dist.py --check || rc=$? # Only 0 (clean) and 1 (stale) are outcomes scripts/build_dist.py --check documents for itself, so only those two read as a check result. - # Anything else, 127 included, is with_hub_read_lock, hub_uv_script, or the tool itself failing to run rather than a finding, and is reported as the task error it is. + # Anything else, 127 included, is with_hub_read_lock, hub_uv_script, or the tool itself failing to run rather than a finding. + # That failure is reported as the task error it is. case "$rc" in 0) info "Every generated Skills distribution matches .agents/skills/" ;; 1) info "A generated Skills distribution is stale. This menu does not regenerate it from a fetched checkout, since the result has to be committed in the hub itself." ;; @@ -465,7 +540,9 @@ carry_action() { fail "No downstream repo checkout found. Run this menu from inside the target repo's own worktree." return 1 } - # Its hub argument must be exactly on origin/main by carry.py's own requirement, and a non-default --ref checks out something else entirely, so this would always fail deep inside carry.py with no clue why. + # Its hub argument must be exactly on origin/main by carry.py's own requirement. + # A non-default --ref checks out something else entirely. + # So this would always fail deep inside carry.py with no clue why. # Refused here instead, with the actual reason. [[ $REF == "$DEFAULT_REF" ]] || { @@ -511,7 +588,9 @@ print_menu() { log "Hub, $HUB_REPO:" log " 13 Audit a cataloged repo" log " 14 Check the generated Skills distributions are current" - # Also gated on REF: carry.py always rejects a hub checkout that is not exactly on the default ref, so these tasks cannot work in a non-default --ref session regardless of a downstream repo being detected. + # Also gated on REF. + # The carry.py script always rejects a hub checkout that is not exactly on the default ref. + # So these tasks cannot work in a non-default --ref session regardless of a downstream repo being detected. if [[ -n $DOWNSTREAM_ROOT && $REF == "$DEFAULT_REF" ]]; then log "" log "Downstream, the repo this menu is run from:" @@ -525,7 +604,9 @@ print_menu() { log "" } -# A failing task (a real install error, a network hiccup) is reported and returns to the menu rather than ending the session, so dispatch's own exit status cannot double as "quit": QUIT is a separate flag the q/Q case sets, read by the loop after every dispatch regardless of whether the task it ran succeeded. +# A failing task (a real install error, a network hiccup) is reported and returns to the menu rather than ending the session. +# So dispatch's own exit status cannot double as "quit". +# QUIT is a separate flag the q/Q case sets, read by the loop after every dispatch regardless of whether the task it ran succeeded. QUIT=false BAD_CHOICE=false @@ -546,7 +627,8 @@ dispatch() { 13) audit_repo ;; 14) check_skills_dist ;; 15) carry_action check ;; - # --dry-run changes nothing, and carry.py itself has no dry-run mode, so a dry-run apply reads as its own check instead of silently mutating the downstream worktree. + # --dry-run changes nothing, and carry.py itself has no dry-run mode. + # So a dry-run apply reads as its own check instead of silently mutating the downstream worktree. 16) if [[ $DRY_RUN == true ]]; then carry_action check @@ -580,7 +662,9 @@ interactive_menu() { rc=0 dispatch "$choice" || rc=$? [[ $QUIT == true ]] && break - # A dedicated flag rather than a reserved return code: every dispatched task's own exit code passes through unchanged, and a tool that happens to exit 2 for its own reason (scripts/carry.py's "not uniquely registered", for one) must not be misread as an unrecognized choice. + # A dedicated flag rather than a reserved return code. + # Every dispatched task's own exit code passes through unchanged. + # A tool that happens to exit 2 for its own reason (scripts/carry.py's "not uniquely registered", for one) must not be misread as an unrecognized choice. [[ $BAD_CHOICE == true ]] && continue if ((rc == 0)); then step "Done" diff --git a/host-setup/windows/setup-github.ps1 b/host-setup/windows/setup-github.ps1 index 320da48e9..c62d19600 100644 --- a/host-setup/windows/setup-github.ps1 +++ b/host-setup/windows/setup-github.ps1 @@ -1,11 +1,16 @@ # Sets up git and GitHub on a Windows host: the SSH key, the git configuration, and commit signing. # Every step is idempotent, so a re-run repairs a half configured host rather than duplicating what is already there. # -# Two steps cannot be automated, because they happen in a browser: registering the public key as an authentication key, and registering the same key as a signing key. +# Two steps cannot be automated, because they happen in a browser: +# - Registering the public key as an authentication key. +# - Registering the same key as a signing key. # Both are gates rather than suggestions. -# The script stops at each, prints the key to paste and where to paste it, and checks afterwards that the registration took, by reading the keys GitHub publishes for the account. +# The script stops at each and prints the key to paste and where to paste it. +# The script checks afterwards that the registration took, by reading the keys GitHub publishes for the account. # -# The path settings are written in the tilde form the Linux peer writes, because git expands it on Windows too, and a home shared with a WSL distribution then carries one value rather than two that disagree. +# The path settings are written in the tilde form the Linux peer writes, because of the following. +# - Git expands it on Windows too. +# - A home shared with a WSL distribution then carries one value rather than two that disagree. [CmdletBinding()] param( @@ -29,7 +34,9 @@ $PSNativeCommandUseErrorActionPreference = $false $KEY = Join-Path $HOME '.ssh\id_ed25519' $ALLOWED_SIGNERS = Join-Path $HOME '.config\git\allowed_signers' -# Git expands a leading tilde in a path setting on Windows as well, and the hosts already configured by hand hold the tilde form, so writing that form leaves an already configured host untouched. +# Writing the tilde form leaves an already configured host untouched, because of the following. +# - Git expands a leading tilde in a path setting on Windows as well. +# - The hosts already configured by hand hold the tilde form. # File operations use the expanded paths above, since only git expands the tilde. $KEY_SETTING = '~/.ssh/id_ed25519.pub' $ALLOWED_SIGNERS_SETTING = '~/.config/git/allowed_signers' @@ -45,7 +52,8 @@ $DEFAULT_NAME = 'Pieter Viljoen' $DEFAULT_EMAIL = 'ptr727@users.noreply.github.com' # Every parameter is read into a variable here rather than from inside a function. -# A script parameter reached only from a nested scope reads as declared and never used, which is a finding on each one and hides a parameter that genuinely is unused. +# A script parameter reached only from a nested scope reads as declared and never used. +# That is a finding on each one, and it hides a parameter that genuinely is unused. $ACTIONS = [ordered]@{ status = [bool]$Status configure = [bool]$Configure @@ -133,7 +141,9 @@ function run { return 0 } # The command's own output goes to the console rather than down the pipeline. - # A native command writes to this function's output stream, so without this the caller receives every line the command printed with the exit code appended, and a check against 0 then compares against the first line of output. + # A native command writes to this function's output stream. + # So without this the caller receives every line the command printed with the exit code appended. + # A check against 0 then compares against the first line of output. & $Command @Arguments | Out-Host return $LASTEXITCODE } @@ -153,7 +163,8 @@ function confirm { # Both git and ssh ship with Git for Windows, and neither is installed from here. # Installing them belongs to install-tools.ps1, so a gap is named with the command that closes it rather than closed twice in two scripts. # Every executable this script calls is named, rather than the three it is most obviously about. -# One left out fails partway through with whatever that command says when it is missing, in place of the one message here that names the remedy. +# One left out fails partway through with whatever that command says when it is missing. +# That failure comes in place of the one message here that names the remedy. function Test-Prerequisite { $absent = @() foreach ($tool in 'git', 'ssh', 'ssh-keygen', 'ssh-keyscan', 'ssh-add') { @@ -189,7 +200,9 @@ function New-KeyIfAbsent { } # The ACL is reported and never rewritten. -# Creating the key with ssh-keygen already sets it correctly on Windows, and rewriting an ACL is hard to undo and hard to preview under -DryRun, so a wrong one is named with the command that repairs it. +# A wrong one is named with the command that repairs it, because of the following. +# - Creating the key with ssh-keygen already sets it correctly on Windows. +# - Rewriting an ACL is hard to undo and hard to preview under -DryRun. function Test-KeyAcl { if (-not (Test-Path $script:KEY)) { return $null } $acl = Get-Acl $script:KEY @@ -222,7 +235,8 @@ function Add-KnownHost { } # What was offered is checked against what GitHub publishes before it is recorded, matching the Linux peer. - # Recording whatever answered would persist a substituted key on the one run where nothing yet pins the real one, and every later connection would then verify against it. + # Recording whatever answered would persist a substituted key on the one run where nothing yet pins the real one. + # Every later connection would then verify against it. $temp = Join-Path ([IO.Path]::GetTempPath()) ([Guid]::NewGuid().ToString() + '.pub') try { [IO.File]::WriteAllText($temp, ($scanned -join "`n") + "`n") @@ -260,7 +274,8 @@ function Add-KnownHost { # --- GitHub, read only --- -# Every probe refuses to write a host key, so asking whether authentication works cannot enroll github.com behind the reader's back and a status run stays a read-only action. +# Every probe refuses to write a host key. +# So asking whether authentication works cannot enroll github.com behind the reader's back, and a status run stays a read-only action. # A status run also refuses to prompt, since it may be run unattended and a passphrase prompt would hang it. function Get-SshGreeting { param([switch]$ManagedKeyOnly) @@ -268,7 +283,8 @@ function Get-SshGreeting { if ($script:MODE -eq 'status') { $options += @('-o', 'BatchMode=yes') } if ($ManagedKeyOnly) { if (-not (Test-Path $script:KEY)) { return '' } - # The ssh client offers the default identity files and an agent's keys besides whatever is named, so a host carrying another account's key authenticates as that account. + # The ssh client offers the default identity files and an agent's keys besides whatever is named. + # So a host carrying another account's key authenticates as that account. # An empty config file is what isolates the managed key, since the default identity files count as configured identities on their own. $options += @('-F', 'NUL', '-o', 'IdentitiesOnly=yes', '-i', $script:KEY) } @@ -281,7 +297,8 @@ function Get-GreetingUser { return '' } -# Prefer the account the managed key belongs to, since that is the one the registration checks are about, and say so when the host answers as somebody else. +# Prefer the account the managed key belongs to, since that is the one the registration checks are about. +# Say so when the host answers as somebody else. function Resolve-GitHubUser { $general = Get-GreetingUser (Get-SshGreeting) $managed = Get-GreetingUser (Get-SshGreeting -ManagedKeyOnly) @@ -297,8 +314,10 @@ function Resolve-GitHubUser { $script:GITHUB_USER = $general } -# GitHub publishes both key lists for an account, so a registration can be checked from the host without a token and without the browser that made it. -# Each returns registered, missing, or unknown, and the third is not the second: a momentary failure to reach GitHub reported as "not registered" sends the reader to register a key that is already there. +# GitHub publishes both key lists for an account. +# So a registration can be checked from the host without a token and without the browser that made it. +# Each returns registered, missing, or unknown, and the third is not the second. +# A momentary failure to reach GitHub reported as "not registered" sends the reader to register a key that is already there. # PowerShell separates those two by itself, since an account with no signing key returns an empty list where an unreachable GitHub throws. function Test-KeyRegistered { param([ValidateSet('auth', 'signing')][string]$Kind) @@ -381,7 +400,8 @@ function Set-GitIdentity { $changed = 0 if (Set-GitConfig 'user.name' $script:WANT_NAME) { $changed++ } if (Set-GitConfig 'user.email' $script:WANT_EMAIL) { $changed++ } - # Git Credential Manager ships with Git for Windows and is what gh expects there, where the Linux peer writes a cache helper because no manager exists. + # Git Credential Manager ships with Git for Windows and is what gh expects there. + # The Linux peer writes a cache helper instead, because no manager exists on Linux. # Written only where nothing is set, so a host already carrying a helper keeps it. if (-not (Get-GitConfig 'credential.helper')) { if (Set-GitConfig 'credential.helper' 'manager') { $changed++ } @@ -439,7 +459,8 @@ function Set-Signing { } # Sign a commit in a throwaway repository and verify it. -# This proves the configuration end to end, which reading the settings back cannot: a wrong allowed signers entry reads as correct and fails only when a signature is checked. +# This proves the configuration end to end, which reading the settings back cannot. +# A wrong allowed signers entry reads as correct and fails only when a signature is checked. function Test-Signing { $repository = Join-Path ([IO.Path]::GetTempPath()) ([Guid]::NewGuid().ToString()) try { @@ -460,7 +481,8 @@ function Test-Signing { # --- Agent --- # On Windows the agent is a service rather than a socket, and both starting it and setting it to start automatically need administrator. -# Reported with the command that fixes it rather than run, on the same rule as the key ACL: this script does not elevate, and naming the change is better than half applying it. +# Reported with the command that fixes it rather than run, on the same rule as the key ACL. +# This script does not elevate, and naming the change is better than half applying it. function Show-AgentStatus { log '' log 'SSH agent' @@ -580,7 +602,8 @@ function Show-Status { log '' log 'Registration, as GitHub publishes it' - # Which account to ask about comes from authentication, so with none there is no question to ask, and that reads differently from having asked and failed. + # Which account to ask about comes from authentication, so with none there is no question to ask. + # That reads differently from having asked and failed. if (-not $script:GITHUB_USER -or -not (Get-KeyBody)) { missing 'Authentication key registered, unknown until authentication works' missing 'Signing key registered, unknown until authentication works' @@ -602,7 +625,9 @@ function Show-Status { if ($value) { ok "$key = $value" } else { missing $key } } - # The shared checkout settings are reported apart, because absent is the right state for a host one account uses and listing them as missing would read as two gaps to close. + # The shared checkout settings are reported apart, because of the following. + # - Absent is the right state for a host one account uses. + # - Listing them as missing would read as two gaps to close. $shared = @(& git config --global --get-all safe.directory 2>$null) if ($shared.Count -gt 0) { $unique = @($shared | Sort-Object -Unique) @@ -629,7 +654,10 @@ function Invoke-Configure { Add-KnownHost # The local configuration comes first, because none of it needs GitHub. - # A host that cannot authenticate yet, which is every host between creating its key and registering it, still ends this run with its identity, its signing configuration, and a commit that verifies locally. + # A host that cannot authenticate yet, which is every host between creating its key and registering it, still ends this run with: + # - Its identity. + # - Its signing configuration. + # - A commit that verifies locally. Set-GitIdentity Set-SharedCheckout Set-Signing diff --git a/host-setup/windows/upgrade-host.ps1 b/host-setup/windows/upgrade-host.ps1 index 7e51798b8..517dd4ac8 100644 --- a/host-setup/windows/upgrade-host.ps1 +++ b/host-setup/windows/upgrade-host.ps1 @@ -1,11 +1,16 @@ # Upgrades this host, on native Windows. -# Two things are brought current: the packages winget manages, which is routine, and the WSL platform itself, which is the Windows peer of a kernel and moves on its own release schedule. +# Two things are brought current: +# - The packages winget manages, which is routine. +# - The WSL platform itself, which is the Windows peer of a kernel and moves on its own release schedule. # -# A WSL platform update is refused while Docker Desktop is running, because Docker holds the WSL service open and the update then fails part way rather than declining. +# A WSL platform update is refused while Docker Desktop is running. +# The reason is that Docker holds the WSL service open, so the update then fails part way rather than declining. # Refusing is the point of running this rather than the two commands by hand: each is one line, and the guard between them is not. # # There is no action for a Windows feature update, where the Linux peer moves a host to the next release. -# Windows Update owns that upgrade, it is not driven the way an apt sources rewrite is, and an action that pretended otherwise would be the one thing this script must not carry. +# Windows Update owns that upgrade. +# It is not driven the way an apt sources rewrite is. +# An action that pretended otherwise would be the one thing this script must not carry. [CmdletBinding()] param( @@ -34,7 +39,8 @@ $SELF_UPDATE_NOTE = @{ } # Every parameter is read into a variable here rather than from inside a function. -# A script parameter reached only from a nested scope reads as declared and never used, which is a finding on each one and hides a parameter that genuinely is unused. +# A script parameter reached only from a nested scope reads as declared and never used. +# That is a finding on each one, and it hides a parameter that genuinely is unused. $ACTIONS = [ordered]@{ status = [bool]$Status packages = [bool]$Packages @@ -122,7 +128,9 @@ function run { return 0 } # The command's own output goes to the console rather than down the pipeline. - # A native command writes to this function's output stream, so without this the caller receives every line the command printed with the exit code appended, and a check against 0 then compares against the first line of output. + # A native command writes to this function's output stream. + # So without this the caller receives every line the command printed with the exit code appended. + # A check against 0 then compares against the first line of output. & $Command @Arguments | Out-Host return $LASTEXITCODE } @@ -140,7 +148,8 @@ function confirm { # --- winget --- # What winget would upgrade, split into the packages it will move and the ones it will not. -# The two halves are counted here rather than read off winget's own closing count, which sums both and so reports a number that matches neither list. +# The two halves are counted here rather than read off winget's own closing count. +# That count sums both and so reports a number that matches neither list. # Each row is parsed by header offset, since a package name carries spaces and splitting on them moves the version into the name. function Get-Upgradable { $result = @{ Ready = @(); Explicit = @() } @@ -186,7 +195,8 @@ function Read-UpgradeTable { # --- WSL --- # Every wsl.exe call goes through here, because wsl.exe emits UTF-16 by default and its output then reads as NUL separated characters. -# WSL_UTF8 changes what wsl.exe emits, where setting the console encoding would only change how this process decodes it and would corrupt in-distribution output that is already UTF-8. +# WSL_UTF8 changes what wsl.exe emits, where setting the console encoding would only change how this process decodes it. +# Setting the console encoding would also corrupt in-distribution output that is already UTF-8. # A host whose WSL predates WSL_UTF8 still answers in UTF-16, so a result carrying a NUL is stripped rather than reported as unreadable. function Invoke-Wsl { param([Parameter(ValueFromRemainingArguments)][string[]]$Arguments) @@ -217,14 +227,17 @@ function Get-WslVersion { # --- Docker --- # Whether Docker Desktop is running, which is what blocks a WSL platform update. -# The service is not the probe: com.docker.service reads Stopped on a host with Docker Desktop plainly running, since it is the elevated helper rather than the engine. +# The service is not the probe. +# The com.docker.service entry reads Stopped on a host with Docker Desktop plainly running, since it is the elevated helper rather than the engine. # Neither wslservice nor vmmemWSL is the probe either, since both run for any distribution and would report Docker on a host that has none. function Get-DockerProcess { return , @(Get-Process -Name $script:DOCKER_PROCESSES -ErrorAction SilentlyContinue) } # A WSL platform update stops and restarts the WSL service, and Docker Desktop holds it open, so the update fails part way rather than declining. -# The guard runs under -DryRun too, since it is a read that decides what to do rather than a change to the host, and a dry run that printed the command would say the update was available when it is not. +# The guard runs under -DryRun too, because of the following. +# - It is a read that decides what to do rather than a change to the host. +# - A dry run that printed the command would say the update was available when it is not. function Assert-DockerStopped { $running = Get-DockerProcess if ($running.Count -eq 0) { return } @@ -264,7 +277,9 @@ function Show-Status { log 'Self-updating, so winget reports the version it was installed at rather than the version it runs:' foreach ($row in $upgradable.Explicit) { info "$($row.Id) installed at $($row.Version), source carries $($row.Available)" - # No command is printed for these, deliberately: winget cannot move them, and offering one invites a full reinstall over a working copy in pursuit of a number that does not change. + # No command is printed for these, deliberately, because of the following. + # - winget cannot move them. + # - Offering one invites a full reinstall over a working copy in pursuit of a number that does not change. if ($script:SELF_UPDATE_NOTE.ContainsKey($row.Id)) { info " $($script:SELF_UPDATE_NOTE[$row.Id])" } else { diff --git a/tests/host_capability.py b/tests/host_capability.py index 5e9fb221d..5884e3579 100644 --- a/tests/host_capability.py +++ b/tests/host_capability.py @@ -1,7 +1,7 @@ """Host capabilities a test needs and some hosts lack. -A test that needs one of these skips with the reason when the host lacks it, rather than -erroring on a precondition that has nothing to do with the code under test. +A test that needs one of these skips with the reason when the host lacks it. The skip replaces an +error on a precondition that has nothing to do with the code under test. """ from __future__ import annotations @@ -63,8 +63,8 @@ def unprivileged_root_is_available() -> bool: A guard reading `$EUID` cannot be fooled by setting a variable, since bash makes it read-only. A sudo'd run therefore has to be a real EUID 0. A user namespace is the one way to - reach that without root, and it is unavailable on a host that restricts unprivileged user - namespaces, which is why this is probed rather than assumed. + reach that without root. A user namespace is unavailable on a host that restricts unprivileged + user namespaces, which is why this is probed rather than assumed. """ if sys.platform != "linux" or not shutil.which("unshare"): return False @@ -125,16 +125,16 @@ def bash_path() -> str | None: A bare "bash" passed to subprocess on Windows is resolved by CreateProcess, which searches System32 before PATH and so finds the WSL launcher. That launcher hands the script to a second - shell that re-parses it, which mangles any quoting or expansion the script relies on, so the - PATH entry is named instead, passing over the launcher wherever PATH lists it. There each - absolute directory is probed for bash.exe itself rather than through shutil.which, which on - Windows would also look in the current directory first and admit a bash.cmd shim that cmd.exe - re-parses. Elsewhere shutil.which answers, as it did for a bare "bash". PATH is read on every + shell that re-parses it, which mangles any quoting or expansion the script relies on. The + PATH entry is therefore named instead, passing over the launcher wherever PATH lists it. There + each absolute directory is probed for bash.exe itself rather than through shutil.which. On + Windows shutil.which would also look in the current directory first and admit a bash.cmd shim + that cmd.exe re-parses. Elsewhere shutil.which answers, as it did for a bare "bash". PATH is read on every call rather than cached, since a test may change it for the duration of its run. A script holding a doubled backslash is handed to this bash as a file rather than through - `-c`. subprocess writes the pair literally into the Windows command line, and Git Bash's own - parse of that line collapses it to one, which a jq filter's escaped class then fails to compile. + `-c`. subprocess writes the pair literally into the Windows command line. Git Bash's own + parse of that line collapses the pair to one, which a jq filter's escaped class then fails to compile. """ if not _WINDOWS: return shutil.which("bash") diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index 8825d3526..31e9b5a9b 100755 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -5,17 +5,17 @@ The loader invariant is what keeps `host-setup/bootstrap.sh` and `host-setup/bootstrap.ps1` outside the reach of the `Hub-Hosted Tooling` rule rather than exempt from it. A loader obtains a tree and -hands control to one entry point inside it. The moment it reads a second path in that tree it has -become a tool that reads hub content, and the rule applies to it in full. That boundary is a property +hands control to one entry point inside it. The moment a loader reads a second path in that tree, it has +become a tool that reads hub content. The rule then applies to that loader in full. That boundary is a property of each file, so it is asserted here rather than promised in prose. The coverage assertion is the only connection between the floors in `spec/host-tools.json` and the tooling that installs them. Nothing joins the two at runtime, deliberately: the gate measures a host and the tooling changes one, and neither calls the other. Without a check at this level a tool could -be declared required and be one nothing here can install, which a host would discover as a gate it -cannot satisfy. +be declared required and be one nothing here can install. A host would discover that tool as a gate +it cannot satisfy. -That assertion runs once per platform, because the two installers do not manage the same set and the +That assertion runs once per platform. The reason is that the two installers do not manage the same set and the difference is a decision rather than an accident. `git-restore-mtime` serves a Linux deploy path and the spec declares it not applicable on Windows. @@ -54,7 +54,8 @@ TOOLS_DECLARATION = re.compile(r"^readonly TOOLS=\(([^)]*)\)", re.MULTILINE) # The same, for the windows tooling, whose registry is a list of records rather than a flat array. -# The names are read out of the records themselves rather than from a second list beside them, so there is one declaration to keep true rather than two that can agree wrongly. +# The names are read out of the records themselves rather than from a second list beside them. +# That leaves one declaration to keep true rather than two that can agree wrongly. PS_TOOLS_OPEN = re.compile(r"^\$TOOLS\s*=\s*@\(", re.MULTILINE) PS_TOOLS_CLOSE = re.compile(r"^\)", re.MULTILINE) PS_TOOL_NAME = re.compile(r"^\s*@\{\s*Name\s*=\s*'([^']+)'", re.MULTILINE) @@ -80,8 +81,12 @@ sys.platform == "linux", "executes install-tools.sh, which only a Linux host can run" ) -# A spec tool an installer deliberately does not manage, and the reason, recorded so an omission is a decision somebody made rather than one nobody noticed. -# Both sets are empty, which is itself the assertion: docker installs the same way on a hypervisor and a workstation on both platforms now, and the one case that differs, a WSL distribution, is handled inside install-tools.sh itself (it skips the native install and points at Docker Desktop's own WSL integration) rather than by leaving docker unmanaged on Linux entirely. +# A spec tool an installer deliberately does not manage is recorded here with its reason. +# The record makes an omission a decision somebody made rather than one nobody noticed. +# Both sets are empty, which is itself the assertion. +# Docker installs the same way on a hypervisor and a workstation on both platforms now. +# The one case that differs, a WSL distribution, is handled inside install-tools.sh itself rather than by leaving docker unmanaged on Linux entirely. +# That handling skips the native install and points at Docker Desktop's own WSL integration. NOT_MANAGED: dict[str, dict[str, str]] = { "linux": {}, "windows": {}, @@ -103,8 +108,8 @@ class TestLoaderInvariant(unittest.TestCase): """`bootstrap.sh` and `bootstrap.ps1` each read exactly one path into the fetched tree. Both loaders write that one reference as a single interpolated, forward-slashed string - (`$TREE/host-setup//$tool` in each), rather than building it from parts, which is - what lets one pattern check either file unmodified. + (`$TREE/host-setup//$tool` in each), rather than building it from parts. + Writing it that way is what lets one pattern check either file unmodified. """ def _assert_reads_one_path(self, path: Path, expected: str) -> None: @@ -177,7 +182,8 @@ def declared_windows_tools(self) -> set[str]: ) # A missing close marker is a failure rather than a scan to end of file. - # Reading on past the registry collects every later `Name = '...'` in the script, so a broken registry answers with a larger tool set than it declares and the coverage check below passes on it. + # Reading on past the registry collects every later `Name = '...'` in the script. + # A broken registry therefore answers with a larger tool set than it declares, and the coverage check below passes on it. closed = PS_TOOLS_CLOSE.search(text, opened.end()) if closed is None: self.fail( @@ -194,7 +200,8 @@ def declared_windows_tools(self) -> set[str]: def spec_tools(self) -> list[dict]: """The tools the spec declares.""" - # A malformed or unreadable spec is a finding this reports beside the others, rather than a traceback that ends the run and takes the checks after it with it. + # A malformed or unreadable spec is a finding this reports beside the others, rather than a traceback that ends the run. + # Such a traceback would also take the checks after it with it. try: return json.loads(HOST_TOOLS.read_text(encoding="utf-8"))["tools"] except (OSError, ValueError, KeyError) as error: @@ -212,7 +219,8 @@ def _assert_coverage(self, platform: str, managed: set[str], installer: str) -> # Every required tool is checked, including one whose declaration names no source for this platform. # A tool is in scope because the spec requires it, never because its declaration happens to describe where this platform gets it. - # Reading a missing `source.linux` as "not a Linux tool" skipped docker, git and uv, which is half the required set and the whole of what NOT_MANAGED exists to record. + # Reading a missing `source.linux` as "not a Linux tool" skipped docker, git and uv. + # Those tools are half the required set and the whole of what NOT_MANAGED exists to record. expected = ALIASES[platform].get(name, name) with self.subTest(tool=name): if name in NOT_MANAGED[platform]: @@ -415,11 +423,12 @@ def test_windows_installer_supplies_the_python3_name(self) -> None: """Windows has no `python3` of its own, so the installer both clears the lie and supplies the name. Two halves, and neither is sufficient alone. Windows ships app-execution alias stubs that - answer to `python` and `python3` and only offer to open the Microsoft Store. They fail - loudly, on stderr with exit 9009, so the harm is not a false pass but an occupied name: the - alias directory is on `PATH` by default, so the stub answers wherever it sits ahead of the - install directory. Removing them alone would leave `python3` simply absent on a host that - does have Python, so the installer also puts a real one beside the interpreter it manages. + answer to `python` and `python3` and only offer to open the Microsoft Store. The stubs fail + loudly, on stderr with exit 9009, so the harm is not a false pass but an occupied name. The + alias directory is on `PATH` by default, so a stub answers wherever it sits ahead of the + install directory. Removing the stubs alone would leave `python3` simply absent on a host + that does have Python. The installer therefore also puts a real one beside the interpreter + it manages. The reparse-tag guard is what keeps the removal from deleting a real executable someone put in that directory. """ @@ -434,13 +443,13 @@ def test_windows_python3_shim_names_no_version(self) -> None: """The shim follows whatever line the managed package installs, so its code hardcodes none. The package ID in `$TOOLS` is the one place a Python version is written down. A literal - anywhere in the repair path would silently keep targeting the old line the day that ID moves, - which is the failure mode this asserts against rather than merely documents. + anywhere in the repair path would silently keep targeting the old line the day that ID moves. + That outcome is the failure mode this asserts against rather than merely documents. - Comments are stripped before the match, because the invariant is about what the code - resolves and not about what the prose may use as an example. The architecture-qualified tag - rule is far clearer written as `3.13` beside `3.13-arm64` than described in the abstract, and - a version named there cannot make a lookup target the wrong line. + Comments are stripped before the match. The reason is that the invariant is about what the code + resolves and not about what the prose may use as an example. The + architecture-qualified tag rule is far clearer written as `3.13` beside `3.13-arm64` than + described in the abstract. A version named there cannot make a lookup target the wrong line. """ text = (WINDOWS / "install-tools.ps1").read_text(encoding="utf-8") section = text.split("# --- Python ---", 1) @@ -453,10 +462,10 @@ def test_windows_python3_shim_names_no_version(self) -> None: def test_every_declared_floor_carries_a_total_remedy_mapping(self) -> None: """Each floored tool names a runnable remedy on every platform, or carries a recorded exception. - The gate prints the remedy under a below-floor failure, so a missing platform key is a + The gate prints the remedy under a below-floor failure. A missing platform key is therefore a failure that tells the operator to upgrade and not how. A remedy that hands back into an - installer here is also checked to name a tool that installer manages, so the command it - prints can actually run. + installer here is also checked to name a tool that installer manages. The check makes sure + the command the remedy prints can actually run. """ linux_managed = self.declared_tools() windows_managed = self.declared_windows_tools() @@ -554,10 +563,10 @@ def test_every_managed_tool_is_executable(self) -> None: def test_every_windows_script_is_present(self) -> None: """Each Windows script is present, and none opens with a shebang. - The exec bit is the Linux form of "this will run", and on Windows the equivalent property - is the absence of a shebang: `scripts/repo_gate.py --check eol-coverage` requires git to - resolve any tracked file opening `#!` to `eol=lf` via a `.gitattributes` pin, and these - files carry none of their own. A shebang added later would fail that gate from a file + The exec bit is the Linux form of "this will run". On Windows the equivalent property + is the absence of a shebang. The check `scripts/repo_gate.py --check eol-coverage` requires + git to resolve any tracked file opening `#!` to `eol=lf` via a `.gitattributes` pin. These + files carry no pin of their own. A shebang added later would fail that gate from a file nobody would think to look at. """ scripts = ( @@ -586,8 +595,8 @@ def test_every_windows_script_is_present(self) -> None: def test_bootstrap_ps1_is_present_and_unmarked(self) -> None: """`bootstrap.ps1` is present, and does not open with a shebang. - Kept apart from `test_every_windows_script_is_present` rather than folded into it, because - `bootstrap.ps1` deliberately sits beside `bootstrap.sh` at `host-setup/`, not inside + Kept apart from `test_every_windows_script_is_present` rather than folded into it. The reason + is that `bootstrap.ps1` deliberately sits beside `bootstrap.sh` at `host-setup/`, not inside `host-setup/windows/` with the four scripts that test checks. Same reasoning as that test: the `eol-coverage` gate requires a `.gitattributes` pin for any tracked file opening `#!`, and this file carries none of its own. @@ -603,13 +612,13 @@ def test_bootstrap_ps1_is_present_and_unmarked(self) -> None: def test_bootstrap_ps1_names_the_system32_tar(self) -> None: """`bootstrap.ps1` reaches tar only through `Get-TarPath`, which names the System32 copy. - A pwsh launched from Git Bash finds MSYS tar first on `PATH`, and that tar reads the drive - prefix of a Windows archive path as a remote host, so a bare `tar` makes the extraction - depend on the shell the loader was started from. A path to any other tar, Git's own + A pwsh launched from Git Bash finds MSYS tar first on `PATH`. That tar reads the drive + prefix of a Windows archive path as a remote host. A bare `tar` therefore makes the + extraction depend on the shell the loader was started from. A path to any other tar, Git's own included, reaches the same MSYS tar without `PATH`. The check is an allow-list over every code line that names tar, so it holds whatever form the invocation on such a line takes. A `tar.gz` is not tar named only as an archive extension (`name.tar.gz`) or as a URL path - segment (`/tar.gz/`), neither of which can name an executable. + segment (`/tar.gz/`). Neither of those can name an executable. """ text = BOOTSTRAP_PS.read_text(encoding="utf-8") definition = "function Get-TarPath { Join-Path $env:SystemRoot 'System32\\tar.exe' }" @@ -659,7 +668,7 @@ def installer_tree(root: Path, platform: str, wrapper: str) -> Path: class TestDirectoryLockOrder(unittest.TestCase): """A run refused the directory lock stops before anything that removes a tree. - Each loader's cleanup removes trees by their fixed names, so a refused run that reached it would + Each loader's cleanup removes trees by their fixed names. So a refused run that reached that cleanup would delete the trees of the very run holding the lock. """ @@ -1015,8 +1024,8 @@ class TestPowerShellKeptTreeHandling(unittest.TestCase): """The Windows loader's removal and swap of the trees it owns, driven through its own functions. Each case runs `bootstrap.ps1`'s own functions, extracted by AST, against a scratch directory. - An entry a case locks is held for real, by an open handle on Windows, so the rename and delete - failures the loader is shaped around are the host's own rather than a stub's. + An entry a case locks is held for real, by an open handle on Windows. The rename and delete + failures the loader is shaped around are therefore the host's own rather than a stub's. """ def setUp(self) -> None: @@ -2063,8 +2072,9 @@ def test_a_hub_checkout_the_menu_runs_from_installs_from_that_checkout(self) -> class HubCleanupCases: """Which hub cache a menu's exit removes, the same cases for both menus. - A session removes only the tree whose marker still records its own fetch's token, since another - session sharing the directory may have fetched again since, and the tree is then that session's. + A session removes only the tree whose marker still records its own fetch's token. The restriction + exists because another session sharing the directory may have fetched again since, and the tree + is then that session's. """ TOKEN = "0123456789abcdef0123456789abcdef" @@ -2480,7 +2490,7 @@ class HubRefFreshnessCases: """Whether a menu reuses the cached hub tree for its session's own ref, the same cases for both menus. Each run is one session sharing the cache directory, cloning from a local origin whose `main` - and `develop` sit on different commits, reached through `insteadOf` so the menus' own hub URL + and `develop` sit on different commits. The origin is reached through `insteadOf` so the menus' own hub URL resolves to it. """ diff --git a/tests/test_setup_github.py b/tests/test_setup_github.py index 504499de7..7dd43c311 100755 --- a/tests/test_setup_github.py +++ b/tests/test_setup_github.py @@ -2,9 +2,9 @@ """Tests for the GitHub CLI half of the setup-github scripts, on both platforms. Both scripts report the account `gh` is logged in as and whether its git protocol for github.com -is ssh, and neither changes that protocol unless the opt-in flag asks for it, `--configure` -included. Each script is driven through its own functions with `gh` stubbed, since a real run reads -and writes the configuration of the host it runs on. +is ssh. Neither script changes that protocol unless the opt-in flag asks for it, `--configure` +included. Each script is driven through its own functions with the `gh` command stubbed. The reason is that a +real run reads and writes the configuration of the host it runs on. Run as `python3 tests/test_setup_github.py`, or under `python3 -m unittest discover -s tests`. """