Review

Solid, well-scoped fix. The approach correctly mirrors deno publish: collect_asset_files() walks the filesystem with the package's publish FilePatterns via the same FileCollector builder chain as cli/tools/publish/paths.rs::collect_paths, then excludes anything already in the tarball (transpiled source modules by their original source path, the source config file, README/LICENSE) before reading the rest verbatim. Behavior stays consistent between pack and publish, which is the right call.

Strengths

• Faithful reuse of FileCollector + member_dir.to_publish_config()?.files — matches publish line for line (git/node_modules/vendor ignores, .gitignore, .DS_Store/.gitignore skip).
• Dedup excludes the original source path (not the emitted output path) — correct, and clearly commented.
• Deterministic sort, consistent with collect_graph_modules.
• The root-level .tgz reproducibility guard is a genuinely thoughtful edge case (re-running pack would otherwise embed the prior tarball).
• Tests cover both the real fix and the dry-run log line.

Suggestions (non-blocking)

• Symlinks silently dropped. The new collector returns false for non-regular files with no diagnostic, unlike read_auto_included_file and publish's collector, which surface a warning/error. For pack this is defensible, but a log::warn! for a symlink matched by an include pattern would avoid a confusing "why is my file missing?" report.
• Unreachable source files get packed raw. A .ts under src/** that isn't reachable from the graph won't be in collected_paths, so it's shipped verbatim (untranspiled) into the npm artifact. This matches publish semantics but is more surprising for an npm package — worth confirming it's intended.
• --output outside root isn't excluded. The .tgz guard only catches root-level tarballs; pack --output ./assets/foo.tgz (inside an included dir) could be re-bundled on a later run. Narrow edge case, but the comment's "never bundle it" slightly overstates coverage.
• Double FS traversal (graph walk + asset walk) vs. publish's single walk — not a correctness issue, just noting for any future consolidation.

Correctness / compile

• &Arc<CliOptions> coerces to the &CliOptions param via deref — fine.
• collect_file_patterns returns Vec<PathBuf> (infallible), so the missing ? is correct.
• #[allow(clippy::too_many_arguments)] is reasonable for tarball assembly.

Approve with minor nits. None are blocking; the symlink-drop and raw-.ts behaviors are the two worth a quick confirmation before merge.

Thanks for the review. Pushed a follow-up (7f9f1e1) addressing the nits:

• Symlinks: now log::warn!s when a symlink matched by the publish config is skipped, mirroring publish's UnsupportedFileType diagnostic, instead of dropping it silently.
• --output outside root: the resolved --output path is now added to the excluded set (resolved against the cwd, like create_npm_tarball), so pack --output ./assets/foo.tgz won't be re-bundled on a later run. Updated the root-level .tgz comment to note it only covers the default-named case.

On the two behaviors you flagged for confirmation:

• Unreachable .ts packed raw: intended — this matches publish semantics. A source file matched by publish.include but not reachable from the graph is shipped verbatim, exactly as publish would include it. Keeping pack and publish consistent is the goal here, so I left this as-is rather than special-casing untranspiled .ts in the npm artifact.
• Symlink drop: kept as a drop (can't put a symlink in the tarball meaningfully), now surfaced via the warning above.

Left the double FS traversal as-is — noted as a future consolidation, not a correctness issue.

Thanks for the fix — the approach is right: walking the filesystem with the publish FilePatterns is the only way to catch non-imported assets, and reusing publish's machinery keeps the two commands aligned. The de-dup reasoning is well documented. A few things I'd consider before merge:

1. (medium) The root-level .tgz heuristic over-matches. collect_asset_files drops any root-level *.tgz, but the default output name is exactly knowable — npm_tarball.rs computes format!("{}-{}.tgz", normalized, version). A user who legitimately lists a root-level tarball in publish.include would have it silently dropped. Suggestion: compute that same default filename and exclude the exact path (the way --output is already excluded) instead of a blanket extension match. That removes the heuristic and its special-case comment entirely.

2. (low) The test doesn't lock in the no-duplicate guarantee. It only asserts package/assets/icon.svg is present. The whole de-dup design hinges on raw src/mod.ts not appearing next to its transpiled src/mod.js/src/mod.d.ts. Worth asserting tarContent.includes('package/src/mod.ts') === false (and presence of mod.js) so a future regression in the path-equality exclusion gets caught.

3. (low) Exclusion is exact path-equality. De-dup relies on path.specifier.to_file_path() (from the graph) being byte-identical to the FileCollector output. Both derive from package_dir so it holds in practice, but symlinked path prefixes or case-insensitive filesystems could theoretically let a source file through as a duplicate. The test in #2 would surface that.

4. (low, confirm) Parity when publish.include is absent. With no include, to_publish_config().files matches broadly, so root files like deno.lock would now be packed as assets. This matches deno publish, so it's probably intended — just flagging it as a conscious behavior change since pack never included them before.

Nothing blocking beyond #1; #2 is cheap insurance. Nice work.

Pushed a follow-up addressing the feedback:

#1 — Removed the root-level .tgz heuristic. The default output name is fully knowable, so I extracted it into npm_tarball::default_tarball_filename (scope-name-version.tgz) and exclude that exact path (same treatment as --output). A user who legitimately lists a root-level tarball in publish.include now gets it packed instead of silently dropped. Verified: with publish.include: ["src/**", "data.tgz"] the tarball now contains package/data.tgz, while a re-run still self-excludes the default output tarball (0 assets collected, reproducibility intact).

#2 — The spec test now also asserts package/src/mod.js is present and package/src/mod.ts is absent, locking in the de-dup guarantee (and, per #3, this would catch a path-equality regression).

#4 — Confirming this is intended: with no publish.include, pack now matches the same broad set as deno publish. Conscious parity change.

CI was failing on pack::reproducible::two_packs_produce_identical_tarball. Root cause: that test packs twice in the same dir (--output a.tgz then --output b.tgz), and after switching to exact-output-name exclusion, the second run collected the leftover a.tgz as an asset and embedded it, so the two tarballs were no longer byte-identical.

Fix (44f9e41): restore the root-level .tgz artifact drop, but gate it on the package not having an explicit publish.include. So we still honor the original motivation here — a package that explicitly lists a root-level tarball in publish.include keeps it verbatim — while defaulting to dropping stray .tgz files (pack/npm pack leftovers) when include is the implicit "everything". The exact tarball being written is still always excluded. Full pack:: spec suite passes locally (28/28). The remaining red check (test-worker-fshandles-open-close-on-termination.js timing out on Windows node_compat) is an unrelated flaky timeout.