diff --git a/CHANGELOG.md b/CHANGELOG.md index 01b8a6a0..fe8fec79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ not per individual commit. ## [Unreleased] +### Added + +- Kiro installs write a short `ks-/SKILL.md` pointer per skill into `.kiro/skills/` and list + them in every agent, so Kiro shows each skill natively. The pointer loads the body with + `get_skill`, reading the file only if the server is not connected. `install --no-skill-pointers` opts out. + ### Changed - The orchestrators (`konductor`, `konductor-mux-orchestrator`, `konductor-cmux-orchestrator`) diff --git a/README.md b/README.md index f4299cf5..4b055ead 100644 --- a/README.md +++ b/README.md @@ -207,7 +207,11 @@ auto-detection. Paths are relative to `--target `, or `$HOME` if you don't | `claude` | `.claude/agents/` | `.claude/skills/` | `.claude/skills/sop-/SKILL.md` | Kiro skills live in `.konductor/skills/`, not `.kiro/skills/`, so Kiro doesn't expose every skill -to every agent. `kiro-cli-v2` and `kiro-v3` share those paths, so installing one over the other +body to every agent. Each skill also gets a short `ks-/SKILL.md` pointer under `.kiro/skills/` +with its name and a short description, so Kiro lists it natively. The pointer tells the agent to load +the body with `get_skill` on the `konductor-skills` MCP server, and to read the file only if that +server is not connected. Pass `--no-skill-pointers` to `install` to skip the pointers (`update` keeps +that choice). `kiro-cli-v2` and `kiro-v3` share those paths, so installing one over the other replaces it (with a warning). `claude` installs alongside either one in the same target. Without `--from`, `install` fetches a published GitHub release, including the platform's diff --git a/cli/README.md b/cli/README.md index c0745388..25a72a71 100644 --- a/cli/README.md +++ b/cli/README.md @@ -281,6 +281,30 @@ install merges into `/.konductor/skills/`: a skill directory this instal not emit (e.g. hand-authored) is left untouched, but a skill directory it does own is fully replaced so a file removed from the source doesn't linger in the destination. +#### Skill pointers (Kiro harnesses) + +Only a small pointer per skill goes into `.kiro/skills/ks-/SKILL.md`. It carries the +skill's name and a description cut to 200 characters, so Kiro lists every skill natively. +The pointer tells the agent to call `get_skill` on the `konductor-skills` MCP server first, +and to read `/.konductor/skills//SKILL.md` only if that server is not +connected. If `get_skill` says the skill is not found, the agent is told to stop. + +- Each Kiro agent also gets a `skill://.kiro/skills/ks-*/SKILL.md` resource, so the + pointers are listed for it explicitly. +- The `ks-` prefix stays: pointers would otherwise clash with same-named skills other + tools install in `.kiro/skills/`, and the pointer text names the real skill. +- Pointers add every skill's name and description to every agent's prompt, and a + home-directory install puts them in `~/.kiro/skills/`, where other Kiro agents see them + too. `install --no-skill-pointers` skips the pointers and the agent resource, removes + ones an earlier install wrote, and is recorded in `.konductor/no-skill-pointers`, so + `update` keeps it. A later `install` without the flag turns pointers back on. +- Because the fallback path is absolute, a project install writes your local path into + `.kiro/skills/ks-*/SKILL.md`. Don't commit that directory. +- Pointers are tracked in the manifest. `update` removes the pointer of a skill the source + dropped, and `uninstall` removes them all. Claude installs get none. +- A read of the fallback file does not go through `get_skill`, so it is not counted in + skill-load telemetry. + ### Installing without `--from`: the GitHub-release / main-branch-`dist/` fallback chain ```bash diff --git a/cli/konductor-rs/src/cli.rs b/cli/konductor-rs/src/cli.rs index 6593a1a6..b7faf7f5 100644 --- a/cli/konductor-rs/src/cli.rs +++ b/cli/konductor-rs/src/cli.rs @@ -189,6 +189,14 @@ pub enum Commands { #[arg(long, display_order = 4)] no_telemetry: bool, + /// Kiro harnesses only: do not write the `ks-` skill pointers + /// under `.kiro/skills/` or the matching `skill://` agent resource, + /// and remove ones an earlier install wrote. The choice is recorded + /// for this target, so `update` keeps it; a later `install` without + /// this flag turns the pointers back on. + #[arg(long = "no-skill-pointers", action = ArgAction::SetTrue)] + no_skill_pointers: bool, + /// Opt in to reading `GITHUB_TOKEN` from the environment for the /// no-`--from` remote install path. Without this flag, /// `GITHUB_TOKEN` is never read even if set in the shell. Has no diff --git a/cli/konductor-rs/src/cli/dispatch.rs b/cli/konductor-rs/src/cli/dispatch.rs index a3896509..0941ff9c 100644 --- a/cli/konductor-rs/src/cli/dispatch.rs +++ b/cli/konductor-rs/src/cli/dispatch.rs @@ -37,15 +37,17 @@ pub fn dispatch(command: Commands, verbose: bool, json: bool, color: ColorMode) harness, link_bin, no_telemetry, + no_skill_pointers, use_github_token, release_version, force, - } => crate::cli::install::dispatch_install_with( + } => crate::cli::install::dispatch_install_with_skill_pointer_choice( from, target, harness, link_bin, no_telemetry, + no_skill_pointers, use_github_token, release_version, force, @@ -1011,6 +1013,7 @@ mod tests { harness: "kiro-cli-v2".to_string(), link_bin: true, no_telemetry: false, + no_skill_pointers: false, use_github_token: false, release_version: None, force: false, @@ -1050,6 +1053,7 @@ mod tests { harness: "kiro-cli-v2".to_string(), link_bin: false, no_telemetry: false, + no_skill_pointers: false, use_github_token: false, release_version: None, force: false, @@ -1084,6 +1088,7 @@ mod tests { harness: "kiro-cli-v2".to_string(), link_bin: true, no_telemetry: false, + no_skill_pointers: false, use_github_token: false, release_version: None, force: false, diff --git a/cli/konductor-rs/src/cli/install.rs b/cli/konductor-rs/src/cli/install.rs index 66525b7d..8deafcfd 100644 --- a/cli/konductor-rs/src/cli/install.rs +++ b/cli/konductor-rs/src/cli/install.rs @@ -698,6 +698,67 @@ pub fn dispatch_install_with( ) } +/// Harness names whose installs write `ks-` skill pointers. +fn harness_writes_skill_pointers(harness: &str) -> bool { + matches!(harness, "kiro-cli-v2" | "kiro-v3") +} + +/// `dispatch_install_with` after recording this run's `--no-skill-pointers` +/// choice for a Kiro target. The strategy reads the recorded choice +/// during the install, and `update` reads it too, so the opt-out +/// outlasts this run. A changed choice also bypasses the +/// already-at-version skip, since that skip would otherwise ignore it. +#[allow(clippy::too_many_arguments)] +pub fn dispatch_install_with_skill_pointer_choice( + from: Option, + target: Option, + harness: String, + link_bin: bool, + no_telemetry: bool, + no_skill_pointers: bool, + use_github_token: bool, + release_version: Option, + force: bool, + verbose: bool, + json: bool, + color: ColorMode, +) -> u8 { + let mut force = force; + if harness_writes_skill_pointers(&harness) { + if let Ok(destination) = resolve_destination(target.as_deref()) { + match kiro_cli::set_skill_pointers_opt_out(&destination, no_skill_pointers) { + Ok(changed) => force |= changed, + Err(message) => { + super::report::report_error( + "install", + "install.skill_pointer_choice_failed", + &destination, + no_telemetry, + &format!("could not record --no-skill-pointers: {message}"), + Vec::new(), + json, + color, + ); + return EXIT_USAGE_ERROR; + } + } + } + } + dispatch_install_with( + from, + target, + harness, + link_bin, + no_telemetry, + use_github_token, + release_version, + force, + verbose, + json, + color, + ) +} + /// The full `dispatch_install_with` implementation, parameterized by /// `remote_installer` -- the no-`--from` remote-install attempt. In /// production, `dispatch_install_with` passes a closure that reaches @@ -1558,7 +1619,12 @@ impl InstallCounts { skill_dirs.insert((".konductor/skills/", name)); } } else if let Some(rest) = file.path.strip_prefix(".kiro/skills/") { - if let Some(name) = rest.split('/').next().filter(|s| !s.is_empty()) { + // `ks-` pointers stand for skills already counted above. + if let Some(name) = rest + .split('/') + .next() + .filter(|s| !s.is_empty() && !s.starts_with(kiro_cli::SKILL_POINTER_PREFIX)) + { skill_dirs.insert((".kiro/skills/", name)); } } else if let Some(rest) = file.path.strip_prefix(claude_skills_prefix.as_str()) { diff --git a/cli/konductor-rs/src/cli/install/claude.rs b/cli/konductor-rs/src/cli/install/claude.rs index 3770acce..62c25c83 100644 --- a/cli/konductor-rs/src/cli/install/claude.rs +++ b/cli/konductor-rs/src/cli/install/claude.rs @@ -915,7 +915,7 @@ fn strip_trailing_punctuation_artifacts(text: &str) -> &str { /// derived values). This is what makes an Overview line containing a /// colon safe as a YAML scalar: unlike a plain/unquoted scalar, `": "` /// inside a double-quoted string is never a mapping separator. -fn yaml_double_quote(text: &str) -> String { +pub(super) fn yaml_double_quote(text: &str) -> String { let mut out = String::with_capacity(text.len() + 2); out.push('"'); for c in text.chars() { diff --git a/cli/konductor-rs/src/cli/install/kiro_cli.rs b/cli/konductor-rs/src/cli/install/kiro_cli.rs index d0dc1669..350b6fb1 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli.rs @@ -11,17 +11,20 @@ // Skills land under `.konductor/skills/`, not `.kiro/skills/`: Kiro CLI's // native skill discovery scans `.kiro/skills/` unconditionally and makes // every skill visible to every agent, which defeats per-agent scoping. -// Keeping skills outside that scanned directory means a skill is only +// Keeping skill bodies outside that scanned directory means a body is only // visible to an agent that references it via a `skill://` resource entry. +// Only the small `ks-/SKILL.md` pointers (`skill_pointers.rs`) land in +// `.kiro/skills/`, so every skill's name and description is discoverable. // // Skill install merges into `.konductor/skills/`: a directory this install // did not emit is left untouched; one it does own is replaced wholesale so // removed files don't linger. // // This file's logic is split under `kiro_cli/`: `plan.rs` (write-ahead -// planning), `copy.rs` (copy/install execution), `fs_util.rs` (shared -// filesystem primitives). This file keeps the `InstallStrategy` impl and -// re-exports what sibling modules and tests depend on. +// planning), `copy.rs` (copy/install execution), `skill_pointers.rs` +// (native skill pointers), `fs_util.rs` (shared filesystem primitives). +// This file keeps the `InstallStrategy` impl and re-exports what sibling +// modules and tests depend on. use std::path::Path; @@ -309,6 +312,7 @@ impl InstallStrategy for KiroCliInstallStrategy { mod copy; mod fs_util; mod plan; +mod skill_pointers; // Re-exports so sibling `install` modules (`claude`, `mcp_server`) and this // module's own tests can keep using unqualified paths after the split into @@ -324,6 +328,13 @@ pub(super) use plan::{ plan_claude_hooks_file, plan_claude_settings_grant, plan_skill_dir_recursive, read_skill_scopes_sidecar, read_sop_scopes_sidecar, PlannedFile, }; +pub(super) use skill_pointers::{ + install_kiro_skill_pointers, plan_kiro_skill_pointer_files, SKILL_POINTER_PREFIX, + SKILL_POINTER_RESOURCE, +}; +pub(crate) use skill_pointers::{ + remove_skill_pointers_opt_out, set_skill_pointers_opt_out, skill_pointers_enabled, +}; #[cfg(test)] pub(super) mod test_support { @@ -1583,7 +1594,10 @@ mod tests { "file://{}", target_dir.join(".kiro/context/routing-rules.md").display() ); - assert_eq!(value["resources"], serde_json::json!([expected])); + assert_eq!( + value["resources"], + serde_json::json!([expected, "skill://.kiro/skills/ks-*/SKILL.md"]) + ); fs::remove_dir_all(&target_dir).ok(); fs::remove_dir_all(&repo_root).ok(); @@ -1626,14 +1640,17 @@ mod tests { "file://{}", target_dir.join(".kiro/context/routing-rules.md").display() ); - assert_eq!(value["resources"], serde_json::json!([expected])); + assert_eq!( + value["resources"], + serde_json::json!([expected, "skill://.kiro/skills/ks-*/SKILL.md"]) + ); fs::remove_dir_all(&target_dir).ok(); fs::remove_dir_all(&repo_root).ok(); } #[test] - fn install_from_local_agent_with_no_context_names_is_unchanged() { + fn install_from_local_agent_with_no_context_names_only_gains_the_pointer_glob() { let target_dir = scratch_dir("no-context-target"); let repo_root = scratch_dir("no-context-repo"); seed_synthed_agent( @@ -1654,7 +1671,10 @@ mod tests { let installed_agent = target_dir.join(".kiro/agents/k-example.json"); let value: serde_json::Value = serde_json::from_str(&fs::read_to_string(&installed_agent).unwrap()).unwrap(); - assert_eq!(value["resources"], serde_json::json!(["file://AGENTS.md"])); + assert_eq!( + value["resources"], + serde_json::json!(["file://AGENTS.md", "skill://.kiro/skills/ks-*/SKILL.md"]) + ); fs::remove_dir_all(&target_dir).ok(); fs::remove_dir_all(&repo_root).ok(); @@ -1808,9 +1828,13 @@ mod tests { ); assert_eq!( value["resources"], - serde_json::json!(["file:///etc/hand-authored.md", expected_context]), + serde_json::json!([ + "file:///etc/hand-authored.md", + expected_context, + "skill://.kiro/skills/ks-*/SKILL.md" + ]), "hand-authored absolute entry must pass through untouched, in its original position, \ - with the rewritten context entry deterministically after it" + with the rewritten context entry deterministically after it, then the pointer glob" ); fs::remove_dir_all(&target_dir).ok(); @@ -1862,8 +1886,14 @@ mod tests { "file://{}", target_b.join(".kiro/context/routing-rules.md").display() ); - assert_eq!(value_a["resources"], serde_json::json!([expected_a])); - assert_eq!(value_b["resources"], serde_json::json!([expected_b])); + assert_eq!( + value_a["resources"], + serde_json::json!([expected_a, "skill://.kiro/skills/ks-*/SKILL.md"]) + ); + assert_eq!( + value_b["resources"], + serde_json::json!([expected_b, "skill://.kiro/skills/ks-*/SKILL.md"]) + ); assert_ne!( value_a["resources"], value_b["resources"], "each root's installed agent must carry its own absolute path" @@ -2130,7 +2160,10 @@ mod tests { .join(".konductor/skills/constraints/SKILL.md") .display() ); - assert_eq!(value["resources"], serde_json::json!([expected])); + assert_eq!( + value["resources"], + serde_json::json!([expected, "skill://.kiro/skills/ks-*/SKILL.md"]) + ); fs::remove_dir_all(&target_dir).ok(); fs::remove_dir_all(&repo_root).ok(); @@ -2248,7 +2281,10 @@ mod tests { serde_json::from_str(&fs::read_to_string(&installed_agent).unwrap()).unwrap(); assert_eq!( value["resources"], - serde_json::json!(["skill://.kiro/skills/ws-*/SKILL.md"]) + serde_json::json!([ + "skill://.kiro/skills/ws-*/SKILL.md", + "skill://.kiro/skills/ks-*/SKILL.md" + ]) ); fs::remove_dir_all(&target_dir).ok(); @@ -2421,8 +2457,14 @@ mod tests { .join(".konductor/skills/constraints/SKILL.md") .display() ); - assert_eq!(value_a["resources"], serde_json::json!([expected_a])); - assert_eq!(value_b["resources"], serde_json::json!([expected_b])); + assert_eq!( + value_a["resources"], + serde_json::json!([expected_a, "skill://.kiro/skills/ks-*/SKILL.md"]) + ); + assert_eq!( + value_b["resources"], + serde_json::json!([expected_b, "skill://.kiro/skills/ks-*/SKILL.md"]) + ); assert_ne!(value_a["resources"], value_b["resources"]); fs::remove_dir_all(&repo_root).ok(); diff --git a/cli/konductor-rs/src/cli/install/kiro_cli/copy.rs b/cli/konductor-rs/src/cli/install/kiro_cli/copy.rs index 9a2f55f3..26ab9b72 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli/copy.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli/copy.rs @@ -12,6 +12,7 @@ use super::super::artifact::sha256_hex; use super::super::manifest::{ManifestFile, Provenance, StrategyManifest}; use super::fs_util::{is_executable, reject_unsafe_file_name, set_executable}; use super::plan::{content_manifest_path, read_skill_scopes_sidecar, read_sop_scopes_sidecar}; +use super::skill_pointers::skill_pointers_enabled; use super::{KIRO_DESTINATION_ROOT, KONDUCTOR_DESTINATION_ROOT}; use crate::cli::synth::kiro_cli_v2::{ AGENTS_CONTENT_TYPE_DIR, CONTEXT_CONTENT_TYPE_DIR, SKILLS_CONTENT_TYPE_DIR, @@ -496,7 +497,11 @@ fn copy_agent_files_rewriting_resources( file_names: Vec, no_telemetry: bool, ) -> Result<(Vec, bool), String> { - let passes = super::super::resource_rewrite::standard_passes(no_telemetry); + let pointers_on = ctx.install_root().is_some_and(skill_pointers_enabled); + let passes = super::super::resource_rewrite::with_skill_pointer_pass( + super::super::resource_rewrite::standard_passes(no_telemetry), + pointers_on, + ); let mut files = Vec::with_capacity(file_names.len()); let mut any_mcp_server_injected = false; @@ -535,7 +540,12 @@ fn copy_agent_files_rewriting_resources( // injection for exactly the agents that most need it. let contents = String::from_utf8(original_bytes.clone()) .map_err(|e| format!("{} is not valid UTF-8: {e}", src.display()))?; + // The pointer glob alone does not justify parsing: a file that is + // not JSON and needs no other rewrite stays a verbatim copy. + let needs_pointer_glob = + pointers_on && serde_json::from_str::(&contents).is_ok(); if no_telemetry + && !needs_pointer_glob && !contents.contains(CONTEXT_RESOURCE_PREFIX) && !contents.contains(super::super::resource_rewrite::SKILL_RESOURCE_PREFIX) && !agent_has_sop_names diff --git a/cli/konductor-rs/src/cli/install/kiro_cli/plan.rs b/cli/konductor-rs/src/cli/install/kiro_cli/plan.rs index c6670c8b..a7f9ee2a 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli/plan.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli/plan.rs @@ -13,8 +13,8 @@ use std::path::Path; use super::super::manifest::{classify_provenance, ManifestFile, Provenance, StrategyManifest}; use super::super::runtime::{detect_runtimes, Runtime}; use super::{ - list_agent_files, list_agent_files_like, list_skill_dirs, CONTEXT_RESOURCE_PREFIX, - KIRO_DESTINATION_ROOT, KONDUCTOR_DESTINATION_ROOT, + list_agent_files, list_agent_files_like, list_skill_dirs, plan_kiro_skill_pointer_files, + CONTEXT_RESOURCE_PREFIX, KIRO_DESTINATION_ROOT, KONDUCTOR_DESTINATION_ROOT, }; use crate::cli::synth::kiro_cli_v2::{ AGENTS_CONTENT_TYPE_DIR, CONTEXT_CONTENT_TYPE_DIR, SKILLS_CONTENT_TYPE_DIR, @@ -114,6 +114,11 @@ pub(in crate::cli::install) fn plan_all_files( target_dir, prior_manifest, )?); + plan.extend(plan_kiro_skill_pointer_files( + harness_dir, + target_dir, + prior_manifest, + )?); plan.extend(plan_agent_files(harness_dir, target_dir, prior_manifest)?); Ok(plan) } diff --git a/cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs b/cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs new file mode 100644 index 00000000..619c2601 --- /dev/null +++ b/cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs @@ -0,0 +1,591 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// install/kiro_cli/skill_pointers.rs — one small `ks-/SKILL.md` +// pointer per synthed skill under `.kiro/skills/`, so Kiro lists every +// skill's name and description natively while the body is still loaded +// through the `konductor-skills` MCP server. +// +// Each Kiro agent also gets a `skill://.kiro/skills/ks-*/SKILL.md` +// resource (`resource_rewrite::SkillPointerResourcePass`) so the pointers +// are listed for that agent explicitly rather than by relying on Kiro's +// directory scan. +// +// `install --no-skill-pointers` records a marker file under `.konductor/`; +// install and update read it, so an opt-out survives `update`. + +use std::path::{Path, PathBuf}; + +use super::super::artifact::sha256_hex; +use super::super::claude::yaml_double_quote; +use super::super::manifest::{classify_provenance, ManifestFile, Provenance, StrategyManifest}; +use super::plan::{content_manifest_path, PlannedFile}; +use super::{list_skill_dirs, KIRO_DESTINATION_ROOT, KONDUCTOR_DESTINATION_ROOT}; +use crate::cli::atomic_write::write_atomic; +use crate::cli::synth::kiro_cli_v2::SKILLS_CONTENT_TYPE_DIR; +use crate::cli::synth::parse_canonical::split_frontmatter; +use crate::cli::synth::path_safety::reject_unsafe_skill_name; + +/// Directory-name prefix of every pointer. Keeps pointers apart from +/// same-named skills other tools put in `.kiro/skills/`. The prefix is +/// not the bare skill name, so a pointer is listed as `ks-`. +pub(in crate::cli::install) const SKILL_POINTER_PREFIX: &str = "ks-"; + +/// The `resources` entry that lists every pointer for an agent. Must +/// match `SKILL_POINTER_PREFIX`. +pub(in crate::cli::install) const SKILL_POINTER_RESOURCE: &str = + "skill://.kiro/skills/ks-*/SKILL.md"; + +/// Longest pointer description, in characters, ellipsis included. Every +/// pointer is listed in each agent's prompt, so this bounds their +/// combined size. +const POINTER_DESCRIPTION_MAX_CHARS: usize = 200; + +const ELLIPSIS: &str = "..."; + +const SKILL_FILE_NAME: &str = "SKILL.md"; + +/// Marker file under `.konductor/` that records `install +/// --no-skill-pointers`. +const OPT_OUT_MARKER_NAME: &str = "no-skill-pointers"; + +/// One synthed skill, reduced to what its pointer needs. +struct SkillPointer { + skill_name: String, + description: String, +} + +impl SkillPointer { + fn dir_name(&self) -> String { + format!("{SKILL_POINTER_PREFIX}{}", self.skill_name) + } + + fn manifest_path(&self) -> String { + content_manifest_path( + KIRO_DESTINATION_ROOT, + SKILLS_CONTENT_TYPE_DIR, + &format!("{}/{SKILL_FILE_NAME}", self.dir_name()), + ) + } +} + +/// The pointers a harness dir yields, plus the skills that get none. +struct PointerSet { + pointers: Vec, + /// `(skill name, reason)` for each skill without a usable description. + skipped: Vec<(String, String)>, +} + +fn opt_out_marker_path(target_dir: &Path) -> PathBuf { + target_dir + .join(KONDUCTOR_DESTINATION_ROOT) + .join(OPT_OUT_MARKER_NAME) +} + +/// Whether pointers are written for `target_dir`: true unless an +/// `install --no-skill-pointers` left its marker there. +pub(crate) fn skill_pointers_enabled(target_dir: &Path) -> bool { + !opt_out_marker_path(target_dir).exists() +} + +/// Records (`opt_out`) or clears the `--no-skill-pointers` choice for +/// `target_dir`. Returns whether the recorded choice changed. +pub(crate) fn set_skill_pointers_opt_out(target_dir: &Path, opt_out: bool) -> Result { + let marker = opt_out_marker_path(target_dir); + if opt_out == marker.exists() { + return Ok(false); + } + if opt_out { + std::fs::create_dir_all(target_dir.join(KONDUCTOR_DESTINATION_ROOT)) + .map_err(|e| format!("failed to create {}: {e}", marker.display()))?; + write_atomic( + &marker, + b"install --no-skill-pointers was used here; install and update skip the ks-* pointers.\n", + ) + .map_err(|e| format!("failed to write {}: {e}", marker.display()))?; + } else { + std::fs::remove_file(&marker) + .map_err(|e| format!("failed to remove {}: {e}", marker.display()))?; + } + Ok(true) +} + +/// Removes the opt-out marker, if any, on a full uninstall. +pub(crate) fn remove_skill_pointers_opt_out(target_dir: &Path) -> std::io::Result<()> { + match std::fs::remove_file(opt_out_marker_path(target_dir)) { + Err(err) if err.kind() != std::io::ErrorKind::NotFound => Err(err), + _ => Ok(()), + } +} + +/// Writes one pointer `SKILL.md` per skill staged under +/// `/skills/` into `/.kiro/skills/ks-/`, +/// and removes the pointers `prior_manifest` tracked that this run no +/// longer writes (a dropped skill, or `--no-skill-pointers`). Returns an +/// empty `Vec` when pointers are off or no skills are staged. +pub(in crate::cli::install) fn install_kiro_skill_pointers( + harness_dir: &Path, + target_dir: &Path, + prior_manifest: Option<&StrategyManifest>, +) -> Result, String> { + let mut files = Vec::new(); + if skill_pointers_enabled(target_dir) { + let set = list_skill_pointers(harness_dir)?; + for (skill, reason) in &set.skipped { + eprintln!( + "konductor install: warning: no skill pointer for '{skill}': {reason}; \ + the skill itself is still installed" + ); + } + let fallback_dir = fallback_skills_dir(target_dir); + for pointer in &set.pointers { + let rendered = render_pointer_md(pointer, &fallback_dir); + let dst = target_dir.join(pointer.manifest_path()); + write_pointer(&dst, &rendered)?; + files.push(ManifestFile { + path: pointer.manifest_path(), + sha256: Some(sha256_hex(rendered.as_bytes())), + // Overwritten by `attach_provenance`. + provenance: Provenance::Created, + }); + } + } + prune_stale_pointers(target_dir, prior_manifest, &files); + Ok(files) +} + +/// Plans every file `install_kiro_skill_pointers` will write, without +/// writing anything. +pub(in crate::cli::install) fn plan_kiro_skill_pointer_files( + harness_dir: &Path, + target_dir: &Path, + prior_manifest: Option<&StrategyManifest>, +) -> Result, String> { + if !skill_pointers_enabled(target_dir) { + return Ok(Vec::new()); + } + Ok(list_skill_pointers(harness_dir)? + .pointers + .iter() + .map(|pointer| { + let manifest_path = pointer.manifest_path(); + let provenance = classify_provenance( + &target_dir.join(&manifest_path), + &manifest_path, + prior_manifest, + ); + PlannedFile { + manifest_path, + provenance, + } + }) + .collect()) +} + +/// Deletes the pointer files `prior_manifest` tracked that `written` no +/// longer contains. Only manifest-tracked paths are touched, and a pointer +/// directory is removed only once empty. Best-effort: a file the user +/// already removed is fine. +fn prune_stale_pointers( + target_dir: &Path, + prior_manifest: Option<&StrategyManifest>, + written: &[ManifestFile], +) { + let Some(prior) = prior_manifest else { + return; + }; + let pointer_root = content_manifest_path( + KIRO_DESTINATION_ROOT, + SKILLS_CONTENT_TYPE_DIR, + SKILL_POINTER_PREFIX, + ); + for prior_file in &prior.files { + if !prior_file.path.starts_with(&pointer_root) + || written.iter().any(|f| f.path == prior_file.path) + { + continue; + } + let stale = target_dir.join(&prior_file.path); + let _ = std::fs::remove_file(&stale); + if let Some(dir) = stale.parent() { + let _ = std::fs::remove_dir(dir); + } + } +} + +fn write_pointer(dst: &Path, rendered: &str) -> Result<(), String> { + if let Some(dir) = dst.parent() { + std::fs::create_dir_all(dir) + .map_err(|e| format!("failed to create {}: {e}", dir.display()))?; + } + write_atomic(dst, rendered.as_bytes()) + .map_err(|e| format!("failed to write {}: {e}", dst.display())) +} + +/// A skill whose `SKILL.md` cannot be read fails the install. A skill +/// whose `description` is missing or unusable gets no pointer and is +/// reported in `skipped`: synth rejects such skills, so only a hand-staged +/// tree can hit this. +fn list_skill_pointers(harness_dir: &Path) -> Result { + let source_root = harness_dir.join(SKILLS_CONTENT_TYPE_DIR); + let mut set = PointerSet { + pointers: Vec::new(), + skipped: Vec::new(), + }; + for skill_name in list_skill_dirs(&source_root)? { + reject_unsafe_skill_name(&format!("{SKILL_POINTER_PREFIX}{skill_name}"))?; + let skill_md = source_root.join(&skill_name).join(SKILL_FILE_NAME); + let raw = std::fs::read_to_string(&skill_md) + .map_err(|e| format!("failed to read {}: {e}", skill_md.display()))?; + match read_description(&raw) { + Ok(description) => set.pointers.push(SkillPointer { + skill_name, + description, + }), + Err(reason) => set.skipped.push((skill_name, reason)), + } + } + Ok(set) +} + +/// The skill's `description`, flattened to one line of printable +/// characters and cut to `POINTER_DESCRIPTION_MAX_CHARS`. +fn read_description(skill_md: &str) -> Result { + let (frontmatter, _) = split_frontmatter(skill_md, SKILL_FILE_NAME) + .map_err(|_| "SKILL.md has no frontmatter".to_string())?; + let value: serde_yaml::Value = serde_yaml::from_str(frontmatter) + .map_err(|e| format!("SKILL.md frontmatter is not valid YAML ({e})"))?; + let description = value + .get("description") + .ok_or_else(|| "SKILL.md has no `description`".to_string())? + .as_str() + .ok_or_else(|| "`description` is not a string".to_string())?; + let one_line = description.split_whitespace().collect::>().join(" "); + // `yaml_double_quote` assumes no control characters. + let printable: String = one_line.chars().filter(|c| !c.is_control()).collect(); + if printable.is_empty() { + return Err("`description` is empty".to_string()); + } + Ok(truncate_description(&printable)) +} + +/// Cuts `text` to `POINTER_DESCRIPTION_MAX_CHARS` characters, at a word +/// boundary when one falls in the back half, and ends it with an ellipsis. +fn truncate_description(text: &str) -> String { + if text.chars().count() <= POINTER_DESCRIPTION_MAX_CHARS { + return text.to_string(); + } + let keep: String = text + .chars() + .take(POINTER_DESCRIPTION_MAX_CHARS - ELLIPSIS.len()) + .collect(); + let cut = match keep.rfind(' ') { + Some(i) if i >= keep.len() / 2 => &keep[..i], + _ => keep.as_str(), + }; + format!("{}{ELLIPSIS}", cut.trim_end()) +} + +/// The absolute path of the installed skill bodies, so the fallback read +/// works from any working directory without relying on `~` expansion. +fn fallback_skills_dir(target_dir: &Path) -> String { + std::path::absolute(target_dir) + .unwrap_or_else(|_| target_dir.to_path_buf()) + .join(KONDUCTOR_DESTINATION_ROOT) + .join(SKILLS_CONTENT_TYPE_DIR) + .display() + .to_string() +} + +fn render_pointer_md(pointer: &SkillPointer, fallback_dir: &str) -> String { + let name = &pointer.skill_name; + format!( + "---\nname: {}\ndescription: {}\n---\n\n\ + This file points to the `{name}` skill. The skill's instructions are not in this file.\n\n\ + 1. Call `get_skill` with `name: \"{name}\"` on the `konductor-skills` MCP server. \ + Do this first. If the tool is not loaded yet, load it, then call it.\n\ + 2. Only if the `konductor-skills` server is not connected, or it has no `get_skill` tool, \ + read `{fallback_dir}/{name}/{SKILL_FILE_NAME}`.\n\ + 3. If `get_skill` says the skill is not found, stop. The skill is outside your scope. \ + Do not read the file.\n", + yaml_double_quote(&pointer.dir_name()), + yaml_double_quote(&pointer.description), + ) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::cli::install::manifest::Status; + + fn scratch_dir(name: &str) -> std::path::PathBuf { + super::super::test_support::scratch_dir(&format!("skill-pointers-{name}")) + } + + fn stage_skill(harness_dir: &Path, name: &str, skill_md: &str) { + let dir = harness_dir.join(SKILLS_CONTENT_TYPE_DIR).join(name); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join(SKILL_FILE_NAME), skill_md).unwrap(); + } + + fn skill_md(name: &str, description: &str) -> String { + format!("---\nname: {name}\ndescription: {description}\n---\n\nBody\n") + } + + fn manifest_of(files: &[ManifestFile]) -> StrategyManifest { + StrategyManifest::new( + "kiro-cli-v2", + "2026-01-01T00:00:00Z", + ".", + None, + Status::Complete, + files.to_vec(), + ) + } + + fn paths(files: &[ManifestFile]) -> Vec<&str> { + files.iter().map(|f| f.path.as_str()).collect() + } + + #[test] + fn description_is_flattened_to_one_line() { + let md = "---\nname: a\ndescription: >-\n one\n two\n---\n\nBody\n"; + assert_eq!(read_description(md).as_deref(), Ok("one two")); + } + + #[test] + fn description_errors_without_frontmatter_or_key() { + assert!(read_description("# just a body\n").is_err()); + assert!(read_description("---\nname: a\n---\n\nBody\n").is_err()); + } + + #[test] + fn list_valued_description_is_reported_not_flattened() { + let md = "---\nname: a\ndescription:\n - one\n - two\n---\n\nBody\n"; + let err = read_description(md).unwrap_err(); + assert!(err.contains("not a string"), "{err}"); + } + + #[test] + fn control_characters_are_stripped_from_the_description() { + let md = "---\nname: a\ndescription: \"red \\u001b[31mtext\\u0007\"\n---\n\nBody\n"; + assert_eq!(read_description(md).as_deref(), Ok("red [31mtext")); + } + + #[test] + fn description_of_only_control_characters_is_an_error() { + let md = "---\nname: a\ndescription: \"\\u001b\"\n---\n\nBody\n"; + assert!(read_description(md).unwrap_err().contains("empty")); + } + + #[test] + fn long_description_is_cut_to_the_cap_at_a_word_boundary() { + let long = "word ".repeat(100); + let cut = truncate_description(long.trim()); + assert!( + cut.chars().count() <= POINTER_DESCRIPTION_MAX_CHARS, + "{cut}" + ); + assert!(cut.ends_with(ELLIPSIS), "{cut}"); + assert!(cut.trim_end_matches(ELLIPSIS).ends_with("word"), "{cut}"); + } + + #[test] + fn description_at_the_cap_is_unchanged() { + let exact = "a".repeat(POINTER_DESCRIPTION_MAX_CHARS); + assert_eq!(truncate_description(&exact), exact); + let over = "a".repeat(POINTER_DESCRIPTION_MAX_CHARS + 1); + let cut = truncate_description(&over); + assert_eq!(cut.chars().count(), POINTER_DESCRIPTION_MAX_CHARS); + } + + #[test] + fn truncation_never_splits_a_multibyte_character() { + let wide = "é".repeat(POINTER_DESCRIPTION_MAX_CHARS + 50); + let cut = truncate_description(&wide); + assert_eq!(cut.chars().count(), POINTER_DESCRIPTION_MAX_CHARS); + } + + #[test] + fn resource_glob_matches_the_pointer_prefix() { + assert_eq!( + SKILL_POINTER_RESOURCE, + format!("skill://.kiro/skills/{SKILL_POINTER_PREFIX}*/SKILL.md") + ); + } + + #[test] + fn rendered_pointer_is_valid_frontmatter_with_prefixed_name() { + let pointer = SkillPointer { + skill_name: "alpha".to_string(), + description: "Say \"hi\": now".to_string(), + }; + let rendered = render_pointer_md(&pointer, "/abs/.konductor/skills"); + let (frontmatter, body) = split_frontmatter(&rendered, "pointer").unwrap(); + let value: serde_yaml::Value = serde_yaml::from_str(frontmatter).unwrap(); + assert_eq!(value["name"].as_str(), Some("ks-alpha")); + assert_eq!(value["description"].as_str(), Some("Say \"hi\": now")); + assert!(body.contains("/abs/.konductor/skills/alpha/SKILL.md")); + } + + /// The scope check lives in the server's `get_skill`, so the body must + /// send the agent there first, allow the file only when the server is + /// absent, and forbid it after a not-found answer. + #[test] + fn pointer_body_orders_get_skill_before_the_file_and_stops_on_not_found() { + let pointer = SkillPointer { + skill_name: "alpha".to_string(), + description: "d".to_string(), + }; + let rendered = render_pointer_md(&pointer, "/abs/.konductor/skills"); + let (_, body) = split_frontmatter(&rendered, "pointer").unwrap(); + let get_skill = body.find("Call `get_skill`").expect("step 1"); + let file = body + .find("read `/abs/.konductor/skills/alpha/SKILL.md`") + .expect("step 2"); + assert!(get_skill < file, "get_skill comes first: {body}"); + assert!( + body.contains("Only if the `konductor-skills` server is not connected"), + "{body}" + ); + assert!(body.contains("says the skill is not found, stop"), "{body}"); + assert!(body.contains("Do not read the file"), "{body}"); + assert!( + body.contains("If the tool is not loaded yet, load it"), + "{body}" + ); + assert!(!body.contains("deferred"), "{body}"); + assert!(!body.contains('~'), "fallback path is absolute: {body}"); + } + + #[test] + fn install_writes_one_pointer_per_described_skill_and_plan_agrees() { + let harness_dir = scratch_dir("install-harness"); + let target_dir = scratch_dir("install-target"); + stage_skill(&harness_dir, "alpha", &skill_md("alpha", "A.")); + stage_skill(&harness_dir, "bare", "no frontmatter\n"); + + let files = install_kiro_skill_pointers(&harness_dir, &target_dir, None).unwrap(); + let planned = plan_kiro_skill_pointer_files(&harness_dir, &target_dir, None).unwrap(); + + assert_eq!(paths(&files), [".kiro/skills/ks-alpha/SKILL.md"]); + assert_eq!(planned.len(), 1); + assert_eq!(planned[0].manifest_path, files[0].path); + assert!(target_dir.join(&files[0].path).is_file()); + assert!(!target_dir.join(".kiro/skills/ks-bare").exists()); + } + + #[test] + fn unreadable_skill_md_fails_the_install() { + let harness_dir = scratch_dir("unreadable-harness"); + let target_dir = scratch_dir("unreadable-target"); + // A directory named SKILL.md cannot be read as a file. + let dir = harness_dir.join(SKILLS_CONTENT_TYPE_DIR).join("alpha"); + std::fs::create_dir_all(dir.join(SKILL_FILE_NAME)).unwrap(); + + let err = install_kiro_skill_pointers(&harness_dir, &target_dir, None).unwrap_err(); + assert!(err.contains("failed to read"), "{err}"); + } + + #[test] + fn install_returns_nothing_when_no_skills_are_staged() { + let harness_dir = scratch_dir("empty-harness"); + let target_dir = scratch_dir("empty-target"); + assert!(install_kiro_skill_pointers(&harness_dir, &target_dir, None) + .unwrap() + .is_empty()); + } + + #[test] + fn unsafe_skill_directory_name_is_rejected() { + let harness_dir = scratch_dir("unsafe-harness"); + stage_skill(&harness_dir, "bad name", "---\ndescription: x\n---\n"); + assert!(list_skill_pointers(&harness_dir).is_err()); + } + + #[test] + fn dropped_skill_pointer_is_pruned_on_the_next_install() { + let harness_dir = scratch_dir("prune-harness"); + let target_dir = scratch_dir("prune-target"); + stage_skill(&harness_dir, "alpha", &skill_md("alpha", "A.")); + stage_skill(&harness_dir, "beta", &skill_md("beta", "B.")); + let first = install_kiro_skill_pointers(&harness_dir, &target_dir, None).unwrap(); + assert_eq!(first.len(), 2); + + std::fs::remove_dir_all(harness_dir.join(SKILLS_CONTENT_TYPE_DIR).join("beta")).unwrap(); + let second = + install_kiro_skill_pointers(&harness_dir, &target_dir, Some(&manifest_of(&first))) + .unwrap(); + + assert_eq!(paths(&second), [".kiro/skills/ks-alpha/SKILL.md"]); + assert!(target_dir.join(".kiro/skills/ks-alpha/SKILL.md").is_file()); + assert!(!target_dir.join(".kiro/skills/ks-beta").exists()); + } + + #[test] + fn prune_leaves_untracked_files_and_directories_alone() { + let harness_dir = scratch_dir("prune-foreign-harness"); + let target_dir = scratch_dir("prune-foreign-target"); + stage_skill(&harness_dir, "alpha", &skill_md("alpha", "A.")); + let first = install_kiro_skill_pointers(&harness_dir, &target_dir, None).unwrap(); + let foreign = target_dir.join(".kiro/skills/ks-alpha/notes.txt"); + std::fs::write(&foreign, "mine").unwrap(); + let other = target_dir.join(".kiro/skills/ks-other/SKILL.md"); + std::fs::create_dir_all(other.parent().unwrap()).unwrap(); + std::fs::write(&other, "not ours").unwrap(); + + std::fs::remove_dir_all(harness_dir.join(SKILLS_CONTENT_TYPE_DIR).join("alpha")).unwrap(); + install_kiro_skill_pointers(&harness_dir, &target_dir, Some(&manifest_of(&first))).unwrap(); + + assert!(!target_dir.join(".kiro/skills/ks-alpha/SKILL.md").exists()); + assert!( + foreign.is_file(), + "an untracked file in a pointer dir stays" + ); + assert!(other.is_file(), "an untracked pointer-like dir stays"); + } + + #[test] + fn opt_out_is_recorded_skips_pointers_and_prunes_the_old_ones() { + let harness_dir = scratch_dir("optout-harness"); + let target_dir = scratch_dir("optout-target"); + stage_skill(&harness_dir, "alpha", &skill_md("alpha", "A.")); + assert!(skill_pointers_enabled(&target_dir)); + let first = install_kiro_skill_pointers(&harness_dir, &target_dir, None).unwrap(); + assert_eq!(first.len(), 1); + + assert!(set_skill_pointers_opt_out(&target_dir, true).unwrap()); + assert!( + !set_skill_pointers_opt_out(&target_dir, true).unwrap(), + "no change" + ); + assert!(!skill_pointers_enabled(&target_dir)); + let second = + install_kiro_skill_pointers(&harness_dir, &target_dir, Some(&manifest_of(&first))) + .unwrap(); + assert!(second.is_empty()); + assert!(!target_dir.join(".kiro/skills/ks-alpha").exists()); + assert!( + plan_kiro_skill_pointer_files(&harness_dir, &target_dir, None) + .unwrap() + .is_empty() + ); + + assert!(set_skill_pointers_opt_out(&target_dir, false).unwrap()); + assert!(skill_pointers_enabled(&target_dir)); + assert_eq!( + install_kiro_skill_pointers(&harness_dir, &target_dir, None) + .unwrap() + .len(), + 1 + ); + } + + #[test] + fn removing_the_marker_twice_is_fine() { + let target_dir = scratch_dir("marker-remove"); + set_skill_pointers_opt_out(&target_dir, true).unwrap(); + remove_skill_pointers_opt_out(&target_dir).unwrap(); + remove_skill_pointers_opt_out(&target_dir).unwrap(); + assert!(skill_pointers_enabled(&target_dir)); + } +} diff --git a/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs b/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs index 53aa6918..342c8197 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs @@ -52,17 +52,18 @@ use std::path::Path; use super::kiro_cli::{ - attach_provenance, content_manifest_path, install_context, install_kiro_sop_skills, - install_skills, install_sops, list_agent_files, plan_additive_claude_sop_skill_files, - plan_all_files, plan_claude_hooks_file, plan_claude_settings_grant, read_skill_scopes_sidecar, - read_sop_scopes_sidecar, reject_unsafe_file_name, PlannedFile, KIRO_DESTINATION_ROOT, + attach_provenance, content_manifest_path, install_context, install_kiro_skill_pointers, + install_kiro_sop_skills, install_skills, install_sops, list_agent_files, + plan_additive_claude_sop_skill_files, plan_all_files, plan_claude_hooks_file, + plan_claude_settings_grant, read_skill_scopes_sidecar, read_sop_scopes_sidecar, + reject_unsafe_file_name, skill_pointers_enabled, PlannedFile, KIRO_DESTINATION_ROOT, KONDUCTOR_DESTINATION_ROOT, }; use super::manifest::{classify_provenance, ManifestFile, Provenance, Status, StrategyManifest}; use super::resource_rewrite::{ apply_all, apply_claude_settings_grant_and_hooks, apply_v3_standalone_telemetry_hook, - remove_v3_standalone_telemetry_hook, standard_passes_v3, RewriteContext, MCP_SERVER_NAME, - SKILL_RESOURCE_PREFIX, V3_STANDALONE_HOOKS_RELATIVE_PATH, + remove_v3_standalone_telemetry_hook, standard_passes_v3, with_skill_pointer_pass, + RewriteContext, MCP_SERVER_NAME, SKILL_RESOURCE_PREFIX, V3_STANDALONE_HOOKS_RELATIVE_PATH, }; use super::runtime::{detect_runtimes, Runtime}; use super::InstallError; @@ -315,6 +316,11 @@ impl InstallStrategy for KiroCliV3InstallStrategy { // Kiro-discoverable conversion, alongside the raw copy above -- // see `install_kiro_sop_skills`'s own doc comment. raw_files.extend(install_kiro_sop_skills(&harness_dir, target_dir)?); + raw_files.extend(install_kiro_skill_pointers( + &harness_dir, + target_dir, + prior_manifest.as_ref(), + )?); // Additive Claude branch, mirroring `SopInstallPhase::run`'s // dual-marker branch: this run is this strategy's own, but the // target also has a pre-existing `.claude` marker. Always @@ -566,7 +572,8 @@ fn copy_agent_files_rewriting_resources( ctx: &RewriteContext<'_>, file_names: Vec, ) -> Result<(Vec, bool), String> { - let passes = standard_passes_v3(); + let pointers_on = ctx.install_root().is_some_and(skill_pointers_enabled); + let passes = with_skill_pointer_pass(standard_passes_v3(), pointers_on); let mut files = Vec::with_capacity(file_names.len()); let mut any_mcp_server_injected = false; @@ -592,7 +599,12 @@ fn copy_agent_files_rewriting_resources( let contents = String::from_utf8(original_bytes.clone()) .map_err(|e| format!("{} is not valid UTF-8: {e}", src.display()))?; - if !contents.contains(super::kiro_cli::CONTEXT_RESOURCE_PREFIX) + // The pointer glob alone does not justify parsing: a file that is + // not JSON and needs no other rewrite stays a verbatim copy. + let needs_pointer_glob = + pointers_on && serde_json::from_str::(&contents).is_ok(); + if !needs_pointer_glob + && !contents.contains(super::kiro_cli::CONTEXT_RESOURCE_PREFIX) && !contents.contains(SKILL_RESOURCE_PREFIX) && !agent_has_sop_names && !agent_has_skill_names @@ -877,7 +889,10 @@ mod tests { serde_json::from_slice(&fs::read(&installed).unwrap()).unwrap(); assert_eq!( installed_json["resources"], - serde_json::json!([format!("file://{}", expected_context_path.display())]) + serde_json::json!([ + format!("file://{}", expected_context_path.display()), + "skill://.kiro/skills/ks-*/SKILL.md" + ]) ); let manifest = super::super::manifest::read_manifest(&target_dir) diff --git a/cli/konductor-rs/src/cli/install/phases.rs b/cli/konductor-rs/src/cli/install/phases.rs index e8ae952e..f8545b09 100644 --- a/cli/konductor-rs/src/cli/install/phases.rs +++ b/cli/konductor-rs/src/cli/install/phases.rs @@ -221,13 +221,14 @@ pub(super) fn run_all_phases( Ok(outputs.all_files()) } -/// The default phase list: all 5 artifact categories +/// The default phase list: all 6 artifact categories /// `install_from_local` handles today, in the order their real /// dependencies require (skills, the MCP binary, and context before /// agents). `SopInstallPhase` copies every staged `.sop.md` file into /// `.konductor/sops/` (and additively converts them into /// `.claude/skills/sop-/SKILL.md` when a `.claude` marker -/// already exists at the target). Adding a sixth phase is a new +/// already exists at the target). `SkillPointerInstallPhase` writes the +/// `.kiro/skills/ks-/SKILL.md` pointers. Adding a seventh phase is a new /// `struct FooInstallPhase` plus one `Box::new(FooInstallPhase)` line /// here. pub(super) fn standard_install_phases() -> Vec> { @@ -235,6 +236,7 @@ pub(super) fn standard_install_phases() -> Vec> { Box::new(SkillInstallPhase), Box::new(McpInstallPhase), Box::new(SopInstallPhase), + Box::new(SkillPointerInstallPhase), Box::new(ContextInstallPhase), Box::new(AgentInstallPhase), ] @@ -404,6 +406,36 @@ impl InstallPhase for SopInstallPhase { } } +// ── Phase: skill pointers ─────────────────────────────────────────────── + +/// Writes the `.kiro/skills/ks-/SKILL.md` pointers (see +/// `kiro_cli::skill_pointers`) for the Kiro chain and prunes the ones a +/// prior install tracked that this run no longer writes. Only in the Kiro +/// chain: Claude installs get no pointers. +pub(super) struct SkillPointerInstallPhase; + +impl InstallPhase for SkillPointerInstallPhase { + fn name(&self) -> &'static str { + "skill-pointers" + } + + fn run( + &self, + staged_root: &Path, + target_dir: &Path, + _repo_root: Option<&Path>, + prior_manifest: Option<&StrategyManifest>, + _phase_outputs: &PhaseOutputs, + _no_telemetry: bool, + ) -> Result, InstallError> { + Ok(super::kiro_cli::install_kiro_skill_pointers( + staged_root, + target_dir, + prior_manifest, + )?) + } +} + // ── Phase: context ────────────────────────────────────────────────────── /// Wraps the existing `install_context` logic: copies synthed context @@ -674,10 +706,10 @@ mod tests { } #[test] - fn standard_install_phases_returns_five_phases_with_agents_last() { + fn standard_install_phases_returns_six_phases_with_agents_last() { let phases = standard_install_phases(); let names: Vec<&str> = phases.iter().map(|p| p.name()).collect(); - assert_eq!(names.len(), 5); + assert_eq!(names.len(), 6); assert_eq!( names.last().copied(), Some("agents"), @@ -687,6 +719,7 @@ mod tests { assert!(names.contains(&"skills")); assert!(names.contains(&"mcp")); assert!(names.contains(&"sops")); + assert!(names.contains(&"skill-pointers")); assert!(names.contains(&"context")); let agent_index = names.iter().position(|n| *n == "agents").unwrap(); let skills_index = names.iter().position(|n| *n == "skills").unwrap(); diff --git a/cli/konductor-rs/src/cli/install/resource_rewrite/mod.rs b/cli/konductor-rs/src/cli/install/resource_rewrite/mod.rs index cdb62ce5..2873f18f 100644 --- a/cli/konductor-rs/src/cli/install/resource_rewrite/mod.rs +++ b/cli/konductor-rs/src/cli/install/resource_rewrite/mod.rs @@ -28,6 +28,7 @@ use std::path::Path; +use super::kiro_cli::SKILL_POINTER_RESOURCE; use super::manifest::ManifestFile; mod claude_settings; @@ -374,6 +375,64 @@ impl ResourceRewritePass for SkillResourcePass { } } +// ── Pass 2b: skill pointer resource ───────────────────────────────────── + +/// Adds `skill://.kiro/skills/ks-*/SKILL.md` to an agent's `resources` +/// so Kiro lists the `ks-` pointers for that agent explicitly. Only +/// in the pipeline when the target writes pointers (see +/// `with_skill_pointer_pass`). The glob contains `*`, so +/// `agent_needs_mcp_server` ignores it, like the `ws-*` glob. +pub(super) struct SkillPointerResourcePass; + +impl ResourceRewritePass for SkillPointerResourcePass { + fn matches(&self, value: &serde_json::Value, _ctx: &RewriteContext<'_>) -> bool { + value.is_object() + && value + .get("resources") + .is_none_or(serde_json::Value::is_array) + } + + fn rewrite(&self, value: &mut serde_json::Value, _ctx: &RewriteContext<'_>) { + push_unique_str(value, "resources", SKILL_POINTER_RESOURCE); + } + + fn verify( + &self, + value: &serde_json::Value, + _ctx: &RewriteContext<'_>, + agent_file: &Path, + ) -> Result<(), String> { + let listed = value + .get("resources") + .and_then(|r| r.as_array()) + .is_some_and(|resources| { + resources + .iter() + .any(|entry| entry.as_str() == Some(SKILL_POINTER_RESOURCE)) + }); + if listed { + return Ok(()); + } + Err(format!( + "agent '{}' is missing the '{SKILL_POINTER_RESOURCE}' resource", + agent_file.display() + )) + } +} + +/// Inserts `SkillPointerResourcePass` right after `SkillResourcePass` +/// when `enabled`; otherwise returns `passes` unchanged. +pub(super) fn with_skill_pointer_pass( + mut passes: Vec>, + enabled: bool, +) -> Vec> { + if enabled { + let after_skill_pass = passes.len().min(2); + passes.insert(after_skill_pass, Box::new(SkillPointerResourcePass)); + } + passes +} + /// The V3 (KAS) ordered pipeline: reuses `ContextResourcePass` and /// `SkillResourcePass` unchanged (V3's `resources` field is rendered /// identically to V2's), swapping only the MCP-wiring pass for @@ -768,4 +827,111 @@ mod tests { ); fs::remove_dir_all(&dir).ok(); } + + // ── SkillPointerResourcePass ──────────────────────────────────── + + #[test] + fn pointer_pass_adds_the_glob_once_to_an_agent_with_resources() { + let dir = scratch_dir("pointer-pass-add"); + let mut value = serde_json::json!({"resources": ["skill://.kiro/skills/ws-*/SKILL.md"]}); + let passes = with_skill_pointer_pass(Vec::new(), true); + let ctx = ctx(&dir, &dir, &dir, &[]); + for _ in 0..2 { + apply_all(&passes, &mut value, &ctx, Path::new("/tmp/agent.json")).unwrap(); + } + assert_eq!( + value["resources"], + serde_json::json!([ + "skill://.kiro/skills/ws-*/SKILL.md", + "skill://.kiro/skills/ks-*/SKILL.md" + ]) + ); + fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn pointer_pass_creates_resources_when_absent_and_skips_non_array_resources() { + let dir = scratch_dir("pointer-pass-create"); + let ctx = ctx(&dir, &dir, &dir, &[]); + let mut bare = serde_json::json!({"name": "k-example"}); + assert!(SkillPointerResourcePass.matches(&bare, &ctx)); + SkillPointerResourcePass.rewrite(&mut bare, &ctx); + assert_eq!( + bare["resources"], + serde_json::json!(["skill://.kiro/skills/ks-*/SKILL.md"]) + ); + + let odd = serde_json::json!({"resources": "not-an-array"}); + assert!(!SkillPointerResourcePass.matches(&odd, &ctx)); + fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn pointer_pass_verify_fails_when_the_glob_is_missing() { + let dir = scratch_dir("pointer-pass-verify"); + let err = SkillPointerResourcePass + .verify( + &serde_json::json!({"resources": []}), + &ctx(&dir, &dir, &dir, &[]), + Path::new("/tmp/agent.json"), + ) + .unwrap_err(); + assert!(err.contains("ks-*"), "{err}"); + fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn pointer_pass_is_present_only_when_enabled_and_follows_the_skill_pass() { + assert_eq!( + with_skill_pointer_pass(standard_passes(true), false).len(), + 3 + ); + assert_eq!( + with_skill_pointer_pass(standard_passes(true), true).len(), + 4 + ); + assert_eq!(with_skill_pointer_pass(standard_passes_v3(), true).len(), 4); + + // Running the real pipeline: the glob lands after the rewritten + // skill entry, and the MCP pass still sees only the real skill. + let dir = scratch_dir("pointer-pass-order"); + let skills_dir = dir.join("skills"); + fs::create_dir_all(skills_dir.join("constraints")).unwrap(); + fs::write(skills_dir.join("constraints/SKILL.md"), b"body").unwrap(); + let mut value = serde_json::json!({"resources": ["skill://skills/constraints/SKILL.md"]}); + let passes = with_skill_pointer_pass(standard_passes(true), true); + apply_all( + &passes, + &mut value, + &ctx(&dir.join("context"), &skills_dir, &dir.join("bin"), &[]), + Path::new("/tmp/agent.json"), + ) + .unwrap(); + let resources = value["resources"].as_array().unwrap(); + assert_eq!(resources.len(), 2); + assert_eq!( + resources[1].as_str(), + Some("skill://.kiro/skills/ks-*/SKILL.md") + ); + fs::remove_dir_all(&dir).ok(); + } + + /// A pointer glob alone gives an agent no `konductor-skills` MCP + /// server: only a real skill resource or scoping does. + #[test] + fn pointer_glob_alone_does_not_trigger_the_mcp_server_grant() { + let dir = scratch_dir("pointer-pass-no-mcp"); + let mut value = serde_json::json!({"name": "k-example"}); + let bin_files = [bin_file_entry()]; + let passes = with_skill_pointer_pass(standard_passes(true), true); + apply_all( + &passes, + &mut value, + &ctx(&dir, &dir, &dir, &bin_files), + Path::new("/tmp/agent.json"), + ) + .unwrap(); + assert!(value.get("mcpServers").is_none(), "{value:?}"); + fs::remove_dir_all(&dir).ok(); + } } diff --git a/cli/konductor-rs/src/cli/synth/parse_canonical.rs b/cli/konductor-rs/src/cli/synth/parse_canonical.rs index 70c2cf62..d63c3575 100644 --- a/cli/konductor-rs/src/cli/synth/parse_canonical.rs +++ b/cli/konductor-rs/src/cli/synth/parse_canonical.rs @@ -671,7 +671,10 @@ fn parse_skill_dir(dir: &Path, skill_md: &Path, dir_name: &str) -> Result(raw: &'a str, file_label: &str) -> Result<(&'a str, &'a str), ParseError> { +pub(crate) fn split_frontmatter<'a>( + raw: &'a str, + file_label: &str, +) -> Result<(&'a str, &'a str), ParseError> { let rest = raw.strip_prefix("---").ok_or_else(|| ParseError { file: file_label.to_string(), field: None, diff --git a/cli/konductor-rs/src/cli/uninstall.rs b/cli/konductor-rs/src/cli/uninstall.rs index 43d2e028..6c20ed96 100644 --- a/cli/konductor-rs/src/cli/uninstall.rs +++ b/cli/konductor-rs/src/cli/uninstall.rs @@ -1256,6 +1256,14 @@ fn uninstall_one_impl( ); } } + if let Err(err) = crate::cli::install::kiro_cli::remove_skill_pointers_opt_out(target_path) + { + eprintln!( + "konductor uninstall: warning: could not remove the --no-skill-pointers marker \ + under {}: {err}", + target_path.display() + ); + } let identity_path = crate::cli::telemetry::identity_path(target_path); if let Err(err) = std::fs::remove_file(&identity_path) { if err.kind() != std::io::ErrorKind::NotFound { diff --git a/cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs b/cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs new file mode 100644 index 00000000..49fbe013 --- /dev/null +++ b/cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs @@ -0,0 +1,512 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// kiro_skill_pointers_e2e.rs — drives the REAL compiled binary through +// synth, install, update and uninstall to prove the Kiro harnesses write +// a `ks-/SKILL.md` pointer per skill under `.kiro/skills/`, track +// it in the install manifest, and remove it on uninstall. +// +// Isolation: every invocation runs with `HOME` set to a scratch dir, and +// every test holds a `RealInstallsGuard` that fails the test if the +// runner's real `~/.konductor/installs` changes while it runs. The guard +// watches only that index, so it is a tripwire: it also fails if someone +// runs a real install on the same machine while the tests run. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +const POINTER_PREFIX: &str = "ks-"; +const KIRO_HARNESSES: [&str; 2] = ["kiro-cli-v2", "kiro-v3"]; + +const POINTER_GLOB: &str = "skill://.kiro/skills/ks-*/SKILL.md"; + +const PLAIN_DESCRIPTION: &str = "Use when: a \"quoted\" thing, with a colon."; +const FOLDED_DESCRIPTION: &str = "First line second line"; + +fn bin() -> &'static str { + env!("CARGO_BIN_EXE_konductor") +} + +fn scratch_dir(name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "konductor-kiro-skill-pointers-e2e-{name}-{}", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +/// Fails its test when the file it watches changes while it lives. +struct RealInstallsGuard { + path: PathBuf, + before: Option>, +} + +impl RealInstallsGuard { + fn watch(path: PathBuf) -> Self { + let before = std::fs::read(&path).ok(); + Self { path, before } + } + + /// Watches the install index under the test runner's real `HOME`. + fn real() -> Self { + let home = std::env::var_os("HOME").expect("HOME must be set for the runner"); + Self::watch(PathBuf::from(home).join(".konductor/installs")) + } + + fn check(&self) -> Result<(), String> { + let now = std::fs::read(&self.path).ok(); + if now == self.before { + return Ok(()); + } + Err(format!("{} was modified by a test", self.path.display())) + } +} + +impl Drop for RealInstallsGuard { + fn drop(&mut self) { + if !std::thread::panicking() { + self.check().unwrap(); + } + } +} + +fn run_konductor(home: &Path, sink: &telemetry_test_sink::TelemetrySink, args: &[&str]) -> Output { + let mut command = Command::new(bin()); + command.args(args).current_dir(home).env("HOME", home); + for var in sink.env_vars() { + command.env(var.name, &var.value); + } + let output = command.output().expect("failed to spawn konductor binary"); + assert!( + output.status.success(), + "`konductor {}` failed: {}", + args.join(" "), + String::from_utf8_lossy(&output.stderr) + ); + output +} + +fn seed_repo(repo_root: &Path, agent_client_config: &str) { + let agents_dir = repo_root.join("agents"); + std::fs::create_dir_all(&agents_dir).unwrap(); + std::fs::write( + agents_dir.join("k-example.agent-spec.json"), + format!( + r#"{{"schemaVersion":"1","name":"k-example", +"config":{{"description":"An example agent.","model":"claude-sonnet-5","systemPrompt":"Hi."}}, +"clientConfig":{{"{agent_client_config}":{{}}}}}}"# + ), + ) + .unwrap(); + seed_skill( + repo_root, + "alpha", + &format!("'{}'", PLAIN_DESCRIPTION.replace('\'', "''")), + ); + seed_skill(repo_root, "beta", ">-\n First line\n second line"); +} + +fn seed_skill(repo_root: &Path, name: &str, description_yaml: &str) { + let dir = repo_root.join("skills").join(name); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("SKILL.md"), + format!("---\nname: {name}\ndescription: {description_yaml}\n---\n\n# {name}\n\nBody.\n"), + ) + .unwrap(); +} + +fn synthed_repo(home: &Path, sink: &telemetry_test_sink::TelemetrySink, client: &str) -> PathBuf { + let repo_root = scratch_dir("repo"); + seed_repo(&repo_root, client); + run_konductor( + home, + sink, + &["synth", "--from", &repo_root.display().to_string()], + ); + repo_root +} + +fn install( + home: &Path, + sink: &telemetry_test_sink::TelemetrySink, + repo: &Path, + target: &Path, + harness: &str, +) -> Output { + install_with(home, sink, repo, target, harness, &[]) +} + +fn install_with( + home: &Path, + sink: &telemetry_test_sink::TelemetrySink, + repo: &Path, + target: &Path, + harness: &str, + extra: &[&str], +) -> Output { + let repo = repo.display().to_string(); + let target = target.display().to_string(); + let mut args = vec![ + "install", + "--from", + &repo, + "--target", + &target, + "--harness", + harness, + ]; + args.extend_from_slice(extra); + run_konductor(home, sink, &args) +} + +fn update(home: &Path, sink: &telemetry_test_sink::TelemetrySink, repo: &Path, target: &Path) { + run_konductor( + home, + sink, + &[ + "update", + "--from", + &repo.display().to_string(), + "--target", + &target.display().to_string(), + "--force", + ], + ); +} + +fn uninstall(home: &Path, sink: &telemetry_test_sink::TelemetrySink, target: &Path, harness: &str) { + run_konductor( + home, + sink, + &[ + "uninstall", + "--target", + &target.display().to_string(), + "--harness", + harness, + ], + ); +} + +fn agent_resources(target: &Path) -> Vec { + let raw = std::fs::read_to_string(target.join(".kiro/agents/k-example.json")).unwrap(); + let value: serde_json::Value = serde_json::from_str(&raw).unwrap(); + value["resources"] + .as_array() + .map(|r| { + r.iter() + .filter_map(|e| e.as_str().map(String::from)) + .collect() + }) + .unwrap_or_default() +} + +fn has_pointer_dirs(target: &Path) -> bool { + std::fs::read_dir(target.join(".kiro/skills")) + .map(|entries| { + entries + .flatten() + .any(|e| e.file_name().to_string_lossy().starts_with(POINTER_PREFIX)) + }) + .unwrap_or(false) +} + +fn pointer_path(target: &Path, skill: &str) -> PathBuf { + target + .join(".kiro/skills") + .join(format!("{POINTER_PREFIX}{skill}")) + .join("SKILL.md") +} + +/// Splits a pointer into its parsed frontmatter and its body. +fn read_pointer(path: &Path) -> (serde_yaml::Value, String) { + let raw = std::fs::read_to_string(path) + .unwrap_or_else(|e| panic!("missing pointer {}: {e}", path.display())); + let rest = raw + .strip_prefix("---\n") + .expect("pointer starts with frontmatter"); + let (frontmatter, body) = rest.split_once("\n---\n").expect("frontmatter is closed"); + (serde_yaml::from_str(frontmatter).unwrap(), body.to_string()) +} + +fn assert_pointer(target: &Path, skill: &str, description: &str, fallback_dir: &str) { + let (frontmatter, body) = read_pointer(&pointer_path(target, skill)); + assert_eq!( + frontmatter["name"].as_str(), + Some(format!("{POINTER_PREFIX}{skill}").as_str()) + ); + assert_eq!(frontmatter["description"].as_str(), Some(description)); + assert!( + body.contains("get_skill"), + "body names the MCP tool: {body}" + ); + assert!( + body.contains("konductor-skills"), + "body names the MCP server: {body}" + ); + assert!( + body.contains(&format!("name: \"{skill}\"")), + "body names the real skill: {body}" + ); + let fallback = format!("{fallback_dir}/{skill}/SKILL.md"); + assert!( + body.contains(&fallback), + "body carries the fallback path {fallback}: {body}" + ); +} + +fn manifest_text(target: &Path) -> String { + std::fs::read_to_string(target.join(".konductor/manifest")).unwrap() +} + +#[test] +fn kiro_install_writes_tracked_pointers_and_uninstall_removes_them() { + let _real = RealInstallsGuard::real(); + let sink = telemetry_test_sink::TelemetrySink::start(); + for harness in KIRO_HARNESSES { + let home = scratch_dir(&format!("home-{harness}")); + let repo = synthed_repo(&home, &sink, "kiroCli"); + + install(&home, &sink, &repo, &home, harness); + + assert!( + home.join(".konductor/installs").is_file(), + "{harness}: index went to the scratch HOME" + ); + let fallback_dir = home.join(".konductor/skills").display().to_string(); + assert_pointer(&home, "alpha", PLAIN_DESCRIPTION, &fallback_dir); + assert_pointer(&home, "beta", FOLDED_DESCRIPTION, &fallback_dir); + assert!( + home.join(".konductor/skills/alpha/SKILL.md").is_file(), + "{harness}: the fallback file the pointer names exists" + ); + let manifest = manifest_text(&home); + assert!( + manifest.contains(".kiro/skills/ks-alpha/SKILL.md"), + "{harness}: tracked: {manifest}" + ); + assert!( + manifest.contains(".kiro/skills/ks-beta/SKILL.md"), + "{harness}: tracked: {manifest}" + ); + + std::fs::remove_file(pointer_path(&home, "alpha")).unwrap(); + update(&home, &sink, &repo, &home); + assert_pointer(&home, "alpha", PLAIN_DESCRIPTION, &fallback_dir); + + uninstall(&home, &sink, &home, harness); + assert!( + !pointer_path(&home, "alpha").exists(), + "{harness}: pointer removed" + ); + assert!( + !pointer_path(&home, "beta").exists(), + "{harness}: pointer removed" + ); + assert!( + !home.join(".kiro/skills/ks-alpha").exists(), + "{harness}: pointer dir removed" + ); + + std::fs::remove_dir_all(&home).ok(); + std::fs::remove_dir_all(&repo).ok(); + } +} + +#[test] +fn pointer_fallback_path_follows_a_target_that_is_not_home() { + let _real = RealInstallsGuard::real(); + let sink = telemetry_test_sink::TelemetrySink::start(); + let home = scratch_dir("home-other-target"); + let target = scratch_dir("other-target"); + let repo = synthed_repo(&home, &sink, "kiroCli"); + + install(&home, &sink, &repo, &target, "kiro-v3"); + + let fallback_dir = target.join(".konductor/skills").display().to_string(); + assert_pointer(&target, "alpha", PLAIN_DESCRIPTION, &fallback_dir); + assert!(Path::new(&fallback_dir).join("alpha/SKILL.md").is_file()); + + std::fs::remove_dir_all(&home).ok(); + std::fs::remove_dir_all(&target).ok(); + std::fs::remove_dir_all(&repo).ok(); +} + +#[test] +fn every_kiro_agent_lists_the_pointer_glob_and_the_install_count_excludes_pointers() { + let _real = RealInstallsGuard::real(); + let sink = telemetry_test_sink::TelemetrySink::start(); + for harness in KIRO_HARNESSES { + let home = scratch_dir(&format!("home-glob-{harness}")); + let repo = synthed_repo(&home, &sink, "kiroCli"); + + let output = install(&home, &sink, &repo, &home, harness); + + assert!( + agent_resources(&home).contains(&POINTER_GLOB.to_string()), + "{harness}: agent lists the glob: {:?}", + agent_resources(&home) + ); + let summary = String::from_utf8_lossy(&output.stdout).to_string() + + &String::from_utf8_lossy(&output.stderr); + assert!( + summary.contains("2 skill(s)"), + "{harness}: two skills, pointers not counted: {summary}" + ); + + // `update` keeps the glob. + update(&home, &sink, &repo, &home); + assert!(agent_resources(&home).contains(&POINTER_GLOB.to_string())); + + std::fs::remove_dir_all(&home).ok(); + std::fs::remove_dir_all(&repo).ok(); + } +} + +#[test] +fn no_skill_pointers_is_recorded_survives_update_and_a_plain_install_turns_it_back_on() { + let _real = RealInstallsGuard::real(); + let sink = telemetry_test_sink::TelemetrySink::start(); + for harness in KIRO_HARNESSES { + let home = scratch_dir(&format!("home-optout-{harness}")); + let repo = synthed_repo(&home, &sink, "kiroCli"); + + install(&home, &sink, &repo, &home, harness); + assert!(has_pointer_dirs(&home), "{harness}: pointers by default"); + + // Opting out removes the pointers and the glob. + install_with( + &home, + &sink, + &repo, + &home, + harness, + &["--no-skill-pointers"], + ); + assert!(!has_pointer_dirs(&home), "{harness}: pointers removed"); + assert!(!agent_resources(&home).contains(&POINTER_GLOB.to_string())); + assert!( + !manifest_text(&home).contains("ks-"), + "{harness}: untracked" + ); + assert!( + home.join(".konductor/skills/alpha/SKILL.md").is_file(), + "{harness}: skill bodies are untouched" + ); + + // `update` respects the recorded choice. + update(&home, &sink, &repo, &home); + assert!( + !has_pointer_dirs(&home), + "{harness}: update kept the opt-out" + ); + assert!(!agent_resources(&home).contains(&POINTER_GLOB.to_string())); + + // A plain install turns pointers back on. + install(&home, &sink, &repo, &home, harness); + assert!(has_pointer_dirs(&home), "{harness}: pointers back"); + assert!(agent_resources(&home).contains(&POINTER_GLOB.to_string())); + + // Opt out again, then uninstall clears the marker. + install_with( + &home, + &sink, + &repo, + &home, + harness, + &["--no-skill-pointers"], + ); + assert!(home.join(".konductor/no-skill-pointers").is_file()); + uninstall(&home, &sink, &home, harness); + assert!( + !home.join(".konductor/no-skill-pointers").exists(), + "{harness}: uninstall removes the marker" + ); + + std::fs::remove_dir_all(&home).ok(); + std::fs::remove_dir_all(&repo).ok(); + } +} + +#[test] +fn update_prunes_the_pointer_of_a_skill_the_source_dropped() { + let _real = RealInstallsGuard::real(); + let sink = telemetry_test_sink::TelemetrySink::start(); + for harness in KIRO_HARNESSES { + let home = scratch_dir(&format!("home-prune-{harness}")); + let repo = synthed_repo(&home, &sink, "kiroCli"); + install(&home, &sink, &repo, &home, harness); + assert!(pointer_path(&home, "beta").is_file()); + + std::fs::remove_dir_all(repo.join("skills/beta")).unwrap(); + run_konductor( + &home, + &sink, + &["synth", "--from", &repo.display().to_string()], + ); + update(&home, &sink, &repo, &home); + + assert!(pointer_path(&home, "alpha").is_file(), "{harness}: kept"); + assert!( + !home.join(".kiro/skills/ks-beta").exists(), + "{harness}: dropped skill's pointer pruned" + ); + assert!(!manifest_text(&home).contains("ks-beta")); + + std::fs::remove_dir_all(&home).ok(); + std::fs::remove_dir_all(&repo).ok(); + } +} + +#[test] +fn claude_install_writes_no_pointers() { + let _real = RealInstallsGuard::real(); + let sink = telemetry_test_sink::TelemetrySink::start(); + let home = scratch_dir("home-claude"); + let repo = synthed_repo(&home, &sink, "claudeCli"); + + install_with( + &home, + &sink, + &repo, + &home, + "claude", + &["--no-skill-pointers"], + ); + + assert!( + home.join(".claude/skills/alpha/SKILL.md").is_file(), + "claude keeps its own skills" + ); + assert!( + !home.join(".konductor/no-skill-pointers").exists(), + "the flag records nothing for a Claude install" + ); + assert!( + !home.join(".kiro/skills").exists(), + "no Kiro pointers on a Claude install" + ); + + std::fs::remove_dir_all(&home).ok(); + std::fs::remove_dir_all(&repo).ok(); +} + +#[test] +fn installs_guard_fails_when_the_watched_file_changes() { + let dir = scratch_dir("guard"); + let watched = dir.join("installs"); + std::fs::write(&watched, b"original").unwrap(); + let guard = RealInstallsGuard::watch(watched.clone()); + assert!(guard.check().is_ok()); + + std::fs::write(&watched, b"touched").unwrap(); + assert!(guard.check().is_err()); + + std::fs::write(&watched, b"original").unwrap(); + drop(guard); + std::fs::remove_dir_all(&dir).ok(); +} diff --git a/cli/konductor-rs/tests/synth_install_e2e.rs b/cli/konductor-rs/tests/synth_install_e2e.rs index ca141a8b..9ab4b16f 100644 --- a/cli/konductor-rs/tests/synth_install_e2e.rs +++ b/cli/konductor-rs/tests/synth_install_e2e.rs @@ -221,10 +221,17 @@ fn real_synth_then_real_install_agree_on_output_path_and_bytes() { let mut installed_value: serde_json::Value = serde_json::from_slice(&installed_bytes).unwrap(); synth_value.as_object_mut().unwrap().remove("hooks"); installed_value.as_object_mut().unwrap().remove("hooks"); + // Install also lists the `ks-*` pointer glob; nothing else in `resources` changes. + assert_eq!( + installed_value["resources"], + serde_json::json!(["skill://.kiro/skills/ks-*/SKILL.md"]) + ); + synth_value.as_object_mut().unwrap().remove("resources"); + installed_value.as_object_mut().unwrap().remove("resources"); assert_eq!( installed_value, synth_value, - "every field other than \"hooks\" must still agree between synth's pristine output and \ - install's rewritten copy" + "every field other than \"hooks\" and \"resources\" must still agree between synth's \ + pristine output and install's rewritten copy" ); let installed_full: serde_json::Value = serde_json::from_slice(&installed_bytes).unwrap(); assert!( @@ -281,11 +288,11 @@ fn real_synth_then_real_install_agree_on_output_path_and_bytes() { "synth's real skill output file must not be empty" ); - let installed_skill_files = find_files_named(&target_dir, "SKILL.md"); + let installed_skill_files = find_files_named(&target_dir.join(".konductor/skills"), "SKILL.md"); assert_eq!( installed_skill_files.len(), 1, - "expected exactly one installed SKILL.md somewhere under {}, found: {:?}", + "expected exactly one installed SKILL.md under {}/.konductor/skills, found: {:?}", target_dir.display(), installed_skill_files ); @@ -756,7 +763,7 @@ fn real_synth_then_real_install_rewrites_context_resource_to_absolute_path() { ); assert_eq!( installed_value["resources"], - serde_json::json!([expected_absolute]), + serde_json::json!([expected_absolute, "skill://.kiro/skills/ks-*/SKILL.md"]), "installed agent's context resource must be rewritten to an absolute path" ); @@ -795,7 +802,7 @@ fn real_synth_then_real_install_rewrites_context_resource_to_absolute_path() { serde_json::from_slice(&std::fs::read(&installed_files[0]).unwrap()).unwrap(); assert_eq!( reinstalled_value["resources"], - serde_json::json!([expected_absolute]), + serde_json::json!([expected_absolute, "skill://.kiro/skills/ks-*/SKILL.md"]), "a second install must not duplicate or double-rewrite the context resource entry" ); @@ -927,13 +934,13 @@ fn real_synth_then_real_install_scopes_skill_resources_to_konductor_skills() { ); assert_eq!( value_a["resources"], - serde_json::json!([expected_a]), - "agent-a must reference only skill-a" + serde_json::json!([expected_a, "skill://.kiro/skills/ks-*/SKILL.md"]), + "agent-a must reference only skill-a (plus the pointer glob)" ); assert_eq!( value_b["resources"], - serde_json::json!([expected_b]), - "agent-b must reference only skill-b" + serde_json::json!([expected_b, "skill://.kiro/skills/ks-*/SKILL.md"]), + "agent-b must reference only skill-b (plus the pointer glob)" ); assert!(target_dir @@ -942,7 +949,13 @@ fn real_synth_then_real_install_scopes_skill_resources_to_konductor_skills() { assert!(target_dir .join(".konductor/skills/skill-b/SKILL.md") .is_file()); - assert!(!target_dir.join(".kiro/skills").exists()); + // Skill bodies stay out of `.kiro/skills/`; only the `ks-` pointers land there. + let mut kiro_skill_dirs: Vec = std::fs::read_dir(target_dir.join(".kiro/skills")) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + kiro_skill_dirs.sort(); + assert_eq!(kiro_skill_dirs, ["ks-skill-a", "ks-skill-b"]); std::fs::remove_dir_all(&repo_root).ok(); std::fs::remove_dir_all(&target_dir).ok(); diff --git a/docs/user-guide/agents.md b/docs/user-guide/agents.md index 0e9dcd43..c37586c3 100644 --- a/docs/user-guide/agents.md +++ b/docs/user-guide/agents.md @@ -375,7 +375,7 @@ images, and diagrams — not a skill library. Point it at a file and ask a quest | Permission model | two lists: `tools` grants with a prompt, `allowedTools` pre-approves | one list: `clientConfig.claudeCli.tools` is the ceiling, and your `settings.json` decides what prompts | | `k-browser` / `k-media-analyzer` | reads pre-approved; writes and shell ask first | write and shell granted | | `k-product-manager` / `k-tpm` | shell asks first | shell granted | -| Skills resolution | through the bundled `skill-lookup` MCP server. Skills install to `.konductor/skills/`, deliberately outside the `.kiro/skills/` tree Kiro CLI scans, and the agent reaches them through that server | native Claude Code skills: `clientConfig.claudeCli.skills` names the set, and each body installs to `.claude/skills//SKILL.md` | +| Skills resolution | through the bundled `skill-lookup` MCP server. Skills install to `.konductor/skills/`, deliberately outside the `.kiro/skills/` tree Kiro CLI scans, and the agent reaches them through that server. A small `ks-` pointer per skill in `.kiro/skills/` makes each one listed natively | native Claude Code skills: `clientConfig.claudeCli.skills` names the set, and each body installs to `.claude/skills//SKILL.md` | | Extra setup | none | agent-teams env var **and** a permissions allowlist | The delegation row is the one that changes behaviour most: a workflow that relies on one diff --git a/docs/user-guide/appendix/contributing.md b/docs/user-guide/appendix/contributing.md index 4ce41b23..bdaec55e 100644 --- a/docs/user-guide/appendix/contributing.md +++ b/docs/user-guide/appendix/contributing.md @@ -67,7 +67,8 @@ list `konductor synth` reads for either harness, and the Claude Code skill list: There is no per-skill entry under `clientConfig.kiroCli.resources`. Kiro CLI reaches installed skills through the bundled `skill-lookup` MCP server, not through its own `.kiro/skills/` scan — see [Runtime differences that matter](../agents.md#runtime-differences-that-matter). The one -`skill://` resource each spec does carry is the `ws-*` glob for workspace skills. +`skill://` resource each spec does carry is the `ws-*` glob for workspace skills. `install` adds +one more, the `ks-*` glob that lists the skill pointers; don't add it to a spec. Append to the existing arrays rather than replacing them. A skill directory that exists under `skills/` but is declared by no agent cannot be loaded by anything — see diff --git a/docs/user-guide/reference.md b/docs/user-guide/reference.md index 0845a294..d530b10a 100644 --- a/docs/user-guide/reference.md +++ b/docs/user-guide/reference.md @@ -94,6 +94,7 @@ Accepted before or after a subcommand. | `install` | `--target ` | path | No | Destination. Defaults to `$HOME` | | `install` | `--link-bin` | flag | No | Symlink `konductor` into `$HOME/.local/bin` | | `install` | `--no-telemetry` | flag | No | Suppress telemetry for this invocation | +| `install` | `--no-skill-pointers` | flag | No | Kiro harnesses: skip the `ks-` skill pointers in `.kiro/skills/` and remove ones already written. Recorded for the target, so `update` keeps it | | `install` | `--use-github-token` | flag | No | Authenticate the release fetch with a GitHub token | | `update` | `--from ` | path | No | Source repo root. No default — pass it every time | | `update` | `--target ` | path | No | Which tracked install to update. Conflicts with `--all` |