Simone/local-ci-cd-system
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

archive: move completed plans and sprint test plan to dated folders

Browse Source 
- plans/archived/2026-05-13/: final-master-plan (done), 4 AI analysis/review
  docs (gpt55-analysis, gpt55-review-of-opus, opus47-analysis,
  opus47-review-of-gpt55)
- docs/archived/2026-05-13/: TEST-PLAN-v1.3-to-HEAD (sprint complete)
This commit is contained in:
Simone
2026-05-13 10:47:06 +02:00
parent 1dbeec0f8e
commit 081f7e1d16
6 changed files with 0 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plans/archived/2026-05-13/opus47-analysis.md
+758
View File
@@ -0,0 +1,758 @@
# 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:
```powershell
#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`:
```powershell
$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:
```powershell
$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.**
```powershell
# 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:
```powershell
$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:
```powershell
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**.
```powershell
# 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`:
```yaml
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:
```powershell
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:
```powershell
# 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.*