fix(publish): create http tarball urls correcly#23223
Conversation
|
Updated 6:11 PM PT - Oct 7th, 2025
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 23223That installs a local version of the PR into your bun-23223 --bun |
WalkthroughParses and normalizes registry URLs in the publish command (forcing http), builds tarball URLs via URL.join, introduces new C++ URL bindings and Zig wrappers (including setProtocol and changed signatures), removes older URL C-exports, adjusts SocketAddress URL error handling, and adds a test verifying tarball creation and registry metadata. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/cli/publish_command.zig(1 hunks)src/string/immutable.zig(1 hunks)test/cli/install/bun-publish.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/install/bun-publish.test.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/install/bun-publish.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/install/bun-publish.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/cli/install/bun-publish.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/cli/install/bun-publish.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/cli/install/bun-publish.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/string/immutable.zigsrc/cli/publish_command.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/string/immutable.zigsrc/cli/publish_command.zig
🔇 Additional comments (3)
src/string/immutable.zig (1)
2146-2151: LGTM! Clean runtime prefix removal.The function correctly mirrors the existing
withoutPrefixComptime/withoutSuffixComptimepattern for runtime usage. Logic is straightforward and consistent with the codebase conventions.test/cli/install/bun-publish.test.ts (2)
852-893: Good test coverage for tarball creation and metadata.The test validates tarball creation, registry metadata, and URL normalization. The snapshot assertion confirms that
dist.tarball,dist.integrity, andshasumfields are populated correctly after publish.
895-910: LGTM! Clean URL normalization helper.The function correctly normalizes localhost URLs in nested objects and strings, ensuring stable snapshot comparisons. The recursive approach handles all data types appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/bun.js/api/bun/socket/SocketAddress.zig(1 hunks)src/bun.js/bindings/BunString.cpp(1 hunks)src/bun.js/bindings/URL.zig(2 hunks)src/cli/publish_command.zig(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/publish_command.zig
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/BunString.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/BunString.cpp
**/*.{cpp,cc,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format C/C++ sources/headers with clang-format (bun run clang-format)
Files:
src/bun.js/bindings/BunString.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: For JS classes with a public constructor, implement Foo, FooPrototype, and FooConstructor (JSC patterns)
Define class properties using HashTableValue arrays in JSC bindings
Add iso subspaces for classes with C++ fields in JSC bindings
Cache structures in ZigGlobalObject for JSC-bound classes
Files:
src/bun.js/bindings/BunString.cpp
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/bindings/URL.zigsrc/bun.js/api/bun/socket/SocketAddress.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/bindings/URL.zigsrc/bun.js/api/bun/socket/SocketAddress.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/bindings/URL.zigsrc/bun.js/api/bun/socket/SocketAddress.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/bun/socket/SocketAddress.zig
🧬 Code graph analysis (1)
src/bun.js/bindings/BunString.cpp (1)
src/bun.js/bindings/webcore/WebSocket.cpp (2)
url(924-927)url(924-924)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (4)
src/bun.js/api/bun/socket/SocketAddress.zig (1)
119-119: LGTM!The change from
orelsetocatchcorrectly adapts to the new error-based URL parsing API while preserving identical behavior.src/bun.js/bindings/BunString.cpp (1)
642-646: LGTM!The implementation correctly bridges Zig to WebKit's URL protocol setter. Using
ZeroCopymode is appropriate for the temporary string passed tosetProtocol.src/bun.js/bindings/URL.zig (2)
5-5: LGTM!The extern declaration correctly matches the C++ implementation in BunString.cpp.
62-71: All jsc.URL.fromString/fromUTF8 calls handle errors correctly.
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
src/bun.js/bindings/URL.cpp (1)
109-112: Consider adding a null check for clarity.While
delete nullptris safe in C++, adding an explicit check improves code clarity and consistency with other functions.Apply this diff to add a null check:
extern "C" void URL__deinit(WTF::URL* url) { + if (!url) + return; delete url; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/bindings/BunString.cpp(0 hunks)src/bun.js/bindings/URL.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- src/bun.js/bindings/BunString.cpp
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/URL.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/URL.cpp
**/*.{cpp,cc,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format C/C++ sources/headers with clang-format (bun run clang-format)
Files:
src/bun.js/bindings/URL.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: For JS classes with a public constructor, implement Foo, FooPrototype, and FooConstructor (JSC patterns)
Define class properties using HashTableValue arrays in JSC bindings
Add iso subspaces for classes with C++ fields in JSC bindings
Cache structures in ZigGlobalObject for JSC-bound classes
Files:
src/bun.js/bindings/URL.cpp
🧬 Code graph analysis (1)
src/bun.js/bindings/URL.cpp (1)
src/bun.js/bindings/BunString.cpp (8)
toStringRef(213-228)toStringRef(213-213)toStringRef(252-259)toStringRef(252-252)toStringRef(260-267)toStringRef(260-260)toStringRef(268-276)toStringRef(268-268)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (3)
src/bun.js/bindings/URL.cpp (3)
40-55: LGTM!The function correctly handles exceptions and converts JSValue to URL string with appropriate error returns.
34-34: Verify WTF::URL validity covers nullThe
!url.isValid() || url.isNull()check may be redundant ifisValid()already returns false for null URLs. Confirm in the WebCore/WTF::URL documentation or source before removingurl.isNull().
134-137: Add null check for url parameter.The function dereferences
urlwithout checking for null, which can cause a crash.Apply this diff to add a null check:
extern "C" BunString URL__host(WTF::URL* url) { + if (!url) + return { BunStringTag::Dead }; return Bun::toStringRef(url->host().toStringWithoutCopying()); }Likely an incorrect or invalid review comment.
Windows doesn't preserve filesystem attribute metadata the same way as POSIX systems, resulting in different tarball hashes. Use separate inline snapshots for Windows and POSIX platforms. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
| // always use replace https with http | ||
| // https://github.com/npm/cli/blob/9281ebf8e428d40450ad75ba61bc6f040b3bf896/workspaces/libnpmpublish/lib/publish.js#L120 | ||
| strings.withoutTrailingSlash(strings.withoutPrefixComptime(registry.url.href, "https://")), | ||
| .data = try std.fmt.allocPrint(allocator, "{s}/{s}/-/{}", .{ |
There was a problem hiding this comment.
@dylan-conway this should use the URL parser.. The protocol is unlikely to cause issues. But, the pathname could very much cause issues.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
If we don't use the URL parser to construct pathnames from user input, the URL may be invalid and the request will fail
Replace allocPrint string concatenation with URL.join to properly handle URL path joining. This ensures the pathname is correctly joined with the base registry URL.
The tarball URL string was being freed via `defer tarball_url_slice.deinit()` before it was actually used in the dist properties. This caused a use-after-free bug that manifested as assertion failures, particularly on Windows in debug builds. The fix duplicates the string using the allocator so it persists beyond the defer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/cli/publish_command.zig(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bun
Files:
src/cli/publish_command.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/cli/publish_command.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
| if (registry_url.protocol().eqlUTF8("https")) { | ||
| registry_url.setProtocol(bun.String.static("http")); | ||
| } |
There was a problem hiding this comment.
Release the protocol string returned by registry_url.protocol().
bun.String instances carry a ref that must be deref()’d. Grabbing the protocol inline leaks that string every publish. Please capture it, compare, and defer the deref():
- if (registry_url.protocol().eqlUTF8("https")) {
+ var protocol = registry_url.protocol();
+ defer protocol.deref();
+ if (protocol.eqlUTF8("https")) {
registry_url.setProtocol(bun.String.static("http"));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (registry_url.protocol().eqlUTF8("https")) { | |
| registry_url.setProtocol(bun.String.static("http")); | |
| } | |
| var protocol = registry_url.protocol(); | |
| defer protocol.deref(); | |
| if (protocol.eqlUTF8("https")) { | |
| registry_url.setProtocol(bun.String.static("http")); | |
| } |
🤖 Prompt for AI Agents
In src/cli/publish_command.zig around lines 982 to 984, the code calls
registry_url.protocol() inline which returns a bun.String that must be
deref()'d; capture the protocol into a local variable (e.g., const proto =
registry_url.protocol()), defer proto.deref(), use proto.eqlUTF8("https") for
the comparison, and then call registry_url.setProtocol(...) if needed so the
temporary bun.String is properly released and no leak occurs.
3 participants
What does this PR do?
"tarball"was prefixed with two"http://"if the registry url was http.How did you verify your code works?
Added a test