purge's near-miss note names all three causes, not two (#497)

[philcunliffe]

Deferred, non-blocking review findings from #493, actionable now that it has
merged. Each was re-verified against master before any code was written.

No maintainer decision in the thread. gh issue view 497 --json comments
returns {"comments":[]}, so nothing overrides the brief.

Finding 1: sameDirectoryOnDisk swallows every stat error - behavioural (message-only), FIXED

Verified on master. The premise holds. sameDirectoryOnDisk catches any
error from statSync and returns false, so an alias spelling that is present
on disk but unreadable retains exactly like one that is absent, and the run has
no way to tell them apart. The stderr note enumerated two causes as though they
were exhaustive:

(genuinely different, or no longer on disk)

For a swallowed EACCES / ELOOP / ENOTDIR neither member holds: nothing
adjudicated the two spellings different, and the spelling is not gone.

Fix. The note now reads (genuinely different, no longer on disk, or could not be checked), which asserts none of the three. The same three-cause
enumeration is now carried in PurgeSummary's contract comment
(src/core/cache/types.d.ts), buildPredicate's retainedAliases JSDoc, and
LLP 0104 #spellings.

The swallow itself is kept, and is now documented as deliberate
(sameDirectoryOnDisk JSDoc). Widening a deletion on a failed probe would
delete rows under a directory no filesystem ever identified with the target, so
every errno has to collapse to the same false. What the collapse bounds is the
message, not the decision.

FAIL on master

New test run against pristine origin/master source in a separate worktree:

not ok 30 - the near-miss note does not say "no longer on disk" of an alias that is on disk but unreadable
  ---
  location: '/work/wt-497-base/test/core/purge-command.test.js:783:1'
  failureType: 'testCodeFailure'
  error: 'the enumeration has to admit the cause this run actually hit'
  code: 'ERR_ASSERTION'
  actual: |-
    note: 1 cached row under a similarly spelled directory was left in place - this filesystem does not report it as the directory you named (genuinely different, or no longer on disk):
      /tmp/hyp-purge-eloop-eEumtH/café/sub
    tip: purge that exact spelling too if you meant it as well
  operator: 'match'
  ...
# pass 30
# fail 1

PASS on this branch

ok 30 - the near-miss note does not say "no longer on disk" of an alias that is on disk but unreadable
ok 31 - no stat errno makes an unprovable alias deletable
# tests 31
# pass 31
# fail 0

The fixture uses a self-referential symlink (ELOOP) rather than a chmod-ed
ancestor (EACCES): it needs no injected statSync, so it exercises the real
predicate end to end through runPurge, and unlike a permission bit it still
fails the stat if the suite ever runs as root.

Proof that no deletion decision moved

Three independent lines of evidence.

1. Structural. The entire src/ change set, with comment lines removed, is
one string literal:

$ git diff -U0 -- src/ | grep -E '^[+-]' | grep -vE '^(\+\+\+|---)' | grep -vE '^[+-]\s*(//|\*|/\*)'
-      `(genuinely different, or no longer on disk):\n`
+      `(genuinely different, no longer on disk, or could not be checked):\n`

No predicate, branch, return value or type changed. The remaining src/ diff is
JSDoc and comments.

2. Asserted in the new test. The ELOOP test asserts the retention as well
as the wording (purged 0 rows, and the row still present in the table
afterwards). Those assertions passed on master too - only the wording
assertion failed - so the run is a direct before/after on the deletion decision
for the exact case the fix is about.

3. Pinned against future regression. test('no stat errno makes an unprovable alias deletable') drives scopeGovernance(..., { proveAliases: true })
with an injected statSync throwing each of ENOENT, EACCES, EPERM,
ELOOP, ENOTDIR, EIO and asserts 'aliased' every time, never 'governs'.
This reproduces triage's by-execution claim as a standing test, so a later
attempt to "improve" the swallow by treating some errno as reachable fails here
rather than in someone's cache.

Finding 2: symlink-inside-target - pre-existing, NOT fixed, documented instead

Verified on master. Reproduced directly against pristine origin/master
with target <root>/proj, a real directory <root>/elsewhere/sub, and
<root>/proj/link -> <root>/elsewhere:

realpath(cwd) = /tmp/probe-symlink-qjzFYf/elsewhere/sub
scopeGovernance(cwd, target, proveAliases) = governs
scopeGovernance(cwd, target) plain        = governs
scopeGoverns (gate shape)                 = true

Confirmed as filed: identical with the proveAliases opt-in off, so it is
the plain canonicalSpellings / matchDepth match from #482, not #493's fold.

Concluded not fixable here. The third line is the decisive one: the
unwidened gate predicate returns true for the same fixture. So hyp policy show, hyp ignore --check and the adapter gate all consider that row inside
the target. Making purge alone retain it would desynchronise purge from the
gate and directly break the first consequence LLP 0104 already commits to:

hyp ignore --check's residual count stops being a dead end: it can now
point at the verb that clears it.

The user would be told rows are in scope by one verb and refused by the other,
with no spelling they could name to clear them. That is a change to
LLP 0050 §canonicalization's set-of-spellings semantics, with its own disclosure
argument, not a consequence of #493 or of this PR. The issue itself says it is
"worth a decision, separately".

Recorded as a ## Consequences bullet in LLP 0104 so the next reader finds the
reasoning rather than rediscovering the behaviour, and left open for a
maintainer decision.

Finding 3: @ref separated from its construct - prose/annotation, FIXED, no test by construction

The blank line between the @ref LLP 0104#spellings [tests] and the test(...)
it annotates at test/core/purge-command.test.js:718 is closed. No test -
this is an annotation attachment convention with no runtime effect, so there is
nothing to assert; manufacturing one would be theatre.

What I deliberately did not change, and why

The neighbour at line 442 is left as it is, and a broader normalization is
left to a maintainer. Evidence: this shape is not a slip in this file, it is the
established convention across the test corpus. Scanning every @ref in src/,
test/ and hypaware-core/ for one followed by a blank line finds 31, in
28 files, essentially all of them section-level headers over a block of tests
(usage-policy-symlink.test.js, status-layered.test.js,
config-merge.test.js, cli/wizard/*.test.js, and so on).

Two further notes for whoever takes that decision:

• CLAUDE.md is the only place the "a blank line breaks attachment" rule is
  written. LLP 0000 does not define attachment semantics, although
  .claude/skills/ref-check/SKILL.md cites it as if it did.
• Closing the blank at line 442 specifically would attach a section-level gloss
  ("purge deletes an aliased spelling the filesystem proves...") to const NFC,
  which is a worse annotation than the one it replaces.

I did not amend CLAUDE.md. Rewriting the repo's agent-instruction file to
dissolve a convention finding is not a call an autonomous worker should make
unilaterally, however good the corpus evidence.

Checks

Both suites run serially in separate worktrees, never concurrently.

Check	Result
npm test (pristine origin/master baseline)	# tests 3087 # pass 3078 # fail 8 # skipped 1
npm test (this branch)	# tests 3089 # pass 3080 # fail 8 # skipped 1
npm run typecheck	exit 0
npm run smoke -- purge_removes_cached_rows	ok

The 8 failures are the pre-existing test/core/leave-command.test.js ones,
confirmed by name against my own baseline and byte-identical between the two
runs: leave after join removes the seed and reports the server, leave clears an applied central slot, not just the seed, leave reverses org-driven attaches and drops the forward identity, leave after join also warns about a local central sink that keeps forwarding, leave is idempotent: a second leave is the not-connected no-op, leave still tears down when only a stale attach marker survives a prior partial leave, leave removes the assets its attach marker records, and leaves manual copies alone, leave self-heals an org attach whose plugin is gone: drops the marker, warns, stays clean. Test count rises by
exactly the 2 tests added; pass count by the same 2.

@ref validation over the touched files: every annotation resolves
(LLP 0104#spellings, LLP 0050#canonicalization, LLP 0050#normalization,
LLP 0049#fail-safe all exist; LLP 0104 is Accepted). No new LLP number was
minted, so nothing collides with 0160-0164.

No collision with work in flight

File lists pulled live with gh pr diff <n> --name-only. #491
(fix/issue-481) and #502 (fix/issue-421) share no file with this branch.

Fixes #497

[philcunliffe]

Neutral review, round 1 of 2

Audited 2eb6f63 (the head when the round opened). One LOW finding, fixed and pushed;
head is now 5b19c3d. Direct audit: the code-review skill is not installed in this
container and codex is not on PATH, so everything below is hand-run.

Verdict: approve the substance. The message-only claim is true, and I verified it
rather than accepting the author's evidence. The one thing I found is in the new test's
fixture, not in the fix.

---

The central claim: is this message-only?

Yes. Executed, and by a stronger method than the PR body's.

The body's proof (a) is a grep -vE '^[+-]\s*(//|\*|/\*)' over the diff, which is a
line-shape heuristic: it would miss a behavioural change that happened to sit on a line
starting with *, and it cannot see a change inside a multi-line comment. I re-ran it
as a real tokenizer pass instead. I stripped every comment from both revisions of each
src/ file with a stateful scanner that tracks string, template and escape context,
dropped blank lines, and diffed the results:

file	base vs head, after full comment strip
src/core/cache/purge.js	identical
src/core/usage-policy/fold.js	identical
src/core/cache/types.d.ts	identical
src/core/commands/purge.js	one line

-      `(genuinely different, or no longer on disk):\n`
+      `(genuinely different, no longer on disk, or could not be checked):\n`

Claim (a) holds. No predicate, branch, return, type or control-flow edge moved.

Independent verification that no deletion decision moved

I did not take the author's word or the test's word for this. I built the cases and ran
them against the head tree (scopeGovernance, proveAliases: true, real NFC/NFD fixture).

Executed. Every swallowed error resolves to aliased, never governs:

• 13 injected errnos (ENOENT EACCES EPERM ELOOP ENOTDIR EIO EMFILE ENAMETOOLONG EOVERFLOW ENOMEM EBUSY EROFS ETIMEDOUT) -> aliased, all 13. This is a superset of
  the 6 the new test pins.
• Error thrown on the second stat only (the target side, not the alias side) ->
  aliased.
• realpathSync throwing inside canonicalSpellings (EACCES, ELOOP, ENOENT) ->
  aliased. The failure cannot leak in through the canonicalization side either.
• Real ELOOP, no injection at all, self-referential symlink, uid 1001:
  sameDirectoryOnDisk -> false, scopeGovernance -> aliased, plain -> outside.
• Positive control: a genuinely proven alias (symlink, real dev/ino identity)
  still returns governs. The retention is not coming from the predicate being inert.

Reasoned, from reading matcher.js:711-727: in the proveAliases branch the only
edge to 'governs' is a true from sameDirectoryOnDisk at line 722; a false sets
aliased = true and falls through to return aliased ? 'aliased' : 'outside'. There is
no third path. Combined with the execution sweep above, a swallowed stat error cannot
reach a deletion.

The new test is not vacuous, and it closes a real hole

Mutation-tested, executed:

mutation	result
revert the string literal to master's wording	reddens only test 30
sameDirectoryOnDisk returns true on any error (widen)	reddens tests 28, 29, 30, 31
return errnoOf(err) !== 'enoent' (privilege ENOENT, widen the rest)	reddens only tests 30 and 31

That last row is the interesting one. It is exactly the "improvement" the author says
test 31 exists to prevent, and I ran it against pristine master's full suite: it
passes all 3087 tests undetected. So before this PR, a change that silently widened a
destructive predicate for every non-ENOENT errno had zero test coverage. Test 31
closes that. This PR is a genuine safety improvement, not only a wording fix.

Fixture claim

Confirmed executed: the self-referential symlink yields a real ELOOP from stat with
no injected statSync, driving the real predicate through runPurge. Reasoned: ELOOP
is returned by kernel path resolution regardless of credentials, so unlike a chmod'd
ancestor it does still fail as root. The choice is correct.

---

Finding 1 (LOW, test-only): the ELOOP fixture EEXISTs on macOS. Fixed in 5b19c3d.

test/core/purge-command.test.js:789-792 (at 2eb6f63)

This is the only fixture in the corpus that creates both an NFC and an NFD entry on
one real volume; every other NFC/NFD test either injects statSync or uses spellings
that exist on no host. On a normalization-insensitive volume (APFS, HFS+)
path.join(root, NFD) already names the directory created by the line before, so

await fs.mkdir(path.join(root, NFC))
await fs.symlink(path.join(root, NFD), path.join(root, NFD))   // EEXIST on APFS/HFS+

errors before asserting anything. Demonstrated by execution: with the entry present at
the symlink path, symlink returns EEXIST; on ext4 both calls succeed and the
directory has 2 entries.

CI is ubuntu-latest only, so this never reddens in CI, which is why it is LOW rather
than higher. But it fails on every Mac, and this project treats macOS as first-class
(launchd install paths, APFS reasoning throughout this very file).

It also makes a claim added in the same diff untrue. The section comment at line 718-719
says the tests "use spellings that exist on no host, or exist but cannot be read, so
none of them depends on whether the test volume folds". True of the others; this one
depends on the volume not folding.

Fix pushed: guard the fixture with the file's own established idiom
(t.skip(...) + return, as at first-sync-hold.test.js:139), and correct the section
comment. A folded volume cannot host this cause at all (there the alias is proven and
deleted), and the ENOENT case above still covers the retention on such a host.
Verified the guard cannot fire on Linux (the NFD entry does not exist pre-symlink), and
re-ran both mutations afterwards: tests 30 and 31 still redden exactly as before, so the
guard did not weaken them.

---

The two deliberate non-changes: both correctly deferred

1. Symlink-inside-target. The characterisation is accurate, verified by execution
against both head and pristine master, byte-identical results:

cwd as-given   /tmp/.../proj/link/sub      realpath  /tmp/.../elsewhere/sub
scopeGovernance proveAliases = governs
scopeGovernance plain        = governs      <- opt-in OFF, so not #493's fold
scopeGoverns (GATE shape)    = true         <- the unwidened gate agrees

Both halves of the author's argument check out: it is the plain
canonicalSpellings/matchDepth match and predates proveAliases, and the unwidened
gate predicate returns true for the same fixture. Making purge alone retain it would
leave one verb saying rows are in scope and another refusing to clear them, with no
spelling the user could name. Correctly deferred as an LLP 0050 canonicalization
question, and correctly recorded as an LLP 0104 consequence rather than silently left.

2. CLAUDE.md not amended. Agreed, and not filed. Declining to rewrite the repo's
agent-instruction file to dissolve one's own finding is the right call.

Spot-check of the 31-in-28 count: the number is understated. Counting // @ref
lines followed by a truly empty line across src/, test/, hypaware-core/, I measure
38 in 34 files on the head tree (39/34 on master; the diff between them is exactly
the one closed in purge-command.test.js, which I verified). All 38 are in test/;
src/ and hypaware-core/ have none. I sampled several and they are genuine
file-header @refs sitting above the import block. Every example the body names
(usage-policy-symlink.test.js, status-layered.test.js, config-merge.test.js,
cli/wizard/*.test.js) appears in my list, and line 442 is there too, exactly as
described. So the substantive claim (established corpus-wide convention, not a slip in
this file) is verified and in fact stronger than stated; only the arithmetic is off
by about 7. Not a defect in the change, but worth correcting if the number is quoted
again. Per the brief I am not filing the convention gap itself. For the record,
llp-ref-hygiene.test.js enforces @ref spelling but not attachment, so nothing in
CI contradicts the corpus convention.

---

Other checks

• Completeness of the fix across surfaces. Executed: the only remaining occurrence
  of the old two-cause string anywhere in the repo is the intentional
  assert.doesNotMatch in the new test. The --json path
  (commands/purge.js:118-125) and the purge.result log carry counts and paths, no
  prose, so there is no second surface still under-enumerating.
• Accuracy of the new prose. The three-cause enumeration in types.d.ts,
  buildPredicate's JSDoc, sameDirectoryOnDisk's JSDoc, the runPurge comment and
  LLP 0104 all agree with each other and with what I observed by execution.
• LLP refs resolve. LLP 0104 {#spellings}, LLP 0050 {#canonicalization},
  {#normalization} all exist; LLP 0104 is Accepted; the new consequence bullet
  matches my probe output exactly. llp-ref-hygiene.test.js passes 9/9.
• House style. No U+2014 (nor U+2013/U+2212) in any added line, no trailing
  semicolons, no @typedef, no inline import('...') types, no trailing whitespace.
  Same checks pass on my commit.

Check results

Two suites run serially in separate detached worktrees, each with node_modules
symlinked, never concurrently. Baseline is my own, from pristine origin/master.

check	baseline (origin/master)	head 2eb6f63	after my 5b19c3d
npm test	3087 / 3078 pass / 8 fail / 1 skip	3089 / 3080 / 8 / 1	3089 / 3080 / 8 / 1
npm run typecheck		exit 0	exit 0
npm run smoke -- purge_removes_cached_rows		ok	ok

The 8 failures are the pre-existing test/core/leave-command.test.js ones. Confirmed
by name against my own baseline run, byte-identical across all three runs
(not ok 892, 893, 894, 896, 897, 898, 899, 900). The PR body's counts reproduce
exactly. Test count rises by the 2 tests added and no more.

---

Round 1 of 2 complete. The fix is correct, the proof of it is sound, and the deferrals
are justified. The one finding is in test portability, is fixed, and does not touch the
deletion path. Still a draft, so no merge action taken.

[philcunliffe]

Verdict: approve with two low-severity findings, both fixed and pushed

The change is sound and does what #497 finding 1 asks. I re-derived the central
safety claim independently rather than taking the PR body's word for it:
sameDirectoryOnDisk swallows every errno to false, scopeGovernance turns
that into aliased (never governs), and the only src/ behavioural change in
the diff is one string literal. No deletion decision moves. The ELOOP fixture
is a good choice: it drives the real predicate through runPurge end to end
with no injection, and it fails the stat even as root, which a chmod-ed
ancestor would not.

I also reproduced finding 2 (symlink-inside-target) against this head to check
the new LLP bullet is telling the truth, and it is:

realpath(cwd) = /tmp/probe-symlink-7QeHAO/elsewhere/sub
scopeGovernance(cwd, target, proveAliases) = governs
scopeGovernance(cwd, target) plain         = governs
scopeGoverns (gate shape)                  = true

The unwidened gate predicate really does return true for the same fixture, so
the argument for documenting rather than fixing it here (purge would desync from
hyp ignore --check) holds. Deferring it is the right call.

Checks on my head (756dabc): npm test gives `# tests 3089 # pass 3080

fail 8 # skipped 1`, unchanged from this PR's own head since my fixes add

assertions rather than tests. The 8 are the pre-existing
test/core/leave-command.test.js failures, identical by name to the PR's
baseline. node --test test/core/purge-command.test.js is 31/31 green. npm run typecheck exit 0. Every @ref
in the touched files resolves (LLP 0104, LLP 0104#spellings,
LLP 0050#canonicalization, LLP 0050#normalization, LLP 0049#fail-safe).
House style clean: no semicolons, no U+2014 anywhere in the diff, no inline
import('...') types, no new @typedef.

---

Finding 1 (low, doc correctness): the three-cause enumeration stops one file

short of where aliased is produced

src/core/usage-policy/matcher.js:667 (pre-fix)

The PR carries the fix to PurgeSummary (src/core/cache/types.d.ts:46),
buildPredicate's retainedAliases param (src/core/cache/purge.js:92),
sameDirectoryOnDisk (src/core/usage-policy/fold.js:111) and
LLP 0104 #spellings. It does not touch scopeGovernance, which is where the
aliased verdict is minted and therefore the doc a future renderer reads
first. That JSDoc still said:

 * `aliased` is the absence of a proof, not a proof of difference: it covers
 * both a live pair with two inodes and a spelling that could not be `stat`ed at
 * all, so a caller rendering it must not report a verdict the second never gave.

Two problems, and they are the two halves of this PR's own thesis. It gives the
two-cause reading that #497 finding 1 identifies as the defect, collapsing
"gone" and "unreadable" into one bucket. And it carries only one of the two
prohibitions the PR establishes elsewhere: "must not report a verdict" is there,
"must not claim the spelling is absent" is not. A caller who reads only this
comment before writing a new message writes exactly the sentence this PR is
removing. Not a runtime defect, but this is the one place where a rendering rule
was already being stated and was left stating half of it.

Fixed in 756dabc: scopeGovernance's JSDoc now enumerates all three and
carries both prohibitions, pointing at src/core/commands/purge.js as the
renderer and LLP 0104 #spellings as the reason.

Finding 2 (low, test quality): the errno pin cannot fail for the reason it exists

test/core/purge-command.test.js:847-860 (pre-fix)

test('no stat errno makes an unprovable alias deletable') is the PR's standing
guard against a future "improvement" to the swallow, and it is the right test to
have written. But it is vacuum-vulnerable. The fixture spellings (/vol/<NFD>/sub,
/vol/<NFC>) exist on no host, so the real statSync would raise ENOENT and
produce aliased too. If a later refactor stopped threading deps.statSync
through scopeGovernance into sameDirectoryOnDisk, all six iterations would
still return aliased, the test would still be green, and the errno under test
would never have been raised at all. The pin would rot silently, which is the
one failure mode a standing guard must not have.

I confirmed the injection is consulted today
(scopeGovernance passes deps straight to sameDirectoryOnDisk(alias, base, deps)
at src/core/usage-policy/matcher.js:722), so this is prophylaxis, not a live
hole.

Fixed in 756dabc: the fake statSync now counts its calls and the test
asserts calls > 0 per errno, so dropping the injection fails the test instead
of passing it by accident.

Considered and deliberately not raised

• The @ref at test/core/purge-command.test.js:442, left as-is. The PR's
  corpus evidence (31 section-level refs followed by a blank line, in 28 files)
  is correct, and closing that specific blank would attach a block-level gloss to
  const NFC, which is worse than the annotation it replaces. Agreed: this is a
  convention question for a maintainer, not a change for this PR. The :718
  instance the issue actually files is closed.
• The hyphen soft-wrap in the new LLP 0104 list item ("normalization-\n
  sensitive"), which renders as normalization- sensitive. Left alone: the same
  shape already exists in that file's untouched Consequences block
  (purge-then-\n re-record), so it is the file's convention, not a slip in this
  diff.
• Surfacing the concrete errno on stderr. The PR argues it would mean
  threading a per-row cause through the deletion predicate for a message-only
  gain, and keeps it on usage_policy.alias_probe_skipped. That is the right
  trade: the predicate is the one place in this subsystem where an extra
  parameter is a liability.

[philcunliffe]

Triage rung (LLP 0017): PR can ship safely

Review's 2-round fix-loop closed at head 756dabc with three residual findings
still open. Judgement: all three are non-blocking (preference / correctly
deferred design questions), none is a production defect (wrong purge behaviour,
data loss, security hole, crash, or perf regression), so this PR can merge
safely as-is.

• Symlink-inside-target (finding 2, documented not fixed): re-reproduced
  independently against 756dabc; the plain (unwidened) gate predicate used by
  hyp ignore --check / policy show already returns governs for the same
  fixture with proveAliases off, so purge and the gate stay consistent as
  documented in LLP 0104 §Consequences. Pre-existing since Usage-policy gate matches directories, not path spellings: canonicalize both sides #482, unrelated to
  this PR's diff, and fixing it unilaterally in purge alone would desync it from
  the gate (an unclearable residual) rather than improve safety. Correctly
  deferred to a separate LLP 0050 canonicalization decision.
• @ref at test/core/purge-command.test.js:442 separated from its
  construct by a blank line: zero runtime effect, matches an established
  38-instance/34-file corpus convention, and closing it here would attach a
  worse gloss to the wrong construct.
• Hyphen soft-wrap in the new LLP 0104 list item
  (llp/0104-hyp-purge.decision.md:134): cosmetic Markdown rendering only,
  matches the file's own pre-existing convention elsewhere in the same doc.

Filed as a follow-up issue with full evidence and a backlink: #526

Also re-verified independently rather than trusting the PR body / review
records: both round-2 fixes (the scopeGovernance JSDoc three-cause
enumeration, and the calls > 0 guard on the errno pin test) are present in the
committed tree at 756dabc; node --test test/core/purge-command.test.js is
31/31 green; full npm test gives # tests 3089 # pass 3080 # fail 8 # skipped 1,
matching the PR's own reported counts, with the 8 failures identical by name to
the pre-existing test/core/leave-command.test.js baseline (issue #512) — no
new failures.