fix(task): honor task_config.includes order so a local task can override a git::include#10191
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR changes task include precedence so when multiple includes define the same task name, the last matching include in ChangesTask Include Precedence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes the task-include precedence bug introduced in #9147: previously,
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to the include-loading loop in load_tasks_in_dir and validated by three concrete e2e scenarios. The core Rust change is a clean refactor with no new state, no unsafe code, and no changes to the public API. The merge_file_and_config_tasks logic is untouched. The only callers of task_includes_for_dir not touched by this PR use it solely for diagnostics, so their continued exclusion of git:: URLs is harmless. No files require special attention; the test file's potential slow-run time is the only item worth a second glance before merge. Important Files Changed
Reviews (2): Last reviewed commit: "fix(task): honor task_config.includes or..." | Re-trigger Greptile |
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_local_overrides_git_include`:
- Around line 14-53: The script starts the git HTTP server and assigns
SERVER_PID before registering the cleanup trap and assumes $TMPDIR exists; move
the trap registration so it runs immediately after SERVER_PID is set (i.e.,
place trap cleanup EXIT right after SERVER_PID=$!), and change
PORT_FILE/READY_FILE/INFO_FILE initialization to use a TMPDIR fallback (e.g.,
${TMPDIR:-/tmp}) when constructing MISE_GIT_HTTP_* file paths so the files and
cleanup work even if TMPDIR is unset; keep the existing cleanup() and
wait_for_server() logic unchanged but ensure trap is registered before calling
wait_for_server.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7c7b626-7412-4cba-93a9-cd5dd1b26c76
📒 Files selected for processing (3)
docs/tasks/task-configuration.mde2e/tasks/test_task_local_overrides_git_includesrc/config/mod.rs
|
I feel last win is more intuitive as other config like tools or env are last win. I'm just curious but are there any reasons for the first win? |
There is no real reason for first to win. I just wanted to fix the issue that was introduced were remote tasks would always take precedence over local ones. I will update the PR to have last define wins :) |
|
I see, thanks! |
…des an earlier one PR jdx#9147 introduced an early dedup in merge_file_and_config_tasks and, combined with load_tasks_in_dir loading all non-git includes before all git:: includes, ignored the declared task_config.includes order: a git:: include could no longer be overridden by a later local include (and vice versa), regardless of list position. Load includes in a single ordered pass (mirroring load_file_tasks) and keep the file-task dedup last-wins, so the later include in the list takes precedence, uniformly for directory, toml-file, and git:: includes. To override a remote git:: task with a local one, list the local directory after the git:: entry. The TOML [tasks.*] overlay behavior from jdx#9147 is unchanged. Document the include precedence and add an e2e test covering local-last, git-last, and two same-named local includes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bcbebfe to
472866a
Compare
PR #9147 introduced an early last-wins dedup in merge_file_and_config_tasks and, combined with load_tasks_in_dir loading all non-git includes before all git:: includes, made a git:: include always clobber a same-named local task regardless of its position in task_config.includes.
Load includes in their declared order via a single ordered pass (mirroring load_file_tasks) and keep the file-task dedup first-wins, so the earlier include in the list takes precedence uniformly for directory, toml-file, and git:: includes. The TOML [tasks.*] overlay behavior from #9147 is unchanged.
Document the include precedence and add an e2e test covering local-first, git-first, and two same-named local includes.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests