From f645e6654fa5afa6131f2324fe818117eb139353 Mon Sep 17 00:00:00 2001 From: pi Date: Wed, 26 Aug 2026 08:09:09 +0200 Subject: [PATCH] smoke: fix the snapshot canary that blocked v1.8.7, and make it bidirectional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 589 built the base cleanly and then failed both smoke jobs 81-passed/1-failed on 'mempalace skill snapshot is current'. That canary greps a phrase from the vendored mempalace skill to detect a stale snapshot, and the phrase it pinned was 'Attribute what you file yourself' — the heading of the hand-stamping instruction that THIS release withdraws. So it fired correctly: the snapshot changed and the expectation did not. Every publish job was skipped, so nothing reached the registry and v1.8.7 was never consumed. Rather than bump the string: * the assertion is now BIDIRECTIONAL — the new phrase must be present AND the withdrawn one absent. A one-way canary only catches half the drift: it cannot notice a re-vendored stale snapshot that happens to contain the pinned phrase. Verified against v1.8.6's snapshot, which now correctly fails. * the comment records the structural limit rather than just the fix: a phrase canary can only ever detect 'older than what I remembered to pin', never 'older than skillset main'. Only a diff against the skillset repo can do that, which is now a Still-open item — it needs a CI clone credential for a private repo, i.e. a policy decision, not a code change. Changelog consolidated: the SSH sidecar multiplexing default moves from Unreleased into v1.8.7, since the retag will sit on a commit that contains it, and the v1.8.7 summary now records the failed first attempt rather than quietly presenting the second one as the whole story. --- CHANGELOG.md | 151 +++++++++++++++++++++++++----------------- scripts/smoke-test.sh | 16 ++++- 2 files changed, 105 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b03798..b316933 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,67 +11,6 @@ Pre-v1.0.0 tags followed the pi npm version (`v{pi_version}[letter]`). --- -## Unreleased - -### Added - -- **The SSH sidecar now defaults to connection multiplexing, without overriding - anyone's explicit choice.** `~/.ssh-local/config` already forced `ControlPath` - into the writable sidecar dir, but nothing supplied `ControlMaster` for targets - coming from the user's own bind-mounted `~/.ssh/config`. An entry that never - mentioned it therefore opened a **fresh TCP connection per `ssh` call** — and - an agent doing a dozen calls in a few minutes is exactly the traffic shape that - trips fail2ban or a CGNAT flow-table cap. Observed 2026-08-25 on this fleet: - ~12 connections to one host in 15 minutes, after which port 22 stopped - answering while HTTPS to the same estate stayed healthy in 0.44 s (that - asymmetry is the tell for rate-limiting rather than an outage). - - **The fix is where the block sits, not what it says.** `ssh_config` is - first-value-wins, so position encodes intent, and the two settings need - opposite treatment: - - | Setting | Position | Meaning | Why | - |---|---|---|---| - | `ControlPath` | **before** `Include ~/.ssh/config` | override | the user's value points at read-only `~/.ssh`; it cannot work here, so it must lose | - | `ControlMaster auto` + `ControlPersist 10m` | **after** the `Include` | default | an explicit per-host `ControlMaster no` must keep winning; we only supply an opinion where the user expressed none | - - *Force what is broken, default what is merely absent.* The first draft of this - put both in the leading block, which would have silently overridden an explicit - `ControlMaster no` — the counterfactual is in the test below. - - Verified with `ssh -G` (the resolved-config oracle) rather than by reading the - man page, against a fixture with one host set to `no`, one silent, one set to - `auto`: the explicit `no` resolves to `controlmaster false` **and** still gets - the writable `ControlPath`, the silent host resolves to `auto`, and the same - fixture under the rejected layout flips the `no` host to `auto` — so the test - discriminates the *position*, not merely the presence of the block. Then - end-to-end: the real script rendered in a sandbox `HOME`, block last, `bash -n` - clean, `shellcheck -S error` clean (the gate added in v1.8.7). - - Measured effect on the author's own config (41 host aliases): **22 were silent - about `ControlMaster` and gain `auto` + 10 m persist; 0 are overridden**, since - the fleet contains no explicit `no`. Worth noting *how* the one deliberate - exception is written — `proxmox002-vpn` carries `# No ControlMaster — VPN means - direct route, no CGNAT flow cap`, i.e. the intent is expressed as **absence - plus a comment**, which `ssh` cannot distinguish from "no opinion". That host - does now get multiplexing; its comment says multiplexing is *unnecessary* - there, not harmful. Anything that must stay unmultiplexed needs a literal - `ControlMaster no`. - - Why the ordering matters beyond this one config: `~/.ssh/config` is - **per-machine**, differs across the fleet, and future machines' versions do not - exist yet to be audited. A default-not-override design is correct without - needing to inspect any of them. - - `ControlPersist` is deliberately short (10 m idle, and each new session resets - the idle timer — long enough to collapse an agent's burst, short enough that an - abandoned socket ages out). A per-host entry that sets its own value keeps it: - hosts already specifying `ControlPersist 4h` still resolve to 4 h. The known - cost of multiplexing is the **stale master** — socket present, daemon gone, - after a suspend or network change — which makes every later `ssh` to that host - hang; recovery is `ssh -F ~/.ssh-local/config -O exit `, now documented - in the `pi-devbox-environment` skill along with `-O check`. - ## v1.8.7 — 2026-08-25 Patch release, and the fastest turnaround in the series (~9 h after v1.8.6) for @@ -93,6 +32,16 @@ time as usual. **Base rebuild is forced twice over** — `Dockerfile.base` chang mempalace-toolkit SHA ("otherwise a toolkit-only fix never lands") — so expect ~67 min, and note that either cause alone would have sufficed. +⚠️ **The first tag of this version did not publish.** Run 589 built the base +fine, then **both** smoke jobs failed 81-passed/1-failed on a single assertion — +`mempalace skill snapshot is current`, a canary pinning a phrase from the +vendored skill. The phrase it pinned was the heading of the very instruction this +release *withdraws*, so refreshing the snapshot without re-pinning the canary +made it fire correctly on a healthy image. Every publish job was skipped, so +nothing reached the registry and the version was never consumed; the tag was +moved to include the fix below. Fixing the canary is what this release is +*for*, in miniature: the gate was right and the expectation was stale. + ### Added - **Palace writes now carry the device that made them, and diary entries say so @@ -191,8 +140,77 @@ mempalace-toolkit SHA ("otherwise a toolkit-only fix never lands") — so expect does not exist would repeat the shipped-false-claim mistake corrected below). Expect ~67 min, as for v1.8.5/v1.8.6. +- **The SSH sidecar now defaults to connection multiplexing, without overriding + anyone's explicit choice.** `~/.ssh-local/config` already forced `ControlPath` + into the writable sidecar dir, but nothing supplied `ControlMaster` for targets + coming from the user's own bind-mounted `~/.ssh/config`. An entry that never + mentioned it therefore opened a **fresh TCP connection per `ssh` call** — and + an agent doing a dozen calls in a few minutes is exactly the traffic shape that + trips fail2ban or a CGNAT flow-table cap. Observed 2026-08-25 on this fleet: + ~12 connections to one host in 15 minutes, after which port 22 stopped + answering while HTTPS to the same estate stayed healthy in 0.44 s (that + asymmetry is the tell for rate-limiting rather than an outage). + + **The fix is where the block sits, not what it says.** `ssh_config` is + first-value-wins, so position encodes intent, and the two settings need + opposite treatment: + + | Setting | Position | Meaning | Why | + |---|---|---|---| + | `ControlPath` | **before** `Include ~/.ssh/config` | override | the user's value points at read-only `~/.ssh`; it cannot work here, so it must lose | + | `ControlMaster auto` + `ControlPersist 10m` | **after** the `Include` | default | an explicit per-host `ControlMaster no` must keep winning; we only supply an opinion where the user expressed none | + + *Force what is broken, default what is merely absent.* The first draft of this + put both in the leading block, which would have silently overridden an explicit + `ControlMaster no` — the counterfactual is in the test below. + + Verified with `ssh -G` (the resolved-config oracle) rather than by reading the + man page, against a fixture with one host set to `no`, one silent, one set to + `auto`: the explicit `no` resolves to `controlmaster false` **and** still gets + the writable `ControlPath`, the silent host resolves to `auto`, and the same + fixture under the rejected layout flips the `no` host to `auto` — so the test + discriminates the *position*, not merely the presence of the block. Then + end-to-end: the real script rendered in a sandbox `HOME`, block last, `bash -n` + clean, `shellcheck -S error` clean (the gate added in v1.8.7). + + Measured effect on the author's own config (41 host aliases): **22 were silent + about `ControlMaster` and gain `auto` + 10 m persist; 0 are overridden**, since + the fleet contains no explicit `no`. Worth noting *how* the one deliberate + exception is written — `proxmox002-vpn` carries `# No ControlMaster — VPN means + direct route, no CGNAT flow cap`, i.e. the intent is expressed as **absence + plus a comment**, which `ssh` cannot distinguish from "no opinion". That host + does now get multiplexing; its comment says multiplexing is *unnecessary* + there, not harmful. Anything that must stay unmultiplexed needs a literal + `ControlMaster no`. + + Why the ordering matters beyond this one config: `~/.ssh/config` is + **per-machine**, differs across the fleet, and future machines' versions do not + exist yet to be audited. A default-not-override design is correct without + needing to inspect any of them. + + `ControlPersist` is deliberately short (10 m idle, and each new session resets + the idle timer — long enough to collapse an agent's burst, short enough that an + abandoned socket ages out). A per-host entry that sets its own value keeps it: + hosts already specifying `ControlPersist 4h` still resolve to 4 h. The known + cost of multiplexing is the **stale master** — socket present, daemon gone, + after a suspend or network change — which makes every later `ssh` to that host + hang; recovery is `ssh -F ~/.ssh-local/config -O exit `, now documented + in the `pi-devbox-environment` skill along with `-O check`. + ### Fixed +- **The vendored-snapshot canary was one-way, and pinned a phrase the same + release deleted.** `mempalace skill snapshot is current` grepped for + *"Attribute what you file yourself"* — the heading of the hand-stamping + instruction withdrawn above. It therefore did its job (snapshot changed, + expectation did not) and blocked an otherwise-green build. Two changes rather + than a string bump: the assertion is now **bidirectional** (the new phrase must + be present **and** the withdrawn one absent, so a re-vendored *stale* snapshot + fails as loudly as a forgotten bump — verified by running it against v1.8.6's + snapshot, which correctly fails), and the comment now states the structural + limit: a phrase canary can only detect *"older than what I remembered to pin"*, + never *"older than skillset main"*. + - **Four build-provenance smoke assertions verified the presence of a manifest field name and never looked at its value.** The originals were literally: @@ -262,6 +280,17 @@ mempalace-toolkit SHA ("otherwise a toolkit-only fix never lands") — so expect ### Still open +- **Make the vendored-snapshot check automatic instead of a remembered string.** + Tonight's failure is the third iteration of the same maintenance burden (v1.8.4: + phrase present in both copies; v1.8.7: phrase deleted by the release that + refreshed the snapshot). A phrase canary structurally cannot answer *"is this + snapshot older than skillset main?"* — only a diff can. Proposed: a lint job + that clones the skillset repo and compares + `rootfs/usr/local/share/pi-devbox/skills/mempalace/SKILL.md` against it, + failing with the diff when they drift. Open question first: the skillset repo is + **private**, so this needs a CI clone credential, which is a policy decision + rather than a code change. + - **Provenance stops at Chroma's metadata.** The hourly reconciler on the palace host stamps `device`/`agent_kind` in `chroma.sqlite3`, but knowledge-graph triples and coordination events live in *separate* SQLite files diff --git a/scripts/smoke-test.sh b/scripts/smoke-test.sh index e9bfe8c..315e0c2 100755 --- a/scripts/smoke-test.sh +++ b/scripts/smoke-test.sh @@ -527,7 +527,21 @@ exec_test "mempalace skill linked (fallback)" 'test -L $HOME/.agents/skills # multiple harnesses", a phrase present in BOTH the stale and the fresh copy. # A snapshot canary must pin the NEWEST section, so update this string whenever # the snapshot is refreshed — that is the point of it. -exec_test "mempalace skill snapshot is current" 'grep -q "Attribute what you file yourself" $HOME/.agents/skills/mempalace/SKILL.md && echo ok' +# +# v1.8.7: this fired for real, and on the release that changed the snapshot. The +# pinned phrase was "Attribute what you file yourself", the heading of the +# instruction telling agents to hand-stamp added_by — which that same release +# WITHDREW (RFC 001 §7.3.2 ranks agent-side stamping worst-possible; the bridge +# now does it). So the canary correctly reported "snapshot changed, expectation +# did not", and blocked publication of an otherwise-green build (81 passed, 1 +# failed, twice). Two lessons kept in the assertion itself: +# * it is now BIDIRECTIONAL — the new phrase must be present AND the withdrawn +# one absent, so a re-vendored stale snapshot fails just as loudly as a +# forgotten bump. A one-way canary only catches half the drift. +# * a phrase canary can only ever detect "older than what I remembered to pin", +# never "older than skillset main". The real fix is a CI job diffing this +# file against the skillset repo — see the Unreleased changelog note. +exec_test "mempalace skill snapshot is current" 'f=$HOME/.agents/skills/mempalace/SKILL.md; grep -q "Provenance is stamped for you" "$f" && ! grep -q "Attribute what you file yourself" "$f" && echo ok' # Link TARGETS, not just link existence: with no skillset mounted (as here) the # baked tree must be what resolves, for all three vendored skills. exec_test "vendored skills resolve to the baked tree (no skillset mounted)" \