Narrow the retired-asset prune to direct children, and stop reading unreadable as gone (#746)

[philcunliffe]

Closes#746. Three narrowings and two recorded residuals from the ship review of #745. Nothing here widens the four-condition predicate: items 1 and 2 each add a conjunct or split a branch that previously fell through to a delete or to silence.

LLP 0219 is merged and settled, so every doc change below is additive: three new bullets in the Decision section, and two paragraphs that state something the existing anchors left implicit. Nothing it decided is edited.

1. Prune candidates must be a direct child of an asset base dir

isRemovableAsset admitted any strict descendant of a base dir. The materializer only ever writes <base>/<name> or <base>/<name>.md, so a ledger record naming anything deeper cannot be describing our own write - and with a digest that matches, it drove a recursive fs.rm of a subtree nobody installed.

functionisRemovableAsset(dest,baseDirs){constresolved=path.resolve(dest)returnbaseDirs.some((baseDir)=>{constbase=path.resolve(baseDir)returnresolved!==base&&path.dirname(resolved)===base})}

The resolved !== base term is kept rather than folded into the dirname comparison. path.dirname('/') is '/', so a degenerate base would otherwise match itself, and dropping the term would have made this the one edit in the set that could widen. Keeping it makes the new predicate a strict conjunct of the old one, which is checkable by reading.

Verification that the narrowing is right (the issue made this a precondition of shipping it): the only construction of a destination anywhere in the tree is planClientAssets, path.join(baseDir, asset.name) / path.join(baseDir, ${asset.name}.md). copyDir has exactly one caller (copyAsset, src/core/runtime/client_assets.js). Both skills.register and agents.register reject any name that is not a single safe path segment (isSafeContributionName, src/core/runtime/activation.js:269,312). The marker's installed_assets is likewise the materializer's own dest list (action_attach.js:195,214), so the detach path - the other caller of removeClientAssets - cannot be handing it a grandchild either. No materializer path writes a grandchild.

Judgement call: I narrowed isRemovableAsset itself, which also narrows removeClientAssets (used by hyp detach and the reconciler's reverse). The alternative was to narrow only inside pruneOneAsset and leave detach admitting descendants. Rejected: detach's input is the same persisted JSON from the same writer, so a grandchild there is the same corruption signal, and two different containment rules on one module is the drift LLP 0138 collapsed four loops to prevent.

Judgement call: the refusal messages now read "resolves outside this client's asset directories, or deeper into them than HypAware writes". A grandchild is not literally "outside", and a refusal on a corrupt install record is exactly the diagnostic a human reads. The alternative (a second message keyed on which condition failed) adds a branch to a corruption path for no user benefit.

Discrimination

Test: a recorded destination deeper than a direct child is refused, digest or no digest. Fixture: a user-owned ~/.claude/skills/my-own-skill/reference/notes.md, plus a ledger record naming .../my-own-skill/referencewith a real matching digest, so conditions 1, 2 and 4 all hold and only depth can stop it.

Reverted the hunk by hand (restored the old isWithinDir-based body) and re-ran:

not ok 12 - a recorded destination deeper than a direct child is refused, digest or no digest
error: "ENOENT: no such file or directory, open '/tmp/hypaware-prune-orq4Lg/.claude/skills/my-own-skill/reference/notes.md'"
# tests 21 # pass 20 # fail 1

Pre-fix the user's file is deleted. Restored: 21/21. The test also asserts the retired direct child hypaware-ignore is still pruned in the same run, so it is not passing by the prune standing down.

Note on the fixture: the issue frames this as a subtree of a currently planned skill. That exact fixture cannot be built, because copyAsset does fs.rm -rf on a planned skill dir before copying, so a grandchild under a planned skill is destroyed by the copy loop before the prune ever runs. The user-owned directory is the reachable version of the same defect, and it is the worse one: nothing in the subtree was ever ours.

2. "Digest unreadable" is no longer read as "already gone"

digestClientAsset returns undefined both for a path that is not there and for one that is there but unreadable. pruneOneAsset treated both as gone: it dropped the ledger record and said nothing, so the copy became permanently unprunable and unreportable, which is the leave-behind class LLP 0219 exists to end.

inspectClientAsset (new, in client_asset_ledger.js) returns { digest?, missing }. digestClientAsset is now a one-line wrapper over it, so there is still one hash implementation and every existing caller is untouched. Only ENOENT sets missing; every other errno carries the record forward and reports client_assets.prune_withheld with error_kind: digest_unreadable plus a stderr line.

The carried record cannot itself become a delete. It is carried verbatim - no digest is taken of what we could not read - so a later run's removal is still gated on the digest recorded at the moment we wrote the bytes. This is the same move LLP 0219 #open-marker-self-heal explicitly rejects for marker paths ("record the digest of whatever is at the path on first sight"), and not doing it is the point. The test asserts the carried record's digest field is byte-identical to the one the install wrote.

Judgement call: only ENOENT counts as gone. ENOTDIR (a parent component is a file) also means the asset is not there, but treating it as unreadable errs toward "remove less, report more", and it is unreachable in practice for a dest that is a direct child of an existing asset directory.

Discrimination

Test: a retired asset that cannot be read is named and kept on the books. Fixture: install two skills, then chmod 000 a directory inside the retired one, so the top-level stat succeeds (the copy is plainly still there) and hashTree fails with EACCES inside it.

Neutralized the hunk by hand (if (missing || !digest) return { carried: undefined, removed: false }, i.e. the pre-fix behaviour) and re-ran:

not ok 9 - a retired asset that cannot be read is named and kept on the books
error: |-
The input did not match the regular expression /hypaware-ignore/. Input:
''
# tests 21 # pass 20 # fail 1

Pre-fix stderr is empty: the record is dropped in complete silence. Restored: 21/21.

The test has three phases and the last two both discriminate: (a) the withheld report appears on stderr, (b) the ledger record survives with its original digest, (c) with permissions restored and the bytes back to what was installed, the next run prunes it and says so - which pre-fix is impossible, because run 2 dropped the only record naming the path.

Not verified: this test needs a directory the running user cannot read. It calls t.skip(...) if readdir still succeeds after chmod 000 (a root-owned CI run). It did not skip here (uid 1001): all runs above show # skipped 0 for this file. If CI runs as root this test is a no-op there, and I have no way to check that from this worktree.

3. The fifth door for unavailablePlugins: taken as intended, and pinned

A config-enabled plugin whose directory is wholly absent is in neither unloadable (nothing failed to load) nor the withheld-by-profile term (nothing was there to withhold), so unavailablePlugins does not name it and its ledgered assets prune as retired.

Per the issue's disposition I did not add configEnabled - pool to that list. boot.js is unchanged. The reading is recorded in LLP 0219 under a new adjacent anchor {#uninstalled-is-retired} next to #incomplete-activation-prunes-nothing (rather than inside it: the four routes there are all ways a plugin present on the machine leaves a boot's plan, and a new case does not belong under settled text), and pinned by a test.

Discrimination

Test: a config-enabled plugin that is no longer installed is retired, and its skill prunes. Two real boots over the existing synthetic bundled workspace: boot 1 installs and ledgers the opt-in plugin's skill, then the plugin's whole directory is deleted and boot 2 materializes again.

There is no fix hunk to revert for a pin test, so I discriminated it against the alternative the issue rejects: I temporarily added ...[...configEnabled].filter((name) => !pool.some((m) => m.manifest.name === name)) to unavailablePlugins in boot.js and re-ran:

not ok 16 - a config-enabled plugin that is no longer installed is retired, and its skill prunes
error: |-
a wholly absent plugin directory walks through none of the four doors
+ actual - expected
+ '@hypaware/gascity'
# tests 21 # pass 20 # fail 1

Reverted; 21/21. The test asserts both halves: unavailablePlugins is empty (the reading), and the skill is gone (the consequence). Either edit to boot.js trips it.

4 and 5. The two doc-only residuals

Both are additive paragraphs in LLP 0219, each citing #746 so a later reader lands on the item rather than re-deriving it.

• Wizard finale prunes on the pre-wizard config - one paragraph appended under #incomplete-activation-prunes-nothing, immediately after the paragraph that documents the intersection, since that is the sentence it qualifies. Recorded as accepted (transient, self-healing, HypAware-written bytes only), not fixed.
• The digest-check-then-rm race - one paragraph appended under #edited-assets-are-not-ours, where the gate is described, explicitly labelled an accepted residual so a future review reaches the line instead of re-litigating it.

No new LLP was needed: nothing here required changing what 0219 settled.

Safety claims, and exactly which test proves each

Stated narrowly, because a prior PR in this series shipped a guarantee its body could not support:

• A ledger record naming a path deeper than a direct child of an asset directory cannot drive a delete, even with a matching digest - a recorded destination deeper than a direct child is refused, digest or no digest.
• A retired asset that exists but cannot be read is never removed, is reported, and keeps its record with the digest recorded at install time - a retired asset that cannot be read is named and kept on the books.
• An uninstalled config-enabled plugin's byte-identical assets do prune - a config-enabled plugin that is no longer installed is retired, and its skill prunes.

I am not claiming anything broader about hand-edited assets; the digest gate that covers that case is unchanged by this PR and its existing tests are unchanged.

What I could not verify

• The root case for the EACCES test (above): skipped rather than failed if the runner can read a chmod 000 directory. Not exercised here.
• Real-machine behaviour. Everything below is temp-home and hermetic. No acceptance procedure in docs/ACCEPTANCE.md covers the prune, and I did not run one.
• walkthrough_picker_to_first_query fails, and it is pre-existing. Verified by git stash push -u and re-running on the pristine base c483c1a, where it fails on the identical assertion (config: Phase 5 picker config matches expected shape). Prune retired client assets that HypAware itself installed (#726) #745's body reports the same. Not touched by this PR.

Checks (fresh npm install in the worktree)

npm test # tests 4028 pass 4027 fail 0 skipped 1 (pre-existing)
npm run typecheck # clean
npm run smoke -- client_attach_idempotent # ok
npm run smoke -- claude_attach_detach # ok
npm run smoke -- client_attach_on_join # ok
npm run smoke -- cli_bundled_plugins_activated # ok
npm run smoke -- gascity_attach_writes_partition # ok
npm run smoke -- status_diagnostics # ok
npm run smoke -- walkthrough_picker_to_first_query # FAIL, pre-existing (see above)
node --test test/core/llp-ref-hygiene.test.js # 11/11

Smokes chosen by grep -l "client_assets\|clientAssets\|skills install\|attach" hypaware-core/smoke/flows - that is the full list of flows that mention client assets or attach.

🤖 Generated with Claude Code

[philcunliffe]

neutral review - round 1

Head reviewed: e07920ff4d7a9f67433ab43e7b513a60614c0a2f. All 9 checks SUCCESS at
that SHA. Reviewed in a detached worktree; nothing was written to the branch.

Headline: finding 1 is a real widening of a delete predicate, reproduced
end-to-end through removeClientAssets (the hyp detach path, which has no
digest gate at all), deleting two user-authored directories that the pre-PR code
refused. The PR's central warrant, that the narrowing "may only shrink", does not
hold for a basename beginning with .., because the old containment came from a
rel.startsWith('..')prefix check rather than a segment check.

Finding 4 is a decision I am escalating rather than absorbing: item 3's reading
("an uninstalled plugin is retired, so its assets prune") is defensible and its
alternative is genuinely worse, but it is the same fault shape that #745's round 1
rated major, with the evidence removed. A human should sign it off before this
merges. The fix round will add the caveat sentence; the sign-off is not something
this loop can supply.

---

Review round 1 of e07920f. Verdict below. The worktree is back at the review head, clean; nothing was pushed, committed, or commented.

VERDICT: findings

---

1. major - src/core/runtime/client_assets.js:693 - the narrowing is not a strict conjunct of the old predicate: it widens for names beginning with ..

The PR body's central claim is "Keeping [the !== term] makes the new predicate a strict conjunct of the old one, which is checkable by reading", and the JSDoc at :681 says "this predicate is only ever allowed to shrink". Both are false.

The old predicate's containment came from isWithinDir (src/core/runtime/contribution_names.js:44), which rejects on rel.startsWith('..') - a prefix check, not a path-segment check. So path.relative(base, '<base>/..stash') is '..stash', which starts with '..', and the old predicate refused it. The new predicate asks only path.dirname(resolved) === base, which admits it.

Reproduced against the real removeClientAssets (the hyp detach / reconciler-reverse path, which has no digest gate at all), with two user-authored directories under ~/.claude/skills:

--- NEW predicate (PR head) ---
removed: [ '..stash', '...notes' ]
failed : []
user file under ..stash survives: false
user file under ...notes survives: false
--- OLD predicate (base c483c1a) ---
removed: []
failed : [ '..stash', '...notes' ]
user file under ..stash survives: true
user file under ...notes survives: true

The same holds on path.win32. The full sweep (posix and win32) over trailing separators, resolved === base, .. segments, case-only differences, root and UNC bases showed every other input either identical or narrower; ..-prefixed basenames are the only widening, and they widen in the delete direction.

Reachability is limited - isSafeContributionName permits ..stash (it rejects only exactly . and ..), but planClientAssets:143 re-checks with the sameisWithinDir, so the write side skips such a name and no ledger or marker HypAware writes can name one. It therefore takes a hand-edited or corrupt ledger/marker to reach, which is the "same trust domain" class #745's triage accepted. That is why this is major and not a blocker. But item 1's entire warrant is "this predicate may only shrink", and this is the one edit in the set that does not.

Exact fix - restore the strict-conjunct property by keeping the old term as a conjunct (isWithinDir is already imported at :16):

returnbaseDirs.some((baseDir)=>{constbase=path.resolve(baseDir)returnresolved!==base&&path.dirname(resolved)===base&&isWithinDir(resolved,base)})

Do not instead "fix" isWithinDir's prefix check: it is shared with the write side (planClientAssets), and loosening it there would let a ..-prefixed contribution name be written in the first place. Add a test case to a recorded destination deeper than a direct child is refused… (or a sibling) with a <skills>/..stash dest, since no test in the suite currently covers a ..-prefixed basename.

---

2. minor - src/core/runtime/client_asset_ledger.js:213 - missing is set for an ENOENT raised belowdest, so the record is still dropped silently while the path is still on disk

The JSDoc at :195-197 promises "missing is true only for a path that is not there", and LLP 0219 #unreadable-is-not-absent (llp/0219-…decision.md:233-235) enumerates exactly three outcomes with ENOENT meaning "a path that is not there". The single try wraps fs.stat, hashTree and fs.readFile together, so any ENOENT from anywhere in the walk returns { missing: true }.

Reproduced:

(a) dangling symlink at dest -> { missing: true } | lstat says the entry exists: true
(b) dangling symlink inside a skill dir -> { digest: 'af16ca1a…', missing: false }

(b) is correct - hashTree marks non-file/non-dir entries o: and never reads them. (a) is not: the entry exists (fs.rm would remove it, statShape also returns undefined so the shape gate is skipped), the record is dropped in silence, and the leave-behind becomes permanently unprunable and unreportable - the exact class item 2 exists to close. The same hole covers a file vanishing between readdir and readFile mid-walk, and a dest on a volume that unmounts between the stat and the walk.

Not a regression (the base dropped these silently too), which is why it is minor - but the PR body's "Only ENOENT drops a ledger record silently" and the LLP's three-outcome framing both overstate what the code does.

Exact fix - make missing mean what it says by scoping it to the top-level probe:

exportasyncfunctioninspectClientAsset(dest){/** @type {Stats} */letstattry{stat=awaitfs.stat(dest)}catch(err){return{missing: errCode(err)==='ENOENT'}}consthash=createHash('sha256')try{if(stat.isDirectory()){hash.update('dir\n')awaithashTree(dest,dest,hash)}else{hash.update('file\n')hash.update(awaitfs.readFile(dest))}}catch{return{missing: false}}return{digest: hash.digest('hex'),missing: false}}

(Stats goes on the existing @import line at :39.) A test with a dangling symlink at dest asserting the record survives and the path is reported would pin it.

---

3. minor - llp/0219-retired-client-assets-are-pruned.decision.md:96 and :230 - two new decisions are added to a merged Accepted LLP, not two notes

Items 4 and 5 are genuinely additive notes and are fine. #only-direct-children and #unreadable-is-not-absent are not: they narrow condition three of #prune-on-materialize and split a branch it settled, i.e. they change what the doc decided. CLAUDE.md says twice that an Accepted doc is changed "by extending it (a new LLP, noted on the old doc's Extended-by: line)", and #745's own triage note drew the line explicitly: the in-place edits were justified because "a doc that has never been on master is still part of the change under review. Once merged, it is settled." 0219 merged in c483c1a.

The PR body asserts "every doc change below is additive… Nothing it decided is edited", which is true of the characters but not of the decision: after this PR, condition three no longer means what #prune-on-materialize says it means.

Exact fix - mint one new decision LLP (e.g. NNNN-the-prune-predicate-is-narrowed-to-direct-children.decision.md) carrying the #only-direct-children and #unreadable-is-not-absent bullets, @refing 0219; add **Extended-by:** LLP NNNN to 0219's header and a one-line forward-ref on the #prune-on-materialize and #edited-assets-are-not-ours bullets; repoint the three code @refs (client_assets.js:531, :686, client_asset_ledger.js:198) and the test's @ref at test/core/client-assets-prune.test.js:830. #uninstalled-is-retired and items 4/5 can stay in 0219 - those are records of a reading, not new decisions.

If the human reviewer judges the in-place addition acceptable (a defensible reading: the PR closes an issue the merged doc's own review spawned), then finding 5 below is the minimum.

---

4. minor - item 3 (#uninstalled-is-retired) is a decision a human should sign off on, and the doc omits the one sub-case that makes it hard

Asked directly by the review brief, so stated directly: yes, I think this one needs a human, not a doc note. The test pins it honestly and the LLP text does describe what the code does (I verified boot.js:213-237: unloadable needs a manifest that was found and failed; wantedButWithheld needs the plugin in pool; a wholly absent directory enters neither, so unavailablePlugins is [] and the prune runs). My concern is not that the doc misdescribes the code - it is what the decision costs.

Round 1 of #745 rated it major that "a transient fault silently strips a client's skill surface", and that finding is why #incomplete-activation-prunes-nothing exists at all. The fifth door is the same fault shape with the evidence removed: an interrupted npm i -g, a plugin tree on a volume that is not mounted at boot, a half-applied upgrade, or an emptied plugin-state directory is indistinguishable from a deliberate uninstall, and it deletes rather than standing down. The LLP's counter-argument is real and I agree with it as far as it goes - configEnabled - pool would stand the prune down permanently after every genuine uninstall, which reinstates the leave-behind - and the third option (record the owning plugin per ledger entry and stand down only that plugin's candidates) is explicitly rejected elsewhere in the same doc. So the trade is genuine and there is no free answer. That is exactly why it should be a human's call rather than a paragraph a follow-up PR wrote about itself.

Exact fix - either (a) escalate: leave the code as is and ask the maintainer to confirm the reading before this merges, or (b) at minimum, extend #uninstalled-is-retired (:167-181) with one sentence naming the sub-case it does not cover - "absent" also covers an install tree that is transiently absent (an unmounted volume, an interrupted upgrade), which this reading deletes and a later attach restores; accepted because the alternative never prunes after a real uninstall - so a future review lands on the line instead of re-deriving it, which is precisely what items 4 and 5 do for their residuals.

---

5. nit - llp/0219-…decision.md:68 - the four-condition summary still states the old, wider condition three

#prune-on-materialize reads "it sits strictly inside that client's own asset directories". That is no longer the predicate, and the summary is the paragraph a reader reaches first. The new bullet at :96 does say it narrows condition three, but a reader who stops at the summary gets the wrong rule.

Exact fix - a forward-ref, which the convention explicitly permits on a settled doc ("Trivial editorial fixes (typos, links, forward-refs) are fine"): change :68 to it is a direct child of that client's own asset directories (#only-direct-children), or append (narrowed by #only-direct-children). The corresponding JSDoc in client_assets.js:307-310 was already updated correctly.

---

6. nit - test/core/client-assets-prune.test.js:427 - a failure between the chmod 000 and the restore leaves a directory rm -rf cannot remove

fs.chmod(locked, 0o000) is at :427; the restore is at :456, after the five assertions at :447-455. Any of those failing leaves /tmp/hypaware-prune-*/.claude/skills/hypaware-ignore/reference at mode 000, and the makeHome-based tests in this file never remove their temp home (unlike the three boot tests at :691, :824, :880). Confirmed that this is not self-clearing:

rm: cannot remove '/tmp/tmp.XTwtG1OcaT/reference': Permission denied
STILL PRESENT (rm -rf failed)

The t.skip guard itself is correct: it restores the mode before returning, and it fires on the root case (readdir still succeeding after chmod 000). It did not fire here - # skipped 0 on every run of this file - so the test genuinely exercised the EACCES path under uid 1001. The PR body's "not verified for a root CI runner" caveat stands as written.

Exact fix - register the restore the moment the mode is dropped, immediately after :427:

t.after(()=>fs.chmod(locked,0o755).catch(()=>{}))

and, optionally, t.after(() => fs.rm(home, { recursive: true, force: true })) for parity with the boot tests.

---

Delete-safety, re-derived

Condition 1 - HypAware's own record says it wrote the path. Two sources, both traced to their writers. The ledger: readClientAssetLedger (client_asset_ledger.js:79-114) drops any entry with a bad kind/name/client/dest, and drops whole any record whose digest key is present but not a non-empty string (:104) - so corruption can only shrink the candidate set. The marker: attachMarkerAssets (client_assets.js:651) reads installed_assets through the single accessor readInstalledAssets (action_reconciler.js:500), which filters to non-empty strings and never synthesizes a path. git log -S installed_assets shows the field was introduced in 052acc8, the same commit that created the one materializer, so every value it has ever held came from path.join(baseDir, name) / path.join(baseDir, name + '.md'). Marker-sourced candidates carry record === undefined and therefore always exit at the digest gate as a report, never a delete - re-verified by reading pruneOneAsset:565.

Condition 2 - the whole run's plan does not contain it.keepAll (client_assets.js:360) is built from planned, not from this client's share, and is the set consulted by the candidate loop (:385, :389) and by the carry loop (:415). Unchanged by this PR; re-read to confirm the round-2 fix is still in place.

Condition 3 - strictly inside the client's asset dirs, and now only one level in. This is where finding 1 lives.

• Cannot widen: established by exhaustive comparison of the old and new predicates over the edge inputs, on path.posix and path.win32, not by reading. Trailing separator on either side: path.resolve strips it, both agree. resolved === base: both refuse, and the !== term is genuinely load-bearing for a degenerate '/' base as the author says (path.dirname('/') === '/'). .. segments: path.resolve collapses them before either predicate looks, so <base>/../evil refuses in both. Case-only difference: the new one is narrower on win32 (dirname+=== is case-sensitive where path.relative is not), and that narrowing is unreachable because the ledger's dests and the run's baseDirs come from the same path.join. Root/UNC bases: identical. The one widening is a basename beginning with .. - finding 1, reproduced end-to-end through removeClientAssets.
• Canonicalization: resolved is notrealpath'd, only path.resolve'd (lexical, cwd-anchored). That is correct and unchanged: fs.rm is also lexical and does not follow the final symlink, so predicate and delete agree, and a symlinked ~/.claude/skills is installed and pruned through consistently. inspectClientAsset does follow symlinks (fs.stat), an asymmetry that can only make a digest fail to match, never spuriously match.
• Cannot under-delete into a new leave-behind: I verified the author's fixture reasoning (copyAsset:741 does fs.rm -rf on a planned skill dir before copyDir, so a grandchild under a currently-planned skill cannot survive to be pruned) - correct, and the substituted user-authored fixture is the reachable and worse version. Then the other direction: git show 052acc8:src/core/runtime/client_assets.js shows the original materializer joined path.join(baseDir, asset.name) / ${asset.name}.md exactly as today, so no version of the materializer has ever written a grandchild. Pre-052acc8 install loops wrote no ledger and no installed_assets, so nothing names their writes and they were never candidates. installed_assets cannot hold a grandchild for the same reason (single writer since its introduction). Both removeClientAssets callers outside the module (commands/clients.js:1137, action_attach.js:400) pass marker-sourced dests. Conclusion: nothing that HypAware wrote or recorded becomes unprunable, and the full 4028-test suite passing is consistent with that.

Condition 4 - a recorded digest still matches. Unchanged by this PR except for the new !digest branch inserted ahead of it. I confirmed the "verbatim, no digest re-taken" claim in the code, not the body: pruneOneAsset:546 returns { carried: record }, the same object read out of the ledger, with no write to record.digest anywhere in the function (the only digestClientAsset call in the module is :422, over installed dests, which by construction excludes candidates). Carry-forward traced across runs: a carried record re-enters next (:395), is deduped on (client, dest) by writeClientAssetLedger:134, and so cannot accumulate; on a later run it becomes a candidate again and the removal is gated on the install-time digest, so a file that changed while unreadable fails the gate when permissions return (hashTree frames relative paths as well as bytes). The test's third phase demonstrates the intended positive case - permissions restored and bytes back to the installed tree, then the prune completes - which is only possible because the record survived. A marker-only candidate that is unreadable carries nothing (there was no record) but is still reported, which is right.

---

Also checked, clean

• Ran (fresh npm install in the review worktree): npm test - 4028 tests, 4027 pass, 0 fail, 1 pre-existing skip. npm run typecheck - clean. node --test test/core/client-assets-prune.test.js - 21/21, # skipped 0 (the EACCES fixture really ran). node --test test/core/llp-ref-hygiene.test.js - 11/11. Smokes client_attach_idempotent, claude_attach_detach, client_attach_on_join - all ok. walkthrough_picker_to_first_query not run; pre-existing failure tracked in walkthrough_picker_to_first_query is red on master, and no smoke runs in CI #750.
• Item-1 discrimination re-derived independently. Reverted the isRemovableAsset body to the base isWithinDir form by hand and ran the new test: not ok 1 … error: "ENOENT: no such file or directory, open '/tmp/hypaware-prune-8ffWBj/.claude/skills/my-own-skill/reference/notes.md'" - the user's file is gone, exactly as the body reports. Restored via git checkout --; back to green. The test does not pass by the prune standing down: it also asserts hypaware-ignore is pruned and reported in the same run.
• Item-3 pinning test is not vacuous. It asserts both halves. The unavailablePlugins deepEqual is trivially broken by the rejected alternative. The consequence half is also load-bearing: if boot 2 had installed nothing for claude, the client would be out of scope and the skill would survive, failing the assertion - so it cannot pass by the prune never running.
• @ref honesty. All three new anchors resolve (llp:96, :167, :230); llp-ref-hygiene 11/11. The test's @ref … [tests]: with an empty gloss matches the sibling block at :886 already in the file. The updated JSDoc for condition 3 (client_assets.js:307-310) and for isRemovableAsset (:665-687) both describe the code accurately (modulo finding 1's .. case, which no prose mentions).
• Conventions. No U+2014 in any of the four changed files (grep -cP '\x{2014}' → 0, 0, 0, 0). No trailing semicolons in the added JS. No @typedef, no inline import('…') types. No new type imports were needed. The → characters are U+2192 and pre-existing throughout the module.
• Refusal message wording. The widened message "resolves outside this client's asset directories, or deeper into them than HypAware writes" is correct for both removeClientAssets:261 and pruneOneAsset:478 and is asserted by the new test. The author's judgement to narrow removeClientAssets too (rather than only pruneOneAsset) is right: its input is the same persisted JSON from the same writer, and two containment rules in one module is the drift LLP 0138 collapsed.
• digestClientAsset as a wrapper. One hash implementation, all existing callers untouched, the undefined contract preserved; test/core/client-assets-prune.test.js still exercises it directly at :897.
• Items 4 and 5 doc paragraphs. Both are genuinely additive, both cite Prune hardening follow-ups from the #745 ship review (no blockers) #746, both sit under the anchor whose sentence they qualify, and both describe the code correctly (configEnabled is computed once at boot.js:225; the gate-to-rm window is client_assets.js:565 → :589).

[philcunliffe]

Cleanup done; worktree clean at 66e6028, nothing written to the branch.

---

VERDICT: findings

Round 2 of 66e60288a4efe58525b903df6d2a375f22f7d84d. All 9 checks SUCCESS at that SHA. Reviewed in a detached worktree; nothing pushed, committed, or commented.

Headline: round 1's finding 1 is genuinely fixed, and I re-derived the fix from scratch rather than reading it. An exhaustive old-vs-new comparison over path.posix and path.win32 finds zero inputs the new predicate admits that the pre-PR predicate refused, and the only inputs it refuses that the old one admitted are grandchildren-and-deeper — exactly the intended narrowing, with no leave-behind for anything the materializer can write. Nothing below is ship-blocking. Three findings remain: one stale-record problem in the PR description, and two nits.

One item the triage rung must decide, not a defect: round 1 escalated #uninstalled-is-retired for human sign-off before merge, offering "(a) escalate … or (b) at minimum, extend it with one sentence". The fixer took (b), which round 1 named as an acceptable minimum, so finding 4 is closed on its own terms. But no human has signed the reading off — the PR's only comment is the round-1 review. That sign-off is outside what this loop can supply and remains open going into triage.

---

1. minor — PR #749 description — the body still presents the pre-fix predicate as what ships, and still asserts the claim round 1 falsified

Not ship-blocking on code: client_assets.js, LLP 0223, and the commit message on 66e6028 are all correct. But the PR description was never updated after round 1, and it is the review record a reader (and a merge-commit body, in "create a merge commit" mode) inherits. It currently states:

returnresolved!==base&&path.dirname(resolved)===base

as the shipped isRemovableAsset body — the two-conjunct version that round 1 proved widens for ..-prefixed basenames — followed by "Keeping it makes the new predicate a strict conjunct of the old one, which is checkable by reading." That sentence is the exact false warrant round 1 escalated to major, still standing verbatim over a delete predicate. The body also says "No new LLP was needed: nothing here required changing what 0219 settled" while llp/0223-… is in the diff, and its discrimination transcripts and check counts are stale (# tests 21 against 24 in the file; npm test … 4028 against 4031).

Why it matters: this PR series' own retrospective is "a prior PR in this series shipped a guarantee its body could not support." Landing a description that documents a delete predicate as narrower-by-one-conjunct than it is repeats that precisely.

Exact fix — update the description: replace the code block with the three-conjunct body, replace the "strict conjunct, checkable by reading" sentence with the reason the third conjunct is required (isWithinDir's prefix test refuses ..stash, which path.dirname alone admits), replace "No new LLP was needed" with the LLP 0223 / Extended-by account, and refresh the four transcripts and the check counts. No code change.

2. nit — test/core/client-assets-prune.test.js:732 — the test name promises win32 coverage the test does not have

test('removeClientAssets refuses a basename beginning with ".." on posix and win32 alike', …) calls removeClientAssets exactly once, on the host's path flavor. On CI (Linux/macOS) the win32 half is never exercised. The claim happens to be true — I verified it independently by running both predicates under path.win32 over C:\…, C:\, \\server\share, and \\server\share\skills bases, with ..stash / ...notes / .. / trailing-separator / mid-path-.. / case variants; zero widenings — but it is established by reasoning, not by this test.

Exact fix — rename to removeClientAssets refuses a basename beginning with "..", and either drop the claim or add a separate pure-predicate assertion over path.win32 (the module-level isRemovableAsset is not exported, so this would mean testing isWithinDir under path.win32 directly, or leaving the win32 reasoning to LLP 0223, where it already is).

3. nit — src/core/runtime/client_asset_ledger.js:238 and src/core/runtime/client_assets.js:532-546 — the new catch-all discards the errno, and the user-facing advice is hard-coded to the permissions case

The second try ends } catch {, so the errno of a below-dest failure is gone. The withheld report is then error_kind: 'digest_unreadable' with no errno attribute, and the stderr line ends left in place - check its permissions. For the exact case this round's new test simulates (a file readdir listed that vanishes before readFile) and for a device error, "check its permissions" is wrong advice and the log cannot tell an operator which of the three it was. CLAUDE.md's Log-Driven Development section asks that failures "identify the broken step" and names error_kind granularity on error paths.

Exact fix — catch (err) { return { missing: false } } is not enough on its own since the errno must reach the caller; simplest honest version is to widen the return to { digest?, missing, errno? } (or have inspectClientAsset log the errno itself), then add errno to the client_assets.prune_withheld attributes and soften the stderr tail to left in place - check its permissions, or whether something else is writing to it. Purely additive; no gate changes.

---

Round-1 findings, re-derived

1. major, isRemovableAsset widened for ..-prefixed basenames — FIXED. Re-derived from scratch, not read. I rebuilt both predicates parameterized on the path flavor and swept {..stash, ...notes, .., ...., ..a/b, a/b, a/b/c, sub/../skill, ../sibling, ../../etc/passwd, x/..stash, ..stash/inner, SKILL, skill., a..b, skill.md} × {base, base+sep, base+sep+sep} × {dest, dest+sep} over posix bases /home/u/.claude/skills, /, /a, /home/u/.claude/skills/ and win32 bases C:\Users\u\.claude\skills, C:\, C:\a, \\server\share, \\server\share\skills: widenings 0 on both flavors; 120 (posix) / 126 (win32) narrowings, all grandchild-or-deeper. resolved === base refused by both (the !== term is still load-bearing for a / base, path.dirname('/') === '/'). Trailing separators collapse in path.resolve before either looks. .. segments mid-path collapse likewise. Case-only differences on win32 are narrower under the new predicate (=== is case-sensitive where path.relative is not) — unreachable, since a run's baseDirs and the ledger dest both come from the same path.join(homeDir, descriptor.skillDir); this is unchanged from round 1's head, not new here. Bases are path.resolve'd lexically, never realpath'd, so a symlinked ~/.claude/skills is installed and pruned through consistently, and fs.rm (also lexical on the final component) agrees with the predicate. Other direction, no new leave-behind: the third conjunct can only refuse where dirname(resolved) === baseandisWithinDir is false, which reduces to exactly "basename begins with .." — and planClientAssets:143 gates every write on the sameisWithinDir, so no such dest can ever have been written (skills or .md agents alike). Confirmed positively with an independent script against the real removeClientAssets: ..stash, ...notes, .., nested/deeper all refused with the user's files intact, while normal-skill and a.md are still removed.
2. minor, missing set for an ENOENT anywhere in the walk — FIXED, structurally. Verified empirically against the shipped code: EACCES below dest → {missing:false}; EACCES at dest (mode-000 dir, stat succeeds) → {missing:false}; ENOTDIR → {missing:false}; dangling symlink inside the tree → digest returned, missing:false (matches LLP 0223's hashTree/Dirent account: o: by name, never followed). The only behavioral delta versus the round-1 head is ENOENT-below-dest, which is the whole of the fix.
3. minor, two new decisions added to a merged Accepted LLP — FIXED, and cleanly.git show c483c1a:llp/0219-… confirms neither {#only-direct-children} nor {#unreadable-is-not-absent} was ever on master, so relocating them is this PR tidying its own unmerged text, not editing settled text. Both anchors now live only in 0223; grep finds zero remaining refs to LLP 0219#only-direct-children / #unreadable-is-not-absent anywhere. 0219 keeps Extended-by: LLP 0223 plus two stub bullets and two inline forward-refs, all of which point rather than decide. 0223 is a genuine decision doc, not a copy: Context/Decision/Consequences, and it carries material 0219 never had — the prefix-test rationale for the conjunct, the "structural, not a branch to keep in sync" argument for the split try, and explicit dispositions for both symlink cases. Number 0223 is free: enumerated llp/022* across all 46 remote heads and local refs; only this branch holds it, and only under this slug.
4. minor, #uninstalled-is-retired omitted transient absence — FIXED per its own exact fix. The sentence is appended at llp/0219-…:182-186, verbatim in substance to what round 1 asked for; boot.js is untouched in the whole PR. The human sign-off round 1 escalated is still outstanding (see preamble).
5. nit, 0219's four-condition summary stated the old condition three — FIXED.:69-72 now reads "it is a direct child of that client's own asset directories … narrowed from 'sits strictly inside' by LLP 0223 #only-direct-children". Naming what changed keeps the record honest, and a forward-ref is explicitly permitted on a settled doc.
6. nit, chmod 000 restore after five assertions — FIXED.t.after(() => fs.chmod(locked, 0o755).catch(() => {})) at :436, registered on the line after the chmod and before the t.after that removes home. I confirmed on this Node (v22.23.1) that t.after hooks run in registration order, so the mode restore really does precede the recursive removal — the code comment's claim is correct, not assumed.

Disclosed deviation A — the dangling-symlink repro does not discriminate: the fixer is RIGHT, round 1 was wrong. Measured directly: fs.stat on a dangling symlink throws ENOENT, so it is caught by the top-level probe in the new code and by the single try in the old, and both return {missing: true} — identical. Round 1's own proposed fix is character-for-character what shipped, so it would not have changed this either. The correction to the JSDoc, to LLP 0223, and the rebuild of the test around an ENOENT below dest are all correct. My verdict on the case round 1 was actually pointing at (lstat says the entry exists, stat says gone): correctly out of scope for this PR, worth an optional follow-up at most. Reasons, stated so a later review does not re-derive them: (i) it is not a regression — identical on c483c1a; (ii) it is no longer silent in the record — LLP 0223 disposes of it explicitly twice, in #unreadable-is-not-absent and in §Consequences ("fs.stat follows it, finds nothing, and the record is dropped as gone"), which is exactly the "a future review lands on the line" treatment items 4 and 5 got; (iii) the leave-behind class 0219 exists to end is model-invocable content, and a dangling symlink at dest has no content to invoke — the record is dropped but nothing readable survives; (iv) the other members of the class round 1 named are now genuinely fixed, not merely documented — a file vanishing mid-walk and a volume lost between the top-level stat and the walk both land in the second try and are carried and reported. What remains is one inert broken link that only a user's own edit can create. A follow-up could make it reportable by probing fs.lstat after an ENOENT from fs.stat; it is not worth a round.

Disclosed deviation B — the @ref at client-assets-prune.test.js:830: the fixer is RIGHT.git show e07920f:test/core/client-assets-prune.test.js confirms that ref anchored #uninstalled-is-retired, which stays in 0219, so round 1's instruction to repoint it at the new LLP would have minted a dangling anchor. It correctly stayed put (now :994, resolving to llp/0219-…:166). All refs that exist now are honest: five @ref LLP 0223#… in the test file (:398, :493, :626, :685, :730) and three in source (client_asset_ledger.js:213, client_assets.js:531, :695), every one resolving to an anchor defined in 0223; 0223's own two @ref LLP 0219#… in its header blockquote resolve to #prune-on-materialize and #edited-assets-are-not-ours, both still in 0219. node --test test/core/llp-ref-hygiene.test.js → 11/11. Every ref is attached with no intervening blank line, and each says something the code and filename do not.

---

Also checked, clean

• Ran (fresh npm install in the review worktree): npm test → 4031 tests, 4030 pass, 0 fail, 1 pre-existing skip. npm run typecheck → clean. node --test test/core/client-assets-prune.test.js → 24/24, # skipped 0, so the chmod 000 EACCES fixture genuinely ran here (uid 1001); the PR body's "no-op under a root CI runner" caveat stands unverified as written. node --test test/core/llp-ref-hygiene.test.js → 11/11. Smokes client_attach_idempotent, claude_attach_detach, client_attach_on_join → all ok. walkthrough_picker_to_first_query not run (pre-existing failure, walkthrough_picker_to_first_query is red on master, and no smoke runs in CI #750).
• Both new finding-1 tests discriminate, and the integration one fails by an actual deletion. Dropping only && isWithinDir(resolved, base) by hand: not ok 14 … error: "ENOENT: no such file or directory, open '/tmp/hypaware-prune-tA65aY/.claude/skills/..stash/notes.md'" — the user's file is gone, not an assertion mismatch. not ok 15 fails on removed containing both ..stash and ...notes; that array is appended only after a successful fs.rm, so it is still proof of deletion, though the diagnostic names the return value rather than the file. Restored → 24/24. Neither test passes by the prune standing down: the deeper-than-direct-child test asserts hypaware-ignore is pruned and reported in the same run, and my independent script confirms legitimate direct children (normal-skill, a.md) are still removed under the three-conjunct predicate.
• The t.mock.method test is sound and pins the real contract. It fails against the pre-fix inspectClientAsset (I restored the single-try body by hand): not ok 10 … The input did not match the regular expression /hypaware-ignore/. Input: '' — stderr empty, the record dropped in silence, which is the defect. The mock is captured (const original = fs.readFile) before installation, delegates every other path to the real implementation, is keyed on one exact absolute path, and is guarded by assert.ok(intercepted, …) so a walk that never reaches it fails loudly rather than passing vacuously. It is bound to the test context, so node:test restores it automatically at test end; the 15 tests after it in the same file pass in the same process. Crucially the assertions are on real facts, not the mock's shape: the retired directory still exists, stderr says could not be read, and the carried ledger record's digest is byte-identical to the one the install wrote. The /could not be read/ assertion is what discriminates — had the mock not fired, the added reference/notes.md would have made the digest mismatch and produced the "changed since HypAware installed it" branch instead, which the regex rejects.
• Delete safety, all four conditions, re-traced after this round's edits. The round-2 diff touches pruneOneAsset only in one @ref line, so conditions 1, 2 and 4 are byte-identical to what round 1 traced; I re-read them anyway. Condition 2's keepAll is still built from the whole run's planned and consulted by both the candidate loop and the carry loop. Carry-forward, specifically: the candidate set takes records where !keepAll.has(dest) and the trailing carry loop takes records where keepAll.has(dest) && !landed.has(dest) — disjoint, so a carried record cannot be pushed twice, and writeClientAssetLedger dedupes on (client, dest) regardless, so it cannot accumulate across runs. The only digestClientAsset call in the module is over installed dests, which are in keepAll and therefore never candidates — so no digest is ever re-taken for a carried record, and a later run's removal stays gated on the digest recorded when the bytes were written. A file that changed while unreadable therefore fails the gate when permissions return (hashTree frames relative paths as well as bytes). The EACCES test's third phase demonstrates the positive case end to end.
• The one containment implementation.grep over src, hypaware-core, bin: isRemovableAsset has exactly two call sites (removeClientAssets:257, pruneOneAsset:475) and one definition; no parallel dirname-based check anywhere. isWithinDir is unchanged, as LLP 0223 says, so the write side's refusal of ..-prefixed names is intact.
• Conventions.grep -cP '\x{2014}' → 0 on all five changed files. No @typedef, no inline import('…') types, no trailing semicolons on any added JS line, no line over 122 chars in the added source. The one new type import is @import { Stats } from 'node:fs' — a built-in specifier, so the root-anchored .js rule does not apply; tsc --noEmit accepts the /** @type {Stats} */ let stat definite-assignment pattern. No lint script exists in package.json to run.
• LLP header conventions. 0223's header block (Type/Status/Systems/Author/Date/Related + an "extends" blockquote) matches 0219's and the corpus; **Extended-by:** on 0219 matches the 20 existing uses. There is no LLP index file to update.
• git status in the worktree is clean at 66e6028; every hand-reverted hunk was restored via git checkout --, and all my scratch scripts are deleted.

[philcunliffe]

neutral triage - ship, with one pre-merge task for the human

Review budget exhausted at 2 rounds. Three residual findings plus one open
question. All are preferences. This PR can merge, but #755 item 0 is a
pre-merge task, not a post-merge follow-up.

The open question, and why it did not block

Round 1 escalated LLP 0219's #uninstalled-is-retired reading for human sign-off
before merge (a config-enabled plugin whose directory is wholly absent counts as
retired, so its ledgered client assets prune). No human has signed it: every comment
on this PR is neutral's own. Triage judged it on merits rather than deferring to the
label, and three facts decided it:

Blocking cannot prevent the behaviour.boot.js is untouched by this PR. The
fifth door shipped on master in c483c1a (#745). What #749 adds is the paragraph
naming it, the transient-absence caveat, and a pin test so it cannot drift silently.
Parking this PR would leave production with the identical deletion policy plus
the wider strictly-inside delete predicate and the silent EACCES record-drop that
this PR closes. That protects no one.

The transient-absence class is far narrower than the abstract shape, verified in
code. A user-installed plugin whose lock entry survives but whose directory is
missing does not reach the fifth door at all: loadManifest fails ENOENT,
discoverInstalledPlugins puts it in failed, boot.js:236-238 puts it in
unloadable, unavailablePlugins is non-empty, and the prune stands down entirely.
Total workspace loss also stands down, via LLP 0219's guard that a client with no
successful copy this pass is not pruned. What is reachable is selective absence of
one opt-in bundled plugin's directory with the rest of the tree intact and an attach
inside that window, or partial HYP_HOME loss with the lock emptied and the ledger
surviving, which is the trust domain that could forge the ledger directly.

What is lost is recoverable and is only our own bytes. The four-condition gate
requires HypAware's own ledger record for the exact path, absence from the whole
run's plan, direct-child containment, and a shape-framed sha256 recorded at install
that still matches on disk. A user edit of any size fails the digest and is withheld
with a stderr line and a prune_withheld signal; marker-only candidates carry no
digest and are always withheld. So the worst case is a byte-identical copy of a
skill HypAware itself wrote disappearing during a narrow fault window and
reappearing after the fault clears and an attach runs.

Absence of sign-off is therefore a process gap, not a production defect. The ship
disposition supplies the missing human: merging is the sign-off, and #755 item 0
states the reading plainly so it is an informed one. If the reading is rejected, the
remedy is a new LLP plus a boot.js change on a follow-up, because this PR is not
where the behaviour lives.

The pre-merge task

This PR's description still shows the pre-fix two-conjunct isRemovableAsset and
the claim round 1 falsified: "Keeping it makes the new predicate a strict conjunct
of the old one, which is checkable by reading." The code, LLP 0223 and the commit
message on 66e6028 are all correct; only the body was never updated. Since a merge
commit inherits the body, please fix it before merging (or squash-merge, the commit
message is already right). Exact edits in #755 item 0(b).

This series' own retrospective is that a prior PR shipped a guarantee its body could
not support. Landing a description that documents a delete predicate as narrower
than it is would repeat that.

The other two residuals

Both deferred to #755: the discarded errno on a below-dest read failure (the path
already fails safe and reports; what is missing is which of three causes it was),
and a test name promising win32 coverage the test does not exercise (the claim is
true, established by review sweep, just not by that test).

Verified at head 66e6028: npm test 4030 pass / 0 fail / 1 pre-existing skip,
npm run typecheck clean, node --test test/core/client-assets-prune.test.js 24/24
with 0 skips (the EACCES fixture genuinely ran), smokes client_attach_idempotent,
claude_attach_detach and client_attach_on_join all ok.