From b5810654f615020239e1ff6b2f053d32ee1d0ce8 Mon Sep 17 00:00:00 2001 From: Joakim Persson Date: Sun, 23 Aug 2026 20:59:14 +0200 Subject: [PATCH] skills: let the skillset own the skills it owns, and stop a dangling link from killing boot Baked skill links won over the live skillset clone for all three vendored skills, so a pushed edit to skills/mempalace/SKILL.md was invisible in every container until the next image build -- measured on two hosts (live md5 129bcc4752 vs baked 5236024fef). Cause was ordering, not intent: the baked links are created early with a create-only-when-absent guard to close a smoke readiness race, and the skillset deploy runs last and treats them as foreign. The comment claimed the opposite of the behaviour. The fix is not "skillset always wins". Ownership is per-skill: pi-extensions is owned by its package repo and copied over the snapshot at build time, so the skillset's lagging duplicate must keep losing; pi-devbox-environment is authored here. Only mempalace is skillset-owned. devbox-skill-reconcile therefore runs after the deploy and repoints only the names in skills/skillset-owned.txt, replacing a link solely when it points into the baked tree, so a real directory or a link pointing elsewhere is never disturbed. Precedence is now user override -> live clone (owned names) -> baked snapshot, with the early links intact as the fallback so the readiness race stays closed. Reviewing that turned up a latent boot-abort in the pre-existing baked-link block: `[ ! -e "$link" ]` is TRUE for a dangling symlink, so once a link can point into /workspace/skillset, a vanished mount makes plain `ln -s` fail with "File exists" -- and under `set -euo pipefail` that aborts container start before `exec "$@"`. Reachable on `docker restart` or a host reboot, not on a recreate, since ~/.agents is not a volume on any host. Now `ln -sfn`, which heals the link back to the baked fallback. Smoke additions cover what let this ship: the stale-snapshot canary grepped a phrase present in BOTH the stale and fresh copies, so it passed throughout; it now pins the newest section. Link targets are asserted, not just `test -L`; the owned-list content is asserted both ways; and the reconciler's replace path -- which no CI container exercises, since none mounts a skillset -- is covered by fabricating one. A mutation test showed the obvious three assertions still pass with the "is this link ours?" guard deleted, so a discriminating case was added: an owned name whose link is a user override outside the baked tree. Also refreshes the mempalace snapshot to skillset 670f7f1 (without it the fix helps only hosts that mount skillset) and corrects README, which documented the old, wrong precedence in three places. Verified with 12 fixture cases plus 2 mutants: ownership respected against the real trees, user overrides preserved, relative/trailing-slash/CRLF/space/glob inputs handled, dangling link healed, read-only skills dir exits 0, idempotent. --- CHANGELOG.md | 143 ++++++++++++++---- Dockerfile.base | 2 + README.md | 21 ++- entrypoint-user.sh | 33 +++- rootfs/usr/local/bin/devbox-skill-reconcile | 91 +++++++++++ .../local/share/pi-devbox/skills/VENDORED.md | 43 +++++- .../share/pi-devbox/skills/mempalace/SKILL.md | 1 + .../share/pi-devbox/skills/skillset-owned.txt | 16 ++ scripts/smoke-test.sh | 58 ++++++- 9 files changed, 365 insertions(+), 43 deletions(-) create mode 100755 rootfs/usr/local/bin/devbox-skill-reconcile create mode 100644 rootfs/usr/local/share/pi-devbox/skills/skillset-owned.txt diff --git a/CHANGELOG.md b/CHANGELOG.md index c048dfb..a2ebd4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,43 +13,116 @@ Pre-v1.0.0 tags followed the pi npm version (`v{pi_version}[letter]`). ## Unreleased -Docs only so far, but one of the two files ships **inside** the image. +### Fixed -### Known issue (proposed for v1.8.5, not yet fixed) - -- **Vendored skills silently shadow their live skillset counterparts — an - ordering bug, and the code comment claims the opposite.** `~/.agents/skills` - is *asymmetric*: `mempalace`, `pi-devbox-environment` and `pi-extensions` - resolve to the baked `/usr/local/share/pi-devbox/skills/…`, while every other - skill resolves to the live `/workspace/skillset/skills/…`. Root cause is - precedence-by-ordering in `entrypoint-user.sh`: the baked links are created at - **line 65** (deliberately early, to close a smoke-test readiness race) with - `[ ! -e … ]` so they are "created only when absent", and the skillset deploy - runs **last** at line 387, where it classifies the existing links as - foreign and leaves them alone. The comment at line 61 states the goal as "a - same-named skillset skill … is never clobbered" — but with baked-first plus - create-when-absent, the *skillset* skill is precisely the one that loses. +- **Vendored skills no longer silently shadow their live skillset + counterparts.** `~/.agents/skills` was *asymmetric*: `mempalace`, + `pi-devbox-environment` and `pi-extensions` resolved to the baked + `/usr/local/share/pi-devbox/skills/…`, while every other skill resolved to the + live `/workspace/skillset/skills/…`. Root cause was precedence-by-ordering in + `entrypoint-user.sh`: the baked links are created **early** (line 65 in + v1.8.4; the loop moved down as this fix added comments) — deliberately so, to + close a smoke-test readiness race — with `[ ! -e … ]` so they are + "created only when absent", and the skillset deploy runs **last**, where it + classifies the existing links as foreign and leaves them alone. The comment at + line 61 claimed the goal was "a same-named skillset skill … is never + clobbered" — but with baked-first plus create-when-absent, the *skillset* skill + was precisely the one that lost. Comment now describes the actual behaviour. Observed cost, on two hosts independently: an edit to `skillset/skills/mempalace/SKILL.md` (adding a drawer-attribution rule) was pushed and present in the live clone (`md5 129bcc4752`), yet both the EMB-7KJ4VR4G and tor-ms22 containers kept loading the baked copy (`md5 5236024fef`) with zero occurrences of the new rule. The tor-ms22 agent - had to fetch the rule from the Gitea API to read it at all. Editing a - skillset skill therefore *appears* to work and silently does nothing until an - image rebuild — for exactly the three skills most likely to be iterated on, - since they are the pi-devbox-specific ones. + had to fetch the rule from the Gitea API to read it at all. Editing a skillset + skill therefore *appeared* to work and silently did nothing until an image + rebuild — for exactly the three skills most likely to be iterated on. - Proposed fix (surgical, keeps the race fix): leave the early baked links as - the fallback, and let the skillset deploy at line 387 **replace** links that - point into `/usr/local/share/pi-devbox/skills/` when it ships a skill of the - same name — i.e. baked = default, live clone = preferred when present. User - overrides (a real directory, or a symlink pointing elsewhere) must still win - over both. Verify after the change with - `readlink -f ~/.agents/skills/mempalace`, not by reading the entrypoint. + **The fix is not "the skillset always wins"**, because ownership is per-skill + (`rootfs/usr/local/share/pi-devbox/skills/VENDORED.md`): `pi-extensions`' + authoritative source is the *package* repo, copied over the snapshot at build + time, and `skillset` carries a downstream copy that can lag — handing that one + to the clone would regress the skill. So a new helper + `devbox-skill-reconcile` runs immediately after the skillset deploy and + repoints only the skills named in `skills/skillset-owned.txt` (today: + `mempalace`). Precedence is now user override → live skillset clone (owned + names only) → baked snapshot, with the early links untouched as the fallback, + so the readiness race stays closed. It only ever replaces a symlink that + points into the baked tree, so a real directory or a link pointing elsewhere + is never disturbed. Verify with `readlink -f ~/.agents/skills/mempalace`, not + by reading the entrypoint. + +- **A latent boot-abort in the baked-link block, found while reviewing the fix + above and fixed with it.** `[ ! -e "$link" ]` is TRUE for a *dangling* symlink + (`-e` follows the link), so once a link may point into `/workspace/skillset` + — which the fix above makes possible — a vanished mount turns the guard into + "create over a broken link", and plain `ln -s` then fails with `File exists`. + Under the entrypoint's `set -euo pipefail` that **aborts container start** + before `exec "$@"`, with a cryptic `ln` error and no pi. Reachable on a + `docker restart` or a host reboot under `restart: unless-stopped` (the writable + layer survives and `~/.agents` is not a volume on any host), though not on a + `compose up -d` recreate. Now `ln -sfn`, which heals the broken link back to + the baked fallback; the reconciler re-points it in the same boot if the clone + is back. A comment at the call site records why the `-f` must stay. + +- **README's skill-precedence documentation was wrong** in the same way the + entrypoint comment was: it claimed baked skills are "created only when absent + so a same-named skillset skill … is never clobbered" and that "a mounted + skillset always overrides them". Rewritten to state the real, per-skill + precedence and to name `skillset-owned.txt` and `devbox-skill-reconcile`. + +- **The smoke canary for a stale `mempalace` snapshot could not detect + staleness.** It grepped `"Shared palace: multiple harnesses"` — a phrase + present in *both* the stale and the fresh copy, so it passed throughout the + shadowing bug above. It now pins the newest section + (`"Attribute what you file yourself"`), and `VENDORED.md` records that + updating this string is part of refreshing the snapshot. Three further + assertions close the gaps that let the bug ship: skill link **targets** are + asserted (not merely `test -L`), the `skillset-owned.txt` list is asserted to + contain `mempalace` and *not* `pi-extensions`, and the reconciler's replace + path — which CI never exercises, since no smoke container mounts a skillset — + is covered by fabricating a skillset and asserting all three outcomes (owned + skill repointed, unowned skill left baked, user override untouched) — plus a + second case that a mutation test proved necessary: with the reconciler's + "is this link ours?" guard deleted, all three of those assertions still + passed, so the discriminating case is an *owned* name whose link is a user + override pointing outside the baked tree. ### Changed +- **Vendored `mempalace` skill snapshot refreshed** from `skillset` `936fed8` → + `670f7f1` (`md5 5236024fef` → `129bcc4752`), which adds the "Attribute what + you file yourself" rule: hand-filed drawers should carry + `added_by="@"`. Without this refresh the symlink fix above + would only help hosts that mount `skillset`; a bare container would still ship + the pre-attribution-rule skill. + +- **Component audit for this release — no pin edits needed.** Every component + except `pi`/`pi-atelier` is pinned to a moving ref that CI resolves at build + time, and each was checked against upstream on 2026-08-23: `pi` `0.84.2` + (still npm latest, published 2026-08-14), `pi-atelier` `v0.8.2` (newest tag; + the `≥0.7.1` floor for `pi ≥ 0.84` is satisfied), `pi-fork` `f1ff8087`, + `pi-observational-memory` `ce9fc982`, `pi-toolkit` `0e1369e6`, + `pi-extensions` `20228878`, `pi-studio` `v0.9.48` → `c3b83680` — all + **byte-identical to what v1.8.4 shipped**. `MEMPALACE_VERSION` stays `3.7.1` + (still PyPI latest, and the version the central palace serves, so no + client/server skew). The one component that moved is `mempalace-toolkit` + `fd8b15f5` → `0fe64c4`, which is this release's other payload: the feeder now + defaults `--agent` to `pi@$MEMPALACE_PI_DEVICE` so palace writes carry + provenance, with `$USER` still the fallback when the variable is unset + (`AGENT="${MEMPALACE_PI_DEVICE:+pi@${MEMPALACE_PI_DEVICE}}"`), so un-enrolled + hosts are unaffected. Nothing landed upstream after the + `pi-observational-memory` merge `ce9fc982`, so the eight-week-bug fix in + v1.8.4 is not destabilised. + + Two notes for whoever runs the build. This release changes + `entrypoint-user.sh`, `rootfs/**` and the resolved toolkit SHA — all three feed + the base-image hash — so expect a **full multi-arch base rebuild** (~95 min, + as on v1.8.3/CI 562), not a fast variant-only publish. And that rebuild + re-resolves the ~14 base-tooling `ARG *_VERSION=latest` pins; measured drift + on 2026-08-23 was one patch (`nvim v0.12.4 → v0.12.5`), so the window is + favourable, but it is not covered by version assertions. + - **`pi-devbox-environment` skill — new §2 subsection "A negative result is usually your own filter", plus ControlMaster masking in §3.** This is baked (`rootfs/usr/local/share/pi-devbox/skills/`, symlinked to @@ -80,6 +153,24 @@ Docs only so far, but one of the two files ships **inside** the image. --- +### Still open + +- `build-manifest.json` records the `mempalace-toolkit` SHA but **not** the + mempalace **core** version, so a palace bug cannot be correlated with an + image. `mempalace --version` prints it; adding it is a one-line change to the + manifest `RUN` in `Dockerfile.variant` plus one smoke assertion, and is + variant-only (no base rebuild cost). +- Smoke asserts the `pi-observational-memory` clone **exists** but not that it + contains the ambient-auth fix. npm still ships pre-fix `3.0.4`, so an + accidental switch from the `/opt` clone to an npm install would be a silent + regression. Cheap guard: `grep -rl availability_recheck` must be ≥1. +- The feeder's new `pi@` default has no behavioural test hook + (`--dry-run` never prints the agent; `--self-test` only covers the remote-mine + response classifier). Cheapest available check is a source-shape grep for + `MEMPALACE_PI_DEVICE:+pi@`. + +--- + ## v1.8.4 — 2026-08-22 Patch release, and the one that ends an eight-week bug: **the baked diff --git a/Dockerfile.base b/Dockerfile.base index 89980c9..8ebfd73 100644 --- a/Dockerfile.base +++ b/Dockerfile.base @@ -686,12 +686,14 @@ COPY rootfs/usr/local/share/pi-devbox/ /usr/local/share/pi-devbox/ COPY rootfs/usr/local/bin/studio-expose /usr/local/bin/studio-expose COPY rootfs/usr/local/bin/dot-watch /usr/local/bin/dot-watch COPY rootfs/usr/local/bin/pi-devbox-version /usr/local/bin/pi-devbox-version +COPY rootfs/usr/local/bin/devbox-skill-reconcile /usr/local/bin/devbox-skill-reconcile COPY entrypoint.sh /usr/local/bin/entrypoint.sh COPY entrypoint-user.sh /usr/local/bin/entrypoint-user.sh RUN chmod +x /usr/local/bin/entrypoint.sh /usr/local/bin/entrypoint-user.sh \ /usr/local/bin/studio-expose \ /usr/local/bin/dot-watch \ /usr/local/bin/pi-devbox-version \ + /usr/local/bin/devbox-skill-reconcile \ /usr/local/lib/pi-devbox/*.sh 2>/dev/null || true # Start as root — entrypoint adjusts UID/GID then drops to developer diff --git a/README.md b/README.md index 379a883..7b3b95e 100644 --- a/README.md +++ b/README.md @@ -547,7 +547,8 @@ directory, and they compose: `~/.agents/skills/` by `entrypoint-user.sh` on every start. They need no external mount, survive volume recreate (the source is an image path, not a home dir a named volume would shadow), and are created only when absent so a - same-named skillset skill or user override is never clobbered. The bundled + user override is never clobbered. Precedence against a mounted `skillset` repo + is per-skill, not blanket — see *Skillset repo* below. The bundled **`pi-devbox-environment`** skill is delivered this way — it teaches agents the container's persistence model, host/LAN SSH reachability, split-DNS mechanisms, the interactive-vs-tool-shell alias gotcha (`dssh`/`dscp`), @@ -558,8 +559,11 @@ directory, and they compose: pi session to read `~/.agents/skills/pi-extensions/SKILL.md` at start (to fix fork/recall under-utilisation). That pointer would dangle in a container started *without* the private `skillset` repo, so the image also bakes - fallback copies of **`pi-extensions`** and **`mempalace`**. They are - symlinked only when absent, so a mounted skillset always overrides them. The + fallback copies of **`pi-extensions`** and **`mempalace`**. Whether a mounted + skillset overrides them depends on who *owns* the skill (see *Skillset repo*): + `mempalace` is skillset-owned, so the live clone wins; `pi-extensions` is + owned by its package repo, so the baked copy keeps winning — the skillset's + copy of it is a downstream duplicate that can lag. The `pi-extensions` skill is *layered*: a committed snapshot in `rootfs/` is the floor, and `Dockerfile.variant` copies the canonical, package-owned copy from the pinned `pi-extensions` clone (`/opt/pi-extensions/skill/`) over it at @@ -573,7 +577,16 @@ directory, and they compose: - **Skillset repo (optional).** If a `skillset` repo is mounted (at `$HOME/skillset` or `/workspace/skillset`, or via `SKILLSET_CONTAINER_PATH`), `deploy-skills.sh` symlinks its skills in too. Image-baked skills are - classified as foreign-links by its `--prune-stale` pass and left untouched. + classified as foreign-links by its `--prune-stale` pass and left untouched — + which through v1.8.4 meant the baked copy *always* won, so an edit pushed to a + skillset-owned skill was invisible until the next image build. Since v1.8.5 + `devbox-skill-reconcile` runs right after the deploy and repoints the links for + skills the skillset owns, listed in + `/usr/local/share/pi-devbox/skills/skillset-owned.txt` (today: `mempalace`). + Effective precedence, highest first: **user override** (a real directory, or a + symlink pointing outside the baked tree) → **live skillset clone** (owned names + only) → **baked snapshot** (everything else, and every skill when no skillset + is mounted). Check with `readlink -f ~/.agents/skills/`. To make agents *proactively* load a baked skill at session start (rather than only on description match), the image appends a short, gated pointer to the diff --git a/entrypoint-user.sh b/entrypoint-user.sh index e546bb2..ddbc9b7 100755 --- a/entrypoint-user.sh +++ b/entrypoint-user.sh @@ -58,10 +58,17 @@ fi # the runtime skill-link assertion. Pointing at the image path (/usr/local/...) # keeps the skill fresh from the image and surviving volume recreate (unlike # anything baked under a home dir, which a named volume would shadow). Created -# only when absent, so a same-named skillset skill (deployed later, at the end -# of this script) or a user override is never clobbered; the skillset deploy -# classifies these as foreign-links and its --prune-stale pass leaves them -# alone (only dangling symlinks are pruned). +# only when absent, so a user override is never clobbered. +# +# NB: "created only when absent" does NOT hand a same-named skillset skill +# priority — the opposite. The skillset deploy runs at the end of this script +# and classifies these links as foreign, so through v1.8.4 the BAKED copy +# always won and an edit pushed to a skillset-owned skill was invisible until +# the next image build. The links below are therefore the FALLBACK only; +# devbox-skill-reconcile (invoked right after the skillset deploy) hands the +# skillset-OWNED skills back to the live clone. Ownership is per-skill, listed +# in skills/skillset-owned.txt — see VENDORED.md for why pi-extensions must +# keep losing to the baked copy. DEVBOX_SKILLS_SRC=/usr/local/share/pi-devbox/skills if [ -d "$DEVBOX_SKILLS_SRC" ]; then mkdir -p "$HOME/.agents/skills" @@ -69,7 +76,16 @@ if [ -d "$DEVBOX_SKILLS_SRC" ]; then [ -d "$_sk" ] || continue _skname=$(basename "$_sk") if [ ! -e "$HOME/.agents/skills/$_skname" ]; then - ln -s "${_sk%/}" "$HOME/.agents/skills/$_skname" + # -sfn, not -s: `[ ! -e ]` is TRUE for a DANGLING symlink (-e follows the + # link), and since v1.8.5 these links can point into /workspace/skillset + # (see devbox-skill-reconcile, invoked after the skillset deploy). If that + # mount vanishes while the writable layer survives — a `docker restart` or + # a host reboot under restart: unless-stopped, as opposed to a recreate — + # plain `ln -s` fails with "File exists" and, under `set -e`, aborts + # container start before `exec "$@"`. With -f the broken link heals back to + # the baked fallback, and the reconciler re-points it in the same boot if + # the clone is back. + ln -sfn "${_sk%/}" "$HOME/.agents/skills/$_skname" fi done fi @@ -385,6 +401,13 @@ elif [ -x /workspace/skillset/deploy-skills.sh ]; then fi if [ -n "$SKILLSET_DEPLOY" ]; then "$SKILLSET_DEPLOY" --bootstrap --prune-stale >/dev/null 2>&1 || true + # The deploy leaves the early baked links (above) in place as foreign links, + # which silently shadows the live clone for skills the skillset OWNS. Repoint + # just those; baked stays the fallback, user overrides still win. `|| true`: + # a skill-link refinement must never break container start. + if command -v devbox-skill-reconcile >/dev/null 2>&1; then + devbox-skill-reconcile "$(dirname "$SKILLSET_DEPLOY")" || true + fi fi # ── Execute command ────────────────────────────────────────────────── diff --git a/rootfs/usr/local/bin/devbox-skill-reconcile b/rootfs/usr/local/bin/devbox-skill-reconcile new file mode 100755 index 0000000..79faccf --- /dev/null +++ b/rootfs/usr/local/bin/devbox-skill-reconcile @@ -0,0 +1,91 @@ +#!/bin/sh +# devbox-skill-reconcile — hand skillset-OWNED skills back to the live clone. +# +# WHY THIS EXISTS +# --------------- +# entrypoint-user.sh links the image-baked skills into ~/.agents/skills/ EARLY +# (before pi-deploy), because the smoke readiness probe gates on markers that +# only land later, and a link created after that gate produced a flaky +# assertion. Those links are created with a `[ ! -e ]` guard — "only when +# absent" — and the skillset deploy runs LAST, treating already-present links +# as foreign and leaving them alone. Net effect through v1.8.4: the baked copy +# always won, so an edit pushed to a skillset-owned skill was invisible in +# every container until the next image build (measured on two hosts: live +# skillset md5 129bcc4752 vs baked 5236024fef, the new section absent). +# +# The fix is NOT "the skillset always wins". Ownership is per-skill (see +# rootfs/usr/local/share/pi-devbox/skills/VENDORED.md): +# +# pi-devbox-environment authored in pi-devbox → baked IS canonical +# pi-extensions owned by the package repo, copied over the snapshot +# at build time; skillset carries a DOWNSTREAM copy +# that can lag → baked must keep winning +# mempalace owned by the skillset repo; baked is a snapshot +# fallback for containers with no skillset mounted +# → the live clone must win when it is present +# +# So only skills listed in skills/skillset-owned.txt are handed over. Baked +# links stay as the fallback (the early-link race fix is untouched), and a user +# override always beats both: a real directory is never replaced, and neither is +# a symlink that already points somewhere other than the baked tree. +# +# Usage: devbox-skill-reconcile [skills-dir] [baked-src] +# skillset-root the mounted skillset repo (contains skills//) +# skills-dir default $HOME/.agents/skills +# baked-src default /usr/local/share/pi-devbox/skills +# +# Idempotent, and silent unless it changes something. Exits 0 when there is +# nothing to do (no skillset, no list) so the entrypoint never fails on it. +set -eu + +SKILLSET_ROOT="${1:-}" +SKILLS_DIR="${2:-$HOME/.agents/skills}" +BAKED_SRC="${3:-/usr/local/share/pi-devbox/skills}" +BAKED_SRC="${BAKED_SRC%/}" # a trailing slash would make the prefix + # match below ("$BAKED_SRC"/*) match nothing + +[ -n "$SKILLSET_ROOT" ] || exit 0 +[ -d "$SKILLSET_ROOT/skills" ] || exit 0 +[ -d "$SKILLS_DIR" ] || exit 0 + +# Absolutise BOTH roots before they are used, because each has its own way of +# failing silently when relative: a relative symlink TARGET is resolved against +# the link's directory (~/.agents/skills), not $PWD, so it would dangle on +# creation; and a relative BAKED_SRC would never prefix-match the absolute +# target that `readlink` reports, so every skill would be skipped and the fix +# would look like it had simply done nothing. +SKILLSET_ROOT=$(CDPATH= cd -- "$SKILLSET_ROOT" 2>/dev/null && pwd) || exit 0 +BAKED_SRC=$(CDPATH= cd -- "$BAKED_SRC" 2>/dev/null && pwd) || exit 0 +OWNED_LIST="$BAKED_SRC/skillset-owned.txt" +[ -f "$OWNED_LIST" ] || exit 0 + +while IFS= read -r _line || [ -n "$_line" ]; do + # strip comments and surrounding whitespace; skip blanks + _name=$(printf '%s\n' "$_line" | sed -e 's/#.*$//' -e 's/^[[:space:]]*//' -e 's/[[:space:]]*$//') + [ -n "$_name" ] || continue + # defensive: a list entry must be a plain skill name, never a path + case "$_name" in */*|.*) continue ;; esac + + _live="$SKILLSET_ROOT/skills/$_name" + _link="$SKILLS_DIR/$_name" + + # the skillset does not ship it → the baked fallback is all there is + [ -d "$_live" ] || continue + # a real directory is a user override → never touch + [ -L "$_link" ] || continue + + # only ever replace OUR OWN link. readlink is deliberate: `readlink -f` + # would resolve a link that already points into the skillset clone and, + # since both trees hold a same-named skill, could not tell them apart. + _target=$(readlink "$_link" 2>/dev/null || true) + case "$_target" in + "$BAKED_SRC"/*|"$BAKED_SRC") ;; # baked link → ours to replace + *) continue ;; # user/foreign target → leave alone + esac + + # -n so an existing symlink-to-directory is replaced rather than followed + # (without it, ln would create $_link/$_name inside the baked tree). + if ln -sfn "$_live" "$_link" 2>/dev/null; then + printf 'skill %s: baked snapshot -> live skillset (%s)\n' "$_name" "$_live" + fi +done < "$OWNED_LIST" diff --git a/rootfs/usr/local/share/pi-devbox/skills/VENDORED.md b/rootfs/usr/local/share/pi-devbox/skills/VENDORED.md index 17d952a..ac9c3f6 100644 --- a/rootfs/usr/local/share/pi-devbox/skills/VENDORED.md +++ b/rootfs/usr/local/share/pi-devbox/skills/VENDORED.md @@ -1,9 +1,10 @@ # Vendored fallback skills Most directories here are **image-baked skills** that `entrypoint-user.sh` -symlinks into `~/.agents/skills/` on container start (only when a skill of the -same name is not already present, so a mounted `skillset` repo or a user -override always wins). +symlinks into `~/.agents/skills/` on container start. They are the **fallback** +layer: see *Runtime precedence* below for which copy actually wins when a +`skillset` repo is mounted (through v1.8.4 the answer was "always the baked +one", which was a bug). | skill | owner | how it gets here | |-------|-------|------------------| @@ -38,6 +39,35 @@ its skill file needed baking. *different* skill, `opencode-mempalace-bridge`), so there is no public package source to copy from. This snapshot is refreshed manually per release. +## Runtime precedence (v1.8.5+) + +The baked links are created **early** in `entrypoint-user.sh` (before pi-deploy, +to close a smoke readiness race) with a create-only-when-absent guard, and the +skillset deploy runs **last** and treats them as foreign links. Through v1.8.4 +that combination meant the baked snapshot always won: an edit pushed to +`skillset/skills/mempalace/SKILL.md` was invisible in every container until the +next image build (measured on two hosts — live `md5 129bcc4752` vs baked +`5236024fef`, new section absent). Editing those skills *appeared* to work. + +`devbox-skill-reconcile` now runs immediately after the skillset deploy and +repoints the links for skills the **skillset owns**, listed one per line in +`skillset-owned.txt`. Precedence, highest first: + +1. **user override** — a real directory, or a symlink pointing outside the baked + tree; never touched by anything +2. **live skillset clone** — but only for names in `skillset-owned.txt` +3. **baked snapshot** — everything else, and every skill when no skillset is + mounted + +Ownership is per-skill on purpose: `pi-extensions`' authoritative source is the +package repo (copied over the snapshot at build), and `skillset` carries a +downstream copy that can lag, so handing it to the clone would *regress* the +skill. Only `mempalace` is skillset-owned today. + +Verify with `readlink -f ~/.agents/skills/` — not by reading the +entrypoint. Smoke covers both directions (baked resolution with no skillset +mounted, plus a fabricated-skillset run of the reconciler). + ## Refreshing the snapshots cp /skill/SKILL.md pi-extensions/SKILL.md @@ -50,4 +80,9 @@ 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. -Snapshot provenance at last refresh: skillset `936fed8`, pi-extensions pkg `e73cb9f`. +Snapshot provenance at last refresh: skillset `670f7f1`, pi-extensions pkg `e73cb9f`. + +When you refresh the `mempalace` snapshot, also update the phrase asserted by +the "mempalace skill snapshot is current" smoke test — it deliberately pins the +**newest** section, because the previous canary grepped a phrase that survived +the very edit that made the snapshot stale, and so passed on stale content. 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 e97e6eb..3f809d2 100644 --- a/rootfs/usr/local/share/pi-devbox/skills/mempalace/SKILL.md +++ b/rootfs/usr/local/share/pi-devbox/skills/mempalace/SKILL.md @@ -293,6 +293,7 @@ Zechner's pi-coding-agent). Implications: When the palace is **central** (shared across machines), five more things apply: - **Check which machine a conversation came from.** Transcripts are fed per device, so `source_path` reads `…/mempalace-feed//pi_.jsonl` while the displayed `source_file` is only the basename. One search can legitimately return hits from several machines at once — look at the device segment before attributing a decision to *this* project. +- **Attribute what you file yourself.** Drawers now carry `device` and `agent_kind` metadata (plus `device_source`/`agent_kind_source` recording *how* each was determined, so an inference is never mistaken for a fact). Mined content gets these for free — the inbox path gives the device, the filename shape gives the harness — and a timer on the palace host re-stamps hourly, because live re-mining replaces metadata rows and silently drops earlier stamps. But for anything **you** file by hand, the only signal is what you pass: set `added_by="@"` (e.g. `pi@emb-7kj4vr4g`, from `$MEMPALACE_PI_DEVICE`) on `add_drawer`/`checkpoint`/`mine`. Skip it and your drawer joins the ~16k historic `/workspace` project mines that are permanently unattributable, because `/workspace` exists identically on every devbox. Note the palace preserves `source_file` in full (see `source_path`) but *displays* only the basename — so a device prefix there survives storage even though it looks stripped. - **Mined drawers carry the MINE date, not the session date.** When history is imported, or re-mined on the palace host, `filed_at`/`created_at` is the *import* time — so sorting by them does not give chronological order. Real session time is recoverable from the UUIDv7 in `pi_.jsonl`: the first 12 hex digits are milliseconds since the epoch (and UUIDv7 sorts lexicographically in time order, so a plain filename sort is already chronological). Agent-authored drawers and diaries have no such backdoor — for those `filed_at` is the only chronology, which is why it must never be restamped. - **Beware the timezone mismatch when you combine those.** Palace `filed_at`/`created_at` are naive timestamps in the palace host's local time, while a UUIDv7 decodes to UTC. Comparing them directly introduces a silent offset (2 h for a CEST host). Normalise before drawing conclusions about ordering. - **`agent_name` is not device-scoped.** `mempalace_diary_read(agent_name="pi")` returns *every* machine's `pi` diary, interleaved. Read the entry before assuming it is your own history. diff --git a/rootfs/usr/local/share/pi-devbox/skills/skillset-owned.txt b/rootfs/usr/local/share/pi-devbox/skills/skillset-owned.txt new file mode 100644 index 0000000..0362e49 --- /dev/null +++ b/rootfs/usr/local/share/pi-devbox/skills/skillset-owned.txt @@ -0,0 +1,16 @@ +# Skills in this directory whose OWNER is the skillset repo. +# +# Read by devbox-skill-reconcile, which runs after the skillset deploy in +# entrypoint-user.sh: for each name below, if the mounted skillset ships a +# skill of that name, the baked link in ~/.agents/skills/ is repointed at the +# live clone. The baked copy remains the fallback for containers started +# WITHOUT a skillset mount, and a user override always wins over both. +# +# Add a name here ONLY if the skillset repo is the authoritative source (see +# the ownership table in VENDORED.md). Do NOT add: +# pi-devbox-environment — authored in this repo; baked IS canonical +# pi-extensions — owned by the pi-extensions package repo and copied +# over the snapshot at build time; the skillset copy +# is a downstream duplicate that can lag, so letting +# it win would regress the skill. +mempalace diff --git a/scripts/smoke-test.sh b/scripts/smoke-test.sh index fab4522..b743a9d 100755 --- a/scripts/smoke-test.sh +++ b/scripts/smoke-test.sh @@ -235,6 +235,16 @@ run "image-baked mempalace fallback skill" \ # baked copy must be the fresh package copy (Option 1), not the stale snapshot. run "pi-extensions skill refreshed from package when present" \ "if [ -f /opt/pi-extensions/skill/SKILL.md ]; then cmp -s /opt/pi-extensions/skill/SKILL.md /usr/local/share/pi-devbox/skills/pi-extensions/SKILL.md; else true; fi" +# Runtime ownership handover (v1.8.5): the baked links are a FALLBACK, and +# skillset-OWNED skills must be repointed at the live clone when one is mounted. +# The list is data, so assert its content, not just its presence: mempalace in, +# pi-extensions deliberately out (its skillset copy is a lagging duplicate). +run "devbox-skill-reconcile helper present + executable" \ + "test -x /usr/local/bin/devbox-skill-reconcile" +run "skillset-owned list ships and names mempalace" \ + "grep -qx 'mempalace' /usr/local/share/pi-devbox/skills/skillset-owned.txt" +run "skillset-owned list excludes pi-extensions (ownership)" \ + "! grep -qx 'pi-extensions' /usr/local/share/pi-devbox/skills/skillset-owned.txt" # ── tmux 0-indexing (required for pi-studio variants) ───────────────── echo "" @@ -369,10 +379,50 @@ exec_test "pi-devbox-environment skill linked" 'test -L $HOME/.agents/skills exec_test "pi-extensions skill linked (fallback)" 'test -L $HOME/.agents/skills/pi-extensions && test -f $HOME/.agents/skills/pi-extensions/SKILL.md && echo ok' exec_test "mempalace skill linked (fallback)" 'test -L $HOME/.agents/skills/mempalace && test -f $HOME/.agents/skills/mempalace/SKILL.md && echo ok' # The vendored mempalace snapshot is refreshed MANUALLY per release (see -# rootfs/usr/local/share/pi-devbox/skills/VENDORED.md). It silently shadows the -# skillset copy in a devbox container, so a stale snapshot is invisible: assert -# the multi-machine shared-palace guidance is actually present, not just the file. -exec_test "mempalace skill snapshot is current" 'grep -q "Shared palace: multiple harnesses" $HOME/.agents/skills/mempalace/SKILL.md && echo ok' +# rootfs/usr/local/share/pi-devbox/skills/VENDORED.md). Through v1.8.4 it also +# silently SHADOWED the live skillset copy, so staleness was invisible — and the +# canary that was supposed to catch it could not: it grepped "Shared palace: +# 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' +# 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)" \ + 'for s in mempalace pi-extensions pi-devbox-environment; do + case "$(readlink -f $HOME/.agents/skills/$s)" in + /usr/local/share/pi-devbox/skills/$s) ;; + *) echo "$s resolves to $(readlink -f $HOME/.agents/skills/$s)" >&2; exit 1 ;; + esac + done; echo ok' +# The handover path itself. CI never mounts a skillset, so without this the +# v1.8.5 fix would ship untested: fabricate a skillset + a skills dir holding +# baked-style links, run the reconciler, and assert all three outcomes — +# owned skill repointed, unowned skill left baked, user override untouched. +exec_test "reconciler: owned skill handed to live clone, others untouched" \ + 'set -e; t=$(mktemp -d); mkdir -p $t/ss/skills/mempalace $t/ss/skills/pi-extensions $t/skills + echo LIVE > $t/ss/skills/mempalace/SKILL.md; echo LIVE > $t/ss/skills/pi-extensions/SKILL.md + ln -s /usr/local/share/pi-devbox/skills/mempalace $t/skills/mempalace + ln -s /usr/local/share/pi-devbox/skills/pi-extensions $t/skills/pi-extensions + mkdir -p $t/skills/mine; echo MINE > $t/skills/mine/SKILL.md + devbox-skill-reconcile $t/ss $t/skills >/dev/null + devbox-skill-reconcile $t/ss $t/skills >/dev/null # idempotent + [ "$(readlink $t/skills/mempalace)" = "$t/ss/skills/mempalace" ] || { echo "owned skill NOT repointed" >&2; exit 1; } + [ "$(readlink $t/skills/pi-extensions)" = /usr/local/share/pi-devbox/skills/pi-extensions ] || { echo "unowned skill was repointed" >&2; exit 1; } + [ "$(cat $t/skills/mine/SKILL.md)" = MINE ] || { echo "user override clobbered" >&2; exit 1; } + rm -rf $t; echo ok' +# The case above cannot fail if the reconciler stops checking WHERE a link +# points — a mutation test showed all three of its assertions still passing with +# that guard deleted, which is the same false-green shape as the old snapshot +# canary. This one discriminates: an OWNED name (so it is considered) whose link +# is a user override pointing outside the baked tree (so it must be left alone). +exec_test "reconciler: user override on an owned name is left alone" \ + 'set -e; t=$(mktemp -d); mkdir -p $t/ss/skills/mempalace $t/skills $t/mine-skill + echo LIVE > $t/ss/skills/mempalace/SKILL.md; echo USERLINK > $t/mine-skill/SKILL.md + ln -sfn $t/mine-skill $t/skills/mempalace + devbox-skill-reconcile $t/ss $t/skills >/dev/null + [ "$(cat $t/skills/mempalace/SKILL.md)" = USERLINK ] || { echo "user symlink override clobbered" >&2; exit 1; } + rm -rf $t; echo ok' # mempalace-census gained a /usr/local/bin symlink in v1.8.3; its three siblings # had one since they were added, so this asserts the set stays complete. exec_test "mempalace-census on PATH" 'command -v mempalace-census >/dev/null && mempalace-census --help >/dev/null && echo ok'