Repository navigation
feat(task): support arguments in sources and outputs - #11823
Closed
Marukome0743 wants to merge 1 commit into
Closed
Marukome0743 wants to merge 1 commit into
Marukome0743 wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughTask arguments and flags now support usage-dependent ChangesUsage-aware task templates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RunWatch as run/watch
participant TaskContextBuilder
participant Task
participant FreshnessCache as freshness/cache
RunWatch->>TaskContextBuilder: resolve task tools and environment
TaskContextBuilder->>Task: render task file templates with arguments
Task-->>RunWatch: return rendered sources and outputs
RunWatch->>FreshnessCache: perform freshness and cache checks
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR adds invocation-argument support to task source and output templates.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (4): Last reviewed commit: "feat(task): support arguments in sources..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/task/task_script_parser.rs (2)
85-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMatch the conflict error by a shared constant, not by an inline literal.
collect_spec_templatedecides whether to propagate an error by searching for the literal"conflicting definitions for task". The three producers of that message are insetup_tera_for_spec_parsingand use their own literals. If one message is reworded, conflict detection stops propagating and the parser silently accepts conflicting definitions.Extract the prefix into one constant and use it in all four places.
♻️ Proposed refactor
+const CONFLICTING_DEFINITION_PREFIX: &str = "conflicting definitions for task"; + fn collect_spec_template( tera: &mut TeraEngine, template: &str, ctx: &tera::Context, ) -> Result<()> { if let Err(err) = Self::render_script_with_context(tera, template, ctx) - && format!("{err:#}").contains("conflicting definitions for task") + && format!("{err:#}").contains(CONFLICTING_DEFINITION_PREFIX) { return Err(err); } Ok(()) }Then build each producer message from the same constant, for example:
return Err(format!( "{CONFLICTING_DEFINITION_PREFIX} flag '{name}'" ));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/task/task_script_parser.rs` around lines 85 - 96, Extract the shared “conflicting definitions for task” prefix into a module-level constant, then replace the inline literal in collect_spec_template and all three conflict-error producers in setup_tera_for_spec_parsing with that constant when matching or constructing messages. Preserve the existing error propagation and message-specific suffixes.
372-383: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAcquire
arg_orderbeforeinput_args, or drop theinput_argsguard first.Line 382 locks
arg_orderwhile theinput_argsguard is held. The collection code at lines 693-705 locksarg_orderfirst and theninput_args. The two orders are inverted. Rendering and collection currently run sequentially on one thread, so no deadlock occurs today. The inversion still becomes a deadlock as soon as any caller renders templates concurrently with collection.Record the order after the duplicate check without nesting the guards.
♻️ Proposed change
- let mut input_args = input_args.lock().map_err(Self::lock_error)?; - if let Some(existing) = input_args.iter().find(|existing| existing.name == name) { - if format!("{existing:?}") != format!("{arg:?}") { - return Err(format!( - "conflicting definitions for task argument '{name}'" - )); - } - trace!("already seen {name}"); - return Ok(json!("")); - } - arg_order.lock().map_err(Self::lock_error)?.insert(name, i); - input_args.push(arg); + { + let mut input_args = input_args.lock().map_err(Self::lock_error)?; + if let Some(existing) = input_args.iter().find(|e| e.name == name) { + if format!("{existing:?}") != format!("{arg:?}") { + return Err(format!( + "conflicting definitions for task argument '{name}'" + )); + } + trace!("already seen {name}"); + return Ok(json!("")); + } + input_args.push(arg); + } + arg_order.lock().map_err(Self::lock_error)?.insert(name, i);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/task/task_script_parser.rs` around lines 372 - 383, Update the argument-registration logic around the duplicate check to avoid locking arg_order while the input_args guard is held. Preserve the existing duplicate validation and early returns, then release the input_args guard before acquiring arg_order and recording the argument order, maintaining the same lock order used by the collection code.src/task/task_context_builder.rs (1)
55-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftShare the context setup with
TaskExecutor::prepare_task_context.Lines 67-83 duplicate the tool collection,
task_cfselection, toolset build, and env resolution performed byTaskExecutor::prepare_task_contextinsrc/task/task_executor.rs(lines 1932-1962). The two copies must stay identical, because a source template must render against the same environment the task later executes with.Two consequences follow from the duplication:
prepare_task_contextadditionally injectsMISE_TASK_NAME,MISE_TASK_DIR,MISE_TASK_FILE,MISE_PROJECT_ROOT, and related keys. A source or output template that combines{{usage.*}}with one of thoseenv.*keys renders here without them.- For a task that declares its own
envortools, the builder caches nothing, so the toolset and env resolution run twice per task.Extract the common prefix into one helper that both call sites use.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/task/task_context_builder.rs` around lines 55 - 86, Extract the duplicated tool collection, task configuration selection, toolset construction, and environment resolution from render_task_file_templates and TaskExecutor::prepare_task_context into a shared helper. Ensure the helper also performs the context-specific injections currently added by prepare_task_context, and have both callers reuse its returned context so template rendering matches task execution and avoids duplicate resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/tasks/task-configuration.md`:
- Around line 409-431: Update the task configuration documentation near the
parsed-argument sources and outputs examples to state that argument templates
require usage-based argument parsing. Document that they are unavailable when
raw_args is enabled or when arguments are passed after --, and mention the
resulting error or required alternative.
In `@e2e/tasks/test_task_watch_usage_sources`:
- Around line 13-20: Increase the retry window in wait_for_watchexec beyond the
current 50 iterations while preserving the 0.1-second polling interval and
existing timeout failure message.
In `@src/task/task_script_parser.rs`:
- Around line 931-939: Update the usage parser error handling in
parallelize_tasks to match parse_run_scripts_with_args: display the parser
result through the existing usage error path and exit with status 1, rather than
wrapping it with eyre::eyre! and propagating it. Preserve successful parsing and
source/output rendering behavior.
---
Nitpick comments:
In `@src/task/task_context_builder.rs`:
- Around line 55-86: Extract the duplicated tool collection, task configuration
selection, toolset construction, and environment resolution from
render_task_file_templates and TaskExecutor::prepare_task_context into a shared
helper. Ensure the helper also performs the context-specific injections
currently added by prepare_task_context, and have both callers reuse its
returned context so template rendering matches task execution and avoids
duplicate resolution.
In `@src/task/task_script_parser.rs`:
- Around line 85-96: Extract the shared “conflicting definitions for task”
prefix into a module-level constant, then replace the inline literal in
collect_spec_template and all three conflict-error producers in
setup_tera_for_spec_parsing with that constant when matching or constructing
messages. Preserve the existing error propagation and message-specific suffixes.
- Around line 372-383: Update the argument-registration logic around the
duplicate check to avoid locking arg_order while the input_args guard is held.
Preserve the existing duplicate validation and early returns, then release the
input_args guard before acquiring arg_order and recording the argument order,
maintaining the same lock order used by the collection code.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: fc83d656-d3ca-4d21-b837-28957ab4ba11
📒 Files selected for processing (12)
docs/tasks/task-arguments.mddocs/tasks/task-configuration.mde2e/tasks/test_task_usage_sources_outputse2e/tasks/test_task_usage_sources_outputs_depse2e/tasks/test_task_watch_usage_sourcessrc/cli/run.rssrc/cli/watch.rssrc/task/mod.rssrc/task/task_context_builder.rssrc/task/task_executor.rssrc/task/task_script_parser.rssrc/task/task_sources.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/tasks/test_task_watch_usage_sources`:
- Line 28: Update the assertions for the watch command in the task-watch
usage-source test to verify the rendered source filter rather than merely
matching selected.txt in the full command. Check for “-f selected.txt” or
restrict the inspected watchexec line to the portion before “ -- ”, and apply
the same correction to both affected assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 7189b3ba-9960-4000-b96a-05443bdda03f
📒 Files selected for processing (4)
docs/tasks/task-configuration.mde2e/tasks/test_task_usage_sources_outputse2e/tasks/test_task_watch_usage_sourcessrc/task/task_script_parser.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/tasks/task-configuration.md
- e2e/tasks/test_task_usage_sources_outputs
- src/task/task_script_parser.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
e2e/tasks/test_task_usage_sources_outputs (1)
69-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the rendered output paths, not only the rendered source paths.
Lines 69-75 verify the
sourcesrendering throughtask_source_files()and through freshness skipping. No assertion checks thatoutputsrendered to a different path per invocation. A regression that renders every output todebug/app.outwould still pass this block.💚 Proposed additional assertions
assert "mise run -q build app" "debug/app.in" assert_empty "mise run -q build app" assert "mise run -q build app --release" "release/app.in" +[[ -f debug/app.out ]] || fail "debug output was not created" +[[ -f release/app.out ]] || fail "release output was not created"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tasks/test_task_usage_sources_outputs` around lines 69 - 75, Add assertions in the task usage test around the existing debug and release build invocations to verify the rendered output paths, including that each invocation uses its expected distinct output path rather than always rendering to debug/app.out. Keep the current source-path and freshness assertions unchanged, and use the existing output-path rendering helpers or assertion style.src/task/task_executor.rs (1)
1948-1964: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the injected metadata key list with
inject_task_metadata.This literal list must stay identical to the keys written by
TaskContextBuilder::inject_task_metadatainsrc/task/task_context_builder.rs(Lines 118-143). If a key is added in one place only, that key leaks into__MISE_DIFFand changes the environment observed by nestedmiseinvocations. Export a single constant and use it in both files.♻️ Proposed refactor
In
src/task/task_context_builder.rs:pub(crate) const TASK_METADATA_ENV_KEYS: &[&str] = &[ "MISE_PROJECT_ROOT", "MISE_MONOREPO_ROOT", "MISE_TASK_NAME", "MISE_TASK_FILE", "MISE_TASK_DIR", "MISE_CONFIG_ROOT", ];In
src/task/task_executor.rs:- .chain( - [ - "MISE_PROJECT_ROOT", - "MISE_MONOREPO_ROOT", - "MISE_TASK_NAME", - "MISE_TASK_FILE", - "MISE_TASK_DIR", - "MISE_CONFIG_ROOT", - ] - .into_iter() - .map(str::to_string), - ) + .chain( + crate::task::task_context_builder::TASK_METADATA_ENV_KEYS + .iter() + .copied() + .map(str::to_string), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/task/task_executor.rs` around lines 1948 - 1964, Extract the shared metadata key list into a crate-visible constant such as TASK_METADATA_ENV_KEYS alongside TaskContextBuilder::inject_task_metadata, then replace the duplicated literal list in the nested_mise_diff_exclude_keys construction with that constant. Ensure both locations use the same keys so metadata injected by inject_task_metadata is excluded from __MISE_DIFF.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/task/task_context_builder.rs`:
- Around line 62-77: Update render_task_file_templates to avoid resolving the
full task context before dependency processing. Use only the task/dependency
environment required for argument interpolation when calling
render_file_templates_with_args, and leave full context resolution to the
scheduler’s execution-start path.
In `@src/task/task_executor.rs`:
- Around line 1789-1790: Ensure watch-task scheduling performs argument
validation before building the dependency graph. In the flow around
render_task_file_templates, invoke preflight_task_usage after template rendering
(or an equivalent guard) so dynamic-file-template tasks with static usage reject
missing required arguments before dependencies start.
---
Nitpick comments:
In `@e2e/tasks/test_task_usage_sources_outputs`:
- Around line 69-75: Add assertions in the task usage test around the existing
debug and release build invocations to verify the rendered output paths,
including that each invocation uses its expected distinct output path rather
than always rendering to debug/app.out. Keep the current source-path and
freshness assertions unchanged, and use the existing output-path rendering
helpers or assertion style.
In `@src/task/task_executor.rs`:
- Around line 1948-1964: Extract the shared metadata key list into a
crate-visible constant such as TASK_METADATA_ENV_KEYS alongside
TaskContextBuilder::inject_task_metadata, then replace the duplicated literal
list in the nested_mise_diff_exclude_keys construction with that constant.
Ensure both locations use the same keys so metadata injected by
inject_task_metadata is excluded from __MISE_DIFF.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a64a0479-3672-4c7a-bb32-fe12afa4e5b7
📒 Files selected for processing (5)
e2e/tasks/test_task_usage_sources_outputse2e/tasks/test_task_watch_usage_sourcessrc/task/task_context_builder.rssrc/task/task_executor.rssrc/task/task_script_parser.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/tasks/test_task_watch_usage_sources
- src/task/task_script_parser.rs
Owner
|
not ready to support this, sorry |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
{{usage.*}}values and the deprecatedarg(),option(), andflag()helpers in file patternstask_source_files(), andmise watchDetails
Previously, sources and outputs were rendered while loading configuration, before task arguments and dependency environments were available. This left
{{usage.*}}without invocation values and made the legacy helpers unknown. The new flow preserves raw patterns, validates every scheduled occurrence before the scheduler starts, then renders filesystem patterns with the complete task-local toolset, environment, and vars after that occurrence's dependencies finish and before its freshness/cache checks.This ordering keeps preflight side-effect free: it does not run
_.sourceor plugin environment hooks before a dependency can produce their inputs.mise watch, which must determine filters before starting watchexec, renders from the already-loaded config context plus literal task/dependency overrides and performs the same argument preflight first. Filesystem values remain unquoted, while existing shell escaping for run scripts is unchanged. Normal dependencies, post dependencies, injected tasks, and watch targets receive their own arguments and dependency environment. Task file rendering and execution share the standardMISE_TASK_*metadata keys. Tasks that bypass usage parsing throughraw_argsor double-dash help passthrough fail before scheduling when they contain argument-dependent patterns. This restriction is documented, and argument parse failures use the same unwrapped usage error output as run scripts.Testing
Passed:
mise exec -- cargo fmt --all -- --checkmise exec -- shfmt -dfor the three new E2E filesmise exec -- shellcheckfor the three new E2E filesmise exec -- cargo check --all-featuresmise exec -- cargo clippy --workspace --all-features --all-targets -- -D warningsmise exec -- cargo test --all-features task_script_parsermise exec -- cargo test --all-features test_template_has_usage_inputmise run test:e2e e2e/tasks/test_task_usage_sources_outputsmise run test:e2e e2e/tasks/test_task_usage_sources_outputs_depsmise run test:e2e e2e/tasks/test_task_watch_usage_sourcesmise run test:e2e e2e/tasks/test_task_run_sourcesmise run test:e2e e2e/tasks/test_task_usage_preflightmise run test:e2e e2e/tasks/test_task_watch_skip_depsThe CodeRabbit follow-ups were additionally verified with the affected E2Es, the task script parser unit suite, Rustfmt, shfmt, ShellCheck, Cargo check, and Clippy. Regression coverage now verifies watch preflight, distinct Debug/Release output paths, and a task whose dependency generates the env file used by its argument-dependent source/output patterns. The watch assertion checks the rendered
-ffilter rather than the task argument portion of the watchexec command.mise run formatandmise run lintcould not run their hk wrapper in this Sapling working copy because hk reportedfailed to find git repository; the underlying Rustfmt, shfmt, ShellCheck, Cargo check, and Clippy checks above passed individually.Addresses #5229
AI-assisted — Tool: Codex; model: OpenAI/GPT-5; version: unavailable.
Summary by CodeRabbit
New Features
sourcesandoutputscan now use parsed arguments and flags throughusage.*templates.--usage-template scenarios.arg(),option(), andflag()templates remain supported with compatibility guidance.Bug Fixes
Documentation