feat(task): add support for usage values in task confirm dialog#7924
feat(task): add support for usage values in task confirm dialog#7924
Conversation
Summary of ChangesHello @roele, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to task confirmation dialogs, enabling them to be dynamic and context-aware. By integrating Tera templating with task Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds a great new feature: support for templating in task confirmation prompts using usage arguments. The implementation is solid, moving the confirmation logic to after argument parsing and updating the documentation accordingly.
I have one suggestion to improve the implementation in check_confirmation to preserve type information for usage arguments, which will make the feature more robust and consistent with how other parts of mise handle task arguments. Overall, this is a valuable addition.
| // Add usage values from parsed environment | ||
| let mut usage_ctx = std::collections::HashMap::new(); | ||
| for (key, value) in env { | ||
| if let Some(usage_key) = key.strip_prefix("usage_") { | ||
| usage_ctx.insert(usage_key.to_string(), tera::Value::String(value.clone())); | ||
| } | ||
| } | ||
| tera_ctx.insert("usage", &usage_ctx); |
There was a problem hiding this comment.
This implementation correctly makes usage arguments available to the confirm prompt's Tera template. However, by extracting them from environment variables (which are all strings), all values are stringified. This causes a loss of type information for boolean flags and numeric counts.
For example, a count flag like --verbose --verbose would be available as usage.verbose with a string value of "2", not a number. This prevents numeric comparisons like {% if usage.verbose > 1 %} in the template. Similarly, boolean flags become strings like "true".
To improve this and make it consistent with how run scripts handle usage variables, I recommend refactoring to use the usage::parse::ParseOutput object directly. This would involve:
- Modifying
parse_usage_spec_and_init_envto return theusage::parse::ParseOutput. - Passing this output to
check_confirmation. - In
check_confirmation, using a function liketask_script_parser::make_usage_ctx(which would need to be made public) to build theusagecontext with proper types.
This would ensure that boolean, numeric, and string arguments are correctly typed within the confirm template, providing a more robust and predictable experience for users.
There was a problem hiding this comment.
Pull request overview
Adds support for using parsed usage argument values inside a task’s confirm prompt by deferring confirmation rendering until after usage parsing.
Changes:
- Moves task confirmation prompting to occur after
usageargs/flags are parsed into the task environment. - Stops pre-rendering
Task.confirmduring task template rendering so it can be rendered later withusagevalues available. - Documents
confirmtemplating withusage.*references.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/task/task_script_parser.rs | Exposes helper for building default usage template context from a spec. |
| src/task/task_executor.rs | Defers confirmation to post-usage parsing and renders confirm as a Tera template with injected usage context. |
| src/task/mod.rs | Removes early rendering of confirm during task rendering so it can be rendered later at execution time. |
| docs/tasks/task-configuration.md | Adds documentation/example for confirm templates referencing usage args. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| && !Settings::get().yes | ||
| { | ||
| let config_root = task.config_root.clone().unwrap_or_default(); | ||
| let mut tera = crate::tera::get_tera(Some(&config_root)); |
There was a problem hiding this comment.
tera is never mutated in this function; it doesn't need to be declared as mut.
| let mut tera = crate::tera::get_tera(Some(&config_root)); | |
| let tera = crate::tera::get_tera(Some(&config_root)); |
| // Add usage values from parsed environment | ||
| let mut usage_ctx = std::collections::HashMap::new(); | ||
| for (key, value) in env { | ||
| if let Some(usage_key) = key.strip_prefix("usage_") { | ||
| usage_ctx.insert(usage_key.to_string(), tera::Value::String(value.clone())); | ||
| } | ||
| } |
There was a problem hiding this comment.
usage values are inserted into the Tera context as tera::Value::String for every usage_* env var. This breaks boolean semantics in templates (e.g. {% if usage.force %} will treat the string "false" as truthy), and also prevents arrays/count flags from being represented correctly.
Consider deriving typed tera::Values from the usage parse output (or from the usage spec) so flags become Value::Bool, count/var flags become Value::Array, etc. There is already similar type-aware logic in TaskScriptParser::make_usage_ctx / make_usage_ctx_from_spec_defaults that could be reused if parse_usage_spec_and_init_env returned the parse output or a prebuilt usage context.
| // Add usage values from parsed environment | |
| let mut usage_ctx = std::collections::HashMap::new(); | |
| for (key, value) in env { | |
| if let Some(usage_key) = key.strip_prefix("usage_") { | |
| usage_ctx.insert(usage_key.to_string(), tera::Value::String(value.clone())); | |
| } | |
| } | |
| // Add usage values from parsed environment with basic typing (bools and arrays) | |
| // First, separate scalar and indexed usage_* keys from the env. | |
| let mut scalars: std::collections::HashMap<String, String> = | |
| std::collections::HashMap::new(); | |
| let mut indexed: std::collections::HashMap<String, Vec<(usize, String)>> = | |
| std::collections::HashMap::new(); | |
| for (key, value) in env { | |
| if let Some(usage_key) = key.strip_prefix("usage_") { | |
| // Detect keys of the form "<base>_<index>" where index is a usize. | |
| if let Some((base, idx_str)) = usage_key.rsplit_once('_') { | |
| if let Ok(idx) = idx_str.parse::<usize>() { | |
| indexed | |
| .entry(base.to_string()) | |
| .or_default() | |
| .push((idx, value.clone())); | |
| continue; | |
| } | |
| } | |
| // Non-indexed key: treat as scalar. | |
| scalars.insert(usage_key.to_string(), value.clone()); | |
| } | |
| } | |
| // Local helper to convert string values into typed Tera values. | |
| fn to_tera_typed_value(raw: &str) -> tera::Value { | |
| match raw { | |
| "true" | "True" | "TRUE" => tera::Value::Bool(true), | |
| "false" | "False" | "FALSE" => tera::Value::Bool(false), | |
| _ => tera::Value::String(raw.to_string()), | |
| } | |
| } | |
| let mut usage_ctx: std::collections::HashMap<String, tera::Value> = | |
| std::collections::HashMap::new(); | |
| // Build array values from indexed keys, sorted by index. | |
| for (base, mut items) in indexed { | |
| items.sort_by_key(|(idx, _)| *idx); | |
| let arr: Vec<tera::Value> = items | |
| .into_iter() | |
| .map(|(_, v)| to_tera_typed_value(&v)) | |
| .collect(); | |
| usage_ctx.insert(base, tera::Value::Array(arr)); | |
| } | |
| // Add scalar values that don't have an indexed array counterpart. | |
| for (key, value) in scalars { | |
| if !usage_ctx.contains_key(&key) { | |
| usage_ctx.insert(key, to_tera_typed_value(&value)); | |
| } | |
| } |
| /// - Count flags (`count = true`) use a `Vec<bool>` whose length is | ||
| /// derived from the default (parsed as a usize) or an empty array. | ||
| /// - Simple flags use `false`. | ||
| fn make_usage_ctx_from_spec_defaults(spec: &usage::Spec) -> HashMap<String, tera::Value> { | ||
| pub fn make_usage_ctx_from_spec_defaults(spec: &usage::Spec) -> HashMap<String, tera::Value> { | ||
| let mut usage_ctx: HashMap<String, tera::Value> = HashMap::new(); |
There was a problem hiding this comment.
The doc comment says count flags (count = true) use a Vec<bool> whose length is derived from the default, but the implementation currently always returns an empty array (Vec::new()) for flag.count.
Since this helper is now pub, either update the comment to match current behavior or implement the described behavior by using flag.default (when present) to size/populate the bool array.
### 🚀 Features - **(edit)** add interactive config editor (`mise edit`) by @jdx in [#7930](#7930) - **(lockfile)** graduate lockfiles from experimental by @jdx in [#7929](#7929) - **(task)** add support for usage values in task confirm dialog by @roele in [#7924](#7924) - **(task)** improve source freshness checking with edge case handling by @jdx in [#7932](#7932) ### 🐛 Bug Fixes - **(activate)** preserve ordering of paths appended after mise activate by @jdx in [#7919](#7919) - **(install)** sort failed installations for deterministic error output by @jdx in [#7936](#7936) - **(lockfile)** preserve URL and prefer sha256 when merging platform info by @jdx in [#7923](#7923) - **(lockfile)** add atomic writes and cache invalidation by @jdx in [#7927](#7927) - **(templates)** use sha256 for hash filter instead of blake3 by @jdx in [#7925](#7925) - **(upgrade)** respect tracked configs when pruning old versions by @jdx in [#7926](#7926) ### 🚜 Refactor - **(progress)** migrate from indicatif to clx by @jdx in [#7928](#7928) ### 📚 Documentation - improve clarity on uvx and pipx dependencies by @ygormutti in [#7878](#7878) ### ⚡ Performance - **(install)** use Kahn's algorithm for dependency scheduling by @jdx in [#7933](#7933) - use Aho-Corasick for efficient redaction by @jdx in [#7931](#7931) ### 🧪 Testing - remove flaky test_http_version_list test by @jdx in [#7934](#7934) ### Chore - use github backend instead of ubi in mise.lock by @jdx in [#7922](#7922) ### New Contributors - @ygormutti made their first contribution in [#7878](#7878)
### 🚀 Features - **(edit)** add interactive config editor (`mise edit`) by @jdx in [jdx#7930](jdx#7930) - **(lockfile)** graduate lockfiles from experimental by @jdx in [jdx#7929](jdx#7929) - **(task)** add support for usage values in task confirm dialog by @roele in [jdx#7924](jdx#7924) - **(task)** improve source freshness checking with edge case handling by @jdx in [jdx#7932](jdx#7932) ### 🐛 Bug Fixes - **(activate)** preserve ordering of paths appended after mise activate by @jdx in [jdx#7919](jdx#7919) - **(install)** sort failed installations for deterministic error output by @jdx in [jdx#7936](jdx#7936) - **(lockfile)** preserve URL and prefer sha256 when merging platform info by @jdx in [jdx#7923](jdx#7923) - **(lockfile)** add atomic writes and cache invalidation by @jdx in [jdx#7927](jdx#7927) - **(templates)** use sha256 for hash filter instead of blake3 by @jdx in [jdx#7925](jdx#7925) - **(upgrade)** respect tracked configs when pruning old versions by @jdx in [jdx#7926](jdx#7926) ### 🚜 Refactor - **(progress)** migrate from indicatif to clx by @jdx in [jdx#7928](jdx#7928) ### 📚 Documentation - improve clarity on uvx and pipx dependencies by @ygormutti in [jdx#7878](jdx#7878) ### ⚡ Performance - **(install)** use Kahn's algorithm for dependency scheduling by @jdx in [jdx#7933](jdx#7933) - use Aho-Corasick for efficient redaction by @jdx in [jdx#7931](jdx#7931) ### 🧪 Testing - remove flaky test_http_version_list test by @jdx in [jdx#7934](jdx#7934) ### Chore - use github backend instead of ubi in mise.lock by @jdx in [jdx#7922](jdx#7922) ### New Contributors - @ygormutti made their first contribution in [jdx#7878](jdx#7878)
See #6117