Client-scoped materialize keeps a dest another client still contributes

[philcunliffe]

The bug

reconcileClientAssetLedger built its prune keep-set from this run's plan:

constkeepAll=newSet(planned.map(({ dest })=>dest))

planClientAssets filters by options.clients, and every attach path is client-scoped: materializeAttachAssets passes clients: [name], the daemon reconciler passes clients: [client], the wizard finale passes clients: clientsPicked. So the keep-set never contained another client's contributions, and the invariant the doc block states (not contributed, for any client) only held under --client all.

claude and claude-desktop both declare .claude/skills, so one physical dest can be recorded under one client and contributed by the other. A skill whose clients narrows from ['claude','claude-desktop'] to ['claude-desktop'] was deleted by the very next claude-scoped attach, and nothing re-installs it: the reconciler never attaches probe-less claude-desktop.

Reproduction

New test a client-scoped run never prunes a destination another client still contributes in test/core/client-assets-prune.test.js. On master it fails with exactly the reported line:

installed skill 'hypaware-query' -> .../.claude/skills/hypaware-query
removed retired skill 'hypaware-privacy' -> .../.claude/skills/hypaware-privacy

The existing shared-dest test could not catch this: its installWith helper always ran hyp skills install with the default --client all. The helper now takes an optional client, which is what drives the scoped run.

The fix

The keep-set is re-planned with clients: 'all' (without an stderr, since the run's own plan already wrote any warnings), so "is this path still contributed?" is asked of every client's contributions rather than of the clients this run was scoped to. When the run is already 'all' the existing plan is reused.

Pruning is not disabled on scoped runs: the second new test, a client-scoped run still prunes a destination no client contributes any more, pins that a name no manifest declares any more is still retired and still removed under --client claude. It passes before and after.

The carry-forward loop and condition 2 of the doc block are reworded to match, and the widening carries an @ref LLP 0219#prune-on-materialize.

Checks

• npm test: 4485 pass, 0 fail, 1 skipped
• npm run typecheck: clean

Fixes#884

[philcunliffe]

Neutral review round: head e56fe8c5

Verdict: findings (3) — 2 fixed and pushed, 1 recorded as an open question. The core fix is correct and ships as written.

The behaviour change was reproduced both ways in isolated worktrees: on base b1afa7c2 a --client claude-scoped skills install after narrowing a shared skill to claude-desktop prints removed retired skill and deletes ~/.claude/skills/<name>; on e56fe8c5 it leaves it in place. test/core/client-assets-prune.test.js and the whole client-asset*/*attach*/*skills* core set pass on the head. The widening is safe at every call site: descriptors is the full catalog map in all four callers (runSkillsInstall, materializeAttachAssets, attachedAssetOptions, the walkthrough), planClientAssets/resolveAssets are pure so re-running is side-effect free, and writeClientAssetLedger dedupes on (client, dest) with last-write-wins so the extra carried records cannot accumulate or shadow the fresh install records.

Finding 1 — medium — the new @ref asserted the opposite of the section it cited, and LLP 0219 was not updated. Fixed.

src/core/runtime/client_assets.js:377 (head) added @ref LLP 0219#prune-on-materialize [implements]: the plan check is over every client's contributions. llp/0219-retired-client-assets-are-pruned.decision.md is Accepted and settles that section as "The plan check is over the whole run, not over the one client's share of it" and "So the carry is asked of the whole run's plan too." The PR changes both and rewrites the doc block accordingly, but left the LLP untouched, so the ref told the next reader the code does something it no longer does and /ref-story would narrate the wrong rationale. Per CLAUDE.md this is a change to what an Accepted doc settled, not a mechanical edit.

Fixed in 4fe9620b by minting llp/0266-prune-asks-every-client-not-the-run.decision.md (the shape LLP 0226 already used for the two narrowings from #745's ship review): it records #contributions-not-the-run — contributions not clients-in-scope, re-planned rather than re-derived, the widened pass silent, and scoped runs still prune. Extended-by: LLP 0266 added to 0219, and the code's ref repointed to LLP 0266#contributions-not-the-run.

Finding 2 — low — the stated reason for dropping stderr from the widened plan was false. Fixed.

src/core/runtime/client_assets.js:374 (head) said "every warning planClientAssets has to write was already written when it planned the run's own copies above." It was not: planClientAssets filters by wantedbefore the unknown-client check (if (wanted && !wanted.has(client)) continue, ~line 124), so a contribution naming a client outside a scoped run's filter (e.g. a typo'd clients: ['claude-desktp'] under hyp attach claude) warns in neither pass. Suppressing is still the right behaviour, but a false justification invites a later "fix" that re-attaches stderr and starts printing other clients' warnings on every scoped attach. The comment now gives the real reason: an explicit client list never warns about what it excluded, and --client all still surfaces every one.

Finding 3 — low — a carried-forward record keeps a stale digest, so the asset it protects can end up unprunable. Not fixed; recorded as LLP 0266#open-stale-carried-digest.

The widened keepAll retains a record for (client, dest) where this client no longer contributes the dest but another still does, and that record's digest is never refreshed while the other client rewrites the bytes. Sequence run against the head: (1) install all, skill p targets both clients, body v1; (2) --client claude with p narrowed to claude-desktop — the claude record is now carried instead of pruned; (3) --client claude-desktop with body v2 — disk is v2, the claude record is untouched by next = ledger.filter(r => !scope.has(r.client)) and still v1; (4) p retired everywhere, --client claude — digest mismatch, so pruneOneAsset withholds with changed since HypAware installed it; left in place - remove it by hand. The asset stays on disk and the message blames the user for a rewrite HypAware made.

The same window is reachable on base via 1 -> 3 -> 4, so this is a pre-existing gap this PR makes reachable by one more route, not a regression introduced here (and the PR's outcome is loud where base's was silent). Not closed in this round because both candidate fixes change what the ledger is: refreshing a carried record's digest re-records evidence for a copy that client did not make (close to the move 0219#open-marker-self-heal explicitly rejects), and keying the ledger on dest with a set of contributing clients is a format change to a file older versions read. Recorded in 0266's open questions so the work starts from that paragraph.

Checks on the pushed head 4fe9620b

• npm test: 4485 pass, 0 fail, 1 skipped
• npm run typecheck: clean
• Diff vs the reviewed head is confined to src/core/runtime/client_assets.js (comment + @ref only, no behaviour change), llp/0219-... (Extended-by: line), and the new llp/0266-....

[philcunliffe]

Neutral review round: 4fe9620b (own PR)

Verdict: findings (1 medium), fixed and pushed as 5309d1dc.

Reviewed src/core/runtime/client_assets.js, test/core/client-assets-prune.test.js, the new llp/0266, and the Extended-by: line on llp/0219, at head SHA 4fe9620b24c2bd081a09728851151f8db9d1d1e5.

Verification performed on the change itself

• npm test -> 4485 pass / 0 fail / 1 skipped. npm run typecheck -> clean.
• Reverting the keepAll widening makes the new test a client-scoped run never prunes a destination another client still contributes fail, and leaves a client-scoped run still prunes a destination no client contributes any more passing. The pair genuinely pins both directions, so the "widened, not stood down" claim in the PR body is load-bearing and held.
• All four materializeClientAssets call sites (clients.js attach, runSkillsInstall, walkthrough.js finale, action_attach.js reconciler) pass a full descriptor map, so clients: 'all' really widens rather than being a no-op.
• claude and claude-desktop both declare .claude/skills and .claude/agents, and claude-desktop declares no attach_probe. Both premises of the rationale hold.
• planClientAssets filters on wantedbefore the unknown-client warning, so the second commit's justification for the silent widened pass is accurate.

Findings

1. medium - src/core/runtime/client_assets.js:385 (reconcileClientAssetLedger, the widened keepAll): the widening stops the deletion but nothing refreshes the surviving copy, and the record did not say so.

Reproduced with a scratch test: install hypaware-privacy with clients: ['claude','claude-desktop'] and body v1; upgrade so the source body becomes v2 and clients narrows to ['claude-desktop']; run a claude-scoped install. ~/.claude/skills/hypaware-privacy/SKILL.md still contains v1 afterwards, and the ledger keeps both the claude and claude-desktop records carrying the v1 digest. The scoped plan no longer contains the dest (so no copy), and the widened plan only answers "still contributed?" (it never copies), while the reconciler never attaches probe-less claude-desktop.

llp/0266 #consequences claimed the asset "survives the next scoped attach" and stopped there, which reads as if the shared-directory case is now fully handled. It is not: surviving is all it gets.

This is not a regression the PR introduces. Before the widening the same scoped run deleted that copy outright and nothing re-installed it, which is strictly worse than a stale copy. The change moves the failure from "gone" to "old". So the actionable defect was the honesty of the record, not the code.

Fixed in 5309d1dc: llp/0266 gains #open-kept-not-refreshed in #open-questions, next to the existing #open-stale-carried-digest, and the #consequences bullet now points at it instead of overclaiming.

Deliberately not changed in code, with the reasoning recorded in the new section:

• Having a scoped run copy every dest the widened plan keeps would make hyp attach claude install contributions nobody asked it to install, which is the same thing "the widened pass is silent" refuses on the reporting side.
• The narrower "copy only what lands in this client's own asset directories" changes what a scoped run is and deserves its own weighing, not a rider on this one.
• The refresh already exists, it is just not automatic: hyp skills install defaults to --client all, and an explicit hyp attach claude-desktop plans the dest.

Considered and declined

• The widened plan is computed above the activationIncomplete branch, where keepAll is never read, so a daemon reconcile with a failed plugin does one wasted in-memory registry walk. Left as is: the cost is a walk of already-loaded registries, and making keepAll conditionally empty on a delete path is a worse footgun than the walk is a cost.

Clean on everything else

options.clients === 'all' ? planned : ... is equivalent to re-planning; the stderr-stripped quiet spread carries every field planClientAssets reads; the ledger dedupes on (client, dest) so the "two records for one dest" price stays bounded; the @ref points at LLP 0266#contributions-not-the-run, an anchor that exists; llp/0219's only edit is the Extended-by: forward-ref, which is the mechanical edit an Accepted doc allows.

Nothing left open

Post-fix on 5309d1dc: npm test 4485 pass / 0 fail / 1 skipped, npm run typecheck clean, no em dashes, no semicolons introduced.

[philcunliffe]

Neutral triage at the review-round cap: head 5309d1dc

Outcome: no residual finding blocks the merge. The two deferred items are tracked in #921.

Both review rounds' fixable findings were fixed on the branch (round 1 findings 1-2 in 4fe9620b, round 2 finding 1's record-honesty half in 5309d1dc, which is docs-only relative to the last fully test-verified commit). What remains open at this head:

1. Stale carried digest (LLP 0266#open-stale-carried-digest, src/core/runtime/client_assets.js:423, :595) - non-blocking. Pre-existing on master, conservative failure (an asset is withheld from deletion with a misleading message, never wrongly deleted), and the fix requires a ledger-design decision of its own.
2. Kept dest is not refreshed (LLP 0266#open-kept-not-refreshed, src/core/runtime/client_assets.js:385-388) - non-blocking. Strictly better than master, where the same scoped run deleted the copy outright; a manual refresh path exists (hyp skills install, hyp attach claude-desktop).

Neither could cause a production defect this PR is responsible for: both are pre-existing or strictly-improved failure modes, both are recorded as open questions in the branch's LLP 0266 with the rejected quick fixes and why, and the PR's core behaviour (a client-scoped run no longer deletes a dest another client still contributes, while still pruning genuinely retired dests) is pinned by tests in test/core/client-assets-prune.test.js and was reproduced both ways by review round 1.

Merge-time caveat: #907 - this branch's llp/0266-* collides by number with two other open branches. Renumber per #907 whenever this is not the first of the three to merge (mechanical edit under LLP 0156).

Deferred-findings issue: #921.
ee are enumerated with file:line detail in #920.

[philcunliffe]

Closed as contained by merged #929. #929 includes the all-client candidacy fix from this PR and the per-path digest correction, with the final LLPs landed as 0284 and 0288.