Skip to content

Add the Windows Host Setup Tooling, and a PowerShell Gate to Check It - #674

Merged
ptr727 merged 9 commits into
developfrom
feature/windows-host-setup-tooling
Aug 11, 2026
Merged

Add the Windows Host Setup Tooling, and a PowerShell Gate to Check It#674
ptr727 merged 9 commits into
developfrom
feature/windows-host-setup-tooling

Conversation

@ptr727

@ptr727 ptr727 commented Aug 11, 2026

Copy link
Copy Markdown
Owner

host-setup/ carried Linux tooling only, so every Windows setup step was a manual read of the matrix in #483. This makes that column executable, and it was written and run on a Windows host rather than translated.

What Lands

host-setup/windows/, four scripts and a README, mirroring the Linux design where it applies and diverging where Windows genuinely differs.

Script Does
install-tools.ps1 Installs and upgrades the eight managed tools, and reports version, source, scope and status
upgrade-host.ps1 Upgrades the winget packages, and updates the WSL platform behind a Docker guard
setup-github.ps1 The SSH key, git identity, commit signing, the agent, and gh
setup-wsl.ps1 Installs a distribution, and reports the Docker Desktop integration

Plus PSScriptAnalyzerSettings.psd1 and a CI step, per-platform coverage in scripts/test_bootstrap.py, and the documentation.

Three Decisions Worth Reviewing

winget is the only source. Every tool in the contract has a winget package, measured, so the three-source machinery the Linux script needs for feeds that trail upstream has nothing to do here. A tool that ever lacks one is a finding to raise rather than a second source to add quietly.

Nothing elevates, and no scope is passed unless one is named. Forcing user scope installs a second copy beside a machine wide one rather than upgrading it, some installers fail when launched from an already elevated process, and a user scope install made from an elevated process lands in the administrator's profile. Letting each installer raise its own prompt is the path with the fewest failures. Scope drift is reported instead, with a SCOPE column, and -Reinstall is the only action that removes anything.

A package winget will not move is reported apart, with no command beside it. Its manifest asks for that because the application updates itself, so the version winget shows is the installer's rather than the one it runs. MSYS2 is the worked example: it upgrades through pacman, and printing winget upgrade --id MSYS2.MSYS2 beside a permanently stale number invites a reinstall over a working copy.

The PowerShell Gate

Built as the peer of the shellcheck step, per the same model: the file list comes from git ls-files, the checker runs as a container, and the same invocation is documented in GOVERNANCE.md and OPERATIONS.md so a local run and CI are the same check.

The module version is pinned beside the image, because the image alone does not fix it. 1.23.0 rather than the newest, since 1.24.0 needs a newer System.Management.Automation than the image carries and fails to import after installing cleanly, which reads as a broken gate rather than a version mismatch.

The file count is printed because this gate lied on its first invocation. It reported no findings while analyzing zero files, since the argument list never reached the container. That is the failure mode the repository is built against, and the count is what caught it.

Two rules are excluded with their reasons in the settings file: PSAvoidUsingWriteHost, since these scripts write their report to the console as their whole purpose and returning exit codes through the pipeline makes Write-Output wrong rather than merely different, and PSUseShouldProcessForStateChangingFunctions, since every script already carries -DryRun and a confirm prompt and adding -WhatIf beside them would give a reader two spellings of preview.

test_bootstrap.py

Coverage now asserts per platform, since the two installers do not manage the same set and the difference is a decision: git-restore-mtime is not applicable on Windows, and docker is one winget package there where the Linux answer differs by host role. NOT_MANAGED['windows'] is empty rather than absent, which is itself the assertion.

The executability check now reads the mode git records rather than the filesystem's. NTFS carries no exec bit, so st_mode reported every file as non-executable and the check failed on Windows against a correct tree. What the loader depends on is the mode a Linux checkout gets, which is the one git stores.

The loader invariant is deliberately unchanged. No bootstrap.ps1 lands, so bootstrap.sh still reads exactly one path, and widening that set before a Windows loader exists would retire the invariant ahead of the thing it protects.

Documentation

docs/host-setup.md gains the PowerShell verification block #483 deferred until somebody had run it on a Windows host, and the real reason for py -3 over python: an activated virtual environment shadows python, so the host gate would grade the environment rather than the host. The Store alias stub rules out python3 and chooses nothing between the other two, which is a weaker argument than the doc previously carried.

Verified on This Host

Windows 11 Pro 10.0.26200, PowerShell 7.6.4, winget v1.29.280, WSL 2.7.10.0, unelevated.

Every repository gate passes: prose_lint, repo_gate, markdownlint, actionlint, editorconfig-checker, PSScriptAnalyzer over 5 files, and all seven script self-tests. git ls-files --eol confirms CRLF on every new file, and no .ps1 opens with a shebang.

Both guards were proven to fire under -DryRun rather than printing a command: upgrade-host.ps1 -Wsl refuses while Docker Desktop runs, and an -Upgrade whose -Scope disagrees with the installed copy refuses and names -Reinstall. The coverage assertion was proven to bite by deleting docker from the registry and watching the test fail.

The apply path ran for real: install-tools.ps1 -Upgrade uv -Yes moved uv from 0.11.32 to 0.12.2 and the re-report reads current. Its first attempt failed because a running process held uv.exe, which is why the failure path now names the holding process instead of leaving winget's bare access denial.

Raised, Not Fixed Here

Three findings about the Linux side or the fleet, none blocking this: #671 the linter category is an undocumented decision rather than a missing feature, #672 gh uses https where the contract prescribes ssh and neither script checks it, and #673 the Linux scripts should refuse two actions instead of letting the last one win. All three are recorded in TODO.md.

Closing keywords are deliberately absent, per the branching model: they belong on the develop -> main promotion.

host-setup/ carried Linux tooling only, so every Windows setup step was a
manual read of the matrix in #483. This makes that column executable.

winget is the only source, because every tool in the contract has a winget
package, so the three-source machinery the Linux script needs for feeds that
trail upstream has nothing to do here.

Nothing elevates and no scope is passed unless one is named. Forcing user
scope installs a second copy beside a machine wide one, and some installers
fail when launched from an already elevated process, so letting each
installer raise its own prompt is the path with the fewest failures. Scope
drift is reported instead, and -Reinstall is the only action that removes
anything.

A package winget will not move is reported apart and left alone, with no
command beside it. Its manifest asks for that because the application
updates itself, so the version winget shows is the installer's rather than
the one it runs, and offering a command invites a reinstall over a working
copy in pursuit of a number that does not change.

PSScriptAnalyzer runs as the peer of the shellcheck step, built the same way:
the file list comes from git, the checker runs as a container, and the module
version is pinned beside the image because the image alone does not fix it.
1.23.0 rather than the newest, since 1.24.0 needs a newer
System.Management.Automation than the image carries and fails to import after
installing cleanly. The file count is printed because a run that read no
files reports the same clean as one that read them all, which this gate did
on its first invocation.

test_bootstrap.py asserts coverage per platform, and its executability check
now reads the mode git records rather than the filesystem's, since NTFS
carries no exec bit and the check failed on Windows against a correct tree.

docs/host-setup.md gains the PowerShell verification block #483 deferred
until somebody ran it on a Windows host, and the reason for py -3 over
python: an activated virtual environment shadows python, so the host gate
would grade the environment rather than the host.
Copilot AI lite review requested due to automatic review settings August 11, 2026 14:53
Resolve-InstalledVersion picked the newest of several side by side builds
with Sort-Object over a computed key, which orders a version as text. So
10.0.9 outranked 10.0.10 and 8.0.2 outranked 8.0.19, and the report named
an older build as the installed one.

The three dotnet builds this was written against hid it, since 110, 204 and
302 are all three digits and sort the same either way.

Compare-HostVersion already pads and compares numerically and was correct,
so the fix is to fold over it rather than to sort at all.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds first-class Windows host setup automation under host-setup/windows/ and enforces it in CI via a new PSScriptAnalyzer gate, while extending the existing bootstrap self-tests to validate Windows coverage and Windows-specific constraints.

Changes:

  • Introduce Windows host setup scripts (install/upgrade tools, configure Git/GitHub signing, manage WSL) plus Windows-specific documentation.
  • Add PSScriptAnalyzer settings + Docker-based PowerShell lint gate in CI, and document the same invocation for local runs.
  • Extend scripts/test_bootstrap.py to assert platform-specific installer coverage and to validate Windows script invariants.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
TODO.md Records follow-up issues surfaced while implementing Windows host tooling.
scripts/test_bootstrap.py Adds per-platform installer coverage checks and Windows script assertions; switches exec-bit check to git-indexed modes.
PSScriptAnalyzerSettings.psd1 Configures PSScriptAnalyzer exclusions with rationale for host-setup scripts.
OPERATIONS.md Documents local Docker invocations for shellcheck and PSScriptAnalyzer.
host-setup/windows/install-tools.ps1 Implements winget-based tool install/upgrade/report for Windows hosts.
host-setup/windows/upgrade-host.ps1 Upgrades winget packages and updates WSL with Docker-running guard.
host-setup/windows/setup-github.ps1 Configures SSH keys, git identity/signing, and reports GitHub/agent status on Windows.
host-setup/windows/setup-wsl.ps1 Installs WSL distros and reports Docker Desktop WSL integration settings.
host-setup/windows/README.md Explains Windows host tooling usage, requirements, and design decisions.
host-setup/README.md Adds Windows tooling to the host-setup entry point docs and updates cross-platform rules.
GOVERNANCE.md Documents the PSScriptAnalyzer gate and its Docker invocation alongside existing lint surfaces.
docs/host-setup.md Adds verified Windows-native verification block and references Windows host-setup tooling.
.github/workflows/validate-task.yml Adds CI step to run PSScriptAnalyzer in a container over tracked .ps1 files.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread host-setup/windows/install-tools.ps1
Comment thread host-setup/windows/setup-github.ps1 Outdated
Copilot AI review requested due to automatic review settings August 11, 2026 14:57
Get-WingetInstalled returns null where winget failed to answer and an empty
list where nothing is installed, and its comment says the two are different.
Get-ToolState then folded null into the empty list, so a tool whose state
could not be read reported as missing, or as unmanaged where it answers on
PATH. The comment described a distinction the code discarded.

A read that failed now carries its own status. An install or upgrade skips
such a tool and collects it as a failure rather than installing against a
state nobody measured, which is how a second copy lands beside a first one
that was there all along.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (3)

OPERATIONS.md:26

  • This OPERATIONS.md one-liner is intended to mirror the CI PowerShell gate, but it currently differs from .github/workflows/validate-task.yml (no early-exit when $files is empty, and Format-Table uses default columns). To keep "local run == CI" true, align it with the workflow's PowerShell block.
docker run --rm --pull=always -e PS_SCRIPTS="$(git ls-files '*.ps1')" -v "$PWD":/mnt --workdir /mnt mcr.microsoft.com/powershell:latest pwsh -NoProfile -Command 'Set-PSRepository PSGallery -InstallationPolicy Trusted; Install-Module PSScriptAnalyzer -RequiredVersion 1.23.0 -Force -Scope AllUsers; Import-Module PSScriptAnalyzer; $files = $env:PS_SCRIPTS -split "`n" | Where-Object { $_ }; $found = @(); foreach ($f in $files) { $found += Invoke-ScriptAnalyzer -Path $f -Settings ./PSScriptAnalyzerSettings.psd1 }; Write-Host "Checked $($files.Count) file(s)"; if ($found) { $found | Format-Table -AutoSize | Out-String -Width 200 | Write-Host; exit 1 }; Write-Host "no findings"'

scripts/test_bootstrap.py:94

  • declared_windows_tools() will silently scan to EOF if it cannot find the closing ) for the $TOOLS = @( registry, which can produce a misleading tool set and hide a broken registry. Treat a missing close marker as an explicit coverage-check failure (like the missing open marker case).
    closed = PS_TOOLS_CLOSE.search(text, opened.end())
    body = text[opened.end():closed.start() if closed else len(text)]
    names = set(PS_TOOL_NAME.findall(body))
    if not names:
        failures.append('install-tools.ps1 declares a $TOOLS registry with no Name fields this can read')
    return names

GOVERNANCE.md:416

  • The documented PSScriptAnalyzer invocation here does not match the CI step in .github/workflows/validate-task.yml: CI exits early when no .ps1 files are tracked and prints a table with stable columns (RuleName, Severity, ScriptName, Line, Message). Keeping these snippets identical matters because the PR description says local and CI runs are the same check, and it avoids "clean" output that hides a mis-wired file list.
      $files = $env:PS_SCRIPTS -split "`n" | Where-Object { $_ }
      $found = @()
      foreach ($file in $files) { $found += Invoke-ScriptAnalyzer -Path $file -Settings ./PSScriptAnalyzerSettings.psd1 }
      Write-Host "Checked $($files.Count) file(s)"
      if ($found) { $found | Format-Table -AutoSize | Out-String -Width 200 | Write-Host; exit 1 }

Copilot AI review requested due to automatic review settings August 11, 2026 15:01
Two findings from the first review round.

Invoke-WingetRemove passed the scope the caller asked to end up in rather
than the one holding the copy, so -Reinstall gh -Scope user ran the uninstall
against user scope, found nothing where the copy is machine wide, and the
install then added a second copy beside the first. That is the state
-Reinstall exists to clear. Every copy is now removed in the scope it was
found in, which also covers a tool installed in both.

Test-Prerequisite named git, ssh and ssh-keygen while the script also calls
ssh-keyscan and ssh-add, so a host missing either failed partway through with
whatever that command says instead of the one message that names the remedy.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (2)

host-setup/windows/install-tools.ps1:397

  • Add-ToolNote logs the resolved python path verbatim. On Windows this path often contains the username (for example C:\Users\<name>\...), which the surrounding comment says to avoid; it also tends to leak into logs/issues. Consider redacting %LOCALAPPDATA% / %USERPROFILE% prefixes before printing.
        # Written unexpanded, because the expanded form names a real account and the prose gate rejects that.
        note 'python' 'python3 resolves to the Microsoft Store alias stub under %LOCALAPPDATA%\Microsoft\WindowsApps, so py -3 is the name this contract uses here'
        $resolved = Get-Command python -ErrorAction SilentlyContinue
        if ($resolved -and $resolved.Source -notmatch 'Python\d') {

OPERATIONS.md:26

  • The local-verification command for PSScriptAnalyzer doesn't match the CI invocation in validate-task.yml (CI handles the empty-file case explicitly and formats findings with stable columns). Since this section says to run the CI invocations, it would be better to mirror the workflow step here to avoid drift.
docker run --rm --pull=always -v "$PWD":/check --workdir /check mstruebing/editorconfig-checker:latest
docker run --rm --pull=always -v "$PWD":/mnt --workdir /mnt koalaman/shellcheck:stable $(git ls-files '*.sh')
docker run --rm --pull=always -e PS_SCRIPTS="$(git ls-files '*.ps1')" -v "$PWD":/mnt --workdir /mnt mcr.microsoft.com/powershell:latest pwsh -NoProfile -Command 'Set-PSRepository PSGallery -InstallationPolicy Trusted; Install-Module PSScriptAnalyzer -RequiredVersion 1.23.0 -Force -Scope AllUsers; Import-Module PSScriptAnalyzer; $files = $env:PS_SCRIPTS -split "`n" | Where-Object { $_ }; $found = @(); foreach ($f in $files) { $found += Invoke-ScriptAnalyzer -Path $f -Settings ./PSScriptAnalyzerSettings.psd1 }; Write-Host "Checked $($files.Count) file(s)"; if ($found) { $found | Format-Table -AutoSize | Out-String -Width 200 | Write-Host; exit 1 }; Write-Host "no findings"'

Copilot AI review requested due to automatic review settings August 11, 2026 15:07
…gistry with no end

Three suppressed findings from the first review round, all correct.

The invocations in GOVERNANCE.md and OPERATIONS.md differed from the CI step,
which undercut the one claim they exist to support, that a local run and CI
are the same check. Both now carry the empty-list exit and the explicit
column list.

Running the documented line verbatim then found a defect none of the three
named. The list split on a newline alone, which is what a shell joins it
with, while PowerShell joins it with spaces. From PowerShell the analyzer
received one path holding every file, reported it as one file it could not
find, and exited 0 having checked nothing. It splits on whitespace now, and
the regex is double-quoted so the invocation survives the single-quoted
-Command a shell passes. Run verbatim it reports five files from either
shell.

declared_windows_tools scanned to end of file where it could not find the
registry's closing parenthesis, collecting every later Name field in the
script, so a broken registry answered with a larger tool set than it declares
and the coverage check passed on it. A missing close marker is a failure now,
matching the missing open marker beside it.
@ptr727

ptr727 commented Aug 11, 2026

Copy link
Copy Markdown
Owner Author

Answering the 3 Suppressed Findings

These carry no thread, so per the runbook they are answered here. All three were correct and all three are fixed in 02909d3.

1. OPERATIONS.md:26 and 3. GOVERNANCE.md:416 — the documented invocations differed from CI.

Correct, and the divergence undercut the one claim those snippets exist to support. Both now carry the empty-list exit and the explicit RuleName,Severity,ScriptName,Line,Message column list, so the three copies are the same command.

Running the documented line verbatim then found a defect none of the three findings named, and it is the more serious one. The file list split on a newline alone. That is what a shell joins git ls-files output with, so CI was fine, but PowerShell joins it with spaces, and this repository is worked from both. From PowerShell the analyzer received one argument holding all five paths, reported it as one file it could not find, and exited 0 having checked nothing:

Invoke-ScriptAnalyzer: Cannot find path '/mnt/host-setup/agent-safety/install.ps1 host-setup/windows/install-tools.ps1 ...' because it does not exist.
Checked 1 file(s)
no findings

That is the same false clean this step's file count was added to catch, one level up. It splits on whitespace now, and the regex is double-quoted rather than single-quoted so the invocation survives the single-quoted -Command a shell passes, since PowerShell escapes with a backtick and leaves the backslash alone. Verified by running the OPERATIONS.md line verbatim from both shells, each reporting Checked 5 file(s).

2. scripts/test_bootstrap.py:94 — a registry with no closing marker scanned to EOF.

Correct, and worse than a misleading tool set: PS_TOOL_NAME would go on to match every later Name = '...' in the script, so a broken registry answers with a larger set than it declares and the coverage assertion passes on it. A missing close marker is now an explicit failure, matching the missing open marker beside it.

Proven by removing the closing ) from the registry while leaving the later Name fields in place:

[FAIL] bootstrap  1 issue(s)
         install-tools.ps1 opens a $TOOLS registry this cannot find the end of, so coverage cannot be checked

Also fixed this round, found by self-review rather than by the reviewer, and worth naming since both are the same class:

  • Resolve-InstalledVersion picked the newest of several side-by-side builds with Sort-Object, which orders a version as text, so 10.0.9 outranked 10.0.10 and 8.0.2 outranked 8.0.19. The three dotnet builds it was written against hid it, since 110, 204 and 302 are all three digits. Fixed in 65d3594, and it now folds over Compare-HostVersion, which was already correct. 13 cases cover it, including the ones that were wrong.
  • Get-WingetInstalled documents a three-valued answer, null for "could not read" and empty for "not installed", and Get-ToolState folded the two together, so a tool whose state could not be read reported as missing. The comment described a distinction the code discarded. Fixed in 9f7ab82 with an unreadable status that an install or upgrade refuses to act on, verified against a shim that makes winget exit non-zero.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (2)

OPERATIONS.md:25

  • The shellcheck invocation uses an unquoted command substitution, which does not match the CI invocation and can split unexpectedly if any tracked script path contains whitespace. Since this section says to run the CI invocations, mirror the workflow's array-based form here.
docker run --rm --pull=always -v "$PWD":/mnt --workdir /mnt koalaman/shellcheck:stable $(git ls-files '*.sh')

host-setup/windows/README.md:28

  • pwsh -File .\install-tools.ps1 is still subject to the current PowerShell execution policy; it just avoids the command-discovery rule that requires a .\ prefix. If the intent is to document a policy-independent invocation, it should include -ExecutionPolicy Bypass (or reword to avoid implying policies are irrelevant).
**Script execution.** A `git clone` carries no mark of the web, so these run under the default `RemoteSigned` policy. A browser-downloaded zip does not, and is blocked until `Unblock-File` clears the mark. The `.\` prefix is required when running a script from the current directory, exactly as it is for [`agent-safety/install.ps1`][agent-safety]. `pwsh -File .\install-tools.ps1` is the form that depends on no policy at all.

…he home path

Three suppressed findings from the second review round, all correct.

The README said pwsh -File depends on no policy. It does not: -File answers
the command discovery rule that wants a leading .\ and leaves the execution
policy applying, so a marked file under RemoteSigned fails with a
SecurityError naming it as unsigned. Measured, rather than reasoned about.
The policy independent form is -ExecutionPolicy Bypass, which
docs/host-setup.md already gives for the write-safety installer, and the
text now prefers clearing the mark over bypassing since a bypass covers
every script that run touches.

The shellcheck line added to OPERATIONS.md used an unquoted command
substitution where CI reads the list into an array, so a tracked path
carrying whitespace would word-split. It now matches the workflow.

The python note printed the resolved interpreter path verbatim, which on
Windows carries the account name into whatever the report is pasted into,
against the reasoning the comment two lines above it states. The home
directory is replaced by the variable that names it, which a reader expands
themselves. Only this note is redacted: the paths in setup-github.ps1 sit
inside commands the reader runs, and a placeholder there would leave a
remedy that does not work when pasted.
Copilot AI review requested due to automatic review settings August 11, 2026 15:15
@ptr727

ptr727 commented Aug 11, 2026

Copy link
Copy Markdown
Owner Author

Answering the 3 New Suppressed Findings

Round 2's findings, all correct, all fixed in c936709.

1. host-setup/windows/README.md:28pwsh -File is still subject to the execution policy.

Correct, and the sentence was simply wrong. -File answers the command-discovery rule that wants a leading .\ and leaves the policy applying. Measured rather than reasoned about, on a file carrying the mark of the web:

$ pwsh -NoProfile -ExecutionPolicy RemoteSigned -File policytest.ps1
SecurityError: File ...\policytest.ps1 cannot be loaded. The file ... is not digitally signed.
exit=1

$ pwsh -NoProfile -ExecutionPolicy Bypass -File policytest.ps1
script ran
exit=0

The first attempt at this test proved nothing, because the Process scope in that session was already Bypass; forcing the policy on the command line is what made the two cases separable.

The text now names -ExecutionPolicy Bypass as the policy-independent form, matching what docs/host-setup.md already gives for the write-safety installer, and prefers Unblock-File over bypassing since a bypass covers every script that run touches rather than the one being fixed.

2. OPERATIONS.md:25 — the shellcheck line used an unquoted command substitution.

Correct, and it was a line this PR added, so the drift arrived with the change rather than predating it. It now reads the list into an array exactly as the workflow does:

mapfile -t scripts < <(git ls-files '*.sh'); docker run ... koalaman/shellcheck:stable "${scripts[@]}"

Verified by running that line verbatim from OPERATIONS.md, exit 0.

3. host-setup/windows/install-tools.ps1:397 — the python note printed the resolved path verbatim.

Correct, and it contradicted the comment two lines above it, which writes %LOCALAPPDATA% unexpanded precisely so the report does not name an account. The note now passes the path through a redaction that replaces the home directory with the variable naming it:

before  python resolves to C:\Users\<name>\Source\Repos\...\.venv\Scripts\python.exe
after   python resolves to %USERPROFILE%\Source\Repos\...\.venv\Scripts\python.exe

Scoped to this note deliberately, and the boundary is worth stating. setup-github.ps1 also prints the home path, and it is left alone because those paths sit inside commands the reader is meant to run, such as ssh-add <key> and the icacls repair. A %USERPROFILE% placeholder does not expand in PowerShell, so redacting there would trade a leaked account name for a remedy that fails when pasted. The rule applied is that an informational path is redacted and a path inside a runnable command is not.


All six suppressed findings raised on this PR are now answered, and the two inline threads are resolved. Head is c936709.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

OPERATIONS.md:25

  • This one-liner uses mapfile, which is not available in the default macOS bash (3.2). Since this section is a local-run runbook and already relies on bash features (process substitution), use a bash-3-compatible loop so the command works on macOS without requiring a newer shell.
mapfile -t scripts < <(git ls-files '*.sh'); docker run --rm --pull=always -v "$PWD":/mnt --workdir /mnt koalaman/shellcheck:stable "${scripts[@]}"

Comment thread host-setup/windows/setup-github.ps1
The line added last round matched the workflow by using mapfile, which
arrives in bash 4 while macOS ships 3.2, so the runbook command failed on a
platform GOVERNANCE.md names as supported and the fleet has a machine of.

A while read loop collects the same array on bash 3, so the list is still
passed as one argument per path and a path carrying whitespace still
survives. The check itself is unchanged.

Both container lines taking a file list now differ from the workflow in form
rather than in what they check, so the reason is written down beside them
rather than left to be rediscovered as drift.
Copilot AI review requested due to automatic review settings August 11, 2026 15:22
@ptr727

ptr727 commented Aug 11, 2026

Copy link
Copy Markdown
Owner Author

Answering the Round 3 Suppressed Finding

One new finding, correct, fixed in ef35935.

OPERATIONS.md:25mapfile is not in macOS bash.

Correct. mapfile arrives in bash 4 and macOS ships 3.2, so the line failed on a platform GOVERNANCE.md "Cross-platform by default" names as supported and that the fleet has a machine of.

This finding and round 2's pull in opposite directions, and both are satisfiable at once. Round 2 asked the line to mirror CI's array-based form so a path carrying whitespace is not word-split; this round asks it not to use the bash 4 builtin CI uses to build that array. A while read loop gives the array without the builtin:

scripts=(); while IFS= read -r f; do scripts+=("$f"); done < <(git ls-files '*.sh'); docker run ... "${scripts[@]}"

Verified two ways: run verbatim from OPERATIONS.md it exits 0 over the 8 tracked scripts, and fed a b.sh it yields count=2 first=[a b.sh], so the whitespace property round 2 asked for is intact. Process substitution is bash 3.2 material, so the remaining construct is safe there.

The runbook now states why its two file-list lines differ from the workflow, since that difference has been raised twice and would otherwise be rediscovered as drift a third time. Both differ in form and neither in what it checks, and both differences exist because the runbook runs on a developer's machine where the workflow runs on ubuntu-latest: the shell list avoids mapfile for the reason above, and the PowerShell list splits on whitespace because a shell joins git ls-files output with newlines while PowerShell joins it with spaces.


All seven suppressed findings raised on this PR are now answered and fixed, the two inline threads are resolved, and no finding has been declined. Head is ef35935.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (1)

.github/workflows/validate-task.yml:60

  • The comment describing the failure mode when splitting the file list by newline is inaccurate. In that case PowerShell passes one combined path to Invoke-ScriptAnalyzer, so the analyzer effectively runs over no files (not “the rest”). This matters because the comment is meant to explain how the gate can falsely report clean.
      # Splitting on the newline alone hands the analyzer one path holding every file, which it reports as one file it cannot find and a clean run over the rest.

Add-KnownHost recorded whatever ssh-keyscan answered, where the Linux peer
checks the offered key against the fingerprints GitHub publishes and refuses
to record one that does not match. Trust on first use is the whole exposure:
nothing pins the real key yet on the one run that enrolls it, so a
substituted key recorded there is what every later connection verifies
against. The check is now the peer's, including its refusal where the
published list cannot be read at all.

Enrolling on this host also failed for a second reason the message did not
name. The OpenSSH under System32 is older than the key exchange github.com
offers and returns no key at all, where the copy Git for Windows ships
succeeds, so the run died reporting that nothing came back rather than why.
That is also why this host had no github.com entry to begin with, and
recording the checked key made SSH authentication start working.

pr_review.py read gh output as the platform locale. On Windows that is
cp1252, so a review body carrying one typographic quote raised a
UnicodeDecodeError inside the reader thread and the caller then parsed a null
stdout. The wait command died that way on this pull request. Every text mode
read now names UTF-8, which is what gh emits everywhere.
Copilot AI review requested due to automatic review settings August 11, 2026 15:34
@ptr727

ptr727 commented Aug 11, 2026

Copy link
Copy Markdown
Owner Author

Answering the Round 4 Suppressed Finding, and a Defect It Exposed in the Review Tooling

.github/workflows/validate-task.yml:60 — the comment described the failure mode inaccurately.

Correct. The comment said a newline-only split leaves "a clean run over the rest", and there is no rest: PowerShell passes one combined argument, so the analyzer runs over nothing and the step prints a count of one with no findings. Fixed in 2dc1670.

Worth noting the finding was precise about which copy was wrong. The same explanation appears three times, and GOVERNANCE.md and OPERATIONS.md already said "over nothing". Only the workflow comment carried the inaccurate wording, and that is the one it flagged.


scripts/pr_review.py Crashed on Windows, and Is Fixed Here

Raising this because it is outside this PR's stated scope and I changed it anyway.

The wait command died mid-round:

UnicodeDecodeError: 'charmap' codec can't decode byte 0x9d in position 11962
  ...
TypeError: the JSON object must be str, bytes or bytearray, not NoneType

Every subprocess.run(..., text=True) in the script decoded with the platform locale, which on Windows is cp1252. gh emits UTF-8 everywhere, so a review body carrying a single typographic quote killed the reader thread, and the caller then parsed a None stdout into that second traceback. The four text-mode reads now name encoding='utf-8'.

Why it is in this PR rather than an issue. This PR exists to make Windows a first-class host, .github/copilot-instructions.md routes the review loop through this script, and the loop could not complete a round on the platform being added. The change is four keyword arguments and a comment, the 228 self-tests in scripts/test_pr_review.py pass, and the command that crashed now runs. Happy to split it out if you would rather it landed separately.

This also means every earlier round of this review was read through a script that would crash on any non-ASCII byte, which is worth knowing when reading the rounds above.


All 8 suppressed findings and all 3 inline threads raised on this PR are now fixed and answered, none declined. Head is 2dc1670.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (1)

host-setup/windows/install-tools.ps1:301

  • Invoke-WingetUpgrade ignores -Scope: unlike Invoke-WingetInstall (which appends --scope when $WANT_SCOPE is set), the upgrade path never passes --scope. This makes -Scope ineffective for upgrades and contradicts the script's documented behavior that scope is only acted on when explicitly named.
function Invoke-WingetUpgrade {
    param([Parameter(Mandatory)][string]$Id)
    $arguments = @('upgrade', '--id', $Id, '--exact', '--source', 'winget', '--disable-interactivity',
        '--accept-source-agreements', '--accept-package-agreements', '--silent', '--include-unknown')
    return (run 'winget' @arguments)
}

Invoke-WingetUpgrade dropped -Scope while Invoke-WingetInstall honored it, so
the option was documented as naming the copy to act on and then acted on
nothing for the action most likely to use it.

The comment justifying the omission described a hazard that has since moved.
Naming a scope the installed copy does not sit in is refused before either
call runs, so a scope reaching the upgrade is one a copy is actually in, and
passing it is what says which copy to move where a tool is installed in both
scopes at once. That is the case -Scope exists for on an upgrade.
Copilot AI review requested due to automatic review settings August 11, 2026 15:40
@ptr727

ptr727 commented Aug 11, 2026

Copy link
Copy Markdown
Owner Author

Answering the Round 5 Suppressed Finding

host-setup/windows/install-tools.ps1:301Invoke-WingetUpgrade ignored -Scope.

Correct, and it was an inconsistency I had reasoned myself into. Fixed in f170a0b.

Invoke-WingetInstall appended --scope when the caller named one and the upgrade path did not, so -Scope was documented as naming the copy to act on and then acted on nothing for the action most likely to use it.

The comment justifying the omission described a hazard that had since moved. It said naming a scope that disagrees with the found install adds a second copy beside it, which was true when written. That case is now refused by the scope-mismatch guard added two rounds ago, before either call runs. So a scope reaching the upgrade is one an installed copy is actually in, and passing it is safe.

It is also the point of the option there. Where a tool is installed in both scopes, an upgrade with no scope leaves winget to choose which copy to move; -Scope is what says which. That is the same shadowing case -Reinstall exists for, and the two now agree.

Verified on this host across all three paths:

python (user, outdated) + -Scope user     -> ... --include-unknown --scope user
python (user, outdated) + no -Scope       -> ... --include-unknown
docker (machine, outdated) + -Scope machine -> ... --include-unknown --scope machine
gh (machine) + -Scope user                -> refused, names -Reinstall

All 9 suppressed findings and all 3 inline threads on this PR are fixed and answered, none declined. Head is f170a0b.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

@ptr727
ptr727 merged commit 7f9caaa into develop Aug 11, 2026
7 checks passed
@ptr727
ptr727 deleted the feature/windows-host-setup-tooling branch August 13, 2026 16:46
ptr727 added a commit that referenced this pull request Aug 15, 2026
…Python CI Gates (#718)

Thirty-one squashes, `56f4d7d..d54862a`. 115 files, +20436/-5298.

**Merge with a merge commit, never a squash, and never with
`--delete-branch`.** This pull request's head is `develop` itself.

## What lands

**Fleet Skills.** The `.agents/skills/` source tree, the generated
`.claude-plugin/` distribution, `scripts/build_dist.py` with its
`--check` gate, and `scripts/skills_install.py` with its host stamp
(#676). Packaged as skills on top of the scaffold: PR review conduct and
Copilot instructions upkeep (#677), comment and doc style (#678),
resync-a-repo and fleet-conformance-check (#679), the per-language
codestyles (#680), git commit conventions and operational vs release
workflow (#681), stand up a repo (#683), and repo-worktree (#717).
Coverage gaps closed in three passes (#690, #691, #692) plus the P4
sentence-length opt-in (#697).

**Host setup.** The Windows host-setup tooling and its PowerShell gate
(#674), the Windows bootstrap loader (#682), Docker install and upgrade
on Linux and Windows with a version floor (#701, #705), a `uv` floor in
`spec/host-tools.json` (#698), self-healing of a shadowing `uv`, `jq`,
or `git-restore-mtime` copy (#689), node's real winget package id
(#696), and a README for the Linux host-setup nuances (#710).

**Python and CI.** Python tooling in CI with the script tests moved to
`scripts/tests` (#704), `ruff format` adopted and gated (#709), and the
PSScriptAnalyzer claim conditioned on repos that carry `.ps1` files
(#686).

**Conduct rules.** Triage-order and scope guardrails in
pr-review-conduct (#684), `pr_review.py wait` requesting a review rather
than only polling for one (#685), a tech-agnostic signed-commit
verification (#708), execution rather than analogy to verify
platform-specific code (#715), and a unique worktree for every task
(#717).

**Docs.** The fleet map and gap register with peer messaging declared
(#687), mermaid flow diagrams in the kept-authority docs (#702), and the
map pointed at the shipped diagrams and current tooling (#703).

## Issues this promotion closes

Each landed on `develop` on its own pull request. The keyword fires only
on a merge into `main`, so it sits here rather than on the feature pull
requests.

Closes #700
Closes #707
Closes #711
Closes #712
Closes #714
Closes #688

#699 stays open on purpose: #717 shipped the layout convention and the
skill, and the physical migration of existing checkouts is still tracked
there.

## Review record

Every squash closed its own Copilot loop on its own pull request before
merging to `develop`. This promotion carries no new content of its own,
so its review is the merged tree as a whole.

## Consequence worth stating

The `GOVERNANCE.md` and `AGENTS.md` sections these squashes changed
become the canonical the moment this reaches `main`, and every carrying
repository reads as drifted from that point until it resyncs. That is
the ordinary consequence of a canonical moving rather than a defect. The
Skills installer added here is also how a machine picks the new skills
up, so a session that keeps restating a rule already packaged as a skill
is the signal to run it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants