Uh oh!
There was an error while loading. Please reload this page.
Prune retired client assets that HypAware itself installed (#726) - #745
Conversation
…l record (#726) `materializeClientAssets` only ever copied. A version that stops contributing a skill deleted the source and left the installed copy running: five cases after LLP 0212/0215, on top of `hypaware-sensitive-scan` from LLP 0142 (#660). The installed `hypaware-ignore` still carries the stale 8787 fallback and the missing LLP 0066 R9 caveat that were the reasons for retiring it. The hard part is the evidence, not the delete. "No plugin declares this name" says nothing: a skill the user wrote is absent from the registries exactly like a retired one, and a directory under ~/.claude/skills carries no provenance. LLP 0138 #marker-undo settled this for the org-driven half (the attach marker records installed_assets) and named the hole in the same sentence: manual `hyp skills install` copies "record no marker". So the materializer now keeps that record for every install path, in <HYP_HOME>/hypaware/client-assets.json, with a content digest per destination, and removes a recorded path this version's plan no longer contains. Four conditions gate every removal: we recorded writing it; the plan does not contain it; it sits strictly inside that client's own asset directories; and the bytes are still the bytes we wrote. A digest mismatch is positive evidence the user took the file over, so it is named and left alone. The client scope is what landed this run, never what was asked for, so a boot whose plugins failed to load prunes nothing. Automatic, not confirmed, and not silent: removing a byte-identical copy of a file we wrote destroys nothing the user authored, three of the four callers have nobody at a terminal, and every removal and withheld removal is printed and logged. The attach marker is read as a second evidence source, which is the only one reaching back before this ledger existed: a fleet machine self-heals, a stand-alone one covers every future retirement after one install cycle. Machines carrying a pre-existing manual install of an already-retired asset keep it; the one-time migration that would need a shipped record of historical digests is deliberately not decided here. Decision recorded in LLP 0218; LLP 0138 gains the Extended-by forward ref. `attach-endpoint-drift` asserted the old leave-behind on a withdrawn plugin and now asserts the removal, with the marker union unchanged.
LLP 0218 was minted independently on both this branch (retired client assets are pruned) and fix/issue-739 (compaction failed attempt reported). Per LLP 0156, the number goes to whichever document reaches master first; fix/issue-739 is already merged, so this branch's document renumbers to 0219, the next free number across master and every remote branch. Renames llp/0218-retired-client-assets-are-pruned.decision.md to llp/0219-*, and updates the doc's own header, the Extended-by line on LLP 0138, the three @ref LLP 0218 annotations in src/core/runtime/client_assets.js and client_asset_ledger.js, and the two non-@ref textual mentions in types.d.ts and test/core/attach-endpoint-drift.test.js. No other content changed.
Round 1 of review proved the four-condition safety predicate did not apply to the whole candidate set, and reproduced two deletions of user work. Every change here narrows what the prune touches. The digest gate is now `record?.digest !== digest`, so a candidate with no recorded digest is withheld and reported instead of deleted. That covers the marker-sourced candidate (the marker names paths, never bytes), the record written by a copy whose digest failed transiently, and the record whose digest field is corrupt. Two reproduced deletions go away with it: a hand-edited pre-ledger install of `hypaware-ignore`, and a skill the user authored themselves at a name `installed_assets` still carries because that list is unioned across every rewrite and never shrinks. The consequence is honest in the doc: the attach marker is now a reporting source, not a deletion source, and the fleet self-heal is gone. Restoring it would need `installed_assets` to shrink on a successful prune, which changes what LLP 0138 settled, so it is recorded as an open question in LLP 0219 and not attempted. Belt-and-braces on the same failure: `readClientAssetLedger` now drops a record whose `digest` key is present but not a non-empty string, rather than keeping the record and silently discarding the field. The "not in this run's plan" check is over the whole run's plan, not one client's share of it. Destinations are physical paths and both the `claude` and `claude-desktop` manifests declare `.claude/skills`, so asked per client a path one client is contributing right now reads as another client's retired copy: one run printed `installed skill 'hypaware-privacy'` and `removed retired skill 'hypaware-privacy'` for the same path. The per-client map stays for the failed-copy carry loop, which is genuinely per-client. The "failed plugins prune nothing" guard held only for total failure, which is not what the loader produces: `activatePlugins` catches per plugin and `bootKernel` returns normally, so one plugin throwing leaves the client in scope with its assets missing from the plan and indistinguishable from retired ones. `failedPlugins` is threaded from boot to every materialize call site (CLI dispatch, wizard finale, manual attach, the daemon reconciler) and stands the whole prune down. A containment refusal during prune returned before `removeClientAssets` and so fired none of its reporting: ledger and marker entries naming `$HOME/.ssh`, `/` and the skills dir itself were correctly refused and produced zero output. They now warn on stderr and emit `client_assets.prune_refused`. LLP 0219 is corrected throughout: `#automatic-not-gated` no longer rests on a false exclusion claim, `#marker-is-evidence` describes a reporting source, the fail-safe claim moves to the new `#incomplete-activation-prunes-nothing`, and the "can only ever remove less" claim is now true of both directions.
philcunliffe
commented
Aug 13, 2026
Review round 1 of The reviewer built hostile fixtures and drove the real install path twice against a temp home, mirroring an in-place upgrade. Most of the predicate held: traversal, absolute paths, 1. blocker - marker-sourced candidates carried no digest, so the gate protecting user content was absent. FIXED
Two reproductions:
This is precisely what the PR body promised could not happen. LLP 0219 did name the exposure, but justified it as "the same exposure detach has always had" - and that justification is wrong: detach is a one-shot removal the user explicitly asked for, while prune fires automatically on every install against a list that never shrinks. A permanent trap on a fixed set of path names, not the same exposure. Fixed with 2. blocker - a missing or wrong-typed digest failed OPEN. FIXED
This contradicted the module's own stated invariant ("an unreadable ledger must never widen what gets deleted"), LLP 0219's "can only ever remove less", and the type contract's "a missing digest never reads as a match". Reachable without corruption: a transient Fixed by the same gate change, plus the reader now drops a record whose 3. major - the plan check was per-client while destinations are physical paths. FIXEDBoth the Not reachable with today's shipped manifests, but one manifest line away, and it silently deletes a currently-contributed asset. Fixed by building the filter from the whole run's plan. 4. major - the fail-safe guard only held for total activation failure. FIXED
No user data is lost - a re-run after fixing the plugin restores the copies - but a transient fault silently strips a client's skill surface, and it falsified the very guard LLP 0219 called "the guard that makes the whole mechanism fail safe". Fixed: the prune stands down entirely when any plugin failed to activate, threaded from 5. minor - a containment refusal was completely silent. FIXEDLedger and marker entries naming The consequence, stated plainlyThe marker is now a reporting source, not a deletion source, and the fleet self-heal this PR originally claimed is gone. Machines that installed a retired asset before this ships keep their copy - manual and org-driven alike - and get a report instead. Restoring the self-heal would need Every LLP 0219 claim these findings falsified has been rewritten, including the EvidenceIndependently re-derived: with the two source files reverted to the PR head and the new tests kept, 7 of 12 fail, including both blocker reproductions with Suite 4006 pass / 0 fail / 1 pre-existing skip; typecheck clean; Also checked, clean
The head has moved to |
…throw (#726) Round 2 review of the retired-client-asset prune. Four narrowings, each one making the delete path do strictly less. The stand-down only covered plugins whose `activate()` threw, because both call sites re-derived it from `boot.activations`. Three other routes take a selected plugin out of a boot's plan without ever reaching that array: a dep-graph elimination for an unsatisfied `requires`, a boot profile that drops a config-enabled opt-in (`all-available`, what `hyp init` boots), and a manifest that would not load. With the shipped plugin set that is a real flap: `hyp attach claude` installs and ledgers `@hypaware/gascity`'s skill, the next `hyp init` finds it in the ledger, absent from the plan, inside `.claude/skills` with a matching digest, and deletes it. `bootKernel` now returns one `unavailablePlugins` list covering all four, and both call sites read it. The profile term is intersected with what the config enables, which is what keeps an ordinary `config` boot from standing the prune down forever. The digest had no type domain, so a user-created object could match a recorded one: an empty directory and an empty file both hashed to `e3b0c442...`, and a skill tree holding one `SKILL.md` of `body\n` hashed exactly like a file whose bytes are `SKILL.md\nbody\n`. The shape is now seeded before any content, each tree entry's shape leads its path, and a candidate whose shape contradicts its record's `kind` is refused outright. The wizard finale withholds `stdout` so per-copy lines do not bury its step summary, which made it the one call site with a human at the terminal and the only one that deleted in silence, while the candidates it declined to delete stayed visible on stderr. `materializeClientAssets` now returns what it pruned and withheld alongside what it installed, and the finale counts the removals. Finally, a ledger record was lost when a dest moved between two clients sharing an asset directory and the new client's copy failed: the carry loop asked this client's plan where the candidate loop asks the whole run's, so the dest fell through both and the copy on disk became permanently unprunable and unreportable. The carry now asks the same plan the candidates do. LLP 0219 updated to match: #incomplete-activation-prunes-nothing enumerates the four routes and drops its claim that "every way" already ended in prune nothing, #edited-assets-are-not-ours states the shape separation, #automatic-not-gated states counts on the finale and paths elsewhere, and #prune-on-materialize covers the record carry.
philcunliffe
commented
Aug 13, 2026
Review round 2 of 1. major - the stand-down only covered plugins whose |
philcunliffe
commented
Aug 13, 2026
Triage after the review budget (LLP 0017). Two rounds ran, nine findings, two of them blockers that deleted user work, and one blocker that came back in a different form after being fixed. Judged mergeable. Hardening items in #746. Given that history I asked the triage not to re-read the review records but to try to find a tenth finding itself. It built 14 hostile fixtures against this head. What it tried, and what heldSymlink swap (user moves the skill out and symlinks the dest back) - symlink pruned, moved copy survives. A user file added inside an installed skill dir - withheld. Both empty-file/empty-dir cross-kind collisions, round 2's exact bug, both directions - withheld. A hard-linked user copy inside a retired skill - content survives. A marker-only candidate over a user-authored skill, built against the real It also learned something by accident worth recording: six of its first ten fixtures were vacuous, because a second run with an empty registry never reaches the digest gate at all - the scope guard stands the prune down before any candidate is examined. That is a fail-safe nobody had verified deliberately. The one that landed, and why it is not a blockerA corrupt ledger record naming a subpath of a live skill, carrying a valid domain-separated digest of that subtree, is deleted. Reaching it requires a writer inside Has this had enough eyesThe finding history, read carefully, supports shipping rather than undermining it. Rounds 1 and 2 share one shape: the gate was right and its scope was wrong - marker candidates skipped the gate, the stand-down covered one of four doors, the plan check covered one of two clients. Every fix collapsed a scope derivation into a single shared source: one digest gate every candidate passes through ( That structural change is why the third-round hunt behaved differently. Ten distinct routes to a wrongful delete were probed; the only one that landed required forging HypAware's own evidence from inside its own trust domain. A third review round would find more findings - it found these - but they are now of the "narrower, self-healing, same-trust-domain" species rather than the "deletes user-authored work" species. The asymmetry, enumeratedWhat it now declines to delete: pre-ledger installs (report only, forever), marker-only candidates, digest missing or mismatched or unreadable, shape mismatches, anything during a partial boot by any of four routes, anything for a client with no landed copy, and everything when the ledger is unreadable. All recoverable by hand or by the named report. What it could still wrongly delete: same-trust-domain corruption with a valid digest, or a millisecond check-then-act race. Every failure mode constructible falls on the under-delete side. On the LLP 0219 in-place editJudged right as done. The doc was created inside this same unmerged PR and corrected in the same commits as the code, which is what "land the doc edit in the same commit as the code" asks for. "Accepted docs are settled" protects the merged record; a doc that has never been on master is still part of the change under review. Shipping false claims and immediately minting a superseding LLP would be ceremony that leaves untrue statements in the permanent record. Once merged, it is settled. |
Uh oh!
There was an error while loading. Please reload this page.
…nreadable as gone (#746) (#749) * Narrow the retired-asset prune to direct children, and stop reading unreadable as gone (#746) Three narrowings and two recorded residuals from the ship review of PR #745, none of them reachable as user-data loss on the shipped head. - `isRemovableAsset` now admits only a direct child of a client asset directory, which is the exact shape the copy side writes. A corrupt ledger record naming `<base>/<dir>/subdir` with a matching digest could otherwise drive a recursive delete of a subtree nobody installed. - A candidate whose digest cannot be read is no longer mistaken for one that is already gone. Only ENOENT drops the record in silence; every other errno carries the record forward verbatim (no digest re-taken) and reports the path withheld with a `digest_unreadable` kind. - A config-enabled plugin whose directory is wholly absent is taken as retired, which is the intended reading rather than a fifth stand-down door. Recorded in LLP 0219 and pinned by a test. LLP 0219 also gains the two doc-only residuals the review asked for: the wizard finale prunes on the pre-wizard config, and the digest-check-then-rm race. Co-Authored-By: Claude <noreply@anthropic.com> * Round-1 hardening: direct-child containment conjunct, split ENOENT scoping (#746) Closes the six round-1 review findings on #749: - isRemovableAsset now keeps isWithinDir as a conjunct alongside the dirname check, so a basename beginning with ".." (a prefix the copy side's containment check already refuses) is not widened back in by the dirname-only direct-child check. - inspectClientAsset splits the top-level fs.stat probe into its own try/catch, so an ENOENT raised while walking or reading below dest can never be misread as dest itself being gone. - LLP 0223 is minted to carry the #only-direct-children and #unreadable-is-not-absent narrowings out of the Accepted LLP 0219, which now carries an Extended-by forward-ref and stubs pointing at the new doc; #uninstalled-is-retired gains one sentence on transient absence. - The cannot-be-read test's chmod(0o000) now registers its t.after restore immediately, before any assertion can leave a mode-000 directory behind. Every new/changed test was verified to fail against the pre-fix code and pass against the fix: the ".." basename tests fail by actually deleting the user's file/directory pre-fix (not just an assertion mismatch), and the new inspectClientAsset test mocks a readFile ENOENT below dest (a literal dangling symlink at dest turns out to be caught by the top-level stat in both old and new code, so it does not discriminate the fix) and fails pre-fix by silently dropping the ledger record. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: test <test@test.com> Co-authored-by: Claude <noreply@anthropic.com>
…says why Review of #893 found the new `@ref` asserting the opposite of the section it cited. LLP 0219 #prune-on-materialize is Accepted and settles the prune's second condition as "the whole run's plan", and the carry as "asked of the whole run's plan too". #893 widens both to every client's contributions, which changes what 0219 decided rather than restating it, so it is recorded as its own decision extending 0219 (the shape LLP 0226 already used for the two narrowings from #745's ship review), with an `Extended-by:` forward-ref on 0219 and the code's `@ref` repointed at the new anchor. The `stderr`-less widened plan also carried a false justification: it claimed every warning `planClientAssets` writes had already been written by the run's own plan. It had not. `planClientAssets` filters by `wanted` before the unknown-client check, so a contribution naming a client outside a scoped run's filter warns in neither pass. Suppressing is still right (an explicit client list never warns about what it excluded, and `--client all` still surfaces them), and the comment now says that instead, so a later reader does not "fix" it by re-attaching `stderr`. LLP 0266 #open-stale-carried-digest records the residual the review also found: a carried record's digest is never refreshed, so a dest another client rewrites can later be withheld from pruning with a message blaming the user for HypAware's own rewrite. Pre-existing, reachable by one more route after this widening, and not closed here because both candidate fixes change what the ledger is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hyp skills installnow removes a skill or agent that HypAware itself installed and that no currently-loaded manifest still declares. After PR #720 retired five assets, an in-place upgrade left them all on disk and model-invocable - including the driftedhypaware-ignorewith the stale8787port that motivated retiring it.This deletes files from a user's home directory, so the safety criteria are the substance
Four sources of ownership evidence were considered:
hypaware-*) - rejected. Retiring or renaming a bundled skill does not remove it from machines that already installed it #660 already ruled it out and a user can author that name. A test asserts a hand-made~/.claude/skills/hypaware-graphsurvives.installed_assets- evidence a path was ours, but not evidence the bytes still are. It is written only by the reconciler's attach handler, andaction_reconciler.jsunions it across every rewrite so paths from older versions survive upgrades. It already drivesfs.rm -rfon detach. Buthyp skills install, manualhyp attachand the walkthrough finale write no marker at all (LLP 0138 #marker-undo says so). So it covers fleet machines and nothing else.A path is removed only when all four hold: HypAware's own record (the new ledger, or the attach marker) says it wrote that path; this run's plan does not contain it; it sits strictly inside that client's own asset dirs (checked twice, including the pre-existing containment refusal); and the recorded content digest still matches what is on disk.
Two further guards worth naming. The client scope is derived from what landed this run, so a boot whose plugins failed to load prunes nothing at all - the failure mode is "prune less", never "prune more". And the ledger is anchored on the home directory the assets went to, so one home's record can never drive a delete in another (which also keeps the test suite away from a developer's real
~/.claude).A hand-edited asset is never deleted (corrected in review - see below; this was false as originally written for marker-sourced candidates). The digest mismatch stops the removal and turns it into a stderr line plus a
client_assets.prune_withheldsignal. The asymmetry with the copy path is deliberate: overwriting a contributed asset's edits is documentedhyp skills installbehaviour and the source is right there to re-copy from, but a retired asset has no source left, so the delete is unrecoverable. A directory the user has taken ownership of is caught by the same mechanism - taking it over is exactly what the digest measures.Automatic, not gated - and why
The issue left this open. Automatic, and reported.
Removing a byte-identical copy of a file HypAware itself wrote destroys nothing the user authored, and everything the user did author is already excluded by the digest gate - so there is no question left for a prompt to ask. Three of the four call sites (walkthrough finale, reconciler attach, org-driven install) have nobody at a terminal, and this path has no confirmation pattern; a rule that holds on one caller in four is the drift LLP 0138 collapsed four loops to prevent.
Not silent: every removal prints
removed retired <kind> '<name>' -> <dest>and emitsclient_assets.pruned; every withheld one prints and emitsclient_assets.prune_withheld. That is the reporting half #660 asked for.Evidence
Tests written first. Independently re-derived by the reconciler by reverting the five changed source files to
masterand removing the new module, keeping the tests:After: 6/6. Test 6 - "nothing is removed when this run installs nothing" - passes pre-fix and post-fix, which is what makes it a guard rather than a driver. Tests 3 and 4 are the over-removal guards and they fail pre-fix only on their anti-inertia assertions, which is the point.
Full suite 4000 pass / 0 fail / 1 pre-existing skip; typecheck clean;
llp-ref-hygiene11/11. Smokesclient_attach_idempotentandcli_bundled_plugins_activatedok.walkthrough_picker_to_first_queryfails, and that is pre-existing - verified by stashing every change and re-running on the pristine base, where it fails identically.What this does NOT close, and needs your disposition
Retroactive cleanup of assets installed before this ships. A machine that installed
hypaware-ignoreviahyp skills installhas no ledger entry and no marker, so the only remaining criterion would be a name-shaped guess - which #660 already rejected. Fleet machines self-heal via the attach marker (tested); a stand-alone machine covers every future retirement after one install cycle, but the existing five cases stay.The one-time migration that would fix them everywhere needs a shipped record of historical content digests, which is a real design question with real maintenance cost. It is recorded in the new LLP as explicitly not decided rather than invented under a bugfix.
One behaviour change to an existing test, flagged
test/core/attach-endpoint-drift.test.jsasserted "the withdrawn skill is still on disk" when an org withdraws a plugin. It now asserts removal. That assertion recorded the gap rather than a decision - LLP 0138 #marker-undo's union exists so the record is not lost, and the record is untouched (the adjacentinstalled_assetsassertion still passes unchanged).LLP
New LLP 0219 (Decision), with
Extended-by:added to LLP 0138. Originally minted as 0218 and renumbered before this PR opened: PR #741 had already claimed 0218 and has since merged, so per LLP 0156 the later claimant renumbers. Verified 0219 is unique across every remote branch, andllp-ref-hygiene's duplicate-number check passes.Refs #726, #660
Corrected in review round 1 (head
f61cae4)Two blockers: the digest gate did not cover the whole candidate set, and this PR as originally written deleted user-authored files.
Marker-sourced candidates carried no digest, and the gate short-circuited on
record?.digest &&, so they fell straight through tofs.rm -rf. Sinceinstalled_assetsis unioned across every rewrite and never shrinks, once a path appeared there it was a deletion candidate forever. Two reproductions: a hand-edited pre-ledgerhypaware-ignore(exactly the user #726 describes) was deleted; and after a prune, a skill the user later authored at that same name was deleted on the next install - nothing HypAware ever wrote, unrecoverable.Separately, a record whose
digestwas missing or wrong-typed failed open, becoming an unconditional delete - reachable without corruption, since a transientdigestClientAssetfailure records no digest.Both are fixed by making the gate
record?.digest !== digest, so absence of evidence can never read as the evidence we wanted.The consequence, stated plainly: the marker is now a reporting source, not a deletion source, and the fleet self-heal this PR originally claimed is gone. Machines that installed a retired asset before this ships keep their copy - manual and org-driven alike - and get a report instead. Restoring the self-heal needs
installed_assetsto shrink on a successful prune, which changes what LLP 0138 settled; it is recorded as an open question in LLP 0219 and deliberately not attempted here.Two further majors, both narrowing: the plan check is now whole-run rather than per-client (
claudeandclaude-desktopshare.claude/skills, so a dest planned by one and recorded by the other was installed and deleted in the same pass), and the prune now stands down entirely when any plugin failed to activate (the loader catches per plugin and continues, so partial failure - not total - is the real failure mode, and a transient fault silently stripped a client's skill surface).A containment refusal is also no longer silent: a recorded dest naming
/or$HOME/.sshnow warns and emitsclient_assets.prune_refused.Every claim in LLP 0219 that these findings falsified has been rewritten to match the code.