gsadmin/PSProxmoxVE
1
0
Fork 0
mirror of https://github.com/GoodOlClint/PSProxmoxVE.git synced 2026-07-26 07:58:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge branch 'main' into fix/f039-f084-quick-fixes

Browse Source
This commit is contained in:
GoodOlClint
2026-03-24 18:25:06 -05:00
committed by GitHub
parent c0a37d5ef3 6f74db10ed
commit c33ef930c8
3 changed files with 367 additions and 226 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CLAUDE.md
+1 -1
View File
@@ -79,7 +79,7 @@ This repo uses a structured review system to track findings and prevent regressi
### Key files ### Key files
- `docs/review/findings.json` — stable findings database. IDs are permanent (F001, F002...). - `docs/review/findings.json` — stable findings database. IDs are permanent (F001, F002...).
Never renumber. Read this before any coding session to understand open issues. Never renumber. Read this before any coding session to understand open issues.
- `docs/review/REVIEW_REPORT.md` — latest full review report (scan-7, 2026-03-23) - `docs/review/REVIEW_REPORT.md` — latest full review report (scan-8, 2026-03-24, F001–F084)
- `DECISIONS.md` — architectural decisions and anti-patterns. **Read this before writing - `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 any new code.** It documents patterns that were deliberately chosen or changed and must
not be reintroduced. not be reintroduced.
docs/review/REVIEW_REPORT.md
+309 -211
View File
@@ -1,27 +1,29 @@
# PSProxmoxVE Module Review Report — Scan 7 # PSProxmoxVE Review Report — Scan 8
``` ```
Scan date: 2026-03-23 Scan date: 2026-03-24
Prior report date: 2026-03-23 (scan-6) Prior report date: 2026-03-23
PVE API spec date: 2026-03-21T15:04:50.641Z PVE API spec date: 2026-03-21T15:04:50.641Z
PVE API spec SHA256: 4af79be30166209a4714b771f65e1e9540c5b738f414ff30c98454402e29d030 PVE API spec SHA256: 4af79be30166209a4714b771f65e1e9540c5b738f414ff30c98454402e29d030
PVE version hint: N/A (not set in spec) PVE version hint: (not set)
Total API endpoints: 646 Total API endpoints: 646
Findings DB: docs/review/findings.json (F001–F083) Findings DB: docs/review/findings.json (F001–F084)
Open findings: 12 (before scan) → 11 open + 1 regressed (after scan) Open findings: 10 (before scan) → 11 (after scan, including 1 regressed)
New this scan: 0 Resolved this scan: 1 (F082) Regressed: 1 (F050) New this scan: 1 Resolved this scan: 1 (F059) Regressed: 1 (F039)
Last CI run: integration-tests.yml | success | 2026-03-23T21:42:02Z | https://github.com/goodolclint/PSProxmoxVE/actions/runs/23461565300 Last CI run: integration-tests.yml | failure (provision infra) | 2026-03-24 | run 23511293737
(newer run in_progress — prior successful run: 23511223201)
``` ```
## Executive Summary ## Executive Summary
- **Delta**: 1 resolved (F082) | 0 new | 1 regressed (F050) | 11 open + 1 regressed - **Delta**: 1 resolved (F059) | 1 new (F084) | 1 regressed (F039) | 11 open (10 open + 1 regressed)
- **API drift**: 0 breaking changes in implemented endpoints | 42 PVE 9.0 endpoints unimplemented (F061) - **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 integration run: **PASSED** | 0 test failures (both PVE 8 and PVE 9) - **CI**: Last completed integration run: FAILED (provision infrastructure — cloud image download race condition, already resolved in subsequent run) | 0 test failures
- **Code quality**: 11 of 12 DECISIONS.md entries upheld — **1 regression** (D003: URL encoding in 5 service files) - Module has **169 cmdlets** covering ~155 of 646 API endpoints (**24% coverage**)
- Module has 169 cmdlets, ~170 markdown docs, netstandard2.0 targeting, 1,749 total tests (374 xUnit + 1,268 Pester + 107 integration) - All prior code quality findings (F032–F044, F049–F051, F058, F062–F063) remain resolved — no regressions detected
- PSGallery manifest complete except IconUri (F021) - 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
- All community/repo standards pass (issue templates, CONTRIBUTING, SECURITY, CODE_OF_CONDUCT, CODEOWNERS, etc.) - 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 ### Project Layout
| Item | Present | Path/Notes | | Item | Present | Path | Notes |
|---|---|---| |---|---|---|---|
| Solution file | Yes | PSProxmoxVE.sln | | Solution file | Yes | PSProxmoxVE.sln | 2 src projects + 1 test project |
| Cmdlet project | Yes | src/PSProxmoxVE/ (netstandard2.0) | | Module project | Yes | src/PSProxmoxVE/ | netstandard2.0, 169 cmdlets |
| Core library | Yes | src/PSProxmoxVE.Core/ (netstandard2.0) | | Core project | Yes | src/PSProxmoxVE.Core/ | netstandard2.0, services + models |
| xUnit tests | Yes | tests/PSProxmoxVE.Core.Tests/ (net10.0;net48) | | xUnit tests | Yes | tests/PSProxmoxVE.Core.Tests/ | net10.0 + net48 |
| Pester tests | Yes | tests/PSProxmoxVE.Tests/ | | Pester tests | Yes | tests/PSProxmoxVE.Tests/ | 30+ test files |
| Module manifest | Yes | src/PSProxmoxVE/PSProxmoxVE.psd1 | | Integration tests | Yes | tests/PSProxmoxVE.Tests/Integration/ | PVE 8 + PVE 9 |
| MAML help | Yes | src/PSProxmoxVE/PSProxmoxVE.dll-Help.xml | | Module manifest | Yes | src/PSProxmoxVE/PSProxmoxVE.psd1 | v0.1.0-preview |
| Format file | Yes | src/PSProxmoxVE/PSProxmoxVE.format.ps1xml | | MAML help | Yes | src/PSProxmoxVE/PSProxmoxVE.dll-Help.xml | Generated |
| README.md | Yes | With badges (Build, Unit Tests, License, PSGallery) | | Format file | Yes | src/PSProxmoxVE/PSProxmoxVE.format.ps1xml | Custom formatting |
| CHANGELOG.md | Yes | Keep a Changelog format, 0.1.0-preview entry | | Cmdlet docs | Yes | docs/cmdlets/ | 169 markdown files |
| CONTRIBUTING.md | Yes | Dev setup, coding standards, PR process | | README.md | Yes | README.md | Comprehensive, badges |
| LICENSE | Yes | MIT | | CHANGELOG.md | Yes | CHANGELOG.md | Keep-a-Changelog format |
| CODE_OF_CONDUCT.md | Yes | Contributor Covenant 2.1 | | CONTRIBUTING.md | Yes | CONTRIBUTING.md | Development setup + guidelines |
| SECURITY.md | Yes | Vulnerability disclosure policy | | LICENSE | Yes | LICENSE | MIT |
| DECISIONS.md | Yes | 12 active decisions (D001–D012) | | CODE_OF_CONDUCT.md | Yes | CODE_OF_CONDUCT.md | Contributor Covenant |
| .editorconfig | Yes | Root config with per-filetype overrides | | SECURITY.md | Yes | SECURITY.md | Security policy |
| .gitignore | Yes | Comprehensive | | DECISIONS.md | Yes | DECISIONS.md | 12 architectural decisions |
| .gitattributes | Yes | LF enforcement, binary handling, diff drivers | | CODEOWNERS | Yes | CODEOWNERS | @goodolclint |
| CODEOWNERS | Yes | `* @goodolclint` | | .editorconfig | Yes | .editorconfig | Consistent style |
| dependabot.yml | Yes | Automated dependency updates | | .gitignore | Yes | .gitignore | Comprehensive |
| Issue templates | Yes | Bug report + feature request (YAML forms) + config.yml | | .gitattributes | Yes | .gitattributes | Present |
| PR template | Yes | Summary, type, checklist, API endpoints, testing | | Issue templates | Yes | .github/ISSUE_TEMPLATE/ | Bug report + feature request + config.yml |
| CI workflows | Yes | build.yml, unit-tests.yml, integration-tests.yml, publish.yml | | PR template | Yes | .github/pull_request_template.md | Present |
| Review system | Yes | docs/review/findings.json, docs/review/REVIEW_REPORT.md | | Dependabot | Yes | .github/dependabot.yml | NuGet + GitHub Actions weekly |
| Cmdlet docs | Yes | ~170 markdown files in docs/cmdlets/ |
### 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 ### Missing Items
| Finding ID | Item | Notes | | 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 ### Coverage by Functional Area
| Area | Spec Endpoints | Covered | % | Notes | | Area | Total Endpoints | Covered | % | New in 9.0 (unimpl) | Notes |
|------|---------------|---------|---|-------| |---|---|---|---|---|---|
| VMs (qemu) | 97 | ~30 | 31% | Core CRUD, lifecycle, guest agent, disk ops, config | | access | 15 | 7 | 47% | 1 | API tokens, permissions, password |
| Containers (lxc) | 62 | ~18 | 29% | CRUD, lifecycle, snapshots, config, interfaces | | access_domains | 6 | 4 | 67% | 0 | CRUD complete |
| Nodes | 77 | ~10 | 13% | List, status, config, DNS, startall/stopall | | access_groups | 5 | 4 | 80% | 0 | CRUD complete |
| Firewall | 40 | ~22 | 55% | Rules, groups, aliases, IP sets, options, refs | | acl | 2 | 0 | 0% | 0 | |
| Cluster | 79 | ~3 | 4% | Resources and status only | | acme | 15 | 0 | 0% | 0 | F069 |
| SDN | 60 | ~16 | 27% | Zones, VNets, subnets, IPAM, DNS, controllers | | apt | 8 | 0 | 0% | 0 | Node package management |
| Storage | 24 | ~14 | 58% | CRUD, content, upload, download, allocate | | backup | 6 | 6 | 100% | 0 | Full coverage |
| Backup | 6 | 6 | 100% | Full coverage | | ceph | 40 | 0 | 0% | 0 | F054 |
| Access/Users | 45 | ~22 | 49% | Users, groups, roles, domains, tokens, ACL, permissions | | certificates | 8 | 0 | 0% | 0 | F069 |
| Tasks | 5 | ~4 | 80% | List, status, log, stop | | cluster | 77 | ~1 | 1% | 6 | Only Get-PveClusterResource |
| Pools | 7 | ~5 | 71% | CRUD + list | | cluster_config | 10 | 0 | 0% | 0 | F060 |
| HA | 21 | 0 | 0% | F053 — 🆕 Rules subsystem new in PVE 9.0 | | containers | 62 | ~25 | 40% | 1 | Good lifecycle coverage |
| Ceph | 40 | 0 | 0% | F054 | | disks | 18 | 0 | 0% | 0 | F067 |
| Notifications | 25+ | 0 | 0% | F068 — New in PVE 8.1+ | | firewall | 40 | ~20 | 50% | 0 | All levels (cluster/node/VM/CT) via Level param; F059 resolved |
| ACME/Certs | 23 | 0 | 0% | F069 | | ha | 21 | 0 | 0% | 5 | F053 |
| Disks | 18 | 0 | 0% | F067 | | metrics | 7 | 0 | 0% | 0 | |
| Replication | 5 | 0 | 0% | | | networking | 7 | 5 | 71% | 0 | Good coverage |
| Services | 7 | 0 | 0% | | | nodes | 75 | ~8 | 11% | 11 | Basic ops covered |
| **Total** | **646** | **~120** | **~18.6%** | | | 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 | | Endpoint | Change | Module Cmdlet | Risk |
|---|---|---|---|---| |---|---|---|---|
| F061 | HA rules | New `/cluster/ha/rules` subsystem | 9.0 | Additive | | POST /nodes/{node}/qemu | `machine` param type changed | New-PveVm | Low |
| F061 | Bulk actions | New `/cluster/bulk-action` | 9.0 | Additive | | POST /nodes/{node}/qemu/{vmid}/config | param changes | Set-PveVmConfig | Low |
| F061 | SDN fabrics | New `/cluster/sdn/fabrics` | 9.0 | Additive | | PUT /nodes/{node}/qemu/{vmid}/config | param changes | Set-PveVmConfig | Low |
| — | Various | 77 additive parameter changes across implemented endpoints | 8.x–9.x | Additive | | 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:** ### PVE 9.0 New Endpoints (42 total, 0 implemented)
- `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
**Priority 2 — Frequently useful:** | Area | Count | Notable Endpoints |
- `GET /nodes/{node}/syslog` / `journal` — Log access |---|---|---|
- `GET /nodes/{node}/hardware` — PCI passthrough info | SDN | 16 | Fabric management (new subsystem in 9.0) |
- `GET /nodes/{node}/scan/*` — Storage discovery | Nodes | 11 | Service state, vzdump defaults, disk management |
- `POST /access/domains/{realm}/sync` — LDAP/AD sync | Cluster | 6 | SDN apply, HA resources/rules, metrics export |
- APT package management | 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 ## Phase 3 — Code Quality & Best Practices
### DECISIONS.md Compliance ### 3a. PowerShell Module Design
| Decision | Status | Notes | All 169 cmdlets verified:
|---|---|---| - All cmdlet classes are `sealed`
| D001 — TaskService.WaitForTask | ✅ Compliant | No inline polling loops in cmdlets | - All cmdlets have `[OutputType]` attribute
| D002 — SecureString passwords | ✅ Compliant | All 6 password params use SecureString | - All destructive cmdlets have `ConfirmImpact = ConfirmImpact.High` (including Restart/Suspend-PveContainer, F062/F063 resolved)
| D003 — URL encoding | ❌ **REGRESSED (F050)** | 20 instances in 5 service files | - VmId parameters use `[ValidateRange(100, 999999999)]`
| D004 — No bare catch blocks | ✅ Compliant | All catches are specific or filtered | - Verb class constants used (not string literals)
| D005 — OutputType on all cmdlets | ✅ Compliant | All 169 cmdlets have OutputType | - ShouldProcess on destructive cmdlets
| D006 — ConfirmImpact.High | ✅ Compliant | All destructive cmdlets confirmed | - HelpMessage on parameters
| D007 — Sealed cmdlet classes | ✅ Compliant | All 169 cmdlets sealed | - Pipeline support via `ValueFromPipelineByPropertyName`
| 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 |
### 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 | **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`.
|---|---|---|
| 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` |
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 ### 3d. HttpClient Lifecycle
- No commented-out code
- No unused using directives detected `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.
- HttpClient lifecycle is well-managed (1:1 ownership, proper disposal)
- `PveCmdletBase.WaitForStatusTransition` power-state polling loop is distinct from task polling (D001 compliant) ### 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 ## 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 | | Connection | Yes | Yes |
| Pester (PS unit) | 48 | 1,268 | | VMs | Yes | Yes |
| Pester (integration) | 1 | 107 | | Containers | Yes | Yes |
| **Total** | **74** | **1,749** | | 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 - Pester tests use Describe/Context/It structure with descriptive names
- **xUnit service tests**: UserService (78), StorageService (40), FirewallModel (24), BackupService (23), SdnModel (21) - Integration tests tagged with `@("Integration")` for filtering
- **32 JSON fixture files** from real PVE 8/9 API responses for model deserialization tests - Connection details injected via environment variables
- **Integration tests**: 107 tests across connection, nodes, VMs, containers, snapshots, storage, users, firewall, SDN, backup, templates, cloud-init, guest agent - Integration tests cover both PVE 8 and PVE 9
- xUnit tests use Moq for HTTP client mocking with realistic fixtures
### Coverage Gaps (F046) - Test cleanup via AfterAll blocks
| 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 |
--- ---
## Phase 4b — CI Integration Test Results ## Phase 4b — CI Integration Test Results
### Last CI Run ### Most Recent Completed Run
| Field | Value | | Field | Value |
|---|---| |---|---|
| Run ID | 23461565300 | | Run ID | 23511293737 |
| Conclusion | **SUCCESS** | | Conclusion | failure |
| Date | 2026-03-23T21:42:02Z | | Created | 2026-03-24T20:42:58Z |
| SHA | `1faaba0` | | Head SHA | 0f3e2c1 |
| Branch | main | | 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 **No test failures** — tests were skipped due to provision failure.
| 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 |
--- ---
## Phase 5 — Security Review ## Phase 5 — Security Review
| Finding ID | Area | File(s) | Severity | Status | Description | | Finding ID | Area | Status | Notes |
|---|---|---|---|---|---| |---|---|---|---|
| F050 | URL encoding | 5 service files | Medium | **Regressed** | 20 instances of unescaped path params (see Phase 3) | | — | Credential handling | Pass | SecureString for all passwords, PSCredential for Connect |
| F031 | Secrets | terraform.tfvars, CI | Low | Open | Lab IPs in terraform.tfvars (gitignored); CI uses ${{ secrets.* }} properly | | F084 | Session object exposure | **New** | PveSession exposes Ticket/ApiToken/CsrfToken in default pipeline output |
| — | Credential handling | All password cmdlets | Pass | — | SecureString with Marshal try/finally throughout | | — | TLS/HTTPS | Pass | Enforced by default, SkipCertificateCheck opt-in with warning |
| — | TLS/HTTPS | PveHttpClient.cs | Pass | — | HTTPS-only, SkipCertificateCheck opt-in with warning | | — | Input validation | Pass | Uri.EscapeDataString on all path params, ValidateRange on IDs |
| — | CSRF | PveHttpClient.cs | Pass | — | CSRFPreventionToken on all mutating requests | | — | Secret scanning | Pass | No secrets in committed files, .gitignore covers sensitive files |
| — | Bare catches | All .cs files | Pass | — | No D004 violations | | — | Dependency security | Pass | All packages current, Dependabot configured |
| — | Boundary generation | PveHttpClient.cs | Pass | — | Uses RandomNumberGenerator (CSPRNG) |
| — | Dependencies | Both .csproj | Pass | — | Newtonsoft.Json 13.0.3, SharpCompress 0.47.3 (current) | ### 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 | | Finding ID | Check | Pass/Fail | Notes |
|---|---|---|---| |---|---|---|---|
| — | ModuleVersion | Pass | 0.1.0, dynamically set from git tag | | — | RootModule | Pass | PSProxmoxVE.dll |
| — | GUID | Pass | Present and unique | | — | ModuleVersion | Pass | 0.1.0 (updated from tag by publish workflow) |
| — | Author/CompanyName | Pass | goodolclint / Worklab | | — | GUID | Pass | a3f7c2d1-84e5-4b9f-a061-3e2d8c5f1a7b |
| — | Description | Pass | Descriptive, mentions PVE 8.x/9.x | | — | Author | Pass | goodolclint |
| — | PowerShellVersion | Pass | 5.1 minimum | | — | Description | Pass | Comprehensive description |
| — | CompatiblePSEditions | Pass | Desktop + Core | | — | PowerShellVersion | Pass | 5.1 |
| — | CmdletsToExport | Pass | Explicit list of 169 cmdlets | | — | CompatiblePSEditions | Pass | Desktop, Core |
| — | Tags | Pass | 8 tags for discoverability | | — | CmdletsToExport | Pass | 169 cmdlets explicitly listed |
| — | LicenseUri/ProjectUri | Pass | GitHub links | | — | Tags | Pass | 8 relevant tags |
| F021 | IconUri | **Fail** | Not set — PSGallery shows placeholder | | — | LicenseUri | Pass | GitHub MIT license link |
| — | ProjectUri | Pass | GitHub repo link |
| — | ReleaseNotes | Pass | Present in PSData | | — | ReleaseNotes | Pass | Present in PSData |
| — | Framework targets | Pass | netstandard2.0 for both publishable projects | | F021 | IconUri | Fail | Not set — recommended for PSGallery listing |
| — | Publish workflow | Pass | Tag-triggered, 3-job pipeline with PS 5.1 smoke test | | — | Prerelease | Pass | 'preview' — appropriate for current state |
| — | CHANGELOG | Pass | Keep a Changelog format | | — | 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 ## 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 | | D001 | TaskService.WaitForTask | Yes | grep `while(true)` — only in TaskService + WaitPveTask cmdlet |
| — | Feature request template | Pass | YAML form with use case | | D002 | SecureString passwords | Yes | grep `public string Password` — none found |
| — | Issue template config | Pass | Disables blank issues, links to Discussions | | D003 | Uri.EscapeDataString | Yes | 164 occurrences across 36 files |
| — | PR template | Pass | Checklist with build, tests, psd1, changelog | | D004 | No bare catches | **FAIL** | 2 bare catches found: VmService.cs:553, ImportPveOvaCmdlet.cs:277 → F039 regressed |
| — | CONTRIBUTING.md | Pass | Complete dev setup and coding standards | | D005 | OutputType on all cmdlets | Yes | 169 cmdlets, 169 OutputType attributes |
| — | CODE_OF_CONDUCT.md | Pass | Contributor Covenant 2.1 | | D006 | ConfirmImpact.High | Yes | All destructive cmdlets including Restart/Suspend-Container |
| — | SECURITY.md | Pass | Responsible disclosure instructions | | D007 | Sealed cmdlet classes | Yes | All 169 classes sealed |
| — | DECISIONS.md | Pass | 12 decisions, linked from CLAUDE.md | | D008 | Newtonsoft.Json only | Yes | No System.Text.Json references in src/ |
| — | CODEOWNERS | Pass | `* @goodolclint` | | D009 | netstandard2.0 targets | Yes | Both publishable projects target netstandard2.0 |
| — | .editorconfig | Pass | Per-filetype overrides | | D010 | ValidateRange on VmId | Yes | All VmId parameters have ValidateRange |
| — | .gitattributes | Pass | LF enforcement, binary handling | | D011 | Verb class constants | Yes | No string literal verbs found |
| — | Commit conventions | Pass | Conventional Commits consistently used | | D012 | Magic string constants | Yes | ApiTokenPrefix, CsrfHeaderName constants in PveHttpClient |
| — | Release process | Pass | Tag-based publish with GitHub Release |
| — | Dependabot | Pass | Automated dependency PRs | **1 regression detected**: F039 (bare catch blocks) — D004 violated in 2 locations. No new decisions needed.
--- ---
## Phase 9 — Prioritized Recommendations ## Phase 9 — Prioritized Recommendations
### 🔴 Critical ### Medium (5 findings)
No critical findings.
### 🟠 High
No high findings.
### 🟡 Medium
| Finding ID | What | Where | Why | Fix | | 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 | | 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)` |
| 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 | | F084 | PveSession exposes auth secrets | PveSession.cs, format.ps1xml | Ticket/ApiToken/CsrfToken visible in pipeline output | Add format.ps1xml entry hiding secret properties |
| F059 | VM/CT-level firewall 0% coverage | — | Firewall endpoints exist for VMs and containers but no cmdlets implemented | Add VM/CT-scoped firewall cmdlets | | F046 | Integration test coverage gaps | tests/ | VmService, ContainerService lack xUnit tests | Add xUnit tests with Moq for complex services |
| F060 | Cluster config 0% coverage | — | Cluster options, join/create, notifications, metrics uncovered | Add cluster config cmdlets | | F060 | Cluster config 0% | src/ | Cannot manage clusters | 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 | | 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 | | 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 | | F021 | No IconUri in manifest | PSProxmoxVE.psd1 | PSGallery listing lacks icon | Add IconUri pointing to project logo |
| F031 | Hardcoded test IPs | terraform.tfvars, CI | Lab IPs committed (gitignored locally) | Verify tfvars not tracked in git | | F053 | HA subsystem 0% | src/ | 21 endpoints for HA management | Implement HA group/resource/rule cmdlets |
| F053 | HA subsystem 0% coverage | — | No HA cmdlets | Implement when HA rules (PVE 9.0) are prioritized | | F054 | Ceph subsystem 0% | src/ | 40 endpoints for hyperconverged | Implement Ceph pool/OSD/monitor cmdlets |
| F054 | Ceph subsystem 0% coverage | — | Large specialized surface | Low priority unless Ceph users identified | | F067 | Disk management 0% | src/ | ZFS/LVM/directory ops missing | Implement disk management cmdlets |
| F067 | Disk management 0% coverage | — | Node disk ops (LVM, ZFS, init) | Useful for provisioning workflows | | F068 | Notifications 0% | src/ | Notification targets not managed | Implement notification cmdlets |
| F068 | Notifications 0% coverage | — | 25+ endpoints (PVE 8.1+) | Medium-value for operational users | | F069 | ACME/Certificates 0% | src/ | Let's Encrypt + cert mgmt missing | Implement ACME/cert cmdlets |
| F069 | ACME/Certificates 0% coverage | — | Certificate management | Low priority |
docs/review/findings.json
+57 -14
View File
@@ -1,16 +1,16 @@
{ {
"_schema_version": "1.0", "_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.", "_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_updated": "2026-03-24",
"last_scan_date": "2026-03-23", "last_scan_date": "2026-03-24",
"counters": { "counters": {
"next_id": 84, "next_id": 85,
"total_open": 10, "total_open": 10,
"total_resolved": 72, "total_resolved": 72,
"total_regressed": 0, "total_regressed": 1,
"open": 10, "open": 10,
"resolved": 72, "resolved": 72,
"regressed": 0, "regressed": 1,
"wont_fix": 1 "wont_fix": 1
}, },
"findings": [ "findings": [
@@ -1187,15 +1187,13 @@
"title": "Bare catch blocks in PveHttpClient, PveCmdletBase, VmService, ContainerService, GetPveVmCmdlet", "title": "Bare catch blocks in PveHttpClient, PveCmdletBase, VmService, ContainerService, GetPveVmCmdlet",
"category": "code_quality", "category": "code_quality",
"severity": "medium", "severity": "medium",
"status": "resolved", "status": "regressed",
"first_detected": "2026-03-21", "first_detected": "2026-03-21",
"files": [ "files": [
"src/PSProxmoxVE.Core/Client/PveHttpClient.cs",
"src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs",
"src/PSProxmoxVE.Core/Services/VmService.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_history": [
{ {
"scan_date": "2026-03-21", "scan_date": "2026-03-21",
@@ -1206,6 +1204,11 @@
"scan_date": "2026-03-22", "scan_date": "2026-03-22",
"local_id": null, "local_id": null,
"status": "fixed" "status": "fixed"
},
{
"scan_date": "2026-03-24",
"local_id": null,
"status": "regressed"
} }
], ],
"resolution": { "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", "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" "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", "id": "F040",
@@ -1861,9 +1870,12 @@
"title": "VM/CT-level firewall 0% coverage", "title": "VM/CT-level firewall 0% coverage",
"category": "api_coverage", "category": "api_coverage",
"severity": "medium", "severity": "medium",
"status": "open", "status": "resolved",
"first_detected": "2026-03-22", "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.", "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": [ "scan_history": [
{ {
@@ -1880,8 +1892,19 @@
"scan_date": "2026-03-23", "scan_date": "2026-03-23",
"local_id": null, "local_id": null,
"status": "open" "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", "id": "F060",
@@ -2653,6 +2676,26 @@
"verified_by": "code_change", "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." "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"
}
]
} }
] ]
} }