fix(scripts): accept CRLF tar listings in check-package - #451
Open
MFA-G wants to merge 1 commit into
Open
Conversation
The non-regular entry check ran a multiline regular expression over the raw `tar -tvzf` output. Both CR and LF are line terminators for multiline anchors in JavaScript, so `^` matched between the CR and the LF and the following LF was tested as the first character of a line. A tar that emits CRLF therefore failed a valid tarball containing only regular files. Normalize the listing before inspecting entry types, sharing the split with the check just below it that already normalized the same output.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sdk/typescript/scripts/check-package.mjsrejects a valid npm tarball when the localtaremits CRLF line endings fortar -tvzf, as reported in #32.The non-regular entry check ran a multiline regular expression directly over the raw listing:
In JavaScript both CR and LF are line terminators for multiline anchors, so
^matches between the CR and the LF. The following\nis then tested as if it were the first character of a new line, it is neitherdnor-, and a tarball containing only regular files and directories is reported as containing a link, device, or pipe.The check immediately below already normalized the very same output with
split(/\r?\n/u), so the two validations disagreed about what a line is.Changes
sdk/typescript/scripts/tar-listing.mjswith two small helpers:tarListingLines()normalizes atar -tvlisting to entry lines (LF or CRLF), andhasOnlyRegularEntries()checks the entry-type column of already-split lines.check-package.mjsso entry types are inspected per line instead of via a multiline regex over the raw output.entriesnow reusestarListingLines()rather than repeating the same split expression.sdk/typescript/tests-ts/tar-listing.test.tscovering LF and CRLF listings, unterminated and empty listings, and rejection of symbolic links, hard links, devices, and pipes under both line endings.Behavior is unchanged for LF listings, and genuinely non-regular entries are still rejected — the symbolic-link, hard-link, device, and pipe cases are asserted with CRLF as well as LF, so the fix cannot silently weaken the check.
Testing
Run from
sdk/typescripton Linux (Node.js 22.23.2, pnpm 11.9.0, bun 1.3.14):bun test tests-ts/tar-listing.test.ts— 8 pass, 0 fail.pnpm run types— passes.pnpm run format— "All matched files use Prettier code style!".pnpm run build&&pnpm packthennode scripts/check-package.mjs ../../dist/<tarball>.tgz— every tar listing and entry-type check passes. The run then stops in the later installed-package smoke step, because installing the packed tarball resolves an upstream dependency version that is not published to the registry from this environment (ETARGET ... @openai/codex@<alpha>). That failure is unrelated to this change and reproduces on an unmodified checkout.I confirmed the fix addresses the reported cause rather than the symptom, by exercising the old and new logic against the same fixtures:
I do not have a Windows machine available, so the CRLF path is covered by the fixtures above rather than by a real CRLF-emitting
tar.Risk and rollout
Low. The change is confined to a build-time packaging validation script; no runtime or published SDK code is touched. It makes the check accept listings it should always have accepted, and adds no new dependency.
Public disclosure review
Fixes #32