From fa8cd0fee7221771ea34fc5a46f716786c21fe3b Mon Sep 17 00:00:00 2001 From: Nihit <24500178+knihit@users.noreply.github.com> Date: Tue, 27 Oct 2026 19:50:25 +0000 Subject: [PATCH 1/4] feat(install): write native skill pointers for the Kiro harnesses Each skill gets a small ks-/SKILL.md under .kiro/skills/ carrying its name and description, so Kiro lists it natively. The pointer loads the body through the konductor-skills get_skill tool and falls back to reading .konductor/skills//SKILL.md. Pointers are tracked in the manifest; Claude installs are unchanged. --- CHANGELOG.md | 8 + README.md | 5 +- cli/README.md | 8 +- cli/konductor-rs/src/cli/install/claude.rs | 2 +- cli/konductor-rs/src/cli/install/kiro_cli.rs | 13 +- .../src/cli/install/kiro_cli/plan.rs | 9 +- .../cli/install/kiro_cli/skill_pointers.rs | 239 +++++++++++++ .../src/cli/install/kiro_cli_v3.rs | 11 +- cli/konductor-rs/src/cli/install/phases.rs | 4 + .../src/cli/synth/parse_canonical.rs | 5 +- .../tests/kiro_skill_pointers_e2e.rs | 328 ++++++++++++++++++ cli/konductor-rs/tests/synth_install_e2e.rs | 12 +- 12 files changed, 626 insertions(+), 18 deletions(-) create mode 100644 cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs create mode 100644 cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 01b8a6a02..bd8e9fd26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ not per individual commit. ## [Unreleased] +### Added + +- Kiro installs (`kiro-cli-v2`, `kiro-v3`) write a small `ks-/SKILL.md` pointer per skill + into `.kiro/skills/`, so Kiro lists every skill by name and description. The body still loads + through the `konductor-skills` MCP server's `get_skill`, so skill-load telemetry is unchanged; + if that tool is unavailable the pointer falls back to reading `.konductor/skills//SKILL.md`. + Pointers are tracked in the manifest and removed by `uninstall`. Claude installs are unchanged. + ### Changed - The orchestrators (`konductor`, `konductor-mux-orchestrator`, `konductor-cmux-orchestrator`) diff --git a/README.md b/README.md index f4299cf58..6cec3d3e8 100644 --- a/README.md +++ b/README.md @@ -207,7 +207,10 @@ 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/` +that carries its name and description, so Kiro lists it natively; the pointer tells the agent to load +the body through the `konductor-skills` MCP server, or read `.konductor/skills//SKILL.md` if that +tool is unavailable. `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 c07453883..0d1f92410 100644 --- a/cli/README.md +++ b/cli/README.md @@ -276,7 +276,13 @@ below. Skills land under `.konductor/skills/`, deliberately outside `.kiro/skills/`: Kiro CLI's own skill discovery scans `.kiro/skills/` unconditionally and makes every skill visible -to every agent regardless of what it declares, which defeats per-agent scoping. Skill +to every agent regardless of what it declares, which defeats per-agent scoping. Only a +small `ks-/SKILL.md` pointer per skill is written there, carrying the skill's name +and description plus instructions to load the body with the `konductor-skills` MCP +server's `get_skill` tool (falling back to reading `.konductor/skills//SKILL.md`); +the `ks-` prefix keeps pointers apart from same-named skills other tools install. The +pointers are tracked in the manifest, so `update` and `uninstall` handle them; Claude +installs get none. Skill install merges into `/.konductor/skills/`: a skill directory this install did 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. diff --git a/cli/konductor-rs/src/cli/install/claude.rs b/cli/konductor-rs/src/cli/install/claude.rs index 3770acceb..62c25c831 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 d0dc1669e..5c268abb0 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,7 @@ 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}; #[cfg(test)] pub(super) mod test_support { 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 c6670c8b6..a7f9ee2a4 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 000000000..10e5879b3 --- /dev/null +++ b/cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs @@ -0,0 +1,239 @@ +// 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. + +use std::path::Path; + +use super::super::artifact::sha256_hex; +use super::super::claude::yaml_double_quote; +use super::super::index::env_home_dir; +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/`. +const SKILL_POINTER_PREFIX: &str = "ks-"; + +const SKILL_FILE_NAME: &str = "SKILL.md"; + +/// 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()), + ) + } +} + +/// Writes one pointer `SKILL.md` per skill staged under +/// `/skills/` into `/.kiro/skills/ks-/`. +/// Returns an empty `Vec` when no skills are staged. +pub(in crate::cli::install) fn install_kiro_skill_pointers( + harness_dir: &Path, + target_dir: &Path, +) -> Result, String> { + let fallback_dir = fallback_skills_dir(target_dir); + let mut files = Vec::new(); + for pointer in list_skill_pointers(harness_dir)? { + 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, + }); + } + 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> { + Ok(list_skill_pointers(harness_dir)? + .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()) +} + +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())) +} + +/// Skills whose `SKILL.md` has no readable `description` get no +/// pointer: synth rejects them, so only a hand-staged tree can hit this. +fn list_skill_pointers(harness_dir: &Path) -> Result, String> { + let source_root = harness_dir.join(SKILLS_CONTENT_TYPE_DIR); + let mut pointers = 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 description = std::fs::read_to_string(skill_md) + .ok() + .and_then(|raw| read_description(&raw)); + pointers.extend(description.map(|description| SkillPointer { + skill_name, + description, + })); + } + Ok(pointers) +} + +fn read_description(skill_md: &str) -> Option { + let (frontmatter, _) = split_frontmatter(skill_md, SKILL_FILE_NAME).ok()?; + let value: serde_yaml::Value = serde_yaml::from_str(frontmatter).ok()?; + let description = value.get("description")?.as_str()?; + Some(description.split_whitespace().collect::>().join(" ")) +} + +/// `~/.konductor/skills` when `target_dir` is the home directory, +/// otherwise the target's absolute skills directory, so the fallback +/// path always points at the install that wrote the pointer. +fn fallback_skills_dir(target_dir: &Path) -> String { + if is_home_dir(target_dir) { + return format!("~/{KONDUCTOR_DESTINATION_ROOT}/{SKILLS_CONTENT_TYPE_DIR}"); + } + target_dir + .join(KONDUCTOR_DESTINATION_ROOT) + .join(SKILLS_CONTENT_TYPE_DIR) + .display() + .to_string() +} + +fn is_home_dir(target_dir: &Path) -> bool { + env_home_dir() + .and_then(|home| Some(home.canonicalize().ok()? == target_dir.canonicalize().ok()?)) + .unwrap_or(false) +} + +fn render_pointer_md(pointer: &SkillPointer, fallback_dir: &str) -> String { + let name = &pointer.skill_name; + format!( + "---\nname: {}\ndescription: {}\n---\n\n\ + This is a pointer. Load the full `{name}` skill before following it:\n\n\ + 1. Call `get_skill` with `name: \"{name}\"` on the `konductor-skills` MCP server \ + (load the tool first if it is deferred).\n\ + 2. If that tool is unavailable, read `{fallback_dir}/{name}/{SKILL_FILE_NAME}`.\n", + yaml_double_quote(&pointer.dir_name()), + yaml_double_quote(&pointer.description), + ) +} + +#[cfg(test)] +mod tests { + use super::*; + + 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(); + } + + #[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(), Some("one two")); + } + + #[test] + fn description_is_absent_without_frontmatter_or_key() { + assert_eq!(read_description("# just a body\n"), None); + assert_eq!(read_description("---\nname: a\n---\n\nBody\n"), None); + } + + #[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, "~/.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("~/.konductor/skills/alpha/SKILL.md")); + } + + #[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", + "---\nname: alpha\ndescription: A.\n---\n\nBody\n", + ); + stage_skill(&harness_dir, "bare", "no frontmatter\n"); + + let files = install_kiro_skill_pointers(&harness_dir, &target_dir).unwrap(); + let planned = plan_kiro_skill_pointer_files(&harness_dir, &target_dir, None).unwrap(); + + let paths: Vec<_> = files.iter().map(|f| f.path.as_str()).collect(); + assert_eq!(paths, [".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 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) + .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()); + } +} 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 53aa6918a..2f388eb11 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs @@ -52,11 +52,11 @@ 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, - KONDUCTOR_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, PlannedFile, KIRO_DESTINATION_ROOT, KONDUCTOR_DESTINATION_ROOT, }; use super::manifest::{classify_provenance, ManifestFile, Provenance, Status, StrategyManifest}; use super::resource_rewrite::{ @@ -315,6 +315,7 @@ 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)?); // 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 diff --git a/cli/konductor-rs/src/cli/install/phases.rs b/cli/konductor-rs/src/cli/install/phases.rs index e8ae952ed..1cd6aa64d 100644 --- a/cli/konductor-rs/src/cli/install/phases.rs +++ b/cli/konductor-rs/src/cli/install/phases.rs @@ -382,6 +382,10 @@ impl InstallPhase for SopInstallPhase { staged_root, target_dir, )?); + files.extend(super::kiro_cli::install_kiro_skill_pointers( + staged_root, + target_dir, + )?); // Additive Claude branch: this run is Kiro's own, but the // target also has a pre-existing `.claude` marker. diff --git a/cli/konductor-rs/src/cli/synth/parse_canonical.rs b/cli/konductor-rs/src/cli/synth/parse_canonical.rs index 70c2cf626..d63c35758 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/tests/kiro_skill_pointers_e2e.rs b/cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs new file mode 100644 index 000000000..6275bedb7 --- /dev/null +++ b/cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs @@ -0,0 +1,328 @@ +// 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. + +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 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, +) { + run_konductor( + home, + sink, + &[ + "install", + "--from", + &repo.display().to_string(), + "--target", + &target.display().to_string(), + "--harness", + harness, + ], + ); +} + +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" + ); + assert_pointer(&home, "alpha", PLAIN_DESCRIPTION, "~/.konductor/skills"); + assert_pointer(&home, "beta", FOLDED_DESCRIPTION, "~/.konductor/skills"); + 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(); + run_konductor( + &home, + &sink, + &[ + "update", + "--from", + &repo.display().to_string(), + "--target", + &home.display().to_string(), + "--force", + ], + ); + assert_pointer(&home, "alpha", PLAIN_DESCRIPTION, "~/.konductor/skills"); + + run_konductor( + &home, + &sink, + &[ + "uninstall", + "--target", + &home.display().to_string(), + "--harness", + 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 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(&home, &sink, &repo, &home, "claude"); + + assert!( + home.join(".claude/skills/alpha/SKILL.md").is_file(), + "claude keeps its own skills" + ); + 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 ca141a8bc..173a0eec0 100644 --- a/cli/konductor-rs/tests/synth_install_e2e.rs +++ b/cli/konductor-rs/tests/synth_install_e2e.rs @@ -281,11 +281,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 ); @@ -942,7 +942,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(); From b66065dd89dc05b29914e808ec104a59abacb408 Mon Sep 17 00:00:00 2001 From: Nihit <24500178+knihit@users.noreply.github.com> Date: Tue, 27 Oct 2026 20:19:47 +0000 Subject: [PATCH 2/4] fix(install): close review findings on native skill pointers Discovery: every Kiro agent now lists skill://.kiro/skills/ks-*/SKILL.md, added through a resource-rewrite pass for both Kiro harnesses. Pointer text: call get_skill first, read the file only when the konductor-skills server is not connected, and stop on a not-found answer. The fallback path is absolute. Descriptions are cut to 200 characters, stripped of control characters, and an unreadable SKILL.md fails the install while a missing description warns. Opt-out: install --no-skill-pointers skips pointers and the agent resource, prunes earlier ones, and is recorded in .konductor so update keeps it. update also prunes pointers for skills the source dropped. Pointers get their own install phase, and the install summary no longer counts them as skills. --- CHANGELOG.md | 8 +- README.md | 7 +- cli/README.md | 32 +- cli/konductor-rs/src/cli.rs | 8 + cli/konductor-rs/src/cli/dispatch.rs | 7 +- cli/konductor-rs/src/cli/install.rs | 68 ++- cli/konductor-rs/src/cli/install/kiro_cli.rs | 63 ++- .../src/cli/install/kiro_cli/copy.rs | 12 +- .../cli/install/kiro_cli/skill_pointers.rs | 484 +++++++++++++++--- .../src/cli/install/kiro_cli_v3.rs | 28 +- cli/konductor-rs/src/cli/install/phases.rs | 45 +- .../src/cli/install/resource_rewrite/mod.rs | 166 ++++++ cli/konductor-rs/src/cli/uninstall.rs | 8 + .../tests/kiro_skill_pointers_e2e.rs | 244 +++++++-- cli/konductor-rs/tests/synth_install_e2e.rs | 23 +- docs/user-guide/agents.md | 2 +- docs/user-guide/appendix/contributing.md | 3 +- docs/user-guide/reference.md | 1 + 18 files changed, 1057 insertions(+), 152 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd8e9fd26..fe8fec793 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,11 +10,9 @@ not per individual commit. ### Added -- Kiro installs (`kiro-cli-v2`, `kiro-v3`) write a small `ks-/SKILL.md` pointer per skill - into `.kiro/skills/`, so Kiro lists every skill by name and description. The body still loads - through the `konductor-skills` MCP server's `get_skill`, so skill-load telemetry is unchanged; - if that tool is unavailable the pointer falls back to reading `.konductor/skills//SKILL.md`. - Pointers are tracked in the manifest and removed by `uninstall`. Claude installs are unchanged. +- 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 diff --git a/README.md b/README.md index 6cec3d3e8..4b055ead0 100644 --- a/README.md +++ b/README.md @@ -208,9 +208,10 @@ auto-detection. Paths are relative to `--target `, or `$HOME` if you don't Kiro skills live in `.konductor/skills/`, not `.kiro/skills/`, so Kiro doesn't expose every skill body to every agent. Each skill also gets a short `ks-/SKILL.md` pointer under `.kiro/skills/` -that carries its name and description, so Kiro lists it natively; the pointer tells the agent to load -the body through the `konductor-skills` MCP server, or read `.konductor/skills//SKILL.md` if that -tool is unavailable. `kiro-cli-v2` and `kiro-v3` share those paths, so installing one over the other +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 0d1f92410..25a72a719 100644 --- a/cli/README.md +++ b/cli/README.md @@ -276,17 +276,35 @@ below. Skills land under `.konductor/skills/`, deliberately outside `.kiro/skills/`: Kiro CLI's own skill discovery scans `.kiro/skills/` unconditionally and makes every skill visible -to every agent regardless of what it declares, which defeats per-agent scoping. Only a -small `ks-/SKILL.md` pointer per skill is written there, carrying the skill's name -and description plus instructions to load the body with the `konductor-skills` MCP -server's `get_skill` tool (falling back to reading `.konductor/skills//SKILL.md`); -the `ks-` prefix keeps pointers apart from same-named skills other tools install. The -pointers are tracked in the manifest, so `update` and `uninstall` handle them; Claude -installs get none. Skill +to every agent regardless of what it declares, which defeats per-agent scoping. Skill install merges into `/.konductor/skills/`: a skill directory this install did 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 6593a1a63..b7faf7f50 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 a38965091..0941ff9c0 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 66525b7d7..8deafcfd2 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/kiro_cli.rs b/cli/konductor-rs/src/cli/install/kiro_cli.rs index 5c268abb0..350b6fb14 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli.rs @@ -328,7 +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}; +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 { @@ -1588,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(); @@ -1631,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( @@ -1659,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(); @@ -1813,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(); @@ -1867,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" @@ -2135,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(); @@ -2253,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(); @@ -2426,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 9a2f55f34..26ab9b722 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/skill_pointers.rs b/cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs index 10e5879b3..619c26015 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs @@ -4,12 +4,19 @@ // 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; +use std::path::{Path, PathBuf}; use super::super::artifact::sha256_hex; use super::super::claude::yaml_double_quote; -use super::super::index::env_home_dir; 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}; @@ -19,11 +26,28 @@ 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/`. -const SKILL_POINTER_PREFIX: &str = "ks-"; +/// 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, @@ -44,26 +68,88 @@ impl SkillPointer { } } +/// 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-/`. -/// Returns an empty `Vec` when no skills are staged. +/// `/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 fallback_dir = fallback_skills_dir(target_dir); let mut files = Vec::new(); - for pointer in list_skill_pointers(harness_dir)? { - 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, - }); + 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) } @@ -74,7 +160,11 @@ pub(in crate::cli::install) fn plan_kiro_skill_pointer_files( 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(); @@ -91,6 +181,37 @@ pub(in crate::cli::install) fn plan_kiro_skill_pointer_files( .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) @@ -100,60 +221,92 @@ fn write_pointer(dst: &Path, rendered: &str) -> Result<(), String> { .map_err(|e| format!("failed to write {}: {e}", dst.display())) } -/// Skills whose `SKILL.md` has no readable `description` get no -/// pointer: synth rejects them, so only a hand-staged tree can hit this. -fn list_skill_pointers(harness_dir: &Path) -> Result, String> { +/// 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 pointers = Vec::new(); + 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 description = std::fs::read_to_string(skill_md) - .ok() - .and_then(|raw| read_description(&raw)); - pointers.extend(description.map(|description| SkillPointer { - skill_name, - description, - })); + 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(pointers) + Ok(set) } -fn read_description(skill_md: &str) -> Option { - let (frontmatter, _) = split_frontmatter(skill_md, SKILL_FILE_NAME).ok()?; - let value: serde_yaml::Value = serde_yaml::from_str(frontmatter).ok()?; - let description = value.get("description")?.as_str()?; - Some(description.split_whitespace().collect::>().join(" ")) +/// 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)) } -/// `~/.konductor/skills` when `target_dir` is the home directory, -/// otherwise the target's absolute skills directory, so the fallback -/// path always points at the install that wrote the pointer. -fn fallback_skills_dir(target_dir: &Path) -> String { - if is_home_dir(target_dir) { - return format!("~/{KONDUCTOR_DESTINATION_ROOT}/{SKILLS_CONTENT_TYPE_DIR}"); +/// 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(); } - target_dir + 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 is_home_dir(target_dir: &Path) -> bool { - env_home_dir() - .and_then(|home| Some(home.canonicalize().ok()? == target_dir.canonicalize().ok()?)) - .unwrap_or(false) -} - fn render_pointer_md(pointer: &SkillPointer, fallback_dir: &str) -> String { let name = &pointer.skill_name; format!( "---\nname: {}\ndescription: {}\n---\n\n\ - This is a pointer. Load the full `{name}` skill before following it:\n\n\ - 1. Call `get_skill` with `name: \"{name}\"` on the `konductor-skills` MCP server \ - (load the tool first if it is deferred).\n\ - 2. If that tool is unavailable, read `{fallback_dir}/{name}/{SKILL_FILE_NAME}`.\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), ) @@ -162,6 +315,7 @@ fn render_pointer_md(pointer: &SkillPointer, fallback_dir: &str) -> String { #[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}")) @@ -173,16 +327,90 @@ mod tests { 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(), Some("one two")); + assert_eq!(read_description(md).as_deref(), Ok("one two")); } #[test] - fn description_is_absent_without_frontmatter_or_key() { - assert_eq!(read_description("# just a body\n"), None); - assert_eq!(read_description("---\nname: a\n---\n\nBody\n"), None); + 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] @@ -191,41 +419,78 @@ mod tests { skill_name: "alpha".to_string(), description: "Say \"hi\": now".to_string(), }; - let rendered = render_pointer_md(&pointer, "~/.konductor/skills"); + 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("~/.konductor/skills/alpha/SKILL.md")); + 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", - "---\nname: alpha\ndescription: A.\n---\n\nBody\n", - ); + 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).unwrap(); + 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(); - let paths: Vec<_> = files.iter().map(|f| f.path.as_str()).collect(); - assert_eq!(paths, [".kiro/skills/ks-alpha/SKILL.md"]); + 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) + assert!(install_kiro_skill_pointers(&harness_dir, &target_dir, None) .unwrap() .is_empty()); } @@ -236,4 +501,91 @@ mod tests { 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 2f388eb11..342c8197e 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs @@ -56,13 +56,14 @@ use super::kiro_cli::{ 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, KONDUCTOR_DESTINATION_ROOT, + 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,7 +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)?); + 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 @@ -567,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; @@ -593,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 @@ -878,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 1cd6aa64d..f8545b09a 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), ] @@ -382,10 +384,6 @@ impl InstallPhase for SopInstallPhase { staged_root, target_dir, )?); - files.extend(super::kiro_cli::install_kiro_skill_pointers( - staged_root, - target_dir, - )?); // Additive Claude branch: this run is Kiro's own, but the // target also has a pre-existing `.claude` marker. @@ -408,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 @@ -678,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"), @@ -691,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 cdb62ce50..2873f18f1 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/uninstall.rs b/cli/konductor-rs/src/cli/uninstall.rs index 43d2e0280..6c20ed964 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 index 6275bedb7..49fbe0138 100644 --- a/cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs +++ b/cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs @@ -7,7 +7,9 @@ // // 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. +// 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}; @@ -15,6 +17,8 @@ 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"; @@ -132,22 +136,85 @@ fn install( 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, &[ - "install", + "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") @@ -210,8 +277,9 @@ fn kiro_install_writes_tracked_pointers_and_uninstall_removes_them() { home.join(".konductor/installs").is_file(), "{harness}: index went to the scratch HOME" ); - assert_pointer(&home, "alpha", PLAIN_DESCRIPTION, "~/.konductor/skills"); - assert_pointer(&home, "beta", FOLDED_DESCRIPTION, "~/.konductor/skills"); + 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" @@ -227,31 +295,10 @@ fn kiro_install_writes_tracked_pointers_and_uninstall_removes_them() { ); std::fs::remove_file(pointer_path(&home, "alpha")).unwrap(); - run_konductor( - &home, - &sink, - &[ - "update", - "--from", - &repo.display().to_string(), - "--target", - &home.display().to_string(), - "--force", - ], - ); - assert_pointer(&home, "alpha", PLAIN_DESCRIPTION, "~/.konductor/skills"); + update(&home, &sink, &repo, &home); + assert_pointer(&home, "alpha", PLAIN_DESCRIPTION, &fallback_dir); - run_konductor( - &home, - &sink, - &[ - "uninstall", - "--target", - &home.display().to_string(), - "--harness", - harness, - ], - ); + uninstall(&home, &sink, &home, harness); assert!( !pointer_path(&home, "alpha").exists(), "{harness}: pointer removed" @@ -289,6 +336,132 @@ fn pointer_fallback_path_follows_a_target_that_is_not_home() { 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(); @@ -296,12 +469,23 @@ fn claude_install_writes_no_pointers() { let home = scratch_dir("home-claude"); let repo = synthed_repo(&home, &sink, "claudeCli"); - install(&home, &sink, &repo, &home, "claude"); + 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" diff --git a/cli/konductor-rs/tests/synth_install_e2e.rs b/cli/konductor-rs/tests/synth_install_e2e.rs index 173a0eec0..9ab4b16ff 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!( @@ -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 diff --git a/docs/user-guide/agents.md b/docs/user-guide/agents.md index 0e9dcd437..c37586c36 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 4ce41b232..bdaec55e4 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 0845a294f..d530b10a9 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` | From 9cd955c3f70437a89f169e372ca27d6adaddc168 Mon Sep 17 00:00:00 2001 From: Nihit <24500178+knihit@users.noreply.github.com> Date: Tue, 27 Oct 2026 21:31:42 +0000 Subject: [PATCH 3/4] refactor(install): make --no-telemetry the single switch for skill pointers Drop --no-skill-pointers and its .konductor/no-skill-pointers marker. Pointers are always written for the Kiro harnesses. With telemetry on a pointer calls get_skill; with --no-telemetry it reads the skill file directly. pointer_instructions picks the text from the install's telemetry setting, and install --no-telemetry over a telemetry-on target at the same version is no longer skipped as already-at-version, so the pointers are rewritten. --- CHANGELOG.md | 5 +- README.md | 4 +- cli/README.md | 19 +- cli/konductor-rs/src/cli.rs | 12 +- cli/konductor-rs/src/cli/dispatch.rs | 7 +- cli/konductor-rs/src/cli/install.rs | 122 ++++--- cli/konductor-rs/src/cli/install/kiro_cli.rs | 3 - .../src/cli/install/kiro_cli/copy.rs | 6 +- .../cli/install/kiro_cli/skill_pointers.rs | 272 +++++++--------- .../src/cli/install/kiro_cli_v3.rs | 10 +- cli/konductor-rs/src/cli/install/phases.rs | 6 +- .../src/cli/install/resource_rewrite/mod.rs | 36 +-- cli/konductor-rs/src/cli/uninstall.rs | 8 - .../tests/kiro_skill_pointers_e2e.rs | 306 +++++++++--------- docs/user-guide/reference.md | 3 +- 15 files changed, 380 insertions(+), 439 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe8fec793..003a5307f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,9 +10,8 @@ not per individual commit. ### 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. +- Kiro installs write a `ks-/SKILL.md` pointer per skill, so Kiro lists each skill natively. +- A pointer calls `get_skill`; with `--no-telemetry` it reads the skill file directly instead. ### Changed diff --git a/README.md b/README.md index 4b055ead0..c1401cbc7 100644 --- a/README.md +++ b/README.md @@ -210,8 +210,8 @@ Kiro skills live in `.konductor/skills/`, not `.kiro/skills/`, so Kiro doesn't e 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 +server is not connected. With `--no-telemetry`, the pointer reads the file directly instead +(`update` keeps the install's setting). `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 25a72a719..7e0797c0a 100644 --- a/cli/README.md +++ b/cli/README.md @@ -285,9 +285,12 @@ fully replaced so a file removed from the source doesn't linger in the destinati 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. +With telemetry on, 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. +With `install --no-telemetry`, the pointer tells the agent to read that file directly, with +no MCP step. `--no-telemetry` is the only switch: `update` rewrites the pointers to match the +install's telemetry setting, and a later `install` without the flag turns `get_skill` back on. - Each Kiro agent also gets a `skill://.kiro/skills/ks-*/SKILL.md` resource, so the pointers are listed for it explicitly. @@ -295,15 +298,13 @@ connected. If `get_skill` says the skill is not found, the agent is told to stop 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 + too. +- Because the file 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. +- A direct file read does not go through `get_skill`, so it is not counted in skill-load + telemetry and skips the server's per-agent scope check. ### Installing without `--from`: the GitHub-release / main-branch-`dist/` fallback chain diff --git a/cli/konductor-rs/src/cli.rs b/cli/konductor-rs/src/cli.rs index b7faf7f50..4b64f7d95 100644 --- a/cli/konductor-rs/src/cli.rs +++ b/cli/konductor-rs/src/cli.rs @@ -185,18 +185,12 @@ pub enum Commands { /// Opt out of usage-analytics telemetry for this install. No /// install record or telemetry hook is written, and any a prior - /// install at this target wrote is removed. + /// install at this target wrote is removed. Kiro skill pointers + /// then read the skill file directly instead of calling the MCP + /// server's `get_skill`. #[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 0941ff9c0..a38965091 100644 --- a/cli/konductor-rs/src/cli/dispatch.rs +++ b/cli/konductor-rs/src/cli/dispatch.rs @@ -37,17 +37,15 @@ 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_skill_pointer_choice( + } => crate::cli::install::dispatch_install_with( from, target, harness, link_bin, no_telemetry, - no_skill_pointers, use_github_token, release_version, force, @@ -1013,7 +1011,6 @@ 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, @@ -1053,7 +1050,6 @@ 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, @@ -1088,7 +1084,6 @@ 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 8deafcfd2..715559c75 100644 --- a/cli/konductor-rs/src/cli/install.rs +++ b/cli/konductor-rs/src/cli/install.rs @@ -698,67 +698,6 @@ 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 @@ -981,9 +920,14 @@ fn dispatch_install_with_remote_installer( // This is what makes `install --version ` skip only when the // target is already at exactly that requested version, not // merely because it happens to also match "latest". + // + // A record only exists while telemetry is on, so `--no-telemetry` + // over it is a change of setting, not an already-at-version no-op: + // the Kiro skill pointers must be rewritten to match. let recorded_for_this_harness = crate::cli::telemetry::read_install_info(&destination) .and_then(|record| (record.harness == harness).then_some(record.agent_version)) - .flatten(); + .flatten() + .filter(|_| !no_telemetry); if let Some(recorded) = recorded_for_this_harness { let incoming_tag = match release_version.as_deref() { Some(requested) => Some(requested.to_string()), @@ -4356,6 +4300,60 @@ mod tests { fs::remove_dir_all(&repo_root).ok(); } + /// Turning telemetry off over a target that is already at the requested + /// version is a change of setting, not an already-at-version no-op: the + /// install must reach the fetch so the skill pointers are rewritten. + #[test] + fn dispatch_install_with_no_telemetry_over_a_matching_recorded_version_still_fetches() { + let _home = HomeGuard::new("install-no-telemetry-matching-home"); + let target = scratch_dir("install-no-telemetry-matching-target"); + let repo_root = scratch_dir("install-no-telemetry-matching-repo"); + seed_dist_version(&repo_root, "2.0.0"); + crate::cli::telemetry::write_install_info( + &target, + &repo_root, + "kiro-cli-v2", + "2026-01-01T00:00:00Z", + ) + .unwrap(); + + let remote_installer_called = std::rc::Rc::new(std::cell::Cell::new(false)); + let remote_installer_called_check = remote_installer_called.clone(); + let code = dispatch_install_with_remote_installer( + None, + Some(target.to_str().unwrap().to_string()), + "kiro-cli-v2".to_string(), + false, + true, // --no-telemetry + false, + Some("2.0.0".to_string()), + false, + false, + false, + ColorMode::disabled(), + move |_strategy, _destination, _installed_at, _no_telemetry| { + remote_installer_called_check.set(true); + Err(remote_orchestrate::FallbackChainError::ReleaseOnly( + remote_orchestrate::RemoteOrchestrationError::Fetch( + github::GithubFetchError::Network( + "fake network error injected by a test -- no real network call \ + was made" + .to_string(), + ), + ), + )) + }, + ); + assert!( + remote_installer_called.get(), + "--no-telemetry over a telemetry-on target must not be skipped as already-at-version" + ); + assert_eq!(code, EXIT_USAGE_ERROR); + + fs::remove_dir_all(&target).ok(); + fs::remove_dir_all(&repo_root).ok(); + } + /// `--version ` for a tag that does not match the recorded /// version must proceed to the real fetch, never skip. #[test] diff --git a/cli/konductor-rs/src/cli/install/kiro_cli.rs b/cli/konductor-rs/src/cli/install/kiro_cli.rs index 350b6fb14..c76239322 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli.rs @@ -332,9 +332,6 @@ 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 { 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 26ab9b722..91c06a0ea 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli/copy.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli/copy.rs @@ -12,7 +12,6 @@ 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, @@ -497,10 +496,8 @@ fn copy_agent_files_rewriting_resources( file_names: Vec, no_telemetry: bool, ) -> Result<(Vec, bool), String> { - 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()); @@ -542,8 +539,7 @@ fn copy_agent_files_rewriting_resources( .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(); + let needs_pointer_glob = serde_json::from_str::(&contents).is_ok(); if no_telemetry && !needs_pointer_glob && !contents.contains(CONTEXT_RESOURCE_PREFIX) 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 index 619c26015..0a6bd67c5 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs @@ -2,18 +2,17 @@ // // 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. +// skill's name and description natively. With telemetry on, a pointer +// sends the agent to the `konductor-skills` MCP server's `get_skill`; with +// `--no-telemetry`, it names the installed skill file to read directly +// (`pointer_instructions`). // // 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 std::path::Path; use super::super::artifact::sha256_hex; use super::super::claude::yaml_double_quote; @@ -44,10 +43,6 @@ 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, @@ -75,79 +70,37 @@ struct PointerSet { 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. +/// longer writes (a dropped skill). Returns an empty `Vec` when no skills +/// are staged. `no_telemetry` picks the body of every pointer (see +/// `pointer_instructions`), so a run that changes it rewrites them all. pub(in crate::cli::install) fn install_kiro_skill_pointers( harness_dir: &Path, target_dir: &Path, prior_manifest: Option<&StrategyManifest>, + no_telemetry: bool, ) -> 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, - }); - } + 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 skills_dir = installed_skills_dir(target_dir); + for pointer in &set.pointers { + let rendered = render_pointer_md(pointer, &skills_dir, no_telemetry); + 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) @@ -160,9 +113,6 @@ pub(in crate::cli::install) fn plan_kiro_skill_pointer_files( 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() @@ -285,9 +235,9 @@ fn truncate_description(text: &str) -> String { format!("{}{ELLIPSIS}", cut.trim_end()) } -/// The absolute path of the installed skill bodies, so the fallback read +/// The absolute path of the installed skill bodies, so a direct read /// works from any working directory without relying on `~` expansion. -fn fallback_skills_dir(target_dir: &Path) -> String { +fn installed_skills_dir(target_dir: &Path) -> String { std::path::absolute(target_dir) .unwrap_or_else(|_| target_dir.to_path_buf()) .join(KONDUCTOR_DESTINATION_ROOT) @@ -296,19 +246,34 @@ fn fallback_skills_dir(target_dir: &Path) -> String { .to_string() } -fn render_pointer_md(pointer: &SkillPointer, fallback_dir: &str) -> String { - let name = &pointer.skill_name; +/// The numbered steps of a pointer body. The one place that turns the +/// install's telemetry setting into pointer text: with telemetry on the +/// agent loads the skill through the server's `get_skill` (which also +/// enforces the agent's scope) and reads `skill_file` only if the server +/// is absent; with `--no-telemetry` it reads `skill_file` and nothing else. +fn pointer_instructions(name: &str, skill_file: &str, no_telemetry: bool) -> String { + if no_telemetry { + return format!("1. Read `{skill_file}`.\n"); + } 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. \ + "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\ + read `{skill_file}`.\n\ 3. If `get_skill` says the skill is not found, stop. The skill is outside your scope. \ - Do not read the file.\n", + Do not read the file.\n" + ) +} + +fn render_pointer_md(pointer: &SkillPointer, skills_dir: &str, no_telemetry: bool) -> String { + let name = &pointer.skill_name; + let skill_file = format!("{skills_dir}/{name}/{SKILL_FILE_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{}", yaml_double_quote(&pointer.dir_name()), yaml_double_quote(&pointer.description), + pointer_instructions(name, &skill_file, no_telemetry), ) } @@ -413,30 +378,31 @@ mod tests { ); } - #[test] - fn rendered_pointer_is_valid_frontmatter_with_prefixed_name() { - let pointer = SkillPointer { + fn alpha() -> SkillPointer { + 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")); + } + } + + #[test] + fn rendered_pointer_is_valid_frontmatter_with_prefixed_name() { + for no_telemetry in [false, true] { + let rendered = render_pointer_md(&alpha(), "/abs/.konductor/skills", no_telemetry); + 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"); + fn telemetry_on_body_orders_get_skill_before_the_file_and_stops_on_not_found() { + let rendered = render_pointer_md(&alpha(), "/abs/.konductor/skills", false); let (_, body) = split_frontmatter(&rendered, "pointer").unwrap(); let get_skill = body.find("Call `get_skill`").expect("step 1"); let file = body @@ -454,7 +420,21 @@ mod tests { "{body}" ); assert!(!body.contains("deferred"), "{body}"); - assert!(!body.contains('~'), "fallback path is absolute: {body}"); + assert!(!body.contains('~'), "path is absolute: {body}"); + } + + #[test] + fn telemetry_off_body_reads_the_absolute_file_and_never_mentions_mcp() { + let rendered = render_pointer_md(&alpha(), "/abs/.konductor/skills", true); + let (_, body) = split_frontmatter(&rendered, "pointer").unwrap(); + assert!( + body.contains("1. Read `/abs/.konductor/skills/alpha/SKILL.md`."), + "{body}" + ); + assert!(!body.contains("get_skill"), "{body}"); + assert!(!body.contains("MCP"), "{body}"); + assert!(!body.contains("konductor-skills"), "{body}"); + assert!(!body.contains("2."), "a single step: {body}"); } #[test] @@ -464,7 +444,7 @@ mod tests { 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 files = install_kiro_skill_pointers(&harness_dir, &target_dir, None, false).unwrap(); let planned = plan_kiro_skill_pointer_files(&harness_dir, &target_dir, None).unwrap(); assert_eq!(paths(&files), [".kiro/skills/ks-alpha/SKILL.md"]); @@ -482,7 +462,7 @@ mod tests { 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(); + let err = install_kiro_skill_pointers(&harness_dir, &target_dir, None, false).unwrap_err(); assert!(err.contains("failed to read"), "{err}"); } @@ -490,9 +470,11 @@ mod tests { 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()); + assert!( + install_kiro_skill_pointers(&harness_dir, &target_dir, None, false) + .unwrap() + .is_empty() + ); } #[test] @@ -508,13 +490,17 @@ mod tests { 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(); + let first = install_kiro_skill_pointers(&harness_dir, &target_dir, None, false).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(); + let second = install_kiro_skill_pointers( + &harness_dir, + &target_dir, + Some(&manifest_of(&first)), + false, + ) + .unwrap(); assert_eq!(paths(&second), [".kiro/skills/ks-alpha/SKILL.md"]); assert!(target_dir.join(".kiro/skills/ks-alpha/SKILL.md").is_file()); @@ -526,7 +512,7 @@ mod tests { 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 first = install_kiro_skill_pointers(&harness_dir, &target_dir, None, false).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"); @@ -534,7 +520,8 @@ mod tests { 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(); + install_kiro_skill_pointers(&harness_dir, &target_dir, Some(&manifest_of(&first)), false) + .unwrap(); assert!(!target_dir.join(".kiro/skills/ks-alpha/SKILL.md").exists()); assert!( @@ -545,47 +532,26 @@ mod tests { } #[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"); + fn changing_the_telemetry_setting_rewrites_every_pointer_in_place() { + let harness_dir = scratch_dir("switch-harness"); + let target_dir = scratch_dir("switch-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() - ); + let pointer = target_dir.join(".kiro/skills/ks-alpha/SKILL.md"); - 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 - ); - } + let on = install_kiro_skill_pointers(&harness_dir, &target_dir, None, false).unwrap(); + let on_text = std::fs::read_to_string(&pointer).unwrap(); + assert!(on_text.contains("Call `get_skill`"), "{on_text}"); - #[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)); + let off = + install_kiro_skill_pointers(&harness_dir, &target_dir, Some(&manifest_of(&on)), true) + .unwrap(); + let off_text = std::fs::read_to_string(&pointer).unwrap(); + assert!(!off_text.contains("get_skill"), "{off_text}"); + assert_ne!(on[0].sha256, off[0].sha256); + assert_eq!(paths(&on), paths(&off)); + + install_kiro_skill_pointers(&harness_dir, &target_dir, Some(&manifest_of(&off)), false) + .unwrap(); + assert_eq!(std::fs::read_to_string(&pointer).unwrap(), on_text); } } 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 342c8197e..01049bb79 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs @@ -56,8 +56,7 @@ use super::kiro_cli::{ 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, + reject_unsafe_file_name, PlannedFile, KIRO_DESTINATION_ROOT, KONDUCTOR_DESTINATION_ROOT, }; use super::manifest::{classify_provenance, ManifestFile, Provenance, Status, StrategyManifest}; use super::resource_rewrite::{ @@ -320,6 +319,7 @@ impl InstallStrategy for KiroCliV3InstallStrategy { &harness_dir, target_dir, prior_manifest.as_ref(), + no_telemetry, )?); // Additive Claude branch, mirroring `SopInstallPhase::run`'s // dual-marker branch: this run is this strategy's own, but the @@ -572,8 +572,7 @@ fn copy_agent_files_rewriting_resources( ctx: &RewriteContext<'_>, file_names: Vec, ) -> Result<(Vec, bool), String> { - let pointers_on = ctx.install_root().is_some_and(skill_pointers_enabled); - let passes = with_skill_pointer_pass(standard_passes_v3(), pointers_on); + let passes = with_skill_pointer_pass(standard_passes_v3()); let mut files = Vec::with_capacity(file_names.len()); let mut any_mcp_server_injected = false; @@ -601,8 +600,7 @@ fn copy_agent_files_rewriting_resources( .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(); + let needs_pointer_glob = serde_json::from_str::(&contents).is_ok(); if !needs_pointer_glob && !contents.contains(super::kiro_cli::CONTEXT_RESOURCE_PREFIX) && !contents.contains(SKILL_RESOURCE_PREFIX) diff --git a/cli/konductor-rs/src/cli/install/phases.rs b/cli/konductor-rs/src/cli/install/phases.rs index f8545b09a..fff74454e 100644 --- a/cli/konductor-rs/src/cli/install/phases.rs +++ b/cli/konductor-rs/src/cli/install/phases.rs @@ -411,7 +411,8 @@ impl InstallPhase for SopInstallPhase { /// 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. +/// chain: Claude installs get no pointers. The pointer body follows +/// `no_telemetry`. pub(super) struct SkillPointerInstallPhase; impl InstallPhase for SkillPointerInstallPhase { @@ -426,12 +427,13 @@ impl InstallPhase for SkillPointerInstallPhase { _repo_root: Option<&Path>, prior_manifest: Option<&StrategyManifest>, _phase_outputs: &PhaseOutputs, - _no_telemetry: bool, + no_telemetry: bool, ) -> Result, InstallError> { Ok(super::kiro_cli::install_kiro_skill_pointers( staged_root, target_dir, prior_manifest, + no_telemetry, )?) } } 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 2873f18f1..652d50c6c 100644 --- a/cli/konductor-rs/src/cli/install/resource_rewrite/mod.rs +++ b/cli/konductor-rs/src/cli/install/resource_rewrite/mod.rs @@ -378,10 +378,9 @@ 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. +/// so Kiro lists the `ks-` pointers for that agent explicitly. Added +/// to the Kiro pipelines by `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 { @@ -420,16 +419,12 @@ impl ResourceRewritePass for SkillPointerResourcePass { } } -/// Inserts `SkillPointerResourcePass` right after `SkillResourcePass` -/// when `enabled`; otherwise returns `passes` unchanged. +/// Inserts `SkillPointerResourcePass` right after `SkillResourcePass`. 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)); - } + let after_skill_pass = passes.len().min(2); + passes.insert(after_skill_pass, Box::new(SkillPointerResourcePass)); passes } @@ -834,7 +829,7 @@ mod tests { 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 passes = with_skill_pointer_pass(Vec::new()); let ctx = ctx(&dir, &dir, &dir, &[]); for _ in 0..2 { apply_all(&passes, &mut value, &ctx, Path::new("/tmp/agent.json")).unwrap(); @@ -881,16 +876,9 @@ mod tests { } #[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); + fn pointer_pass_follows_the_skill_pass() { + assert_eq!(with_skill_pointer_pass(standard_passes(true)).len(), 4); + assert_eq!(with_skill_pointer_pass(standard_passes_v3()).len(), 4); // Running the real pipeline: the glob lands after the rewritten // skill entry, and the MCP pass still sees only the real skill. @@ -899,7 +887,7 @@ mod tests { 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); + let passes = with_skill_pointer_pass(standard_passes(true)); apply_all( &passes, &mut value, @@ -923,7 +911,7 @@ mod tests { 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); + let passes = with_skill_pointer_pass(standard_passes(true)); apply_all( &passes, &mut value, diff --git a/cli/konductor-rs/src/cli/uninstall.rs b/cli/konductor-rs/src/cli/uninstall.rs index 6c20ed964..43d2e0280 100644 --- a/cli/konductor-rs/src/cli/uninstall.rs +++ b/cli/konductor-rs/src/cli/uninstall.rs @@ -1256,14 +1256,6 @@ 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 index 49fbe0138..274393d15 100644 --- a/cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs +++ b/cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs @@ -3,7 +3,9 @@ // 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. +// it in the install manifest, and remove it on uninstall. A pointer's body +// follows the install's telemetry setting: `get_skill` with telemetry on, +// a direct file read with `--no-telemetry`. // // Isolation: every invocation runs with `HOME` set to a scratch dir, and // every test holds a `RealInstallsGuard` that fails the test if the @@ -164,18 +166,21 @@ fn install_with( } 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", - ], - ); + update_with(home, sink, repo, target, &[]); +} + +fn update_with( + home: &Path, + sink: &telemetry_test_sink::TelemetrySink, + repo: &Path, + target: &Path, + extra: &[&str], +) { + let repo = repo.display().to_string(); + let target = target.display().to_string(); + let mut args = vec!["update", "--from", &repo, "--target", &target, "--force"]; + args.extend_from_slice(extra); + run_konductor(home, sink, &args); } fn uninstall(home: &Path, sink: &telemetry_test_sink::TelemetrySink, target: &Path, harness: &str) { @@ -205,16 +210,6 @@ fn agent_resources(target: &Path) -> Vec { .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") @@ -233,30 +228,68 @@ fn read_pointer(path: &Path) -> (serde_yaml::Value, String) { (serde_yaml::from_str(frontmatter).unwrap(), body.to_string()) } -fn assert_pointer(target: &Path, skill: &str, description: &str, fallback_dir: &str) { +/// How a pointer's body tells the agent to load the skill. +#[derive(Clone, Copy, PartialEq, Debug)] +enum Telemetry { + On, + Off, +} + +impl Telemetry { + fn install_args(self) -> &'static [&'static str] { + match self { + Telemetry::On => &[], + Telemetry::Off => &["--no-telemetry"], + } + } +} + +fn assert_pointer( + target: &Path, + skill: &str, + description: &str, + skills_dir: &str, + telemetry: Telemetry, +) { 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)); + let skill_file = format!("{skills_dir}/{skill}/SKILL.md"); 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}" + body.contains(&format!("`{skill_file}`")), + "body carries the file path {skill_file}: {body}" ); - let fallback = format!("{fallback_dir}/{skill}/SKILL.md"); assert!( - body.contains(&fallback), - "body carries the fallback path {fallback}: {body}" + Path::new(&skill_file).is_file(), + "the file the pointer names exists: {skill_file}" ); + match telemetry { + Telemetry::On => { + assert!(body.contains("get_skill"), "names the MCP tool: {body}"); + assert!( + body.contains("konductor-skills"), + "names the MCP server: {body}" + ); + assert!( + body.contains(&format!("name: \"{skill}\"")), + "names the real skill: {body}" + ); + assert!( + body.contains("Only if the `konductor-skills` server is not connected"), + "the file is the fallback: {body}" + ); + } + Telemetry::Off => { + assert!( + !body.contains("get_skill") && !body.contains("konductor-skills"), + "no MCP step: {body}" + ); + assert!(body.contains("Read `"), "reads the file directly: {body}"); + } + } } fn manifest_text(target: &Path) -> String { @@ -268,57 +301,62 @@ 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(); + for telemetry in [Telemetry::On, Telemetry::Off] { + let home = scratch_dir(&format!("home-{harness}-{telemetry:?}")); + let repo = synthed_repo(&home, &sink, "kiroCli"); + + install_with( + &home, + &sink, + &repo, + &home, + harness, + telemetry.install_args(), + ); + + assert!( + home.join(".konductor/installs").is_file(), + "{harness}: index went to the scratch HOME" + ); + let skills_dir = home.join(".konductor/skills").display().to_string(); + assert_pointer(&home, "alpha", PLAIN_DESCRIPTION, &skills_dir, telemetry); + assert_pointer(&home, "beta", FOLDED_DESCRIPTION, &skills_dir, telemetry); + 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, &skills_dir, telemetry); + + 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() { +fn pointer_file_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"); @@ -327,9 +365,14 @@ fn pointer_fallback_path_follows_a_target_that_is_not_home() { 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()); + let skills_dir = target.join(".konductor/skills").display().to_string(); + assert_pointer( + &target, + "alpha", + PLAIN_DESCRIPTION, + &skills_dir, + Telemetry::On, + ); std::fs::remove_dir_all(&home).ok(); std::fs::remove_dir_all(&target).ok(); @@ -367,65 +410,49 @@ fn every_kiro_agent_lists_the_pointer_glob_and_the_install_count_excludes_pointe } } +/// Both pointers of the example repo must carry `telemetry`'s body, and the +/// agent must still list the pointer glob. +fn assert_all_pointers(home: &Path, harness: &str, telemetry: Telemetry) { + let skills_dir = home.join(".konductor/skills").display().to_string(); + assert_pointer(home, "alpha", PLAIN_DESCRIPTION, &skills_dir, telemetry); + assert_pointer(home, "beta", FOLDED_DESCRIPTION, &skills_dir, telemetry); + assert!( + agent_resources(home).contains(&POINTER_GLOB.to_string()), + "{harness}: agent lists the glob" + ); +} + #[test] -fn no_skill_pointers_is_recorded_survives_update_and_a_plain_install_turns_it_back_on() { +fn pointers_follow_the_telemetry_setting_across_install_and_update() { 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 home = scratch_dir(&format!("home-switch-{harness}")); let repo = synthed_repo(&home, &sink, "kiroCli"); install(&home, &sink, &repo, &home, harness); - assert!(has_pointer_dirs(&home), "{harness}: pointers by default"); + assert_all_pointers(&home, harness, Telemetry::On); - // 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" - ); + // Telemetry on to off: the install rewrites the pointers in place. + install_with(&home, &sink, &repo, &home, harness, &["--no-telemetry"]); + assert_all_pointers(&home, harness, Telemetry::Off); - // `update` respects the recorded choice. + // `update` carries the off setting forward. 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())); + assert_all_pointers(&home, harness, Telemetry::Off); - // A plain install turns pointers back on. + // And 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())); + assert_all_pointers(&home, harness, Telemetry::On); + update(&home, &sink, &repo, &home); + assert_all_pointers(&home, harness, Telemetry::On); - // 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" - ); + // `update --no-telemetry` switches this run to the direct read, and + // the next plain `update` switches back. + update_with(&home, &sink, &repo, &home, &["--no-telemetry"]); + assert_all_pointers(&home, harness, Telemetry::Off); + update(&home, &sink, &repo, &home); + assert_all_pointers(&home, harness, Telemetry::On); std::fs::remove_dir_all(&home).ok(); std::fs::remove_dir_all(&repo).ok(); @@ -469,23 +496,12 @@ fn claude_install_writes_no_pointers() { let home = scratch_dir("home-claude"); let repo = synthed_repo(&home, &sink, "claudeCli"); - install_with( - &home, - &sink, - &repo, - &home, - "claude", - &["--no-skill-pointers"], - ); + install(&home, &sink, &repo, &home, "claude"); 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" diff --git a/docs/user-guide/reference.md b/docs/user-guide/reference.md index d530b10a9..fb499184f 100644 --- a/docs/user-guide/reference.md +++ b/docs/user-guide/reference.md @@ -93,8 +93,7 @@ Accepted before or after a subcommand. | `install` | `--from ` | path | No | Install from a local repo root with already-synthed content instead of fetching a release. A maintainer path — the documented workflows all install from a release | | `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` | `--no-telemetry` | flag | No | Suppress telemetry for this invocation. Kiro skill pointers then read the skill file directly instead of calling `get_skill` | | `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` | From 44be1384730a662a2122a43430b127dc50c0de9c Mon Sep 17 00:00:00 2001 From: Nihit <24500178+knihit@users.noreply.github.com> Date: Tue, 27 Oct 2026 21:37:42 +0000 Subject: [PATCH 4/4] Revert "refactor(install): make --no-telemetry the single switch for skill pointers" This reverts commit 9cd955c3f70437a89f169e372ca27d6adaddc168. --- CHANGELOG.md | 5 +- README.md | 4 +- cli/README.md | 19 +- cli/konductor-rs/src/cli.rs | 12 +- cli/konductor-rs/src/cli/dispatch.rs | 7 +- cli/konductor-rs/src/cli/install.rs | 122 +++---- cli/konductor-rs/src/cli/install/kiro_cli.rs | 3 + .../src/cli/install/kiro_cli/copy.rs | 6 +- .../cli/install/kiro_cli/skill_pointers.rs | 272 +++++++++------- .../src/cli/install/kiro_cli_v3.rs | 10 +- cli/konductor-rs/src/cli/install/phases.rs | 6 +- .../src/cli/install/resource_rewrite/mod.rs | 36 ++- cli/konductor-rs/src/cli/uninstall.rs | 8 + .../tests/kiro_skill_pointers_e2e.rs | 306 +++++++++--------- docs/user-guide/reference.md | 3 +- 15 files changed, 439 insertions(+), 380 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 003a5307f..fe8fec793 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,9 @@ not per individual commit. ### Added -- Kiro installs write a `ks-/SKILL.md` pointer per skill, so Kiro lists each skill natively. -- A pointer calls `get_skill`; with `--no-telemetry` it reads the skill file directly instead. +- 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 diff --git a/README.md b/README.md index c1401cbc7..4b055ead0 100644 --- a/README.md +++ b/README.md @@ -210,8 +210,8 @@ Kiro skills live in `.konductor/skills/`, not `.kiro/skills/`, so Kiro doesn't e 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. With `--no-telemetry`, the pointer reads the file directly instead -(`update` keeps the install's setting). `kiro-cli-v2` and `kiro-v3` share those paths, so installing one over the other +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 7e0797c0a..25a72a719 100644 --- a/cli/README.md +++ b/cli/README.md @@ -285,12 +285,9 @@ fully replaced so a file removed from the source doesn't linger in the destinati 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. -With telemetry on, 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. -With `install --no-telemetry`, the pointer tells the agent to read that file directly, with -no MCP step. `--no-telemetry` is the only switch: `update` rewrites the pointers to match the -install's telemetry setting, and a later `install` without the flag turns `get_skill` back on. +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. @@ -298,13 +295,15 @@ install's telemetry setting, and a later `install` without the flag turns `get_s 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. -- Because the file path is absolute, a project install writes your local path into + 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 direct file read does not go through `get_skill`, so it is not counted in skill-load - telemetry and skips the server's per-agent scope check. +- 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 diff --git a/cli/konductor-rs/src/cli.rs b/cli/konductor-rs/src/cli.rs index 4b64f7d95..b7faf7f50 100644 --- a/cli/konductor-rs/src/cli.rs +++ b/cli/konductor-rs/src/cli.rs @@ -185,12 +185,18 @@ pub enum Commands { /// Opt out of usage-analytics telemetry for this install. No /// install record or telemetry hook is written, and any a prior - /// install at this target wrote is removed. Kiro skill pointers - /// then read the skill file directly instead of calling the MCP - /// server's `get_skill`. + /// install at this target wrote is removed. #[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 a38965091..0941ff9c0 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 715559c75..8deafcfd2 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 @@ -920,14 +981,9 @@ fn dispatch_install_with_remote_installer( // This is what makes `install --version ` skip only when the // target is already at exactly that requested version, not // merely because it happens to also match "latest". - // - // A record only exists while telemetry is on, so `--no-telemetry` - // over it is a change of setting, not an already-at-version no-op: - // the Kiro skill pointers must be rewritten to match. let recorded_for_this_harness = crate::cli::telemetry::read_install_info(&destination) .and_then(|record| (record.harness == harness).then_some(record.agent_version)) - .flatten() - .filter(|_| !no_telemetry); + .flatten(); if let Some(recorded) = recorded_for_this_harness { let incoming_tag = match release_version.as_deref() { Some(requested) => Some(requested.to_string()), @@ -4300,60 +4356,6 @@ mod tests { fs::remove_dir_all(&repo_root).ok(); } - /// Turning telemetry off over a target that is already at the requested - /// version is a change of setting, not an already-at-version no-op: the - /// install must reach the fetch so the skill pointers are rewritten. - #[test] - fn dispatch_install_with_no_telemetry_over_a_matching_recorded_version_still_fetches() { - let _home = HomeGuard::new("install-no-telemetry-matching-home"); - let target = scratch_dir("install-no-telemetry-matching-target"); - let repo_root = scratch_dir("install-no-telemetry-matching-repo"); - seed_dist_version(&repo_root, "2.0.0"); - crate::cli::telemetry::write_install_info( - &target, - &repo_root, - "kiro-cli-v2", - "2026-01-01T00:00:00Z", - ) - .unwrap(); - - let remote_installer_called = std::rc::Rc::new(std::cell::Cell::new(false)); - let remote_installer_called_check = remote_installer_called.clone(); - let code = dispatch_install_with_remote_installer( - None, - Some(target.to_str().unwrap().to_string()), - "kiro-cli-v2".to_string(), - false, - true, // --no-telemetry - false, - Some("2.0.0".to_string()), - false, - false, - false, - ColorMode::disabled(), - move |_strategy, _destination, _installed_at, _no_telemetry| { - remote_installer_called_check.set(true); - Err(remote_orchestrate::FallbackChainError::ReleaseOnly( - remote_orchestrate::RemoteOrchestrationError::Fetch( - github::GithubFetchError::Network( - "fake network error injected by a test -- no real network call \ - was made" - .to_string(), - ), - ), - )) - }, - ); - assert!( - remote_installer_called.get(), - "--no-telemetry over a telemetry-on target must not be skipped as already-at-version" - ); - assert_eq!(code, EXIT_USAGE_ERROR); - - fs::remove_dir_all(&target).ok(); - fs::remove_dir_all(&repo_root).ok(); - } - /// `--version ` for a tag that does not match the recorded /// version must proceed to the real fetch, never skip. #[test] diff --git a/cli/konductor-rs/src/cli/install/kiro_cli.rs b/cli/konductor-rs/src/cli/install/kiro_cli.rs index c76239322..350b6fb14 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli.rs @@ -332,6 +332,9 @@ 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 { 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 91c06a0ea..26ab9b722 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,8 +497,10 @@ fn copy_agent_files_rewriting_resources( file_names: Vec, no_telemetry: bool, ) -> Result<(Vec, bool), String> { + 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()); @@ -539,7 +542,8 @@ fn copy_agent_files_rewriting_resources( .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 = serde_json::from_str::(&contents).is_ok(); + let needs_pointer_glob = + pointers_on && serde_json::from_str::(&contents).is_ok(); if no_telemetry && !needs_pointer_glob && !contents.contains(CONTEXT_RESOURCE_PREFIX) 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 index 0a6bd67c5..619c26015 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli/skill_pointers.rs @@ -2,17 +2,18 @@ // // 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. With telemetry on, a pointer -// sends the agent to the `konductor-skills` MCP server's `get_skill`; with -// `--no-telemetry`, it names the installed skill file to read directly -// (`pointer_instructions`). +// 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; +use std::path::{Path, PathBuf}; use super::super::artifact::sha256_hex; use super::super::claude::yaml_double_quote; @@ -43,6 +44,10 @@ 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, @@ -70,37 +75,79 @@ struct PointerSet { 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). Returns an empty `Vec` when no skills -/// are staged. `no_telemetry` picks the body of every pointer (see -/// `pointer_instructions`), so a run that changes it rewrites them all. +/// 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>, - no_telemetry: bool, ) -> Result, String> { let mut files = Vec::new(); - 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 skills_dir = installed_skills_dir(target_dir); - for pointer in &set.pointers { - let rendered = render_pointer_md(pointer, &skills_dir, no_telemetry); - 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, - }); + 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) @@ -113,6 +160,9 @@ pub(in crate::cli::install) fn plan_kiro_skill_pointer_files( 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() @@ -235,9 +285,9 @@ fn truncate_description(text: &str) -> String { format!("{}{ELLIPSIS}", cut.trim_end()) } -/// The absolute path of the installed skill bodies, so a direct read +/// The absolute path of the installed skill bodies, so the fallback read /// works from any working directory without relying on `~` expansion. -fn installed_skills_dir(target_dir: &Path) -> String { +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) @@ -246,34 +296,19 @@ fn installed_skills_dir(target_dir: &Path) -> String { .to_string() } -/// The numbered steps of a pointer body. The one place that turns the -/// install's telemetry setting into pointer text: with telemetry on the -/// agent loads the skill through the server's `get_skill` (which also -/// enforces the agent's scope) and reads `skill_file` only if the server -/// is absent; with `--no-telemetry` it reads `skill_file` and nothing else. -fn pointer_instructions(name: &str, skill_file: &str, no_telemetry: bool) -> String { - if no_telemetry { - return format!("1. Read `{skill_file}`.\n"); - } +fn render_pointer_md(pointer: &SkillPointer, fallback_dir: &str) -> String { + let name = &pointer.skill_name; format!( - "1. Call `get_skill` with `name: \"{name}\"` on the `konductor-skills` MCP server. \ + "---\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 `{skill_file}`.\n\ + 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" - ) -} - -fn render_pointer_md(pointer: &SkillPointer, skills_dir: &str, no_telemetry: bool) -> String { - let name = &pointer.skill_name; - let skill_file = format!("{skills_dir}/{name}/{SKILL_FILE_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{}", + Do not read the file.\n", yaml_double_quote(&pointer.dir_name()), yaml_double_quote(&pointer.description), - pointer_instructions(name, &skill_file, no_telemetry), ) } @@ -378,31 +413,30 @@ mod tests { ); } - fn alpha() -> SkillPointer { - SkillPointer { - skill_name: "alpha".to_string(), - description: "Say \"hi\": now".to_string(), - } - } - #[test] fn rendered_pointer_is_valid_frontmatter_with_prefixed_name() { - for no_telemetry in [false, true] { - let rendered = render_pointer_md(&alpha(), "/abs/.konductor/skills", no_telemetry); - 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")); - } + 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 telemetry_on_body_orders_get_skill_before_the_file_and_stops_on_not_found() { - let rendered = render_pointer_md(&alpha(), "/abs/.konductor/skills", false); + 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 @@ -420,21 +454,7 @@ mod tests { "{body}" ); assert!(!body.contains("deferred"), "{body}"); - assert!(!body.contains('~'), "path is absolute: {body}"); - } - - #[test] - fn telemetry_off_body_reads_the_absolute_file_and_never_mentions_mcp() { - let rendered = render_pointer_md(&alpha(), "/abs/.konductor/skills", true); - let (_, body) = split_frontmatter(&rendered, "pointer").unwrap(); - assert!( - body.contains("1. Read `/abs/.konductor/skills/alpha/SKILL.md`."), - "{body}" - ); - assert!(!body.contains("get_skill"), "{body}"); - assert!(!body.contains("MCP"), "{body}"); - assert!(!body.contains("konductor-skills"), "{body}"); - assert!(!body.contains("2."), "a single step: {body}"); + assert!(!body.contains('~'), "fallback path is absolute: {body}"); } #[test] @@ -444,7 +464,7 @@ mod tests { 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, false).unwrap(); + 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"]); @@ -462,7 +482,7 @@ mod tests { 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, false).unwrap_err(); + let err = install_kiro_skill_pointers(&harness_dir, &target_dir, None).unwrap_err(); assert!(err.contains("failed to read"), "{err}"); } @@ -470,11 +490,9 @@ mod tests { 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, false) - .unwrap() - .is_empty() - ); + assert!(install_kiro_skill_pointers(&harness_dir, &target_dir, None) + .unwrap() + .is_empty()); } #[test] @@ -490,17 +508,13 @@ mod tests { 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, false).unwrap(); + 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)), - false, - ) - .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()); @@ -512,7 +526,7 @@ mod tests { 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, false).unwrap(); + 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"); @@ -520,8 +534,7 @@ mod tests { 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)), false) - .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!( @@ -532,26 +545,47 @@ mod tests { } #[test] - fn changing_the_telemetry_setting_rewrites_every_pointer_in_place() { - let harness_dir = scratch_dir("switch-harness"); - let target_dir = scratch_dir("switch-target"); + 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.")); - let pointer = target_dir.join(".kiro/skills/ks-alpha/SKILL.md"); + assert!(skill_pointers_enabled(&target_dir)); + let first = install_kiro_skill_pointers(&harness_dir, &target_dir, None).unwrap(); + assert_eq!(first.len(), 1); - let on = install_kiro_skill_pointers(&harness_dir, &target_dir, None, false).unwrap(); - let on_text = std::fs::read_to_string(&pointer).unwrap(); - assert!(on_text.contains("Call `get_skill`"), "{on_text}"); - - let off = - install_kiro_skill_pointers(&harness_dir, &target_dir, Some(&manifest_of(&on)), true) + 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(); - let off_text = std::fs::read_to_string(&pointer).unwrap(); - assert!(!off_text.contains("get_skill"), "{off_text}"); - assert_ne!(on[0].sha256, off[0].sha256); - assert_eq!(paths(&on), paths(&off)); - - install_kiro_skill_pointers(&harness_dir, &target_dir, Some(&manifest_of(&off)), false) - .unwrap(); - assert_eq!(std::fs::read_to_string(&pointer).unwrap(), on_text); + 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 01049bb79..342c8197e 100644 --- a/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs +++ b/cli/konductor-rs/src/cli/install/kiro_cli_v3.rs @@ -56,7 +56,8 @@ use super::kiro_cli::{ 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, KONDUCTOR_DESTINATION_ROOT, + 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::{ @@ -319,7 +320,6 @@ impl InstallStrategy for KiroCliV3InstallStrategy { &harness_dir, target_dir, prior_manifest.as_ref(), - no_telemetry, )?); // Additive Claude branch, mirroring `SopInstallPhase::run`'s // dual-marker branch: this run is this strategy's own, but the @@ -572,7 +572,8 @@ fn copy_agent_files_rewriting_resources( ctx: &RewriteContext<'_>, file_names: Vec, ) -> Result<(Vec, bool), String> { - let passes = with_skill_pointer_pass(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; @@ -600,7 +601,8 @@ fn copy_agent_files_rewriting_resources( .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 = serde_json::from_str::(&contents).is_ok(); + 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) diff --git a/cli/konductor-rs/src/cli/install/phases.rs b/cli/konductor-rs/src/cli/install/phases.rs index fff74454e..f8545b09a 100644 --- a/cli/konductor-rs/src/cli/install/phases.rs +++ b/cli/konductor-rs/src/cli/install/phases.rs @@ -411,8 +411,7 @@ impl InstallPhase for SopInstallPhase { /// 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. The pointer body follows -/// `no_telemetry`. +/// chain: Claude installs get no pointers. pub(super) struct SkillPointerInstallPhase; impl InstallPhase for SkillPointerInstallPhase { @@ -427,13 +426,12 @@ impl InstallPhase for SkillPointerInstallPhase { _repo_root: Option<&Path>, prior_manifest: Option<&StrategyManifest>, _phase_outputs: &PhaseOutputs, - no_telemetry: bool, + _no_telemetry: bool, ) -> Result, InstallError> { Ok(super::kiro_cli::install_kiro_skill_pointers( staged_root, target_dir, prior_manifest, - no_telemetry, )?) } } 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 652d50c6c..2873f18f1 100644 --- a/cli/konductor-rs/src/cli/install/resource_rewrite/mod.rs +++ b/cli/konductor-rs/src/cli/install/resource_rewrite/mod.rs @@ -378,9 +378,10 @@ 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. Added -/// to the Kiro pipelines by `with_skill_pointer_pass`. The glob contains -/// `*`, so `agent_needs_mcp_server` ignores it, like the `ws-*` glob. +/// 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 { @@ -419,12 +420,16 @@ impl ResourceRewritePass for SkillPointerResourcePass { } } -/// Inserts `SkillPointerResourcePass` right after `SkillResourcePass`. +/// Inserts `SkillPointerResourcePass` right after `SkillResourcePass` +/// when `enabled`; otherwise returns `passes` unchanged. pub(super) fn with_skill_pointer_pass( mut passes: Vec>, + enabled: bool, ) -> Vec> { - let after_skill_pass = passes.len().min(2); - passes.insert(after_skill_pass, Box::new(SkillPointerResourcePass)); + if enabled { + let after_skill_pass = passes.len().min(2); + passes.insert(after_skill_pass, Box::new(SkillPointerResourcePass)); + } passes } @@ -829,7 +834,7 @@ mod tests { 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()); + 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(); @@ -876,9 +881,16 @@ mod tests { } #[test] - fn pointer_pass_follows_the_skill_pass() { - assert_eq!(with_skill_pointer_pass(standard_passes(true)).len(), 4); - assert_eq!(with_skill_pointer_pass(standard_passes_v3()).len(), 4); + 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. @@ -887,7 +899,7 @@ mod tests { 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)); + let passes = with_skill_pointer_pass(standard_passes(true), true); apply_all( &passes, &mut value, @@ -911,7 +923,7 @@ mod tests { 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)); + let passes = with_skill_pointer_pass(standard_passes(true), true); apply_all( &passes, &mut value, diff --git a/cli/konductor-rs/src/cli/uninstall.rs b/cli/konductor-rs/src/cli/uninstall.rs index 43d2e0280..6c20ed964 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 index 274393d15..49fbe0138 100644 --- a/cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs +++ b/cli/konductor-rs/tests/kiro_skill_pointers_e2e.rs @@ -3,9 +3,7 @@ // 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. A pointer's body -// follows the install's telemetry setting: `get_skill` with telemetry on, -// a direct file read with `--no-telemetry`. +// 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 @@ -166,21 +164,18 @@ fn install_with( } fn update(home: &Path, sink: &telemetry_test_sink::TelemetrySink, repo: &Path, target: &Path) { - update_with(home, sink, repo, target, &[]); -} - -fn update_with( - home: &Path, - sink: &telemetry_test_sink::TelemetrySink, - repo: &Path, - target: &Path, - extra: &[&str], -) { - let repo = repo.display().to_string(); - let target = target.display().to_string(); - let mut args = vec!["update", "--from", &repo, "--target", &target, "--force"]; - args.extend_from_slice(extra); - run_konductor(home, sink, &args); + 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) { @@ -210,6 +205,16 @@ fn agent_resources(target: &Path) -> Vec { .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") @@ -228,68 +233,30 @@ fn read_pointer(path: &Path) -> (serde_yaml::Value, String) { (serde_yaml::from_str(frontmatter).unwrap(), body.to_string()) } -/// How a pointer's body tells the agent to load the skill. -#[derive(Clone, Copy, PartialEq, Debug)] -enum Telemetry { - On, - Off, -} - -impl Telemetry { - fn install_args(self) -> &'static [&'static str] { - match self { - Telemetry::On => &[], - Telemetry::Off => &["--no-telemetry"], - } - } -} - -fn assert_pointer( - target: &Path, - skill: &str, - description: &str, - skills_dir: &str, - telemetry: Telemetry, -) { +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)); - let skill_file = format!("{skills_dir}/{skill}/SKILL.md"); assert!( - body.contains(&format!("`{skill_file}`")), - "body carries the file path {skill_file}: {body}" + body.contains("get_skill"), + "body names the MCP tool: {body}" ); assert!( - Path::new(&skill_file).is_file(), - "the file the pointer names exists: {skill_file}" + 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}" ); - match telemetry { - Telemetry::On => { - assert!(body.contains("get_skill"), "names the MCP tool: {body}"); - assert!( - body.contains("konductor-skills"), - "names the MCP server: {body}" - ); - assert!( - body.contains(&format!("name: \"{skill}\"")), - "names the real skill: {body}" - ); - assert!( - body.contains("Only if the `konductor-skills` server is not connected"), - "the file is the fallback: {body}" - ); - } - Telemetry::Off => { - assert!( - !body.contains("get_skill") && !body.contains("konductor-skills"), - "no MCP step: {body}" - ); - assert!(body.contains("Read `"), "reads the file directly: {body}"); - } - } } fn manifest_text(target: &Path) -> String { @@ -301,62 +268,57 @@ 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 { - for telemetry in [Telemetry::On, Telemetry::Off] { - let home = scratch_dir(&format!("home-{harness}-{telemetry:?}")); - let repo = synthed_repo(&home, &sink, "kiroCli"); - - install_with( - &home, - &sink, - &repo, - &home, - harness, - telemetry.install_args(), - ); - - assert!( - home.join(".konductor/installs").is_file(), - "{harness}: index went to the scratch HOME" - ); - let skills_dir = home.join(".konductor/skills").display().to_string(); - assert_pointer(&home, "alpha", PLAIN_DESCRIPTION, &skills_dir, telemetry); - assert_pointer(&home, "beta", FOLDED_DESCRIPTION, &skills_dir, telemetry); - 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, &skills_dir, telemetry); - - 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(); - } + 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_file_path_follows_a_target_that_is_not_home() { +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"); @@ -365,14 +327,9 @@ fn pointer_file_path_follows_a_target_that_is_not_home() { install(&home, &sink, &repo, &target, "kiro-v3"); - let skills_dir = target.join(".konductor/skills").display().to_string(); - assert_pointer( - &target, - "alpha", - PLAIN_DESCRIPTION, - &skills_dir, - Telemetry::On, - ); + 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(); @@ -410,49 +367,65 @@ fn every_kiro_agent_lists_the_pointer_glob_and_the_install_count_excludes_pointe } } -/// Both pointers of the example repo must carry `telemetry`'s body, and the -/// agent must still list the pointer glob. -fn assert_all_pointers(home: &Path, harness: &str, telemetry: Telemetry) { - let skills_dir = home.join(".konductor/skills").display().to_string(); - assert_pointer(home, "alpha", PLAIN_DESCRIPTION, &skills_dir, telemetry); - assert_pointer(home, "beta", FOLDED_DESCRIPTION, &skills_dir, telemetry); - assert!( - agent_resources(home).contains(&POINTER_GLOB.to_string()), - "{harness}: agent lists the glob" - ); -} - #[test] -fn pointers_follow_the_telemetry_setting_across_install_and_update() { +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-switch-{harness}")); + let home = scratch_dir(&format!("home-optout-{harness}")); let repo = synthed_repo(&home, &sink, "kiroCli"); install(&home, &sink, &repo, &home, harness); - assert_all_pointers(&home, harness, Telemetry::On); + assert!(has_pointer_dirs(&home), "{harness}: pointers by default"); - // Telemetry on to off: the install rewrites the pointers in place. - install_with(&home, &sink, &repo, &home, harness, &["--no-telemetry"]); - assert_all_pointers(&home, harness, Telemetry::Off); + // 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` carries the off setting forward. + // `update` respects the recorded choice. update(&home, &sink, &repo, &home); - assert_all_pointers(&home, harness, Telemetry::Off); + assert!( + !has_pointer_dirs(&home), + "{harness}: update kept the opt-out" + ); + assert!(!agent_resources(&home).contains(&POINTER_GLOB.to_string())); - // And back on. + // A plain install turns pointers back on. install(&home, &sink, &repo, &home, harness); - assert_all_pointers(&home, harness, Telemetry::On); - update(&home, &sink, &repo, &home); - assert_all_pointers(&home, harness, Telemetry::On); + assert!(has_pointer_dirs(&home), "{harness}: pointers back"); + assert!(agent_resources(&home).contains(&POINTER_GLOB.to_string())); - // `update --no-telemetry` switches this run to the direct read, and - // the next plain `update` switches back. - update_with(&home, &sink, &repo, &home, &["--no-telemetry"]); - assert_all_pointers(&home, harness, Telemetry::Off); - update(&home, &sink, &repo, &home); - assert_all_pointers(&home, harness, Telemetry::On); + // 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(); @@ -496,12 +469,23 @@ fn claude_install_writes_no_pointers() { let home = scratch_dir("home-claude"); let repo = synthed_repo(&home, &sink, "claudeCli"); - install(&home, &sink, &repo, &home, "claude"); + 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" diff --git a/docs/user-guide/reference.md b/docs/user-guide/reference.md index fb499184f..d530b10a9 100644 --- a/docs/user-guide/reference.md +++ b/docs/user-guide/reference.md @@ -93,7 +93,8 @@ Accepted before or after a subcommand. | `install` | `--from ` | path | No | Install from a local repo root with already-synthed content instead of fetching a release. A maintainer path — the documented workflows all install from a release | | `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. Kiro skill pointers then read the skill file directly instead of calling `get_skill` | +| `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` |