✨ Add fallback for loading version manifest from remote URL#191
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughAdds explicit remote manifest fetching (loadManifestFromRemote with 30s timeout) and a forceRemote option to loadManifest/getManifestEntries; getManifestEntries now retries with remote-only loading when local manifest yields no matches. Minor test comment wording changes for timeouts. ChangesManifest Loading and Fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/utils/download.ts`:
- Line 87: Replace the hardcoded 30000ms AbortSignal.timeout with a configurable
timeout value: read an integer from an env var (e.g.
process.env.DOWNLOAD_TIMEOUT_MS) or a function parameter, parse it to a number
with a safe fallback to 30000, validate it (>=0), and pass that value into
AbortSignal.timeout instead of the literal; update any callers or export an
option if the download function (refer to the AbortSignal.timeout usage in this
file) is invoked elsewhere so the timeout can be overridden in tests/CI.
- Around line 79-83: Document and validate the environment-derived repo/ref
before building manifestUrl: add a short comment near actionRepo/actionRef
explaining these come from GitHub Actions runtime (GITHUB_ACTION_REPOSITORY and
GITHUB_ACTION_REF) and that defaults are used when running locally or outside
the marketplace; then validate/sanitize actionRepo (must match owner/repo
pattern like /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/) and actionRef (allow safe ref
chars like /^[A-Za-z0-9_.\-\/]+$/), log a warning and fall back to the defaults
if validation fails, and ensure manifestUrl is only constructed from the
validated/sanitized values (referencing actionRepo, actionRef, manifestUrl).
- Around line 55-63: When entries.length === 0 and !forceRemote (the fallback
branch that calls getManifestEntries(version, platform, architecture, debug,
true)), add a debug log using core.debug before calling the remote retry so it's
visible why we’re falling back; import core at the top of the file and log a
message referencing version, platform, architecture and that local manifest had
no matching entries and we are retrying remotely to aid debugging.
- Line 94: The code currently returns (await response.json()) as ManifestEntry[]
without runtime validation; change this to parse the JSON into a variable,
verify it's an array (Array.isArray) and that each element contains the required
ManifestEntry properties (check presence and types of the fields declared on the
ManifestEntry type), and throw a descriptive error if validation fails; update
the return to the validated array (or map/normalize items) so callers receive a
well-formed ManifestEntry[] instead of blindly casting the response.json()
result.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 622fa451-e80e-40d8-932f-9476d786c2e7
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (1)
src/utils/download.ts
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #191 +/- ##
==========================================
- Coverage 88.97% 88.65% -0.32%
==========================================
Files 7 7
Lines 381 388 +7
Branches 90 94 +4
==========================================
+ Hits 339 344 +5
- Misses 42 44 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@burgholzer, do you have any input on the open CodeRabbit comments? I feel like they are all a stretch. 🤔 |
All of them sound overly defensive. I'd simply ignore them. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
This PR adds a fallback for loading the version manifest from the remote URL. This allows us to use new versions of MLIR without necessarily releasing a new version of the action.
Furthermore, this PR updates the release of
v1.4.0.