diff --git a/crates/but-error/src/lib.rs b/crates/but-error/src/lib.rs index 55ecc55875f..c40c9c18f5b 100644 --- a/crates/but-error/src/lib.rs +++ b/crates/but-error/src/lib.rs @@ -166,6 +166,9 @@ pub enum Code { /// the `but` CLI. Not a real failure — the frontend swaps it for a /// neutral info toast. CliInstallCancelled, + /// The user dismissed the macOS admin-privileges prompt when uninstalling + /// the `but` CLI. Like [`Code::CliInstallCancelled`], not a real failure. + CliUninstallCancelled, /// The GitHub access token was rejected. Currently only synthesized by /// the frontend when an Octokit response message starts with /// "Not Found -" — kept here so the wire-level `Code` enum is the diff --git a/crates/but-skill/src/cli_link.rs b/crates/but-skill/src/cli_link.rs index 2fee86c9093..2ef716938bb 100644 --- a/crates/but-skill/src/cli_link.rs +++ b/crates/but-skill/src/cli_link.rs @@ -16,11 +16,225 @@ pub fn get_cli_path() -> anyhow::Result { const UNIX_LINK_PATH: &str = "/usr/local/bin/but"; +/// Where the `but` symlink lives, or `None` on Windows where there is no +/// symlink-based install at all. +/// +/// When `E2E_TEST_APP_DATA_DIR` is set the link is redirected under that +/// directory, so tests can exercise install and uninstall without touching +/// `/usr/local/bin` — mirroring [`but_path::home_dir`]. +pub fn link_path() -> Option { + if cfg!(windows) { + return None; + } + if let Some(test_dir) = std::env::var_os("E2E_TEST_APP_DATA_DIR") { + return Some(std::path::PathBuf::from(test_dir).join("bin").join("but")); + } + Some(std::path::PathBuf::from(UNIX_LINK_PATH)) +} + pub enum InstallMode { AllowPrivilegeElevation, CurrentUserOnly, } +/// What currently sits at the CLI link path. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +#[serde(rename_all = "camelCase", tag = "status")] +pub enum CliLinkStatus { + /// A symlink pointing at the binary this app would install. + Installed, + /// A symlink pointing somewhere else. We refuse to remove it, since it is + /// most likely another GitButler channel's install. + InstalledElsewhere { + /// Where the existing link actually points. + actual: String, + }, + /// Nothing at the link path. + NotInstalled, + /// A regular file or directory sits there. Never ours to touch — a + /// package manager's real `but` binary looks exactly like this. + Blocked, + /// Windows, where there is no symlink install path. + Unsupported, +} + +/// Everything the UI needs to describe, install, and uninstall the CLI link. +#[derive(Debug, Clone, serde::Serialize)] +#[serde(rename_all = "camelCase")] +pub struct CliInstallState { + /// The `but` binary this app links to. + pub target_path: String, + /// Whether that binary is actually present. False in a dev build that + /// hasn't built `but` yet. + pub target_exists: bool, + /// The link location, absent on Windows. + pub link_path: Option, + /// What is at [`Self::link_path`] right now. + pub status: CliLinkStatus, +} + +impl CliInstallState { + /// Whether the CLI is installed and pointing at this app's binary. + pub fn is_installed(&self) -> bool { + matches!(self.status, CliLinkStatus::Installed) + } +} + +/// Inspect the CLI link without changing anything. +/// +/// Deliberately read-only: [`auto_fix_broken_but_cli_symlink`] already repairs +/// stale links at app startup, and a status query that silently rewrote the +/// filesystem would make the UI's "not installed" state unreproducible. +pub fn cli_install_state() -> anyhow::Result { + let cli_path = get_cli_path()?; + let target_exists = cli_path.exists(); + let target_path = cli_path.to_string_lossy().to_string(); + + let Some(link) = link_path() else { + return Ok(CliInstallState { + target_path, + target_exists, + link_path: None, + status: CliLinkStatus::Unsupported, + }); + }; + + let status = link_state(&link, &cli_path)?; + + Ok(CliInstallState { + target_path, + target_exists, + link_path: Some(link.to_string_lossy().to_string()), + status, + }) +} + +/// What sits at `link`, judged against the binary we would install. +/// +/// Split out from [`cli_install_state`] so the decision can be tested against +/// temporary paths without redirecting the real link location. +fn link_state(link: &std::path::Path, cli_path: &std::path::Path) -> anyhow::Result { + Ok(match std::fs::symlink_metadata(link) { + Err(err) if err.kind() == std::io::ErrorKind::NotFound => CliLinkStatus::NotInstalled, + Err(err) => return Err(err).context(format!("Failed to inspect {}", link.display())), + Ok(md) if !md.is_symlink() => CliLinkStatus::Blocked, + Ok(_) => { + let actual = std::fs::read_link(link) + .with_context(|| format!("Failed to read link {}", link.display()))?; + if actual == cli_path { + CliLinkStatus::Installed + } else { + CliLinkStatus::InstalledElsewhere { + actual: actual.to_string_lossy().to_string(), + } + } + } + }) +} + +/// Remove the `but` CLI symlink. +/// +/// Only ever removes a symlink we can identify as ours: one pointing at this +/// app's binary, or a dangling `but` link (the stale-install case +/// [`auto_fix_broken_but_cli_symlink`] exists to repair). A regular file, or a +/// link pointing at some other binary, is left alone and reported instead — +/// deleting a package manager's real `but` would be unrecoverable. +/// +/// Returns the resulting state so callers can refresh their UI in one round +/// trip. +pub fn uninstall_cli() -> anyhow::Result { + let Some(link) = link_path() else { + return cli_install_state(); + }; + uninstall_link(&link, &get_cli_path()?)?; + cli_install_state() +} + +/// The removal decision and action, against explicit paths. +/// +/// Returns `false` when there was nothing to remove. +fn uninstall_link(link: &std::path::Path, cli_path: &std::path::Path) -> anyhow::Result { + match link_state(link, cli_path)? { + CliLinkStatus::NotInstalled | CliLinkStatus::Unsupported => return Ok(false), + CliLinkStatus::Blocked => bail!( + "Refusing to remove '{}': it is a real file, not a symlink created by GitButler.", + link.display() + ), + CliLinkStatus::InstalledElsewhere { actual } => { + // A dangling link named `but` is still ours — that is exactly the + // stale state auto-repair handles. Anything else is not. + let dangling_but = !std::path::Path::new(&actual).exists() + && link.file_name().is_some_and(|name| { + name == std::ffi::OsStr::new("but") || name == std::ffi::OsStr::new("but.exe") + }); + if !dangling_but { + bail!( + "Refusing to remove '{}': it points at '{actual}', not at GitButler's `but` binary.", + link.display() + ); + } + } + CliLinkStatus::Installed => {} + } + + // Removing a symlink removes the link itself, never the binary it targets. + match std::fs::remove_file(link) { + Ok(()) => Ok(true), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => { + remove_link_with_privileges(link)?; + Ok(true) + } + Err(err) => Err(err).context(format!("Failed to remove {}", link.display())), + } +} + +/// Fall back to an authenticated `rm` when the link directory is not writable. +/// `/usr/local/bin` is usually user-writable after Homebrew, so most users +/// never see the prompt. +fn remove_link_with_privileges(link: &std::path::Path) -> anyhow::Result<()> { + if !cfg!(target_os = "macos") { + bail!( + "Would probably need to run \"rm -f '{}'\" with root permissions", + link.display() + ); + } + + let status = std::process::Command::new("/usr/bin/osascript") + .args([ + "-e", + &format!( + "do shell script \" rm -f \'{}\' \" with administrator privileges", + link.display() + ), + ]) + .stdout(std::process::Stdio::inherit()) + .stderr(std::process::Stdio::inherit()) + .status() + .context("Failed to run osascript")?; + + if status.success() { + Ok(()) + } else if status.code() == Some(1) { + // Same benign-abort convention as `do_install_cli`: exit 1 means the + // user dismissed the privileges prompt. + Err( + anyhow!("osascript exited with status 1").context(ErrorContext::new_static( + Code::CliUninstallCancelled, + "CLI uninstall cancelled", + )), + ) + } else { + Err(anyhow!( + "osascript exited with status {}", + status + .code() + .map(|c| c.to_string()) + .unwrap_or_else(|| "unknown".into()) + )) + } +} + pub fn do_install_cli(mode: InstallMode) -> anyhow::Result<()> { let cli_path = get_cli_path()?; #[cfg(windows)] @@ -28,22 +242,25 @@ pub fn do_install_cli(mode: InstallMode) -> anyhow::Result<()> { return install_cli_windows(cli_path); } - match std::fs::symlink_metadata(UNIX_LINK_PATH) { + #[cfg(not(windows))] + let link = link_path().context("No CLI link path on this platform")?; + #[cfg(not(windows))] + let link_display = link.display(); + + #[cfg(not(windows))] + match std::fs::symlink_metadata(&link) { Ok(md) => { if !md.is_symlink() { - bail!( - "Refusing to install symlink onto existing non-symlink at '{UNIX_LINK_PATH}'" - ); + bail!("Refusing to install symlink onto existing non-symlink at '{link_display}'"); } - let current_link = std::fs::read_link(UNIX_LINK_PATH) - .context(format!("error reading existing link: {UNIX_LINK_PATH}"))?; + let current_link = std::fs::read_link(&link) + .context(format!("error reading existing link: {link_display}"))?; if current_link == cli_path { return Ok(()); } ensure_cli_path_exists_prior_to_link(&cli_path)?; - #[cfg(not(windows))] - if std::fs::remove_file(UNIX_LINK_PATH) - .and_then(|_| std::os::unix::fs::symlink(&cli_path, UNIX_LINK_PATH)) + if std::fs::remove_file(&link) + .and_then(|_| std::os::unix::fs::symlink(&cli_path, &link)) .is_ok() { return Ok(()); @@ -51,8 +268,11 @@ pub fn do_install_cli(mode: InstallMode) -> anyhow::Result<()> { } Err(err) if err.kind() == std::io::ErrorKind::NotFound => { ensure_cli_path_exists_prior_to_link(&cli_path)?; - #[cfg(not(windows))] - if std::os::unix::fs::symlink(&cli_path, UNIX_LINK_PATH).is_ok() { + // The parent may not exist yet under a redirected test root. + if let Some(parent) = link.parent() { + let _ = std::fs::create_dir_all(parent); + } + if std::os::unix::fs::symlink(&cli_path, &link).is_ok() { return Ok(()); } } @@ -101,7 +321,7 @@ pub fn do_install_cli(mode: InstallMode) -> anyhow::Result<()> { } } else { Err(anyhow!( - "Would probably need to run \"ln -sf '{}' '{UNIX_LINK_PATH}'\"{privilege}", + "Would probably need to run \"ln -sf '{}' '{link_display}'\"{privilege}", cli_path.display(), privilege = if can_elevate_privileges { " with root permissions" @@ -157,7 +377,10 @@ fn install_cli_windows(cli_path: std::path::PathBuf) -> anyhow::Result<()> { } pub fn auto_fix_broken_but_cli_symlink() { - let Ok(absolute_link_destination) = std::fs::read_link(UNIX_LINK_PATH) else { + let Some(link) = link_path() else { + return; + }; + let Ok(absolute_link_destination) = std::fs::read_link(&link) else { return; }; if absolute_link_destination.exists() { @@ -167,12 +390,114 @@ pub fn auto_fix_broken_but_cli_symlink() { match do_install_cli(InstallMode::CurrentUserOnly) { Ok(_) => { tracing::info!( - "Successfully fixed symlink at {UNIX_LINK_PATH}, which pointed to non-existing location '{}'", + "Successfully fixed symlink at {}, which pointed to non-existing location '{}'", + link.display(), absolute_link_destination.display() ); } Err(err) => { - tracing::error!(?err, "Failed to fix symlink at {UNIX_LINK_PATH}"); + tracing::error!(?err, "Failed to fix symlink at {}", link.display()); } } } + +#[cfg(all(test, unix))] +mod tests { + use std::path::Path; + + use super::*; + + fn link_to(link: &Path, target: &Path) { + std::os::unix::fs::symlink(target, link).unwrap(); + } + + /// The whole point of uninstall: drop the link, keep the binary. Getting + /// this backwards would delete the user's `but` (or, with `builtin-but`, + /// the running app itself). + #[test] + fn removes_our_symlink_and_leaves_the_target_binary_intact() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("but-binary"); + std::fs::write(&target, b"#!/bin/sh\n").unwrap(); + let link = dir.path().join("but"); + link_to(&link, &target); + + assert!(uninstall_link(&link, &target).unwrap(), "it removed a link"); + assert!(link.symlink_metadata().is_err(), "the symlink is gone"); + assert!(target.is_file(), "the binary it pointed at still exists"); + } + + #[test] + fn refuses_a_regular_file() { + let dir = tempfile::tempdir().unwrap(); + let link = dir.path().join("but"); + std::fs::write(&link, b"a real binary from a package manager").unwrap(); + + let err = uninstall_link(&link, &dir.path().join("but-binary")).unwrap_err(); + assert!( + err.to_string().contains("not a symlink"), + "explains why it refused, got: {err}" + ); + assert!(link.is_file(), "the real binary is untouched"); + } + + #[test] + fn refuses_a_symlink_pointing_at_another_binary() { + let dir = tempfile::tempdir().unwrap(); + let other = dir.path().join("some-other-but"); + std::fs::write(&other, b"#!/bin/sh\n").unwrap(); + let link = dir.path().join("but"); + link_to(&link, &other); + + let err = uninstall_link(&link, &dir.path().join("but-binary")).unwrap_err(); + assert!( + err.to_string().contains("it points at"), + "names the unexpected target, got: {err}" + ); + assert!(link.symlink_metadata().is_ok(), "the link is untouched"); + assert!(other.is_file(), "the other binary is untouched"); + } + + /// A link left behind by a previous install whose binary has since moved. + /// That is the state `auto_fix_broken_but_cli_symlink` repairs, so it is + /// unambiguously ours to remove. + #[test] + fn removes_a_dangling_but_link() { + let dir = tempfile::tempdir().unwrap(); + let link = dir.path().join("but"); + link_to(&link, &dir.path().join("gone-away")); + + assert!(uninstall_link(&link, &dir.path().join("but-binary")).unwrap()); + assert!(link.symlink_metadata().is_err(), "the stale link is gone"); + } + + #[test] + fn is_a_no_op_when_nothing_is_installed() { + let dir = tempfile::tempdir().unwrap(); + let link = dir.path().join("but"); + + assert!( + !uninstall_link(&link, &dir.path().join("but-binary")).unwrap(), + "reports that there was nothing to remove" + ); + } + + #[test] + fn link_state_distinguishes_ours_from_everything_else() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("but-binary"); + std::fs::write(&target, b"#!/bin/sh\n").unwrap(); + let link = dir.path().join("but"); + + assert_eq!( + link_state(&link, &target).unwrap(), + CliLinkStatus::NotInstalled + ); + + link_to(&link, &target); + assert_eq!( + link_state(&link, &target).unwrap(), + CliLinkStatus::Installed + ); + } +} diff --git a/crates/but-skill/src/files.rs b/crates/but-skill/src/files.rs index c1db3539474..f84f1cd0034 100644 --- a/crates/but-skill/src/files.rs +++ b/crates/but-skill/src/files.rs @@ -177,3 +177,154 @@ fn append_managed_block(existing: &str, block: &str) -> String { updated.push_str(&block); updated } + +/// The contents of the managed block in `existing`, markers included, or +/// `None` when there is no block. +/// +/// Errors on a malformed marker pair for the same reason +/// [`upsert_managed_block`] does: a partial block is not ours to interpret. +pub fn read_managed_block(existing: &str) -> Result> { + Ok(managed_block_spans(existing)? + .first() + .map(|span| existing[span.clone()].to_string())) +} + +/// Like [`read_managed_block`], but reads `path` first. A missing file has no +/// block. +pub fn read_managed_block_file(path: &Path) -> Result> { + match std::fs::read_to_string(path) { + Ok(content) => read_managed_block(&content), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(err) => Err(err).with_context(|| format!("Failed to read {}", path.display())), + } +} + +/// Splice every managed block out of `existing`, returning `None` when there +/// was none to remove. +/// +/// Inherits [`managed_block_spans`]'s protections: markers quoted in prose or +/// shown inside a fenced code block are left alone, and a partial or reversed +/// marker pair errors rather than guessing at a span to delete. +pub fn remove_managed_block(existing: &str) -> Result> { + let spans = managed_block_spans(existing)?; + if spans.is_empty() { + return Ok(None); + } + + let mut updated = String::with_capacity(existing.len()); + let mut copied = 0; + for span in spans { + updated.push_str(&existing[copied..span.start]); + copied = span.end; + // Take the block's own line terminator with it, so removing a block + // does not leave a widening gap behind each time. + if existing[copied..].starts_with("\r\n") { + copied += 2; + } else if existing[copied..].starts_with('\n') { + copied += 1; + } + // `append_managed_block` separates the block from preceding text with + // a blank line. Take that back too, but only when leaving it would + // strand a trailing or doubled blank line — otherwise a block that + // merely follows a blank line would lose it. + let rest_starts_new_line = + existing[copied..].is_empty() || existing[copied..].starts_with('\n'); + if rest_starts_new_line { + if updated.ends_with("\r\n\r\n") { + updated.truncate(updated.len() - 2); + } else if updated.ends_with("\n\n") { + updated.truncate(updated.len() - 1); + } + } + } + updated.push_str(&existing[copied..]); + Ok(Some(updated)) +} + +/// Remove the managed block from `path`, leaving the rest of the file intact. +/// +/// Never deletes the file, even when nothing but the block was in it: these +/// are usually git-tracked files the user owns, and removing one is a +/// surprising, visible side effect of uninstalling a skill. +/// +/// Returns whether anything changed. +pub fn remove_managed_block_file(path: &Path) -> Result { + let original = match std::fs::read_to_string(path) { + Ok(content) => content, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(false), + Err(err) => return Err(err).with_context(|| format!("Failed to read {}", path.display())), + }; + let Some(updated) = remove_managed_block(&original)? else { + return Ok(false); + }; + std::fs::write(path, updated).with_context(|| format!("Failed to write {}", path.display()))?; + Ok(true) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn managed(body: &str) -> String { + format!("{MANAGED_BLOCK_START}\n{body}\n{MANAGED_BLOCK_END}\n") + } + + #[test] + fn removes_the_block_and_nothing_else() { + let existing = format!("# Rules\n\n{}\nAfter.\n", managed("- policy")); + let updated = remove_managed_block(&existing).unwrap().unwrap(); + assert_eq!(updated, "# Rules\n\nAfter.\n"); + } + + #[test] + fn reports_no_change_when_there_is_no_block() { + assert_eq!(remove_managed_block("# Just my notes\n").unwrap(), None); + } + + /// The same refusal `upsert_managed_block` makes: half a marker pair is + /// not a span we can safely delete. + #[test] + fn refuses_a_partial_marker_pair() { + let existing = format!("# Rules\n\n{MANAGED_BLOCK_START}\n- policy\n"); + assert!(remove_managed_block(&existing).is_err()); + } + + /// A marker shown as documentation inside a fenced block is not a real + /// delimiter, so nothing should be spliced out around it. + #[test] + fn ignores_markers_inside_a_fenced_code_block() { + let existing = + format!("# Docs\n\n```\n{MANAGED_BLOCK_START}\n{MANAGED_BLOCK_END}\n```\n\nEnd.\n"); + assert_eq!(remove_managed_block(&existing).unwrap(), None); + } + + #[test] + fn preserves_crlf_content_around_the_block() { + let existing = format!( + "# Rules\r\n\r\n{MANAGED_BLOCK_START}\r\n- policy\r\n{MANAGED_BLOCK_END}\r\nAfter.\r\n" + ); + let updated = remove_managed_block(&existing).unwrap().unwrap(); + assert_eq!(updated, "# Rules\r\n\r\nAfter.\r\n"); + } + + /// Uninstalling a skill must not delete a file the user owns and git + /// tracks, even if the block was all it contained. + #[test] + fn leaves_an_emptied_file_in_place() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("AGENTS.md"); + std::fs::write(&path, managed("- policy")).unwrap(); + + assert!(remove_managed_block_file(&path).unwrap()); + assert!(path.is_file(), "the file still exists"); + assert_eq!(std::fs::read_to_string(&path).unwrap(), ""); + } + + #[test] + fn round_trips_with_upsert() { + let original = "# Rules\n\nMy own notes.\n"; + let block = managed("- policy"); + let written = upsert_managed_block(original, &block).unwrap(); + assert_eq!(remove_managed_block(&written).unwrap().unwrap(), original); + } +} diff --git a/crates/but-skill/src/framework.rs b/crates/but-skill/src/framework.rs new file mode 100644 index 00000000000..87c22b10994 --- /dev/null +++ b/crates/but-skill/src/framework.rs @@ -0,0 +1,394 @@ +//! The coding-agent frameworks GitButler knows how to set up. +//! +//! [`SKILL_FORMATS`](crate::format::SKILL_FORMATS) answers "where does a skill +//! install"; this answers "who is this agent, does the user actually use it, +//! and which file holds its steering instructions". They are separate tables +//! because several frameworks have *two* skill formats (a repo-local and a +//! global one) while detection markers and instruction files are per-framework. +//! +//! Markers are written out explicitly rather than derived from install paths. +//! Deriving them looks tempting but is wrong: GitHub Copilot's local skill path +//! is `.github/skills/gitbutler`, so a derived repo marker would be `.github` — +//! present in essentially every repository on GitHub. + +use std::path::Path; + +use crate::{ + format::{SKILL_FORMATS, SkillFormat}, + plan::{RepoInfo, Scope, join_components}, +}; + +/// A coding agent GitButler can install a skill for. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Framework { + /// Stable kebab-case identifier. This is the key mutating APIs take, so it + /// must not change once shipped. + pub id: &'static str, + /// The [`SkillFormat::name`] this framework installs under. + pub name: &'static str, + /// One-line description for a settings row. + pub description: &'static str, + /// Config location under `$HOME` whose presence means the user has this + /// agent set up. `None` for formats with nothing agent-specific to find. + pub home_marker: Option<&'static [&'static str]>, + /// An unambiguous per-repository marker. `AGENTS.md` is deliberately never + /// used: it is shared by many agents, so it proves nothing about any one. + pub repo_marker: Option<&'static [&'static str]>, + /// The repository-scoped instruction file the managed policy block goes in. + pub repo_instructions: &'static [&'static str], + /// The global instruction file, when this agent has a supported one. + /// `None` means the policy can only be shown for manual copying. + pub global_instructions: Option<&'static [&'static str]>, +} + +/// Shorthand for the common case: an agent whose steering lives in the shared +/// repo-level `AGENTS.md` and which has no known global instruction file. +const fn agents_md( + id: &'static str, + name: &'static str, + description: &'static str, + home_marker: Option<&'static [&'static str]>, +) -> Framework { + Framework { + id, + name, + description, + home_marker, + repo_marker: None, + repo_instructions: &["AGENTS.md"], + global_instructions: None, + } +} + +/// Every framework, in the order the UI should present them. +/// +/// The first eight are the ones `but agent setup` offers interactively; the +/// rest can install skills but are not part of the wizard's curated list. +pub const FRAMEWORKS: &[Framework] = &[ + Framework { + id: "codex", + name: "Codex", + description: "Install the Codex skill and write Codex AGENTS.md steering.", + home_marker: Some(&[".codex"]), + repo_marker: None, + repo_instructions: &["AGENTS.md"], + global_instructions: Some(&[".codex", "AGENTS.md"]), + }, + Framework { + id: "claude-code", + name: "Claude Code", + description: "Install the Claude Code skill and write Claude instruction files.", + home_marker: Some(&[".claude"]), + repo_marker: Some(&["CLAUDE.md"]), + repo_instructions: &["CLAUDE.md"], + global_instructions: Some(&[".claude", "rules", "gitbutler.md"]), + }, + Framework { + // Cursor reads AGENTS.md without rule metadata, so prefer it over a + // `.cursor/rules/*.mdc` file, which would need YAML frontmatter + // (e.g. `alwaysApply: true`) to be loaded automatically. + id: "cursor", + name: "Cursor", + description: "Install the Cursor skill and write supported Cursor project steering.", + home_marker: Some(&[".cursor"]), + repo_marker: Some(&[".cursor"]), + repo_instructions: &["AGENTS.md"], + global_instructions: None, + }, + Framework { + id: "github-copilot", + name: "GitHub Copilot", + description: "Install the Copilot skill and write supported Copilot instructions.", + home_marker: Some(&[".copilot"]), + repo_marker: Some(&[".github", "copilot-instructions.md"]), + repo_instructions: &[".github", "copilot-instructions.md"], + global_instructions: Some(&[".copilot", "copilot-instructions.md"]), + }, + Framework { + id: "windsurf", + name: "Windsurf", + description: "Install the Windsurf skill and write Cascade-compatible AGENTS.md steering.", + home_marker: Some(&[".codeium"]), + repo_marker: None, + repo_instructions: &["AGENTS.md"], + global_instructions: Some(&[".codeium", "windsurf", "memories", "global_rules.md"]), + }, + Framework { + id: "opencode", + name: "OpenCode", + description: "Install the OpenCode skill and write OpenCode AGENTS.md steering.", + home_marker: Some(&[".config", "opencode"]), + repo_marker: None, + repo_instructions: &["AGENTS.md"], + global_instructions: Some(&[".config", "opencode", "AGENTS.md"]), + }, + Framework { + id: "poolside", + name: "Poolside", + description: "Install the Poolside skill and write Poolside AGENTS.md steering.", + home_marker: Some(&[".config", "poolside"]), + repo_marker: Some(&[".poolside"]), + repo_instructions: &["AGENTS.md"], + global_instructions: Some(&[".config", "poolside", "AGENTS.md"]), + }, + Framework { + // The shared `.agents` format has no agent-specific config to detect. + id: "agent-skills", + name: "Agent Skills", + description: "Install the shared .agents skill format and write generic AGENTS.md steering.", + home_marker: None, + repo_marker: None, + repo_instructions: &["AGENTS.md"], + global_instructions: None, + }, + // Frameworks below install skills but are not offered by the CLI wizard. + // None has a documented global instruction file we are confident writing + // to, so their policy is shown for manual copying rather than guessed at. + Framework { + id: "kiro", + name: "Kiro", + description: "Install the Kiro skill.", + home_marker: Some(&[".kiro"]), + repo_marker: Some(&[".kiro"]), + repo_instructions: &["AGENTS.md"], + global_instructions: None, + }, + Framework { + id: "junie", + name: "Junie", + description: "Install the Junie skill.", + home_marker: Some(&[".junie"]), + repo_marker: Some(&[".junie"]), + repo_instructions: &["AGENTS.md"], + global_instructions: None, + }, + agents_md( + "gemini-cli", + "Gemini CLI", + "Install the Gemini CLI skill.", + Some(&[".gemini"]), + ), + agents_md( + "augment", + "Augment", + "Install the Augment skill.", + Some(&[".augment"]), + ), + agents_md( + "antigravity", + "Antigravity", + "Install the Antigravity skill.", + Some(&[".gemini", "antigravity"]), + ), + agents_md( + "universal-agents", + "Universal Agents", + "Install the shared ~/.config/agents skill format.", + Some(&[".config", "agents"]), + ), + agents_md( + "crush", + "Crush", + "Install the Crush skill.", + Some(&[".config", "crush"]), + ), + agents_md( + "goose", + "Goose", + "Install the Goose skill.", + Some(&[".config", "goose"]), + ), + agents_md( + "roo-code", + "Roo Code", + "Install the Roo Code skill.", + Some(&[".roo"]), + ), + agents_md("trae", "Trae", "Install the Trae skill.", Some(&[".trae"])), + agents_md( + "tabnine-cli", + "Tabnine CLI", + "Install the Tabnine CLI skill.", + Some(&[".tabnine"]), + ), + agents_md("pi", "Pi", "Install the Pi skill.", Some(&[".pi"])), + agents_md( + "devin", + "Devin", + "Install the Devin skill.", + Some(&[".config", "devin"]), + ), +]; + +/// Look a framework up by its stable id. +pub fn framework_by_id(id: &str) -> Option<&'static Framework> { + FRAMEWORKS.iter().find(|framework| framework.id == id) +} + +/// Look a framework up by its [`SkillFormat::name`]. +pub fn framework_by_name(name: &str) -> Option<&'static Framework> { + FRAMEWORKS.iter().find(|framework| framework.name == name) +} + +impl Framework { + /// The skill formats this framework offers at `global` scope, if any. + pub fn format(&self, global: bool) -> Option<&'static SkillFormat> { + SKILL_FORMATS + .iter() + .find(|format| format.name == self.name && format.is_available_for(global)) + } + + /// Whether a skill can be installed for this framework at `scope`. + pub fn supports(&self, scope: Scope) -> bool { + match scope { + Scope::Global => self.format(true).is_some(), + Scope::Repository => self.format(false).is_some(), + Scope::Both => self.format(true).is_some() || self.format(false).is_some(), + } + } + + /// The instruction file for `scope`, relative components only. `None` when + /// this framework has no supported file at that scope. + pub fn instruction_components(&self, scope: Scope) -> Option<&'static [&'static str]> { + match scope { + Scope::Repository => Some(self.repo_instructions), + Scope::Global => self.global_instructions, + // `Both` is expanded into single scopes before this is reached. + Scope::Both => None, + } + } + + /// Whether this agent looks like it is already in use on this machine, so + /// the UI can pre-select it. Looks for the agent's config under `$HOME`, + /// then for an unambiguous per-repository marker, then for a GitButler + /// skill already installed for it — the last makes a re-run re-select + /// agents that `but skill` previously set up. + pub fn in_use(&self, home: Option<&Path>, repo: Option<&RepoInfo>) -> bool { + self.detected_globally(home) || self.detected_in_repo(repo) + } + + /// Whether the user's home directory shows this agent in use. + pub fn detected_globally(&self, home: Option<&Path>) -> bool { + let Some(home) = home else { return false }; + marker_exists(home, self.home_marker) + || marker_exists(home, self.skill_path_components(Scope::Global)) + } + + /// Whether the repository shows this agent in use. + pub fn detected_in_repo(&self, repo: Option<&RepoInfo>) -> bool { + let Some(repo) = repo else { return false }; + marker_exists(&repo.root, self.repo_marker) + || marker_exists(&repo.root, self.skill_path_components(Scope::Repository)) + } + + /// Where this framework's skill installs, relative to a base directory. + pub fn skill_path_components(&self, scope: Scope) -> Option<&'static [&'static str]> { + self.format(matches!(scope, Scope::Global)) + .map(|format| format.path_components) + } +} + +/// Whether `base` joined with `marker`'s components exists on disk. A `None` +/// marker (the agent has no such location) is never present. +fn marker_exists(base: &Path, marker: Option<&'static [&'static str]>) -> bool { + marker.is_some_and(|components| join_components(base, components).exists()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn every_framework_has_a_skill_format_in_some_scope() { + for framework in FRAMEWORKS { + assert!( + framework.supports(Scope::Both), + "{} should install somewhere", + framework.id + ); + } + } + + #[test] + fn ids_and_names_are_unique() { + for (i, framework) in FRAMEWORKS.iter().enumerate() { + for other in &FRAMEWORKS[i + 1..] { + assert_ne!(framework.id, other.id, "duplicate id"); + assert_ne!(framework.name, other.name, "duplicate name"); + } + } + } + + #[test] + fn every_skill_format_name_maps_to_a_framework() { + for format in SKILL_FORMATS { + assert!( + framework_by_name(format.name).is_some(), + "skill format {} has no framework entry", + format.name + ); + } + } + + /// `.github` exists in almost every repository on GitHub, so it must never + /// on its own imply the user works with Copilot. + #[test] + fn a_bare_dot_github_directory_is_not_a_copilot_marker() { + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(dir.path().join(".github")).unwrap(); + let repo = RepoInfo { + root: dir.path().to_path_buf(), + needs_setup: false, + }; + + let copilot = framework_by_id("github-copilot").unwrap(); + assert!( + !copilot.detected_in_repo(Some(&repo)), + "a bare .github directory should not count" + ); + + std::fs::write( + dir.path().join(".github").join("copilot-instructions.md"), + "hi", + ) + .unwrap(); + assert!( + copilot.detected_in_repo(Some(&repo)), + "the instructions file is the real marker" + ); + } + + /// `AGENTS.md` is shared by many agents, so it proves nothing about any one. + #[test] + fn a_shared_agents_md_is_not_a_marker_for_anyone() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("AGENTS.md"), "shared").unwrap(); + let repo = RepoInfo { + root: dir.path().to_path_buf(), + needs_setup: false, + }; + + for framework in FRAMEWORKS { + assert!( + !framework.detected_in_repo(Some(&repo)), + "{} should not be detected from a shared AGENTS.md", + framework.id + ); + } + } + + #[test] + fn home_config_directory_marks_an_agent_in_use() { + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(dir.path().join(".claude")).unwrap(); + + let claude = framework_by_id("claude-code").unwrap(); + assert!(claude.detected_globally(Some(dir.path()))); + assert!( + !framework_by_id("codex") + .unwrap() + .detected_globally(Some(dir.path())), + "an unrelated agent's marker must not match" + ); + } +} diff --git a/crates/but-skill/src/install.rs b/crates/but-skill/src/install.rs index 658010b1b2c..47dc69393ae 100644 --- a/crates/but-skill/src/install.rs +++ b/crates/but-skill/src/install.rs @@ -85,3 +85,167 @@ fn write_skill_file(path: &std::path::Path, content: &[u8], name: &str) -> Resul ) }) } + +/// What [`remove_skill_files`] managed to clean up. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +#[serde(rename_all = "camelCase", tag = "outcome")] +pub enum RemovalOutcome { + /// The skill files and their directory are gone. + Removed, + /// The skill files are gone, but the directory still holds files we did + /// not put there, so it was left in place. + PartiallyRemoved { + /// Names of the entries left behind, for the UI to report. + remaining: Vec, + }, +} + +/// Remove an installed GitButler skill from `install_path`. +/// +/// Refuses anything whose `SKILL.md` does not identify it as ours, using the +/// same check discovery uses — so uninstall and discovery can never disagree +/// about what belongs to GitButler. +/// +/// Only the files this crate writes are deleted, and the directory itself is +/// removed non-recursively. If the user kept notes alongside the skill, or a +/// newer version wrote a file this one does not know about, the directory +/// survives and is reported via [`RemovalOutcome::PartiallyRemoved`]. That is +/// the difference between uninstalling a skill and deleting a user's folder. +/// +/// The parent `skills/` directory belongs to the agent, not to us, and is +/// never touched. +pub fn remove_skill_files(install_path: &std::path::Path) -> Result { + if !crate::status::is_gitbutler_skill(&install_path.join("SKILL.md")) { + anyhow::bail!( + "Refusing to remove {}: it does not contain a GitButler skill.", + install_path.display() + ); + } + + for file in SKILL_FILES { + let path = file.get_install_path(install_path); + match std::fs::remove_file(&path) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => { + return Err(err).with_context(|| format!("Failed to remove {}", path.display())); + } + } + } + + // `references/` only ever holds our files, so an empty one goes too. A + // non-empty one means the user put something there. + let references = install_path.join("references"); + if references.is_dir() && dir_entry_names(&references)?.is_empty() { + std::fs::remove_dir(&references) + .with_context(|| format!("Failed to remove {}", references.display()))?; + } + + let remaining = dir_entry_names(install_path)?; + if !remaining.is_empty() { + return Ok(RemovalOutcome::PartiallyRemoved { remaining }); + } + + std::fs::remove_dir(install_path) + .with_context(|| format!("Failed to remove {}", install_path.display()))?; + Ok(RemovalOutcome::Removed) +} + +/// The names of everything directly inside `dir`, sorted for stable reporting. +fn dir_entry_names(dir: &std::path::Path) -> Result> { + let mut names: Vec = std::fs::read_dir(dir) + .with_context(|| format!("Failed to read {}", dir.display()))? + .flatten() + .map(|entry| entry.file_name().to_string_lossy().to_string()) + .collect(); + names.sort(); + Ok(names) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn install(dir: &std::path::Path) -> std::path::PathBuf { + let path = dir.join(".claude").join("skills").join("gitbutler"); + write_skill_files(&path).unwrap(); + path + } + + #[test] + fn removes_an_installed_skill_and_its_directory() { + let temp = tempfile::tempdir().unwrap(); + let path = install(temp.path()); + + assert_eq!(remove_skill_files(&path).unwrap(), RemovalOutcome::Removed); + assert!(!path.exists(), "the skill directory is gone"); + assert!( + path.parent().unwrap().is_dir(), + "the agent's own skills directory is left alone" + ); + } + + /// Discovery accepts any folder name and identifies a skill by its + /// frontmatter, so removal must refuse anything that is not ours — even + /// when it sits exactly where a GitButler skill would. + #[test] + fn refuses_a_directory_that_is_not_a_gitbutler_skill() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("some-other-skill"); + std::fs::create_dir_all(&path).unwrap(); + std::fs::write(path.join("SKILL.md"), "---\nname: something-else\n---\n").unwrap(); + + let err = remove_skill_files(&path).unwrap_err(); + assert!( + err.to_string() + .contains("does not contain a GitButler skill"), + "explains the refusal, got: {err}" + ); + assert!(path.join("SKILL.md").is_file(), "nothing was deleted"); + } + + #[test] + fn refuses_a_directory_with_no_skill_at_all() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("empty"); + std::fs::create_dir_all(&path).unwrap(); + + assert!(remove_skill_files(&path).is_err()); + assert!(path.is_dir(), "the directory survives"); + } + + /// Anything the user added alongside the skill must survive, and the UI + /// needs to be told so it can say the folder is still there. + #[test] + fn keeps_and_reports_files_we_did_not_write() { + let temp = tempfile::tempdir().unwrap(); + let path = install(temp.path()); + std::fs::write(path.join("my-notes.md"), "personal").unwrap(); + + assert_eq!( + remove_skill_files(&path).unwrap(), + RemovalOutcome::PartiallyRemoved { + remaining: vec!["my-notes.md".to_string()], + } + ); + assert!(path.is_dir(), "the directory survives"); + assert!(path.join("my-notes.md").is_file(), "the note survives"); + assert!(!path.join("SKILL.md").exists(), "our files are gone"); + } + + #[test] + fn keeps_a_references_directory_holding_foreign_files() { + let temp = tempfile::tempdir().unwrap(); + let path = install(temp.path()); + std::fs::write(path.join("references").join("mine.md"), "personal").unwrap(); + + assert_eq!( + remove_skill_files(&path).unwrap(), + RemovalOutcome::PartiallyRemoved { + remaining: vec!["references".to_string()], + } + ); + assert!(path.join("references").join("mine.md").is_file()); + assert!(!path.join("references").join("concepts.md").exists()); + } +} diff --git a/crates/but-skill/src/lib.rs b/crates/but-skill/src/lib.rs index a9c5fbba74b..13d6197416e 100644 --- a/crates/but-skill/src/lib.rs +++ b/crates/but-skill/src/lib.rs @@ -13,6 +13,7 @@ pub mod files; pub mod format; #[cfg(test)] mod format_tests; +pub mod framework; pub mod freshness; pub mod install; pub mod plan; diff --git a/crates/but-skill/src/policy.rs b/crates/but-skill/src/policy.rs index 3144619246c..f1335cc9767 100644 --- a/crates/but-skill/src/policy.rs +++ b/crates/but-skill/src/policy.rs @@ -98,6 +98,26 @@ impl WorkflowOption { matches!(self, Self::PushToTarget) } + /// The `###` heading this option renders as inside the managed block. + /// + /// Single-sourced because [`parse_managed_policy_block`] maps these + /// headings back to options: reword one here and the parser follows, + /// instead of silently failing to recognise existing installs. + pub fn section_title(self) -> &'static str { + match self { + Self::FoldFixes => "Amend local fixes into the right commits", + Self::SuggestSplits => "Split unrelated changes into separate commits", + Self::StackedBranches => "Create stacked pull requests", + Self::AutoUpdate => "Update from the target branch automatically", + Self::DraftPrs => "Open draft pull requests by default", + Self::PushToTarget => "Skip pull requests and land onto the target", + Self::PublishPhrase => "Publish on a shortcut phrase", + Self::BranchPattern => "Branch naming", + Self::CommitConvention => "Commit message convention", + Self::CommitAfterTurn => "Commit checkpoints after each turn", + } + } + /// Help shown for a repo-local-only option when the current setup is not /// scoped to a single repository: spells out how to enable it and what it /// does. @@ -121,7 +141,7 @@ impl Default for WizardAnswers { .into_iter() .filter(|option| option.default_selected()) .collect(), - publish_phrase: "ship it".to_string(), + publish_phrase: default_publish_phrase().to_string(), branch_pattern: None, commit_convention: None, } @@ -132,6 +152,41 @@ impl WizardAnswers { pub fn has(&self, option: WorkflowOption) -> bool { self.selected.contains(&option) } + + /// The answers as they survive a render/parse round trip. + /// + /// Rendering is not injective: the branch-pattern and commit-convention + /// sections render from their value being set rather than from the option + /// being selected, and a publish phrase is only written when its option is + /// on. Normalizing resolves those disagreements the same way rendering + /// does, so a settings UI that saves and reloads sees a stable value + /// instead of a checkbox that silently un-checks itself. + pub fn normalized(&self) -> Self { + let mut selected: Vec<_> = WorkflowOption::ALL + .into_iter() + .filter(|option| match option { + // These two render from the value, not the checkbox. + WorkflowOption::BranchPattern => self.branch_pattern.is_some(), + WorkflowOption::CommitConvention => self.commit_convention.is_some(), + other => self.has(*other), + }) + .collect(); + selected.dedup(); + + let publish_phrase = if selected.contains(&WorkflowOption::PublishPhrase) { + self.publish_phrase.clone() + } else { + // Not rendered, so it cannot be read back; keep it predictable. + default_publish_phrase().to_string() + }; + + Self { + selected, + publish_phrase, + branch_pattern: self.branch_pattern.clone(), + commit_convention: self.commit_convention.clone(), + } + } } /// The selected-change commit fast-path rule. Named so `super::cleanup` can @@ -169,7 +224,7 @@ pub fn render_managed_policy_block(answers: &WizardAnswers) -> String { if answers.has(WorkflowOption::FoldFixes) { write_section( &mut body, - "Amend local fixes into the right commits", + WorkflowOption::FoldFixes.section_title(), &[ "For small cleanup or follow-up fixes, amend an unpublished local commit when the change clearly belongs with that commit's intent.", "Do not create tiny fixup commits unless the user asks.", @@ -181,7 +236,7 @@ pub fn render_managed_policy_block(answers: &WizardAnswers) -> String { if answers.has(WorkflowOption::SuggestSplits) { write_section( &mut body, - "Split unrelated changes into separate commits", + WorkflowOption::SuggestSplits.section_title(), &[ "If one file contains unrelated changes, split them by hunk instead of committing the whole file.", "Keep tests with the behavior they verify.", @@ -193,7 +248,7 @@ pub fn render_managed_policy_block(answers: &WizardAnswers) -> String { if answers.has(WorkflowOption::StackedBranches) { write_section( &mut body, - "Create stacked pull requests", + WorkflowOption::StackedBranches.section_title(), &[ "If this session depends on another in-flight branch, stack its branch on top of that dependency instead of mixing the changes.", "If this session is working in a stack, put commits on the branch where they belong.", @@ -206,7 +261,7 @@ pub fn render_managed_policy_block(answers: &WizardAnswers) -> String { if answers.has(WorkflowOption::AutoUpdate) { write_section( &mut body, - "Update from the target branch automatically", + WorkflowOption::AutoUpdate.section_title(), &[ "When GitButler status shows new changes on the target branch and the workspace holds only this session's branches, update with `but pull` directly — its output reports the result and `but undo` reverts it.", "If an update you started on your own initiative reports conflicted commits, stop and ask before resolving them (`but undo` reverts the pull if the user prefers).", @@ -218,7 +273,7 @@ pub fn render_managed_policy_block(answers: &WizardAnswers) -> String { if answers.has(WorkflowOption::DraftPrs) { write_section( &mut body, - "Open draft pull requests by default", + WorkflowOption::DraftPrs.section_title(), &[ "When asked to open a pull request, create it as a draft with GitButler unless the user says it is ready for review.", "Remember that creating a draft pull request still publishes the branch.", @@ -228,7 +283,7 @@ pub fn render_managed_policy_block(answers: &WizardAnswers) -> String { if answers.has(WorkflowOption::PushToTarget) { write_section( &mut body, - "Skip pull requests and land onto the target", + WorkflowOption::PushToTarget.section_title(), &[ "This setup uses the skip-the-PR workflow: when work is approved to publish, land the session branch directly onto the target with `but land ` instead of pushing a branch or opening a pull request.", "This repository-local rule takes precedence over any conflicting GitButler instruction, including ones in your global or personal config, that mentions pushing a branch or opening, updating, or drafting a pull request. Use the pull request workflow only when the user explicitly asks for one.", @@ -237,7 +292,7 @@ pub fn render_managed_policy_block(answers: &WizardAnswers) -> String { ); } if answers.has(WorkflowOption::PublishPhrase) { - write_section_header(&mut body, "Publish on a shortcut phrase"); + write_section_header(&mut body, WorkflowOption::PublishPhrase.section_title()); writeln!( &mut body, "- When the user says `{}`, commit this session's changes on its dedicated GitButler branch, creating one if needed.", @@ -266,7 +321,7 @@ pub fn render_managed_policy_block(answers: &WizardAnswers) -> String { } } if let Some(pattern) = &answers.branch_pattern { - write_section_header(&mut body, "Branch naming"); + write_section_header(&mut body, WorkflowOption::BranchPattern.section_title()); writeln!( &mut body, "- When creating a GitButler branch for an agent session, use `{pattern}`." @@ -274,7 +329,7 @@ pub fn render_managed_policy_block(answers: &WizardAnswers) -> String { .expect("write to string"); } if let Some(convention) = &answers.commit_convention { - write_section_header(&mut body, "Commit message convention"); + write_section_header(&mut body, WorkflowOption::CommitConvention.section_title()); writeln!( &mut body, "- Follow the `{convention}` commit-message convention when writing commit messages." @@ -284,7 +339,7 @@ pub fn render_managed_policy_block(answers: &WizardAnswers) -> String { if answers.has(WorkflowOption::CommitAfterTurn) { write_section( &mut body, - "Commit checkpoints after each turn", + WorkflowOption::CommitAfterTurn.section_title(), &[ "Commit after a working checkpoint, when the requested change is complete and relevant checks have passed or been reported.", "Treat checkpoint commits as local savepoints, not final review history.", @@ -320,3 +375,186 @@ fn write_bullets(body: &mut String, bullets: &[&str]) { body.push('\n'); } } + +/// Read the answers back out of a rendered managed block. +/// +/// Works by matching the `###` headings emitted by +/// [`render_managed_policy_block`], so it recognises blocks already sitting on +/// users' disks — the wizard never persisted its answers anywhere else. +/// Unknown headings are ignored, so a block written by a newer version parses +/// as far as this version understands it rather than failing outright. +/// +/// Note this is inherently lossy in one direction: an option whose section +/// renders nothing (a branch pattern with no pattern set) leaves no trace to +/// read back. [`WizardAnswers::normalized`] models exactly that loss, which is +/// what makes `parse(render(a)) == a.normalized()` hold. +pub fn parse_managed_policy_block(block: &str) -> WizardAnswers { + let mut selected = Vec::new(); + for option in WorkflowOption::ALL { + if has_section(block, option.section_title()) { + selected.push(option); + } + } + + // The free-text values live in the single bullet under their heading, each + // wrapped in an inline-code span. `prompt_optional_text` strips backticks + // from user input, so the closing backtick is unambiguous. + let publish_phrase = inline_code_after(block, "- When the user says `") + .unwrap_or_else(|| default_publish_phrase().to_string()); + let branch_pattern = inline_code_after( + block, + "- When creating a GitButler branch for an agent session, use `", + ); + let commit_convention = inline_code_after(block, "- Follow the `"); + + WizardAnswers { + selected, + publish_phrase, + branch_pattern, + commit_convention, + } + .normalized() +} + +/// Whether `block` contains a line-anchored `### {title}` heading. +fn has_section(block: &str, title: &str) -> bool { + block + .lines() + .any(|line| line.strip_prefix("### ").is_some_and(|rest| rest == title)) +} + +/// The contents of the inline-code span that immediately follows `prefix`. +fn inline_code_after(block: &str, prefix: &str) -> Option { + let start = block.find(prefix)? + prefix.len(); + let rest = &block[start..]; + let end = rest.find('`')?; + // A heading with an empty value would round-trip to `None`, so treat blank + // as absent rather than inventing an empty pattern. + let value = rest[..end].trim(); + (!value.is_empty()).then(|| value.to_string()) +} + +/// The phrase used when the shortcut-publish option is on but no phrase was +/// chosen. +pub fn default_publish_phrase() -> &'static str { + "ship it" +} + +#[cfg(test)] +mod tests { + use super::*; + + fn answers(selected: &[WorkflowOption]) -> WizardAnswers { + WizardAnswers { + selected: selected.to_vec(), + publish_phrase: "ship it".to_string(), + branch_pattern: None, + commit_convention: None, + } + } + + /// The property that makes heading-matching safe: every combination of + /// options survives a render/parse round trip. This fails the moment a + /// `###` title is reworded without updating `section_title`. + #[test] + fn every_option_combination_round_trips() { + let all = WorkflowOption::ALL; + for bits in 0u32..(1 << all.len()) { + let selected: Vec<_> = all + .into_iter() + .enumerate() + .filter(|(i, _)| bits & (1 << i) != 0) + .map(|(_, option)| option) + .collect(); + let original = answers(&selected); + let parsed = parse_managed_policy_block(&render_managed_policy_block(&original)); + assert_eq!( + parsed.selected, + original.normalized().selected, + "options {selected:?} should survive a round trip" + ); + } + } + + #[test] + fn free_text_values_round_trip() { + let original = WizardAnswers { + selected: vec![ + WorkflowOption::PublishPhrase, + WorkflowOption::BranchPattern, + WorkflowOption::CommitConvention, + ], + publish_phrase: "make it so".to_string(), + branch_pattern: "/".to_string().into(), + commit_convention: "type(scope): summary".to_string().into(), + }; + + let parsed = parse_managed_policy_block(&render_managed_policy_block(&original)); + assert_eq!(parsed.publish_phrase, "make it so"); + assert_eq!( + parsed.branch_pattern.as_deref(), + Some("/") + ); + assert_eq!( + parsed.commit_convention.as_deref(), + Some("type(scope): summary") + ); + } + + /// A pattern option ticked with no pattern renders nothing, so it must not + /// come back selected — otherwise the settings UI would show a checkbox + /// that never persists. + #[test] + fn a_pattern_option_without_a_value_is_dropped() { + let original = answers(&[WorkflowOption::BranchPattern]); + assert!(!original.normalized().has(WorkflowOption::BranchPattern)); + + let parsed = parse_managed_policy_block(&render_managed_policy_block(&original)); + assert!(!parsed.has(WorkflowOption::BranchPattern)); + } + + /// Conversely, a value with no ticked option still renders, so parsing + /// must report the option as on. + #[test] + fn a_value_without_its_option_still_counts_as_selected() { + let original = WizardAnswers { + branch_pattern: Some("feature/".into()), + ..answers(&[]) + }; + + let parsed = parse_managed_policy_block(&render_managed_policy_block(&original)); + assert!(parsed.has(WorkflowOption::BranchPattern)); + assert_eq!(parsed.branch_pattern.as_deref(), Some("feature/")); + } + + #[test] + fn normalizing_is_idempotent() { + let original = WizardAnswers { + selected: vec![WorkflowOption::PublishPhrase, WorkflowOption::FoldFixes], + publish_phrase: "ship it".to_string(), + branch_pattern: Some("x".into()), + commit_convention: None, + }; + let once = original.normalized(); + assert_eq!(once.selected, once.normalized().selected); + } + + /// A block a user hand-edited, or one written by a newer version, must not + /// break parsing of the parts this version does understand. + #[test] + fn unknown_headings_and_prose_are_ignored() { + let mut block = render_managed_policy_block(&answers(&[WorkflowOption::FoldFixes])); + block.push_str("\n### Something a future version added\n\n- A bullet.\n"); + + let parsed = parse_managed_policy_block(&block); + assert_eq!(parsed.selected, vec![WorkflowOption::FoldFixes]); + } + + /// The parser must not treat the option list itself as a rendered section. + #[test] + fn a_block_with_no_optional_sections_parses_as_defaults_off() { + let parsed = parse_managed_policy_block(&render_managed_policy_block(&answers(&[]))); + assert!(parsed.selected.is_empty(), "got {:?}", parsed.selected); + assert_eq!(parsed.publish_phrase, default_publish_phrase()); + } +} diff --git a/crates/but-skill/src/target.rs b/crates/but-skill/src/target.rs index 93f3782b122..5b773f6af2b 100644 --- a/crates/but-skill/src/target.rs +++ b/crates/but-skill/src/target.rs @@ -4,7 +4,8 @@ use std::path::Path; use crate::detect::Agent; -use crate::plan::{RepoInfo, Scope, marker_exists}; +use crate::framework::{Framework, framework_by_name}; +use crate::plan::{RepoInfo, Scope}; #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] pub enum AgentTarget { @@ -108,47 +109,15 @@ impl AgentTarget { // In use if the agent has config under $HOME, an unambiguous repo marker, // or a GitButler skill already installed for it — the last makes a re-run // of the wizard re-select agents it (or `but skill`) previously set up. - if let Some(home) = home - && (marker_exists(home, self.home_config_marker()) - || marker_exists(home, self.skill_path_components(Scope::Global))) - { - return true; - } - if let Some(repo) = repo - && (marker_exists(&repo.root, self.repo_config_marker()) - || marker_exists(&repo.root, self.skill_path_components(Scope::Repository))) - { - return true; - } - false - } - - /// The agent's config directory under `$HOME`; its presence means the agent - /// is set up for this user. - pub fn home_config_marker(self) -> Option<&'static [&'static str]> { - Some(match self { - Self::Codex => &[".codex"], - Self::ClaudeCode => &[".claude"], - Self::Cursor => &[".cursor"], - Self::GitHubCopilot => &[".copilot"], - Self::OpenCode => &[".config", "opencode"], - Self::Poolside => &[".config", "poolside"], - Self::Windsurf => &[".codeium"], - // The shared `.agents` format has no agent-specific config to detect. - Self::AgentSkills => return None, - }) + self.framework().in_use(home, repo) } - /// An unambiguous per-repository marker for this agent. `AGENTS.md` is shared - /// by several agents, so it is intentionally not treated as a marker. - pub fn repo_config_marker(self) -> Option<&'static [&'static str]> { - Some(match self { - Self::ClaudeCode => &["CLAUDE.md"], - Self::GitHubCopilot => &[".github", "copilot-instructions.md"], - Self::Cursor => &[".cursor"], - Self::Poolside => &[".poolside"], - Self::Codex | Self::OpenCode | Self::Windsurf | Self::AgentSkills => return None, - }) + /// This target's entry in the shared framework table, which owns the + /// detection markers and instruction paths so the CLI wizard and the + /// desktop app can never disagree about them. + pub fn framework(self) -> &'static Framework { + framework_by_name(self.skill_format_name()) + .expect("every AgentTarget has a framework entry") } /// Where this agent's skill installs, relative to a base directory. Derived @@ -176,30 +145,10 @@ impl AgentTarget { } pub fn shared_instruction_components(self) -> &'static [&'static str] { - match self { - // Cursor reads AGENTS.md without rule metadata, so prefer it over a - // `.cursor/rules/*.mdc` file, which would need YAML frontmatter - // (e.g. `alwaysApply: true`) to be loaded automatically. - Self::Codex - | Self::OpenCode - | Self::Poolside - | Self::AgentSkills - | Self::Cursor - | Self::Windsurf => &["AGENTS.md"], - Self::ClaudeCode => &["CLAUDE.md"], - Self::GitHubCopilot => &[".github", "copilot-instructions.md"], - } + self.framework().repo_instructions } pub fn global_instruction_components(self) -> Option<&'static [&'static str]> { - match self { - Self::Codex => Some(&[".codex", "AGENTS.md"]), - Self::ClaudeCode => Some(&[".claude", "rules", "gitbutler.md"]), - Self::GitHubCopilot => Some(&[".copilot", "copilot-instructions.md"]), - Self::OpenCode => Some(&[".config", "opencode", "AGENTS.md"]), - Self::Poolside => Some(&[".config", "poolside", "AGENTS.md"]), - Self::Windsurf => Some(&[".codeium", "windsurf", "memories", "global_rules.md"]), - Self::Cursor | Self::AgentSkills => None, - } + self.framework().global_instructions } }