refactor: biome warnings sweep + cognitive-complexity extract-method (63→1)

[oorabona]

Summary

• Pure refactor sweep across the workspace: 63 → 1 biome warnings (-98.4%).
• Behavior-preserving extract-method on src + test files. Zero API change, zero version bump, zero test regression.
• Two recently-hardened files (file.ts extractFile post-PR fix(tar-xz): close Win32 symlink-swap TOCTOU with JS-pure 'wx'+retry fail-closed #114 Win32 TOCTOU + extract.ts streaming post-PR feat(tar-xz): true streaming for Node extract()/list() — O(largest entry) #113) refactored with surgical preservation; pre-push opus senior-review verdict: SAFE-TO-PUSH with byte-equivalent security-error string + per-entry dataGenInFlight isolation confirmed.

Metrics

Metric	Before	After
Biome warnings	63	1 (pre-existing unrelated suppression)
Tests	671 pass / 0 fail / 3 skip	671 / 0 / 3 (identical)
TS errors (root + tar-xz + nxz-cli)	0 / 0 / 0	0 / 0 / 0
Net diff	—	27 files / +802/-577

Phases

Phase	Scope	Strategy	Δ warnings
2	auto-fix sweep	biome check --write	-23 (63→40)
3+4	manual narrow + cycle-ignore + useForOf	per-site type narrowing	-26 (40→14)
5a	test-file extract-method	buildEntryBlocks, recalculateChecksum, buildGlobalPaxBlock helpers	-8 (14→6)
5b-1	low-risk src extract-method	parsePaxHeaderBlock, extractEntryContent, splitLongName, buildEntryPath	-3 (6→3)
5b-2	high-risk security src extract-method	openFileExclusive, writeFileEntryWin32, writeFileEntryPosix, extractSymlinkEntry, extractHardlinkEntry, nextParseEvent, drainEntryChunks, drainSkippedEntry, createEntryDataPull (11 helpers)	-2 (3→1)

Security & streaming contract preservation (pre-push opus verdict)

Invariant	Verdict	Evidence
Win32 'wx' atomic create	✅ byte-identical	openFileExclusive line 311
EEXIST handler (unlink ENOENT-pass-through + retry)	✅ preserved	mirrors PR #114 nesting
Security error message (em-dash U+2014)	✅ byte-identical	runtime resolved string unchanged
chmod/utimes best-effort wrap on Win32	✅ preserved	FAT32/cloud-share regression-safe
POSIX O_NOFOLLOW strict propagation	✅ preserved	no try/catch added
for await (const chunk of entry.data) consumption	✅ preserved	both POSIX and Win32 paths
HARDLINK leaf+ancestor symlink validation	✅ preserved	ensureSafeLinkname mirrors inline check
dataGenInFlight per-entry closure isolation	✅ preserved	createEntryDataPull constructs fresh closure per entry
D-5 TAR_PARSER_INVARIANT concurrent-iteration detection	✅ preserved	same error class, same string
D-2 swallow policy on drain errors	✅ preserved	drainSkippedEntry mirrors original try/catch
Generator return semantics (async function* outer + helpers)	✅ preserved	yield/return shape unchanged

Test plan

• pnpm test: 671 pass / 0 fail / 3 skip (identical to baseline at every phase boundary)
• pnpm type-check x3 packages: 0 errors throughout
• rtk proxy biome lint: 63 → 1 warnings
• packages/tar-xz/test/security-win32.spec.ts: 4/4 pass (Win32 TOCTOU contract)
• packages/tar-xz/test/security.spec.ts: 22/22 pass (general security regression)
• Pre-push opus senior-review: SAFE-TO-PUSH

Out-of-scope follow-ups (filed in TODO)

• 1 residual biome warning: test/node-api.spec.ts:249 suppressions/unused (pre-existing, unrelated to this sweep)
• F-002: pre-existing latent HARDLINK + undefined linkname + strip=0 → TypeError instead of structured error. Defensive hardening; not introduced by this refactor.

Story

REFACTOR-BIOME-2026-04-29 — branch fix/biome-warnings-sweep, 10 commits.

[Copilot]

Pull request overview

Workspace-wide refactor focused on reducing Biome warnings and lowering cognitive complexity via extract-method, while preserving existing runtime behavior across Node, browser, WASM, and tar-xz security/streaming paths.

Changes:

• Refactors (extract-method + narrowing) across src and tests to eliminate non-null assertions / complexity warnings.
• Splits several tar-xz extraction and security-critical branches into private helpers (Win32 'wx' path, entry draining, PAX parsing, checksum recalculation).
• Small lint-motivated cleanups in examples, CLI, docs, and ignore rules.

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/wasm/decompress-memlimit.test.ts	Replaces concat with template literal and removes result! via explicit guard.
src/pool.ts	Adds Biome import-cycle suppression comment; removes non-null assertion on queue.shift().
src/lzma.ts	Adds Biome import-cycle suppression comment on pool re-export.
src/errors.ts	Removes non-null assertion when indexing error message table; adds safe fallback.
packages/tar-xz/test/xz-helpers.spec.ts	Renames unused counter to satisfy lint.
packages/tar-xz/test/tar-parser-stream.spec.ts	Extracts TAR-building helpers to reduce cognitive complexity.
packages/tar-xz/test/security.spec.ts	Extracts checksum + TAR-building helpers; removes non-null assertions in error checks.
packages/tar-xz/test/node-api.spec.ts	Removes non-null assertion via optional chaining in assertion path.
packages/tar-xz/test/memory-shape.spec.ts	Uses optional chaining for gc calls under runIf(hasGc).
packages/tar-xz/test/coverage.spec.ts	Extracts checksum + TAR-building helpers; removes non-null assertions.
packages/tar-xz/src/tar/format.ts	Safer indexing under noUncheckedIndexedAccess; extracts splitLongName.
packages/tar-xz/src/tar/checksum.ts	Removes non-null assertions; handles undefined bytes defensively.
packages/tar-xz/src/node/file.ts	Extracts Win32/POSIX file-write and link handling into helpers; keeps TOCTOU hardening structure.
packages/tar-xz/src/node/extract.ts	Extracts parsing/draining/data-pull helpers to reduce complexity.
packages/tar-xz/src/browser/extract.ts	Extracts PAX/content parsing helpers to reduce complexity.
packages/nxz/src/nxz.ts	Replaces non-null assertions with explicit guards/helpers in CLI/tar-create path building.
examples/browser/main.ts	Replaces DOM non-null assertion with explicit runtime error.
docs/plans/WIN32-TOCTOU-2026-04-29.md	Updates plan text to match refactored helper locations.
TODO.md	Updates tracking items for the refactor sweep + residual warning note.
.gitignore	Ignores additional local tool state files.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

ExtractOptions.filter is documented/typed to return boolean (“return false to skip an entry”). Changing the check to filter?.(strippedEntry) === false makes any other falsy return value (e.g. 0, '', null, undefined) not skip the entry, which is a behavior change and also diverges from the Browser implementation (if (filter && !filter(...))). Use a standard falsy check (filter && !filter(strippedEntry)) or explicitly coerce to boolean to preserve the existing contract.

Suggested change

	if (filter?.(strippedEntry) === false) {
	if (filter && !filter(strippedEntry)) {

[Copilot]

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The D-5 “concurrent entry.data iteration” invariant error is thrown as a plain Error without setting err.code. Elsewhere in this module invariant violations attach code: 'TAR_PARSER_INVARIANT' (e.g., the stray-chunk check), and downstream tests/consumers may rely on the code to distinguish corruption/invariant failures from ordinary runtime errors. Consider assigning code = 'TAR_PARSER_INVARIANT' on this error (while keeping the same message) for consistency.

Suggested change

	throw new Error('concurrent entry.data iteration is not supported');
	const err = new Error('concurrent entry.data iteration is not supported') as Error & {
	code?: string;
	};
	err.code = 'TAR_PARSER_INVARIANT';
	throw err;

[Copilot]

TarEntry.linkname is typed as a required string in packages/tar-xz/src/types.ts, so the entry.linkname !== undefined guard and the doc text about “empty or undefined linkname fields” are misleading here. Consider simplifying the condition (or updating the comment) to reflect that linkname is always present and that the only invalid case being rejected is the empty/NUL-containing string.

Suggested change

	* Only SYMLINK and HARDLINK entries are required to have non-empty linknames;
	* other entry types may legitimately have empty or undefined linkname fields.
	* Delegates to `ensureSafeName` for the actual rejection logic.
	*/
	function ensureSafeLinkname(entry: TarEntry): void {
	if (
	(entry.type === TarEntryType.SYMLINK || entry.type === TarEntryType.HARDLINK) &&
	entry.linkname !== undefined
	) {
	* `TarEntry.linkname` is always present as a string, but only SYMLINK and
	* HARDLINK entries are validated here. Delegates to `ensureSafeName` for the
	* actual rejection logic, including empty or NUL-containing linknames.
	*/
	function ensureSafeLinkname(entry: TarEntry): void {
	if (entry.type === TarEntryType.SYMLINK || entry.type === TarEntryType.HARDLINK) {

[Copilot]

This TODO item claims entry.linkname can be undefined (leading to resolve(undefined)), but TarEntry.linkname is currently a required string in packages/tar-xz/src/types.ts and the TAR header parser returns '' for an empty linkname field. Unless there’s another entry source that can produce undefined, this follow-up looks stale/mischaracterized and should be reworded (e.g., to refer to empty linknames) or removed to avoid tracking a non-issue.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

resolveTarOutput() now falls back to files[0] ?? '', which can silently produce an output name of .tar.xz if it’s ever called with an empty files array. Previously this would have thrown (via basename(undefined)), which is a much clearer failure mode for an invariant violation. Consider making the precondition explicit (e.g., throw when files.length === 0) instead of defaulting to an empty string.

Suggested change

	const firstFile = files[0] ?? '';
	if (files.length === 0) {
	throw new Error('resolveTarOutput requires at least one input file when no output path is provided');
	}
	const firstFile = files[0];
	if (firstFile === undefined) {
	throw new Error('resolveTarOutput requires at least one input file when no output path is provided');
	}

[Copilot]

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

isExcludedByFilter uses filter !== undefined to decide whether to call the predicate, but the comment says this preserves the historical filter && !filter(entry) semantics. These are not equivalent: a null (or other falsy non-undefined) filter would previously be treated as “no filter”, but will now be invoked and throw at runtime. To truly preserve prior behavior (and match the browser/node create implementations), gate on typeof filter === 'function' (or filter &&) before calling it.

Suggested change

	return filter !== undefined && !filter(entry);
	return typeof filter === 'function' && !filter(entry);

[Copilot]

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

packages/nxz/src/nxz.ts:728

• findCommonParent() splits on '/' and joins with '/', but it is computing a common parent for filesystem paths (path.resolve() output). On Windows these paths use \\, so this will typically return '/' (or otherwise compute the wrong cwd), breaking tar-create behavior. Use pathModule.sep/pathModule.parse() or pathModule.relative()-based logic instead of hardcoding POSIX separators for OS paths.

  const parts = paths.map((p) => p.split('/'));
  const common: string[] = [];
  const first = parts[0];
  if (first === undefined) return process.cwd();
  for (let i = 0; i < first.length; i++) {
    const segment = first[i];
    if (segment !== undefined && parts.every((p) => p[i] === segment)) {
      common.push(segment);
    } else {
      break;
    }
  }
  return common.join('/') || '/';

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

processQueue() already guards this.queue.length === 0, so shift() returning undefined would indicate an internal invariant violation. Returning silently here can leave queued tasks unprocessed and makes the pool fail “quietly” if the invariant is ever broken (e.g., future refactors/re-entrancy). Prefer keeping the non-null assertion, or throw an explicit invariant error (and/or emit an error event) rather than returning.

Suggested change

	if (item === undefined) return; // invariant: queue was non-empty above
	if (item === undefined) {
	throw new Error('Invariant violation: queue was non-empty but shift() returned undefined');
	}

[Copilot]

options.files.length > 0 is asserted by the preceding stdin check, but the fallback options.files[0] ?? '' will silently change behavior if that invariant is ever violated (e.g., future argument parsing changes): determineMode() would run with an empty filename. Prefer an explicit invariant check/throw (or keep a non-null assertion) so failures are fail-fast and easier to diagnose.

Suggested change

	const firstFile = options.files[0] ?? '';
	const firstFile = options.files[0];
	if (firstFile === undefined) {
	throw new Error('Internal error: expected at least one input file after stdin handling');
	}

[Copilot]

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment refers to processQueue.length > 0 as the guard ensuring shift() returns a value, but the actual guard is this.queue.length === 0 above. processQueue.length is the function arity and unrelated here, so this comment is misleading—update it to reference this.queue.length (or the "queue empty" guard) instead.

[Copilot]

This comment says "We do NOT validate entry.linkname here", but extractFile() now always calls ensureSafeLinkname(entry) before branching into extractSymlinkEntry(), so linkname is validated (non-empty / no NUL / not '.' or '..') earlier. Clarify that the only thing not validated here is whether the symlink target stays within cwd (if that's the intent), or remove the statement to avoid contradicting the actual behavior.

Suggested change

	// We do NOT validate entry.linkname here because symlinks can legitimately
	// point to relative paths inside the archive; the TOCTOU risk comes from
	// follow-up entries — those are guarded by ensureSafeTarget() on each entry.
	// `entry.linkname` has already been syntactically validated before reaching
	// this function; what we do NOT enforce here is that the symlink target stays
	// within `cwd`. The TOCTOU risk comes from follow-up entries, which are
	// guarded by ensureSafeTarget() on each extraction target.

[Copilot]

The rationale comment for setting err.code = 'TAR_PARSER_INVARIANT' mentions drainSkippedEntry needing to distinguish this error from runtime/IO errors, but this error is thrown on creating a second data generator (i.e. calling makeDataGen() twice). In the current design makeTarEntryWithData() calls dataPull() exactly once and never exposes makeDataGen, so this path is effectively unreachable; the comment (and possibly the guard) reads misleadingly as "concurrent entry.data iteration" protection. Consider adjusting the comment to describe what is actually guarded (multiple generator creation) or remove the drainSkippedEntry justification.