fix: mandatory provider_skill_id and strict catalog validation

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

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)

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(),

src/skills/catalog.rs

@@ -857,6 +857,7 @@ fn normalize_skill_definition(raw_skill: &RawCatalogSkill) -> Result<CatalogSkil
     let title = require_non_empty("title", &raw_skill.title)?;
     let summary = require_non_empty("summary", &raw_skill.summary)?;
     validate_local_skill_id(local_skill_id)?;
+    validate_provider_skill_id(provider_skill_id)?;
 
     Ok(CatalogSkillDefinition {
         provider_skill_id: provider_skill_id.to_string(),
@@ -1034,3 +1035,45 @@ fn validate_local_skill_id(skill_id: &str) -> 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(())
+}

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<PathBuf> {
+    // 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());
     }

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())

tests/unit/suggest_catalog.rs

@@ -409,21 +409,21 @@ 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"
 
 [[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