local-ci-cd-system/plans/opus47-analysis.md

Local-CI-CD-System — Comprehensive Technical Review

Reviewer role: lead architect / senior DevOps engineer. Scope: complete repository audit at HEAD. Style: direct, opinionated, no hedging. Severities: CRITICAL / HIGH / MEDIUM / LOW / NICE-TO-HAVE.

---

1. Executive Summary

This is a well-engineered homelab CI/CD system that solves a real, narrow problem: give a single workstation the ability to run isolated, reproducible builds against ephemeral Windows and Linux VMs, triggered from a self-hosted Gitea, with artifact collection and lifecycle management. The scope is appropriate, the implementation is largely competent, and the audit trail in TODO.md shows the author has been doing serious, deliberate engineering rather than vibe-coding.

That said, the system has the typical pathologies of a one-person homelab CI: the "tests" do not test what matters, the documented "production-ready" status rests on three manual e2e runs (e2e-008, e2e-009, §3.3 e2e), and several large operational gaps remain hidden behind the Home Lab — Deferred label in TODO.md. Those gaps are not catastrophic, but they would be unacceptable in a paid environment.

The single biggest architectural concern is not security — the network is air-gapped enough for a homelab — but operational resilience under concurrency. capacity: 4 is configured in runner/config.yaml, but the IP-allocation mutex in scripts/Invoke-CIJob.ps1 has only ever been exercised with a single job at a time. Whether the lock semantics, the DHCP behaviour on VMnet8, and the vmrun start race actually behave correctly when four real builds collide is an open question.

The second largest concern is the complete absence of automated regression testing for the orchestration itself. The four Pester files use a fake vmrun.cmd and exercise argument parsing only. There is no CI for the CI.

Top three priorities, in order:

1. CRITICAL — Validate or downgrade capacity: 4. Either run a real 4-way concurrency burn-in or set capacity: 1 until validated. Today the production claim is a guess.
2. HIGH — Add a single end-to-end smoke test runnable on demand. Not a unit test. A scripted Measure-CIBenchmark.ps1-style invocation that exercises Phase 1–6 against a real VM and exits non-zero on failure. Wire it into a manual Gitea workflow.
3. HIGH — Eliminate the runner/Install-Runner.ps1 deprecated script and the Setup-Host.ps1 default-password ghost. Dead code and lying defaults erode confidence in everything else.

The rest of this document expands on these and 30+ smaller findings.

---

2. Project Completeness

The system declares itself "production-ready" in README.md line 1. Measured against what a reasonable definition of that phrase requires, here is the state:

Capability	Status	Evidence
Ephemeral VM lifecycle (clone, start, destroy)	Complete	scripts/New-BuildVM.ps1, Remove-BuildVM.ps1, Invoke-CIJob.ps1
Windows guest (WinRM HTTPS)	Complete	Invoke-RemoteBuild.ps1, Get-BuildArtifacts.ps1 Windows branch
Linux guest (SSH)	Complete	_Transport.psm1, Linux branches in build/artifact scripts
Auto-detect guest OS from VMX	Complete	Invoke-CIJob.ps1 guestOS regex
Host-clone mode + zip transfer (Mode 1)	Complete	Default path in Invoke-RemoteBuild.ps1
In-VM git clone (Mode 2)	Complete	-UseGitClone, PAT via http.extraHeader
Credential storage (CredentialManager)	Complete	BuildVMGuest, GiteaPAT
Reusable composite action	Complete	gitea/actions/local-ci-build/action.yml
Build matrix Windows + Linux	Complete	gitea/workflows/build-nsInnoUnp.yml
Shared NuGet/pip cache via HGFS	Complete	Set-TemplateSharedFolders.ps1, -UseSharedCache
Structured JSONL logs	Complete	Write-JobEvent in Invoke-CIJob.ps1
Scheduled maintenance	Complete	Register-CIScheduledTasks.ps1 (4 tasks)
Disk-space + runner-health watchdogs	Complete	Watch-DiskSpace.ps1, Watch-RunnerHealth.ps1
Retention policy with aggressive fallback	Complete	Invoke-RetentionPolicy.ps1
Orphan cleanup	Complete	Cleanup-OrphanedBuildVMs.ps1
Template backup before refresh	Complete	Backup-CITemplate.ps1
Benchmark harness	Complete	Measure-CIBenchmark.ps1
Secret injection into guest (ExtraGuestEnv)	Complete	Forwarded host → action → Invoke-CIJob → Invoke-RemoteBuild
Pester unit tests	Stub-level	tests/*.Tests.ps1, all use fake vmrun.cmd
Automated integration test	Missing	No automated harness exercises real VMs
Concurrency burn-in	Missing	TODO.md §2.1 admits "race non riprodotta in e2e-008/009"
Multi-host federation	Not in scope	TODO.md §6.3 deferred
Template snapshot refresh procedure	Partial	Backup-CITemplate.ps1 exists; refresh procedure not codified
Setup-Host post-conditions documented	Partial	Setup-Host.ps1 does the work but Validate-DeployState.ps1 covers templates, not host

Overall completion against the original scope: ~85%. The remaining 15% is concentrated in test coverage and concurrency validation — exactly the parts that justify the "production-ready" label.

---

3. Architecture Deep Dive

3.1 Component Map

+---------------------+      +------------------------+
|  Gitea (10.10.20.11)|<-----|  act_runner (NSSM svc) |
+---------------------+      |  F:\CI\act_runner       |
                              |  capacity: 4            |
                              +-----------+-------------+
                                          |
                                          v
                              +-----------+-------------+
                              |  gitea/actions/         |
                              |   local-ci-build        |
                              |   action.yml            |
                              +-----------+-------------+
                                          |
                                          v
                              +-----------+-------------+
                              |  Invoke-CIJob.ps1       |
                              |  (orchestrator)         |
                              +-----+---+---+-----+-----+
                                    |   |   |     |
                  +-----------------+   |   |     +-----------------+
                  |                     |   |                       |
                  v                     v   v                       v
        +---------+--------+   +--------+--+--------+   +-----------+-----+
        | New-BuildVM.ps1  |   | Wait-VMReady.ps1   |   | Invoke-Remote   |
        | (vmrun clone)    |   | (list, ping/22,    |   |  Build.ps1      |
        |                  |   |  5986/ssh echo)    |   | (WinRM or SSH)  |
        +---------+--------+   +--------------------+   +-----------+-----+
                  |                                                 |
                  |                                                 v
                  |                                    +------------+--------+
                  |                                    | Get-BuildArtifacts  |
                  |                                    | (Copy-Item -From    |
                  |                                    |  Session or scp)    |
                  |                                    +------------+--------+
                  v                                                 |
        +---------+--------+                                        |
        | Remove-BuildVM   | <--------- always runs in finally -----+
        | (stop, deleteVM) |
        +------------------+

This is a clean pipeline. The orchestrator is a single script that owns the try/finally and the state file (leaseFile). Sub-scripts are pure: they receive parameters and return either a value or throw. That is the right shape.

3.2 Layering — What Is Right

• _Common.psm1 and _Transport.psm1 correctly separate two distinct concerns: WinRM/vmrun helpers (host-side, used by Windows path) and SSH helpers (host→guest, used by Linux path). Invoke-Vmrun returning [pscustomobject]@{ExitCode, Output} is the right abstraction — most callers want both, and $LASTEXITCODE is fragile across pipelines.
• The composite action local-ci-build is the right boundary. The calling repository (nsis-plugin-nsinnounp, per build-nsInnoUnp.yml) declares intent (build-command, artifact-source) and the action handles transport. Adding a second consumer would require zero changes here.
• The file-based IP mutex + lease is conceptually correct. Serialize the racey phase (clone + start + IP acquisition), then release. Build phases run parallel. That is the right granularity.
• The Mode 1 / Mode 2 source-transfer split. Mode 1 (host clone → tar/zip → ship) is the safe default. Mode 2 (-UseGitClone, PAT via http.extraHeader) is the right optimization for large repos with submodules — and the http.extraHeader Basic-auth approach keeps the PAT out of the URL, argv, and git config.
• Phase 1 readiness check uses vmrun list, not getGuestIPAddress. This is correct and the rationale in the comments of scripts/Wait-VMReady.ps1 is exactly right. The author has clearly been burned by this in the past.

3.3 Layering — What Is Wrong or Suspect

• Measure-CIBenchmark.ps1 uses getGuestIPAddress directly (line ~135) for the IP-acquire phase, contradicting the lesson encoded in Wait-VMReady.ps1. The benchmark therefore measures something subtly different than what production does. It also has no fallback. MEDIUM.
• Invoke-RemoteBuild.ps1 has two PAT-injection paths: a Linux branch that builds an authenticated URL with [uri]::EscapeDataString and a Windows branch that uses http.extraHeader. The Linux version writes the PAT into the URL of an export GIT_CLONE_URL=...; git clone "$GIT_CLONE_URL" ...; unset sequence. That URL is visible in the SSH wire — encrypted, but the PAT is also visible in /proc/<pid>/environ for whoever can read it (the same ci_build user). The Windows path is strictly safer. MEDIUM. Make Linux use http.extraHeader too.
• The IP-allocation lock is held during New-BuildVM.ps1 (clone) AND vmrun start AND IP detection (Invoke-CIJob.ps1 lines ~340–430). A linked clone takes a few seconds; vmrun start headless + IP-acquire takes 30–90 s on a typical Windows guest. With capacity: 4, the worst-case wait for the fourth job to enter the lock is ~4 × 90 s = 6 minutes before its build even starts. The build phase is parallel, so amortized this is fine, but document this explicitly. MEDIUM.
• No per-IP allocation strategy. The system relies entirely on VMware's NAT DHCP to hand out unique IPs. The lock prevents simultaneous starts but does nothing if DHCP hands the same IP to two VMs on the same VMnet8 lease window. The lease-collision check at Invoke-CIJob.ps1:~430 (if (Test-Path $leaseFile) { throw }) is a fail-fast trap but never a recovery path. MEDIUM — see §15 for the suggested static-IP scheme.

3.4 The "Auto" GuestOS Detection

This is clever and correct: read the VMX, regex guestOS = "...", classify as Linux if it contains ubuntu or linux, else Windows. It works for the two templates that exist. It will silently mis-classify a future guestOS = "windows2025srv-64" if Microsoft renames the family — but that is acceptable in this scope. LOW: assert against a known list and warn if the family is unknown.

3.5 The guestVar ci-ip Channel

Invoke-CIJob.ps1 reads guestVar 'ci-ip' as the primary IP-discovery channel and falls back to getGuestIPAddress. The guest side is ci-report-ip.service writing via vmware-rpctool — referenced but not in any file I read. The fallback works for Windows; the primary is the only viable path for Linux because getGuestIPAddress on Linux is notoriously unreliable.

The risk: there is no validation in Validate-DeployState.ps1 that the guest service is enabled in a fresh template. If someone refreshes the Linux template and forgets the systemd unit, every Linux job will silently use the (broken) getGuestIPAddress fallback until it times out at 120 s. HIGH: add a Validate-DeployState.ps1 check that fails the deploy if ci-report-ip.service is not present on Linux templates.

---

4. Code Quality

4.1 PowerShell 5.1 Discipline

The codebase respects the PS 5.1 constraint rigorously. Every script I read has the standard preamble:

#Requires -Version 5.1
Set-StrictMode -Version Latest
$ErrorActionPreference = 'Stop'

No null-coalescing operators, no ternaries, no &&/||, no ForEach-Object -Parallel. This is consistent across 14 scripts. The author understands the runtime.

A subtler PS 5.1 issue does appear in scripts/Backup-CITemplate.ps1:

$totalBytes = [long]0
if ($backupFiles) { foreach ($f in $backupFiles) { $totalBytes += $f.Length } }

This manual sum is there because Measure-Object -Sum plus Set-StrictMode -Version Latest interacts badly when the input is empty. That is correct defensive coding for PS 5.1 + StrictMode. Good.

4.2 ErrorActionPreference Discipline

Three scripts intentionally relax $ErrorActionPreference to Continue:

• Cleanup-OrphanedBuildVMs.ps1 — cleanup must proceed through all orphans.
• Invoke-RetentionPolicy.ps1 — same rationale.
• Watch-DiskSpace.ps1 and Watch-RunnerHealth.ps1 — exit-code-driven contract with Task Scheduler.

This is correct. But three places set Continue for narrower reasons:

• Invoke-CIJob.ps1:~265 wraps git clone to allow capturing exit code.
• Wait-VMReady.ps1:~190 wraps ssh to demote the "Permanently added host key" stderr.
• Invoke-CIJob.ps1:~395 wraps vmrun readVariable guestVar ci-ip to allow exit-code inspection.

All three save/restore $savedEap = $ErrorActionPreference; ...; $ErrorActionPreference = $savedEap correctly. This is the only safe pattern in PS 5.1 — try { ... } finally { $ErrorActionPreference = ... } would also work but is more verbose. No issue.

4.3 Parameter Validation

Excellent in places, missing in others.

Strong:

• IP regex ValidatePattern('^((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)\.){3}(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)$') is used consistently in Invoke-CIJob.ps1, Invoke-RemoteBuild.ps1, Get-BuildArtifacts.ps1, Wait-VMReady.ps1.
• ValidateRange on numeric parameters (MaxAgeHours, RetentionDays, MinFreeGB, Iterations, TimeoutSeconds, MaxRestarts).
• ValidateSet on GuestOS ('Windows', 'Linux', 'Auto') and Transport ('WinRM', 'SSH').
• ValidatePattern('^[A-Za-z]$') on DriveLetter.

Missing:

• JobId in Invoke-CIJob.ps1 is [string]$JobId mandatory but unvalidated. It is used as a directory name and a Credential-Manager target lookup key. A user-supplied JobId containing ..\..\.. or ..\Logs\${someone-else}\ would let a malicious workflow write outside F:\CI\Logs. In this homelab that surface is local-only, but it is one line of defense to add: [ValidatePattern('^[A-Za-z0-9._-]+$')]. LOW.
• TemplatePath in Invoke-CIJob.ps1 is read from $env:GITEA_CI_TEMPLATE_PATH and validated only with Test-Path -PathType Leaf. Not a security issue (the runner controls the env var), but a sanity-check that the path lives under F:\CI\Templates would prevent operator typos pointing at a production VMX. LOW.
• BuildCommand in Invoke-CIJob.ps1 is forwarded verbatim into the guest. By design — the caller can run anything — but documenting that the build-command runs as the privileged build user with no sandboxing is worth saying explicitly in the README. LOW.

4.4 Style

PSScriptAnalyzer is configured in PSScriptAnalyzerSettings.psd1 with the right exclusions for this project (PSAvoidUsingWriteHost excluded because act_runner captures Write-Host and Write-Output interferes with return values). Indentation is 4-space consistent. Function naming follows verb-noun (Write-JobEvent, Compress-BuildArtifact, Remove-OldJobDirs). Comment-based help is present on every script. This is genuinely good housekeeping.

One stylistic complaint: the inline Invoke-Command -Session $session -ScriptBlock { ... } blocks in Invoke-RemoteBuild.ps1 are getting long enough (~50 lines each, twice in the file) that splitting them into named local functions on the host side would help testability. NICE-TO-HAVE.

4.5 Dead Code

• runner/Install-Runner.ps1 is marked DEPRECATED at the top of the file and superseded by Setup-Host.ps1. It should be deleted. LOW but immediate.
• Setup-Host.ps1 .DESCRIPTION references a default password 'CIBuild!ChangeMe2026' but the actual param([string]$GuestPassword = '') is empty. The documentation lies. LOW — pick one and align.

4.6 Logging Density

Write-Host is used liberally and consistently with [Component] prefixes ([Invoke-CIJob], [Wait-VMReady], [Cleanup], etc.). Combined with the JSONL log via Write-JobEvent, the runtime observability is genuinely good. The transcript is preserved per job under F:\CI\Logs\$JobId\invoke-ci.log and invoke-ci.jsonl. This is better than most paid CI systems give you.

---

5. Security

The threat model documented in docs/BEST-PRACTICES.md §2.1 (per TODO.md) explicitly accepts: Defender / Firewall / UAC disabled inside the build VM, self-signed WinRM cert with SkipCACheck/SkipCNCheck/SkipRevocationCheck, SSH StrictHostKeyChecking=no with UserKnownHostsFile=NUL. For an isolated VMnet8 NAT network in a single-tenant home lab, this is defensible — the attack surface is host-local.

Findings within that scope:

5.1 PAT Handling (Mode 2 Windows path) — GOOD

scripts/Invoke-RemoteBuild.ps1 Mode 2 Windows branch reads the PAT from Get-StoredCredential -Target 'GiteaPAT' on the host, base64-encodes user:pat, and injects it via git -c http.extraHeader=Authorization: Basic <b64>. The PAT never appears in:

• the clone URL
• argv (because -c key=value is one arg)
• git config files
• the transcript log (the Write-Host calls echo $cloneUrl, not the auth-injected version)

The PAT does live in memory inside the WinRM session for the duration of the clone. The WinRM session is Authentication Basic over TLS to the local guest. This is acceptable.

5.2 PAT Handling (Mode 2 Linux path) — WEAKER, MEDIUM

The Linux branch rewrites the URL to https://user:pat@host/repo.git and passes it via env var:

$envCloneCmd = "export GIT_CLONE_URL='$cloneTargetUrl'; git clone --depth 1 --branch '$CloneBranch'"
...
$envCloneCmd += " `"`$GIT_CLONE_URL`" '$GuestLinuxWorkDir'; unset GIT_CLONE_URL"

The PAT is exposed in:

• /proc/<pid>/environ while git clone runs (readable by the same ci_build user, but also root, but also any other process under the same UID).
• The SSH command line on the host side as a single command string (encrypted in transit, but visible in ps aux to anyone on the host watching ssh.exe's commandline).

Fix: mirror the Windows approach. Pass user and pat as separate variables, use git -c http.extraHeader=Authorization: Basic <base64>. MEDIUM, fix.

# Suggested Linux Mode 2 replacement
$b64 = [Convert]::ToBase64String([Text.Encoding]::UTF8.GetBytes("$($pat.UserName):$($pat.GetNetworkCredential().Password)"))
$cloneCmd = @(
    "GIT_TERMINAL_PROMPT=0 git",
    "-c 'credential.helper='",
    "-c 'http.extraHeader=Authorization: Basic $b64'",
    "clone --depth 1 --branch '$CloneBranch'",
    $(if ($CloneSubmodules) { '--recurse-submodules' }),
    "'$CloneUrl' '$GuestLinuxWorkDir'"
) -join ' '
Invoke-SshCommand -IP $IPAddress -User $SshUser -KeyPath $SshKeyPath -Command $cloneCmd

The base64 still appears in the SSH command line argument briefly, but it is no longer a credential URL that could be cached by git or leaked via a redirected error.

5.3 Guest Credential Storage

Get-StoredCredential -Target 'BuildVMGuest' is correct usage of Windows Credential Manager. Setup-Host.ps1 stores it once. The PSCredential object is forwarded to all scripts. Good. No issue.

5.4 The Lab-Wide BuildVMGuest Account

Every clone has the same credentials. A compromised build (malicious build-command, e.g. on a public PR) could:

1. Read $Credential.GetNetworkCredential().Password from its own WinRM session: it does not have this — credentials live on the host, only the WinRM service receives the authenticated handshake. Not exploitable in-guest.
2. Persist data into the template by mounting \\vmware-host\Shared Folders\ (HGFS). Yes — the shared folders for NuGet/pip caches are read-write. A malicious build could poison a NuGet package and have it served to the next build. MEDIUM.

Mitigation: mark the shared folders read-only in the VMX for builds that do not need to write to them. Or, more practically, use per-template-version cache subdirs and accept the risk for a homelab. Document it. MEDIUM.

5.5 ExtraGuestEnv Secret Injection

The path secrets.SIGN_PASS → workflow → action input extra-guest-env-json → ConvertFrom-Json → hashtable → Invoke-CIJob.ps1 → Invoke-RemoteBuild.ps1 → [System.Environment]::SetEnvironmentVariable($pair.Key, $pair.Value, 'Process') is clean.

But: the action.yml line INPUT_EXTRA_GUEST_ENV_JSON: ${{ inputs.extra-guest-env-json }} puts the secret JSON into a process environment variable on the runner host. Gitea Actions masks secrets.* in logs by string match. If a secret value happens to be a substring of a path or filename, the masking can miss it. This is a Gitea limitation, not yours, but worth a one-liner in docs/WORKFLOW-AUTHORING.md. LOW.

5.6 Setup-Host.ps1 Password Default

Currently [string] $GuestPassword = '', which triggers a prompt — fine. But the help text claims 'CIBuild!ChangeMe2026' is the default. If a future user reads the help and assumes the default is acceptable, they may not change it. LOW — strip the default password from documentation or make it Read-Host -AsSecureString-only.

5.7 OWASP Top 10 Coverage (in scope)

Risk	Status
A01 Broken Access Control	Local host only. Acceptable.
A02 Cryptographic Failures	Self-signed TLS for lab use. Acceptable. PAT base64 in command line is the only real concern (§5.2).
A03 Injection	BuildCommand is by-design caller-controlled. JSON env injection parses safely via ConvertFrom-Json. SSH commands built from string concatenation in Invoke-RemoteBuild.ps1 Linux branch — $GuestLinuxWorkDir, $CloneBranch, etc. are not user-supplied in the trigger path but a workflow author could pass '$(rm -rf /)' in submodules etc. The single-quote escape on ExtraGuestEnv values is correct; other interpolated strings are not escaped. MEDIUM — see §6.7.
A04 Insecure Design	Acceptable for homelab.
A05 Security Misconfiguration	Defender/Firewall/UAC off, documented in BEST-PRACTICES. Acceptable for isolated lab.
A06 Vulnerable Components	TODO.md §1.3 SHA256 pinning was deferred. Toolchain installer downloads (Tier-1/Tier-2) use only placeholders. MEDIUM — see §6.8.
A07 Auth Failures	Shared BuildVMGuest account — see §5.4.
A08 Software/Data Integrity	No artifact signing. NICE-TO-HAVE for a homelab.
A09 Logging Failures	Strong (invoke-ci.jsonl, Event Log, webhook). Good.
A10 SSRF	Not applicable.

---

6. Reliability

6.1 IP Allocation Under Concurrency — CRITICAL UNVALIDATED

Invoke-CIJob.ps1 lines ~310–460 implement: open F:\CI\State\vm-start.lock with FileShare.None (acts as cross-process mutex on Windows), 10-minute deadline, retry every 5 s. Inside the lock: clone, start, acquire IP, write lease file. Release the lock.

This is sound design. The bugs hiding here are:

1. The lock is implemented as a file handle with FileMode.OpenOrCreate. If the host crashes while the lock is held, the file remains on disk. On reboot, the next process can re-open it because the kernel has released the lock — that part is fine. But a process inside its own lock that throws before try { ... } finally reaches the lock release would leak the handle. The try { acquire } ... finally { release } pattern in lines ~340–460 is correct, but the lock acquisition itself is outside a try/finally in the surrounding try. Trace it carefully — there is one path where [System.IO.File]::Open(...) succeeds, then the while ($true) { ... break } exits normally, and execution continues into Write-Host "[Invoke-CIJob] VM-start lock acquired." and then into the inner try { ... } finally { release }. The handle is closed correctly on every path. OK.

2. Remove-Item $lockPath -Force inside the inner finally. This deletes the lock file on the filesystem after closing the handle. If two processes are racing — process A has just released and deleted the file, process B is in its while ($true) retry loop — process B will successfully OpenOrCreate a new file. That is fine. But the Remove-Item is a useful indicator-of-cleanliness on disk, nothing more. Some Windows AV products briefly hold a handle on file deletes which would cause Remove-Item to fail — the -ErrorAction SilentlyContinue handles that. No issue.

3. IP detection inside the lock: 120 s deadline, polling every 2 s. The slowest plausible case is a Windows guest booting cold and waiting for VMware Tools to publish an IP. 120 s is generous and validated by Measure-CIBenchmark.ps1 data (assumed; not actually inspected). OK.

4. The unvalidated case: four concurrent jobs all hit the lock within milliseconds of each other. Job 1 enters, takes ~60 s. Jobs 2/3/4 retry every 5 s. After job 1 releases, only one of 2/3/4 will win the race (whichever's OpenOrCreate happens first in the kernel). The other two retry. This serializes the four jobs into ~60 s × 4 = 4 minutes of pre-build phase. The build phase then runs parallel. Acceptable but unmeasured. The TODO.md §2.1 — Home Lab: deferred. Race non riprodotta in e2e-008/009. is honest but incomplete. HIGH — actually run Invoke-CIJob.ps1 four times in parallel from a PowerShell harness and confirm zero IP collisions, zero lock leaks, zero hangs.

6.2 VM Destroy Resilience

Remove-BuildVM.ps1's soft-stop-then-hard-stop-then-poll-then-deleteVM-with-3-retries sequence is well thought out. The 20 s "wait until VMX disappears from vmrun list" step exists because vmware-vmx.exe holds a file lock briefly after stop. This is real and the workaround is correct.

The fallback Remove-Item -Recurse after deleteVM failure is the right "give up gracefully" path. Combined with Cleanup-OrphanedBuildVMs.ps1 scheduled every 6 h with MaxAgeHours 4, anything that survives the immediate destroy path is reaped within hours. GOOD.

6.3 act_runner Auto-Restart

Watch-RunnerHealth.ps1 is rate-limited (3 restarts/h via runner-restart-log.json rolling window). Beyond that limit it writes EventId 1004 Error and exits non-zero so Task Scheduler shows red. Webhook posted on every restart. This is genuinely good.

One subtle bug: the cooldown log is JSON-serialized as an array of ISO timestamp strings. Line ~118 $restartLog = @($raw | ConvertFrom-Json). If the log contains exactly one entry, ConvertFrom-Json returns a single string, not an array. The @(...) coercion handles that. If the log file is empty (zero bytes), ConvertFrom-Json throws — but $raw is checked for truthiness first. OK.

A more relevant flaw: the script does not check whether the service was stopped intentionally (e.g. operator running Backup-CITemplate.ps1 which calls Stop-Service act_runner). It will try to restart it. Backup-CITemplate.ps1 itself uses Stop-Service -Force and Start-Service in a try/finally, so the conflict window is short, but a Watch-RunnerHealth.ps1 invocation that falls inside that window will count as a "restart" in the cooldown log. This is benign but noisy. LOW — Backup-CITemplate.ps1 could disable the scheduled task while it works, or Watch-RunnerHealth.ps1 could check for a F:\CI\State\runner-maintenance.flag and exit 0.

6.4 Disk Space Alerting

Watch-DiskSpace.ps1 checks every 15 min, writes Event Log on alert, optionally posts to webhook. Exits non-zero so Task Scheduler shows red. Pairs with Invoke-RetentionPolicy.ps1's aggressive-mode fallback (below 50 GB free → 7-day retention instead of 30). GOOD.

6.5 Retention Policy

Invoke-RetentionPolicy.ps1 correctly handles the case where the drive is missing (falls back to [double]::MaxValue to avoid aggressive mode firing on no data). The stale-lease cleanup (>12 h) is the right defensive belt-and-suspenders for crashed-host recovery. GOOD.

One thing missing: there is no retention on F:\CI\BuildVMs\ itself. Orphan cleanup handles VMs older than 4 h; the retention policy handles artifacts and logs. But a partial clone (a directory created without a .vmx because vmrun clone failed mid-way) older than 4 h with no .vmx is removed by Cleanup-OrphanedBuildVMs.ps1 only via the dir-removal-only fallback — good, no issue.

6.6 Wait-VMReady Phase Logic

The 3-phase wait is the right shape and the comment in lines ~108–120 correctly documents why getGuestIPAddress is not used for Phase 1 (Tools-dependent). The fact that the Windows fallback in Invoke-CIJob.ps1 IP detection still uses getGuestIPAddress is consistent — by Phase 3b the VM has been up long enough that Tools should be responsive.

6.7 String Interpolation into SSH Commands

Invoke-RemoteBuild.ps1 Linux branch builds shell commands by string concatenation:

$buildCmd = "${envPrefix}cd '$GuestLinuxWorkDir' && $BuildCommand"

$GuestLinuxWorkDir is single-quoted, which is correct. $BuildCommand is not — by design, because the caller provides shell. $envPrefix correctly single-quote-escapes values via $envVal = $ExtraGuestEnv[$envKey] -replace "'", "'\''". OK for trusted inputs.

But:

Invoke-SshCommand -IP $IPAddress -User $SshUser -KeyPath $SshKeyPath `
    -Command "rm -rf '$GuestLinuxWorkDir' && mkdir -p '$GuestLinuxWorkDir'"

If a workflow author somehow gets $GuestLinuxWorkDir to contain ' && rm -rf / # (it cannot today — it is hardcoded in Invoke-CIJob.ps1), they would have remote code execution as ci_build. Today this is not exploitable because all interpolated values into SSH commands originate from Invoke-CIJob.ps1 parameters that are not workflow-controllable. LOW. Audit boundary should be documented.

6.8 Toolchain Installer Pinning

Install-CIToolchain-WinBuild2025.ps1 and Install-CIToolchain-Linux2404.sh (per TODO.md §1.3) use SHA256 placeholders — they were deferred for the homelab. This means a man-in-the-middle on the public internet at template-rebuild time could substitute compromised toolchains. The probability is low (TLS protects everything), but the defense in depth is missing. MEDIUM — implement real hash pinning before any non-homelab use.

---

7. Operational Gaps

Gap	Severity	Notes
No documented template-refresh runbook	HIGH	Backup-CITemplate.ps1 exists, but the procedure (Win KMS reactivation, snapshot rebuild, validation) is not codified.
No automated end-to-end smoke test	HIGH	The "production-ready" claim rests on e2e-008/e2e-009 manual runs.
capacity: 4 never validated	HIGH	See §6.1.
No CI for the CI	HIGH	PSScriptAnalyzer lint workflow exists; no Pester run on commit; no integration test on commit.
No alerting on stuck jobs	MEDIUM	A job that hangs at 1 h 50 min (just under the 2 h runner timeout) is invisible. Watchdog only checks the runner service, not job duration.
No central log aggregation	MEDIUM	JSONL files are per-job. Cross-job analysis requires manual grep. A Get-CIJobStats.ps1 summarizer would help.
No quota/limit on PR builds	MEDIUM	A misbehaving workflow could fork the orchestrator. Concurrency lock prevents IP collisions but not disk fill. MaxAgeHours 4 provides eventual cleanup.
No multi-host fan-out	NICE-TO-HAVE	§6.3 deferred. Single point of failure is the workstation. Acceptable for homelab.
No remote-access procedure for debugging a stuck VM	LOW	Cleanup-OrphanedBuildVMs.ps1 will eventually destroy a stuck VM. Manual inspection between job and cleanup requires knowing the clone path.
No host hardware monitoring	LOW	CPU/RAM/disk-IO not measured. Measure-CIBenchmark.ps1 is phase-time only.
BaseClean snapshot age not tracked	LOW	Windows KMS lease is 180 days. After that, builds will silently degrade (activation issues affecting dotnet).
runner/Install-Runner.ps1 deprecated but present	LOW	Delete it.
Setup-Host.ps1 password documentation mismatch	LOW	See §4.5.

---

8. CI/CD Workflow Quality

8.1 The Composite Action — Mostly Good

gitea/actions/local-ci-build/action.yml does the right things:

• Forwards every workflow input through env: rather than direct interpolation into the shell. This prevents shell injection from inputs.build-command. GOOD.
• Builds the Invoke-CIJob.ps1 parameter hashtable conditionally based on which inputs are populated. GOOD.
• Emits artifact-path and artifact-name as step outputs for downstream actions/upload-artifact. GOOD.
• Has both a success-path upload-artifact and a failure-path log upload. GOOD.

Concerns:

• Hardcoded $ciScriptsDir = 'N:\Code\Workspace\Local-CI-CD-System\scripts'. If the user clones the repo elsewhere, the action breaks. The composite action should resolve scripts via ${{ github.action_path }}\..\..\scripts (i.e. relative to the action file). MEDIUM — this couples the action to one specific host filesystem layout.
• The action's outputs: block declares artifact-path and artifact-name but they appear inside the inputs: map by mistake (lines just before runs:). YAML-wise this still parses as an outputs:-like key at the wrong level, but a stricter linter would flag it. LOW — restructure.
• No input validation. inputs.guest-os accepts any string; if a typo 'Linus' is passed, it propagates to Invoke-CIJob.ps1 which rejects it via ValidateSet. The error is clear at runtime but late. LOW.
• No timeouts at the action level. Relies entirely on the runner-level timeout: 2h in runner/config.yaml. LOW.

8.2 The Calling Workflow build-nsInnoUnp.yml

• Matrix on [windows, linux], fail-fast: false. GOOD.
• job-id-suffix: '${{ matrix.target }}' to disambiguate the artifact directories. GOOD — this is exactly the right use of the suffix.
• submodules: 'true' as string (the action only checks eq 'true'). OK.
• repo-url: 'ssh://gitea-ci/Simone/nsis-plugin-nsinnounp.git' uses the host-side SSH alias configured in Setup-Host.ps1. The alias lives in the user's ~/.ssh/config — which user? act_runner runs as SYSTEM (per Register-CIScheduledTasks.ps1). SYSTEM's home is C:\Windows\System32\config\systemprofile\. So Setup-Host.ps1 writes the SSH config to the user's ~/.ssh/config, not SYSTEM's. HIGH — this only works if the runner was installed under the user account, not SYSTEM. Verify. If act_runner is installed as a service under SYSTEM, this URL alias is broken.

I cannot fully verify §8.2 last point without reading the NSSM install args in Setup-Host.ps1 more carefully (the snippet I read showed sc.exe failure configurations but the actual nssm install line for the service account was not in my reads). Treat as HIGH unverified — exact service account configuration must be checked.

8.3 Lint Workflow

gitea/workflows/lint.yml (not read in this session) is referenced in TODO.md as completed for PSScriptAnalyzer. The fact that there is no equivalent for Pester is the gap. HIGH — add a tests.yml workflow that runs the Pester suite on every push.

8.4 Workflow Example

gitea/workflow-example.yml exists at the repo root as a reference for downstream users. GOOD — useful for onboarding.

---

9. Test Coverage

Four Pester v5 files exist in tests/:

• _Common.Tests.ps1
• New-BuildVM.Tests.ps1
• Remove-BuildVM.Tests.ps1
• Wait-VMReady.Tests.ps1

Per TODO.md §5.1, they all use a fake vmrun.cmd driven by $env:FAKE_VMRUN_EXIT. What this means in practice:

Tested	Not Tested
Argument construction for vmrun clone	Whether vmrun clone actually clones
Invoke-Vmrun return shape	Whether Invoke-Vmrun works with real vmrun.exe
Parameter validation regexes	Whether Test-NetConnection -Port 5986 works
Path resolution helpers	Whether Start-Sleep polling deadlines fire correctly
Throw-on-error paths via fake exit codes	The actual failure modes (vmware-vmx.exe file lock, DHCP timeout, etc.)
WinRM session options object shape	Whether WinRM actually accepts self-signed cert
Phase 1 readiness loop (with fake vmrun list output)	Whether real VM boot timing fits the deadline

This is meaningful coverage — the Pester suite catches regressions in argument shape, parameter validation, return types, and exit-code interpretation. It is NOT integration coverage. Calling this "production-ready" testing is overstating it.

What is missing in priority order:

1. HIGH — a tests/Integration/ folder with at least one script that does Measure-CIBenchmark.ps1 -Iterations 1 and asserts all phases under thresholds. Runnable on demand (manual workflow_dispatch in Gitea). Wire to a [ci-burnin] commit tag for full 4× concurrency.
2. HIGH — Pester tests for Invoke-CIJob.ps1 phase orchestration (mock the sub-scripts via function override in the test scope). Today the orchestrator is the most complex script and has the least coverage.
3. MEDIUM — Pester tests for Invoke-RemoteBuild.ps1 Linux PAT-injection (after fixing §5.2).
4. MEDIUM — Pester tests for Invoke-RetentionPolicy.ps1 aggressive-mode threshold logic and stale-lease cleanup with mocked file dates.
5. LOW — a single "make sure all 14 scripts parse cleanly" check via [System.Management.Automation.Language.Parser]::ParseFile in CI. The lint workflow probably already does this implicitly.

Estimated test coverage (lines exercised by Pester / lines of production code): ~15%. That is below typical homelab expectations but consistent with a system written by one operator who tests by running the real thing.

---

10. Observability

This is the area I am most positive about. The system genuinely has better observability than most paid CI systems give you:

Channel	Purpose	Quality
F:\CI\Logs\$JobId\invoke-ci.log	Per-job transcript	High
F:\CI\Logs\$JobId\invoke-ci.jsonl	Per-job structured phase events	High
F:\CI\Logs\benchmark.jsonl	Cross-job benchmark trend	High
Windows Event Log CI-DiskAlert (EventId 1001)	Disk alert	Medium
Windows Event Log CI-RunnerHealth (EventIds 1002/1003/1004)	Runner state changes	High
Webhook (Discord/Gitea-compatible)	Real-time notification	High
F:\CI\State\runner-restart-log.json	Rolling 1-hour restart cooldown	High
F:\CI\State\ip-leases\<ip>.lease	Active IP allocations	Medium
Task Scheduler history	Scheduled-task health	Medium
PSScriptAnalyzer lint workflow output	Code health	Medium

What is missing:

• Job duration alerting. A job pinned at 1 h 55 min before timing out at 2 h is invisible until it fails. A simple per-job watchdog (Phase 5 start time + 90 min → webhook warning) would help. MEDIUM.
• A Get-CIJobSummary.ps1 script that scans the last N days of invoke-ci.jsonl files and produces a table of jobs, phases, durations, success rate. Today this is a manual grep. LOW.
• Host hardware metrics. CPU/RAM/disk-IO over time would let you correlate slow builds with host contention. Not in scope today. NICE-TO-HAVE.
• No metric on linked-clone delta size growth. If something starts modifying the template files outside of refresh cycles, deltas balloon. Measure-CIBenchmark.ps1 doesn't track this. LOW.

---

11. Technical Debt

In priority order:

1. runner/Install-Runner.ps1 deprecated. Delete.
2. Setup-Host.ps1 password documentation mismatch. Align help with actual default.
3. Measure-CIBenchmark.ps1 uses getGuestIPAddress instead of guestVar ci-ip. Inconsistent with production path. Refactor to share IP-acquire logic with Invoke-CIJob.ps1 via a helper in _Common.psm1.
4. Hardcoded N:\Code\Workspace\Local-CI-CD-System\ paths in gitea/actions/local-ci-build/action.yml and Register-CIScheduledTasks.ps1. The action should use ${{ github.action_path }}; the scheduled tasks could read from a config file in F:\CI\State\.
5. Linux PAT injection rewrites the URL. Convert to http.extraHeader (§5.2).
6. Two divergent SHA256 placeholder strategies in toolchain installers (TODO.md §1.3). Real pinning would fix this, but at minimum keep the placeholder shape identical across both installers.
7. Invoke-RemoteBuild.ps1 is 500+ lines with two ~50-line ScriptBlocks inline. Extract New-GuestBuildScriptBlock and New-GuestCloneScriptBlock into named functions for readability and testability.
8. Duplicate parameter blocks for IPAddress, Credential, GuestOS, SshKeyPath, SshUser repeated across Invoke-RemoteBuild.ps1, Get-BuildArtifacts.ps1, Wait-VMReady.ps1. A shared parameter splat helper would reduce drift risk.
9. docs/archived/2026-05-10/ holds old plan/README/test docs. Fine as history, but the TEST-PLAN-v1.3-to-HEAD.md at top level should reference these explicitly so readers know where to find prior state.
10. No CHANGELOG. The TODO.md is acting as one. Move historical "done" sections into docs/CHANGELOG.md once a release is tagged.

---

12. Issues by Priority

CRITICAL

• capacity: 4 is unvalidated. Either burn-in or downgrade. Right now the system claims a concurrency capability it has not measured. (runner/config.yaml, scripts/Invoke-CIJob.ps1)

HIGH

• No automated end-to-end test. Measure-CIBenchmark.ps1 exists but is not wired to CI and runs without assertions. (scripts/Measure-CIBenchmark.ps1)
• act_runner service account vs SSH alias mismatch potential. If runner runs as SYSTEM, the gitea-ci SSH alias written by Setup-Host.ps1 lives in the wrong ~/.ssh/config. (Setup-Host.ps1, gitea/workflows/build-nsInnoUnp.yml)
• No CI for the CI: Pester tests never run on commit, no integration test. (gitea/workflows/)
• Template-refresh procedure not codified. KMS re-activation, snapshot rebuild, validation steps live in operator memory. (docs/, missing)
• Validate-DeployState.ps1 does not assert ci-report-ip.service presence on Linux templates. Silent fallback to 120 s timeout if the service is missing. (template/Validate-DeployState.ps1)

MEDIUM

• Linux Mode 2 PAT injection rewrites the URL. Move to http.extraHeader (§5.2). (scripts/Invoke-RemoteBuild.ps1)
• Measure-CIBenchmark.ps1 uses getGuestIPAddress (anti-pattern documented elsewhere). (scripts/Measure-CIBenchmark.ps1)
• Shared HGFS caches are read-write from the guest. A compromised build can poison NuGet/pip caches. (scripts/Set-TemplateSharedFolders.ps1)
• Hardcoded N:\Code\Workspace\... paths in composite action and scheduled tasks. (gitea/actions/local-ci-build/action.yml, scripts/Register-CIScheduledTasks.ps1)
• No retention on F:\CI\BuildVMs\ partial-clone directories below the 4 h orphan threshold. (scripts/Cleanup-OrphanedBuildVMs.ps1)
• No SHA256 pinning of toolchain installers (homelab-deferred but worth a flag). (template/Install-CIToolchain-WinBuild2025.ps1)
• No job-duration watchdog. Jobs near the 2 h timeout invisible until they fail. (scripts/, missing)
• Invoke-RemoteBuild.ps1 has duplicate PAT-injection logic between Linux and Windows branches. (scripts/Invoke-RemoteBuild.ps1)

LOW

• runner/Install-Runner.ps1 deprecated — delete.
• Setup-Host.ps1 password default documentation mismatch.
• JobId parameter unvalidated — path-traversal-shaped values accepted.
• TemplatePath parameter not asserted to live under F:\CI\Templates\.
• Composite action's outputs: block sits inside inputs: map — YAML layout error.
• Watch-RunnerHealth.ps1 cannot distinguish operator-initiated stops from crashes.
• Backup-CITemplate.ps1 does not validate that the template is fully powered-off before copy — relies on Stop-Service act_runner but does not check for residual vmware-vmx.exe.
• Get-CIJobStats.ps1 summarizer missing.
• docs/CHANGELOG.md missing (TODO.md is acting as one).
• String interpolation in SSH commands with non-user-controlled paths — not exploitable today, audit-document.

NICE-TO-HAVE

• Artifact signing.
• Host hardware metrics.
• Multi-host federation (TODO.md §6.3).
• Quota system for runaway builds.
• Per-build Defender exclusion lift/restore on the host (faster I/O during clone).
• Linked-clone delta size monitoring.

---

13. Top 10 Quick Wins

These are the changes that pay back the most for the least effort.

1. Delete runner/Install-Runner.ps1. Five-second change. Removes a misleading file.

2. Align Setup-Host.ps1 password documentation with code.

   # In param block
   [string] $GuestPassword = ''   # keep
   # In help text
   .PARAMETER GuestPassword
       Plain-text password for the guest VM build account.
       When empty (default), the script will prompt with Read-Host -AsSecureString.
       Do NOT hardcode this in production scripts.
   

3. Add a [ValidatePattern('^[A-Za-z0-9._-]+$')] to JobId in Invoke-CIJob.ps1 and the composite action. One line each. Closes path-traversal even though not exploitable today.

4. Replace hardcoded $ciScriptsDir in action.yml with ${{ github.action_path }}\..\..\scripts:

   run: |
     $ciScriptsDir = Resolve-Path (Join-Path '${{ github.action_path }}' '..\..\scripts')
   

   Decouples the action from one specific clone location.

5. Fix Linux Mode 2 PAT injection to use http.extraHeader (snippet in §5.2). Removes PAT from URL/argv/process environment.

6. Add a Validate-DeployState.ps1 check for ci-report-ip.service on Linux templates. A single systemctl is-enabled ci-report-ip.service over SSH after deploy.

7. Wire Measure-CIBenchmark.ps1 -Iterations 1 to a workflow_dispatch job named "ci-self-test" in a new gitea/workflows/self-test.yml. Exits non-zero on any phase failure. The runner runs it against itself.

8. Add a single test for the orchestrator's IP-detection fallback path in tests/Invoke-CIJob.Tests.ps1. Mock Invoke-Vmrun to return empty guestVar, assert fallback to getGuestIPAddress. ~30 lines.

9. Add Get-CIJobSummary.ps1 that reads the last 7 days of F:\CI\Logs\*\invoke-ci.jsonl and prints a table:

   JobId         Started              Elapsed   Phase1   Phase5    Status
   42-1          2026-05-10 14:22     04:32     0:08     03:42     success
   43-1-windows  2026-05-10 14:30     03:18     0:07     02:48     success
   

   ~40 lines.

10. Add a 90-minute "phase 5 still running" webhook warning to Invoke-CIJob.ps1. A background Start-Job with a sleep and a POST. Fires once per job. ~15 lines.

---

14. Completion Roadmap

What separates "working homelab" from "actually production-ready, even by homelab standards":

Phase A — Validation (1–2 sessions of work)

• Run a real 4-way concurrent burn-in. Document the result. Decision: keep capacity: 4 or downgrade.
• Wire the self-test workflow (Quick Win #7).
• Wire a Pester run on commit.
• Verify the act_runner service-account vs SSH-alias question (§8.2).

Phase B — Hardening (1–2 sessions)

• Fix Linux Mode 2 PAT injection.
• Add Validate-DeployState check for ci-report-ip.service.
• Document the template-refresh runbook.
• Resolve hardcoded paths in the composite action.
• Add job-duration watchdog (Quick Win #10).

Phase C — Polish (background work)

• Add Get-CIJobSummary.ps1.
• Add JobId and TemplatePath validators.
• Add Pester tests for Invoke-CIJob.ps1 (mock sub-scripts).
• Move TODO.md done-sections to docs/CHANGELOG.md.
• Delete runner/Install-Runner.ps1 and fix the Setup-Host.ps1 doc mismatch.

Phase D — Optional (if scope grows)

• Implement SHA256 pinning for toolchain installers.
• Implement multi-host federation (TODO.md §6.3).
• Implement read-only HGFS shared folders option.
• Implement artifact signing.

The system is genuinely useful today. Phase A is the difference between "works for me" and "works repeatably under load". Phase B is the difference between "works repeatably" and "I would trust it with somebody else's code".

---

15. What Would I Change

If I were starting this project from scratch today, given the same constraints (Windows 11 host, VMware Workstation, Gitea, single-operator homelab), here is what I would do differently:

15.1 IP Allocation: Static, Not DHCP

The file mutex + lease pattern is correct in shape but solves a problem you do not need to have. Configure VMnet8 with a small static-IP pool (192.168.79.100–192.168.79.107, eight slots for capacity: 8 headroom) and assign each clone a specific IP from the pool at clone time. Done via:

1. A F:\CI\State\ip-pool.json listing slots { ip: "192.168.79.100", inUse: false, jobId: null }.
2. New-BuildVM.ps1 claims a slot atomically (lock the JSON, mark inUse: true, write jobId).
3. The clone gets a bootCmd or cloud-init that assigns the static IP at boot. For Windows: netsh interface ip set address in the autounattend. For Linux: a cloud-init network-config file.
4. Wait-VMReady.ps1 polls the known IP rather than discovering it.

Benefits: no DHCP race, no getGuestIPAddress dependency, no guestVar ci-ip plumbing, no 120 s polling deadline, simpler Invoke-CIJob.ps1. The lock becomes per-slot instead of global, so all four jobs can clone+start in parallel.

Cost: one more file to maintain, template autounattend has a single static-IP placeholder. Worth it.

15.2 Drop the WinRM-vs-SSH Branch Duplication

Invoke-RemoteBuild.ps1, Get-BuildArtifacts.ps1, Wait-VMReady.ps1 all have if ($GuestOS -eq 'Linux') { ... } else { ... } branches. This is fine for two transports but does not scale and is a known maintenance pain.

A cleaner shape: a _Transport.psm1 that exports a single interface:

Invoke-GuestCommand    -Transport <SSH|WinRM> -IP -Auth -Command
Copy-GuestItem         -Transport ... -Direction -Source -Destination
Wait-GuestTransportReady -Transport ... -IP -TimeoutSeconds

with two implementations behind the same surface. Each of the three high-level scripts then becomes ~half the size and ~half the maintenance.

15.3 Centralize Configuration

F:\CI\ paths and BuildVMGuest credential target are hardcoded in many places. A single F:\CI\State\config.json read at the top of each script via a Get-CIConfig helper in _Common.psm1 would centralize this. Today changing the artifact root requires editing 6+ scripts.

15.4 Move the JSONL Log to a SQLite Sink

JSONL is fine for emitting. For querying ("show me all jobs that failed in Phase 5 last week"), SQLite is much better. Append to JSONL for forward compatibility, batch-import into F:\CI\State\history.db from Get-CIJobSummary.ps1. Single dependency.

15.5 Treat the Composite Action's Inputs as a Schema

Today inputs.guest-os accepts any string. Define a JSON schema for the inputs in action.yml and validate at action entry. Bonus: this becomes self-documenting.

15.6 Write the Integration Test First, Not Last

The four Pester unit tests are useful but they were not the test you needed. The test you needed was: "given a fresh template, run a real job, assert artifact exists, assert phase timings within bounds." Build that first before writing PowerShell. It would have caught real bugs faster than fake-vmrun.cmd tests.

15.7 Stop Using Write-Host for Logging

Write-Host is necessary for act_runner output capture per PSScriptAnalyzerSettings.psd1 exclusion. But it conflates user-facing operator output with structured log emission. Define a Write-CILog -Level Info|Warn|Error -Component "..." -Message "..." helper that also writes JSONL and console. One channel, one helper. Today every script writes Write-Host "[Component] msg" and also calls Write-JobEvent. They drift.

15.8 Use pwsh (PowerShell 7) Not PS 5.1

This is the biggest "what would I change" and the one most outside the documented constraints. The PS 5.1 mandate per AGENTS.md is a real constraint that was clearly hit (the $LASTEXITCODE && other_cmd workaround pattern is everywhere). PS 7 is a side-by-side install, does not break PS 5.1, and gives:

• && / || for shell-style chaining
• ?? / ??=
• ForEach-Object -Parallel (could parallelize Phase 1 host clones)
• Test-Json for the extra-guest-env-json validation
• Better error messages

Reasonable counter-argument: PS 5.1 is built-in on Windows 11, PS 7 is one more thing to manage. For a homelab single operator, that argument is weak.

15.9 Bake the Validation Step Into the Orchestrator

template/Validate-DeployState.ps1 and template/Validate-SetupState.ps1 exist as one-shot validation scripts but are not invoked automatically. The orchestrator should run a quick template-health check at the start of each job (cached if recent) and fail-fast if the template is wrong:

# Inside Invoke-CIJob.ps1, very early
$validationStamp = Join-Path $stateDir "template-validated-$($TemplateHash).flag"
if (-not (Test-Path $validationStamp) -or $validationStamp.LastWriteTime -lt (Get-Date).AddDays(-7)) {
    & "$scriptDir\..\template\Validate-DeployState.ps1" -TemplatePath $TemplatePath
    if ($LASTEXITCODE -ne 0) { throw "Template validation failed." }
    Set-Content $validationStamp -Value (Get-Date -Format 'o')
}

This catches the "someone manually edited the template and broke ci-report-ip.service" failure mode.

15.10 Drop the Mode 1 / Mode 2 Choice

Mode 2 (-UseGitClone) is faster per TODO.md (-25.7% vs baseline). Mode 1 exists as the default for the case where the runner has the source already (actions/checkout@v4 in the workflow). But actions/checkout in this setup just checks out on the host, then the host re-zips and ships it to the guest. That double-work is the slow path. The fast path (Mode 2) is faster because the guest has Git installed and can hit Gitea directly over the LAN.

Make Mode 2 the default. Keep Mode 1 only for the offline-template-test edge case. Reduces parameter surface, reduces code paths to maintain.

---

16. Final Verdict

This is genuinely good homelab engineering. The author understands the constraints (PS 5.1, single workstation, VMware Workstation Pro), has documented those constraints in AGENTS.md for their future self, has built a proper audit trail in TODO.md, has named scripts thoughtfully, has separated concerns into the right modules, and has handled the obvious failure modes (orphans, disk fill, runner crash). The composite action is the right abstraction. The retention policy with aggressive fallback is the right shape. The JSONL+Event Log+webhook observability layer is better than most paid CIs.

What stops me from agreeing with the README.md claim of "production-ready":

1. capacity: 4 is a promise the system has not measured itself keeping. The IP-allocation lock could be correct; or it could leak handles on a specific failure path; or DHCP could hand the same IP twice. The author admits this in TODO.md §2.1 — race non riprodotta in e2e-008/009. Either run the burn-in or set capacity: 1 until it is done.
2. The Pester suite tests argument-shape, not behavior. A real integration test takes one afternoon to write — that the system does not have one is the single biggest reason this is not actually "production-ready".
3. The composite action depends on a hardcoded N:\ path and the act_runner service account / SSH alias question. One of these is likely a runtime bug waiting to be discovered.
4. The Linux PAT-injection path is weaker than the Windows one with no documented reason. This is sloppy.
5. The deprecated runner/Install-Runner.ps1 file and the Setup-Host.ps1 ghost-password documentation are small, but they are exactly the kind of small thing that says "this codebase has not had a recent close read by a second pair of eyes." They suggest other small things have also drifted.

Honest grade:

Dimension	Grade
Architecture	A-
Code Quality	B+
Security (in scope)	B
Reliability (in scope)	B- (would be A- with §6.1 validated)
Test Coverage	C
Observability	A-
Documentation	B+
Operational Readiness	B-
Overall	B+

Production-ready for the homelab use case it was built for, with the caveats above. Not production-ready by any commercial standard, but it was never built to be, and that is fine.

If I were the author, the next 4 hours I spent on this project would be: (a) run the burn-in, (b) write the integration self-test, (c) fix the Linux PAT path, (d) delete the dead Install-Runner.ps1. Those four changes move it from B+ to A-.

---

End of review.