Merge pull request #109 from GoodOlClint/docs/lane2-decisions

docs: record D017 and D018 for the two-lane CI split

DECISIONS.md

@@ -590,3 +590,127 @@ var task = vmService.RebootVm(session, node, vmid, timeout);
 if (Wait.IsPresent)
     task = WaitForStatusTransition(session, node, task, vmid, "running", timeout);
 ```
+
+---
+
+## D017 — CI runs two lanes: a pinned gating lane and a report-only currency lane
+
+**Status**: Active
+**Finding refs**: (none — arose from integration runs 176–180, root-caused 2026-09-01)
+**Resolved in scan**: n/a
+
+### Decision
+CI provisions nested PVE nodes in two distinct modes, and they must not be merged into one:
+
+- **Lane 1, `integration-tests.yml`** — nodes stay pinned to what the ISO ships. `first-boot.sh`
+  must never run `apt-get upgrade` or `dist-upgrade`. This lane gates merges.
+- **Lane 2, `package-currency.yml`** — nodes are `dist-upgrade`d to current PVE and the suite runs
+  against them. **Report-only**: test failures do not fail the job.
+
+Both declare `concurrency: group: integration-tests`. They drive the same nested VMIDs on the same
+parent node, so they must never run at once.
+
+### Rationale
+`apt-get upgrade` holds back packages that need new dependencies. On the nested nodes that produced
+`pve-cluster` 9.1.6 against `libpve-cluster-api-perl` 9.1.0 — a combination no real install ever
+has — and its symptom was not a package error. The node's pmxcfs came back in local mode after a
+cluster join, `/etc/pve/corosync.conf` never appeared, and the node reported `online=0` while
+corosync itself had healthy 2-node membership. Three CI runs went into diagnosing that, and removing
+the upgrade was the entire fix: run 180 was the first fully green integration run.
+
+So the pin is what makes lane 1 a trustworthy merge gate. But a permanently pinned CI never
+exercises the module against a current PVE, and that gap is exactly where an upstream regression
+would hide. Lane 2 closes it without putting the instability back into the gating path.
+
+Report-only is deliberate. A scheduled job that goes red on an upstream change nobody has chosen to
+chase becomes noise, and a noisy cron gets ignored — which is the failure mode that makes a canary
+worthless. The signal is the rolling issue and the recorded package set, not the check colour.
+
+**Consequence accepted**: a module genuinely broken against current PVE shows a green weekly check
+plus an updated issue. Operator ruling 2026-09-01, to be revisited after a few releases.
+
+A failure of the lane's own machinery — provisioning, the upgrade, the reboot, an unreachable node —
+still fails the job. `run-integration.sh` returns 3 for a genuine test failure and 4 when it cannot
+reach or authenticate to a node; only 3 is suppressed. Suppressing both would let a botched reboot
+report success while the lane learned nothing.
+
+### Anti-pattern (do not reintroduce)
+```bash
+# NEVER in first-boot.sh — this is the mismatch that left a node unclustered
+apt-get update -qq
+apt-get -y upgrade
+```
+
+### Correct pattern
+```bash
+# first-boot.sh installs only what provisioning needs; the ISO is the pin
+apt-get update -qq
+apt-get install -y -qq --no-install-recommends qemu-guest-agent open-iscsi
+```
+```yaml
+# package-currency.yml opts in explicitly; lane 1 never sets this
+env:
+  PVE_DIST_UPGRADE: '1'
+```
+
+---
+
+## D018 — The currency lane reboots after dist-upgrade, and proves it rebooted
+
+**Status**: Active
+**Finding refs**: (none — found in pre-push review of PR #106, 2026-09-01)
+**Resolved in scan**: n/a
+
+### Decision
+After `dist-upgrade`, `prepare-test-environment.sh` reboots the node **unconditionally** and then
+**verifies the reboot happened** by comparing `/proc/sys/kernel/random/boot_id` before and after.
+An unchanged boot id is fatal. The reboot lives here, not in `first-boot.sh`.
+
+### Rationale
+A PVE `dist-upgrade` pulls `proxmox-kernel-*`. Without a reboot the node runs new userspace on the
+old kernel, so the lane records a package set it never actually ran and is blind to kernel
+regressions — it would report "current PVE" while testing something that never booted.
+
+The reboot cannot live in `first-boot.sh`: that runs `ordering = "fully-up"` while the parent is
+still polling, so `wait-for-pve.sh` can discover the IP, see the API, pass auth, and then have the
+node reboot out from under provisioning. That presents as an intermittent network fault.
+
+It is unconditional rather than gated on `/var/run/reboot-required`, because that file comes from
+`update-notifier-common`, which is not guaranteed present on a PVE node.
+
+Verification is the part that is easy to omit and was omitted in the first draft. `ssh … reboot`
+returns non-zero when the connection dies, so it needs `|| true` — which swallows *every* ssh
+failure, including the reboot never being issued. `wait-for-api.sh` then matches the
+**still-running pre-reboot** pveproxy on its first poll and returns `responsive after 0s`. The
+script exits 0 having proved nothing. A blind `sleep` before polling does not fix this; it is wrong
+in both directions and verifies nothing either way.
+
+Order matters: prove the boot id changed first (ssh returns before pveproxy does), then wait for the
+API, then wait for pmxcfs — `pvesm set` writes `/etc/pve/storage.cfg`, which needs `/etc/pve`
+mounted, and on a fresh boot that lags the API by seconds.
+
+### Anti-pattern (do not reintroduce)
+```bash
+# NEVER — `|| true` hides a reboot that never happened, and the poll then
+# matches the pre-reboot node and returns immediately
+${SSH_CMD} "systemctl reboot" || true
+sleep 30
+bash "${SCRIPT_DIR}/wait-for-api.sh" "${NESTED_IP}" 8006 600
+```
+
+### Correct pattern
+```bash
+boot_before="$(${SSH_CMD} "cat /proc/sys/kernel/random/boot_id")"
+${SSH_CMD} "systemctl reboot" || true
+boot_after=""
+for _ in $(seq 1 60); do
+    boot_after="$(${SSH_CMD} "cat /proc/sys/kernel/random/boot_id" 2>/dev/null || true)"
+    [[ -n "${boot_after}" && "${boot_after}" != "${boot_before}" ]] && break
+    sleep 5
+done
+if [[ -z "${boot_after}" || "${boot_after}" == "${boot_before}" ]]; then
+    echo "ERROR: ${NESTED_IP} did not reboot (boot_id unchanged)" >&2
+    exit 1
+fi
+bash "${SCRIPT_DIR}/wait-for-api.sh" "${NESTED_IP}" 8006 600
+```

docs/lane2-change-plan.md

@@ -27,7 +27,7 @@ From the cluster/CI workstream tracker, and not reopened here:
 |---|---|---|
 | 1 | Lane home | New `.github/workflows/package-currency.yml` |
 | 2 | Scope | Full Pester suite; test failures reported, **not** fatal |
-| 3 | Baseline store | Committed file, updated by an auto-merging PR, only when the package set actually changed |
+| 3 | Baseline store | **Superseded during build** — see 3.3. An unprotected `ci/package-baseline` data branch, not a committed file in a PR |
 
 ### 3.1 Why a separate workflow
 
@@ -39,22 +39,20 @@ Package drift that doesn't break anything is not interesting. The thing worth kn
 
 **Consequence to accept:** a genuinely broken module against current PVE is a green check with an updated issue. The issue is the signal, not the check colour.
 
-### 3.3 Baseline: what works and what doesn't
+### 3.3 Baseline: superseded during implementation
 
-The operator asked whether the baseline could go through a PR that auto-merges and skips CI, committing only when versions differ. Three parts, and they don't all hold:
+The original choice was a committed baseline file updated by an auto-merging PR. **That cannot work on this repo**, and the reason only surfaced in review:
 
-**Commit only on change — yes.** The lane computes the set, diffs against the committed baseline, and does nothing when identical. On a pinned no-subscription repo most ticks are no-ops.
+- A PR opened with `GITHUB_TOKEN` never triggers `pull_request` workflows — GitHub suppresses them to prevent recursion. `build.yml`, `unit-tests.yml` and `claude-code-review.yml` are all `pull_request`-triggered, so the baseline PR would report **zero required checks** and be permanently unmergeable under branch protection. Not "waits for a human to merge it" — cannot be merged.
+- Auto-merge is enabled on the repo, but auto-merge waits for required checks that will never report, so it does not help.
+- `pve_api` solves this by pushing straight to `main` as `github-actions[bot]` with `contents: write`. That works because its `main` is unprotected. This repo's is (required checks, required review, admin enforced), so the same action is rejected.
+- Using a GitHub App installation token would fire the checks — but it puts output from a machine that just ran `dist-upgrade` against an upstream repo in front of the `claude-review` agent, which holds `pull-requests: write` and is instructed to end with `--approve`. That is a known [required-reviews bypass](https://medium.com/cider-sec/bypassing-required-reviews-using-github-actions-6e1b29135cc7) shape, and not worth opening for a generated data file.
 
-**Auto-merge — yes.** `enablePullRequestAutoMerge` merges once required checks pass and required reviews are satisfied. PR #100 established that `claude[bot]`'s APPROVED alone takes `mergeable_state` to `clean` on this repo, with the operator's code-owner review still pending. So the chain closes with no human: App opens the baseline PR → `claude-review` approves → build + unit tests go green → auto-merge fires. "Allow auto-merge" is already enabled on this repo (operator confirmed 2026-09-01).
+**Adopted instead: an unprotected data branch**, `ci/package-baseline`, holding exactly one file. This is the established pattern for generated data against a protected main — [github-archive-action](https://github.com/githubocto/github-archive-action) writes to an orphan branch for the same reasons. Branch protection covers `main` only, so `contents: write` plus `GITHUB_TOKEN` is sufficient: no PR, no checks, no protection conflict, and no path from node output to the review agent.
 
-**Skip CI — no, and it's worth being precise about why.** Both mechanisms deadlock against branch protection:
+It is written with git plumbing (`hash-object` → `mktree` → `commit-tree` → `push <sha>:refs/heads/…`) so the job's checkout is never touched and no local branch is created, which also makes a re-run unable to collide with itself.
 
-- `paths-ignore` on `build.yml` / `unit-tests.yml` → the workflow never runs → the required check never reports → the PR is blocked forever, and auto-merge waits forever.
+Artifacts alone were considered — the operator's own suggestion — and rejected on the stated goal: artifacts expire (90 days by default), so a weekly cadence would retain roughly 13 data points and lose the history. `git log ci/package-baseline` keeps it indefinitely.
-- `[skip ci]` in the commit message → GitHub skips the whole run → identical deadlock.
-
-The only way to skip the *work* while still satisfying protection is to keep the trigger and have each job short-circuit to an immediate `exit 0` on a baseline-only diff, so the check still reports success. That means editing `build.yml` and `unit-tests.yml` — two files with no other stake in this lane — to special-case it.
-
-**Recommendation: let CI run.** Both workflows trigger on every PR to `main` with no path filters today. On PR #100 the full set settled in roughly two minutes, and a baseline PR only exists on ticks where versions actually moved. Paying two minutes a handful of times a year is cheaper than a permanent special case in the two workflows that gate every merge. If the baseline turns out to churn weekly, add the short-circuit then.
 
 ## 4. Change plan
 
@@ -77,13 +75,18 @@ Add an opt-in third argument (default off), so lane 1's behaviour is byte-identi
 
 Schedule + `workflow_dispatch`; `concurrency: group: integration-tests`; provisions with `PVE_DIST_UPGRADE=1`; runs the full suite with `continue-on-error` on the test step; uploads the package set as an artifact.
 
-### Commit 3 — reporting
+### Commit 3 — reporting *(landed as #108, revised)*
 
-Diff the recorded set against `tests/infrastructure/pve-package-baseline.txt`. On difference: update the rolling issue (find by label, create if absent) with the diff and the suite result, and open the baseline-bump PR with auto-merge enabled. On no difference: exit quietly.
+Diff the reference node against the baseline on `ci/package-baseline`. On difference: update the data branch, then upsert the rolling issue (`pve-currency`). On no difference: exit quietly.
+
+Two additions beyond the plan:
+
+- **Node-vs-node comparison.** A package mismatch *between* the two nested nodes is the failure that left a node unclustered and cost three CI runs to diagnose (see D017). It is reported even when the set is otherwise unchanged.
+- **Input validation.** The package files come from a machine that just installed from an upstream repo and their contents reach a GitHub issue body, so anything that is not a dpkg name/version pair fails the run.
 
 ### Commit 4 — `DECISIONS.md`
 
-D017 (two-lane CI: pinned gating lane + report-only currency lane, and why `first-boot.sh` must never upgrade) and D018 (currency lane reboots unconditionally after `dist-upgrade`).
+D017 (two-lane CI: pinned gating lane + report-only currency lane, and why `first-boot.sh` must never upgrade) and D018 (the currency lane reboots after `dist-upgrade` **and proves it rebooted** — the verification was missing from the first draft and is the part easiest to omit).
 
 ## 5. Convention conflict, surfaced