feat(installer): use GitHub API as primary fetch with raw CDN fallback#2248
Merged
feat(installer): use GitHub API as primary fetch with raw CDN fallback#2248
Conversation
🤖 Augment PR SummarySummary: This PR makes the installer fetch marketplace registry files via the GitHub Contents API first, with a fallback to raw GitHub URLs when the API path fails. Changes:
Rationale: Some corporate networks block Testing: Adds new unit tests for cascade logic and parsing; CI now runs them via 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis change introduces GitHub-specific fetching methods to the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Installer Module
participant RC as RegistryClient
participant GitAPI as GitHub API
participant RawCDN as raw.githubusercontent.com
Caller->>RC: fetchGitHubYaml(owner, repo, path, ref)
RC->>RC: fetchGitHubFile(...)
RC->>GitAPI: GET /repos/owner/repo/contents/path (with Accept header)
alt API Success
GitAPI-->>RC: 200 + content
RC->>RC: Parse YAML
RC-->>Caller: Parsed YAML object
else API Failure
GitAPI-->>RC: Error/Non-200
RC->>RawCDN: GET owner/repo/ref/path
alt CDN Success
RawCDN-->>RC: 200 + content
RC->>RC: Parse YAML
RC-->>Caller: Parsed YAML object
else CDN Failure
RawCDN-->>RC: Error
RC-->>Caller: Throw error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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)
tools/installer/modules/external-manager.js (1)
35-35: Consider centralizing marketplace repo coordinates.This hardcoded owner/repo/ref now duplicates
tools/installer/modules/community-manager.jsconstants, which can drift over time. A shared constant module would keep both managers aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/installer/modules/external-manager.js` at line 35, The call in external-manager.js that hardcodes the marketplace repo coordinates (the fetchGitHubYaml invocation with 'bmad-code-org', 'bmad-plugins-marketplace', 'registry/official.yaml', 'main') duplicates constants used in community-manager.js; extract these owner/repo/ref values into a shared module (e.g., export MARKETPLACE_OWNER, MARKETPLACE_REPO, MARKETPLACE_REF or a single MARKETPLACE_COORDS object) and replace the literal strings in external-manager.js (and update community-manager.js to import the same symbols) so both managers consume the centralized constants.test/test-registry-client.js (1)
54-58: Add an assertion for API headers in the stubbed call.The tests validate URL construction, but not the expected
Acceptmedia type passed to_fetchWithHeaders(). Capturing/asserting headers would harden regression detection.Also applies to: 126-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-registry-client.js` around lines 54 - 58, Update the stub for _fetchWithHeaders to accept and record the headers argument (e.g., push a tuple or an object like {url, headers} into calls) and add assertions in the tests that the captured headers include the correct Accept media type; specifically modify the stubbed client._fetchWithHeaders and the tests around those stubs (also the other occurrence mentioned) to assert headers['Accept'] === the expected media type string used by the Registry client under test so the Accept header is validated alongside the URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/installer/modules/registry-client.js`:
- Around line 85-91: When falling back from this._fetchWithHeaders(apiUrl, ...)
to this.fetch(rawUrl, ...), preserve the original API error context so failures
show both causes: catch the error thrown by _fetchWithHeaders (capture it as
apiErr), then if the raw CDN call also fails capture that error (rawErr) and
throw a new Error that includes both apiErr and rawErr messages (or attach them
as properties) so callers can see the API failure plus the CDN failure; update
the try/catch around _fetchWithHeaders and the subsequent this.fetch(rawUrl,
...) to rethrow a combined error when both attempts fail and ensure the thrown
error contains apiUrl and rawUrl and the original error objects for debugging.
- Around line 147-149: The _fetchWithHeaders function currently follows Location
redirects unboundedly; add a redirect cap by introducing a maxRedirects
parameter (or redirectsLeft) with a sensible default (e.g., 5–10) to
_fetchWithHeaders and decrement it on each recursive call when res.statusCode is
3xx and res.headers.location; if the redirect limit is exceeded, reject/throw a
descriptive error (including the URL and status) instead of recursing, and
ensure callers that invoke _fetchWithHeaders propagate that rejection so the
tool exits with a non-zero error code.
---
Nitpick comments:
In `@test/test-registry-client.js`:
- Around line 54-58: Update the stub for _fetchWithHeaders to accept and record
the headers argument (e.g., push a tuple or an object like {url, headers} into
calls) and add assertions in the tests that the captured headers include the
correct Accept media type; specifically modify the stubbed
client._fetchWithHeaders and the tests around those stubs (also the other
occurrence mentioned) to assert headers['Accept'] === the expected media type
string used by the Registry client under test so the Accept header is validated
alongside the URL.
In `@tools/installer/modules/external-manager.js`:
- Line 35: The call in external-manager.js that hardcodes the marketplace repo
coordinates (the fetchGitHubYaml invocation with 'bmad-code-org',
'bmad-plugins-marketplace', 'registry/official.yaml', 'main') duplicates
constants used in community-manager.js; extract these owner/repo/ref values into
a shared module (e.g., export MARKETPLACE_OWNER, MARKETPLACE_REPO,
MARKETPLACE_REF or a single MARKETPLACE_COORDS object) and replace the literal
strings in external-manager.js (and update community-manager.js to import the
same symbols) so both managers consume the centralized constants.
🪄 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: CHILL
Plan: Pro
Run ID: 862fd9ee-7a6f-4aeb-920b-9305f05fbfaf
📒 Files selected for processing (5)
package.jsontest/test-registry-client.jstools/installer/modules/community-manager.jstools/installer/modules/external-manager.jstools/installer/modules/registry-client.js
Collaborator
Author
Triage Summary5 findings from Augment + CodeRabbit — FIX: 3, DISMISS: 2, DEFER: 0
Fixes in commit 745a402. |
Corporate proxies commonly block raw.githubusercontent.com while allowing api.github.com. Add fetchGitHubFile() to RegistryClient that tries the GitHub Contents API first, falling back to the raw CDN transparently. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add maxRedirects parameter to fetch() and _fetchWithHeaders() to prevent unbounded redirect recursion. Wrap CDN fallback in try/catch and throw AggregateError with both API and CDN errors for better diagnostics. Extract marketplace repo coordinates into named constants in external-manager.
Neither method has any callers. Also drop the corresponding test.
alexeyv
force-pushed
the
feat/github-api-fallback
branch
from
745a402 to
104dd9f
Compare
No reason for RegistryClient tests to be a separate runner — the same file already tests the registry consumers in Suite 33. Drop test:registry from package.json scripts and quality gate.
… errors Non-2xx responses previously yielded bare `HTTP 403`. Now surface the request URL, GitHub's JSON error message (or body snippet), X-RateLimit-Reset when quota is exhausted, and Retry-After. Turns a mystery 403 into 'rate limit exhausted; resets at 2026-04-15T18:00:00Z' — the difference between 'try GITHUB_TOKEN' and a wild goose chase.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fetchGitHubFile()toRegistryClientthat tries the GitHub Contents API (api.github.com) first, falling back toraw.githubusercontent.comon failureExternalModuleManagerandCommunityModuleManagerto use structured owner/repo/path params instead of hardcoded raw URLsraw.githubusercontent.comwhile allowingapi.github.com— this makes the installer work on more networksTest plan
npm run qualitypassesbmad installworks withraw.githubusercontent.comblocked at proxy/hosts level🤖 Generated with Claude Code