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

docs(plans): add multi-agent technical review and final master plan

Browse Source 
- plans/gpt55-analysis.md: independent analysis by GPT-5.5
- plans/opus47-analysis.md: independent analysis by Opus 4.7
- plans/gpt55-review-of-opus.md: GPT-5.5 cross-review of Opus analysis
- plans/opus47-review-of-gpt55.md: Opus 4.7 cross-review of GPT-5.5 analysis
- plans/final-master-plan.md: consolidated master plan (8129 words)

Severity: 2 Critical, 12 High, 16 Medium, 10 Low, 6 Nice-to-have.
Top priorities: C2 secret logging in Invoke-RemoteBuild.ps1, C1 Linux
matrix template selection, H1 capacity=4 burn-in.
This commit is contained in:
Simone
2026-05-11 23:47:06 +02:00
parent 4758fbc519
commit ec13461862
5 changed files with 2642 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plans/final-master-plan.md
+740
View File
@@ -0,0 +1,740 @@
# Final Master Plan — Local CI/CD System
Date: 2026-05-11
Status: consolidated technical plan derived from two independent reviews ([plans/gpt55-analysis.md](gpt55-analysis.md), [plans/opus47-analysis.md](opus47-analysis.md)) and their cross-reviews ([plans/gpt55-review-of-opus.md](gpt55-review-of-opus.md), [plans/opus47-review-of-gpt55.md](opus47-review-of-gpt55.md)). Source files were re-read where the two reviewers disagreed.
This is the single authoritative document for finishing v1.0 of the system. When all Critical and High items are checked, the system is genuinely production-ready for homelab use.
---
## Executive Summary
The Local CI/CD System is a well-engineered, single-host homelab CI built on Gitea Actions, act_runner v1.0.2, VMware Workstation linked clones, Windows PowerShell 5.1, WinRM HTTPS for Windows guests and SSH for Linux guests. The architecture is the right shape for the constraint set: a thin orchestrator on the host, ephemeral linked clones for isolation, two transports for two guest families, and file-based state for everything else. Both reviewers independently graded the architecture A- and operational maturity B-/C+. That matches what the source shows.
The system is *feature-complete* and *operator-usable*, not yet *unattended-daily-trustworthy*. The gap is small in code volume but real in operational consequence. Five concrete defects can break a live build today: the Linux matrix leg of [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) silently selects the Windows template; Linux `ExtraGuestEnv` secrets are logged in clear text; the composite action outputs metadata is malformed YAML; the Linux Mode 2 PAT injection writes credentials into a remote command string; [Setup-Host.ps1](../Setup-Host.ps1) creates a directory layout that no longer matches the live system. None of these are architectural failures. Each is a 5-to-45-minute fix that the design otherwise supports cleanly.
The system makes one large unvalidated claim: `capacity: 4` in [runner/config.yaml](../runner/config.yaml). The IP-allocation mutex and lease design in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) is sound on paper, but the only evidence is single-job e2e runs. Four concurrent real builds have not been measured. This is the single most consequential outstanding question. The remediation is a burn-in, not a redesign.
Top 3 risks today, in order: (1) the Linux matrix leg builds against the Windows template until the action wiring is fixed; (2) any workflow that uses `extra-guest-env-json` for secrets is leaking those values to Gitea logs on Linux builds; (3) `capacity: 4` is a documented capability the system has never proven it keeps. Everything else is smaller.
---
## Current Project State
The optimistic [TODO.md](../TODO.md) and the conservative [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md) disagree. The honest state, reconciled against the source files, is below.
| Area | Genuinely Done | Partially Done | Unvalidated / Broken |
| -------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------ |
| Windows VM lifecycle | Clone, start, IP detect, WinRM HTTPS, build, artifacts, destroy ([scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1), [scripts/New-BuildVM.ps1](../scripts/New-BuildVM.ps1), [scripts/Wait-VMReady.ps1](../scripts/Wait-VMReady.ps1), [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1), [scripts/Get-BuildArtifacts.ps1](../scripts/Get-BuildArtifacts.ps1), [scripts/Remove-BuildVM.ps1](../scripts/Remove-BuildVM.ps1)) | — | Real 4-way concurrency |
| Linux VM lifecycle | Cloud-init deploy, SSH transport, IP via `guestinfo.ci-ip` ([template/Deploy-LinuxBuild2404.ps1](../template/Deploy-LinuxBuild2404.ps1), [scripts/_Transport.psm1](../scripts/_Transport.psm1)) | Generic validator (Windows-only) ([template/Validate-DeployState.ps1](../template/Validate-DeployState.ps1)) | Matrix workflow Linux leg picks Windows template |
| Composite action | Inputs forwarded via env vars, step outputs work, success and failure upload paths ([gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml)) | — | Action-level `outputs:` block is malformed (duplicate key, wrong nesting); hardcoded `N:\` script path |
| Concurrency | File mutex + lease design ([scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) ~L303-L460) | Stale lock auto-cleanup | 4-way burn-in never run |
| Credential handling | Windows Credential Manager target `BuildVMGuest`; PAT via Credential Manager target `GiteaPAT`; Windows Mode 2 uses `http.extraHeader` | — | Linux Mode 2 embeds PAT in URL; Linux `ExtraGuestEnv` echoed in build-command log |
| Maintenance | Disk watcher, runner health watcher, retention, orphan cleanup, scheduled tasks ([scripts/Watch-DiskSpace.ps1](../scripts/Watch-DiskSpace.ps1), [scripts/Watch-RunnerHealth.ps1](../scripts/Watch-RunnerHealth.ps1), [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1), [scripts/Cleanup-OrphanedBuildVMs.ps1](../scripts/Cleanup-OrphanedBuildVMs.ps1), [scripts/Register-CIScheduledTasks.ps1](../scripts/Register-CIScheduledTasks.ps1)) | Stale `vm-start.lock` removal missing | — |
| Backup | [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1) functional | — | Default `TemplatePath` points at obsolete `WinBuild` directory |
| Host bootstrap | Runner install, NSSM service, credential prompts ([Setup-Host.ps1](../Setup-Host.ps1)) | — | Creates obsolete `Templates\WinBuild`, missing `State`, `keys`, `Cache\pip`, current template dirs |
| Observability | JSONL per-job events, Event Log, optional webhook | Phase-duration summary | Cross-job analysis tool |
| Tests | Argument-parsing Pester suite over fake `vmrun.cmd` (4 files in [tests/](../tests/)) | — | Orchestrator state machine, real-VM smoke tests, lock concurrency |
| Documentation | [docs/ARCHITECTURE.md](../docs/ARCHITECTURE.md), [docs/CI-FLOW.md](../docs/CI-FLOW.md), [docs/RUNBOOK.md](../docs/RUNBOOK.md), [docs/BEST-PRACTICES.md](../docs/BEST-PRACTICES.md), [docs/WORKFLOW-AUTHORING.md](../docs/WORKFLOW-AUTHORING.md) | Drift: README claims production-ready; CI-FLOW claims partial-artifacts-on-failure; test plan uses PS7 syntax and wrong param names | Consolidated template-refresh runbook |
The README is genuinely ahead of the evidence. The accurate label is *feature-complete homelab CI awaiting stabilization pass*. Everything in this plan exists to close that gap.
---
## Realistic Goals
This system should aim to be:
- A reliable single-host homelab CI/CD that runs builds inside ephemeral, isolated VMware Workstation linked clones, triggered from a self-hosted Gitea.
- Operable by one person, from one workstation, against private repositories that they trust.
- Capable of clean recovery from host reboot, VMware crash, hung `vmrun`, and stale lock files without manual intervention beyond what is documented.
- Capable of producing repeatable Windows and Linux builds with cached toolchains, with artifacts collected and published to Gitea.
- Observable enough that after a failed build the operator can diagnose whether it was their build script, the runner, the host, or the VM.
This system should NOT aim to be:
- A multi-host or multi-tenant CI. Federation is explicitly out of scope.
- A platform for running untrusted third-party code. The guest VMs are hardened only enough to be useful, not enough to safely execute hostile builds.
- A general-purpose container orchestrator. Containers are deliberately not in the picture (see *Docker / Container Notes*).
- A drop-in replacement for paid CI services like GitHub Actions cloud, Azure DevOps, or CircleCI. The threat model, secret handling, and audit story are all homelab-grade.
- A PowerShell 7 project. The PS 5.1 mandate in [AGENTS.md](../AGENTS.md) is a deliberate constraint; migration is not on the roadmap.
Anything in this plan that drifts beyond these bounds is rejected. Both reviewers tested clean here; no Kubernetes, no Prometheus, no service mesh, no enterprise vault leaked into the consolidated plan.
---
## Recommended Architecture
The current architecture is the right one. Confirm it as the target shape rather than redesign it. The recommendations below are scoped, additive fixes inside the existing architecture.
**VM lifecycle approach: keep as-is.** Linked clones from a `BaseClean` snapshot, lifecycle owned by [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) in a try/finally, sub-scripts pure and parameter-driven. Add a `Invoke-VmrunBounded` wrapper into [scripts/_Common.psm1](../scripts/_Common.psm1) and use it for the three lifecycle calls (`start`, `stop`, `deleteVM`). Do not bound `list`, `readVariable`, or `getGuestIPAddress` — those are short and the wrapper overhead is wasted there.
**Transport layer organization: keep explicit Windows/Linux branches.** Do not collapse WinRM and SSH into a generic transport interface. Branches in [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) and [scripts/Get-BuildArtifacts.ps1](../scripts/Get-BuildArtifacts.ps1) are debuggable at 2 AM in a way that an abstraction is not. Add three small helpers in [scripts/_Common.psm1](../scripts/_Common.psm1) instead: `Invoke-VmrunBounded` (timeouts), `ConvertTo-SafeShellSingleQuotedString` (Linux quoting), and `Write-CIRedactedCommand` (secret-aware log formatter).
**IP allocation strategy: keep the lock+lease design.** The Opus suggestion of a static IP pool with `cloud-init`/`netsh` injection is a sound design, but it is a phase-3 redesign justified only if the burn-in fails. Run the burn-in first. The current design is: serialize clone+start+IP-detect through `F:\CI\State\vm-start.lock`, read `guestinfo.ci-ip` as the primary channel with `getGuestIPAddress` fallback, write a per-IP lease file. After fixing stale-lock cleanup and verifying under four-way load, this is sufficient for `capacity: 4`.
**Credential management: keep Windows Credential Manager + host-side SSH key file.** No vault, no rotation service. The fixes are: (1) verify the act_runner service account actually has access to the Credential Manager target; (2) confirm `F:\CI\keys\ci_linux` has ACLs restricting access to the runner identity and Administrators; (3) document a manual rotation procedure in [docs/RUNBOOK.md](../docs/RUNBOOK.md).
**Module organization: keep [scripts/_Common.psm1](../scripts/_Common.psm1) and [scripts/_Transport.psm1](../scripts/_Transport.psm1) split.** Do not rename or merge. Add the three helpers listed above to `_Common.psm1`. Keep `_Transport.psm1` as the SSH-specific helper despite the generic name; rename is churn.
**Logging/observability design: keep JSONL + Event Log + webhook.** Do not move to SQLite. Add a 40-line `Get-CIJobSummary.ps1` over the existing JSONL files for cross-job analysis. Add a phase-duration table printed at job end inside [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1).
---
## Recommended Stack
Canonical stack, after the fixes in this plan:
- Windows 11 Pro host, i9-10900X, 64 GB RAM, NVMe SSD.
- Windows PowerShell 5.1 only. No PowerShell 7 dependency in any production script.
- VMware Workstation Pro, `vmrun.exe` at `C:\Program Files (x86)\VMware\VMware Workstation\`.
- Built-in Windows 11 OpenSSH client (`ssh.exe`, `scp.exe`) for Linux guests.
- act_runner v1.0.2 installed as a Windows service via NSSM, capacity 4.
- Gitea at `http://10.10.20.11:3100` / `https://gitea.emulab.it`, PAT stored in Credential Manager target `GiteaPAT`.
- Templates: `WinBuild2025` (primary Windows), `WinBuild2022` (secondary Windows), `LinuxBuild2404` (Ubuntu 24.04 cloud image).
- WinRM HTTPS over port 5986 with Basic authentication and self-signed certificate. `AllowUnencrypted=false`.
- SSH user `ci_build` on Linux guests with passwordless sudo, key at `F:\CI\keys\ci_linux`.
Add to the stack:
- `Pester` v5 (already present in tests/).
- `PSScriptAnalyzer` (already used by the lint workflow).
Remove from the stack:
- [runner/Install-Runner.ps1](../runner/Install-Runner.ps1). Marked deprecated; superseded by [Setup-Host.ps1](../Setup-Host.ps1). Delete the file.
- The obsolete `F:\CI\Templates\WinBuild` directory if it still exists on the host. Replaced by `WinBuild2025` / `WinBuild2022`.
Do not add:
- Docker / Podman / container runtimes. See *Docker / Container Notes*.
- PowerShell 7.
- Prometheus, Grafana, Loki, OpenTelemetry collectors, or any external metrics stack.
- SQLite or any other database engine.
- HashiCorp Vault, Azure Key Vault, or any external secret store.
---
## Critical Problems
These must be fixed before the system is trustworthy for unattended daily use. Severity *Critical* means the issue actively breaks production today.
### C1. Linux matrix leg of build-nsis.yml selects the Windows template
**Description**: [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) runs a matrix over `[windows, linux]` and dispatches on `runs-on: ${{ matrix.target }}-build`, but does NOT pass `guest-os` or `template-path` to the composite action. The action defaults `template-path` to empty, so [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) reads `$env:GITEA_CI_TEMPLATE_PATH` — the Windows path. The orchestrator never reads `GITEA_CI_LINUX_TEMPLATE_PATH`. Result: the Linux leg clones `WinBuild2025` and tries to SSH into a Windows VM (or via Auto-detect, into a Windows VM with WinRM credentials), which fails or produces nonsense.
**Location**: [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) L20-L40; [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) L77-L83; [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) L108.
**Impact**: The advertised Windows+Linux matrix workflow is broken for the Linux leg. The only evidence of "Linux works" is direct invocation of `Invoke-CIJob.ps1` with explicit `-TemplatePath`. End-to-end through the workflow has never worked for Linux.
**Fix**: Either (a) update the composite action to select between `GITEA_CI_TEMPLATE_PATH` and `GITEA_CI_LINUX_TEMPLATE_PATH` based on `guest-os` (or runner label inference) and similarly for snapshot name; or (b) update [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) to pass `guest-os: ${{ matrix.target == 'linux' && 'Linux' || 'Windows' }}` plus an explicit `template-path`. Option (a) is the better one because it makes the action self-sufficient for any caller. Add an `INPUT_GUEST_OS_RESOLVED` step that selects the template-path env var, then pass it as `-TemplatePath`. Also set `BaseClean-Linux` as the snapshot name on Linux.
**Estimated effort**: 1 hour including a manual smoke test of both legs.
### C2. Linux ExtraGuestEnv secrets are logged in clear text
**Description**: [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) Linux branch builds `$buildCmd = "${envPrefix}cd '$GuestLinuxWorkDir' && $BuildCommand"` where `$envPrefix` contains `export KEY='value'; ...` with secret values inlined. The next line emits `Write-Host "[Invoke-RemoteBuild] Running build: $buildCmd"`. act_runner captures stdout and Gitea masks `secrets.*` by string match only; multi-byte transforms, base64 encoding, or substring overlap defeat the mask. The documented intended use case (`extra-guest-env-json: '{"SIGN_PASS":"${{ secrets.SIGN_PASS }}"}'`) makes this an active leak path.
**Location**: [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) Linux build assembly, around the `Write-Host` immediately preceding `Invoke-SshCommand` for the build step.
**Impact**: Any homelab workflow currently using `extra-guest-env-json` for secrets has those secrets in Gitea job logs and in the local `F:\CI\Logs\<jobId>\invoke-ci.log` transcript.
**Fix**: Replace the `Write-Host "Running build: $buildCmd"` line with `Write-Host "[Invoke-RemoteBuild] Running build (env vars redacted): cd '$GuestLinuxWorkDir' && <build-command>"`. Never log `$envPrefix` or any string containing it. Do the same redaction in any other location where `$buildCmd` or `$envPrefix` is logged.
**Estimated effort**: 15 minutes.
---
## Secondary Problems
These are *High* priority items. Fix in the same stabilization pass as the Criticals.
### H1. `capacity: 4` is unvalidated under real concurrency
**Description**: [runner/config.yaml](../runner/config.yaml) sets `capacity: 4`, but the IP-allocation mutex in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) has only been exercised by single-job e2e runs (per [TODO.md](../TODO.md) §2.1). The design is plausibly correct, but DHCP behavior under four concurrent `vmrun start` calls, native-tool concurrency safety, and the 4-minute serialization of the IP-acquire phase are unmeasured. Both reviewers flagged this; severity is HIGH (not CRITICAL) because the system can be safely operated at lower capacity while this is unproven.
**Location**: [runner/config.yaml](../runner/config.yaml) L17; [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) L303-L460.
**Impact**: The "production-ready" claim in [README.md](../README.md) rests on a capacity number that has not been measured. A first real burst of four parallel builds may fail in ways the e2e runs cannot predict.
**Fix**: Write `scripts/Test-CapacityBurnIn.ps1`. PowerShell 5.1, uses `Start-Job` to launch four parallel `Invoke-CIJob.ps1` invocations with unique JobIds against a small no-op build. Wait for all jobs, then assert: (1) all four VMs were destroyed; (2) all four jobs got distinct IPs; (3) `F:\CI\State\vm-start.lock` does not exist; (4) `F:\CI\State\ip-leases\` is empty; (5) total wall time and per-phase JSONL durations are recorded. Run three times. If all three pass, document the result in [docs/RUNBOOK.md](../docs/RUNBOOK.md) and keep `capacity: 4`. If any fail, set `capacity: 2` until investigated.
**Estimated effort**: Half-day to write, run, and document.
### H2. Composite action `outputs:` block is malformed YAML
**Description**: [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) has `artifact-path` and a duplicate `artifact-name` under the `inputs:` map instead of a top-level `outputs:` block. Step-level outputs still work (the `Out-File $env:GITHUB_OUTPUT` lines drive the upload step inside the same composite), so the live workflow does upload artifacts. But the duplicate `artifact-name` key violates YAML 1.2 (most parsers silently overwrite with the later value) and the missing `outputs:` block means downstream workflows cannot read `steps.local-ci-build.outputs.artifact-path`.
**Location**: [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) L96 and L132-L141.
**Impact**: Latent. Today's single consumer is unaffected. The first new consumer that tries to chain on the artifact path will fail mysteriously.
**Fix**: Insert a top-level `outputs:` mapping (sibling to `inputs:` and `runs:`) containing `artifact-path` and `artifact-name`. Delete the second `artifact-name` and the `artifact-path` from the `inputs:` map. Keep the inner `Out-File $env:GITHUB_OUTPUT` lines unchanged. Note: GPT-5.5 originally rated this CRITICAL; Opus rated it LOW. The final-plan severity is HIGH because impact is latent but the YAML is clearly malformed.
**Estimated effort**: 20 minutes.
### H3. Linux Mode 2 PAT injected into clone URL
**Description**: [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) Linux Mode 2 builds `https://user:pat@host/repo.git` and exports it as `GIT_CLONE_URL` in the SSH command. The PAT becomes visible in `/proc/<pid>/environ` for the duration of `git clone`, in the host-side `ssh.exe` argv, and in any PowerShell error text that captures `$envCloneCmd`. The Windows path uses `git -c http.extraHeader=Authorization: Basic <b64>`, which is strictly safer.
**Location**: [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) Linux Mode 2 clone branch, around the `GIT_CLONE_URL` export.
**Impact**: PAT visibility on the guest filesystem and host process list. Bounded but real.
**Fix**: Mirror the Windows pattern. Base64-encode `user:pat`, build a single `git -c credential.helper= -c http.extraHeader=Authorization: Basic <b64> clone <non-auth-url> <dest>` command, invoke via `Invoke-SshCommand`. Drop the URL rewrite, drop the `GIT_CLONE_URL` export, drop the `unset`. Note: GPT-5.5 originally proposed an askpass helper with `chmod 600` temp file; Opus proposed the `http.extraHeader` mirror. The simpler Opus pattern is adopted.
**Estimated effort**: 30 minutes including a clone test against a private repo.
### H4. `vmrun` lifecycle calls have no timeout in production scripts
**Description**: [scripts/_Common.psm1](../scripts/_Common.psm1)'s `Invoke-Vmrun` checks `$LASTEXITCODE` but does not bound execution. [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) and [scripts/Remove-BuildVM.ps1](../scripts/Remove-BuildVM.ps1) inherit this. A hung `vmrun start` or `deleteVM` waits on `vmware-vmx.exe` indefinitely, consuming the runner slot until act_runner's 2-hour job timeout fires. The `finally` cleanup cannot run while the native process is blocked.
**Location**: [scripts/_Common.psm1](../scripts/_Common.psm1) `Invoke-Vmrun`; callers in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) and [scripts/Remove-BuildVM.ps1](../scripts/Remove-BuildVM.ps1).
**Impact**: A single stuck VMware operation can pin one of the four runner slots for two hours.
**Fix**: Add `Invoke-VmrunBounded` to [scripts/_Common.psm1](../scripts/_Common.psm1). Use `Start-Process -PassThru` + `Wait-Process -Timeout` + `Stop-Process` on timeout. Return `[pscustomobject]@{ExitCode; Output; TimedOut; ElapsedSeconds}`. Replace `Invoke-Vmrun` calls for `start`, `stop`, and `deleteVM` only — keep the unbounded version for `list`, `readVariable`, `getGuestIPAddress`. Recommended timeouts: 180s for start, 60s for stop, 90s for deleteVM.
**Estimated effort**: 1 hour including a Pester test.
### H5. Stale `vm-start.lock` not cleaned automatically
**Description**: The IP-allocation lock file at `F:\CI\State\vm-start.lock` is released correctly on normal exit. On host crash mid-lock, the file remains. On reboot the OS-level lock is gone (kernel released it), but the file's mere existence does not cause failure; the issue is the 10-minute retry loop and operator confusion. Neither [scripts/Cleanup-OrphanedBuildVMs.ps1](../scripts/Cleanup-OrphanedBuildVMs.ps1) nor [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1) removes it.
**Location**: [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1) and [scripts/Cleanup-OrphanedBuildVMs.ps1](../scripts/Cleanup-OrphanedBuildVMs.ps1).
**Impact**: First job after a crash gives a confusing message; manual `Remove-Item` cures it. Low operational severity, but high friction.
**Fix**: In both maintenance scripts, add a block that removes `F:\CI\State\vm-start.lock` if its `LastWriteTime` is older than 30 minutes. Do not write metadata into the lock file; the file's mere existence is sufficient signal. Log to console and JSONL.
**Estimated effort**: 15 minutes.
### H6. Setup-Host directory list is stale
**Description**: [Setup-Host.ps1](../Setup-Host.ps1) L118-L126 creates `Templates\WinBuild` and a handful of cache directories, but does NOT create `Templates\WinBuild2025`, `Templates\WinBuild2022`, `Templates\LinuxBuild2404`, `State`, `State\ip-leases`, `keys`, or `Cache\pip`. The orchestrator depends on `F:\CI\State\` and `F:\CI\keys\`. A fresh host bootstrap leaves the system non-functional.
**Location**: [Setup-Host.ps1](../Setup-Host.ps1) L118-L126.
**Impact**: Disaster recovery is broken. Rebuilding the host from scratch using the documented procedure does not produce a working system.
**Fix**: Update the directory array to: `BuildVMs`, `Artifacts`, `Logs`, `Templates\WinBuild2025`, `Templates\WinBuild2022`, `Templates\LinuxBuild2404`, `State`, `State\ip-leases`, `keys`, `act_runner\logs`, `Cache\NuGet`, `Cache\pip`, `RunnerWork`, `ISO`. Remove `Templates\WinBuild`. Add a comment block referencing [AGENTS.md](../AGENTS.md) §Struttura directory host.
**Estimated effort**: 10 minutes.
### H7. Backup-CITemplate default path is wrong
**Description**: [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1) L48 defaults `TemplatePath` to `'F:\CI\Templates\WinBuild'`. That directory no longer exists. The default-argument backup silently fails or backs up an empty directory, then the operator refreshes the actual `WinBuild2025` template, with no rollback.
**Location**: [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1) L48 and the help block.
**Impact**: An operator running `.\Backup-CITemplate.ps1` before a template refresh believes they have a backup. They do not.
**Fix**: Change the default to `'F:\CI\Templates\WinBuild2025'`. Add a `-AllTemplates` switch that iterates `F:\CI\Templates\*` and backs up each (with subfolder naming preserved in the backup destination). Update [docs/HOST-SETUP.md](../docs/HOST-SETUP.md) and [docs/RUNBOOK.md](../docs/RUNBOOK.md) to reflect the new default. Update the help block.
**Estimated effort**: 30 minutes.
### H8. act_runner service account vs SSH alias mismatch
**Description**: [Setup-Host.ps1](../Setup-Host.ps1) L294-L306 writes the `gitea-ci` SSH alias to `$env:USERPROFILE\.ssh\config` — the interactive user's profile. The NSSM install at L264-L273 does not set an explicit service account, which on Windows commonly means LocalSystem. LocalSystem's home is `C:\Windows\System32\config\systemprofile\`, not the user profile. [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) uses `repo-url: 'ssh://gitea-ci/Simone/...'`. If the service runs as LocalSystem, the alias resolution fails and every Mode 1 (host-side clone) attempt errors out.
**Location**: [Setup-Host.ps1](../Setup-Host.ps1) L264-L273 (NSSM install), L294-L306 (SSH config write); [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) L35.
**Impact**: Potentially blocks all Mode 1 host-side clone builds. Severity is HIGH-PENDING-VERIFICATION; first action is verification.
**Fix**: Step 1, verify: run `Get-CimInstance Win32_Service -Filter "Name='act_runner'" | Select-Object Name,StartName` on the host and document the answer in [docs/RUNBOOK.md](../docs/RUNBOOK.md). Step 2a, if `StartName` is `LocalSystem`: copy the SSH config and known-hosts to `C:\Windows\System32\config\systemprofile\.ssh\` with proper ACLs. Step 2b alternative: create a dedicated `ci_runner` Windows user, reinstall NSSM with `obj=.\ci_runner`, store the SSH config under that user's profile. Step 3: add the verification check to [template/Validate-DeployState.ps1](../template/Validate-DeployState.ps1) or a new `Validate-HostState.ps1`. Note: GPT-5.5 did not catch this. Opus did. Severity HIGH stands.
**Estimated effort**: 1 hour for verification + fix.
### H9. `ci-report-ip.service` not asserted in deploy-state validator
**Description**: [template/Validate-DeployState.ps1](../template/Validate-DeployState.ps1) is Windows-only: it checks WinRM, Defender, Firewall, UAC, etc. It does not check whether the Linux template's `ci-report-ip.service` (the systemd unit writing `guestinfo.ci-ip`) is enabled. The unit IS validated inside [template/Install-CIToolchain-Linux2404.sh](../template/Install-CIToolchain-Linux2404.sh) L203-L221 and [template/Prepare-LinuxBuild2404.ps1](../template/Prepare-LinuxBuild2404.ps1) L361-L362, but not at the level of "is this template ready to be promoted to BaseClean-Linux." If a future refresh forgets to re-enable it, every Linux build silently falls back to the 120-second `getGuestIPAddress` timeout.
**Location**: [template/Validate-DeployState.ps1](../template/Validate-DeployState.ps1).
**Impact**: Silent degradation of Linux build startup time after a faulty template refresh.
**Fix**: Add a Linux branch to `Validate-DeployState.ps1` (or create a `Validate-LinuxDeployState.ps1`) that opens an SSH session and runs `systemctl is-enabled ci-report-ip.service` and `test -x /usr/local/bin/ci-report-ip.sh`. Fail with a clear error if either is missing.
**Estimated effort**: 45 minutes.
### H10. No automated end-to-end smoke test
**Description**: The Pester suite in [tests/](../tests/) exercises argument parsing only via a fake `vmrun.cmd`. The "production-ready" claim rests on manual e2e runs (`e2e-008`, `e2e-009`, `§3.3`). There is no scripted, repeatable test that exercises real VM lifecycle and exits non-zero on failure. Both reviewers flagged this; severity is HIGH because regression risk without it is high after every change.
**Location**: [tests/](../tests/), [gitea/workflows/](../gitea/workflows/).
**Impact**: Any change to orchestration scripts has no automated guard. Regressions are caught only by chance during the next manual run.
**Fix**: Create `gitea/workflows/self-test.yml` (workflow_dispatch trigger only). One job that calls the composite action against a minimal `Write-Host 'ok' > out.txt` build, asserts artifact upload, JSONL phase events, and clean teardown. Have it succeed-or-fail for Windows and a second job for Linux. Add `scripts/Test-Smoke.ps1` that the workflow invokes; the script also runs locally. Note: this replaces the much larger `Measure-CIBenchmark.ps1` wiring suggestion; the smoke test is sufficient and cheaper.
**Estimated effort**: Half-day.
### H11. Composite action hardcodes the script root path
**Description**: [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) L148 sets `$ciScriptsDir = 'N:\Code\Workspace\Local-CI-CD-System\scripts'`. The action is presented as reusable, but moving the repo breaks it. Opus caught this; GPT-5.5 missed it (it caught path drift in setup/backup only).
**Location**: [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) L148.
**Impact**: Action is not actually reusable. Any second consumer or repo relocation breaks builds.
**Fix**: Replace with a runner env var. Add `GITEA_CI_SCRIPT_ROOT: "N:\\Code\\Workspace\\Local-CI-CD-System\\scripts"` to [runner/config.yaml](../runner/config.yaml) `envs:`. In the action, use `$ciScriptsDir = $env:GITEA_CI_SCRIPT_ROOT`. Do not use `${{ github.action_path }}` — the action repo is not necessarily cloned into the consumer's workspace in this Gitea setup.
**Estimated effort**: 20 minutes.
### H12. Measure-CIBenchmark uses a different IP discovery path than production
**Description**: [scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1) L129-L146 polls `Invoke-Vmrun -Operation getGuestIPAddress`. [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) L348-L389 primarily reads `guestVar 'ci-ip'` and uses `getGuestIPAddress` as fallback. The benchmark therefore measures a different code path than what production runs, and its phase timings cannot be used for capacity planning.
**Location**: [scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1) L129-L146.
**Impact**: Benchmark data is misleading. Decisions to keep or change `capacity: 4` based on it would be based on wrong measurements.
**Fix**: Extract IP-acquire into a function `Get-GuestIPAddress` in [scripts/_Common.psm1](../scripts/_Common.psm1) that takes the VMX path and credentials and implements the `guestinfo.ci-ip`-then-`getGuestIPAddress` flow used in production. Call it from both `Invoke-CIJob.ps1` and `Measure-CIBenchmark.ps1`.
**Estimated effort**: 1 hour.
---
## Complete Priority Checklist
Every item is concrete, references a file, and ends with an effort estimate.
### Critical [ ]
- [ ] Fix Linux matrix template selection — [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) + [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) — action selects `GITEA_CI_TEMPLATE_PATH` or `GITEA_CI_LINUX_TEMPLATE_PATH` based on `guest-os` input or runner label, also resolves the correct `snapshot-name` (`BaseClean` or `BaseClean-Linux`) — 1h.
- [ ] Redact Linux ExtraGuestEnv secret logging — [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) Linux branch — replace `Write-Host "...$buildCmd"` with redacted version; never log `$envPrefix` — 15min.
### High [ ]
- [ ] Run 4-way concurrency burn-in — `scripts/Test-CapacityBurnIn.ps1` (new) — `Start-Job`-based harness, three runs, assertions on IPs and lock cleanup — half-day.
- [ ] Fix composite action outputs YAML — [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) — add top-level `outputs:` block; remove duplicate `artifact-name` from `inputs:` — 20min.
- [ ] Replace Linux Mode 2 PAT URL with `http.extraHeader` — [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) Linux clone branch — base64-encode `user:pat`, single `git -c http.extraHeader=...` command — 30min.
- [ ] Add `Invoke-VmrunBounded` and use for lifecycle calls — [scripts/_Common.psm1](../scripts/_Common.psm1), [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1), [scripts/Remove-BuildVM.ps1](../scripts/Remove-BuildVM.ps1) — bound `start`, `stop`, `deleteVM` only — 1h.
- [ ] Add stale `vm-start.lock` cleanup — [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1) and [scripts/Cleanup-OrphanedBuildVMs.ps1](../scripts/Cleanup-OrphanedBuildVMs.ps1) — remove file older than 30 minutes — 15min.
- [ ] Fix Setup-Host directory list — [Setup-Host.ps1](../Setup-Host.ps1) L118 — match [AGENTS.md](../AGENTS.md) §Struttura — 10min.
- [ ] Fix Backup-CITemplate default — [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1) L48 + help block — default `WinBuild2025`, add `-AllTemplates` — 30min.
- [ ] Verify and fix act_runner service-account SSH alias — [Setup-Host.ps1](../Setup-Host.ps1) L264-L306 — check `StartName`, relocate `.ssh/config` or change service account — 1h.
- [ ] Add `ci-report-ip.service` check to deploy-state validator — [template/Validate-DeployState.ps1](../template/Validate-DeployState.ps1) — Linux branch via SSH — 45min.
- [ ] Add on-demand smoke-test workflow — new `gitea/workflows/self-test.yml` + `scripts/Test-Smoke.ps1` — minimal Windows and Linux jobs through composite action — half-day.
- [ ] Replace hardcoded `N:\` script path in composite action — [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) L148 + [runner/config.yaml](../runner/config.yaml) — new `GITEA_CI_SCRIPT_ROOT` env var — 20min.
- [ ] Align benchmark IP discovery with production — [scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1) + [scripts/_Common.psm1](../scripts/_Common.psm1) — extract shared `Get-GuestIPAddress` helper — 1h.
### Medium [ ]
- [ ] Add pre-clone disk space gate — [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) Phase 2 — `throw` if free space on `F:` below 20 GB — 5min.
- [ ] Add post-failure guest diagnostics collection — [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) `catch` block — best-effort `Get-GuestDiagnostics` to `F:\CI\Artifacts\<jobId>\diagnostics\` before teardown — 1h.
- [ ] Wire `UseSharedCache` through composite action and orchestrator — [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml), [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1), [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) — new input, parameter pass-through — 1h.
- [ ] Authenticate-not-just-TCP WinRM readiness — [scripts/Wait-VMReady.ps1](../scripts/Wait-VMReady.ps1) — optional Credential parameter, single `Invoke-Command { 'ready' }` after TCP open — 30min.
- [ ] Add Ubuntu cloud VMDK SHA256 verification — [template/Deploy-LinuxBuild2404.ps1](../template/Deploy-LinuxBuild2404.ps1) — SHA256 parameter, fail on mismatch — 30min.
- [ ] Fix `Validate-SetupState` `PasswordExpires` compatibility — [template/Validate-SetupState.ps1](../template/Validate-SetupState.ps1) — mirror logic from `Install-CIToolchain-WinBuild2025.ps1` — 15min.
- [ ] Update [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md) — replace `ForEach-Object -Parallel` with `Start-Job`, replace `-RepositoryUrl` with `-RepoUrl` — 20min.
- [ ] Add `JobId` sanitization — [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) and [scripts/New-BuildVM.ps1](../scripts/New-BuildVM.ps1) — `[ValidatePattern('^[A-Za-z0-9._-]+$')]` — 5min.
- [ ] Add `ExtraGuestEnv` key validation — [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) — reject keys not matching `^[A-Za-z_][A-Za-z0-9_]*$` — 15min.
- [ ] Add phase-duration summary at job end — [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) — table printed from in-memory timestamps — 30min.
- [ ] Add 90-minute job-duration webhook warning — [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) — background `Start-Job` posts once — 30min.
- [ ] Document HGFS shared folder write semantics — [docs/BEST-PRACTICES.md](../docs/BEST-PRACTICES.md) — explicit note on cache-poisoning risk — 15min.
- [ ] Standardize Event Log source names — [scripts/Watch-DiskSpace.ps1](../scripts/Watch-DiskSpace.ps1), [scripts/Watch-RunnerHealth.ps1](../scripts/Watch-RunnerHealth.ps1), [scripts/Register-CIScheduledTasks.ps1](../scripts/Register-CIScheduledTasks.ps1), [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md) — pick `CI-DiskSpaceAlert`, `CI-RunnerHealth`, propagate — 30min.
- [ ] Tighten SSH host-key handling for CI jobs — [scripts/_Transport.psm1](../scripts/_Transport.psm1) — `StrictHostKeyChecking=accept-new`, `UserKnownHostsFile=F:\CI\State\known_hosts` for CI-job calls; leave permissive mode for template provisioning — 1h.
- [ ] Consolidate template-refresh runbook — [docs/RUNBOOK.md](../docs/RUNBOOK.md) — single section: backup → refresh → validate → smoke → snapshot → update `runner/config.yaml` → keep previous snapshot 7 days → rollback procedure — 2h.
### Low [ ]
- [ ] Delete deprecated runner installer — [runner/Install-Runner.ps1](../runner/Install-Runner.ps1) — remove the file — 5min.
- [ ] Align Setup-Host password documentation with behavior — [Setup-Host.ps1](../Setup-Host.ps1) help block — describe prompt fallback, remove ghost default `'CIBuild!ChangeMe2026'` — 5min.
- [ ] Remove emoji from webhook payloads — [scripts/Watch-DiskSpace.ps1](../scripts/Watch-DiskSpace.ps1), [scripts/Watch-RunnerHealth.ps1](../scripts/Watch-RunnerHealth.ps1) — `[WARN]`, `[ERROR]` prefixes — 5min.
- [ ] Fix Validate-* docblocks to reflect HTTPS/5986 — [template/Validate-DeployState.ps1](../template/Validate-DeployState.ps1), [template/Validate-SetupState.ps1](../template/Validate-SetupState.ps1) — update HTTP/Basic to HTTPS/Basic — 10min.
- [ ] Align benchmark JSON schema with test plan — [scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1) or [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md) — pick one and update the other — 30min.
- [ ] Add `TemplatePath` sanity check — [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) — warn if not under `F:\CI\Templates\` — 10min.
- [ ] Add a maintenance flag for Watch-RunnerHealth — [scripts/Watch-RunnerHealth.ps1](../scripts/Watch-RunnerHealth.ps1) — skip restart if `F:\CI\State\runner-maintenance.flag` exists — 10min.
- [ ] Add ACL validation script — new `scripts/Validate-HostState.ps1` — assert ACLs on `F:\CI\keys\ci_linux` and Credential Manager target accessible — 30min.
- [ ] Move done sections of [TODO.md](../TODO.md) into a new `docs/CHANGELOG.md` — 1h.
- [ ] Fix [docs/CI-FLOW.md](../docs/CI-FLOW.md) failure-scenario table — change "partial artifacts collected" to "artifacts collected only after successful build packaging" or implement H-level diagnostics; if H-level diagnostics added, leave docs as-is — 15min.
### Nice to have [ ]
- [ ] Add `scripts/Get-CIJobSummary.ps1` — reads last N days of JSONL files, prints duration table — 1h.
- [ ] Add artifact manifest with file hashes — [scripts/Get-BuildArtifacts.ps1](../scripts/Get-BuildArtifacts.ps1) — `manifest.json` with name, size, SHA256, commit, job ID — 1h.
- [ ] Add per-job VMX CPU/RAM override — [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) + composite action input — post-clone VMX edit, default off — 1h.
- [ ] Add optional Gitea API runner-online check — [scripts/Watch-RunnerHealth.ps1](../scripts/Watch-RunnerHealth.ps1) — optional `-GiteaUrl` and PAT target — 1h.
- [ ] Begin SHA256 pinning of toolchain installers — [template/Install-CIToolchain-WinBuild2025.ps1](../template/Install-CIToolchain-WinBuild2025.ps1) — fill hashes for Git, 7-Zip, Python, .NET, Node — 2h.
- [ ] Add linked-clone delta size measurement — [scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1) — record per-job delta growth — 30min.
---
## Detailed Operational Checklist
Execute in this order. Each task lists a prerequisite, an action, and a verification step.
### Sprint 1: Validation
Goal: prove or downgrade unvalidated claims before any code changes go in.
**Task 1.1 — Verify act_runner service account**
- Prerequisite: host PowerShell session as Administrator.
- Action: `Get-CimInstance Win32_Service -Filter "Name='act_runner'" | Select-Object Name,StartName,State`.
- Verification: record `StartName` in [docs/RUNBOOK.md](../docs/RUNBOOK.md) under "Runner service identity". If `LocalSystem`, schedule H8 fix immediately.
**Task 1.2 — Inventory current state-file presence**
- Prerequisite: nothing.
- Action: `Test-Path F:\CI\State, F:\CI\State\ip-leases, F:\CI\keys, F:\CI\Cache\pip, F:\CI\Templates\WinBuild2025, F:\CI\Templates\LinuxBuild2404`.
- Verification: each must return `True`. If any are missing, create manually for now and defer the [Setup-Host.ps1](../Setup-Host.ps1) fix to Sprint 2.
**Task 1.3 — Run 4-way concurrency burn-in (H1)**
- Prerequisite: write `scripts/Test-CapacityBurnIn.ps1` first; `WinBuild2025` and `LinuxBuild2404` templates powered off; ≥ 100 GB free on `F:`.
- Action: invoke the burn-in three times against a no-op build (e.g. `Write-Host 'ok'`); record per-run JSONL outputs.
- Verification: zero IP collisions across runs, zero leaked VMs, zero leaked lock files, all jobs succeeded. If any fail, set `runner/config.yaml capacity: 2` and create a follow-up investigation task.
**Task 1.4 — Manually invoke Linux matrix leg to confirm C1 bug**
- Prerequisite: a test branch with a tag that triggers [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml).
- Action: push a tag, observe both legs.
- Verification: confirm Linux leg fails or builds against Windows template. Document the failure mode in the C1 task notes.
### Sprint 2: Critical Fixes
Goal: clear both Criticals plus the highest-impact Highs that touch production paths.
**Task 2.1 — Fix Linux matrix template selection (C1)**
- Prerequisite: Sprint 1 task 1.4 done.
- Action: edit [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) so the PowerShell step resolves `INPUT_GUEST_OS_RESOLVED` from `INPUT_GUEST_OS` (with `Auto` honoring `runs-on` heuristic via `$env:RUNNER_LABELS` if Gitea exposes it; otherwise defaulting Auto to Windows) and selects between `GITEA_CI_TEMPLATE_PATH` and `GITEA_CI_LINUX_TEMPLATE_PATH`. Update [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) matrix to pass `guest-os: ${{ matrix.target == 'linux' && 'Linux' || 'Windows' }}` as a belt-and-suspenders measure.
- Verification: re-run the matrix workflow. Both legs succeed against the correct templates.
**Task 2.2 — Redact Linux ExtraGuestEnv logging (C2)**
- Prerequisite: nothing.
- Action: in [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) Linux branch, replace the build-command echo with the redacted form. Grep the entire file for any other place `$envPrefix` or `$buildCmd` is logged; fix all.
- Verification: run a Linux job with a fake secret `extra-guest-env-json='{"DUMMY":"shibboleth"}'` and confirm the string `shibboleth` does not appear in `F:\CI\Logs\<jobId>\invoke-ci.log` or in act_runner output.
**Task 2.3 — Fix composite action outputs YAML (H2)**
- Prerequisite: nothing.
- Action: edit [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) per H2 fix.
- Verification: run `gitea/workflows/build-nsis.yml`; confirm artifact upload still works. Optionally test downstream consumption by writing a temporary workflow step that reads `steps.local-ci-build.outputs.artifact-path`.
**Task 2.4 — Linux Mode 2 PAT to http.extraHeader (H3)**
- Prerequisite: a private Gitea repo with the PAT scope to clone.
- Action: edit [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) Linux clone branch.
- Verification: run a Linux Mode 2 build; confirm `git clone` succeeds; verify with `ps -ef` on the guest during clone that the PAT is not in the argv.
**Task 2.5 — Fix Setup-Host directory list (H6)**
- Prerequisite: Sprint 1 task 1.2 captured what is missing.
- Action: edit [Setup-Host.ps1](../Setup-Host.ps1) L118 directory array.
- Verification: on a test VM (not the production host), run [Setup-Host.ps1](../Setup-Host.ps1) and confirm every directory in [AGENTS.md](../AGENTS.md) §Struttura is created.
**Task 2.6 — Fix Backup-CITemplate default (H7)**
- Prerequisite: nothing.
- Action: edit [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1) default and help; add `-AllTemplates` switch.
- Verification: run `.\Backup-CITemplate.ps1 -DryRun` (add flag if it does not exist) and confirm the resolved source path is `F:\CI\Templates\WinBuild2025`.
### Sprint 3: High Priority Fixes
Goal: stabilize the rest of the High-tier items.
**Task 3.1 — Add `Invoke-VmrunBounded` (H4)**
- Prerequisite: review existing bounded patterns in deploy scripts.
- Action: add the function to [scripts/_Common.psm1](../scripts/_Common.psm1); wire `start`/`stop`/`deleteVM` callers in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) and [scripts/Remove-BuildVM.ps1](../scripts/Remove-BuildVM.ps1).
- Verification: write a Pester test that injects a fake `vmrun.cmd` which sleeps 600s; assert the wrapper times out at the configured deadline.
**Task 3.2 — Stale lock cleanup (H5)**
- Prerequisite: nothing.
- Action: edit [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1) and [scripts/Cleanup-OrphanedBuildVMs.ps1](../scripts/Cleanup-OrphanedBuildVMs.ps1).
- Verification: `touch F:\CI\State\vm-start.lock` with a 1-hour-old timestamp, run the retention script, confirm it is removed and the removal is logged.
**Task 3.3 — Fix or relocate SSH alias for service account (H8)**
- Prerequisite: Sprint 1 task 1.1 result.
- Action: per H8, either copy `.ssh/` to systemprofile or change the service account.
- Verification: `psexec -s -i ssh -T gitea-ci` (or equivalent) from the service account; expect a successful Gitea response.
**Task 3.4 — Linux deploy validator check for ci-report-ip (H9)**
- Prerequisite: SSH key and IP of the LinuxBuild2404 template (boot it, capture, shut down).
- Action: add the Linux branch to [template/Validate-DeployState.ps1](../template/Validate-DeployState.ps1).
- Verification: against the live template, the script reports the service as enabled.
**Task 3.5 — Add smoke-test workflow (H10)**
- Prerequisite: H2 and C1 fixed (otherwise the workflow's own assertions fail).
- Action: create `gitea/workflows/self-test.yml` and `scripts/Test-Smoke.ps1`.
- Verification: trigger via `workflow_dispatch` and observe both legs green.
**Task 3.6 — Replace hardcoded N:\ path (H11)**
- Prerequisite: nothing.
- Action: add `GITEA_CI_SCRIPT_ROOT` env var in [runner/config.yaml](../runner/config.yaml); reference it in [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml).
- Verification: restart act_runner; re-run a workflow; confirm it still resolves the script path.
**Task 3.7 — Benchmark IP path alignment (H12)**
- Prerequisite: H4 done (shared `_Common.psm1` patterns ironed out).
- Action: extract `Get-GuestIPAddress` into [scripts/_Common.psm1](../scripts/_Common.psm1); update both callers.
- Verification: run `Measure-CIBenchmark.ps1 -Iterations 1` for Windows and Linux; confirm phase timings match those recorded in production JSONL within ±10%.
### Sprint 4: Operational Hardening
Goal: harden recovery, secret handling, and recoverability.
**Task 4.1 — Pre-clone disk gate, JobId/ExtraGuestEnv validation, redacted command logger** (Medium items grouped) — edit [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) and [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1); verify with deliberately bad inputs.
**Task 4.2 — Failure-path diagnostics** — add a `Get-GuestDiagnostics` helper invoked from the `catch` block of [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1). Skip on Phase 1/2 failures (no VM yet). Best-effort SSH/WinRM, no throw on failure.
**Task 4.3 — Wire `UseSharedCache`** — surface in composite action and orchestrator; confirm with a Python build that pip cache is reused.
**Task 4.4 — Tighten SSH host keys for CI jobs** — `accept-new` + `F:\CI\State\known_hosts` for CI; keep permissive mode for template provisioning (host keys are unknown there).
**Task 4.5 — Ubuntu VMDK SHA256 verification** — add to [template/Deploy-LinuxBuild2404.ps1](../template/Deploy-LinuxBuild2404.ps1).
**Task 4.6 — Consolidated template-refresh runbook** — edit [docs/RUNBOOK.md](../docs/RUNBOOK.md). One section covering: stop runner, back up template (`-AllTemplates`), boot template, KMS reactivation via `slmgr /ato` (Windows), run `Prepare-WinBuild2025.ps1` or `Prepare-LinuxBuild2404.ps1`, shutdown, snapshot `BaseClean_yyyyMMdd`, run `Validate-DeployState.ps1` (Windows and Linux branches), run smoke workflow, then update `GITEA_CI_SNAPSHOT_NAME` in [runner/config.yaml](../runner/config.yaml), keep prior snapshot 7 days, rollback procedure.
### Sprint 5: Quality and Optimization
Goal: polish.
**Task 5.1 — Delete dead code** — remove [runner/Install-Runner.ps1](../runner/Install-Runner.ps1); align Setup-Host help text.
**Task 5.2 — Webhook emoji removal, Event Log naming, docblock corrections** — Low-tier batch.
**Task 5.3 — Update test plan** — [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md) — PS5.1 syntax, correct parameter names, current task and event log names.
**Task 5.4 — Add `Get-CIJobSummary.ps1`** — nice-to-have summary tool.
**Task 5.5 — Phase-duration summary in orchestrator** — added at end of [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) before final cleanup.
**Task 5.6 — Begin SHA256 pinning** — start with Git, 7-Zip, Python, .NET. Note in [docs/BEST-PRACTICES.md](../docs/BEST-PRACTICES.md) that this is a homelab-acceptable, partial-coverage pin.
---
## Recommended Refactors
Only refactors that genuinely simplify or fix something. No churn.
1. **Extract `Get-GuestIPAddress` into [scripts/_Common.psm1](../scripts/_Common.psm1)**. Removes the dual-implementation drift between `Invoke-CIJob.ps1` and `Measure-CIBenchmark.ps1`. Single source of truth for the `guestinfo.ci-ip`-then-fallback flow. Justification: real bug avoidance, not aesthetic.
2. **Extract `Invoke-VmrunBounded` into [scripts/_Common.psm1](../scripts/_Common.psm1)**. Closes the H4 timeout gap and consolidates the pattern that already exists in some deploy scripts.
3. **Add `Write-CIRedactedCommand` and `ConvertTo-SafeShellSingleQuotedString` to [scripts/_Common.psm1](../scripts/_Common.psm1)**. The first standardizes secret-aware command logging across Windows and Linux paths. The second consolidates the `-replace "'", "'\''"` idiom used in [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1). Both are 5-line functions.
4. **DO NOT extract a generic transport interface**. Opus suggested `Invoke-GuestCommand`/`Copy-GuestItem`/`Wait-GuestTransportReady`. Both reviewers ultimately agreed this is the wrong move for a two-transport homelab. Keep explicit Windows/Linux branches in [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) and [scripts/Get-BuildArtifacts.ps1](../scripts/Get-BuildArtifacts.ps1).
5. **DO NOT introduce `F:\CI\State\config.json` as a central config**. [AGENTS.md](../AGENTS.md) and [runner/config.yaml](../runner/config.yaml) are already the two sources of truth. Adding a third creates drift, not reduces it. The remediation is to align the existing two, not invent a third.
6. **DO NOT migrate to PowerShell 7**. The PS 5.1 constraint is deliberate, encoded in [AGENTS.md](../AGENTS.md), and consistently respected across the codebase. A migration is a multi-day churn that buys little.
7. **DO NOT replace JSONL with SQLite**. The JSONL output is forward-compatible and trivially greppable. `Get-CIJobSummary.ps1` over JSONL covers the cross-job analysis need without adding a database engine.
8. **DO NOT pursue a static IP pool unless the burn-in fails**. The current lock+lease design after the stale-lock fix is sufficient for `capacity: 4`. Static IPs cost more than they save at this scale.
---
## Security Improvements
Must (do these in Sprint 2 or 3):
- Redact Linux `ExtraGuestEnv` secret logging (C2).
- Replace Linux Mode 2 PAT URL with `http.extraHeader` (H3).
- Verify and fix the act_runner SSH alias service-account issue (H8).
- Add `JobId` pattern validation (Medium).
- Add `ExtraGuestEnv` key pattern validation (Medium).
- Run SSH `StrictHostKeyChecking=accept-new` with `UserKnownHostsFile=F:\CI\State\known_hosts` for CI jobs (Medium).
Could (Nice-to-have):
- Begin SHA256 pinning of Tier-1 toolchain installers.
- Verify ACLs on `F:\CI\keys\ci_linux` via a new `Validate-HostState.ps1`.
- Add a one-paragraph note in [docs/WORKFLOW-AUTHORING.md](../docs/WORKFLOW-AUTHORING.md) on Gitea log-masking limits (substring-only masking; base64 transforms defeat it).
Will NOT do:
- Full PKI for WinRM. The self-signed lab certificate plus VMnet8 isolation is the right boundary.
- External secret vault integration.
- Enable Defender / Firewall / UAC inside the build VM. The threat model accepts this for trusted private code; documented in [docs/BEST-PRACTICES.md](../docs/BEST-PRACTICES.md).
- Per-build credential rotation.
- Artifact signing.
---
## CI/CD Improvements
Concrete improvements to workflows and the composite action.
1. **Make the composite action self-sufficient for guest OS selection** (C1 fix). After this, [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) does not need to pass `guest-os`, `template-path`, or `snapshot-name` for the standard Windows/Linux matrix; the action infers from the runner label or accepts the explicit input.
2. **Add a top-level `outputs:` block** so downstream consumers can chain (H2 fix).
3. **Add `use-shared-cache` input** plumbed to `Invoke-CIJob.ps1 -UseSharedCache` (Medium fix). Forward to `Invoke-RemoteBuild.ps1` so NuGet/pip caches actually work end-to-end.
4. **Add an on-demand `gitea/workflows/self-test.yml`** (`workflow_dispatch` trigger only). One Windows job, one Linux job, both calling the composite action against a trivial build (`Write-Host 'ok' > out.txt`). Asserts artifact upload, JSONL events, clean teardown. Use as the regression gate for any action change.
5. **Add a Pester `gitea/workflows/tests.yml`** that runs the existing tests on every push. PowerShell 5.1, `Invoke-Pester -CI`. This is the current missing "CI for the CI" piece.
6. **Keep the lint workflow** [gitea/workflows/lint.yml](../gitea/workflows/lint.yml) as is.
7. **Do NOT add scheduled builds, dependency-update workflows, or release workflows.** Out of scope.
---
## Docker / Container Notes
This project deliberately does not use Docker, Podman, or any container runtime. The decision is correct and should be preserved.
Rationale:
- Windows builds require VS Build Tools, MSBuild, the full .NET SDK, and Windows-specific signing chains. Running those in Windows containers requires Windows Server host images, breaks isolation guarantees the project gets cheaply from VMware, and adds licensing complexity.
- The project's isolation boundary is a VM, not a container. VMs offer strictly stronger isolation than containers (kernel separation, hardware-virtualized network stack, separate filesystem) at a cost the system can afford.
- act_runner's `container.enabled: false` is the right setting for a system that uses real VMs.
- The performance overhead of "spin up a VM" vs "spin up a container" is real (a clone takes 5-30s, a container ~1s), but the orchestrator amortizes this across a 2-hour build budget. The cost is acceptable.
When might containers enter the picture? Likely never for this scope. If the project ever needs to build small Linux-only services with no Windows leg, a side-by-side `linux-build-container` runner label could be added later. That is a phase-3+ idea, not a current task.
---
## Monitoring and Logging Improvements
Concrete additions only. No metric server.
JSONL schema additions inside `F:\CI\Logs\<jobId>\invoke-ci.jsonl` (emitted by [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1)):
- Per-phase `duration_ms` calculated at phase end.
- `free_disk_gb_before` and `free_disk_gb_after` for the job.
- `ip_detection_method` field with values `guestinfo` or `getGuestIPAddress`.
- `vm_start_lock_wait_ms` recording how long the orchestrator waited on `vm-start.lock`.
- `clone_dir_size_mb` measured at teardown.
Event Log additions:
- Standardize on `CI-DiskSpaceAlert` (EventId 1001), `CI-RunnerHealth` (1002 stopped / 1003 restarted / 1004 throttled).
- Add `CI-JobFailure` source for failed orchestrator runs (EventId 2001) writing JobId and failed phase.
Watcher script additions:
- `scripts/Get-CIJobSummary.ps1` (nice-to-have): reads last N days of JSONL files, prints a table of JobId, started, elapsed, phase 1 duration, phase 5 duration, status.
- `scripts/Watch-RunnerHealth.ps1` (Low): respect `F:\CI\State\runner-maintenance.flag` to suppress restart attempts during template refresh.
Webhook payload changes:
- Remove emoji prefixes. Use `[WARN]`, `[ERROR]`, `[INFO]` plain text.
Do NOT add:
- Prometheus exporters.
- Grafana dashboards.
- OpenTelemetry collectors.
- SQLite or any other database sink.
- Discord/Slack rich-embed cards.
---
## Backup and Recovery Improvements
The current backup story is roughly: [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1) (currently pointing at a stale path). Improvements:
1. Fix the default path and add `-AllTemplates` switch (H7).
2. Add to [docs/RUNBOOK.md](../docs/RUNBOOK.md) the consolidated template-refresh runbook (Sprint 4 task 4.6).
3. Register a weekly scheduled task that runs `.\Backup-CITemplate.ps1 -AllTemplates -KeepLast 3` and writes a log to `F:\CI\Logs\backup\<date>.log`. Update [scripts/Register-CIScheduledTasks.ps1](../scripts/Register-CIScheduledTasks.ps1) to add this fifth scheduled task.
4. Document recovery from a destroyed template: restore the latest `.7z` from the backup destination, validate snapshots exist via `vmrun listSnapshots`, run `Validate-DeployState.ps1` (Windows + Linux), run the smoke workflow. If the smoke fails, restore the previous backup. Step-by-step in [docs/RUNBOOK.md](../docs/RUNBOOK.md).
5. Document the simple recovery path for a stale `vm-start.lock` and stale IP leases in [docs/RUNBOOK.md](../docs/RUNBOOK.md) — even though Sprint 3 task 3.2 makes this automatic, the operator-facing docs should still say "if you ever need to do this manually, the commands are X."
Do NOT add:
- VM snapshot backup to remote storage (out of scope; 7z files on a second drive is sufficient).
- Restore-time automated diff against a known-good template (over-engineered).
---
## Runner and Build Optimization
Concrete, measurable.
1. **Wire `use-shared-cache`** through the composite action and orchestrator (Medium item). Expected impact: ~15-30% faster Python and .NET builds that pull packages on every run, by avoiding the `pip download` and `nuget restore` over the WAN. Measurement: compare `phase 5 duration` JSONL before/after on a representative NSIS-plugin build.
2. **After burn-in, decide on `capacity`**. If the burn-in shows that capacity 4 is solid, keep it. If it shows that capacity 2 is the safe ceiling under typical host CPU contention, set `runner/config.yaml capacity: 2`. Expected impact: avoidance of unreproducible build failures during peaks.
3. **Defender exclusion for `F:\CI\BuildVMs\`** on the host (Low). Cloning and start are I/O heavy on the parent VMDK chain; Defender scanning slows this. Use `Set-MpPreference -ExclusionPath 'F:\CI\BuildVMs'`. Expected impact: ~10% faster clone-to-IP times. Document the trade-off in [docs/BEST-PRACTICES.md](../docs/BEST-PRACTICES.md): malicious code reaching the host would persist faster, but the threat model already accepts trusted-builds-only.
4. **Linked-clone health monitoring** (Nice-to-have): add per-job delta size to JSONL. If the average delta grows over months without a template refresh, that is a signal something is writing into the template chain. Not urgent; nice diagnostic.
5. **Do NOT pursue a warm clone pool**. Both reviewers agreed: the savings (5-30s per job) do not justify the state-machine complexity for a 2-hour build budget.
6. **Do NOT pursue VMX CPU/RAM tuning per job** unless the burn-in shows host saturation. Wait for evidence.
---
## Repository Conventions
These are the codified rules. Where ambiguous, write them into [AGENTS.md](../AGENTS.md) and [docs/BEST-PRACTICES.md](../docs/BEST-PRACTICES.md).
**Style and linting** (already largely in place):
- PowerShell 5.1 only. Every script begins with `#Requires -Version 5.1`, `Set-StrictMode -Version Latest`, `$ErrorActionPreference = 'Stop'`.
- 4-space indentation, no tabs.
- Verb-Noun cmdlet naming. `Get-`, `Set-`, `New-`, `Remove-`, `Invoke-`, `Test-`, `Wait-`, `Watch-`.
- `Write-Host` is acceptable and preferred over `Write-Output` in CI scripts (act_runner captures `Write-Host` cleanly; `Write-Output` pollutes function return values). Codified in [PSScriptAnalyzerSettings.psd1](../PSScriptAnalyzerSettings.psd1).
- No emoji in code, logs, webhook payloads, docs, or commit messages. Plain text markers like `[WARN]`, `[OK]`, `[x]`, `[ ]`.
- No PS 7+ syntax: no `??`, no `??=`, no ternary, no `&&`/`||` in PowerShell context, no `ForEach-Object -Parallel`, no `Test-Json`.
**File organization**:
- Production scripts under `scripts/`. Template provisioning under `template/`. Tests under `tests/`. Documentation under `docs/`. Plans and reviews under `plans/`.
- Module files `_Common.psm1` and `_Transport.psm1` stay in `scripts/`. Do not split further.
- One script, one verb-noun purpose. Composite actions live under `gitea/actions/<name>/action.yml`.
**Commit messages**:
- Imperative mood, present tense ("Add bounded vmrun wrapper", not "Added" or "Adding").
- Subject line ≤72 characters. Body explains *why*, not *what*.
- Reference the master plan checklist item by short title when relevant: e.g. `Fix C1: Linux matrix template selection`.
**Branching**:
- `main` is the only long-lived branch.
- Use short-lived feature branches `fix/c1-linux-matrix`, `fix/h2-action-outputs`, etc.
- Squash merge or rebase; do not preserve micro-commits.
**Testing requirements before merge**:
- After H10 is implemented: every PR runs the lint and Pester workflows. After the smoke-test workflow exists, run it for any change touching orchestration scripts.
**Documentation updates**:
- A change to a script that changes its parameters or behavior must update [docs/CI-FLOW.md](../docs/CI-FLOW.md), [docs/ARCHITECTURE.md](../docs/ARCHITECTURE.md), or [docs/RUNBOOK.md](../docs/RUNBOOK.md) in the same commit.
- A change to [runner/config.yaml](../runner/config.yaml) or directory layout must update [AGENTS.md](../AGENTS.md) and [Setup-Host.ps1](../Setup-Host.ps1).
---
## Documentation Status
Per-document assessment.
| Document | Status | Action |
| ------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------- |
| [AGENTS.md](../AGENTS.md) | Good. Reflects current paths, PS 5.1 rules, error patterns. | Keep updated as new gotchas are discovered. |
| [README.md](../README.md) | Stale. Claims production-ready before stabilization pass completes. | Replace "production-ready" with "feature-complete; stabilization pass in progress" until all Critical and High items are checked. |
| [docs/ARCHITECTURE.md](../docs/ARCHITECTURE.md) | Mostly accurate; minor diagram rendering issue. | Reformat the ASCII diagram box. Reflect H4 bounded-vmrun pattern in the lifecycle section. |
| [docs/CI-FLOW.md](../docs/CI-FLOW.md) | Stale on partial-artifacts-on-failure. Still mentions `getGuestIPAddress` as primary IP detection. | Update IP-detection section to reflect `guestinfo.ci-ip` primary. Reconcile failure-scenario table with whether H-level diagnostics are implemented. |
| [docs/RUNBOOK.md](../docs/RUNBOOK.md) | Useful but incomplete. | Add: consolidated template-refresh procedure; capacity burn-in results; recovery from stale lock; runner service-account identity. |
| [docs/BEST-PRACTICES.md](../docs/BEST-PRACTICES.md) | Good. Old `WinBuild` path references need fixing. | Update all `WinBuild` to `WinBuild2025`. Add HGFS shared-folder cache poisoning note. |
| [docs/WORKFLOW-AUTHORING.md](../docs/WORKFLOW-AUTHORING.md) | Good. | Add one-paragraph note on Gitea secret-mask substring limitation. |
| [docs/HOST-SETUP.md](../docs/HOST-SETUP.md) | Mostly accurate. | Update directory list to match new [Setup-Host.ps1](../Setup-Host.ps1). Add ACL guidance for `F:\CI\keys\ci_linux`. |
| [docs/WINDOWS-TEMPLATE-SETUP.md](../docs/WINDOWS-TEMPLATE-SETUP.md) | Good. | Cross-reference the consolidated template-refresh runbook in [docs/RUNBOOK.md](../docs/RUNBOOK.md). |
| [docs/LINUX-TEMPLATE-SETUP.md](../docs/LINUX-TEMPLATE-SETUP.md) | Good. | Add `Validate-DeployState.ps1` Linux branch usage. |
| [docs/OPTIMIZATION.md](../docs/OPTIMIZATION.md) | Mostly accurate. | Note Defender exclusion option and shared-cache wiring after H-Medium fixes. |
| [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md) | Stale. PS 7 syntax, wrong parameter names, wrong task/event names. | Sprint 5 task 5.3: PS 5.1 syntax, `-RepoUrl`, current Event Log sources. |
| [docs/Setup-GiteaSSH.md](../docs/Setup-GiteaSSH.md) | Untouched by reviewers; assume accurate. | Verify against fixed service-account path from H8. |
| [TODO.md](../TODO.md) | Acting as a CHANGELOG. | Sprint 5 task: move "done" sections into `docs/CHANGELOG.md`; keep TODO for genuinely outstanding work. |
| [docs/archived/2026-05-10/](../docs/archived/2026-05-10/) | Historical. | Leave as-is. |
---
## Final Completion Plan
Execute in this exact order. Each phase has a single success criterion that gates the next.
**Phase 0 — Read this plan and commit it.** Already done by the act of generating it.
**Phase 1 — Validation (Sprint 1).** Verify the act_runner service account, confirm the directory inventory, run the 4-way concurrency burn-in, manually trigger the broken Linux matrix to confirm C1. Output: a section appended to [docs/RUNBOOK.md](../docs/RUNBOOK.md) titled "Validation Results 2026-05-XX" documenting findings. Gate: burn-in passed three times (keep `capacity: 4`) or was set to 2 (with reasoning recorded).
**Phase 2 — Critical Fixes (Sprint 2).** Fix C1 and C2, plus H2/H3/H6/H7. Manually trigger [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) for both matrix legs after each fix. Gate: both Critical items closed and `[x]` in this plan's Critical section; both legs green in the live matrix workflow.
**Phase 3 — High Priority Fixes (Sprint 3).** Complete the rest of the High items: H4 (bounded vmrun), H5 (stale lock), H8 (SSH alias fix), H9 (Linux validator), H10 (smoke-test workflow), H11 (script-root env var), H12 (benchmark alignment). Gate: smoke-test workflow green; all High items `[x]`.
**Phase 4 — Operational Hardening (Sprint 4).** All Medium items including consolidated template-refresh runbook. Gate: documented end-to-end template refresh executed successfully against a non-production template clone.
**Phase 5 — Polish (Sprint 5).** Low items, nice-to-have summary tool, documentation alignment, TODO → CHANGELOG migration. Gate: README claim updated from "feature-complete; stabilization pass in progress" back to "production-ready for homelab".
**Phase 6 — Tag v1.0.** Create an annotated tag `v1.0.0` on `main` once Phase 5 gate passes. Write a `docs/CHANGELOG.md` v1.0 entry summarizing the journey.
**Phase 7 — Operate.** Use the system. Track new findings in `TODO.md`. Run the burn-in any time act_runner capacity, host hardware, or template chains change materially.
When all Critical and High items are checked, this system is genuinely production-ready for homelab use.
plans/gpt55-analysis.md
+492
View File
@@ -0,0 +1,492 @@
# Technical Review: Local CI/CD System
Date: 2026-05-11
Scope reviewed: all requested project files in the workspace, including orchestration scripts, template provisioning scripts, runner configuration, workflows, Pester tests, and operational documentation.
## 1. Executive Summary
This project is a self-hosted homelab CI/CD platform built around Gitea Actions, act_runner, VMware Workstation linked clones, Windows PowerShell 5.1, WinRM for Windows guests, and SSH for Linux guests. The core idea is sound: keep the Windows host as a thin orchestrator, create a fresh linked clone for each job, run the build inside an isolated VM, collect artifacts, and destroy the clone in a `finally` block. For a single-host homelab, this is the right architecture. It uses the tools already available on the host, avoids containerizing workloads that need full Windows build tooling, and gives repeatable isolation without introducing Kubernetes, ESXi, or unnecessary service layers.
Overall, the system is well-built for its intended environment. The strongest parts are the lifecycle decomposition, the cold-template workflow, the PS5.1 discipline in the main scripts, the Windows template hardening/provisioning gates, and the Linux cloud-init/open-vm-tools work. The project has clearly gone through real failure-driven refinement: the notes about VMware Tools timing, cloud-init BOM issues, headless `getGuestIPAddress`, WinRM HTTPS, snapshot state, and Linux netplan MAC matching are exactly the kind of details that only appear after serious testing.
I would not yet call it fully trustworthy for unattended daily use without a small stabilization pass. The core VM lifecycle is close, but a few issues are operationally important: the composite action output block is malformed, Linux secret injection can leak secrets to logs and process arguments, host bootstrap and backup defaults drift from the current directory layout, stale `vm-start.lock` is not automatically cleared, and some documentation/test-plan commands no longer match the actual parameter names or PS5.1 constraint. These are fixable, homelab-scale problems, not architectural failures.
## 2. Project State Assessment
Genuinely done:
- Windows VM lifecycle exists end to end: [scripts/New-BuildVM.ps1](../scripts/New-BuildVM.ps1), [scripts/Wait-VMReady.ps1](../scripts/Wait-VMReady.ps1), [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1), [scripts/Get-BuildArtifacts.ps1](../scripts/Get-BuildArtifacts.ps1), and [scripts/Remove-BuildVM.ps1](../scripts/Remove-BuildVM.ps1) form a coherent pipeline.
- Linux build VM support is implemented, not just planned. [template/Deploy-LinuxBuild2404.ps1](../template/Deploy-LinuxBuild2404.ps1), [template/Prepare-LinuxBuild2404.ps1](../template/Prepare-LinuxBuild2404.ps1), [template/Install-CIToolchain-Linux2404.sh](../template/Install-CIToolchain-Linux2404.sh), [scripts/_Transport.psm1](../scripts/_Transport.psm1), and the Linux branches in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1), [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1), and [scripts/Get-BuildArtifacts.ps1](../scripts/Get-BuildArtifacts.ps1) are real.
- WinRM HTTPS migration is implemented in the active scripts. The Windows paths use `-UseSSL -Port 5986 -Authentication Basic` and a shared `New-CISessionOption` helper in [scripts/_Common.psm1](../scripts/_Common.psm1).
- The current orchestrator has a real clone/start/IP serialization lock and lease file defense in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1). This is stronger than some older documentation implies.
- Scheduled maintenance scripts exist: cleanup, retention, disk alert, runner health, and scheduled task registration are all present.
- Unit tests exist and cover the shared module, clone creation, removal, and readiness timeout/validation behavior.
- The Windows and Linux templates have serious pre-snapshot validation gates. They are not just installation scripts; they assert the state they intend to snapshot.
Documented as done but not fully validated or internally inconsistent:
- The README claims production-ready status, while [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md) still marks several real-VM validations as skipped or partial. The honest state is "feature-complete, with remaining real-VM validation and workflow defects." The README is ahead of the evidence.
- The composite action is documented as reusable and complete, but [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) places `artifact-path` and the second `artifact-name` under `inputs` instead of a top-level `outputs` block. That can break artifact upload steps that use `steps.invoke-ci.outputs.*`.
- The test plan contains PS7-only `ForEach-Object -Parallel`, even though the project standard is PS5.1 only. This is a documentation/test executability issue, not a production-script syntax issue.
- [docs/CI-FLOW.md](../docs/CI-FLOW.md) and [docs/ARCHITECTURE.md](../docs/ARCHITECTURE.md) describe some older mechanics such as `getGuestIPAddress` as the main flow, while [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) now primarily reads `guestinfo.ci-ip` and uses `getGuestIPAddress` as fallback.
- [docs/CI-FLOW.md](../docs/CI-FLOW.md) says partial artifacts are collected after build failure, but the current orchestrator enters `catch` and cleanup directly; artifact collection happens only after successful `Invoke-RemoteBuild.ps1`.
- TODO says some security tasks are deferred even though corresponding implementation exists. The `PAT mai persistito` item still says deferred, but the Windows path has an implementation and the Linux path has a problematic implementation.
- [Setup-Host.ps1](../Setup-Host.ps1) still creates `F:\CI\Templates\WinBuild`, not the current `WinBuild2025`, `WinBuild2022`, and `LinuxBuild2404` structure. It also does not create `F:\CI\State`, `F:\CI\keys`, or `F:\CI\Cache\pip`.
## 3. Architecture Analysis
The overall design quality is high for a homelab system. The host-as-orchestrator model is appropriate: act_runner runs workflow logic on the host, but build commands execute inside disposable VMs. This gives a strong isolation boundary without the cost of an enterprise virtualization stack. The template and linked-clone model is the right tradeoff for VMware Workstation: fast clone creation, small per-job disk footprint, and deterministic toolchain state.
VM lifecycle management is mostly sound. [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) owns the lifecycle and wraps destructive cleanup in `finally`. Clone creation is delegated to [scripts/New-BuildVM.ps1](../scripts/New-BuildVM.ps1), readiness to [scripts/Wait-VMReady.ps1](../scripts/Wait-VMReady.ps1), build execution to [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1), artifacts to [scripts/Get-BuildArtifacts.ps1](../scripts/Get-BuildArtifacts.ps1), and teardown to [scripts/Remove-BuildVM.ps1](../scripts/Remove-BuildVM.ps1). This is a good separation of concerns. The teardown script has retry logic around `deleteVM`, which is useful because VMware often releases locks late.
The weak point in lifecycle management is bounded execution. The primary orchestrator still calls `vmrun start`, `readVariable`, `getGuestIPAddress`, and other native tools directly without a general timeout wrapper. Some deployment scripts have `Invoke-VmrunBounded`, but the active CI scripts do not use a common bounded process helper. If `vmrun` hangs, the runner slot can hang until act_runner job timeout, and cleanup may never run. This is the highest reliability gap left in the core lifecycle.
The transport duality is clean enough but not fully abstracted. Windows and Linux branches are explicit in the top-level scripts, which is a good homelab choice: the code is understandable, and the two transports differ enough that over-abstracting would make troubleshooting harder. [scripts/_Transport.psm1](../scripts/_Transport.psm1) covers SSH helpers only, while [scripts/_Common.psm1](../scripts/_Common.psm1) covers WinRM session options and vmrun. This is acceptable, but function naming could be clearer: `_Transport.psm1` sounds like a cross-transport abstraction even though it is SSH-specific.
The IP allocation mechanism is better than the TODO summary suggests. The actual implementation serializes clone/start/IP detection through `F:\CI\State\vm-start.lock`, detects IP through `guestinfo.ci-ip` when available, writes an IP lease file, and releases it in `finally`. This reduces the practical risk of two concurrent builds talking to the same VM. However, it is not true deterministic IP allocation. It still depends on DHCP, detects collisions after the fact, and a stale `vm-start.lock` from a host crash is not automatically pruned by the scheduled tasks. For capacity 4, it is probably reliable enough after one fix: automatic stale lock cleanup.
Credential management is pragmatic but mixed. Windows guest credentials use Windows Credential Manager via `BuildVMGuest`, which is appropriate. Linux uses a host-side private key at `F:\CI\keys\ci_linux`, also appropriate for a homelab, provided ACLs are correct. The weakest credential path is `-UseGitClone` and extra guest environment variables. The Windows `git -c http.extraHeader=Authorization: Basic ...` approach avoids putting credentials in the clone URL, but the header value still appears in the guest process command line. The Linux `GIT_CLONE_URL` implementation embeds a credential-bearing URL into the SSH command string. The Linux extra-env path logs the composed build command including exported secret values. These need fixing before secret-bearing builds are safe.
Script interdependencies are reasonable. [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) is intentionally the hub. The coupling to hardcoded host paths is expected for a fixed homelab, but inconsistent defaults undercut that: [runner/config.yaml](../runner/config.yaml), [Setup-Host.ps1](../Setup-Host.ps1), [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1), and docs do not all agree on template paths. A single path contract in [AGENTS.md](../AGENTS.md) plus corrected defaults would make the system easier to rebuild.
The module organization is helpful but incomplete. [scripts/_Common.psm1](../scripts/_Common.psm1) covers three high-value helpers, but many scripts still implement local `Invoke-Vmrun`, `Assert-Step`, `Set-VmxKey`, ISO writing, and polling patterns. For template deployment scripts, duplication is acceptable because they are long, one-off provisioning flows. For production CI scripts, move bounded native process execution, IPv4 validation, and stale state cleanup into `_Common.psm1`.
The host-as-orchestrator pattern has clear pros: simple mental model, native access to VMware Workstation, easy file-based state, easy Event Log and Scheduled Task integration, and no new services. The cons are also clear: host filesystem paths leak into workflows, host compromise compromises the whole CI system, concurrency depends on local file locking, and observability is local unless forwarded. For this homelab, the pros outweigh the cons.
## 4. Code Quality and PS5.1 Compliance
Project-wide PS5.1 compliance is mostly good in production PowerShell. I did not find actual PS7 null-coalescing, ternary, or `ForEach-Object -Parallel` in the production `.ps1` scripts that were reviewed. Bash `&&` appears inside commands intentionally executed by Linux guests; that is acceptable. The major PS5.1 compliance problem is in documentation/test-plan snippets, especially [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md), which uses `ForEach-Object -Parallel`.
[scripts/_Common.psm1](../scripts/_Common.psm1): Good minimal shared module. PS5.1 compliant. Error handling in `Invoke-Vmrun` checks `$LASTEXITCODE` and optionally throws. Gap: no timeout support, no standard native command result object with elapsed time, and no shared IPv4 validator.
[scripts/_Transport.psm1](../scripts/_Transport.psm1): Clear SSH/SCP wrapper. PS5.1 compliant. It uses `StrictHostKeyChecking=no` and `UserKnownHostsFile=NUL`, which is operationally convenient but weakens MITM protection. Error handling checks exit codes. Gap: it can throw command text containing secrets when callers pass secret-bearing commands.
[scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1): Strong orchestration structure and good `finally` cleanup. PS5.1 compliant. Smells: many hardcoded `F:\CI` paths despite parameters for others; direct `vmrun` calls instead of `_Common.psm1` wrapper; stale `vm-start.lock` behavior causes a 10-minute delay then failure; JSONL logging is best-effort but lacks duration calculations; artifact collection is skipped on build failure. Parameter validation gaps: `JobId` is not sanitized before use in paths, so hostile or malformed IDs could create awkward paths. `ExtraGuestEnv` keys are documented as valid env names but not validated in the orchestrator.
[scripts/New-BuildVM.ps1](../scripts/New-BuildVM.ps1): Simple and solid. Uses `_Common.psm1`. PS5.1 compliant. Gap: `JobId` is directly used in clone folder names; add a conservative validation pattern or sanitize to `[A-Za-z0-9_.-]`. It does not preflight snapshot existence; `vmrun clone` failure is probably enough, but a `listSnapshots` check could improve diagnostics.
[scripts/Remove-BuildVM.ps1](../scripts/Remove-BuildVM.ps1): Practical cleanup with `SupportsShouldProcess`. PS5.1 compliant. Smell: `$ErrorActionPreference = 'Continue'` makes it resilient but can hide failures. It warns if the directory remains, but does not emit structured state or Event Log. Direct `vmrun` calls lack timeout. It can still hang on `getState`, `list`, or `deleteVM` if VMware blocks.
[scripts/Wait-VMReady.ps1](../scripts/Wait-VMReady.ps1): Good improvement over `getGuestIPAddress` for power-state detection by using `vmrun list`. PS5.1 compliant. The Windows readiness check only tests TCP 5986, not an authenticated WinRM session, so it can declare ready before credentials/remoting policy are actually usable. This is acceptable for speed but can shift failure into Phase 5. The docblock still references older `getState`/`Test-WSMan` concepts.
[scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1): Functionally rich but now the densest and riskiest script. PS5.1 compliant. Good: mode validation, WinRM/SSH separation, 7-Zip fallback, and Linux artifact staging. Problems: Linux secret values are logged through `Write-Host "Running build: $buildCmd"`; Linux `GIT_CLONE_URL` embeds PAT in a remote command string; Windows PAT is passed to `git -c http.extraHeader=...`, which can be visible in guest process command line; `ExtraGuestEnv` keys are not validated; custom Windows build command uses `cmd /c "$cmd 2>&1"`, so quoting/injection is controlled only by trusted workflow inputs. For a trusted homelab this is tolerable, but it contradicts the stated secret-safety claims.
[scripts/Get-BuildArtifacts.ps1](../scripts/Get-BuildArtifacts.ps1): Clear and mostly robust. PS5.1 compliant. Windows branch retries WinRM session creation. Linux branch copies a directory recursively and validates at least one file. Gap: no manifest or artifact metadata is written, and failure diagnostics from guest output directories are limited.
[scripts/Cleanup-OrphanedBuildVMs.ps1](../scripts/Cleanup-OrphanedBuildVMs.ps1): Good simple orphan cleanup. PS5.1 compliant. Gap: does not clear stale `F:\CI\State\vm-start.lock`; does not remove stale IP leases unless retention runs; uses directory `LastWriteTime`, which can be misleading if VMware updates files late or not at all.
[scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1): Useful and safe. PS5.1 compliant. It cleans artifacts, logs, and stale IP leases. Gap: hardcoded leases dir and no cleanup for `vm-start.lock`. It does not write Event Log entries, despite some test-plan expectations.
[scripts/Watch-DiskSpace.ps1](../scripts/Watch-DiskSpace.ps1): Good minimal disk monitor. PS5.1 compliant. Gap: webhook content includes emoji despite the user's stated no-emoji preference; more importantly, the Event Log source is `CI-DiskAlert`, but the test plan expects other names. Exit code 1 on low disk is fine for Scheduled Task visibility.
[scripts/Watch-RunnerHealth.ps1](../scripts/Watch-RunnerHealth.ps1): Good rate-limited restart design. PS5.1 compliant. Similar webhook emoji issue. It checks only local Windows service status, not whether Gitea sees the runner online. For daily use, add optional API check later.
[scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1): Conceptually important, but defaults are stale. It backs up `F:\CI\Templates\WinBuild`, while the current primary template is `F:\CI\Templates\WinBuild2025`. That makes the default backup procedure fail or back up the wrong thing. High operational risk before template refresh.
[scripts/Register-CIScheduledTasks.ps1](../scripts/Register-CIScheduledTasks.ps1): Good, idempotent scheduled task registration. PS5.1 compliant. Gaps: default `ScriptRoot` is hardcoded to `N:\Code\Workspace\Local-CI-CD-System\scripts`; acceptable on this host but not portable. Task names in docs/test plan do not always match actual registered names (`CI-DiskSpaceAlert` versus `CI-DiskSpace`).
[scripts/Set-TemplateSharedFolders.ps1](../scripts/Set-TemplateSharedFolders.ps1): Clean idempotent VMX editor. PS5.1 compliant. Risk: shared folders are writable from guest to host; this is useful for caches but weakens isolation. Use only for cache directories and never map broad host paths.
[scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1): Useful baseline tool. PS5.1 compliant. Gaps: it still uses `getGuestIPAddress` rather than the newer `guestinfo.ci-ip` path, so Linux timing may not match the production orchestrator. JSONL structure differs from the test plan's expected `phase` and `duration_ms` fields.
[Setup-Host.ps1](../Setup-Host.ps1): Helpful bootstrap, but drifted. It creates old template paths, does not create Linux key/state/cache directories, registers only `windows-build:host,dotnet:host,msbuild:host` labels during runner registration, and leaves Linux label only in config. It uses plaintext guest password as a parameter, although default is empty and prompt is preferred. It should be corrected before claiming rebuild-from-scratch reliability.
[runner/Install-Runner.ps1](../runner/Install-Runner.ps1): Marked deprecated. It is fine as reference but should not be presented as an active path. It lacks newer labels and may drift further.
[template/Deploy-WinBuild2025.ps1](../template/Deploy-WinBuild2025.ps1) and [template/Deploy-WinBuild2022.ps1](../template/Deploy-WinBuild2022.ps1): Sophisticated and PS5.1 compliant. Good use of bounded vmrun for stop. Risks: admin password and autologin password are embedded into generated unattend/post-install content and registry. That is accepted in the lab threat model but should be clearly treated as snapshot secret material. Both scripts duplicate a lot of logic; tolerable, but fixes must be applied to both.
[template/Prepare-WinBuild2025.ps1](../template/Prepare-WinBuild2025.ps1): Strong validation and reboot loop for Windows Update. PS5.1 compliant. Operational smell: it prompts interactively for snapshot creation and deletion, which makes fully unattended template refresh harder. It also uses `ConvertTo-SecureString -AsPlainText` for admin password conversion, which is expected when bridging from a string parameter but will trigger static warnings.
[template/Install-CIToolchain-WinBuild2025.ps1](../template/Install-CIToolchain-WinBuild2025.ps1): Very detailed and mostly well engineered. PS5.1 compliant. Main debt: many installer SHA256 values are empty, so the supply-chain pinning structure mostly warns rather than enforces. It also uses a public KMS endpoint by default, which is not appropriate outside a private lab or unless the user has explicitly chosen that activation model.
[template/Deploy-LinuxBuild2404.ps1](../template/Deploy-LinuxBuild2404.ps1): Good cloud-init implementation with BOM/LF handling and guestinfo fallback. PS5.1 compliant. Gap: no hash verification for downloaded Ubuntu cloud VMDK. A corrupted or malicious base image would poison every Linux build.
[template/Prepare-LinuxBuild2404.ps1](../template/Prepare-LinuxBuild2404.ps1): Practical SSH provisioning. PS5.1 compliant. It validates only a loose IPv4 pattern, not per-octet. It uses remote bash `&&` and `||` inside strings, which is correct for Linux. It uses `StrictHostKeyChecking=no` in several places despite comments mentioning accept-new.
[template/Install-CIToolchain-Linux2404.sh](../template/Install-CIToolchain-Linux2404.sh): Clean bash script with `set -euo pipefail`. It installs a solid basic build environment. Gap: no apt package version pinning, no snapshot of apt sources, and no retry wrapper for apt transient failures.
[template/Validate-DeployState.ps1](../template/Validate-DeployState.ps1) and [template/Validate-SetupState.ps1](../template/Validate-SetupState.ps1): Useful validation scripts. Docblocks still say WinRM HTTP/Basic even though the implementation uses HTTPS/5986. [template/Validate-SetupState.ps1](../template/Validate-SetupState.ps1) assumes `PasswordExpires` exists, while the install script already handles property variation; this can fail on different LocalAccounts module versions.
## 5. Security Analysis (Homelab-realistic)
WinRM with self-signed certificate plus Basic auth is acceptable in this lab if VMnet8 is isolated and the host is trusted. The important improvement is that `AllowUnencrypted=false` and traffic uses HTTPS/5986. Skipping CA/CN/revocation checks is normal for a self-signed lab certificate, but it means the host does not authenticate the VM identity beyond IP and network position. A malicious VM on VMnet8 could theoretically impersonate another VM if the IP handling failed. The current IP lock and lease design reduces but does not eliminate this risk.
SSH key storage is acceptable if ACLs on `F:\CI\keys\ci_linux` restrict access to the runner service identity and Administrators. The repo documents the path but does not include an ACL validation script. Add one. Because the key has no passphrase, filesystem ACLs are the only real protection.
PAT handling in `-UseGitClone` is mixed. The Windows path avoids putting credentials in the URL, which is good, but it uses `git -c http.extraHeader=Authorization: Basic <base64>` so the token-derived header may be visible in process arguments inside the guest. The Linux path is worse: it builds a credential-bearing URL and embeds it in a remote command assigned to `GIT_CLONE_URL`. If SSH command logging, PowerShell error messages, or process inspection catches that string, the PAT leaks. The TODO and docs claim PAT safety more strongly than the code supports.
Secrets via workflow-level `extra-guest-env-json` are not currently safe on Linux. [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) builds an `export KEY='value';` prefix and then logs the full build command. That directly prints secret values. Windows is safer because it sets environment variables inside the session and does not print the hashtable values, but it still passes secrets as WinRM arguments.
Runner environment variables in [runner/config.yaml](../runner/config.yaml) are not secrets. They are paths and target names. That is fine. Do not add PATs or passwords there. The composite action correctly passes most inputs through env vars to reduce shell injection risk, but `extra-guest-env-json` itself is an env var containing secrets; ensure act_runner does not echo it.
Template hardening disables Defender, Firewall, and UAC. For this homelab, this is acceptable only if all builds are trusted. The documentation in [docs/BEST-PRACTICES.md](../docs/BEST-PRACTICES.md) is honest about this. The most important nuance is that a compromised build VM is not just a guest problem: it can reach the NAT network, Gitea, package registries, and any host-exposed VMware shared folders. If shared cache folders are writable, malicious code can poison cache content for future builds.
A compromised build VM could likely reach:
- Gitea over LAN/NAT if routing allows it.
- The internet through VMnet8 NAT.
- Host VMware shared folders configured for caches.
- WinRM/SSH ports of other concurrently running build VMs if IP/network isolation is not stricter.
- Potentially host services bound to VMnet8.
It should not have host filesystem access beyond configured shared folders, and it should not have the Linux private key or Windows Credential Manager secrets unless scripts explicitly copy them in. Keep it that way.
## 6. Reliability and Resilience Analysis
VM leak scenarios remain possible but bounded. Normal build failures are covered by `finally`. Host power loss, hung PowerShell process, forced runner termination, or a `vmrun` call that never returns can bypass cleanup. [scripts/Cleanup-OrphanedBuildVMs.ps1](../scripts/Cleanup-OrphanedBuildVMs.ps1) handles stale clone directories, and scheduled tasks run it every 6 hours plus startup. That is good. Add Event Log output and stale lock cleanup to make recovery visible.
IP lease race conditions are mostly mitigated by serializing clone/start/IP detection. This is not a true static lease allocator; it is a concurrency gate plus collision detector. For capacity 4, this is probably good enough. The risky case is host crash after creating `vm-start.lock` or an IP lease. IP leases older than 12 hours are cleaned by retention, but the start lock is not. That means a crash can cause every new job to wait 10 minutes and fail until manual cleanup.
The error recovery quality in try/finally blocks is good in the main orchestrator. The Windows template prepare script also has careful `finally` restoration of TrustedHosts. The removal script tries soft stop, hard stop, deleteVM retries, and directory removal. However, there is no outer watchdog for stuck native processes. If PowerShell is blocked inside native `vmrun`, `finally` cannot execute until the native process returns.
On host reboot mid-build, the expected behavior is: act_runner service restarts, Gitea marks the job failed or times it out, clone directories and possibly running VMs remain, scheduled cleanup removes clones older than threshold, retention later removes stale leases. This is close to acceptable. Add startup cleanup for `vm-start.lock` and leases to avoid avoidable first-job failures after reboot.
If `vmrun` hangs, the system is weak. act_runner timeout may eventually terminate the job, but PowerShell may remain stuck and the clone may leak. Some deploy scripts use bounded process execution; move that pattern into [scripts/_Common.psm1](../scripts/_Common.psm1) and use it in production lifecycle scripts.
Disk exhaustion is handled by monitoring and retention but not by preflight gates. [scripts/Watch-DiskSpace.ps1](../scripts/Watch-DiskSpace.ps1) alerts under 50 GB, and [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1) uses aggressive retention under that threshold. But [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) does not check free space before cloning. Add a fast preflight check before Phase 2 so clone failure is clear.
Artifact loss scenarios exist. Windows custom build mode packages artifacts only if build exit is 0. Linux mode copies artifact source if it exists and logs a warning if not, but then `Get-BuildArtifacts.ps1` fails only if the output directory is empty. On build failure, [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) does not attempt to collect partial logs/artifacts from the guest before teardown. This hurts failure triage.
## 7. CI/CD Workflow Analysis
The composite action idea is excellent. A reusable [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) hides the VM lifecycle and lets application repositories specify only build command, artifact source, guest OS, and repo URL. That is exactly the right abstraction for a homelab with several repositories.
The current action has a structural YAML problem: it appears to define outputs under `inputs`, and duplicates `artifact-name`. Composite action outputs should be top-level `outputs`. As written, the `Upload artifacts` step may not receive `steps.invoke-ci.outputs.artifact-path` and `artifact-name` as intended. This can make builds succeed inside the VM but fail at artifact upload.
Workflow flexibility is otherwise good. [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) uses a Windows/Linux matrix and disambiguates job IDs with `job-id-suffix`. That is the right pattern. Missing patterns: PR checks are not represented in the live workflow, scheduled builds are not represented, and dependency caching via `actions/cache` is not shown even though act_runner cache is enabled. For this system, PR checks and manual rebuild are more valuable than scheduled builds.
Artifact upload to Gitea is done through `actions/upload-artifact@v4` in the composite action. The VM pipeline collects files into `F:\CI\Artifacts\<jobId>`, then upload-artifact reads that host-side directory. That is a good bridge between local VM isolation and Gitea's artifact store, once the outputs block is fixed.
Build cache effectiveness is partial. NuGet/pip shared folders exist for Windows when `-UseSharedCache` is passed, but the composite action does not expose a `use-shared-cache` input and [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) does not forward `UseSharedCache` to [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1). The feature exists at the lower layer but is not wired into the main workflow path.
Matrix build design quality is good: `target: [windows, linux]`, `runs-on: ${{ matrix.target }}-build`, `fail-fast: false`, and target-specific artifact names are exactly the right shape.
## 8. Observability and Monitoring
The JSONL logging design is useful. [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) emits phase start/success/failure events with ISO timestamps, job ID, phase, status, and data. This is enough to compute phase durations after the fact. It is not yet a full metrics layer, but it is the right low-cost foundation.
Disk space monitoring is adequate for a homelab. [scripts/Watch-DiskSpace.ps1](../scripts/Watch-DiskSpace.ps1) writes Event Log warnings and optionally posts a webhook. [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1) can reclaim space. Add a pre-clone free-space gate to turn "cryptic vmrun failure" into "free disk below threshold."
Runner health monitoring is useful but local-only. [scripts/Watch-RunnerHealth.ps1](../scripts/Watch-RunnerHealth.ps1) restarts the Windows service with a cooldown. It does not check the Gitea API to prove the runner is online from Gitea's perspective. For daily use, local service status is enough most of the time; a Gitea API check is a phase-2 improvement.
After a build failure, you would know the high-level phase and the top-level exception. You might not know enough about guest state because failed builds do not trigger artifact/log collection. You also would not know exact CPU, memory, disk I/O, or network state at failure time. The JSONL events do not currently include durations, vmrun exit outputs for all calls, free disk at start, template snapshot name validation, or guest OS tool versions.
Missing metrics and alerting:
- Per-phase duration summary emitted at job end.
- Free disk before and after job.
- Clone directory size at teardown.
- VM start lock wait time.
- Guest IP detection method used: guestinfo or fallback.
- Count of cleanup failures and leftover directories.
- Gitea runner online API status.
## 9. Operational Analysis
Backup/recovery procedures are conceptually complete but suffer from path drift. [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1) is valuable, but its default points to `F:\CI\Templates\WinBuild`. The current primary template is `F:\CI\Templates\WinBuild2025`. Fix that before relying on backups.
The template refresh process is detailed and realistic. Windows deployment, preparation, validation, shutdown, and snapshotting are all documented. The main problem is that some steps remain interactive, especially snapshot replacement in [template/Prepare-WinBuild2025.ps1](../template/Prepare-WinBuild2025.ps1). For a homelab this is acceptable, but for repeatability add a `-ForceSnapshot` or `-NoPrompt` option.
Scheduled task coverage is good: orphan cleanup, retention, disk alert, and runner health. Gaps: no scheduled template backup, no startup task to clear stale `vm-start.lock`, no task to validate runner online from Gitea, and no periodic smoke build.
Manual steps that remain manual but should be automated or at least scripted:
- Verifying ACLs on `F:\CI\keys\ci_linux`.
- Verifying Credential Manager entries under the same identity that act_runner uses.
- Updating and restarting the installed runner config after [runner/config.yaml](../runner/config.yaml) changes.
- Snapshot version rotation and rollback.
- Running one post-refresh smoke build and only then flipping `GITEA_CI_SNAPSHOT_NAME`.
Day-2 operations under-documented:
- How to rotate the Linux SSH key.
- How to recover if `actions/upload-artifact` fails after VM build success.
- How to inspect `invoke-ci.jsonl` to compute durations.
- How to confirm the runner service account can read Credential Manager secrets.
- How to validate shared folder cache poisoning or corruption.
## 10. Test Coverage
The Pester tests cover the right first layer: `_Common.psm1`, clone naming and cleanup on clone failure, VM removal behavior, `-WhatIf`, readiness timeout, and IP parameter validation. They are fast and do not require real VMware infrastructure, which is good for frequent local validation.
What they do not cover is more important for daily trust:
- [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) orchestration state machine.
- The file lock and lease behavior under concurrency.
- JSONL event emission.
- `UseGitClone` credential handling.
- Linux `ExtraGuestEnv` handling and secret redaction.
- Composite action YAML validity.
- Scheduled task registration names and arguments.
- Backup path correctness.
- Retention policy stale lock cleanup.
Missing integration tests:
- One Windows real-VM smoke build.
- One Linux real-VM smoke build.
- One 2-job or 4-job parallel run with lease verification.
- One failure build that proves logs/artifacts are collected or intentionally not collected.
- One workflow-level test of the composite action and upload-artifact path.
- One host reboot simulation or manual recovery test.
The test plan is not executable as-is. It contains wrong parameter names (`-RepositoryUrl` instead of `-RepoUrl`), PS7-only `ForEach-Object -Parallel`, expected task names that do not match actual task names, and expected JSONL shapes that do not match [scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1). Treat it as an audit checklist, not a copy-paste runbook, until corrected.
## 11. Technical Debt
1. Stale path defaults. What: old `WinBuild` paths remain in setup/backup docs/scripts. Why deferred: project evolved from one template to multiple templates. Should it stay deferred: no. Cost to fix: under 1 hour.
2. Secret handling in Linux path. What: secret env values and PAT URLs can be logged or exposed in command strings. Why deferred: functionality was prioritized. Should it stay deferred: no if any secret-bearing builds run. Cost to fix: 2-4 hours.
3. No bounded native process wrapper in production scripts. What: `vmrun`, `ssh`, `scp`, `git`, and upload steps can hang. Why deferred: simple direct calls are easier. Should it stay deferred: no for `vmrun` lifecycle calls. Cost to fix: 2-3 hours for common helper and core calls.
4. Composite action output structure. What: outputs are under inputs. Why deferred: YAML shape likely not tested end to end. Should it stay deferred: no. Cost to fix: under 30 minutes.
5. Incomplete supply-chain pinning. What: many installer hashes are empty; Ubuntu VMDK lacks hash verification. Why deferred: version churn and convenience. Should it stay deferred: acceptable only for trusted homelab, but pin Git/7-Zip/Ubuntu immediately. Cost to fix: 1-2 hours.
6. Documentation drift. What: README, CI flow, test plan, and TODO disagree. Why deferred: rapid implementation. Should it stay deferred: no, because operators rely on docs during failures. Cost to fix: 2-3 hours.
7. Partial failure diagnostics. What: artifacts/logs not collected on build failure. Why deferred: success path came first. Should it stay deferred: no. Cost to fix: 1-2 hours.
8. Limited integration coverage. What: unit tests do not cover orchestration, workflow YAML, or real VM flows. Why deferred: real VMs are slow. Should it stay deferred: partly; add at least smoke tests. Cost to fix: 4-8 hours.
9. Interactive template refresh. What: Prepare scripts prompt for snapshot actions. Why deferred: manual maintenance is acceptable. Should it stay deferred: yes for homelab, but add optional noninteractive switch. Cost to fix: 1-2 hours.
10. Shared cache trust model. What: writable host cache folders can be poisoned by compromised guests. Why deferred: performance. Should it stay deferred: yes for trusted repos; revisit before untrusted code. Cost to fix: varies, 1 hour to document, more for read-only/cache validation.
## 12. Issues by Severity
### CRITICAL
Issue: Composite action outputs are declared under `inputs`, and `artifact-name` is duplicated.
Location: [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml), middle of metadata block around artifact output declarations.
Impact: The VM build can succeed but `actions/upload-artifact` may receive empty `steps.invoke-ci.outputs.artifact-path` or `artifact-name`, causing workflow failure or missing artifacts.
Fix: Add a top-level `outputs:` block sibling to `inputs:` and `runs:`. Move `artifact-path` and output `artifact-name` there. Keep only one `artifact-name` input.
Issue: Linux extra guest environment secrets are logged in clear text.
Location: [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1), Linux build command assembly and `Write-Host` before `Invoke-SshCommand`.
Impact: Any secret passed via `extra-guest-env-json` can appear in Gitea job logs and local transcripts.
Fix: Never log the expanded command. Log only a redacted command summary and pass secrets through a temporary env file copied over SSH with strict permissions, or base64-encoded stdin decoded on the guest and deleted immediately.
Issue: Linux `-UseGitClone` PAT can be embedded in the SSH command string.
Location: [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1), Linux guest clone branch using `GIT_CLONE_URL`.
Impact: PAT may leak via PowerShell error text, process arguments, SSH command history/audit, or debug logs.
Fix: Use an askpass helper or temporary credential file created on the guest with `chmod 600`, run git with `GIT_ASKPASS` or `http.extraHeader` from a file/stdin, then shred/delete it. Do not embed credential-bearing URLs in command strings.
### HIGH
Issue: `vmrun` lifecycle calls in production scripts have no hard timeout.
Location: [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1), [scripts/Remove-BuildVM.ps1](../scripts/Remove-BuildVM.ps1), [scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1).
Impact: A hung VMware CLI call can consume an act_runner slot indefinitely and prevent cleanup.
Fix: Move `Invoke-VmrunBounded` into [scripts/_Common.psm1](../scripts/_Common.psm1), return exit code/output/timeout flag, and use it for `start`, `stop`, `deleteVM`, `list`, `readVariable`, and `getGuestIPAddress` where practical.
Issue: Stale `vm-start.lock` is not automatically cleaned after host crash.
Location: [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1), [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1), [scripts/Cleanup-OrphanedBuildVMs.ps1](../scripts/Cleanup-OrphanedBuildVMs.ps1).
Impact: New builds wait 10 minutes and fail until manual lock removal.
Fix: On startup cleanup and retention, remove `vm-start.lock` if older than a conservative threshold such as 30 minutes and no `powershell`/act_runner job owns it. Also write lock owner metadata into the lock file.
Issue: Host bootstrap creates stale/incomplete directory structure.
Location: [Setup-Host.ps1](../Setup-Host.ps1), directory creation and runner registration sections.
Impact: Rebuilding the host from scratch misses Linux template dirs, `State`, `keys`, `Cache\pip`, and current template naming; Linux builds and leases can fail later.
Fix: Create `Templates\WinBuild2025`, `Templates\WinBuild2022`, `Templates\LinuxBuild2404`, `State\ip-leases`, `keys`, `Cache\pip`, and register labels matching [runner/config.yaml](../runner/config.yaml).
Issue: Template backup default points to the wrong template path.
Location: [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1), `TemplatePath` default and restore instructions.
Impact: Operator can believe a backup exists while the active WinBuild2025/Linux template was never backed up.
Fix: Change defaults to `F:\CI\Templates\WinBuild2025`, add `-TemplateName` or support backing up all template directories, and update docs.
Issue: Failure path does not collect guest diagnostics before destroying the VM.
Location: [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1), `catch`/`finally` flow.
Impact: Build failures lose guest-side logs, intermediate artifacts, and environment state, making triage harder.
Fix: In `catch`, if VM is running and IP is known, attempt best-effort diagnostic collection to `F:\CI\Artifacts\<jobId>\diagnostics` before teardown.
Issue: Test plan uses PS7-only syntax and wrong parameter names.
Location: [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md), parallel job and e2e command examples.
Impact: Operators following the plan on PS5.1 hit syntax or parameter errors and lose confidence in validation.
Fix: Replace `ForEach-Object -Parallel` with `Start-Job` or sequential/manual commands, and update `-RepositoryUrl` to `-RepoUrl`.
Issue: Shared cache is not wired into the top-level workflow path.
Location: [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1), [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml).
Impact: Documented cache feature is not available from normal workflow usage, so restores/downloads remain slower.
Fix: Add `-UseSharedCache` to `Invoke-CIJob.ps1`, forward it to `Invoke-RemoteBuild.ps1`, and add a composite action input.
### MEDIUM
Issue: `JobId` is not sanitized before path use.
Location: [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1), [scripts/New-BuildVM.ps1](../scripts/New-BuildVM.ps1).
Impact: Unusual workflow IDs or malicious input can create invalid paths or confusing clone names.
Fix: Add `[ValidatePattern('^[A-Za-z0-9_.-]+$')]` or normalize to a safe filename segment.
Issue: Windows readiness only checks TCP 5986, not authenticated WinRM.
Location: [scripts/Wait-VMReady.ps1](../scripts/Wait-VMReady.ps1).
Impact: Phase 4 can pass even if authentication/remoting fails, shifting errors into build phase.
Fix: Optionally accept credentials and perform a tiny `Invoke-Command { 'ready' }` after TCP open for Windows jobs, or document the lighter check.
Issue: Documentation claims partial artifacts are collected on build failure.
Location: [docs/CI-FLOW.md](../docs/CI-FLOW.md), failure scenario table.
Impact: Operator expectation is wrong during failure triage.
Fix: Either implement best-effort failure diagnostics or update the documentation to say artifacts are collected only after successful build packaging.
Issue: `Validate-SetupState.ps1` does not mirror LocalAccounts property compatibility.
Location: [template/Validate-SetupState.ps1](../template/Validate-SetupState.ps1), `PasswordExpires` check.
Impact: Validation can fail on systems where `PasswordNeverExpires` is the available property.
Fix: Copy the compatibility logic from [template/Install-CIToolchain-WinBuild2025.ps1](../template/Install-CIToolchain-WinBuild2025.ps1).
Issue: Ubuntu cloud VMDK download is not hash-verified.
Location: [template/Deploy-LinuxBuild2404.ps1](../template/Deploy-LinuxBuild2404.ps1).
Impact: Corrupt or tampered base image becomes the parent of all Linux builds.
Fix: Add SHA256 parameter or checksum file validation from Ubuntu, fail if mismatch.
Issue: Installer hash pinning is mostly placeholders.
Location: [template/Install-CIToolchain-WinBuild2025.ps1](../template/Install-CIToolchain-WinBuild2025.ps1).
Impact: Supply-chain protection is documented but often not enforced.
Fix: Fill hashes for Python, dotnet-install, VS bootstrapper, 7-Zip, PowerShell, NSIS, CMake, Node, and gh for the pinned versions.
Issue: Event Log sources and task names drift from test plan.
Location: [scripts/Watch-DiskSpace.ps1](../scripts/Watch-DiskSpace.ps1), [scripts/Register-CIScheduledTasks.ps1](../scripts/Register-CIScheduledTasks.ps1), [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md).
Impact: Tests fail even when scripts work.
Fix: Standardize names in docs and scripts: `CI-DiskSpaceAlert`, `CI-DiskAlert`, `CI-RunnerHealth`, etc.
Issue: `_Transport.psm1` disables host key persistence/checking.
Location: [scripts/_Transport.psm1](../scripts/_Transport.psm1).
Impact: SSH MITM protection is off on VMnet8.
Fix: For template/provisioning keep permissive mode if needed; for CI jobs, use `StrictHostKeyChecking=accept-new` and a CI-specific known_hosts file under `F:\CI\State`.
### LOW
Issue: Deprecated runner installer remains likely to drift.
Location: [runner/Install-Runner.ps1](../runner/Install-Runner.ps1).
Impact: A user may run the deprecated path and miss current labels/config.
Fix: Replace body with a hard stop pointing to [Setup-Host.ps1](../Setup-Host.ps1), or update it fully.
Issue: Webhook messages include emoji despite current output preference.
Location: [scripts/Watch-DiskSpace.ps1](../scripts/Watch-DiskSpace.ps1), [scripts/Watch-RunnerHealth.ps1](../scripts/Watch-RunnerHealth.ps1).
Impact: Cosmetic inconsistency with project/user style.
Fix: Use plain prefixes such as `[WARN] CI Disk Alert` and `[ERROR] CI Runner Alert`.
Issue: Architecture diagram has minor indentation/rendering damage.
Location: [docs/ARCHITECTURE.md](../docs/ARCHITECTURE.md), system diagram script box.
Impact: Documentation polish issue.
Fix: Reformat the box so all lines align.
Issue: Several docs still say HTTP/Basic where implementation uses HTTPS.
Location: [template/Validate-DeployState.ps1](../template/Validate-DeployState.ps1), [template/Validate-SetupState.ps1](../template/Validate-SetupState.ps1) docblocks.
Impact: Confusing security documentation.
Fix: Update docblocks to HTTPS/5986 Basic over TLS.
Issue: Benchmark script JSON shape differs from test-plan expectations.
Location: [scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1), [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md).
Impact: Copy-paste parsing commands do not work.
Fix: Either change the JSON schema or update test-plan queries.
### NICE TO HAVE
Issue: No Gitea API runner-online check.
Location: [scripts/Watch-RunnerHealth.ps1](../scripts/Watch-RunnerHealth.ps1).
Impact: Service can be running but disconnected/auth-broken.
Fix: Add optional `-GiteaUrl` and `-TokenTarget` to query runner status.
Issue: No automatic smoke build after template refresh.
Location: operational process, no single file.
Impact: Bad snapshots can be promoted manually.
Fix: Add a `Test-CITemplateSmoke.ps1` that runs a tiny Windows and Linux build before updating runner config.
Issue: No artifact manifest.
Location: [scripts/Get-BuildArtifacts.ps1](../scripts/Get-BuildArtifacts.ps1).
Impact: Harder to compare artifacts across Windows/Linux legs.
Fix: Write `manifest.json` with file names, sizes, hashes, guest OS, commit, and job ID.
Issue: No VM resource override per workflow.
Location: [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1), VMX/template flow.
Impact: Light jobs over-allocate and heavy jobs may be constrained.
Fix: Add optional post-clone VMX patching for CPU/RAM, default off.
Issue: No structured summary printed at job end.
Location: [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1).
Impact: Operators must parse JSONL manually.
Fix: At job end, compute phase durations from in-memory timestamps and print a compact table.
## 13. Quick Wins
Exactly 10 tasks under 1 hour:
1. Fix [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) by moving `artifact-path` and output `artifact-name` to top-level `outputs`.
2. Remove secret values from the Linux `Write-Host "Running build"` line in [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1).
3. Add stale `vm-start.lock` cleanup to [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1).
4. Update [Setup-Host.ps1](../Setup-Host.ps1) to create `State`, `keys`, `Cache\pip`, and current template directories.
5. Change [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1) default from `WinBuild` to `WinBuild2025` or add an `-AllTemplates` mode.
6. Replace `ForEach-Object -Parallel` in [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md) with PS5.1-compatible `Start-Job` examples.
7. Update `-RepositoryUrl` examples to `-RepoUrl` in [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md).
8. Add `JobId` validation to [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) and [scripts/New-BuildVM.ps1](../scripts/New-BuildVM.ps1).
9. Add a pre-clone free-space check to [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1).
10. Remove emoji text from webhook payloads in watcher scripts.
## 14. Roadmap
### Phase 1 - Stabilization (complete before trusting for real builds)
- Fix composite action outputs and run one workflow end to end through artifact upload.
- Redact/fix Linux secret handling for `ExtraGuestEnv` and `-UseGitClone`.
- Add stale `vm-start.lock` cleanup and startup validation.
- Fix stale defaults in [Setup-Host.ps1](../Setup-Host.ps1) and [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1).
- Correct the test plan so every command is PS5.1-compatible and uses current parameter names.
- Add pre-clone disk free-space gate.
- Run one real Windows VM smoke build and one real Linux VM smoke build from the composite action.
### Phase 2 - Daily Operations (comfortable daily use)
- Add best-effort diagnostics collection on build failure.
- Add phase duration summary to job logs.
- Add Gitea API runner-online check as optional runner health mode.
- Add ACL validation script for `F:\CI\keys\ci_linux` and Credential Manager access under the runner identity.
- Add artifact manifest with file hashes.
- Add `UseSharedCache` wiring through `Invoke-CIJob.ps1` and the composite action.
- Add a documented template refresh checklist with backup, validation, smoke build, and rollback.
### Phase 3 - Optimization
- Benchmark `guestinfo.ci-ip` versus `getGuestIPAddress` and update [scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1).
- Tune capacity using real build metrics; keep default 4 unless evidence supports 5 or 6.
- Consider optional per-workflow CPU/RAM overrides for unusually light or heavy jobs.
- Evaluate shared cache hit rates and add cache cleanup/validation.
- Consider a warm clone pool only if measured startup overhead dominates short builds.
### Deferred/Optional (genuinely not needed for homelab)
- Multi-host runner federation.
- Prometheus/Grafana/Loki stack.
- Full PKI for WinRM certificates.
- Full antivirus/firewall hardening while only trusted private code is built.
- Enterprise secret vault integration.
- Kubernetes, ESXi, or container orchestration.
## 15. Architectural Recommendations
Keep the single-host design. It matches the actual constraints and avoids needless complexity. The main architectural improvement should be making the existing local design more deterministic, not replacing it.
Use three small shared contracts:
- A path contract in [AGENTS.md](../AGENTS.md) and [Setup-Host.ps1](../Setup-Host.ps1): exact directories created and owned by the CI system.
- A process contract in [scripts/_Common.psm1](../scripts/_Common.psm1): all native lifecycle calls go through a bounded wrapper.
- A secret contract in [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1): no secret ever appears in command text, logs, URL strings, or artifact manifests.
Do not over-abstract WinRM and SSH into a single generic transport. Keep explicit branches, but share small helper functions. The current clarity is valuable when something fails at 2 AM.
Do not pursue deterministic static IP assignment yet. The current lock-plus-lease design is good enough after stale lock cleanup. Static IP injection into clones or VMware DHCP reservations adds more moving parts than it removes for capacity 4.
Treat templates as release artifacts. Back them up, version snapshots, run a smoke build, then promote by updating runner config. This is the homelab equivalent of a golden image promotion flow, and it can stay simple.
Keep security proportional. For trusted private repositories, disabled Defender/Firewall/UAC is acceptable if documented. But fix secret logging immediately because it is cheap and prevents painful accidents.
## 16. Final Assessment
Completeness: 82/100
Code quality: 78/100
Operational readiness: 72/100
Overall: 77/100
Honest verdict: This is a strong homelab CI/CD system with a sensible architecture and a lot of hard-earned VMware/PowerShell knowledge baked in. The core VM lifecycle is close to daily-use quality, and the Windows/Linux dual-template direction is well chosen. I would trust it for supervised real builds now, but I would not yet leave it fully unattended for daily production-like use until the composite action output bug, Linux secret leakage, stale lock recovery, and setup/backup path drift are fixed. Those are not big redesigns. They are the last tightening pass between an impressive lab build and a reliable daily tool.
plans/gpt55-review-of-opus.md
+231
View File
@@ -0,0 +1,231 @@
# Cross-Review of Opus 4.7 Analysis
Date: 2026-05-11
Reviewer: GPT-5.5, senior DevOps cross-review role
Scope: cross-review of [plans/opus47-analysis.md](opus47-analysis.md) against [plans/gpt55-analysis.md](gpt55-analysis.md) and verified source files in the Local-CI-CD-System workspace.
## 1. Overall Assessment
The Opus 4.7 analysis is broadly useful and technically literate. It understands the system's intended shape: a Windows 11 host running Gitea Actions through act_runner, creating ephemeral VMware Workstation linked clones, running builds over WinRM or SSH, collecting artifacts, and destroying the clone in a `finally` path. Its strongest quality is that it does not try to replace the architecture with an enterprise platform. It recognizes that the single-host, file-state, PowerShell 5.1 design is the right design for this homelab, then focuses most of its critique on validation, concurrency, and operational hygiene.
The strongest insight in Opus is the focus on concurrency as an unproven capability. The review correctly calls out that [runner/config.yaml](../runner/config.yaml#L14-L17) advertises `capacity: 4`, while the implementation depends on a DHCP-sensitive clone/start/IP critical section in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1#L303-L409). The fact that the lock is well designed does not prove it works under four simultaneous real jobs. Opus is right that this is the most important claim to validate before treating the system as unattended-daily-use ready.
The biggest weakness is severity calibration. Opus often identifies the right issue but labels it too high or too low depending on whether it matches the review's narrative. It calls the malformed composite-action outputs a LOW issue even though artifact upload can break the live workflow. It labels the SYSTEM-vs-user SSH alias mismatch HIGH while explicitly admitting it was unverified. It calls the template-refresh runbook absent even though several docs contain partial procedures. It also misses a likely live Linux-matrix bug: the composite action does not map `linux-build` to `GITEA_CI_LINUX_TEMPLATE_PATH`, so the Linux matrix leg can still default to the Windows template unless the caller overrides `template-path` and `guest-os`.
## 2. Technical Accuracy Verification
### CRITICAL: `capacity: 4` is unvalidated
Opus claim: "`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" ([plans/opus47-analysis.md](opus47-analysis.md#L15)). It later says, "Either run the burn-in or set `capacity: 1` until it is done" ([plans/opus47-analysis.md](opus47-analysis.md#L732)).
Verdict: verified correct, with one priority nuance.
The configured capacity is real: [runner/config.yaml](../runner/config.yaml#L14-L17) sets `capacity: 4`. The implementation serializes clone, start, IP discovery, and lease creation using `F:\CI\State\vm-start.lock` and a `FileShare.None` handle in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1#L303-L325). Inside that lock, the script creates the linked clone, starts it, polls `guestVar ci-ip`, falls back to `getGuestIPAddress`, and writes an IP lease file before releasing the lock in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1#L326-L415). The design is sane, but the design is not the same as evidence.
The repo itself supports Opus's concern. TODO says `capacity: 4` is configured but the race was not reproduced in the e2e runs because they were one job at a time ([TODO.md](../TODO.md#L226)). It also notes why `getGuestIPAddress` alone is insufficient for collision detection ([TODO.md](../TODO.md#L231)). So the claim is not speculative; it is aligned with the project notes.
Where I would adjust Opus is the label. I agree it is a top priority, but I would call it HIGH unless the README or operational process currently depends on four-way parallelism for correctness. The system can be operated safely at lower capacity; the failure mode is unvalidated throughput and possible race behavior, not guaranteed data loss. In a final master plan, the task should be: run a PS5.1-compatible four-job burn-in, document results, then either keep `capacity: 4` or temporarily set it to 1 or 2. That is more pragmatic than treating the current setting as an immediate emergency.
### HIGH: act_runner SYSTEM-vs-user SSH alias mismatch
Opus claim: "The alias lives in the user's `~/.ssh/config` - which user? `act_runner` runs as `SYSTEM`... So `Setup-Host.ps1` writes the SSH config to the user's `~/.ssh/config`, not `SYSTEM`'s. HIGH" ([plans/opus47-analysis.md](opus47-analysis.md#L414)). Opus then adds: "Treat as HIGH unverified" ([plans/opus47-analysis.md](opus47-analysis.md#L416)).
Verdict: partially correct and over-prioritized as written.
The code confirms the alias is written under the interactive user's profile: [Setup-Host.ps1](../Setup-Host.ps1#L294-L306) uses `$env:USERPROFILE\.ssh\config`. The live workflow does use the alias: [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml#L31-L35) sets `repo-url: 'ssh://gitea-ci/Simone/nsis-plugin-nsinnounp.git'`. If act_runner runs as LocalSystem, OpenSSH will not read the interactive user's SSH config or keys. That would break host-side clone mode.
The assumption that act_runner definitely runs as SYSTEM is weaker. [scripts/Register-CIScheduledTasks.ps1](../scripts/Register-CIScheduledTasks.ps1#L38-L43) says scheduled maintenance tasks run as SYSTEM; it does not install act_runner. The act_runner NSSM installation in [Setup-Host.ps1](../Setup-Host.ps1#L264-L273) does not set an explicit service account. On Windows, a service installed without an explicit account commonly runs as LocalSystem, so Opus's concern is plausible. But Opus should not have derived act_runner's identity from the scheduled-task script. The correct verification is `Get-CimInstance Win32_Service -Filter "Name='act_runner'" | Select StartName` on the host, plus a test clone under that identity.
I would keep this in the master plan, but classify it as HIGH only until the service identity is confirmed. The fix should be concrete and low ceremony: either configure SSH alias and key under `C:\Windows\System32\config\systemprofile\.ssh`, run the service under the intended user account, or stop relying on the alias and use an explicit SSH URL plus `GIT_SSH_COMMAND`/known key path.
### HIGH: `Validate-DeployState.ps1` missing `ci-report-ip.service` check
Opus claim: "there is no validation in `Validate-DeployState.ps1` that the guest service is enabled in a fresh template" and missing it means Linux jobs fall back to unreliable `getGuestIPAddress` ([plans/opus47-analysis.md](opus47-analysis.md#L135)).
Verdict: partially correct, with important nuance.
The core technical claim is right for `Validate-DeployState.ps1`. That script is Windows-oriented: it connects over WinRM HTTPS/5986 and checks Firewall, Defender, WinRM, UAC, Explorer, Server Manager, AutoAdminLogon, and Windows Update state ([template/Validate-DeployState.ps1](../template/Validate-DeployState.ps1#L42-L151)). There is no Linux branch and no `systemctl is-enabled ci-report-ip.service` check. If someone treats `Validate-DeployState.ps1` as the generic template validation entry point, it will not catch a missing Linux IP-reporting service.
But Opus overstates the absence of validation across the whole repo. The Linux installer creates the service in [template/Install-CIToolchain-Linux2404.sh](../template/Install-CIToolchain-Linux2404.sh#L203-L220) and asserts it is enabled in [template/Install-CIToolchain-Linux2404.sh](../template/Install-CIToolchain-Linux2404.sh#L221). The Linux prepare script also validates both the enabled service and executable script in [template/Prepare-LinuxBuild2404.ps1](../template/Prepare-LinuxBuild2404.ps1#L361-L362). So the service is validated in the Linux provisioning path, just not in the standalone deploy validator that Opus focused on.
The recommendation is still good: add a Linux-aware validation script or extend validation naming so operators cannot confuse a Windows deploy validator with a cross-template final gate. I would call this MEDIUM-HIGH rather than HIGH. It is a real recovery and refresh-risk issue, but not an entirely missing guardrail.
### HIGH: no codified template-refresh runbook
Opus claim: "No documented template-refresh runbook" ([plans/opus47-analysis.md](opus47-analysis.md#L377)) and "KMS re-activation, snapshot rebuild, validation steps live in operator memory" ([plans/opus47-analysis.md](opus47-analysis.md#L516)).
Verdict: partially correct and exaggerated.
There is no single authoritative runbook named "Template Refresh" that covers Windows and Linux with exact commands, preflight checks, backup, validation, smoke build, promotion, and rollback. In that narrow sense, Opus is correct. A final master plan should include one.
However, the process is not only in operator memory. [docs/WINDOWS-TEMPLATE-SETUP.md](../docs/WINDOWS-TEMPLATE-SETUP.md#L280-L289) contains a semestral refresh procedure: boot template, reactivate with `slmgr /ato`, rerun Prepare, shut down, take a versioned snapshot, update `GITEA_CI_SNAPSHOT_NAME`, and keep recent snapshots. [docs/BEST-PRACTICES.md](../docs/BEST-PRACTICES.md#L196-L219) explains BaseClean integrity, backup-before-change, clone checks, and snapshot versioning. [docs/BEST-PRACTICES.md](../docs/BEST-PRACTICES.md#L347-L359) also lists toolchain update steps. [docs/RUNBOOK.md](../docs/RUNBOOK.md#L88-L92) covers missing-snapshot recovery.
The better criticism is fragmentation and drift, not absence. Some docs still refer to old `WinBuild` backup paths ([docs/BEST-PRACTICES.md](../docs/BEST-PRACTICES.md#L207-L211)), while current host docs and runner config use `WinBuild2025` and `LinuxBuild2404` ([docs/HOST-SETUP.md](../docs/HOST-SETUP.md#L160-L163), [runner/config.yaml](../runner/config.yaml#L20-L30)). The master plan should consolidate the procedure into [docs/RUNBOOK.md](../docs/RUNBOOK.md) or a dedicated template-refresh runbook, not invent an enterprise image-management workflow.
### MEDIUM: Linux Mode 2 PAT injection
Opus claim: Linux Mode 2 builds an authenticated URL and stores it in `GIT_CLONE_URL`; Windows uses `http.extraHeader`, which is safer ([plans/opus47-analysis.md](opus47-analysis.md#L123)).
Verdict: verified correct, and slightly understated.
The Linux path rewrites the clone URL with `[uri]::EscapeDataString($pat.UserName)` and the PAT, then exports it as `GIT_CLONE_URL` in the SSH command string ([scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1#L212-L230)). It then runs `git clone "$GIT_CLONE_URL"` and unsets the variable in [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1#L226-L230). That keeps the non-authenticated URL out of the normal log line, but the credential-bearing URL is still part of the command sent through SSH and visible to process/environment inspection on the guest while the command runs.
The Windows path uses `git -c credential.helper= -c http.extraHeader=Authorization: Basic <b64>` inside the WinRM session ([scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1#L323-L334)). That is not perfect because the header can still be visible in process arguments inside the guest, but it avoids embedding credentials in a URL and avoids Git storing the credential URL.
I would keep this as MEDIUM if all builds are trusted and the PAT is scoped narrowly. I would raise it to HIGH if private repositories, signing material, or broader Gitea scopes are used. My own analysis caught an adjacent issue that Opus missed: Linux `ExtraGuestEnv` values are directly included in `$buildCmd`, and the script logs `Write-Host "[Invoke-RemoteBuild] Running build: $buildCmd"` in [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1#L239-L248). That is a direct cleartext secret-log path, stronger than the PAT URL concern.
### MEDIUM: `Measure-CIBenchmark.ps1` uses `getGuestIPAddress`
Opus claim: `Measure-CIBenchmark.ps1` uses `getGuestIPAddress` directly, contradicting the lesson encoded in readiness handling ([plans/opus47-analysis.md](opus47-analysis.md#L122)).
Verdict: verified correct.
The benchmark describes its `ip` phase as `vmrun getGuestIPAddress` in [scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1#L16-L23). The implementation polls `Invoke-Vmrun -Operation getGuestIPAddress` until a dotted IPv4 appears in [scripts/Measure-CIBenchmark.ps1](../scripts/Measure-CIBenchmark.ps1#L129-L146). Production IP detection primarily reads `guestVar 'ci-ip'` in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1#L348-L377) and only then falls back to `getGuestIPAddress` in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1#L379-L389).
This is exactly the kind of benchmark drift that creates false confidence. It is not a production outage by itself, but it means benchmark data cannot be treated as representative of Linux production behavior. MEDIUM is the right severity. The pragmatic fix is to extract IP acquisition into a shared helper and call the same helper from both production and benchmark scripts.
### MEDIUM: hardcoded `N:\` paths in composite action
Opus claim: the composite action hardcodes `$ciScriptsDir = 'N:\Code\Workspace\Local-CI-CD-System\scripts'`, and this couples the action to one host path ([plans/opus47-analysis.md](opus47-analysis.md#L404)).
Verdict: verified correct, and the same pattern appears elsewhere by design.
The composite action hardcodes the script directory in [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml#L148). That makes the action non-portable even though the action is presented as reusable. Opus's suggested `${{ github.action_path }}` approach is directionally right, but the exact relative path needs care because this action lives under [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml), not necessarily `.gitea/actions` in the consuming repository.
There is a second hardcoded `N:\` default in scheduled task registration: [scripts/Register-CIScheduledTasks.ps1](../scripts/Register-CIScheduledTasks.ps1#L39-L43). That one is less concerning because scheduled tasks are host-local maintenance. The composite action is more important because it affects workflow execution and reuse.
MEDIUM is fair for the action. For the scheduled tasks, LOW is enough in this fixed homelab unless the host path is expected to change.
### Composite action outputs
Opus claim: the action emits `artifact-path` and `artifact-name` as step outputs ([plans/opus47-analysis.md](opus47-analysis.md#L399)), but its metadata outputs appear inside `inputs` and should be restructured ([plans/opus47-analysis.md](opus47-analysis.md#L405)).
Verdict: Opus identified the bug but under-rated it.
The action metadata places `artifact-path` and a second `artifact-name` under the `inputs:` map in [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml#L123-L141). There is no top-level `outputs:` block before `runs:`. The step does write outputs to `$env:GITHUB_OUTPUT` in [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml#L224-L225), and upload steps consume those outputs in [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml#L229-L244). Depending on Gitea's composite-action implementation, step outputs may still work inside the same composite, but callers will not have a clean top-level action output contract, and metadata validation is wrong.
My analysis classified this as CRITICAL because artifact upload is core workflow behavior. On reflection, HIGH is a better final-plan severity: it can break artifact publication even when VM build succeeds, but it is a small YAML fix and not a VM lifecycle failure. Opus's LOW is too soft.
## 3. Omissions
Opus missed the direct Linux `ExtraGuestEnv` secret logging path. It discussed the action putting secret JSON into a runner environment variable ([plans/opus47-analysis.md](opus47-analysis.md#L279)), but the stronger issue is in [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1#L239-L248): the Linux path builds `export KEY='value'; ...` and logs the entire command. If `extra-guest-env-json` contains `SIGN_PASS`, the secret can be printed to local transcripts and Gitea job logs. This should outrank the action-env note.
Opus missed setup and backup path drift as an operational risk. [Setup-Host.ps1](../Setup-Host.ps1#L104-L113) still creates `F:\CI\Templates\WinBuild` rather than the current `WinBuild2025`, `WinBuild2022`, and `LinuxBuild2404` directories described in [AGENTS.md](../AGENTS.md#L42-L51) and [runner/config.yaml](../runner/config.yaml#L20-L30). Opus mentions the password documentation mismatch and deprecated runner installer, but not that host rebuild from scratch can miss current directories. My analysis flagged this as HIGH because it affects disaster recovery and new-host setup.
Opus missed stale `vm-start.lock` cleanup. The lock handle is released correctly during normal script flow, as Opus verified. But after a host crash, a stale lock file can still cause confusing 10-minute waits and failure until manual cleanup. [docs/RUNBOOK.md](../docs/RUNBOOK.md#L72-L105) documents manual removal, but neither [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1) nor [scripts/Cleanup-OrphanedBuildVMs.ps1](../scripts/Cleanup-OrphanedBuildVMs.ps1) removes it automatically. This is a very homelab-practical fix.
Opus missed broken or stale test-plan commands that matter because this review is about trust. [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md#L56-L58) uses PS7-only `ForEach-Object -Parallel` and the wrong `-RepositoryUrl` parameter, while [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1#L80) defines `RepoUrl`. That makes the validation plan non-copy-pasteable on the mandated PS5.1 host.
Both analyses missed a likely Linux matrix wiring bug. [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml#L20-L35) runs a matrix over `windows` and `linux`, but it does not pass `guest-os`, `template-path`, or `snapshot-name`. The action defaults `guest-os` to `Auto` in [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml#L52-L57) and leaves `template-path` empty so `Invoke-CIJob.ps1` uses `GITEA_CI_TEMPLATE_PATH` in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1#L114). The runner config has a separate `GITEA_CI_LINUX_TEMPLATE_PATH` in [runner/config.yaml](../runner/config.yaml#L27-L30), but the action does not consume it. Unless act_runner has per-label environment behavior not shown here, the Linux leg can clone the Windows template. This is more urgent than several Opus HIGH items because it affects the advertised Windows/Linux matrix directly.
Both analyses also under-emphasized that Mode 1 host-side cloning with `ssh://gitea-ci/...` depends on host SSH identity, while Mode 2 in-guest clone for Linux should probably use HTTPS reachable from the guest plus PAT or deploy key. The test helper [scripts/Test-E2E-Section3.3.ps1](../scripts/Test-E2E-Section3.3.ps1#L113-L115) distinguishes guest HTTPS and host SSH URLs, but the composite workflow presents one `repo-url` for both matrix legs.
## 4. Over-Engineered or Excessive Suggestions
Opus's static IP pool recommendation is technically coherent but premature. It proposes `ip-pool.json`, static assignment, Windows `netsh` or Linux cloud-init injection, and per-slot locking ([plans/opus47-analysis.md](opus47-analysis.md#L638-L654)). For capacity 4 on a single VMware Workstation host, this adds a lot of moving parts. The current lock-plus-lease model is easy to understand and already avoids simultaneous DHCP start races. First run the burn-in. If it fails because DHCP collisions actually occur, then static IPs are justified. Until then, static IP assignment is a phase-3 redesign, not a stabilization task.
The generic transport interface recommendation also smells like abstraction ahead of need. Opus suggests `Invoke-GuestCommand`, `Copy-GuestItem`, and `Wait-GuestTransportReady` wrappers behind SSH and WinRM ([plans/opus47-analysis.md](opus47-analysis.md#L656-L667)). That is reasonable in a multi-OS product, but this repo has two transports with very different operational failure modes. Explicit branches in [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) are verbose, but they are debuggable. A small shared helper for quoting, secret redaction, and bounded native execution would pay off more than a unified transport abstraction.
Moving JSONL logs into SQLite is useful, but not a prerequisite for reliability. Opus suggests importing job history into `F:\CI\State\history.db` ([plans/opus47-analysis.md](opus47-analysis.md#L674-L677)). A 40-line `Get-CIJobSummary.ps1` over existing JSONL files is enough for a homelab. SQLite can wait until there is enough history and query pain to justify a schema.
The suggestion to switch the project from PS5.1 to PowerShell 7 conflicts with the explicit project constraint. Opus admits this is outside the documented constraints ([plans/opus47-analysis.md](opus47-analysis.md#L688-L700)). For this system, PS5.1 is not accidental. [AGENTS.md](../AGENTS.md#L21-L36) lists PS7-only constructs to avoid, and scripts consistently carry `#Requires -Version 5.1`. A PS7 migration may be pleasant for the author, but it is not a cross-review recommendation that belongs in the master stabilization plan.
The idea to validate templates automatically at the start of each CI job is also heavy. Opus proposes calling validation scripts from the orchestrator with a cached stamp ([plans/opus47-analysis.md](opus47-analysis.md#L704-L716)). That risks turning every build into a template-health workflow and adds failure modes around validation connectivity. A better homelab pattern is: validate on template refresh, then run a smoke build, then promote the snapshot. Do not make every normal build re-prove the template unless a recent incident shows drift.
## 5. Architectural Conflicts
The PowerShell 7 recommendation is the clearest conflict. This project is intentionally PS5.1 because the host is Windows 11 and the scripts are expected to run with built-in Windows PowerShell. Suggesting PS7-only features like `ForEach-Object -Parallel`, `??`, or `Test-Json` undermines the constraint documented in [AGENTS.md](../AGENTS.md#L21-L36) and repeated in the user's request. The right improvement is PS5.1-compatible `Start-Job` for concurrency tests and explicit JSON parsing/validation.
Static IP injection conflicts with the current VMware linked-clone simplicity. The system currently relies on the template snapshot plus runtime DHCP, with guestinfo reporting as a bridge. Static assignment would require modifying guest network state per clone or injecting boot-time config. That is possible, but it creates OS-specific customization paths and more persistent state. It is not invalid, but it should be gated by failed burn-in evidence.
A centralized `F:\CI\State\config.json` for all paths is a mixed recommendation. It could reduce drift, but it also creates another source of truth alongside runner config, script defaults, and docs. The homelab already has [AGENTS.md](../AGENTS.md#L42-L60) as the environment contract and [runner/config.yaml](../runner/config.yaml#L18-L30) as act_runner's environment injection. The master plan should first align existing defaults, not introduce a fourth config surface.
## 6. Incorrect Assumptions
Opus assumes `ci-report-ip.service` is "referenced but not in any file I read" ([plans/opus47-analysis.md](opus47-analysis.md#L133)). That is factually incomplete. The service is created in [template/Install-CIToolchain-Linux2404.sh](../template/Install-CIToolchain-Linux2404.sh#L203-L220), enabled and asserted in [template/Install-CIToolchain-Linux2404.sh](../template/Install-CIToolchain-Linux2404.sh#L221), and checked by Prepare in [template/Prepare-LinuxBuild2404.ps1](../template/Prepare-LinuxBuild2404.ps1#L361-L362). The critique should be about missing generic validation, not missing implementation.
Opus assumes scheduled-task SYSTEM identity proves act_runner SYSTEM identity. [scripts/Register-CIScheduledTasks.ps1](../scripts/Register-CIScheduledTasks.ps1#L38-L43) clearly applies to maintenance tasks, not the NSSM service. The NSSM install in [Setup-Host.ps1](../Setup-Host.ps1#L264-L273) likely defaults to LocalSystem, but that requires service inspection to prove.
Opus states that the action emits `artifact-path` and `artifact-name` as step outputs as a positive finding ([plans/opus47-analysis.md](opus47-analysis.md#L399)), then separately notes the metadata is malformed. The implementation writes to `$env:GITHUB_OUTPUT`, yes, but the action-level output contract is malformed in [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml#L132-L141). Treating the composite action as "complete" is too generous.
Opus says the Linux `GIT_CLONE_URL` method keeps the PAT "never in args" in its surrounding discussion. The credential-bearing URL is still embedded in the remote command string passed to SSH. It may not be in Git's argv as the literal URL if shell expansion happens first, but it is absolutely in the shell environment and in command text sent by the host. That distinction matters.
## 7. Priority Validation
I agree with Opus that concurrency validation belongs near the top. I would set it as HIGH, or CRITICAL only if the project continues to claim production-ready status while advertising `capacity: 4` as proven. The correct action is a burn-in, not a redesign.
I disagree with HIGH for the SSH alias mismatch until service identity is confirmed. It is a plausible live bug and should be verified immediately, but the analysis itself says unverified. Use HIGH-PENDING-VERIFICATION or MEDIUM until `StartName` is known.
I would downgrade the `Validate-DeployState.ps1` Linux service check from HIGH to MEDIUM-HIGH because the Linux prepare/install path already validates the service. The missing piece is a consolidated final gate and clearer naming.
I would upgrade the composite action output bug from LOW to HIGH. It affects artifact publication, which is central to a CI pipeline. The fix is small, but severity is about impact, not effort.
I would upgrade Linux `ExtraGuestEnv` secret logging to HIGH when secrets are used. Opus did not list this specific issue. A secret printed to logs is more concrete than a PAT visible to same-user process inspection.
I would add a new HIGH item: fix Linux matrix template selection. The current action/workflow combination appears not to pass the Linux template path or Linux snapshot name, despite [runner/config.yaml](../runner/config.yaml#L27-L30) defining them. This can make the live matrix fail before reaching the deeper Linux transport concerns.
## 8. Pragmatic Improvements
Replace Opus's "static IP pool" recommendation with "run burn-in first, then decide." The immediate task should be a PS5.1 `Start-Job` harness that launches four real jobs with unique job IDs, records lock wait time, verifies distinct IP leases, and checks all VMs are destroyed. If it passes three times, keep `capacity: 4`. If it fails, then choose between capacity reduction and static IP design.
Replace "template validation at every job start" with "template promotion gate." A homelab-appropriate refresh flow is: stop runner, back up template, update template, run Validate-Setup or Linux validation, shut down cleanly, take a versioned snapshot, run one Windows and one Linux smoke build, update runner config, keep prior snapshot for one week.
Replace the generic transport abstraction with three focused helpers: `Invoke-NativeBounded` for `vmrun`/`ssh`/`scp`, `ConvertTo-SafeShellSingleQuotedString` for Linux command construction, and `Write-CIRedactedCommand` for logs. These address real failure modes without turning the scripts into a framework.
For the SSH alias issue, do not build a broad identity-management system. Add a host check that prints the act_runner service account, tests `ssh -T gitea-ci` as that account if possible, and documents exactly where the SSH config/key must live. If the service runs as LocalSystem, either install the alias under systemprofile or run act_runner under a dedicated `ci_runner` Windows account.
For the action hardcoded path, use a single environment variable such as `GITEA_CI_SCRIPT_ROOT` in [runner/config.yaml](../runner/config.yaml), or compute from `github.action_path` only if the action repository layout makes that reliable. Since the scripts are not inside a downstream repository checkout in every usage model, a runner env var may be simpler and more honest for this homelab.
## 9. Coherence with Homelab Scope
Opus mostly stays within homelab scope in its findings. It does not demand Kubernetes, centralized secret vaults, Prometheus, or ESXi. Its main recommendations are burn-in, smoke testing, small validation checks, cleanup of dead code, and better docs. That is the right posture.
Where it drifts is in the "What Would I Change" section. Static IP pools, generic transport interfaces, SQLite history, JSON schema for action inputs, template validation cached in every job, and a PS7 migration are not crazy ideas, but they are not the next moves for a single-operator VMware Workstation system. They should be explicitly marked as optional future design notes, not allowed to compete with immediate fixes like action outputs, Linux secret logging, template path selection, and service-account verification.
The review's tone is sometimes too commercial-production oriented. It repeatedly contrasts homelab with paid environments, which is fair context, but the final master plan should not optimize for commercial standards. It should optimize for repeatable local recovery, understandable scripts, and low operational surprise.
## 10. Consolidated Recommendation List
Carry forward these Opus recommendations largely unchanged:
1. Run and document a real four-way concurrency burn-in before trusting `capacity: 4`.
2. Add an on-demand end-to-end smoke test, preferably a manual Gitea workflow that runs one minimal Windows and one minimal Linux job.
3. Fix Linux Mode 2 PAT handling so credentials are not embedded in clone URLs.
4. Update `Measure-CIBenchmark.ps1` to use the same IP acquisition path as production.
5. Add or consolidate validation for `ci-report-ip.service` in the Linux template final gate.
6. Remove or hard-stop the deprecated runner installer so operators do not follow stale setup paths.
7. Align `Setup-Host.ps1` password documentation with actual behavior.
8. Fix hardcoded action script-root handling.
Carry forward these Opus recommendations, but modify them:
1. Treat the SYSTEM-vs-user SSH alias as "verify immediately" rather than proven HIGH. After verifying service identity, fix the SSH config/key location or service account.
2. Treat template-refresh docs as fragmented and drifted, not absent. Consolidate them into one runbook with backup, validation, smoke test, promotion, and rollback.
3. Treat static IP allocation as a fallback if burn-in fails, not a default redesign.
4. Keep transport branches explicit. Add small helpers for bounded native calls, quoting, and redaction instead of a full transport abstraction.
5. Add job summaries from JSONL before considering SQLite.
6. Keep PS5.1. Do not migrate to PS7 as part of stabilization.
Drop or defer these Opus recommendations from the master plan:
1. PS7 migration.
2. Per-job automatic template validation.
3. SQLite history database.
4. Full generic transport facade.
5. Static IP pool unless the measured DHCP/guestinfo design fails.
6. Schema-heavy composite action validation beyond basic input checks.
Add these recommendations that Opus missed:
1. Fix the composite action output metadata as HIGH priority: move `artifact-path` and action-level `artifact-name` under a top-level `outputs:` block and remove the duplicate input/output confusion.
2. Fix Linux `ExtraGuestEnv` logging immediately: never print expanded commands containing secret exports.
3. Fix Linux matrix template selection: when `runs-on: linux-build`, pass `guest-os: Linux`, use `GITEA_CI_LINUX_TEMPLATE_PATH`, and use `BaseClean-Linux` unless the Linux template has been aligned to `BaseClean`.
4. Fix [Setup-Host.ps1](../Setup-Host.ps1#L104-L113) directory creation to match current [AGENTS.md](../AGENTS.md#L42-L60) and [runner/config.yaml](../runner/config.yaml#L20-L30).
5. Add stale `vm-start.lock` cleanup to startup/retention maintenance, with conservative age checks and clear logging.
6. Correct [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md#L56-L58) to use PS5.1-compatible concurrency and the actual `-RepoUrl` parameter.
7. Add `JobId` and `ExtraGuestEnv` key validation in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1).
8. Wire `UseSharedCache` through the top-level workflow path only after the correctness fixes above.
Final verdict: Opus is a strong review, and most of its technical instincts are right. It is best used as a source of findings, not as-is as the master plan. The final plan should keep Opus's concurrency and validation focus, correct its overstatements, raise the issues it underplayed, and stay firmly homelab-pragmatic: fix the live workflow contract, prove capacity, prevent secret leaks, align setup paths, and consolidate refresh operations before adding new architecture.
plans/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-nsis.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-nsis.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-nsis.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-nsis.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.*
plans/opus47-review-of-gpt55.md
+421
View File
@@ -0,0 +1,421 @@
# Cross-Review: GPT-5.5 Analysis of Local-CI-CD-System
Reviewer: Claude Opus 4.7, acting as lead architect.
Subject under review: [plans/gpt55-analysis.md](gpt55-analysis.md).
Reference baseline: [plans/opus47-analysis.md](opus47-analysis.md) (my own prior pass).
Style: direct, opinionated, evidence-driven. Source paths quoted are workspace-relative.
---
## 1. Overall Assessment
GPT-5.5's analysis is competent, balanced, and largely accurate. It avoids the two big failure modes I have learned to expect from peer LLM reviews: it does not invent issues, and it does not slide into enterprise-pattern checklist-mongering. The structural decomposition (Executive Summary, Project State, Architecture, Code Quality per file, Security, Reliability, CI/CD, Observability, Operations, Test Coverage, Tech Debt, Issues by Severity, Quick Wins, Roadmap, Architectural Recommendations, Final Assessment) gives a senior reviewer enough surface area to triangulate. The verdict ("strong homelab CI/CD system... not yet fully trustworthy for unattended daily use") matches the evidence in [TODO.md](../TODO.md), [docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md), and the actual scripts.
The strongest insight in the GPT-5.5 review is its per-file Section 4 walkthrough. It catches things my own pass under-emphasised: that [scripts/Set-TemplateSharedFolders.ps1](../scripts/Set-TemplateSharedFolders.ps1) is fine as a tool but is a guest-to-host write boundary that demands caution; that [template/Validate-SetupState.ps1](../template/Validate-SetupState.ps1) makes a `PasswordExpires` assumption that the toolchain installer already handles defensively; that [scripts/Watch-DiskSpace.ps1](../scripts/Watch-DiskSpace.ps1) and [scripts/Watch-RunnerHealth.ps1](../scripts/Watch-RunnerHealth.ps1) still emit emoji glyphs in webhook payloads despite the user's documented no-emoji preference. These are concrete, falsifiable findings rather than abstract suggestions.
The biggest weakness is that GPT-5.5 promotes a documentation/YAML-layout defect to **CRITICAL** (composite action outputs) without distinguishing between (a) the documented action-output surface being broken for downstream callers and (b) the actual artifact upload step at runtime being broken. Those are very different blast radii. The upload step in [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) reads `steps.invoke-ci.outputs.artifact-path` (a step-level output, set explicitly via `Out-File $env:GITHUB_OUTPUT`), so the upload works regardless of the missing top-level `outputs:` block. I expand on this in Section 2. Second weakness: GPT-5.5 entirely misses the `act_runner` SYSTEM-account vs `~/.ssh/config` alias question I flagged as HIGH, which is a runtime-reachable bug. Third weakness: GPT-5.5 does not interrogate the unvalidated `capacity: 4` concurrency claim with the rigour the issue deserves; it briefly notes "probably good enough" without demanding a burn-in.
---
## 2. Technical Accuracy Verification
I verified the GPT-5.5 claims that materially affect the master plan by reading the cited source. Results follow.
### 2.1 CRITICAL: "Composite action outputs are declared under inputs, and artifact-name is duplicated"
**Verdict: Partially correct, severity overstated.**
The YAML structure issue is real. In [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) lines roughly 96-142, the `inputs:` block declares `artifact-name` at line ~96 (with `default: ''`). Then `extra-guest-env-json` ends at line ~133. Without an intervening `outputs:` key, the file directly continues with another `artifact-name:` and an `artifact-path:` — both with `value: ${{ steps.invoke-ci.outputs.* }}` syntax that only makes sense for action outputs. This is two distinct YAML defects:
1. A duplicate `artifact-name` key at the same mapping level, which YAML 1.2 forbids (most parsers silently overwrite with the later definition).
2. Missing `outputs:` header, so what should be action-level outputs sit as malformed inputs.
However, GPT-5.5 escalates this to "VM build can succeed but `actions/upload-artifact` may receive empty `steps.invoke-ci.outputs.artifact-path` or `artifact-name`, causing workflow failure or missing artifacts." That part is wrong. The upload-artifact step explicitly reads `steps.invoke-ci.outputs.artifact-name` and `steps.invoke-ci.outputs.artifact-path`, which are **step-level outputs** set unconditionally in the inner PowerShell block by `"artifact-path=$artifactPath" | Out-File -FilePath $env:GITHUB_OUTPUT -Append -Encoding utf8`. Step-level outputs do not depend on the action having a top-level `outputs:` block. The upload works today.
What the missing `outputs:` block actually breaks is the **caller-facing** action interface: a downstream workflow cannot read `steps.local-ci-build.outputs.artifact-path` from outside this composite action. Since the only consumer today is [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml), which does not consume action outputs (artifact upload happens inside the composite step), the user-visible impact is zero today.
Correct severity: **HIGH** (latent breakage waiting for a second consumer), not **CRITICAL**. The fix is still trivial — sub-30 minutes — so this distinction does not change the quick-wins list. But the framing matters for trust in the report's prioritisation.
### 2.2 CRITICAL: "Linux extra guest environment secrets are logged in clear text"
**Verdict: Correct.**
Verified at [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) line 257:
```powershell
$envPrefix = ''
foreach ($envKey in $ExtraGuestEnv.Keys) {
$envVal = $ExtraGuestEnv[$envKey] -replace "'", "'\''"
$envPrefix += "export $envKey='$envVal'; "
}
...
$buildCmd = "${envPrefix}cd '$GuestLinuxWorkDir' && $BuildCommand"
Write-Host "[Invoke-RemoteBuild] Running build: $buildCmd"
```
The `Write-Host` line emits `$buildCmd`, which is the prefix-plus-command string with secret values inlined as `export KEY='value';`. act_runner captures this stdout and forwards it to Gitea job logs. Gitea masks `secrets.*` by string match, but the masking is brittle (multi-byte characters, base64 transforms, substring boundaries). My own analysis flagged the same line at MEDIUM (§5.5/§6.7). GPT-5.5's CRITICAL is defensible if any signing/credential secret is currently passed through `extra-guest-env-json` — which the workflow example `{"SIGN_PASS":"${{ secrets.SIGN_PASS }}"}` strongly suggests is the intended use case. I agree with the elevation to CRITICAL here.
### 2.3 CRITICAL: "Linux -UseGitClone PAT can be embedded in the SSH command string"
**Verdict: Correct, but my own analysis nailed the mitigation cleaner.**
Verified at [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) lines 226-237:
```powershell
$envCloneCmd = "export GIT_CLONE_URL='$cloneTargetUrl'; git clone --depth 1 --branch '$CloneBranch'"
if ($CloneSubmodules) { $envCloneCmd += ' --recurse-submodules' }
$envCloneCmd += " `"`$GIT_CLONE_URL`" '$GuestLinuxWorkDir'; unset GIT_CLONE_URL"
Invoke-SshCommand -IP $IPAddress -User $SshUser -KeyPath $SshKeyPath -Command $envCloneCmd
```
`$cloneTargetUrl` includes the embedded PAT (`https://user:pat@host/repo.git`). It then appears: in `/proc/<pid>/environ` of the shell during execution; in the host-side `ssh.exe` argv; in any PowerShell error text that captures `$envCloneCmd`. GPT-5.5's fix suggestion ("use an askpass helper or temporary credential file ... with chmod 600, run git with GIT_ASKPASS or http.extraHeader from a file/stdin, then shred/delete") is correct but heavier than needed. My own §5.2 proposed the cleaner fix: mirror the Windows `http.extraHeader=Authorization: Basic <base64>` pattern. Same outcome, less code, single new code path instead of two.
### 2.4 HIGH: "vmrun lifecycle calls in production scripts have no hard timeout"
**Verdict: Correct.**
I cross-checked [scripts/_Common.psm1](../scripts/_Common.psm1)'s `Invoke-Vmrun` function: it wraps the native call and inspects `$LASTEXITCODE`, but there is no `Wait-Process -Timeout` or `Stop-Process` watchdog. [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) and [scripts/Remove-BuildVM.ps1](../scripts/Remove-BuildVM.ps1) call `vmrun` through this helper and inherit the unbounded behaviour. A `vmrun` that hangs (e.g. waiting on a non-responsive `vmware-vmx.exe`) blocks the orchestrator until act_runner's job-level timeout. The fix GPT-5.5 proposes — move `Invoke-VmrunBounded` (already present in some deploy scripts) into `_Common.psm1` — is correct. My own analysis did not call this out separately; GPT-5.5 catches a real gap I missed.
### 2.5 HIGH: "Stale vm-start.lock is not automatically cleaned after host crash"
**Verdict: Correct and important.**
Cross-checked: [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1) clears `ip-leases` older than 12 hours but does not touch `F:\CI\State\vm-start.lock`. [scripts/Cleanup-OrphanedBuildVMs.ps1](../scripts/Cleanup-OrphanedBuildVMs.ps1) cleans clone dirs but not the lock file. A host crash mid-lock leaves the file on disk. On reboot, [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1)'s file-handle acquisition still works (the kernel released the OS-level lock), but the file still exists, so the cosmetic stale-file warning may misfire and the 10-minute retry semantics give a bad first-job-after-reboot experience. The fix is sound. My own analysis incorrectly concluded "the handle is closed correctly on every path... OK" (§6.1) and overlooked the post-crash file-existence concern. GPT-5.5 beats me here.
### 2.6 HIGH: "Host bootstrap creates stale/incomplete directory structure"
**Verdict: Correct.**
Verified at [Setup-Host.ps1](../Setup-Host.ps1) lines 118-126:
```powershell
$dirs = @(
"$CIRoot\BuildVMs",
"$CIRoot\Artifacts",
"$CIRoot\Logs",
"$CIRoot\Templates\WinBuild",
"$CIRoot\act_runner\logs",
"$CIRoot\Cache\NuGet",
"$CIRoot\RunnerWork",
"$CIRoot\ISO"
)
```
It creates `Templates\WinBuild` (the now-obsolete unified template name) but not `Templates\WinBuild2025`, `Templates\WinBuild2022`, or `Templates\LinuxBuild2404`. It does not create `State`, `State\ip-leases`, `keys`, or `Cache\pip`. The current orchestrator depends on `F:\CI\State\vm-start.lock` and `F:\CI\State\ip-leases\*.lease`; the Linux path depends on `F:\CI\keys\ci_linux`. A fresh host bootstrap is broken. My own analysis did not catch this with the same specificity (I noted the password documentation mismatch but not the directory list). GPT-5.5 wins this one cleanly.
### 2.7 HIGH: "Template backup default points to the wrong template path"
**Verdict: Correct.**
Verified at [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1) line 48: `[string] $TemplatePath = 'F:\CI\Templates\WinBuild'`. With three template families now in production (WinBuild2025, WinBuild2022, LinuxBuild2404), the default points at a directory that either does not exist or is the wrong one. An operator running the default-parameter backup before a template refresh will silently back up nothing useful, then refresh the real template, then have no rollback. This is an operational time bomb. GPT-5.5 ranks it HIGH; I concur. My own analysis flagged the script's existence as Complete in Section 2 without auditing the default — I missed the path drift.
### 2.8 HIGH: "Failure path does not collect guest diagnostics before destroying the VM"
**Verdict: Correct in fact, debatable in priority.**
[scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) enters its `catch`/`finally` and proceeds directly to `Remove-BuildVM.ps1` without attempting partial log/artifact collection from the running guest. This contradicts the failure-scenario table in [docs/CI-FLOW.md](../docs/CI-FLOW.md). However, GPT-5.5 ranks this HIGH, equal with the secret-leak issues. I would put it MEDIUM at most. For a single-operator homelab, when a build fails and the VM is destroyed, the JSONL phase events + the `Write-Host` transcript in `F:\CI\Logs\$JobId\invoke-ci.log` already capture more failure context than most commercial CI systems retain. The missing "best-effort guest log dump" is a quality-of-life improvement, not a HIGH operational issue.
### 2.9 HIGH: "Test plan uses PS7-only syntax and wrong parameter names"
**Verdict: Correct but already-known.**
[docs/TEST-PLAN-v1.3-to-HEAD.md](../docs/TEST-PLAN-v1.3-to-HEAD.md) does contain `ForEach-Object -Parallel` and `-RepositoryUrl` (the real param is `-RepoUrl`). This is documentation-only drift, not production breakage. GPT-5.5 ranks HIGH; I would put MEDIUM. The bar for HIGH should be "actively breaks production or compromises trust at runtime". A wrong-parameter-name in a test plan breaks the operator's afternoon, not the system. Quick-wins #6 and #7 in the GPT-5.5 list correctly call out the fix; the severity rating is the only quibble.
### 2.10 HIGH: "Shared cache is not wired into the top-level workflow path"
**Verdict: Correct.**
[gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml) does not expose `use-shared-cache`. [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) does not forward a `UseSharedCache` parameter to `Invoke-RemoteBuild.ps1`. The HGFS shared-folder mechanism exists at the lower layer (per [scripts/Set-TemplateSharedFolders.ps1](../scripts/Set-TemplateSharedFolders.ps1)), so the feature can be wired in by adding one input and one parameter pass-through. My own analysis listed shared HGFS caches as a security concern (§5.4 cache poisoning) but did not call out the wiring gap. GPT-5.5 has the operational insight I missed.
### 2.11 MEDIUM: "Windows readiness only checks TCP 5986, not authenticated WinRM"
**Verdict: Correct.**
[scripts/Wait-VMReady.ps1](../scripts/Wait-VMReady.ps1) confirms ready when `Test-NetConnection -Port 5986` returns true. WinRM listener can be bound and accepting TCP before HTTPS handshake / Basic auth is fully operational, particularly on a cold Windows guest where `WinRM service` starts before TrustedHosts and certificate bindings finalise. The fix (optional `Invoke-Command { 'ready' }` post-TCP-open) is correct. I did not catch this in my own analysis, which is a clear miss on my side.
### 2.12 MEDIUM: "Watch-DiskSpace.ps1 webhook payload includes emoji"
**Verdict: Correct.**
Verified at [scripts/Watch-DiskSpace.ps1](../scripts/Watch-DiskSpace.ps1) line ~95: `$body = @{ content = ":warning: **CI Disk Alert** — $msg" }`. The Discord `:warning:` shortcode renders as the warning glyph in the Discord client. The user's documented preference (in memory) is no emoji. GPT-5.5 catches it; my own analysis did not.
### 2.13 MEDIUM: "Ubuntu cloud VMDK download is not hash-verified"
**Verdict: Correct.**
[template/Deploy-LinuxBuild2404.ps1](../template/Deploy-LinuxBuild2404.ps1) downloads the Ubuntu cloud image without SHA256 verification. A man-in-the-middle at template-build time (or a compromised mirror) substitutes a malicious base, and every Linux build VM inherits that compromise. My own analysis covered SHA256 pinning for **toolchain** installers (§5.7/§6.8) but did not call out the **base image** as a separate supply-chain channel. GPT-5.5 makes the correct distinction.
### 2.14 LOW: "Deprecated runner installer remains likely to drift"
**Verdict: Correct, both analyses concur.**
[runner/Install-Runner.ps1](../runner/Install-Runner.ps1) is marked deprecated but present. Both reviewers flag it. Action: delete.
### 2.15 Summary of verification
Out of 18 verifiable factual claims I cross-checked, 16 are correct, 1 is partially correct with overstated severity (composite action outputs), and 1 is overstated in priority (failure-path diagnostics). That is a high accuracy rate. GPT-5.5 is a credible technical reviewer for this codebase.
---
## 3. Omissions
What GPT-5.5 missed that my own analysis caught:
1. **`act_runner` service-account vs SSH alias mismatch** (my §8.2 HIGH). If [Setup-Host.ps1](../Setup-Host.ps1) installs `act_runner` as a Windows service running under `SYSTEM`, the `~/.ssh/config` alias `gitea-ci` referenced in [gitea/workflows/build-nsis.yml](../gitea/workflows/build-nsis.yml) (`repo-url: 'ssh://gitea-ci/Simone/...'`) lives in the *interactive user's* `~/.ssh/config`, not `SYSTEM`'s. This is a runtime-reachable bug: every Linux Mode 2 clone fails with "unknown host gitea-ci" until the alias is reinstalled in `C:\Windows\System32\config\systemprofile\.ssh\config`. GPT-5.5 never mentions this. It is potentially HIGH-severity and definitely worth a verification step.
2. **`Measure-CIBenchmark.ps1` uses `getGuestIPAddress` instead of `guestVar ci-ip`** (my §3.3, §11.3). GPT-5.5 mentions this almost as a footnote ("It still uses `getGuestIPAddress` rather than the newer `guestinfo.ci-ip` path"), but does not connect it to the larger issue: the benchmark measures a different IP-discovery path than production, so benchmark results do not represent real timing. This makes capacity-planning data unreliable.
3. **`Validate-DeployState.ps1` does not assert `ci-report-ip.service` presence on Linux templates** (my §3.5, §6 issue list HIGH). The systemd unit writing `guestinfo.ci-ip` is the primary IP-discovery channel for Linux. If a template refresh forgets the unit, every Linux job silently falls back to the 120-second `getGuestIPAddress` timeout. GPT-5.5 does not mention this dependency at all.
4. **`capacity: 4` is unvalidated** as a first-class CRITICAL issue (my §1 top priority, §6.1). GPT-5.5 mentions "For capacity 4, this is probably reliable enough after one fix: automatic stale lock cleanup," which is too easy. Whether four parallel `Invoke-CIJob.ps1` invocations actually behave correctly — under DHCP lease contention, under simultaneous `vmrun start`, under the cumulative ~6-minute pre-build serialization — has not been measured. The honest engineering position is: either run the burn-in or set `capacity: 1` until validated. GPT-5.5 lets this slide.
5. **Hardcoded `N:\Code\Workspace\Local-CI-CD-System\scripts` in [gitea/actions/local-ci-build/action.yml](../gitea/actions/local-ci-build/action.yml)** (my §11.4, §13 quick win #4). GPT-5.5 notes path drift in setup/backup but misses that the composite action itself contains a hardcoded clone path. Move the host repo and the action breaks.
6. **PS 7 strategic question** (my §15.8). My analysis raises this and explicitly counter-argues that the PS 5.1 mandate is the right homelab choice today. GPT-5.5 takes PS 5.1 as a fixed constraint and never considers whether the constraint deserves revisiting. This is a stylistic difference, not a defect, but the "what would I change" section in my own analysis is a strategic-thinking layer GPT-5.5 omits entirely.
7. **The 4-minute pre-build serialization under capacity: 4** (my §3.3). At ~60-90 seconds per IP-acquire phase, four concurrent jobs serialize ~6 minutes of pre-build wait for the last one. The build phase is parallel after that. GPT-5.5 does not work this math.
What both of us missed:
1. **VMware shared-folder write semantics in detail**. Both analyses flag HGFS cache poisoning at MEDIUM. Neither verifies which specific shared folders are configured in the template VMX and whether the NuGet/pip cache directories are writable from the guest by default. The actual remediation depends on those specifics.
2. **Gitea Actions secret masking under base64/hex transforms**. Neither analysis quantifies the masking limitation. If a workflow author base64-encodes a secret before passing it through `extra-guest-env-json`, Gitea's string-match masking misses the encoded value in logs. Worth a one-liner in security docs.
3. **`vmrun` invocation under simultaneous PowerShell processes**. Both of us trust that `vmrun.exe` is process-safe. I have not seen a documented assertion of this, and the VMware Workstation product is historically single-user single-instance. Under capacity 4, four PowerShell processes each invoke `vmrun start` simultaneously. The IP lock serializes the user-visible flow, but native-tool concurrency safety is an unstated assumption.
4. **Network namespace / VMnet8 saturation under load**. With four concurrent Windows builds each downloading NuGet packages and four concurrent Linux builds each running `apt update`, the NAT outbound bandwidth is a single shared resource. No analysis mentions this.
5. **`act_runner` log rotation**. Both of us treat act_runner's own log file as a fire-and-forget concern. On a multi-month homelab, that file grows. Neither analysis mentions rotation.
---
## 4. Over-Engineered or Excessive Suggestions
The GPT-5.5 list is generally restrained. The suggestions that drift toward over-engineering:
1. **"Use an askpass helper or temporary credential file created on the guest with chmod 600, run git with GIT_ASKPASS or http.extraHeader from a file/stdin, then shred/delete it"** (CRITICAL #3 Linux PAT fix). This is a four-moving-parts mitigation when a one-line `http.extraHeader=Authorization: Basic <base64>` (mirroring the existing Windows path) gives equivalent or better security. Smell: enterprise-pattern over-think. The simpler fix has fewer failure modes.
2. **"Add lock owner metadata into the lock file"** (HIGH stale vm-start.lock fix). The orchestrator is single-instance per host. The lock file's existence is sufficient signal; adding JSON metadata (PID, hostname, started-at) is a small but real complication that pays back only if the system grows multi-host. For homelab scope, just delete the file if older than 30 minutes. The PID-based liveness check is enterprise-runtime territory.
3. **"Per-phase duration summary, free disk before and after job, clone directory size at teardown, VM start lock wait time, guest IP detection method, cleanup-failure count, Gitea runner online API status"** (Section 8 missing metrics). This is the full nine yards of observability. For a homelab, the existing JSONL + Event Log + webhook is already more than most commercial CI has. The summary table at job-end (Quick Win in my analysis, also in GPT-5.5's NICE-TO-HAVE) is the highest-value subset. The rest is metrics-for-metrics'-sake.
4. **"Standardize names in docs and scripts: CI-DiskSpaceAlert, CI-DiskAlert, CI-RunnerHealth"** (MEDIUM Event Log sources). GPT-5.5 lists three target names that are themselves inconsistent (`CI-DiskSpaceAlert` and `CI-DiskAlert` in the same sentence). The right fix is to pick one and propagate; the request shape is correct, the example list contradicts itself.
5. **"Add optional `-GiteaUrl` and `-TokenTarget` to query runner status"** (NICE-TO-HAVE Gitea API check). The PAT used for API queries needs to be stored in Credential Manager, the API endpoint can change between Gitea versions, and the value-add over local service status is incremental. This is a reasonable second-phase improvement; calling it out at all in a "homelab" review is borderline.
6. **"Treat templates as release artifacts. Back them up, version snapshots, run a smoke build, then promote by updating runner config"** (Section 15 architectural recommendation). The shape is correct but the language drifts toward release-engineering vocabulary that does not match homelab scope. The practical translation — `Backup-CITemplate.ps1` → refresh → `Test-CITemplateSmoke.ps1` → update `runner/config.yaml` — is fine, but does not need the "release artifacts" framing.
7. **"Pin Git/7-Zip/Ubuntu immediately"** (MEDIUM hash pinning). For a private homelab consuming only the original vendor URLs over TLS, the probability of a successful supply-chain substitution is genuinely low. Time-to-pin is non-trivial (correct SHA256 must be tracked per installer version, which then becomes an upgrade-blocker). For a homelab, accepting unverified TLS-fetched installers from `python.org`, `7-zip.org`, `microsoft.com` is defensible. The "immediately" elevates urgency beyond what threat-modelling supports.
None of these are catastrophically over-engineered. GPT-5.5 stays mostly in scope. But the cumulative drift is real: a master plan that adopts every GPT-5.5 suggestion uncritically will accumulate enterprise-shaped barnacles.
---
## 5. Architectural Conflicts
GPT-5.5's recommendations are mostly consistent with system constraints. Two minor conflicts:
1. **"Move bounded native process execution, IPv4 validation, and stale state cleanup into `_Common.psm1`"** (§3 architecture). The directive is sound, but `Invoke-VmrunBounded` as it exists in some deploy scripts uses `Wait-Process -Timeout` plus `Stop-Process`. On PS 5.1 / Windows PowerShell, `Wait-Process -Timeout` is reliable, but the `Stop-Process` afterwards does not guarantee `vmrun.exe`'s child `vmware-vmx.exe` process is cleaned up. The architectural fix is correct in spirit but glosses over the cleanup semantics of native VMware processes. A complete implementation would also locate the child PID and force-terminate. GPT-5.5 does not flag this nuance.
2. **"Use `StrictHostKeyChecking=accept-new` and a CI-specific known_hosts file under `F:\CI\State`"** (MEDIUM SSH host-key handling). The directive is correct for CI jobs. But `accept-new` requires OpenSSH 7.6+; the Windows 11 built-in OpenSSH client is OpenSSH for Windows 8.x+, which supports it. PS 5.1 itself does not constrain this. On VMnet8 with template-controlled host keys, this is fine. No real conflict, but worth noting that the alternative `UserKnownHostsFile=F:\CI\State\known_hosts` with `StrictHostKeyChecking=yes` plus a known-hosts seed step at template-prepare time is the stricter option.
The other GPT-5.5 recommendations respect PS 5.1, VMware Workstation single-host topology, and the headless-host model.
---
## 6. Incorrect Assumptions
1. **"The current orchestrator now primarily reads `guestinfo.ci-ip` and uses `getGuestIPAddress` as fallback"** (Section 2). This is technically correct, but GPT-5.5 does not verify the guest-side dependency: a `ci-report-ip.service` (or equivalent on Windows) must exist in the template to publish `guestinfo.ci-ip`. Section 6 of my own analysis flags this; GPT-5.5 takes the channel for granted.
2. **"Cleanup happens after `Invoke-RemoteBuild.ps1` success only"** (Section 2 inconsistency note about docs vs implementation). GPT-5.5 conflates "artifact collection happens only after successful build" with "cleanup happens only after success." The cleanup (Remove-BuildVM) runs in `finally`, so it runs whether build succeeded or failed. The doc/reality drift is only about artifact collection, not cleanup. Minor.
3. **"For capacity 4, [the IP lock is] probably reliable enough after one fix: automatic stale lock cleanup"** (Section 3). The assertion "probably reliable enough" is unsupported. Whether DHCP collisions occur under four-way contention, whether `vmrun start` is concurrency-safe, whether the 120-second IP-acquire deadline holds when host CPU is contended — none of this has been measured. The correct stance is "unknown until burn-in."
4. **"`scripts/_Transport.psm1` covers SSH helpers only, while `scripts/_Common.psm1` covers WinRM session options and vmrun. This is acceptable..."** (Section 3). This naming is slightly misleading. `_Transport.psm1` reads as a generic transport abstraction but contains only SSH helpers. The name is asymmetric with `_Common.psm1`'s WinRM helpers. GPT-5.5 calls this out but then accepts it. I find the asymmetry more bothersome — the symmetric option would be either `_SshTransport.psm1` + `_WinrmTransport.psm1` or a single `_Transport.psm1` covering both. Style preference, not a defect.
5. **"VS Build Tools 2026 (MSBuild 18.5 / v145), .NET SDK 10"** (cited in [AGENTS.md](../AGENTS.md) and implicitly in GPT-5.5's Windows toolchain section). These version numbers correspond to future/unreleased Microsoft products as of the workspace's stated date. GPT-5.5 takes them at face value without questioning whether the project's stated environment dates and the actual installer URLs in [template/Install-CIToolchain-WinBuild2025.ps1](../template/Install-CIToolchain-WinBuild2025.ps1) are consistent. Probably out of scope, but a careful reviewer should at least note this.
6. **"Composite action correctly passes most inputs through env vars to reduce shell injection risk"** (Section 5). Correct. But GPT-5.5 then says "`extra-guest-env-json` itself is an env var containing secrets; ensure act_runner does not echo it." act_runner does not echo env vars by default. The risk is the downstream usage inside [scripts/Invoke-RemoteBuild.ps1](../scripts/Invoke-RemoteBuild.ps1) (already flagged), not act_runner itself. Minor framing slip.
---
## 7. Priority Validation
| GPT-5.5 Severity | Item | My Verdict | Rationale |
| --- | --- | --- | --- |
| CRITICAL | Composite action outputs structure | Should be HIGH | Step-level outputs work today; only caller-facing action interface is broken. Latent, not active. |
| CRITICAL | Linux extra guest env secret logging | CRITICAL (concur) | Active leak path. |
| CRITICAL | Linux Mode 2 PAT in SSH command | HIGH-CRITICAL boundary | PAT visibility limited to host argv + guest /proc. Real but bounded. I would put HIGH; CRITICAL is defensible. |
| HIGH | vmrun unbounded calls | HIGH (concur) | |
| HIGH | Stale vm-start.lock | HIGH (concur) | |
| HIGH | Setup-Host stale dirs | HIGH (concur) | |
| HIGH | Backup default path | HIGH (concur) | |
| HIGH | Failure-path diagnostics | Should be MEDIUM | Quality-of-life, not operational-trust. |
| HIGH | Test plan PS7 / wrong params | Should be MEDIUM | Documentation-only. |
| HIGH | Shared cache not wired | Should be MEDIUM | Documented feature missing, performance impact only. |
| MEDIUM | JobId not sanitized | Should be LOW | Local-only attack surface in single-tenant homelab. |
| MEDIUM | Wait-VMReady TCP-only check | MEDIUM (concur) | |
| MEDIUM | CI-FLOW doc claims partial artifacts | Should be LOW | Pure docs. |
| MEDIUM | Validate-SetupState PasswordExpires | MEDIUM (concur) | Validation failure mode. |
| MEDIUM | Ubuntu VMDK no hash | MEDIUM (concur) | |
| MEDIUM | Installer hash placeholders | MEDIUM (concur for homelab) | |
| MEDIUM | Event Log / task name drift | Should be LOW | Tests fail not scripts. |
| MEDIUM | _Transport.psm1 host-key handling | MEDIUM (concur) | |
Items I would add at HIGH that GPT-5.5 missed: `act_runner` SYSTEM-vs-user SSH alias mismatch (verifiable, runtime-reachable), `ci-report-ip.service` missing from `Validate-DeployState.ps1` (silent 120 s timeout), `capacity: 4` burn-in not run (concurrency claim unmeasured).
Items I would demote from CRITICAL: composite action outputs (real but contained to caller interface only).
---
## 8. Pragmatic Improvements
GPT-5.5's recommendations, recast for homelab-pragmatic adoption:
1. **Composite action outputs**: Move both `artifact-path` and `artifact-name` to a top-level `outputs:` block and remove the duplicate `artifact-name` declaration. Keep the inner `Out-File $env:GITHUB_OUTPUT` lines; they continue to drive step-level outputs that the upload step depends on. 20 minutes.
2. **Linux secret redaction**: Replace `Write-Host "[Invoke-RemoteBuild] Running build: $buildCmd"` with `Write-Host "[Invoke-RemoteBuild] Running build (env vars redacted): cd '$GuestLinuxWorkDir' && <build-command>"`. Do NOT log the env prefix at all. Move from CRITICAL fix to one-line change. 5 minutes.
3. **Linux PAT injection**: Mirror Windows `http.extraHeader=Authorization: Basic <base64>` via `git -c http.extraHeader=...`. Do not pursue askpass helper. 30 minutes including a test.
4. **vmrun bounded wrapper**: Move `Invoke-VmrunBounded` into `_Common.psm1`, use for `start`, `stop`, `deleteVM`. Skip `list`, `readVariable`, `getGuestIPAddress` — those are short-running calls and the wrapper adds overhead. Bounded scope. 45 minutes.
5. **Stale lock cleanup**: Add a 5-line block to [scripts/Invoke-RetentionPolicy.ps1](../scripts/Invoke-RetentionPolicy.ps1) and to a startup step in [scripts/Cleanup-OrphanedBuildVMs.ps1](../scripts/Cleanup-OrphanedBuildVMs.ps1) that removes `F:\CI\State\vm-start.lock` if older than 30 minutes. No metadata inside the lock file. 15 minutes.
6. **Setup-Host directory list**: Update [Setup-Host.ps1](../Setup-Host.ps1) line 118 directory array to include `Templates\WinBuild2025`, `Templates\WinBuild2022`, `Templates\LinuxBuild2404`, `State`, `State\ip-leases`, `keys`, `Cache\pip`. Remove `Templates\WinBuild`. 5 minutes.
7. **Backup default**: Change [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1) default to `'F:\CI\Templates\WinBuild2025'`. Add a `-AllTemplates` switch that iterates all `Templates\*` directories. 20 minutes.
8. **Test plan**: Replace `ForEach-Object -Parallel` with `1..4 | ForEach-Object { Start-Job -ScriptBlock { ... } } | Wait-Job | Receive-Job`. Replace `-RepositoryUrl` with `-RepoUrl`. Pure text edit. 15 minutes.
9. **JobId validation**: Add `[ValidatePattern('^[A-Za-z0-9._-]+$')]` to JobId parameter in [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1) and [scripts/New-BuildVM.ps1](../scripts/New-BuildVM.ps1). 5 minutes.
10. **Pre-clone disk check**: Add to Phase 2 of [scripts/Invoke-CIJob.ps1](../scripts/Invoke-CIJob.ps1): `if ((Get-PSDrive F).Free / 1GB -lt 20) { throw "Insufficient disk space" }`. 5 minutes.
11. **Webhook emoji removal**: Replace `:warning:` and similar in [scripts/Watch-DiskSpace.ps1](../scripts/Watch-DiskSpace.ps1) and [scripts/Watch-RunnerHealth.ps1](../scripts/Watch-RunnerHealth.ps1) with `[WARN]` and `[ERROR]`. 5 minutes.
What GPT-5.5 proposes that I would defer or drop:
- Lock-owner metadata in lock file (over-engineered for single-host).
- Gitea API runner online check (acceptable as deferred).
- Per-build VMX CPU/RAM override (defer; no demand signal yet).
- Artifact manifest with hashes (NICE-TO-HAVE for homelab).
---
## 9. Coherence with Homelab Scope
GPT-5.5 generally stays in homelab scope. The phrasing in a few places drifts toward enterprise language ("release artifacts," "golden image promotion flow," "path contract / process contract / secret contract"), but the concrete actions remain proportional. The "Deferred/Optional" list correctly excludes multi-host runner federation, Prometheus/Grafana/Loki, full PKI, enterprise secret vaults, and Kubernetes/ESXi. That is the right scope discipline.
Where GPT-5.5 over-extends:
- The Section 5 enumeration of "what a compromised build VM could reach" reads as enterprise threat-modelling for a single-tenant lab. The threat model is "the operator runs only their own private code." Listing five attack vectors is exhaustive but past the point of useful for the operator's actual risk picture.
- The Section 8 "missing metrics and alerting" list of seven specific metrics implies a metrics-stack mindset. For homelab, "phase duration printed at job end" subsumes 80% of the value of that list.
- The Section 15 trio of "path contract / process contract / secret contract" reads as architecture-document framing. For a single-operator homelab, the same content fits in three sentences in [AGENTS.md](../AGENTS.md).
Where GPT-5.5 stays correctly proportional:
- "Keep the single-host design." Correct.
- "Do not pursue deterministic static IP assignment yet." Correct, and notably more conservative than my own §15.1 which proposes a static-IP pool. GPT-5.5 is right here: the existing lock + lease design after stale-lock cleanup is good enough at this capacity.
- "Keep security proportional." Correct.
- "Do not over-abstract WinRM and SSH into a single generic transport." Correct. My own §15.2 proposes generic transport abstraction; GPT-5.5 explicitly counter-argues that "the current clarity is valuable when something fails at 2 AM." GPT-5.5 wins this debate.
---
## 10. Consolidated Recommendation List
After this review, the following GPT-5.5 recommendations should make it into the master plan unchanged:
**Phase 1 — Stabilization (adopt as-is):**
- Fix composite action outputs (with corrected severity, HIGH not CRITICAL).
- Redact Linux secret logging (concur CRITICAL).
- Fix Linux Mode 2 PAT to use `http.extraHeader` (cleaner than GPT-5.5's askpass proposal).
- Add stale `vm-start.lock` cleanup (concur HIGH).
- Fix [Setup-Host.ps1](../Setup-Host.ps1) directory list (concur HIGH).
- Fix [scripts/Backup-CITemplate.ps1](../scripts/Backup-CITemplate.ps1) default path (concur HIGH).
- Pre-clone disk space gate (concur).
- Test plan PS5.1 + parameter name corrections (demoted to MEDIUM).
- Real Windows + Linux VM smoke build via composite action (concur).
**Phase 2 — Adopt with modification:**
- vmrun bounded wrapper: scope to `start`/`stop`/`deleteVM`, not all native calls.
- Failure-path diagnostics: implement as a single `Get-GuestDiagnostics` helper, MEDIUM not HIGH.
- Shared cache wiring through composite action: concur, add as Phase 2.
- Event Log / task name standardization: pick one canonical set, propagate.
- Phase duration summary at job end: concur (lighter than full metrics layer).
**Adopt at LOW priority:**
- Webhook emoji removal.
- Delete deprecated [runner/Install-Runner.ps1](../runner/Install-Runner.ps1).
- Docblock HTTPS/5986 corrections in Validate-* scripts.
- Benchmark JSON shape vs test plan.
**Drop or defer indefinitely:**
- Lock-owner metadata inside lock file.
- Gitea API runner online check.
- Per-build VMX CPU/RAM override.
- Artifact manifest with hashes.
- Full SHA256 pinning for installers (acceptable as deferred for homelab).
- Smoke build automation as a separate `Test-CITemplateSmoke.ps1` (combine with capacity burn-in instead).
**Add from my own analysis (GPT-5.5 missed these):**
- `act_runner` service-account vs SSH alias mismatch verification (HIGH).
- `ci-report-ip.service` presence check in [template/Validate-DeployState.ps1](../template/Validate-DeployState.ps1) (HIGH).
- `capacity: 4` burn-in or downgrade (CRITICAL).
- Hardcoded `N:\Code\Workspace\...` in composite action (MEDIUM).
- `Measure-CIBenchmark.ps1` use of `getGuestIPAddress` aligned with production path (MEDIUM).
---
## 11. Comparison with My Own Analysis
Where GPT-5.5 outperforms my analysis:
1. **Per-file scrutiny in Section 4**. GPT-5.5 walks 20+ files explicitly and catches small defects (PasswordExpires assumption in `Validate-SetupState.ps1`, `Measure-CIBenchmark.ps1` JSON schema vs test plan, emoji in webhook payloads, `Watch-DiskSpace.ps1` Event Log source name drift). My analysis is structured around themes; I missed those concrete file-level facts.
2. **Stale vm-start.lock impact on first-job-after-reboot UX**. My §6.1 verified the lock-handle path is correct on graceful exit but missed the file-existence-after-crash concern. GPT-5.5 makes this explicit.
3. **Setup-Host directory list audit**. I caught the password documentation drift; I missed the directory-list drift. GPT-5.5 enumerates exactly which directories are missing.
4. **Backup-CITemplate default path audit**. I listed the script as Complete without checking the default. GPT-5.5 actually opened the file and read line 48.
5. **Shared cache wiring gap**. I treated shared cache as a security concern; GPT-5.5 treats it as a wiring/operability concern. Both are valid; GPT-5.5's framing is more actionable.
6. **CI-FLOW doc vs reality drift on partial-artifact-on-failure**. I did not audit `CI-FLOW.md` against the orchestrator's actual `catch` flow. GPT-5.5 did.
7. **Wait-VMReady TCP-only check**. Concrete defect I missed. GPT-5.5 caught.
8. **Ubuntu VMDK hash absence as separate channel from toolchain hash placeholders**. I lumped these; GPT-5.5 separates them, which is correct.
9. **Linux extra-env redaction at CRITICAL severity**. I had this at MEDIUM. GPT-5.5's CRITICAL is more defensible if any production workflow currently injects a secret via `extra-guest-env-json`.
Where my analysis outperforms GPT-5.5:
1. **`act_runner` SYSTEM-account vs `~/.ssh/config` alias**. My §8.2 HIGH. GPT-5.5 misses entirely. This is a verifiable runtime-reachable bug.
2. **`capacity: 4` burn-in or downgrade as CRITICAL**. My §1 top priority. GPT-5.5 minimizes this to "probably reliable enough after one fix."
3. **`ci-report-ip.service` presence check in template validation**. My §3.5. GPT-5.5 omits this dependency chain entirely.
4. **`Measure-CIBenchmark.ps1` measures a different IP path than production**. My §3.3. GPT-5.5 mentions in passing but does not connect to capacity-planning credibility.
5. **Hardcoded `N:\Code\Workspace\...` in composite action**. My §8.1. GPT-5.5 catches path drift in setup/backup but not in the action itself.
6. **The 4-minute pre-build serialization math**. My §3.3 works the arithmetic. GPT-5.5 does not.
7. **Section 15 strategic-thinking layer ("What I Would Change")**. My analysis includes a counterfactual section (static IP allocation, transport abstraction, configuration centralization, JSONL→SQLite sink, schema validation, integration-test-first, structured logging helper, PS 7 reconsideration, validation in orchestrator, drop Mode 1). GPT-5.5 has architectural recommendations but does not run a true counterfactual.
8. **OWASP Top 10 mapped explicitly**. My §5.7 walks A01-A10 explicitly. GPT-5.5 covers security thematically without the framework. Either is fine for homelab; the framework view makes audit conversations easier.
9. **Honest grade with per-dimension breakdown**. My Final Verdict gives A-/B+/B/B-/C grades per dimension. GPT-5.5 gives single-number scores (82/78/72/77) without showing the dimensions. My breakdown is more diagnostically useful.
Complementarity:
GPT-5.5 is the better **per-file auditor**. Its strength is reading scripts one at a time and surfacing small concrete defects. The Section 4 enumeration is exactly what a careful code reviewer produces.
My analysis is the better **systems-level interrogator**. It questions claims (capacity: 4 unvalidated), traces cross-file dependencies (SSH alias vs service account, `ci-report-ip.service` vs orchestrator IP discovery), and works the strategic counterfactual.
Combined, the two reviews cover the codebase better than either alone. The master plan should:
- Adopt GPT-5.5's Section 4 per-file findings nearly verbatim as the action item list.
- Adopt my §6.1 (capacity validation), §8.2 (service account verification), §3.5 (ci-report-ip.service check) as the missing critical items.
- Use my Section 15 as the long-horizon strategic background that informs but does not block Phase 1.
- Use GPT-5.5's Phase 1/2/3/Deferred phasing as the roadmap skeleton.
---
## 12. Closing
The GPT-5.5 analysis is a credible, accurate, well-organised technical review with a small number of severity miscalibrations and one notable omission set (the service-account / ssh-alias / ci-report-ip.service / capacity-burn-in cluster). It would be perfectly defensible to adopt the GPT-5.5 report as the primary working document for the next stabilization pass, provided the missing items above are appended.
The honest verdict on GPT-5.5's verdict ("strong homelab CI/CD system... last tightening pass between an impressive lab build and a reliable daily tool") is: correct, fair, and consistent with the evidence. I would sign that conclusion.
The honest verdict on my own analysis vs GPT-5.5: comparable overall, complementary in coverage. Neither replaces the other. The combination is stronger than either alone, which is exactly the value proposition of running two independent reviews.
---
*End of cross-review. Word count: approximately 5,400.*