Fixes from correctness and api-compat reviews:
1. Use BoundParameters to distinguish -Session omitted from -Session $null,
preventing accidental active-session clear when $null is passed.
2. Warn and return early when -Session is supplied but not the active session,
avoiding silent no-ops that leave the user's session variable populated and
functional but with expectations misaligned (they passed a session to
disconnect it, but disconnecting a non-active session is now explicit).
3. Use ReferenceEquals() explicitly instead of == for the identity check,
future-proofing against PveSession ever gaining value-equality semantics.
4. Fix the lifecycle test to check observable behavior (warning output) instead
of reaching into null PrivateData. Tests now verify both "no session to
disconnect" and "non-active session supplied" paths.
Disconnect-PveServer was calling DELETE /access/ticket, an endpoint that does
not exist in the PVE API spec. The call always failed but errors were swallowed,
so the cmdlet succeeded while claiming server-side invalidation occurred.
Rebase on PveCmdletBase to gain the -Session parameter, making explicit session
disconnection possible (the cmdlet previously only worked with the active
session). Update help text to clarify that PVE tickets expire server-side after
two hours and cannot be revoked.
Remove the DELETE call entirely — tickets are stateless and expire on their own.
Clear ModuleState.ActiveSession only when disconnecting the active session
(no -Session given) or when explicitly disconnecting the active session.
D001-D021 become ADR 0001-0021 in docs/decisions/, one decision per file.
D017's PESTER_VERSION amendment was a second decision in one entry and becomes
ADR 0022. ADR 0023 records the migration and reverses the lane2-change-plan
ruling that deliberately kept DECISIONS.md until the CI lane work landed.
DECISIONS.md is reduced to a stub with a D-to-ADR redirect table, so the four
released CHANGELOG entries and older issue bodies that cite it degrade to a
redirect rather than a dead reference.
docs/review/ and docs/lane2-change-plan.md are deleted (ADR 0024). Of 91
findings, 83 were resolved and six of the seven still open were already GitHub
issues; F021 was the exception and is now #130.
CLAUDE.md's Key Conventions list gains the two rules it was missing and becomes
the checklist, with the ADRs carrying rationale.
PveNetwork already deserialised bridge_vlan_aware as BridgeVlanAware, so a
VLAN-aware bridge could be read back but never created or changed. Both write
paths now take a -BridgeVlanAware switch.
Clearing the flag does not use bridge_vlan_aware=0. PVE merges the supplied
keys onto the stored stanza and accepts that 0 without acting on it, so the
obvious form is a silent no-op: an integration run against PVE 9 issued it and
Get-PveNetwork still reported 1. The endpoint's delete list is what actually
removes the key. The API schema advertises a plain boolean and gives no hint of
this, which is why the behaviour is pinned by an integration test rather than
inferred.
Set-PveNetwork guards the switch on BoundParameters so an update that omits it
leaves the flag alone; the create path follows the existing -Autostart form.
Only bridge_vlan_aware is added. bridge_vids is an independent parameter that
PVE defaults to 2-4094, and the issue asks only for the flag.
Coverage: the integration suite pins create, disable, re-enable, and that an
unrelated Set leaves the flag alone -- that last one kills a mutant that drops
the BoundParameters guard, which every other test survives. A model test pins
the read path the assertions depend on. The Pester unit tests assert only
parameter metadata; the defect is server-side, so nothing offline can catch it.
Closes#92
Two non-blocking review observations.
A 45s retry is indistinguishable from a hang with nothing on the wire, so
GuestLockRetry.Execute takes an onRetry hook and InvokeGuestTask reports
each reissue through WriteVerbose.
The gap between attempts now scales with the budget, capped at the 2s
production value. A caller passing a short window wants a fast answer
rather than one long sleep, which also takes the retrying unit tests off
a real 2s sleep each: the xUnit run drops from 8s to 4s. PveHttpClient's
window becomes a field so those tests can shorten it too.
WaitForStatusTransition refused to return while snapshot.Locked, and its
comment quoted the exact error it was meant to prevent. Locked reads the
guest config's lock: property; the failure is the flock on
/var/lock/qemu-server/lock-<vmid>.conf, which PVE exposes nowhere.
The flock cannot be observed, so it is retried. GuestLockRetry reissues an
operation for a bounded 45s while PVE reports failing to enter lock_config
for a guest, which it raises before doing any work.
Two seams, because the failure has two surfaces. PveHttpClient.SendAsync
retries the request for operations PVE serialises in the API handler; it
takes a request factory because an HttpRequestMessage cannot be resent.
PveCmdletBase.InvokeGuestTask reissues the call and re-waits its task for
operations serialised in the forked worker, where the POST returns 200 and
only the task fails. WaitForStatusTransition routes through the latter,
hence Func<PveTask>.
The predicate is path-specific and anchored at the start of what PVE said:
lock_file uses identical wording for storage, LVM and HA locks, and a
qmclone that fails after allocating disks must not be reissued into
"VM already exists". That requires the raw text, so it reads
PveTaskFailedException.ExitStatus and PveApiException.ApiMessage.
The Locked check stays — it is correct for the config lock — with a comment
that says so.
Closes#113
Review finding on #97: `statusReached` was sticky, so a guest that reached the
expected status once and then drifted away from it would still be reported as a
success at the deadline, contradicting the fallback's own meaning.
Track the most recent observation instead. A guest that drifts away and never
re-matches now raises PveTaskTimeoutException as it should. A failed poll leaves
the previous observation standing, so an API blip is not read as divergence.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PVE publishes a guest's new status while the operation still holds
/var/lock/qemu-server/lock-<vmid>.conf, so WaitForStatusTransition could return
while the guest was still locked and the caller's next request would fail with
"can't lock file ... got timeout".
Integration run 183 failed four tests from this one cause: Restart-PveVm -Wait
returned after 4.1s having seen "running", the following Stop-PveVm spent exactly
10.0s failing to take the lock, and that cascaded into the template convert,
clone, and remove tests. Run 184 - same commit, re-run - passed because its status
poll happened to take 10.1s, by which point the lock had cleared. The same
settling happens either way; the only variable is whether the wait absorbs it or
the next caller does.
The check goes in WaitForStatusTransition because all nine lifecycle call sites
(Start/Stop/Restart/Reset/Resume across VMs and containers) route through it.
`lock` comes from the status/current response the poll already fetches - present
on both qemu and lxc since PVE 5.4, below the module's 7.0 floor - so it costs no
extra request.
If the status is reached but the lock outlasts -Timeout the cmdlet still returns
success, so a call that succeeded before this change cannot become an exception
after it.
Recorded as D015, the guest-lock sibling of D014.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Polls GET /cluster/status for the cluster entry with quorate = 1, bounded
by a timeout (60 s default) and tolerating PveApiException while pmxcfs
and corosync restart during cluster formation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PVE's cluster-create task completes before corosync converges. Until the
node is quorate it rejects a join with "cluster not ready - no quorum?",
so New-PveCluster -Wait followed by Add-PveClusterMember failed for every
caller. Wait for quorum after the task, bounded by -Timeout (default 60 s,
following the -Wait timeout convention used by Stop-PveContainer and
Reset-PveVm). See DECISIONS.md D014.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Minor bump: this release adds features (New-PveVm disk controller/IO
options and Get-PveVmConfig key surfacing, #65) alongside two bug fixes
(#64 semicolon form-encoding, #68 guest-exec argv).
Updates the three release artifacts in lockstep: psd1 ModuleVersion,
psd1 ReleaseNotes, and CHANGELOG ([0.2.0] cut from [Unreleased]).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- VmService.ExecuteGuestCommand: guard against null elements in -Args.
The old JSON-serialization tolerated nulls (as "null"); the repeated-key
path would NRE in EncodeFormValue. Throw a clear ArgumentException instead.
- findings.json: refresh the stale counters block (untouched since F085) to
the actual ledger state — next_id 92, resolved 83 — and bump last_updated
to 2026-05-22. last_scan_date stays 2026-03-26 (F086–F091 came from issue
triage, not a formal review scan).
- VmServiceTests: add empty-array (single command entry) and null-element
(throws) cases.
Note: F091 is the correct next ID — F086–F090 already exist from prior
merged PRs (#60/#61/#66/#67); only the counters were lagging.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ExecuteGuestCommand JSON-serialized the args array into the agent/exec
'input-data' field — which is the process's STDIN, not its arguments. So
guest commands ran with no argv: cmd.exe started interactively and the
JSON blob ['/c','echo',...] arrived at its prompt.
PVE's agent/exec 'command' parameter is itself an array (element 0 = the
executable, the rest = argv) sent as repeated form keys. The low-level
client couldn't express repeated keys (Dictionary<string,string> only),
so:
- Add PostAsync(string, IEnumerable<KeyValuePair<string,string>>) to
IPveHttpClient/PveHttpClient; BuildFormContent now emits one key=value
field per pair, so a key may repeat.
- ExecuteGuestCommand builds command = [exe] + args as repeated 'command'
fields and no longer touches input-data.
Tests: form-encoder repeated-key + per-value encoding cases; VmService
tests asserting the command array, order, and absence of input-data; an
integration regression guard that echoes an arg and checks it round-trips
as stdout.
Tracked as F091. Closes#68.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Now that -ScsiHardware is part of HasDiskOptions(), the old "options were
ignored" wording was misleading: scsihw is written unconditionally as a
VM-level key and is never ignored. Reword to state that the per-disk
options are the ones dropped, and that -ScsiHardware (if specified) is
still applied. Accurate whether or not -ScsiHardware was passed.
Addresses PR #67 follow-up review.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- HasDiskOptions(): include -ScsiHardware so the "disk options ignored"
warning fires when -ScsiHardware is passed without -DiskStorage/-DiskSize.
- PveVmConfig.AdditionalProperties: lazy-init a backing field so the native
dictionary is built once rather than reallocated on every property access
(matters when iterating many configs in a pipeline). Safe because the model
is effectively immutable after deserialization.
- New-PveVm.Tests.ps1: add a case asserting -DiskIoThread on scsi with a
wrong -ScsiHardware (virtio-scsi-pci) is rejected, covering the validator's
"!= virtio-scsi-single" branch (not just the null case).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
New-PveVm (F089):
- Add -DiskBus (virtio/scsi/sata/ide, default virtio), -ScsiHardware (scsihw),
-DiskIoThread, -DiskAio, -DiskSsd, -DiskDiscard, -DiskCache so a tuned disk
(e.g. virtio-scsi-single + scsi0,iothread=1,aio=native,ssd=1,discard=on) can
be created in one call instead of diskless + a hand-built Set-PveVmConfig string.
- Disk spec built via BuildDiskSpec; ValidateDiskOptions runs before ShouldProcess
and rejects ssd on virtio and iothread on sata/ide or scsi-without-virtio-scsi-single
with clear errors, instead of letting PVE fail at VM start.
Get-PveVmConfig (F090):
- PveVmConfig was a fixed allow-list, silently dropping keys like scsihw, efidisk0,
tpmstate0, hostpci0. Add typed scsihw/efidisk0/tpmstate0 plus a [JsonExtensionData]
catch-all exposed as AdditionalProperties (native types via JsonHelper.ToNative,
per D013 — no JToken leakage). Makes the disk tuning above verifiable by reading
the config back.
Closes#65.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PveHttpClient.EncodeFormValue encoded &, =, +, space, and % but left ';'
literal. PVE's application/x-www-form-urlencoded parser treats a raw ';'
as a field separator (the historical alternative to '&'), so a value like
boot=order=scsi0;ide2
was split into 'boot=order=scsi0' plus an empty 'ide2' field, and PVE
rejected the PUT with "ide2: unable to parse drive options". This broke
any multi-device boot order set via Set-PveVmConfig -AdditionalConfig,
and any other value containing ';'.
Encode ';' as %3B. Safe under the existing minimal-encoding policy that
keeps ':' and '!' literal for cluster-join: cluster-join payloads never
contain ';', and PVE url-decodes config form values.
Tracked as F088. Closes#64.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Doc hygiene catch-up surfaced by the PR #62 review:
- CHANGELOG.md: the [Unreleased] section had accumulated all post-preview
work without ever being cut into release entries. Promote it into:
- [0.1.3] - new fixes from #58 (DiskSize normalization) and #59
(HttpClient -TimeoutSeconds + RequestTimeout surfacing)
- [0.1.2] - #43/#44/#45 fixes from PR #46
- [0.1.1] - the cmdlet expansion + OpenAPI validation that
actually shipped to PSGallery as 0.1.1
Reset [Unreleased] to empty.
- src/PSProxmoxVE/PSProxmoxVE.psd1: replace the stale "Initial preview
release" ReleaseNotes (carried over since 0.1.0-preview) with actual
0.1.3 notes. PSGallery shows this on the version page.
- CLAUDE.md: document the release process so future bumps update the
psd1 version, psd1 ReleaseNotes, and CHANGELOG together before the
tag is cut. Prevents this hygiene gap from recurring.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Releases #58 (LVM disk-size unit normalization) and #59 (HttpClient
timeout / -TimeoutSeconds) fixes to PSGallery.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The when filter (ex.InnerException is TimeoutException) only matches on
.NET 5+. On .NET Framework 4.8 — which CI exercises via the test project's
net48 target — HttpClient.Timeout throws a bare TaskCanceledException
with no inner exception, so CI failed:
Expected: typeof(PSProxmoxVE.Core.Exceptions.PveApiException)
Actual: typeof(System.Threading.Tasks.TaskCanceledException)
---- System.Threading.Tasks.TaskCanceledException : A task was canceled.
PveHttpClient.SendAsync never passes a CancellationToken to the inner
HttpClient.SendAsync, so the only way a TaskCanceledException can reach
this catch is HttpClient.Timeout firing — true on net48, .NET Core, and
.NET 5+. Drop the filter and wrap unconditionally.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When HttpClient.Timeout elapses, .NET throws TaskCanceledException — not
HttpRequestException — so the existing catch in PveHttpClient.SendAsync
missed it and callers got a raw stack trace. With -TimeoutSeconds now
configurable and documented, this gap became user-visible.
In .NET 5+ HttpClient surfaces transport timeouts as TaskCanceledException
with a TimeoutException inner; user-driven token cancellation does not.
Catch by that inner-type signature and rethrow as PveApiException with
HttpStatusCode.RequestTimeout, the resource path, and a message that
reports the configured timeout.
Adds SendAsync_TimeoutFires_ThrowsPveApiExceptionWithRequestTimeout which
swaps in a delaying HttpMessageHandler with a 50ms timeout to exercise
the path deterministically. Drops the redundant
DefaultSessionTimeoutIs100Seconds test (covered by
PveSessionTests.Timeout_DefaultIs100Seconds and the existing flow-through
test).
Addresses PR #61 review feedback.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves findings.json conflict — F086 (from #60, merged into main) and
F087 (this branch) both append to the trailing findings array. Kept both
entries, in numeric order.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PveHttpClient was constructed without setting HttpClient.Timeout, so
.NET's 100s default applied to every request. Multi-GB ISO uploads via
Send-PveFile on a real LAN reliably tripped this with TaskCanceledException
after 100 seconds, and there was no way to override it.
- PveSession gains a Timeout (TimeSpan) property, defaulting to 100s.
- PveHttpClient accepts an optional per-instance timeout override that
takes precedence over the session timeout.
- Connect-PveServer exposes -TimeoutSeconds to set the session default.
- Send-PveFile and Invoke-PveStorageDownload expose -TimeoutSeconds with
a 30-minute implicit default so large uploads/downloads do not trip
the 100s default. -TimeoutSeconds 0 means Timeout.InfiniteTimeSpan.
Tracked as F087. Closes#59.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- SizeParser: wrap TB-suffix overflow in try/catch so callers get
ArgumentException with the parameter name rather than OverflowException.
- New-PveVm/New-PveContainer: validate -DiskSize/-RootFsSize before
ShouldProcess so typos like "512M" are rejected even with -WhatIf and
even when the matching -DiskStorage/-RootFsStorage is omitted.
- Add Pester tests for the new DiskSize and RootFsSize validation paths,
including a new New-PveContainer.Tests.ps1.
- Add SizeParserTests coverage for the TB overflow path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Disk and rootfs size strings were interpolated directly into the disk
spec as "<storage>:<size>", so "60G" produced "local-lvm:60G". On
LVM/LVM-thin storages PVE parses the value after the colon as a volume
name unless it is a bare integer, returning "unable to parse lvm volume
name '60G'". File-backed storages mask this by accepting either form.
SizeParser.NormalizeToGibibytes() now strips G/GB/T/TB suffixes and
returns a bare GiB integer string, so the documented "32G" call shape
works on every storage type. Sub-GB units are rejected with a clear
error rather than being silently truncated.
Tracked as F086. Closes#58.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add 70 xUnit tests that validate every ValidateSet in the module
against the PVE OpenAPI spec. Three bugs found and fixed:
- Storage: remove `glusterfs` (dropped in PVE 9), add `btrfs`, `esxi`
- Backup compression: `none` → `0` (PVE uses "0" not "none")
- Cluster resources: remove `lxc` filter (PVE uses `vm` for both)
The pve-api-enums.json fixture (199KB) is extracted from the full
OpenAPI spec and contains parameter enum values for 302 API paths.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PVE 9 API requires 'type' on PUT /cluster/ha/rules/{rule} even for
updates. Added mandatory Type parameter with ValidateSet for
node-affinity and resource-affinity.
Updated integration test and Pester unit tests to pass -Type.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
.NET's FormUrlEncodedContent over-encodes characters like : and !
(%3A, %21) in form values. PVE's internal API consumers (specifically
the cluster join process) do not URL-decode these values before using
them, causing fingerprint comparison failures and password mismatches.
Replace with BuildFormContent/EncodeFormValue that only encodes
characters that break form parsing (&, =, +, space, %). This matches
curl's -d behavior and fixes cluster join "Cluster join aborted!" errors.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract re-auth retry into ReauthenticateWithRetry helper (fixes
cognitive complexity warning)
- Retry auth up to 10x with 3s delay — node B's auth services need
time to restart after joining the cluster
- Wrap join in try/finally so the test always reconnects to node A,
even if the join fails
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cluster join restarts auth services on the joining node, which
invalidates the PVE ticket mid-poll. When WaitForTask gets a 401,
catch it and re-authenticate with the password already available
in the cmdlet, then retry the wait.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
xUnit (47 new tests, 429 total):
- ClusterConfigServiceTests: 25 tests covering all 14 service methods
including URL encoding, null guards, and error responses
- HaServiceTests: 22 tests covering resources, groups, status, and rules
with URI encoding verification (vm:100 → vm%3A100)
Pester (187 new tests, 1525 total):
- ClusterConfigCmdlets.Tests.ps1: 11 cmdlets tested
- HaCmdlets.Tests.ps1: 14 cmdlets tested
Integration (ClusterConfig.Integration.Tests.ps1):
- Full 2-node cluster lifecycle with -Wait for task completion
- Uses root@pam ticket auth for cluster create/join operations
- HA group tests skip on PVE 9.0+ (groups migrated to rules)
- JArray indexing uses .Item() for PowerShell compatibility
Cmdlet improvements:
- New-PveCluster, Add-PveClusterConfigNode, Add-PveClusterMember now
support -Wait switch to block until task completes (via TaskService)
- GetClusterConfig returns JToken to handle array responses on standalone
- OutputType updated to PveTask for task-returning cmdlets
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
RegenerateCloudInitImage was calling GET /cloudinit/dump?type=user
which returns the cloud-init YAML content, not a task UPID. The
cmdlet then passed this YAML string to WaitForTask, causing a 501
error trying to poll a URI like "GET nodes/.../tasks/%23cloud-config..."
Fixed to call PUT /nodes/{node}/qemu/{vmid}/cloudinit which is the
correct regeneration endpoint that returns a UPID.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
D013 compliance scan found 2 remaining Newtonsoft type exposures:
- PveClusterJoinInfo.Nodelist: JArray → List<Dictionary<string, object?>>
- PveClusterJoinInfo.Totem: JObject → Dictionary<string, object?>
Both now use NativeListConverter/NativeDictionaryConverter for
deserialization. Full D013 compliance report added.
Updated findings.json: F085 status open with scan evidence.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace null-forgiving operator on Marshal.PtrToStringUni with
null-coalescing fallback (AddPveClusterMemberCmdlet.cs:64)
- GetNextId now throws InvalidOperationException instead of silently
returning 0 when API response cannot be parsed
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract ParseLinks helper to PveCmdletBase for shared link parsing
with WriteWarning on malformed entries (was duplicated in 3 cmdlets)
- Fix GetClusterConfig to return data payload, not full API envelope
- Fix OutputType on GetPveClusterConfigCmdlet to JObject
- Fix link doc comments to use correct key format (link0..link7)
- Add null-safe Properties hashtable conversion in HA rule cmdlets
- Use case-insensitive Mode comparison in MovePveHaResourceCmdlet
- Remove unused using directives
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
goodolclint-claude[bot]