diff --git a/AGENTS.md b/AGENTS.md index accc739..fa4ff24 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -66,18 +66,32 @@ re-brand of opencode-devbox's `pi-only` variant. the upstream changelog to include in `CHANGELOG.md`. 2. **Refresh the vendored mempalace skill snapshot if the skillset moved:** `scripts/vendor-mempalace-skill.sh --check` (reads a real skillset clone, - writes nothing). Exit 1 means either the recorded `SKILLSET_SNAPSHOT_REF` - does not describe the shipped bytes, or upstream has moved past it — the - message distinguishes the two. Refresh with - `scripts/vendor-mempalace-skill.sh`, which rewrites the file **and** the ARG - together so they cannot drift apart. - Two consequences to accept deliberately: the snapshot is hashed into - `base_tag`, so refreshing costs a base rebuild (~67 min); and if the section - the phrase canary names has changed, re-pin it in `scripts/smoke-test.sh`. - **Skipping this is legitimate** — every enrolled host reads its own live - skillset clone, so the baked copy is a no-mount fallback. What is *not* - legitimate is skipping it silently: the drift is visible in - `pi-devbox-version` and in the manifest, so decide rather than forget. + writes nothing). Three exit codes, not two — a stale-but-truthful record is + **not** a release blocker, so don't treat any non-zero exit as "must + refresh" without reading which one it was: + - **0** — the record is truthful. This includes stale-but-truthful + (upstream has moved past the recorded ref, or the local clone has + uncommitted changes) — a `NOTICE` is printed, but nothing is lying. + **Skipping the refresh in this case is the legitimate, sanctioned + outcome** — every enrolled host reads its own live skillset clone, so + the baked copy is only a no-mount fallback. What is not legitimate is + skipping it *silently*: the drift is visible here, in + `pi-devbox-version`, and in the manifest, so decide rather than forget. + - **1** — a confirmed problem: the vendored bytes provably do NOT match + the file at the recorded ref (a lying record), or the recorded ref + doesn't even resolve to that path in this clone. Refresh. + - **2** — cannot determine (the recorded ref itself isn't resolvable in + this clone — commonly a shallow checkout missing history). Fetch full + history and re-check before deciding; don't refresh blind. + Refresh with `scripts/vendor-mempalace-skill.sh`, which rewrites the file + **and** the ARG together so they cannot drift apart, and refuses (exit 1) + rather than silently rewinding provenance if the skillset clone's HEAD is + behind the already-recorded ref (detached HEAD, older checkout) — pass + `--force` only if that rewind is genuinely intended. + Two consequences to accept deliberately on an actual refresh: the snapshot + is hashed into `base_tag`, so it costs a base rebuild (~67 min); and if the + section the phrase canary names has changed, re-pin it in + `scripts/smoke-test.sh`. 3. Update `CHANGELOG.md` Unreleased → vX.Y.Z section. 4. Verify `docker compose up` works locally with the current `latest` image if you're upgrading users from a previous version. Then run the diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d9cf08..0a5f4e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 : | 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 (` --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. diff --git a/Dockerfile.variant b/Dockerfile.variant index 88f495e..2e1232a 100644 --- a/Dockerfile.variant +++ b/Dockerfile.variant @@ -305,7 +305,7 @@ ARG MEMPALACE_TOOLKIT_REF=main # no ~67-minute base rebuild. (scripts/check-base-hash.sh scans only # Dockerfile.base, so no folding into the base hash is required — nor would # it be correct, since this ARG changes nothing about the base's contents.) -ARG SKILLSET_SNAPSHOT_REF=c04cd156592decf8258cfce7aa5e123ac0a5c7e2 +ARG SKILLSET_SNAPSHOT_REF=5fd0d5c406df506fd0e70977b6bd87f5fbc3b803 # Dockerfile.base sets description="pi-devbox — base image (variant-independent)" # and every variant INHERITS it, so both published images used to advertise @@ -365,12 +365,26 @@ RUN set -e; \ # reader with the skillset checked out — which on this fleet is every # host, since all four compose stacks mount it — verify the claim at # RUNTIME, without CI ever needing access to the private repo. Degrades - # to JSON null rather than failing the build if the file is absent; the - # smoke assertion is what turns that into a loud failure. + # to JSON null rather than failing the build if the directory is absent; + # the smoke assertion is what turns that into a loud failure. + # + # Hashes the whole DIRECTORY, not just SKILL.md: a single-file hash + # answers "did this one file change", not "is the live copy the same + # skill" — a live checkout that added or edited a SIBLING file (a + # reference/ doc, a helper script) would still report "identical to + # baked snapshot" against a file-only hash. pi-extensions already ships + # two files for exactly this reason (SKILL.md + evaluate-extension-usage.py), + # so this is not a hypothetical. Deterministic over `find | sort`, never + # readdir order: relative paths + per-file sha256, folded into one hash. + # pi-devbox-version mirrors this exact pipeline over the live directory so + # the two sides are comparable — if you change this, change that too. + tree_sha256() { \ + ( cd "$1" && find . -type f -print | LC_ALL=C sort | xargs -r sha256sum ) 2>/dev/null | sha256sum | cut -d' ' -f1; \ + }; \ SKILL_SNAP='null'; \ - _snap_file=/usr/local/share/pi-devbox/skills/mempalace/SKILL.md; \ - if [ -f "$_snap_file" ]; then \ - SKILL_SNAP="\"$(sha256sum "$_snap_file" | cut -d' ' -f1)\""; \ + _snap_dir=/usr/local/share/pi-devbox/skills/mempalace; \ + if [ -d "$_snap_dir" ] && [ -n "$(find "$_snap_dir" -type f -print -quit)" ]; then \ + SKILL_SNAP="\"$(tree_sha256 "$_snap_dir")\""; \ fi; \ { \ echo '{'; \ @@ -388,8 +402,10 @@ RUN set -e; \ # lie a future reader would act on), and `pi-devbox-version` renders # every components{} value with .value[0:12] — which would truncate # a 64-hex sha256 into something that looks like a short commit. + # Named `_tree_sha256`, not `_sha256`: it measures every file under the + # vendored skill directory, not one file — see tree_sha256() above. echo " \"skillset_snapshot_ref\": \"${SKILLSET_SNAPSHOT_REF}\","; \ - echo " \"skillset_snapshot_sha256\": ${SKILL_SNAP},"; \ + echo " \"skillset_snapshot_tree_sha256\": ${SKILL_SNAP},"; \ echo " \"components\": {"; \ echo " \"pi-toolkit\": \"$(rev /opt/pi-toolkit)\","; \ echo " \"pi-extensions\": \"$(rev /opt/pi-extensions)\","; \ diff --git a/rootfs/usr/local/bin/pi-devbox-version b/rootfs/usr/local/bin/pi-devbox-version index b83c117..4b17958 100755 --- a/rootfs/usr/local/bin/pi-devbox-version +++ b/rootfs/usr/local/bin/pi-devbox-version @@ -28,15 +28,33 @@ MANIFEST=/etc/pi-devbox/build-manifest.json MODE="human" SHOW_SKILLS="yes" -case "${1:-}" in - --json) MODE="json" ;; - --quiet|-q) MODE="quiet" ;; - --no-skills) SHOW_SKILLS="no" ;; - --help|-h) - sed -n '2,22p' "$0" | sed 's/^# \?//' - exit 0 - ;; -esac +# A `case "${1:-}"` here only ever looked at the FIRST argument, so +# `--no-skills --json` matched --no-skills, silently dropped --json, and +# printed human text to a caller expecting JSON (a real failure: a jq +# consumer piping that output gets a parse error, not a wrong-but-parseable +# answer). Loop over every argument instead, and reject anything unknown +# rather than silently ignoring it the same way. +for _arg in "$@"; do + case "$_arg" in + --json) MODE="json" ;; + --quiet|-q) MODE="quiet" ;; + --no-skills) SHOW_SKILLS="no" ;; + --help|-h) + # Print the leading `#`-comment block verbatim, stopping at the first + # non-comment line, rather than a hardcoded line range: `sed -n + # '2,22p'` was silently truncating --help because this file has grown + # usage lines since that range was written, and a fixed range will + # drift again the next time a comment is added above it. + awk 'NR==1{next} /^#/{sub(/^# ?/,""); print; next} {exit}' "$0" + exit 0 + ;; + *) + echo "pi-devbox-version: unknown option: $_arg" >&2 + echo " try --help" >&2 + exit 2 + ;; + esac +done if [ ! -f "$MANIFEST" ]; then echo "pi-devbox-version: no build manifest at $MANIFEST" >&2 @@ -132,8 +150,21 @@ if [ "$SHOW_SKILLS" = "yes" ] && [ -d "$BAKED_SKILLS" ] && [ -d "$SKILLS_DIR" ]; # Recorded provenance of the vendored mempalace snapshot (absent on images # built before this existed — `// empty` so a JSON null never prints as the # 4-char string "null", the same trap noted for mempalace_version above). + # `_tree_sha256`, not `_sha256`: it is a hash over every file in the + # vendored skill DIRECTORY (see tree_sha256() below), not one file, because + # a single-file hash reports "identical" against a live checkout that added + # or edited a sibling file — pi-extensions already ships two files, so this + # is not hypothetical. snap_ref=$(jq -r '.skillset_snapshot_ref // empty' "$MANIFEST") - snap_sha=$(jq -r '.skillset_snapshot_sha256 // empty' "$MANIFEST") + snap_sha=$(jq -r '.skillset_snapshot_tree_sha256 // empty' "$MANIFEST") + # Same pipeline Dockerfile.variant uses to measure the baked directory at + # build time: relative paths in `find | sort` order, each hashed, the whole + # listing folded into one sha256. Keep the two definitions identical — they + # run in different processes (image build vs. this container) and are + # meaningless to compare unless they agree byte-for-byte on the algorithm. + tree_sha256() { + ( cd "$1" && find . -type f -print | LC_ALL=C sort | xargs -r sha256sum ) 2>/dev/null | sha256sum | cut -d' ' -f1 + } # Iterate the baked tree rather than a hardcoded name list, so vendoring a # fourth skill needs no edit here. The header prints only if the tree is @@ -176,9 +207,12 @@ if [ "$SHOW_SKILLS" = "yes" ] && [ -d "$BAKED_SKILLS" ] && [ -d "$SKILLS_DIR" ]; # For the one skill whose baked fingerprint we recorded, say plainly # whether the live copy differs from what shipped. This is the check CI # cannot perform (the skillset is private) and the container can, free. + # Hash the whole live DIRECTORY with the same tree_sha256() used to + # measure the baked one in Dockerfile.variant — a SKILL.md-only compare + # would silently ignore a changed or added sibling file. _live_sha="" - if [ -n "$snap_sha" ] && [ "$_name" = "mempalace" ] && [ -f "$_target/SKILL.md" ]; then - _live_sha=$(sha256sum "$_target/SKILL.md" 2>/dev/null | cut -d' ' -f1 || echo "") + if [ -n "$snap_sha" ] && [ "$_name" = "mempalace" ] && [ -d "$_target" ]; then + _live_sha=$(tree_sha256 "$_target") fi if [ -z "$_live_sha" ]; then printf ' %-22s %s\n' "$_name" "$_where" diff --git a/rootfs/usr/local/share/pi-devbox/skills/VENDORED.md b/rootfs/usr/local/share/pi-devbox/skills/VENDORED.md index 838d14b..10d25f2 100644 --- a/rootfs/usr/local/share/pi-devbox/skills/VENDORED.md +++ b/rootfs/usr/local/share/pi-devbox/skills/VENDORED.md @@ -114,15 +114,31 @@ mounted, plus a fabricated-skillset run of the reconciler). cp /skill/SKILL.md pi-extensions/SKILL.md cp /skill/evaluate-extension-usage.py pi-extensions/ - cp /skills/mempalace/SKILL.md mempalace/SKILL.md -Copy each snapshot **from its owner in the table above** — `pi-extensions` from -the package repo's `skill/` (since `a7f3044` co-located it there; `skillset` -also carries a copy, but it is a downstream duplicate and can lag), and -`mempalace` from `skillset`. Copying `pi-extensions` from `skillset` would -regress the snapshot to whatever that repo last mirrored. +Copy `pi-extensions` **from its owner in the table above** — the package +repo's `skill/` (since `a7f3044` co-located it there; `skillset` also carries a +copy, but it is a downstream duplicate and can lag). Copying `pi-extensions` +from `skillset` would regress the snapshot to whatever that repo last mirrored. -Snapshot provenance at last refresh: skillset `670f7f1`, pi-extensions pkg `e73cb9f`. +`mempalace` is **not** refreshed by `cp` — see the *Freshness model* section +above: `scripts/vendor-mempalace-skill.sh ` is the only thing +that should ever touch that snapshot, because a bare copy can update the bytes +without updating the ref that claims to describe them, which produces a +manifest that confidently lies. + +Neither vendored skill has a hand-maintained "last refreshed at" line here on +purpose — one previously existed (skillset `670f7f1`, pi-extensions pkg +`e73cb9f`) and went stale within hours, because nothing forced it to move +when the ARGs did. `670f7f1` is now a cautionary example rather than a fact +worth recording: it is the commit that told agents to hand-stamp `added_by`, +which a later skillset commit (and the pi-devbox edge stamper) withdrew — so a +reader trusting that line would have been pointed at superseded guidance. +Both facts it tried to capture now live somewhere that cannot drift by hand: + +| Fact | Where | +|---|---| +| which skillset commit `mempalace`'s bytes came from | `ARG SKILLSET_SNAPSHOT_REF` (Dockerfile.variant) + `skillset_snapshot_ref` in `build-manifest.json`, written *only* by `vendor-mempalace-skill.sh` | +| which pi-extensions package commit was vendored | `ARG PI_EXTENSIONS_REF` (Dockerfile.variant, CI-resolved to a 40-hex commit) → OCI label `se.jordbo.pi-devbox.pi-extensions-ref` and `build-manifest.json`'s `components.pi-extensions`, both read from the actual `/opt/pi-extensions` checkout, not from intent | When you refresh the `mempalace` snapshot, also update the phrase asserted by the "mempalace skill snapshot is current" smoke test — it deliberately pins the diff --git a/rootfs/usr/local/share/pi-devbox/skills/mempalace/SKILL.md b/rootfs/usr/local/share/pi-devbox/skills/mempalace/SKILL.md index ccc7569..eca8708 100644 --- a/rootfs/usr/local/share/pi-devbox/skills/mempalace/SKILL.md +++ b/rootfs/usr/local/share/pi-devbox/skills/mempalace/SKILL.md @@ -41,6 +41,21 @@ Run these immediately when a session begins, before responding to the user: mempalace_kg_query(entity="") ``` +4. **Check your mailbox.** Just run it — an empty result is a fine answer and + costs one call. Do not try to decide first whether coordination "applies to + you"; that test is what used to be wrong here (see *Cross-Machine + Coordination* below): + ``` + mempalace_event_list(to_agent="@", status="open") + ``` + This is a candidate list, not a to-do list — `status` never changes after an + event is written, so finished asks keep matching. Subtract the ones you have + already answered using the rule in *What you actually owe*, below. + Another machine may have asked you something, or corrected something you are + about to rely on. This costs one call and is the only way you will find out: + nothing pushes an event into your session unless your bridge delivers it for + you, and if it does you will already have seen it before reading this. + Do NOT announce this to the user. Just do it silently to orient yourself. ### Temporal grounding — compute time deltas, don't guess @@ -340,6 +355,158 @@ mempalace_kg_invalidate(subject="...", predicate="...", object="...", ended="@` +stamping in *Provenance is stamped for you* is load-bearing here and not mere +tidiness. + +### Reading your mailbox + +``` +mempalace_event_list(to_agent="@", status="open") +``` + +- `to_agent=` **also matches `*` broadcasts**, so one call covers both. No + second query needed. +- `status="open"` narrows the mailbox to what a sender *said was an ask at the + time of writing* — that is all it can do. It is a good first filter (on a real + stream it cut 5 events to 2), but it is **not** a list of what you owe, and it + never shrinks as you work. Treating it as owed-ness is the mistake this + section previously made: an earlier draft cited "5 unfiltered, exactly 1 + filtered — the one that needed a reply" as proof the filter tracked + obligation. It did not. That single result was an event which had *already + been acked* half an hour earlier; the filter looked decisive only because the + stream happened to contain one directed `open` event. **Unfiltered mailboxes + train you to ignore them — and so does a filter that keeps showing you + finished work.** +- To resume where you left off, use `since_event_id`, **never** + `since_created_at`. A timestamp cursor permanently skips an event that synced + in late — it is a time window ("what happened today"), not a cursor. +- Read `metadata` before acting: senders put the load-bearing specifics there + (which host verified what, which run failed, what a change retracts). + +### The ack contract — the sender declares whether a reply is owed + +An obligation you never agreed to is noise, so the sender states it: + +| Sender writes | Means | Recipient owes | +|---|---|---| +| `to_agent=""` + `status="open"` | an ask | an ack or a reply (the event itself keeps matching forever — see below) | +| `to_agent="*"` (any status) | broadcast FYI | nothing | +| any other status (`ready`, `applied`, `blocked`, …) | a statement of fact | nothing | + +Ack with `mempalace_event_ack(event_id=…, from_agent="", status=…)`. It +**appends a new event** and never mutates the original; the correlation id is +copied for you, and `metadata.ack_of` is set to the event you answered. + +#### What you actually owe — derive it, do not read it off `status` + +The log is append-only and `status` is written **once**, so it is an honest +statement about an item *at the moment it was written* and nothing more. It is +not mutable state, and asking it to carry mutable state is what breaks: +acking appends a new event and changes nothing about the old one, so **a +directed `open` event matches your mailbox query forever, answered or not.** +Nothing is ever "dismissed" — which also means a deferred ask cannot be +accidentally lost, only that you must compute what is outstanding: + +``` +candidates = mempalace_event_list(to_agent="", status="open") +mine = mempalace_event_list(from_agent="") +``` + +A candidate is **answered** when one of your own events + +1. has a **higher `seq`** than the candidate, and +2. joins to it — `metadata.ack_of == candidate.id` (exact, written for you by + `event_ack`) or the same `correlation_id` (the fallback), and +3. carries a **terminal** status: `applied`, `superseded`, `failed`, `blocked`. + +Everything else is still owed. Two calls, constant cost. + +**Compare `seq`, never `created_at`** — the same reason you resume with +`since_event_id`. Without the ordering test, one terminal reply would suppress +every later ask on the same `correlation_id` for good; verified on a live thread +where a `ready` reply at `seq` 16 sits *before* the request at `seq` 17 that it +obviously cannot have answered. + +This also supplies the "taken, not finished" state that looked missing: +`claimed` and `ready` are deliberately **not** terminal, so work you have picked +up keeps resurfacing until you close it out. No extra convention, no new field. + +Two consequences worth internalising: + +- **"Seen, not doing it" is a legitimate ack** — `status="blocked"` or + `"superseded"` plus the reason. Silence is not, and it is not merely rude: + with no terminal event of yours to join to, the ask stays in the owed set + indefinitely and there is nothing anyone can do about it from the other end. +- **Nothing expires, and it should not.** An `open` with no terminal reply is + still live by definition, and the finished threads are valuable history. If + content is genuinely perishable ("do not push to main for the next hour"), say + so in `metadata.expires_at` — metadata is stored verbatim — and honour it as a + hint when reading. An old `open` that the derivation still counts as owed is a + signal, not garbage: it means somebody asked and nobody answered. + +### Writing to another machine + +- **Address the stamped name you actually saw** in a `from_agent` field, e.g. + `pi@tor-ms22`. A bare `pi` reaches nobody's mailbox once stamping is live, and + older events in the log still carry bare names — do not copy them. +- **Use `status="open"` only when you truly need an answer.** It places an + obligation on another machine. +- **Never broadcast an ask.** `to_agent="*"` + `status="open"` obliges everyone + and therefore no one. +- **Always set a `correlation_id` on a directed `open`,** and reply with the + same one. It is not just for reconstructing a conversation later: it is the + join the owed-set derivation depends on. An uncorrelated ask can only ever be + closed by an `event_ack` (which sets `ack_of` for you) — a plain reply cannot + be matched to it at all. +- **Corrections are new events, never edits.** Say explicitly what you retract + and name the id — drawer or event — that carried the withdrawn claim. +- **Put a retraction where the reader will look.** An event reaches a live agent; + a *drawer* is what a future semantic search finds. If you filed advice as a + drawer and later withdraw it, file the withdrawal as a drawer too — otherwise + the next agent finds your original confident advice and no trace of the + correction. (This is a real incident, not a hypothetical.) +- **Hand over exact content as an artifact**, not prose: `mempalace_artifact_put` + or `mempalace_patch_submit` store bytes with a sha256, and the event references + the id. Never paste a diff into a body and hope it survives. +- **Waiting on a specific reply?** `mempalace_event_wait` blocks with backoff — + do not poll `event_list` in a loop. A timeout there is a normal result, not an + error. + ## Palace Structure ### Wings @@ -413,4 +580,6 @@ Entity-relationship triples with temporal validity. Query with `mempalace_kg_que - **Don't mine .git directories or node_modules.** The CLI miner respects .gitignore by default. - **Don't create duplicate drawers.** Use `mempalace_check_duplicate` before adding manually. - **Don't treat the palace as a task list.** It's for knowledge and context, not todos. +- **Don't broadcast an ask, and don't leave one unanswered.** On a shared palace, `to_agent="*"` + `status="open"` obliges every machine and therefore none of them. And don't expect acking to tidy your mailbox: `status` is immutable, so the event keeps matching either way — what a terminal reply buys you is that the *derived* owed set (see *What you actually owe*) stops counting it. Leave asks unanswered and that set only grows, until everyone learns to stop looking. "Seen, not doing it" is a complete answer — silence is not. +- **Don't assume you would have heard.** Nothing pushes another machine's message into your session. If you did not run the mailbox query at wake-up, a correction addressed to you by name can sit unread while you confidently rebuild the thing it warned you about. - **Don't invent provenance metadata, and don't hand-stamp it either.** An earlier version of this list told you to set `added_by="@"` by hand; that instruction has been withdrawn, because RFC 001 §7.3.2 places provenance at the client/server boundary and the pi bridge now does it uniformly (see *Provenance is stamped for you* above) — but the withdrawal only holds where the bridge is live, so run the one-line check in that bullet first; on an older image hand-stamping is still the only signal a hand-filed drawer gets. DO NOT invent values for the palace's own metadata fields (`device`, `agent_kind`, `origin_device`): those are stamped by infrastructure that also records *how* each was determined, and a fabricated value is worse than none because it silently corrupts a future merge. DO pass `source_drawer_id` on `kg_add`. And never put a machine name in a diary's `agent_name` — it becomes the wing name and hides your entries from `diary_read`. diff --git a/scripts/smoke-test.sh b/scripts/smoke-test.sh index f8b5de1..b86c016 100755 --- a/scripts/smoke-test.sh +++ b/scripts/smoke-test.sh @@ -480,18 +480,23 @@ run_expect "pi-devbox-version --quiet is a compact one-liner" \ run "manifest records the vendored skill snapshot provenance" ' j=/etc/pi-devbox/build-manifest.json r=$(jq -r ".skillset_snapshot_ref // empty" $j) - s=$(jq -r ".skillset_snapshot_sha256 // empty" $j) - echo "ref=[$r] sha256=[$s]" >&2 + s=$(jq -r ".skillset_snapshot_tree_sha256 // empty" $j) + echo "ref=[$r] tree_sha256=[$s]" >&2 printf "%s" "$r" | grep -qxE "[0-9a-f]{40}" \ || { echo "skillset_snapshot_ref is not a 40-hex commit" >&2; exit 1; } printf "%s" "$s" | grep -qxE "[0-9a-f]{64}" \ - || { echo "skillset_snapshot_sha256 is not a 64-hex digest" >&2; exit 1; } + || { echo "skillset_snapshot_tree_sha256 is not a 64-hex digest" >&2; exit 1; } ' +# Recomputes over the whole DIRECTORY with the same tree_sha256() pipeline +# Dockerfile.variant used to measure it, not a plain `sha256sum SKILL.md` — +# a file-only compare here would pass even if the manifest recorded a +# fingerprint over a directory that has since grown a second file (this is +# not hypothetical: pi-extensions already ships two files for its skill). run "manifest skill fingerprint matches the baked snapshot" ' j=/etc/pi-devbox/build-manifest.json - f=/usr/local/share/pi-devbox/skills/mempalace/SKILL.md - m=$(jq -r ".skillset_snapshot_sha256 // empty" $j) - a=$(sha256sum "$f" | cut -d" " -f1) + d=/usr/local/share/pi-devbox/skills/mempalace + m=$(jq -r ".skillset_snapshot_tree_sha256 // empty" $j) + a=$( (cd "$d" && find . -type f -print | LC_ALL=C sort | xargs -r sha256sum) | sha256sum | cut -d" " -f1) echo "manifest=[$m] actual=[$a]" >&2 [ -n "$m" ] && [ "$m" = "$a" ] ' @@ -615,8 +620,12 @@ exec_test "pi-devbox-version reports skill sources (all baked, no skillset here) # version FIRST, before the baked links exist and long before the skillset # deploy + reconcile run last, so anything it said about skill sources would be # a pre-reconcile state that is about to change. +# A bare negative (`! grep -q "skills:"`) passes if the tool crashes or +# prints nothing at all — it cannot tell "correctly omitted the section" +# apart from "the binary is broken". Anchor it positively: the command must +# still succeed and still print its normal release-tag line. exec_test "pi-devbox-version --no-skills omits the skills section" \ - '! pi-devbox-version --no-skills | grep -q "skills:"' + 'out=$(pi-devbox-version --no-skills) && echo "$out" | grep -q "^pi-devbox " && ! echo "$out" | grep -q "skills:"' exec_test "entrypoint prints the version banner with --no-skills" \ 'grep -q "pi-devbox-version --no-skills" /usr/local/bin/entrypoint-user.sh' # The handover path itself. CI never mounts a skillset, so without this the diff --git a/scripts/vendor-mempalace-skill.sh b/scripts/vendor-mempalace-skill.sh index b10b00e..385295e 100755 --- a/scripts/vendor-mempalace-skill.sh +++ b/scripts/vendor-mempalace-skill.sh @@ -19,20 +19,53 @@ # matching ARG bump produces a manifest that CONFIDENTLY LIES — worse than the # anonymous snapshot it replaced. # +# HARDENED after peer review (pi@emb-7kj4vr4g, logstream correlation +# skills-provenance-review, 2026-08-26) proved the original --check could print +# OK and exit 0 without actually verifying anything: `git show :` +# emits NOTHING when the ref/path doesn't resolve, and `sha256sum` still hashes +# that empty stdin, so "ref not found" silently collided with "the file really +# is 0 bytes". Depending on which side of the comparison hit the collision this +# fell through as either a false MISMATCH (blaming provenance for what was +# really an incomplete clone) or, worse, a false OK. See EXIT STATUS below — +# "cannot determine" is now its own outcome, distinct from "confirmed wrong", +# which is the same distinction the phrase canary this script replaced lacked. +# # USAGE -# scripts/vendor-mempalace-skill.sh [skillset-root] refresh + record -# scripts/vendor-mempalace-skill.sh --check [root] verify, write nothing +# scripts/vendor-mempalace-skill.sh [skillset-root] [--force] +# refresh: rewrite the snapshot and the ARG together. +# scripts/vendor-mempalace-skill.sh --check [skillset-root] +# verify only, writes nothing. The root path and any flag may appear in +# either order — a positional-only parser previously made ` +# --check` silently run a refresh instead of the verification asked for. # # skillset-root defaults to /workspace/skillset, then $HOME/skillset. # +# --force (refresh mode only) proceed even when the recorded ref cannot be +# proven to be an ancestor of the skillset's current HEAD — i.e. +# skip the guard against silently REWINDING provenance, which a +# detached HEAD, an older checkout, or a shallow clone lacking the +# recorded commit can all trigger. Meant to be used deliberately, +# not habitually: each use is a human deciding a rewind is fine. +# # --check answers "is the committed snapshot really skillset@?" # — the question CI cannot answer without a credential for the private repo, -# and which anyone with the skillset checked out can answer for free. Exit 0 -# when the snapshot is honest and current, 1 when it is not. +# and which anyone with the skillset checked out can answer for free. # -# EXIT STATUS -# 0 refreshed (or already up to date; --check: snapshot verified) -# 1 refused: dirty upstream file, missing repo, or --check mismatch +# EXIT STATUS (same three codes in both modes) +# 0 the operation succeeded, or (--check) the record is verified truthful. +# This INCLUDES a truthful record that is merely stale — upstream has +# moved on since the recorded ref, or the local working tree has since +# diverged. A NOTICE is printed to stderr, but the snapshot is not being +# accused of lying, so this is not a release-blocking failure. Skipping a +# refresh is a legitimate release-day choice (see AGENTS.md); this exit +# code is what makes that choice checkable rather than merely asserted. +# 1 refused: a CONFIRMED problem. Dirty upstream file; a refresh that would +# rewind past the recorded ref; or (--check) the vendored bytes provably +# do NOT match the file at the recorded ref — a lying record. +# 2 cannot determine: the recorded ref, or the path at that ref, is not +# resolvable in this clone. Commonly a shallow clone missing history, or +# a ref that was rewritten or never pushed. Deliberately NOT the same as +# 1 — "I can't tell" must never be reported as "it's wrong". set -euo pipefail cd "$(dirname "$0")/.." @@ -42,13 +75,28 @@ VENDORED="rootfs/usr/local/share/pi-devbox/skills/mempalace/SKILL.md" ARG_NAME="SKILLSET_SNAPSHOT_REF" REL_PATH="skills/mempalace/SKILL.md" +die() { printf '%s: %s\n' "$(basename "$0")" "$1" >&2; exit 1; } + +# Parse flags and the optional root path in either order, and reject anything +# unrecognised rather than silently absorbing it. MODE="refresh" -if [ "${1:-}" = "--check" ]; then - MODE="check" - shift +FORCE=0 +ROOT="" +for arg in "$@"; do + case "$arg" in + --check) MODE="check" ;; + --force) FORCE=1 ;; + --*) die "unknown option: $arg" ;; + *) + [ -z "$ROOT" ] || die "unexpected extra argument: $arg (root already set to $ROOT)" + ROOT="$arg" + ;; + esac +done +if [ "$MODE" = "check" ] && [ "$FORCE" = 1 ]; then + die "--force has no effect with --check (nothing is written); remove it" fi -ROOT="${1:-}" if [ -z "$ROOT" ]; then for candidate in /workspace/skillset "$HOME/skillset"; do if [ -d "$candidate/.git" ]; then @@ -58,35 +106,33 @@ if [ -z "$ROOT" ]; then done fi -die() { printf '%s: %s\n' "$(basename "$0")" "$1" >&2; exit 1; } - [ -n "$ROOT" ] || die "no skillset clone found (pass one: $(basename "$0") /path/to/skillset)" [ -d "$ROOT/.git" ] || die "not a git clone: $ROOT" [ -f "$ROOT/$REL_PATH" ] || die "no $REL_PATH in $ROOT" [ -f "$VENDORED" ] || die "vendored snapshot missing: $VENDORED" +[ -s "$VENDORED" ] || die "vendored snapshot is empty: $VENDORED" head_sha=$(git -C "$ROOT" rev-parse HEAD 2>/dev/null) || die "cannot read HEAD of $ROOT" recorded=$(grep -oE "^ARG ${ARG_NAME}=[0-9a-f]{40}$" "$DOCKERFILE" | cut -d= -f2 || true) [ -n "$recorded" ] || die "no 'ARG ${ARG_NAME}=<40-hex>' line in $DOCKERFILE" sha_of() { sha256sum "$1" | cut -d' ' -f1; } -sha_empty=$(printf '' | sha256sum | cut -d' ' -f1) vendored_sha=$(sha_of "$VENDORED") upstream_sha=$(sha_of "$ROOT/$REL_PATH") -# The recorded ref must PROVABLY describe the recorded bytes. Reviewed by -# pi@emb-7kj4vr4g (logstream correlation skillset-vendor-drift, 2026-08-26): -# "make sure the resync script writes the ref it ACTUALLY copied from, or the -# provenance field inherits the same class of bug the canary just had." So this -# does not copy the working tree and then hope a `git diff` was enough — a -# clean-looking `git diff` says nothing about an UNTRACKED file, and a detached -# or behind checkout can be clean while HEAD names something else. Instead the -# snapshot is CONSTRUCTED from the ref being recorded (below), and the working -# tree is compared only so a local edit produces a refusal rather than a silent -# surprise. Order matters here: everything this block reads is defined above it. -blob_sha=$(git -C "$ROOT" show "HEAD:$REL_PATH" 2>/dev/null | sha256sum | cut -d' ' -f1 || true) +# Does $REL_PATH exist at HEAD at all? Proven with `cat-file -e` BEFORE +# hashing anything. Piping a failed `git show` straight into sha256sum, as +# this script used to, hashes an EMPTY stream and produces sha256(""): a real, +# collidable value — not a representation of absence. That collapsed "doesn't +# exist" and "exists and happens to be empty" into the same signal, which is +# exactly the defect class the peer review found in --check's at_ref, below. +blob_sha="" +if git -C "$ROOT" cat-file -e "HEAD:$REL_PATH" 2>/dev/null; then + blob_sha=$(git -C "$ROOT" show "HEAD:$REL_PATH" | sha256sum | cut -d' ' -f1) +fi + upstream_dirty="" -if [ -z "$blob_sha" ] || [ "$blob_sha" = "$sha_empty" ]; then +if [ -z "$blob_sha" ]; then upstream_dirty="not present at HEAD (untracked, or absent at this commit)" elif [ "$blob_sha" != "$upstream_sha" ]; then if ! git -C "$ROOT" diff --quiet -- "$REL_PATH" 2>/dev/null; then @@ -99,17 +145,41 @@ elif [ "$blob_sha" != "$upstream_sha" ]; then fi if [ "$MODE" = "check" ]; then - # Compare the committed snapshot against the file AT THE RECORDED REF, not - # against the working tree: the question is whether the record is truthful, - # which is independent of how current it is. Both are reported. - at_ref=$(git -C "$ROOT" show "${recorded}:${REL_PATH}" 2>/dev/null | sha256sum | cut -d' ' -f1 || true) + # Resolve the recorded ref the same careful way: existence is proven with + # `cat-file -e` before anything is hashed, and "the ref itself is missing" + # is reported distinctly from "the ref resolves but the path isn't there + # at it" — both used to be silently swallowed into a plausible sha256(""). + ref_exists=0 + path_at_ref_exists=0 + at_ref="" + if git -C "$ROOT" cat-file -e "${recorded}^{commit}" 2>/dev/null; then + ref_exists=1 + if git -C "$ROOT" cat-file -e "${recorded}:${REL_PATH}" 2>/dev/null; then + path_at_ref_exists=1 + at_ref=$(git -C "$ROOT" show "${recorded}:${REL_PATH}" | sha256sum | cut -d' ' -f1) + fi + fi + printf 'recorded ref: %s\n' "$recorded" printf 'vendored sha256: %s\n' "$vendored_sha" - printf 'sha256 at ref: %s\n' "${at_ref:-}" - printf 'skillset HEAD: %s (%s)\n' "$head_sha" "$(sha_of "$ROOT/$REL_PATH")" + if [ "$path_at_ref_exists" = 1 ]; then + printf 'sha256 at ref: %s\n' "$at_ref" + elif [ "$ref_exists" = 1 ]; then + printf 'sha256 at ref: <%s not present at %s>\n' "$REL_PATH" "${recorded:0:7}" + else + printf 'sha256 at ref: <%s not present in this clone>\n' "${recorded:0:7}" + fi + printf 'skillset HEAD: %s (%s)\n' "$head_sha" "$upstream_sha" + if [ -n "$upstream_dirty" ]; then + printf 'live working tree: %s\n' "$upstream_dirty" + fi + rc=0 - if [ -z "$at_ref" ]; then - printf 'UNKNOWN: %s is not in this clone — fetch, or check against a complete one\n' "$recorded" >&2 + if [ "$ref_exists" != 1 ]; then + printf 'CANNOT-DETERMINE: %s is not present in %s — fetch, or check against a complete clone\n' "$recorded" "$ROOT" >&2 + rc=2 + elif [ "$path_at_ref_exists" != 1 ]; then + printf 'MISMATCH: %s does not exist at %s in this clone — the recorded ref cannot be describing these bytes\n' "$REL_PATH" "$recorded" >&2 rc=1 elif [ "$at_ref" != "$vendored_sha" ]; then printf 'MISMATCH: the vendored snapshot is NOT the file at the recorded ref\n' >&2 @@ -117,19 +187,27 @@ if [ "$MODE" = "check" ]; then else printf 'OK: the vendored snapshot is exactly skillset@%s:%s\n' "${recorded:0:7}" "$REL_PATH" fi - if [ "$vendored_sha" != "$upstream_sha" ]; then + + # Staleness is orthogonal to truthfulness: a record can correctly describe + # an old commit even after upstream has moved on, and a dirty local working + # tree in $ROOT doesn't rewrite git history either — it says nothing about + # whether the RECORDED, committed ref describes the RECORDED, committed + # bytes. Only worth reporting once we already know rc=0 (truthful) — a + # MISMATCH or CANNOT-DETERMINE is the dominant fact and a staleness note + # would only muddy it. + if [ "$rc" = 0 ] && [ "$vendored_sha" != "$upstream_sha" ]; then # Name the ACTUAL cause. "working tree differs" is wrong when the tree is - # clean and the ref simply moved on — a message that names the wrong cause is - # the same defect class as a canary pinned to a deleted phrase. + # clean and the ref simply moved on — a message that names the wrong cause + # is the same defect class as a canary pinned to a deleted phrase. if [ "$recorded" != "$head_sha" ] && [ "$blob_sha" = "$upstream_sha" ]; then - printf 'STALE: %s has moved to %s; the snapshot describes the older %s\n' \ + printf 'NOTICE: %s has moved to %s; the snapshot describes the older %s (stale, not untruthful)\n' \ "$ROOT" "${head_sha:0:7}" "${recorded:0:7}" >&2 else - printf 'STALE: the working tree of %s differs from the snapshot (HEAD %s)\n' \ - "$ROOT" "${head_sha:0:7}" >&2 + printf 'NOTICE: the working tree of %s differs from the snapshot (HEAD %s)\n' \ + "$ROOT" "${head_sha:0:7}" >&2 fi - rc=1 fi + exit "$rc" fi @@ -140,21 +218,45 @@ if [ "$vendored_sha" = "$upstream_sha" ] && [ "$recorded" = "$head_sha" ]; then exit 0 fi -# Written FROM THE REF, not copied from the working tree, so the pair cannot be -# a lie by construction. Via a temp file so a failed write cannot leave a +# Refuse to silently REWIND provenance. `git checkout `, a detached HEAD, +# or an older checkout can all leave $ROOT's HEAD behind the already-recorded +# ref; without this guard a refresh there would happily rewrite both the ARG +# and the bytes backwards and report it as an ordinary update. +if [ "$recorded" != "$head_sha" ]; then + if git -C "$ROOT" cat-file -e "${recorded}^{commit}" 2>/dev/null; then + if ! git -C "$ROOT" merge-base --is-ancestor "$recorded" "$head_sha" 2>/dev/null; then + if [ "$FORCE" != 1 ]; then + die "refusing: $ROOT's HEAD ($head_sha) is not a descendant of the recorded ref ($recorded) — this looks like a rewind. Pass --force if this is intentional." + fi + printf 'WARNING: --force set; %s is not an ancestor of HEAD %s — proceeding anyway\n' "${recorded:0:7}" "${head_sha:0:7}" >&2 + fi + else + if [ "$FORCE" != 1 ]; then + printf 'CANNOT-DETERMINE: %s is not present in %s (shallow clone?) — fetch full history to verify this refresh moves forward, or pass --force to proceed without that guarantee\n' "$recorded" "$ROOT" >&2 + exit 2 + fi + printf 'WARNING: --force set; %s could not be resolved in %s — proceeding without verifying forward motion\n' "${recorded:0:7}" "$ROOT" >&2 + fi +fi + +# Written FROM THE REF, not copied from the working tree, so the pair cannot +# be a lie by construction. Via a temp file so a failed write cannot leave a # half-vendored snapshot behind. snap_tmp=$(mktemp) if ! git -C "$ROOT" show "HEAD:$REL_PATH" > "$snap_tmp" 2>/dev/null; then rm -f -- "$snap_tmp" die "cannot read HEAD:$REL_PATH from $ROOT" fi +chmod 0644 -- "$snap_tmp" mv -- "$snap_tmp" "$VENDORED" [ "$(sha_of "$VENDORED")" = "$blob_sha" ] \ || die "internal: written snapshot does not match HEAD:$REL_PATH" -# In-place, and only the exact pinned line: a broad sed on this Dockerfile could -# rewrite one of the other *_REF ARGs. + +# In-place, and only the exact pinned line: a broad sed on this Dockerfile +# could rewrite one of the other *_REF ARGs. tmp=$(mktemp) -sed "s|^ARG ${ARG_NAME}=.*$|ARG ${ARG_NAME}=${head_sha}|" "$DOCKERFILE" > "$tmp" +sed "s|^ARG ${ARG_NAME}=.*\$|ARG ${ARG_NAME}=${head_sha}|" "$DOCKERFILE" > "$tmp" +chmod 0644 -- "$tmp" mv -- "$tmp" "$DOCKERFILE" new_recorded=$(grep -oE "^ARG ${ARG_NAME}=[0-9a-f]{40}$" "$DOCKERFILE" | cut -d= -f2 || true)