fix: match installed skills by source repo, not just name#205
Open
mwmdev wants to merge 2 commits intospacedriveapp:mainfrom
Open
fix: match installed skills by source repo, not just name#205mwmdev wants to merge 2 commits intospacedriveapp:mainfrom
mwmdev wants to merge 2 commits intospacedriveapp:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds optional source_repo to skill data across backend, API, installer, and frontend; installer injects source_repo into SKILL.md during GitHub installs; frontend uses composite keys (source_repo/name) to determine installed status, preventing name-collision false positives. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
|
|
||
| // Extract and install | ||
| let installed = extract_and_install(&zip_path, target_dir, skill_path.as_deref()).await?; | ||
| let source_repo = format!("{owner}/{repo}"); |
There was a problem hiding this comment.
Since source_repo ends up embedded into YAML frontmatter, it might be worth ensuring it can’t contain newlines (e.g. if spec is weird) so we don’t accidentally corrupt SKILL.md.
Suggested change
| let source_repo = format!("{owner}/{repo}"); | |
| let mut source_repo = format!("{owner}/{repo}"); | |
| source_repo.retain(|ch| ch != '\n' && ch != '\r'); |
| } | ||
|
|
||
| let after_opening = &trimmed[3..]; | ||
| if let Some(end_pos) = after_opening.find("\n---") { |
There was a problem hiding this comment.
find("\n---") can match a literal --- line inside a multiline YAML value. Matching \n---\n first (and falling back) makes the split a bit safer.
Suggested change
| if let Some(end_pos) = after_opening.find("\n---") { | |
| let after_opening = &trimmed[3..]; | |
| let Some((end_pos, delimiter_len)) = after_opening | |
| .find("\n---\n") | |
| .map(|pos| (pos, 5)) | |
| .or_else(|| after_opening.find("\n---").map(|pos| (pos, 4))) | |
| else { | |
| // Malformed frontmatter, prepend | |
| return format!("---\n{line}\n---\n{content}"); | |
| }; | |
| let fm_block = &after_opening[..end_pos]; | |
| let body = &after_opening[end_pos + delimiter_len..]; |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/skills/installer.rs (1)
36-41:⚠️ Potential issue | 🟠 MajorNo timeout on the GitHub archive download.
reqwest::Client::new()uses no timeout; a stalled or slow response from GitHub will hang the install task indefinitely. The registry proxy insrc/api/skills.rsalready uses.timeout(Duration::from_secs(10))as a precedent.🔧 Proposed fix
- let client = reqwest::Client::new(); - let response = client - .get(&download_url) - .send() + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(60)) + .build() + .context("failed to build HTTP client")?; + let response = client + .get(&download_url) + .send()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/skills/installer.rs` around lines 36 - 41, The download currently uses reqwest::Client::new() with no timeout which can hang; replace the client creation with a timed client (use reqwest::Client::builder().timeout(Duration::from_secs(10)).build()) and use that client for the existing client.get(&download_url).send().await call, adding a std::time::Duration import if missing so the GitHub archive download in installer.rs times out like the registry proxy does.
🧹 Nitpick comments (2)
interface/src/routes/AgentSkills.tsx (2)
234-240: Core composite-key logic is correct; document the known limitation of the name-only fallback.The key shapes match correctly:
- New installs:
installedKeysentry ="${source_repo}/${name}"(e.g."anthropics/skills/weather");compositeKey="${skill.source}/${skill.name}"→ same shape → correct match.- Different source, same name: keys differ → no false positive → fixes
#190✓.- Legacy installs (no
source_repo):installedKeysentry ="weather"→ the fallbackinstalledKeys.has(skill.name.toLowerCase())marks all registry skills with that name as installed, which is the original bug behaviour.This is intentional per the PR description, but it is worth a brief inline comment so future contributors understand why the fallback exists and the trade-off it carries:
📝 Suggested documentation comment
+ // installedKeys uses source_repo/name when available (new installs) for + // per-repo precision. Skills installed before this change have no source_repo + // and fall back to name-only matching, which re-exhibits the original false- + // positive behaviour until those skills are re-installed. const installedKeys = new Set( installedSkills.map((s) => s.source_repo ? `${s.source_repo}/${s.name}`.toLowerCase() : s.name.toLowerCase(), ), );Also applies to: 371-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentSkills.tsx` around lines 234 - 240, Add a brief inline comment next to the installedKeys construction and the fallback membership check explaining that legacy installs (installedSkills entries without source_repo) are stored as name-only (e.g., "weather"), and the fallback installedKeys.has(skill.name.toLowerCase()) will therefore mark any registry skill with the same name as installed; state this is an intentional trade-off to preserve legacy behavior and reference the compositeKey (`${skill.source}/${skill.name}`) versus name-only key formats so future contributors understand the limitation and why the fallback exists.
506-517:key={skill.name}onInstalledSkillis safe today but fragile.
SkillSetalready enforces one-skill-per-lowercase-name, so duplicate keys cannot occur now. However, if that invariant ever relaxes (e.g. a future multi-source install mode), React will silently render only the last matching child.Consider
key={skill.source_repo ? \${skill.source_repo}/${skill.name}` : skill.name}` to make the key stable and unique under any future extension.✏️ Proposed change
- key={skill.name} + key={skill.source_repo ? `${skill.source_repo}/${skill.name}` : skill.name}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentSkills.tsx` around lines 506 - 517, The current InstalledSkill list uses key={skill.name} which relies on a fragile uniqueness invariant; change the key generation in the installedSkills.map to a stable unique composite (e.g., include skill.source_repo when present) so keys remain unique if multiple skills share a name. Update the InstalledSkill mapping to compute the key as something like `${skill.source_repo}/${skill.name}` when skill.source_repo exists, otherwise fallback to skill.name, ensuring InstalledSkill receives that composite key and preventing React child key collisions in future multi-source installs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/skills/installer.rs`:
- Around line 184-198: The read of SKILL.md currently swallows errors with `if
let Ok(content) = fs::read_to_string(&skill_md).await`, so add error
handling/logging for the read path: replace the `if let Ok(...)` with a match or
`if let Err(e)` branch so that when `fs::read_to_string(&skill_md).await` fails
you emit a `tracing::warn!` (including `skill = %skill_name`, `%e`, and a clear
message like "failed to read SKILL.md") before returning/continuing; keep the
existing logic that calls `inject_source_repo(&content, repo)` and logs write
failures for `fs::write(&skill_md, patched).await`.
---
Outside diff comments:
In `@src/skills/installer.rs`:
- Around line 36-41: The download currently uses reqwest::Client::new() with no
timeout which can hang; replace the client creation with a timed client (use
reqwest::Client::builder().timeout(Duration::from_secs(10)).build()) and use
that client for the existing client.get(&download_url).send().await call, adding
a std::time::Duration import if missing so the GitHub archive download in
installer.rs times out like the registry proxy does.
---
Nitpick comments:
In `@interface/src/routes/AgentSkills.tsx`:
- Around line 234-240: Add a brief inline comment next to the installedKeys
construction and the fallback membership check explaining that legacy installs
(installedSkills entries without source_repo) are stored as name-only (e.g.,
"weather"), and the fallback installedKeys.has(skill.name.toLowerCase()) will
therefore mark any registry skill with the same name as installed; state this is
an intentional trade-off to preserve legacy behavior and reference the
compositeKey (`${skill.source}/${skill.name}`) versus name-only key formats so
future contributors understand the limitation and why the fallback exists.
- Around line 506-517: The current InstalledSkill list uses key={skill.name}
which relies on a fragile uniqueness invariant; change the key generation in the
installedSkills.map to a stable unique composite (e.g., include
skill.source_repo when present) so keys remain unique if multiple skills share a
name. Update the InstalledSkill mapping to compute the key as something like
`${skill.source_repo}/${skill.name}` when skill.source_repo exists, otherwise
fallback to skill.name, ensuring InstalledSkill receives that composite key and
preventing React child key collisions in future multi-source installs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
interface/src/api/client.tsinterface/src/routes/AgentSkills.tsxsrc/api/skills.rssrc/skills.rssrc/skills/installer.rs
Comment on lines
+184
to
+198
| if let Some(repo) = source_repo { | ||
| let skill_md = target_skill_dir.join("SKILL.md"); | ||
| if skill_md.exists() { | ||
| if let Ok(content) = fs::read_to_string(&skill_md).await { | ||
| let patched = inject_source_repo(&content, repo); | ||
| if let Err(error) = fs::write(&skill_md, patched).await { | ||
| tracing::warn!( | ||
| skill = %skill_name, | ||
| %error, | ||
| "failed to write source_repo to SKILL.md" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent discard of read_to_string error violates the no-silent-error guideline.
The if let Ok(content) = ... silently swallows any IO error when reading SKILL.md, while the subsequent write error is properly logged. Both paths should emit a warning.
🛡️ Proposed fix
- if let Ok(content) = fs::read_to_string(&skill_md).await {
- let patched = inject_source_repo(&content, repo);
- if let Err(error) = fs::write(&skill_md, patched).await {
- tracing::warn!(
- skill = %skill_name,
- %error,
- "failed to write source_repo to SKILL.md"
- );
- }
- }
+ match fs::read_to_string(&skill_md).await {
+ Ok(content) => {
+ let patched = inject_source_repo(&content, repo);
+ if let Err(error) = fs::write(&skill_md, patched).await {
+ tracing::warn!(
+ skill = %skill_name,
+ %error,
+ "failed to write source_repo to SKILL.md"
+ );
+ }
+ }
+ Err(error) => {
+ tracing::warn!(
+ skill = %skill_name,
+ %error,
+ "failed to read SKILL.md for source_repo injection"
+ );
+ }
+ }As per coding guidelines: "Don't silently discard errors; no let _ on Results. Handle, log, or propagate errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/skills/installer.rs` around lines 184 - 198, The read of SKILL.md
currently swallows errors with `if let Ok(content) =
fs::read_to_string(&skill_md).await`, so add error handling/logging for the read
path: replace the `if let Ok(...)` with a match or `if let Err(e)` branch so
that when `fs::read_to_string(&skill_md).await` fails you emit a
`tracing::warn!` (including `skill = %skill_name`, `%e`, and a clear message
like "failed to read SKILL.md") before returning/continuing; keep the existing
logic that calls `inject_source_repo(&content, repo)` and logs write failures
for `fs::write(&skill_md, patched).await`.
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
source_repo(e.g.anthropics/skills) in SKILL.md frontmatter during GitHub installsource_repothrough the API and usessource_repo/nameas a composite key for installed-status checks in the frontendTest plan
cargo test --lib skills— all 14 tests pass (including 5 new tests forinject_source_repo)source_repoin frontmatterNote
Changes overview: Adds
source_repotracking throughout the skill system. Backend now captures GitHub org/repo during installation viainject_source_repo()helper that safely modifies SKILL.md frontmatter (handles missing/malformed frontmatter gracefully). Frontend UI updated to build composite keys (source_repo/name) for installed-skill lookups, with fallback to name-only matching for backward compatibility. API surfaces the new optionalsource_repofield. All types (Skill, SkillInfo, SkillSet) updated to carry this metadata.Written by Tembo for commit 826f255. This will update automatically on new commits.