Conversation
|
Updated 5:57 AM PT - Aug 13th, 2026
✅ @robobun, your commit b90818554271e39ae5f9be33ff434e50ed0a5a64 passed in 🧪 To try this PR locally: bunx bun-pr 28515That installs a local version of the PR into your bun-28515 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/outdated_command.zig`:
- Around line 717-725: The current logic treats any repo_url containing '/' as a
GitHub shorthand and rewrites it, which mangles values like
"github.com/user/repo" or "git@github.com:user/repo"; update the branch that now
checks strings.contains(repo_url, "/") to only treat true owner/repo shorthands:
require that repo_url contains a single '/' and does NOT contain '.' or '@' (and
does not start with "git" or "http") before prepending "https://github.com/";
otherwise leave repo_url verbatim (so cases like "github.com/...", "git@...", or
other hosts are printed unchanged). Reference symbols: repo_url, package_name,
strings.hasPrefixComptime, strings.contains, and Output.prettyln in
outdated_command.zig.
In `@src/install/npm.zig`:
- Around line 919-920: The cached extended manifest flag is being trusted even
when repository_url is empty, causing changelog lookups to be skipped; update
the loading/lookup logic in PackageManifestMap (and any deserialization in
npm.zig where repository_url: []const u8 = &.{}) to treat a cached manifest as
invalid extended data if repository_url is empty — either (A) reject/clear
has_extended_manifest when repository_url is missing so callers won't assume a
repository URL exists, or (B) persist repository_url to disk alongside
has_extended_manifest during serialization so it is restored on load; modify the
code paths that set/verify has_extended_manifest (PackageManifestMap
lookup/validation) to explicitly check repository_url.len > 0 before accepting
the cached extended manifest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc7e526e-6dc9-4bf6-9de5-858f565a402a
📒 Files selected for processing (6)
src/cli/outdated_command.zigsrc/install/PackageManager/CommandLineArguments.zigsrc/install/PackageManager/PackageManagerOptions.zigsrc/install/PackageManager/PopulateManifestCache.zigsrc/install/npm.zigtest/regression/issue/28513.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/install/PackageManager/PopulateManifestCache.zig (1)
61-70:⚠️ Potential issue | 🟠 MajorMirror the warm-cache
repository_urlretry in the.allbranch.This path still treats any cached extended manifest as complete. A
populateManifestCache(.all)call with--changelogcan therefore reuse a disk-loaded manifest whose transientrepository_urlwas never serialized, and the changelog URL stays missing. The.idsbranch already fixes that case;.allneeds the same guard.Suggested fix
- _ = manager.manifests.byName( + const cached = manager.manifests.byName( manager, manager.scopeForPackageName(pkg_name.slice(string_buf)), pkg_name.slice(string_buf), .load_from_memory_fallback_to_disk, needs_extended_manifest, - ) orelse { + ); + if (cached == null or (manager.options.changelog and cached.?.repository_url.len == 0)) { try startManifestTask(manager, pkg_name.slice(string_buf), dep, needs_extended_manifest); - }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/install/PackageManager/PopulateManifestCache.zig` around lines 61 - 70, The `.all` branch currently treats a cached extended manifest as complete even when transient fields like repository_url may be missing; update the lookup in manager.manifests.byName inside PopulateManifestCache (.all path) to mirror the `.ids` branch’s retry logic by checking whether the cached manifest actually contains repository_url when needs_extended_manifest is true (i.e., when manager.options.changelog or minimum_release_age_ms is set) and, if missing, call startManifestTask(manager, pkg_name.slice(string_buf), dep, needs_extended_manifest) to re-fetch and populate the repository_url before returning the cached manifest.src/install/npm.zig (1)
2806-2820:⚠️ Potential issue | 🟠 MajorHandle non-GitHub hosted repository syntaxes instead of dropping or misrouting them.
This helper only canonicalizes GitHub-specific hosted forms.
git@gitlab.com:group/project.gitreturns empty here, andgitlab:group/projectfalls through unchanged and is later rendered as a GitHub URL byoutdated_command.zig. That means--changelogeither hides or misrenders valid repository fields for non-GitHub hosts. Preserve enough host/path information to printhttps://{host}/{path}(or normalize those hosted prefixes explicitly) instead of returning&.{}/ passing them through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/install/npm.zig` around lines 2806 - 2820, The current normalization drops non-GitHub SCP-style and hosted prefixes by returning &.{} or leaving them unchanged; update the SCP/host handling around the url variable so that when strings.hasPrefixComptime(url, "git@") and the remaining string contains a ':' you parse the part before the ':' as the host and the part after as the path, strip any trailing ".git", and return/produce a normalized host/path (e.g. "https://{host}/{path}") instead of returning &.{}; similarly extend the hosted-prefix branch (where you check "github:" today) to recognize/normalize other hosted prefixes like "gitlab:" by mapping them to their proper host/path form; use the existing strings.hasPrefixComptime and strings.indexOfChar helpers to locate separators and ensure you do not lose the host when constructing the normalized URL so outdated_command.zig receives a proper https://host/path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28513.test.ts`:
- Around line 85-123: Add a second, cross-process invocation of the same temp
project to exercise the warm-cache branch in PopulateManifestCache.zig (the
cached.?.repository_url.len == 0 case): after the first `bun outdated
--changelog` run (the cold-cache run) keep caching enabled and spawn a second
`Bun.spawn` against the same directory, capture its stdout/stderr/exit code, and
assert the changelog section still correctly shows or omits the repository URL
(same expectations as the first run); apply the same pattern for the other test
block referenced (lines 126-161) so the post-reload manifest cache path is
exercised.
---
Duplicate comments:
In `@src/install/npm.zig`:
- Around line 2806-2820: The current normalization drops non-GitHub SCP-style
and hosted prefixes by returning &.{} or leaving them unchanged; update the
SCP/host handling around the url variable so that when
strings.hasPrefixComptime(url, "git@") and the remaining string contains a ':'
you parse the part before the ':' as the host and the part after as the path,
strip any trailing ".git", and return/produce a normalized host/path (e.g.
"https://{host}/{path}") instead of returning &.{}; similarly extend the
hosted-prefix branch (where you check "github:" today) to recognize/normalize
other hosted prefixes like "gitlab:" by mapping them to their proper host/path
form; use the existing strings.hasPrefixComptime and strings.indexOfChar helpers
to locate separators and ensure you do not lose the host when constructing the
normalized URL so outdated_command.zig receives a proper https://host/path.
In `@src/install/PackageManager/PopulateManifestCache.zig`:
- Around line 61-70: The `.all` branch currently treats a cached extended
manifest as complete even when transient fields like repository_url may be
missing; update the lookup in manager.manifests.byName inside
PopulateManifestCache (.all path) to mirror the `.ids` branch’s retry logic by
checking whether the cached manifest actually contains repository_url when
needs_extended_manifest is true (i.e., when manager.options.changelog or
minimum_release_age_ms is set) and, if missing, call startManifestTask(manager,
pkg_name.slice(string_buf), dep, needs_extended_manifest) to re-fetch and
populate the repository_url before returning the cached manifest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffc93dc9-6d38-489c-8d0f-3fcb9458de5c
📒 Files selected for processing (4)
src/cli/outdated_command.zigsrc/install/PackageManager/PopulateManifestCache.zigsrc/install/npm.zigtest/regression/issue/28513.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/cli/outdated_command.zig (1)
722-728:⚠️ Potential issue | 🟡 MinorDon't treat unsupported schemes as GitHub shorthands.
The slash fallback still rewrites values like
file://...orftp://...tohttps://github.com/.... Please either print any*://value verbatim here or havenormalizeRepositoryUrl()drop unsupported schemes before they reach this branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/outdated_command.zig` around lines 722 - 728, The current fallback treats any string containing "/" (checked in the branch using strings.contains(repo_url, "/")) as a GitHub shorthand and rewrites values like "file://..." or "ftp://..." to "https://github.com/..."; update the logic in outdated_command.zig so before the "/" fallback you detect and handle explicit schemes (e.g., any repo_url containing "://") and either print that value verbatim via Output.prettyln (using the same formatting as the other branches) or ensure normalizeRepositoryUrl() strips unsupported schemes earlier; specifically adjust the branch decisions around hasDomainPrefix(repo_url), strings.contains(repo_url, "/"), and/or add a new check for strings.contains(repo_url, "://") so unsupported schemes are not rewritten to github shorthands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/npm.zig`:
- Around line 2080-2085: The code is appending normalized repository URLs into
the durable string buffer (string_buf) via string_builder.count(normalized),
causing transient repository_url to be persisted and cached; instead, stop
writing normalized into the persistent string buffer and keep repository_url in
a transient storage. Concretely: in the block using normalizeRepositoryUrl(url)
(and the similar sites around the other mentions), remove the call to
string_builder.count(normalized) and assign repository_url to the normalized
slice stored in a transient/local buffer or allocated memory that is not part of
string_buf; ensure Serializer.write() still skips repository_url and that no
other code pushes repository_url into string_buf (search for
string_builder.count(repository_url) or similar) so repository data remains out
of the serialized string_buf.
In `@test/regression/issue/28513.test.ts`:
- Around line 86-96: The helper runOutdatedChangelog ignores the test's custom
cache env (testEnv with BUN_INSTALL_CACHE_DIR); change runOutdatedChangelog to
accept an env parameter (e.g., env?: Record<string,string>) and use that when
calling Bun.spawn (merge with bunEnv as currently done) instead of hardcoding
env, and remove the internal expect(exitCode).toBe(0) so the caller can assert
exit code and preserve the stdout-before-exitCode ordering; update call sites to
pass testEnv where needed (referencing runOutdatedChangelog and the testEnv
construction that sets BUN_INSTALL_CACHE_DIR).
---
Duplicate comments:
In `@src/cli/outdated_command.zig`:
- Around line 722-728: The current fallback treats any string containing "/"
(checked in the branch using strings.contains(repo_url, "/")) as a GitHub
shorthand and rewrites values like "file://..." or "ftp://..." to
"https://github.com/..."; update the logic in outdated_command.zig so before the
"/" fallback you detect and handle explicit schemes (e.g., any repo_url
containing "://") and either print that value verbatim via Output.prettyln
(using the same formatting as the other branches) or ensure
normalizeRepositoryUrl() strips unsupported schemes earlier; specifically adjust
the branch decisions around hasDomainPrefix(repo_url),
strings.contains(repo_url, "/"), and/or add a new check for
strings.contains(repo_url, "://") so unsupported schemes are not rewritten to
github shorthands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: abe366ce-5861-47a8-8031-71a8ee05a69d
📒 Files selected for processing (3)
src/cli/outdated_command.zigsrc/install/npm.zigtest/regression/issue/28513.test.ts
There was a problem hiding this comment.
LGTM — core functionality is correct, all major issues from prior review rounds have been addressed.
Extended reasoning...
Overview
This PR adds a --changelog flag to bun outdated across 6 files: CLI argument parsing (CommandLineArguments.zig, PackageManagerOptions.zig), manifest cache population (PopulateManifestCache.zig), npm manifest parsing (npm.zig), the outdated command output (outdated_command.zig), and a regression test (28513.test.ts).
Security risks
No security-sensitive code is touched. The change only reads metadata from npm registry responses and formats strings for terminal output. No auth, crypto, or permission-related code is modified.
Level of scrutiny
This is a new user-facing feature (new CLI flag) with supporting plumbing. The changes are additive and isolated — existing bun outdated behavior is unchanged when --changelog is not passed. The repository_url field is explicitly excluded from the disk cache serializer to avoid format changes, and the warm-cache re-fetch logic correctly handles the case where a cached manifest has has_extended_manifest=true but an empty repository_url.
Other factors
Multiple rounds of review identified and fixed: URL normalization bugs (git://, ssh://git@, non-GitHub SCP URLs), the warm-cache silent empty section, duplicate entries in monorepos, misattribution of bitbucket/gitlab shorthands, and CLAUDE.md violations in tests. All were addressed. The two remaining nit-level issues (warm-cache test accidentally using the default cache and a port-number edge case in ssh:// URLs) are covered by inline comments and have negligible practical impact.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
test/regression/issue/28513.test.ts (1)
86-99:⚠️ Potential issue | 🟡 MinorLet each test assert stdout before the exit code.
runOutdatedChangelog()fails onexitCodebefore the callers can inspect stdout, which hides the more useful failure context when this regresses. Return the process result and keep the stdout assertions ahead of the exit-code assertion in each test.As per coding guidelines, "In tests, expect stdout before exit code: expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0) for more useful error messages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/28513.test.ts` around lines 86 - 99, The helper runOutdatedChangelog currently awaits proc.stdout.text() and proc.exited then asserts exitCode first which hides stdout on failure; modify runOutdatedChangelog (the function name) to return the full process result (stdout, stderr, exitCode) instead of asserting inside the helper, and update each test caller to assert the stdout expectations (expect(stdout).toBe(...) or similar) before asserting expect(exitCode).toBe(0) so test failures show stdout first for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/npm.zig`:
- Around line 2644-2649: Replace the silent OOM swallow in the repository URL
duplication: instead of using "repository_url_duped = default_allocator.dupe(u8,
repo_url) catch &.{};", call bun.handleOom around the allocator dupe so
OutOfMemory turns into a crash while other errors still propagate (e.g.,
repository_url_duped = bun.handleOom(default_allocator.dupe(u8, repo_url));),
referencing repository_url_duped, default_allocator.dupe and bun.handleOom.
- Around line 919-920: The repository_url field is transient but still
participates in Serializer.sizes/ sizes.fields derived from
std.meta.fields(PackageManifest) (and gets re-ordered by the unstable pdq sort),
causing incompatible serialized layouts; either remove repository_url from the
fields list before sorting (exclude it from sizes.fields) or increment the
manifest-cache version constant so old caches are invalidated; also replace
default_allocator.dupe(u8, repo_url) catch &.{ } with bun.handleOom() to
properly propagate OOM, and revise normalizeRepositoryUrl() to preserve
non-GitHub SCP and non-GitHub shorthand forms (return the original repo string
or a normalized fallback instead of "") so repository metadata isn’t lost.
In `@test/regression/issue/28513.test.ts`:
- Around line 44-72: The test's mock registry always includes opts.repository so
it can't verify that the client requested the extended manifest; update the
Bun.serve fetch handler (the fetch function that constructs meta and returns
Response.json(meta)) to only add meta.repository when the incoming request shape
indicates an extended/full manifest request (e.g., inspect req.url or headers
inside fetch for the flag your client uses when --changelog requests extended
metadata) or alternatively assert in the handler that the request is for the
extended manifest before including repository; this ensures the test for the
--changelog behavior exercises the full-manifest path rather than passing with
abbreviated requests.
---
Duplicate comments:
In `@test/regression/issue/28513.test.ts`:
- Around line 86-99: The helper runOutdatedChangelog currently awaits
proc.stdout.text() and proc.exited then asserts exitCode first which hides
stdout on failure; modify runOutdatedChangelog (the function name) to return the
full process result (stdout, stderr, exitCode) instead of asserting inside the
helper, and update each test caller to assert the stdout expectations
(expect(stdout).toBe(...) or similar) before asserting expect(exitCode).toBe(0)
so test failures show stdout first for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd288d36-ca58-4c2c-a321-ddd76a1f9b1b
📒 Files selected for processing (2)
src/install/npm.zigtest/regression/issue/28513.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/regression/issue/28513.test.ts (1)
44-72: 🛠️ Refactor suggestion | 🟠 MajorThe mock still doesn't prove
--changelogrequested the full manifest.
setupMockRegistry()always addsmeta.repositorywhenopts.repositoryis set, so these tests stay green even if the client accidentally falls back to abbreviated metadata and the registry just happens to return the full document anyway. Gaterepositoryon the request shape that only the extended-manifest path uses, or assert that request explicitly, so the regression actually covers the fetch-mode switch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/28513.test.ts` around lines 44 - 72, The mock registry currently always injects meta.repository when opts.repository is set, which masks whether the client requested the full manifest; update the server fetch handler created by setupMockRegistry (the Bun.serve fetch(req) using url.pathname and opts.name) to only add meta.repository when the request matches the extended-manifest shape (i.e., the specific path or query the client uses to request full manifest) or alternatively add an explicit assertion in the test that the incoming req URL equals the extended-manifest endpoint before returning repository in meta; adjust the logic around opts.repository and the URL check so tests fail if the client uses abbreviated metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/npm.zig`:
- Around line 2797-2811: The normalization falls through for unknown
scheme-based URLs (e.g., file://, svn://) leaving them unchanged and later
causing bogus GitHub links; update the if/else chain in src/install/npm.zig
around the url variable and strings.hasPrefixComptime checks to detect any other
"*://" scheme (e.g., check for a "://"-style prefix) and immediately return the
empty slice (return &.{};) for those cases so unsupported scheme-based forms are
omitted rather than treated as GitHub shorthand.
---
Duplicate comments:
In `@test/regression/issue/28513.test.ts`:
- Around line 44-72: The mock registry currently always injects meta.repository
when opts.repository is set, which masks whether the client requested the full
manifest; update the server fetch handler created by setupMockRegistry (the
Bun.serve fetch(req) using url.pathname and opts.name) to only add
meta.repository when the request matches the extended-manifest shape (i.e., the
specific path or query the client uses to request full manifest) or
alternatively add an explicit assertion in the test that the incoming req URL
equals the extended-manifest endpoint before returning repository in meta;
adjust the logic around opts.repository and the URL check so tests fail if the
client uses abbreviated metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 418d34d4-b670-4127-8bb7-b9f5235cb5ce
📒 Files selected for processing (2)
src/install/npm.zigtest/regression/issue/28513.test.ts
There was a problem hiding this comment.
No bugs found, but this PR touches non-trivial serializer logic (compile-time field exclusion in PackageManifest) and network re-fetch logic in PopulateManifestCache.zig that would benefit from a human look.
Extended reasoning...
Overview
This PR adds a --changelog flag to bun outdated that fetches and displays repository/changelog URLs for outdated packages. Modified files: src/cli/outdated_command.zig, src/install/PackageManager/CommandLineArguments.zig, src/install/PackageManager/PackageManagerOptions.zig, src/install/PackageManager/PopulateManifestCache.zig, src/install/npm.zig, and a new test file.
Security risks
No meaningful security risks. The URL normalization only processes and displays strings — URLs are never executed or followed programmatically. No auth or permission-sensitive code is touched.
Level of scrutiny
Medium. The PR is a feature addition rather than a trivial config change. The most sensitive part is the PackageManifest serializer: a new repository_url: []const u8 field is added but must be excluded from disk cache serialization. The existing serializer uses a compile-time sorted field layout, and the PR correctly computes a serialized_count at comptime to build an array that omits repository_url. If this exclusion were wrong it could silently change the serialized cache format. The URL normalization function (normalizeRepositoryUrl) is also non-trivial — it handles git+, ssh://, git://, git@, SCP-style, .git suffix, and shorthand forms — and has been iterated on multiple times across the PR history.
Other factors
The PR has gone through ~12 commits and multiple review cycles addressing URL edge cases, port-number ambiguity for numeric org names, warm-cache bugs, and string buffer leaks. The automated bug hunter found no issues. Despite the cleanup, the non-trivial serializer interaction and the re-fetch heuristic in PopulateManifestCache.zig (re-fetches when repository_url.len == 0 on a cached entry) are worth a human confirming before merge.
b3c858c to
8c7a55e
Compare
There was a problem hiding this comment.
LGTM — all previously flagged bugs have been resolved.
Extended reasoning...
Overview
This PR adds a --changelog flag to bun outdated that displays repository URLs for outdated packages. It touches 6 files: two CLI/options files for the new flag, PopulateManifestCache.zig for warm-cache re-fetch logic, npm.zig for URL extraction and normalization, outdated_command.zig for the Changelogs display section, and a new regression test.
Security Risks
No security-sensitive code is touched. The URL normalization is purely string manipulation with no shell execution or untrusted deserialization. The repository_url field is read-only data from the npm registry manifest, displayed as-is to the terminal.
Level of Scrutiny
This PR went through ~10 rounds of review and the author addressed every issue raised, including subtle correctness bugs (warm-cache serialization interaction, unstable pdq sort field ordering, port-number vs numeric GitHub org ambiguity, dotted-username hasDomainPrefix misclassification). All fixes are confirmed present in the current codebase.
Other Factors
The test file correctly drains both stdout and stderr pipes with Promise.all, uses port: 0 for random ports, and does not assert on panic messages. The describe.concurrent was intentionally dropped in a simplification commit that also removed the warm-cache test. The normalizeRepositoryUrl function handles all edge cases: git+https://, git://, ssh://, SCP-style git@ URLs, GitHub/bitbucket/gitlab shorthands, unrecognized schemes, and .git suffix stripping — with bun.handleOom for all allocations.
1887f35 to
74d75a4
Compare
74d75a4 to
78e4bf6
Compare
|
Rebased onto current I re-implemented the feature directly against the Rust codebase and force-pushed a single clean commit:
Note: CodeRabbit's walkthrough/review comments above predate the rebase and still reference the |
ad5a91b to
4161659
Compare
|
CI status: the only red lanes are the same tests in
The feature's own tests ( |
There was a problem hiding this comment.
No new issues found after the latest round of fixes — but this is a new user-facing feature that threads through the package manager (manifest cache re-fetch logic, a transient non-serialized PackageManifest field, and a fairly intricate URL normalizer that took many iterations to get right), plus it was fully reimplemented Zig→Rust mid-PR, so it's worth a human pass before merging.
Extended reasoning...
Overview
This PR adds a --changelog flag to bun outdated that prints repository URLs for outdated packages below the version table. It touches 8 files: CLI flag parsing (CommandLineArguments.rs), option threading (PackageManagerOptions.rs), manifest-cache re-fetch logic (PopulateManifestCache.rs), npm manifest parsing + a new transient repository_url field + the normalize_repository_url helper (npm.rs), the Changelogs output section + has_domain_prefix (outdated_command.rs), shell completions, docs, and a 5-case test file.
Security risks
None identified. The feature reads the repository field from npm registry metadata (already fetched by bun outdated) and prints it to stdout. No auth, crypto, permissions, filesystem writes, or network endpoints beyond the existing registry fetch. The URL is display-only — not fetched or executed.
Level of scrutiny
Medium-high. While the feature itself is opt-in and cosmetic (a convenience listing), the implementation touches package-manager internals in non-trivial ways: it changes when manifests are re-fetched on warm-cache hits (PopulateManifestCache.rs), adds a field to PackageManifest that is intentionally excluded from on-disk serialization, and contains ~80 lines of URL-normalization logic whose edge cases (git+, ssh://, SCP, userinfo, ports vs. numeric org names, hosted shorthands, non-GitHub hosts) drove roughly 15 rounds of review feedback. The PR was also fully reimplemented from Zig to Rust mid-review after main landed the rewrite, so the entire current diff is effectively a fresh implementation that has only had automated review.
Other factors
- All ~20+ prior inline comments (from both me and CodeRabbit) are resolved, and the current bug-hunting pass found nothing new.
- Test coverage is reasonable: cold-cache with/without repo, warm-cache re-fetch, non-GitHub host preservation, and userinfo stripping — 5 concurrent tests.
- CI shows musl build failures and Windows
napi.test.tsfailures on the latest commit; these look like pre-existing/infra issues unrelated to this change, but worth confirming before merge. - No human reviewer has engaged on this PR yet — review has been entirely bot-driven.
Given the scope, the mid-PR rewrite, and the manifest-cache interaction, this exceeds my threshold for auto-approval and should get a human look.
4161659 to
a08ada2
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
a08ada2 to
b5c6bb4
Compare
b5c6bb4 to
f833b07
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. All prior automated-review feedback (URL normalization edge cases, userinfo/port handling, test placement, completions/docs, magic numbers) has been addressed in the current diff. Because it adds a new user-facing CLI flag with a hand-rolled repository-URL normalizer and changes manifest-cache re-fetch behavior, a maintainer sign-off on the API surface and output format would still be worthwhile.
What was reviewed:
normalize_repository_urltraced againstgit+,git://,ssh://, http(s) with/without userinfo,github:, bareuser/repo, SCP form, and explicit ports — thehad_url_schemegate now covers all URL-scheme branches and non-normalizable forms return empty rather than mis-render.PackageManifest.repository_urlis a plainBox<[u8]>populated only underis_extended_manifest; confirmed the on-disk cache serializer is field-list-driven so adding a struct field does not change the cache format.- Warm-cache re-fetch in
PopulateManifestCache.rsis gated onoptions.changelog, so defaultbun outdated/bun installpaths are unaffected. - Tests are hermetic (local
Bun.servemock registry,port: 0,tempDir,describe.concurrent), drain both pipes, and assert output before exit code.
Extended reasoning...
Overview
This PR adds a --changelog flag to bun outdated that prints repository URLs for outdated packages below the version table. It touches 8 files (~450 lines): CLI arg parsing and options threading, the PopulateManifestCache extended-manifest path, a new normalize_repository_url helper and transient repository_url field in npm.rs, the changelog-section printer in outdated_command.rs, plus completions, docs, and a 6-case test file.
Security risks
None identified. The only new external-input path is the npm registry repository field, which is parsed via existing Expr::get/as_utf8_string_literal and then sliced by prefix/suffix checks — no allocation-size arithmetic, no path/fs use, and the output is display-only. Userinfo (e.g. git@) is stripped before printing so credentials embedded in a repository URL are not echoed.
Level of scrutiny
Medium-high. The change is opt-in and display-only, so blast radius is small; but it is new user-facing API surface (flag name, output format, which URL forms are normalized vs. skipped), and the URL normalizer is a hand-rolled heuristic that went through several correction rounds during review. Per the repo's landing-PR guidance, new API surface benefits from a maintainer look. The manifest-cache re-fetch change is narrowly gated on options.changelog and mirrors the existing minimum_release_age_ms extended-manifest pattern, so the risk to install/outdated defaults is low.
Other factors
This PR has been through ~23 iterations with every prior automated finding addressed and marked resolved (double-prefixing, non-GitHub host attribution, userinfo pass-through, port-as-org, magic 4 offsets, test-file placement, missing completions/docs, stderr-before-exitCode). The current run's bug hunt found nothing new. Tests pass on both debug+ASAN and release, and fail on main without the change. Given the size and the API-surface nature, I'm deferring rather than approving so a maintainer can confirm the flag/output shape is what they want.
Displays repository URLs for each outdated package below the version table. Repository URLs come from the npm registry's extended (full) metadata and are normalized from git+https, git://, ssh://, git@ SCP, and GitHub-shorthand forms to browseable HTTPS URLs. - CommandLineArguments/Options: parse and thread the --changelog flag - PopulateManifestCache: request the extended manifest when --changelog is set, and re-fetch when a cached manifest lacks repository_url (the field is transient and not written to the on-disk cache) - npm: parse and normalize the repository field into a transient PackageManifest.repository_url (excluded from cache serialization); strip any user@ userinfo from the authority so the link is shown without embedded credentials - outdated_command: print a deduplicated "Changelogs" section; has_domain_prefix uses dot-before-first-slash so any hosting domain (gitlab.gnome.org, self-hosted, etc.) keeps its own host - completions + docs: list --changelog for bun outdated Ported to the Rust codebase following the Zig->Rust rewrite (#30412). Closes #28513
f833b07 to
b908185
Compare
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues on the current revision — all earlier findings (URL normalization edge cases, port-as-org, userinfo handling, warm-cache re-fetch, test placement/hygiene) have been addressed and are resolved. Because this adds a new user-facing CLI flag plus a hand-rolled repository-URL normalizer and changes manifest-cache re-fetch behavior, a maintainer sign-off on the API surface and normalization policy would still be worthwhile.
What was reviewed:
normalize_repository_urlinsrc/install/npm.rs— traced git+/git://, ssh://, http(s) with userinfo, SCP,github:shorthand, port-skip and.gitstripping; verifiedhad_url_schemenow gates the port-skip for all URL-scheme branches.PackageManifest.repository_url— confirmedSerializer::writedoes not emit it andread_all(viaPackageManifest::default()) leaves it empty, so the on-disk cache format (v0.0.7) is unchanged.PopulateManifestCache.rsre-fetch arm — checked bothPackages::AllandPackages::Idspaths mirror the sameneeds_refetchlogic.- Test file — 6 concurrent tests, local mock registry, per-test temp dirs, pipes drained; verified fails-on-main / passes-on-PR evidence in the description.
Extended reasoning...
Overview
This PR adds a --changelog flag to bun outdated that prints normalized repository URLs for outdated packages below the version table. It touches 8 files (~450 lines added): CLI arg parsing and options plumbing (mechanical), a new transient repository_url: Box<[u8]> field on PackageManifest populated from the extended npm manifest, a ~90-line normalize_repository_url function in src/install/npm.rs, warm-cache re-fetch logic in PopulateManifestCache.rs, ~70 lines of display logic plus a has_domain_prefix helper in outdated_command.rs, docs, completions, and a 6-test file with a mock registry.
Security risks
Low. The feature is opt-in and display-only — it reads the repository field from npm manifest JSON and prints a normalized form to stdout. The normalizer strips userinfo (so git@/user:pass@ never reaches the terminal), returns an empty slice for forms it can't normalize allocation-free, and is byte-slice indexing over registry-supplied text that never becomes a filesystem path or a spawned command. No auth, crypto, or permissions code is touched.
Level of scrutiny
Medium-high, and it has already received it: this PR has cycled through 23 iterations with many rounds of automated review that found and fixed real bugs in the URL normalizer (double-prefixed git:// output, non-GitHub SCP misattribution to github.com, ssh:// leaving git@ in the output, bitbucket:/gitlab: shorthands rendering as GitHub, port numbers rendering as org names, workspace duplicates). The current diff addresses all of those and every review thread is resolved. What remains for a human is the design/API judgment: whether --changelog is the right flag shape for bun outdated, whether the skip-rather-than-mis-render policy for unusual URL forms is acceptable, and whether re-fetching manifests on every warm-cache --changelog run (rather than serializing repository_url and bumping the cache format version) is the right trade-off.
Other factors
I verified independently that Serializer::write in npm.rs writes only the seven pre-existing arrays plus pkg and does not include repository_url, and that read_all starts from PackageManifest::default() — so the on-disk cache format is genuinely unchanged and no version bump is needed. The two Packages::All / Packages::Ids arms in PopulateManifestCache.rs carry identical needs_extended_manifest and needs_refetch logic. The tests are hermetic (local Bun.serve({port: 0}) registry, isolated temp dirs, describe.concurrent, pipes drained via Promise.all), and the robobun evidence shows they fail on main and pass on the PR under both debug+ASAN and release. This is a well-tested, thoroughly-reviewed feature — deferring solely because it introduces new user-facing API surface, which per repo convention warrants maintainer sign-off.
This branch was successfully deployed
1 participant
Closes #28513
Problem
bun outdatedlists outdated dependencies but does not show where to find changelogs or release notes. Users have to look up each package's repository manually.Solution
Add a
--changelogflag that prints repository URLs for each outdated package below the version table:How it works
--changelogforces fetching full (non-abbreviated) npm registry metadata, which includes therepositoryfield.git+https://,git://,ssh://,git@SCP,github:/ bareuser/reposhorthand,.gitsuffix).has_domain_prefixuses dot-before-first-slash, so any hosting domain (github.com, gitlab.gnome.org, self-hosted, …) keeps its own host. Forms that cannot be normalized without allocation (non-GitHub SCP,gitlab:/bitbucket:/gist:shorthands, unsupported schemes, andhttps://URLs carrying userinfo likegit@) are skipped rather than mis-rendered.PackageManifest.repository_urlfield, excluded from the on-disk cache serializer so the cache format is unchanged. On a warm-cache hit where the field is empty, the manifest is re-fetched.Changelogssection lists one URL per package name.Completions (
completions/bun-cli.json) and docs (docs/pm/cli/outdated.mdx) both list the new flag.Note on the rebase
This PR originally targeted the Zig sources. Since then
mainlanded the Zig to Rust rewrite (#30412) and removed the.zigreference sources (#32621), so the original diff no longer applied. The feature is implemented against the Rust codebase:src/install/PackageManager/CommandLineArguments.rs— parse--changelogsrc/install/PackageManager/PackageManagerOptions.rs— thread the optionsrc/install/PackageManager/PopulateManifestCache.rs— extended-manifest fetch + warm-cache re-fetchsrc/install/npm.rs—repositoryparse +normalize_repository_url+ transientrepository_url(not serialized)src/runtime/cli/outdated_command.rs—Changelogssection +has_domain_prefix(Any CodeRabbit walkthrough still referencing
.zigpaths is stale; the current diff is all Rust.)Rebased again onto current
main. Besides visibility tightening (pubtopub(crate)) on neighboring fields, the only substantive resolution was insrc/install/npm.rs:Expr::as_string(&bump)no longer exists, so therepositoryfield extraction now usesExpr::getandExpr::as_utf8_string_literal, matching how the rest of the manifest parser reads JSON strings onmain.Verification
bun bd test test/cli/install/outdated-changelog.test.ts— 5 tests pass (repo URL shown, omitted when absent, non-GitHub host keeps its domain, userinfo stripped, warm cache still shows the URL).USE_SYSTEM_BUN=1 bun test test/cli/install/outdated-changelog.test.ts— fails (released bun has no--changelog), confirming the tests exercise the feature.[review] gate passed · iteration 23 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 23
evidence per changed file
root cause · written by the author bot
The changelog URLs depend on the repository field, which is absent from the abbreviated npm manifests Bun normally requests and is also excluded from the on-disk manifest cache, so warm cache runs and default fetches had no repository data to display. The fix treats the changelog flag as requiring extended manifests, parses the repository field into a transient repository_url during manifest population, and schedules a refetch when a cached manifest lacks that field. The outdated command then normalizes these URLs into browseable links and prints a deduplicated changelogs section for the ou…