Skip to content

hyp purge reaches an aliased spelling it can prove, and reports one it cannot - #493

Merged
philcunliffe merged 3 commits into
masterfrom
fix/issue-485
Jul 31, 2026
Merged

hyp purge reaches an aliased spelling it can prove, and reports one it cannot#493
philcunliffe merged 3 commits into
masterfrom
fix/issue-485

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

The gap

hyp purge <path> reported success and exited 0 while retaining every row recorded under a different spelling of the target directory. Reproduced against pristine origin/master (73b4618) via the CLI wrapper:

purged 0 rows from 0 partitions   # stdout
                                  # stderr: '' (empty)
exit 0

The outcome was byte-identical to "that directory had nothing cached". Worse, the failing path was the quieter one: a purge that actually deletes prints the LLP 0104 resurrection warning on stderr, so silence read as a clean result.

Root cause

src/core/cache/purge.js's subtree branch routed through scopeGoverns, which PR #482 made symlink-aware (canonicalSpellings) but which deliberately stops short of foldPath. The remaining aliasing mechanisms, Unicode normalization and case, were compared byte-for-byte, so a cwd recorded NFD never matched a target typed NFC (or the reverse, or a case variant). Two processes at different times produce the two strings (a CLI resolving the purge target, versus a client recording a cwd), so the divergence is ordinary rather than user error.

--session, --all and --ignored were never affected; --ignored already classified each row through the folded gate.

Why not "drop foldPath into the predicate"

The issue and LLP 0050 §normalization both warn against it, and the warning is correct. At the gate the fold is free: the resolved class is max(declared, folded), so a fold that merges two genuinely distinct directories can only over-suppress. A deletion predicate has no such safety net. On every ext4 volume caf + U+00E9 and cafe + U+0301 are two directories with two inodes, as are Proj and proj, so a folding predicate on Linux would delete cached rows belonging to a directory the user never named. The two error directions are not symmetric: under-deleting retains data, over-deleting destroys it irrecoverably.

The fix: the fold proposes, the filesystem disposes

Rather than add the per-volume normalization-insensitivity probe LLP 0050 sketched, this demotes the fold from verdict to candidate generator inside the one shared predicate. scopeGovernance (in matcher.js, the widened form scopeGoverns now delegates to) gains an opt-in proveAliases mode that purge.js, and only purge.js, passes:

  1. Plain canonical matching runs first, unchanged, so the common case costs exactly what it did.
  2. Only on a miss does foldPath run, and only to propose the prefix of the row's cwd that a spelling-folding volume would treat as the target. (Two foldPath properties carry this: it distributes over /, so segment-awareness survives, and it preserves segment count, so the prefix can be cut by counting segments rather than characters.)
  3. That candidate is deleted only when sameDirectoryOnDisk (new, in fold.js) reports the same dev/ino - the identity test createVolumeCaseProbe already uses, applied to the actual pair instead of a synthesized case-flip.

Over-deletion is structurally unreachable, not merely unlikely. Every extra row deleted sits under a path the filesystem itself identified with the one the user named, so the widening never rests on a rule about strings. This is also why the candidate generator folds case with no probe at all: a volume verdict answers "does this volume fold case?", from which a caller must still infer that two particular spellings are one directory. Comparing the pair directly needs no inference, and no assumption that the probed volume is the volume both spellings live on.

A retention it cannot prove is reported, never silent. When the fold proposes and the filesystem refuses (a genuinely case-sensitive volume, or an aliased directory that no longer exists), the rows stay, and PurgeSummary now carries retainedAliasRows / retainedAliasCwds so the CLI names them on stderr and --json emits them. That closes the half of the defect the counts alone do not.

Test evidence, both directions

New tests in test/core/purge-command.test.js. The two spellings are \u escapes so no editor, merge tool, or git filter can re-normalize the source and make them pass vacuously (the tripwire pattern from test/core/usage-policy-fold.test.js), plus an explicit premise test asserting they are different strings that NFC folds together.

The deleting direction needs an injected statSync: whether a volume folds two spellings is a property of the filesystem and no ext4 CI host has one. The retaining direction needs no injection, and runs against the real filesystem.

FAIL on pristine origin/master (73b4618, separate worktree)

scopeGovernance does not exist on master, so its one unit test and its import were stripped for this run; everything else is behavioural and ran unchanged.

ok 19 - #485 premise: the two fixture spellings are different strings that NFC folds together
not ok 20 - purge subtree deletes a row recorded NFD when the target is typed NFC, on a volume that folds them
not ok 21 - purge subtree deletes a row recorded NFC when the target is typed NFD, on a volume that folds them
not ok 22 - purge subtree deletes a row recorded under a case variant, on a case-insensitive volume
not ok 23 - purge subtree never widens onto a spelling the volume says is a different directory
not ok 24 - purge subtree agrees with the real filesystem about an NFC/NFD pair, and never over-deletes
not ok 25 - purge subtree agrees with the real filesystem about a case variant, and never over-deletes
not ok 26 - runPurge says so when it leaves a lookalike spelling in place, instead of exiting 0 in silence
# tests 26
# pass 19
# fail 7

The two that matter, verbatim:

not ok 20 - purge subtree deletes a row recorded NFD when the target is typed NFC, on a volume that folds them
  error: |-
    the NFD-spelled row is inside the directory the user named

    0 !== 1
not ok 26 - runPurge says so when it leaves a lookalike spelling in place, instead of exiting 0 in silence
  error: |-
    The input did not match the regular expression /1 cached row under a similarly spelled directory was left in place/. Input:

    ''

That empty stderr is the issue's exact symptom.

PASS on this branch

1..27
# tests 27
# pass 27
# fail 0

Coverage

Under-deletion (the bug), injected spelling-insensitive volume:

  • rows recorded NFD, target typed NFC -> deleted; the freed cwd lands in purgedCwds, so the resurrection warning now fires for it too
  • rows recorded NFC, target typed NFD -> deleted
  • rows recorded lowercase, target typed Proj -> deleted

Over-deletion (the hazard) - these must never regress:

  • injected ext4-style volume (both spellings exist, two inodes): the lookalike row survives and is reported via retainedAliasRows / retainedAliasCwds
  • real filesystem, NFC/NFD pair created for real: the test reads dev/ino ground truth and asserts purge agrees with whatever this host reports rather than with a platform assumption. On the Linux CI box that is retention
  • real filesystem, Proj / proj pair: same shape
  • a sibling sharing the folded prefix (caf + U+00E9 + -other) survives on every volume, and is not even reported as a near-miss
  • scopeGovernance without proveAliases still answers outside for the NFD/NFC pair, pinning the CLI membership sites to their pre-fix behaviour

Deliberately NOT changed

  • policy unset / policy show / ignore --check. They share scopeGoverns, whose default is bit-for-bit what it was. Their disagreement with the gate fails toward privacy (an opt-out spelled the other way stays on), and --check's residual row count makes it a disclosure predicate with its own argument. Widening those is a separate decision. LLP 0050 "Not covered" keeps their bullets.
  • sameDirectory (stored-entry identity): unchanged; merging two declarations silently drops a class the user declared.
  • No per-volume normalization-insensitivity probe was added. Pairwise dev/ino identity is strictly stronger and needs no volume-level generalization.
  • No second folding rule. Everything goes through the existing foldPath and the one shared scopeGoverns / scopeGovernance pair, per Two independent readers of Codex session_meta enforce the same privacy-relevant invariant; it has silently drifted twice already #465.

Docs

  • LLP 0104 gains #spellings: the decision, the asymmetry of the two error directions, why no probe, the report-what-you-cannot-prove rule, and what stayed put.
  • LLP 0050 #normalization is restated as "do not reuse foldPath as a verdict", and its "Not covered" purge bullet - which named this the one site failing away from privacy - now records the closure and points at LLP 0104 #spellings. canonicalScope's JSDoc is updated to match.
  • New/updated @refs: LLP 0104#spellings [implements] on the purge predicate and sameDirectoryOnDisk, [constrained-by] on canonicalScope, [tests] on the new test block.

Checks

  • npm test: 3037 pass, 8 fail. All 8 are test/core/leave-command.test.js and are pre-existing: a pristine origin/master worktree fails the identical 8 by name (leave after join removes the seed and reports the server, leave clears an applied central slot..., leave reverses org-driven attaches..., leave after join also warns about a local central sink..., leave is idempotent..., leave still tears down when only a stale attach marker..., leave removes the assets its attach marker records..., leave self-heals an org attach whose plugin is gone...). Nothing else changed state.
  • npm run typecheck: clean.
  • npm run smoke -- purge_removes_cached_rows: ok.
  • Local runs are advisory; CI on this PR is the authority.

Fixes #485

neutral and others added 2 commits July 30, 2026 22:33
…t cannot (#485)

`hyp purge <path>` printed `purged 0 rows from 0 partitions`, wrote nothing
to stderr, and exited 0 while retaining every row recorded under a different
spelling of the target directory. The subtree predicate folded symlinks
(LLP 0050 #canonicalization) but compared Unicode normalization and case
byte-for-byte, so rows recorded NFD survived an argument typed NFC and the
reverse, as did case variants. The user asked for data to be destroyed and
was told it had been.

Not fixed by folding the predicate. At the gate the fold is free because the
resolved class is max(declared, folded), so a fold that merges two distinct
directories can only over-suppress. Purge deletes, and on ext4 `caf` + U+00E9
and `cafe` + U+0301 genuinely are two inodes, so the same fold would destroy
a stranger's rows. The two failures are not symmetric: under-deleting leaves
data behind, over-deleting cannot be undone.

So the fold is demoted to a candidate generator in the one shared predicate.
`scopeGovernance` (the widened form of `scopeGoverns`) gains an opt-in
`proveAliases` mode that purge, and only purge, passes: plain canonical
matching runs first unchanged; only on a miss does `foldPath` propose the
prefix of the row's `cwd` a spelling-folding volume would treat as the
target; and that candidate is deleted only when `sameDirectoryOnDisk` reports
the same `dev`/`ino`. The widening therefore never rests on a rule about
strings, which makes over-deletion unreachable rather than unlikely, and is
why no per-volume normalization probe was needed.

Where the proof fails the rows stay, which is right, and `PurgeSummary` now
carries `retainedAliasRows` / `retainedAliasCwds` so the CLI names them on
stderr. That closes the other half of the defect: `purged 0 rows` with empty
stderr was byte-identical to "that directory had nothing cached".

Unchanged on purpose: `policy unset`, `policy show` and `ignore --check`
share `scopeGoverns` and keep their unwidened answers, since their
disagreement with the gate fails toward privacy and widening a disclosure
predicate is a separate decision.

LLP 0104 gains #spellings; LLP 0050 #normalization and its "Not covered"
section are updated, the latter having named this the one site that fails
away from privacy.

Co-Authored-By: Claude <noreply@anthropic.com>
Review round 1 of PR #493. Two defects in the new report, both executed:

1. `aliased` has two causes and the note asserted only one of them. When a
   spelling cannot be `stat`ed at all (the ordinary case: the user purges a
   project directory they already deleted, so neither the target nor its
   respelling is on disk), the run printed "this filesystem reports it is a
   different directory". The filesystem reported `ENOENT`; it adjudicated
   nothing. LLP 0104 #spellings already names both causes, so the string, not
   the design, was wrong.

2. Subject-verb disagreement: "2 cached rows ... was left in place". The verb
   was keyed to the retained *directory* count while the subject is the row
   count.

Also adds `retained_alias_rows` to the `purge.result` log event, so the branch
that ran leaves an internal signal and not only stderr (CLAUDE.md's
log-driven-development rule). A count, never a path.

Regression tests use spellings that exist on no host, so both hold whether or
not the test volume folds. Verified they redden against the pre-fix string.

Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Neutral review, round 1 of 2 - head fa57aa2

Verdict: the core design claim survives adversarial execution. Three findings, all in
the reporting half, all fixed and pushed. No over-deletion found.

The code-review skill is not installed on this host and codex is not on PATH, so the
audit below was run directly rather than through either. Everything labelled executed
was run in a detached worktree at fa57aa2 (/work/hypaware untouched, read-only); everything
labelled reasoned is an argument I could not run on this ext4 host.


1. The over-deletion attack (the first duty)

The claim under test: demoting foldPath to a candidate generator and gating on
sameDirectoryOnDisk's dev/ino makes over-deletion structurally unreachable, not merely
unlikely. I tried to break it three ways.

1a. Hand-built adversarial fixtures on the real filesystem - executed

Real directories under a temp root, then scopeGovernance(cwd, target, { proveAliases: true })
compared against origin/master's scopeGoverns:

fixture PR master over-delete?
control: <t>/sub inside target governs true -
NFD sibling dir, target NFC (two live inodes) aliased false no
NFD dir itself vs NFC target aliased false no
folded-prefix sibling caf+U+00E9+-other outside false no
prefix-string caf+U+00E9+xyz outside false no
case variant proj vs Proj (ext4) aliased false no
case variant Proj vs proj (ext4) aliased false no
row under a symlink inside the target pointing outside governs true pre-existing, see §5
row under an NFD-spelled symlink to the NFC target governs true no (genuinely one dir)
row cwd gone from disk, NFD spelling, target alive aliased false no
row cwd gone and target gone aliased false no (but see finding F1)

Not one case where #493 returns governs and master returns false for a directory that is
not genuinely the same directory.

1b. Randomized differential fuzz on the real filesystem - executed

400 random trees of NFC/NFD/case/prefix-sibling/symlink/vanished directories; every
(cwd, target, suffix) pair run through both trees, with an independent oracle
(realpath + dev/ino walk up the cwd's ancestor chain) adjudicating any disagreement.

cases 6012   widened 0   unsafe 0   unknowable 0

Zero widenings is the expected result on ext4 (nothing folds here), which is exactly why it
proves too little on its own - hence 1c.

1c. Randomized fuzz against an injected folding volume - executed, and the one that matters

The widening branch is unreachable on any ext4 host, so I modelled the volume as a real tree of
nodes with per-directory folding rules drawn at random (case-folding and NFC-folding chosen
independently per directory, so mixed and partially-folding volumes are in the population), fed
it in as statSync, and adjudicated every widening with a tree walk that shares no code with the
string fold: does resolving cwd's segments on this virtual volume pass through the target
node?

cases 108900   widened 1479   unsafe 0

1479 widenings actually executed, none of them onto a directory the user did not name.

Oracle validated by injecting the bug it is meant to catch - sameDirectoryOnDisk forced to
return true (i.e. the fold as verdict, which is what the issue warns against):

cases 108900   widened 3102   unsafe 1623
  { cwd: "/café/Proj", target: "/café/proj", targetExists: true }   <- exactly the ext4 hazard

So the harness can see over-deletion, and does not see any in #493.

Conclusion (executed): I could not make this PR over-delete. The structural argument holds
where I could run it: every extra row deleted sits under a path the filesystem itself identified
with the target, and on a refusing volume the proposal is simply declined. The two foldPath
properties the design leans on (distributes over /, preserves segment count) are load-bearing
for the cut, but not for safety - dev/ino is downstream of them, which is why mutation D
below (segment-awareness removed) still could not produce a deletion, only a misreport.


2. Findings

F1 - MEDIUM: the near-miss note asserts a verdict the filesystem never gave. Fixed.

src/core/commands/purge.js:139-141 (pre-fix) printed, unconditionally:

note: 1 cached row under a similarly spelled directory was left in place - this filesystem
reports it is a different directory:

aliased has two causes and only one is the filesystem adjudicating: two live directories
with two inodes, or a stat that never landed. Executed reproduction - target and respelling
both absent from disk, which is the ordinary case when a user purges a project directory they
already deleted:

$ purge <tmp>/gone/caf<U+00E9>   # rows recorded under the NFD spelling; neither path exists
exit 0 | stdout: "purged 0 rows from 0 partitions"
stderr: "note: 1 cached row under a similarly spelled directory was left in place -
         this filesystem reports it is a different directory:
           <tmp>/gone/cafe<U+0301>/sub
         tip: purge that exact spelling too if you meant it as well"

The filesystem reported ENOENT. It reported nothing about difference. This is not a design
disagreement: LLP 0104 #spellings already names both causes ("this really is a
case-sensitive volume, or the aliased directory no longer exists"), and sameDirectoryOnDisk's
own JSDoc is careful about it. The string had drifted from the doc, and it matters because a
user told "the filesystem says it's a different directory" reasonably concludes the retained
rows belong to someone else and stops - when in fact nothing was proven and the rows may well
be theirs.

Fixed to state the retention and both reasons, claiming only what the run established. Also
corrected the same overclaim in the two places that define the field semantics:
src/core/cache/types.d.ts (the PurgeSummary contract) and scopeGovernance's opening
definition of aliased in matcher.js. The narrative body of the author's JSDoc is untouched.

F2 - LOW: the note disagrees in number with itself. Fixed.

src/core/commands/purge.js:139 keyed was/were to the retained directory count while
the sentence's subject is the row count. Executed, 2 rows under 1 directory:

note: 2 cached rows under a similarly spelled directory was left in place - ...

Verb now keyed to rows, noun to directories.

F3 - LOW: the new decision has no structured signal. Fixed.

purge.result (src/core/commands/purge.js:94-101) carried rows_deleted and
partitions_affected but nothing about the near-miss branch, so the retention was visible only
on stderr. CLAUDE.md's log-driven-development rule asks a smoke to assert "the user-visible
result and the internal signal that proves the intended path ran". Added
retained_alias_rows - a count, never a path, consistent with the hashed-path discipline
everywhere else at this seam.

Disposition note: F1 and F2 are pinned by new tests that I verified redden against the
pre-fix string (see §4). F3 is verified by inspection and typecheck only - this repo has no
in-memory log-record harness for tests and getLogger returns a fresh object per call, so
asserting it would have meant building one, which is out of scope for a review round. Flagging
that plainly rather than claiming coverage I do not have.

Not a finding, recorded as a design observation

The PR chose dev/ino on the pair over LLP 0050's sketched per-volume probe. Having tried to
break it, I think the deviation is better than the design it replaces and the docs argue it
honestly, so I did not touch it. One consequence worth a triage eye: the proof is unavailable
whenever the target directory itself is gone, so purging an already-deleted project can never
reach its aliased spellings - it can only report them. That is the safe direction and LLP 0104
records it under Consequences, but it means the workaround (hyp purge --ignored, or naming the
exact spelling) stays load-bearing for that case.


3. Does proveAliases leak? - executed. No.

  • Static: the only non-purge caller of either predicate is runUnmarkMachineLocal
    (src/core/commands/clients.js:1027, backing policy unset / unignore --*), which passes
    { component } alone. policy show and ignore --check route through the resolver /
    governingListEntry, neither touched.
  • Behavioural: 57,600 differential comparisons of scopeGoverns and sameDirectory between
    fa57aa2 and origin/master, with the exact deps those call sites pass, over random real
    trees of NFC/NFD/case/symlink/vanished directories:
comparisons 57600   differences 0

The gate is bit-for-bit unchanged. No privacy regression in the opposite direction.
--session / --all / --ignored return retainedAliasRows: 0 and an empty
retainedAliasCwds (executed via --all --json).

4. Do the tests mean anything? - executed. Yes.

Six mutations of the source; in every case the intended tests reddened and no others.

mutation reddens
A: sameDirectoryOnDisk -> always true (the over-delete bug) 4 (never widens..., both real-fs, the CLI note)
B: proveAliases gate removed 1 (scopeGovernance stays unwidened...) - exactly the leak test
C: foldedAliasOf -> always null (fold never proposes) 7 (every new behavioural test)
D: segment-boundary check -> plain startsWith 3
E: aliased collapsed to outside (silent retention returns) 4
F: candidate generator stops folding case 2 (both case tests)

The injected-statSync tests are not tautological: A, C, D and F all reach them. The
spellingInsensitiveVolume fake is a genuine second implementation of folding identity, not a
restatement of the predicate.

Tripwire: verified. The file is pure ASCII apart from one pre-existing em dash in an
unrelated comment - LC_ALL=C grep '[^ -~\t]' finds no raw NFC/NFD pair anywhere, so
re-normalizing the file is a no-op and cannot collapse the constants. The premise test
(assert.notEqual(NFC, NFD) + NFD.normalize('NFC') === NFC) is present and would catch the
collapse if the escapes were ever expanded. I confirmed by substituting 'café' for
'café' (what a re-normalizing editor produces): the premise test fails first, exactly as
designed. My two new tests reuse the same escaped constants.

5. Reporting honesty - executed

  • Counts correct: 4 rows over 2 alias cwds plus 1 real row -> rowsDeleted 1,
    retainedAliasRows 3, retainedAliasCwds length 2. Rows are counted per row, cwds
    deduplicated, and the memoized verdict does not distort either.
  • Resurrection warning: still driven off purgedCwds, and the aliased-but-deleted cwd now
    lands there, so a widened deletion warns too (asserted at the summary level by the PR's own
    test 20; the CLI wrapper takes no deps, so this cannot be driven end-to-end on ext4 -
    reasoned for the CLI layer, executed for the summary).
  • Raw paths: the note prints absolute paths to stderr and --json, matching the existing
    resurrection warning exactly. Structured telemetry stays hashed (hashPath in
    alias_probe_skipped, target_hash in purge.run), and my F3 addition is a bare count. No
    new disclosure.
  • Pre-existing, not a #493 regression: a row whose cwd is lexically inside the target but
    whose real directory is outside it (a symlink within the target pointing away) is deleted -
    by #493 and by origin/master (executed, both rowsDeleted 1). That is Usage-policy gate matches directories, not path spellings: canonicalize both sides #482's
    set-of-spellings semantics and out of scope here; noting it so the next reader does not
    attribute it to this PR.

6. Are the LLP edits honest? - reasoned, against the rendered diff

Yes. LLP 0050 #normalization is restated as "do not reuse foldPath as a verdict" and points
at 0104 #spellings for how purge bought the widening; the "Not covered" purge bullet no longer
describes the open gap and correctly keeps the policy unset / --check bullets open with their
own argument. LLP 0104 #spellings states the deviation from the sketched probe as a deviation
and gives the reason (a volume verdict needs an inference a pairwise proof does not), records the
asymmetry, the report-what-you-cannot-prove rule, and the deleted-directory residue under
Consequences. No section still describes the probe design as the plan. Anchors #spellings,
#normalization, #canonicalization, #requirements all resolve; the blockquote @ref in
0104 follows the convention used by ~10 other LLPs. The @refs added on the purge predicate,
sameDirectoryOnDisk, canonicalScope and the test block all attach to a construct with no
blank line and say something the filename does not.

7. Style (CLAUDE.md)

No semicolons, no em dashes added anywhere in the diff (git diff -U0 | grep '^+' for
U+2014: zero hits, in code and in the LLP prose - the 0050 rewrap even removes some), JSDoc
types only, no @typedef, no inline import('...') types (the PR in fact removes one, hoisting
PurgeSummary into the @import block), root-anchored .js type-import specifiers. My own
changes hold to the same.

8. Checks

check result
npm test @ fa57aa2 3037 pass, 8 fail
npm test @ pristine origin/master (73b4618, separate worktree) the identical 8, by name and by test number (not ok 890-892, 894-898, all leave *)
npm test @ my pushed head 7075976 3039 pass, the same 8 fail
npm run typecheck clean, at both heads
npm run smoke -- purge_removes_cached_rows ok

The 8 leave-command failures are pre-existing and confirmed against a pristine
origin/master worktree; nothing is attributed to this PR. CI is the authority; this run
corroborates.

9. What I pushed, and how I verified it landed

Commit 7075976 on fix/issue-485 (parent fa57aa2). Verified against the committed
remote tree
, not against a green suite:

$ git diff --stat fa57aa2 origin/fix/issue-485
 src/core/cache/types.d.ts        | 13 +++----
 src/core/commands/purge.js       | 23 +++++++++---
 src/core/usage-policy/matcher.js |  8 +++---
 test/core/purge-command.test.js  | 58 ++++++++++++++++++++++++++++++
$ git show origin/fix/issue-485:src/core/commands/purge.js | grep -c 'filesystem reports'   -> 0
$ git show origin/fix/issue-485:src/core/commands/purge.js | grep -n 'does not report'      -> 154
$ git show origin/fix/issue-485:src/core/commands/purge.js | grep -n 'retained_alias_rows'  -> 103
$ git show origin/fix/issue-485:test/core/purge-command.test.js | grep -c 'near-miss note'  -> 3

And the tests are load-bearing: reverting only the message string (leaving everything else in
place) reddens exactly the two new tests -

not ok 28 - the near-miss note does not claim a verdict an ENOENT never gave
not ok 29 - the near-miss note agrees in number with the rows it is counting

Both new tests use spellings that exist on no host, so neither depends on whether the test volume
folds - they hold identically on ext4 and on APFS.


Recommendation: the deviation from LLP 0050's probe design is sound and better argued than
the design it replaces; I attacked it and it held. Nothing here needs the author to redesign.
The PR is still a draft - the remaining call is a human's.

…have given

The near-miss report was corrected in the message, in the `PurgeSummary`
contract and in `scopeGovernance`, but the two statements in `purgeCache`
itself still said the filesystem "says is a different directory". That is the
sink that populates `retainedAliases`, so it is the most authoritative internal
definition of the field and the one a later reader is most likely to copy.
`aliased` is the absence of a proof: two live directories with two inodes, or a
`stat` that never landed. Both sites now say so, matching LLP 0104 #spellings.

Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Neutral review, round 2 of 2 - head 7075976

Verdict: the round-1 fixes are correct, and the safety property is intact at 7075976. One
finding: F1 was correct but incomplete - two more sites still carried the old overclaim, both
inside purgeCache itself. Fixed and pushed (1c5c620). Nothing else actionable.

This round audits only the delta fa57aa2..7075976 plus what round 1 left unverified. I did not
re-run the round-1 differential or the full 108,900-case fuzz.

The code-review skill is not installed on this host and codex is not on PATH, so the audit
below was run directly rather than through either. Work happened in detached worktrees at
7075976, fa57aa2 and pristine origin/master; /work/hypaware was never written.
Executed means I ran it; reasoned means I argued it from the source.


1. The delta

git diff fa57aa2..7075976 touches four files and is, in executable terms, one added log field
and one rewritten stderr string. matcher.js and types.d.ts are comment-only; I diffed them
line by line to confirm that, which is what licenses the regression argument in section 3.

F1 - correct, and now complete. New finding, fixed.

The three corrected statements do agree with each other and with LLP 0104 #spellings
("this really is a case-sensitive volume, or the aliased directory no longer exists"):

site says
src/core/commands/purge.js:154 "this filesystem does not report it as the directory you named (genuinely different, or no longer on disk)"
src/core/cache/types.d.ts:44 "does not report as the target directory ... because they really are two directories, or because the respelling is no longer on disk to be stated at all"
src/core/usage-policy/matcher.js:666 "aliased is the absence of a proof, not a proof of difference"

All three now describe aliased as a failure to prove, not a proof of difference, and all three
name both causes. matcher.js is the sharpest of them and is the one I would keep.

R2-F1 (LOW, documentation): two further sites still carried the pre-fix overclaim.
grep for it, as asked:

  • src/core/cache/purge.js:40-42 - "the rows deliberately not purged because they sit under a
    lookalike spelling this filesystem says is a different directory"
  • src/core/cache/purge.js:90-92 - @param retainedAliases "rows whose cwd is spelled as if it
    were inside the target but which this filesystem says is a different directory"

Both are text this PR introduced (the file is +45 lines here), and the second is not incidental:
it documents the very sink that retainedAliases.rows++ writes into, so it is the most
authoritative in-repo definition of the field and the one a later reader is most likely to copy
forward. Leaving it meant the corrected trio and the uncorrected pair disagreed about what
aliased means, which is the exact condition F1 existed to remove.

Low severity because it is JSDoc, not a user-facing string: no user is misled today. Fixed anyway,
because "correct and complete" was the question. Both now state the retention and both causes.

Post-fix grep over src/ and test/ for the old phrasing returns exactly three hits, all
legitimate: src/core/commands/purge.js:145 quotes the rejected wording to explain why it was
rejected, test/core/purge-command.test.js:563 names a case where the injected volume genuinely
does adjudicate, and :738 is round 1's negative assertion. Executed.

F2 - verified. Executed.

Driven through the real runPurge with two rows under one retained directory:

note: 2 cached rows under a similarly spelled directory were left in place - ...

Verb keyed to rows, noun to directories, and the singular form is correct in the 1-row run above
it. Round 1's pinning test covers it.

F3 - verified by execution, not only by inspection. Executed.

Round 1 flagged that retained_alias_rows was verified by inspection and typecheck alone. I did
not need a log-capture harness: HYP_DEV_TELEMETRY=1 with a temp HYP_HOME installs the real
LoggerProvider and writes purge.result to dev-telemetry/logs-*.jsonl. I ran the production
bootstrap (installObservability) against a temp home, drove five purges through runPurge,
flushed, and read the JSONL back:

run target_kind rows_deleted retained_alias_rows
A subtree, near-miss, neither spelling on disk subtree 0 2
B subtree, near-miss, respelling on disk, target absent subtree 0 1
C --all all 3 0
D --session s1 session 1 0
E subtree, no near-miss subtree 1 0

The field is present on every purge.result and carries the right count. It is a bare integer on
all five, never a path, and --all / --session report 0 rather than omitting the key. That is
the right call and I would not change it: a structured-log schema that varies its key set by
target kind is harder to query than one where the count is always there, and the value 0 is
meaningful (no near-miss branch ran). Round 1's "absent for --all / --session" expectation is
satisfied in substance.

I agree the log-capture harness is out of scope and did not build one. The dev-telemetry path
above is production machinery this repo already ships, not new test scaffolding, and it proves the
emission end to end. If the team wants this pinned in npm test rather than re-derived by hand,
that is a follow-up ticket, not a review-round deliverable.


2. Regressions from the fix commit - executed. None.

The fixes touched a shared predicate's description, so the risk was that someone had also
touched its behaviour. They had not: the matcher.js delta is entirely inside a JSDoc block, and
scopeGovernance's body is byte-identical between fa57aa2 and 7075976.

I confirmed that by running rather than by reading. A reduced re-run of round 1's injected
folding-volume fuzz: virtual trees with per-directory folding rules (case-fold and NFC-fold
drawn independently per directory, so mixed volumes are in the population), fed in as statSync
and realpathSync, with an oracle that shares no code with the string fold (does the prefix of
cwd at the target's depth resolve, on this volume, to the node the target names?). A widening is
counted only when scopeGovernance(..., proveAliases) says governs where plain scopeGoverns
says false.

head 7075976   cases 145574   widened 8060   unsafe 0
head fa57aa2   cases 145574   widened 8060   unsafe 0

Identical, to the widening. 8,060 widenings actually executed at the reviewed head, none onto a
directory the user did not name.

Oracle validated by a positive control that models the bug LLP 0104 warns against (the fold as
verdict, injected as a statSync that reports one inode for everything):

control        cases 145574   widened 9641   unsafe 1581
  { cwd: "/Src/proj", target: "/Src/PROJ", targetExists: true }

So the harness can see over-deletion, and does not see any at 7075976.

One construction note for whoever reads this next, because it took me a wrong turn first: a
realpathSync that rewrites a component to its on-disk case/normalization makes the widening
branch unreachable, because canonicalSpellings then folds the pair before foldedAliasOf is
ever reached (my first run reported widened 0). Real realpath(3) resolves symlinks and does not
correct case or normalization, so the faithful model is the one that returns the given spelling.
That is why the fold branch exists at all.


3. What round 1 recorded but did not act on - executed. The characterisation is right.

A row whose cwd is lexically inside the target but whose real directory is outside it (a symlink
within the target pointing away) is deleted. Reproduced on a real filesystem against both trees:

PR head        {"rowsDeleted":1,"retainedAliasRows":0}
origin/master  {"rowsDeleted":1}

Identical, so it is #482's set-of-spellings semantics (the lexical spelling is one of the
canonicalSpellings, and it matches) and not attributable to this PR. Recording it plainly so
a human is not surprised: hyp purge <dir> deletes rows for a directory that is not under <dir>
when a symlink inside <dir> points at it. That is a pre-existing behaviour, arguably the intended
one, and out of scope here.


4. Recorded, not rewritten

Two things I considered and deliberately left alone. Neither is a defect I would hold the PR on.

  • The near-miss note's parenthetical is a two-item list, and the code has a third case.
    sameDirectoryOnDisk returns false on any stat error, not only ENOENT: EACCES on a
    parent, ELOOP, ENOTDIR all land in the same branch and are reported as "no longer on disk".
    The leading clause ("this filesystem does not report it as the directory you named") is true in
    every one of those cases, LLP 0104 hedges its own enumeration as "the usual reasons", and the
    code already records the real errno in alias_probe_skipped. Churning a user-facing string
    that round 1 has just pinned with two tests, to cover a case that needs a permission-denied
    parent, is not worth the trade. Noted for triage rather than fixed.
  • The new @ref at test/core/purge-command.test.js:718 is separated from the first test( by
    a blank line
    , which CLAUDE.md says breaks attachment. It mirrors the block annotation already
    at :442 (landed in fa57aa2), so it reads as a section annotation over the pair of tests
    below it, consistent with the file's own convention. Left as-is rather than made inconsistent
    with its neighbour.

5. Checks

check result
npm test @ 7075976 3039 pass, 8 fail
npm test @ pristine origin/master (73b4618, separate worktree) 3028 pass, the same 8, identical by name
npm test @ my pushed head 1c5c620 3039 pass, the same 8 fail
npm run typecheck clean at 7075976 and at 1c5c620
npm run smoke -- purge_removes_cached_rows ok at both heads

The 8 failures are all leave * in test/core/leave-command.test.js, confirmed pre-existing
by a name-for-name diff against a pristine origin/master worktree (zero differences). Nothing
is attributed to this PR. Both worktrees had node_modules linked before any run, so the suite
collected fully. CI is the authority; this run corroborates.

6. What I pushed, and how I verified it landed

Commit 1c5c620 on fix/issue-485 (parent 7075976), JSDoc only, one file. Verified against
the committed remote tree, not against a green suite:

$ git show origin/fix/issue-485:src/core/cache/purge.js | grep -c 'says is a different directory'
0
$ git show origin/fix/issue-485:src/core/cache/purge.js | grep -n 'did not confirm their lookalike'
41
$ git show origin/fix/issue-485:src/core/cache/purge.js | grep -n 'without this filesystem confirming'
94
$ git rev-parse origin/fix/issue-485
1c5c620d2be89662410cd1edd52c2e199ee0e992

No em dashes and no semicolons introduced (git diff -U0 | grep '^+' for U+2014: zero hits).
npm run typecheck clean and all 29 purge-command tests green after the edit.


This was round 2 of 2. The next rung is triage.

Summary for whoever picks it up: over two rounds the central safety claim was attacked with a
purpose-built folding-volume fuzz and did not break, the proveAliases opt-in was shown not to
leak into the other scopeGoverns callers, and every finding either round produced was in the
reporting half and has been fixed in-branch. The two things a human still owns are the design
deviation from LLP 0050's sketched per-volume probe (which I think is better than the design it
replaces, and which the docs argue honestly), and the draft status of the PR itself.

@philcunliffe

philcunliffe commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Triage (LLP 0017): both residual review findings were verified by execution and classified non-blocking (all in the reporting/documentation half; the safety property - over-deletion is structurally unreachable - held under an independent 20,000-case differential fuzz with a positive control). Deferred findings tracked in #497.

@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Jul 30, 2026
@philcunliffe
philcunliffe marked this pull request as ready for review July 31, 2026 00:05
@philcunliffe
philcunliffe merged commit 7e7070e into master Jul 31, 2026
9 checks passed
@philcunliffe
philcunliffe deleted the fix/issue-485 branch July 31, 2026 02:15
philcunliffe added a commit that referenced this pull request Jul 31, 2026
* purge's near-miss note names all three causes, not two (#497 findings 1 and 3)

Deferred review findings from #493, now that it has merged.

Finding 1 (behavioural, message-only). `sameDirectoryOnDisk` returns `false`
for any `stat` error, not only `ENOENT`, so an alias that is present on disk
but unreadable (`EACCES` on an ancestor, `ELOOP`, `ENOTDIR`) retains exactly
like one that is absent. The stderr note offered "genuinely different, or no
longer on disk" as an exhaustive pair, and neither member holds in that case:
nothing adjudicated the spellings different, and the spelling is not gone. The
note now says "genuinely different, no longer on disk, or could not be
checked", which asserts none of the three.

The swallow itself is kept and is now documented as deliberate. Widening a
deletion onto an unproven pair would destroy rows under a directory no
filesystem identified with the target, so every errno has to collapse to the
same `false`; what the collapse bounds is the message, not the decision. The
concrete errno stays a diagnostic on `usage_policy.alias_probe_skipped` rather
than being threaded through the deletion predicate for a message-only gain.

Deletion behaviour is unchanged by construction: the whole `src/` change set,
excluding comments, is one stderr string literal. `test('no stat errno makes
an unprovable alias deletable')` pins that for six errnos, so a later attempt
to make some errno "reachable" fails here rather than in a user's cache.

Finding 3 (annotation). Closed the blank line between the `@ref LLP
0104#spellings [tests]` at purge-command.test.js:718 and the test it
annotates, per CLAUDE.md's attachment rule.

Finding 2 is not fixed, deliberately. See the PR body.

Fixes #497

Co-Authored-By: Claude <noreply@anthropic.com>

* The ELOOP fixture skips where the volume folds, instead of EEXISTing

The new near-miss test is the one fixture in this file that has to *build*
both spellings rather than merely name them, so it is the one that needs the
volume to keep them apart. On a normalization-insensitive volume (APFS, HFS+)
`path.join(root, NFD)` already names the NFC directory created the line before,
so `symlink` returns EEXIST and the test errors out before it asserts anything.
CI is ubuntu-only, so this never reddened there, but it fails on every Mac,
which is a first-class dev platform for this project.

Guard the fixture with the file's established `t.skip(...)` idiom: 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.

Also corrects the section comment added alongside the test, which claimed
"none of them depends on whether the test volume folds" - true of the other
fixtures, not of this one.

Co-Authored-By: Claude <noreply@anthropic.com>

* Review: carry the three-cause enumeration to where `aliased` is produced

Two review findings on #505.

`scopeGovernance`'s JSDoc is the definition site of the `aliased` verdict and
the doc any future renderer reads first, but it kept the two-cause reading the
PR fixed everywhere else: it named "a live pair with two inodes" and "a spelling
that could not be `stat`ed at all", and warned only against claiming a verdict.
That is the exact conflation that produced #497 finding 1, left standing in the
one place a new caller would consult before writing a new message. It now names
all three and carries both prohibitions.

The `no stat errno makes an unprovable alias deletable` pin could not fail for
the reason it exists. Neither fixture spelling is on disk on any host, so a
build that stopped threading `statSync` into `sameDirectoryOnDisk` would reach
`aliased` through a real `ENOENT` and the test would still pass, having never
raised the errno under test. It now asserts the injected `statSync` was called.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: test <test@test.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: test <test@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hyp purge <path> reports success and exits 0 while silently retaining rows recorded under an aliased spelling of that directory

2 participants