fix(atomic): preserve long destination filenames - #191
Conversation
Keep private staging names independent of destination basenames across archive extraction, guarded writes, native Windows writes, and sync JSON. Reuse the existing archive default instead of adding a destination prefix. Preserve public producer filenames, custom temporary prefixes, literal JSON parent segments, and the existing identity and publication checks.
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
|
Codex review: needs changes before merge. Reviewed September 1, 2026, 7:02 AM ET / 11:02 UTC. ClawSweeper reviewWhat this changesThe PR replaces destination-derived private staging names with bounded UUID-based names across archive extraction, Root writes, Windows native writes, and synchronous JSON writes, with long-filename regressions. Merge readinessKeep open: the fix addresses a source-proven long-filename failure, but this public package behavior change lacks the required Unreleased changelog entry. Priority: P2 Review scores
Verification
How this fits togetherAtomic writes and archive extraction stage content beside the destination, validate it, then rename it into place. The private staging filename must fit the filesystem independently of the final user-selected filename. flowchart LR
A[Destination basename] --> B[Atomic write or archive extraction]
B --> C[Private staging filename]
C --> D[Guarded content write]
D --> E[Identity-checked rename]
E --> F[Published destination]
G[Long-name regression tests] --> B
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Add a concise 0.7.1 Unreleased entry describing support for filesystem-admitted long destination filenames, then retain the bounded private staging implementation and its regressions. Do we have a high-confidence way to reproduce the issue? Yes: current-main source appends staging suffixes to full destination basenames, and the added regressions establish direct filesystem admission before exercising the affected APIs. This review did not execute the tests. Is this the best way to solve the issue? Yes, aside from the required release note: decoupling private temporary names from destination names is narrower than restricting valid destinations and retains the existing guarded publication design. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 790472f73bb4. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Merged as 8d4ff13365c33db031c4b7cbd48e40777a98733c. All twelve changed files match the reviewed head, and the task checkout is clean on main after a fast-forward pull. A fresh managed P0–P2 review found no actionable findings. Production: net −2 lines; tests: +58; docs: +9. No package version, tag, or npm publication was performed. The long-filename repair retains the guarded publication and identity checks. All eleven selected regressions pass after five intended baseline failures; normal The ClawSweeper changelog finding and matching rank-up move are intentionally skipped: changelog/version edits belong to release preparation. This repair PR preserves the release-note context in its body, and does not choose a future version, tag, or publish a package. The user-visible change is that filesystem-admitted destination basenames no longer overflow when private atomic-write/archive staging names are created. No remaining code or security finding was reported. |
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(atomic): preserve long destination filenames This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
Finalize lock reliability, native ZIP integrity, and long-filename release notes through PR #191. Align all package/crate versions and exact native pins, preserving historical notes and existing trusted-publishing behavior.
1 participant
What Problem This Solves
Fixes an issue where consumers writing or extracting valid long filenames would receive
ENAMETOOLONG, or a wrapped filesystem error, because an internal temporary filename copied the entire destination basename before adding a random suffix.On current main
790472f73bb499ec8eb0ee7a823de1e98b0a94c0, this reproduces for 200/240-byte ZIP basenames, 240-byteRoot.writeandRoot.copyInbasenames, and 240-byte synchronous JSON basenames. Each regression first creates and reads the destination directly to establish that the filesystem accepts it.Why This Change Was Made
Private staging names now use a fixed prefix and UUID independent of the destination. Archive extraction reuses its existing bounded default. JSON staging preserves literal parent segments and drive-relative semantics rather than normalizing the path through a different parent.
The repair leaves public producer/callback filenames, custom
tempPrefix, final destinations, exports, configuration, permissions, and identity checks unchanged. The existing retained-descriptor, publication, and cleanup logic remains responsible for safety. A partial-write test now identifies write handles by their flags and asserts that a short write actually occurred, so it no longer depends on an internal filename containing the destination.Production: +10/−12, net −2. Tests: +59/−1, net +58. Docs: +12/−3, net +9.
User Impact
Consumers can use long filenames accepted by the filesystem without private staging consuming extra filename space. Short-name controls and the public naming contracts retain their behavior. No new fallback, setting, dependency, or migration is introduced. Release-note context is recorded here; changelog changes remain release-owned.
Evidence
pnpm check:changed(the repository'spnpm checkalias) passed lint, build, documentation checks, the full suite (4,026 passed / 2,333 skipped), and real root-tarball consumer installation/import/public-API checks. No dependency or lockfile changes and no test retries.pnpm package:smokerequires a prebuilt host.nodepayload that is absent locally; its normal CI jobs remain required before landing. The packed-consumer check above is not claimed as native-platform proof.Focused command before and after:
pnpm test test/archive.test.ts test/fs-safe.test.ts test/json.test.ts \ -t filesystem-admitted --reporter=json --outputFile=results.json pnpm check:changedEnvironment: macOS arm64, Node 24.20.0, pnpm 11.24.0. Introduction provenance is unknown.