Teach but-skill what a GUI needs - #15190
Open
schacon wants to merge 4 commits into
Open
Conversation
Only the eight agents the setup wizard offers had detection markers, while twenty-one have skill install paths. The desktop settings tab needs to list and detect all of them, so detection has to cover the full set. Adds a framework table keyed by skill-format name holding the stable id, display text, detection markers, and instruction-file paths. It is a separate table from SKILL_FORMATS because four frameworks have two formats each (a repo-local and a global one) while markers and instruction files are per-framework; putting markers on SkillFormat would duplicate them across those pairs. AgentTarget now delegates to this table, so the wizard and the app can never disagree about where an agent's config or instructions live. Markers are written out explicitly rather than derived from install paths. Deriving looks tempting but is wrong: Copilot's local skill path is .github/skills/gitbutler, so a derived repo marker would be .github - present in essentially every repository on GitHub. A regression test pins that, and another pins that a shared AGENTS.md never identifies a specific agent. The thirteen newly covered agents have no global instruction file we are confident writing to, so their policy is surfaced for manual copying rather than guessed at.
The app could install the `but` symlink but had no way to report whether it was installed, and no way to remove it. The settings tab needs both. Adds `cli_install_state` (pure inspection) and `uninstall_cli`. The query deliberately does not repair a stale link the way startup does, so a status call cannot silently rewrite the filesystem and make 'not installed' unreproducible. Uninstall only removes a symlink it can identify as ours: one pointing at this app's binary, or a dangling `but` link, which is exactly the stale state auto-repair handles. A regular file is refused - a package manager's real `but` binary looks exactly like that and deleting it would be unrecoverable. A link pointing at some other binary is refused and reported. The link path is now resolved through `link_path()`, which honours E2E_TEST_APP_DATA_DIR, and the removal decision is split into a path-explicit core so it can be tested against temporary directories. The key test asserts the target binary still exists after the link is removed; getting that backwards would delete the user's CLI. Adds Code::CliUninstallCancelled so a dismissed macOS privileges prompt reads as a neutral abort, matching the existing install behavior.
The setup wizard's answers were never persisted anywhere, so an installed policy block could be rendered but not read back. A settings UI that lets users customize their preferences has to show what is currently installed, which means parsing the block that is already on disk. Adds section_title() as the single source for each option's heading, makes rendering use it, and adds a parser that maps those headings back to options. Matching headings rather than introducing a new machine-readable payload is what lets this work on blocks users already have; a payload would only help installs made from this version onward. Rendering is not injective, so WizardAnswers::normalized models the loss: the branch-pattern and commit-convention sections render from their value being set rather than from the checkbox, and a publish phrase is only written when its option is on. Without normalizing, a settings UI would show a checkbox that silently un-checks itself after saving. A property test covers all 1024 option combinations, which is what makes heading-matching safe: reword a heading without updating section_title and it fails immediately. Unknown headings are ignored so a block from a newer version still parses as far as this version understands it.
Nothing in the codebase could uninstall a skill or take its policy back out of an instruction file, so the settings tab had no way to undo an install. remove_skill_files refuses anything whose SKILL.md does not identify it as ours, reusing the same check discovery uses - so uninstall and discovery can never disagree about what belongs to GitButler. It deletes only the files this crate writes and removes the directory non-recursively, never remove_dir_all. 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 the leftovers are reported so the UI can say so. The agent's own skills/ directory is never touched. remove_managed_block splices the block out of an instruction file and nothing else. It inherits the existing marker protections, so a marker quoted in prose or shown inside a fenced code block is left alone and a malformed pair errors rather than guessing at a span to delete. It never deletes the file, even when the block was all it contained: these are usually git-tracked files the user owns, and removing one would be a surprising side effect of uninstalling a skill. Removal takes back the blank separator line that appending inserted, but only when leaving it would strand a trailing or doubled blank line, so a block that merely follows a blank line does not lose it.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the missing “GUI needs” capabilities to but-skill so the desktop settings UI can inspect, render, and safely remove agent setup artifacts (skills, managed blocks, and the CLI link) without relying on CLI-only behaviors.
Changes:
- Introduces a unified
Frameworktable (markers + instruction paths) and routesAgentTargetdetection/instruction resolution through it. - Makes managed workflow policy blocks round-trippable by single-sourcing section headings and adding a parser + normalization.
- Adds uninstall/removal primitives for installed skills, managed policy blocks, and the
butCLI symlink (plus UI-friendly inspection state).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/but-skill/src/framework.rs | Adds the shared framework table (stable id, display name, markers, instruction paths) and detection helpers. |
| crates/but-skill/src/target.rs | Delegates AgentTarget detection/instruction paths to the framework table to prevent wizard/UI drift. |
| crates/but-skill/src/policy.rs | Single-sources section titles, adds parser + normalization, and property/tests for round-trip stability. |
| crates/but-skill/src/files.rs | Adds managed-block read/remove primitives (conservative marker handling, never deletes files). |
| crates/but-skill/src/install.rs | Adds remove_skill_files() and RemovalOutcome for safe skill uninstall and UI reporting. |
| crates/but-skill/src/cli_link.rs | Adds read-only CLI install-state inspection and safe uninstall logic around the but symlink. |
| crates/but-skill/src/lib.rs | Exposes the new framework module. |
| crates/but-error/src/lib.rs | Adds CliUninstallCancelled error code for macOS privilege prompt dismissal. |
Suppressed comments (1)
crates/but-skill/src/cli_link.rs:170
- Dangling-link detection uses
Path::new(&actual).exists(), butactualcomes fromread_link()and may be relative. Interpreting a relative target against the current working directory can cause a non-dangling link to be treated as dangling (or vice versa). Resolve relative targets against the symlink’s parent directory before checkingexists().
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 {
Comment on lines
+121
to
+131
| 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(), | ||
| } | ||
| } | ||
| } |
Comment on lines
+271
to
+275
| // 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() { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four capabilities the desktop settings screen needs and the CLI never had. Stacked on #15189.
The theme for review
Every operation here touches the user's filesystem. Three of them delete things. The guards are the interesting part of this PR, so they are called out individually below.
1. Framework table covering all supported agents
AgentTarget(8 agents, has detection markers) andSKILL_FORMATS(21 agents, has install paths) were different taxonomies. The settings tab needs to list and detect all of them.Adds a framework table keyed by skill-format name, holding the stable id, display text, detection markers, and instruction paths. Kept separate from
SKILL_FORMATSbecause four frameworks have two formats each (repo-local and global) while markers and instruction files are per-framework — putting markers onSkillFormatwould duplicate them across those pairs.AgentTargetnow delegates to this table, so the wizard and the app cannot disagree.The 13 newly covered agents have no global instruction file I was confident writing to, so their policy is surfaced for manual copying rather than guessed at.
2. CLI install-state and uninstall
cli_install_state()is pure inspection — deliberately not repairing a stale link the way startup does, so a status query cannot silently rewrite the filesystem and make "not installed" unreproducible.uninstall_cli()only removes a symlink it can identify as ours:butlooks exactly like that, and deleting it would be unrecoverablebutlink is ours (that is the stale state auto-repair exists to fix)The link path resolves through
link_path(), which honoursE2E_TEST_APP_DATA_DIR, and the removal decision is split into a path-explicit core so it is testable against temp dirs. The load-bearing test asserts the target binary still exists after the link is removed — getting that backwards would delete the user's CLI, or withbuiltin-but, the running app.3. Round-trippable workflow policy
cleanup.rssaid it outright: "The wizard's answers are not persisted, so the full block cannot be re-rendered for existing installs." There was a renderer and no parser, so an installed policy could be written but never read back — and a settings UI has to show what is currently installed.Adds
section_title()as the single source for each option's heading, makes rendering use it, and parses those headings back. Matching headings rather than embedding a machine-readable payload is what makes this work on blocks users already have; a payload would only help installs made from this version onward.Rendering is not injective, so
WizardAnswers::normalized()models the loss: the branch-pattern and commit-convention sections render from their value being set rather than from the checkbox, and a publish phrase is only written when its option is on. Without normalizing, a settings UI would show a checkbox that silently un-checks itself after saving.4. Removal primitives
remove_skill_filesrefuses anything whoseSKILL.mddoes not identify it as ours, reusing the same check discovery uses — so uninstall and discovery agree by construction. It deletes only the four files this crate writes and removes the directory non-recursively, neverremove_dir_all. If the user kept notes alongside the skill, the directory survives and the leftovers are reported so the UI can say so. The agent's ownskills/directory is never touched.remove_managed_blocksplices the block out of an instruction file and nothing else. It inherits the existing marker protections — a marker quoted in prose or inside a fenced code block is left alone, a malformed pair errors rather than guessing at a span to delete. It never deletes the file, even when the block was all it contained: these are usually git-tracked files the user owns.Verification
cargo test -p but-skill— 175 pass, including the 1024-case property test and the uninstall safety tests.cargo clippy --workspaceclean. CLI integration tests still pass unmodified.