Connect-PveServer -ApiToken was a plain string, so the token landed verbatim
in PSReadLine history and any transcript, and PveSession published ApiToken,
Ticket and CsrfToken as public getters, so Format-List *, ConvertTo-Json and
Export-Clixml of a session printed them.
The parameter is now a SecureString, extracted at the cmdlet boundary with the
Marshal/ZeroFree pattern ADR 0002 established for passwords. A plain string
still binds for one minor release through an argument transformation, and the
cmdlet warns that the string form goes away in the next major; the marker
lives in a ConditionalWeakTable keyed on the converted instance, so an
abandoned binding neither retains the secret nor mislabels a later call.
The three session getters become internal. PveHttpClient is in the same
assembly and the xUnit project already has InternalsVisibleTo, so the header
construction and its tests are unchanged.
Refs ADR 0028, issue #147.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* fix: renew ticket sessions inside PveHttpClient before expiry and once on a 401
A ticket session died two hours after Connect-PveServer, and any -Wait that
crossed that line surfaced as a raw 401 from the status poll. PveHttpClient
now renews a ticket past half its lifetime by posting it as the password to
/access/ticket, and after a 401 on a ticket-mode request renews once and
retries once; a failed renewal is PveSessionExpiredException with the 401
inner. Renewals are single-flighted per session through an in-flight task
so concurrent callers share one POST and its outcome, the credential is one
immutable snapshot so no request mixes two tickets, and the renewal POST is
bounded by the session timeout rather than the calling client's. API-token
sessions never renew. Implements ADR 0027.
* fix: keep the cluster-join password re-auth fallback across the client's ticket renewal
A cluster join rotates the auth key, so the 401 the status poll gets back
now reaches Add-PveClusterMember as PveSessionExpiredException after the
client's own renewal fails. Widen the fallback's catch so the password
re-authentication, the only path that survives a key rotation, still runs.
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
NodeService.GetNodeConfig/GetNodeDns, ClusterConfigService.GetClusterConfig,
BackupService.GetNotBackedUp and VmService.GetGuestExecStatus returned raw
Dictionary/List<Dictionary> instead of a Pve* model, per issue #157. Each now
has a typed model under Models/{Nodes,Cluster,Backup,Vms}/ with [JsonProperty]
for documented fields and a [JsonExtensionData]-backed AdditionalProperties
catch-all, following the PveVmConfig pattern. The five consuming cmdlets and
their [OutputType] attributes are updated to match.
GetClusterConfig also fixes a latent bug: GET /cluster/config returns a JSON
array (a directory index), but the old code did `data is JObject obj ? ... :
empty dict`, which silently always returned an empty dictionary since data was
a JArray. PveClusterConfigEntry decodes the array correctly and exposes a
typed Name property (the array items' schema documents no named fields, but
the endpoint's "links" metadata gives the child-URL template as "{name}").
The guest-exec poll loop in InvokePveVmGuestExecCmdlet keeps its exact
Stopwatch + Thread.Sleep(1000) structure (ADR 0001 accepted exception); only
the type it reads from changed. A TolerantBooleanConverter was added so
PveGuestExecStatus.Exited keeps accepting PVE's boolean/integer/string forms,
matching what ApiValueHelper.IsExited already tolerated for the old
dictionary path.
Reviewed with codex-rescue, correctness-reviewer and api-compat-reviewer
before commit; both real findings above (the name/subdir key and the Exited
string-form regression) came from that pass and are mutation-tested.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
WaitForStatusTransition and InvokeGuestTask lived on PveCmdletBase with
hard-coded qemu/lxc paths, a Thread.Sleep(2000) loop and their own
PveHttpClient, so ADR 0015's lock-clear wait and ADR 0020's flock retry
could only be exercised through the 45-minute integration lane.
Moves both into a new GuestLifecycleService on PveServiceBase, following
TaskService's shape: a parameterless ctor, an IPveHttpClient-injecting
ctor, and an internal ctor with a Func<TimeSpan, Task> pollDelay seam so
a test can drive the poll loop without sleeping. PveCmdletBase keeps the
same protected method signatures as thin forwarders wiring WriteVerbose
into a single Action<string>? onProgress callback, so none of the 14
cmdlet call sites change.
ParseLinks moves to a pure CorosyncLinks.Parse in Core (dictionary plus
the malformed entries), with PveCmdletBase.ParseLinks kept as a forwarder
that emits the WriteWarning — the same forwarder shape as the lifecycle
methods, so the warning stays in one place instead of being copied into
the three cluster cmdlets that call it.
Behaviour is unchanged: same status/current polling per guest type, same
GuestStatusSnapshot.Evaluate lock semantics, same filtered PveApiException
catch, same GuestLockRetry wrapping, same PveTaskTimeoutException.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
A binary module assembly loads once per process, so the internal static
ActiveSession slot was shared by every runspace in the host. Under
ForEach-Object -Parallel, a multi-runspace host or a JEA endpoint, one
runspace's Connect-PveServer retargeted another runspace's next cmdlet,
and a credential established in one runspace was reachable from all of
them.
The session now lives in the module's own session state, which
PowerShell creates once per runspace, read and written through
ModuleState.GetActiveSession/SetActiveSession behind the existing single
read point PveCmdletBase.GetSession(). The name is scope-qualified so a
nested scope cannot write a copy that dies with it, nor read through to a
same-named global. A module imported from the assembly rather than the
manifest has no session state; that path falls back to the runspace's
global scope, which is still per-runspace.
No IModuleAssemblyCleanup: no mutable static remains to clear.
Closes#150
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Delete CloudInitService.GetCloudInitConfig, ClusterConfigService.GetTotem/
GetQdevice/GetApiVersion, ContainerService.GetContainer, HaService.GetManagerStatus,
NodeService.GetVersion, TaskService.GetTaskLog, UserService.GetUser, and
VmService.ShutdownVm. #207 found these had no caller outside tests/; re-confirmed by
grep against main at 3ea0e11 before this change. Also deletes the xUnit tests that
existed only to exercise each method, CloudInitService's now-unused CloudInitFields
array, and HaService's now-unused JsonHelper using directive.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
VmService.GetVms(node: null) now issues a single GET cluster/resources?type=vm
instead of GET /nodes followed by one GET /nodes/{n}/qemu per node -- 17 round
trips down to 1 on a 16-node cluster. VmService.GetVm(session, node, vmid) now
fetches nodes/{node}/qemu/{vmid}/status/current directly instead of listing
the whole node and filtering client-side. GetPveVmCmdlet's -Detailed path
shares one IPveHttpClient across the enrichment loop and calls WriteObject
per VM as it is enriched, instead of materializing the whole list first and
opening a fresh client per VM.
cluster/resources's "type=vm" filter returns both qemu and lxc rows (per the
PVE OpenAPI spec), so the mapping filters to type=="qemu" explicitly. The
resources endpoint does not carry QmpStatus/Pid/AgentStatus, so those stay
null until -Detailed or GetVm enrich from status/current; the default table
view (VmId/Name/EffectiveStatus/Node/CpuCount/MaxMem/Uptime) is unaffected.
GetVm's status/current call converts a 404 or 500 into the pre-existing
InvalidOperationException("not found") contract that ImportPveOvaCmdlet
depends on, but 502/503/504 propagate unchanged since PveHttpClient wraps a
connectivity failure as 503 -- a down node must not read as a missing VM.
Removing the old per-node fan-out (issue #142's node-skip behavior) means
onNodeSkipped is no longer invoked for the all-nodes path: a single
cluster/resources call has no per-node failure to report, so a
node-unreachable condition now surfaces via that node's rows rather than a
WriteWarning. The parameter stays on VmService.GetVms/TemplateService.GetTemplates
for source compatibility with the cmdlets that still wire it.
TemplateService.GetTemplates and its tests are also updated since it
delegates to VmService.GetVms and inherited the same per-node fan-out.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
OvfMetadata.cs opened files and drove SharpCompress.Readers.ReaderFactory
inside a Models/ type, the only SharpCompress consumer in the codebase.
Move TAR extraction and OVF XML parsing to a new static
PSProxmoxVE.Core.Utilities.OvfReader (ReadOva, internal ParseOvf) and
leave OvfMetadata as a data type: properties plus the pure MapNicModel
helper.
The move is behavior-preserving: bus-type inference, href validation,
DtdProcessing.Prohibit, and disk-slot handling are unchanged.
OvfMetadata.FromOva is removed rather than kept as an [Obsolete]
forwarder — PowerShell ignores ObsoleteAttribute at invocation, the only
other caller is a non-code-owned integration test, and ADR 0028's
deprecation-path bar is for parameter-binding surface, not a Core
static method reached by fully-qualified type name. The integration
test now calls OvfReader.ReadOva directly.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
PveApiException carried the status, resource, method and API message an
ErrorRecord needs, but only about 25 of 194 cmdlets built a record. Every
other failing call escaped into PowerShell's default wrapper, so a 403, a
404 and an unreachable server all arrived as ErrorCategory.NotSpecified
with error id PveApiException and a null TargetObject. $_.CategoryInfo,
-ErrorAction filtering and typed catch blocks had nothing to work with.
The classification is a pure function in PSProxmoxVE.Core so it has an
offline test path: PveErrorMapper.Describe returns a kind, a stable error
id and a target derived from the exception. PveCmdletBase translates the
kind to an ErrorCategory and builds the record.
The try/catch is a template method rather than 194 hand-written blocks:
ProcessRecord is sealed and wraps a new abstract ProcessPveRecord in one
catch filtered by PveErrorMapper.IsRecognized, so only the module's own
exception types are mapped and every other exception reaches the engine
exactly as before, including the ones carrying their own error record.
The 192 cmdlets on the base class rename their override; behaviour stays
terminating, as an escaped exception already was.
Closes#155
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* test: drop the null-session guard tests only a test can reach (#154, part C)
Deletes xUnit tests across tests/PSProxmoxVE.Core.Tests/Services/ that
assert ArgumentNullException for a null PveSession, or for a null
string identifier fed only from a cmdlet parameter PowerShell's
mandatory binding already blocks from being null. Both are paths only
a test's null! literal can trigger; the guard code in src/ stays as
documentation. Kept: constructor null-dependency guards, a null action
to PveServiceBase.Invoke, null-argument tests on optional parameters
or on service methods with no cmdlet caller, and whitespace-guard
tests a real caller can still produce (mandatory binding blocks
null/empty but not whitespace-only strings).
* test: drop the null-session guard tests only a test can reach (#154, part C) [2/2]
* test: drop the null-session guard tests only a test can reach (#154, part C) [3/3]
* test: drop the null-session guard tests only a test can reach (#154, part C) [4/4]
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* ci: fail the PR when the generated help is out of date
docs/cmdlets/ and the MAML are outputs of generate-help.ps1, but no
workflow ran it, so the help drifted 25 cmdlets behind the source (#153)
and the stale MAML shipped in every PSGallery release. A help-current job
regenerates and fails on a dirty tree, comparing content rather than the
CRLF platyPS writes.
Depends on #205, which commits the regenerated help; before that merges
this job fails on its first run.
* ci: normalize untracked help stubs too, and quote the pathspecs
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* refactor: one task-response parser, and the dead code #154 names (part A)
Unifies the 8 byte-similar private ParseTask methods in BackupService,
ContainerService, NetworkService, NodeService, SnapshotService,
StorageService, TemplateService, and VmService into one shared
PveTaskResponse.Parse(json, node) utility, on the variant that stamps
Status = "running" for a bare-UPID response. BackupService, NodeService,
TemplateService, and VmService previously left Status null for that case;
their non--Wait task output now reports "running" like the other four
services already did.
Removes the duplicate ClusterConfigService.GetClusterStatus in favor of
ClusterService's; ClusterConfigService now holds a ClusterService built
from the same injected/default client, and WaitForQuorum and
Get-PveClusterStatus go through it.
Removes dead code named in issue #154 and its fold-in comments: the
never-called PveHttpClient/IPveHttpClient sync wrappers Put/Delete, the
never-thrown PveAuthenticationException, a #pragma around an
already-nullable field, the unreferenced TestHelper mock-handler helpers,
three hand-rolled version-warning blocks (now PveCmdletBase.WarnIfBelowVersion),
an unreachable catch(HttpRequestException) arm in WaitForStatusTransition,
dead ExitStatus-checking branches in ImportPveOvaCmdlet after WaitForTask
(which already throws on failure), and an unreachable int branch in
ApiValueHelper.IsExited.
Fixes the WaitForStatusTransition catch removal's premise: PveHttpClient
read the response body outside its HttpRequestException try block, so a
mid-body stream drop could still escape unwrapped. Moves the body read
inside the try so every HttpRequestException the client can throw becomes
a PveApiException, matching what the removed catch assumed.
Part of #154.
* Add service files: BackupService, ClusterConfigService, ContainerService, NetworkService
* Add service files: NodeService, SnapshotService, StorageService, TemplateService
* Add VmService and Utilities files
* Remove unused PveAuthenticationException (never thrown, caught, or tested)
* Add cmdlet files: GetPveClusterStatus, SDN subnets, PveCmdletBase, SendPveFile, ImportPveOva
* Add test files: BackupServiceTests, ClusterConfigServiceTests, NodeServiceTests
* Add remaining test files: TemplateServiceTests, VmServiceTests, TestHelper, ApiValueHelperTests, PveTaskResponseTests
* Add VmServiceTests
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Skip-IfMissing skipped a test whenever the cmdlet under test was absent
from the build, including the test asserting it exists. Every cmdlet
compiles into one assembly, so there is no partial-build case for it to
serve; all it did was hide a missing cmdlet. Delete the 37 copies and
every call site, and drop the residual conditional skips in the same
family (CmdExists probes, an attribute-presence skip in SdnCmdlets, a
lifecycle helper skipping on a cmdlet-name collision fixed long ago).
894 It blocks only reflected [Cmdlet] and [Parameter] attributes back at
the compiler: existence, CommandType -eq 'Cmdlet', Parameters.ContainsKey,
IsMandatory reflection, and per-file CmdletsToExport asserts for whichever
names an author remembered. Nothing reflected over the built assembly, so
a new cmdlet missing from the manifest shipped invisible. One data-driven
file replaces them: it diffs CmdletsToExport against the assembly's cmdlet
types in both directions and asserts the conventions reflection can see.
Behavioural tests are untouched: no-session errors, binding rejections,
ShouldProcess and -WhatIf, ConfirmImpact, ValidateSet and ValidateRange
values, parameter types, positions and pipeline binding.
The generated help covered 169 of 194 cmdlets. Regenerated with the repo's
own generate-help.ps1: 25 new markdown stubs, 13 existing docs picking up
parameters added in earlier waves, and a rebuilt MAML.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Collapses the ~24-line Level/Node/VmId/Group validation block duplicated
across 18 Firewall cmdlets into a single FirewallScope.TryValidate helper
in PSProxmoxVE.Core, beside FirewallService.BuildBasePath which already
owns the level-to-path mapping (ADR 0021: request-payload/validation
correctness is proven offline, not against a live cluster).
Each cmdlet now makes one TryValidate call and, on failure, one
ThrowTerminatingError with the same ErrorId (NodeRequired/VmIdRequired/
GroupRequired), ErrorCategory.InvalidArgument, target object (null) and
message text it used before, so the Pester assertions under
tests/PSProxmoxVE.Tests/Firewall/ keep passing unedited.
Part of #154.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* refactor: route the storage cmdlets through StorageService (#126)
New-PveStorage, Invoke-PveStorageDownload and Send-PveFile each built
their own PveHttpClient. They now call StorageService, which already
had CreateStorage/DownloadUrl/UploadIso with zero callers.
StorageService.UploadIso previously hardcoded content=iso regardless
of the cmdlet's ContentType parameter (iso/vztmpl/import), which would
have silently broken vztmpl/import uploads on conversion; it now takes
an optional contentType parameter. DownloadUrl gained an optional
timeout parameter so Invoke-PveStorageDownload -TimeoutSeconds keeps
working, matching UploadIso's existing default. Per issue #194, both
cmdlets construct a fresh StorageService() with no injected client so
the timeout override reaches PveServiceBase.CreateClient instead of
being silently dropped.
ParseTask now stamps Status = "running" on the UPID-string branch,
matching what the cmdlets stamped locally before conversion (same
rule PR #196 established for SnapshotService).
* test: add StorageService coverage for the #126 storage seam
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
FirewallService.GetGroupRules/CreateGroupRule/UpdateGroupRule/RemoveGroupRule
already existed with zero callers. Get/New/Set/Remove-PveFirewallRule gain a
Group value in their -Level ValidateSet and a -Group parameter, required and
validated the same way Node/VmId are validated for the other levels, and
dispatch to those service methods instead of the generic BuildBasePath-driven
ones. No cmdlet in this area built its own client, so unlike the other #126
areas there is no inline request to strip.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Wires the 12 direct-PveHttpClient Network/SDN cmdlets (Get/New/Set/Remove-PveNetwork,
Invoke-PveNetworkApply, Get/New-PveSdnZone, Get/New-PveSdnVnet, Get/New-PveSdnSubnet,
Remove-PveSdnSubnet) to the existing NetworkService methods, which previously had zero
callers for the New/Set/Get paths. NetworkService.ParseTask now stamps
Status = "running" on the UPID-string branch, matching SnapshotService's ParseTask from
PR #196 (the pattern PR). Offline tests for every wired service method are added to
NetworkServiceTests.cs.
Fold-in from issue #126's own comment: ValidatePattern identifier validation, already
present on Remove-PveSdnZone/Vnet and Remove-PveStorage from PR #161, is added to the
Zone/Vnet identifier parameters on the New-PveSdnZone/Vnet/Subnet and
Remove-PveSdnSubnet cmdlets converted here.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Get-PveCloudInitConfig and Get-PveTemplate each built their own PveHttpClient
inline. They now go through CloudInitService and TemplateService, which is
offline-testable per ADR 0021.
Get-PveCloudInitConfig's shipped output is the full PveVmConfig (per its own
synopsis), not the CI-key-filtered PveCloudInitConfig that
CloudInitService.GetCloudInitConfig already returns for a different purpose.
CloudInitService gains GetFullVmConfig, a thin delegation to the existing
VmService.GetVmConfig (already used by Get-PveVmConfig) rather than a
parallel HTTP implementation.
Get-PveTemplate's TemplateService.GetTemplates now forwards VmService.GetVms's
onNodeSkipped callback, matching Get-PveVm/Get-PveContainer, so an unreachable
node during the all-nodes listing produces a warning instead of a silently
short result. Two review-found regressions were fixed before commit: an empty
-Node value used to mean "query all nodes" and now does again (it had
started reaching the API literally as nodes//qemu), and the per-node warning
above.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
New-PveContainerSnapshot, Remove-PveContainerSnapshot and
Restore-PveContainerSnapshot each built their own PveHttpClient and parsed
the response inline while ContainerService.CreateContainerSnapshot /
RemoveContainerSnapshot / RollbackContainerSnapshot carried the same three
requests with no callers. The cmdlets now call the service, so the path
and form each sends is asserted offline (ADR 0021).
New-PveContainer already called ContainerService.CreateContainer but
allocated its container ID with a private PveHttpClient hitting
cluster/nextid; it now goes through ClusterConfigService.GetNextId, the
seam Copy-PveVm, Copy-PveContainer, New-PveVm and Import-PveOva already
use.
ContainerService.ParseTask's UPID-string branch now stamps
Status = "running", matching SnapshotService.ParseTask from the #126
pattern PR (#196). ParseTask is shared by every ContainerService lifecycle
method, so this also changes the non-Wait Status output of ten cmdlets
beyond the four converted here, from null to "running" — pinned with a
dedicated test.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Get-PveRole, Get-PveUser, New-PveRole and Set-PvePermission each built
their own PveHttpClient and hand-rolled requests, while UserService's
GetRoles/GetUsers/CreateRole/SetPermission carried the same requests
with zero callers. Wire the cmdlets to the service and change
SetPermission's propagate/delete parameters from bool with defaults
to nullable bool so the omitted-when-unset shipped behaviour of
Set-PvePermission survives the move.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Get-PveNode and Get-PveNodeStatus built their own PveHttpClient and
parsed the response inline. Both now call the existing NodeService
methods (GetNodes, GetNodeStatus), which already carried matching
requests. NodeService.GetNodeStatus gained the node-name stamp the
cmdlet used to apply after deserializing, and both GetNodes and
GetNodeStatus regained the missing-data guard the cmdlets had, which
the pre-existing service implementation lacked.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
New-PveSnapshot, Remove-PveSnapshot and Restore-PveSnapshot built their own
PveHttpClient and parsed the response inline while SnapshotService carried
the same three requests with no callers. The cmdlets now call the service,
so the path and form each sends is asserted offline (ADR 0021).
Reconciled toward the shipped cmdlet behaviour: CreateSnapshot omits
vmstate unless it is true, and ParseTask stamps Status = "running" on a
UPID-string response.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Every service method built and disposed its own PveHttpClient, and each
client owned a fresh HttpClientHandler, so every API call was a TCP connect
plus a TLS handshake and WaitForTask paid that 300 times over a ten-minute
wait.
PveServiceBase now owns the injected-or-fresh client lifetime behind one
Invoke helper; the 201 hand-written try/finally blocks across the 16
services collapse to calls on it, and the nested NodeService/VmService
instances receive the injected client. PveHttpClient takes its handler
from a process-wide PveHandlerCache keyed on (host, port,
skipCertificateCheck) and never disposes it, so the connection pool
outlives any one client. WaitForTask holds one client for the whole wait
and, when no pollInterval is supplied, backs off from 1 s toward a 10 s
cap, never sleeping past the deadline.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* docs: record that changelog entries are batched, and that the up-to-date rule is off
The automated reviewer on #182 requested a CHANGELOG.md edit on a fix
PR because the batching convention lived only in the remediation plan.
The reviewer reads CLAUDE.md from main on every run, so the rule goes
here. Also records the branch-protection change from 2026-09-02.
Closes#177
* docs: drop the remediation reference from the branch-protection note
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* docs: changelog entries for the wave 2 fixes
Lifts the Changelog section of #178, #179, #180, #181 and #182 into
[Unreleased].
* docs: do not call the Import-PveOva fallback documented; it was not
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Adds global.json (10.0 SDK, rollForward latestFeature) so local dev
resolves the same SDK line CI pins in five workflows, instead of
floating to whatever is installed.
Adds Directory.Build.props for LangVersion/Nullable, the two
properties identical across all three csproj (TargetFramework stays
per-project since the test project multi-targets net10.0;net48).
Adds Directory.Packages.props with central package management, moving
every PackageReference version out of the three csproj into one file.
Newtonsoft.Json bumped 13.0.3 -> 13.0.4 in the single place instead of
two independent declarations that could drift. xunit.runner.visualstudio
stays at 4.0.0 (it runs xUnit v1/v2/v3 per its own description; nothing
in the issue's stated xunit 2.9.3 pairing required a downgrade).
Removes the self-referential $(NoWarn) from the Core csproj, a no-op.
Adds an explicit System.Memory 4.6.3 PackageReference to the net48
leg of the test project, which resolves the MSB3277 conflict between
System.Memory 4.0.1.2 and 4.0.5.0 (110 warnings -> 0). Scoped to net48
only per the issue, which names only the net48 test target.
Adds PackageVersionCentralizationTests, an xUnit test asserting none
of the three csproj declare a PackageReference Version attribute
outside Directory.Packages.props.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* fix: retarget Import-PveOva's not-found catch and add upload timeouts
VmService.GetVm throws InvalidOperationException when the VM is not yet
listed on the node, never PveApiException(NotFound) — the API call itself
returns 200. Import-PveOva's tail catch was for the exception GetVm never
throws, so a successful import without -Wait raised the InvalidOperationException
unhandled instead of falling back to a basic PveVm. Retarget the catch, and
narrow its try region to the GetVm call only so the fallback can no longer
fire for a WriteObject failure on an already-retrieved VM.
VmService.UploadOva and StorageService.UploadIso built their PveHttpClient
with no timeout override, so large OVA/ISO uploads inherited the session's
100s default and aborted mid-transfer. Both now take a TimeSpan? timeout
(default 30 minutes), matching Send-PveFile. Import-PveOva gains
-TimeoutSeconds mirroring Send-PveFileCmdlet's parameter.
Also drops the trailing null, null, null on WaitForTask calls in
ImportPveOvaCmdlet.cs and NewPveVmCmdlet.cs, left over from #140.
Closes#139
* fix: give UploadOva/UploadIso a timeout override and add coverage
Completes the #139 fix: VmService.UploadOva and StorageService.UploadIso
now take a TimeSpan? timeout (default 30 minutes) instead of always using
the session's 100s default. Adds a GetVm not-found regression test and a
timeout-propagation test suite for both upload methods.
* test: release the upload before deleting its temp file
The two default-timeout tests left the upload in flight and then
deleted the file it still had open. Windows refuses that, so both
build-and-test legs failed on windows-latest.
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* fix: surface swallowed errors in status polling and per-node listing
WaitForStatusTransition's poll loop caught every exception except OOM/
StackOverflow and discarded it silently. An expired ticket, a deleted VM, or
a wrong node name under -Wait -Timeout spun for the full timeout and then
raised PveTaskTimeoutException instead of the real 401/403/404. The loop now
catches only PveApiException (excluding 401/403/404, which propagate) and
HttpRequestException, and WriteVerbose's what it swallows.
VmService.GetVms and ContainerService.GetContainers caught PveApiException
of any status per node and continued, so a permission problem or an
unreachable node looked identical to "no VMs". The per-node catch now
narrows to a 5xx/408/connectivity failure (IsNodeUnreachable) and takes an
optional onNodeSkipped callback; Get-PveVm and Get-PveContainer wire it to
WriteWarning. Any other status (401/403/404 included) propagates.
Adds xUnit coverage for the per-node aggregation path (403 propagates, 500/
408/connectivity failures are skipped and reported, the other nodes' results
still come back), reaching NodeService's internal client via reflection
since it is not otherwise constructor-injectable from VmService/
ContainerService.
Closes#142
* test: add per-node aggregation coverage and wire onNodeSkipped
Adds the remaining changes: VmService/ContainerService per-node catch
narrowing plus onNodeSkipped callback, the Get-PveVm/Get-PveContainer
WriteWarning wiring, and the xUnit coverage for the aggregation loop.
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* fix: allocate a real VMID for Copy-PveVm/Copy-PveContainer and honor -Storage
Copy-PveVm and Copy-PveContainer defaulted newid to 0 when -NewVmId
was omitted, and never sent -Storage on the clone request even
though both cmdlets declare it. Both cmdlets now allocate via
ClusterConfigService.GetNextId when -NewVmId is null, and forward
-Storage into the clone form body. Since PVE rejects storage on a
linked clone, -Storage without -Full now fails fast client-side
instead of failing later against the API.
NewPveVmCmdlet and ImportPveOvaCmdlet hand-rolled the same
GET cluster/nextid call with a manual JObject parse; both now go
through ClusterConfigService.GetNextId so a response without a
data field raises the service's diagnosable InvalidOperationException
rather than a NullReferenceException.
Closes#135
* test: pin CloneVm/CloneContainer storage and newid form-body behavior
Offline xUnit coverage per ADR 0021: storage present in the clone
form body when supplied, absent when omitted, and newid forwarded
verbatim (never coerced to 0) by the service layer.
* fix: add missing storage parameter to VmService.CloneVm
VmService.cs was omitted from the earlier push; this restores the
storage parameter and form-body wiring that belongs with this fix.
* fix: drop the client-side -Storage/-Full guard from the Copy cmdlets
PVE returns the same error itself when storage is sent on a linked
clone, so the guard only saved one round trip, had no test, and rested
on a behaviour claim the OpenAPI spec does not document.
* test: cover the -Storage/-Full guard in Copy-PveVm and Copy-PveContainer
Offline Pester coverage for the client-side StorageRequiresFullClone
guard: -Storage without -Full throws before a session is required,
and -Storage with -Full does not trip the check.
* test: revert the Pester cases for the removed -Storage/-Full guard
The guard was dropped in bfe2483, so these cases assert an error the
cmdlets no longer raise.
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
WaitPveTaskCmdlet hand-rolled its own poll loop (while(true) /
Thread.Sleep / JObject.Parse), the exact pattern ADR 0001 forbids.
It now delegates to TaskService.WaitForTask, passing a progress
callback that drives WriteProgress — the seam WaitForTask was built
for and that nothing called until now.
Preserves the cmdlet's own documented contract (an omitted -Timeout
waits indefinitely) by passing a 100-year sentinel instead of null,
since TaskService.WaitForTask treats a null timeout as its own
10-minute default, not infinite. Reuses one PveHttpClient for the
whole wait instead of letting TaskService open a fresh one per poll.
Also strips the redundant trailing 'null, null, null' default
arguments from 12 other WaitForTask call sites so they use the short
form, per the issue. Left CopyPveContainerCmdlet.cs,
ImportPveOvaCmdlet.cs and NewPveVmCmdlet.cs alone — issue #135 is
touching those concurrently.
Adds three xUnit tests to TaskServiceTests.cs proving the behavior
the inline loop got wrong: no sleep before the first status check,
the 1-second MinPollInterval clamp, and the progress callback firing
on every poll. Mutation-tested by breaking each behavior in turn and
confirming the corresponding test fails.
Closes#140
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* docs: a PVE-behaviour claim the PR documents against the spec is verified, not deferred
The reviewer fetches the cited proxmox_api permalink with gh api, names it in
the review, and approves when the spec supports the claim. Only inferred
claims still go to the operator.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* docs: record ADR 0026 and tighten the spec-verified rule per review
Commit-anchored permalinks only, the gh api invocation spelled out, and the
trade-off against the server-behaviour caveat stated in the prompt and in
the ADR.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* docs: cite the per-version OpenAPI file, and attribute the reserved list to the prompt
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
* docs: deferred reviews mention the operator so the decision reaches their inbox
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* ci: request the operator's review when the automated reviewer defers
A COMMENTED verdict sends no notification. The workflow now turns a
deferral into a formal review request, which reaches the operator's
queue and inbox. The prompt-only mention is dropped in favour of this.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
The tag name was substituted into the pwsh script text by expression, so a
tag containing a quote could run arbitrary PowerShell in the job that holds
NUGET_API_KEY. The script now reads it from an env var and rejects anything
that is not vX.Y.Z with an optional prerelease suffix.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* fix: plumb skiplock parameter through Remove-PveVm and Remove-PveContainer
When -Force is specified, both cmdlets now pass skiplock=1 to PVE, which bypasses locks.
PVE honours the skiplock parameter for root@pam only.
Also updated help text on both cmdlets to clarify the limitation and behaviour.
Fixes#136.
* fix: plumb skiplock parameter for VMs, force for containers
VM removal: add skiplock=1 parameter when -Force is specified. PVE honors it
for root@pam only; non-root callers receive 403 errors. Updated help text.
Container removal: map -Force to force=1 (LXC-specific parameter for forcing
removal of running containers). Containers do not support skiplock.
Also: clarified class and parameter documentation to remove false claims about
-Force suppressing confirmation (it does not).
Fixes#136. Addresses correctness reviewer findings.
* fix: revert unrelated formatting churn, add container force test
The em-dash-to-hyphen sweep in commit 75f4bbf touched VmService.cs and
VmServiceTests.cs outside the skiplock/force change; restore the
original text there, including the <vmid> XML-doc escape that
had been un-escaped and was malforming the generated documentation.
Add ContainerServiceTests.cs, covering the untested force=1
query-string plumbing in ContainerService.RemoveContainer.
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
UserService.GetPermissions unwrapped the path-keyed /access/permissions
response and discarded prop.Value, so every returned PvePermission had a
path but no privilege data. Add a Privileges dictionary populated from
the privilege map; a key's presence means the privilege is granted and
its value is whether the grant propagates to sub-paths, matching PVE's
documented "propagate boolean" contract for that endpoint.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* fix: remove defunct DELETE /access/ticket call and gain -Session parameter
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.
* fix: address reviewer findings on -Session handling and test coverage
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.
* fix: remove unreliable offline lifecycle tests from Pester suite
The lifecycle tests attempted to capture warning output and verify module state,
but both approaches fail in the offline Pester environment:
1. WarningVariable captures don't work as expected in Pester contexts
2. ModuleState is not exposed through PrivateData and cannot be inspected from
outside the module in offline tests
Keep the reliable metadata test that verifies -Session parameter is exposed.
The behavioral verification of the warning logic happens in the correctness
review and will be validated in integration testing, not in offline unit tests.
* fix: move reference equality check before ShouldProcess
The ShouldProcess check was firing before validating whether the supplied
-Session is actually the active session. This means -WhatIf would prompt
"Disconnect host:port?" even for sessions that will not be disconnected
(where the cmdlet just warns and returns).
Move the ReferenceEquals check before ShouldProcess so mismatched sessions
short-circuit with a warning before any prompting occurs. This accurately
represents what -WhatIf/-Confirm is about to do.
* test: restore and harden offline coverage for Disconnect-PveServer session handling
Commit 3f75d4a deleted two offline Pester tests for the reference-equality
logic added in fa930db, with a wrong justification (WarningVariable
captures don't work in Pester). WarningVariable works fine; the real
defect in the deleted mismatched-session test was passing a
[PSCustomObject] to a parameter typed PveSession, a type-binding
failure, not a WarningVariable failure.
Restores both cases via reflection against PveSession's internal
constructor and ModuleState's internal static ActiveSession property
(both types are internal/have internal members, so Pester has no other
way to construct a real session or observe module state), and adds
coverage for the two branches the deleted tests never exercised: the
active-session clear itself, -Session pointing at the active session,
and -WhatIf leaving the active session untouched. Per ADR 0021 this
logic makes no server call and must be pinned offline.
* fix: word the mismatched-session warning for the session's auth mode
API-token sessions do not expire and can be revoked with Remove-PveApiToken,
so the ticket wording was wrong for them.
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Import-PveOva ignored disk.BusType and always assigned scsi{i}; the
controller-mapping helper itself had CIM ResourceType 6 (Parallel SCSI HBA)
and 20 (Other storage device, VMware's SATA AHCI controller) swapped. Both
are fixed together since honouring BusType is what makes the mapping bug
observable. The cmdlet now tracks a running index per bus (scsi/sata/ide),
falls back to scsi for an unrecognized or missing bus type, and refuses to
emit a slot number beyond what PVE's qemu-server schema accepts (ide0-3,
sata0-5, scsi0-30), overflowing to scsi and erroring out before upload if
even that is exhausted.
OvfMetadata also trusted the OVF descriptor's ovf:href attribute (an
attacker-controlled disk file name inside a downloaded OVA) unvalidated,
letting a crafted href inject extra keys into the comma-separated PVE
property string written for import-from. hrefs are now checked against
^[A-Za-z0-9._-]+$ and rejected outright if they are exactly "." or "..",
closing a path-segment escape the character class alone did not block.
The regex is anchored with \A/\z rather than ^/$ so a trailing line feed
cannot slip past under .NET's default multiline-$ semantics.
The descriptor is now parsed through XmlReader with DtdProcessing.Prohibit
and XmlResolver = null instead of XmlDocument.LoadXml directly, closing
the internal-entity-expansion memory-exhaustion route a hostile descriptor
could otherwise use.
ParseOvfXml is now internal (assembly already has InternalsVisibleTo the
test project) so the new tests can drive it directly with inline OVF XML
strings instead of building TAR archives.
Reviewer findings acted on (Codex, correctness-reviewer, security-reviewer,
run in parallel against the working tree): the dot/dot-dot href bypass and
the missing \A/\z anchoring were found independently by all three and
fixed; the bus-slot ceiling was found by two and fixed. Findings not acted
on, with reasons: validating the local -Path OVA filename the same way
(security-reviewer, medium) — that name is operator-supplied, not drawn
from the attacker-controlled archive contents the issue names, and touching
it would widen the change past the two issues' stated scope; percent-
decoding/loosening the href character class to admit spec-legal names with
spaces or encoding (security-reviewer, correctness-reviewer, medium) —
issue #148 specifies this exact character class, and loosening it reopens
the property-string injection the issue asks to close; capping the raw
byte size of the extracted .ovf entry against decompression-bomb exhaustion
(security-reviewer, low/medium) — a distinct DoS vector from the DTD
entity expansion issue #148 names, not part of its stated scope; branching
CIM ResourceType 20 on ResourceSubType to separate SATA from NVMe
(security-reviewer, low) — issue #138 specifies the 6/20 mapping exactly
as fixed here. An existing Integration-tagged Pester assertion
(tests/PSProxmoxVE.Tests/Integration/12_OVA.Tests.ps1:66) checks
$config.Scsi0 specifically; if that suite's fixture OVA ever carries a
type-20 controller the disk will now land on sata0 instead and the
assertion will need updating — left alone here since it is excluded from
every offline run this change was verified against and touching it without
being able to run it against live PVE would be guessing.
Closes#138Closes#148
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* fix: recognize boolean and varied integer formats for guest exec exited status
The guest-agent schema declares 'exited' as a boolean, but the PVE API
may return it in different formats: as a JSON boolean true, or as integers
1/0, or string '1'/'0'. The cmdlet was only checking for long 1L, causing
it to timeout on any PVE build that passed a boolean true unchanged.
Add ApiValueHelper.IsExited() to normalize these values and recognize
true, 1L, 1, and '1' as exited. Update the polling loop to use it.
* test: add integration tests for ApiValueHelper with real JSON payloads
Add tests that feed JSON data through the actual JsonHelper.ToNative
parsing pipeline to verify ApiValueHelper.IsExited correctly recognizes
boolean true and numeric 1 values as they arrive from the PVE API.
These tests pin the contract between the JSON parsing layer and the
value-recognition logic, ensuring the fix for issue #141 works end-to-end
with real API response shapes.
* fix: restore correct IsExited implementation with type checks
The helper must handle boolean true, long/int 1, and string "1" as the
issue specifies. This restores the correct multi-type check that was
inadvertently reverted.
* fix: correct boolean value handling in IsExited
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* fix: verify checksums for downloaded ISOs/images, keep sshpass off argv
ensure-base-iso.sh downloaded the PVE install ISO over plain HTTP with no
checksum, caching it on the persistent /opt/pve-integration mount and
booting it as the nested trust root the integration suite relies on.
ensure-cloud-images.sh fetched the Ubuntu cloud image and OVA over HTTPS
but never checked them either. prepare-test-environment.sh and
diagnose-cluster.sh passed the nested root password to sshpass via -p,
putting it in the process table. create-api-token.sh, unused anywhere in
the repo, minted a privsep=0 root token and echoed the secret unmasked.
- ensure-base-iso.sh now downloads from https://enterprise.proxmox.com/iso
and verifies against its SHA256SUMS on every run, including a cache hit.
download.proxmox.com's own TLS cert does not list download.proxmox.com in
its SAN (confirmed with curl/openssl from this environment), so https to
that name fails certificate validation; enterprise.proxmox.com serves the
identical ISO tree over a valid cert. Verification happens before the
downloaded file is moved to its canonical cache path.
- ensure-cloud-images.sh verifies the cloud image and OVA against Ubuntu's
published SHA256SUMS the same way, matching by upstream filename since
the cloud image is cached locally under a different extension (.img
upstream, .qcow2 cached — the bytes are already qcow2-formatted).
- prepare-test-environment.sh and diagnose-cluster.sh now export SSHPASS
and call sshpass -e, keeping the password out of argv/ps. This also fixes
a latent bug: the old unquoted `sshpass -p ${ROOT_PASS}` word-split any
password containing whitespace.
- create-api-token.sh deleted; grep across the repo found no caller.
Reviewers (codex:codex-rescue, correctness-reviewer, security-reviewer) all
independently found the same blocking bug in the first pass: when a cached
file failed verification and the subsequent redownload then failed,
ensure-cloud-images.sh fell through to a "keep the stale copy" branch and
returned that same known-bad file with exit 0 — verification could be
bypassed by inducing one failed redownload. Fixed by deleting the file
immediately on a failed verification, before the redownload is attempted,
so the later "is there a safe stale copy" check can no longer find it.
Added a test case (case 5) that reproduces this exact sequence and
mutation-tested it against the unfixed code. The three reviews also
flagged a real but separate bug already fixed in this same change: `trap
... RETURN` inside a function nested in another function is not scoped to
that function in bash — it re-fires on the OUTER function's return,
referencing an out-of-scope local. Both verify_checksum() helpers now
clean up their temp file explicitly instead of via trap.
Findings not acted on, judged out of scope for this fix:
- SHA256SUMS-fetch failures are treated the same as a checksum mismatch
(delete + fail) rather than left untouched — a transient network blip
destroys a good multi-GB cached ISO. This is the safer failure direction
(never silently trust unverified bytes) and was a deliberate trade-off,
not a defect.
- ensure-cloud-images.sh's 7-day cache window can span an upstream
republish of noble/current, causing a legitimate re-verification churn
(not a security issue, a cache-hit-rate one). Pre-existing cache design,
unrelated to adding verification.
- wait-for-pve.sh (curl -d with the password on argv) and
prepare-test-environment.sh's own positional password argument (from
run-integration.sh) carry the same password-on-argv pattern this issue
targeted in create-api-token.sh, sshpass -p and diagnose-cluster.sh, but
neither script nor run-integration.sh was named in the issue. Left
untouched per scope; worth a follow-up issue.
- GPG/detached-signature verification of the upstream SHA256SUMS was not
added — the new checks defend against cache poisoning and transit
corruption, not a compromised origin. Worth a follow-up issue.
- The two new self-checks (ensure-base-iso.test.sh,
ensure-cloud-images.test.sh) are not wired into
.github/workflows/unit-tests.yml's shell-selfchecks job. That file is
code-owned and out of scope for this change; needs an operator follow-up.
Password rotation (the Testpass123! value from before it moved to a
secret) is unaddressed here per the contract — flagged for the operator.
Mutation-tested: broke the post-download checksum check in
ensure-base-iso.sh, confirmed the affected test cases failed, restored it.
Broke the sshpass -e change back to -p, confirmed the new assertions in
prepare-test-environment.test.sh failed, restored it. Broke the fail-open
fix in ensure-cloud-images.sh, confirmed case 5 failed, restored it.
Closes#149
* fix: also verify the stale-by-age fallback copy in ensure-cloud-images.sh
PR review on #166 (COMMENTED, non-blocking) found the sibling of the
fail-open bug already fixed in this branch: when the cached cloud image
is stale by *age* (>= 7 days) rather than failed verification, the
redownload-failure fallback could hand back that file with exit 0
without ever re-verifying it in this run. A file that failed the
earlier verification is already deleted by the time the fallback runs,
but a stale-by-age file skips verification entirely on the way in.
Fixed by verifying the stale-by-age file at the point of actual
fallback use — after the redownload has failed, not proactively before
it's attempted, so a copy the redownload was about to replace anyway
isn't deleted along a path that would have succeeded. Added two test
cases (6, 7): a still-verifying stale-by-age copy is used as a
fallback; one that no longer verifies is not. Mutation-tested by
reverting to the unfixed fallback and confirming case 7 fails, then
restored.
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
PveHttpClientLockRetryTests set GuestLockRetry's retry budget via reflection
on a private field with no production writer, and scripted two lock
failures to land inside a 400ms window. A cold or loaded CI runner's
first-attempt JIT and scheduling could burn past 400ms before the second
attempt started, failing the test though the retry itself was correct
(#134).
GuestLockRetry.ExecuteAsync gains an internal overload that takes the
inter-attempt delay as a Func<TimeSpan, Task>; the public overload keeps
defaulting to Task.Delay, so production behaviour (45s window, budget/4
capped at 2s) is unchanged. PveHttpClient gains a matching internal
constructor seam (window, handler, delay), replacing the reflection the
tests used for both the private HttpClient and the retry window. Tests
now pass a no-op delay, so the retry loop's real elapsed time drops to
microseconds and the production 45s window can never be exhausted by
runner speed.
Two tests pin that production still waits for real: one records the
delay invocations through the internal seam and asserts the computed
interval, the other drives the public overload with a small window and
asserts wall-clock time actually advances. Both were mutation-tested
against a no-op-default regression and fail without the fix.
The give-up test was renamed (PutAsync_DoesNotReissueWhenTheRetryWindowIsAlreadySpent)
to describe what TimeSpan.Zero actually proves: the client never attempts
a reissue once the budget reads spent, not a multi-attempt exhaustion
sequence — a review finding on the original name.
Out of scope, noted for follow-up: GuestLockRetryTests.cs's synchronous
Execute() tests still use a 400ms ShortWindow with real Thread.Sleep,
which is the same flake shape on the sync path; Execute() has no delay
seam. PveHttpClientTimeoutTests.cs and PveHttpClientFormEncodingTests.cs
still reflect on the private _httpClient field, which the new handler
seam could also retire.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* fix: escape dynamic path segments in three Remove-* cmdlets
Wrap Storage, Vnet, and Zone parameters with Uri.EscapeDataString() in
RemovePveStorageCmdlet, RemovePveSdnVnetCmdlet, and RemovePveSdnZoneCmdlet
to prevent path traversal attacks via the API path.
Add xUnit test demonstrating that escaped paths preserve percent-encoding
(preventing path collapse) while unescaped paths allow segment traversal.
Fixes#145
* fix: route Remove-Pve{Storage,SdnVnet,SdnZone} through their services
Delete the private PveHttpClient construction and inline
Uri.EscapeDataString call in RemovePveStorageCmdlet,
RemovePveSdnVnetCmdlet and RemovePveSdnZoneCmdlet; call
StorageService.RemoveStorage / NetworkService.RemoveSdnZone /
NetworkService.RemoveSdnVnet instead, which already escape the
identifier identically and are now the single place doing so.
Add ValidatePattern on the Storage/Vnet/Zone parameters as defense
in depth, anchored with \A/\z so a trailing newline cannot slip a
disallowed character past the gate.
Replace PveHttpClientPathEscapingTests with a version that actually
regression-tests the real Uri parser (asserts both that %2F survives
and that the unescaped form is absent), and add
StorageServiceTests/NetworkServiceTests cases that mock
IPveHttpClient and verify the exact escaped DELETE path for a
traversal-attempt name.
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Every merge to main queued a ~45 minute integration run behind fixes that
are already pinned by offline tests. Runs now happen on workflow_dispatch
at wave boundaries. Restore the push trigger when #134-#157 have landed.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* docs: record the branch protection that is actually configured
CLAUDE.md has claimed since March that main had 'required build checks,
required review, admin enforced'. Two of those three were wrong: direct pushes
to main were still permitted until 2026-09-02, and admin bypass is now
deliberately available rather than enforced.
Records what is actually set: required checks, required PR with an approving
review, stale reviews dismissed on push, and review from Code Owners required.
Explains the narrow CODEOWNERS, so a future session knows why a workflow or ADR
change needs the operator while ordinary work does not.
Admin bypass is available on purpose. GitHub does not let an author approve
their own PR, so without it an operator-authored governance change would
deadlock. It is not a hole here: App installations do not get admin bypass, so
it does nothing for a compromised or injected bot.
ADR 0025 previously said enabling code-owner review was the operator's action
and not done by that decision. It has been done, so the dismissal gate is now
defence-in-depth rather than the load-bearing control - GitHub enforces the
property directly, including against approvals from claude.yml, the ungated
path the gate cannot see.
* docs: trim a clause ADR 0025 stated twice in four lines
Review nit on #133. The 'claude.yml is an ungated second path the dismissal
step cannot see' clause appeared in consecutive paragraphs. The first states
it; the second now refers back rather than restating.
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
* ci: take review instructions from the default branch, and gate self-approval
Moves the review prompt out of claude-code-review.yml into
.github/review-prompt.md, materialized from the default branch at review time.
Prompt edits then neither trip claude-code-action's anti-tamper gate nor
influence the review of the PR making them. PR #131 is what prompted this: 38
files of ADR migration were blocked from review by a four-line prompt edit.
A security review of the first draft found the naive materialization was itself
exploitable, and that is fixed here rather than shipped:
- git show "origin/$DEFAULT_BRANCH:..." takes an UNQUALIFIED refname, and
gitrevisions(7) resolves refs/tags/<name> before refs/remotes/<name>. With
fetch-depth: 0 fetching all tags, a tag named "origin/main" would supply the
review instructions for every PR, exiting 0 with only a stderr warning.
Reproduced end to end. Now resolves refs/remotes/origin/<default> to a SHA,
logs it, and reads by SHA.
- CLAUDE.md is materialized from the default branch too. The prompt judges
against its Key Conventions list, so reading it from the PR checkout let a PR
edit the list to permit its own violation.
- A PR touching review-prompt.md, .github/workflows/ or CLAUDE.md now has any
claude[bot] APPROVED dismissed via the API and the check failed. Prose alone
cannot protect the root of trust.
- The sentinel is grepped in the materialize step rather than only asserted by
the model it protects.
- Fork PRs are skipped, not failed. A required check permanently red on outside
contributions trains the operator to override red checks.
Also drops track_progress and gh pr comment so the review body and inline
comments are the only channel, and adds actions: read plus gh pr checks / gh run
view so the reviewer can verify build and test claims against CI's own result.
It deliberately gets no build or test tools: those execute PR-authored code in a
job that can approve the PR.
ADR 0025 records the decision.
* ci: fix three defects found in second-opinion review
- Job-level 'actions: read' was missing. An explicit permissions block sets
every unlisted permission to none, so additional_permissions: actions: read
on the action alone granted nothing and the reviewer could not have read the
check runs it was just told to verify claims against. Athena has both; only
the action-level half was copied.
- The governance detector checked review-prompt.md, .github/workflows/ and
CLAUDE.md, while review-prompt.md told the reviewer DECISIONS.md and
docs/decisions/ were mechanically covered too. A PR adding an ADR could
escape the guard it was promised to be under. Detector now covers both, which
means every ADR PR needs operator approval — that is the intended reading of
ADR 0023, since the reviewer defers to ADRs as precedent.
- The formal-review check counted ANY historical claude[bot] review, so a
re-run after a new push went green on a verdict about the previous commit.
Now scoped to the head SHA. Pre-existing, fixed here because the file was
already open.
Adds a fork-notice job. A skipped job reports its required check as PASSING, so
skipping the review on fork PRs made them green with nothing reviewed and no
trace of why; the notice puts it in the run summary.
Prompt: pending checks are a race, so say unverified rather than reporting a
queued check as a failure; and defer-to-operator maps to --comment, not
--request-changes.
* ci: withhold approval on review-governing PRs, not the review itself
Operator ruling: Claude should still review the protected files and report what
it finds; only the power to approve is reserved.
The gate failed the check unconditionally whenever a PR touched a governance
path, even when the reviewer had correctly submitted COMMENTED. That discarded a
review that was wanted, and made a red check the normal outcome for a whole
class of PR — training exactly the merge-past-red habit the fork-notice change
exists to avoid.
Now: a COMMENTED deferral passes, with a notice saying the check is green
because the reviewer behaved, not because the PR is approved. Merge still waits
for the operator, since COMMENTED does not satisfy branch protection. The step
fails only when claude[bot] actually approved — that approval is dismissed and
the red check records the disobedience.
ADRs stay in the protected set, per the same ruling.
* ci: close a green-with-live-approval hole and widen the governance detector
Third security pass. The two that mattered:
- Both review queries were unpaginated. GitHub returns 30 reviews oldest-first,
so an approval submitted now sits on page 2 of any PR that already has 30
review objects — the withhold step would find nothing, print 'reviewer
behaved correctly' and exit 0 green while the approval stood and satisfied
branch protection. Reachable without an attacker: every inline comment
creates a review object. Both queries now --paginate.
- The detector covered CLAUDE.md but not .claude/, .mcp.json, AGENTS.md, or
nested CLAUDE.md. .claude/settings.json is tracked and .gitignore had no
claude entry, so a PR could add .claude/settings.local.json - which outranks
settings.json - carrying env (redirect model traffic) or hooks (arbitrary
shell in the job holding the approval token). Those are read by the runtime
before the model starts, so no prompt-level rule can defend against them.
Added to the detector and to .gitignore.
Also: dismissal now matches .user.type == 'Bot' rather than the claude[bot]
login literal, so an approval from another App is not invisible; the test
fixtures, psd1 and CHANGELOG join the detector, since the prompt already
reserved release tagging to the operator and did not enforce it; a failed
dismissal says so loudly instead of aborting silently under bash -e and
claiming success; the pre-review notice no longer promises a red check on the
path that goes green; show-ref --verify replaces rev-parse, which still DWIMs
on a ref that does not exist; and a concurrency group stops two runs
interleaving dismissals.
Dropped the '@claude re-review' suggestion from the fail-closed message: it
pointed at an ungated workflow on exactly the PRs where approval is reserved.
ADR 0025 now states the premise the whole design rests on - that code-owner
review is off, deliberately, because enabling it would end bot merging - and
records claude.yml as an open second path to a binding approval.
* ci: narrow CODEOWNERS so code-owner review becomes usable
Operator's suggestion, and it is better than what ADR 0025 previously recorded.
CODEOWNERS was '* @goodolclint'. At that breadth 'Require review from Code
Owners' is unusable — it would demand the operator on every PR and end the
verdict-gated merge loop — which is why the setting is off and why the
self-approval guard had to live in the workflow.
Narrowed to the governance and release paths only, matching the detector. An
ordinary PR has no code owner and an automated approval still merges it; a PR
touching what governs review or what gets published requires the operator.
That makes the setting safe to enable, and GitHub then enforces the property
better than the workflow step can: not one-shot, no pagination limit, no bot
identity to match, no dismissal permission needed, and it covers an approval
from any source — including claude.yml, the ungated second path the dismissal
step cannot see.
Enabling the setting is the operator's action, not this commit's. Until then
the workflow gate remains load-bearing, and it stays either way as
defence-in-depth. Both files carry a keep-in-sync note; drift is silent in the
direction that matters.
ADR 0025 records the edge case: GitHub does not let an author approve their own
PR, so an operator-authored governance PR would need admin enforcement toggled
or to go through the bot.
* ci: stop the green-path notice claiming more than it checked
Third-party re-review: the empty-id branch announced 'It reviewed and deferred,
as intended', but an empty list only means no automated APPROVED was found. It
cannot distinguish a deferral from CHANGES_REQUESTED, from no verdict, or from
no review at all — that a formal review exists at this head SHA is established
by the verify step, not this one. The notice now says what was actually
checked, and says plainly that green does not mean approved or adequately
reviewed.
ADR 0025 said a prompt edit 'gets a red check', contradicting its own statement
two paragraphs earlier that a deferral passes. Corrected, and it now records
the one governance path that genuinely gets no review: this workflow itself,
where the action's anti-tamper gate means there is no verdict to observe.
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
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.
Records the testing strategy decided while planning #120: an integration test
must earn its place by testing something only a live PVE can answer, and
request-payload correctness is verified offline against the mock IPveHttpClient
harness.
The boundary comes from #92. Set-PveNetwork sent bridge_vlan_aware=0 to clear a
VLAN-aware bridge; the schema advertises a plain boolean, so the request
succeeded, and PVE merged the key onto the stored stanza and ignored the 0. Only
delete=bridge_vlan_aware works, and only a real PVE 9 revealed it.
37 of 194 concrete cmdlets construct PveHttpClient directly and have no offline
seam; they convert before the next large coverage push. The suite tiers by area
on PRs, ACME is covered by contract tests because a CA and DNS reachability
cannot exist in CI, and Ceph lives behind an opt-in provisioning profile.
goodolclint-claude[bot]