* 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>
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>
* 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.
Pester was installed with -MinimumVersion 5.0 and no ceiling in the CI job
image, both install steps in unit-tests.yml, and both Import-Module calls, plus
the suite's own import inside the container. The image is rebuilt on every CI
run and Pester is installed fresh on every unit-test run, so PSGallery chose the
version — a new major could reach the required PR checks with no commit here,
surfacing as unexplained test breakage on whichever PR ran next.
It had already happened. Steps named "Install Pester 5" were resolving 6.1.0 on
both legs, because Pester 6 declares PowerShellVersion 5.1 and so installs on
Windows PowerShell too. Nothing broke — the suite uses only constructs common to
5 and 6, and runs 1566/0 under 6.1.0 with no deprecation warnings — but nobody
chose it. The step names are corrected; they had been describing an install that
stopped happening some time ago.
Pinning the install alone is not enough, in two ways review found:
An unset variable does not fail. -RequiredVersion accepts an empty value and
degrades to "latest" for Install-Module and to "any" for Import-Module, both
exiting 0, so a renamed or dropped env key would silently restore the float this
commit removes. A guard step now fails the job instead.
The point of use was still floored. run-integration.sh imported the suite's
Pester with -MinimumVersion 5.0, so a second Pester reaching PSModulePath would
win regardless of what was installed. The Dockerfile now promotes the ARG to ENV
so the version is discoverable at runtime, and that import is pinned to it.
The pin lives in two files, so shell-selfchecks asserts they agree — split-brain
between the workflow and the image is precisely the unexplained breakage this is
meant to prevent. CONTRIBUTING.md and CLAUDE.md are updated too; the contributor
instructions were a third floating install site.
Recorded as an amendment to D017 — the same principle as the nested PVE package
pin, applied to the lane's own tooling.
Two filed issues in one rewrite of preflight-cleanup.sh's ISO block, because
they are the same twenty lines.
#111 — ISO_FILENAME was interpolated into python3 -c PROGRAM TEXT inside a
single-quoted literal, so a quote in the value escaped it and executed
arbitrary Python in a container holding PVE_API_TOKEN, PVE_PASSWORD, the
Terraform state and the storage VM's SSH key. It now arrives through the
environment and is read with os.environ. The volid is passed to urllib's
quote() via argv for the same reason, and an empty encode result now skips
the volume instead of issuing a DELETE against the bare collection URL.
#105 — generated ISOs embed a hash of first-boot.sh, so every change to that
script mints a new filename. Deleting only the exact current name orphaned
each earlier ISO on the storage permanently, because force-cleanup wipes the
Terraform state that could otherwise reclaim it. The family is now swept by
rebuilding the full generated shape: the captured prefix plus twelve hex
characters plus .iso. A prefix test alone would also have matched a longer
FQDN's family and any hand-uploaded "-manual-backup.iso" sibling, which in a
script whose job is deletion is worse than the leak it fixes.
Multi-delete applies only to that family. A name that is not generated — the
storage VM's cloud image — keeps the original one-shot behaviour, since a
basename can repeat across content namespaces and a plain name carries nothing
that identifies a family.
Adds preflight-cleanup.test.sh, wired into shell-selfchecks. The script had no
coverage at all. It stubs curl and sleep, then asserts on the DELETEs issued:
the family goes, the pinned base ISO and unrelated uploads stay, a non-hash
sibling stays, the cloud image takes only itself, a quoted payload is data
rather than code, and unset storage skips only the ISO branch. Every case also
asserts the script ran to completion and removed the Terraform state, so a path
that dies early cannot pass by having issued the right DELETEs first.
The local dev environment had drifted badly from CI. Remove the parts that no
longer describe anything real, and make the rest match how CI actually runs.
Delete tests/dev.ps1. It wrapped run-integration.sh, which CI calls directly,
and duplicated the module build that script already performs internally. As a
second entry point it drifted: it still offered the PVE 8 leg retired in #88,
mounted the Docker socket for storage containers replaced by the storage VM in
#87, and pointed its remote-host examples at a runner decommissioned in the ARC
migration. All four documents describing it used a positional syntax that bound
the bare word to -Tests and then fell through to -Shell, so every documented
command silently opened a container shell. Recorded as D019.
Delete tests/infrastructure/runner/, a self-hosted-runner-in-Docker superseded
by Actions Runner Controller.
Make disk_storage and iso_storage required. Their defaults named a NAS that the
lab replaced with Ceph, and CI overrides both from repository variables, so the
defaults only ever misled local runs. require_env now fails at the top of a run
rather than at terraform apply, and the descriptions point at tests/.env.test
because cmd_provision deletes terraform.tfvars before applying.
preflight-cleanup.sh no longer falls back to the literal "local" storage. An
unset TF_VAR_iso_storage now skips only the ISO branch, leaving VM destroy and
state cleanup intact, and emits a workflow annotation: force-cleanup is the
only cleanup CI runs and it wipes Terraform state, so a silent skip strands the
uploaded ISO with nothing left to reclaim it.
Drop docker-ce-cli and the /var/run/docker.sock mount. Nothing in the container
has called docker since #87 moved storage into a VM; the remaining docker calls
run inside that VM over SSH. The CI job image is built from the same target, so
this also removes a third-party apt repository from its supply chain.
Rewrite tests/.env.test.example against what the code now requires, and fix the
documented commands in CLAUDE.md, README.md, copilot-instructions.md and the
integration README.
The nested nodes were pinned to proxmox-ve_9.1-1.iso while production runs
9.2, so the suite was validating a PVE nobody uses. run-integration.sh already
reads PVE9_ISO with the 9.1 name only as a fallback, so this needs no script
change; setting it at the workflow level keeps provision and cleanup agreeing
on the ISO name, which force-cleanup needs to find the right file to delete.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The diagnostic step runs before cleanup so it reaches the nodes while
they still exist. keep_vms skips cleanup entirely for a dispatch run,
leaving the nested nodes up for hands-on inspection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The repo stops holding opinions about CI VLAN addressing: the VM boots
via DHCP with hostname pvetest-storage (deterministic MAC for an
optional reservation), registers in the operator's CI DNS zone, and
everything addresses it by STORAGE_VM_FQDN. Replaces the static-IP +
explicit-DNS variables.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PVE 8 reached EOL 2026-08-31. The full matrix needs 42GiB free on one
parent node; no node has that after the fleet rebuild, so the headroom
guard blocks every run. Dropping the 8a/8b pair brings the requirement
to 26GiB. The module keeps PVE 8 runtime support and the script still
accepts PVE_VERSIONS='9 8' for local runs; the PVE-8-only Pester
contexts stay for SKIP_PROVISION runs against real PVE 8 hosts. Also
counts the storage VM in the headroom error message.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The nested PVE guests live on an isolated VLAN with no route to the
runner network (ADR 0032, homelab repo), and the runner-hosted Docker
services died with each ephemeral ARC pod anyway. A small cloud-image VM
in the ci pool now serves NFS, iSCSI, and the auto-install answer files
from inside the sandbox. Includes fixes from Codex and in-house review:
bpg provider >= 0.79.0, serial console for the resized cloud image,
key-based SSH (cloud images refuse password auth), CIDR validation,
storage VM in the headroom check, docker socket mounts removed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Previous approach (/code-review:code-review --comment) was based on
incorrect documentation research. Reviewing the actual official
examples at anthropics/claude-code-action/examples/pr-review-*.yml
reveals the correct pattern:
1. Use a custom prompt with explicit review instructions (not a
plugin slash command)
2. Use claude_args with --allowedTools to enable the MCP inline
comment tool and gh pr CLI commands — this is what lets Claude
actually post to the PR
3. Enable track_progress: true for visual progress tracking
Without --allowedTools, Claude has no way to post anything because
the tools for PR commenting aren't allowed by default.
Also removed the plugins and plugin_marketplaces inputs since
they're not needed — the review runs via prompt instructions and
the allowed tools alone.
The custom prompt is tailored to PSProxmoxVE with focus areas
specific to the module: DECISIONS.md compliance, cmdlet
conventions, API correctness against the PVE OpenAPI spec,
test coverage, and security.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dependabot PRs were being rejected with:
Workflow initiated by non-human actor: dependabot (type: Bot)
Add allowed_bots: 'dependabot[bot]' to permit dependency update reviews.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Publish-Module -Path requires the directory name to match the module
name. Changed artifact download path from ./publish/netstandard2.0/
to ./publish/PSProxmoxVE/ so Publish-Module can find the manifest.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CI cleanup should always succeed regardless of Terraform state
(cancelled runs, timeouts, partial failures). force-cleanup uses
direct API calls + docker rm instead of terraform destroy.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Terraform state was stored in the container's working directory
(fresh checkout), so it was lost between CI jobs. The cleanup job
couldn't destroy resources because it had no state.
Now stores state at /opt/pve-integration/work/terraform.tfstate via
-state flag on all terraform commands. This persists across the
provision → test → cleanup job chain in GitHub Actions.
Also:
- Force cleanup now removes state from both local and shared paths
- Added -reconfigure to terraform init (avoids backend mismatch errors)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Prevents overlapping integration test runs on the self-hosted runner.
Uses cancel-in-progress: false so each run completes its full
lifecycle (including cleanup) before the next one starts.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix storage_ip derivation to use default route first, not Swarm RemoteManagers
- Make iSCSI container entrypoint idempotent (check before create for target/LUN/bind)
- Pin erichough/nfs-server from 'latest' to '2.2.1' for reproducibility
- Replace curl|sh Docker install with apt repo in Dockerfile.test
- Add docker.sock security warnings in docker-compose.test.yml and CI workflow
- Update stale 'storage VM' references in comments and .env.test.example"
Co-authored-by: GoodOlClint <151449+GoodOlClint@users.noreply.github.com>
Agent-Logs-Url: https://github.com/GoodOlClint/PSProxmoxVE/sessions/534c0a63-2988-496c-b0ae-e291fcfa050b
Provision two PVE nodes per version (a/b) for future cluster testing,
plus Docker-based iSCSI target and NFS server for shared storage tests.
Multi-node changes:
- Each PVE version gets two nodes: 9a/9b and 8a/8b (4 VMs total)
- Parameterized answer.toml FQDN for unique hostnames per node
- Per-node auto-install ISOs with unique answer files
- Node name discovered from FQDN and included in test config
- API token creation handles pre-existing tokens (delete + recreate)
- New test env vars: PVETEST_HOST_B, PVETEST_APITOKEN_B
- Removed preflight cleanup from provision (use explicit cleanup instead)
Docker storage services:
- New docker-compose.storage.yml with iSCSI (tgt) and NFS containers
- Host networking so PVE nodes can reach storage services
- Docker socket mounted into dev-infra container for host Docker access
- Docker CLI added to dev-infra Dockerfile stage
- New test env vars: PVETEST_STORAGE_VM_IP, PVETEST_ISCSI_IQN, PVETEST_NFS_EXPORT
Other fixes:
- first-boot.sh installs open-iscsi on PVE nodes
- preflight-cleanup.sh handles empty ISO filename gracefully
- TMPDIR set to work dir to avoid /tmp overflow during ISO uploads
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace tests/infrastructure/Dockerfile with tests/Dockerfile.test
(single multi-stage Dockerfile for both CI and local dev)
- CI container-image job now builds from Dockerfile.test target dev-infra
- Add ARM support: PowerShell installed via dotnet tool on arm64,
APT package on amd64
- Replace tests/dev.sh (bash) with tests/dev.ps1 (PowerShell) for
cross-platform support (Windows, macOS, Linux)
- Add -DockerHost parameter for running x86 containers on a remote
Docker host from ARM Macs (rsyncs repo, uses SSH Docker transport)
- Add -NoCleanup switch to keep nested PVE VMs after integration tests
- integration command now provisions nested PVE VMs instead of testing
against a pre-existing PVE directly
- Share /opt/pve-isos host path between CI and local dev (was separate
Docker named volume)
- Delete tools/Invoke-Tests.ps1 (unused, overlapped with run-integration.sh)
- Add .gitignore entries for Terraform state/artifacts
- Update all documentation references
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The node name "pve" was being masked in all CI logs because GitHub
Actions auto-masks secret values. Since the node name is not sensitive,
use vars.PVE_TARGET_NODE instead of secrets.PVE_TARGET_NODE.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Provision, test, and cleanup jobs now call run-integration.sh instead
of inlining ~170 lines of shell. Logic changes only need to be made
in one place, shared between CI and local dev.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- CI workflow: PVE_PASSWORD now reads from secrets.PVE_TEST_PASSWORD
- variables.tf: removed default password, requires TF_VAR env var
- Integration README: examples use <your-test-password> placeholder
- create-api-token.sh: example IP changed to pve.example.com
- .gitignore: added .env/.env.* exclusion
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Runner VM now has 2 runner instances registered, so both test matrix
entries can execute simultaneously against their respective pre-
provisioned nested PVE VMs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
goodolclint-claude[bot]