18 Commits

Author SHA1 Message Date
Clint Branham 56dcf22cea fix: address PR #69 review feedback
- 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>
2026-05-22 14:38:54 -05:00
Clint Branham bc71ed4a12 fix: deliver Invoke-PveVmGuestExec -Args to the guest as argv
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>
2026-05-22 14:32:04 -05:00
Clint Branham 63ee16a9e7 Merge remote-tracking branch 'origin/main' into feat/new-pvevm-disk-options
# Conflicts:
#	docs/review/findings.json
2026-05-22 14:15:20 -05:00
Clint Branham c1714048b4 feat: disk controller/IO options on New-PveVm + surface all Get-PveVmConfig keys
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>
2026-05-22 14:10:14 -05:00
Clint Branham 3fd770d84c fix: percent-encode ';' in form values
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>
2026-05-22 14:04:31 -05:00
Clint Branham ea2bcdc336 Merge main into fix/http-client-timeout
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>
2026-05-20 18:17:20 -05:00
Clint Branham f3b06171b2 fix: add -TimeoutSeconds for long-running HTTP calls
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>
2026-05-20 18:08:26 -05:00
Clint Branham fa361a3691 fix: address PR #60 review feedback
- 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>
2026-05-20 18:06:13 -05:00
Clint Branham 6181c8ce77 fix: normalize -DiskSize and -RootFsSize before sending to PVE
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>
2026-05-20 17:59:21 -05:00
Clint Branham 197e206123 chore: scan-9 review findings + fix publish secret name
- Resolve F039 (bare catch regression), F053 (HA coverage), F060
  (cluster config coverage), F084 (PveSession secret hiding)
- Update findings.json: 11 open → 7 open, 0 regressions
- Update REVIEW_REPORT.md with scan-9 results
- Fix publish.yml to use NUGET_API_KEY secret (matches other projects)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-26 08:58:35 -05:00
copilot-swe-agent[bot] 4a63055c10 chore: remove pre-correction D013-compliance.md and fix JsonHelper XML doc comment nullability
Co-authored-by: GoodOlClint <151449+GoodOlClint@users.noreply.github.com>
Agent-Logs-Url: https://github.com/GoodOlClint/PSProxmoxVE/sessions/2feecd30-2ba1-40bc-b1b5-c1fcc485a319
2026-03-25 16:42:29 +00:00
Clint Branham 10c277af10 fix: resolve F085 — PveClusterJoinInfo Nodelist/Totem D013 violations
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>
2026-03-25 11:31:05 -05:00
Clint Branham f311c738b4 docs: review scan-8 findings update
Scan-8 full re-scan of PSProxmoxVE module (2026-03-24).

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

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

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

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

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-24 18:15:46 -05:00
Clint Branham b4754c72a0 docs: update README, CHANGELOG, SECURITY.md, and review artifacts
- README: move Set-PveStorage to Storage section, Set-PveApiToken to
  Access section (were miscategorized under "Access — Groups & Domains")
- CHANGELOG: add Changed section with 13 architectural improvements,
  expand Fixed section with all ConfirmImpact and credential fixes
- SECURITY.md: add SecureString detail, Input Validation section,
  Dependabot mention, SkipCertificateCheck warning note
- CLAUDE.md: update review system reference to scan-7
- findings.json: F050 resolved (URL encoding), F031 resolved (credentials)
- REVIEW_REPORT.md: scan-7 report

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-23 17:41:19 -05:00
Clint Branham a424dd18ac refactor(ci): parallel provisioning, file caching, provision/test/cleanup split
Restructures the integration test workflow from a monolithic sequential
job into separate provision → test → cleanup stages:

- provision: creates ALL nested PVE VMs in a single terraform apply
  (parallel), waits for APIs, creates tokens, passes outputs to tests
- test: matrix [pve9, pve8] consumes provision outputs, no provisioning
- cleanup: always runs, API-only teardown for all VMs

Terraform refactored to for_each with pve_instances map variable,
enabling parallel ISO upload and VM creation.

New caching scripts reduce redundant downloads:
- ensure-base-iso.sh: downloads PVE ISOs to /opt/pve-isos if missing
- ensure-cloud-images.sh: caches cloud image + OVA with 7-day TTL
- prepare-auto-iso.sh: --cache-dir flag with hash-based skip

Runner no longer needs manual ISO provisioning (zero-touch setup).
cleanup-images bumped to min-versions-to-keep: 3 to survive overlapping
runs. All jobs gated against dependabot.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-23 16:05:20 -05:00
Clint Branham 89bead7a0e fix: add -Confirm:$false to Restart-PveContainer integration test (F080, F081)
Restart-PveContainer gained ConfirmImpact.High in the F062 fix, which
correctly prompts for confirmation on destructive operations. The
integration test was missing -Confirm:$false, causing a
NullReferenceException in non-interactive CI. The cascading failure also
broke the Copy-PveContainer test (F081).

Also updates CONTRIBUTING.md to reference .NET SDK 10.0+ (was 9.0+) to
match all CI workflows and test project TFM (F083).

Includes scan-6 review report and findings database update (F001-F083).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-23 14:48:11 -05:00
Clint Branham 68dadbfdc0 fix: remediate findings F045, F047, F048, F064, F070, F071, F076-F079
Phase 1 — Trivial fixes:
- F071: Add Uri.EscapeDataString to GetPveTemplateCmdlet node path
- F077: Add ValidateRange(100, 999999999) to GetPveTaskListCmdlet.VmId
- F076: Create .github/dependabot.yml (nuget + github-actions, weekly)
- F079: Fix unit-tests.yml dotnet SDK from 9.0.x to 10.0.x
- F048: Mark wont_fix — sync-over-async accepted for PS 5.1 compat

Phase 2 — Framework targeting (D009 compliance):
- F047: Reduce publishable csproj to netstandard2.0 only, remove all
  #if NET48/NETSTANDARD2_0 conditionals from PveHttpClient.cs,
  restructure build.yml for netstandard2.0 publish + net10.0/net48 tests
- F064: Resolved by F047 — SMA 7.5.0 ItemGroup removed with net10.0 TFM
- F070: Add PS 5.1 smoke-test job to publish.yml (windows-latest)

Phase 3 — IPveHttpClient interface extraction (F045):
- Extract IPveHttpClient interface from PveHttpClient
- Add constructor injection to all 14 service classes
- Services use injected client when available, create+dispose when not

Phase 4 — Service unit tests (F078):
- 196 new xUnit tests across 10 service test files
- All services tested via Moq-mocked IPveHttpClient
- Total test count: 382 (was 186)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-23 13:51:37 -05:00
Clint Branham 0fa92d689f docs(review): scan-4 review report and findings update
Add scan-4 full review: 1 resolved (F040), 6 new findings (F071-F076).
27 open findings, 49 resolved. No regressions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-23 09:07:51 -05:00