feat: Check for existing bun before downloading #138
Conversation
WalkthroughAdds a pre-check that detects an existing Bun at Changes
Pre-merge checks✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (2)
src/action.ts (2)
94-97: Avoid logging undefined revision on “corrupted” cache.Revision is undefined here; message is confusing. Log path/key instead.
Apply this diff:
- warning( - `Found a cached version of Bun: ${revision} (but it appears to be corrupted?)`, - ); + warning("Cache restored but Bun version could not be read; cache may be corrupted.");
110-112: Fix inaccurate error message.This can trigger even when reusing existing or cached binaries.
Apply this diff:
- "Downloaded a new version of Bun, but failed to check its version? Try again.", + "Failed to determine Bun revision after setup. Try again.",
📜 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 ignored due to path filters (1)
dist/setup/index.jsis excluded by!**/dist/**
📒 Files selected for processing (1)
src/action.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/action.ts (1)
src/utils.ts (1)
retry(6-19)
🪛 Biome (2.1.2)
src/action.ts
[error] 145-145: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
🔇 Additional comments (1)
src/action.ts (1)
101-105: Fallback download path with retry looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/action.ts (2)
135-146: Syntax error and missing robustness checks.The issues flagged in the previous review are still present:
- Syntax error on line 145: Extra
)causing parse failure- Missing non-pinned value check: Should return
falseforlatest,canary,action(these mean "fetch latest", not "match existing")- Missing version normalization: Should strip leading
vfrom both sides before comparisonApply the diff from the previous review:
-function isVersionMatch(existingRevision: string, requestedVersion?: string): boolean { - // If no version specified, default is "latest" - don't match existing - if (!requestedVersion) { - return false; - } - - // Extract version from revision (format: "1.2.3+hash" or "1.2.3") - const [existingVersion] = existingRevision.split("+"); - - // We only return true if version matches exactly, 'latest' will never match - return existingVersion === requestedVersion) -} +function isVersionMatch( + existingRevision: string, + requestedVersion?: string, +): boolean { + if (!requestedVersion) return false; + // Ignore non-pinned values + if (/^(latest|canary|action)$/i.test(requestedVersion)) return false; + // Normalize 'v' prefix and compare exact semver + const [existingVersion] = existingRevision.trim().split("+"); + const normalizedReq = requestedVersion.trim().replace(/^v/i, ""); + return existingVersion.replace(/^v/i, "") === normalizedReq; +}
78-83: Critical: Skip existing installation check when customUrl is provided.When
options.customUrlis set, the user intends to use a specific custom build. Reusing an existing local binary (even if the version matches) could result in using the wrong build variant, hash, or configuration.Apply this diff to skip reuse when customUrl is provided:
// Check if Bun executable already exists and matches requested version const existingRevision = await getRevision(bunPath); - if (existingRevision && isVersionMatch(existingRevision, options.version)) { + if (!options.customUrl && existingRevision && isVersionMatch(existingRevision, options.version)) { revision = existingRevision; cacheHit = true; // Treat as cache hit to avoid unnecessary network requests info(`Using existing Bun installation: ${revision}`);Note: The try-catch in
getRevision(lines 219-238) now handles ENOENT gracefully by returningundefined, so the explicitexistsSyncguard suggested in previous reviews is no longer strictly necessary. However, addingexistsSyncwould make the intent clearer and avoid an unnecessary async call when the file doesn't exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 ignored due to path filters (1)
dist/setup/index.jsis excluded by!**/dist/**
📒 Files selected for processing (1)
src/action.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/action.ts (1)
src/utils.ts (1)
retry(6-19)
🔇 Additional comments (1)
src/action.ts (1)
219-238: LGTM: Robust error handling.The try-catch wrapper correctly handles cases where the executable doesn't exist or fails to run, returning
undefinedinstead of throwing. This graceful degradation is essential for the early-exit optimization in lines 78-83.
src/action.ts
Outdated
| revision = existingRevision; | ||
| cacheHit = true; // Treat as cache hit to avoid unnecessary network requests | ||
| info(`Using existing Bun installation: ${revision}`); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider clarifying the semantic difference between cache hit and existing installation.
Setting cacheHit = true when reusing an existing installation (line 81) conflates two distinct concepts:
- Cache hit: Restored from
actions/cache - Existing installation: Found locally without cache lookup
While both avoid downloads (same practical effect), downstream consumers of cacheHit might assume it indicates actions/cache was used. Consider introducing a separate flag like existingInstallation or adjusting the output structure to distinguish these cases.
🤖 Prompt for AI Agents
In src/action.ts around lines 80-82, setting cacheHit = true when reusing an
existing local installation conflates “restored from actions/cache” with “found
locally”; introduce a new boolean (e.g., existingInstallation) or update the
output structure to distinguish the two, set cacheHit only when restored from
cache and set existingInstallation = true when the install was reused from the
local system, and update any downstream references/logging/outputs that read
cacheHit to also consider the new flag.
Check for existing bun with same version number, if it exists we skip any futher actions. This avoids downloading cache or bun over the network.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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/action.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/action.ts (1)
src/utils.ts (1)
retry(6-19)
🔇 Additional comments (2)
src/action.ts (2)
10-10: LGTM!The
existsSyncimport is correctly added and properly used to guard thegetRevisioncall.
88-111: LGTM!The restructured logic correctly prioritizes existing installations over cache restoration and downloads. The flow is clear and maintains backward compatibility.
| // Check if Bun executable already exists and matches requested version | ||
| if (!options.customUrl && existsSync(bunPath)) { | ||
| const existingRevision = await getRevision(bunPath); | ||
| if (existingRevision && isVersionMatch(existingRevision, options.version)) { | ||
| revision = existingRevision; | ||
| cacheHit = true; // Treat as cache hit to avoid unnecessary network requests | ||
| info(`Using existing Bun installation: ${revision}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling to gracefully fall back when getRevision fails.
If getRevision throws an error (e.g., file exists but is corrupted or not executable), the action will fail instead of falling back to download or cache restoration. This breaks the robustness of the setup action.
Apply this diff to add error handling:
// Check if Bun executable already exists and matches requested version
if (!options.customUrl && existsSync(bunPath)) {
- const existingRevision = await getRevision(bunPath);
- if (existingRevision && isVersionMatch(existingRevision, options.version)) {
- revision = existingRevision;
- cacheHit = true; // Treat as cache hit to avoid unnecessary network requests
- info(`Using existing Bun installation: ${revision}`);
+ try {
+ const existingRevision = await getRevision(bunPath);
+ if (existingRevision && isVersionMatch(existingRevision, options.version)) {
+ revision = existingRevision;
+ cacheHit = true; // Treat as cache hit to avoid unnecessary network requests
+ info(`Using existing Bun installation: ${revision}`);
+ }
+ } catch (error) {
+ // Existing installation is invalid, fall back to cache/download
+ info(`Existing Bun installation is invalid: ${error.message}`);
}
}Note: The semantic concern about conflating cacheHit with existing installation (flagged in previous reviews) remains unaddressed but is a known nitpick.
📝 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.
| // Check if Bun executable already exists and matches requested version | |
| if (!options.customUrl && existsSync(bunPath)) { | |
| const existingRevision = await getRevision(bunPath); | |
| if (existingRevision && isVersionMatch(existingRevision, options.version)) { | |
| revision = existingRevision; | |
| cacheHit = true; // Treat as cache hit to avoid unnecessary network requests | |
| info(`Using existing Bun installation: ${revision}`); | |
| } | |
| } | |
| // Check if Bun executable already exists and matches requested version | |
| if (!options.customUrl && existsSync(bunPath)) { | |
| try { | |
| const existingRevision = await getRevision(bunPath); | |
| if (existingRevision && isVersionMatch(existingRevision, options.version)) { | |
| revision = existingRevision; | |
| cacheHit = true; // Treat as cache hit to avoid unnecessary network requests | |
| info(`Using existing Bun installation: ${revision}`); | |
| } | |
| } catch (error) { | |
| // Existing installation is invalid, fall back to cache/download | |
| info(`Existing Bun installation is invalid: ${error.message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/action.ts around lines 78 to 86, calling getRevision(bunPath) can throw
(e.g., corrupted or non-executable file) and currently will crash the action;
wrap the getRevision call in a try/catch, and on error log a warning including
the error and the path, ensure you do not set revision or cacheHit in that
failure case so the code falls through to the normal download/cache-restore
path, and proceed without rethrowing the error.
1 participant
Skip cache or bun download if bun with the required version already exists locally. This cuts 5-15 seconds for each install.
Fixes #137
Other tool installers also do this, and without it, bun-setup takes more time than eg. node-setup in our use case. With this fix it was consistently faster.