fix: Harden catalog skill installation validation and update tests#326
Merged
yacosta738 merged 4 commits intomainfrom Apr 6, 2026
Merged
fix: Harden catalog skill installation validation and update tests#326yacosta738 merged 4 commits intomainfrom
yacosta738 merged 4 commits intomainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds embedded-catalog fields and lookup paths (install_source, archive_subpath, legacy_local_skill_ids), updates provider and install-source resolution to prefer embedded catalog entries, extends suggestion/installed-state logic to consider legacy aliases, introduces E2E catalog workflow, and converts integration test to iterate embedded catalog entries. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SuggestionService
participant EmbeddedCatalog
participant Provider
participant Installer
participant Registry
Client->>SuggestionService: request recommendation/install for skill
SuggestionService->>EmbeddedCatalog: lookup skill definition by provider/local id
alt definition found with install_source
EmbeddedCatalog-->>SuggestionService: return install_source / archive_subpath / legacy IDs
else no catalog install_source
SuggestionService->>Provider: resolve download URL / install source
Provider-->>SuggestionService: return download URL
end
SuggestionService->>Registry: check installed state (canonical + legacy IDs)
alt already installed
SuggestionService-->>Client: skip install (AlreadyInstalled)
else not installed
SuggestionService->>Installer: fetch & install using resolved source
Installer-->>Registry: record installed skill
Installer-->>SuggestionService: success/failure
SuggestionService-->>Client: report result
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/catalog-e2e.yml:
- Around line 49-53: Add a pre-check in the "Run catalog installation E2E" step
to fail fast if the vendored skills repo path in AGENTSYNC_LOCAL_SKILLS_REPO is
missing: before running cargo test, check that the directory referenced by the
environment variable AGENTSYNC_LOCAL_SKILLS_REPO exists (e.g., test -d
"$AGENTSYNC_LOCAL_SKILLS_REPO") and output a clear error and exit non-zero if it
does not; update the step that currently runs cargo test to run the directory
existence check first and only proceed to cargo test --test
test_catalog_integration -- --ignored --nocapture when the check passes so the
workflow fails fast on missing vendor checkout.
In `@README.md`:
- Around line 62-66: The README's "Local run" command omits the prerequisite of
having the canonical skills repo available; update the instruction to require
cloning or pointing to a local agents-skills checkout by setting
AGENTSYNC_LOCAL_SKILLS_REPO (or placing a sibling agents-skills checkout) before
running the tests referenced in tests/test_catalog_integration.rs, e.g. instruct
users to git clone the agents-skills repo or export AGENTSYNC_LOCAL_SKILLS_REPO
to the clone path and then run the existing RUN_E2E=1 cargo test command so the
test_catalog_integration can find the canonical repo.
In `@src/commands/skill.rs`:
- Around line 1082-1092: The function infer_install_source_format currently
assumes any HTTP(S) URL that isn't .tar.gz/.tgz is a "zip", which can be
misleading; update infer_install_source_format to explicitly check for ".zip"
(and treat it as "zip"), then for other HTTP(S) URLs return a clearer sentinel
such as "url" or "unknown" instead of always "zip", so callers can distinguish
explicit zip archives from other remote URLs; keep the existing "dir" return for
non-HTTP sources.
- Around line 928-934: The code constructs SkillInstallInfo with a computed
format from infer_install_source_format in the EmbeddedSkillCatalog branch but
that format value is never used by the install pipeline (see
blocking_fetch_and_install_skill and fetch_and_unpack_to_tempdir), so either
remove the dead computation or document its intent: delete the format field
population in the EmbeddedSkillCatalog path (remove the
infer_install_source_format call) if unused, or add a short comment next to
SkillInstallInfo construction (and/or on the SkillInstallInfo type) explaining
the format is informational/reserved for future use; update references to
EmbeddedSkillCatalog, get_install_source, SkillInstallInfo, and
infer_install_source_format accordingly.
In `@src/skills/catalog.rs`:
- Around line 237-244: The method get_skill_definition_by_local_id currently
does a linear scan over self.skill_definitions.values(), which can be slow at
scale; add a reverse index to map local_skill_id to the full definition or to
the provider_skill_id and then do an O(log N)/O(1) lookup. Concretely: add a new
field (e.g. local_to_provider: BTreeMap<String, String> or local_to_definition:
BTreeMap<String, CatalogSkillDefinition>) to the catalog struct, populate and
maintain it wherever skills are inserted/updated/removed (the same code paths
that touch skill_definitions and local_skills/CatalogSkillMetadata), and change
get_skill_definition_by_local_id to consult this new map (lookup provider id
then return skill_definitions.get(...) or return the stored definition
directly).
In `@src/skills/catalog.v1.toml`:
- Around line 807-812: The provider_skill_id
"nodnarbnitram/claude-code-extensions/tauri-v2" does not match the
install_source repository "https://github.com/delexw/claude-code-misc...";
update the metadata so they align by either changing provider_skill_id to
"delexw/claude-code-misc/tauri-v2" (or the correct owner/path) to match
install_source, or add a clarifying comment/field documenting that the skill was
forked/moved and kept local_skill_id "tauri-v2" and title "Tauri v2"
intentionally point to the delexw repo; ensure provider_skill_id,
install_source, and any publishing metadata consistently reference the same
source.
In `@src/skills/suggest.rs`:
- Around line 331-334: annotate_recommendations currently checks installed
status by directly doing get(&recommendation.skill_id), which ignores
legacy_local_skill_ids; update annotate_recommendations to call
installed_state_for_recommendation(&installed_skill_states, recommendation) (the
same helper used by build_response) when computing the installed state and then
pass that into recommendation.annotate_installed_state(...), so renamed/aliased
skills are correctly detected for callers like recommend_skills ->
annotate_recommendations; ensure you reference the existing
installed_skill_states parameter and the installed_state_for_recommendation
helper rather than direct map lookups.
In `@tests/test_catalog_integration.rs`:
- Around line 45-62: The test helper resolve_install_source duplicates
resolution logic from SuggestInstallProvider::resolve; replace the duplicated
logic by reusing the production resolver or extracting the shared resolution
into a common function: either (A) call
SuggestInstallProvider::default().resolve(provider_skill_id) and return its
download_url (expose SuggestInstallProvider if necessary), or (B) move the
resolution logic that checks EmbeddedSkillCatalog::get_install_source,
DALLAY_SKILLS_PREFIX handling, and provider.resolve into a new shared function
(e.g., resolve_install_source_shared) used by both the test and
src/commands/skill.rs; update imports and signatures (local_skill_source_dir,
EmbeddedSkillCatalog, DALLAY_SKILLS_PREFIX) accordingly.
- Around line 64-76: install_with_retry currently discards the details of the
first failure if the retry succeeds; modify install_with_retry (which calls
blocking_fetch_and_install_skill) to log the first_error before sleeping and
retrying so the initial failure is available for debugging (use the project's
logging facility or eprintln!/warn! as appropriate), then keep the existing
retry behavior and existing combined anyhow::anyhow! on double failure.
🪄 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: 862450bb-487e-4152-803a-8ac56a6f1dd4
📒 Files selected for processing (12)
.github/workflows/catalog-e2e.ymlREADME.mdsrc/commands/skill.rssrc/skills/catalog.rssrc/skills/catalog.v1.tomlsrc/skills/provider.rssrc/skills/suggest.rstests/integration/skill_suggest.rstests/test_catalog_integration.rstests/unit/provider.rstests/unit/suggest_catalog.rstests/unit/suggest_install.rs
Comment on lines
+331
to
+334
| recommendation.annotate_installed_state(installed_state_for_recommendation( | ||
| &installed_skill_states, | ||
| recommendation, | ||
| )); |
Contributor
There was a problem hiding this comment.
Finish applying the alias-aware installed lookup to the public helper.
build_response now honors legacy_local_skill_ids, but annotate_recommendations(...) below still does a direct get(&recommendation.skill_id). Any caller that uses recommend_skills(...) followed by annotate_recommendations(...) will keep reporting renamed skills as not installed, so this feature is only correct through SuggestionService.
🔧 Suggested fix
pub fn annotate_recommendations(
recommendations: &mut [SkillSuggestion],
installed_skill_states: &BTreeMap<String, InstalledSkillState>,
) {
for recommendation in recommendations {
- recommendation
- .annotate_installed_state(installed_skill_states.get(&recommendation.skill_id));
+ recommendation.annotate_installed_state(installed_state_for_recommendation(
+ installed_skill_states,
+ recommendation,
+ ));
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/skills/suggest.rs` around lines 331 - 334, annotate_recommendations
currently checks installed status by directly doing
get(&recommendation.skill_id), which ignores legacy_local_skill_ids; update
annotate_recommendations to call
installed_state_for_recommendation(&installed_skill_states, recommendation) (the
same helper used by build_response) when computing the installed state and then
pass that into recommendation.annotate_installed_state(...), so renamed/aliased
skills are correctly detected for callers like recommend_skills ->
annotate_recommendations; ensure you reference the existing
installed_skill_states parameter and the installed_state_for_recommendation
helper rather than direct map lookups.
|
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.
This pull request introduces a comprehensive set of improvements to the skill catalog system, focusing on enhanced catalog-driven skill resolution, support for legacy skill IDs, and improved skill suggestion accuracy. It also adds a new end-to-end (E2E) GitHub Actions workflow to validate the catalog, and updates documentation accordingly.
Catalog-driven skill resolution and installation:
EmbeddedSkillCatalogis now used as the primary source for resolving skill install sources and archive subpaths, both in the CLI and in theSkillsShProvider, allowing for deterministic and overrideable skill installation. [1] [2] [3]CatalogSkillDefinition,ProviderCatalogSkill, etc.) now support new fields:archive_subpath,legacy_local_skill_ids, andinstall_source, enabling more flexible and accurate skill resolution. [1] [2] [3] [4] [5]Legacy skill ID support and improved suggestion logic:
legacy_local_skill_ids, ensuring that skills installed under previous IDs are recognized and not re-suggested. [1] [2] [3] [4] [5] [6] [7] [8]Catalog E2E validation workflow:
.github/workflows/catalog-e2e.yml) that runs a scheduled and manual E2E test to ensure all catalog skills can be resolved, installed, and registered.README.mdto document the new workflow and provide instructions for running the E2E validation locally or via GitHub Actions. [1] [2]Skill provider improvements:
SkillsShProvidernow consults the embedded catalog for overrides and supports a wider range of skill repository naming conventions. [1] [2]Other improvements:
tar.gz,zip, ordir) based on the URL, improving installation reliability.These changes collectively improve the reliability, maintainability, and user experience of the skill catalog and installation system.