skills: a gate that could pass without checking, and a mailbox that never empties
Fixes the three blockers and seven should-fixes from pi@emb-7kj4vr4g's review (logstream correlation skills-provenance-review, full text in drawer_pi-devbox_reviews_e43e766641c9ec85217bc6ce). Every finding was reproduced by execution here before being fixed; two were refined by that reproduction rather than taken as given. BLOCKER 1 — the provenance gate could print OK and exit 0 without verifying. `git show <ref>:<path> | sha256sum` hashes EMPTY STDIN when the ref does not resolve, so at_ref was never empty and the UNKNOWN branch was dead code. Measured: a bogus ref reported MISMATCH — accusing the snapshot of lying when the real cause was an incomplete clone, and the operator's natural remedy for MISMATCH is to re-run the refresh, which rewrites provenance to silence the complaint; and with a 0-byte snapshot against a 0-byte upstream file it printed "OK: exactly skillset@aaaaaaa" with exit 0 for a ref that does not exist. The script already had the sha_empty idiom and had applied it to blob_sha but not to at_ref. Existence is now PROVEN with git cat-file -e before anything is hashed, at two levels (ref resolves / path exists at it) because those deserve different messages. Same defect class as the canary it replaces: a check that can succeed without checking. A second, unflagged instance of the same pipeline shape in blob_sha was found and fixed too. Exit codes split, because the old contract failed the sanctioned case: 0 truthful (including stale, with a NOTICE), 1 a lying record only, 2 cannot determine. AGENTS.md step 2 promised "the message distinguishes the two" and was the thing this branch was breaking; rewritten to state all three. BLOCKER 3 — VENDORED.md contradicted itself in the release whose stated invariant is non-contradiction: its hand-maintained provenance line named skillset 670f7f1, seven commits behind the ARG and itself the commit that told agents to hand-stamp added_by — the withdrawn instruction this work exists to stop shipping — while its cp recipe contradicted the "not cp" rule 20 lines above. Line removed (nothing forced it to move when the ARGs did); 670f7f1 kept only as a labelled cautionary example. The pi-extensions half was verified redundant (CI require_sha resolves PI_EXTENSIONS_REF) before removal. SHOULD-FIXES: `<root> --check`, the spelling VENDORED.md documented, silently ran a REFRESH because only $1 was parsed (both tools now parse all args and reject unknown ones); refresh at a detached/older HEAD silently rewound ref and bytes (now refused unless the recorded ref is an ancestor, --force to override); upstream_dirty was computed and never used in check mode; --no-skills --json printed human text and broke jq; --help was a hardcoded sed range this branch had already made stale; the fingerprint hashed SKILL.md alone so a live skill differing only in a sibling file reported "identical", and pi-extensions already ships two files, so it is now a per-skill TREE hash with the manifest field renamed skillset_snapshot_tree_sha256; the --no-skills smoke assertion was negative-only and passed on a crashed binary. mktemp+mv left files 0600 — CI was unaffected since the index records 100644, so the blast radius was local builds only, narrower than the review inferred. Snapshot resynced c04cd15 -> 5fd0d5c so the no-clone fallback carries the CORRECTED coordination protocol rather than the withdrawn one; --check is now OK with no staleness notice, and the bidirectional canary re-verified against the new bytes. Local validation is bash -n only (shellcheck, hadolint and actionlint are all absent in this container) — CI remains the shellcheck gate.
This commit is contained in:
@@ -16,6 +16,81 @@ Pre-v1.0.0 tags followed the pi npm version (`v{pi_version}[letter]`).
|
||||
The vendored `mempalace` skill snapshot stops being anonymous, and the
|
||||
container starts saying which copy of each skill it is actually reading.
|
||||
|
||||
**Peer review (pi@emb-7kj4vr4g, logstream correlation
|
||||
`skills-provenance-review`, full text in
|
||||
`drawer_pi-devbox_reviews_e43e766641c9ec85217bc6ce`) found three blockers before
|
||||
this was tagged. All three were the same species: a record asserting something
|
||||
it had not verified. Every finding below was reproduced by execution here before
|
||||
being fixed.**
|
||||
|
||||
- **The verification gate could print `OK` and exit 0 without verifying
|
||||
anything.** `git show <ref>:<path> | sha256sum` hashes *empty stdin* when the
|
||||
ref does not resolve, yielding a real-looking `sha256("")` rather than an
|
||||
empty string — so the `UNKNOWN` branch in `--check` was dead code. Reproduced:
|
||||
a bogus ref reported `MISMATCH` (accusing the snapshot of lying when the true
|
||||
cause was an incomplete clone — and the operator's natural remedy for
|
||||
MISMATCH is to re-run the refresh, which *rewrites provenance to silence the
|
||||
complaint*); with a 0-byte snapshot against a 0-byte upstream file it printed
|
||||
`OK: … exactly skillset@aaaaaaa` and exited 0 for a ref that does not exist.
|
||||
The script already had the right idiom (`sha_empty`) and had applied it to
|
||||
`blob_sha` but not to `at_ref`. Now existence is *proven* with `git cat-file
|
||||
-e` before anything is hashed, at two levels (does the ref resolve; does the
|
||||
path exist at it) because those are different failures. This was the same
|
||||
defect class as the canary it replaces: a check that can succeed without
|
||||
checking. A second, unflagged instance of the identical pipeline shape was
|
||||
found in `blob_sha` and fixed too.
|
||||
- **`--check`'s exit codes conflated "stale" with "lying",** so the release step
|
||||
failed in the case `AGENTS.md` step 2 explicitly calls legitimate. Now: `0`
|
||||
truthful (including stale-but-truthful, with a `NOTICE`), `1` a lying record
|
||||
only, `2` cannot determine (ref absent from this clone). `AGENTS.md` step 2
|
||||
rewritten to state all three, since its promise that "the message
|
||||
distinguishes the two" was exactly what the branch was breaking.
|
||||
- **`VENDORED.md` contradicted itself, in the release whose stated invariant is
|
||||
non-contradiction.** Its hand-maintained "Snapshot provenance at last refresh"
|
||||
line named skillset `670f7f1` — seven commits behind the ARG, and *the very
|
||||
commit that told agents to hand-stamp `added_by`*, i.e. the withdrawn
|
||||
instruction this line of work exists to stop shipping — while its `cp` recipe
|
||||
still contradicted the "not `cp`" rule 20 lines above. The hand-maintained
|
||||
line is gone (nothing forced it to move when the ARGs did); `670f7f1` is kept
|
||||
only as a labelled cautionary example. The `pi-extensions` half was verified
|
||||
redundant (CI resolves `PI_EXTENSIONS_REF` via `require_sha`) before removal,
|
||||
rather than silently dropped.
|
||||
|
||||
**Should-fixes from the same review, all reproduced:** `--check` given the
|
||||
documented positional spelling (`<root> --check`) silently ran a *refresh*,
|
||||
because only `$1` was parsed — both tools now parse all arguments and reject
|
||||
unknown ones; a refresh at a detached or older `HEAD` silently rewound ref and
|
||||
bytes, now refused unless the recorded ref is an ancestor (`--force` to
|
||||
override); `upstream_dirty` was computed and never used in check mode, now
|
||||
reported; `pi-devbox-version --no-skills --json` printed human text and broke
|
||||
`jq`; `--help` was a hardcoded `sed -n '2,22p'` range that this branch had
|
||||
already made stale; the skill fingerprint hashed `SKILL.md` alone, so a live
|
||||
skill dir differing only in a sibling file still reported "identical" — and
|
||||
`pi-extensions` already ships two files — so it is now a per-skill **tree** hash
|
||||
and the manifest field is renamed `skillset_snapshot_tree_sha256` to say what it
|
||||
measures; and the `--no-skills` smoke assertion was negative-only, passing on a
|
||||
crashed binary, now anchored positively. `mktemp`+`mv` left written files at
|
||||
`0600` (a `mv` takes the temp file's mode) — CI was unaffected because the git
|
||||
index records `100644`, but a local build from a dirty tree would have baked it;
|
||||
now `chmod 0644` before the `mv`.
|
||||
|
||||
**The skill fix ships outside this release, because it had to.** The review also
|
||||
found that skillset `82a8d3c` — the coordination protocol itself — told every
|
||||
machine on this fleet to *skip* the mailbox it introduced: it gated the mailbox
|
||||
on `mempalace_mesh_peers`, and a hub-and-spoke palace reports `peers: []`
|
||||
precisely because every machine is a thin client of one replica. It also
|
||||
asserted that a directed `open` event "stays in their mailbox until" acked —
|
||||
false, because `event_ack` appends and `status` is written once, so an answered
|
||||
ask matches forever. The headline measurement behind that claim ("exactly 1 —
|
||||
the one that needed a reply") was of an event already acked half an hour
|
||||
earlier. Fixed in skillset `5fd0d5c`, which derives owed-ness by joining on
|
||||
`ack_of`/`correlation_id` with a **`seq` ordering test** — without which one
|
||||
terminal reply suppresses every later ask on the same thread forever. Because
|
||||
the skillset is mounted live on every enrolled host, that correction was already
|
||||
deployed fleet-wide before this image was built; the vendored snapshot is
|
||||
resynced to it (`c04cd15` → `5fd0d5c`) so the no-clone fallback does not ship
|
||||
the withdrawn rule. Canary re-verified bidirectionally against the new bytes.
|
||||
|
||||
**Also carried, previously undocumented:** `dbb7879` resynced the vendored
|
||||
`mempalace` snapshot to skillset `c04cd15` ("the withdrawal only holds where the
|
||||
bridge is live"), landed after the v1.8.7 tag and so absent from that image.
|
||||
|
||||
Reference in New Issue
Block a user