Deferred, non-blocking findings from the two neutral-review rounds on #493
(hyp purge reaches an aliased spelling it can prove, and reports one it cannot, fixing #485). Triage classified all of them as preferences, not
production blockers, and cleared the PR for merge. Recording them here so
they aren't lost.
1. sameDirectoryOnDisk swallows every stat error, not only ENOENT
src/core/usage-policy/fold.js (sameDirectoryOnDisk) catches any error from
statSync(a) / statSync(b) - EACCES on a parent, ELOOP, ENOTDIR, not
only ENOENT - and returns false uniformly. The near-miss note's
parenthetical (src/core/commands/purge.js, src/core/cache/purge.js,
src/core/cache/types.d.ts) enumerates only two causes ("genuinely different,
or no longer on disk"), so a permission-denied ancestor or a symlink loop gets
misreported under that same two-item explanation even though the filesystem
never adjudicated "different" for that reason.
Triage verified by execution (injected statSync throwing each of EACCES,
ELOOP, ENOTDIR through scopeGovernance(..., { proveAliases: true }))
that every one of these resolves to 'aliased', never 'governs': a
swallowed stat error can only ever cause a wrong message, never a wrong
deletion. Retention is the safe direction. Low severity, message precision
only.
Possible fix: broaden the parenthetical to name "or could not be checked" in
addition to the two current causes, and/or surface the real errno (already
captured in the alias_probe_skipped log line) in the CLI note.
2. A row lexically inside the target but really outside it (symlink-inside-target) is deleted
A cwd recorded under a path that is lexically inside the purge target, but
where a symlink inside the target actually points elsewhere, is deleted by
hyp purge <target>. Confirmed pre-existing: this happens via the plain
lexical matchDepth/canonicalSpellings check that predates #493 entirely
(landed in #482), not via the new proveAliases fold logic. Triage reproduced
it directly against both the PR head and pristine origin/master
(scopeGoverns/scopeGovernance both return true/'governs' for the
identical fixture on both trees; purgeCache reports rowsDeleted: 1 on
both).
Not attributable to #493 - it ships no regression here - so it did not block
this PR. It is, however, a pre-existing correctness question in its own right
(#482's "set of spellings" semantics): should a symlink recorded inside a
purge target that resolves outside it be deleted along with the target, or
retained? Worth a decision, separately from #493.
3. An @ref at test/core/purge-command.test.js:718 is separated from its construct by a blank line
CLAUDE.md: "Attach it directly above the construct; a blank line breaks
attachment." The @ref LLP 0104#spellings [tests] added by this PR's review
round 1 (commit 7075976) at line 718 has a blank line before the test(...)
it's meant to annotate. It mirrors an already-existing neighbor at line 442
(landed in the PR's first commit, fa57aa2), which has the same shape and
reads as a section-level annotation over the block of tests below it rather
than a single-construct annotation.
Verified by git blame: both lines confirmed as described. Purely a
convention nit with no behavioral effect - worth normalizing (either close
the blank line, or make the convention for section-level refs explicit) the
next time this file is touched.
Backlink: #493
Deferred, non-blocking findings from the two neutral-review rounds on #493
(
hyp purge reaches an aliased spelling it can prove, and reports one it cannot, fixing #485). Triage classified all of them as preferences, notproduction blockers, and cleared the PR for merge. Recording them here so
they aren't lost.
1.
sameDirectoryOnDiskswallows everystaterror, not onlyENOENTsrc/core/usage-policy/fold.js(sameDirectoryOnDisk) catches any error fromstatSync(a)/statSync(b)-EACCESon a parent,ELOOP,ENOTDIR, notonly
ENOENT- and returnsfalseuniformly. The near-miss note'sparenthetical (
src/core/commands/purge.js,src/core/cache/purge.js,src/core/cache/types.d.ts) enumerates only two causes ("genuinely different,or no longer on disk"), so a permission-denied ancestor or a symlink loop gets
misreported under that same two-item explanation even though the filesystem
never adjudicated "different" for that reason.
Triage verified by execution (injected
statSyncthrowing each ofEACCES,ELOOP,ENOTDIRthroughscopeGovernance(..., { proveAliases: true }))that every one of these resolves to
'aliased', never'governs': aswallowed stat error can only ever cause a wrong message, never a wrong
deletion. Retention is the safe direction. Low severity, message precision
only.
Possible fix: broaden the parenthetical to name "or could not be checked" in
addition to the two current causes, and/or surface the real
errno(alreadycaptured in the
alias_probe_skippedlog line) in the CLI note.2. A row lexically inside the target but really outside it (symlink-inside-target) is deleted
A
cwdrecorded under a path that is lexically inside the purge target, butwhere a symlink inside the target actually points elsewhere, is deleted by
hyp purge <target>. Confirmed pre-existing: this happens via the plainlexical
matchDepth/canonicalSpellingscheck that predates #493 entirely(landed in #482), not via the new
proveAliasesfold logic. Triage reproducedit directly against both the PR head and pristine
origin/master(
scopeGoverns/scopeGovernanceboth returntrue/'governs'for theidentical fixture on both trees;
purgeCachereportsrowsDeleted: 1onboth).
Not attributable to #493 - it ships no regression here - so it did not block
this PR. It is, however, a pre-existing correctness question in its own right
(#482's "set of spellings" semantics): should a symlink recorded inside a
purge target that resolves outside it be deleted along with the target, or
retained? Worth a decision, separately from #493.
3. An
@refattest/core/purge-command.test.js:718is separated from its construct by a blank lineCLAUDE.md: "Attach it directly above the construct; a blank line breaks
attachment." The
@ref LLP 0104#spellings [tests]added by this PR's reviewround 1 (commit
7075976) at line 718 has a blank line before thetest(...)it's meant to annotate. It mirrors an already-existing neighbor at line 442
(landed in the PR's first commit,
fa57aa2), which has the same shape andreads as a section-level annotation over the block of tests below it rather
than a single-construct annotation.
Verified by
git blame: both lines confirmed as described. Purely aconvention nit with no behavioral effect - worth normalizing (either close
the blank line, or make the convention for section-level refs explicit) the
next time this file is touched.
Backlink: #493