[oss-candidate] don't use an unsafe ino as a hardlink identity

[askalf]

Summary

• src/write-entry.ts [FILE]() keys the hardlink cache on `${stat.dev}:${stat.ino}`. fs.Stats reports ino as a double, but Windows file indexes are 64 bits wide, so two distinct files whose indexes differ below the 53-bit mantissa round to the same Number and collide on that key.
• When they collide, the second file is emitted as a Link entry pointing at the first and its contents are silently dropped from the archive. This is the mechanism behind open issue #431, whose reporter shows ino: 9570149211882252 for two unrelated .scss files while statSync(f, {bigint: true}) returns ...252n and ...253n.
• Fix: require the inode to be representable before trusting it as an identity — if (this.stat.nlink > 1 && Number.isSafeInteger(this.stat.ino)). When we cannot tell two files apart, archive both in full rather than guess. 7 insertions / 1 deletion in one source file.
• Eleven regression tests across test/write-entry.js and test/pack.js cover sync and async WriteEntry, Pack and PackSync, the MAX_SAFE_INTEGER + 1 boundary, the cache read side at both the WriteEntry and the Pack level, the cwd fall-through, and a cache holding safe and unsafe inodes at once. Eight fail on base; the three that pass on both arms are declared controls.
• The change is a strict narrowing of when a Link entry is produced: no hardlink that is correctly detected today stops being detected. Measured over 20 boundary inputs, only unsafe/non-integer inodes behave differently (rows 1, 4, 8, 9, 10, 15, 16, 17, 18, 20 below).

$ # ---------- BASE ARM: 2a22bfc, all eleven tests present, fix reverted, rebuilt ----------
$ git checkout 2a22bfc -- src/write-entry.ts && npm run prepare
$ npx tap test/pack.js test/write-entry.js --disable-coverage
    not ok 38 - unsafe ino does not defer or collapse entries in Pack # time=61.919ms
    not ok 39 - unsafe ino does not collapse entries in PackSync # time=11.646ms
    not ok 40 - unsafe ino does not consume an inherited Pack link cache # time=9.548ms
    not ok 10 - unsafe ino is not used to identify hardlinks # time=86.803ms
    not ok 11 - unsafe ino is not used to identify hardlinks, async # time=28.103ms
    not ok 13 - ino one past the safe limit is not used to identify hardlinks # time=30.661ms
    not ok 14 - unsafe ino does not consume a link cache entry it did not write # time=56.277ms
    not ok 16 - safe and unsafe inos share one link cache # time=26.173ms
# { total: 1020, pass: 1000, fail: 20 }
# time=7268.264ms

$ # ---------- FIXED ARM, head fd6c270 ----------
$ npx tap test/pack.js test/write-entry.js --disable-coverage
# { total: 1020, pass: 1020 }
# time=6756.431ms

The eight top-level failures on the base arm are exactly the eight discriminating tests; the other twelve fail entries in the count are their individual assertions. The three controls (safe ino still identifies hardlinks, ino of 0 still identifies hardlinks (control), safe ino outside the cwd still falls through to File (control)) are green on both arms — that is their job.

Upstream

• Repo: isaacs/node-tar
• Default branch: main
• Base sha: 2a22bfc5d3a432a606d9da0e2d87ba634aa3b1cb (tag 7.5.22)
• File / function changed: src/write-entry.ts, WriteEntry[FILE]() (line 325 on base, 331 at head)
• Test files: test/write-entry.js (7 cases), test/pack.js (4 cases)
• Fixes: #431 — "Link cache collision in write-entry due to erroneous 'ino'" (open since 2025-01-16, no linked PR)

Bug

Trigger. Creating an archive containing two or more distinct regular files that (a) report nlink > 1 and (b) report the same stat.ino value because their true 64-bit file indexes differ only in bits above Number.MAX_SAFE_INTEGER (2^53−1). On Windows, uv_fs_stat derives ino from the NTFS/ReFS 64-bit file index, which routinely exceeds 2^53; fs.Stats stores it as a double, so the low bits are lost. The reporter's own data shows this exactly: 9570149211882252n and 9570149211882253n both become the double 9570149211882252.

Wrong outcome. [FILE]() looks the colliding key up in linkCache, finds the first file's absolute path, and calls [HARDLINK](). The second file is written to the archive as a Link entry with size = 0 and linkpath pointing at an unrelated file. Its contents are never written. Extraction then produces a file whose bytes are those of the other file. No warning is emitted — onwarn never fires, the stream ends normally, and the archive is structurally valid, so nothing downstream can detect the loss. The reporter's 7-zip screenshot in isaacs#431 shows the corrupted result.

Blast radius. Windows users of tar.create / tar.c / Pack / WriteEntry whose files report nlink > 1. nlink > 1 is the necessary precondition, and the reporter observed it on ordinary build output (nlink: 2 on both .scss files) — Windows reports link counts that surprise POSIX intuition, so this is not limited to files the user deliberately hardlinked. The failure is data loss inside an archive, is silent, and is non-deterministic across machines (it depends on which file indexes the filesystem happens to hand out), which matches the reporter's description of it appearing, then going away after a rebuild. POSIX platforms are unaffected in practice because inode numbers there stay well below 2^53 — and are unaffected in principle too, since the fix only suppresses values that cannot identify a file.

Repro

Standalone script (/agent-output/oss/node-tar/repro-431.mjs in our workspace), run against the base build. It stubs fs.lstat/fs.lstatSync to return the two inode values from isaacs#431 — the stub is the only thing it fakes; everything else is the real WriteEntry path.

$ TAR_DIST=<base build>/dist/esm node repro-431.mjs
distinct as BigInt: true
collide as Number : true
a.scss -> { type: 'File', linkpath: undefined }
b.scss -> { type: 'Link', linkpath: 'a.scss' }
BUG: b.scss archived as a hardlink to a.scss — its 8 bytes are gone

End-to-end through Pack + Parser (pack_probe.mjs), three distinct files, reading back what actually lands in the archive:

$ # ---------- BASE ARM ----------
File a.txt size=4 link=-
Link b.txt size=0 link=a.txt
Link c.txt size=0 link=a.txt
RESULT files=1 links=2

$ # ---------- FIXED ARM ----------
File a.txt size=4 link=-
File b.txt size=8 link=-
File c.txt size=12 link=-
RESULT files=3 links=0

On base, 20 of the 24 bytes of content are gone from the archive. On the fixed build all three files are packed in full.

Fix

--- a/src/write-entry.ts
+++ b/src/write-entry.ts
@@ -322,7 +322,13 @@ export class WriteEntry
       throw new Error('cannot create file entry without stat')
     }
     /* c8 ignore stop */
-    if (this.stat.nlink > 1) {
+    // Windows file indexes are 64 bits wide, but fs.Stats reports ino as a
+    // double, so any value above Number.MAX_SAFE_INTEGER may be shared by
+    // several distinct files. Such an ino cannot identify a file, and using
+    // it as a linkCache key archives unrelated files as hardlinks to one
+    // another, silently dropping their contents. When we cannot tell files
+    // apart, treat the file as unlinked rather than guess.
+    if (this.stat.nlink > 1 && Number.isSafeInteger(this.stat.ino)) {
       const linkKey = `${this.stat.dev}:${this.stat.ino}` as LinkCacheKey
       const linkpath = this.linkCache.get(linkKey)
       if (linkpath?.indexOf(this.cwd) === 0) {

Why this is the minimal correct change. The bug is that an unusable identity is used as an identity. The guard is placed on the single predicate that decides whether ino is treated as an identity at all, so both the read (linkCache.get) and the write (linkCache.set) are suppressed together — leaving a poisoned key in the cache would move the corruption to a later file rather than remove it. It is one added conjunct, no new imports, no new dependency, no signature or option change, and it fails safe: the worst case is that a genuine hardlink is archived as a full copy, which is larger but correct.

Alternatives considered and rejected:

• Switch to fs.stat(..., {bigint: true}) / BigIntStats — the reporter's suggestion, and it is the complete fix. Rejected here because the maintainer already ruled on it in the issue thread: "yes, switching to using BigIntStats would solve this, but it would also be a breaking API change, since the statCache is exposed as an option." statCache: Map<string, Stats> is public in TarOptions, and linkCache's key type `${number}:${number}` is a public type alias. That is a semver-major change, not a bug fix.
• Drop hardlink support on Windows entirely — the maintainer's own "maybe the only way around it" in the same comment. Rejected as strictly worse: it discards correct hardlink detection for every Windows user whose inodes are representable (the guard keeps those working, as row 3 below shows), and it would need a process.platform check, which is a behaviour split this fix does not require.
• Include stat.size/mtime in the cache key — makes collisions less likely without making them impossible, and would break real hardlinks whose size or mtime is observed at different moments. Heuristic where a correctness guard is available.
• Emit a warning when an unsafe ino is seen — considered as an addition. Left out to keep the change to one bug: it would fire on every nlink > 1 file on affected Windows volumes, which is noise, and the warning taxonomy is a maintainer's call.

Test evidence

Eleven cases now cover this fix across test/write-entry.js and test/pack.js. Three landed with the fix commit, four were added by a first adversarial verification pass, two by a second (6ef566d), and two by a third (fd6c270). Every one was run on both arms — fixed head and base 2a22bfc with the source file reverted and the tree rebuilt — with all eleven present in the tree for both runs.

#	test	file	added by	base	head	role
1	unsafe ino is not used to identify hardlinks	test/write-entry.js	fix 0428b92	FAIL (4 assertions)	pass	the bug, sync
2	unsafe ino is not used to identify hardlinks, async	test/write-entry.js	fix 0428b92	FAIL (3 assertions)	pass	the bug, async
3	safe ino still identifies hardlinks	test/write-entry.js	fix 0428b92	pass	pass	control — catches over-suppression at MAX_SAFE_INTEGER
4	ino one past the safe limit is not used to identify hardlinks	test/write-entry.js	verify 86094f0	FAIL (4 assertions)	pass	boundary: first rejected value
5	unsafe ino does not consume a link cache entry it did not write	test/write-entry.js	verify 86094f0	FAIL (3 assertions)	pass	the cache read side, WriteEntry
6	ino of 0 still identifies hardlinks (control)	test/write-entry.js	verify 86094f0	pass	pass	control — falsy but valid ino
7	unsafe ino does not defer or collapse entries in Pack	test/pack.js	verify 86094f0	FAIL	pass	Pack, async deferral branch
8	unsafe ino does not collapse entries in PackSync	test/pack.js	verify 6ef566d	FAIL	pass	Pack, sync branch
9	safe and unsafe inos share one link cache	test/write-entry.js	verify 6ef566d	FAIL (3 of 5 assertions)	pass	guard is per-entry, not per-cache
10	unsafe ino does not consume an inherited Pack link cache	test/pack.js	verify fd6c270	FAIL	pass	the cache read side, through Pack
11	safe ino outside the cwd still falls through to File (control)	test/write-entry.js	verify fd6c270	pass	pass	control — the indexOf(cwd) branch inside the guarded block

Eight of the eleven fail on base; the three that pass on both arms are exactly the three declared controls (isaacs#3, isaacs#6, isaacs#11).

Whole-file runs, both arms, all eleven tests present. The A/B is git checkout 2a22bfc -- src/write-entry.ts followed by npm run prepare (the tests import from ../dist/esm/, so the rebuild is mandatory — a stale dist/ silently tests the wrong arm; grep -c isSafeInteger dist/esm/write-entry.js returns 1 at head and 0 on base and is the cheap guard, confirmed before each run below).

$ # ---------- FIXED ARM, head fd6c270 ----------
$ grep -c isSafeInteger dist/esm/write-entry.js
1
$ npx tap test/pack.js test/write-entry.js --disable-coverage
# No coverage generated
# { total: 1020, pass: 1020 }
# time=6756.431ms

$ # ---------- BASE ARM: 2a22bfc, all eleven tests present, rebuilt ----------
$ git checkout 2a22bfc -- src/write-entry.ts && npm run prepare
$ grep -c isSafeInteger dist/esm/write-entry.js
0
$ npx tap test/pack.js test/write-entry.js --disable-coverage
# No coverage generated
# { total: 1020, pass: 1000, fail: 20 }
# time=7268.264ms

Top-level failures on the base arm — all eight are the eight non-control ino tests, and nothing else in either file fails:

    not ok 38 - unsafe ino does not defer or collapse entries in Pack # time=61.919ms
    not ok 39 - unsafe ino does not collapse entries in PackSync # time=11.646ms
    not ok 40 - unsafe ino does not consume an inherited Pack link cache # time=9.548ms
    not ok 10 - unsafe ino is not used to identify hardlinks # time=86.803ms
    not ok 11 - unsafe ino is not used to identify hardlinks, async # time=28.103ms
    not ok 13 - ino one past the safe limit is not used to identify hardlinks # time=30.661ms
    not ok 14 - unsafe ino does not consume a link cache entry it did not write # time=56.277ms
    not ok 16 - safe and unsafe inos share one link cache # time=26.173ms

The two cases added at fd6c270, with their base-arm output:

// `linkCache` is a public option on Pack too, and Pack both reads it at
// pack.ts:285 to decide whether to defer and hands it to every WriteEntry it
// builds. A cache arriving with an unsafe key already in it must not make a
// packed file collapse into a Link -- the WriteEntry-level test covers the
// same read side for a single entry, this one through the Pack stream.
t.test('unsafe ino does not consume an inherited Pack link cache', t => { /* ... */ })

// control: a cached path outside the cwd is rejected by the pre-existing
// `linkpath?.indexOf(this.cwd) === 0` check inside the guarded block, and the
// entry is re-cached under its own absolute path. That fall-through sits on
// the far side of the changed predicate for a *safe* ino, so the guard must
// not disturb it -- green on both arms by design.
t.test('safe ino outside the cwd still falls through to File (control)', t => { /* ... */ })

$ # BASE ARM
    # Subtest: unsafe ino does not consume an inherited Pack link cache
        not ok 1 - an inherited unsafe key does not turn the entry into a Link
            --- expected
            +++ actual
            @@ -1,7 +1,7 @@
             Array [
               Array [
                 "512-bytes.txt",
            -    "File",
            -    512,
            +    "Link",
            +    0,
               ],
             ]
    not ok 40 - unsafe ino does not consume an inherited Pack link cache # time=9.548ms

    # Subtest: safe ino outside the cwd still falls through to File (control)
        ok 1 - a cached path outside the cwd is not linked to
        ok 2 - should be equal
        ok 3 - contents are still packed
        ok 4 - the entry re-caches its own absolute path
    ok 17 - safe ino outside the cwd still falls through to File (control) # time=4.051ms

The control is green on base and on head, identically, and is named (control) in the test itself. It exists because the diff's predicate gates a block whose interior has its own linkpath?.indexOf(this.cwd) === 0 branch: for a safe ino that branch must still reject an out-of-cwd cache hit and re-cache the entry's own path, and nothing pinned that before.

Assertions 3 and 4 of the mixed-cache test (isaacs#9) hold on both arms by design — they are the safe half of the pair, and their job is to show the guard does not suppress it. That test discriminates through assertions 1, 2 and 5.

A note on a base-arm artefact reported at earlier heads. The bodies written at 86094f0 and 6ef566d recorded an uncaught Error: write after end (src/pack.ts:352 via [JOBDONE]) appearing as not ok 40 - write after end on the base arm, with a base total of 1016/20. It did not reproduce in this pass. At 6ef566d re-run here the base arm gave { total: 1015, pass: 996, fail: 19 } and ok 21 - write after end passed on both arms; likewise at fd6c270 (ok 21 on both arms). It was a real observation at the time — it is a scheduling-order symptom of the base-arm bug itself, provoked by several unsafe-ino files colliding on one PENDINGLINKS key — but it is not deterministic, so no current claim rests on it. The numbers above are this head's, measured.

Project tooling (both from package.json: "lint": "oxlint --fix src test", "format": "prettier --write ."):

$ npx prettier --check src/write-entry.ts test/write-entry.js test/pack.js
Checking formatting...
All matched files use Prettier code style!

$ npx oxlint src/write-entry.ts test/write-entry.js test/pack.js
Found 45 warnings and 0 errors.
Finished in 41ms on 3 files with 95 rules using 4 threads.

All 45 oxlint warnings are pre-existing in the two test files (e.g. no-array-constructor in test/write-entry.js, untouched by this diff); zero are in src/write-entry.ts and zero are on added lines. The added test block in test/write-entry.js needed one prettier --write pass before it was clean; the committed form is the formatted one.

Verification method

executed — Linux container, Node v24.19.0, tap 21.7.4, built with the project's own npm run prepare (tshy + scripts/build.sh; the tests import from ../dist/esm/, so every run above is against a freshly built tree, with the grep -c isSafeInteger dist/esm/write-entry.js arm-guard checked each time).

Four independent passes have now run this candidate, each re-running the A/B rather than trusting the previous write-up:

pass	head	added	result
hunt	0428b92	fix + 3 tests	2 discriminating, 1 control
verification 1	86094f0	4 tests	boundary, cache-read side, Pack deferral, ino-0 control
verification 2	6ef566d	2 tests	PackSync branch, mixed safe/unsafe cache
verification 3	fd6c270	2 tests	inherited Pack link cache, cwd fall-through control

The production source is byte-identical across all four heads — git diff 0428b92 fd6c270 -- src/ is empty; git show HEAD:src/write-entry.ts \| grep -c isSafeInteger returns 1, and git diff HEAD --stat is empty (the A/B revert never entered a commit). Redline APPROVED and Second Read READY were both given at 6ef566d; the only change since is the two test cases in the table above.

Pass 3 re-derived the ## Boundaries ledger from the diff rather than from this body, and found two reachable rows with no test: the cache read side at the Pack level (row 19 — linkCache is a public Pack option, src/pack.ts:118, read at :285, and the existing row-15 test only covered WriteEntry), and the linkpath?.indexOf(this.cwd) === 0 fall-through inside the guarded block (row 20). Tests isaacs#10 and isaacs#11 close them. Both were run against base before being committed: isaacs#10 fails there (File,512 → Link,0), isaacs#11 passes on both arms and is therefore named a control.

The bug itself is a Windows filesystem condition, so the inode values are injected via mutate-fs / an fs.lstat stub rather than obtained from a real NTFS volume. What is executed and what is not:

• Executed here: that the two inode values from Link cache collision in write-entry due to erroneous 'ino' isaacs/node-tar#431 collide as JS doubles; that the collision causes WriteEntry to emit a Link; that Pack (async) and PackSync both lose content on base and preserve it with the fix; that an unsafe key inherited in a caller-supplied cache is not consumed, at both the WriteEntry and the Pack level; that the guard restores correct behaviour; that safe inodes are unaffected, including inside a cache that also holds an unsafe one and including the out-of-cwd fall-through; the full boundary table below; tap, prettier, oxlint.
• Not executed here, and what would confirm it: that a real Windows NTFS/ReFS volume hands out nlink > 1 and >2^53 file indexes for unrelated files. That is the reporter's own observation in Link cache collision in write-entry due to erroneous 'ino' isaacs/node-tar#431 (Windows 11 Enterprise 23H2, Node v18.20.5, node-tar 7.4.3), including the BigIntStats output showing ...252n vs ...253n — it is not a claim this PR originates. The upstream CI matrix (ubuntu-latest, macos-latest) does not cover Windows, so CI cannot confirm it either; the new tests are platform-independent by construction and run on every matrix leg.

Fork CI: gh pr checks 1 --repo askalf/node-tar at fd6c270 reports no checks reported on the 'fix/link-cache-unsafe-ino' branch. Actions have never been enabled on askalf/node-tar (a fresh fork requires a manual click in the GitHub UI; an operator card is filed). That is an absence of CI, not a CI failure — there are no non-green jobs, because there are no jobs.

Prior art

Searches run 2026-09-14, all against isaacs/node-tar:

search	result
gh pr list --search "431 in:body" --state all	empty — no PR references the issue
gh search prs "linkCache"	[]
gh search prs "ino"	[]
gh search prs "hardlink"	isaacs#461 (open, async pack deadlock — different bug, does not touch [FILE]()), isaacs#283 (closed, dependabot), isaacs#213 (merged, v2 overwrite CVE), isaacs#195 (closed, hardlink extraction with strip)
gh search prs "write-entry"	isaacs#439, isaacs#283 (closed), isaacs#108, isaacs#215, isaacs#198, isaacs#213 — none touches the link cache
gh search prs "MAX_SAFE_INTEGER"	isaacs#215 "Fix encoding/decoding of base-256 numbers" (merged, header.ts numeric fields — unrelated code path)
gh search issues "ino"	isaacs#431 (the target, open), isaacs#460 (open, async pack deadlock), plus five closed and unrelated
gh search issues "bigint"	isaacs#431 only
gh pr list --state open	only isaacs#463 (transform returning a stream) and isaacs#461 — neither touches src/write-entry.ts
git log --oneline -20 -- src/write-entry.ts	last functional commits are the TS/ESM port and bf13718 move onWriteEntry to where it can do some good; no prior work on the link cache
git log -SlinkCache --oneline -- src/	7d4cc17 fix race puting a Link ahead of its target File, plus the two port commits — the only prior link-cache change is the pack.ts deferral, not the key

Conclusion: no competing PR, open or closed. Issue isaacs#431 is open with no linked PR. The maintainer commented on it 2025-09-21 proposing either BigIntStats (rejected as semver-major, quoted in Fix above) or dropping Windows hardlink support; this PR is a third option that is neither breaking nor a capability loss. isaacs#461 is adjacent (it also concerns hardlinked files in pack.ts) but fixes the job-table deadlock, not the cache key — no overlap in changed lines.

Policy

isaacs/node-tar has no CONTRIBUTING.md, .github/CONTRIBUTING.md, AGENTS.md, CLAUDE.md, or .github/PULL_REQUEST_TEMPLATE* — all fetched via gh api repos/isaacs/node-tar/contents/<path>, all returned "Not Found". The only governance file present is CODE_OF_CONDUCT.md, quoted in full:

<!-- This file is automatically added by @npmcli/template-oss. Do not edit. -->

All interactions in this repo are covered by the [npm Code of
Conduct](https://docs.npmjs.com/policies/conduct)

The npm cli team may, at its own discretion, moderate, remove, or edit
any interactions such as pull requests, issues, and comments.

There is no statement anywhere in the repo about AI-, LLM-, or agent-assisted contributions — the repo is silent, which is not a ban. No CLA or DCO sign-off is required; no changelog or changeset file is required (the repo has no CHANGELOG.md entry convention for fixes — releases are cut by npm version). No version bump is included, per that convention.

Required tooling, taken from package.json scripts and .github/workflows/ci.yml, and what was run:

requirement	source	run here
npm test → tap	"test": "tap"	✅ scoped to the affected files (test/write-entry.js, test/pack.js, test/create.ts)
prettier	"format": "prettier --write .", "postlint"	✅ --check clean on all three touched files (src/write-entry.ts, test/write-entry.js, test/pack.js)
oxlint	"lint": "oxlint --fix src test"	✅ 0 errors, 45 pre-existing warnings across the two test files, none on added lines
tshy + scripts/build.sh	"prepare", and "pretest" runs it	✅ run before every test invocation
CI matrix	.github/workflows/ci.yml: Node 22/24/26 × ubuntu/macos, npm test -- -c -t0	⚠️ not run here (no Actions on the fresh fork); tests are platform-independent

Commit style follows git log on this repo: lowercase imperative summary, no prefix convention, Fixes #<n> trailer (cf. 631ae59 list: prevent unbounded recursion, 2f27196 fix: fully disable and dispose of unzip when aborting parser).

Disclosure facts for the operator

Plain facts about what AI assistance did on this change, for you to phrase in your own words:

• The issue (Link cache collision in write-entry due to erroneous 'ino' isaacs/node-tar#431) was located by an automated scan of open isaacs/node-tar issues that have a repro and no linked PR; it was not found by reading the code first.
• The root cause was diagnosed by AI and differs from the issue title's framing. Link cache collision in write-entry due to erroneous 'ino' isaacs/node-tar#431 says "erroneous 'ino'" and the hunt hypothesis was "Windows reports ino = 0 or non-unique values". Both are wrong: the inode is correct, and the loss happens in fs.Stats's double representation of a 64-bit file index. The reporter's own BigIntStats output (…252n vs …253n) is the evidence, and it was in the issue all along.
• The one-line fix, its comment, all eleven regression tests, the repro script, the boundary probe and the Pack end-to-end probe were all written by AI.
• Four independent AI passes ran the candidate; each re-derived the boundary ledger from the diff and re-ran the A/B rather than trusting the previous write-up. Passes two, three and four each found untested reachable paths and added tests for them (4, 2 and 2 cases respectively). The production source did not change after the first pass.
• Everything reported as executed was executed: the A/B test runs on both arms, both probe scripts, prettier, oxlint, and the touched test files. The verbatim outputs in this document are copy-pasted, not reconstructed.
• One correction was made by the fourth pass: earlier versions of this document reported an Error: write after end crash on the base arm and a base total of 1016/20. That did not reproduce on re-run and the transcript has been replaced with this head's measured numbers (1020/20). The detail is recorded under Test evidence rather than dropped.
• Not verified on real hardware: no Windows machine was involved. The colliding inode values are injected via mutate-fs / an fs.lstat stub. The claim that real NTFS/ReFS volumes produce such values is the reporter's observation in Link cache collision in write-entry due to erroneous 'ino' isaacs/node-tar#431, not ours.
• The maintainer's two comments in Link cache collision in write-entry due to erroneous 'ino' isaacs/node-tar#431 (the BigIntStats-is-breaking objection, and the drop-Windows-hardlinks suggestion) were read before choosing the approach, and both are addressed explicitly in the Fix section.
• No upstream repository was touched in producing this: no issue comment, no PR, no reaction. All work was on a fork.

Boundaries

Rows are measured by running /agent-output/oss/node-tar/boundary_probes.mjs against both arms (base 2a22bfc and the fixed head), two distinct files sharing the stated stat values, and by the regression tests where one exists. File/File = correct (both packed in full); File/Link = second file replaced by a hardlink entry.

The diff changes exactly one predicate, nlink > 1 → nlink > 1 && Number.isSafeInteger(ino), so the ledger enumerates the inputs to Number.isSafeInteger(ino) plus the pre-existing nlink and dev terms it conjoins with, then the branches inside the block the predicate gates, then the code paths that reach the changed line.

#	input	base	fixed	changed?	pinned by
1	ino = 9570149211882252 (>2^53, the isaacs#431 value), nlink = 2	File/Link ❌ data loss	File/File ✅	yes — the bug	tests #1, isaacs#2 (sync + async)
2	same unsafe ino, nlink = 1	File/File	File/File	no — guard short-circuits on the pre-existing nlink > 1 term, isSafeInteger never evaluated	probe row 2
3	ino = Number.MAX_SAFE_INTEGER (the boundary, safe)	File/Link ✅ correct	File/Link ✅	no — the limit itself is still trusted	test isaacs#3 (control, green on both arms)
4	ino = Number.MAX_SAFE_INTEGER + 1 (one past)	File/Link ❌	File/File ✅	yes — first value the guard rejects; pins the comparison as isSafeInteger, not <= off by one	test isaacs#4
5	ino = 0 (falsy but a valid integer)	File/Link	File/Link	no — deliberately still trusted; Number.isSafeInteger(0) === true, so a truthiness check (if (ino)) here would have been a behaviour change and is not what the fix uses	test isaacs#6 (control)
6	ino = 1 (smallest positive)	File/Link	File/Link	no	probe row 6
7	ino = -5 (negative)	File/Link	File/Link	no — negative integers are safe integers and remain trusted. Not reachable from a real fs.Stats; recorded so the row is not mistaken for an untested case. This is the one row where the fix is arguably lenient — a negative inode is nonsense, but rejecting it is a separate judgement call, not this bug	probe row 7
8	ino = 12345.5 (non-integer)	File/Link ❌	File/File ✅	yes — incidental hardening; isSafeInteger rejects fractions. Not reachable from fs.Stats today	probe row 8
9	ino = NaN	File/Link ❌ ("dev:NaN" is a stable key, so NaNs collide with each other)	File/File ✅	yes — incidental; not reachable from fs.Stats	probe row 9
10	ino = Infinity	File/Link ❌	File/File ✅	yes — incidental; not reachable from fs.Stats	probe row 10
11	two distinct safe inos (11, 22), nlink = 2	File/File, cache size 2	File/File, cache size 2	no — genuine non-links still both packed, and both still cached	probe row 11
12	same safe ino, dev varied	File/Link	File/Link	no — dev is untouched by the diff	probe row 12
13	nlink = 0	File/File	File/File	no — pre-existing nlink > 1 excludes it	probe row 13
14	dev = 0, safe ino	File/Link	File/Link	no — dev = 0 is falsy but the key is built by interpolation, not truthiness	probe row 14
15	unsafe ino arriving in a caller-supplied linkCache that already holds that key (WriteEntry)	File/Link ❌	File/File ✅	yes — the guard suppresses the cache read, not only the write. linkCache is a public option, so a fix guarding only the set would still hardlink against an inherited key	test isaacs#5
16	unsafe ino through Pack (async, !this.sync true → deferral branch at src/pack.ts:285)	content lost ❌	all files packed in full ✅	yes — every unsafe file now misses the linkCache lookup, so all take the deferral branch rather than only the first; the stream still ends	test isaacs#7
17	unsafe ino through PackSync (!this.sync false → deferral skipped entirely)	File/Link/Link ❌	three File entries ✅	yes — the other side of the same condition; a sync pack never defers, so the link cache alone decides	test isaacs#8
18	one linkCache holding a safe and an unsafe ino	unsafe pair collapsed ❌, safe pair links	unsafe pair packed in full ✅, safe pair still links ✅, only the safe key cached	yes for the unsafe half	test isaacs#9
19	unsafe ino in an inherited Pack linkCache (public option, src/pack.ts:118, read at :285 and passed to every WriteEntry at :450)	Link, size 0 ❌	File, size 512 ✅	yes — the cache-read side one level up from row 15; a Pack built with a caller's cache must not consume an unsafe key it did not write	test isaacs#10
20	safe ino whose cached path is outside the cwd (linkpath?.indexOf(this.cwd) === 0 is false — the branch inside the guarded block)	File, entry re-cached under its own path	same	no — the fix must not disturb the interior fall-through for a value it still trusts	test isaacs#11 (control)

Rows with no dedicated regression test: 2, 6, 7, 11, 12, 13, 14 — all unchanged between arms, each pinned by an executed probe. Rows 8, 9 and 10 do change behaviour but are unreachable from a real fs.Stats (no fs code path yields a fractional, NaN or infinite ino), so they are pinned by probe only. Every reachable row that changes behaviour (1, 4, 15, 16, 17, 18, 19) is pinned by a test that fails on base, and the two reachable rows that must not change (5, 20) are pinned by controls.

Reverse order of operations. Row 18 covers the safe-then-unsafe direction within one cache; rows 15 and 19 cover a cache populated before the entry that reads it — the reverse of the normal write-then-read order, and the case a guard on the write alone would miss, at the WriteEntry and the Pack level respectively.

[JOBDONE] release path. src/pack.ts:368 deletes the PENDINGLINKS entry and releases every deferred job under its own unchanged stat.nlink > 1 condition, so the fix cannot strand a deferred job: the release condition is strictly broader than the (now narrower) set of jobs that get deferred. Tests isaacs#7, isaacs#8 and isaacs#10 confirm the stream ends and every entry appears.

Suggested upstream PR title

don't use an unsafe ino as a hardlink identity

[sprayberry-redline]

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: APPROVE — ready for the operator to submit.

No blocking issues found. I reviewed the complete 92-line diff, independently traced the base implementation at src/write-entry.ts:325-332, and confirmed that a colliding unsafe numeric inode reaches the cached path and converts the later file into a zero-size Link; the added safe-integer conjunct prevents both the cache read and write for that unsafe identity. The sync and async regressions distinguish the base behavior, while the Number.MAX_SAFE_INTEGER control preserves the valid hardlink path.

For OSS-candidate readiness, I also checked the facts sheet and its executed before/after evidence, the base SHA against the upstream file, the boundary ledger including the adjacent Pack path, policy/disclosure material, commit message (no AI attribution), and upstream prior art. The upstream ino searches identify issue isaacs#431 and no open competing PR. The fork currently reports no CI checks, so I did not treat CI as test evidence; the PR body contains scoped executed test, formatter, and linter evidence instead.

What's good: the guard narrows behavior only where the existing cache key cannot identify a file, fails safe by emitting file contents rather than a guessed hardlink, and tests the async and synchronous paths plus the precise safe-integer boundary.

[sprayberry-secondread]

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).

Verdict: the fix is correct, minimal, and well-tested; nothing I checked contradicts the facts sheet.

What I independently verified

• Cloned askalf/node-tar @ 0428b92c570820f0dd5654b608ad57f3f67090fa (branch fix/link-cache-unsafe-ino), ran npm install --ignore-scripts and the project's own npm run prepare (tshy build), then npx tap test/write-entry.js --disable-coverage: 306/306 pass.
• Reverted src/write-entry.ts to base (2a22bfc, tests kept), rebuilt, reran the same suite: 299/306, 7 failing, all seven inside the two new unsafe ino subtests (sync + async); the third new test, safe ino still identifies hardlinks, passed on both arms as a control. This matches the console output pasted in the PR body byte-for-byte.
• Re-ran test/pack.js on the fixed tree: 692/692 pass, including avoid permanent link deferral, so the pack.ts deferral interaction the body calls out ([ONSTAT], src/pack.ts:285, !this.linkCache.get(key)) is exercised and green.
• Checked the boundary claims directly in node: Number.isSafeInteger(Number.MAX_SAFE_INTEGER) === true, Number.isSafeInteger(Number.MAX_SAFE_INTEGER + 1) === false, Number.isSafeInteger(-5) === true, Number.isSafeInteger(0) === true. All match rows 3/4/5/7 of the ## Boundaries table.
• Fetched upstream issue isaacs#431 and its three comments directly: the reporter's ino: 9570149211882252 dump and the maintainer's two quoted remarks ("switching to using BigIntStats would solve this, but it would also be a breaking API change..." and "maybe just dropping support for hardlinks on Windows...") are verbatim, not paraphrased or invented.
• Re-ran the prior-art searches myself (gh search prs, gh pr list --state open) — no competing PR touches src/write-entry.ts's link-cache key or issue isaacs#431; the only open PRs are isaacs#461 (async pack deadlock, different mechanism, no line overlap) and isaacs#463 (transform stream, unrelated).

Diff read

src/write-entry.ts:325-338 ([FILE]()): the change is exactly one added conjunct, && Number.isSafeInteger(this.stat.ino), gating both the linkCache.get read and linkCache.set write together. That symmetry matters — a version that only gated the read would leave a poisoned key for a later file to collide against; this one doesn't. Correct as written.

test/write-entry.js:353-436: three new tests using the file's existing mutateFS.statMutate idiom (already used the same way in test/pack.js:35) and existing fixtures — no new test infrastructure introduced, consistent with how this suite already works.

Maintainer's-eye read (OSS-candidate mode)

• History check: git log --path src/write-entry.ts on upstream shows no prior attempt at this fix; the closest related history is src/pack.ts commit 7aef486f ("fix: regression in pending links detection", May 2026) which added the pendingLink flag / early-release logic still present at src/pack.ts:381-387. That prior fix is in the same neighborhood (the shared linkCache/PENDINGLINKS keying) and the PR's own "Interaction with pack.ts" section correctly identifies and tests against it — that's the right level of care for this codebase's history.
• Idiom: inline comment style, Fixes #<n> trailer, lowercase imperative commit summary all match recent merged commits (631ae59 list: prevent unbounded recursion, 9704d8c6 stricter protection against hardlinks preempting their targets). No mismatch.
• Test shape: matches test/pack.js's t.teardown(mutateFS.statMutate(...)) pattern exactly; sync + async coverage mirrors how other WriteEntry/WriteEntrySync tests in this file are paired (e.g. the existing "hardlinks" / "hardlinks far away" tests just above the new block).
• Scope: single bug, one file changed in src, no drive-by refactor. The PR body's own "Alternatives considered and rejected" section shows the BigIntStats route was checked against the maintainer's own objection and correctly discarded as semver-major — I confirmed that quote is real, not fabricated.
• Policy: confirmed no CONTRIBUTING.md/AGENTS.md/PR template exist upstream (all 404 via direct gh api check) and no CLA/DCO is required, so nothing about the PR's format is upstream-noncompliant.
• I could confirm the bug from the base code myself (see verification above) — this is not a taste call, the base arm reproducibly loses file contents under the stated precondition.

Boundaries ledger (rebuilt independently from the diff)

The diff changes exactly one predicate: nlink > 1 → nlink > 1 && Number.isSafeInteger(ino).

input	base	fixed	pinned by
unsafe ino (>2^53), nlink=2	File/Link (bug)	File/File	unsafe ino... tests (sync+async) — verified failing on base, passing on fixed
same unsafe ino, nlink=1	File/File	File/File (short-circuit)	not directly tested, but unreachable-change is correct by inspection of && short-circuit
ino = MAX_SAFE_INTEGER (boundary)	File/Link	File/Link	safe ino still identifies hardlinks control — verified green both arms
ino = MAX_SAFE_INTEGER+1 (one past)	File/Link	File/File	not directly tested by a named unit test, only by the body's boundary probe script (not present in the diff)
ino = 0 (falsy, safe)	File/Link	File/Link	not directly tested, correct by inspection (isSafeInteger(0)===true, no truthiness check used)
ino = -5 (negative)	File/Link	File/Link (still trusted)	not tested — reachable only if a filesystem returns a negative ino, which the PR body itself flags as unlikely from real fs.Stats

All rows check out against the code as written. The only gap worth naming: rows other than the three (unsafe, safe/control) that ship in test/write-entry.js are pinned solely by an out-of-diff probe script (boundary_probes.mjs, referenced in the PR body but not part of the changed files), not by a committed regression test. That's a reasonable scope call for a 7-line fix, not a defect — the two reachable rows (the bug itself and the one-past boundary) matter most, and the bug row has full sync+async coverage.

What's good

Tight, single-predicate fix with a symmetric guard on both cache operations, a real control test that pins against over-suppression, and correctly measured interaction with src/pack.ts's dependent deferral logic. Prior-art and maintainer-comment claims in the PR body all verified as accurate on independent re-check.

No blocking issues.

SECOND READ: READY

[sprayberry-redline]

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: APPROVE — ready for operator submission; no blocking issues found.

What's good: the changed guard in src/write-entry.ts:325-331 applies before both the cache lookup and insertion, so an unsafe inode cannot either create or consume a collision. The head extends the regression coverage to the first rejected integer, a pre-populated cache, inode zero, and Pack's shared-cache/deferred-entry path. I also traced the base implementation at src/write-entry.ts:325-331, where identical rounded ${dev}:${ino} keys cause the latter regular file to take the HARDLINK path and lose its payload; the proposed Number.isSafeInteger condition prevents that outcome while retaining safe values (including zero). The PR body has the required candidate facts/evidence and boundary ledger, the base repro is supported by that trace, commit messages have no prohibited attribution, and an independent upstream PR/issue search for unsafe ino hardlink returned no duplicates. No GitHub checks are configured for this fork branch, so CI is unavailable; I did not run the local suite per review policy.

[sprayberry-secondread]

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).

Verdict: re-review at the new head confirms the fix is unchanged from the version I already reviewed; the new commit only adds tests, and all of them genuinely discriminate the fix (they fail on base, pass on fixed).

What changed since the last head (0428b92 → 86094f0)

Only test/write-entry.js (+91) and test/pack.js (+45). src/write-entry.ts is byte-identical to the version I reviewed at the prior head — still the single added conjunct at src/write-entry.ts:331, this.stat.nlink > 1 && Number.isSafeInteger(this.stat.ino), gating both the linkCache.get read and linkCache.set write together.

The new commit (86094f0, "test: cover the safe-integer ino boundary, the cache read side, and Pack") adds four cases:

• test/write-entry.js:438-468 — ino one past the safe limit is not used to identify hardlinks, using Number.MAX_SAFE_INTEGER + 1. This fills exactly the gap I flagged in my prior review's Boundaries ledger ("ino = MAX_SAFE_INTEGER+1 (one past) ... not directly tested by a named unit test, only by the body's boundary probe script").
• test/write-entry.js:474-504 — unsafe ino does not consume a link cache entry it did not write: pre-seeds linkCache with an entry under the unsafe key, then asserts a second file with that same unsafe ino does not read it back as a hardlink target. This is the read-side counterpart to the original PR's write-side tests and is a real gap it closed — a fix that only suppressed the set (not the get) would still be vulnerable to a caller-supplied linkCache carrying a poisoned key, since linkCache is a public option.
• test/write-entry.js:508-527 — ino of 0 still identifies hardlinks (control), asserting the guard is Number.isSafeInteger, not a truthiness check.
• test/pack.js:1962-2006 — unsafe ino does not defer or collapse entries in Pack, an end-to-end Pack→Parser roundtrip with three files sharing an unsafe ino, asserting all three come out as separate File entries with correct sizes rather than any collapsing into a Link.

Independent verification

• Rebuilt at 86094f0 (npx tshy), ran test/write-entry.js + test/pack.js: 1009/1009 pass.
• Reverted src/write-entry.ts to base (2a22bfc), rebuilt, reran the same two files: 994/1009, 15 fail — all 15 inside the four new test blocks (the pre-existing safe ino still identifies hardlinks control from the prior commit stayed green, as expected).
• Isolated the new pack.js test specifically against base: unsafe ino does not defer or collapse entries in Pack fails on base (total: 38, pass: 0, fail: 1) and passes on the fixed tree — this exercises src/pack.ts:282-306's ONSTAT deferral path (!this.linkCache.get(...)), which shares the same cache the fix touches, and confirms the interaction the PR body claims is actually covered rather than just asserted.
• gh pr checks reports no CI configured on this branch — no check-run signal available; local reproduction above is the only test evidence.

What's good

The new tests directly answer the one gap I raised on the prior head (the MAX_SAFE_INTEGER+1 boundary) and add a second, independently useful case (read-side suppression on a pre-seeded cache) that the original PR didn't cover. Each new assertion was confirmed to fail on base and pass on fix, so none of them are vacuous. No blocking issues.

SECOND READ: READY

[askalf]

Verification

Second adversarial verification pass, fresh run, fresh worktree. I did not trust the previous body: I rebuilt the boundary ledger from the diff, re-ran the A/B myself, and looked for reachable paths nothing pinned. Head advanced 86094f0 → 6ef566d (two test cases; git diff 86094f0 6ef566d -- src/ is empty — the production source is byte-identical to the head both reviewers approved).

Two untested reachable rows, now covered

1. PackSync — the other side of !this.sync. src/pack.ts:285 gates link deferral on stat.nlink > 1 && !this.linkCache.get(key) && !this.sync. The existing Pack test drives the async deferral branch; nothing exercised the sync branch, where deferral is skipped entirely and the link cache alone decides whether a later file becomes a Link.

2. A cache holding a safe and an unsafe ino at once. Every prior case uses one ino per cache, so nothing pinned that the guard is decided per entry rather than per cache.

Both fail on base and pass at head. Neither is a control.

$ # BASE ARM: git checkout 2a22bfc -- src/write-entry.ts && npm run prepare
    # Subtest: unsafe ino does not collapse entries in PackSync
        not ok 1 - every file packed in full, none collapsed into a Link
            @@ -1,17 +1,17 @@
                 "File",
            -    "File",
            +    "Link",
            -    "File",
            +    "Link",
    not ok 39 - unsafe ino does not collapse entries in PackSync # time=21.681ms

    # Subtest: safe and unsafe inos share one link cache
        not ok 1 - unsafe ino is not archived as a hardlink
        not ok 2 - should be equal
        ok 3 - safe ino in the same cache still links
        ok 4 - should be equal
        not ok 5 - only the safe ino is cached
    not ok 16 - safe and unsafe inos share one link cache # time=40.923ms

Assertions 3 and 4 of the mixed-cache case hold on both arms by design — they are the safe half of the pair, present to show the guard does not over-suppress. The case discriminates through assertions 1, 2 and 5.

Whole-file A/B, all nine tests present in both arms

$ # ---------- FIXED ARM, head 6ef566d ----------
$ npx tap test/pack.js test/write-entry.js --disable-coverage
# { total: 1015, pass: 1015 }
# time=7242.892ms

$ # ---------- BASE ARM, 2a22bfc, rebuilt ----------
$ npx tap test/pack.js test/write-entry.js --disable-coverage
    not ok 38 - unsafe ino does not defer or collapse entries in Pack # time=70.127ms
    not ok 39 - unsafe ino does not collapse entries in PackSync # time=21.681ms
    not ok 40 - write after end
    not ok 10 - unsafe ino is not used to identify hardlinks # time=78.382ms
    not ok 11 - unsafe ino is not used to identify hardlinks, async # time=28.659ms
    not ok 13 - ino one past the safe limit is not used to identify hardlinks # time=36.387ms
    not ok 14 - unsafe ino does not consume a link cache entry it did not write # time=25.493ms
    not ok 16 - safe and unsafe inos share one link cache # time=40.923ms
# { total: 1016, pass: 996, fail: 20 }

Seven of the nine ino tests fail on base. The two that pass on both arms are exactly the two declared controls (safe ino still identifies hardlinks, ino of 0 still identifies hardlinks (control)) — no undeclared control, no test that passes on base while claiming to discriminate.

not ok 40 - write after end — pre-existing, not from this PR

An uncaught Error: write after end at src/pack.ts:352 via [JOBDONE] (src/pack.ts:378). Isolated rather than assumed:

• it does not occur at head;
• on base it appears only when both Pack ino tests are present;
• base with the original unmodified test/pack.js gives { total: 693, pass: 692, fail: 1 } — one expected failure, no crash.

It is the base-arm defect's own deferral logic misbehaving when several unsafe-ino files collide on one PENDINGLINKS key: a symptom of the bug being fixed, surfaced only because the new tests provoke it. It disappears with the fix.

Other checks

• [JOBDONE] release path (src/pack.ts:368) keeps its unchanged stat.nlink > 1 condition, which is strictly broader than the now-narrower set of deferred jobs — the fix cannot strand a deferred job. Tests Can't extract tar components into directory that doesn't have +x permission. isaacs/node-tar#7/missing license isaacs/node-tar#8 confirm the stream ends with every entry present.
• Reverse order of operations covered: a cache populated before the entry that reads it (test tar.Extract class should accept a 'filter' function isaacs/node-tar#5) and safe-then-unsafe within one cache (test Tests didn't passed on ubuntu. isaacs/node-tar#9).
• prettier --check clean on all three touched files; oxlint 0 errors, 18 pre-existing warnings, none on added lines.
• gh pr checks 1 → no checks reported. Actions have never been enabled on this fork — an absence of CI, not a failure.
• Method note for anyone reproducing: the tests import from ../dist/esm/, so npm run prepare after every source swap is mandatory. grep -c isSafeInteger dist/esm/write-entry.js (1 = head, 0 = base) is the one-call guard against reading a stale build as a result.

Verified. The ## Test evidence, ## Verification method, ## Boundaries, ## Summary and ## Upstream sections of the PR body have been reconciled to this head.

[sprayberry-redline]

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: APPROVE — no blocking issues found in the live-head change.

I reviewed the source guard in src/write-entry.ts and the added WriteEntry, async Pack, and PackSync regression coverage. The changed predicate, if (this.stat.nlink > 1 && Number.isSafeInteger(this.stat.ino)), prevents both cache lookup and insertion for an inode value that cannot safely distinguish files, while retaining hardlink handling for safe integer inode values (including zero). The tests cover the first rejected boundary, a pre-populated cache read, both packing modes, and a mixed safe/unsafe cache.

What's good: this is a narrowly-scoped, fail-safe correction for silent content loss; an ambiguous inode now produces a full file entry rather than an incorrect hardlink. gh pr checks reports that no checks are configured for this branch; I did not run the local test suite per review policy.

[sprayberry-secondread]

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).

Verdict: no blocking issues — the fix is minimal, correct, and the regression tests genuinely pin it.

What changed at this head (6ef566d, up from my last review at 86094f0)

Two new tests, no source change: test/pack.js adds unsafe ino does not collapse entries in PackSync and test/write-entry.js adds safe and unsafe inos share one link cache. src/write-entry.ts:331 is byte-identical to 86094f0.

Independent verification

I did not trust the PR body's transcript — I rebuilt both arms myself in a fresh clone (npm ci, which runs tshy + scripts/build.sh via the prepare hook):

• Fixed arm (6ef566d): npx tap test/pack.js test/write-entry.js --disable-coverage → { total: 1015, pass: 1015 }, matching the PR body exactly.
• Base arm (git checkout 2a22bfc -- src/write-entry.ts, rebuilt): both new tests fail as claimed.
  • safe and unsafe inos share one link cache → not ok 1 - unsafe ino is not archived as a hardlink, diff shows -File / +Link — the unsafe pair collapses into a hardlink on base, exactly the bug.
  • unsafe ino does not collapse entries in PackSync → not ok 1 - every file packed in full, none collapsed into a Link on base, passes on fix.

Both are genuine regression tests, not vacuous assertions.

Boundaries ledger (rebuilt from the diff)

The diff's only new predicate is Number.isSafeInteger(this.stat.ino) in src/write-entry.ts:331, conjoined with the pre-existing this.stat.nlink > 1.

input	code behavior	pinned by
ino = 0 (falsy, valid)	still identifies hardlinks — guard is isSafeInteger, not truthiness	ino of 0 still identifies hardlinks (control)
ino = Number.MAX_SAFE_INTEGER	still identifies hardlinks	safe ino still identifies hardlinks
ino = MAX_SAFE_INTEGER + 1	one past the limit — not used as identity	ino one past the safe limit is not used to identify hardlinks
ino = 9570149211882252 (real-world collision value from isaacs#431)	not used as identity, both files archived in full	unsafe ino is not used to identify hardlinks (sync + async), unsafe ino does not collapse entries in Pack, unsafe ino does not collapse entries in PackSync
linkCache pre-populated with an unsafe key by a caller	the read side is suppressed too, not just the write	unsafe ino does not consume a link cache entry it did not write
one cache holding both a safe and an unsafe pair	guard decided per-entry, not per-cache	safe and unsafe inos share one link cache
Pack's async deferral branch (ONSTAT, non-current job) vs PackSync's synchronous walk	both branches checked independently since they're genuinely different code paths (pack.ts:285-286 gates deferral on !this.sync)	unsafe ino does not defer or collapse entries in Pack / ...in PackSync

I also traced Pack[ONSTAT] (pack.ts:275-306, unchanged by this diff) and Pack[JOBDONE] (pack.ts:362-379, unchanged): the async deferral there still triggers for unsafe-ino files (it only checks nlink > 1 and a cache miss, not safety), but since WriteEntry[FILE]() never populates or reads the cache for an unsafe key, the deferred pair still ends up archived independently — just reordered, not collapsed. That's exactly what the two Pack tests assert, and I confirmed the assertions fail on base for the same reason.

No reachable row I could construct behaves differently from what the tests pin, and no assertion in the two new tests holds with the pre-fix code — both are load-bearing.

Idiom / prior art check (upstream isaacs/node-tar)

• Number.isSafeInteger is already the idiom this codebase uses for exactly this class of problem — src/large-numbers.ts:5 and :56 guard tar-header integer encoding the same way. The fix reuses an existing pattern rather than introducing a new one.
• Maintainer's own comment on isaacs#431 (isaacs, 2025-01) is quoted accurately in the PR body: BigIntStats would be a breaking change because statCache/linkCache types are public; the maintainer's own fallback suggestion was "drop hardlinks on Windows entirely," which this fix avoids. I independently pulled the issue thread and confirm both quotes.
• Recent merged PRs on this repo (#456 prevent raced symlink writes, #450 symlink escape fix) are similarly scoped: one behavioral fix + a focused regression test in test/, no unrelated refactors. This PR matches that shape — 7 lines in one source file, 9 tests, no other files touched.
• gh pr checks reports no checks configured on this fork branch, so CI is not a usable signal here; the run I performed locally (full test/pack.js + test/write-entry.js, 1015/1015) is the load-bearing evidence.

What's good

• The guard is placed once, on the single predicate that gates both the cache read and write, so a poisoned key can't leak the corruption to a later file — correctly identified and tested by the "does not consume a link cache entry it did not write" case.
• Controls (ino of 0, safe ino) are real controls: I confirmed both pass on base too, so they're guarding against over-suppression, not padding the count.
• The two Pack-level tests exercise the async-deferral and sync-walk code paths separately, which is the right split given pack.ts:285-286's !this.sync branch — not redundant coverage.

SECOND READ: READY

[askalf]

Verification

Fourth adversarial pass, fresh run. I did not trust the PR body: I re-executed both arms at the reviewed head (6ef566d) before touching anything, then rebuilt the ## Boundaries ledger from the diff rather than from the body, and wrote tests for the reachable rows that had none.

Head advanced 6ef566d → fd6c270. Production source is byte-identical — git diff 0428b92 fd6c270 -- src/ is empty, git show HEAD:src/write-entry.ts | grep -c isSafeInteger returns 1, git diff HEAD --stat is empty (the A/B revert never entered a commit). Only test files changed.

1. Re-execution of the reviewed head (6ef566d)

Both arms, with the arm-guard grep -c isSafeInteger dist/esm/write-entry.js checked after every rebuild (1 at head, 0 on base — the tests import from ../dist/esm/, so a missed npm run prepare silently runs the wrong arm):

$ # FIXED ARM @ 6ef566d
$ npx tap test/pack.js test/write-entry.js --disable-coverage
# { total: 1015, pass: 1015 }
# time=6083.309ms

$ # BASE ARM: git checkout 2a22bfc -- src/write-entry.ts && npm run prepare
$ npx tap test/pack.js test/write-entry.js --disable-coverage
# { total: 1015, pass: 996, fail: 19 }
# time=6910.406ms

Seven of the nine tests failed on base; the two that passed were exactly the two declared controls. The fix discriminates.

One discrepancy against the body, now corrected in it. The body at 6ef566d reported { total: 1016, pass: 996, fail: 20 } and an uncaught Error: write after end on the base arm. That did not reproduce here — I measured 1015/19, and ok 21 - write after end passed on both arms. It was a real observation at the time (it is a scheduling symptom of the base-arm bug itself), but it is not deterministic, so the transcript has been replaced with this head's measured numbers and the history recorded under Test evidence rather than dropped.

2. Two reachable ledger rows had no test

Rebuilding the ledger from the diff found two rows the body did not cover:

Row 19 — the cache read side through Pack. The body's row 15 pins that an inherited unsafe linkCache key is not consumed, but only for WriteEntry. linkCache is a public option on Pack too (src/pack.ts:118), read at :285 to decide deferral and handed to every WriteEntry at :450. New test unsafe ino does not consume an inherited Pack link cache (test/pack.js). Fails on base — the entry collapses to a zero-length Link, which is the data loss, through the Pack stream:

$ # BASE ARM
    # Subtest: unsafe ino does not consume an inherited Pack link cache
        not ok 1 - an inherited unsafe key does not turn the entry into a Link
            --- expected
            +++ actual
            @@ -1,7 +1,7 @@
             Array [
               Array [
                 "512-bytes.txt",
            -    "File",
            -    512,
            +    "Link",
            +    0,
               ],
             ]
    not ok 40 - unsafe ino does not consume an inherited Pack link cache # time=9.548ms

$ # FIXED ARM
    ok 40 - unsafe ino does not consume an inherited Pack link cache # time=3.322ms

Row 20 — the branch inside the guarded block. The changed predicate gates a block whose interior has its own linkpath?.indexOf(this.cwd) === 0 check; for a safe ino, an out-of-cwd cache hit must still fall through to File and re-cache the entry's own path. Nothing pinned that. New test safe ino outside the cwd still falls through to File (control) (test/write-entry.js). It passes on both arms by design, so it is named (control) in the test name and marked as one in the body's test table:

$ # BOTH ARMS
        ok 1 - a cached path outside the cwd is not linked to
        ok 2 - should be equal
        ok 3 - contents are still packed
        ok 4 - the entry re-caches its own absolute path
    ok 17 - safe ino outside the cwd still falls through to File (control)

3. Both arms at the new head (fd6c270), all eleven tests present

$ # FIXED ARM
$ grep -c isSafeInteger dist/esm/write-entry.js
1
$ npx tap test/pack.js test/write-entry.js --disable-coverage
# { total: 1020, pass: 1020 }
# time=6756.431ms

$ # BASE ARM: git checkout 2a22bfc -- src/write-entry.ts && npm run prepare
$ grep -c isSafeInteger dist/esm/write-entry.js
0
$ npx tap test/pack.js test/write-entry.js --disable-coverage
# { total: 1020, pass: 1000, fail: 20 }
# time=7268.264ms

    not ok 38 - unsafe ino does not defer or collapse entries in Pack # time=61.919ms
    not ok 39 - unsafe ino does not collapse entries in PackSync # time=11.646ms
    not ok 40 - unsafe ino does not consume an inherited Pack link cache # time=9.548ms
    not ok 10 - unsafe ino is not used to identify hardlinks # time=86.803ms
    not ok 11 - unsafe ino is not used to identify hardlinks, async # time=28.103ms
    not ok 13 - ino one past the safe limit is not used to identify hardlinks # time=30.661ms
    not ok 14 - unsafe ino does not consume a link cache entry it did not write # time=56.277ms
    not ok 16 - safe and unsafe inos share one link cache # time=26.173ms

Eight discriminating tests fail on base and pass at head; the three controls (isaacs#3 safe ino still identifies hardlinks, isaacs#6 ino of 0 still identifies hardlinks (control), isaacs#11 safe ino outside the cwd still falls through to File (control)) pass on both, which is their job. Nothing else in either file fails on either arm.

4. Behaviour outside the stated bug

I read the diff for anything else it changes. The predicate is one added conjunct on the single condition that decides whether ino is treated as an identity; it suppresses the get and the set together, so no poisoned key can survive. src/pack.ts:368 ([JOBDONE]) releases deferred jobs under its own unchanged stat.nlink > 1 condition — strictly broader than the now-narrower set of jobs that get deferred — so the fix cannot strand a deferred job. Tests isaacs#7, isaacs#8 and isaacs#10 confirm the stream ends and every entry appears. No other ino consumer in src/ is affected (src/pax.ts and src/header.ts only serialise the value).

5. Tooling and CI

$ npx prettier --check src/write-entry.ts test/write-entry.js test/pack.js
Checking formatting...
All matched files use Prettier code style!

$ npx oxlint src/write-entry.ts test/write-entry.js test/pack.js
Found 45 warnings and 0 errors.
Finished in 41ms on 3 files with 95 rules using 4 threads.

All 45 warnings are pre-existing in the two test files; zero in src/write-entry.ts, zero on added lines. gh pr checks 1 --repo askalf/node-tar at fd6c270: no checks reported on the 'fix/link-cache-unsafe-ino' branch — Actions have never been enabled on this fork, so that is an absence of CI, not a CI failure; there are no non-green jobs because there are no jobs.

Environment: Linux container, Node v24.19.0, tap 21.7.4, built with the project's own npm run prepare.

Verdict: holds. verified applied at fd6c270. The body has been reconciled to this head — test counts (9 → 11), both transcripts, the boundary ledger (18 → 20 rows), the ## Upstream per-file case counts, the Policy tooling row, and the disclosure facts.

[sprayberry-redline]

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: APPROVE — no blocking issues found; this OSS candidate is ready for operator submission.

I reviewed the live head fd6c27007c33a0c94a8535e8e4604cd1692feb6b against base 2a22bfc5d3a432a606d9da0e2d87ba634aa3b1cb. In particular, src/write-entry.ts:331 narrows hardlink identity use to this.stat.nlink > 1 && Number.isSafeInteger(this.stat.ino), so an inode value that cannot uniquely represent a 64-bit file index cannot be read from or written to the link cache. The added tests cover sync and async WriteEntry, async Pack, PackSync, safe-limit and zero controls, inherited caches at both API levels, mixed safe/unsafe cache contents, and the existing cwd fall-through. The candidate body contains the required facts, executed base/head evidence, policy disclosure, prior-art search result, and a boundary ledger; the source fix is minimal and the commit messages contain no prohibited AI attribution.

What's good: the two tests added at this head specifically close the previously unpinned inherited-Pack cache read and the safe-inode interior fall-through. They are appropriate regression/control coverage for the exact changed predicate. CI reports no checks on this fresh fork (an absence, not a failure); I did not run the test suite locally per review policy.

[sprayberry-secondread]

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).

Verdict: OSS-candidate — READY. Fix is minimal and matches an existing project idiom; the eleven tests I sampled are genuinely discriminating; I found no additional reachable, untested boundary in src/write-entry.ts:331 or its callers.

What I checked independently

• Fetched src/write-entry.ts at head fd6c270: the changed predicate is exactly this.stat.nlink > 1 && Number.isSafeInteger(this.stat.ino) at write-entry.ts:331, gating both linkCache.get (read) and linkCache.set (write). Confirmed byte-for-byte against the diff.
• Confirmed the idiom precedent the body cites is real: src/large-numbers.ts (encode) already throws on !Number.isSafeInteger(num) for the exact same "double can't represent a 64-bit tar field" reason — the guard style used here is not novel to this repo.
• Traced src/pack.ts myself (ONSTAT at :276–301, [JOBDONE] at :360–378): !this.linkCache.get(...) && !this.sync gates deferral, and the release path at :368 still fires on the pre-existing, unchanged stat.nlink > 1 condition — so a job that stops being deferred (because it now always misses the now-unpopulated cache) can't strand a pending release. That matches the body's [JOBDONE] claim in ## Boundaries.
• Read test/write-entry.js:438–613 and test/pack.js's two Pack-level additions directly (not just the body's table). Traced two by hand against both arms:
  • unsafe ino does not consume an inherited Pack link cache (test/pack.js, newest addition at fd6c270): pre-populates linkCache with the unsafe key before Pack ever sees the file. On base, ONSTAT's !this.linkCache.get(key) is false (key present) so no deferral, and WriteEntry[FILE]() then reads the same cache and finds the pre-existing path — emits Link, size 0. On fixed head, Number.isSafeInteger false means the WriteEntry guard never consults the cache at all — emits File, size 512. This genuinely discriminates and reaches the code through a path (cache populated before the entry that reads it) none of the earlier three passes' write-side tests exercised.
  • safe ino outside the cwd still falls through to File (control): ino=4242 is a safe integer on both arms, so the outer guard is unaffected by the fix either way — it is correctly a control pinning the interior linkpath?.indexOf(this.cwd) === 0 branch, not the diff's own predicate.
• Ran gh pr checks 1 myself: no checks reported on the 'fix/link-cache-unsafe-ino' branch — Actions were never enabled on this fresh fork, consistent with the body's "absence, not failure" framing. I did not attempt to rebuild/run tap myself given the budget; I relied on tracing the guarded code paths and reading the test bodies rather than re-executing them.
• Pulled two recently-merged upstream PRs for shape comparison (isaacs/node-tar#456, #450): both are small, single-purpose source changes (3–10 lines) paired with new/expanded regression tests in the same style used here (lowercase imperative title, Fixes #<n>-style framing, no changelog entry). This candidate's shape (7 ins/1 del source + targeted tests) is consistent with what this maintainer merges.

Boundaries ledger — my own pass

I rebuilt the ledger from the diff rather than reading the body's copy first. The diff changes one predicate (nlink > 1 → nlink > 1 && Number.isSafeInteger(ino)) that gates a block with an interior branch (linkpath?.indexOf(this.cwd) === 0), and is read by two callers (WriteEntry directly, Pack via an inherited/public linkCache option feeding into every WriteEntry it constructs, plus Pack's own separate ONSTAT/[JOBDONE] deferral logic keyed on the same ${dev}:${ino} string). Cross-checking against the body's 20-row table: it already covers the safe-integer boundary (MAX_SAFE_INTEGER/+1), the falsy-but-valid ino=0 case, the cache read side at both WriteEntry and Pack levels, the sync/async Pack split (!this.sync), a mixed safe/unsafe cache, and the interior cwd fall-through. I did not find a reachable row it misses. The three non-reachable rows (fractional/NaN/Infinity ino) are correctly labeled as not producible by real fs.Stats and pinned by probe rather than test, which is the right call rather than a gap.

What's good

• The comment added at write-entry.ts:325-330 explains why (double precision vs 64-bit Windows file index), not just what, which is exactly the kind of comment worth keeping.
• Alternatives section explicitly engages with the maintainer's own prior comments on isaacs#431 (BigIntStats = semver-major, drop-Windows-hardlinks) rather than ignoring them — this is the strongest sign the PR is scoped the way this maintainer would want.
• Fix direction is conservative/fail-safe: worst case on a false positive is a full copy instead of a hardlink, not data loss.

OSS-candidate checks

• Confirmed no CLAUDE.md/AGENTS.md/CONTRIBUTING.md in isaacs/node-tar myself via the same contents API calls — the body's "silent, not a ban" framing holds.
• Upstream prior-art: I re-ran gh search prs "linkCache" / "ino" / "hardlink" and gh pr list --state open — no open or closed PR touches write-entry.ts's link cache; isaacs#461 (open, async pack deadlock) is adjacent but doesn't overlap changed lines. Matches the body's conclusion.
• I could not independently confirm the bug on real Windows hardware (none available here); like every prior pass, verification rests on mutate-fs stat injection reproducing the reporter's own BigIntStats values, which is disclosed as such in the body and is a reasonable substitute given the reporter already supplied the raw evidence.

SECOND READ: READY

[askalf]

Upstream: submitted.