fix(EC-1778): use go-gather WithTransport options for HTTP transports#3327
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR refactors the downloader to initialize OCI and HTTP gatherers once as singletons and route downloads by URL scheme instead of always querying the registry. It also refactors transport initialization to build a shared base with optional tracing before creating separate retry-enabled transports. Dependencies are bumped to match. ChangesGatherer Singleton Architecture and Transport Routing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Review Summary by QodoMigrate downloader to go-gather v1.2.0 WithTransport API
WalkthroughsDescription• Migrate from removed goci.Transport/ghttp.Transport globals to WithTransport functional options • Update go-gather dependency from v1.1.0 to v1.2.0 • Implement Matcher-based dispatch in gatherFunc for OCI/HTTP sources • Refactor _initialize to construct gatherer instances with transport stack • Update all downloader tests to work with new gatherer variable pattern Diagramflowchart LR
A["go-gather v1.2.0"] -->|"WithTransport options"| B["_initialize"]
B -->|"constructs"| C["ociGatherer"]
B -->|"constructs"| D["httpGatherer"]
E["gatherFunc"] -->|"Matcher dispatch"| C
E -->|"Matcher dispatch"| D
E -->|"fallback"| F["registry"]
G["Transport Stack"] -->|"tracing + retry"| B
File Changes1. internal/downloader/downloader.go
|
Code Review by Qodo
1.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@internal/downloader/downloader.go`:
- Around line 55-59: The current routing checks ociGatherer.Matcher(source)
before httpGatherer.Matcher(source), which can misroute HTTP URLs (e.g.,
127.0.0.1) to the OCI gatherer; modify the gather routing so HTTP(s) URLs are
matched first—either by placing the httpGatherer.Matcher(source) case before
ociGatherer.Matcher(source) in the switch or by adding an explicit scheme check
(strings.HasPrefix(source,"http://") || strings.HasPrefix(source,"https://"))
that routes to httpGatherer.Gather(ctx, source, destination) before falling back
to ociGatherer.Gather(ctx, source, destination); update the switch in the gather
function where ociGatherer.Matcher and httpGatherer.Matcher are referenced to
implement this change.
🪄 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: CHILL
Plan: Enterprise
Run ID: 99df4c82-897b-45b9-b2b8-021d7cf07f26
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
docs/superpowers/plans/2026-05-29-ec1778-downloader-withtransport.mddocs/superpowers/specs/2026-05-28-ec1778-downloader-withtransport-design.mdgo.modinternal/downloader/downloader.gointernal/downloader/downloader_test.go
![qodo-for-conforma[bot]](https://avatars.githubusercontent.com/in/3609154?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve three gaps: explicit sync.OnceFunc wiring, concrete TestOCIClientConfiguration strategy, import alias conventions. reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks: go.mod update, production rewrite, 4 test updates, final verification. TDD-sequenced with exact code and commands. reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace removed goci.Transport/ghttp.Transport global mutations with constructed gatherer instances using WithTransport functional options. gatherFunc now dispatches via Matcher (OCI, HTTP) before falling back to the registry for git/file sources. reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace references to removed goci.Transport/ghttp.Transport globals with gatherer var cleanup. TestOCIClientConfiguration simplified to type assertion (transport is private). TestHTTPClientConfiguration inspects httpGatherer.Client.Transport instead of removed global. TestHTTPTracing calls httpGatherer.Gather directly since 127.0.0.1 now matches the OCI registry pattern in the new dispatch order. reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…names The OCI Matcher matches localhost/127.0.0.1 as known registries, which misroutes git-over-HTTP sources hosted on localhost in acceptance tests. Dispatch to custom gatherers only for explicit oci:// or http(s):// scheme prefixes; bare hostnames fall through to the registry where git matchers are checked before OCI matchers. reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@docs/superpowers/plans/2026-05-29-ec1778-downloader-withtransport.md`:
- Around line 190-208: Update the documented gatherFunc to match the real
implementation in downloader.go: replace the Matcher-based dispatch
(ociGatherer.Matcher / httpGatherer.Matcher) with explicit scheme prefix checks
using strings.HasPrefix for OCI ("oci://" or "oci::") and HTTP ("https://" or
"http://"), and preserve the additional conditional call to httpGatherer.Matcher
inside the HTTP branch before delegating to httpGatherer.Gather; keep the
registry fallback using registry.GetGatherer and its error handling unchanged.
In `@docs/superpowers/specs/2026-05-28-ec1778-downloader-withtransport-design.md`:
- Around line 72-88: The spec's dispatch snippet and "Matcher ordering
rationale" must be updated to match the implemented gatherFunc logic: describe
that dispatch uses explicit scheme-prefix checks (e.g., "oci://" routed to
ociGatherer) plus a conditional httpGatherer.Matcher guard for "http(s)://"
while leaving bare hostnames (like "quay.io/repo:tag") to fall through to
registry.GetGatherer; replace the Matcher-only examples (ociGatherer.Matcher,
httpGatherer.Matcher) with the actual scheme-check logic, explain why this
prevents bare hostnames from being captured by the OCI gatherer, and update any
ordering rationale to reflect this implementation detail.
🪄 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: CHILL
Plan: Enterprise
Run ID: 67b02627-4777-4cbf-9d91-0426a8aba259
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
docs/superpowers/plans/2026-05-29-ec1778-downloader-withtransport.mddocs/superpowers/specs/2026-05-28-ec1778-downloader-withtransport-design.mdgo.modinternal/downloader/downloader.gointernal/downloader/downloader_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- internal/downloader/downloader_test.go
These were working artifacts for the implementation process, not deliverables. reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ReviewFindingsMedium
Low
Info
|
fullsend-ai-review
Bot
added
the
requires-manual-review
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
2 participants
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
goci.Transport/ghttp.Transportglobal mutations withWithTransportfunctional options on constructed gatherer instancesgatherFuncnow dispatches OCI/HTTP sources viaMatcherbefore falling back to the registry for git/file sourcesTest plan
go test -race -tags=unit ./internal/downloader/— all 6 tests passmake test— all unit, integration, and generative tests pass across the CLIresolves: EC-1778
🤖 Generated with Claude Code