feat: add arch filter for tool version options#9073
Conversation
Mirrors the existing `os` field on `ToolVersionOptions`, allowing tools
to be restricted to specific CPU architectures via:
[tools]
"aqua:sharkdp/fd" = { version = "latest", arch = ["arm64"] }
"cargo:fd-find" = { version = "latest", arch = ["x64"] }
When both `os` and `arch` are specified, both must match the current
platform for the tool to be active. Accepted values mirror
`crate::cli::version::ARCH` (which normalizes `x86_64` to `x64` and
`aarch64` to `arm64`); other `std::env::consts::ARCH` values pass
through verbatim.
Implementation:
- New `arch: Option<Vec<String>>` field on `ToolVersionOptions` (next to
`os`), included in the manual `Hash` impl.
- New `arch()` accessor on `ToolRequest`, parallel to `os()`.
- Filter logic split out into a private `matches_current_platform()` so
unit tests can exercise `os`/`arch` without the install-state
singleton that `BackendArg::is_os_supported` requires.
- Custom `mise_toml.rs` deserializers gain an `"arch" =>` arm in both
`MiseTomlToolList` and `MiseTomlTool` visitors, mirroring `os`.
- `replace_versions` writes `arch` into the inline table next to `os`,
and `output_empty_opts` accounts for `arch` when deciding whether to
emit an inline table at all.
- `to_tool_request_set` copies `arch` from config onto cached backend
options alongside `os`/`depends`/`install_env`.
- `tool_stub.rs` exposes the same `arch` field on `ToolStubFile` so
stubs can use it.
- Snapshots regenerated to include the new field.
- Documentation updated in `docs/dev-tools/index.md` (new
Architecture-Specific Tools section) and `docs/dev-tools/tool-stubs.md`
(Optional Fields).
Closes jdx#8948
Greptile SummaryThis PR adds an Confidence Score: 5/5Safe to merge — the previous scalar-bypass bug is fixed, filter logic is correct, and all code paths are covered by tests. No P0 or P1 issues remain. The implementation is a clean parallel of the existing No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["mise.toml / tool stub"] -->|"arch = [...]"| B["MiseTomlToolList / MiseTomlTool\ndeserializer"]
B -->|"options.arch = Some(vec![...])"| C["ToolVersionOptions"]
C --> D["to_tool_request_set\nba_opts.arch = options.arch.clone()"]
D --> E["ToolRequest\n(arch field in options)"]
E --> F["is_os_supported()"]
F --> G["matches_current_platform()"]
G -->|"arch filter"| H{"arch.contains(ARCH)?"}
H -->|"yes / arch=None"| I["ba().is_os_supported()"]
H -->|"no"| J["skip tool\n(not installed/used)"]
I -->|"true"| K["Tool active on this platform"]
I -->|"false"| J
Reviews (2): Last reviewed commit: "fix: address review feedback for arch fi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 9ae2bf20d5
ℹ️ 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".
| toml::Value::String(s) => { | ||
| // Convert {{version}} to {version} for backend templating | ||
| let s = s.replace("{{version}}", "{version}"); | ||
| options.opts.insert(k, toml::Value::String(s)); |
There was a problem hiding this comment.
arch string values as platform filters
When arch is provided as a string, this branch stores it in options.opts instead of options.arch, so ToolRequest::arch() stays None and the architecture filter is never enforced. In practice, configs like arch = "arm64" are silently treated as a backend option rather than a platform gate, which can install/use tools on unsupported architectures.
Useful? React with 👍 / 👎.
| if opts.os.is_some() || opts.arch.is_some() || !opts.install_env.is_empty() { | ||
| return false; |
There was a problem hiding this comment.
arch when rewriting multi-version tool entries
Adding opts.arch.is_some() here forces arch-restricted requests down the non-empty serialization path, but the multi-version branch only writes version and opts, not arch. As a result, running replace_versions on a tool with multiple versions and an arch filter will drop the filter from mise.toml, changing behavior on subsequent runs.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements architecture-specific tool filtering by adding an arch field to tool configurations and stubs. The changes include updating the ToolVersionOptions structure, enhancing the ToolRequest platform matching logic, and providing relevant documentation. Review feedback highlighted several issues: the deserialization logic incorrectly handles single-string architecture values by placing them in a generic options map, and the use of unwrap() poses a panic risk. Furthermore, the is_empty method in ToolVersionOptions requires an update to include the new fields, ensuring that platform filters are not lost during configuration serialization.
| "arch" => match v { | ||
| toml::Value::Array(s) => { | ||
| options.arch = Some( | ||
| s.iter().map(|v| v.as_str().unwrap().to_string()).collect(), | ||
| ); | ||
| } | ||
| toml::Value::String(s) => { | ||
| // Convert {{version}} to {version} for backend templating | ||
| let s = s.replace("{{version}}", "{version}"); | ||
| options.opts.insert(k, toml::Value::String(s)); | ||
| } | ||
| _ => { | ||
| return Err(de::Error::custom("arch must be a string or array")); | ||
| } | ||
| }, |
There was a problem hiding this comment.
When arch is provided as a single string, it is currently being inserted into the generic opts map instead of the dedicated arch field. This causes the architecture filter to be ignored by ToolRequest::is_os_supported(). Additionally, the array handling uses unwrap(), which will panic if the array contains non-string values. It should follow the safer pattern used by the depends field.
"arch" => match v {
toml::Value::Array(arr) => {
options.arch = Some(
arr.iter()
.map(|v| {
v.as_str()
.ok_or_else(|| de::Error::custom("arch array must contain only strings"))
.map(|s| s.to_string())
})
.collect::<Result<Vec<_>, _>>()?,
);
}
toml::Value::String(s) => {
options.arch = Some(vec![s]);
}
_ => {
return Err(de::Error::custom("arch must be a string or array"));
}
},| "arch" => match v { | ||
| toml::Value::Array(s) => { | ||
| options.arch = Some( | ||
| s.iter().map(|v| v.as_str().unwrap().to_string()).collect(), | ||
| ); | ||
| } | ||
| toml::Value::String(s) => { | ||
| // Convert {{version}} to {version} for backend templating | ||
| let s = s.replace("{{version}}", "{version}"); | ||
| options.opts.insert(k, toml::Value::String(s)); | ||
| } | ||
| _ => { | ||
| return Err(de::Error::custom("arch must be a string or array")); | ||
| } | ||
| }, |
There was a problem hiding this comment.
When arch is provided as a single string, it is currently being inserted into the generic opts map instead of the dedicated arch field. This causes the architecture filter to be ignored by ToolRequest::is_os_supported(). Additionally, the array handling uses unwrap(), which will panic if the array contains non-string values. It should follow the safer pattern used by the depends field.
"arch" => match v {
toml::Value::Array(arr) => {
options.arch = Some(
arr.iter()
.map(|v| {
v.as_str()
.ok_or_else(|| de::Error::custom("arch array must contain only strings"))
.map(|s| s.to_string())
})
.collect::<Result<Vec<_>, _>>()?,
);
}
toml::Value::String(s) => {
options.arch = Some(vec![s]);
}
_ => {
return Err(de::Error::custom("arch must be a string or array"));
}
},| #[derive(Debug, Default, Clone, PartialEq, serde::Deserialize, serde::Serialize)] | ||
| pub struct ToolVersionOptions { | ||
| pub os: Option<Vec<String>>, | ||
| pub arch: Option<Vec<String>>, |
There was a problem hiding this comment.
The is_empty method in ToolVersionOptions (around line 67) must be updated to include the new arch field (and the existing os field). Currently, if a tool entry only contains os or arch filters, is_empty() will return true. This causes MiseTomlTool::from to set the options to None, leading to the filters being dropped when the configuration is processed or re-saved.
There was a problem hiding this comment.
Pull request overview
Adds an arch platform filter to tool entries (alongside existing os) so tools can be conditionally enabled/disabled per CPU architecture, including support in config parsing/serialization, tool stubs, docs, and tests.
Changes:
- Introduces
arch: Option<Vec<String>>inToolVersionOptionsand exposes it viaToolRequest::arch(), with filtering logic inToolRequest. - Updates
mise.tomlread/write handling to parse/emitarch, plus updates snapshots. - Extends tool stub schema/docs to support
arch, and adds unit tests for the new filter behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/toolset/tool_version_options.rs | Adds arch to tool options and includes it in hashing. |
| src/toolset/tool_request.rs | Exposes arch() accessor and applies os+arch matching in platform support checks; adds tests. |
| src/config/config_file/mise_toml.rs | Parses/serializes arch in mise.toml tool definitions and updates option handling. |
| src/cli/tool_stub.rs | Adds arch support to tool stub files and option deserialization. |
| docs/dev-tools/tool-stubs.md | Documents os/arch optional fields for tool stubs. |
| docs/dev-tools/index.md | Adds “Architecture-Specific Tools” section documenting arch. |
| src/config/config_file/snapshots/*.snap | Snapshot updates reflecting the new arch field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Debug, Default, Clone, PartialEq, serde::Deserialize, serde::Serialize)] | ||
| pub struct ToolVersionOptions { | ||
| pub os: Option<Vec<String>>, | ||
| pub arch: Option<Vec<String>>, | ||
| pub depends: Option<Vec<String>>, | ||
| pub install_env: IndexMap<String, String>, | ||
| #[serde(flatten)] |
There was a problem hiding this comment.
ToolVersionOptions::is_empty() currently ignores the platform filter fields. With the new arch field (and existing os), option sets that only contain os/arch will still be treated as “empty”, which can cause filters to be dropped in places that rely on is_empty() (e.g., omitting options when serializing a MiseTomlTool). Update is_empty() (or introduce a separate helper) so it accounts for non-empty os/arch filters.
| "arch" => match v { | ||
| toml::Value::Array(s) => { | ||
| options.arch = Some( | ||
| s.iter().map(|v| v.as_str().unwrap().to_string()).collect(), |
There was a problem hiding this comment.
In the arch array deserialization, v.as_str().unwrap() will panic if the TOML array contains a non-string (e.g., arch = [1]). This should return a structured deserialization error instead of crashing. Consider using as_str().ok_or_else(...) and collecting into a Result<Vec<_>, _> (similar to the depends parsing below).
| s.iter().map(|v| v.as_str().unwrap().to_string()).collect(), | |
| s.iter() | |
| .map(|v| { | |
| v.as_str() | |
| .ok_or_else(|| de::Error::custom("arch array must contain only strings")) | |
| .map(|s| s.to_string()) | |
| }) | |
| .collect::<Result<Vec<_>, _>>()?, |
| toml::Value::Array(s) => { | ||
| options.arch = Some( | ||
| s.iter().map(|v| v.as_str().unwrap().to_string()).collect(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Same issue as above in MiseTomlTool deserialization: v.as_str().unwrap() inside the arch array branch can panic on non-string array members. Prefer validating each element and returning a de::Error rather than unwrapping.
| let output_empty_opts = |opts: &ToolVersionOptions| { | ||
| if opts.os.is_some() || !opts.install_env.is_empty() { | ||
| if opts.os.is_some() || opts.arch.is_some() || !opts.install_env.is_empty() { | ||
| return false; | ||
| } | ||
| if let Some(reg_ba) = REGISTRY.get(ba.short.as_str()).and_then(|b| b.ba()) |
There was a problem hiding this comment.
replace_versions now treats arch as a non-empty option (via output_empty_opts), but in the versions.len() > 1 serialization path only tr.options().opts is written into the inline table—named fields like os/arch/depends/install_env are omitted. This means running commands that call replace_versions can silently drop arch (and existing os, etc.) filters for multi-version tool entries. The multi-version table serialization should include these named option fields as well (similar to the single-version branch).
|
I think it's better to support tools only supported on Linux and Windows, but only for x64. Some tools lack precompiled binaries only for Windows arm64. It might be better to wait for jdx's decision. Current implementation could be fine. |
Four fixes based on automated code review: 1. **String arch/os handling**: `arch = "arm64"` (without brackets) was silently absorbed into the generic opts map instead of populating options.arch. Now stores it as Some(vec![s]), matching depends. Same fix applied to the os field for consistency. 2. **Multi-version serialization**: replace_versions only wrote named fields (os/arch/install_env) for single-version entries. Multi- version inline tables now include them as well, preventing filter loss on re-save. 3. **Safe array parsing**: arch/os array deserialization used unwrap() which panics on non-string elements. Replaced with the ok_or_else/collect::<Result> pattern used by depends. 4. **is_empty() now includes os/arch**: Previously, tool entries with only os or arch filters were treated as "empty", causing MiseTomlTool::from to set options to None and drop the filters.
|
All review points have been addressed in the latest commit (2953bd9):
This comment was generated by Claude Code. |
|
@risu729 Good point about the cross-product case (e.g. "Linux and Windows, but only x64"). The current Aqua's This comment was generated by Claude Code. |
|
Closing — the os/arch compound syntax in #9088 and schema support in #9095 cover this use case. Thanks @RobertDeRose and @risu729! |
Summary
Adds an
archfilter to[tools]entries, complementing the existingosfilter:Use case from #8948: some prebuilt binaries only ship for
arm64on macOS, so dotfiles managed across Intel and ARM Macs need to fall back to a different backend per architecture. Previously this required external templating (e.g., chezmoi); now it's native.When both
osandarchare specified, both must match. Accepted values mirrorcrate::cli::version::ARCH(x64/arm64); otherstd::env::consts::ARCHvalues pass through verbatim.This is the implementation for #8948 ("Seems fine to me" — @jdx).
Implementation
arch: Option<Vec<String>>field onToolVersionOptions, included in the manualHashimplarch()accessor onToolRequest, parallel toos()matches_current_platform()so unit tests can exercise the filter without touching the install-state singleton thatBackendArg::is_os_supportedconsultsmise_toml.rsdeserializers (bothMiseTomlToolListandMiseTomlTool) gain an"arch" =>arm; serializer writesarchnext toos;output_empty_optsandto_tool_request_setupdated to includearchtool_stub.rsexposes the samearchfield onToolStubFileso stubs can use itdocs/dev-tools/index.md,os/archlisted under Optional Fields indocs/dev-tools/tool-stubs.mdTest plan
tool_requestfilter tests + snapshot updates formise_toml. Full suite passes locally (633/633)Closes #8948
This PR was prepared with Claude Code.