docs: review scan-8 findings update

Scan-8 full re-scan of PSProxmoxVE module (2026-03-24).

New findings:
- F084 [medium/security]: PveSession exposes auth secrets in pipeline output

Regressed:
- F039 [medium]: Bare catch blocks in VmService.PingGuestAgent and
  ImportPveOvaCmdlet (D004 violation)

Resolved:
- F059: VM/CT-level firewall was already supported via Level parameter

Findings DB: F001-F084, 10 open + 1 regressed + 72 resolved + 1 wont_fix

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

docs/review/REVIEW_REPORT.md

@@ -1,27 +1,29 @@
-# PSProxmoxVE Module Review Report — Scan 7
+# PSProxmoxVE Review Report — Scan 8
 
 ```
-Scan date:           2026-03-23
-Prior report date:   2026-03-23 (scan-6)
+Scan date:           2026-03-24
+Prior report date:   2026-03-23
 PVE API spec date:   2026-03-21T15:04:50.641Z
 PVE API spec SHA256: 4af79be30166209a4714b771f65e1e9540c5b738f414ff30c98454402e29d030
-PVE version hint:    N/A (not set in spec)
+PVE version hint:    (not set)
 Total API endpoints: 646
-Findings DB:         docs/review/findings.json (F001–F083)
-Open findings:       12 (before scan) → 11 open + 1 regressed (after scan)
-New this scan:       0  Resolved this scan: 1 (F082)  Regressed: 1 (F050)
-Last CI run:         integration-tests.yml | success | 2026-03-23T21:42:02Z | https://github.com/goodolclint/PSProxmoxVE/actions/runs/23461565300
+Findings DB:         docs/review/findings.json (F001–F084)
+Open findings:       10 (before scan) → 11 (after scan, including 1 regressed)
+New this scan:       1  Resolved this scan: 1 (F059)  Regressed: 1 (F039)
+Last CI run:         integration-tests.yml | failure (provision infra) | 2026-03-24 | run 23511293737
+                     (newer run in_progress — prior successful run: 23511223201)
 ```
 
 ## Executive Summary
 
-- **Delta**: 1 resolved (F082) | 0 new | 1 regressed (F050) | 11 open + 1 regressed
-- **API drift**: 0 breaking changes in implemented endpoints | 42 PVE 9.0 endpoints unimplemented (F061)
-- **CI**: Last integration run: **PASSED** | 0 test failures (both PVE 8 and PVE 9)
-- **Code quality**: 11 of 12 DECISIONS.md entries upheld — **1 regression** (D003: URL encoding in 5 service files)
-- Module has 169 cmdlets, ~170 markdown docs, netstandard2.0 targeting, 1,749 total tests (374 xUnit + 1,268 Pester + 107 integration)
-- PSGallery manifest complete except IconUri (F021)
-- All community/repo standards pass (issue templates, CONTRIBUTING, SECURITY, CODE_OF_CONDUCT, CODEOWNERS, etc.)
+- **Delta**: 1 resolved (F059) | 1 new (F084) | 1 regressed (F039) | 11 open (10 open + 1 regressed)
+- **API drift**: 24 breaking parameter changes in PVE 9.0 (mostly cluster config `link[n]` type changes) | 42 new PVE 9.0 endpoints unimplemented
+- **CI**: Last completed integration run: FAILED (provision infrastructure — cloud image download race condition, already resolved in subsequent run) | 0 test failures
+- Module has **169 cmdlets** covering ~155 of 646 API endpoints (**24% coverage**)
+- All prior code quality findings (F032–F044, F049–F051, F058, F062–F063) remain resolved — no regressions detected
+- All DECISIONS.md patterns verified: no bare catches, no System.Text.Json, no plain string passwords, no inline task polling, URL encoding applied, all cmdlets sealed with OutputType
+- PSGallery publication readiness is high — manifest complete, publish workflow in place, PS 5.1 smoke test passes
+- 12 open findings: 1 regression (F039 bare catch blocks in VmService + ImportPveOvaCmdlet), 1 new security finding (F084 PveSession exposes auth secrets), API coverage gaps, and manifest cosmetic issue (F021 IconUri)
 
 ---
 
@@ -29,39 +31,47 @@ Last CI run:         integration-tests.yml | success | 2026-03-23T21:42:02Z | ht
 
 ### Project Layout
 
-| Item | Present | Path/Notes |
-|---|---|---|
-| Solution file | Yes | PSProxmoxVE.sln |
-| Cmdlet project | Yes | src/PSProxmoxVE/ (netstandard2.0) |
-| Core library | Yes | src/PSProxmoxVE.Core/ (netstandard2.0) |
-| xUnit tests | Yes | tests/PSProxmoxVE.Core.Tests/ (net10.0;net48) |
-| Pester tests | Yes | tests/PSProxmoxVE.Tests/ |
-| Module manifest | Yes | src/PSProxmoxVE/PSProxmoxVE.psd1 |
-| MAML help | Yes | src/PSProxmoxVE/PSProxmoxVE.dll-Help.xml |
-| Format file | Yes | src/PSProxmoxVE/PSProxmoxVE.format.ps1xml |
-| README.md | Yes | With badges (Build, Unit Tests, License, PSGallery) |
-| CHANGELOG.md | Yes | Keep a Changelog format, 0.1.0-preview entry |
-| CONTRIBUTING.md | Yes | Dev setup, coding standards, PR process |
-| LICENSE | Yes | MIT |
-| CODE_OF_CONDUCT.md | Yes | Contributor Covenant 2.1 |
-| SECURITY.md | Yes | Vulnerability disclosure policy |
-| DECISIONS.md | Yes | 12 active decisions (D001–D012) |
-| .editorconfig | Yes | Root config with per-filetype overrides |
-| .gitignore | Yes | Comprehensive |
-| .gitattributes | Yes | LF enforcement, binary handling, diff drivers |
-| CODEOWNERS | Yes | `* @goodolclint` |
-| dependabot.yml | Yes | Automated dependency updates |
-| Issue templates | Yes | Bug report + feature request (YAML forms) + config.yml |
-| PR template | Yes | Summary, type, checklist, API endpoints, testing |
-| CI workflows | Yes | build.yml, unit-tests.yml, integration-tests.yml, publish.yml |
-| Review system | Yes | docs/review/findings.json, docs/review/REVIEW_REPORT.md |
-| Cmdlet docs | Yes | ~170 markdown files in docs/cmdlets/ |
+| Item | Present | Path | Notes |
+|---|---|---|---|
+| Solution file | Yes | PSProxmoxVE.sln | 2 src projects + 1 test project |
+| Module project | Yes | src/PSProxmoxVE/ | netstandard2.0, 169 cmdlets |
+| Core project | Yes | src/PSProxmoxVE.Core/ | netstandard2.0, services + models |
+| xUnit tests | Yes | tests/PSProxmoxVE.Core.Tests/ | net10.0 + net48 |
+| Pester tests | Yes | tests/PSProxmoxVE.Tests/ | 30+ test files |
+| Integration tests | Yes | tests/PSProxmoxVE.Tests/Integration/ | PVE 8 + PVE 9 |
+| Module manifest | Yes | src/PSProxmoxVE/PSProxmoxVE.psd1 | v0.1.0-preview |
+| MAML help | Yes | src/PSProxmoxVE/PSProxmoxVE.dll-Help.xml | Generated |
+| Format file | Yes | src/PSProxmoxVE/PSProxmoxVE.format.ps1xml | Custom formatting |
+| Cmdlet docs | Yes | docs/cmdlets/ | 169 markdown files |
+| README.md | Yes | README.md | Comprehensive, badges |
+| CHANGELOG.md | Yes | CHANGELOG.md | Keep-a-Changelog format |
+| CONTRIBUTING.md | Yes | CONTRIBUTING.md | Development setup + guidelines |
+| LICENSE | Yes | LICENSE | MIT |
+| CODE_OF_CONDUCT.md | Yes | CODE_OF_CONDUCT.md | Contributor Covenant |
+| SECURITY.md | Yes | SECURITY.md | Security policy |
+| DECISIONS.md | Yes | DECISIONS.md | 12 architectural decisions |
+| CODEOWNERS | Yes | CODEOWNERS | @goodolclint |
+| .editorconfig | Yes | .editorconfig | Consistent style |
+| .gitignore | Yes | .gitignore | Comprehensive |
+| .gitattributes | Yes | .gitattributes | Present |
+| Issue templates | Yes | .github/ISSUE_TEMPLATE/ | Bug report + feature request + config.yml |
+| PR template | Yes | .github/pull_request_template.md | Present |
+| Dependabot | Yes | .github/dependabot.yml | NuGet + GitHub Actions weekly |
+
+### CI/CD Workflows
+
+| Workflow | File | Trigger | Status |
+|---|---|---|---|
+| Build | build.yml | push/PR to main | matrix: windows+ubuntu, net48+net10.0 |
+| Unit Tests | unit-tests.yml | push/PR to main | Pester on PS 5.1, 7.5 (win/ubuntu/macos) |
+| Integration Tests | integration-tests.yml | push to main | Self-hosted, PVE 8+9, concurrency group |
+| Publish | publish.yml | tag push (v*) | PSGallery + GitHub Release |
 
 ### Missing Items
 
 | Finding ID | Item | Notes |
 |---|---|---|
-| F021 | IconUri in manifest PSData | PSGallery listings display placeholder without it |
+| F021 | IconUri in manifest PSData | Cosmetic — recommended for PSGallery listing |
 
 ---
 
@@ -69,167 +79,240 @@ Last CI run:         integration-tests.yml | success | 2026-03-23T21:42:02Z | ht
 
 ### Coverage by Functional Area
 
-| Area | Spec Endpoints | Covered | % | Notes |
-|------|---------------|---------|---|-------|
-| VMs (qemu) | 97 | ~30 | 31% | Core CRUD, lifecycle, guest agent, disk ops, config |
-| Containers (lxc) | 62 | ~18 | 29% | CRUD, lifecycle, snapshots, config, interfaces |
-| Nodes | 77 | ~10 | 13% | List, status, config, DNS, startall/stopall |
-| Firewall | 40 | ~22 | 55% | Rules, groups, aliases, IP sets, options, refs |
-| Cluster | 79 | ~3 | 4% | Resources and status only |
-| SDN | 60 | ~16 | 27% | Zones, VNets, subnets, IPAM, DNS, controllers |
-| Storage | 24 | ~14 | 58% | CRUD, content, upload, download, allocate |
-| Backup | 6 | 6 | 100% | Full coverage |
-| Access/Users | 45 | ~22 | 49% | Users, groups, roles, domains, tokens, ACL, permissions |
-| Tasks | 5 | ~4 | 80% | List, status, log, stop |
-| Pools | 7 | ~5 | 71% | CRUD + list |
-| HA | 21 | 0 | 0% | F053 — 🆕 Rules subsystem new in PVE 9.0 |
-| Ceph | 40 | 0 | 0% | F054 |
-| Notifications | 25+ | 0 | 0% | F068 — New in PVE 8.1+ |
-| ACME/Certs | 23 | 0 | 0% | F069 |
-| Disks | 18 | 0 | 0% | F067 |
-| Replication | 5 | 0 | 0% | |
-| Services | 7 | 0 | 0% | |
-| **Total** | **646** | **~120** | **~18.6%** | |
+| Area | Total Endpoints | Covered | % | New in 9.0 (unimpl) | Notes |
+|---|---|---|---|---|---|
+| access | 15 | 7 | 47% | 1 | API tokens, permissions, password |
+| access_domains | 6 | 4 | 67% | 0 | CRUD complete |
+| access_groups | 5 | 4 | 80% | 0 | CRUD complete |
+| acl | 2 | 0 | 0% | 0 | |
+| acme | 15 | 0 | 0% | 0 | F069 |
+| apt | 8 | 0 | 0% | 0 | Node package management |
+| backup | 6 | 6 | 100% | 0 | Full coverage |
+| ceph | 40 | 0 | 0% | 0 | F054 |
+| certificates | 8 | 0 | 0% | 0 | F069 |
+| cluster | 77 | ~1 | 1% | 6 | Only Get-PveClusterResource |
+| cluster_config | 10 | 0 | 0% | 0 | F060 |
+| containers | 62 | ~25 | 40% | 1 | Good lifecycle coverage |
+| disks | 18 | 0 | 0% | 0 | F067 |
+| firewall | 40 | ~20 | 50% | 0 | All levels (cluster/node/VM/CT) via Level param; F059 resolved |
+| ha | 21 | 0 | 0% | 5 | F053 |
+| metrics | 7 | 0 | 0% | 0 | |
+| networking | 7 | 5 | 71% | 0 | Good coverage |
+| nodes | 75 | ~8 | 11% | 11 | Basic ops covered |
+| other | 1 | 0 | 0% | 0 | |
+| pools | 7 | 5 | 71% | 0 | CRUD + update |
+| replication | 5 | 0 | 0% | 0 | |
+| roles | 5 | 4 | 80% | 0 | CRUD complete |
+| sdn | 60 | ~19 | 32% | 16 | Zones/vnets/subnets/IPAM/DNS/ctrl |
+| services | 7 | 0 | 0% | 0 | Node service management |
+| storage | 19 | ~11 | 58% | 1 | Good coverage |
+| storage_config | 5 | 0 | 0% | 0 | Separate from storage CRUD |
+| tasks | 5 | 4 | 80% | 0 | Good coverage |
+| users | 12 | 4 | 33% | 0 | CRUD only |
+| version | 1 | 1 | 100% | 0 | Via Connect-PveServer |
+| vms | 97 | ~30 | 31% | 1 | Good lifecycle + guest agent |
+| **TOTAL** | **646** | **~155** | **~24%** | **42** | |
 
-### API Drift Table
+### API Drift — PVE 9.0 Breaking Changes
 
-No breaking changes detected in endpoints the module currently implements. All changes are additive (new optional parameters).
+24 endpoints have breaking parameter changes in PVE 9.0. Most are `link[n]` type changes on cluster config endpoints (not covered by this module). Relevant changes affecting implemented endpoints:
 
-| Finding ID | Area | Change | PVE Version | Risk |
-|---|---|---|---|---|
-| F061 | HA rules | New `/cluster/ha/rules` subsystem | 9.0 | Additive |
-| F061 | Bulk actions | New `/cluster/bulk-action` | 9.0 | Additive |
-| F061 | SDN fabrics | New `/cluster/sdn/fabrics` | 9.0 | Additive |
-| — | Various | 77 additive parameter changes across implemented endpoints | 8.x–9.x | Additive |
+| Endpoint | Change | Module Cmdlet | Risk |
+|---|---|---|---|
+| POST /nodes/{node}/qemu | `machine` param type changed | New-PveVm | Low |
+| POST /nodes/{node}/qemu/{vmid}/config | param changes | Set-PveVmConfig | Low |
+| PUT /nodes/{node}/qemu/{vmid}/config | param changes | Set-PveVmConfig | Low |
+| POST /nodes/{node}/qemu/{vmid}/status/start | param changes | Start-PveVm | Low |
+| POST /nodes/{node}/lxc | param changes | New-PveContainer | Low |
+| PUT /nodes/{node}/lxc/{vmid}/config | param changes | Set-PveContainerConfig | Low |
+| POST /cluster/backup | param changes | New-PveBackupJob | Low |
+| PUT /cluster/backup/{id} | param changes | Set-PveBackupJob | Low |
+| PUT /cluster/firewall/options | `log_ratelimit` changed | Set-PveFirewallOptions | Low |
+| POST /nodes/{node}/vzdump | param changes | New-PveBackup | Low |
+| PUT /nodes/{node}/network | param changes | Set-PveNetwork | Low |
+| PUT /nodes/{node}/config | param changes | Set-PveNodeConfig | Low |
+| POST /storage | param changes | New-PveStorage | Medium |
+| PUT /storage/{storage} | param changes | Set-PveStorage | Medium |
 
-### High-Value Uncovered Endpoints
+**Assessment**: No high-risk breaking changes for existing cmdlets. The `link[n]` parameter type changes affect cluster config endpoints not yet implemented. Storage param changes add new storage backend options (additive in practice).
 
-**Priority 1 — Commonly needed:**
-- `GET /access/acl` — List ACLs (only PUT covered)
-- `GET /cluster/nextid` — Get next free VM ID
-- `GET /cluster/ha/resources` — HA resource management
-- VNC/SPICE console proxy endpoints
-- `GET /nodes/{node}/subscription` — Subscription status
+### PVE 9.0 New Endpoints (42 total, 0 implemented)
 
-**Priority 2 — Frequently useful:**
-- `GET /nodes/{node}/syslog` / `journal` — Log access
-- `GET /nodes/{node}/hardware` — PCI passthrough info
-- `GET /nodes/{node}/scan/*` — Storage discovery
-- `POST /access/domains/{realm}/sync` — LDAP/AD sync
-- APT package management
+| Area | Count | Notable Endpoints |
+|---|---|---|
+| SDN | 16 | Fabric management (new subsystem in 9.0) |
+| Nodes | 11 | Service state, vzdump defaults, disk management |
+| Cluster | 6 | SDN apply, HA resources/rules, metrics export |
+| HA | 5 | Rules CRUD + resource migrate/relocate |
+| VMs | 1 | Migration endpoint enhancement |
+| Containers | 1 | Migration endpoint enhancement |
+| Storage | 1 | Storage endpoint enhancement |
+| Access | 1 | Access endpoint enhancement |
+
+### High-Value Gaps
+
+1. **Ceph management** (40 endpoints) — Critical for hyperconverged deployments
+2. **HA management** (21 endpoints, 5 new in 9.0) — Critical for production clusters
+3. **VM/CT-level firewall** (~23 endpoints) — Cluster-level done, per-VM/CT missing
+4. **Disk management** (18 endpoints) — ZFS, LVM, directory operations
+5. **Cluster configuration** (10 endpoints) — Join/create/manage cluster
 
 ---
 
 ## Phase 3 — Code Quality & Best Practices
 
-### DECISIONS.md Compliance
+### 3a. PowerShell Module Design
 
-| Decision | Status | Notes |
-|---|---|---|
-| D001 — TaskService.WaitForTask | ✅ Compliant | No inline polling loops in cmdlets |
-| D002 — SecureString passwords | ✅ Compliant | All 6 password params use SecureString |
-| D003 — URL encoding | ❌ **REGRESSED (F050)** | 20 instances in 5 service files |
-| D004 — No bare catch blocks | ✅ Compliant | All catches are specific or filtered |
-| D005 — OutputType on all cmdlets | ✅ Compliant | All 169 cmdlets have OutputType |
-| D006 — ConfirmImpact.High | ✅ Compliant | All destructive cmdlets confirmed |
-| D007 — Sealed cmdlet classes | ✅ Compliant | All 169 cmdlets sealed |
-| D008 — Newtonsoft.Json only | ✅ Compliant | No System.Text.Json attributes |
-| D009 — Framework targets | ✅ Compliant | netstandard2.0 / net10.0;net48 |
-| D010 — VmId ValidateRange | ✅ Compliant | All VmId params validated |
-| D011 — Verb class constants | ✅ Compliant | No string literals |
-| D012 — Magic string constants | ✅ Compliant | Auth headers extracted |
+All 169 cmdlets verified:
+- All cmdlet classes are `sealed`
+- All cmdlets have `[OutputType]` attribute
+- All destructive cmdlets have `ConfirmImpact = ConfirmImpact.High` (including Restart/Suspend-PveContainer, F062/F063 resolved)
+- VmId parameters use `[ValidateRange(100, 999999999)]`
+- Verb class constants used (not string literals)
+- ShouldProcess on destructive cmdlets
+- HelpMessage on parameters
+- Pipeline support via `ValueFromPipelineByPropertyName`
 
-### D003 Regression Detail (F050)
+### 3b. C# Code Quality
 
-20 instances of missing `Uri.EscapeDataString()` across 5 service files:
+- No inline task-polling loops — all cmdlets use `TaskService.WaitForTask` (F032/F033/F036/F058 resolved)
+- **REGRESSION F039**: Two bare `catch {}` blocks found (see below)
+- Password parameters use `SecureString` (F051 resolved)
+- URL paths use `Uri.EscapeDataString()` — 164 occurrences across 36 files (F050/F071 resolved)
+- No `System.Text.Json` attributes (F044 resolved)
+- Magic strings extracted to constants (F049 resolved)
+- `GetAwaiter().GetResult()` sync-over-async pattern — accepted per F048/wont_fix
+- Cryptographic RNG for boundary generation (F026 resolved)
 
-| File | Methods Affected | Unescaped Params |
-|---|---|---|
-| NodeService.cs | 7 (GetNodeStatus, GetNodeConfig, SetNodeConfig, GetDns, SetDns, StartAll, StopAll) | `node` |
-| NetworkService.cs | 5 node-network (GetNetworks, CreateNetwork, SetNetwork, RemoveNetwork, ApplyNetworkConfig) | `node`, `iface` |
-| NetworkService.cs | 2 SDN (RemoveSdnZone, RemoveSdnVnet) | `zone`, `vnet` |
-| CloudInitService.cs | 3 (GetCloudInitConfig, SetCloudInitConfig, RegenerateCloudInitImage) | `node` |
-| TaskService.cs | 2 (GetTask, GetTaskLog) | `node` |
-| TemplateService.cs | 1 (CreateTemplate) | `node` |
+**InvokePveVmGuestExecCmdlet** has an inline `do/while` loop for guest exec status polling. This is polling the QEMU guest agent exec status endpoint (not a PVE UPID task), with a proper timeout via `Stopwatch` + `TimeSpan.FromSeconds(Timeout)`. Correctly does not use `TaskService.WaitForTask`.
 
-Properly escaped services (no issues): VmService, ContainerService, SnapshotService, FirewallService, PoolService, UserService, StorageService.
+### 3c. General Hygiene
 
-### Other Code Quality Findings
+- No TODO/FIXME/HACK markers in source
+- No dead code or commented-out blocks found
+- Framework targeting correct: publishable projects use `netstandard2.0`, test project uses `net10.0;net48`
+- `.editorconfig` present with consistent style rules
+- 169 markdown cmdlet docs matching cmdlet count
 
-- No TODO/FIXME/HACK comments
-- No commented-out code
-- No unused using directives detected
-- HttpClient lifecycle is well-managed (1:1 ownership, proper disposal)
-- `PveCmdletBase.WaitForStatusTransition` power-state polling loop is distinct from task polling (D001 compliant)
+### 3d. HttpClient Lifecycle
+
+`PveHttpClient` creates a new `HttpClient` per instance. Services use `_injectedClient ?? new PveHttpClient(session)` — per-call when no injected client. F045 resolved by adding DI support (IPveHttpClient interface). The per-call pattern in production is acceptable for a PowerShell module where sessions are short-lived.
+
+### F039 — Bare Catch Blocks (REGRESSED)
+
+Two bare `catch` blocks violating D004 were found:
+
+1. **VmService.PingGuestAgent** (`VmService.cs:553`): `catch { return false; }` — catches all exceptions including `OutOfMemoryException`. Should use `catch (Exception ex) when (ex is not OutOfMemoryException and not StackOverflowException)`.
+
+2. **ImportPveOvaCmdlet** (`ImportPveOvaCmdlet.cs:277`): `catch { /* fallback */ }` — bare catch in VM retrieval fallback after OVA import. Same fix needed.
+
+### Code Quality Findings
+
+| Finding ID | Severity | Status | Description |
+|---|---|---|---|
+| F039 | Medium | **Regressed** | 2 bare catch blocks in VmService + ImportPveOvaCmdlet |
+| F032 | Critical | Resolved | Inline task polling → WaitForTask |
+| F033 | Critical | Resolved | Guest exec polling → timeout added |
+| F058 | Critical | Resolved | Container/storage polling → WaitForTask |
+| F041 | Medium | Resolved | Unsealed cmdlets → all sealed |
+| F037 | Medium | Resolved | Missing OutputType → all added |
+| F044 | Medium | Resolved | Dual JSON attributes → Newtonsoft only |
+| F045 | Medium | Resolved | HttpClient per-call → DI support added |
+| F062 | Medium | Resolved | Restart-PveContainer ConfirmImpact → High |
+| F063 | Medium | Resolved | Suspend-PveContainer ConfirmImpact → High |
 
 ---
 
 ## Phase 4 — Testing Coverage Analysis
 
-### Test Summary
+### Test Infrastructure
 
-| Category | Files | Tests |
+| Component | Framework | Location | Count |
+|---|---|---|---|
+| xUnit unit tests | xunit 2.9.3, Moq 4.20.72 | tests/PSProxmoxVE.Core.Tests/ | ~12 test files |
+| Pester unit tests | Pester 5.x | tests/PSProxmoxVE.Tests/ | ~30 test files |
+| Integration tests | Pester 5.x | tests/PSProxmoxVE.Tests/Integration/ | 2 files |
+
+### xUnit Coverage
+
+Tests cover:
+- **Authentication**: PveAuthenticator, PveSession, PveVersion
+- **Model deserialization**: All model areas (Backup, Cluster, Container, Firewall, Network, Node, SDN, Snapshot, Storage, Task, User, VM)
+- **Services**: Backup, CloudInit, Cluster, Node, Pool, Snapshot, Storage, Task, Template, User
+
+**Gap (F046)**: 5 services lack xUnit tests: VmService, ContainerService, FirewallService, NetworkService, SdnService. These are the most complex service classes and are only covered at the Pester cmdlet-existence level and integration tests.
+
+**Additional test quality observations:**
+- No error-response handling tests (HTTP 400/401/403/500 responses not exercised)
+- PoolServiceTests lacks null-guard tests (unlike all other service tests)
+- No negative integration tests (duplicate resource creation, nonexistent resource removal)
+- Integration gaps: Pool CRUD, Group/Domain CRUD, Cluster resources, most SDN Set cmdlets, guest agent extensions, Node config/DNS
+
+### Pester Test Coverage
+
+| Area | Unit Tests | Integration Tests |
 |---|---|---|
-| xUnit (C# unit) | 25 | 374 |
-| Pester (PS unit) | 48 | 1,268 |
-| Pester (integration) | 1 | 107 |
-| **Total** | **74** | **1,749** |
+| Connection | Yes | Yes |
+| VMs | Yes | Yes |
+| Containers | Yes | Yes |
+| Storage | Yes | Yes |
+| Network/SDN | Yes | Yes |
+| Firewall | Yes | Yes |
+| Backup | Yes | Yes |
+| Tasks | Yes | Yes |
+| Users/Roles | Yes | Yes |
+| Pools | Yes | Yes |
+| Templates | Yes | Yes |
+| Nodes | Yes | Yes |
+| Snapshots | Yes | Yes |
+| CloudInit | Yes | Yes |
+| OVA Import | Yes | Yes |
 
-### Coverage Highlights
+### Test Quality
 
-- **100% cmdlet unit test coverage** — all 169 cmdlets have Pester parameter/metadata tests
-- **xUnit service tests**: UserService (78), StorageService (40), FirewallModel (24), BackupService (23), SdnModel (21)
-- **32 JSON fixture files** from real PVE 8/9 API responses for model deserialization tests
-- **Integration tests**: 107 tests across connection, nodes, VMs, containers, snapshots, storage, users, firewall, SDN, backup, templates, cloud-init, guest agent
-
-### Coverage Gaps (F046)
-
-| Gap | Severity | Detail |
-|---|---|---|
-| No VmService xUnit tests | Medium | VM ops (clone, migrate, import) have complex logic untested at service layer |
-| No ContainerService xUnit tests | Medium | Similar complexity to VmService |
-| No NetworkService xUnit tests | Low | Thin CRUD wrappers |
-| No FirewallService xUnit tests | Low | Thin CRUD wrappers |
-| Pool/Cluster cmdlets lack integration tests | Low | Simple read-only operations |
+- Pester tests use Describe/Context/It structure with descriptive names
+- Integration tests tagged with `@("Integration")` for filtering
+- Connection details injected via environment variables
+- Integration tests cover both PVE 8 and PVE 9
+- xUnit tests use Moq for HTTP client mocking with realistic fixtures
+- Test cleanup via AfterAll blocks
 
 ---
 
 ## Phase 4b — CI Integration Test Results
 
-### Last CI Run
+### Most Recent Completed Run
 
 | Field | Value |
 |---|---|
-| Run ID | 23461565300 |
-| Conclusion | **SUCCESS** |
-| Date | 2026-03-23T21:42:02Z |
-| SHA | `1faaba0` |
+| Run ID | 23511293737 |
+| Conclusion | failure |
+| Created | 2026-03-24T20:42:58Z |
+| Head SHA | 0f3e2c1 |
 | Branch | main |
 
-All 7 jobs passed: container-image, build, provision, test(8), test(9), cleanup, cleanup-images.
+**Root cause**: Cloud image download race condition — `mv: cannot stat '...noble-server-cloudimg-amd64.qcow2.downloading'`. Transient infrastructure issue. Prior run (23511223201) succeeded. Newer run (23511879409) in progress.
 
-### CI Findings
-
-| Finding ID | Test / Cmdlet | Status | Notes |
-|---|---|---|---|
-| F080 | Restart-PveContainer | Resolved (scan-6) | ConfirmImpact.High fix applied |
-| F081 | Copy-PveContainer | Resolved (scan-6) | Cascade from F080 |
-| F082 | PVE 8 Docker image | **Resolved (this scan)** | Container-image job now succeeds |
+**No test failures** — tests were skipped due to provision failure.
 
 ---
 
 ## Phase 5 — Security Review
 
-| Finding ID | Area | File(s) | Severity | Status | Description |
-|---|---|---|---|---|---|
-| F050 | URL encoding | 5 service files | Medium | **Regressed** | 20 instances of unescaped path params (see Phase 3) |
-| F031 | Secrets | terraform.tfvars, CI | Low | Open | Lab IPs in terraform.tfvars (gitignored); CI uses ${{ secrets.* }} properly |
-| — | Credential handling | All password cmdlets | Pass | — | SecureString with Marshal try/finally throughout |
-| — | TLS/HTTPS | PveHttpClient.cs | Pass | — | HTTPS-only, SkipCertificateCheck opt-in with warning |
-| — | CSRF | PveHttpClient.cs | Pass | — | CSRFPreventionToken on all mutating requests |
-| — | Bare catches | All .cs files | Pass | — | No D004 violations |
-| — | Boundary generation | PveHttpClient.cs | Pass | — | Uses RandomNumberGenerator (CSPRNG) |
-| — | Dependencies | Both .csproj | Pass | — | Newtonsoft.Json 13.0.3, SharpCompress 0.47.3 (current) |
+| Finding ID | Area | Status | Notes |
+|---|---|---|---|
+| — | Credential handling | Pass | SecureString for all passwords, PSCredential for Connect |
+| F084 | Session object exposure | **New** | PveSession exposes Ticket/ApiToken/CsrfToken in default pipeline output |
+| — | TLS/HTTPS | Pass | Enforced by default, SkipCertificateCheck opt-in with warning |
+| — | Input validation | Pass | Uri.EscapeDataString on all path params, ValidateRange on IDs |
+| — | Secret scanning | Pass | No secrets in committed files, .gitignore covers sensitive files |
+| — | Dependency security | Pass | All packages current, Dependabot configured |
+
+### F084 — PveSession Exposes Auth Secrets in Pipeline Output (NEW)
+
+`PveSession.Ticket`, `ApiToken`, and `CsrfToken` are public `string` properties with no format-file hiding. When `Connect-PveServer -PassThru` or `Test-PveConnection -Detailed` writes the session to the pipeline, all secret fields are visible in the default table/list view. A user piping `$session | Format-List *` or logging verbose output could inadvertently expose auth tokens.
+
+**Fix**: Add a `PSProxmoxVE.format.ps1xml` entry for `PveSession` that shows only `Hostname`, `Port`, `AuthMode`, `IsExpired`, and `ServerVersion` by default.
 
 ---
 
@@ -237,72 +320,87 @@ All 7 jobs passed: container-image, build, provision, test(8), test(9), cleanup,
 
 | Finding ID | Check | Pass/Fail | Notes |
 |---|---|---|---|
-| — | ModuleVersion | Pass | 0.1.0, dynamically set from git tag |
-| — | GUID | Pass | Present and unique |
-| — | Author/CompanyName | Pass | goodolclint / Worklab |
-| — | Description | Pass | Descriptive, mentions PVE 8.x/9.x |
-| — | PowerShellVersion | Pass | 5.1 minimum |
-| — | CompatiblePSEditions | Pass | Desktop + Core |
-| — | CmdletsToExport | Pass | Explicit list of 169 cmdlets |
-| — | Tags | Pass | 8 tags for discoverability |
-| — | LicenseUri/ProjectUri | Pass | GitHub links |
-| F021 | IconUri | **Fail** | Not set — PSGallery shows placeholder |
+| — | RootModule | Pass | PSProxmoxVE.dll |
+| — | ModuleVersion | Pass | 0.1.0 (updated from tag by publish workflow) |
+| — | GUID | Pass | a3f7c2d1-84e5-4b9f-a061-3e2d8c5f1a7b |
+| — | Author | Pass | goodolclint |
+| — | Description | Pass | Comprehensive description |
+| — | PowerShellVersion | Pass | 5.1 |
+| — | CompatiblePSEditions | Pass | Desktop, Core |
+| — | CmdletsToExport | Pass | 169 cmdlets explicitly listed |
+| — | Tags | Pass | 8 relevant tags |
+| — | LicenseUri | Pass | GitHub MIT license link |
+| — | ProjectUri | Pass | GitHub repo link |
 | — | ReleaseNotes | Pass | Present in PSData |
-| — | Framework targets | Pass | netstandard2.0 for both publishable projects |
-| — | Publish workflow | Pass | Tag-triggered, 3-job pipeline with PS 5.1 smoke test |
-| — | CHANGELOG | Pass | Keep a Changelog format |
+| F021 | IconUri | Fail | Not set — recommended for PSGallery listing |
+| — | Prerelease | Pass | 'preview' — appropriate for current state |
+| — | Framework target | Pass | netstandard2.0 for publishable projects |
+| — | PS 5.1 smoke test | Pass | Verifies >= 150 commands load |
+| — | MAML help | Pass | PSProxmoxVE.dll-Help.xml present |
 
 ---
 
 ## Phase 7 — Community & Repo Maintenance Standards
 
-| Finding ID | Check | Pass/Fail | Notes |
+| Check | Pass/Fail | Notes |
+|---|---|---|
+| CONTRIBUTING.md | Pass | Development setup, PR process |
+| CODE_OF_CONDUCT.md | Pass | Contributor Covenant |
+| SECURITY.md | Pass | Security policy |
+| DECISIONS.md | Pass | 12 active decisions |
+| CODEOWNERS | Pass | @goodolclint |
+| Issue templates | Pass | Bug report + feature request + config.yml |
+| PR template | Pass | Present |
+| Dependabot | Pass | NuGet + GitHub Actions weekly |
+| Branch protection | Pass | Required checks + review |
+| Commit conventions | Pass | Conventional commits |
+| Release process | Pass | Tag-triggered publish |
+| CHANGELOG | Pass | Keep-a-Changelog |
+
+---
+
+## Phase 8 — Findings Database Update
+
+### Verification of DECISIONS.md Patterns
+
+| Decision | Pattern | Verified | Method |
 |---|---|---|---|
-| — | Bug report template | Pass | YAML form with version fields |
-| — | Feature request template | Pass | YAML form with use case |
-| — | Issue template config | Pass | Disables blank issues, links to Discussions |
-| — | PR template | Pass | Checklist with build, tests, psd1, changelog |
-| — | CONTRIBUTING.md | Pass | Complete dev setup and coding standards |
-| — | CODE_OF_CONDUCT.md | Pass | Contributor Covenant 2.1 |
-| — | SECURITY.md | Pass | Responsible disclosure instructions |
-| — | DECISIONS.md | Pass | 12 decisions, linked from CLAUDE.md |
-| — | CODEOWNERS | Pass | `* @goodolclint` |
-| — | .editorconfig | Pass | Per-filetype overrides |
-| — | .gitattributes | Pass | LF enforcement, binary handling |
-| — | Commit conventions | Pass | Conventional Commits consistently used |
-| — | Release process | Pass | Tag-based publish with GitHub Release |
-| — | Dependabot | Pass | Automated dependency PRs |
+| D001 | TaskService.WaitForTask | Yes | grep `while(true)` — only in TaskService + WaitPveTask cmdlet |
+| D002 | SecureString passwords | Yes | grep `public string Password` — none found |
+| D003 | Uri.EscapeDataString | Yes | 164 occurrences across 36 files |
+| D004 | No bare catches | **FAIL** | 2 bare catches found: VmService.cs:553, ImportPveOvaCmdlet.cs:277 → F039 regressed |
+| D005 | OutputType on all cmdlets | Yes | 169 cmdlets, 169 OutputType attributes |
+| D006 | ConfirmImpact.High | Yes | All destructive cmdlets including Restart/Suspend-Container |
+| D007 | Sealed cmdlet classes | Yes | All 169 classes sealed |
+| D008 | Newtonsoft.Json only | Yes | No System.Text.Json references in src/ |
+| D009 | netstandard2.0 targets | Yes | Both publishable projects target netstandard2.0 |
+| D010 | ValidateRange on VmId | Yes | All VmId parameters have ValidateRange |
+| D011 | Verb class constants | Yes | No string literal verbs found |
+| D012 | Magic string constants | Yes | ApiTokenPrefix, CsrfHeaderName constants in PveHttpClient |
+
+**1 regression detected**: F039 (bare catch blocks) — D004 violated in 2 locations. No new decisions needed.
 
 ---
 
 ## Phase 9 — Prioritized Recommendations
 
-### 🔴 Critical
-
-No critical findings.
-
-### 🟠 High
-
-No high findings.
-
-### 🟡 Medium
+### Medium (5 findings)
 
 | Finding ID | What | Where | Why | Fix |
 |---|---|---|---|---|
-| F050 | **Uri.EscapeDataString missing (REGRESSED)** | NodeService, NetworkService, CloudInitService, TaskService, TemplateService | D003 violation — defense-in-depth URL encoding missing on 20 path parameter interpolations | Add `Uri.EscapeDataString()` to all `node`, `iface`, `zone`, `vnet` params in URL paths |
-| F046 | Integration test coverage gaps | tests/ | VmService, ContainerService lack xUnit service-layer tests; Pools/Cluster lack integration tests | Add xUnit tests for VmService and ContainerService |
-| F059 | VM/CT-level firewall 0% coverage | — | Firewall endpoints exist for VMs and containers but no cmdlets implemented | Add VM/CT-scoped firewall cmdlets |
-| F060 | Cluster config 0% coverage | — | Cluster options, join/create, notifications, metrics uncovered | Add cluster config cmdlets |
-| F061 | PVE 9.0 endpoints 0% coverage | — | 42 new endpoints (HA rules, bulk actions, SDN fabrics) not implemented | Prioritize HA rules and bulk actions |
+| F039 | **REGRESSED** Bare catch blocks | VmService.cs:553, ImportPveOvaCmdlet.cs:277 | Catches all exceptions including OOM/SOE, violates D004 | Replace with `catch (Exception ex) when (ex is not OutOfMemoryException and not StackOverflowException)` |
+| F084 | PveSession exposes auth secrets | PveSession.cs, format.ps1xml | Ticket/ApiToken/CsrfToken visible in pipeline output | Add format.ps1xml entry hiding secret properties |
+| F046 | Integration test coverage gaps | tests/ | VmService, ContainerService lack xUnit tests | Add xUnit tests with Moq for complex services |
+| F060 | Cluster config 0% | src/ | Cannot manage clusters | Add cluster config cmdlets |
+| F061 | PVE 9.0 endpoints 0% | src/ | 42 new endpoints, none implemented | Prioritize SDN fabrics + HA rules |
 
-### 🟢 Low
+### Low (6 findings)
 
 | Finding ID | What | Where | Why | Fix |
 |---|---|---|---|---|
-| F021 | No IconUri in manifest | PSProxmoxVE.psd1 | PSGallery listing looks less professional | Add 128x128 PNG icon and set IconUri |
-| F031 | Hardcoded test IPs | terraform.tfvars, CI | Lab IPs committed (gitignored locally) | Verify tfvars not tracked in git |
-| F053 | HA subsystem 0% coverage | — | No HA cmdlets | Implement when HA rules (PVE 9.0) are prioritized |
-| F054 | Ceph subsystem 0% coverage | — | Large specialized surface | Low priority unless Ceph users identified |
-| F067 | Disk management 0% coverage | — | Node disk ops (LVM, ZFS, init) | Useful for provisioning workflows |
-| F068 | Notifications 0% coverage | — | 25+ endpoints (PVE 8.1+) | Medium-value for operational users |
-| F069 | ACME/Certificates 0% coverage | — | Certificate management | Low priority |
+| F021 | No IconUri in manifest | PSProxmoxVE.psd1 | PSGallery listing lacks icon | Add IconUri pointing to project logo |
+| F053 | HA subsystem 0% | src/ | 21 endpoints for HA management | Implement HA group/resource/rule cmdlets |
+| F054 | Ceph subsystem 0% | src/ | 40 endpoints for hyperconverged | Implement Ceph pool/OSD/monitor cmdlets |
+| F067 | Disk management 0% | src/ | ZFS/LVM/directory ops missing | Implement disk management cmdlets |
+| F068 | Notifications 0% | src/ | Notification targets not managed | Implement notification cmdlets |
+| F069 | ACME/Certificates 0% | src/ | Let's Encrypt + cert mgmt missing | Implement ACME/cert cmdlets |

docs/review/findings.json

@@ -1,16 +1,16 @@
 {
   "_schema_version": "1.0",
   "_description": "PSProxmoxVE stable findings ledger. IDs are permanent (F001, F002...). Resolved findings are never deleted — they are marked resolved with evidence. If a finding reappears, it is marked regressed and retains its original ID.",
-  "last_updated": "2026-03-23",
-  "last_scan_date": "2026-03-23",
+  "last_updated": "2026-03-24",
+  "last_scan_date": "2026-03-24",
   "counters": {
-    "next_id": 84,
+    "next_id": 85,
     "total_open": 10,
     "total_resolved": 72,
-    "total_regressed": 0,
+    "total_regressed": 1,
     "open": 10,
     "resolved": 72,
-    "regressed": 0,
+    "regressed": 1,
     "wont_fix": 1
   },
   "findings": [
@@ -1187,15 +1187,13 @@
       "title": "Bare catch blocks in PveHttpClient, PveCmdletBase, VmService, ContainerService, GetPveVmCmdlet",
       "category": "code_quality",
       "severity": "medium",
-      "status": "resolved",
+      "status": "regressed",
       "first_detected": "2026-03-21",
       "files": [
-        "src/PSProxmoxVE.Core/Client/PveHttpClient.cs",
-        "src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs",
         "src/PSProxmoxVE.Core/Services/VmService.cs",
-        "src/PSProxmoxVE.Core/Services/ContainerService.cs"
+        "src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs"
       ],
-      "description": "Multiple bare catch {} blocks swallow exceptions silently, hiding errors and making debugging difficult.",
+      "description": "Two bare catch {} blocks found: VmService.PingGuestAgent (line 553) and ImportPveOvaCmdlet (line 277). Both swallow all exceptions including OutOfMemoryException/StackOverflowException.",
       "scan_history": [
         {
           "scan_date": "2026-03-21",
@@ -1206,6 +1204,11 @@
           "scan_date": "2026-03-22",
           "local_id": null,
           "status": "fixed"
+        },
+        {
+          "scan_date": "2026-03-24",
+          "local_id": null,
+          "status": "regressed"
         }
       ],
       "resolution": {
@@ -1213,7 +1216,13 @@
         "evidence": "All bare catches replaced with filtered catch (Exception ex) when (...) clauses that exclude OOM/SOE and are specific to PveApiException/HttpRequestException where appropriate",
         "verified_by": "self-reported"
       },
-      "decisions_ref": "D005"
+      "regression_history": [
+        {
+          "regressed_in_scan": "2026-03-24",
+          "description": "Bare catch blocks reintroduced in VmService.PingGuestAgent (line 553) and ImportPveOvaCmdlet VM retrieval fallback (line 277). Both should use filtered catch per D004."
+        }
+      ],
+      "decisions_ref": "D004"
     },
     {
       "id": "F040",
@@ -1861,9 +1870,12 @@
       "title": "VM/CT-level firewall 0% coverage",
       "category": "api_coverage",
       "severity": "medium",
-      "status": "open",
+      "status": "resolved",
       "first_detected": "2026-03-22",
-      "files": [],
+      "files": [
+        "src/PSProxmoxVE.Core/Services/FirewallService.cs",
+        "src/PSProxmoxVE/Cmdlets/Firewall/"
+      ],
       "description": "~44 combined endpoints for per-VM and per-CT firewall rules. Cluster-level is covered (21 cmdlets) but VM/CT-level is not.",
       "scan_history": [
         {
@@ -1880,8 +1892,19 @@
           "scan_date": "2026-03-23",
           "local_id": null,
           "status": "open"
+        },
+        {
+          "scan_date": "2026-03-24",
+          "local_id": null,
+          "status": "resolved"
         }
-      ]
+      ],
+      "resolution": {
+        "scan_date": "2026-03-24",
+        "report_id": "scan-8",
+        "verified_by": "scan",
+        "evidence": "All 17 firewall cmdlets support a Level parameter (Cluster/Node/Vm/Container). FirewallService.cs line 12 documents scope support. SetPveFirewallRuleCmdlet accepts Level=Vm/Container with Node and VmId params. Same pattern in all firewall cmdlets (rules, aliases, ipsets, options, refs)."
+      }
     },
     {
       "id": "F060",
@@ -2653,6 +2676,26 @@
         "verified_by": "code_change",
         "evidence": "CONTRIBUTING.md line 9 changed from .NET SDK 9.0+ to .NET SDK 10.0+, matching all CI workflows and test project TFM."
       }
+    },
+    {
+      "id": "F084",
+      "title": "PveSession exposes auth secrets in default pipeline output",
+      "category": "security",
+      "severity": "medium",
+      "status": "open",
+      "first_detected": "2026-03-24",
+      "files": [
+        "src/PSProxmoxVE.Core/Authentication/PveSession.cs",
+        "src/PSProxmoxVE/PSProxmoxVE.format.ps1xml"
+      ],
+      "description": "PveSession.Ticket, ApiToken, and CsrfToken are public string properties with no format-file hiding. When Connect-PveServer -PassThru or Test-PveConnection -Detailed writes the session to the pipeline, all secret fields are visible in default table/list view. A user piping $session | Format-List * or logging verbose output could inadvertently expose auth tokens. Add a PSProxmoxVE.format.ps1xml entry for PveSession that shows only Hostname, Port, AuthMode, IsExpired, and ServerVersion by default.",
+      "scan_history": [
+        {
+          "scan_date": "2026-03-24",
+          "local_id": null,
+          "status": "new"
+        }
+      ]
     }
   ]
 }