Repository navigation
fix: improve usage spec element support in tasks - #8623
Conversation
Summary of ChangesHello, 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 significantly improves the flexibility and robustness of Highlights
Changelog
Activity
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
|
nkakouros
changed the title
![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 introduces significant improvements to usage directive handling in mise tasks. It fixes template rendering for optional arguments and flags by providing sensible defaults, correctly renders count flags as integers, and adds comprehensive support for subcommands, including access via {{ usage.cmd }} and $usage_cmd. Additionally, it resolves a command-line argument conflict with the --prefix flag. The changes are well-implemented and accompanied by extensive unit and end-to-end tests, which is great to see. My review found one minor opportunity for improvement by refactoring a small piece of duplicated logic into a shared function.
Greptile SummaryThis PR substantially improves Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Task invoked with args] --> B{spec has args/flags/subcommands?}
B -- No --> C[trace: no args/flags/subcommands]
B -- Yes --> D[usage::Parser.parse args]
D --> E[po.as_env → insert into env BTreeMap]
E --> F{spec has subcommands?}
F -- Yes --> G["env.entry('usage_cmd').or_default()"]
G --> H{subcommand selected?}
H -- Yes --> I["env.insert('usage_cmd', subcmd_name)"]
H -- No --> J["usage_cmd = empty string"]
F -- No --> K[skip usage_cmd handling]
I --> L
J --> L
K --> L
subgraph TeraRender["Tera Template Rendering"]
L["make_usage_ctx_from_spec_defaults(spec)\nRecurses all subcommands → populate defaults/fallbacks"] --> M["make_usage_ctx(parsed_output)\nActual parsed values only"]
M --> N["usage_map = defaults.extend(real_values)\nDefaults survive for unprovided optional args"]
end
N --> O["tera_ctx.insert('usage', usage_map)"]
O --> P[Render script template]
Last reviewed commit: "Merge branch 'main' ..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
622fb6e to
2b4f3ad
Compare
- Seed `{{ usage.* }}` context with spec defaults before overlaying
parsed values, so optional args/flags don't error when not provided.
- Render count flags as integers (e.g. `-vvv` → `3`, not `[true, true, true]`).
- Fix count flag default from `[]` to `0`.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cb0aa8a to
4cf96d3
Compare
|
Failing test seems unrelated. |
What did |
cdcd72d to
9b53d6d
Compare
I removed the commit that removed |
- Expose selected subcommand as `{{ usage.cmd }}` in Tera templates
and `$usage_cmd` in env vars.
- Always export `$usage_cmd` (empty string) when the spec has
subcommands, so `set -u` scripts don't fail when none is chosen.
- Extract `subcommand_name_from_parse` helper shared between
task_script_parser and task_executor.
- Recurse into subcommand specs when building default template context,
so `{{ usage.X }}` works for args/flags defined inside `cmd` blocks.
(SpecCommand::subcommands is an IndexMap, so iteration order is
deterministic.)
- Include `spec.cmd.subcommands` in the `has_any_args_defined` check
so tasks with only subcommands trigger parsing.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
12614fd to
6709132
Compare
| // expose selected subcommand as {{ usage.cmd }} | ||
| if let Some(subcmd) = subcommand_name_from_parse(&usage.cmds) { | ||
| usage_ctx.insert("cmd".to_string(), tera::Value::String(subcmd)); | ||
| } |
There was a problem hiding this comment.
"cmd" key silently overwrites an arg named cmd
make_usage_ctx unconditionally inserts "cmd" into the Tera context after the arg/flag loop. If a spec has an arg literally named <cmd> and also defines subcommands, the arg's parsed value is silently overwritten by the subcommand name:
for (arg, val) in &usage.args {
usage_ctx.insert(arg.name.to_snake_case(), tera_val); // may insert "cmd"
}
// …
if let Some(subcmd) = subcommand_name_from_parse(&usage.cmds) {
usage_ctx.insert("cmd".to_string(), …); // silently clobbers it
}The same clobbering happens in make_usage_ctx_from_spec_defaults (after collect_cmd_defaults the explicit "cmd" = "" insertion overwrites any <cmd> arg default). The same potential collision also exists for a flag named --cmd.
Since "cmd" is effectively a reserved key in the {{ usage.* }} namespace when subcommands are present, consider either:
- Documenting this reserved-key restriction so authors know not to name args/flags
cmdwhen using subcommands, or - Emitting a warning/error if the spec contains an arg or flag whose
snake_casename is"cmd"and subcommands are also defined.
| use tera::Value; | ||
| use usage::parse::ParseValue::*; | ||
| match val { | ||
| MultiBool(v) => Value::Array(v.iter().map(|b| Value::Bool(*b)).collect()), | ||
| MultiBool(v) => Value::Number(serde_json::Number::from(v.len())), |
There was a problem hiding this comment.
MultiBool → integer is a breaking change for existing templates
Before this PR, a count flag like -vvv rendered as [true, true, true] in Tera, so authors who wrote {{ usage.verbose | length }} or {% for _ in usage.verbose %} were technically relying on the array representation. Changing MultiBool to emit a Number is the correct fix, but any existing templates using array filters (length, join, iteration) on a count flag will now fail with a Tera type error at runtime.
If there are any docs or a migration guide, a note about this behavioral change would help existing users upgrade. Otherwise consider whether a semver bump or changelog entry is warranted.
| // always export $usage_cmd when spec has subcommands so | ||
| // shell scripts with `set -u` don't fail when none is chosen | ||
| if !spec.cmd.subcommands.is_empty() { | ||
| env.entry("usage_cmd".to_string()).or_default(); | ||
| } | ||
| if let Some(subcmd) = subcommand_name_from_parse(&po.cmds) { | ||
| trace!("Adding key usage_cmd value {} in env", subcmd); | ||
| env.insert("usage_cmd".to_string(), subcmd); | ||
| } |
There was a problem hiding this comment.
$usage_cmd guard not extended to subcommand-scoped args/flags
The fix at lines 964–966 ensures $usage_cmd is always exported as an empty string when the spec has subcommands (protecting shell scripts that use set -u). However, the same protection is not applied to the args and flags that belong to those subcommands.
Consider the mixed task in the e2e test (lines 90–115 of test_task_usage_cmd):
- Running
mise run mixed stagingselects the root arg<env>but no subcommand, so$usage_version(a subcommand arg) is never exported bypo.as_env(). - Running
mise run mixed deploy v1.0selects thedeploysubcommand, so$usage_env(the root-level arg) is never exported.
Any task script that uses set -u (or set -euo pipefail) will crash with "unbound variable" in these cases. The e2e assertions like "cmd= env=staging version=" pass today only because bash silently expands unset variables to empty without set -u.
A defensive fix (similar to the usage_cmd guard) would iterate over the spec's args/flags across all subcommands and ensure they are pre-inserted with empty defaults before po.as_env() runs — or at a minimum document that set -u is not supported when mixing root-level args with subcommands.
| /// Extract the selected subcommand name from parsed commands. | ||
| /// `cmds[0]` is the root command; subsequent entries are subcommands. | ||
| pub fn subcommand_name_from_parse(cmds: &[usage::SpecCommand]) -> Option<String> { | ||
| if cmds.len() > 1 { | ||
| let names: Vec<String> = cmds.iter().skip(1).map(|c| c.name.clone()).collect(); | ||
| Some(names.join(" ")) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
subcommand_name_from_parse collects all depths unnecessarily
The function collects and joins all entries after cmds[0] with a space. Given that mise tasks only support flat (one-level) subcommands today, cmds will have at most two entries and the names.join(" ") degenerates to a single-element join. The slice-construction and collect are unnecessary for the current use-case, and the implementation is slightly misleading about what values could appear.
A simpler and more explicit form that pins the contract to exactly one subcommand level would be:
| /// Extract the selected subcommand name from parsed commands. | |
| /// `cmds[0]` is the root command; subsequent entries are subcommands. | |
| pub fn subcommand_name_from_parse(cmds: &[usage::SpecCommand]) -> Option<String> { | |
| if cmds.len() > 1 { | |
| let names: Vec<String> = cmds.iter().skip(1).map(|c| c.name.clone()).collect(); | |
| Some(names.join(" ")) | |
| } else { | |
| None | |
| } | |
| } | |
| pub fn subcommand_name_from_parse(cmds: &[usage::SpecCommand]) -> Option<String> { | |
| // cmds[0] is the root command; cmds[1] is the selected subcommand (flat only). | |
| cmds.get(1).map(|c| c.name.clone()) | |
| } |
This also avoids returning a space-containing string if the usage library ever gains nested-subcommand support, which would silently break case "$usage_cmd" in patterns in shell tasks.
| ''' | ||
| run = ''' | ||
| echo "color={{ usage.color }}" | ||
| ''' |
There was a problem hiding this comment.
-- Alice double-dash separator is unnecessary here
The -- end-of-options separator is only needed when a positional value could be misinterpreted as a flag (e.g. a value starting with -). Alice is a plain string that would match [name] directly without the separator.
Using -- Alice here may mislead task authors into thinking the separator is always required for optional args, when the corresponding unit test in test_usage_arg_defaults_for_unprovided already passes &["x"] without one.
| ''' | |
| assert "mise run usage-tera-opt-arg Alice" "hello Alice" |
### 🚀 Features - **(github)** read tokens from gh CLI hosts.yml config by @jdx in [#8692](#8692) - **(task)** support optional `args` and `env` fields in `run` entries by @jdx in [#8687](#8687) - **(task)** add --skip-tools flag to mise run by @jdx in [#8699](#8699) - **(vfox)** add try_get, try_head, try_download_file to Lua HTTP module by @jdx in [#8697](#8697) ### 🐛 Bug Fixes - **(config)** recognize SSH and other non-HTTPS URLs in get_repo_url by @modestman in [#8666](#8666) - **(docs)** add dark mode support to favicon by @jdx in [#8678](#8678) - **(env)** support multiple --env/-E flags by @jdx in [#8686](#8686) - **(github)** rename_exe renames correct binary when archive contains multiple executables by @jdx in [#8700](#8700) - **(implode)** include system data dir in implode cleanup by @jdx in [#8696](#8696) - **(install)** skip GitHub API calls for aqua tools in --locked mode by @jdx in [#8679](#8679) - **(install)** skip redundant provenance verification when lockfile has integrity data by @jdx in [#8688](#8688) - **(lock)** respect existing platforms in lockfile when running `mise lock` by @jdx in [#8708](#8708) - **(lock)** skip global config lockfile by default by @jdx in [#8707](#8707) - **(node)** expand tilde in default_packages_file path by @jdx in [#8709](#8709) - **(shell)** error when no version specified instead of silent no-op by @jdx in [#8693](#8693) - **(shim)** detect shims by checking shims directory instead of binary name by @jdx in [#8694](#8694) - **(task)** inherit task_config.dir for included TOML and file tasks by @jdx in [#8689](#8689) - **(task)** strip inline args when validating run.tasks references by @jdx in [#8701](#8701) - **(task)** include idiomatic version files in monorepo task toolset by @jdx in [#8702](#8702) - **(task)** improve error message when task files are not executable by @jdx in [#8705](#8705) - **(test)** update vfox provenance test for checksum-backed skip by @jdx in [#8703](#8703) - improve usage spec element support in tasks by @nkakouros in [#8623](#8623) - make env plugin (Module) vars available in Tera template context by @victor-founder in [#8682](#8682) - respect MISE_COLOR=0 for color_eyre error output by @jdx in [#8690](#8690) - add windows support for usage tool registry by @jdx in [#8713](#8713) ### 📚 Documentation - **(task)** clarify interactive task blocking behavior by @jdx in [#8685](#8685) - improve visibility of install_before setting by @jdx in [#8712](#8712) ### 📦 Registry - add rtk ([github:rtk-ai/rtk](https://github.com/rtk-ai/rtk)) by @bricelalu in [#8683](#8683) ### New Contributors - @victor-founder made their first contribution in [#8682](#8682) - @modestman made their first contribution in [#8666](#8666) - @bricelalu made their first contribution in [#8683](#8683)
The PR greatly improves support for
usagedirectives in mise tasks. Most of the changes are new tests.Summary
{{ usage.* }}templates erroring when optional args or flags are not provided — they nowdefault to empty string, empty array, or
falseas appropriate-vvv) rendering as[true, true, true]instead of3in templatescmdsubcommand support:{{ usage.cmd }}exposes the selected subcommand name,$usage_cmdis set as an env var, and subcommand args/flags are accessible in templatesmise run -p <value>being silently eaten by the hidden--prefixflag instead ofbeing passed to the task
Test plan
test_task_usage_map_tera)test_task_usage_cmd)cargo fmt,cargo check,shellcheck,shfmtall passtest_task_usage_hashmapupdated to match fixed behavior)I split the work into 3 commits in case some of the edits are controversial.
🤖 Generated with Claude Code