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.
This commit is contained in:
+117
-26
@@ -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="<harness>@<device>"`. 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@<device>` 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
|
||||
|
||||
Reference in New Issue
Block a user