* 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.
PveNetwork already deserialised bridge_vlan_aware as BridgeVlanAware, so a
VLAN-aware bridge could be read back but never created or changed. Both write
paths now take a -BridgeVlanAware switch.
Clearing the flag does not use bridge_vlan_aware=0. PVE merges the supplied
keys onto the stored stanza and accepts that 0 without acting on it, so the
obvious form is a silent no-op: an integration run against PVE 9 issued it and
Get-PveNetwork still reported 1. The endpoint's delete list is what actually
removes the key. The API schema advertises a plain boolean and gives no hint of
this, which is why the behaviour is pinned by an integration test rather than
inferred.
Set-PveNetwork guards the switch on BoundParameters so an update that omits it
leaves the flag alone; the create path follows the existing -Autostart form.
Only bridge_vlan_aware is added. bridge_vids is an independent parameter that
PVE defaults to 2-4094, and the issue asks only for the flag.
Coverage: the integration suite pins create, disable, re-enable, and that an
unrelated Set leaves the flag alone -- that last one kills a mutant that drops
the BoundParameters guard, which every other test survives. A model test pins
the read path the assertions depend on. The Pester unit tests assert only
parameter metadata; the defect is server-side, so nothing offline can catch it.
Closes#92
Two non-blocking review observations.
A 45s retry is indistinguishable from a hang with nothing on the wire, so
GuestLockRetry.Execute takes an onRetry hook and InvokeGuestTask reports
each reissue through WriteVerbose.
The gap between attempts now scales with the budget, capped at the 2s
production value. A caller passing a short window wants a fast answer
rather than one long sleep, which also takes the retrying unit tests off
a real 2s sleep each: the xUnit run drops from 8s to 4s. PveHttpClient's
window becomes a field so those tests can shorten it too.
WaitForStatusTransition refused to return while snapshot.Locked, and its
comment quoted the exact error it was meant to prevent. Locked reads the
guest config's lock: property; the failure is the flock on
/var/lock/qemu-server/lock-<vmid>.conf, which PVE exposes nowhere.
The flock cannot be observed, so it is retried. GuestLockRetry reissues an
operation for a bounded 45s while PVE reports failing to enter lock_config
for a guest, which it raises before doing any work.
Two seams, because the failure has two surfaces. PveHttpClient.SendAsync
retries the request for operations PVE serialises in the API handler; it
takes a request factory because an HttpRequestMessage cannot be resent.
PveCmdletBase.InvokeGuestTask reissues the call and re-waits its task for
operations serialised in the forked worker, where the POST returns 200 and
only the task fails. WaitForStatusTransition routes through the latter,
hence Func<PveTask>.
The predicate is path-specific and anchored at the start of what PVE said:
lock_file uses identical wording for storage, LVM and HA locks, and a
qmclone that fails after allocating disks must not be reissued into
"VM already exists". That requires the raw text, so it reads
PveTaskFailedException.ExitStatus and PveApiException.ApiMessage.
The Locked check stays — it is correct for the config lock — with a comment
that says so.
Closes#113
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.
Expands the Rosetta note into a working recipe, after running the whole
provision -> test -> cleanup lifecycle on an Apple Silicon Mac against the real
parent cluster.
Compose is the wrong entry point on a Mac: its dev-infra service builds rather
than pulls, and bind-mounts /opt/pve-integration, which does not exist there.
The macOS path pulls the image CI already built and drives run-integration.sh
with docker run. Records that GHCR needs a classic PAT, since fine-grained
tokens cannot reach it at all and the failure gives no hint why.
Restores the x86 compose instructions, which the previous commit's rewrite
consumed, and hoists the fixed-VMID warning out of the macOS section — 5080,
5091 and 5092 are shared with CI on the same parent cluster whatever host you
run from, so a local run during a CI run collides, and a skipped force-cleanup
fails the next run's headroom guard.
Also warns that emulation runs the suite ~40% slower and so loses the
qemu-server flock race (#113) that CI wins: Reset-PveVm, clone and
Set-PveVmConfig fail locally on a tree that is green in CI. Provisioning and
cleanup are unaffected.
"x86 only" was too strong. The image is amd64-only — proxmox-auto-install-assistant
and the HashiCorp apt repo publish no arm64 — but it builds and runs on Apple Silicon
once Docker Desktop's Rosetta emulation is on.
Under the default qemu translation pwsh starts and reports its version, then segfaults
on module discovery (uncaught target signal 11), which fails the build at
Install-Module Pester and would fail Pester at test time. With Rosetta enabled the same
Dockerfile builds to within 150 bytes of the image CI pushed for this commit, and
Invoke-Pester runs.
Worth stating explicitly because the failure is silent: the build step exits 1 with no
diagnostic output, which reads as a Dockerfile defect rather than an emulation problem.
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.
goodolclint-claude[bot]