Support Git-style external subcommands (but-<command>) by schacon · Pull Request #13885 · gitbutlerapp/gitbutler

schacon · 2026-05-19T13:24:21Z

Original description by @schacon

Allow unknown subcommands to invoke PATH helpers named but- (e.g. `but forecast` runs `but-forecast`) while preserving legacy behavior for filesystem paths. Add argument/docs updates, an external dispatcher (command/external.rs), wiring into the command dispatch and metrics, tests for Unix PATH delegation, and update help tests and docs. Map spawning NotFound errors to the friendly “not a command” message.

I (@slarse) have completely clobbered this PR with my own stuff now. I pushed to this PR to retain the discussion.

Anyhow, this PR adds in support for Git-style external subcommands by allowing but <COMMAND> [ARGS] to delegate to any program on PATH called but-<COMMAND>, under the following conditions:

<COMMAND> is not a built-in command
<COMMAND> only consists of characters in the set [a-zA-Z_-]. This is overly restrictive but I figured we can loosen it up later, I just didn't feel confident you couldn't pull any shenanigans with this.

An important design goal here is that this external command execution shouldn't make error messages worse. If we fail to match <COMMAND> against anything on PATH (we currently just try to execute), we reparse the command alone with external command support disabled, which will still print the nice Clap error message about the subcommand not being recognized, including any potential near matches. See tests for details.

Note: Given that I could solve the "deterioration" of error messages when failing to match against an external command, I find no good reason to put this behind an app setting, feature toggle or environment variable. It'll just become available to everyone, and most will never notice it's there.

Windows? Nope, not in this PR. I will try to add that separately but I need to setup a Windows dev environment first.

Copilot

Pull request overview

This PR adds Git-style external subcommand delegation to the but CLI, allowing unknown commands to invoke but-<command> helpers from PATH while retaining existing path-opening behavior.

Changes:

Adds hidden clap external-subcommand parsing and dispatch logic for but-<name> helpers.
Updates CLI error/help tests and adds a Unix delegation test.
Updates metrics matching and CLI reference documentation for external commands.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`crates/but/src/args/mod.rs`	Adds hidden external subcommand parsing and updates top-level CLI docs.
`crates/but/src/command/external.rs`	Implements external helper/path dispatch.
`crates/but/src/command/mod.rs`	Wires in the external command module.
`crates/but/src/lib.rs`	Delegates parsed external subcommands before normal command handling.
`crates/but/src/utils/metrics.rs`	Handles the external subcommand variant in metrics command mapping.
`crates/but/tests/but/command/external.rs`	Adds Unix test coverage for PATH helper delegation.
`crates/but/tests/but/command/help.rs`	Updates friendly error test for path-like unknown input.
`crates/but/tests/but/command/mod.rs`	Registers the external command test module.
`crates/but/skill/references/reference.md`	Documents external command helper behavior.

Byron

I took a quick look and tried really hard to abuse this, also in comparison to how Git implements it. Codex helped with that, and I filtered it down to just one possible issue.

It would be great to also summon @slarse to the devsec party.

research by codex

Summary for GitHub:

git foo dispatches through Git’s top-level wrapper. Git parses global options, prepends its trusted exec path (GIT_EXEC_PATH / compiled gitexecdir) to PATH, tries builtins first, then invokes an external git-foo via argv-based process execution. It does not run a shell for normal external commands. Shell execution is limited to explicit ! aliases. Git also prevents aliases from overriding normal commands, except deprecated commands, and repository safety checks such as safe.directory limit reading config/hooks from untrusted ownership contexts. External git-foo helpers are still trusted code from the effective PATH.

The Rust but-{name} dispatcher has the important no-shell property too: Command::new(...).args(...) passes arguments directly. The larger difference is that it relies on the ambient PATH without first prepending or prioritizing a trusted helper directory. That makes execution sensitive to PATH order, relative PATH entries, current-directory entries, or writable directories ahead of the intended install location. It also constructs the helper name with to_string_lossy(), which can alter non-UTF-8 input, and uses a permissive command-name check.

Suggested hardening:

Try builtins before external helpers.
Resolve helpers from a trusted install/libexec directory before ambient PATH.
Resolve the executable to an absolute path before setting current_dir.
Build but-<name> as an OsString, avoiding lossy Unicode conversion.
Restrict helper names to a simple ASCII allowlist such as [A-Za-z0-9-]+.
Treat but-* helpers as trusted plugin code and document that clearly.
Consider sanitizing environment/file descriptor inheritance if helpers may run in sensitive contexts.

Byron · 2026-05-19T14:38:49Z

+        Err(e) => {
+            return Err(e).with_context(|| format!("could not invoke `{prefixed}` from PATH"));
+        }
+    };


I wondered if there was anything else to implementing this, and if this somehow could be abused.
But overall, I think there is "absolutely nothing to worry about"™️.

I don't really see anything either. This seems fine.

Byron · 2026-05-19T14:46:05Z

+    if extra.len() == 1 {
+        let candidate = Path::new(&extra[0]);
+        let resolved = resolve_against_current_dir(current_dir, candidate);
+        if resolved.exists() {
+            return crate::command::gui::open(&resolved)
+                .with_context(|| format!("failed to open path `{}`", resolved.display()));
+        }
+    }


This could be problematic as attacker-controlled repos with submodules maybe can now bait users to open their repos.
It's far-fetched though as this gui::open just launches the GUI on a given directory that also must be a repo to get anywhere, and the question is if this is abusable. Maybe it is if the GUI is vulnerable to certain injection attacks.

Git doesn't have that problem.

It feels like to mitigate this, one would have to search the PATH by hand, and if a program exists one would always have to execute it.

There's an easy solution to this: Kill it. We decided on a removal of this feature in the last engineering meeting as it's a low-frequency command that really doesn't deserve top-level placement. Doing but gui <path> instead is perfectly serviceable.

slarse

I don't really have any major reservations here. While of course it opens up for some amount of misuse, the utility of the feature outweighs any such concerns IMO. Also, if a but-malicious is on the path, whether it's accidentally executed through but or by just running but-malicious outright doesn't seem like a great distinction.

If we want to be extra prudent about it, we could have a command allow-list in our app settings, or potentially just an "allow external commands" setting (e.g. but config external enable). That would limit any potential security concerns to the probably very small subset of people that would ever use this feature. I think that kind of toggle might be worthwhile.

Other than that I think we should get this in ASAP. @schacon @Byron wdyt?

slarse · 2026-05-20T06:05:32Z

+    if extra.len() == 1 {
+        let candidate = Path::new(&extra[0]);
+        let resolved = resolve_against_current_dir(current_dir, candidate);
+        if resolved.exists() {
+            return crate::command::gui::open(&resolved)
+                .with_context(|| format!("failed to open path `{}`", resolved.display()));
+        }
+    }


There's an easy solution to this: Kill it. We decided on a removal of this feature in the last engineering meeting as it's a low-frequency command that really doesn't deserve top-level placement. Doing but gui <path> instead is perfectly serviceable.

slarse · 2026-05-20T06:11:56Z

+        Err(e) => {
+            return Err(e).with_context(|| format!("could not invoke `{prefixed}` from PATH"));
+        }
+    };


I don't really see anything either. This seems fine.

slarse · 2026-05-20T06:18:50Z

We need a lot more testing for the negative cases

slarse · 2026-05-20T06:20:37Z

+#[cfg(all(unix, feature = "legacy"))]
+mod external;


What's legacy about this? And should we also support Windows?

Indeed, no need.

slarse · 2026-05-27T09:08:34Z

I'm taking over this PR :)

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

+                writeln!(
+                    f,
+                    "{}",
+                    bad_input("Unrecognized subcommand").arg_name(command_name.to_string_lossy())


Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

+        Err(e) if e.kind() == ErrorKind::NotFound => {
+            return Err(CliError::ExternalCommandNotFound(
+                subcommand_name.to_owned(),
+            ));


    let result = match args.cmd.take() {
+        #[cfg(unix)]
+        Some(Subcommands::External(extra)) => {
+            command::external::dispatch(&args.current_dir, &extra)


davidpdrsn

LGTM!

slarse · 2026-05-27T14:50:10Z

+            return Err(CliError::ExternalCommandNotFound(
+                subcommand_name.to_owned(),
+            ));


A new CliError variant. This is necessary to signal to the calling code that there was no external command matching the name, so we can act on that. Current action is to re-parse without external command support to generate a nice error message.

slarse · 2026-05-27T15:11:34Z

        }

-        write!(f, "{}", self.message)?;
+        writeln!(f, "{}", self.message)?;


Needed to change this to align with how clap formats its errors, they contain a trailing newline.

slarse · 2026-05-27T15:13:51Z

+        Err(CliError::ExternalCommandNotFound(command_name)) => {
+            // We reparse without external subcommands allowed, which _should_ result in a proper
+            // clap error, including suggestions for "near matches". This gives richer error
+            // information than the plain ExternalCommandNotFound error.
+            let cmd = Args::command();
+            let argv = [OsString::from(cmd.get_name()), command_name.clone()];
+
+            let reparse_result = cmd
+                .external_subcommand_value_parser(None)
+                .allow_external_subcommands(false)
+                .try_get_matches_from(argv);
+
+            match reparse_result {
+                Err(parse_err) => print_and_exit_non_zero(parse_err),
+                _ => {
+                    // If for some reason we succeeded to parse now, we'll just the original error
+                    print_and_exit_non_zero(CliError::ExternalCommandNotFound(command_name))
+                }
+            }


This is where the new error variant is utilized. I think the enum approach over "codes" is paying off here.

And yeah, the reparsing is a bit of a hack but I honestly don't even hate it :D

Note: There may be an issue with colored output here, investigating ...

If there is a program `but-<COMMAND>` on PATH where `<COMMAND>` is not a built-in command, `but <COMMAND> [ARGS]` delegates to `but-<COMMAND> [ARGS]`. This is implemented in such a way that we still retain all the nice features of Clap's command parsing, such as fuzzy matching against other commands if you make a typo. Co-authored-by: Scott Chacon <schacon@gmail.com>

Copilot AI review requested due to automatic review settings May 19, 2026 13:24

github-actions Bot added rust Pull requests that update Rust code CLI The command-line program `but` labels May 19, 2026

schacon force-pushed the sc-external-commands branch from 60b0cf3 to e63a3ed Compare May 19, 2026 13:25

Copilot AI reviewed May 19, 2026

View reviewed changes

Comment thread crates/but/src/command/external.rs Outdated

Byron reviewed May 19, 2026

View reviewed changes

slarse reviewed May 20, 2026

View reviewed changes

slarse force-pushed the sc-external-commands branch from e63a3ed to 91ea283 Compare May 27, 2026 14:16

slarse requested review from Copilot and davidpdrsn May 27, 2026 14:22

slarse force-pushed the sc-external-commands branch from 91ea283 to 84baecd Compare May 27, 2026 14:22

Copilot started reviewing on behalf of slarse May 27, 2026 14:23 View session

Copilot AI reviewed May 27, 2026

View reviewed changes

slarse marked this pull request as draft May 27, 2026 14:40

slarse force-pushed the sc-external-commands branch 3 times, most recently from 75e6770 to b583955 Compare May 27, 2026 15:07

slarse marked this pull request as ready for review May 27, 2026 15:08

Copilot AI review requested due to automatic review settings May 27, 2026 15:08

Copilot started reviewing on behalf of slarse May 27, 2026 15:08 View session

Copilot AI reviewed May 27, 2026

View reviewed changes

slarse force-pushed the sc-external-commands branch from b583955 to 4bcf604 Compare May 27, 2026 15:18

slarse changed the title ~~Support Git-style external subcommands (but-<name>)~~ Support Git-style external subcommands (but-<command>) May 28, 2026

Copilot AI review requested due to automatic review settings May 28, 2026 06:24

slarse force-pushed the sc-external-commands branch from 4bcf604 to b0777f3 Compare May 28, 2026 06:24

Copilot started reviewing on behalf of slarse May 28, 2026 06:24 View session

Copilot AI reviewed May 28, 2026

View reviewed changes

davidpdrsn approved these changes May 28, 2026

View reviewed changes

slarse reviewed May 28, 2026

View reviewed changes

slarse force-pushed the sc-external-commands branch from b0777f3 to 7f8b51e Compare May 28, 2026 06:50

slarse enabled auto-merge May 28, 2026 06:57

slarse merged commit 1f4d758 into master May 28, 2026
39 checks passed

slarse deleted the sc-external-commands branch May 28, 2026 07:05

Conversation

schacon commented May 19, 2026 • edited by slarse Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slarse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slarse commented May 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

davidpdrsn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

schacon commented May 19, 2026 •

edited by slarse

Loading