Fix #1666: copyDir and hashTree disagree on symlinks, so a client-asset source holding one re-copies and rewrites the ledger every boot and can never heal - #1667
Conversation
… a symlink never settles (#1666) `copyDir` copies files and directories and skips every other entry, while `hashTree` folded one into the digest as an `o:<relpath>` line. A source tree holding a symlink could therefore never digest equal to the copy made of it: the refresh's unchanged check failed on every boot, the whole tree was re-copied, and the ledger was rewritten with identical content forever. The hasher now covers exactly the set the copier copies. Digests of trees without a special entry are byte-identical to before, so every recorded ledger digest stays valid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n decision doc Skipping an entry the copier cannot carry does not only make a source tree settle; it narrows what the ownership digest measures. A symlink dropped into an installed copy no longer moves the digest, so the refresh overwrites that copy instead of reporting `asset_edited` and a prune of the retired asset removes it instead of withholding it. LLP 0219 #edited-assets-are-not-ours is Accepted and reads the digest as the measure of "the user took this over", so the repo's own mechanism applies: a new LLP, noted on the old doc's `Extended-by:` line (the same move LLP 0400 made two commits ago). LLP 0401 records the direction, the rejected copier-side alternative, the migration evidence, and the narrowing. LLP 0226's supporting sentence that a symlink inside the tree "hashes as an opaque entry by name" no longer describes the code the `@ref` on `inspectClientAsset` points at, so it takes a forward-ref rather than an edit to what it settled. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The branch was cut before #1665 landed. Both changes touch LLP 0219's `Extended-by:` line, so the merge is resolved to carry both forward-refs (LLP 0400 for the healing evidence, LLP 0401 for the narrowing of what the digest covers). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review round 1 - head
|
| check | result |
|---|---|
npm test |
6580 tests / 6576 pass / 3 fail / 1 skipped |
npm run typecheck |
clean |
node scripts/llp-numbers.js check |
1 LLP number minted against refs/remotes/origin/master, no collision |
| llp-check duplicate-numbers (run by hand) | no duplicates |
The 3 failures are pre-existing and environmental, all in test/core/hyparquet-floor-pin.test.js (icebird uses the root parquet pins..., every read-path dependency that carries hyparquet is held at the floor, the read path resolves the one root hyparquet...). Reproduced on clean origin/master in a separate worktree: # tests 9 / # pass 6 / # fail 3. Not introduced here. The 1 skip is the unrelated ZSTD-availability test, not the uid-0 refresh skip: this ran as uid 1001, so the refresh suite had full coverage.
Style: no em dashes, no NUL bytes, no semicolons, no new runtime dependencies, no invented config keys or schema fields in any changed file (checked with grep -P '\xe2\x80\x94|\x00' over every file in the diff).
One thing I did not land
The narrowing in finding 1 is recorded in LLP 0401 but not pinned by a test. A test asserting that a symlink added to an installed copy leaves the digest matching (and therefore that the prune removes rather than withholds) would stop it drifting back silently, in the way #1665 pinned its own steady state. Left out deliberately under "stop when tests pass" and "smallest change"; noting it here rather than filing an issue, since it is a test for behavior this PR documents rather than a concrete defect.
`hashTree` now mirrors `copyDir`'s predicate, but the invariant lived only in the hasher: `fs_copy.js` said nothing about a second reader depending on what it carries. Widening `copyDir` alone is exactly the edit that reintroduces issue #1666, and its only symptom is an endless `refreshed` line. Comment and `@ref` only, no behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review round 2 (final) - head
|
| what the user did to the installed copy | origin/master |
head 02d71118 |
|---|---|---|
| dropped a symlink in | withheld 1, dir kept |
pruned 1, dir removed |
| dropped a file in | withheld 1, dir kept |
withheld 1, dir kept |
| nothing | pruned 1 |
pruned 1 |
The withheld line in both the unchanged rows is the full warning: retired skill 'alpha' at ... changed since HypAware installed it; left in place - remove it by hand if you no longer want it. So the change is exactly one row wide, and it is the row 0401 #edit-detection-narrows describes.
Whose files can be lost, and what the sequence is. The user drops a symlink (or a fifo, socket, or device node) into a directory HypAware installed under ~/.claude/skills, HypAware later retires that skill, and the prune removes the directory rather than naming it. What is lost is the entry itself, a pointer or a device node. Nothing it points at is touched: fs.rm(dest, { recursive: true, force: true }) lstats and unlinks a symlink rather than following it, which I confirmed directly for both a symlink to a file and a symlink to a populated directory, and again in the table above (the precious.txt outside the tree reads back intact in every row). Any file the user adds, any directory they add, and any edit to the bytes of a file already there all still move the digest and still stop both the prune and the refresh, as the middle row shows.
So the narrowing cannot silently destroy user content; it can destroy a user-made reference inside a directory HypAware installed and has since retired. 0401 states that plainly, including the "(the link with it, never its target)" clause, and states the price honestly against the alternative: covering the entry buys edit detection for one entry kind and pays with a tree that can never settle, is re-copied on every boot, and whose next copy overwrites the user's entry anyway. Correctly scoped and safe. Not a blocker.
The other claims, re-verified
Migration safety - confirmed, my own way. Rather than re-running round 1's spot list, I ran both hashers side by side (pre-fix lifted from origin/master, post-fix from the branch) over every directory in the tree at unbounded depth, 139 roots. Exactly one differs, and it is the repo root, because a git worktree materializes the tracked CLAUDE.md -> AGENTS.md symlink there (git ls-files -s CLAUDE.md -> mode 120000). Independently: find . ! -type f ! -type d over the whole tree returns that one path and nothing else, and git ls-files -s shows exactly one non-100644/100755 mode in the index. The repo root is not a client-asset source; every skills/ and agents/ tree under hypaware-core/plugins-workspace/{claude,codex,gascity} and .claude/skills is symlink-free and digests byte-identically. No recorded ledger digest changes.
The trigger is real, and more reachable than the issue claims. fetchLocalDir copies a local-dir plugin with copyArtifactTree, which recreates symlinks as symlinks (src/core/plugin_install/fetch.js:158-160). So an installed local-dir plugin's skills/ tree genuinely can hold one, and that tree is then the asset.source copyDir reads. This is not a hypothetical entry kind. (It also does not undercut 0401's security argument, which is specifically about materializing a pointer into ~/.claude/skills, a third-party client's content directory, not into HypAware's own plugin state.)
Regression test - non-vacuous, two independent ways.
- The PR's test file dropped onto clean
origin/mastercode:# tests 20 / # pass 19 / # fail 1, and the single failure isnot ok 4 - a source holding a symlink settles instead of re-copying on every boot,expected: 1, actual: 0. So it fails on unfixed code and nothing else fails. git checkout origin/master -- src/core/runtime/client_asset_ledger.jsin the merged tree, keeping the test: all three asset suites give# tests 48 / # pass 47 / # fail 1, the same single failure. Restored:48/48.
The mtime assertion is what makes it a test of LLP 0400 #one-write rather than of the counters: the post-loop write is gated on observed.size > 0 (client_assets.js:310), so a byte-identical rewrite is invisible except through the file's mtime.
Interaction with #1665's healing path - the heal does the right thing, and it now fires. sourceDigest === onDisk is newly reachable for a symlink-bearing source, so #1665's heal gate is too. Driven directly: install a source holding a symlink, corrupt the ledger digest to simulate a pass killed after replaceAsset's swap and before the single ledger write, then refresh. Result {"unchanged":1,"healed":1,"refreshed":0,"skipped":[]}, and the ledger comes back carrying digestClientAsset(dest). That is correct: the copy is ours, being exactly what copyDir produces from the current source. Before this PR that asset could never heal, so the two changes are complementary, not merely compatible. The new false-positive surface is the same one 0401 already documents (a user symlink in the copy makes it digest equal to the source, so the heal reads it as ours), and the heal is keyed by dest, so two sources that now digest alike cannot cross-claim each other's records.
Stale readers of o:<rel> - none. All five digest consumers go through the same digestClientAsset/inspectClientAsset (client_assets.js:171,182,207,289,720,817), nothing parses the hash input, and no test or fixture carries a literal digest. grep for o: framing and "opaque entry" across src/ test/ hypaware-core/ llp/ bin/ docs/ leaves exactly one prose hit outside 0401: llp/0226:110, which is the sentence this PR immediately annotates with the forward-ref. copyDir still has one consumer.
Findings
1. src/core/runtime/client_asset_ledger.js:258-276 - low - hashTree frames each entry's path but not the length of the bytes that follow, so two distinct trees can digest identically. Disposition: defer (issue #1669).
Reproduced, and reproduced identically on origin/master, so the PR neither introduces nor worsens it:
tree A: { a: "", b: "hello" } tree B: { a: "f:b\nhello" }
both -> 561a36c026fd225bfef889ab7b15796c46e4d8aba852f55615e7248d6b02573c
Consequence: a user who edits an installed skill's file to end with the exact framing line of the next sorted sibling and deletes that sibling keeps the recorded digest, so pruneOneAsset reaches recorded.has(digest) === true (client_assets.js:864) and fs.rms their copy, and the refresh calls it unchanged (client_assets.js:252). It needs a crafted edit, not an accident, but it is precisely the property the hasher's own JSDoc claims to defend, on a delete path.
Deferred rather than fixed here, for a reason specific to this PR: the obvious repair (frame the byte length) changes every digest the hasher produces, invalidating every digest already in client-assets.json and reporting every installed asset as asset_edited on the next boot. That is the exact migration hazard LLP 0401 #migration rejects the copier-side alternative for, so it needs a migration decision of its own (versioned digest prefix, a one-boot re-record, or an accepted asset_edited wave) rather than a hunk inside a symlink fix. Concrete, consequential, and clearly outside the current task, so filed: #1669.
2. src/core/util/fs_copy.js:12-21 - low - the invariant this PR creates lives only in the hasher; the copier it pins to says nothing. Disposition: fix (landed).
After this change hashTree is defined in terms of copyDir's predicate and carries @ref LLP 0401#digest-covers-the-copy plus a comment naming the copier as the authority. copyDir carried nothing. Its JSDoc said symlinks are skipped "which is what the skill/agent installers want", with no hint that a second reader is pinned to the same two predicates. Widening copyDir alone - teaching it to recreate symlinks, which is the alternative LLP 0401 explicitly contemplates and rejects, so it is a live future edit - makes the digest cover less than the copy carries and reintroduces #1666, with the only symptom an endless refreshed line. This is the "annotate non-obvious decisions" case, not a mechanical ref: it tells a future editor something neither the code nor the filename does.
Fixed in d3133d60 with four lines of JSDoc and one @ref LLP 0401#digest-covers-the-copy [constrained-by] on copyDir. Comment only, no behavior change.
Round 1's three findings, re-checked: finding 1 (mint LLP 0401) and finding 2 (0226 forward-ref) both landed and are correct, as detailed above. Finding 3 (win32 guard on fs.symlink) stays rejected on the same counter-evidence, which I re-ran: grep -rl skipSymlinks test/ returns 3 files, all path-containment suites where the symlink is the subject, out of the ~20 test files that create one; test/core/usage-policy-symlink.test.js is an entire symlink suite with zero win32 mentions; all CI jobs are ubuntu-latest.
Round 1's one deliberate omission (no test pinning the narrowing itself): I looked for a reason to overturn it and found the opposite. A drift back to hashing the skipped entry is already caught, hard, by the new refresh test - I proved that by reverting the file and getting exactly that one failure. A second test asserting the prune now removes rather than withholds would pin the consequence of a change the corpus documents, at the cost of a test whose failure mode is the same as one that already exists. Rejected, not deferred.
What I pushed
New head: d3133d60 (was 02d71118). One commit, src/core/util/fs_copy.js only, comment and @ref.
Verified it landed by diffing the committed tree against the pre-fix head rather than trusting a green suite: git diff 02d71118..HEAD is exactly the one file, +7/-0, showing the four JSDoc lines and the @ref LLP 0401#digest-covers-the-copy [constrained-by] tag. #digest-covers-the-copy resolves (llp/0401-...:43). Not a behavior change, so there is nothing to pin: npm test is unchanged at 6576 pass and typecheck is clean after it.
CPU and memory pass
hashTree, per entry. Thecontinueis placed beforepath.join, so a skipped entry now costs twoDirentpredicate calls and nothing else: nopath.join, nopath.relative, nohash.update, noBuffer. Strictly less work and strictly less allocation than theo:line it replaces. For a kept entry the predicate count is unchanged - the old ternary chain already calledisDirectory()/isFile()twice each, and the new one calls them twice total plus the guard's two, so the arithmetic is a wash and the allocation is identical.entries.sortstill sees skipped entries, which is one comparison each and no allocation. No recursion change, no new retained state.- The macro win is the point of the fix and it is real. For a symlink-bearing asset every boot previously ran
fs.rm(dest, -r), a fullcopyDirof the tree, a re-hash of the copy, and aclient-assets.jsonwrite. All four are gone permanently, replaced by the one source hash the unchanged path already pays.refreshClientAssets's single post-loop write stays gated onobserved.size > 0(LLP 0400#one-write), and the new test pins the write half by mtime so it cannot silently come back. - Round 2's own change adds comment text and no code, so zero runtime cost.
- Unbounded growth, busy loops, per-record work: none. Work stays bounded by the ledger rather than by uptime, which was 0397's stated bound and is now actually true for every source tree instead of only symlink-free ones. The one thing that grows with data volume, hashing a tree, is unchanged per kept entry and cheaper per skipped one.
No CPU or memory concern. This change is a net reduction in both.
Checks (final head d3133d60)
| check | result |
|---|---|
npm test |
6580 tests / 6576 pass / 3 fail / 1 skipped |
npm run typecheck |
clean |
node scripts/llp-numbers.js check |
1 LLP number minted against refs/remotes/origin/master, no collision |
llp-check duplicate-numbers (run by hand) |
no duplicates |
GitHub CI on 02d71118 |
all 8 checks pass |
The 3 failures are pre-existing and environmental, all in test/core/hyparquet-floor-pin.test.js. Reproduced on clean origin/master in a second worktree: # tests 9 / # pass 6 / # fail 3, the same three. They also pass in GitHub CI on this head, so they are this container, not the branch. The 1 skip is the ZSTD-availability test, not the uid-0 refresh skip: this ran as uid 1001, so the refresh suite had full coverage.
Style: no em dashes, no NUL bytes, no trailing semicolons, no new runtime dependencies, no invented config keys or schema fields, in every file the PR touches and in the file round 2 touched (checked with grep -P '\x{2014}|\x00' over each).
Finding disposition - head
|
| # | finding | disposition |
|---|---|---|
| 1 | (R1) LLP 0219 narrowing unrecorded | resolved at head - nothing remains |
| 2 | (R1) LLP 0226 stale mechanism sentence | resolved at head - nothing remains |
| 3 | (R1) win32 guard on fs.symlink in the test |
rejected - counter-evidence re-verified |
| 4 | (R2) hashTree framing collision |
deferred - issue #1669 |
| 5 | (R2) copyDir carries no invariant annotation |
resolved at head - nothing remains |
1 - resolved. Round 1's repair stands in the tree: llp/0401-the-asset-digest-covers-what-the-copier-carries.decision.md exists, Status Accepted, and its anchors resolve (#digest-covers-the-copy :43, #migration :61, #edit-detection-narrows :69). git diff origin/master...HEAD on LLP 0219 is exactly the one-line Extended-by: append carrying 0401 alongside 0226/0284/0400, the only latitude an Accepted doc allows. client_asset_ledger.js carries @ref LLP 0401#edit-detection-narrows [constrained-by] at :181 and @ref LLP 0401#digest-covers-the-copy [implements] at :265. The narrowing the doc records is real and correctly scoped: I re-confirmed fs.rm unlinks a symlink without following it (targets of a link to a file and to a populated directory both intact after the rm), so the prune change can remove a user-made reference inside a retired HypAware directory, never user content, and file/directory additions and byte edits still withhold.
2 - resolved. LLP 0226 :108-113: the settled sentence survives verbatim to its final full stop, followed by the parenthetical forward-ref to LLP 0401 (4+/1-, annotation not amendment). The @ref LLP 0226#unreadable-is-not-absent at client_asset_ledger.js:223 again resolves to a paragraph matching the code.
3 - rejected, counter-evidence re-run. grep -rl skipSymlinks test/ returns exactly 3 files, all path-containment suites where the symlink is the subject, out of 21 test files that create symlinks; test/core/usage-policy-symlink.test.js (an entire symlink suite) and the PR's new test both have zero win32 mentions; all six CI jobs across ci.yml and llp-check.yml are runs-on: ubuntu-latest. The guard would import a convention the repo does not hold.
4 - deferred to #1669, and the deferral holds on adjudication. Reproduced myself on both heads: {a:"", b:"hello"} and {a:"f:b\nhello"} digest to the same 561a36c0... under digestClientAsset at d3133d60 and on clean origin/master (0bd00358), byte-identical, so this PR neither introduces nor worsens it. Pre-existence alone would not make it deferrable; what does is (a) the obvious repair (frame the byte length) changes every digest the hasher produces, invalidating every recorded client-assets.json digest and marking every installed asset asset_edited on the next boot - the exact migration hazard Accepted LLP 0401 #migration rejects the copier-side alternative for - so the repair needs a migration decision of its own (versioned digest prefix, one-boot re-record, or an accepted asset_edited wave), clearly outside a one-issue symlink fix; and (b) the delete-path consequence needs a deliberately crafted self-edit to a copy the user already owns, never an accident or a remote trigger. Evidenced defect, so it keeps neutral:fix.
Issue identity note: neutral defer-findings keys reuse to the head-keyed marker, so it could not match round 2's #1669 and created #1670. Reconciled to the single identity the record requires: #1670 is closed as a duplicate with its marker defused, #1669 now carries the d3133d60-keyed deferred-finding marker, and a re-run returns {"ordinal":4,"number":1669,"reused":true} creating nothing.
5 - resolved. The head commit itself (d3133d60) is exactly +7/-0 comment lines on src/core/util/fs_copy.js: the JSDoc names hashTree as mirroring the predicate, warns that widening copyDir alone reintroduces #1666, and carries @ref LLP 0401#digest-covers-the-copy [constrained-by], whose anchor resolves.
Checks at d3133d60: all four client-asset suites pass (48/48 across refresh/prune/daemon-refresh, install suite green), npm run typecheck clean. The three hyparquet-floor-pin failures reproduce on clean origin/master here and pass in GitHub CI, so they are environmental. Accepted LLPs 0219, 0226, 0284, 0400 are untouched beyond the forward-ref/annotation latitude; 0397 is Draft and its bullet rewrite is in order; 0401 is the one new doc.
Adjudication consumes no review round; no neutral-review marker is added.
philcunliffe
added
the
neutral:approved
Ship risk:
|
1 participant
Feature or issue
copyDircopies files and directories and skips every other entry (src/core/util/fs_copy.js), whilehashTreefolded any other entry into the digest as ano:<relpath>line (src/core/runtime/client_asset_ledger.js). A client-asset source tree holding a symlink could therefore never digest equal to the copy the materializer makes of it, so the boot refresh'ssourceDigest === onDiskcheck failed every time: the whole tree was re-copied, the new digest was recorded, and the ledger was rewritten with byte-identical content on every daemon boot, forever, visible only as an endlessrefreshedline. No shipped skill source holds a symlink today, butlocal-dirplugin sources (src/core/plugin_install/resolver.js) can carry one.Solution
hashTreenow covers exactly the setcopyDircopies: an entry that is neither a file nor a directory is skipped rather than hashed by name, so a source tree and its copy digest equal and the boot after a copy reportsunchangedand writes nothing.copyDirto recreate symlinks) on three counts. Security: materializing a link verbatim into~/.claude/skillslets a plugin tree place a pointer to anywhere on disk, including outside the tree, into a directory the client loads as skill content; skipping adds no capability the installer did not already have. Completeness:copyDircan recreate a symlink but not a fifo, socket, or device, so that direction leaves the same bug for the other entry kinds. Migration: copying symlinks changesdigest(copy), which would invalidate every ledger digest recorded for such a tree and freeze the asset asasset_edited; this direction leavesdigest(copy)untouched. Verified: digests ofsrc,src/core/cli, and theclaudeplugin tree are byte-identical before and after, so every recorded digest stays valid. The tradeoff accepted is that a special entry dropped into an installed copy after the fact no longer registers as a user edit; its files and directories still do.unchangedis 0, not 1) and passes after; reverting only theclient_asset_ledger.jshunk fails it again and nothing else. CPU/memory: the per-boot re-copy and the per-boot ledger write are gone for such an asset, the hash does strictly less work per skipped entry, and no new per-asset write is introduced.npm test6573 pass / 3 fail, all three pre-existing environmental failures intest/core/hyparquet-floor-pin.test.jsthat reproduce on cleanorigin/master;npm run typecheckclean;node scripts/llp-numbers.js checkclean.Code: +12 / -9 lines
Fixes #1666