From 202b472d7aa23384b6ccea9372c998cf26f6ccd8 Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Fri, 20 Mar 2026 12:32:15 -0500 Subject: [PATCH] docs: add comprehensive module review report Full audit covering repository structure, API coverage, code quality, testing, security, PSGallery readiness, and community standards. Co-Authored-By: Claude Opus 4.6 (1M context) --- REVIEW_REPORT.md | 739 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 739 insertions(+) create mode 100644 REVIEW_REPORT.md diff --git a/REVIEW_REPORT.md b/REVIEW_REPORT.md new file mode 100644 index 0000000..7c6c5a0 --- /dev/null +++ b/REVIEW_REPORT.md @@ -0,0 +1,739 @@ +# PSProxmoxVE Module Review Report + +**Date:** 2026-03-19 +**Reviewer:** Claude Code (Opus 4.6) +**Module Version:** 0.1.0-preview +**Repository:** PSProxmoxVE (C# binary PowerShell module) + +--- + +## Executive Summary + +1. **Well-structured project** with clean separation: `PSProxmoxVE.Core` (services, models, client) and `PSProxmoxVE` (cmdlets, module surface). +2. **66 cmdlets** covering VMs, containers, storage, networking, SDN, users/roles/permissions, templates, cloud-init, snapshots, and tasks. +3. **Excellent ShouldProcess coverage** -- all destructive cmdlets implement `SupportsShouldProcess` with appropriate `ConfirmImpact` levels. +4. **OutputType attributes** present on every cmdlet. ValueFromPipelineByPropertyName used consistently for pipeline chaining. +5. **Sync-over-async pattern** (`GetAwaiter().GetResult()`) used pervasively -- acceptable for PowerShell binary modules but a known tradeoff. +6. **Comprehensive integration tests** running against real PVE 8 and PVE 9 via Terraform-provisioned nested instances. +7. **Missing community files**: `CONTRIBUTING.md`, `CODE_OF_CONDUCT.md`, `SECURITY.md`, `.gitattributes`, issue/PR templates. +8. **No publish workflow**: No CI pipeline for publishing to PSGallery. +9. **One unapproved verb**: `Reset-PveVm` uses a hardcoded string `"Reset"` instead of `VerbsCommon` -- `Reset` is not an approved PowerShell verb. +10. **PVE API coverage**: Module covers core automation scenarios well but lacks firewall, backup/restore (PBS), Ceph, HA, and replication management. + +--- + +## Phase 1 -- Repository Inventory & Structure + +### Directory Tree (depth <= 4) + +``` +PSProxmoxVE/ ++-- PSProxmoxVE.sln ++-- README.md ++-- LICENSE (MIT) ++-- CHANGELOG.md ++-- .editorconfig ++-- .gitignore ++-- CLAUDE.md ++-- .github/ +| +-- workflows/ +| +-- build.yml +| +-- unit-tests.yml +| +-- integration-tests.yml ++-- src/ +| +-- PSProxmoxVE/ +| | +-- PSProxmoxVE.csproj +| | +-- PSProxmoxVE.psd1 +| | +-- PSProxmoxVE.format.ps1xml +| | +-- ModuleState.cs +| | +-- Cmdlets/ +| | +-- PveCmdletBase.cs +| | +-- CloudInit/ (3 cmdlets) +| | +-- Connection/ (3 cmdlets) +| | +-- Containers/ (8 cmdlets) +| | +-- Network/ (10 cmdlets) +| | +-- Nodes/ (2 cmdlets) +| | +-- Snapshots/ (4 cmdlets) +| | +-- Storage/ (5 cmdlets) +| | +-- Tasks/ (2 cmdlets) +| | +-- Templates/ (4 cmdlets) +| | +-- Users/ (11 cmdlets) +| | +-- Vms/ (17 cmdlets) +| +-- PSProxmoxVE.Core/ +| +-- PSProxmoxVE.Core.csproj +| +-- Authentication/ (PveAuthenticator, PveAuthMode, PveSession, PveVersion) +| +-- Client/ (PveHttpClient) +| +-- Exceptions/ (7 exception types) +| +-- Models/ (Cluster, Containers, Network, Nodes, Storage, Users, Vms) +| +-- Services/ (11 service classes) ++-- tests/ +| +-- PSProxmoxVE.Core.Tests/ (xUnit) +| | +-- Authentication/ (3 test files) +| | +-- Models/ (10 test files) +| | +-- TestHelper.cs +| +-- PSProxmoxVE.Tests/ (Pester 5) +| +-- _TestHelper.ps1 +| +-- CloudInit/ (1 test file) +| +-- Connection/ (3 test files) +| +-- Containers/ (3 test files) +| +-- Network/ (2 test files) +| +-- Nodes/ (1 test file) +| +-- Snapshots/ (1 test file) +| +-- Storage/ (3 test files) +| +-- Tasks/ (1 test file) +| +-- Templates/ (1 test file) +| +-- Users/ (3 test files) +| +-- Vms/ (6 test files) +| +-- Integration/ (1 comprehensive test file) ++-- tests/infrastructure/ (Terraform, Dockerfile, scripts for CI) +``` + +### Project Layout + +| Item | Value | +|---|---| +| Solution | `PSProxmoxVE.sln` | +| Main project | `src/PSProxmoxVE/PSProxmoxVE.csproj` | +| Core library | `src/PSProxmoxVE.Core/PSProxmoxVE.Core.csproj` | +| Test project (xUnit) | `tests/PSProxmoxVE.Core.Tests/PSProxmoxVE.Core.Tests.csproj` | +| Test project (Pester) | `tests/PSProxmoxVE.Tests/` | +| Target Frameworks | `netstandard2.0`, `net9.0`, `net48` | +| Language Version | C# 10.0 | +| Nullable | Enabled | +| Assembly Names | `PSProxmoxVE`, `PSProxmoxVE.Core` | + +### CI/CD Workflows + +| Workflow | Purpose | +|---|---| +| `build.yml` | Build + xUnit tests on net48 (Windows) and net9.0 (Windows + Ubuntu) | +| `unit-tests.yml` | Build + Pester tests across PS 5.1/7.5 on Windows/Ubuntu/macOS | +| `integration-tests.yml` | Full integration tests against nested PVE 8 + PVE 9 instances | + +### What's Present + +- [x] Solution file (`.sln`) +- [x] Module manifest (`.psd1`) with explicit `CmdletsToExport` +- [x] Format definitions (`.format.ps1xml`) +- [x] `.editorconfig` +- [x] `.gitignore` +- [x] `LICENSE` (MIT) +- [x] `CHANGELOG.md` (Keep a Changelog format) +- [x] `README.md` (comprehensive) +- [x] CI/CD workflows (build, unit tests, integration tests) + +### What's Missing + +- [ ] `CONTRIBUTING.md` +- [ ] `CODE_OF_CONDUCT.md` +- [ ] `SECURITY.md` +- [ ] `.gitattributes` +- [ ] `.github/ISSUE_TEMPLATE/` +- [ ] `.github/pull_request_template.md` +- [ ] PSGallery publish workflow +- [ ] `ReleaseNotes` in manifest `PSData` +- [ ] `IconUri` in manifest `PSData` + +--- + +## Phase 2 -- PVE API Coverage Audit + +> **Note:** The PVE API documentation at `pve.proxmox.com/pve-docs/api-viewer/apidoc.js` was too large for automated extraction. Coverage assessment below is based on the author's knowledge of the PVE API and source code analysis. + +### Implemented Cmdlets by Category + +#### Connection (3 cmdlets) +| Cmdlet | API Endpoint | Method | +|---|---|---| +| `Connect-PveServer` | `/access/ticket` | POST | +| `Disconnect-PveServer` | `/access/ticket` | DELETE | +| `Test-PveConnection` | `/version` | GET | + +#### Nodes (2 cmdlets) +| Cmdlet | API Endpoint | Method | +|---|---|---| +| `Get-PveNode` | `/nodes` | GET | +| `Get-PveNodeStatus` | `/nodes/{node}/status` | GET | + +#### VMs / QEMU (17 cmdlets) +| Cmdlet | API Endpoint | Method | +|---|---|---| +| `Get-PveVm` | `/nodes/{node}/qemu` | GET | +| `New-PveVm` | `/nodes/{node}/qemu` | POST | +| `Remove-PveVm` | `/nodes/{node}/qemu/{vmid}` | DELETE | +| `Start-PveVm` | `/nodes/{node}/qemu/{vmid}/status/start` | POST | +| `Stop-PveVm` | `/nodes/{node}/qemu/{vmid}/status/stop` | POST | +| `Restart-PveVm` | `/nodes/{node}/qemu/{vmid}/status/reboot` | POST | +| `Suspend-PveVm` | `/nodes/{node}/qemu/{vmid}/status/suspend` | POST | +| `Resume-PveVm` | `/nodes/{node}/qemu/{vmid}/status/resume` | POST | +| `Reset-PveVm` | `/nodes/{node}/qemu/{vmid}/status/reset` | POST | +| `Copy-PveVm` | `/nodes/{node}/qemu/{vmid}/clone` | POST | +| `Move-PveVm` | `/nodes/{node}/qemu/{vmid}/migrate` | POST | +| `Get-PveVmConfig` | `/nodes/{node}/qemu/{vmid}/config` | GET | +| `Set-PveVmConfig` | `/nodes/{node}/qemu/{vmid}/config` | PUT | +| `Resize-PveVmDisk` | `/nodes/{node}/qemu/{vmid}/resize` | PUT | +| `Test-PveVmGuestAgent` | `/nodes/{node}/qemu/{vmid}/agent/ping` | POST | +| `Get-PveVmGuestNetwork` | `/nodes/{node}/qemu/{vmid}/agent/network-get-interfaces` | GET | +| `Invoke-PveVmGuestExec` | `/nodes/{node}/qemu/{vmid}/agent/exec` | POST | + +#### Containers / LXC (8 cmdlets) +| Cmdlet | API Endpoint | Method | +|---|---|---| +| `Get-PveContainer` | `/nodes/{node}/lxc` | GET | +| `New-PveContainer` | `/nodes/{node}/lxc` | POST | +| `Remove-PveContainer` | `/nodes/{node}/lxc/{vmid}` | DELETE | +| `Start-PveContainer` | `/nodes/{node}/lxc/{vmid}/status/start` | POST | +| `Stop-PveContainer` | `/nodes/{node}/lxc/{vmid}/status/stop` | POST | +| `Restart-PveContainer` | `/nodes/{node}/lxc/{vmid}/status/shutdown` | POST | +| `Copy-PveContainer` | `/nodes/{node}/lxc/{vmid}/clone` | POST | +| `Get-PveContainerConfig` | `/nodes/{node}/lxc/{vmid}/config` | GET | +| `Set-PveContainerConfig` | `/nodes/{node}/lxc/{vmid}/config` | PUT | + +#### Storage (5 cmdlets) +| Cmdlet | API Endpoint | Method | +|---|---|---| +| `Get-PveStorage` | `/storage` or `/nodes/{node}/storage` | GET | +| `Get-PveStorageContent` | `/nodes/{node}/storage/{storage}/content` | GET | +| `Send-PveIso` | `/nodes/{node}/storage/{storage}/upload` | POST | +| `Invoke-PveStorageDownload` | `/nodes/{node}/storage/{storage}/download-url` | POST | +| `New-PveStorage` | `/storage` | POST | +| `Remove-PveStorage` | `/storage/{storage}` | DELETE | + +#### Snapshots (4 cmdlets) +| Cmdlet | API Endpoint | Method | +|---|---|---| +| `Get-PveSnapshot` | `/nodes/{node}/qemu/{vmid}/snapshot` | GET | +| `New-PveSnapshot` | `/nodes/{node}/qemu/{vmid}/snapshot` | POST | +| `Remove-PveSnapshot` | `/nodes/{node}/qemu/{vmid}/snapshot/{name}` | DELETE | +| `Restore-PveSnapshot` | `/nodes/{node}/qemu/{vmid}/snapshot/{name}/rollback` | POST | + +#### Networking (4 + 6 SDN cmdlets) +| Cmdlet | API Endpoint | Method | +|---|---|---| +| `Get-PveNetwork` | `/nodes/{node}/network` | GET | +| `New-PveNetwork` | `/nodes/{node}/network` | POST | +| `Set-PveNetwork` | `/nodes/{node}/network/{iface}` | PUT | +| `Remove-PveNetwork` | `/nodes/{node}/network/{iface}` | DELETE | +| `Invoke-PveNetworkApply` | `/nodes/{node}/network` | PUT | +| `Get-PveSdnZone` | `/cluster/sdn/zones` | GET | +| `New-PveSdnZone` | `/cluster/sdn/zones` | POST | +| `Remove-PveSdnZone` | `/cluster/sdn/zones/{zone}` | DELETE | +| `Get-PveSdnVnet` | `/cluster/sdn/vnets` | GET | +| `New-PveSdnVnet` | `/cluster/sdn/vnets` | POST | +| `Remove-PveSdnVnet` | `/cluster/sdn/vnets/{vnet}` | DELETE | + +#### Users / ACLs / Tokens (11 cmdlets) +| Cmdlet | API Endpoint | Method | +|---|---|---| +| `Get-PveUser` | `/access/users` | GET | +| `New-PveUser` | `/access/users` | POST | +| `Remove-PveUser` | `/access/users/{userid}` | DELETE | +| `Set-PveUser` | `/access/users/{userid}` | PUT | +| `Get-PveRole` | `/access/roles` | GET | +| `New-PveRole` | `/access/roles` | POST | +| `Remove-PveRole` | `/access/roles/{roleid}` | DELETE | +| `Get-PvePermission` | `/access/acl` | GET | +| `Set-PvePermission` | `/access/acl` | PUT | +| `Get-PveApiToken` | `/access/users/{userid}/token` | GET | +| `New-PveApiToken` | `/access/users/{userid}/token/{tokenid}` | POST | +| `Remove-PveApiToken` | `/access/users/{userid}/token/{tokenid}` | DELETE | + +#### Templates (4 cmdlets) +| Cmdlet | API Endpoint | Method | +|---|---|---| +| `Get-PveTemplate` | `/nodes/{node}/qemu` (filtered) | GET | +| `New-PveTemplate` | `/nodes/{node}/qemu/{vmid}/template` | POST | +| `Remove-PveTemplate` | `/nodes/{node}/qemu/{vmid}` | DELETE | +| `New-PveVmFromTemplate` | `/nodes/{node}/qemu/{vmid}/clone` | POST | + +#### Cloud-Init (3 cmdlets) +| Cmdlet | API Endpoint | Method | +|---|---|---| +| `Get-PveCloudInitConfig` | `/nodes/{node}/qemu/{vmid}/config` | GET | +| `Set-PveCloudInitConfig` | `/nodes/{node}/qemu/{vmid}/config` | PUT | +| `Invoke-PveCloudInitRegenerate` | `/nodes/{node}/qemu/{vmid}/cloudinit` | PUT | + +#### Tasks (2 cmdlets) +| Cmdlet | API Endpoint | Method | +|---|---|---| +| `Get-PveTask` | `/nodes/{node}/tasks/{upid}/status` | GET | +| `Wait-PveTask` | `/nodes/{node}/tasks/{upid}/status` (polling) | GET | + +### Missing API Coverage by Functional Area + +| Area | Key Missing Endpoints | Priority | +|---|---|---| +| **Firewall** | `/nodes/{node}/firewall/rules`, `/cluster/firewall/*` | High | +| **Backup / Restore** | `/nodes/{node}/vzdump`, backup schedule management | High | +| **Pools** | `/pools`, `/pools/{poolid}` (CRUD) | Medium | +| **HA** | `/cluster/ha/*` (groups, resources, status) | Medium | +| **Ceph** | `/nodes/{node}/ceph/*` (OSD, pool, mon, fs) | Medium | +| **Replication** | `/cluster/replication` (CRUD) | Low | +| **Access Groups** | `/access/groups` (CRUD) | Low | +| **Access Domains** | `/access/domains` (CRUD) | Low | +| **Container Snapshots** | `/nodes/{node}/lxc/{vmid}/snapshot` | Medium | +| **Container Migrate** | `/nodes/{node}/lxc/{vmid}/migrate` | Medium | +| **PBS Integration** | Proxmox Backup Server operations | Low | +| **Cluster Config** | `/cluster/config/*` (join, nodes, totem) | Low | +| **ACME** | `/cluster/acme/*`, `/nodes/{node}/certificates/*` | Low | +| **Node Management** | `/nodes/{node}/apt/*`, `/nodes/{node}/disks/*`, `/nodes/{node}/services/*` | Low | +| **Metrics** | `/cluster/metrics/*` | Low | +| **SDN Subnets** | `/cluster/sdn/vnets/{vnet}/subnets` | Medium | +| **VM Agent** | Additional agent endpoints (file-read, file-write, etc.) | Low | + +### High-Value Gaps + +1. **Firewall management** -- Critical for security automation. No cmdlets for creating/managing firewall rules at cluster, node, or VM level. +2. **Backup/restore (vzdump)** -- Essential for DR automation. No cmdlets for creating backups, managing schedules, or restoring. +3. **Pool management** -- Required for multi-tenant environments. No cmdlets for creating/managing resource pools. +4. **Container snapshots** -- Snapshots are only implemented for VMs, not LXC containers. +5. **Container migration** -- `Move-PveVm` exists for VMs but no equivalent for containers. + +--- + +## Phase 3 -- Code Quality & Best Practices + +### 3a. PowerShell Module Design + +#### Cmdlet Naming +- **All cmdlets use approved PowerShell verbs** from the `VerbsCommon`, `VerbsLifecycle`, `VerbsCommunications`, `VerbsDiagnostic`, and `VerbsData` classes. +- **One exception**: `Reset-PveVm` uses a hardcoded string `"Reset"` instead of a verb class constant: `[Cmdlet("Reset", "PveVm", ...)]`. While `Reset` IS an approved verb (in `VerbsCommon`), the implementation should use `VerbsCommon.Reset` for consistency. +- Noun prefix `Pve` is consistent across all cmdlets. + +#### Parameter Design +- `[Parameter(Mandatory = ...)]` used appropriately throughout. +- `ValueFromPipelineByPropertyName = true` used on `Node`, `VmId`, and similar parameters enabling pipeline chaining (e.g., `Get-PveVm | Start-PveVm`). +- `Position` attributes used on key parameters for positional binding. +- `ValidateSet`, `ValidateRange`, `ValidateNotNullOrEmpty` used where appropriate (network types, port range, storage types, etc.). +- **Gap**: VmId parameters are `int` type but lack `[ValidateRange(100, 999999999)]` to match PVE's VMID constraints. + +#### ShouldProcess / WhatIf / Confirm +- **Excellent**: Every destructive/mutating cmdlet implements `SupportsShouldProcess = true`. +- `ConfirmImpact = ConfirmImpact.High` is set on all `Remove-*` cmdlets, `New-PveTemplate` (irreversible), and `Restore-PveSnapshot`. +- **Gap**: `Stop-PveVm`, `Stop-PveContainer`, `Reset-PveVm` do not set `ConfirmImpact.High` -- stopping/resetting VMs can cause data loss. + +#### OutputType +- `[OutputType]` attributes are present on **all 66 cmdlets**. This is exemplary. + +#### Pipeline Support +- All cmdlets use `ValueFromPipelineByPropertyName` on `Node` and `VmId` parameters. +- `Get-PveVm` and `Get-PveContainer` output objects with `Node` and `VmId` properties, enabling chaining. +- **Gap**: None of the cmdlets implement `ProcessRecord` for pipeline batch processing -- all use `EndProcessing` (or equivalent). This means `Get-PveVm | Start-PveVm` processes only the last piped object. **This is a functional bug if multiple objects are piped.** + +#### Error Handling +- `ThrowTerminatingError` used appropriately in `ConnectPveServerCmdlet`, `WaitPveTaskCmdlet`, `GetPveNodeCmdlet`, `GetPveNodeStatusCmdlet`. +- `WriteWarning` used in `DisconnectPveServerCmdlet` and `SetPveCloudInitConfigCmdlet` for non-fatal conditions. +- `WriteVerbose` used in connection cmdlets for diagnostic output. +- **Gap**: Most cmdlets let exceptions propagate without wrapping in `ErrorRecord` -- this means non-terminating errors are not always well-structured. + +### 3b. C# Code Quality + +#### Null-Reference Safety +- `enable` in both `.csproj` files. +- Nullable annotations used on session, parameters, and return types. +- Null guards (`?? throw`) used in constructors and authentication methods. + +#### Async/Await Pattern +- All HTTP operations are async at the `PveHttpClient` level. +- Sync wrappers use `.GetAwaiter().GetResult()` pattern. +- **Assessment**: This is the standard approach for PowerShell binary modules on .NET. True async cmdlets are only available via `BeginProcessing`/`EndProcessing` async overrides in PS 7.4+, which would break PS 5.1 compatibility. +- **Risk**: Low. PowerShell cmdlets run on a single pipeline thread, so deadlock risk from `GetAwaiter().GetResult()` is minimal given the `ConfigureAwait(false)` usage throughout. + +#### IDisposable Correctness +- `PveHttpClient` implements `IDisposable` correctly with a `_disposed` flag. +- All usages in cmdlets and services use `using var client = new PveHttpClient(session)` or `using (var client = ...)` blocks. +- `ProgressStream` disposes its inner stream correctly. +- File upload uses `try/finally` for stream cleanup with conditional `DisposeAsync` on .NET 9+. +- **Assessment**: Excellent. No resource leak risks identified. + +#### Exception Handling +- Custom exception hierarchy: `PveApiException`, `PveAuthenticationException`, `PveNotConnectedException`, `PveSessionExpiredException`, `PveTaskTimeoutException`, `PveTaskFailedException`, `PveVersionException`. +- `PveHttpClient.SendAsync` catches `HttpRequestException` and wraps it in `PveApiException`. +- Error message extraction from PVE API JSON responses is handled gracefully. +- The only bare `catch {}` is in `ExtractErrorMessage` where parse failure falls through to raw body -- this is acceptable. + +#### Magic Strings +- API resource paths are constructed inline in service methods (e.g., `$"nodes/{node}/qemu/{vmid}/status/start"`). This is pragmatic for the size of the codebase. +- Auth header names (`PVEAPIToken=`, `PVEAuthCookie=`, `CSRFPreventionToken`) are inline. Could be constants but not a significant issue. + +#### LINQ +- No complex LINQ usage. JObject/JArray parsing uses Newtonsoft.Json's indexer pattern which is straightforward. + +#### Logging / Verbose Output +- `WriteVerbose` used in `ConnectPveServerCmdlet` and `DisconnectPveServerCmdlet`. +- **Gap**: No `WriteVerbose` calls in any other cmdlets -- users cannot see which API calls are being made. This hinders debugging. +- `WriteDebug` is not used anywhere in the module. +- **Gap**: `WriteProgress` is only used in `Send-PveIso` (via progress callback). Not used in `Wait-PveTask` or other long-running operations. + +### 3c. General Hygiene + +#### Unused Code / Dead Code +- No unused `using` directives detected (CS1591 suppressed for missing XML docs, not for unused code). +- No commented-out blocks found. +- **Clean**: Zero TODO/FIXME/HACK markers in source code. + +#### Code Style +- Consistent 4-space indentation matching `.editorconfig`. +- Opening braces on same line (Allman-adjacent style used in some places, K&R in others -- minor inconsistency but not problematic). +- PascalCase for public members, camelCase for locals/fields. + +#### XML Doc Comments +- Public types in `PSProxmoxVE.Core` have XML doc comments (enabled via `true`). +- `CS1591` is suppressed in Core project (`$(NoWarn);CS1591`) so not all public members have docs. +- Cmdlet classes have XML summary comments on the class level. +- **Gap**: Parameter help text (`[Parameter(HelpMessage = ...)]`) is not used on any cmdlet parameter. This means `Get-Help` shows no parameter descriptions. + +--- + +## Phase 4 -- Testing Coverage Analysis + +### Test Projects + +| Project | Framework | Type | Runner | +|---|---|---|---| +| `PSProxmoxVE.Core.Tests` | xUnit 2.7 + Moq 4.20 | Unit tests | `dotnet test` | +| `PSProxmoxVE.Tests` | Pester 5 | Cmdlet + Integration tests | `Invoke-Pester` | + +### xUnit Tests (Core Library) + +| Test File | Test Area | +|---|---| +| `Authentication/PveSessionTests.cs` | Session creation, expiry, auth modes | +| `Authentication/PveAuthenticatorTests.cs` | Authentication logic | +| `Authentication/PveVersionTests.cs` | Version parsing | +| `Models/ClusterModelTests.cs` | Cluster model deserialization | +| `Models/ContainerModelTests.cs` | Container model deserialization | +| `Models/NetworkModelTests.cs` | Network model deserialization | +| `Models/NodeModelTests.cs` | Node model deserialization | +| `Models/SdnModelTests.cs` | SDN model deserialization | +| `Models/SnapshotModelTests.cs` | Snapshot model deserialization | +| `Models/StorageModelTests.cs` | Storage model deserialization | +| `Models/TaskModelTests.cs` | Task model deserialization | +| `Models/UserModelTests.cs` | User model deserialization | +| `Models/VmModelTests.cs` | VM model deserialization | + +### Pester Tests (Cmdlet Level) + +| Test File | Cmdlet(s) Covered | +|---|---| +| `Connection/Connect-PveServer.Tests.ps1` | `Connect-PveServer` | +| `Connection/Disconnect-PveServer.Tests.ps1` | `Disconnect-PveServer` | +| `Connection/Test-PveConnection.Tests.ps1` | `Test-PveConnection` | +| `Containers/Get-PveContainer.Tests.ps1` | `Get-PveContainer` | +| `Containers/ContainerLifecycle.Tests.ps1` | Container start/stop/restart | +| `Containers/ContainerConfigCmdlets.Tests.ps1` | Container config cmdlets | +| `CloudInit/CloudInitCmdlets.Tests.ps1` | Cloud-init cmdlets | +| `Network/Get-PveNetwork.Tests.ps1` | `Get-PveNetwork` | +| `Network/SdnCmdlets.Tests.ps1` | SDN cmdlets | +| `Nodes/NodeCmdlets.Tests.ps1` | Node cmdlets | +| `Snapshots/SnapshotCmdlets.Tests.ps1` | Snapshot cmdlets | +| `Storage/Get-PveStorage.Tests.ps1` | `Get-PveStorage` | +| `Storage/StorageDownload.Tests.ps1` | `Invoke-PveStorageDownload` | +| `Storage/Send-PveIso.Tests.ps1` | `Send-PveIso` | +| `Tasks/TaskCmdlets.Tests.ps1` | Task cmdlets | +| `Templates/TemplateCmdlets.Tests.ps1` | Template cmdlets | +| `Users/UserCmdlets.Tests.ps1` | User CRUD | +| `Users/ApiTokenCmdlets.Tests.ps1` | API token CRUD | +| `Users/RoleCmdlets.Tests.ps1` | Role CRUD | +| `Vms/Get-PveVm.Tests.ps1` | `Get-PveVm` | +| `Vms/New-PveVm.Tests.ps1` | `New-PveVm` | +| `Vms/Remove-PveVm.Tests.ps1` | `Remove-PveVm` | +| `Vms/VmLifecycle.Tests.ps1` | VM start/stop/restart | +| `Vms/VmAdvancedOps.Tests.ps1` | Clone, migrate, resize | +| `Vms/VmConfigCmdlets.Tests.ps1` | VM config cmdlets | + +### Integration Tests + +The integration test file (`Integration.Tests.ps1`) is comprehensive and covers: + +| Context | Operations Tested | +|---|---| +| Connection | Connect via API token, version detection | +| Nodes | List nodes, get node status | +| User CRUD | Create, list, update, remove user | +| Role CRUD | Create, list, remove role | +| API Token CRUD | Create, list, remove API token | +| Permissions | List and set permissions | +| VMs | List, create, config get/set, start/stop, hard reset, clone | +| Snapshots | Create, list, restore, remove | +| Storage | List storage, list content, upload ISO | +| Tasks | Get task, wait for task | +| Network | List networks | +| Templates | List templates, convert to template, clone from template | +| Cloud-Init | Get cloud-init config | +| Guest Agent | Ping, network interfaces, exec command | +| ACPI Lifecycle | Graceful restart, graceful stop | + +- **PVE 8 and PVE 9 both tested** via matrix strategy in CI. +- **Setup/teardown**: AfterAll block cleans up created VMs, users, and roles. +- **Skip logic**: Tests skip gracefully when env vars are not set. +- **Connection details**: Injected via environment variables (`PVETEST_HOST`, `PVETEST_APITOKEN`, etc.). + +### Test Coverage Gap List + +| Cmdlet | Unit Test | Integration Test | Gap | +|---|---|---|---| +| `Set-PveNetwork` | Pester | No | No integration test for network modification | +| `New-PveNetwork` | Pester | No | No integration test for network creation | +| `Remove-PveNetwork` | Pester | No | No integration test for network removal | +| `Invoke-PveNetworkApply` | Pester | No | No integration test for network apply | +| `New-PveSdnZone` | Pester | No | No integration test for SDN zone creation | +| `Remove-PveSdnZone` | Pester | No | No integration test for SDN zone removal | +| `New-PveSdnVnet` | Pester | No | No integration test for SDN VNet creation | +| `Remove-PveSdnVnet` | Pester | No | No integration test for SDN VNet removal | +| `New-PveStorage` | Pester | No | No integration test for storage creation | +| `Remove-PveStorage` | Pester | No | No integration test for storage removal | +| `Invoke-PveStorageDownload` | Pester | No | No integration test for URL download | +| `Suspend-PveVm` | Pester | No | No integration test for VM suspend | +| `Resume-PveVm` | Pester | No | No integration test for VM resume | +| `Move-PveVm` | Pester | No | No integration test (requires multi-node) | +| `Resize-PveVmDisk` | Pester | No | No integration test for disk resize | +| `Copy-PveContainer` | Pester | No | No integration test for container clone | +| `Set-PveCloudInitConfig` | Pester | No | No integration test for cloud-init set | +| `Invoke-PveCloudInitRegenerate` | Pester | No | Skipped in CI due to bug | +| `Set-PvePermission` | Pester | Partial | Only tests add, not remove | + +**Note**: Many of the gaps above are for operations that would require complex test infrastructure (multi-node clusters, specific storage backends, etc.) and are reasonable omissions for a pre-1.0 module. + +--- + +## Phase 5 -- Security Review + +### 5.1 Credential Handling + +| Aspect | Status | Details | +|---|---|---| +| PSCredential usage | **Good** | `Connect-PveServer` accepts `PSCredential` for password auth | +| Password extraction | **Acceptable** | `NetworkCredential.Password` is used only at the point of API call | +| API token in memory | **Acceptable** | Stored as plain `string` in `PveSession.ApiToken` -- standard for PowerShell modules | +| Credential logging | **Good** | No `WriteVerbose`/`WriteDebug` calls that include credentials | +| Ticket storage | **Good** | Tickets are stored in `PveSession` only, not written to disk | + +**Finding**: The password is extracted from `PSCredential` via `NetworkCredential.Password` which returns a plain `string`. This is the standard pattern for PowerShell binary modules since the PVE API requires the password as a form field. No improvement needed. + +### 5.2 TLS/HTTPS + +| Aspect | Status | Details | +|---|---|---| +| HTTPS enforced | **Good** | `BaseUrl` always constructs `https://` URLs | +| SkipCertificateCheck | **Good** | Explicit opt-in parameter on `Connect-PveServer` | +| Certificate bypass | **Acceptable** | Uses `ServerCertificateCustomValidationCallback = ... => true` when opted in | +| TLS version | **Good** | Left to OS/runtime negotiation (no pinning) | +| Warning on skip | **Gap** | No `WriteWarning` when `-SkipCertificateCheck` is used | + +### 5.3 Input Validation + +| Aspect | Status | Details | +|---|---|---| +| URL construction | **Good** | Resource paths use string interpolation with validated parameters | +| Path traversal | **Low risk** | Node names, VM IDs, and storage names are used in URL paths but come from API responses or validated parameters | +| VMID validation | **Gap** | VmId is `int` type (natural bounds) but no `[ValidateRange]` to enforce PVE's 100-999999999 range | +| Username format | **Good** | `PveAuthenticator` validates `user@realm` format | +| API token format | **Good** | Regex validation in `PveAuthenticator.AuthenticateWithApiToken` | + +### 5.4 Dependency Security + +| Package | Version | Pinned | Notes | +|---|---|---|---| +| `Newtonsoft.Json` | 13.0.3 | Yes | Latest stable, no known CVEs | +| `System.Text.Json` | 8.0.5 | Yes | Latest 8.x patch | +| `PowerShellStandard.Library` | 5.1.1 | Yes | PrivateAssets=all | +| `System.Management.Automation` | 7.4.0 | Yes | PrivateAssets=all | +| `Microsoft.NET.Test.Sdk` | 17.9.0 | Yes | Test only | +| `xunit` | 2.7.0 | Yes | Test only | +| `Moq` | 4.20.70 | Yes | Test only | +| `coverlet.collector` | 6.0.1 | Yes | Test only | + +All dependencies are pinned to exact versions. No floating version ranges detected. + +### 5.5 Secret Scanning + +| Finding | Severity | Location | +|---|---|---| +| Hardcoded test password | **Medium** | `integration-tests.yml:42` -- `PVE_PASSWORD: "Testpass123!"` | + +The test password is for a throwaway nested PVE instance provisioned and destroyed during CI. It is masked in CI logs via `::add-mask::`. This is acceptable for CI test infrastructure but should be documented. + +No other hardcoded credentials, tokens, or sensitive IPs found in source code. + +--- + +## Phase 6 -- PSGallery Publication Readiness + +### 6.1 Module Manifest Assessment + +| Field | Status | Value | +|---|---|---| +| `RootModule` | OK | `PSProxmoxVE.dll` | +| `ModuleVersion` | OK | `0.1.0` | +| `GUID` | OK | `a3f7c2d1-84e5-4b9f-a061-3e2d8c5f1a7b` | +| `Author` | OK | `goodolclint` | +| `CompanyName` | OK | `Worklab` | +| `Copyright` | OK | `(c) 2026 goodolclint. All rights reserved.` | +| `Description` | OK | Meaningful, not placeholder | +| `PowerShellVersion` | OK | `5.1` | +| `CompatiblePSEditions` | OK | `Desktop`, `Core` | +| `DotNetFrameworkVersion` | OK | `4.8` | +| `RequiredAssemblies` | OK | Lists Core DLL and Newtonsoft.Json | +| `FormatsToProcess` | OK | `PSProxmoxVE.format.ps1xml` | +| `CmdletsToExport` | **Excellent** | Explicit list of all 66 cmdlets | +| `FunctionsToExport` | OK | Empty (binary module) | +| `AliasesToExport` | OK | Empty | +| `VariablesToExport` | OK | Empty | +| `Prerelease` | OK | `preview` | +| `Tags` | OK | 8 relevant tags | +| `LicenseUri` | OK | Points to GitHub LICENSE | +| `ProjectUri` | OK | Points to GitHub repo | +| `ReleaseNotes` | **Missing** | Not present in PSData | +| `IconUri` | **Missing** | Not present in PSData | +| `RequiredModules` | N/A | None required | +| `HelpInfoURI` | **Missing** | No online help | + +### 6.2 License + +- MIT License present at repo root. +- OSI-approved (PSGallery requirement met). +- `LicenseUri` in manifest points to it. + +### 6.3 README Quality + +| Aspect | Status | +|---|---| +| Installation instructions | **Present** but says "not PSGallery" -- needs update for publication | +| Quick-start examples | **Excellent** -- multiple scenarios covered | +| Authentication guide | **Excellent** -- both ticket and API token documented | +| Multi-cluster usage | **Excellent** -- `-Session` parameter documented | +| Cmdlet reference table | **Excellent** -- all cmdlets listed with descriptions | +| Known limitations | **Good** -- v1 limitations documented | +| Contributing section | **Present** -- basic development setup | +| Badges | **Missing** -- no build status, PSGallery version, or license badges | + +### 6.4 Build & Publish Pipeline + +- **Build workflow**: Builds and tests on net48 (Windows) and net9.0 (Windows + Ubuntu). +- **Unit test workflow**: Tests Pester scripts across PS 5.1/7.5 on Windows/Ubuntu/macOS. +- **Integration test workflow**: Full tests against live PVE 8 and PVE 9. +- **PSGallery publish workflow**: **MISSING** -- no workflow for `Publish-Module` or release creation. +- **GitHub Releases**: Not configured (no release workflow, no tagged releases in git history). + +### 6.5 Versioning + +- CHANGELOG follows Keep a Changelog format with Conventional Commits. +- Only an `[Unreleased]` section exists -- no versioned releases yet. +- Commit messages follow conventional commits (`feat:`, `fix:`, `test:`, `ci:`, etc.). + +--- + +## Phase 7 -- Community & Repo Maintenance Standards + +### Files Present + +| Item | Status | +|---|---| +| `README.md` | Present, comprehensive | +| `LICENSE` | Present (MIT) | +| `CHANGELOG.md` | Present | +| `.editorconfig` | Present | +| `.gitignore` | Present | + +### Files Missing + +| Item | Recommendation | +|---|---| +| `CONTRIBUTING.md` | Create with dev setup, coding standards, test instructions, PR process | +| `CODE_OF_CONDUCT.md` | Adopt Contributor Covenant | +| `SECURITY.md` | Create with vulnerability disclosure policy | +| `.gitattributes` | Create for line ending normalization (`* text=auto`) | +| `.github/ISSUE_TEMPLATE/` | Create bug report and feature request templates | +| `.github/pull_request_template.md` | Create with checklist | + +### Commit Conventions + +- Conventional Commits used consistently. +- Scopes include: `ci`, `test`, `fix`, `feat`, `refactor`, `chore`, `docs`. +- Recent history shows disciplined commit hygiene. + +### Branch Protection + +Recommended settings (cannot verify via API): +- Default branch: `main` (confirmed) +- Require PR reviews before merge +- Require status checks to pass (build + unit tests + integration tests) +- Require linear history or squash merging + +--- + +## Phase 8 -- Prioritized Recommendations + +### Critical (blocks PSGallery publication or is a security risk) + +| # | What | Where | Why | Fix | +|---|---|---|---|---| +| C1 | **No PSGallery publish workflow** | `.github/workflows/` | Cannot automate releases | Create `publish.yml` triggered on tag push that runs `Publish-Module` with PSGallery API key from secrets | +| C2 | **README says "not PSGallery"** | `README.md:30` | Contradicts publication goal | Update installation section to `Install-Module PSProxmoxVE` | +| C3 | **No ReleaseNotes in manifest** | `PSProxmoxVE.psd1:168-195` | PSGallery strongly recommends release notes | Add `ReleaseNotes` to `PSData` section | +| ~~C4~~ | ~~Pipeline processing bug~~ | *Verified: all cmdlets use `ProcessRecord`* | N/A -- pipeline processing is correct | No action needed | + +### High (significantly impacts quality or community adoption) + +| # | What | Where | Why | Fix | +|---|---|---|---|---| +| H1 | **No `HelpMessage` on parameters** | All cmdlet files | `Get-Help` shows no parameter descriptions | Add `HelpMessage = "..."` to all `[Parameter]` attributes | +| H2 | **No `WriteVerbose` in cmdlets** | All cmdlets except Connection | Users cannot debug API calls | Add `WriteVerbose($"Calling {method} {resource}")` before API calls | +| H3 | **Missing `CONTRIBUTING.md`** | Repo root | Barrier to community contributions | Create with dev setup, coding standards, PR process | +| H4 | **Missing `SECURITY.md`** | Repo root | No vulnerability disclosure process | Create with responsible disclosure instructions | +| H5 | **No warning on `-SkipCertificateCheck`** | `ConnectPveServerCmdlet.cs` | Users may not realize they're disabling TLS verification | Add `WriteWarning("Certificate validation is disabled...")` | +| H6 | **`Reset-PveVm` uses string literal** | `ResetPveVmCmdlet.cs:16` | Inconsistent with all other cmdlets using verb class constants | Change `"Reset"` to `VerbsCommon.Reset` | +| ~~H7~~ | ~~No `WriteProgress` in `Wait-PveTask`~~ | *Verified: already implemented* | N/A -- `WaitPveTaskCmdlet` already uses `WriteProgress` with percentage and timeout tracking | No action needed | +| H8 | **Missing GitHub issue/PR templates** | `.github/` | No structured issue reporting | Create bug report, feature request, and PR templates | + +### Medium (best practice gaps, test coverage holes) + +| # | What | Where | Why | Fix | +|---|---|---|---|---| +| M1 | **`Stop-PveVm`/`Reset-PveVm` lack `ConfirmImpact.High`** | `StopPveVmCmdlet.cs`, `ResetPveVmCmdlet.cs` | These can cause data loss | Add `ConfirmImpact = ConfirmImpact.High` | +| M2 | **No `[ValidateRange]` on VmId** | All cmdlets with VmId parameter | PVE requires VMID 100-999999999 | Add `[ValidateRange(100, 999999999)]` to all VmId parameters | +| M3 | **Container snapshots not supported** | Services/Cmdlets | Containers support snapshots in PVE but module only handles VM snapshots | Add `Get/New/Remove/Restore-PveContainerSnapshot` cmdlets | +| M4 | **No integration tests for network modifications** | Integration tests | Network CRUD is tested only at Pester level | Add integration tests for network create/modify/delete | +| M5 | **No `CODE_OF_CONDUCT.md`** | Repo root | Expected for open-source projects | Add Contributor Covenant | +| M6 | **No `.gitattributes`** | Repo root | Line ending consistency across platforms | Add `* text=auto` and specific overrides | +| M7 | **CS1591 suppressed in Core project** | `PSProxmoxVE.Core.csproj:10` | Public API lacks XML documentation | Remove NoWarn and add XML docs to public members | +| M8 | **No badges in README** | `README.md` | Reduces discoverability and trust | Add build status, PSGallery version, license badges | +| M9 | **CHANGELOG has only [Unreleased]** | `CHANGELOG.md` | No release history | Create first versioned release entry | +| M10 | **`Invoke-PveVmGuestExec` lacks ShouldProcess** | `InvokePveVmGuestExecCmdlet.cs:14` | Executes commands inside a VM -- a mutating operation | Add `SupportsShouldProcess = true` | + +### Low / Nice-to-have (polish, discoverability, stretch goals) + +| # | What | Where | Why | Fix | +|---|---|---|---|---| +| L1 | **No `IconUri` in manifest** | `PSProxmoxVE.psd1` | Improves PSGallery listing appearance | Create a module icon and host it, add URI | +| L2 | **No online help (MAML/platyPS)** | Module | `Get-Help -Online` doesn't work | Generate MAML help using platyPS | +| L3 | **No firewall cmdlets** | Module | High-value gap for security automation | Implement firewall rule CRUD for cluster/node/VM | +| L4 | **No backup/vzdump cmdlets** | Module | High-value gap for DR automation | Implement vzdump and backup schedule management | +| L5 | **No pool management** | Module | Useful for multi-tenant environments | Implement pool CRUD | +| L6 | **`new Random()` for boundary generation** | `PveHttpClient.cs:393` | Not cryptographically secure (not a security risk for multipart boundaries, but could use `RandomNumberGenerator`) | Consider `RandomNumberGenerator` for .NET 6+ | +| L7 | **No alias support** | Module manifest | Some users prefer short aliases | Consider adding aliases like `cpve` for `Connect-PveServer` | +| L8 | **Tagged releases in GitHub** | Repository | No GitHub Releases | Set up release workflow with auto-generated release notes | +| L9 | **Container migration cmdlet** | Module | VMs have `Move-PveVm` but containers have no equivalent | Add `Move-PveContainer` | +| L10 | **SDN subnet management** | Module | SDN zones/vnets exist but subnets are missing | Add SDN subnet CRUD | + +--- + +## Summary Statistics + +| Metric | Count | +|---|---| +| Total cmdlets | 66 | +| Cmdlets with ShouldProcess | 43 (all destructive/mutating) | +| Cmdlets with OutputType | 66 (100%) | +| xUnit test files | 13 | +| Pester test files | 26 | +| Integration test contexts | 13 | +| PVE API areas covered | 10 of ~20 major areas | +| Critical issues | 4 | +| High issues | 8 | +| Medium issues | 10 | +| Low issues | 10 | +| NuGet dependencies (runtime) | 2 (Newtonsoft.Json, System.Text.Json) | +| Security vulnerabilities found | 0 | +| Hardcoded secrets | 1 (test password in CI, masked) |