Repository navigation
feat(tasks): warn when a task_config.includes path does not exist - #11883
JamBalaya56562 wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTask include handling now detects missing literal paths during loading and validation. It excludes globs, Git URLs, and built-in defaults. Documentation and end-to-end tests cover the new diagnostics and supported include forms. ChangesTask include validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TasksValidate
participant Config
participant TaskIncludes
participant Diagnostics
TasksValidate->>Config: collect task configuration
Config->>TaskIncludes: inspect explicit includes
TaskIncludes-->>Config: return missing literal paths
Config-->>TasksValidate: provide missing include paths
TasksValidate->>Diagnostics: emit missing-include warnings
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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 `@src/config/mod.rs`:
- Around line 4641-4650: Update the warn_once! call in the
missing_literal_include handling to deduplicate by the resolved missing path
rather than the raw include string. Use missing/display_path(&missing) as the
warning key while preserving the existing warning text and behavior.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 2cb65e97-93b2-46e5-b3c7-7f71d9998af7
📒 Files selected for processing (5)
docs/cli/tasks/validate.mde2e/tasks/test_task_broken_includemise.usage.kdlsrc/cli/tasks/validate.rssrc/config/mod.rs
339bcd7 to
8cf837c
Compare
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
b4974d7 to
adff412
Compare
Greptile SummaryThis PR adds diagnostics for explicit, nonexistent
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the eligible follow-up review scope. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "feat(tasks): warn when a task_config.inc..." | Re-trigger Greptile |
adff412 to
8a004df
Compare
9b7019f to
c970f3a
Compare
A literal include that resolves to nothing was dropped in silence, so a typo or a moved file made tasks disappear with nothing on stderr and nothing at --verbose. `mise tasks validate` reported success too. Warn once at load time and report a `missing-include` issue from `mise tasks validate`, both through one `missing_literal_include` helper so the two paths cannot drift. The built-in default includes are excluded: a project has at most one of the five default task directories, so warning on them would fire on nearly every invocation. Globs are excluded too, since matching nothing is the normal state of a directory whose task files are not written yet.
c970f3a to
e111c88
Compare
|
Thanks for working on this, and especially for calling out the ambiguity in the PR description. I don't think a missing This is consistent with similar config and environment-file search paths, which quietly skip absent candidates. Since mise currently has no way to distinguish an optional include from a typo, validation would have the same false positives. If we want required includes in the future, I think that needs explicit syntax rather than being inferred from whether a literal path exists. Because removing the load-time and validation warnings leaves no substantive change here, I'm going to close this. Thanks again for the careful investigation and tests. AI-assisted — Tool: Codex; model: unavailable/unavailable; version: unavailable. |
From #4792. Not
Closes— that thread asks for three things and this is one of them (see "Not in this PR").Problem
A
task_config.includesentry that points at nothing is dropped in silence. Measured on v2026.8.2:Nothing on stderr, and nothing at
--verboseeither — grepping the verbose output for the missing filename returns zero lines.mise tasks validateis just as quiet:So a typo, a renamed file, or a moved directory makes tasks disappear with no way to find out why. The failure mode is silent absence: the tasks that did load look completely normal.
The site is
expand_task_include, whose literal-path branch is:The glob branch immediately above it already
warn!s when expansion fails, so the silence is specific to literal paths.What this changes
warn_once!at load time, and amissing-includeissue inmise tasks validate:Both go through one new pure function,
missing_literal_include, so the two paths cannot drift.The built-in defaults must not warn — this is the whole risk of the change
When
includesis unset,default_task_includes()supplies five directories —mise-tasks,.mise-tasks,.mise/tasks,.config/mise/tasks,mise/tasks— and they go through the same expansion. A project has at most one of them, so warning unconditionally would print four or five lines on essentially every mise invocation.task_include_patterns_for_diralready returns a "used the defaults" flag;load_tasks_from_configsdid not, so this adds one there (uses_default_includes) and gates the warning on it. There is an e2e assertion for exactly this, because without it the regression is invisible in review: a project with a realmise-tasks/and no[task_config]must produce no warning.Globs are deliberately excluded
includes = ["tasks/*.toml"]matching nothing is the normal state of a directory whose task files have not been written yet, so a glob is never reported. Only literal paths are.git::includes are untouchedThey branch to
resolve_git_url_to_pathbefore expansion (config/mod.rs), so an include that has not been fetched yet cannot be mistaken for a missing local path.Noise
mise hook-envandmise envdo not load tasks, so this cannot reach the prompt. Within a run, the same config is loaded once per directory and each resolves the include to the same absolute path, so keyingwarn_once!on the rendered message collapses a monorepo's worth of repeats to one line.Severity is
Warning, and that is a judgement callmise tasks validateexits non-zero only onError, so this does not fail CI. That matches the existing granularity —missing-directoryandnot-executableare warnings,missing-fileis an error — and an include may legitimately be optional in a shared config.If you would rather this fail CI, it is a one-line change (
Severity::Warning→Severity::Errorinvalidate_task_includes). I have no strong view; the reporter's use case is automation, which argues forError, but silently breaking someone'smise tasks validatein CI argues against.Verification
missing_literal_includecover: existing file, existing directory, absent path, glob (never reported), and an absolute include not being joined onto the include root. They live inconfig's existing#[cfg(unix)]test module, so they run in CI, not on my box (Windows).e2e/tasks/test_task_broken_includecovers the warning, thevalidateissue in both human and--jsonoutput, a directory include being satisfied, the defaults not warning, and a zero-match glob not warning.cargo fmt --all,cargo clippy --all-targetsclean on the touched files.One existing test needed a change, and it is worth a look
e2e/tasks/test_task_edit_nested_namescaptured a path with2>&1:The new warning landed in the capture and broke the equality. I changed it to take stdout only, which is what a path capture wants anyway — but the reason it fired is worth your judgement rather than mine.
That block tests a real pattern: "select the first existing directory in include order", which
task_creation_dir_for_dirsupports deliberately. Under that reading a missing entry is a candidate that lost, not a mistake, and warning about it is noise. Under the reading this PR takes — a missing include means tasks are silently absent — it is exactly what the user should hear.I went with the second because
task_config.includesis documented as "files and directories mise should search", not as a candidate list, and the candidate behaviour is specific to wheremise tasks addputs a new task. But if you would rather the loader stay quiet when some other include in the same list did resolve, that is a small change and I am happy to make it.The other three blocks in that file with missing includes are unaffected: they assert with
assert_fail_contains, which tolerates extra stderr.Not in this PR
mise tasks include <path>, the command Add subcommand for mise tasks to include in the global config.toml an external tasks.toml #4792 actually asks for. Worth knowing before anyone builds it: settingincludesreplaces the five default directories rather than adding to them, so a naive append command silently drops the user's existing tasks. Measured — a project whose only task lives inmise-tasks/loses it the moment oneincludesentry is written. Any such command has to seed the defaults on first write, or refuse. That is a design decision, not a mechanical addition.git::includes whose fetch fails. Different code path, different problem.Summary by CodeRabbit
New Features
Documentation
Bug Fixes