Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions interface/src/api/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,7 @@ export interface SkillInfo {
file_path: string;
base_dir: string;
source: "instance" | "workspace";
source_repo?: string;
}

export interface SkillsListResponse {
Expand Down
15 changes: 11 additions & 4 deletions interface/src/routes/AgentSkills.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,13 @@ export function AgentSkills({ agentId }: AgentSkillsProps) {
});

const installedSkills = skillsData?.skills ?? [];
const installedNames = new Set(installedSkills.map((s) => s.name.toLowerCase()));
const installedKeys = new Set(
installedSkills.map((s) =>
s.source_repo
? `${s.source_repo}/${s.name}`.toLowerCase()
: s.name.toLowerCase(),
),
);

// Flatten browse pages or use search results
const registrySkills: RegistrySkill[] = debouncedSearch
Expand Down Expand Up @@ -362,9 +368,10 @@ export function AgentSkills({ agentId }: AgentSkillsProps) {
<div className="grid grid-cols-1 gap-3 sm:grid-cols-2 lg:grid-cols-3 xl:grid-cols-4">
{registrySkills.map((skill) => {
const spec = installSpec(skill);
const isInstalled = installedNames.has(
skill.name.toLowerCase(),
);
const compositeKey = `${skill.source}/${skill.name}`.toLowerCase();
const isInstalled =
installedKeys.has(compositeKey) ||
installedKeys.has(skill.name.toLowerCase());
return (
<RegistrySkillCard
key={`${skill.source}/${skill.skillId}`}
Expand Down
3 changes: 3 additions & 0 deletions src/api/skills.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ pub(super) struct SkillInfo {
file_path: String,
base_dir: String,
source: String,
#[serde(skip_serializing_if = "Option::is_none")]
source_repo: Option<String>,
}

#[derive(Serialize)]
Expand Down Expand Up @@ -126,6 +128,7 @@ pub(super) async fn list_skills(
crate::skills::SkillSource::Instance => "instance".to_string(),
crate::skills::SkillSource::Workspace => "workspace".to_string(),
},
source_repo: s.source_repo,
})
.collect();

Expand Down
8 changes: 8 additions & 0 deletions src/skills.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ pub struct Skill {
pub content: String,
/// Where this skill was loaded from.
pub source: SkillSource,
/// GitHub `owner/repo` that this skill was installed from, if any.
pub source_repo: Option<String>,
}

/// Where a skill was loaded from, used for precedence tracking.
Expand Down Expand Up @@ -213,6 +215,7 @@ impl SkillSet {
file_path: s.file_path.clone(),
base_dir: s.base_dir.clone(),
source: s.source.clone(),
source_repo: s.source_repo.clone(),
})
.collect()
}
Expand All @@ -226,6 +229,7 @@ pub struct SkillInfo {
pub file_path: PathBuf,
pub base_dir: PathBuf,
pub source: SkillSource,
pub source_repo: Option<String>,
}

/// Load all skills from a directory.
Expand Down Expand Up @@ -293,6 +297,7 @@ async fn load_skill(
});

let description = frontmatter.get("description").cloned().unwrap_or_default();
let source_repo = frontmatter.get("source_repo").cloned();

// Resolve {baseDir} template variable in the body
let base_dir_str = base_dir.to_string_lossy();
Expand All @@ -305,6 +310,7 @@ async fn load_skill(
base_dir: base_dir.to_path_buf(),
content,
source,
source_repo,
})
}

Expand Down Expand Up @@ -465,6 +471,7 @@ mod tests {
base_dir: PathBuf::from("/skills/weather"),
content: "# Weather\n\nUse curl.".into(),
source: SkillSource::Instance,
source_repo: None,
},
);

Expand All @@ -487,6 +494,7 @@ mod tests {
base_dir: PathBuf::from("/skills/weather"),
content: "# Weather\n\nUse curl.".into(),
source: SkillSource::Instance,
source_repo: None,
},
);

Expand Down
111 changes: 109 additions & 2 deletions src/skills/installer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@ pub async fn install_from_github(spec: &str, target_dir: &Path) -> Result<Vec<St
drop(file);

// Extract and install
let installed = extract_and_install(&zip_path, target_dir, skill_path.as_deref()).await?;
let source_repo = format!("{owner}/{repo}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 installed =
extract_and_install(&zip_path, target_dir, skill_path.as_deref(), Some(&source_repo))
.await?;

tracing::info!(
installed = ?installed,
Expand All @@ -84,7 +87,7 @@ pub async fn install_from_file(skill_file: &Path, target_dir: &Path) -> Result<V
"installing skill from file"
);

let installed = extract_and_install(skill_file, target_dir, None).await?;
let installed = extract_and_install(skill_file, target_dir, None, None).await?;

tracing::info!(
installed = ?installed,
Expand All @@ -102,6 +105,7 @@ async fn extract_and_install(
zip_path: &Path,
target_dir: &Path,
skill_path: Option<&str>,
source_repo: Option<&str>,
) -> Result<Vec<String>> {
let file = std::fs::File::open(zip_path).context("failed to open zip file")?;

Expand Down Expand Up @@ -176,6 +180,23 @@ async fn extract_and_install(
// Copy skill directory
copy_dir_recursive(&skill_dir, &target_skill_dir).await?;

// Write source_repo into SKILL.md frontmatter so we can track provenance
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"
);
}
}
}
}
Comment on lines +184 to +198
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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`.


installed.push(skill_name.to_string());

tracing::debug!(
Expand All @@ -188,6 +209,41 @@ async fn extract_and_install(
Ok(installed)
}

/// Inject or update `source_repo` in SKILL.md frontmatter.
fn inject_source_repo(content: &str, repo: &str) -> String {
let trimmed = content.trim_start();
let line = format!("source_repo: {repo}");

if !trimmed.starts_with("---") {
// No frontmatter — add one
return format!("---\n{line}\n---\n{content}");
}

let after_opening = &trimmed[3..];
if let Some(end_pos) = after_opening.find("\n---") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..];

let fm_block = &after_opening[..end_pos];
let body = &after_opening[end_pos + 4..];

// Remove any existing source_repo line
let filtered: Vec<&str> = fm_block
.lines()
.filter(|l| {
!l.trim_start()
.starts_with("source_repo:")
})
.collect();

let mut new_fm = filtered.join("\n");
new_fm.push('\n');
new_fm.push_str(&line);

format!("---{new_fm}\n---{body}")
} else {
// Malformed frontmatter, prepend
format!("---\n{line}\n---\n{content}")
}
}

/// Parse a GitHub spec: `owner/repo` or `owner/repo/skill-name`
fn parse_github_spec(spec: &str) -> Result<(String, String, Option<String>)> {
let parts: Vec<&str> = spec.split('/').collect();
Expand Down Expand Up @@ -308,4 +364,55 @@ mod tests {
assert!(parse_github_spec("invalid").is_err());
assert!(parse_github_spec("too/many/slashes/here").is_err());
}

#[test]
fn test_inject_source_repo_into_existing_frontmatter() {
let content = "---\nname: weather\ndescription: Get weather\n---\n\n# Weather\n";
let result = inject_source_repo(content, "anthropics/skills");
assert!(result.contains("source_repo: anthropics/skills"));
assert!(result.contains("name: weather"));
assert!(result.contains("# Weather"));
// source_repo should be inside the frontmatter delimiters
let after_first = result.splitn(2, "---").nth(1).unwrap();
let fm = after_first.splitn(2, "\n---").next().unwrap();
assert!(fm.contains("source_repo: anthropics/skills"));
}

#[test]
fn test_inject_source_repo_no_frontmatter() {
let content = "# Just markdown\n\nNo frontmatter here.";
let result = inject_source_repo(content, "owner/repo");
assert!(result.starts_with("---\nsource_repo: owner/repo\n---\n"));
assert!(result.contains("# Just markdown"));
}

#[test]
fn test_inject_source_repo_updates_existing() {
let content = "---\nname: weather\nsource_repo: old/repo\ndescription: foo\n---\n\nBody\n";
let result = inject_source_repo(content, "new/repo");
assert!(result.contains("source_repo: new/repo"));
assert!(!result.contains("old/repo"));
// Should only have one source_repo line
assert_eq!(result.matches("source_repo:").count(), 1);
}

#[test]
fn test_inject_source_repo_malformed_frontmatter() {
let content = "---\nname: broken\nno closing delimiter";
let result = inject_source_repo(content, "owner/repo");
// Falls back to prepending new frontmatter
assert!(result.starts_with("---\nsource_repo: owner/repo\n---\n"));
assert!(result.contains("no closing delimiter"));
}

#[test]
fn test_inject_source_repo_roundtrip_with_parse() {
use crate::skills::parse_frontmatter;
let content = "---\nname: weather\ndescription: Get weather\n---\n\n# Weather\n";
let patched = inject_source_repo(content, "anthropics/skills");
let (fm, body) = parse_frontmatter(&patched).unwrap();
assert_eq!(fm.get("source_repo").unwrap(), &"anthropics/skills".to_string());
assert_eq!(fm.get("name").unwrap(), &"weather".to_string());
assert!(body.contains("# Weather"));
}
}