[5512] feat(api): catalog provider adapters (GitHub behind a registry) - #6626
[5512] feat(api): catalog provider adapters (GitHub behind a registry)#6626ardaerzin wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 SummarySummary by CodeRabbit
WalkthroughThe skill import flow now uses registered catalog providers. GitHub fetching moves into a provider adapter. APIs use ChangesProvider catalog import flow
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to Imports from distinct providers can be skipped when their repository and path match. Provenance links can target the wrong item for reserved path characters, and web callers cannot select a provider through the exported source wrappers. These contract issues should be resolved before merge. Sequence Diagram(s)sequenceDiagram
participant Client
participant SkillsRouter
participant SkillImportService
participant ProviderRegistry
participant GitHubProvider
Client->>SkillsRouter: Submit source_url and optional provider
SkillsRouter->>SkillImportService: scan_source or import_from_source
SkillImportService->>ProviderRegistry: resolve source
ProviderRegistry->>GitHubProvider: claims source_url
GitHubProvider-->>ProviderRegistry: SourceLocator
ProviderRegistry-->>SkillImportService: Provider and locator
SkillImportService->>GitHubProvider: fetch_snapshot
GitHubProvider-->>SkillImportService: SourceSnapshot
SkillImportService-->>SkillsRouter: Result with source_url, provider, and resolved_version
SkillsRouter-->>Client: Scan or import response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 35.71% which is insufficient. The required threshold is 60.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 42 functions across 15 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
📘 Docs preview
This comment updates in place on every push. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Team
Run ID: 73b97e24-b339-47f1-9174-2da1909083a3
⛔ Files ignored due to path filters (5)
web/packages/agenta-api-client/src/generated/api/resources/skills/client/Client.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/skills/client/requests/SkillSourceImportRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/skills/client/requests/SkillSourceScanRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/types/ImportResult.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/types/SourceScanResult.tsis excluded by!**/generated/**
📒 Files selected for processing (16)
api/entrypoints/routers.pyapi/oss/src/apis/fastapi/skills/models.pyapi/oss/src/apis/fastapi/skills/router.pyapi/oss/src/core/skills/fetcher.pyapi/oss/src/core/skills/import_service.pyapi/oss/src/core/skills/provenance.pyapi/oss/src/core/skills/providers/__init__.pyapi/oss/src/core/skills/providers/base.pyapi/oss/src/core/skills/providers/github.pyapi/oss/src/core/skills/providers/registry.pyapi/oss/tests/pytest/unit/skills/test_import_service.pyapi/oss/tests/pytest/unit/skills/test_providers.pydocs/design/agent-workflows/projects/skill-registry/plan-meta-provenance.mdweb/packages/agenta-skills-ui/src/registrySections.tsweb/packages/agenta-skills/src/api/index.tsweb/packages/agenta-skills/src/core/schema.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review.
| c.path_in_repo | ||
| for c in scan.candidates | ||
| if (repository, c.path_in_repo) in index | ||
| if (locator.repository, c.path_in_repo) in index |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include provider in every import identity check.
build_origin already stores provider, but _origin_index and both duplicate checks use only (repository, path). If two registered providers return the same pair, the second import can be incorrectly skipped. Key the index and the checks by (provider, repository, path), and add a two-provider regression test.
| ) -> Optional[str]: | ||
| if not resolved_version: | ||
| return None | ||
| return f"https://github.com/{locator.repository}/tree/{resolved_version}/{path}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Encode path before creating the GitHub URL.
A skill path containing #, ?, or % produces a different browser URL. The provenance link can then point to the wrong item. Encode path segments and keep / as a separator. Add a regression test with a reserved character.
Proposed fix
+from urllib.parse import quote
+
- return f"https://github.com/{locator.repository}/tree/{resolved_version}/{path}"
+ encoded_path = quote(path, safe="/")
+ return f"https://github.com/{locator.repository}/tree/{resolved_version}/{encoded_path}"📝 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.
| return f"https://github.com/{locator.repository}/tree/{resolved_version}/{path}" | |
| from urllib.parse import quote | |
| encoded_path = quote(path, safe="/") | |
| return f"https://github.com/{locator.repository}/tree/{resolved_version}/{encoded_path}" |
|
|
||
| const data = await getSkillsClient().scanSkillSource( | ||
| {repo_url: repoUrl, ...(ref ? {ref} : {})}, | ||
| {source_url: repoUrl, ...(ref ? {ref} : {})}, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Forward provider through both public source wrappers.
@agenta/skills exports both wrappers, but their parameter interfaces omit provider and discard it before the generated requests. The backend forwards this field to ProviderRegistry.resolve, which selects the named adapter. Although GitHub is currently the only registered adapter, omitting the field prevents callers from selecting any additional registered provider.
Proposed fix
export interface ScanSkillSourceParams {
projectId: string
repoUrl: string
+ provider?: string
ref?: string
}
export async function scanSkillSource({
projectId,
repoUrl,
+ provider,
ref,
}: ScanSkillSourceParams) {
const data = await getSkillsClient().scanSkillSource(
- {source_url: repoUrl, ...(ref ? {ref} : {})},
+ {source_url: repoUrl, ...(provider ? {provider} : {}), ...(ref ? {ref} : {})},
{queryParams: {project_id: projectId}},
)
}
export interface ImportSkillSourceParams {
projectId: string
repoUrl: string
+ provider?: string
ref?: string
paths?: string[]
}
export async function importSkillSource({
projectId,
repoUrl,
+ provider,
ref,
paths,
}: ImportSkillSourceParams) {
const data = await getSkillsClient().importSkillSource(
{
source_url: repoUrl,
+ ...(provider ? {provider} : {}),
...(ref ? {ref} : {}),
...(paths ? {paths} : {}),
},
{queryParams: {project_id: projectId}},
)
}📝 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.
| {source_url: repoUrl, ...(ref ? {ref} : {})}, | |
| {source_url: repoUrl, ...(provider ? {provider} : {}), ...(ref ? {ref} : {})}, |
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Approved. The remaining V1 implementation choices are accepted.
f5b9303 to
fa3f2c2
Compare
d2737d0 to
5be02a0
Compare
fa3f2c2 to
c2cfa9c
Compare
5be02a0 to
0e183eb
Compare
c2cfa9c to
ba17442
Compare
Implements the review's provider direction: a CatalogProvider contract (claims / fetch_snapshot / item_url), neutral locator and snapshot DTOs, and a ProviderRegistry wired in the entrypoints with GitHub as the only registered adapter. Nothing outside providers/ mentions repos, refs, or tarballs: the import service resolves adapters by URL claim (or an explicit provider field), update flows look the adapter up from the stored origin's provider, and provenance links are adapter-supplied. The wire renames repo_url to source_url; the frontend stops building provider URLs and uses the stored provenance link. Unit tests register a filesystem provider, so every import test also exercises the registry.
…link Two catalogs can name the same repository string, so the already-imported index keys on provider + repository + path rather than assuming one catalog. The GitHub adapter also encodes the item path, keeping links valid for paths holding spaces or '#', and both public source wrappers forward the optional provider so a caller can narrow resolution to one adapter.
0e183eb to
4411989
Compare
ba17442 to
9d067fa
Compare
2 participants
Context
The last piece of the backend review's requested direction (#4): import was GitHub-specific end to end — URL parsing, the tarball API, and provenance link shapes leaked through the import service, routes, and frontend. Adding any second catalog kind meant touching all of them.
Changes
New
core/skills/providers/package:CatalogProvidercontract:claims(source_url) -> Locator | None,fetch_snapshot(locator) -> Snapshot,item_url(...).SourceLocator/SourceSnapshotDTOs; the storedorigin.locatorkeeps the sharedrepository/ref/pathkeys (matching the review's canonical schema).ProviderRegistry(resolveby URL claim with an optional explicitprovider;getby stored provider name), wired inentrypoints/with GitHub as the only registered adapter.Everything GitHub-specific moved into
providers/github.py; the shared archive rails (streamed size cap, decompression-bomb ceilings, traversal rejection) stay infetcher.pyfor any archive-shipping provider. Update check/apply look the adapter up from the stored origin'sprovider— the field now does real work. Wire:repo_url→source_urlplus optionalprovideron scan/import (nothing shipped, so no aliasing). The frontend no longer builds provider URLs — it renders the adapter-supplied provenance link.Before:
After:
Adding GitLab or a marketplace later = one adapter file + one registry entry.
Tests
🤖 Generated with Claude Code