install: accept ms-style duration strings for minimumReleaseAge#30529
install: accept ms-style duration strings for minimumReleaseAge#30529robobun wants to merge 2 commits into
Conversation
|
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:
WalkthroughUpdates docs and adds tests for Bun's minimum release age: CLI and bunfig accept ms-style duration strings or seconds, ChangesMinimum Release Age Feature Documentation and Tests
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/bunfig.zig`:
- Around line 799-801: Update the error text in the branch that checks `ms < 0`
so it reflects that zero is allowed: replace the message passed to
`this.addError(min_age.loc, ...)` (currently "Expected a positive duration for
minimumReleaseAge") with wording like "Expected a non-negative duration for
minimumReleaseAge" or "Expected a non-negative duration for minimumReleaseAge (0
disables the filter)"; this change should be made in the `ms < 0` branch where
`this.addError(min_age.loc, ...)` is called to avoid implying that 0 is invalid.
In `@src/install/PackageManager/CommandLineArguments.zig`:
- Around line 846-851: The error messages for parsing --minimum-release-age
currently say "positive" but the code accepts 0; update both Output.errGeneric
calls that reference min_age (the ones guarding ms and the parse branch that
calls Global.crash) to use "non-negative" instead of "positive" so the CLI
wording matches the actual behavior for variable min_age and ms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40fb564d-0f6b-4b39-b64f-3cbdf3dbce67
⛔ Files ignored due to path filters (7)
test/cli/install/__snapshots__/bun-install-registry.test.ts.snapis excluded by!**/*.snaptest/cli/install/__snapshots__/bun-lock.test.ts.snapis excluded by!**/*.snaptest/cli/install/__snapshots__/catalogs.test.ts.snapis excluded by!**/*.snaptest/cli/install/migration/__snapshots__/pnpm-comprehensive.test.ts.snapis excluded by!**/*.snaptest/cli/install/migration/__snapshots__/pnpm-lock-migration.test.ts.snapis excluded by!**/*.snaptest/cli/install/migration/__snapshots__/pnpm-migration-complete.test.ts.snapis excluded by!**/*.snaptest/cli/install/migration/__snapshots__/yarn-lock-migration.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
docs/pm/cli/install.mdxdocs/runtime/bunfig.mdxsrc/bun_core/fmt.zigsrc/cli/bunfig.zigsrc/cli/outdated_command.zigsrc/cli/update_interactive_command.zigsrc/install/ConfigVersion.zigsrc/install/PackageManager.zigsrc/install/PackageManager/CommandLineArguments.zigsrc/install/PackageManager/PackageManagerOptions.zigsrc/install/PackageManager/install_with_manager.zigtest/cli/install/bun-add.test.tstest/cli/install/bun-install-security-provider.test.tstest/cli/install/bun-install.test.tstest/cli/install/bun-workspaces.test.tstest/cli/install/config-version.test.tstest/cli/install/dummy.registry.tstest/cli/install/minimum-release-age.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/minimum-release-age.test.ts`:
- Around line 1328-1338: The spawn-based install test currently only awaits
proc.stderr and proc.exited and asserts exitCode and stderr; update the early
guard to also capture stdout and assert both exitCode and stdout together (e.g.,
await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) and use
expect({ stdout, exitCode }).toMatchObject({ exitCode: 0 })) before any lockfile
or file reads so failures include stdout for clearer diffs; apply the same
change to the other similar spawn blocks referenced in this file (the ones
around lines ~1354, ~1404, ~1455, ~1495, ~1523, ~1559, ~1588) that use Bun.spawn
with proc, stdout/stderr handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8b2fe2c-b104-4d6d-b4b0-991b24aa52e4
📒 Files selected for processing (8)
docs/pm/cli/install.mdxdocs/runtime/bunfig.mdxsrc/bun_core/env_var.zigsrc/cli/bunfig.zigsrc/install/ConfigVersion.zigsrc/install/PackageManager.zigsrc/install/PackageManager/CommandLineArguments.zigtest/cli/install/minimum-release-age.test.ts
robobun
force-pushed
the
farm/9555d6db/minimum-release-age-default-and-ms
branch
from
f38bd23 to
42046bd
Compare
Statusms-style duration strings for
The CIThe red lane on build 60209 ( Diff is green; ready for review/merge. |
There was a problem hiding this comment.
Thanks for addressing the earlier feedback — the trim-before-parse_f64, finite-check, idempotency guard, and bun pm trust configVersion preservation all look right now. I didn't find anything new, but given this introduces ConfigVersion::V2 and the gated 2-day default rollout for 1.4 (plus several new unsafe disjoint-borrow sites), I'd like a human to sign off on the design.
Extended reasoning...
Overview
This PR makes two changes to minimumReleaseAge: (1) accepts ms-style duration strings via a new bun_core::parse_ms() and threads it through bunfig + CLI parsing; (2) introduces ConfigVersion::V2 with a 2-day default minimumReleaseAge for fresh projects, gated behind BREAKING_CHANGES_1_4 / BUN_FEATURE_FLAG_INSTALL_CONFIG_V2. A new apply_config_version_defaults() helper is wired into install_with_manager, bun outdated, bun update -i, and bun pm trust. ~15 files, ~600 lines, plus docs and ~320 lines of new tests.
Security risks
The feature itself is a supply-chain security control. The earlier 1000×-weakening risks (quoted bare-number → ms, trailing-whitespace fallthrough, NaN/inf acceptance) have all been fixed in the latest revision. The remaining risk surface is the configVersion gating: an existing-project lockfile must not be silently bumped to V2. The bun pm trust path is now covered; my earlier note about bun pm migrate was a "please confirm" rather than a confirmed bug. No injection/auth concerns.
Level of scrutiny
High. This is core bun install resolution behaviour, introduces a version-gated default that will change what every new project resolves once 1.4 ships, and adds several unsafe { &mut *pm_ptr } projections with SAFETY comments asserting disjoint borrows against load_result. The MAX_KNOWN vs CURRENT split in ConfigVersion and the env-var promotion logic in apply_config_version_defaults are design decisions a maintainer should ratify.
Other factors
All four of my prior inline issues (bare-number string units, idempotency under update -i, bun pm trust clobber, trim/finite validation) are addressed in 4652952. Test coverage is good (59 passing, including flag-on/flag-off, v1/v2 lockfile preservation, and 0-disables). The one remaining CI failure (fetch-tcp-keepalive.test.ts) is unrelated. The CodeRabbit nit about stdout in test guards is cosmetic. This is well-executed but not the kind of mechanical change I'd approve without a human looking at the rollout design.
robobun
force-pushed
the
farm/9555d6db/minimum-release-age-default-and-ms
branch
from
4652952 to
723dca0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/pm/cli/install.mdx`:
- Around line 344-345: Update the documentation to clarify that the default
value for minimumReleaseAge (and the related minimumReleaseAgeExcludes) is
conditional on the project's configVersion: state that the shown default
(minimumReleaseAge = "2d") applies to new projects or when configVersion ≥ 2,
whereas existing projects with older lockfiles retain their previous behavior;
reference the configVersion symbol and the two keys minimumReleaseAge and
minimumReleaseAgeExcludes so readers know the conditional relationship.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c801de20-7a72-4038-8c81-19eff53fd833
📒 Files selected for processing (5)
docs/pm/cli/install.mdxdocs/runtime/bunfig.mdxsrc/bun_core/env_var.rssrc/bun_core/fmt.rssrc/bun_core/lib.rs
robobun
force-pushed
the
farm/9555d6db/minimum-release-age-default-and-ms
branch
from
5c1d13c to
a9a4a97
Compare
robobun
force-pushed
the
farm/9555d6db/minimum-release-age-default-and-ms
branch
from
10c186c to
a87988f
Compare
There was a problem hiding this comment.
No bugs found and all prior review feedback has been addressed, but this introduces the ConfigVersion V2 rollout mechanism plus a gated security-relevant default that will affect every fresh project in 1.4 — worth a human pass on the design (2-day value, MAX_KNOWN clamping, bun pm migrate → CURRENT) before merging.
Extended reasoning...
Overview
This PR makes two changes to minimumReleaseAge: (1) accepts ms-style duration strings via a new bun_core::parse_ms() helper, wired through both the CLI flag parser and the bunfig EString branch; (2) introduces ConfigVersion::V2 with a gated 2-day default minimumReleaseAge for fresh projects, applied via a new apply_config_version_defaults() method that is called from install_with_manager, bun outdated, bun update -i, and bun pm trust. It also adds ConfigVersion::MAX_KNOWN (distinct from CURRENT while gated), the BUN_FEATURE_FLAG_INSTALL_CONFIG_V2 env var, ~320 lines of new tests, doc updates, a bunEnv harness tweak, and an unrelated bake-codegen.ts bootstrap-compat fix. 17 files total.
Security risks
minimumReleaseAge is itself a supply-chain attack mitigation. The PR does not weaken it for existing users (the V2 default is purely additive and gated behind BREAKING_CHANGES_1_4), and the parse-path changes were hardened during review (bare-number-string → seconds consistency, NaN/inf rejection, leading/trailing whitespace trim). I see no injection, auth, or data-exposure surface here. The main residual risk is design-level: the configVersion gating must correctly distinguish fresh vs. existing projects across every lockfile-writing path, and one such path (bun pm trust) was missed in the initial revision and caught during review — a human should confirm there are no others.
Level of scrutiny
High. This is core package-manager resolution logic, introduces a new versioned-defaults rollout mechanism that will govern how breaking install defaults ship in 1.4 and beyond, and adds four unsafe { &mut *pm_ptr } raw-pointer reborrows with disjoint-field SAFETY arguments across separate command entrypoints. The 2-day default value, the decision that bun pm migrate writes CURRENT (vs. preserving the migrated-from version like implicit migration does), and the MAX_KNOWN clamping semantics for future lockfiles are all product/design choices a maintainer should explicitly ratify.
Other factors
The PR went through five rounds of substantive fixes during review (quoted-bare-number ms-vs-seconds, apply_config_version_defaults idempotency for update -i, bun pm trust configVersion clobber, NaN/inf + trailing-space handling, bunEnv ambient-flag leak), all now resolved with tests. Test coverage for the new behaviour is thorough (59 passing). The current bug-hunting run found nothing. This is well-executed but not the kind of mechanical change that should bypass human review.
robobun
force-pushed
the
farm/9555d6db/minimum-release-age-default-and-ms
branch
from
a87988f to
4acb787
Compare
robobun
force-pushed
the
farm/9555d6db/minimum-release-age-default-and-ms
branch
3 times, most recently
from
f6ae813 to
d08f651
Compare
robobun
force-pushed
the
farm/9555d6db/minimum-release-age-default-and-ms
branch
2 times, most recently
from
cfcacde to
4b6ebcc
Compare
robobun
force-pushed
the
farm/9555d6db/minimum-release-age-default-and-ms
branch
from
4b6ebcc to
843a22a
Compare
robobun
changed the title
robobun
force-pushed
the
farm/9555d6db/minimum-release-age-default-and-ms
branch
from
fbd6b70 to
1ebfa5f
Compare
|
@robobun Stack another pr on this to add a |
robobun
force-pushed
the
farm/9555d6db/minimum-release-age-default-and-ms
branch
from
24f52cb to
29c0f0c
Compare
|
@robobun Just checking you didn't miss this (Bun.ms)? |
Both --minimum-release-age and [install].minimumReleaseAge now accept an ms-style duration string (e.g. "2d", "1 week", "48h") in addition to a bare number of seconds. A bare number (quoted or unquoted) is still interpreted as seconds for backwards compatibility; values are trimmed before parsing so "259200 " stays seconds rather than being read as milliseconds. Non-finite / negative values are rejected. 0 disables the filter. - bun_core::parse_ms(): npm ms-style parser (y/w/d/h/m/s/ms), re-exported - CLI flag + bunfig EString branch: try seconds first, then parse_ms - docs + tests for the duration-string forms
…ed manifests `0` disables the age filter, but it was stored as `Some(0.0)`, so the `.is_some()` checks that decide whether to request the extended (time-bearing) registry manifest still fired — every package fetched the full manifest instead of the abbreviated form even though filtering was a no-op. Treat `Some(0.0)` as "filter disabled" everywhere it's consumed: the manifest-fetch gates (`is_some_and(|ms| ms > 0.0)`), the version filter in npm.rs (so it takes the no-gate path and doesn't require `time`), and the exact-version too-recent check (so it agrees with the range/dist-tag paths). This keeps `Some(0.0)` distinct from `None` for callers that need to know the user configured the option, while making `0` behave exactly like unset for fetching/filtering. Added a test that asserts `minimumReleaseAge = 0` requests the abbreviated manifest.
robobun
force-pushed
the
farm/9555d6db/minimum-release-age-default-and-ms
branch
from
29c0f0c to
fa4b4b2
Compare
There was a problem hiding this comment.
No new issues on this pass — all earlier threads are resolved (the two remaining bare .is_some() reads are deferred to #31608 as noted). I'm not auto-approving since this touches package-manager manifest fetching / version resolution and the semantics of a security filter across several files, so it's worth a maintainer's sign-off; also noting alii's open question about the Bun.ms stack.
Extended reasoning...
Overview
This PR teaches bun install's minimumReleaseAge filter to accept npm-ms-style duration strings ("3d", "1 week", etc.) in addition to bare seconds, and makes 0 behave as a true disable (no extended-manifest fetch). It touches 10 files: a new parse_ms() parser in bun_core/fmt.rs, the CLI flag parser, the bunfig EString branch, Some(0.0)-as-disabled guards across PackageManagerEnqueue.rs / PopulateManifestCache.rs / npm.rs, two doc pages, and ~230 lines of new tests. The configVersion-V2 / 2-day-default half was split out to #31608 and the Bun.ms API to #31613.
Security risks
The feature being modified is itself a supply-chain security control. The risks I looked for earlier in the review — a quoted bare number being read as ms (1000× weakening), trailing-whitespace fallthrough to parse_ms, NaN/inf acceptance, and 0 forcing the extended manifest — have all been fixed and are covered by tests. There is no new attack surface (no auth/crypto/network-origin handling); the only network-facing effect is which Accept header is sent for registry manifests, and that change is in the less-information direction when the filter is disabled. I see no injection or data-exposure risk.
Level of scrutiny
Moderate-to-high. The diff is not large per file, but it spans the package manager's manifest-fetching and version-resolution paths, changes the semantics of a documented security knob (0 now fully disables rather than filtering with a zero threshold), and introduces a new hand-rolled parser with several edge cases. The PR has been through multiple bug-fix rounds during review, which is a good sign for the final state but also indicates the change was non-trivial to get right.
Other factors
- Test coverage is solid: 7 new test cases covering CLI/bunfig duration strings (with/without spaces), bare-number-as-seconds, invalid rejection, and a dedicated registry test asserting the abbreviated-manifest
Acceptheader whenminimumReleaseAge = 0. - All prior inline threads (mine and CodeRabbit's) are resolved; the two remaining bare
.is_some()reads inoutdated_command.rs/update_interactive_command.rsare intentionally deferred to the stacked #31608 per the author, and I'd previously flagged them as nit-level / no user-visible impact. - A maintainer (alii) has an open coordination question from May 30 about the
Bun.msfollow-up that hasn't been answered in-thread (though the PR description and the May 14 status comment both reference #31613). - Given the security-adjacent subsystem and the breadth of files touched, this is past my threshold for auto-approval and deserves a human merge decision.
2 participants
What
bun install's minimum-release-age filter now accepts anms-style duration string in addition to a bare number of seconds, for both the--minimum-release-ageCLI flag and the[install].minimumReleaseAgebunfig key:How
bun_core::parse_ms()— an npmms-style parser (y/w/d/h/m/s/ms, case-insensitive, long and short spellings), re-exported frombun_core.EStringbranch: parse as seconds first (parse_f64), then fall back toparse_ms. A bare number (quoted or unquoted) stays seconds for back-compat; inputs are trimmed before parsing so"259200 "isn't read as milliseconds. Non-finite / negative values are rejected.0disables the filter.Stacked follow-up
The gated 2-day-default-for-new-projects change (configVersion V2, behind a 1.4 feature flag) is split out into #31608, stacked on this PR.
Verification
test/cli/install/minimum-release-age.test.ts— 54 pass / 0 fail on debug+ASAN, including the new duration-string cases (CLI + bunfig, with/without spaces, bare-number-is-seconds, invalid-string rejection).