Skip to content

feat(config): support vars in tool versions#6401

Merged
jdx merged 13 commits intomainfrom
feat/tool-version-vars
Sep 25, 2025
Merged

feat(config): support vars in tool versions#6401
jdx merged 13 commits intomainfrom
feat/tool-version-vars

Conversation

@jdx
Copy link
Owner

@jdx jdx commented Sep 24, 2025

Summary

  • Enable using template variables (vars) in tool version specifications
  • Tool versions can now reference variables defined in the same config file
  • This allows for more dynamic and DRY configuration

Use Case

This feature enables users to define tool versions as variables and reuse them across their configuration. This is particularly useful for:

  • Maintaining consistent versions across multiple tools
  • Making it easier to update versions in one place
  • Supporting environment-specific tool versions

Example

[vars]
NODE_VERSION = "20.11.0"
PYTHON_VERSION = "3.12"
RUBY_VERSION = "3.3.0"

[tools]
node = "{{vars.NODE_VERSION}}"
python = "{{vars.PYTHON_VERSION}}" 
ruby = "{{vars.RUBY_VERSION}}"

Technical Approach

The implementation adds vars from the config file to the template context immediately after parsing the TOML file. This makes them available when tool versions are evaluated during to_tool_request_set().

Test Plan

  • Added e2e test verifying vars work in tool versions
  • Tested with both simple and complex variable references
  • Verified env variables still work in tool versions
  • Confirmed existing env variable templating continues to work

🤖 Generated with Claude Code

Enable using template variables (vars) in tool version specifications.
This allows for more dynamic configuration where tool versions can be
defined as variables and reused across the configuration.

Example:
```toml
[vars]
NODE_VERSION = "20.11.0"
PYTHON_VERSION = "3.12"

[tools]
node = "{{vars.NODE_VERSION}}"
python = "{{vars.PYTHON_VERSION}}"
```

This works by adding vars from the config file to the template context
immediately after parsing, making them available when tool versions are
evaluated.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 24, 2025 20:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for using template variables (vars) in tool version specifications, enabling more dynamic and DRY configuration management.

  • Modifies the TOML config parser to add vars to the template context immediately after parsing
  • Allows tool versions to reference variables defined in the same config file using {{vars.VARIABLE_NAME}} syntax
  • Maintains backward compatibility with existing environment variable templating

Reviewed Changes

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

File Description
src/config/config_file/mise_toml.rs Adds vars from the config file to template context for use in tool versions
e2e/config/test_tool_version_vars Comprehensive end-to-end test covering vars in tool versions and env compatibility

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +136 to +137
if let EnvDirective::Val(k, v, _) = var_directive {
vars_map.insert(k.clone(), v.clone());
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern matching only handles EnvDirective::Val variants but silently ignores other variants. Consider adding a comment explaining why other variants are ignored or handle them explicitly to make the intent clearer.

Suggested change
if let EnvDirective::Val(k, v, _) = var_directive {
vars_map.insert(k.clone(), v.clone());
match var_directive {
EnvDirective::Val(k, v, _) => {
vars_map.insert(k.clone(), v.clone());
}
// Other variants are intentionally ignored here because only Val directives are relevant for context vars.
_ => {}

Copilot uses AI. Check for mistakes.
jdx and others added 2 commits September 24, 2025 15:14
The set_context method is not needed since vars are now added
to the context directly in the from_str method.
The previous approach didn't handle nested templates (vars that reference
other vars). This implementation resolves vars locally within the
to_tool_request_set method, doing multiple passes to handle nested
template references.

This approach avoids the complexity of trying to use the global
EnvResults resolution and instead does a simple local resolution
specifically for tool version parsing.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
cursor[bot]

This comment was marked as outdated.

@github-actions
Copy link

github-actions bot commented Sep 24, 2025

Hyperfine Performance

mise x -- echo

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2025.9.17 x -- echo 19.6 ± 0.3 18.6 21.2 1.01 ± 0.05
mise x -- echo 19.5 ± 0.9 18.6 33.2 1.00

mise env

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2025.9.17 env 18.7 ± 0.4 18.0 24.4 1.01 ± 0.03
mise env 18.5 ± 0.2 18.1 20.1 1.00

mise hook-env

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2025.9.17 hook-env 18.2 ± 0.2 17.6 19.6 1.00
mise hook-env 18.4 ± 0.3 17.8 20.6 1.01 ± 0.02

mise ls

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2025.9.17 ls 16.6 ± 0.4 15.8 19.1 1.01 ± 0.03
mise ls 16.5 ± 0.4 15.8 17.5 1.00

xtasks/test/perf

Command mise-2025.9.17 mise Variance
install (cached) 168ms ✅ 104ms +61%
ls (cached) 63ms 63ms +0%
bin-paths (cached) 69ms 69ms +0%
task-ls (cached) 469ms 477ms -1%

✅ Performance improvement: install cached is 61%

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@jdx jdx requested a review from Copilot September 24, 2025 23:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

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


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +89 to +91
pub fn maybe_get() -> Option<Arc<Self>> {
_CONFIG.read().unwrap().as_ref().cloned()
}
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .unwrap() call will panic if the lock is poisoned. Consider using a more descriptive error message or handling the poison error gracefully with .expect("Config lock poisoned") or proper error handling.

Copilot uses AI. Check for mistakes.
Comment on lines +295 to +299
let version_src = version.to_string();
if !version_src.contains("{{") && !version_src.contains("{%") && !version_src.contains("{#")
{
return Ok(version_src);
}
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template detection logic is duplicated from the parse_template method. Consider extracting this into a helper function like contains_template_syntax() to avoid code duplication and ensure consistency.

Copilot uses AI. Check for mistakes.
Comment on lines +517 to +523
let vars = vars_results
.vars
.iter()
.map(|(k, (v, _))| (k.clone(), v.clone()))
.collect::<IndexMap<_, _>>();
context.insert("vars", &vars);
} else if !config.vars.is_empty() {
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vars collection is being cloned twice on each call (line 520). Consider caching this transformed vars map or using references where possible to avoid unnecessary allocations, especially if this method is called frequently during tool resolution.

Suggested change
let vars = vars_results
.vars
.iter()
.map(|(k, (v, _))| (k.clone(), v.clone()))
.collect::<IndexMap<_, _>>();
context.insert("vars", &vars);
} else if !config.vars.is_empty() {
// Cache the transformed vars map to avoid double cloning
let vars: IndexMap<_, _> = vars_results
.vars
.iter()
.map(|(k, (v, _))| (k.clone(), v.clone()))
.collect();
context.insert("vars", &vars);
} else if !config.vars.is_empty() {
// Insert a reference to the vars map directly to avoid unnecessary cloning

Copilot uses AI. Check for mistakes.
@jdx jdx requested a review from Copilot September 24, 2025 23:40
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

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


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +299 to +306
let version_src = version.to_string();
if !Self::contains_template_syntax(&version_src) {
return Ok(version_src);
}

get_tera(self.path.parent())
.render_str(&version_src, context)
.wrap_err_with(|| eyre!("failed to render tool version template: {version_src}"))
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The render_tool_version method duplicates similar logic from the existing parse_template_with_context method. Consider refactoring to use parse_template_with_context directly instead of duplicating the Tera rendering logic.

Suggested change
let version_src = version.to_string();
if !Self::contains_template_syntax(&version_src) {
return Ok(version_src);
}
get_tera(self.path.parent())
.render_str(&version_src, context)
.wrap_err_with(|| eyre!("failed to render tool version template: {version_src}"))
self.parse_template_with_context(context, &version.to_string())

Copilot uses AI. Check for mistakes.
let mut trs = ToolRequestSet::new();
let tools = self.tools.lock().unwrap();
let mut context = self.context.clone();
if context.get("env").is_none() {
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Inserting an empty EnvMap when env is not present may mask issues where environment variables are expected but not available. Consider documenting this behavior or providing a way to distinguish between intentionally empty and missing env context.

Suggested change
if context.get("env").is_none() {
if context.get("env").is_none() {
// [nitpick] Inserting an empty EnvMap when "env" is not present may mask issues where environment variables are expected but not available.
// Consider handling this case explicitly in downstream code. Here, we document this fallback.
// If logging is available, log a warning:
// log::warn!("'env' context missing; inserting empty EnvMap. This may mask missing environment variables.");

Copilot uses AI. Check for mistakes.
@jdx
Copy link
Owner Author

jdx commented Sep 24, 2025

bugbot run

@jdx jdx requested a review from Copilot September 24, 2025 23:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

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


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +89 to +91
pub fn maybe_get() -> Option<Arc<Self>> {
_CONFIG.read().unwrap().as_ref().cloned()
}
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using expect() with a descriptive message instead of unwrap() to provide better error context if the lock is poisoned.

Copilot uses AI. Check for mistakes.
Comment on lines +501 to +506
let vars = vars_results
.vars
.iter()
.map(|(k, (v, _))| (k.clone(), v.clone()))
.collect::<IndexMap<_, _>>();
context.insert("vars", &vars);
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates unnecessary clones of all keys and values. Consider using references in the IndexMap or restructuring to avoid the clone overhead, especially if vars can be large.

Suggested change
let vars = vars_results
.vars
.iter()
.map(|(k, (v, _))| (k.clone(), v.clone()))
.collect::<IndexMap<_, _>>();
context.insert("vars", &vars);
// Avoid unnecessary clones by passing a reference to the existing vars map
context.insert("vars", &vars_results.vars);

Copilot uses AI. Check for mistakes.
let mut trs = ToolRequestSet::new();
let tools = self.tools.lock().unwrap();
let mut context = self.context.clone();
if context.get("vars").is_none() {
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string literal "vars" is used multiple times in this method. Consider defining it as a constant to avoid magic strings and improve maintainability.

Copilot uses AI. Check for mistakes.
cursor[bot]

This comment was marked as outdated.

@jdx jdx merged commit ae067a1 into main Sep 25, 2025
18 checks passed
@jdx jdx deleted the feat/tool-version-vars branch September 25, 2025 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants