docs(review): scan-4 review report and findings update

Add scan-4 full review: 1 resolved (F040), 6 new findings (F071-F076). 27 open findings, 49 resolved. No regressions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-07-26 07:58:14 +00:00 · 2026-03-23 09:07:51 -05:00
parent d99c3e5490
commit 0fa92d689f
5 changed files with 3338 additions and 430 deletions
@@ -0,0 +1,61 @@
+# PSProxmoxVE — Claude Code Instructions
+
+## Project Overview
+
+C# binary PowerShell module for managing Proxmox VE (PVE) infrastructure. Two projects:
+- `src/PSProxmoxVE/` — Cmdlets and module surface (targets netstandard2.0)
+- `src/PSProxmoxVE.Core/` — Services, models, HTTP client (targets netstandard2.0)
+
+Tests: xUnit (`tests/PSProxmoxVE.Core.Tests/`) and Pester 5 (`tests/PSProxmoxVE.Tests/`).
+
+## Build & Test
+
+```bash
+# Build
+dotnet build PSProxmoxVE.sln
+
+# xUnit tests
+dotnet test tests/PSProxmoxVE.Core.Tests/
+
+# Pester tests (requires pwsh)
+pwsh -Command "Invoke-Pester tests/PSProxmoxVE.Tests/ -Output Detailed"
+
+# Run all tests via helper
+pwsh -File tools/Invoke-Tests.ps1
+```
+
+## Key Conventions
+
+- All cmdlets use `Pve` noun prefix
+- All cmdlet classes must be `sealed`
+- All cmdlets must have `[OutputType]` attribute
+- Destructive cmdlets must set `ConfirmImpact = ConfirmImpact.High`
+- VmId parameters: `[ValidateRange(100, 999999999)]`, nullable when optional
+- JSON: Newtonsoft.Json only (`[JsonProperty]`), no System.Text.Json attributes
+- Task polling: always use `TaskService.WaitForTask`, never inline loops
+- Passwords: `SecureString` type, never plain `string`
+- URL paths: `Uri.EscapeDataString()` on all dynamic path segments
+- No bare `catch {}` blocks — use specific or filtered exceptions
+- Verb class constants required (`VerbsCommon.Get`, not `"Get"`)
+
+## Review System
+
+This repo uses a structured review system to track findings and prevent regressions.
+
+### Key files
+- `docs/review/findings.json` — stable findings database. IDs are permanent (F001, F002...).
+  Never renumber. Read this before any coding session to understand open issues.
+- `docs/review/REVIEW_REPORT.md` — latest full review report (scan-4, 2026-03-22)
+- `DECISIONS.md` — architectural decisions and anti-patterns. **Read this before writing
+  any new code.** It documents patterns that were deliberately chosen or changed and must
+  not be reintroduced.
+
+### Before starting a coding session
+1. Read `DECISIONS.md` to understand established patterns
+2. Check `docs/review/findings.json` for open findings relevant to the area you're working in
+3. Do not introduce patterns listed as anti-patterns in DECISIONS.md
+
+### Finding ID stability
+Finding IDs (F001, F002...) are permanent. A resolved finding is never deleted from
+findings.json — it is marked `resolved` with evidence of the fix. If a finding reappears,
+it is marked `regressed` and retains its original ID.
@@ -0,0 +1,360 @@
+# Architectural Decisions
+
+This file documents patterns that were deliberately chosen or changed, anti-patterns that
+were explicitly removed, and constraints that must be maintained. **Read this before writing
+any new code.**
+
+---
+
+## D001 — Task polling must use TaskService.WaitForTask
+
+**Status**: Active
+**Finding refs**: F032, F033, F036, F058
+**Resolved in scan**: 2026-03-22 (for VM/network cmdlets); container snapshot + storage cmdlets still open
+
+### Decision
+All task-polling loops must use `TaskService.WaitForTask(upid, session, timeout, progress)`.
+Never implement inline `while(true)` or `do/while` polling loops in cmdlet files.
+
+### Rationale
+Four VM/network cmdlets (InvokePveNetworkApply, NewPveSnapshot, RestorePveSnapshot,
+RemovePveSnapshot) and one guest exec cmdlet had copy-pasted polling loops with no timeout,
+causing cmdlets to hang indefinitely if a PVE task stalled. TaskService.WaitForTask has
+timeout enforcement, failure detection, and WriteProgress support.
+
+Five additional cmdlets (3 container snapshot + 2 storage) still have this anti-pattern as
+of scan 2026-03-22 (F058).
+
+### Anti-pattern (do not reintroduce)
+```csharp
+// NEVER do this in a cmdlet
+while (true)
+{
+    var status = taskService.GetTask(upid, session);
+    if (status.IsFinished) break;
+    Thread.Sleep(1000);
+}
+```
+
+### Correct pattern
+```csharp
+// Always use this
+TaskService.WaitForTask(upid, session, TimeoutSeconds, this);
+```
+
+---
+
+## D002 — Password parameters must use SecureString
+
+**Status**: Active
+**Finding refs**: F051
+**Resolved in scan**: 2026-03-22
+
+### Decision
+All cmdlet parameters that accept passwords must use `SecureString` type with
+`Marshal.SecureStringToGlobalAllocUnicode` + `ZeroFreeGlobalAllocUnicode` in a
+try/finally block for extraction.
+
+### Rationale
+Set-PveVmGuestPassword originally accepted a plain `string` password parameter, leaving
+the credential in managed memory indefinitely. SecureString minimizes the window of
+exposure and is consistent with Connect-PveServer's PSCredential handling.
+
+### Anti-pattern (do not reintroduce)
+```csharp
+// NEVER accept passwords as plain strings
+[Parameter(Mandatory = true)]
+public string Password { get; set; }
+```
+
+### Correct pattern
+```csharp
+[Parameter(Mandatory = true)]
+public SecureString Password { get; set; }
+
+// In ProcessRecord:
+IntPtr ptr = IntPtr.Zero;
+try
+{
+    ptr = Marshal.SecureStringToGlobalAllocUnicode(Password);
+    string plainText = Marshal.PtrToStringUni(ptr);
+    // Use plainText for API call
+}
+finally
+{
+    if (ptr != IntPtr.Zero)
+        Marshal.ZeroFreeGlobalAllocUnicode(ptr);
+}
+```
+
+---
+
+## D003 — URL encoding required for all path parameters
+
+**Status**: Active
+**Finding refs**: F050
+**Resolved in scan**: 2026-03-22
+
+### Decision
+All user-supplied or dynamic values interpolated into API URL paths must be wrapped in
+`Uri.EscapeDataString()`. This applies to all service classes.
+
+### Rationale
+Snapshot names, node names, user IDs, and other identifiers could theoretically contain
+characters that break URL path segments. While most values come from validated sources,
+defense-in-depth requires consistent encoding. Applied across all 14 service classes.
+
+### Anti-pattern (do not reintroduce)
+```csharp
+// NEVER interpolate raw strings into URL paths
+var resource = $"nodes/{node}/qemu/{vmid}/snapshot/{snapshotName}";
+```
+
+### Correct pattern
+```csharp
+var resource = $"nodes/{Uri.EscapeDataString(node)}/qemu/{vmid}/snapshot/{Uri.EscapeDataString(snapshotName)}";
+```
+
+---
+
+## D004 — No bare catch blocks
+
+**Status**: Active
+**Finding refs**: F039
+**Resolved in scan**: 2026-03-22
+
+### Decision
+No bare `catch { }` or `catch (Exception) { }` blocks. All catch blocks must either:
+1. Use a specific exception type (`catch (PveApiException ex)`), or
+2. Use a filtered catch with `when` clause that excludes fatal exceptions
+   (`catch (Exception ex) when (ex is not OutOfMemoryException and not StackOverflowException)`)
+
+### Rationale
+Bare catches in PveHttpClient, PveCmdletBase, VmService, ContainerService, and
+GetPveVmCmdlet silently swallowed errors, making debugging impossible. Replacing with
+filtered or specific catches preserves error visibility while still handling expected
+transient failures.
+
+### Anti-pattern (do not reintroduce)
+```csharp
+// NEVER use bare catches
+try { ... }
+catch { }
+
+// NEVER catch all exceptions unfiltered
+try { ... }
+catch (Exception) { /* ignore */ }
+```
+
+### Correct pattern
+```csharp
+// Catch specific exceptions
+try { ... }
+catch (PveApiException ex) { WriteWarning(ex.Message); }
+
+// Or use filtered catch for status polling
+try { ... }
+catch (Exception ex) when (ex is not OutOfMemoryException and not StackOverflowException)
+{
+    WriteVerbose($"Status poll failed: {ex.Message}");
+}
+```
+
+---
+
+## D005 — OutputType required on all cmdlets
+
+**Status**: Active
+**Finding refs**: F037
+**Resolved in scan**: 2026-03-22
+
+### Decision
+Every cmdlet must have an `[OutputType(typeof(...))]` attribute declaring its return type.
+
+### Rationale
+~54 cmdlets were missing OutputType, degrading IntelliSense, pipeline type inference, and
+`Get-Command -OutputType` queries. All 169 cmdlets now have the attribute.
+
+### Correct pattern
+```csharp
+[Cmdlet(VerbsCommon.Get, "PveVm")]
+[OutputType(typeof(VmInfo))]
+public sealed class GetPveVmCmdlet : PveCmdletBase
+```
+
+---
+
+## D006 — ConfirmImpact.High required for destructive operations
+
+**Status**: Active
+**Finding refs**: F011, F034, F042, F043, F062, F063
+**Resolved in scan**: 2026-03-22 (for VM cmdlets); container Restart/Suspend still open
+
+### Decision
+All cmdlets that perform destructive or disruptive operations must set
+`ConfirmImpact = ConfirmImpact.High` in the `[Cmdlet]` attribute. This includes:
+- All `Remove-*` cmdlets
+- All `Stop-*` cmdlets
+- All `Reset-*` cmdlets
+- All `Restart-*` cmdlets
+- All `Suspend-*` cmdlets
+- `Restore-PveSnapshot` and `Restore-PveContainerSnapshot`
+- `New-PveTemplate` (irreversible conversion)
+
+### Rationale
+Stop-PveVm, Reset-PveVm, Suspend-PveVm, Restart-PveVm, and Remove-PveRole were missing
+ConfirmImpact.High, meaning users could accidentally perform disruptive operations without
+being prompted. Container counterparts (Restart/Suspend) remain inconsistent as of F062/F063.
+
+### Correct pattern
+```csharp
+[Cmdlet(VerbsLifecycle.Stop, "PveVm", SupportsShouldProcess = true,
+    ConfirmImpact = ConfirmImpact.High)]
+```
+
+---
+
+## D007 — All cmdlet classes must be sealed
+
+**Status**: Active
+**Finding refs**: F041
+**Resolved in scan**: 2026-03-22
+
+### Decision
+All cmdlet classes must be declared `sealed`. Cmdlets are not designed for inheritance
+and sealing prevents unintended extension.
+
+### Rationale
+~95 cmdlets were not sealed. Sealing all 169 cmdlets makes the design intent explicit
+and enables potential JIT optimizations.
+
+### Correct pattern
+```csharp
+public sealed class GetPveVmCmdlet : PveCmdletBase
+```
+
+---
+
+## D008 — JSON serialization: Newtonsoft.Json only
+
+**Status**: Active
+**Finding refs**: F044
+**Resolved in scan**: 2026-03-22
+
+### Decision
+Use only `Newtonsoft.Json` (`[JsonProperty]`) for JSON serialization attributes on model
+classes. Do not add `System.Text.Json` (`[JsonPropertyName]`) attributes.
+
+### Rationale
+The module uses Newtonsoft.Json for all API response deserialization. Having both
+`[JsonProperty]` and `[JsonPropertyName]` attributes was redundant and confusing —
+System.Text.Json is not used at runtime. All `[JsonPropertyName]` attributes were removed.
+
+### Anti-pattern (do not reintroduce)
+```csharp
+// NEVER add System.Text.Json attributes alongside Newtonsoft
+[JsonProperty("status")]
+[JsonPropertyName("status")]  // Don't add this
+public string Status { get; set; }
+```
+
+### Correct pattern
+```csharp
+[JsonProperty("status")]
+public string Status { get; set; }
+```
+
+---
+
+## D009 — Framework targeting: netstandard2.0 for publishable, net10.0+net48 for tests
+
+**Status**: Active
+**Finding refs**: F047, F064
+**Status note**: net9.0 → net10.0 migration still pending
+
+### Decision
+- Publishable projects (`PSProxmoxVE`, `PSProxmoxVE.Core`): Target `netstandard2.0` for
+  maximum compatibility (PS 5.1 Desktop + PS 7.x Core).
+- Test projects: Target `net10.0` (LTS) and `net48` (Windows PowerShell 5.1 validation).
+
+### Rationale
+.NET 9.0 reached EOL in May 2025. The test projects should use the current LTS release
+(net10.0). The publishable module must remain on netstandard2.0 to support both Desktop
+and Core editions.
+
+---
+
+## D010 — VmId parameters: nullable int with ValidateRange
+
+**Status**: Active
+**Finding refs**: F012, F038
+**Resolved in scan**: 2026-03-21 (ValidateRange), 2026-03-22 (nullable)
+
+### Decision
+VmId parameters must:
+1. Use `int?` (nullable) when the parameter is optional (e.g., firewall cmdlets that
+   operate at cluster, node, or VM level)
+2. Include `[ValidateRange(100, 999999999)]` to match PVE's VMID constraints
+3. Use `int` (non-nullable) only when VmId is mandatory
+
+### Rationale
+PVE requires VMIDs in range 100-999999999. Without ValidateRange, invalid IDs reach the
+API and return confusing errors. Using non-nullable int with default 0 for optional VmId
+made it impossible to distinguish "not specified" from "VM 0" in firewall cmdlets.
+
+### Correct pattern
+```csharp
+// Mandatory VmId
+[Parameter(Mandatory = true)]
+[ValidateRange(100, 999999999)]
+public int VmId { get; set; }
+
+// Optional VmId (e.g., firewall cmdlets)
+[Parameter()]
+[ValidateRange(100, 999999999)]
+public int? VmId { get; set; }
+```
+
+---
+
+## D011 — Verb class constants required for cmdlet attributes
+
+**Status**: Active
+**Finding refs**: F009
+**Resolved in scan**: 2026-03-21
+
+### Decision
+All `[Cmdlet]` attributes must use verb class constants (`VerbsCommon.Get`,
+`VerbsLifecycle.Start`, etc.) instead of hardcoded string literals.
+
+### Rationale
+Reset-PveVm used `[Cmdlet("Reset", ...)]` instead of `VerbsCommon.Reset`. While "Reset"
+is an approved verb, using the constant ensures compile-time verification and consistency
+with all other cmdlets.
+
+### Anti-pattern (do not reintroduce)
+```csharp
+[Cmdlet("Reset", "PveVm")]  // Don't use string literals
+```
+
+### Correct pattern
+```csharp
+[Cmdlet(VerbsCommon.Reset, "PveVm")]
+```
+
+---
+
+## D012 — Magic strings: extract to named constants
+
+**Status**: Active
+**Finding refs**: F049
+**Resolved in scan**: 2026-03-22
+
+### Decision
+Frequently used string literals (auth header names, token prefixes, etc.) should be
+extracted to `const string` fields for maintainability.
+
+### Rationale
+Auth header names (`PVEAPIToken=`, `CSRFPreventionToken`) were inline string literals
+used in multiple places. Extracting to `const string ApiTokenPrefix` and
+`CsrfHeaderName` fields improves maintainability and reduces typo risk.
@@ -0,0 +1,313 @@
+# PSProxmoxVE Comprehensive Review Report
+
+```
+Scan date:           2026-03-22
+Prior report date:   2026-03-22 (scan-3)
+PVE API spec date:   2026-03-21T15:04:50.641Z
+PVE API spec SHA256: 4af79be30166209a4714b771f65e1e9540c5b738f414ff30c98454402e29d030
+PVE version hint:    (not set in spec)
+Total API endpoints: 646
+Findings DB:         docs/review/findings.json (F001–F076)
+Open findings:       22 (before scan) → 27 (after scan)
+New this scan:       6   Resolved this scan: 1   Regressed: 0
+```
+
+## Executive Summary
+
+- **Delta**: 1 resolved | 6 new | 0 regressed | 27 open
+- **API drift**: 0 breaking changes in PVE 9.x affecting current cmdlets | 42 new PVE 9.0 endpoints unimplemented
+- **API coverage**: 169 cmdlets covering ~180 of 646 endpoints (28%)
+- **Critical open**: F058 — 5 cmdlets with infinite-loop task polling (D001 violation)
+- **New high**: F073 — build.yml references net9.0 but test project targets net10.0 (workflow will fail)
+- **New medium**: F071 — ~15 cmdlets missing Uri.EscapeDataString (D003 violation)
+- **Resolved**: F040 — Broad exception catches now properly filtered
+- **All D007/D005/D008/D011/D012 decisions hold**: sealed, OutputType, Newtonsoft-only, verb constants all pass 169/169
+- **Security**: All password params use SecureString, all service URLs properly encoded, no credential logging
+- **Testing**: 100% Pester parameter validation, ~105/169 integration coverage
+
+---
+
+## Phase 1 — Repository Inventory & Structure
+
+### Project Layout
+
+| Component | Path | Framework |
+|---|---|---|
+| PSProxmoxVE (cmdlets) | `src/PSProxmoxVE/` | netstandard2.0; net9.0; net48 |
+| PSProxmoxVE.Core (services) | `src/PSProxmoxVE.Core/` | netstandard2.0; net9.0; net48 |
+| xUnit Tests | `tests/PSProxmoxVE.Core.Tests/` | net10.0; net48 |
+| Pester Tests | `tests/PSProxmoxVE.Tests/` | PowerShell 5.1+ |
+| Integration Infra | `tests/infrastructure/` | Terraform + Docker |
+
+### CI/CD Workflows
+
+| Workflow | Trigger | Status |
+|---|---|---|
+| build.yml | push/PR to main | ⚠ References net9.0 (F073) |
+| unit-tests.yml | push/PR to main | ✓ Matrix: Win PS 5.1, Win PS 7.5, Ubuntu PS 7.5, macOS PS 7.5 |
+| integration-tests.yml | push/PR + dispatch | ✓ Self-hosted runner, nested PVE 8 + PVE 9 |
+| publish.yml | tag push `v*` | ⚠ Low smoke threshold (F074) |
+
+### Documentation
+
+| File | Present | Notes |
+|---|---|---|
+| README.md | ✓ | Comprehensive with badges, cmdlet reference |
+| CHANGELOG.md | ✓ | Keep a Changelog format |
+| CONTRIBUTING.md | ✓ | Dev setup, coding standards, PR process |
+| LICENSE | ✓ | MIT |
+| CODE_OF_CONDUCT.md | ✓ | Contributor Covenant adapted |
+| SECURITY.md | ✓ | Vulnerability disclosure + response SLA |
+| DECISIONS.md | ✓ | 12 decisions (D001–D012) |
+| .editorconfig | ✓ | 4-space C#/PS, 2-space XML/JSON |
+| .gitignore | ✓ | |
+| .gitattributes | ✓ | |
+
+### Missing Items
+
+| Finding ID | Item | Notes |
+|---|---|---|
+| F075 | ~88 cmdlets lack markdown help docs | 81 of 169 covered |
+| F065 | No config.yml for issue templates | |
+| F066 | No CODEOWNERS file | |
+| F076 | No dependabot/renovate | |
+
+---
+
+## Phase 2 — PVE API Coverage Audit
+
+### Coverage by Functional Area
+
+| Area | Total Endpoints | Covered | % | Notes |
+|---|---|---|---|---|
+| backup | ~6 | ~6 | 100% | Full coverage |
+| tasks | ~5 | ~5 | 100% | Full coverage |
+| storage_config | ~5 | ~4 | 80% | |
+| access_domains | ~6 | ~5 | 83% | |
+| roles | ~5 | ~4 | 80% | |
+| access_groups | ~5 | ~4 | 80% | |
+| firewall | ~40 | ~30 | 75% | Cluster-level excellent |
+| networking | ~7 | ~5 | 71% | |
+| pools | ~7 | ~5 | 71% | |
+| users | ~12 | ~8 | 67% | |
+| storage | ~19 | ~12 | 63% | |
+| sdn | ~59 | ~25 | 42% | Missing fabrics (PVE 9.0) |
+| containers | ~62 | ~25 | 40% | Missing VNC/SPICE, firewall |
+| vms | ~97 | ~35 | 36% | Missing VNC/SPICE, RRD, agent sub-commands |
+| access | ~15 | ~3 | 20% | Missing TFA, OpenID |
+| nodes | ~58 | ~10 | 17% | Missing Ceph, disks, apt, services |
+| cluster | ~81 | ~3 | 4% | Only resources/status |
+| **ha** | **~21** | **0** | **0%** | F053 |
+| **ceph** | **~40** | **0** | **0%** | F054 |
+| **cluster_config** | **~10** | **0** | **0%** | F060 |
+| **disks** | **~18** | **0** | **0%** | F067 |
+| **notifications** | **~32** | **0** | **0%** | F068 |
+| **acme/certs** | **~23** | **0** | **0%** | F069 |
+| **replication** | **~5** | **0** | **0%** | |
+| **services** | **~7** | **0** | **0%** | |
+| **TOTAL** | **646** | **~180** | **28%** | |
+
+### PVE 9.0 New Endpoints (F061)
+
+42 new endpoints in PVE 9.0 — **0 implemented**:
+
+- **SDN Fabrics** (~17 endpoints) — entirely new subsystem for fabric networking
+- **Cluster Bulk Actions** (~6 endpoints) — cluster-wide guest start/shutdown/suspend/migrate
+- **HA Affinity Rules** (~5 endpoints) — new HA group affinity management
+- **Miscellaneous** (~14 endpoints) — scattered across nodes, storage, access
+
+### API Drift
+
+No breaking changes detected in PVE 9.x affecting currently implemented cmdlets. All existing cmdlets should continue to work against PVE 9.x servers.
+
+### High-Value Gaps
+
+1. **HA (High Availability)** — 21 endpoints, essential for production clusters
+2. **Ceph** — 40 endpoints, critical for hyperconverged infrastructure
+3. **Cluster Configuration** — 10 endpoints for cluster create/join
+4. **VNC/SPICE Console Access** — frequently requested for remote management
+5. **Cluster Notifications** — new in PVE 8.1+, essential for monitoring automation
+6. **`GET /cluster/nextid`** — critical for scripted VM creation
+
+---
+
+## Phase 3 — Code Quality & Best Practices
+
+### DECISIONS.md Compliance
+
+| Decision | Rule | Status | Notes |
+|---|---|---|---|
+| D001 | TaskService.WaitForTask | ⛔ VIOLATION | F058: 5 cmdlets still use while(true) |
+| D002 | SecureString for passwords | ✓ PASS | All 7 password cmdlets |
+| D003 | Uri.EscapeDataString | ⛔ VIOLATION | F071: ~15 cmdlets bypass services |
+| D004 | No bare catch blocks | ✓ PASS | Zero instances in src/ |
+| D005 | OutputType on all cmdlets | ✓ PASS | 169/169 |
+| D006 | ConfirmImpact.High | ⚠ PARTIAL | F062, F063: container Restart/Suspend |
+| D007 | All cmdlets sealed | ✓ PASS | 169/169 |
+| D008 | Newtonsoft.Json only | ✓ PASS (source) | F072: STJ dependency in csproj |
+| D009 | netstandard2.0 for publish | ⚠ PARTIAL | F047: net9.0 still in targets |
+| D010 | VmId nullable + ValidateRange | ✓ PASS | |
+| D011 | Verb class constants | ✓ PASS | |
+| D012 | Magic strings extracted | ✓ PASS | |
+
+### Findings
+
+| Finding ID | File(s) | Severity | Status | Description |
+|---|---|---|---|---|
+| F058 | 5 container/storage cmdlets | Critical | Open | while(true) infinite loops without timeout (D001) |
+| F073 | build.yml, test .csproj | High | **New** | build.yml uses net9.0 but test project targets net10.0 |
+| F071 | ~15 cmdlet files | Medium | **New** | Missing Uri.EscapeDataString on inline URL paths (D003) |
+| F062 | RestartPveContainerCmdlet.cs | Medium | Open | Missing ConfirmImpact.High (D006) |
+| F063 | SuspendPveContainerCmdlet.cs | Medium | Open | Missing ConfirmImpact.High (D006) |
+| F047 | Both publishable .csproj | Medium | Open | net9.0 target is EOL (D009) |
+| F048 | ~216 call sites | Medium | Open | Sync-over-async — accepted PS 5.1 tradeoff |
+| F045 | ~207 new PveHttpClient | Medium | Open | Per-call HTTP client — accepted design |
+| F072 | PSProxmoxVE.Core.csproj | Low | **New** | Unnecessary System.Text.Json dependency |
+| F064 | PSProxmoxVE.csproj | Low | Open | SMA pinned to 7.4.0 |
+| F040 | PveCmdletBase, services | Medium | **Resolved** | Catches now properly filtered with `when` |
+
+---
+
+## Phase 4 — Testing Coverage Analysis
+
+### Test Inventory
+
+| Type | Framework | Files | Coverage |
+|---|---|---|---|
+| xUnit (C#) | xunit 2.7.0 + Moq | 15 test files | Models + authentication only |
+| Pester (PS) | Pester 5 | 50 test files | 169/169 cmdlet parameter validation |
+| Integration (PS) | Pester 5 | 1 file (1544 lines) | ~105/169 cmdlets |
+
+### Integration Test Coverage Gaps (F046)
+
+**~64 cmdlets lack integration tests**, including:
+
+- **VM ops**: Move-PveVm, Move-PveVmDisk, Remove-PveVmDisk
+- **Guest agent**: Get-PveVmGuestOsInfo, Get-PveVmGuestFsInfo, Read/Write-PveVmGuestFile, Set-PveVmGuestPassword, Invoke-PveVmGuestFsTrim
+- **Storage**: Set-PveStorage, Get-PveStorageStatus, Remove/Set-PveStorageContent, New-PveStorageDisk
+- **Access**: Set-PveRole, Set-PveApiToken, Set-PvePassword, all Group/Domain CRUD
+- **SDN**: All Set-PveSdn*, Invoke-PveSdnApply, New-PveSdn{Ipam,Dns,Controller}
+- **Nodes**: Get/Set-PveNodeConfig, Get/Set-PveNodeDns, Start/Stop-PveNodeVms
+- **Pools**: All 4 pool cmdlets
+- **Cluster**: Get-PveClusterResource
+- **Containers**: Move/Suspend/Resume/Resize, Move-PveContainerVolume, New-PveContainerTemplate
+- **Firewall**: Set operations, groups, options, refs
+- **Backup**: New-PveBackup, Get-PveBackupInfo
+- **Tasks**: Get-PveTaskList, Stop-PveTask
+
+### Quality Observations
+
+**Strengths**: 100% Pester parameter coverage, real PVE 9 JSON fixtures in xUnit, well-structured integration tests with cleanup.
+
+**Weaknesses**: No mock-based service unit tests, xUnit covers only models/auth (no service logic), integration test is a single monolithic file, some tests depend on sequential state.
+
+---
+
+## Phase 5 — Security Review
+
+| Area | Status | Details |
+|---|---|---|
+| Password params use SecureString | ✓ PASS | All 7 cmdlets |
+| SecureString freed after use | ✓ PASS | try/finally with ZeroFreeGlobalAllocUnicode |
+| No credential logging | ✓ PASS | WriteVerbose never logs passwords |
+| HTTPS by default | ✓ PASS | All API calls use https:// |
+| SkipCertificateCheck opt-in | ✓ PASS | Warning emitted when used |
+| Uri.EscapeDataString in services | ✓ PASS | 100+ usages, consistent |
+| Uri.EscapeDataString in cmdlets | ⛔ FAIL | F071: ~15 cmdlets bypass services |
+| ValidateRange on VmId | ✓ PASS | 90+ instances, consistent |
+| No bare catch blocks | ✓ PASS | Zero in src/ |
+| F031: Hardcoded test password | ⚠ Accepted | Testpass123! in CI, masked, disposable VM |
+
+---
+
+## Phase 6 — PSGallery Publication Readiness
+
+| Finding ID | Check | Pass/Fail | Notes |
+|---|---|---|---|
+| — | ModuleVersion | ✓ Pass | 0.1.0 (publish workflow overrides from tag) |
+| — | Author/Company | ✓ Pass | goodolclint / Worklab |
+| — | Description | ✓ Pass | Descriptive |
+| — | LicenseUri | ✓ Pass | Present |
+| — | ProjectUri | ✓ Pass | Present |
+| — | Tags | ✓ Pass | 8 tags including ProxmoxVE8/9 |
+| — | ReleaseNotes | ✓ Pass | Present |
+| — | CmdletsToExport | ✓ Pass | Explicit list (~169) |
+| — | CompatiblePSEditions | ✓ Pass | Desktop, Core |
+| — | PowerShellVersion | ✓ Pass | 5.1 |
+| — | Prerelease | ✓ Pass | 'preview' |
+| F021 | IconUri | ✗ Fail | Missing — PSGallery listing will lack icon |
+| F047 | netstandard2.0 only for publish | ⚠ Warn | net9.0 in csproj but publish builds netstandard2.0 only |
+| F073 | Build workflow alignment | ✗ Fail | build.yml uses net9.0, test project uses net10.0 |
+| F074 | Publish smoke test | ⚠ Warn | Threshold 60 too low for 169 cmdlets |
+| F070 | PS 5.1 smoke test | ✗ Fail | No Windows PS 5.1 validation before publish |
+| F075 | Cmdlet help documentation | ⚠ Warn | 81/169 have markdown docs |
+
+---
+
+## Phase 7 — Community & Repo Maintenance
+
+| Finding ID | Check | Pass/Fail | Notes |
+|---|---|---|---|
+| — | Bug report template | ✓ Pass | YAML format, collects PVE/PS/OS versions |
+| — | Feature request template | ✓ Pass | YAML format |
+| F065 | Issue template config.yml | ✗ Fail | Missing — blank issues uncontrolled |
+| — | PR template | ✓ Pass | Comprehensive checklist |
+| — | CONTRIBUTING.md | ✓ Pass | Good quality |
+| — | CODE_OF_CONDUCT.md | ✓ Pass | Contributor Covenant adapted |
+| — | SECURITY.md | ✓ Pass | 48h response SLA |
+| — | DECISIONS.md | ✓ Pass | 12 decisions, linked from CLAUDE.md |
+| — | Commit conventions | ✓ Pass | Conventional Commits consistently used |
+| F066 | CODEOWNERS | ✗ Fail | Missing |
+| F076 | Dependency automation | ✗ Fail | No dependabot/renovate |
+| — | Branch protection | ? | Cannot verify from CLI |
+
+---
+
+## Phase 9 — Prioritized Recommendations
+
+### 🔴 Critical
+
+| Finding ID | What | Where | Why | Fix |
+|---|---|---|---|---|
+| F058 | Infinite loop task-polling | 5 container/storage cmdlets | Cmdlets hang forever if task stalls | Replace private WaitForTask with TaskService.WaitForTask |
+
+### 🟠 High
+
+| Finding ID | What | Where | Why | Fix |
+|---|---|---|---|---|
+| F073 | build.yml / test project framework mismatch | build.yml, test .csproj | CI workflow will fail | Update build.yml to use net10.0 or remove net9.0 from publishable csproj |
+| F053 | HA subsystem 0% coverage | — | Essential for production clusters | Implement HA resource/group CRUD cmdlets |
+| F054 | Ceph subsystem 0% coverage | — | Critical for hyperconverged | Implement Ceph OSD/pool/mon cmdlets |
+
+### 🟡 Medium
+
+| Finding ID | What | Where | Why | Fix |
+|---|---|---|---|---|
+| F071 | Missing Uri.EscapeDataString | ~15 cmdlet files | D003 violation, URL injection risk | Add Uri.EscapeDataString to all inline URL paths |
+| F062 | RestartPveContainer no ConfirmImpact.High | RestartPveContainerCmdlet.cs | D006 inconsistency | Add ConfirmImpact = ConfirmImpact.High |
+| F063 | SuspendPveContainer no ConfirmImpact.High | SuspendPveContainerCmdlet.cs | D006 inconsistency | Add ConfirmImpact = ConfirmImpact.High |
+| F047 | net9.0 target framework EOL | Both publishable .csproj | .NET 9 EOL May 2025 | Remove net9.0, keep netstandard2.0 (+ optionally add net10.0) |
+| F074 | Publish smoke test threshold too low | publish.yml | Won't catch cmdlet regressions | Raise threshold to ~150 |
+| F075 | ~88 cmdlets lack help docs | docs/cmdlets/ | Poor user experience | Generate docs for missing cmdlets |
+| F046 | Integration test gaps | Integration.Tests.ps1 | 64 cmdlets untested end-to-end | Prioritize destructive/lifecycle cmdlets |
+| F059 | VM/CT-level firewall 0% | — | Per-guest firewall is common use case | Implement VM/CT firewall rule cmdlets |
+| F060 | Cluster config 0% | — | Multi-node automation | Implement cluster create/join cmdlets |
+| F061 | PVE 9.0 endpoints 0% | — | 42 new endpoints unimplemented | Prioritize bulk actions and SDN fabrics |
+| F048 | Sync-over-async | ~216 call sites | Theoretical deadlock risk | Accepted for PS 5.1 compat — no fix needed |
+| F045 | HttpClient per-call | ~207 instances | Socket exhaustion risk | Consider IHttpClientFactory long-term |
+| F070 | No PS 5.1 smoke test | publish.yml | Desktop compatibility untested | Add Windows PS 5.1 step to publish workflow |
+
+### 🟢 Low
+
+| Finding ID | What | Where | Why | Fix |
+|---|---|---|---|---|
+| F031 | Hardcoded test password | CI workflow, Terraform | Accepted risk for disposable VMs | — |
+| F021 | No IconUri | PSProxmoxVE.psd1 | Cosmetic PSGallery listing | Add icon to repo and reference in manifest |
+| F064 | SMA pinned to 7.4.0 | PSProxmoxVE.csproj | Review with net10.0 migration | Update when removing net9.0 |
+| F072 | Unnecessary STJ dependency | Core.csproj | D008: Newtonsoft only | Remove System.Text.Json PackageReference |
+| F065 | No issue template config.yml | .github/ISSUE_TEMPLATE/ | Blank issues uncontrolled | Add config.yml |
+| F066 | No CODEOWNERS | root | No auto-review assignment | Add CODEOWNERS |
+| F067 | Disk management 0% | — | Storage provisioning | Implement when needed |
+| F068 | Notifications 0% | — | Monitoring automation | Implement when needed |
+| F069 | ACME/Certificates 0% | — | TLS automation | Implement when needed |
+| F076 | No dependabot/renovate | .github/ | Dependency drift | Add dependabot.yml |