Repository navigation
Conversation
jeremy
changed the title
kamal-*: support git-style external commands in .kamal/bin and on PATH
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7a409ecdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR adds support for external commands that can extend Kamal's functionality, following a plugin pattern similar to Git's git-foo extension model. External commands are resolved when no builtin command matches, looking first in .kamal/bin/<name> (project-local), then for kamal-<name> on PATH. These external commands bypass config loading, allowing them to work even with broken or missing configuration files, while builtin commands (including prefix matches) are never shadowed.
Changes:
- Added dispatch override in
Kamal::Cli::Mainto resolve and execute external commands - Implemented
resolve_external_commandmethod with security guards against path traversal, directories, non-executable files, and flags - Added comprehensive test coverage for external command resolution, priority, and security scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/kamal/cli/main.rb | Implements dispatch override and external command resolution with security checks |
| test/cli/main_test.rb | Adds 11 comprehensive tests covering external command behavior and security |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Unknown commands are resolved as external executables before falling through to aliases or the error handler. Lookup order: 1. .kamal/bin/<name> (project-local) 2. kamal-<name> on PATH (system-wide) Builtin commands and unambiguous prefix matches are never shadowed. The external command is exec'd with remaining argv, replacing the kamal process.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aea640cd2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ENV["PATH"]&.split(File::PATH_SEPARATOR)&.each do |dir| | ||
| candidate = File.join(dir, "kamal-#{name}") |
There was a problem hiding this comment.
When PATH contains an empty component (for example :/usr/bin, a::b, or a trailing :), this lookup does not follow PATH semantics: File.join("", "kamal-foo") produces /kamal-foo, and String#split discards trailing empty components. As a result, Kamal may inspect or execute a root-level file and fails to find the command in the current directory, even though an empty PATH prefix denotes the current directory and entries must be searched in order according to the POSIX PATH definition. Preserve trailing components and translate an empty directory to . before joining.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not doing this. The root-level half no longer applies: the lookup now drops empty entries (split(File::PATH_SEPARATOR).reject(&:empty?)), so nothing like /kamal-foo is ever inspected or run. Reading an empty entry as the current directory is left out on purpose. .kamal/bin is where a project keeps its own commands, and implicitly running whatever kamal-* sits in the working directory is the behaviour POSIX empty-entry semantics would add. Leaving this open for a maintainer to weigh.
exec with a lone string splits it on spaces, or hands it to a shell when it has metacharacters, so `kamal bar` with kamal-bar in a PATH directory like "~/my tools" ran something else than the file we'd just checked. Pass the path as [command, argv0] so it is always run as is.
An external command found before Thor got to the dynamic command quietly replaced any alias of the same name: install kamal-console anywhere on the PATH and `kamal console` stopped opening the console the config defines. Look an unknown command up the way Kamal always has, as an alias first, and only run an external command when there isn't one: built-in commands, then aliases, then .kamal/bin/<name>, then kamal-<name> on the PATH. Built-in commands still never read the config to get there. An unknown command reads it to look for an alias, unless there's no config file at all, since then there's no alias to find and the external command doesn't need one. The external command gets its arguments as given, including when an alias expands to it, rather than ARGV. And --hosts, --roles and --primary are no longer applied while looking up an alias: the alias starts over with them, and an external command takes them as its own, so `kamal foo -h` no longer fails to match a host named "hosts".
…nment Loading the config sets KAMAL_DESTINATION before rendering its ERB, but looking an alias up in the raw config didn't, so an alias written with ENV["KAMAL_DESTINATION"] only worked when --hosts, --roles or --primary had loaded the whole config first. Now that those no longer load it for an unknown command, set the destination for the raw read too, so `kamal info -d world` sees the same config with or without selectors.
jeremy
changed the title
kamal-*: support git-style external commands in .kamal/bin and on PATHkamal-*: run external commands from .kamal/bin and the PATH, after aliases
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c81c4bd67c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elsif external | ||
| exec [ external, external ], *given_args(instance) |
There was a problem hiding this comment.
When an external command uses a flag that collides with a typed Kamal option, such as kamal foo --lock-wait-timeout unlimited, execution never reaches this exec: the dynamic-command path in Base#initialize first feeds every trailing argument through Thor, which rejects unlimited as non-numeric (and similarly rejects a missing numeric value). Consequently external commands do not actually receive arbitrary arguments unchanged; resolve the external command before validating Kamal options, or otherwise avoid parsing its argument tail.
Useful? React with 👍 / 👎.
kamal fooruns.kamal/bin/fooif there is one, or elsekamal-foofrom the PATH, the waygit foorunsgit-foo. The external command gets the rest of the arguments exactly as given and replaces thekamalprocess.Precedence
A name resolves in this order:
depisdeploy). Neither an alias nor an external command can override one, and resolving one never reads the configuration.-cand-d..kamal/bin/<name>, thenkamal-<name>on the PATH.The first version of this PR checked for external commands before anything else ran, which made
kamal-consoleinstalled anywhere on the PATH quietly replaceconsole: app exec -i "bin/rails console"in a project's config. An alias is something the project wrote down, so it wins.The catch is that finding an alias means reading the configuration, so an unknown command now does read it, where the first version promised external commands would run without one. The exception: with no config file at all there can be no alias, so the external command runs without one. That keeps a
kamal-fooon the PATH usable outside a Kamal project.Changes since the first version
exec [path, path], *args, so the path we just checked is the one that runs. Callingexecwith a single string splits it on spaces or hands it to a shell, sokamal barwithkamal-barin~/my toolsran something else.ARGV, so an alias that expands to an external command (list: foo list) passes its own arguments too.--hosts,--rolesand--primaryaren't applied while looking up an alias. The alias starts over with them once it expands, and an external command takes them as its own. Before this,kamal foo -hfailed with "No --hosts match for hosts" beforefooever ran.KAMAL_DESTINATIONset, the same as loading it does. Before, an alias defined withENV["KAMAL_DESTINATION"]only worked if a selector happened to load the whole config first. It now works the same with or without one..kamal/binis where a project's own commands go, and running whateverkamal-*happens to be in the working directory isn't something to do implicitly.With the pre-configure hook
If the pre-configure hook PR (#1964) lands too, an unknown command runs that hook before the alias lookup, under its own name, because the lookup has to read the destination the hook picks. An external command then inherits whatever the hook exported. This conflicts with #1964 in
lib/kamal/commander.rb: both change the same lines ofCommander#resolve_alias(initialize_commandermerges cleanly), so whichever merges second needs a small rebase.Known limitation
Kamal still parses its own options from an unknown command's arguments, because it needs
-cand-dto look for an alias. So a value that is invalid for one of Kamal's typed options fails before the external command runs:kamal foo --lock-wait-timeout foreverreports "Expected numeric value". The arguments that do reach the external command are always exactly as given.Tests
test/cli/main_test.rbcovers:.kamal/binand PATH lookup, a PATH directory with a space, and.kamal/binover the PATH-hKamal::Configurationexpects no load).kamal/bin/<name>and akamal-<name>of the same name--primaryUnit suite: 876 runs. The only failures are the 7 that also fail on
mainwithout a Docker builder (CliBuildTest ×4, CommandsBuilderTest ×2, ConfigurationBuilderTest ×1). RuboCop is clean.