diff --git a/openspec/specs/skill-recommendations/spec.md b/openspec/specs/skill-recommendations/spec.md index 27c0336c..3a87f454 100644 --- a/openspec/specs/skill-recommendations/spec.md +++ b/openspec/specs/skill-recommendations/spec.md @@ -497,7 +497,8 @@ require new top-level JSON fields. ], "recommendations": [ { - "skill_id": "string", + "skill_id": "makefile", + "provider_skill_id": "dallay/agents-skills/makefile", "matched_technologies": ["string"], "reasons": ["string"], "installed": true @@ -851,7 +852,7 @@ The `skill_id` (local ID) MUST continue to be used for: ### Requirement: JSON Output Includes Provider Skill ID -`SuggestJsonRecommendation` SHOULD include a `provider_skill_id` field in its serialized JSON +`SuggestJsonRecommendation` MUST include a `provider_skill_id` field in its serialized JSON output so that JSON consumers can access the fully qualified provider identifier alongside the local `skill_id`. @@ -871,8 +872,9 @@ The `provider_skill_id` field is additive; existing JSON fields (`skill_id`, - GIVEN a JSON consumer that reads `skill_id`, `matched_technologies`, `reasons`, and `installed` - WHEN the system adds the `provider_skill_id` field to the recommendation JSON -- THEN the existing fields MUST NOT change position, type, or semantics -- AND the new field MUST be additive only +- THEN the existing fields MUST NOT change name, type, or semantics +- AND producers MUST emit `provider_skill_id` in serialized JSON +- AND existing consumers MAY ignore the `provider_skill_id` field without breaking --- diff --git a/spec.patch b/spec.patch new file mode 100644 index 00000000..0d5c41e3 --- /dev/null +++ b/spec.patch @@ -0,0 +1,10 @@ +--- openspec/specs/skill-recommendations/spec.md ++++ openspec/specs/skill-recommendations/spec.md +@@ -676,1 +676,1 @@ +-`SuggestJsonRecommendation` SHOULD include a `provider_skill_id` field in its serialized JSON ++`SuggestJsonRecommendation` MUST include a `provider_skill_id` field in its serialized JSON +@@ -696,2 +696,2 @@ +-- THEN the existing fields MUST NOT change position, type, or semantics +-- AND the new field MUST be additive only ++- THEN the existing fields MUST NOT change name, type, or semantics ++- AND the new `provider_skill_id` field MUST be additive only (i.e., allowed but not required and must not break consumers) diff --git a/src/commands/skill.rs b/src/commands/skill.rs index f9e44282..84b33a11 100644 --- a/src/commands/skill.rs +++ b/src/commands/skill.rs @@ -943,10 +943,8 @@ impl Provider for SuggestInstallProvider { } if let Ok(source_root) = std::env::var("AGENTSYNC_TEST_SKILL_SOURCE_DIR") { - // The id may be a qualified provider_skill_id (e.g., "dallay/agents-skills/foo") - // or a simple local name. Extract the last segment to find the local source directory. - let local_name = id.rsplit('/').next().unwrap_or(id); - let source_path = PathBuf::from(source_root).join(local_name); + // or a simple local name. Use the full ID to find the local source directory. + let source_path = PathBuf::from(source_root).join(id); if source_path.exists() { return Ok(agentsync::skills::provider::SkillInstallInfo { download_url: source_path.display().to_string(), diff --git a/src/skills/catalog.rs b/src/skills/catalog.rs index e12a7c5e..e97a6a80 100644 --- a/src/skills/catalog.rs +++ b/src/skills/catalog.rs @@ -857,6 +857,7 @@ fn normalize_skill_definition(raw_skill: &RawCatalogSkill) -> Result Result<()> { Ok(()) } + +fn validate_provider_skill_id(provider_skill_id: &str) -> Result<()> { + // Reject leading/trailing slashes or backslashes + if provider_skill_id.starts_with('/') + || provider_skill_id.starts_with('\\') + || provider_skill_id.ends_with('/') + || provider_skill_id.ends_with('\\') + { + bail!("provider_skill_id must not start or end with a slash"); + } + + // Split on '/' and require at least two components + let components: Vec<&str> = provider_skill_id.split('/').collect(); + if components.len() < 2 { + bail!( + "provider_skill_id must follow the 'owner/repo/skill' pattern (at least two segments)" + ); + } + + // Validate each component + for component in &components { + // Reject empty strings + if component.is_empty() { + bail!("provider_skill_id must not have empty segments"); + } + // Reject "." or ".." + if *component == "." || *component == ".." { + bail!("provider_skill_id segments must not be '.' or '..'"); + } + // Restrict to safe characters: alphanumeric, dot, underscore, hyphen + if !component + .chars() + .all(|c| c.is_alphanumeric() || c == '.' || c == '_' || c == '-') + { + bail!( + "provider_skill_id segments can only contain alphanumeric characters, dots, underscores, or hyphens" + ); + } + } + + Ok(()) +} diff --git a/src/skills/provider.rs b/src/skills/provider.rs index 00e7bdc8..af780d94 100644 --- a/src/skills/provider.rs +++ b/src/skills/provider.rs @@ -104,20 +104,30 @@ fn repo_uses_skills_subdirectory(repo: &str) -> bool { } fn local_catalog_skill_source_dir( + provider_skill_id: &str, local_skill_id: &str, project_root: Option<&Path>, ) -> Option { + // Try AGENTSYNC_TEST_SKILL_SOURCE_DIR with provider_skill_id (full path like "dallay/agents-skills/rust-async-patterns") if let Ok(path) = std::env::var("AGENTSYNC_TEST_SKILL_SOURCE_DIR") { - let candidate = PathBuf::from(path).join(local_skill_id); + // First try the full provider_skill_id path (e.g., "dallay/agents-skills/rust-async-patterns") + let candidate = PathBuf::from(path.clone()).join(provider_skill_id); if candidate.exists() { return Some(candidate); } + // Fall back to local_skill_id only (e.g., "rust-async-patterns") + let fallback = PathBuf::from(path).join(local_skill_id); + if fallback.exists() { + return Some(fallback); + } } + // Try AGENTSYNC_LOCAL_SKILLS_REPO/skills/local_skill_id if let Ok(path) = std::env::var("AGENTSYNC_LOCAL_SKILLS_REPO") { return Some(PathBuf::from(path).join("skills").join(local_skill_id)); } + // Try project_parent/agents-skills/skills/local_skill_id project_root .and_then(Path::parent) .map(|parent| { @@ -141,7 +151,8 @@ pub fn resolve_catalog_install_source( } if provider_skill_id.starts_with(DALLAY_AGENTS_SKILLS_PREFIX) - && let Some(path) = local_catalog_skill_source_dir(local_skill_id, project_root) + && let Some(path) = + local_catalog_skill_source_dir(provider_skill_id, local_skill_id, project_root) { return Ok(path.to_string_lossy().into_owned()); } diff --git a/tests/integration/skill_suggest.rs b/tests/integration/skill_suggest.rs index ae868165..7c3e8555 100644 --- a/tests/integration/skill_suggest.rs +++ b/tests/integration/skill_suggest.rs @@ -234,7 +234,7 @@ fn skill_suggest_install_all_surfaces_direct_install_failure_semantics() { fs::write(root.join("Cargo.toml"), "[package]\nname='demo'\n").unwrap(); let source_root = root.join("skill-sources"); - let failing_source = source_root.join("rust-async-patterns"); + let failing_source = source_root.join("dallay/agents-skills/rust-async-patterns"); fs::create_dir_all(&failing_source).unwrap(); let suggest_output = Command::new(agentsync_bin()) diff --git a/tests/unit/suggest_catalog.rs b/tests/unit/suggest_catalog.rs index f793c4a1..c8d66496 100644 --- a/tests/unit/suggest_catalog.rs +++ b/tests/unit/suggest_catalog.rs @@ -409,13 +409,13 @@ fn provider_overlay_can_extend_baseline_with_new_supported_technology_entry() { version = "v1" [[skills]] -provider_skill_id = "rust-async-patterns" +provider_skill_id = "dallay/agents-skills/rust-async-patterns" local_skill_id = "rust-async-patterns" title = "Rust Async Patterns" summary = "Rust async guidance" [[skills]] -provider_skill_id = "makefile" +provider_skill_id = "dallay/agents-skills/makefile" local_skill_id = "makefile" title = "Makefile" summary = "Make guidance" @@ -423,7 +423,7 @@ summary = "Make guidance" [[technologies]] id = "rust" name = "Rust" -skills = ["rust-async-patterns"] +skills = ["dallay/agents-skills/rust-async-patterns"] "#, "fixture", "fixture-v1", @@ -435,7 +435,11 @@ skills = ["rust-async-patterns"] version: "2026.03".to_string(), schema_version: "v1".to_string(), skills: vec![], - technologies: vec![provider_technology("make", "Make", &["makefile"])], + technologies: vec![provider_technology( + "make", + "Make", + &["dallay/agents-skills/makefile"], + )], combos: vec![], }; @@ -453,7 +457,7 @@ skills = ["rust-async-patterns"] .get_technology(&TechnologyId::new(TechnologyId::MAKE)) .unwrap(); assert_eq!(make.name, "Make"); - assert_eq!(make.skills, vec!["makefile"]); + assert_eq!(make.skills, vec!["dallay/agents-skills/makefile"]); } #[test] @@ -463,13 +467,13 @@ fn provider_overlay_prefers_combo_override_by_stable_id() { version = "v1" [[skills]] -provider_skill_id = "docker-expert" +provider_skill_id = "dallay/agents-skills/docker-expert" local_skill_id = "docker-expert" title = "Docker Expert" summary = "Docker guidance" [[skills]] -provider_skill_id = "best-practices" +provider_skill_id = "dallay/agents-skills/best-practices" local_skill_id = "best-practices" title = "Best Practices" summary = "Best practices guidance" @@ -478,7 +482,7 @@ summary = "Best practices guidance" id = "rust-docker" name = "Rust + Docker" requires = ["rust", "docker"] -skills = ["docker-expert"] +skills = ["dallay/agents-skills/docker-expert"] enabled = false "#, "fixture", @@ -498,7 +502,7 @@ enabled = false id: "rust-docker".to_string(), name: "Rust + Docker Override".to_string(), requires: vec!["rust".to_string(), "docker".to_string()], - skills: vec!["best-practices".to_string()], + skills: vec!["dallay/agents-skills/best-practices".to_string()], enabled: Some(true), reason_template: Some("Provider combo override".to_string()), }], @@ -509,7 +513,7 @@ enabled = false let combo = catalog.get_combo("rust-docker").unwrap(); assert_eq!(combo.name, "Rust + Docker Override"); - assert_eq!(combo.skills, vec!["best-practices"]); + assert_eq!(combo.skills, vec!["dallay/agents-skills/best-practices"]); assert!(combo.enabled); assert_eq!( catalog